Bug 10863 - Add 2 circulation permissions: force_checkout and lift_restriction
Summary: Add 2 circulation permissions: force_checkout and lift_restriction
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 2720
Blocks: 10859
  Show dependency treegraph
 
Reported: 2013-09-10 12:36 UTC by Jonathan Druart
Modified: 2014-12-07 20:07 UTC (History)
9 users (show)

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


Attachments
Bug 10863: Add 2 circulation permissions: force_checkout and lift_restriction (12.54 KB, patch)
2013-09-10 12:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10863: The lift restriction button should be hide on the moremember page (2.76 KB, patch)
2013-09-11 08:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10863: The lift restriction button should be hide on the moremember page (1.65 KB, patch)
2013-09-12 13:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10863: The force checkout permission should not affect high holds (1.68 KB, patch)
2013-09-12 13:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10863: Add 2 circulation permissions: force_checkout and lift_restriction (12.53 KB, patch)
2013-09-18 12:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10863: The lift restriction button should be hide on the moremember page (1.65 KB, patch)
2013-09-18 12:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10863: The force checkout permission should not affect high holds (1.68 KB, patch)
2013-09-18 12:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10863: Add 2 circulation permissions: force_checkout and lift_restriction (12.84 KB, patch)
2013-09-19 14:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10863: The lift restriction button should be hide on the moremember page (1.70 KB, patch)
2013-09-19 14:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10863: The force checkout permission should not affect high holds (1.88 KB, patch)
2013-09-19 14:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10863: Follow-up: Adding missing permissions to translated files (8.18 KB, patch)
2013-09-28 10:49 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10863: Rename lift_restriction with manage_restrictions (15.59 KB, patch)
2013-09-30 14:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10863: Add 2 circulation permissions: force_checkout and lift_restriction (21.70 KB, patch)
2013-11-05 11:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PATCH][SIGNED OFF] Bug 10863: Add 2 circulation permissions: force_checkout and lift_restriction (21.85 KB, patch)
2013-12-03 11:12 UTC, Mathieu Saby
Details | Diff | Splinter Review
Bug 10863: Add 2 circulation permissions: force_checkout and lift_restriction (21.89 KB, patch)
2013-12-13 19:34 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-09-10 12:36:47 UTC

    
Comment 1 Jonathan Druart 2013-09-10 12:41:34 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2013-09-10 17:54:24 UTC
A couple of issues:

1. When the logged-in user lacks the permission to lift restrictions the "lift restriction" button is gone from the circulation page but not from the patron detail page (moremember.pl).

2. "Cannot checkout" should be "Cannot check out"

3. I don't think the "high holds" warning should fall into the category of actions needing force_checkout permission. I think in this case the staff member is not overriding something which should normally be blocked. In this case the warning is informational: "Do you still want to check this out even though the loan period has been shortened?"

