Bug 22806 - CanBookBeReserve and CanItemBeReserve do not check AllowHoldsOnPatronsPossessions
Summary: CanBookBeReserve and CanItemBeReserve do not check AllowHoldsOnPatronsPossess...
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: master
Hardware: All All
: P4 normal (vote)
Assignee: Arthur Suzuki
QA Contact: Testopia
URL:
Keywords:
: 24466 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-04-29 16:59 UTC by Arthur Suzuki
Modified: 2020-11-16 18:29 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
This enhancement makes sure that checks for the "AllowHoldsOnPatronsPossessions" policy is now made for all interfaces (Staff interface, OPAC, WebServices). Before this change it was not checked for WebServices (ILS-DI). "AllowHoldPolicyOverride" can be used to override this setting for the Staff interface. Note: This enhancement introduces a behaviour change, so if you use either of these system preferences review the settings to make sure they work as expected.
Version(s) released in:
20.11.00


Attachments
Bug 22806 : CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (1.97 KB, patch)
2019-04-29 17:04 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806 : CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (2.05 KB, patch)
2019-05-14 00:33 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806 : (follow-up) CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (1.34 KB, patch)
2019-07-26 00:44 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 22806 : (follow-up) CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (1.40 KB, patch)
2019-10-18 07:40 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806 : CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (2.11 KB, patch)
2019-10-18 07:56 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: Unit Tests (1.79 KB, patch)
2020-01-25 23:31 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806 : CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (2.11 KB, patch)
2020-03-02 10:54 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806 : (follow-up) CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (1.40 KB, patch)
2020-03-02 10:55 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (2.11 KB, patch)
2020-03-02 10:57 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: (follow-up) CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (1.40 KB, patch)
2020-03-02 10:57 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: Unit Tests (1.87 KB, patch)
2020-03-02 10:58 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (2.11 KB, patch)
2020-03-04 10:04 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: (follow-up) CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (1.40 KB, patch)
2020-03-04 10:04 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: Unit Tests (1.87 KB, patch)
2020-03-04 10:04 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (2.16 KB, patch)
2020-03-05 10:34 UTC, David Nind
Details | Diff | Splinter Review
Bug 22806: (follow-up) CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (1.45 KB, patch)
2020-03-05 10:34 UTC, David Nind
Details | Diff | Splinter Review
Bug 22806: Unit Tests (1.99 KB, patch)
2020-03-05 10:34 UTC, David Nind
Details | Diff | Splinter Review
Bug 22806: Unit Tests (4.83 KB, patch)
2020-07-05 20:28 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (2.21 KB, patch)
2020-07-11 07:31 UTC, David Nind
Details | Diff | Splinter Review
Bug 22806: (follow-up) CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (1.50 KB, patch)
2020-07-11 07:31 UTC, David Nind
Details | Diff | Splinter Review
Bug 22806: Unit Tests (4.88 KB, patch)
2020-07-11 07:31 UTC, David Nind
Details | Diff | Splinter Review
Bug 22806: (QA follow-up) (5.05 KB, patch)
2020-08-04 13:00 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: (QA follow-up) (7.82 KB, patch)
2020-08-04 22:24 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (2.22 KB, patch)
2020-09-15 11:37 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: (follow-up) CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (1.50 KB, patch)
2020-09-15 11:37 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: Unit Tests (5.75 KB, patch)
2020-09-15 11:37 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (2.22 KB, patch)
2020-09-30 09:36 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: (follow-up) CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (1.50 KB, patch)
2020-09-30 09:36 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: Unit Tests (5.70 KB, patch)
2020-09-30 09:36 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: (QA follow-up) (8.16 KB, patch)
2020-09-30 09:36 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 22806: CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (2.27 KB, patch)
2020-10-15 16:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22806: (follow-up) CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions (1.55 KB, patch)
2020-10-15 16:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22806: Unit Tests (5.76 KB, patch)
2020-10-15 16:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22806: (QA follow-up) (8.21 KB, patch)
2020-10-15 16:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22806: Remove perlcritic errors (1.41 KB, patch)
2020-11-02 10:30 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 Arthur Suzuki 2019-04-29 16:59:24 UTC
ILS-DI methods HoldTitle and HoldItem do not check this syspref.
Comment 1 Arthur Suzuki 2019-04-29 17:04:21 UTC Comment hidden (obsolete)
Comment 2 Liz Rea 2019-04-30 19:58:37 UTC Comment hidden (obsolete)
Comment 3 Arthur Suzuki 2019-05-14 00:25:09 UTC Comment hidden (obsolete)
Comment 4 Arthur Suzuki 2019-05-14 00:33:56 UTC Comment hidden (obsolete)
Comment 5 Alex Buckley 2019-07-26 00:44:00 UTC Comment hidden (obsolete)
Comment 6 Alex Buckley 2019-07-26 00:47:51 UTC
Hi Arthur, 

