Bug 19135 - AllowHoldsOnPatronsPossessions is not working
Summary: AllowHoldsOnPatronsPossessions is not working
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 14695
Blocks:
  Show dependency treegraph
 
Reported: 2017-08-17 15:49 UTC by Kyle M Hall
Modified: 2019-10-14 19:57 UTC (History)
7 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 19135 - AllowHoldsOnPatronsPossessions not working (1.95 KB, patch)
2017-08-17 15:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED OFF] Bug 19135 - AllowHoldsOnPatronsPossessions not working (2.02 KB, patch)
2017-08-17 17:02 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 19135 - AllowHoldsOnPatronsPossessions not working (2.10 KB, patch)
2017-10-06 09:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19135: Restore AllowHoldsOnPatronsPossessions behaviour (1.69 KB, patch)
2017-10-06 14:39 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2017-08-17 15:49:45 UTC

    
Comment 1 Kyle M Hall 2017-08-17 15:54:11 UTC
Created attachment 66148 [details] [review]
Bug 19135 - AllowHoldsOnPatronsPossessions not working

Test Plan:
1) Set AllowHoldsOnPatronsPossessions to "Don't"
2) Check out an item to a patron
3) Place a hold on that item for the same patron
4) Note you are allowed to with no alert
5) Delete the hold
6) Apply this patch
7) Place a hold on that item for the same patron
8) Note you recieve an alert now

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Christopher Brannon 2017-08-17 17:02:31 UTC
Created attachment 66149 [details] [review]
[SIGNED OFF] Bug 19135 - AllowHoldsOnPatronsPossessions not working

Test Plan:
1) Set AllowHoldsOnPatronsPossessions to "Don't"
2) Check out an item to a patron
3) Place a hold on that item for the same patron
4) Note you are allowed to with no alert
5) Delete the hold
6) Apply this patch
7) Place a hold on that item for the same patron
8) Note you recieve an alert now



Works as expected.

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 3 Jonathan Druart 2017-08-31 14:30:02 UTC
Please update the commit message with something more meaningful (what the patch does?)

Why do not you just remove the superfluous line? I will make the diff easier to read and backport.
Comment 4 Jonathan Druart 2017-10-02 15:50:51 UTC
Upping severity, it impacts last 2 stable releases (17.05.x and 16.11.x).
Comment 5 Katrin Fischer 2017-10-03 00:28:50 UTC
Hi Jonathan, commit message includes a test plan, what did you mean?
Comment 6 Jonathan Druart 2017-10-03 14:14:32 UTC
(In reply to Katrin Fischer from comment #5)
> Hi Jonathan, commit message includes a test plan, what did you mean?

"AllowHoldsOnPatronsPossessions not working" is not a correct and explicit commit title.
Comment 7 Jonathan Druart 2017-10-03 17:49:45 UTC
My remarks should not block QA, I can edit the commit message later if the author of the patch does not do it.
Comment 8 Marcel de Rooy 2017-10-06 09:45:36 UTC
QA: Looking here now
Comment 9 Marcel de Rooy 2017-10-06 09:52:33 UTC
Created attachment 67684 [details] [review]
Bug 19135 - AllowHoldsOnPatronsPossessions not working

Test Plan:
1) Set AllowHoldsOnPatronsPossessions to "Don't"
2) Check out an item to a patron
3) Place a hold on that item for the same patron
4) Note you are allowed to with no alert
5) Delete the hold
6) Apply this patch
7) Place a hold on that item for the same patron
8) Note you recieve an alert now

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2017-10-06 09:54:32 UTC
Hmm two signoffs on my name. Where did this patch come from?
Comment 11 Jonathan Druart 2017-10-06 14:34:07 UTC
Bug caused by
  commit bc39f0392bbebaad4c083f81308f652a325be042
    Bug 14695 - Add ability to place multiple item holds on a given record per patron


(In reply to Jonathan Druart from comment #3)
> Why do not you just remove the superfluous line? I will make the diff easier
> to read and backport.

I am going to amend the patch to only remove the problematic line in order to make the change easier to read.
Comment 12 Jonathan Druart 2017-10-06 14:39:24 UTC
Created attachment 67718 [details] [review]
Bug 19135: Restore AllowHoldsOnPatronsPossessions behaviour

Bug caused by
  commit bc39f0392bbebaad4c083f81308f652a325be042
  Bug 14695 - Add ability to place multiple item holds on a given record per patron

Test Plan:
1) Set AllowHoldsOnPatronsPossessions to "Don't"
2) Check out an item to a patron
3) Place a hold on that item for the same patron
4) Note you are allowed to with no alert
5) Delete the hold
6) Apply this patch
7) Place a hold on that item for the same patron
8) Note you recieve an alert now

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Jonathan Druart 2017-10-06 15:17:45 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 14 Nick Clemens 2017-10-13 14:58:39 UTC
*** Bug 13374 has been marked as a duplicate of this bug. ***
Comment 15 Fridolin Somers 2017-10-25 08:31:46 UTC
Pushed to 17.05.x, will be in 17.05.06.

Lines are impacted by Bug 17829 but rebase was easy.
Comment 16 Katrin Fischer 2017-10-29 18:14:42 UTC
This patch has been pushed to 16.11.x and will be in 16.11.14.