Bug 19935 - Move C4::Members::GetPendingIssues to the Koha namespace
Summary: Move C4::Members::GetPendingIssues to the Koha namespace
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: Josef Moravec
URL:
Keywords:
Depends on: 19926 19933
Blocks: 16846 21486
  Show dependency treegraph
 
Reported: 2018-01-09 14:26 UTC by Jonathan Druart
Modified: 2019-10-14 19:58 UTC (History)
3 users (show)

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


Attachments
Bug 19935: Add Koha::Patron->pending_checkouts (4.62 KB, patch)
2018-01-09 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - IssueSlip (4.98 KB, patch)
2018-01-09 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - ILSDI (1.64 KB, patch)
2018-01-09 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - Discharges (1.42 KB, patch)
2018-01-09 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - deletemem.pl (1.46 KB, patch)
2018-01-09 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - summary-print (2.58 KB, patch)
2018-01-09 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - opac-ics.pl (1.47 KB, patch)
2018-01-09 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - opac-user.pl (4.23 KB, patch)
2018-01-09 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - sco-main (2.62 KB, patch)
2018-01-09 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - SIP (1.01 KB, patch)
2018-01-09 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - Remove the residue (9.54 KB, patch)
2018-01-09 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - deletemem.pl (1.34 KB, patch)
2018-02-16 14:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - summary-print (2.58 KB, patch)
2018-02-16 14:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - opac-ics.pl (1.47 KB, patch)
2018-02-16 14:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - opac-user.pl (4.23 KB, patch)
2018-02-16 14:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - sco-main (2.12 KB, patch)
2018-02-16 14:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - SIP (1.01 KB, patch)
2018-02-16 14:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - Remove the residue (9.54 KB, patch)
2018-02-16 14:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Add Koha::Patron->pending_checkouts (4.67 KB, patch)
2018-03-13 10:02 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - IssueSlip (5.03 KB, patch)
2018-03-13 10:02 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - ILSDI (1.69 KB, patch)
2018-03-13 10:02 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - Discharges (1.47 KB, patch)
2018-03-13 10:02 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - deletemem.pl (1.15 KB, patch)
2018-03-13 10:02 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - summary-print (2.63 KB, patch)
2018-03-13 10:02 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - opac-ics.pl (1.53 KB, patch)
2018-03-13 10:03 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - opac-user.pl (4.28 KB, patch)
2018-03-13 10:03 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - sco-main (2.17 KB, patch)
2018-03-13 10:03 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - SIP (1.06 KB, patch)
2018-03-13 10:03 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - Remove the residue (9.59 KB, patch)
2018-03-13 10:03 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 19935: Add Koha::Patron->pending_checkouts (4.69 KB, patch)
2018-04-03 12:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - IssueSlip (5.04 KB, patch)
2018-04-03 12:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - ILSDI (1.70 KB, patch)
2018-04-03 12:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - Discharges (1.39 KB, patch)
2018-04-03 12:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - deletemem.pl (1.16 KB, patch)
2018-04-03 12:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - summary-print (2.64 KB, patch)
2018-04-03 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - opac-ics.pl (1.54 KB, patch)
2018-04-03 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - opac-user.pl (4.29 KB, patch)
2018-04-03 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - sco-main (2.18 KB, patch)
2018-04-03 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - SIP (1.07 KB, patch)
2018-04-03 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - Remove the residue (9.56 KB, patch)
2018-04-03 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Use ->checkouts when ->pending_checkouts->count was used (1.33 KB, patch)
2018-04-03 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: (follow-up) Replace GetPendingIssues - opac-user.pl (1.09 KB, patch)
2018-04-03 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19935: Add Koha::Patron->pending_checkouts (4.73 KB, patch)
2018-04-03 13:13 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - IssueSlip (5.09 KB, patch)
2018-04-03 13:13 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - ILSDI (1.75 KB, patch)
2018-04-03 13:13 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - Discharges (1.44 KB, patch)
2018-04-03 13:13 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - deletemem.pl (1.20 KB, patch)
2018-04-03 13:13 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - summary-print (2.69 KB, patch)
2018-04-03 13:13 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - opac-ics.pl (1.58 KB, patch)
2018-04-03 13:13 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - opac-user.pl (4.33 KB, patch)
2018-04-03 13:13 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - sco-main (2.23 KB, patch)
2018-04-03 13:13 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - SIP (1.12 KB, patch)
2018-04-03 13:13 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19935: Replace GetPendingIssues - Remove the residue (9.61 KB, patch)
2018-04-03 13:13 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19935: Use ->checkouts when ->pending_checkouts->count was used (1.37 KB, patch)
2018-04-03 13:14 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19935: (follow-up) Replace GetPendingIssues - opac-user.pl (1.13 KB, patch)
2018-04-03 13:14 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2018-01-09 14:26:42 UTC

    
Comment 1 Jonathan Druart 2018-01-09 14:31:16 UTC
Created attachment 70351 [details] [review]
Bug 19935: Add Koha::Patron->pending_checkouts

