|
Description
Arthur Suzuki
2019-04-29 16:59:24 UTC
Created attachment 89039 [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 reply "itemAlreadyOnLoan" Veuillez saisir le message de validation pour vos modifications. Les lignes Hi Arthur, Sorry to report this patch doesn't seem to apply: Applying: Bug 22806 : CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions error: sha1 information is lacking or useless (C4/Reserves.pm). error: could not build fake ancestor Patch failed at 0001 Bug 22806 : CanBookBeReserved and CanItemBeReserved must check AllowHoldsOnPatronsPossessions Please rebase on master and reattach, thanks very much for your contribution! Liz Thanks Liz :) I'm submitting a new patch right away! Created attachment 89724 [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" Created attachment 91784 [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 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 Created attachment 94388 [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> Created attachment 94389 [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> Unit tests please Created attachment 97947 [details] [review] Bug 22806: Unit Tests Patch no longer applies. Created attachment 99881 [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> Created attachment 99882 [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> Created attachment 99883 [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> Created attachment 99884 [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> Created attachment 99885 [details] [review] Bug 22806: Unit Tests (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 Created attachment 100091 [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> Created attachment 100092 [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> Created attachment 100093 [details] [review] Bug 22806: Unit Tests 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" Created attachment 100173 [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 "itemAlreadyOnLoan" Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 100174 [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> Created attachment 100175 [details] [review] Bug 22806: Unit Tests Signed-off-by: David Nind <david@davidnind.com> 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 Created attachment 106564 [details] [review] Bug 22806: Unit Tests Signed-off-by: David Nind <david@davidnind.com> 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 |