Thanks for making this contribution. 

I notice that the $patron and $biblio that you added into the CanItemBeReserved() function were not instantiated and this caused an error. 

I have fixed that up in my followup patch. 

Liz would you be able to test both patches?

Cheers,
Alex
Comment 7 Arthur Suzuki 2019-07-26 07:24:42 UTC
Hi Alex,
Thanks for the followup :)
I guess someone else has to signoff the patch now!
Have a nice one,
Arthur
Comment 8 Arthur Suzuki 2019-10-18 07:40:29 UTC Comment hidden (obsolete)
Comment 9 Arthur Suzuki 2019-10-18 07:56:35 UTC Comment hidden (obsolete)
Comment 10 Nick Clemens 2019-10-25 12:27:55 UTC
Unit tests please
Comment 11 Arthur Suzuki 2020-01-25 23:31:11 UTC Comment hidden (obsolete)
Comment 12 David Nind 2020-02-07 10:29:52 UTC Comment hidden (obsolete)
Comment 13 Arthur Suzuki 2020-03-02 10:54:39 UTC Comment hidden (obsolete)
Comment 14 Arthur Suzuki 2020-03-02 10:55:08 UTC Comment hidden (obsolete)
Comment 15 Arthur Suzuki 2020-03-02 10:57:20 UTC Comment hidden (obsolete)
Comment 16 Arthur Suzuki 2020-03-02 10:57:48 UTC Comment hidden (obsolete)
Comment 17 Arthur Suzuki 2020-03-02 10:58:25 UTC Comment hidden (obsolete)
Comment 18 Arthur Suzuki 2020-03-02 11:00:45 UTC
(In reply to David Nind from comment #12)
> Patch no longer applies.

Hi David,
thanks for your help on that one, I've rebased the patch on current master and made sure the tests and qa tools passes (is it passed qa then?)
patches ready for testing :)
Kr,
Arthur
Comment 19 David Nind 2020-03-03 09:40:17 UTC
Sorry, I wasn't quick enough to retest. The patch no longer applies:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 22806 : CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions
Applying: Bug 22806 : (follow-up) CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions
Applying: Bug 22806: CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions
Using index info to reconstruct a base tree...
M	C4/Reserves.pm
Falling back to patching base and 3-way merge...
Auto-merging C4/Reserves.pm
CONFLICT (content): Merge conflict in C4/Reserves.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 22806: CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions

Notes:

- ILS-DI request used for testing on koha-testing-docker 
  http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=19&bib_id=11&item_id=32

- ILS-DI requests causes DataTable error o Patrons hold tab: see bug 24769
Comment 20 Arthur Suzuki 2020-03-04 10:04:08 UTC Comment hidden (obsolete)
Comment 21 Arthur Suzuki 2020-03-04 10:04:26 UTC Comment hidden (obsolete)
Comment 22 Arthur Suzuki 2020-03-04 10:04:49 UTC Comment hidden (obsolete)
Comment 23 Arthur Suzuki 2020-03-04 10:06:06 UTC
Hi David,
My apologizes, the patch was rebased on another branch than master...
Now ready to test (ahem)
Arthur
Comment 24 David Nind 2020-03-05 10:31:13 UTC
Notes from testing:

- Step 5 of the test plan: message should be "itemAlreadyOnLoan", not "NotHoldable"
Comment 25 David Nind 2020-03-05 10:34:00 UTC Comment hidden (obsolete)
Comment 26 David Nind 2020-03-05 10:34:04 UTC Comment hidden (obsolete)
Comment 27 David Nind 2020-03-05 10:34:08 UTC Comment hidden (obsolete)
Comment 28 Marcel de Rooy 2020-03-06 08:07:14 UTC
QAing
Comment 29 Marcel de Rooy 2020-03-06 08:20:50 UTC
The title of this report and the code changes in C4::Reserves do not match. I expect changes in ISLDI and I see them in C4::Reserves themselves. Please explain.

Since you change code in Reserves, it amazes me too to see only tests in a ILSDI script. What about Holds.t and Reserves.t ?

