Description
Jonathan Druart
2018-01-09 14:26:42 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). 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. 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 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 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 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 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 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 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 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. 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/ 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 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 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 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 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 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. 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/ 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> Tested all affected parts, that I did not even know existed. puh. Great work, Jonathan! 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 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 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 > 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...
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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> Created attachment 73568 [details] [review] Bug 19935: Use ->checkouts when ->pending_checkouts->count was used Created attachment 73569 [details] [review] Bug 19935: (follow-up) Replace GetPendingIssues - opac-user.pl Syntax issue, can be squashed on pushing. 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> Created attachment 73581 [details] [review] Bug 19935: Use ->checkouts when ->pending_checkouts->count was used Signed-off-by: Josef Moravec <josef.moravec@gmail.com> 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> Pushed to master for 18.05, thanks to everybody involved! Enhancement, not backporting to 17.11.x |