Bug 19302

Summary: Pass objects to IsAvailableForItemLevelRequest
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: alexbuckley, hayleypelham, m.de.rooy, martin.renvoize, nick
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Part of the ongoing effort to improve the maintainability of our codebase.
Version(s) released in:
19.05.00
Bug Depends on: 19300, 19301    
Bug Blocks:    
Attachments: Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest
Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest
Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest
Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest
Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest
Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest
Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest

Description Jonathan Druart 2017-09-12 21:39:00 UTC
Pass Koha::Item and Koha::Patron to C4::Reserves::IsAvailableForItemLevelRequest
Comment 1 Jonathan Druart 2017-09-13 19:25:02 UTC
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 2 Josef Moravec 2017-10-29 21:58:10 UTC
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 ?
Comment 3 Jonathan Druart 2017-10-30 13:51:28 UTC
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
Comment 4 Jonathan Druart 2017-10-30 13:51:52 UTC
(In reply to Josef Moravec from comment #2)
> > +                    next unless CanItemBeReserved($b,$itemnumber);
> 
> Shouldn't $b be $borrowernumber ?

Yes, thanks!
Comment 5 Alex Buckley 2018-10-29 22:56:06 UTC
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
Comment 6 Jonathan Druart 2019-03-05 20:31:33 UTC
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
Comment 7 Hayley Pelham 2019-04-16 22:54:13 UTC
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
Comment 8 Jonathan Druart 2019-04-24 21:36:32 UTC
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
Comment 9 Hayley Pelham 2019-04-26 02:07:14 UTC
(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
Comment 10 Jonathan Druart 2019-04-27 19:03:43 UTC
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
Comment 11 Jonathan Druart 2019-04-27 19:04:30 UTC
Conflict with bug 20837 fixed!
Comment 12 Hayley Pelham 2019-05-02 01:50:43 UTC
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>
Comment 13 Marcel de Rooy 2019-05-03 09:32:52 UTC
prove t/db_dependent/Reserves.t fails with me
Comment 14 Jonathan Druart 2019-05-03 13:01:24 UTC
(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?
Comment 15 Marcel de Rooy 2019-05-06 05:33:56 UTC
#   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.
Comment 16 Marcel de Rooy 2019-05-06 05:34:47 UTC
(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..
Comment 17 Marcel de Rooy 2019-05-06 05:36:19 UTC
(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..
Comment 18 Marcel de Rooy 2019-05-06 06:23:38 UTC
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>
Comment 19 Nick Clemens 2019-05-10 19:14:26 UTC
Awesome work all!

Pushed to master for 19.05
Comment 20 Martin Renvoize 2019-05-24 13:26:28 UTC
Enhancement will not be backported to 18.11.x series at this time.