To move this subroutine out of the C4 namespace we face the same
problematic as bug 17553 (with GetOverduesForPatron).
We need to provide an equivalent method and so return all the related
value for a given checkout.
We can acchieve the easily using Koha::Object->unblessed_all_relateds,
but we need to keep in mind that it is a temporary move.
Indeed we will want to use our API to only access/retrive values we really need.
The whole trick could be removed when the current syntax for notices
will be deprecated and removed.

Note: this method returns the same number of elements than ->checkouts
They indeed returns the same things, but it sounds better to me to have a
different method to highlight (from the callers) where does it come
from (C4::Members::GetPendingIssues).
Comment 2 Jonathan Druart 2018-01-09 14:31:19 UTC
Created attachment 70352 [details] [review]
Bug 19935: Replace GetPendingIssues - IssueSlip

Luckily we have a good test coverage here!

Test plan:
Print issue slips before and after these patches (with overdues, etc.)
They should be the same.
Comment 3 Jonathan Druart 2018-01-09 14:31:23 UTC
Created attachment 70353 [details] [review]
Bug 19935: Replace GetPendingIssues - ILSDI

Here we should only access to what we want in the template, but let do
it as it for now.

Test plan:
Hit
/cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=542&show_contact=0&show_loans=1

With 42 a borrowernumber with checkouts

Before and after these patches the XML must be the same
Comment 4 Jonathan Druart 2018-01-09 14:31:26 UTC
Created attachment 70354 [details] [review]
Bug 19935: Replace GetPendingIssues - Discharges

We should actually use Koha::Patron->checkouts here to avoid the
prefetch.

Test plan:
A patron with checkouts cannot get a discharge
Comment 5 Jonathan Druart 2018-01-09 14:31:29 UTC
Created attachment 70355 [details] [review]
Bug 19935: Replace GetPendingIssues - deletemem.pl

Same as previously, we just want Koha::Patron->checkouts->count to know
if a patron has checkouts.

Test plan:
Confirm that you cannot delete a patron's card if they have pending checkouts
Comment 6 Jonathan Druart 2018-01-09 14:31:32 UTC
Created attachment 70356 [details] [review]
Bug 19935: Replace GetPendingIssues - summary-print

We are in the notices part, so we need to fetch all the data to avoid
regressions.

Test plan:
Print a summary slip before and after this patch.
They must be the same
Comment 7 Jonathan Druart 2018-01-09 14:31:35 UTC
Created attachment 70357 [details] [review]
Bug 19935: Replace GetPendingIssues - opac-ics.pl

We only need the biblio title and the barcode, we should not need the
whole prefetch.

Test plan:
On your OPAC summary page export your checkout list using the
"Download as iCal/.ics file" link.
Before and after the patchset, the generated files must be the same
Comment 8 Jonathan Druart 2018-01-09 14:31:38 UTC
Created attachment 70358 [details] [review]
Bug 19935: Replace GetPendingIssues - opac-user.pl

