[Bug 41265] New: NoIssuesChargeGuarantees and NoIssuesChargeGuarantorsWithGuarantees circulation checks not precise enough
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41265 Bug ID: 41265 Summary: NoIssuesChargeGuarantees and NoIssuesChargeGuarantorsWithGuarantees circulation checks not precise enough Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: thibaud.guillot@biblibre.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle@bywatersolutions.com Hello all When a checkout is performed, the NoIssuesChargeGuarantees and NoIssuesChargeGuarantorsWithGuarantees values are checked. This is an ‘if’ statement with check values, but by default, these are empty strings and therefore defined in this way. I therefore believe that the verification is not exactly what we want, particularly when SIP performs the checkout without permission to force it. Furthermore, in kohastructure, the default value would be NULL from what I understand, and it has not been included in the syprefs sql file. Perhaps there are some cases that I am overlooking, but I will suggest a simple fix anyway. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41265 Thibaud Guillot (thibaud_g) <thibaud.guillot@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=39261 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41265 --- Comment #1 from Thibaud Guillot (thibaud_g) <thibaud.guillot@biblibre.com> --- I added Bug 39261 which also deals with the format of these sysprefs -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41265 Thibaud Guillot (thibaud_g) <thibaud.guillot@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Patch complexity|--- |Small patch -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41265 --- Comment #2 from Thibaud Guillot (thibaud_g) <thibaud.guillot@biblibre.com> --- Created attachment 189694 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189694&action=edit Bug 41265: Checks if NoIssuesChargeGuarantees && NoIssuesChargeGuarantorsWithGuarantees sysprefs limits are not empty Test plan: 1) Create relation between guarantor and guarantees 2) Be sure that one (or both) has outstanding charges 3) Be sure that concerning sysprefs are "empty" / not defined 4) Perform a checkout 5) A warning appears normally, you can fore checkout if you have enough permission 6) Apply this patch 7) Repeat step 4), no warning appears Sponsored-by: BibLibre -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41265 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |thibaud.guillot@biblibre.co |ity.org |m Severity|enhancement |minor -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41265 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #3 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Looking at this, I have some questions: 1) As this makes a change to Patrons.pm, could/shoudl we have a unit test highlighting the effect of the change? 2) You check for the length using length() - I haven't found any other occurrences of this in the templates which made me wonder. Should we not just check if the value is > 0? length($no_issues_charge_guarantees) > 0 vs. $no_issues_charge_guarantees > 0 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41265 --- Comment #4 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Katrin Fischer from comment #3)
Looking at this, I have some questions:
1) As this makes a change to Patrons.pm, could/shoudl we have a unit test highlighting the effect of the change?
2) You check for the length using length() - I haven't found any other occurrences of this in the templates which made me wonder. Should we not just check if the value is > 0?
length($no_issues_charge_guarantees) > 0 vs. $no_issues_charge_guarantees > 0
... 0 might also make sense to block. So forget that suggestion. Looking at the code, the common check seems to be != "" git grep "!= ''" *.tt -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org