C4::Members::patronflags does a lot of things and most of the time they are not even used by the callers. We should split this subroutine and fetch only the data we need.
Created attachment 70347 [details] [review] Bug 19933: Remove patronflags - unused vars oldamount/amountold in circulation.pl There is oldamound/amountold variable that is sent to the template but never used later. Let remove it now to avoid useless replacement.
Created attachment 70348 [details] [review] Bug 19933: Remove patronflags - In CanBookBeIssued The change is trivial here, we only want to pass gonenoaddress, lost and is_debared to the template.
Created attachment 70349 [details] [review] Bug 19933: Remove patronflags - easy ones This patch replaces the easy occurrences of patronflags. These calls only need the CHARGES->amount value, that is the non issues charges. Luckily we now have a Koha::Account->non_issues_charges that deal with that.
Created attachment 70350 [details] [review] Bug 19933: Remove patronflags - tricky ones Here we are, patronflags is used in a couple of places where (almost) all flags were really useful: C4::SIP::ILS::Patron->new and circulation.pl This patch only deal with the circulation code as I am not convident enough with SIP code. The change does not seems trivial because of the complexity of the existing code, but the logic is the same. We send a variable to the template depending on the situation of the patron. I guess only code eyes ball could catch issue in this patch quickly. Maybe we need to find a good place in a Koha module to move this code and provide code coverage (especially when C4::SIP::ILS::Patron will reuse it).
Comment on attachment 70348 [details] [review] Bug 19933: Remove patronflags - In CanBookBeIssued Review of attachment 70348 [details] [review]: ----------------------------------------------------------------- Nice.
Created attachment 71803 [details] [review] Bug 19933: Remove patronflags - unused vars oldamount/amountold in circulation.pl There is oldamound/amountold variable that is sent to the template but never used later. Let remove it now to avoid useless replacement.
Created attachment 71804 [details] [review] Bug 19933: Remove patronflags - In CanBookBeIssued The change is trivial here, we only want to pass gonenoaddress, lost and is_debared to the template.
Created attachment 71805 [details] [review] Bug 19933: Remove patronflags - easy ones This patch replaces the easy occurrences of patronflags. These calls only need the CHARGES->amount value, that is the non issues charges. Luckily we now have a Koha::Account->non_issues_charges that deal with that.
Created attachment 71806 [details] [review] Bug 19933: Remove patronflags - tricky ones Here we are, patronflags is used in a couple of places where (almost) all flags were really useful: C4::SIP::ILS::Patron->new and circulation.pl This patch only deal with the circulation code as I am not convident enough with SIP code. The change does not seems trivial because of the complexity of the existing code, but the logic is the same. We send a variable to the template depending on the situation of the patron. I guess only code eyes ball could catch issue in this patch quickly. Maybe we need to find a good place in a Koha module to move this code and provide code coverage (especially when C4::SIP::ILS::Patron will reuse it).
Created attachment 72004 [details] [review] Bug 19933: Remove patronflags - unused vars oldamount/amountold in circulation.pl There is oldamound/amountold variable that is sent to the template but never used later. Let remove it now to avoid useless replacement.
Created attachment 72005 [details] [review] Bug 19933: Remove patronflags - In CanBookBeIssued The change is trivial here, we only want to pass gonenoaddress, lost and is_debared to the template.
Created attachment 72006 [details] [review] Bug 19933: Remove patronflags - easy ones This patch replaces the easy occurrences of patronflags. These calls only need the CHARGES->amount value, that is the non issues charges. Luckily we now have a Koha::Account->non_issues_charges that deal with that.
Created attachment 72007 [details] [review] Bug 19933: Remove patronflags - tricky ones Here we are, patronflags is used in a couple of places where (almost) all flags were really useful: C4::SIP::ILS::Patron->new and circulation.pl This patch only deal with the circulation code as I am not convident enough with SIP code. The change does not seems trivial because of the complexity of the existing code, but the logic is the same. We send a variable to the template depending on the situation of the patron. I guess only code eyes ball could catch issue in this patch quickly. Maybe we need to find a good place in a Koha module to move this code and provide code coverage (especially when C4::SIP::ILS::Patron will reuse it).
Created attachment 72094 [details] [review] Bug 19933: (follow-up) Remove patronflags - easy ones
Created attachment 72095 [details] [review] Bug 19933: (follow-up) Remove patronflags - easy ones
Created attachment 72350 [details] [review] Bug 19933: Remove patronflags - unused vars oldamount/amountold in circulation.pl There is oldamound/amountold variable that is sent to the template but never used later. Let remove it now to avoid useless replacement. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 72351 [details] [review] Bug 19933: Remove patronflags - In CanBookBeIssued The change is trivial here, we only want to pass gonenoaddress, lost and is_debared to the template. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 72352 [details] [review] Bug 19933: Remove patronflags - easy ones This patch replaces the easy occurrences of patronflags. These calls only need the CHARGES->amount value, that is the non issues charges. Luckily we now have a Koha::Account->non_issues_charges that deal with that. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 72353 [details] [review] Bug 19933: Remove patronflags - tricky ones Here we are, patronflags is used in a couple of places where (almost) all flags were really useful: C4::SIP::ILS::Patron->new and circulation.pl This patch only deal with the circulation code as I am not convident enough with SIP code. The change does not seems trivial because of the complexity of the existing code, but the logic is the same. We send a variable to the template depending on the situation of the patron. I guess only code eyes ball could catch issue in this patch quickly. Maybe we need to find a good place in a Koha module to move this code and provide code coverage (especially when C4::SIP::ILS::Patron will reuse it). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 72354 [details] [review] Bug 19933: (follow-up) Remove patronflags - easy ones Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 72355 [details] [review] Bug 19933: Move instantiation of in deletemem.pl Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 72356 [details] [review] Bug 19933: Fix typo in template plugin name Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 72472 [details] [review] Bug 19933: Remove patronflags - unused vars oldamount/amountold in circulation.pl There is oldamound/amountold variable that is sent to the template but never used later. Let remove it now to avoid useless replacement. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72473 [details] [review] Bug 19933: Remove patronflags - In CanBookBeIssued The change is trivial here, we only want to pass gonenoaddress, lost and is_debared to the template. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72474 [details] [review] Bug 19933: Remove patronflags - easy ones This patch replaces the easy occurrences of patronflags. These calls only need the CHARGES->amount value, that is the non issues charges. Luckily we now have a Koha::Account->non_issues_charges that deal with that. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72475 [details] [review] Bug 19933: Remove patronflags - tricky ones Here we are, patronflags is used in a couple of places where (almost) all flags were really useful: C4::SIP::ILS::Patron->new and circulation.pl This patch only deal with the circulation code as I am not convident enough with SIP code. The change does not seems trivial because of the complexity of the existing code, but the logic is the same. We send a variable to the template depending on the situation of the patron. I guess only code eyes ball could catch issue in this patch quickly. Maybe we need to find a good place in a Koha module to move this code and provide code coverage (especially when C4::SIP::ILS::Patron will reuse it). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72476 [details] [review] Bug 19933: (follow-up) Remove patronflags - easy ones Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72477 [details] [review] Bug 19933: Move instantiation of in deletemem.pl Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72478 [details] [review] Bug 19933: Fix typo in template plugin name Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Pushed to master for 18.05, thanks to everybody involved!
Enhancement, not backported for 17.11