At first glance we just need the biblio title and the subtitle (in
addition of the fines info), we should not need the prefetch.

Test plan:
Loggin at the OPAC, on the summary page you should see your checkouts
and overdues with the correct values
Comment 9 Jonathan Druart 2018-01-09 14:31:41 UTC
Created attachment 70359 [details] [review]
Bug 19935: Replace GetPendingIssues - sco-main

Same as previously, we do not need all the prefetched values here, just
a few.

Test plan:
Use the self checkout module to check some items out
Comment 10 Jonathan Druart 2018-01-09 14:31:45 UTC
Created attachment 70360 [details] [review]
Bug 19935: Replace GetPendingIssues - SIP

Sounds like we do not need related fields or 'overdue' flag here.
No idea how to confirm there is no regression here.
Comment 11 Jonathan Druart 2018-01-09 14:31:48 UTC
Created attachment 70361 [details] [review]
Bug 19935: Replace GetPendingIssues - Remove the residue

Here we are, no more occurrences of GetPendingIssues, we can remove the
tests and subroutine \o/
Comment 12 Jonathan Druart 2018-02-16 14:46:29 UTC
Created attachment 71807 [details] [review]
Bug 19935: Replace GetPendingIssues - deletemem.pl

Same as previously, we just want Koha::Patron->checkouts->count to know
if a patron has checkouts.

Test plan:
Confirm that you cannot delete a patron's card if they have pending checkouts
Comment 13 Jonathan Druart 2018-02-16 14:46:32 UTC
Created attachment 71808 [details] [review]
Bug 19935: Replace GetPendingIssues - summary-print

We are in the notices part, so we need to fetch all the data to avoid
regressions.

Test plan:
Print a summary slip before and after this patch.
They must be the same
Comment 14 Jonathan Druart 2018-02-16 14:46:35 UTC
Created attachment 71809 [details] [review]
Bug 19935: Replace GetPendingIssues - opac-ics.pl

We only need the biblio title and the barcode, we should not need the
whole prefetch.

Test plan:
On your OPAC summary page export your checkout list using the
"Download as iCal/.ics file" link.
Before and after the patchset, the generated files must be the same
Comment 15 Jonathan Druart 2018-02-16 14:46:38 UTC
Created attachment 71810 [details] [review]
Bug 19935: Replace GetPendingIssues - opac-user.pl

At first glance we just need the biblio title and the subtitle (in
addition of the fines info), we should not need the prefetch.

Test plan:
Loggin at the OPAC, on the summary page you should see your checkouts
and overdues with the correct values
Comment 16 Jonathan Druart 2018-02-16 14:46:42 UTC
Created attachment 71811 [details] [review]
Bug 19935: Replace GetPendingIssues - sco-main

Same as previously, we do not need all the prefetched values here, just
a few.

Test plan:
Use the self checkout module to check some items out
Comment 17 Jonathan Druart 2018-02-16 14:46:46 UTC
Created attachment 71812 [details] [review]
Bug 19935: Replace GetPendingIssues - SIP

Sounds like we do not need related fields or 'overdue' flag here.
No idea how to confirm there is no regression here.
Comment 18 Jonathan Druart 2018-02-16 14:46:51 UTC
Created attachment 71813 [details] [review]
Bug 19935: Replace GetPendingIssues - Remove the residue

Here we are, no more occurrences of GetPendingIssues, we can remove the
tests and subroutine \o/
Comment 19 Benjamin Rokseth 2018-03-13 10:02:29 UTC
Created attachment 72752 [details] [review]
Bug 19935: Add Koha::Patron->pending_checkouts

