https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28924
--- Comment #79 from Matt Blenkinsop <[email protected]> --- (In reply to Katrin Fischer from comment #66) > 1) Is this correct? > > [% IF ( charges ) %] > - [% INCLUDE 'blocked-fines.inc' borrowernumber = > patron.borrowernumber %] > + [% INCLUDE 'blocked-fines.inc' NoIssuesCharge = chargesamount %] > [% END %] > > It looks like we are losing the borrowernumber? The patron is still picked up by the template and populates the links correctly > > 2) Terminology > > https://wiki.koha-community.org/wiki/Terminology > > a) +=head3 can_borrow > ... > +This method determines whether a borrower is able to borrow based on > various parameters. > +- Debarrments > > I feel like this should be "can_checkout" to follow our terminology guide > and because borrow/lend is a common one to get confused by non-native > speakers. > > Also some small stuff borrower = patron etc. Fixed :) > > b) noissuecharge > > The noissuecharge parameter is badly named and has always been. I feel like > we will never will fully recover from it as it has spread to other system > preferences by now, but I wonder if we could at least start to move into a > better direction with the new database table columns: > > noissuescharge > noissueschargeguarantees > noissueschargeguarantorswithguarantees > > I can see the consistency here, so this is more a question than a blocker. > I was thinking along the lines of checkout_charge_limit or so. I think this probably needs breaking out into a separate bug with some discussion about how to change the terminology and agreement on the wording to use. Then this probably needs to be implemented across Koha more widely including the original sysprefs rather than doing it in just one place otherwise we risk introducing even more inconsistency in these patches > > c) Templates > > So this is a blocker for me, because I can see the consistency with the > codes/prefs, but I think we can do better in the templates and stick to the > agreed on terms: > > + <th scope="col">No issues charge</th> > + <th scope="col">No issues charge guarantees</th> > + <th scope="col">No issues charge guarantors with > guarantees</th> > > Also in the input form. > > What about: > Checkout charge limit... ? And maybe a good hint for the input form with > reference and link to noissuescharge and the other preferences if we want to > make that connection visible. I've updated these as suggested and added a hint > 3) Don't include api/v1/swagger/swagger_bundle.json > > It's in this patch: > Subject: [PATCH] Bug 28924: Add new columns to UI and controller > > It will be automatically generated. Done, its in gitignore so not sure how it made it into the patch > > 4) Translatability > > This smells untranslatable: > > - my $noissuescharge = C4::Context->preference("noissuescharge") || 5; > - $flaginfo{'message'} = sprintf 'Patron owes %.02f', $owing; > - $flaginfo{'amount'} = sprintf "%.02f", $owing; > - if ( $owing > $noissuescharge && > !C4::Context->preference("AllowFineOverride") ) { > + my $noissuescharge = > $patron_charge_limits->{noissuescharge}->{limit} || 5; > + $flaginfo{'message'} = sprintf 'Patron owes %.02f', > $patron_charge_limits->{noissuescharge}->{charge}; > + $flaginfo{'amount'} = sprintf "%.02f", > $patron_charge_limits->{noissuescharge}->{charge}; > > - $flaginfo{'message'} = sprintf 'patron guarantees owe %.02f', > $guarantees_non_issues_charges; > - $flaginfo{'amount'} = $guarantees_non_issues_charges; > + $flaginfo{'message'} = sprintf 'patron guarantees owe %.02f', > $patron_charge_limits->{NoIssuesChargeGuarantees}->{charge}; > + $flaginfo{'amount'} = > $patron_charge_limits->{NoIssuesChargeGuarantees}->{charge}; > > > Also the Price formatting will be wrong (CurrentyFormat). I am not sure > where this is used? This is only used in the SIP module when creating the patron object. There isn't any template that I can see that this gets rendered into as I assume this is handled by the hardware supplier that SIP is connecting to > > 5) Documentation > > + `noissuescharge` int(11) DEFAULT NULL COMMENT 'define maximum amount > withstanding before checkouts are blocked', > + `noissueschargeguarantees` int(11) DEFAULT NULL COMMENT 'define maximum > amount withstanding before checkouts are blocked', > + `noissueschargeguarantorswithguarantees` int(11) DEFAULT NULL COMMENT > 'define maximum amount withstanding before checkouts are blocked', > > These all have the same description - what is the difference? Fixed the descriptions, clearly didn't change them after a copy and paste job! > > 6) Formatting > > Please use the $Price TT filter for proper formatting of the prices on > display, including the 0.00 default: > > + [% IF (category.noissuescharge) %] > + <td>[% category.noissuescharge | html %]</td> > + [% ELSE %] > + <td>0.00</td> > + [% END %] > + [% IF (category.noissueschargeguarantees) %] > + <td>[% category.noissueschargeguarantees | html > %]</td> > + [% ELSE %] > + <td>0.00</td> > + [% END %] > + [% IF > (category.noissueschargeguarantorswithguarantees) %] > + <td>[% > category.noissueschargeguarantorswithguarantees | html %]</td> > + [% ELSE %] > + <td>0.00</td> > + [% END %] > Done > 7) Price handling on input fields > > You need to use $Price on_editing on the input fields, I believe: > > Example: > <input class="decimal" type="text" size="20" name="rrp" id="rrp" value="[% > rrp | $Price on_editing => 1 %] Done -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
