Bug 12811 - Patron 'Details' and 'Check out' pages not working properly for staff users which don't have renewal override permissions
Summary: Patron 'Details' and 'Check out' pages not working properly for staff users w...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jacek Ablewicz
QA Contact: Testopia
URL:
Keywords:
: 12878 (view as bug list)
Depends on: 11703
Blocks: 12920
  Show dependency treegraph
 
Reported: 2014-08-23 14:28 UTC by Jacek Ablewicz
Modified: 2015-06-04 23:30 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 12811 - Patron 'Details' and 'Check out' pages not working properly for staff users which don't have renewal override permissions (3.13 KB, patch)
2014-08-23 14:46 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12811 - Patron 'Details' and 'Check out' pages not working for staff users without renewal override permissions (3.21 KB, patch)
2014-08-27 17:00 UTC, Owen Leonard
Details | Diff | Splinter Review
[PASSED QA] Bug 12811 - Patron 'Details' and 'Check out' pages not working for staff users without renewal override permissions (3.28 KB, patch)
2014-09-07 21:08 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jacek Ablewicz 2014-08-23 14:28:10 UTC
In case when the staff user doesn't have (circulate) -> (override_renewals) permission granted, this code part

   var AllowRenewalLimitOverride = [% CAN_user_circulate_override_renewals && AllowRenewalLimitOverride %];

in circ/circulation.tt and members/moremember.tt leads to javascript error, because TT statement evaluates to empty string.

To reproduce:

- set AllowRenewalLimitOverride syspref to "Don't allow" (this step is possibly redundant / not quite relevant)
- for testing purposes, log in as some (sample) staff user whitch does not have permission granted for "(override_renewals) Override blocked renewals"
- have a look at some patron accounts (preferably, ones with 1+ check-out), observe that page layout is incorrect in "Check Out" and "Details" tab.
Comment 1 Jacek Ablewicz 2014-08-23 14:46:04 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2014-08-27 17:00:56 UTC Comment hidden (obsolete)
Comment 3 Jacek Ablewicz 2014-09-06 08:55:51 UTC
*** Bug 12878 has been marked as a duplicate of this bug. ***
Comment 4 Katrin Fischer 2014-09-07 21:08:23 UTC
Created attachment 31439 [details] [review]
[PASSED QA] Bug 12811 - Patron 'Details' and 'Check out' pages not working for staff users without renewal override permissions

In case when the staff user doesn't have (circulate) ->
(override_renewals) permission granted, this code part

   var AllowRenewalLimitOverride = [%
CAN_user_circulate_override_renewals && AllowRenewalLimitOverride %];

in circ/circulation.tt and members/moremember.tt leads to javascript
error, because TT statement evaluates to empty string.

To reproduce:

- set AllowRenewalLimitOverride syspref to "Don't allow" (this step is
  possibly redundant / not quite relevant),
- for testing purposes, log in as some (sample) staff user whitch does
  not have permission granted for "(override_renewals) Override blocked
  renewals",
- have a look at some patron accounts (preferably, ones with 1+
  check-out), observe that page layout is incorrect in "Check Out" and
  "Details" tab.

To test:

- apply patch,
- retest and ensure that this issue is no longer reproductible,
- make sure that there are no regressions of any kinds regarding renewal
  override permission for staff users (i.e, that it still works like
  intended).

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Works as described, passes QA script and tests.
Comment 5 Kyle M Hall 2014-09-08 17:12:55 UTC
Just an fyi, but I believe AllowRenewalLimitOverride should actually be Koha.Preference('AllowRenewalLimitOverride'). My patch ( in the duplicate bug ) resolves this issue. So we should either push that patch instead, or file a further bug so we can fix that in a followup report.
Comment 6 Jacek Ablewicz 2014-09-09 07:05:39 UTC
(In reply to Kyle M Hall from comment #5)
> Just an fyi, but I believe AllowRenewalLimitOverride should actually be
> Koha.Preference('AllowRenewalLimitOverride'). My patch ( in the duplicate
> bug ) resolves this issue. 

I agree it would be better to use Koha.Preference() in both affected templates, but patch in bug 12878 appears to be incomplete:

- circ/circulation.tt: renewal override is not working properly with this patch applied (due to missing [% USE Koha %] ?),
- Koha.Preference() call is not being used in patched members/moremember.tt - any particular reason to treat this template differently then circ/circulation.tt?
Comment 7 Tomás Cohen Arazi 2014-09-16 19:12:31 UTC
Patch pushed to master.

Thanks Jacek!

Note: I'd prefer that each new patch uses Koha.Preference instead. Yours doesn't introduce the syspref-from-pl situation, so I'm pushing it because is a sensitive issue for the stable branches.

Thanks!
Comment 8 Galen Charlton 2014-09-23 20:35:34 UTC
Pushed to 3.16.x for inclusion in 3.16.4.