To move this subroutine out of the C4 namespace we face the same
problematic as bug 17553 (with GetOverduesForPatron).
We need to provide an equivalent method and so return all the related
value for a given checkout.
We can acchieve the easily using Koha::Object->unblessed_all_relateds,
but we need to keep in mind that it is a temporary move.
Indeed we will want to use our API to only access/retrive values we really need.
The whole trick could be removed when the current syntax for notices
will be deprecated and removed.

Note: this method returns the same number of elements than ->checkouts
They indeed returns the same things, but it sounds better to me to have a
different method to highlight (from the callers) where does it come
from (C4::Members::GetPendingIssues).

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 20 Benjamin Rokseth 2018-03-13 10:02:35 UTC
Created attachment 72753 [details] [review]
Bug 19935: Replace GetPendingIssues - IssueSlip

Luckily we have a good test coverage here!

Test plan:
Print issue slips before and after these patches (with overdues, etc.)
They should be the same.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 21 Benjamin Rokseth 2018-03-13 10:02:40 UTC
Created attachment 72754 [details] [review]
Bug 19935: Replace GetPendingIssues - ILSDI

Here we should only access to what we want in the template, but let do
it as it for now.

Test plan:
Hit
/cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=542&show_contact=0&show_loans=1

With 42 a borrowernumber with checkouts

Before and after these patches the XML must be the same

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 22 Benjamin Rokseth 2018-03-13 10:02:47 UTC
Created attachment 72755 [details] [review]
Bug 19935: Replace GetPendingIssues - Discharges

We should actually use Koha::Patron->checkouts here to avoid the
prefetch.

Test plan:
A patron with checkouts cannot get a discharge

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 23 Benjamin Rokseth 2018-03-13 10:02:52 UTC
Created attachment 72756 [details] [review]
Bug 19935: Replace GetPendingIssues - deletemem.pl

Same as previously, we just want Koha::Patron->checkouts->count to know
if a patron has checkouts.

Test plan:
Confirm that you cannot delete a patron's card if they have pending checkouts

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 24 Benjamin Rokseth 2018-03-13 10:02:57 UTC
Created attachment 72757 [details] [review]
Bug 19935: Replace GetPendingIssues - summary-print

We are in the notices part, so we need to fetch all the data to avoid
regressions.

Test plan:
Print a summary slip before and after this patch.
They must be the same

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 25 Benjamin Rokseth 2018-03-13 10:03:03 UTC
Created attachment 72758 [details] [review]
Bug 19935: Replace GetPendingIssues - opac-ics.pl

We only need the biblio title and the barcode, we should not need the
whole prefetch.

Test plan:
On your OPAC summary page export your checkout list using the
"Download as iCal/.ics file" link.
Before and after the patchset, the generated files must be the same

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 26 Benjamin Rokseth 2018-03-13 10:03:08 UTC
Created attachment 72759 [details] [review]
Bug 19935: Replace GetPendingIssues - opac-user.pl

At first glance we just need the biblio title and the subtitle (in
addition of the fines info), we should not need the prefetch.

Test plan:
Loggin at the OPAC, on the summary page you should see your checkouts
and overdues with the correct values

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 27 Benjamin Rokseth 2018-03-13 10:03:13 UTC
Created attachment 72760 [details] [review]
Bug 19935: Replace GetPendingIssues - sco-main

Same as previously, we do not need all the prefetched values here, just
a few.

Test plan:
Use the self checkout module to check some items out

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 28 Benjamin Rokseth 2018-03-13 10:03:19 UTC
Created attachment 72761 [details] [review]
Bug 19935: Replace GetPendingIssues - SIP

Sounds like we do not need related fields or 'overdue' flag here.
No idea how to confirm there is no regression here.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 29 Benjamin Rokseth 2018-03-13 10:03:26 UTC
Created attachment 72762 [details] [review]
Bug 19935: Replace GetPendingIssues - Remove the residue

Here we are, no more occurrences of GetPendingIssues, we can remove the
tests and subroutine \o/

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 30 Benjamin Rokseth 2018-03-13 10:05:05 UTC
Tested all affected parts, that I did not even know existed. puh.

