Bug 10863

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.

(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!

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."

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

(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).

Signing patches on sandbox (test1) did not work.
Thank you to "Sign-off" on.

Working on this now...

Fails xt/permissions.t for missing entries in various translated permissions.sql files, I will provide a follow-up.

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!

(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.

(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)?

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?

(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.

Patch applied cleanly, go forth and signoff

Patch applied cleanly, go forth and signoff

Bug 2720 breaks existing patches. I squashed them and rewrite a part of the original patches.
Back to needs signoff.

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 .

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

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

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

(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.

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

(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.

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!