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

Hi Alex,
Thanks for the followup :)
I guess someone else has to signoff the patch now!
Have a nice one,
Arthur

Unit tests please

(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

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

Hi David,
My apologizes, the patch was rebased on another branch than master...
Now ready to test (ahem)
Arthur

Notes from testing:

- Step 5 of the test plan: message should be "itemAlreadyOnLoan", not "NotHoldable"

QAing

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?

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

added some tests to C4/Reserves as well :)

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>

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>

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>

yay, super thanks for signing off!

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?

Created attachment 107760 [details] [review]
Bug 22806: (QA follow-up)

(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 :)

(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

Created attachment 107806 [details] [review]
Bug 22806: (QA follow-up)

(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? :)

Made a text proposal on the appropriate field on bugzilla (in the mean time)

*** Bug 24466 has been marked as a duplicate of this bug. ***

(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.

Does this have to be "Needs Signoff" because of the QA followup patch, or can it go back to QA directly?

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

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>

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>

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>

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.

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?

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

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>

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>

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>

Created attachment 110976 [details] [review]
Bug 22806: (QA follow-up)

(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)

Patches apply, tests pass. But I'd feel a bit better if someone else took a look here. Martin or Nick, could you continue?

(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 :)

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>

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>

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>

Created attachment 111774 [details] [review]
Bug 22806: (QA follow-up)

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

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

thanks for the qa :)

(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?

Pushed to master for 20.11, thanks to everybody involved!

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)

Yes! Thank you Jonathan! :)

(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.

patchset does not apply clean to 20.05.x, no backport