https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28924 --- Comment #79 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- (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.