Great work, Jonathan!
Comment 31 Josef Moravec 2018-04-03 11:18:28 UTC
Comment on attachment 72755 [details] [review]
Bug 19935: Replace GetPendingIssues - Discharges

Review of attachment 72755 [details] [review]:
-----------------------------------------------------------------

::: Koha/Patron/Discharge.pm
@@ +9,4 @@
>  
>  use Koha::Database;
>  use Koha::DateUtils qw( dt_from_string output_pref );
> +use Koha::Checkouts;

should remain Koha::Patrons

@@ +35,5 @@
>  
> +    my $patron = Koha::Patrons->find( $params->{borrowernumber} );
> +    return unless $patron;
> +
> +    my $has_pending_checkouts = $patron->pending_checkouts->count;

shouldn't it be this? As said in commit message?
$patron->checkouts->count;
Comment 32 Josef Moravec 2018-04-03 11:19:09 UTC
Comment on attachment 72756 [details] [review]
Bug 19935: Replace GetPendingIssues - deletemem.pl

Review of attachment 72756 [details] [review]:
-----------------------------------------------------------------

::: members/deletemem.pl
@@ +77,2 @@
>  my $charges = $patron->account->non_issues_charges;
> +my $countissues = $patron->pending_checkouts->count;

$patron->checkouts->count; should be enough as said in commit message
Comment 33 Josef Moravec 2018-04-03 11:34:40 UTC
Comment on attachment 72759 [details] [review]
Bug 19935: Replace GetPendingIssues - opac-user.pl

Review of attachment 72759 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-user.pl
@@ +180,4 @@
>  my @overdues;
>  my @issuedat;
>  my $itemtypes = { map { $_->{itemtype} => $_ } @{ Koha::ItemTypes->search_with_localization->unblessed } };
> +my $pending_checkouts = $patron->pending_checkouts({}, { order_by => [ { -desc => 'date_due' }, { -asc => 'issue_id' } ] });

I think it should be $patron->pending_checkouts->search
Comment 34 Josef Moravec 2018-04-03 12:00:02 UTC
> Created attachment 72761 [details] [review] [review]
> Bug 19935: Replace GetPendingIssues - SIP
> 
> Sounds like we do not need related fields or 'overdue' flag here.
> No idea how to confirm there is no regression here.
> 

Reading through the code, it even looks like we donẗ need more then barcode...
Comment 35 Jonathan Druart 2018-04-03 12:54:42 UTC
Created attachment 73557 [details] [review]
Bug 19935: Add Koha::Patron->pending_checkouts

To move this subroutine out of the C4 namespace we face the same
problematic as bug 17553 (with GetOverduesForPatron).
We need to provide an equivalent method and so return all the related
value for a given checkout.
We can acchieve the easily using Koha::Object->unblessed_all_relateds,
but we need to keep in mind that it is a temporary move.
Indeed we will want to use our API to only access/retrive values we really need.
The whole trick could be removed when the current syntax for notices
will be deprecated and removed.

Note: this method returns the same number of elements than ->checkouts
They indeed returns the same things, but it sounds better to me to have a
different method to highlight (from the callers) where does it come
from (C4::Members::GetPendingIssues).

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 36 Jonathan Druart 2018-04-03 12:54:46 UTC
Created attachment 73558 [details] [review]
Bug 19935: Replace GetPendingIssues - IssueSlip

Luckily we have a good test coverage here!

Test plan:
Print issue slips before and after these patches (with overdues, etc.)
They should be the same.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 37 Jonathan Druart 2018-04-03 12:54:50 UTC
Created attachment 73559 [details] [review]
Bug 19935: Replace GetPendingIssues - ILSDI

Here we should only access to what we want in the template, but let do
it as it for now.

Test plan:
Hit
/cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=542&show_contact=0&show_loans=1

With 42 a borrowernumber with checkouts

