Description
Jonathan Druart
2018-01-09 14:13:05 UTC
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 |