If this is not only about ILSDI, the title is misleading?
Comment 30 Arthur Suzuki 2020-07-05 16:36:15 UTC
Goeie avond Marcel,
inderdaat, the change also impact C4/Reserves.
Although I noticed the bug while using ILSDI, operations from OPAC and Staff interface are also impacted.
And I think its better to have as much coherences between all interfaces, hence my modification in C4/Reserves.
I will modify the tests and the title in consequences :)
Mvg,
Arthur
Comment 31 Arthur Suzuki 2020-07-05 20:28:55 UTC Comment hidden (obsolete)
Comment 32 Arthur Suzuki 2020-07-05 20:30:33 UTC
added some tests to C4/Reserves as well :)
Comment 33 David Nind 2020-07-11 07:31:28 UTC
Created attachment 106778 [details] [review]
Bug 22806: CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions

Test plan :
1 - set AllowHoldsOnPatronsPossessions to "Don't Allow"
2 - Checkout an item to a borrower
3 - Try to reserve an item using ILS-DI WebService -> Will work without complaining.
4 - Cancel the hold and apply patch
5 - Repeat 3 -> Should not place hold and show error "NotHoldable"

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 34 David Nind 2020-07-11 07:31:32 UTC
Created attachment 106779 [details] [review]
Bug 22806: (follow-up) CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions

As CanBookBeReserved() was failing as $patron and and $biblio were not
instantiated I fixed that up.

Test plan :
1 - set AllowHoldsOnPatronsPossessions to "Don't Allow"
2 - Checkout an item to a borrower
3 - Try to reserve an item using ILS-DI WebService -> Will work without complaining.
4 - Cancel the hold and apply patch
5 - Repeat 3 -> Should not place hold and show error "NotHoldable"