Before and after these patches the XML must be the same

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 38 Jonathan Druart 2018-04-03 12:54:54 UTC
Created attachment 73560 [details] [review]
Bug 19935: Replace GetPendingIssues - Discharges

We should actually use Koha::Patron->checkouts here to avoid the
prefetch.

Test plan:
A patron with checkouts cannot get a discharge

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 39 Jonathan Druart 2018-04-03 12:54:57 UTC
Created attachment 73561 [details] [review]
Bug 19935: Replace GetPendingIssues - deletemem.pl

Same as previously, we just want Koha::Patron->checkouts->count to know
if a patron has checkouts.

Test plan:
Confirm that you cannot delete a patron's card if they have pending checkouts

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 40 Jonathan Druart 2018-04-03 12:55:01 UTC
Created attachment 73562 [details] [review]
Bug 19935: Replace GetPendingIssues - summary-print

We are in the notices part, so we need to fetch all the data to avoid
regressions.

Test plan:
Print a summary slip before and after this patch.
They must be the same

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 41 Jonathan Druart 2018-04-03 12:55:05 UTC
Created attachment 73563 [details] [review]
Bug 19935: Replace GetPendingIssues - opac-ics.pl

We only need the biblio title and the barcode, we should not need the
whole prefetch.

Test plan:
On your OPAC summary page export your checkout list using the
"Download as iCal/.ics file" link.
Before and after the patchset, the generated files must be the same

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 42 Jonathan Druart 2018-04-03 12:55:09 UTC
Created attachment 73564 [details] [review]
Bug 19935: Replace GetPendingIssues - opac-user.pl

At first glance we just need the biblio title and the subtitle (in
addition of the fines info), we should not need the prefetch.

Test plan:
Loggin at the OPAC, on the summary page you should see your checkouts
and overdues with the correct values

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 43 Jonathan Druart 2018-04-03 12:55:12 UTC
Created attachment 73565 [details] [review]
Bug 19935: Replace GetPendingIssues - sco-main

Same as previously, we do not need all the prefetched values here, just
a few.

Test plan:
Use the self checkout module to check some items out

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 44 Jonathan Druart 2018-04-03 12:55:16 UTC
Created attachment 73566 [details] [review]
Bug 19935: Replace GetPendingIssues - SIP

Sounds like we do not need related fields or 'overdue' flag here.
No idea how to confirm there is no regression here.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 45 Jonathan Druart 2018-04-03 12:55:21 UTC
Created attachment 73567 [details] [review]
Bug 19935: Replace GetPendingIssues - Remove the residue

Here we are, no more occurrences of GetPendingIssues, we can remove the
tests and subroutine \o/

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 46 Jonathan Druart 2018-04-03 12:55:25 UTC
Created attachment 73568 [details] [review]
Bug 19935: Use ->checkouts when ->pending_checkouts->count was used
Comment 47 Jonathan Druart 2018-04-03 12:55:29 UTC
Created attachment 73569 [details] [review]
Bug 19935: (follow-up) Replace GetPendingIssues - opac-user.pl

Syntax issue, can be squashed on pushing.
Comment 48 Josef Moravec 2018-04-03 13:13:18 UTC
Created attachment 73570 [details] [review]
Bug 19935: Add Koha::Patron->pending_checkouts

To move this subroutine out of the C4 namespace we face the same
problematic as bug 17553 (with GetOverduesForPatron).
We need to provide an equivalent method and so return all the related
value for a given checkout.
We can acchieve the easily using Koha::Object->unblessed_all_relateds,
but we need to keep in mind that it is a temporary move.
Indeed we will want to use our API to only access/retrive values we really need.
The whole trick could be removed when the current syntax for notices
will be deprecated and removed.

Note: this method returns the same number of elements than ->checkouts
They indeed returns the same things, but it sounds better to me to have a
different method to highlight (from the callers) where does it come
from (C4::Members::GetPendingIssues).

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 49 Josef Moravec 2018-04-03 13:13:22 UTC
Created attachment 73571 [details] [review]
Bug 19935: Replace GetPendingIssues - IssueSlip

