Bug 15494

Summary: Block renewals by arbitrary item values
Product: Koha Reporter: Nicole C. Engard <nengard>
Component: CirculationAssignee: Nick Clemens <nick>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, jdemuth, katrin.fischer, kyle.m.hall, nick
Version: master   
Hardware: All   
OS: All   
URL: http://devs.bywatersolutions.com/projects/make-longoverduelost-items-not-renewable/
Whiteboard:
Change sponsored?: Sponsored Patch complexity: Small patch
Bot Control: --- When did the bot last check this:
Who signed the patch off: Text to go in the release notes:
Attachments: Bug 15494 - Unit tests
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed
Bug 15494 - Update display of renewal errors
Bug 15494 - Add syspref
Bug 15494 - Unit tests
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed
Bug 15494 - Update display of renewal errors
Bug 15494 - Add syspref
Bug 15494 - Unit tests
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed
Bug 15494 - Update display of renewal errors
Bug 15494 - Add syspref

Description Nicole C. Engard 2016-01-06 16:27:14 UTC
This development would allow a library to prevent the renewal of items based on the values of any field in the items table including but not limited to any itemlost value.

Work to be done:
1) Add new system preference ItemsDeniedRenewal patterned after OpacHiddenItems
2) Modify C4::Circulation::CanBookBeRenewed to check this system preference


Seeking Co-sponsors: http://devs.bywatersolutions.com/projects/make-longoverduelost-items-not-renewable/
Comment 1 Nick Clemens 2017-05-24 14:46:16 UTC
Created attachment 63700 [details] [review]
Bug 15494 - Unit tests

prove t/db_dependent/Circulation.t
Comment 2 Nick Clemens 2017-05-24 14:46:19 UTC
Created attachment 63701 [details] [review]
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed
Comment 3 Nick Clemens 2017-05-24 14:46:22 UTC
Created attachment 63702 [details] [review]
Bug 15494 - Update display of renewal errors

To test:
1 - Define a rule to deny renewal
2 - Checkout an item meeting this rule
3 - Attempt renew an item via the OPAC
4 - Attempt renewal via SIP
5 - Attempt renewal via staff client
6 - Verify errors are reasonable
Comment 4 Nick Clemens 2017-05-24 14:46:25 UTC
Created attachment 63703 [details] [review]
Bug 15494 - Add syspref
Comment 5 Katrin Fischer 2017-10-08 09:51:08 UTC
Code looks good, but there is a bigger conflict in the test file I am not sure about, can you please rebase? 

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 15494 - Unit tests
Using index info to reconstruct a base tree...
M	t/db_dependent/Circulation.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Circulation.t
CONFLICT (content): Merge conflict in t/db_dependent/Circulation.t
error: Failed to merge in the changes.
Patch failed at 0001 Bug 15494 - Unit tests
The copy of the patch that failed is found in: .git/rebase-apply/patch
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".
Patch left in /tmp/Bug-15494---Unit-tests-pu3VeI.patch
Comment 6 Nick Clemens 2017-10-10 16:35:30 UTC
Created attachment 67886 [details] [review]
Bug 15494 - Unit tests

prove t/db_dependent/Circulation.t
Comment 7 Nick Clemens 2017-10-10 16:35:33 UTC
Created attachment 67887 [details] [review]
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed
Comment 8 Nick Clemens 2017-10-10 16:35:36 UTC
Created attachment 67888 [details] [review]
Bug 15494 - Update display of renewal errors

To test:
1 - Define a rule to deny renewal
2 - Checkout an item meeting this rule
3 - Attempt renew an item via the OPAC
4 - Attempt renewal via SIP
5 - Attempt renewal via staff client
6 - Verify errors are reasonable
Comment 9 Nick Clemens 2017-10-10 16:35:39 UTC
Created attachment 67889 [details] [review]
Bug 15494 - Add syspref
Comment 10 Katrin Fischer 2017-10-14 23:09:20 UTC
This seems to work quite well! Also tested renew through checkout, renewal tab and OPAC.

When renewing the item from the list of checkouts and using the renewal limit override functionality, the message displayed is:

Renew failed: item_denied_renewal

This could be improved, maybe with a small follow-up?
Comment 11 Katrin Fischer 2017-10-14 23:09:52 UTC
Created attachment 68146 [details] [review]
Bug 15494 - Unit tests

prove t/db_dependent/Circulation.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2017-10-14 23:09:56 UTC
Created attachment 68147 [details] [review]
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Katrin Fischer 2017-10-14 23:10:00 UTC
Created attachment 68148 [details] [review]
Bug 15494 - Update display of renewal errors

To test:
1 - Define a rule to deny renewal
2 - Checkout an item meeting this rule
3 - Attempt renew an item via the OPAC
4 - Attempt renewal via SIP
5 - Attempt renewal via staff client
6 - Verify errors are reasonable

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2017-10-14 23:10:04 UTC
Created attachment 68149 [details] [review]
Bug 15494 - Add syspref

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>