Bug 10247 - Too many reserves due to syspref does not show warning
Summary: Too many reserves due to syspref does not show warning
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin SOMERS
QA Contact:
URL:
Keywords:
Depends on: 9394
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-15 08:38 UTC by Fridolin SOMERS
Modified: 2014-12-07 20:07 UTC (History)
4 users (show)

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


Attachments
Proposed patch (6.98 KB, patch)
2013-05-15 10:00 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10247 - Too many reserves due to syspref does not show warning (6.46 KB, patch)
2013-06-11 01:47 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 10247 - Too many reserves due to syspref does not show warning (6.50 KB, patch)
2013-06-21 13:40 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 Fridolin SOMERS 2013-05-15 08:38:47 UTC
On intranet, when placing a reserve :
If borrower as reached the reserves quota, a warning should appear : 
  Cannot place hold
  * Too Many Holds: TEST has too many holds.

This works when quota defined in circulation rules is reached but not when quota specified in syspref maxreserves is reached, in case this quota is lower than the one defined is circulation rules.
Comment 1 Fridolin SOMERS 2013-05-15 10:00:25 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2013-06-03 00:36:43 UTC
Then wouldn't it be better to remove maxreserves syspref alltogether?
Comment 3 Fridolin SOMERS 2013-06-06 13:16:39 UTC
(In reply to comment #2)
> Then wouldn't it be better to remove maxreserves syspref alltogether?

Why removing such a feature ? this bug is just a small variable problem.
Maybe open a discussion on koah-devel mailing list.
Comment 4 Srdjan Jankovic 2013-06-06 23:26:38 UTC
Well if it is in circulation rules (where it should be), maybe the syspref is redundant.
Comment 5 Fridolin SOMERS 2013-06-07 07:07:49 UTC
(In reply to comment #4)
> Well if it is in circulation rules (where it should be), maybe the syspref
> is redundant.

It is in circulation rules indeed but I think the syspref is used to temporary reduce reserves for all users, for holidays for example. Changing a syspref is much more easy that all circulation rules.
Comment 6 Srdjan Jankovic 2013-06-11 01:04:40 UTC
Test plan :
    - Select a borrower and an item
    - Look at reserve quota for the borrower category, the item branch and the item type
    - Set maxreserves syspref with a quota lower than the circulation quota
    - Perform reserves for borrower to reach the quota
    - Try to perform one more reserve
    => You get the warning "too many holds"
    - Set maxreserves syspref with a quota higher than the circulation quota
    - Try to perform one more reserve
    => You get the warning "too many holds"
    Check you get a warning for alreadyreserved, none_available and alreadypossession cases.
    Check you get a message for expiry and diffbranch cases.
Comment 7 Srdjan Jankovic 2013-06-11 01:47:42 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2013-06-21 13:40:40 UTC
Created attachment 19190 [details] [review]
Bug 10247 - Too many reserves due to syspref does not show warning

On intranet, when placing a reserve :
If borrower as reached the reserves quota, a warning should appear :
  Cannot place hold
  * Too Many Holds: TEST has too many holds.

This works when quota defined in circulation rules is reached but not when quota specified in syspref maxreserves is reached, in case this quota is lower than the one defined is circulation rules.

This is because template param maxreserves is set twice, this patch corrects this bug.
Also moves warnings and messages conditionnal variable from perl to template because it cause the same bug as for maxreserves.

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Galen Charlton 2013-08-12 15:04:52 UTC
Pushed to master.  Thanks, Fridolyn!
Comment 10 Tomás Cohen Arazi 2013-09-10 18:08:36 UTC
It doesn't apply or work on 3.12.x because of its dependency on 9394. Please provide a patch that applies to the stable branch, or comment if you think 9394 should be backported (maybe other patches depend on it). Thanks
Comment 11 Fridolin SOMERS 2013-12-19 08:24:04 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> It doesn't apply or work on 3.12.x because of its dependency on 9394. Please
> provide a patch that applies to the stable branch, or comment if you think
> 9394 should be backported (maybe other patches depend on it). Thanks

Hie,
For me, this bug does not depend on Bug 9394, its a small patch impacting old code.
And actually, on 3.12.07, patch applies on 3.12.x branch.
Can you confirm ?