Bug 13116 - Make it possible to propagate errors from C4::Reserves::CanItemBeReserved() to the web-templates.
Summary: Make it possible to propagate errors from C4::Reserves::CanItemBeReserved() t...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Olli-Antti Kivilahti
QA Contact: Testopia
URL:
Keywords:
Depends on: 13113
Blocks:
  Show dependency treegraph
 
Reported: 2014-10-20 16:07 UTC by Olli-Antti Kivilahti
Modified: 2015-06-04 23:33 UTC (History)
6 users (show)

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


Attachments
Bug 13116 - Make it possible to propagate errors from C4::Reserves::CanItemBeReserved() to the web-templates. (10.10 KB, patch)
2014-10-20 16:08 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13116 - Make it possible to propagate errors from C4::Reserves::CanItemBeReserved() to the web-templates. (10.87 KB, patch)
2014-10-21 11:48 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13116 - Make it possible to propagate errors from C4::Reserves::CanItemBeReserved() to the web-templates. (11.98 KB, patch)
2014-10-21 12:06 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13116 - Make it possible to propagate errors from C4::Reserves::CanItemBeReserved() to the web-templates. (18.37 KB, patch)
2014-10-21 12:32 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13116 - Make it possible to propagate errors from C4::Reserves::CanItemBeReserved() to the web-templates. (17.92 KB, patch)
2014-10-23 14:46 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13116 - Make it possible to propagate errors from C4::Reserves::CanItemBeReserved() to the web-templates. (17.73 KB, patch)
2014-10-24 09:38 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13116 - Make it possible to propagate errors from C4::Reserves::CanItemBeReserved() to the web-templates. (17.98 KB, patch)
2014-10-29 14:42 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 13116 - Make it possible to propagate errors from C4::Reserves::CanItemBeReserved() to the web-templates. (17.89 KB, patch)
2014-10-31 14:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13116 [QA Followup] - Remove tabs, use unless instead of if (3.06 KB, patch)
2014-10-31 14:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13116 [QA Followup] - Remove tabs, use unless instead of if (3.10 KB, patch)
2014-10-31 14:46 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 Olli-Antti Kivilahti 2014-10-20 16:07:50 UTC
A reservation can be blocked for various reasons, but C4::Reserves::CanItemBeReserved() only either fails or not, regardless of the reason.

This enhancement makes it possible to more easily show different error messages to web-templates.
Comment 1 Olli-Antti Kivilahti 2014-10-20 16:08:51 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2014-10-20 18:32:17 UTC
Should status be "Needs Signoff"?
Comment 3 Magnus Enger 2014-10-21 10:21:59 UTC
No test plan?
Comment 4 Olli-Antti Kivilahti 2014-10-21 11:48:15 UTC Comment hidden (obsolete)
Comment 5 Olli-Antti Kivilahti 2014-10-21 12:06:26 UTC Comment hidden (obsolete)
Comment 6 Olli-Antti Kivilahti 2014-10-21 12:32:35 UTC Comment hidden (obsolete)
Comment 7 Olli-Antti Kivilahti 2014-10-21 12:34:35 UTC
Hi there early birds!
I didn't set "Needs Signoff" because I didn't think the patch was ready for testing. Just wanted to share my work at the days end to see where the feature set is moving to.
Thanks for caring though!

There are still bugs there, but setting the status to "Needs Signoff" for your leisure.

It's all your now :)
Comment 8 Olli-Antti Kivilahti 2014-10-23 14:46:17 UTC Comment hidden (obsolete)
Comment 9 Olli-Antti Kivilahti 2014-10-24 09:38:49 UTC Comment hidden (obsolete)
Comment 10 Owen Leonard 2014-10-24 14:47:56 UTC
Does this duplicate some or all of Bug 11999?
Comment 11 Olli-Antti Kivilahti 2014-10-24 15:18:08 UTC
(In reply to Owen Leonard from comment #10)
> Does this duplicate some or all of Bug 11999?

Yes it does. My feature propagates the error codes from CanItemBeReserved through CanBookBeReserved resulting in more error handling options in the templates.

I went through the Bug 11999 and it is up to preference which one should make it. I can easily refactor my feature chain to support Bug 11999, but I insist that all error handling should be centralized to the CanBook/ItemBeReserved instead of getting scattered all around scripts and modules.

This means CanBookBeReserved should somehow propagate the errors collected at CanItemBeReserved(). I think Bug 11999 has the right base for it.
Comment 12 Marc Véron 2014-10-29 14:42:41 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2014-10-31 14:44:49 UTC
Created attachment 33075 [details] [review]
Bug 13116 - Make it possible to propagate errors from C4::Reserves::CanItemBeReserved() to the web-templates.

This patch changes the way CanBookBeReserved() and CanItemBeReserved() return error
messages and how they are dealt with in the templates. This change makes it possible
to distinguish between different types of reservation failure.

Currently only two types of errors are handled, all the way to the user, from the CanItemBeReserved():
-ageRestricted
-tooManyReserves which translates to maxreserves

 #############
 - TEST PLAN -
 #############
((-- AGE RESTRICTION --))
STAFF CLIENT
1. Find a Record with Items, update the MARC Subfield 521a to "PEGI 16".
2. Get a Borrower who is younger than 16 years.
3. Place a hold for the underage Borrower for the ageRestricted Record.
4. You get a notification, that placing a hold on ageRestricted material is
   forbidden. (previously you just got a notification about maximum amount of reserves reached)

((-- MAXIMUM RESERVES REACHED --))
0. Set the  maxreserves -syspref to 3 (or any low value)
STAFF CLIENT AND OPAC
1. Make a ton of reserves for one borrower.
2. Observe the notification about maximum reserves reached blocking your reservations.

((-- MULTIPLE HOLDS STAFF CLIENT --))
3. Observe the error notification "Cannot place hold on some items"

((-- MULTIPLE HOLDS OPAC --))
1. Make a search with many results, of which atleast one is age restricted to the current borrower.
2. Select few results and "Place hold" from to result summary header element.
       (Not individual results "Place hold")
3. Observe individual Biblios getting the "age restricted"-notification, where others can be
   reserved just fine.

Updated the unit tests to match the new method return values.
t/db_dependent/Holds.t & Reserves.t

Followed test plan. Works as expected and displays meaningful messages for the reason why placing a hold is not possible.

Signed-off-by: Marc Veron <veron@veron.ch>
Comment 14 Kyle M Hall 2014-10-31 14:45:08 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2014-10-31 14:46:01 UTC
Created attachment 33077 [details] [review]
Bug 13116 [QA Followup] - Remove tabs, use unless instead of if
Comment 16 Tomás Cohen Arazi 2014-11-12 14:43:53 UTC
Patches pushed to master.

Thanks Olli!