Bug 18453

Summary: "Export" column is not hidden when ExportCircHistory is off
Product: Koha Reporter: Marc Véron <veron>
Component: CirculationAssignee: Jonathan Druart <jonathan.druart+koha>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: gmcharlt, jonathan.druart+koha, katrin.fischer, kyle.m.hall, kyle
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18257
Change sponsored?: --- Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 15498    
Bug Blocks:    
Attachments: Bug 18453: Syspref ExportCircHistory is broken
[SIGNED-OFF] Bug 18453: Syspref ExportCircHistory is broken
[ALTERNATIVE-PATCH] Bug 18453: Hide the export column when needed
Bug 18453: Remove exports_enabled in circulation_batch_checkouts.tt
Table broken on moremember.pl
Bug 18453: Hide the export column when needed
Bug 18453: Remove exports_enabled in circulation_batch_checkouts.tt
Bug 18453: Hide the export column when needed
Bug 18453: Remove exports_enabled in circulation_batch_checkouts.tt

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.