Pass Koha::Item and Koha::Patron to C4::Reserves::IsAvailableForItemLevelRequest
Created attachment 67124 [details] [review] Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest Almost everywhere we call IsAvailableForItemLevelRequest we already have a Koha::Patron and Koha::Item object. It makes sense to use them to avoid a refetch Test plan: It would be good to test this patch on top of 19300 and 19301 and make sure everything works as expected
Comment on attachment 67124 [details] [review] Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest Review of attachment 67124 [details] [review]: ----------------------------------------------------------------- ::: C4/Circulation.pm @@ +2665,5 @@ > + next if IsItemOnHoldAndFound( $itemnumber ); > + for my $borrowernumber (@borrowernumbers) { > + my $patron = $patrons{$borrowernumber} //= Koha::Patrons->find( $borrowernumber ); > + next unless IsAvailableForItemLevelRequest($item, $patron); > + next unless CanItemBeReserved($b,$itemnumber); Shouldn't $b be $borrowernumber ?
Created attachment 68845 [details] [review] Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest Almost everywhere we call IsAvailableForItemLevelRequest we already have a Koha::Patron and Koha::Item object. It makes sense to use them to avoid a refetch Test plan: It would be good to test this patch on top of 19300 and 19301 and make sure everything works as expected
(In reply to Josef Moravec from comment #2) > > + next unless CanItemBeReserved($b,$itemnumber); > > Shouldn't $b be $borrowernumber ? Yes, thanks!
This still looks to be applicable for master as the item objects are still not being handed to C4::Reserves::IsAvailableForItemLevelRequest(). I tried applying it to master, and the patch doesn't apply cleanly (as the below output shows), can you please rebase? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest fatal: sha1 information is lacking or useless (C4/Reserves.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest The copy of the patch that failed is found in: /home/vagrant/kohaclone/.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-19302-Send-kohaobjects-to-C4ReservesIsAvailabl-wrc9H7.patch
Created attachment 86119 [details] [review] Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest Almost everywhere we call IsAvailableForItemLevelRequest we already have a Koha::Patron and Koha::Item object. It makes sense to use them to avoid a refetch Test plan: It would be good to test this patch on top of 19300 and 19301 and make sure everything works as expected
Patch doesn't apply - merge conflicts, can you take a look? Applying: Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest Using index info to reconstruct a base tree... M C4/Circulation.pm M C4/ILSDI/Services.pm M C4/Reserves.pm M opac/opac-reserve.pl M reserve/request.pl M t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t M t/db_dependent/Reserves.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Reserves.t Auto-merging t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t Auto-merging reserve/request.pl Auto-merging opac/opac-reserve.pl Auto-merging C4/Reserves.pm CONFLICT (content): Merge conflict in C4/Reserves.pm Auto-merging C4/ILSDI/Services.pm Auto-merging C4/Circulation.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest
Created attachment 88660 [details] [review] Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest Almost everywhere we call IsAvailableForItemLevelRequest we already have a Koha::Patron and Koha::Item object. It makes sense to use them to avoid a refetch Test plan: It would be good to test this patch on top of 19300 and 19301 and make sure everything works as expected
(In reply to Jonathan Druart from comment #8) > Created attachment 88660 [details] [review] [review] > Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest > > Almost everywhere we call IsAvailableForItemLevelRequest we already have > a Koha::Patron and Koha::Item object. It makes sense to use them to > avoid a refetch > > Test plan: > It would be good to test this patch on top of 19300 and 19301 and make > sure everything works as expected Sorry Jonathan, still has merge conflicts Applying: Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest Using index info to reconstruct a base tree... M C4/Circulation.pm M C4/Reserves.pm M t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t CONFLICT (content): Merge conflict in t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t Auto-merging C4/Reserves.pm CONFLICT (content): Merge conflict in C4/Reserves.pm Auto-merging C4/Circulation.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest
Created attachment 88987 [details] [review] Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest Almost everywhere we call IsAvailableForItemLevelRequest we already have a Koha::Patron and Koha::Item object. It makes sense to use them to avoid a refetch Test plan: It would be good to test this patch on top of 19300 and 19301 and make sure everything works as expected
Conflict with bug 20837 fixed!
Created attachment 89197 [details] [review] Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest Almost everywhere we call IsAvailableForItemLevelRequest we already have a Koha::Patron and Koha::Item object. It makes sense to use them to avoid a refetch Test plan: It would be good to test this patch on top of 19300 and 19301 and make sure everything works as expected Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
prove t/db_dependent/Reserves.t fails with me
(In reply to Marcel de Rooy from comment #13) > prove t/db_dependent/Reserves.t fails with me It passes for me, which failure do you get? Is it passing on master?
# Failed test 'patron notified when item set to waiting' # at t/db_dependent/Reserves.t line 358. # got: '0' # expected: '1' # Failed test 'patron not notified a second time (bug 11445)' # at t/db_dependent/Reserves.t line 363. # got: '0' # expected: '1' # Failed test 'can successfully generate hold slip (bug 10949)' # at t/db_dependent/Reserves.t line 374.
(In reply to Jonathan Druart from comment #14) > (In reply to Marcel de Rooy from comment #13) > > prove t/db_dependent/Reserves.t fails with me > > It passes for me, which failure do you get? Is it passing on master? No, without these patches it fails too..
(In reply to Marcel de Rooy from comment #16) > (In reply to Jonathan Druart from comment #14) > > (In reply to Marcel de Rooy from comment #13) > > > prove t/db_dependent/Reserves.t fails with me > > > > It passes for me, which failure do you get? Is it passing on master? > > No, without these patches it fails too.. Will refresh my data and try again..
Created attachment 89369 [details] [review] Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest Almost everywhere we call IsAvailableForItemLevelRequest we already have a Koha::Patron and Koha::Item object. It makes sense to use them to avoid a refetch Test plan: It would be good to test this patch on top of 19300 and 19301 and make sure everything works as expected Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Awesome work all! Pushed to master for 19.05
Enhancement will not be backported to 18.11.x series at this time.