Luckily we have a good test coverage here!

Test plan:
Print issue slips before and after these patches (with overdues, etc.)
They should be the same.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 50 Josef Moravec 2018-04-03 13:13:25 UTC
Created attachment 73572 [details] [review]
Bug 19935: Replace GetPendingIssues - ILSDI

Here we should only access to what we want in the template, but let do
it as it for now.

Test plan:
Hit
/cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=542&show_contact=0&show_loans=1

With 42 a borrowernumber with checkouts

Before and after these patches the XML must be the same

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 51 Josef Moravec 2018-04-03 13:13:29 UTC
Created attachment 73573 [details] [review]
Bug 19935: Replace GetPendingIssues - Discharges

We should actually use Koha::Patron->checkouts here to avoid the
prefetch.

Test plan:
A patron with checkouts cannot get a discharge

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 52 Josef Moravec 2018-04-03 13:13:32 UTC
Created attachment 73574 [details] [review]
Bug 19935: Replace GetPendingIssues - deletemem.pl

Same as previously, we just want Koha::Patron->checkouts->count to know
if a patron has checkouts.

Test plan:
Confirm that you cannot delete a patron's card if they have pending checkouts

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 53 Josef Moravec 2018-04-03 13:13:36 UTC
Created attachment 73575 [details] [review]
Bug 19935: Replace GetPendingIssues - summary-print

We are in the notices part, so we need to fetch all the data to avoid
regressions.

Test plan:
Print a summary slip before and after this patch.
They must be the same

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 54 Josef Moravec 2018-04-03 13:13:40 UTC
Created attachment 73576 [details] [review]
Bug 19935: Replace GetPendingIssues - opac-ics.pl

We only need the biblio title and the barcode, we should not need the
whole prefetch.

Test plan:
On your OPAC summary page export your checkout list using the
"Download as iCal/.ics file" link.
Before and after the patchset, the generated files must be the same

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 55 Josef Moravec 2018-04-03 13:13:44 UTC
Created attachment 73577 [details] [review]
Bug 19935: Replace GetPendingIssues - opac-user.pl

At first glance we just need the biblio title and the subtitle (in
addition of the fines info), we should not need the prefetch.

Test plan:
Loggin at the OPAC, on the summary page you should see your checkouts
and overdues with the correct values

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 56 Josef Moravec 2018-04-03 13:13:49 UTC
Created attachment 73578 [details] [review]
Bug 19935: Replace GetPendingIssues - sco-main

Same as previously, we do not need all the prefetched values here, just
a few.

Test plan:
Use the self checkout module to check some items out

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 57 Josef Moravec 2018-04-03 13:13:53 UTC
Created attachment 73579 [details] [review]
Bug 19935: Replace GetPendingIssues - SIP

Sounds like we do not need related fields or 'overdue' flag here.
No idea how to confirm there is no regression here.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 58 Josef Moravec 2018-04-03 13:13:56 UTC
Created attachment 73580 [details] [review]
Bug 19935: Replace GetPendingIssues - Remove the residue

Here we are, no more occurrences of GetPendingIssues, we can remove the
tests and subroutine \o/

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 59 Josef Moravec 2018-04-03 13:14:00 UTC
Created attachment 73581 [details] [review]
Bug 19935: Use ->checkouts when ->pending_checkouts->count was used

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 60 Josef Moravec 2018-04-03 13:14:03 UTC
Created attachment 73582 [details] [review]
Bug 19935: (follow-up) Replace GetPendingIssues - opac-user.pl

Syntax issue, can be squashed on pushing.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 61 Jonathan Druart 2018-04-03 14:49:35 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 62 Nick Clemens 2018-05-14 14:37:01 UTC
Enhancement, not backporting to 17.11.x