This definitely needs (1) fixed, (2) while you're at it. (3) I could be argued about if I misunderstand.
Comment 3 Jonathan Druart 2013-09-11 08:19:32 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2013-09-11 08:20:34 UTC
(In reply to Owen Leonard from comment #2)
> This definitely needs (1) fixed, (2) while you're at it. (3) I could be
> argued about if I misunderstand.

Owen,
Yes you are right on these 3 points. Thank you to catch them!
Comment 5 Owen Leonard 2013-09-11 14:50:34 UTC
Testing this again, I realize that there is a bigger problem with the checkout of high hold items: If the staff member doesn't have force_checkout permission they can't check out an item with high holds at all. There should be a "check out/don't check out" option but there isn't.

I would also like to suggest a change in wording for the permission. Instead of "Force checkout if a limitation exists," how about "Override blocked checkouts."
Comment 6 Jonathan Druart 2013-09-12 13:57:56 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2013-09-12 13:58:01 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2013-09-18 12:47:07 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2013-09-18 12:47:17 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2013-09-18 12:47:23 UTC Comment hidden (obsolete)
Comment 11 cedric.vita 2013-09-19 09:20:59 UTC
All works except Hold found. It is impossible to hold confimr. I do not know if the problem is the patch or not.
Message:
Software error:
Could not find a letter called Expired 'HOLD_PRINT' in the 'reserved' Module at / home/koha/src/C4/Reserves.pm line 1880.
For help, please send mail to the webmaster (webmaster @ koha36-sandbox-matrix), giving this error messages and the time and date of the error.

Set permission added work. Curious to see what kind of permission
Comment 12 Jonathan Druart 2013-09-19 09:24:09 UTC
(In reply to cedric.vita from comment #11)
> All works except Hold found. It is impossible to hold confimr. I do not know
> if the problem is the patch or not.
> Message:
> Software error:
> Could not find a letter called Expired 'HOLD_PRINT' in the 'reserved' Module
> at / home/koha/src/C4/Reserves.pm line 1880.
> For help, please send mail to the webmaster (webmaster @
> koha36-sandbox-matrix), giving this error messages and the time and date of
> the error.
> 
> Set permission added work. Curious to see what kind of permission

You must create a HOLD_PRINT notice (in tools/letters.pl).
Comment 13 cedric.vita 2013-09-19 13:48:51 UTC
Signing patches on sandbox (test1) did not work.
Thank you to "Sign-off" on.
Comment 14 Jonathan Druart 2013-09-19 14:36:16 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2013-09-19 14:36:22 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2013-09-19 14:36:27 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2013-09-27 22:53:49 UTC
Working on this now...
Comment 18 Katrin Fischer 2013-09-27 22:54:48 UTC
Fails xt/permissions.t for missing entries in various translated permissions.sql files, I will provide a follow-up.
Comment 19 Katrin Fischer 2013-09-28 10:49:10 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2013-09-28 10:55:06 UTC
Hi Jonathan, 

I have one bigger and some smaller issues with "Lift restriction for restricted accounts"

I think either the description is wrong, or it does not work like it should.
We have 3 possible restrictions on patron accounts:

- Restricted
- Lost
- Gone no address

It would be nice if it was clearer that it only affected "Restricted", but that is ok. (1)

What I have a problem with is, that you can also not restrict someone if you are missing the permission - which means it's not only for 'lifting' but also for 'setting' the restriction. Is this by design? (2)

If you don't set a date for the restriction, which you can to indicate an unlimited restriction, the message shown without permission is: "Restricted: Yes, until" (3)

Failed for (2) - please verify what the expected behaviour is and provide a follow-up. Thx!
Comment 21 Jonathan Druart 2013-09-30 13:58:26 UTC
(In reply to Katrin Fischer from comment #20)
> Hi Jonathan, 
> 
> I have one bigger and some smaller issues with "Lift restriction for
> restricted accounts"
> 
> I think either the description is wrong, or it does not work like it should.
> We have 3 possible restrictions on patron accounts:
> 
> - Restricted
> - Lost
> - Gone no address
> 
> It would be nice if it was clearer that it only affected "Restricted", but
> that is ok. (1)
> 
> What I have a problem with is, that you can also not restrict someone if you
> are missing the permission - which means it's not only for 'lifting' but
> also for 'setting' the restriction. Is this by design? (2)

Katrin,
In fact I completely forgot a staff can change the flags on editing a patron.
The goal of this patch was to hide/show the "Lift restriction" button on top of the circulation page.
But it is not enough.
Comment 22 Jonathan Druart 2013-09-30 14:06:55 UTC
(In reply to Jonathan Druart from comment #21)
> Katrin,
> In fact I completely forgot a staff can change the flags on editing a patron.
> The goal of this patch was to hide/show the "Lift restriction" button on top
> of the circulation page.
> But it is not enough.

Looking at the patch, I did not forget :)
What do you think about changing the lift_restriction permission to manage_restrictions (including Restricted, Lost and Gone no address)?
Comment 23 Katrin Fischer 2013-09-30 14:12:33 UTC
I am not sure about including the gone no adress and lost flag - but I like manage_restrictions. Maybe we can have a quick poll on IRC?
Comment 24 Jonathan Druart 2013-09-30 14:18:43 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2013-09-30 14:20:20 UTC
(In reply to Katrin Fischer from comment #23)
> I am not sure about including the gone no adress and lost flag - but I like
> manage_restrictions. Maybe we can have a quick poll on IRC?

Oops, I did not wait for your answer.
I think it makes sense to include 3 restrictions, but maybe I'm wrong.
Comment 26 I'm just a bot 2013-10-01 18:42:35 UTC
Patch applied cleanly, go forth and signoff
Comment 27 I'm just a bot 2013-10-28 06:14:45 UTC
Patch applied cleanly, go forth and signoff
Comment 28 Jonathan Druart 2013-11-05 11:20:15 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2013-11-05 11:21:54 UTC
Bug 2720 breaks existing patches. I squashed them and rewrite a part of the original patches.
Back to needs signoff.
Comment 30 Koha Team AMU 2013-11-29 14:46:26 UTC
I applied the patch on a sandbox and had the following message :
The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 10863:
<h1>Something went wrong !</h1>Applying: Bug 10863: Add 2 circulation
permissions: force_checkout and lift_restriction
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/borrower_debarments.inc
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
Failed to merge in the changes.
Patch failed at 0001 Bug 10863: Add 2 circulation permissions: force_checkout
and lift_restriction
When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 10863 - Add 2 circulation permissions: force_checkout and lift_restriction

22717 - Bug 10863: Add 2 circulation permissions: force_checkout and
lift_restriction

Apply? [(y)es, (n)o, (i)nteractive] Patch left in
tmpBug-10863-Add-2-circulation-permissions-forcecheck-Vi1MDW.patch .
Comment 31 Mathieu Saby 2013-11-29 16:22:28 UTC
I'm testing the permission for restriction (with the different types of restrictions).
It's working well, but I remarked that the description of the manage_restriction need to be updated in updatedatabase 
"Lift restriction for restricted accounts"
(and its name could also be changed in the title of the patch, but it's cosmetic)

And 13.00.XXX should now be 15.00.XXX in updatedatabase

Now I'm testing the force_checkout permission.

Mathieu
Comment 32 Mathieu Saby 2013-11-29 16:29:36 UTC
It's not the purpose of this patch, but 2 UX remarks :

- it could maybe be interesting to manage the 2 flags "lost card" and "gone no adress" as 2 kinds of restrictions, using the new restriction system.
By the way, I don't know if "flag" is meaningful in english in this context, but in French the translation has always be difficult.

- when a librarian has the rights to manage restrictions, he must click on restriction tab, or on "View restrictions" button to manage them, but Koha does not tell him that he will have the possibility to manage the restriction.
Maybe the button "View restriction" should be pĥrased "Manage restriction" when the librarian has the manage_restriction permission.

Mathieu
Comment 33 Mathieu Saby 2013-11-29 17:22:24 UTC
For force_checkout permission : 
I tried to play with some Circulation syprefs  ( AllowFineOverride ,  IssueLostItem,  OverduesBlockCirc ) and to make checkouts 
- of a lost item
- of a not for loan item
- of an item checked out to an other patron
- of a document marked "PEGI 12" to a 4 year child
- with a return date choosen in the past with the datepicker
- to a patron with overdues
- to a patron with fines exceding limit
- to a patron with already the maximum number of documents on his card

All these operations were possible with the permission (and with some choices in sysprefs), and not possible without, except one operation : checking out a document marked "PEGI 12" to a 4 year child
(I mapped biblioitems.agerestricted with 301a field, set the preference AgeRestrictionMarker with "PEGI", and AgeRestrictionOverride to "Don't allow")

I'm failing QA just for the last point.
Maybe I misunderstood the expected behavior, but could you take a look at that please? 

Mathieu
Comment 34 Jonathan Druart 2013-12-03 10:59:42 UTC
(In reply to mathieu saby from comment #33)
> All these operations were possible with the permission (and with some
> choices in sysprefs), and not possible without, except one operation :
> checking out a document marked "PEGI 12" to a 4 year child
> (I mapped biblioitems.agerestricted with 301a field, set the preference
> AgeRestrictionMarker with "PEGI", and AgeRestrictionOverride to "Don't
> allow")

Thanks for testing Mathieu!
The AgeRestrictionOverride pref has to be set to "Allow" if you want to override the age restriction, even if the new permission is set.
Comment 35 Mathieu Saby 2013-12-03 11:12:40 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2013-12-13 19:34:40 UTC
Created attachment 23540 [details] [review]
Bug 10863: Add 2 circulation permissions: force_checkout and lift_restriction

This patch adds 2 circulation permissions: force_checkout and
lift_restriction.

Both are added to existing patrons having circulation flags.

force_checkout allows a librarian to force a checkout if a limitation
occurred.

lift_restriction allows a librarian to lift a restriction for a patron.

Test plan:
1/ Find a debarred patron and go on the checkout page. The "Lift
restriction" button should be present only if the logged librarian has
the lift_restriction permission.
2/ If the force_checkout permission is set, a librarian should be
allowed to check out in several cases:
- age restriction
- the item is issued to another patron
- the item is not for loan
- the patron has overdue items
- the item is lost
- the item is a high demand item
- the item is reserved
- another case ?

Signed-off-by: Cedric Vita <cedric.vita@dracenie.com>

Bug 10863: The force checkout permission should not affect high holds

- typo checkout => check out.
- the force checkout permission does not affect high holds.

Signed-off-by: Cedric Vita <cedric.vita@dracenie.com>

Bug 10863: Follow-up: Adding missing permissions to translated files

This patch adds the 2 new permissions to all remaining sample files.
If the permissions don't get installed, the problem is bigger than
having an English description.

Also adds back the question "Check out anyway?" for high demand items (HIGHHOLDS).

Test xt/permissions.t passes now.

Bug 10863: Rename lift_restriction with manage_restrictions

The pref manage_restrictions now takes into account "Lost card" and
"Gone no address" restrictions.

Test plan:
- log in with a user with manage_restrictions permission
- verify you can set/unset restrictions when editing a patron
- log in with a user without manage_restrictions permission
- verify you cannot set/unset restrictions when editing a patron

Signed-off-by: Mathieu Saby <mathieu.saby@univ-rennes2.fr>
Note: The AgeRestrictionOverride pref has to be set to "Allow" if you want to override the age restriction, even if the new permission is set.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Passes koha-qa.pl, works as advertised
Comment 37 Galen Charlton 2013-12-13 20:04:42 UTC
(In reply to mathieu saby from comment #31)
> And 13.00.XXX should now be 15.00.XXX in updatedatabase

As a general note from the RM: I don't care at all what the stub DBrev number is set to so long as it contains XXX as expected by CheckVersion in updatedatabase.pl -- it's going to get changed anyway when the patch is pushed to master.
Comment 38 Galen Charlton 2014-01-07 04:39:23 UTC
Pushed to master, along with a follow-up to make it clear to a circ operator who lacks the force_checkouts permission how to proceed if they cannot make the loan.

Thanks, Jonathan!