Sponsored-By: Catalyst IT
Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 35 David Nind 2020-07-11 07:31:35 UTC
Created attachment 106780 [details] [review]
Bug 22806: Unit Tests

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 36 Arthur Suzuki 2020-07-11 15:20:51 UTC
yay, super thanks for signing off!
Comment 37 Nick Clemens 2020-07-31 17:05:55 UTC
This works as described, I do see that we have checks for this in reserve.pl and opac-reserve.pl - can they be removed now that they are being added to the modules?
Comment 38 Arthur Suzuki 2020-08-04 13:00:57 UTC
Created attachment 107760 [details] [review]
Bug 22806: (QA follow-up)
Comment 39 Arthur Suzuki 2020-08-04 13:02:06 UTC
(In reply to Nick Clemens from comment #37)
> This works as described, I do see that we have checks for this in reserve.pl
> and opac-reserve.pl - can they be removed now that they are being added to
> the modules?

Indeed! Done in the QA patch :)
Comment 40 Nick Clemens 2020-08-04 13:53:29 UTC
(In reply to Arthur Suzuki from comment #39)
> (In reply to Nick Clemens from comment #37)
> > This works as described, I do see that we have checks for this in reserve.pl
> > and opac-reserve.pl - can they be removed now that they are being added to
> > the modules?
> 
> Indeed! Done in the QA patch :)

Thanks Arthur!

Two things I note:

1 - 
With this patchset the message to the patron changes from"
 This title cannot be requested because it's already in your possession.
To:
 There are no items that can be placed on hold.
We should pass the exact reason if possible

2 -
On the staff side before these patches I am warned that patron has this in possession, but I can still place the hold
After the patches I cannot place a hold unless AllowHoldPolicyOverride is set
This is probably appropriate, but it is a behaviour change and should be noted in release notes
Comment 41 Arthur Suzuki 2020-08-04 22:24:16 UTC
Created attachment 107806 [details] [review]
Bug 22806: (QA follow-up)
Comment 42 Arthur Suzuki 2020-08-04 22:31:18 UTC
(In reply to Nick Clemens from comment #40)
> Thanks Arthur!

you're welcome Sir!

> 1 - 
> With this patchset the message to the patron changes from"
>  This title cannot be requested because it's already in your possession.
> To:
>  There are no items that can be placed on hold.
> We should pass the exact reason if possible

this problem has been addressed with new version of the qa patch :)
 
> 2 -
> On the staff side before these patches I am warned that patron has this in
> possession, but I can still place the hold
> After the patches I cannot place a hold unless AllowHoldPolicyOverride is set
> This is probably appropriate, but it is a behaviour change and should be
> noted in release notes

hummm... I can see there is a folder called "misc/release_notes" but not sure which file is to be edited in there, if any.

Some clue to get out of the glue? :)
Comment 43 Arthur Suzuki 2020-08-04 22:39:28 UTC
Made a text proposal on the appropriate field on bugzilla (in the mean time)
Comment 44 Magnus Enger 2020-09-02 11:39:43 UTC
*** Bug 24466 has been marked as a duplicate of this bug. ***
Comment 45 Magnus Enger 2020-09-02 11:58:56 UTC
(In reply to Arthur Suzuki from comment #43)
> Made a text proposal on the appropriate field on bugzilla (in the mean time)

I think that is the right thing to do. There are scripts that make sure "Text to go in the release notes" actually goes into the release notes.
Comment 46 Magnus Enger 2020-09-02 12:05:03 UTC
Does this have to be "Needs Signoff" because of the QA followup patch, or can it go back to QA directly?
Comment 47 David Nind 2020-09-05 21:21:43 UTC
I tried to retest, but the patches no longer apply 8-(

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 22806: CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions
Using index info to reconstruct a base tree...
M	C4/Reserves.pm
Falling back to patching base and 3-way merge...
Auto-merging C4/Reserves.pm
Applying: Bug 22806: (follow-up) CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions
Applying: Bug 22806: Unit Tests
Using index info to reconstruct a base tree...
M	t/db_dependent/ILSDI_Services.t
M	t/db_dependent/Reserves.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Reserves.t
CONFLICT (content): Merge conflict in t/db_dependent/Reserves.t
Auto-merging t/db_dependent/ILSDI_Services.t
CONFLICT (content): Merge conflict in t/db_dependent/ILSDI_Services.t
error: Failed to merge in the changes.
Patch failed at 0001 Bug 22806: 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-22806-Unit-Tests-Z7uKNQ.patch
Comment 48 Arthur Suzuki 2020-09-15 11:37:23 UTC
Created attachment 110100 [details] [review]
Bug 22806: CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions

Test plan :
1 - set AllowHoldsOnPatronsPossessions to "Don't Allow"
2 - Checkout an item to a borrower
3 - Try to reserve an item using ILS-DI WebService -> Will work without complaining.
4 - Cancel the hold and apply patch
5 - Repeat 3 -> Should not place hold and show error "NotHoldable"

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 49 Arthur Suzuki 2020-09-15 11:37:49 UTC
Created attachment 110101 [details] [review]
Bug 22806: (follow-up) CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions

As CanBookBeReserved() was failing as $patron and and $biblio were not
instantiated I fixed that up.

Test plan :
1 - set AllowHoldsOnPatronsPossessions to "Don't Allow"
2 - Checkout an item to a borrower
3 - Try to reserve an item using ILS-DI WebService -> Will work without complaining.
4 - Cancel the hold and apply patch
5 - Repeat 3 -> Should not place hold and show error "NotHoldable"

Sponsored-By: Catalyst IT
Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 50 Arthur Suzuki 2020-09-15 11:37:56 UTC
Created attachment 110102 [details] [review]
Bug 22806: Unit Tests

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 51 Martin Renvoize 2020-09-25 11:15:31 UTC
Sorry Arthur, I don't think you've accounted for the additional return status you've introduced in enough places.

I think you need to:
* add your new return value to the ENUM of failure reasons in club_holds_to_patron_holds
* add handling for the new return value to reserve/placerequest.pl, reserve/request.pl and request.tt

Failing QA for now.. good work in starting the move of these checks into a suitable level.
Comment 52 Martin Renvoize 2020-09-25 11:17:20 UTC
Oh.. hang on.. it looks like Nick caught these issues too and there's conversation about a QA followup patch.. but I'm not seeing it?
Comment 53 Arthur Suzuki 2020-09-28 06:47:45 UTC
yup, still, thanks martin for the helpful technical advises.
And thanks to both of you (nick, martin) for reviewing.
I'll do the follow-up.
Arthur
Comment 54 Arthur Suzuki 2020-09-30 09:36:09 UTC
Created attachment 110973 [details] [review]
Bug 22806: CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions

Test plan :
1 - set AllowHoldsOnPatronsPossessions to "Don't Allow"
2 - Checkout an item to a borrower
3 - Try to reserve an item using ILS-DI WebService -> Will work without complaining.
4 - Cancel the hold and apply patch
5 - Repeat 3 -> Should not place hold and show error "NotHoldable"

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 55 Arthur Suzuki 2020-09-30 09:36:26 UTC
Created attachment 110974 [details] [review]
Bug 22806: (follow-up) CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions

As CanBookBeReserved() was failing as $patron and and $biblio were not
instantiated I fixed that up.

Test plan :
1 - set AllowHoldsOnPatronsPossessions to "Don't Allow"
2 - Checkout an item to a borrower
3 - Try to reserve an item using ILS-DI WebService -> Will work without complaining.
4 - Cancel the hold and apply patch
5 - Repeat 3 -> Should not place hold and show error "NotHoldable"

Sponsored-By: Catalyst IT
Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 56 Arthur Suzuki 2020-09-30 09:36:31 UTC
Created attachment 110975 [details] [review]
Bug 22806: Unit Tests

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 57 Arthur Suzuki 2020-09-30 09:36:35 UTC
Created attachment 110976 [details] [review]
Bug 22806: (QA follow-up)
Comment 58 Arthur Suzuki 2020-09-30 09:38:58 UTC
(In reply to Martin Renvoize from comment #52)
> Oh.. hang on.. it looks like Nick caught these issues too and there's
> conversation about a QA followup patch.. but I'm not seeing it?

I guess I did some mis-manipulation with git bz, it was in the obsoletes :)
re-uploaded and did a rebase of the former patches on the way.
hope it will be good enough this time! (yay, fingercrossed)
Comment 59 Katrin Fischer 2020-10-11 13:12:18 UTC
Patches apply, tests pass. But I'd feel a bit better if someone else took a look here. Martin or Nick, could you continue?
Comment 60 Arthur Suzuki 2020-10-15 09:09:40 UTC
(In reply to Katrin Fischer from comment #59)
> Patches apply, tests pass. But I'd feel a bit better if someone else took a
> look here. Martin or Nick, could you continue?

Thanks to Katrin and the future second QA :)
Comment 61 Nick Clemens 2020-10-15 16:45:19 UTC
Created attachment 111771 [details] [review]
Bug 22806: CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions

Test plan :
1 - set AllowHoldsOnPatronsPossessions to "Don't Allow"
2 - Checkout an item to a borrower
3 - Try to reserve an item using ILS-DI WebService -> Will work without complaining.
4 - Cancel the hold and apply patch
5 - Repeat 3 -> Should not place hold and show error "NotHoldable"

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 62 Nick Clemens 2020-10-15 16:45:25 UTC
Created attachment 111772 [details] [review]
Bug 22806: (follow-up) CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions

As CanBookBeReserved() was failing as $patron and and $biblio were not
instantiated I fixed that up.

Test plan :
1 - set AllowHoldsOnPatronsPossessions to "Don't Allow"
2 - Checkout an item to a borrower
3 - Try to reserve an item using ILS-DI WebService -> Will work without complaining.
4 - Cancel the hold and apply patch
5 - Repeat 3 -> Should not place hold and show error "NotHoldable"

Sponsored-By: Catalyst IT
Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 63 Nick Clemens 2020-10-15 16:45:32 UTC
Created attachment 111773 [details] [review]
Bug 22806: Unit Tests

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 64 Nick Clemens 2020-10-15 16:45:38 UTC
Created attachment 111774 [details] [review]
Bug 22806: (QA follow-up)

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 65 Nick Clemens 2020-10-15 16:47:16 UTC
This will change behaviour on staff side I believe - before there was no check of patron possession, but it is overrideable and so I believe is part of the bug fix, not a problem
Comment 66 Arthur Suzuki 2020-10-19 09:07:01 UTC
thanks for the qa :)
Comment 67 Jonathan Druart 2020-10-25 22:05:50 UTC
(In reply to Martin Renvoize from comment #51)
> I think you need to:
> * add your new return value to the ENUM of failure reasons in
> club_holds_to_patron_holds

Isn't it still valid?
Comment 68 Jonathan Druart 2020-11-02 10:05:22 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 69 Jonathan Druart 2020-11-02 10:30:32 UTC
Created attachment 112824 [details] [review]
Bug 22806: Remove perlcritic errors

Subroutine "new" called using indirect syntax at line 415, column 18.  See page 349 of PBP.  (Severity: 4)
Subroutine "new" called using indirect syntax at line 420, column 14.  See page 349 of PBP.  (Severity: 4)
Comment 70 Arthur Suzuki 2020-11-02 10:37:11 UTC
Yes! Thank you Jonathan! :)
Comment 71 Jonathan Druart 2020-11-02 12:51:27 UTC
(In reply to Jonathan Druart from comment #69)
> Created attachment 112824 [details] [review] [review]
> Bug 22806: Remove perlcritic errors
> 
> Subroutine "new" called using indirect syntax at line 415, column 18.  See
> page 349 of PBP.  (Severity: 4)
> Subroutine "new" called using indirect syntax at line 420, column 14.  See
> page 349 of PBP.  (Severity: 4)

Pushed to master.
Comment 72 Lucas Gass 2020-11-16 18:29:01 UTC
patchset does not apply clean to 20.05.x, no backport