Bug 19280 - CanBookBeIssued must take a Koha::Patron in parameter
Summary: CanBookBeIssued must take a Koha::Patron in parameter
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 19276
Blocks: 20251
  Show dependency treegraph
 
Reported: 2017-09-08 15:53 UTC by Jonathan Druart
Modified: 2018-12-03 20:04 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 19280: Pass a Koha::Patron to CanBookBeIssued (27.66 KB, patch)
2017-09-08 15:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19280: Pass a Koha::Patron to CanBookBeIssued (29.79 KB, patch)
2017-10-25 13:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19280: Pass a Koha::Patron to CanBookBeIssued (29.77 KB, patch)
2017-10-25 14:18 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 19280: Pass a Koha::Patron to CanBookBeIssued (30.13 KB, patch)
2017-11-03 15:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19280: refresh unblessed patron too (857 bytes, patch)
2018-01-05 14:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19280: Pass a Koha::Patron to CanBookBeIssued (30.43 KB, patch)
2018-01-05 15:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19280: refresh unblessed patron too (947 bytes, patch)
2018-01-05 15:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-09-08 15:53:14 UTC
We need to make subroutine from C4 use more Koha::Object objects
Seeing bug 19276, starting here is a good start.
Comment 1 Jonathan Druart 2017-09-08 15:54:40 UTC
Created attachment 66999 [details] [review]
Bug 19280: Pass a Koha::Patron to CanBookBeIssued

We need to make subroutine from C4 use more Koha::Object objects
Seeing bug 19276, starting here is a good start.

Test plan:
The tests should still pass.
Comment 2 Jon Knight 2017-10-24 17:38:08 UTC
Patch fails to apply on my Kohadevbox VM and also on the PTFS-E sandbox:

vagrant@kohadevbox:kohaclone(master)$ git bz apply 19280
Bug 19280 - CanBookBeIssued must take a Koha::Patron in parameter

66999 - Bug 19280: Pass a Koha::Patron to CanBookBeIssued

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 19280: Pass a Koha::Patron to CanBookBeIssued
/home/vagrant/kohaclone/.git/rebase-apply/patch:24: trailing whitespace.
  ( $issuingimpossible, $needsconfirmation ) =  CanBookBeIssued( $patron, 
fatal: sha1 information is lacking or useless (C4/Circulation.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 19280: Pass a Koha::Patron to CanBookBeIssued
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-19280-Pass-a-KohaPatron-to-CanBookBeIssued-qZZNx2.patch
Comment 3 Jonathan Druart 2017-10-25 13:54:33 UTC
Created attachment 68533 [details] [review]
Bug 19280: Pass a Koha::Patron to CanBookBeIssued

We need to make subroutine from C4 use more Koha::Object objects
Seeing bug 19276, starting here is a good start.

Test plan:
The tests should still pass.
Comment 4 Jon Knight 2017-10-25 14:18:52 UTC
Created attachment 68534 [details] [review]
Bug 19280: Pass a Koha::Patron to CanBookBeIssued

We need to make subroutine from C4 use more Koha::Object objects
Seeing bug 19276, starting here is a good start.

Test plan:
The tests should still pass.

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 5 Jonathan Druart 2017-11-03 15:35:06 UTC
Created attachment 68916 [details] [review]
Bug 19280: Pass a Koha::Patron to CanBookBeIssued

We need to make subroutine from C4 use more Koha::Object objects
Seeing bug 19276, starting here is a good start.

Test plan:
The tests should still pass.

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 6 Marcel de Rooy 2018-01-05 06:45:30 UTC
-    $patron = Koha::Patrons->find( $borrower->{borrowernumber} );
+    $patron = Koha::Patrons->find( $patron->borrowernumber ); # FIXME Refetch just in case, to avoid regressions. But must not be needed

[...]

    my $toomany = TooMany( $patron_unblessed, $item->{biblionumber}, $item, { onsite_checkout => $onsite_checkout, switch_onsite_checkout => $switch_onsite_checkout, } );

You do not refetch $patron_unblessed. I would opt for not refetching $patron. But this is not consistent..
Comment 7 Jonathan Druart 2018-01-05 14:00:40 UTC
Created attachment 70287 [details] [review]
Bug 19280: refresh unblessed patron too
Comment 8 Jonathan Druart 2018-01-05 14:01:45 UTC
(In reply to Marcel de Rooy from comment #6)
> -    $patron = Koha::Patrons->find( $borrower->{borrowernumber} );
> +    $patron = Koha::Patrons->find( $patron->borrowernumber ); # FIXME
> Refetch just in case, to avoid regressions. But must not be needed
> 
> [...]
> 
>     my $toomany = TooMany( $patron_unblessed, $item->{biblionumber}, $item,
> { onsite_checkout => $onsite_checkout, switch_onsite_checkout =>
> $switch_onsite_checkout, } );
> 
> You do not refetch $patron_unblessed. I would opt for not refetching
> $patron. But this is not consistent..

Good catch Marcel. I also think it's not needed, that's why I added the FIXME. But I would prefer not to do it on this bug report as it will need more attention.
Comment 9 Marcel de Rooy 2018-01-05 15:09:46 UTC
Created attachment 70291 [details] [review]
Bug 19280: Pass a Koha::Patron to CanBookBeIssued

We need to make subroutine from C4 use more Koha::Object objects
Seeing bug 19276, starting here is a good start.

Test plan:
The tests should still pass.

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2018-01-05 15:09:51 UTC
Created attachment 70292 [details] [review]
Bug 19280: refresh unblessed patron too

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Jonathan Druart 2018-01-09 20:27:57 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 12 Nick Clemens 2018-01-16 12:24:20 UTC
Enhancement, skipping for 17.11.x.
Awesome work everybody!