Bug 18453 - "Export" column is not hidden when ExportCircHistory is off
Summary: "Export" column is not hidden when ExportCircHistory is off
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 15498
Blocks:
  Show dependency treegraph
 
Reported: 2017-04-19 06:59 UTC by Marc Véron
Modified: 2017-12-07 22:16 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18453: Syspref ExportCircHistory is broken (3.57 KB, patch)
2017-04-19 07:10 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18453: Syspref ExportCircHistory is broken (3.63 KB, patch)
2017-04-19 12:33 UTC, Owen Leonard
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 18453: Hide the export column when needed (1.11 KB, patch)
2017-04-19 15:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18453: Remove exports_enabled in circulation_batch_checkouts.tt (1.08 KB, patch)
2017-04-19 15:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Table broken on moremember.pl (28.66 KB, image/png)
2017-04-19 17:11 UTC, Marc Véron
Details
Bug 18453: Hide the export column when needed (1.24 KB, patch)
2017-04-19 17:27 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18453: Remove exports_enabled in circulation_batch_checkouts.tt (1.21 KB, patch)
2017-04-19 17:28 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18453: Hide the export column when needed (1.30 KB, patch)
2017-04-25 17:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18453: Remove exports_enabled in circulation_batch_checkouts.tt (1.26 KB, patch)
2017-04-25 17:46 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2017-04-19 06:59:44 UTC
The checkouts table on a patron's checkout and detail screen always show a column 'Export' and the 'Export checkouts using format' fields/buttons. They should be hidden if syspref 'ExportCircHistory' is set to 'Don't show'.
Comment 1 Marc Véron 2017-04-19 07:10:16 UTC
Created attachment 62353 [details] [review]
Bug 18453: Syspref ExportCircHistory is broken

The checkouts table on a patron's checkout and detail screen always show
a column 'Export' and the 'Export checkouts using format' fields/buttons.
They should be hidden if syspref 'ExportCircHistory' is set to 'Don't show'.

This patch brings some variables in circulation.tt and in moremember.tt in
sync. They are used by the checkouts table (checkouts-table.inc).

To test:
- Apply patch
- Verify that visibility of export fields in checkouts table depends on
syspref 'ExportCircHistory' (for both moremember.pl and circulation.pl)
- Verify that other functionality behaves as expected on both screens.
Comment 2 Owen Leonard 2017-04-19 12:33:44 UTC
Created attachment 62362 [details] [review]
[SIGNED-OFF] Bug 18453: Syspref ExportCircHistory is broken

The checkouts table on a patron's checkout and detail screen always show
a column 'Export' and the 'Export checkouts using format' fields/buttons.
They should be hidden if syspref 'ExportCircHistory' is set to 'Don't show'.

This patch brings some variables in circulation.tt and in moremember.tt in
sync. They are used by the checkouts table (checkouts-table.inc).

To test:
- Apply patch
- Verify that visibility of export fields in checkouts table depends on
syspref 'ExportCircHistory' (for both moremember.pl and circulation.pl)
- Verify that other functionality behaves as expected on both screens.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 3 Jonathan Druart 2017-04-19 15:16:51 UTC
Comment on attachment 62362 [details] [review]
[SIGNED-OFF] Bug 18453: Syspref ExportCircHistory is broken

Review of attachment 62362 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
@@ +35,4 @@
>  var theme = "[% theme %]";
>  var borrowernumber = "[% borrowernumber %]";
>  var branchcode = "[% branch %]";
> +var exports_enabled = [% Koha.Preference('ExportCircHistory') %];

If the pref is empty, the generated JS will be
  var exports_enabled = ;
And will raise a syntax error.

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
@@ +27,4 @@
>  <script type="text/JavaScript">
>  //<![CDATA[
>  /* Set some variable needed in circulation.js */
> +var MSG_DT_LOADING_RECORDS = _("Loading... you may continue scanning.");

You cannot scan from moremember.

@@ +36,3 @@
>  var AllowRenewalLimitOverride = [% (CAN_user_circulate_override_renewals && Koha.Preference('AllowRenewalLimitOverride') )? 1: 0 %];
> +var AllowCirculate = [% (CAN_user_circulate_circulate_remaining_permissions)? 1 : 0 %];
> +var script = "circulation";

Did you find where this script variable is used?

@@ +43,4 @@
>  
>  var MSG_EXPORT_SELECT_CHECKOUTS = _("You must select checkout(s) to export");
>  
> +columns_settings = [% ColumnsSettings.GetColumns( 'circ', 'circulation', 'issues-table', 'json' ) %]

Hum? Why that? We are on moremember, we need to keep the config for moremember.
Comment 4 Jonathan Druart 2017-04-19 15:30:33 UTC
Created attachment 62393 [details] [review]
[ALTERNATIVE-PATCH] Bug 18453: Hide the export column when needed

If exports_enabled is equal to "0" it will be evaluated to true.
Comment 5 Jonathan Druart 2017-04-19 15:31:20 UTC
Marc, this single line patch seems to solve the issue. Please confirm.
Comment 6 Jonathan Druart 2017-04-19 15:32:57 UTC
Created attachment 62394 [details] [review]
Bug 18453: Remove exports_enabled in circulation_batch_checkouts.tt

This is a c/p from circulation.tt, this variable is not needed in this
template
Comment 7 Marc Véron 2017-04-19 17:11:06 UTC
Created attachment 62399 [details]
Table broken on moremember.pl

Hi Jonathan

First I tried a similar one-liner as well (remove the quotes around the value in following line (both circulation.tt and moremember.tt)
var exports_enabled = [% Koha.Preference('ExportCircHistory') %];
That makes the value a number and the following works:
"bVisible": exports_enabled ? true : false,...

But I still had a nasty behaviour on moremember.pl (the check-out column was not visible, if set to visible with button 'Column visibility' it shows up, but the table is broken, see screenshot.

It does not happen on the checkout page.

The same happens with your two patches applied. So there is more to be investigated.

Anyway, I obsolete my first patch.
Comment 8 Marc Véron 2017-04-19 17:25:19 UTC
OK, found. The table problem is an issue with a colum settings in Administration (issues-table > checkin set to hidden by default), for both circulation and patrons.

I will file a new bug for that.

Setting to Needs Signoff for alternative patches.
Comment 9 Marc Véron 2017-04-19 17:27:23 UTC
Created attachment 62401 [details] [review]
Bug 18453: Hide the export column when needed

If exports_enabled is equal to "0" it will be evaluated to true.

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 10 Marc Véron 2017-04-19 17:28:47 UTC
Created attachment 62402 [details] [review]
Bug 18453: Remove exports_enabled in circulation_batch_checkouts.tt

This is a c/p from circulation.tt, this variable is not needed in this
template

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 11 Marc Véron 2017-04-19 17:30:00 UTC
Comment on attachment 62399 [details]
Table broken on moremember.pl

Will move this issue to a new bug
Comment 12 Nick Clemens 2017-04-25 17:46:42 UTC
Created attachment 62694 [details] [review]
Bug 18453: Hide the export column when needed

If exports_enabled is equal to "0" it will be evaluated to true.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Nick Clemens 2017-04-25 17:46:46 UTC
Created attachment 62695 [details] [review]
Bug 18453: Remove exports_enabled in circulation_batch_checkouts.tt

This is a c/p from circulation.tt, this variable is not needed in this
template

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Kyle M Hall 2017-04-28 12:33:21 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 15 Katrin Fischer 2017-05-13 12:22:50 UTC
Dependency is not in 16.11.x.