Bug 11373

Summary: Add "change calculation" feature to the fine payment forms
Product: Koha Reporter: Frédérick Capovilla <fcapovilla>
Component: Fines and feesAssignee: Charles Farmer <charles.farmer>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: enhancement    
Priority: P5 - low CC: aloi54, black23, charles.farmer, colin.campbell, eric.begin, fiona.borthwick, francois.charbonnier, gmcharlt, jburds, jonathan.druart, josef.moravec, katrin.fischer, koha, kyle.m.hall, kyle, liz, m.de.rooy, marjorie.barry-vila, martin.renvoize, mehdi.hamidi, nick, oleonard, patrick.robitaille, philippe.audet-fortin, philippe.blouin, pierre-marc.thibault, rozenn.davenel, sandboxes, simon.pouchol, veron
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=6427
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15654
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13985
Change sponsored?: Sponsored Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This enhancement add an option to specify how much money was collected when paying a fine, as well as defining how much was paid on the fine. If these numbers are different (i.e. more money was collected) a popup displaying the amount of change to be given will be displayed and require confirmation before proceeding
Version(s) released in:
Bug Depends on:    
Bug Blocks: 24496, 22269, 22301, 24208, 24495    
Attachments: Add a "cash register" feature to the fine payment form.
Bug 11373: Add a "cash register" feature to the fine payment form.
Bug 11373: Add a "cash register" feature to the fine payment form.
Bug 11373 - Unit tests for Accounts.pm
Bug 11373 - Fixed: 'Change to give back' not displaying.
Bug 11373: Add a "cash register" feature to the fine payment form.
Bug 11373 - Unit tests for Accounts.pm
Bug 11373 - Fixed: 'Change to give back' not displaying.
Bug 11373: Add a "cash register" feature to the fine payment form.
Bug 11373 - Fixed: 'Change to give back' not displaying.
Bug 11373: Add a "cash register" feature to the fine payment form.
Bug 11373 - Fixed: 'Change to give back' not displaying.
Bug 11373: Add a "cash register" feature to the fine payment form.
Bug 11373: Add a "cash register" feature to the fine payment form.
Bug 11373: Add a "cash register" feature to the fine payment form.
Bug 11373: Add a "cash register" feature to the fine payment form.
Bug 11373: Follow-up after code review
Bug 11373: Add a "cash register" feature to the fine payment form.
Bug 11373: Follow-up after code review
Bug 11373: Add a "cash register" feature to the fine payment form.
Bug 11373: Add a "cash register" feature to the fine payment form.
Bug 11373: Add a "cash register" feature to the fine payment form.
System preference added, dialog removed, disabled input added to show the change in the form.
Bug 11373: System preference added, dialog removed, disabled input added to show the change in the form.
Bug 11373: Add a "cash register" feature to the fine payment form.
Bug 11373: System preference added, dialog removed, disabled input added to show the change in the form.
Bug 11373: Show the difference between the amount collected and the amount paid of a fine payment.
Bug 11373: Show the difference between the amount collected and the amount paid of a fine payment.
Bug 11373: Show the difference between the amount collected and the amount paid of a fine payment.
Bug 11373: Show the difference between the amount collected and the amount paid of a fine payment.
Bug 11373: Show the difference between the amount collected and the amount paid of a fine payment.
Mockup
Bug 11373: Show the difference between the amount collected and the amount paid of a fine payment.
Bug 11373: Show the difference between the amount collected and the amount paid of a fine payment.
Bug 11373: Show the difference between the amount collected and the amount paid of a fine payment.
Bug 11373: (follow-up) Add "change calculation" feature to the fine payment forms
Bug 11373: Show the difference between the amount collected and the amount paid of a fine payment.
Bug 11373: (follow-up) Add "change calculation" feature to the fine payment forms
Bug 11373: Show the difference between the amount collected and the amount paid of a fine payment.
Bug 11373: (follow-up) Add "change calculation" feature to the fine payment forms

Description Frédérick Capovilla 2013-12-10 22:09:11 UTC Comment hidden (obsolete)
Comment 1 Kyle M Hall 2013-12-13 12:56:29 UTC
Excellent! I would like to note that the accounts rewrite ( Bug 6427 ) also has this feature.

Kyle
Comment 2 Julian Maurice 2014-03-11 11:54:03 UTC Comment hidden (obsolete)
Comment 3 Biblibre Sandboxes 2014-03-11 13:10:09 UTC
Patch tested with a sandbox, by Joel Aloi <aloi54@live.fr>
Comment 4 Biblibre Sandboxes 2014-03-11 13:10:33 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2014-04-18 22:13:49 UTC
I am worried about the changes to Accounts.pm. We need unit tests to go with this patch.
Comment 6 Maxime Beaulieu 2014-07-16 14:36:54 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2014-07-17 08:18:10 UTC
This last patch does not need a signoff (will be done by QA).
Comment 8 Katrin Fischer 2014-07-20 19:03:38 UTC
I am sorry, but this doesn't seem to work for me. I have added various fines and fees to my patron account. Then I chose "Pay amount" and changed the collected from patron value to be higher than the total amount owed. I didn't see anything telling me how much change to give back to the patron. The open fines were paid and the additional money was 'lost'.

The same happens for paying individual fines.

Please retest.
Comment 9 Maxime Beaulieu 2014-07-21 17:32:54 UTC Comment hidden (obsolete)
Comment 10 Nicole C. Engard 2015-03-06 10:41:19 UTC
Am I supposed to apply to all these patches? I got the following:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 11373: Add a "cash register" feature to the fine payment form.
/home/nengard/kohaclone/.git/rebase-apply/patch:242: trailing whitespace.
        $template->param( 
/home/nengard/kohaclone/.git/rebase-apply/patch:247: trailing whitespace.
        $template->param( 
warning: 2 lines add whitespace errors.
Using index info to reconstruct a base tree...
<stdin>:242: trailing whitespace.
        $template->param( 
<stdin>:247: trailing whitespace.
        $template->param( 
warning: 2 lines applied after fixing whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging members/paycollect.pl
Auto-merging C4/Accounts.pm
CONFLICT (content): Merge conflict in C4/Accounts.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 11373: Add a "cash register" feature to the fine payment form.
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-11373-Add-a-cash-register-feature-to-the-fine--I0lypc.patch
Comment 11 Maxime Beaulieu 2015-03-13 19:35:09 UTC Comment hidden (obsolete)
Comment 12 Maxime Beaulieu 2015-03-13 19:36:11 UTC Comment hidden (obsolete)
Comment 13 Maxime Beaulieu 2015-03-13 19:36:38 UTC Comment hidden (obsolete)
Comment 14 Maxime Beaulieu 2015-03-13 19:43:48 UTC
Fixed the conflicts.
Comment 15 Mark Tompsett 2015-10-09 18:25:57 UTC
Comment on attachment 36875 [details] [review]
Bug 11373: Add a "cash register" feature to the fine payment form.

Review of attachment 36875 [details] [review]:
-----------------------------------------------------------------

Changing the sign of what was there before would need something done during upgrade to change it to a new way of processing. This wasn't done, so frankly, I think the logic I highlighted is broken. Though, admittedly, I did not run this code yet.

::: C4/Accounts.pm
@@ +722,5 @@
>      my $insert = 'INSERT INTO accountlines (borrowernumber, accountno, date, amount, '
>      .  'description, accounttype, amountoutstanding, itemnumber, manager_id, note) '
>      . ' VALUES (?, ?, now(), ?, ?, ?, 0, ?, ?, ?)';
>  
> +    $dbh->do(  $insert, undef, $borrowernumber, $nextaccntno, $payment,

0-$amount is the negative of $amount. This is a vast change! Is this correct?

@@ +728,5 @@
>  
>      UpdateStats({
>                  branch => $user,
>                  type => 'payment',
> +                amount => $payment,

This is 0-$amount which is the negative of what was put here before.

@@ +739,4 @@
>              action            => 'create_payment',
>              borrowernumber    => $user,
>              accountno         => $nextaccntno,
> +            amount            => $payment,

On a positive note, by putting $payment earlier, it can be used nicely here.

@@ +820,4 @@
>      UpdateStats({
>                  branch => $branch,
>                  type => 'writeoff',
> +                amount => $payment,

Again, 0-$amount is negative of what was there before.
Comment 16 Marc Véron 2016-04-17 13:10:18 UTC
Patch does not apply.
Auto-merging members/paycollect.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/paycollect.tt
Auto-merging C4/Accounts.pm
CONFLICT (content): Merge conflict in C4/Accounts.pm
Comment 17 Mehdi Hamidi 2017-02-21 15:47:57 UTC Comment hidden (obsolete)
Comment 18 Mehdi Hamidi 2017-02-21 15:48:10 UTC Comment hidden (obsolete)
Comment 19 Mehdi Hamidi 2017-02-21 16:22:28 UTC
I rebased the patches and now they apply properly.
I removed the changes in the file C4/Accounts.pm since the sub makepartialpayment was moved and renamed to sub pay in Koha/Accounts.pm, the patch works fine with the new changes.

I obsoleted the second patch which contained the tests for C4/Accounts.pm, because the module has it's own test files.

I added a test plan everyone can test it.

Test Plan
1) Apply patch
2) Select a patron with a fine.
3) Go to the selected details patron.
4) Go to the fines tab.
5) Click the button pay amount or pay selected.
6) Choose the amount paid equal to the outstanding amount(exemple : 3$).
7) Choose the amount collected to be more than the outstanding amount(exemple :5$)
8) Click the button confirm.
- You should be prompted with a message saying : "Change to give back: 2.00"
9) Click on the button above Confirm payment.

The selected patron should not have an outstanding fines left.
Comment 20 Biblibre Sandboxes 2017-03-17 21:08:13 UTC
Patch tested with a sandbox, by Candice Hope <candiceh@payson.org>
Comment 21 Biblibre Sandboxes 2017-03-17 21:08:46 UTC Comment hidden (obsolete)
Comment 22 Biblibre Sandboxes 2017-03-17 21:08:51 UTC Comment hidden (obsolete)
Comment 23 Rozenn Davenel 2017-03-21 09:56:27 UTC
It works.
Comment 24 Philippe Audet-Fortin 2017-05-17 15:44:06 UTC
It looks perfects!
Comment 25 David Bourgault 2017-09-25 18:30:11 UTC Comment hidden (obsolete)
Comment 26 Simon Pouchol 2017-10-19 12:47:36 UTC Comment hidden (obsolete)
Comment 27 Charles Farmer 2017-12-21 15:01:10 UTC Comment hidden (obsolete)
Comment 28 Charles Farmer 2017-12-21 15:06:17 UTC
Necessary rebase to apply on current master
Comment 29 Katrin Fischer 2018-02-17 13:12:55 UTC
Comment on attachment 70043 [details] [review]
Bug 11373: Add a "cash register" feature to the fine payment form.

Review of attachment 70043 [details] [review]:
-----------------------------------------------------------------

I am wondering about the usability here. For paying a partial fine I am forced to manually edit 2 input fields which adds more steps for the busy circulation desk and also might cause more potential for errors. Maybe it would make sense to automatically change the amount collected to the amount paid when it's changed? Or to make this optional?

I'd like to get some librarians to weigh in on these changes if possible. 

Some other small issues noted below. QA tools are clean.

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/paycollect.tt
@@ +98,5 @@
>  </ul>
>  <div class="tabs-container">
> +[% IF (error_negative) %]
> +    <div id="error_message" class="dialog alert">
> +        <span>The amount paid can't be negative!</span>

Why the additional spans everywhere? A message inside a div should not need it (see also other cases below).

@@ +175,5 @@
> +        [% give_change %]
> +    </li>
> +    [% ELSE %]
> +    <li>
> +        <label for="paid">Amount paid :</label>

No additional spaces between text and : please :)

::: members/paycollect.pl
@@ +128,5 @@
> +    elsif ($total_paid < $total_due and $total_collected ne $total_paid) {
> +         $template->param(
> +            amount_paid => sprintf('%.2f', $total_paid),
> +            amount_collected => sprintf('%.2f', $total_collected),
> +            give_change => sprintf('%.2f',($total_collected-$total_paid))

I am wondering about the formatting done in various spots in the patch. I'll ask a second opinion about that. Please note that we have a TT plugin for displaying prices in different ways and CurrencyFormat system preference (entering prices still happens with . ).
Comment 30 Katrin Fischer 2018-02-17 16:09:58 UTC
> I am wondering about the usability here. For paying a partial fine I am
> forced to manually edit 2 input fields which adds more steps for the busy
> circulation desk and also might cause more potential for errors. Maybe it
> would make sense to automatically change the amount collected to the amount
> paid when it's changed? Or to make this optional?
> 
> I'd like to get some librarians to weigh in on these changes if possible. 

Another idea: leave the amount collected empty initially, meaning the amount paid equals the amount collected. And only have people enter an amount if that isn't the case. That means you could still collect payments without extra clicks needed as before, but when you need it use of the field will be easier as you don't need to ovrwrite/delete the amount in it first.
Comment 31 Charles Farmer 2018-02-19 20:58:32 UTC Comment hidden (obsolete)
Comment 32 Charles Farmer 2018-02-19 20:58:38 UTC Comment hidden (obsolete)
Comment 33 Charles Farmer 2018-02-19 21:18:44 UTC
Hi Katrin,

Following your comments, I made cosmetic adjustments to the previous patch:
    - <span> no longer present in alert dialog
        - I also toned down from 'exclamation mark' to a simple period
    - removed the dangling space before ':'
    - server side formatting was changed from sprintf to $Price
        - I also removed the formatting in the <input> fields. It made no sense to me that formatting was used for data traveling between the page and the server, unbeknown to the user.

About the behavior of the form: I agree with you, the UX should definitely be more user-friendly. Now, I'm open to changes, but I'd rather implement everything once we reach a consensus on how it should behave than doing "flavors" of the same patch just for QA. Nothing personal ;)

I'll put this back to 'Needs signoff', for now, just to make it clear that I want people testing this patch, but feel free to dial it back to "Failed QA" or "In discussion". I'll be keeping a close eye on this.
Comment 34 Katrin Fischer 2018-02-22 21:50:29 UTC
> Following your comments, I made cosmetic adjustments to the previous patch:
>     - <span> no longer present in alert dialog
>         - I also toned down from 'exclamation mark' to a simple period
>     - removed the dangling space before ':'
>     - server side formatting was changed from sprintf to $Price
>         - I also removed the formatting in the <input> fields. It made no
> sense to me that formatting was used for data traveling between the page and
> the server, unbeknown to the user.

Thx!


> About the behavior of the form: I agree with you, the UX should definitely
> be more user-friendly. Now, I'm open to changes, but I'd rather implement
> everything once we reach a consensus on how it should behave than doing
> "flavors" of the same patch just for QA. Nothing personal ;)
> 
> I'll put this back to 'Needs signoff', for now, just to make it clear that I
> want people testing this patch, but feel free to dial it back to "Failed QA"
> or "In discussion". I'll be keeping a close eye on this.

I totally agree with this approach. It might make sense to ask on the mailing list as well to get some attention on this.
Comment 35 Charles Farmer 2018-08-08 18:01:32 UTC Comment hidden (obsolete)
Comment 36 Charles Farmer 2018-08-08 18:01:40 UTC Comment hidden (obsolete)
Comment 37 Blou 2018-09-19 18:43:17 UTC Comment hidden (obsolete)
Comment 38 Blou 2018-09-19 18:46:20 UTC
Patch did not apply.  Rebased.  Where is the commit message gone?
Comment 39 Michal Denar 2018-09-19 19:39:54 UTC
Hi  Blou,
Great work. I've just some findings.
Patch is working,but:
- confirmation button has some formatting problem = https://prnt.sc/kwd2do
- Cancel link is broken, without borrower number (http://localhost:8081/cgi-bin/koha/members/pay.pl?borrowernumber=)
- this funkcnionaly should be optional, so weneed enable=disable it from administration menu

Just two steps to heaven :-)

Thank You

Mike
Comment 40 Blou 2018-09-20 12:44:48 UTC Comment hidden (obsolete)
Comment 41 Blou 2018-09-20 12:48:05 UTC
Fixed the rest, but for the padding in the button, we'll need to find another solution since an <input> under a class="dialog message" will show like that.  Not sure why the staff.css specifies that padding, but clearly having a confirm button in a message dialog is not the standard Koha way.
Comment 42 Michal Denar 2018-09-20 21:10:22 UTC Comment hidden (obsolete)
Comment 43 Josef Moravec 2018-09-21 07:54:55 UTC
(In reply to Blou from comment #41)
> Fixed the rest, but for the padding in the button, we'll need to find
> another solution since an <input> under a class="dialog message" will show
> like that.  Not sure why the staff.css specifies that padding, but clearly
> having a confirm button in a message dialog is not the standard Koha way.

That is because you should have icon on that button - try to add a class="approve" to button.
Comment 44 Josef Moravec 2018-09-21 08:06:20 UTC
(In reply to Josef Moravec from comment #43)
> (In reply to Blou from comment #41)
> > Fixed the rest, but for the padding in the button, we'll need to find
> > another solution since an <input> under a class="dialog message" will show
> > like that.  Not sure why the staff.css specifies that padding, but clearly
> > having a confirm button in a message dialog is not the standard Koha way.
> 
> That is because you should have icon on that button - try to add a
> class="approve" to button.

And add the icon itself like this:
<i class="fa fa-fw fa-check"></i>
Comment 45 Josef Moravec 2018-09-21 08:08:58 UTC
The information on this page is duplicate:

https://screenshots.firefox.com/o2GtPtGugLjtJSqq/localhost

why do you even add the dialog?

Also this could be enabled disabled, please add system preference for this.
Comment 46 Josef Moravec 2018-09-21 08:20:28 UTC
Comment on attachment 79188 [details] [review]
Bug 11373: Add a "cash register" feature to the fine payment form.

Review of attachment 79188 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/paycollect.tt
@@ +75,5 @@
> +    <div id="change_message" class="dialog message">
> +        <span>The amount collected is greater than the total amount paid.</span><br />
> +        <strong>Change to give back: [% give_change | $Price %]</strong><br /><br />
> +        <input type="submit" name="submitbutton" value="Confirm payment" />
> +        <a class="cancel" href="/cgi-bin/koha/members/pay.pl?borrowernumber=[% patron.borrowernumber %]">Cancel</a>

use uri filter

@@ +120,5 @@
> +    [% IF ( give_change ) %]
> +    <li>
> +        <label for="paid">Amount paid:</label>
> +        <input type="hidden" name="paid" id="paid" value="[% amount_paid %]" />
> +        <input type="hidden" name="collected" id="collected" value="[% amount_paid %]" />

use html filter

@@ +134,5 @@
> +    </li>
> +    [% ELSE %]
> +    <li>
> +        <label for="paid">Amount paid:</label>
> +        <input name="paid" id="paid" value="[% amountoutstanding | format('%.2f') %]" onchange="moneyFormat(document.payform.paid)"/>

use $Price filter

@@ +141,3 @@
>      <li>
> +        <label for="collected">Collect from patron:</label>
> +        <input name="collected" id="collected" value="[% amountoutstanding %]" onchange="moneyFormat(document.payform.collected)" />

use $Price filter

@@ +233,5 @@
>          </li>
> +    [% IF ( give_change ) %]
> +    <li>
> +        <label for="paid">Amount paid:</label>
> +        <input type="hidden" name="paid" id="paid" value="[% amount_paid %]" />

use html filter

@@ +238,5 @@
> +        [% amount_paid | $Price %]
> +    </li>
> +    <li>
> +        <label>Collected from patron:</label>
> +        <input type="hidden" name="collected" id="collected" value="[% amount_paid %]" />

use html filter

@@ +248,5 @@
> +    </li>
> +    [% ELSE %]
> +    <li>
> +        <label for="paid">Amount paid :</label>
> +        <input name="paid" id="paid" value="[% total %]" onchange="moneyFormat(document.payform.paid)" />

use $Price filter

@@ +259,3 @@
>          [% END %]
>          <!-- default to paying all -->
> +        <input name="collected" id="collected" value="[% total %]" onchange="moneyFormat(document.payform.collected)"/>

use $Price filter
Comment 47 Pierre-Marc Thibault 2018-09-25 16:05:54 UTC
(In reply to Josef Moravec from comment #45)
> The information on this page is duplicate:
> 
> https://screenshots.firefox.com/o2GtPtGugLjtJSqq/localhost
> 
> why do you even add the dialog?
> 
> Also this could be enabled disabled, please add system preference for this.

The dialog is here maybe to explain to the user why they are still on the page, because otherwise they just see a slight different formatted form when the change is not null. If the dialogue is not a matter but the duplication is, removing the "Change to give back:" line could be an option. 

What do you want to add to system preferences precisely? The whole feature with the change? Is that what you mean by "this"?

Thanks for replying.
Comment 48 Josef Moravec 2018-09-25 16:39:14 UTC
(In reply to Pierre-Marc Thibault from comment #47)
> (In reply to Josef Moravec from comment #45)
> > The information on this page is duplicate:
> > 
> > https://screenshots.firefox.com/o2GtPtGugLjtJSqq/localhost
> > 
> > why do you even add the dialog?
> > 
> > Also this could be enabled disabled, please add system preference for this.
> 
> The dialog is here maybe to explain to the user why they are still on the
> page, because otherwise they just see a slight different formatted form when
> the change is not null. If the dialogue is not a matter but the duplication
> is, removing the "Change to give back:" line could be an option. 

Yes, the duplication is problem for me, now it is a bit confusing. I think the form itself should not be there, only dialog with all the information and the amount to give back to patron should be emphasized. 

> What do you want to add to system preferences precisely? The whole feature
> with the change? Is that what you mean by "this"?

Yes, that's exactly what I mean, because this change the behavior of pay functionality.

> 
> Thanks for replying.

Not at all ;)
Comment 49 Pierre-Marc Thibault 2018-09-25 20:38:57 UTC
I think simple is best. Instead of confirming on the form and the dialog and then paying, there should be a disabled input which shows the change each time a number input is changed. You get the change feedback instantly, just click once for paying and there is no duplication data on the page.
Comment 50 Martin Renvoize 2018-09-26 09:15:09 UTC
The bug description is really misleading.. this doesn't add a cash register/till at all, rather it's just a method of displaying change to give.
Comment 51 Colin Campbell 2018-09-26 09:17:07 UTC
The commit should describe what the feature does. 'cash register feature' is not enough as its is ambiguous as to what kind of functionality you are adding.

It might be better to handle this in javascript, theres no need to send to the server for this calculation it could all be handled more efficiently in the browser
Comment 52 Pierre-Marc Thibault 2018-09-27 13:32:08 UTC Comment hidden (obsolete)
Comment 53 Pierre-Marc Thibault 2018-09-27 13:33:29 UTC Comment hidden (obsolete)
Comment 54 Pierre-Marc Thibault 2018-09-27 13:39:29 UTC
Bug status put at Needs Signoff because many things have changed.
Comment 55 Michal Denar 2018-09-27 15:17:34 UTC Comment hidden (obsolete)
Comment 56 Michal Denar 2018-09-27 15:17:42 UTC Comment hidden (obsolete)
Comment 57 Blou 2018-09-27 15:28:13 UTC
I've asked Pierre-Marc to squash these, since the second patch is mostly useless, and the second one is mostly removing the first one's changes.

I know how QA will appreciate looking at this, so he's going to do a new one that's easier to interpret (and will need sign off).  My apologies to Michal who took the time already to sign it off.
Comment 58 Pierre-Marc Thibault 2018-09-27 15:57:07 UTC Comment hidden (obsolete)
Comment 59 Michal Denar 2018-09-27 20:13:28 UTC Comment hidden (obsolete)
Comment 60 Blou 2018-09-28 12:43:34 UTC
This should be further simplified.

- if preference is set, UI should send to backend the exact amount, since we presume the change has been given back.
- no need to overwrite the error_over behavior.  It will be necessary when the pref is not set.
- in fact, the modification to members/paycollect.pl are not necessary, imo.
- if preference is set, make the change comment very obvious (color message?)
- The patch should focus on the functionality.  Do not rewrite all the format() to $Price if not related to the feature.  This should be a separate patch.

To consider: making the submit button have a confirm popup when change is to be given back.  Not need to go to the backend for that.
Comment 61 Pierre-Marc Thibault 2018-10-01 18:48:07 UTC Comment hidden (obsolete)
Comment 62 Pierre-Marc Thibault 2018-10-01 18:58:22 UTC
The code is simpler and the formating is only on new lines. The change comment is similar to the others and is not related to the bug, so it has not been changed. A dialog box has been added, it's possible to reconfirm or not the payment when the change isn't 0.00.
Comment 63 Josef Moravec 2018-10-01 20:31:04 UTC
Comment on attachment 79759 [details] [review]
Bug 11373: Show the difference between the amount collected and the amount paid of a fine payment.

Review of attachment 79759 [details] [review]:
-----------------------------------------------------------------

Great job, just one little thing, reading the code only:

::: members/paycollect.pl
@@ +194,4 @@
>      patron        => $patron,
>      total         => $total_due,
>      ExtendedPatronAttributes => C4::Context->preference('ExtendedPatronAttributes'),
> +    FineChange => C4::Context->preference('FineChange'),

Use Koha.Preference template plugin and do not pass system preferences as template parameter
Comment 64 Pierre-Marc Thibault 2018-10-04 12:32:05 UTC Comment hidden (obsolete)
Comment 65 Michal Denar 2018-10-04 16:58:24 UTC Comment hidden (obsolete)
Comment 66 Martin Renvoize 2018-10-08 15:56:52 UTC
Comment on attachment 79951 [details] [review]
Bug 11373: Show the difference between the amount collected and the amount paid of a fine payment.

Review of attachment 79951 [details] [review]:
-----------------------------------------------------------------

Can one still overpay when using this system?

What if the librarian finds they don't have the appropriate change in the drawer, can they continue with the transaction inputting the full amount and as such creating a credit?

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/paycollect.tt
@@ +108,5 @@
> +        <input id="collected" value="[% amountoutstanding | $Price %]"/>
> +    </li>
> +    <li>
> +        <label>Change to give: </label>
> +        <input id="change" value="0.00" disabled/>

This shouldn't be an 'input' in my opinion, rather it should be more clearly distinguished as information.. perhaps a span with a class highlighting in 'red' if the patron is owed change?

@@ +220,5 @@
> +        <input id="collected" value="[% total | $Price %]"/>
> +    </li>
> +    <li>
> +        <label>Change to give: </label>
> +        <input id="change" value="0.00" disabled/>

As above
Comment 67 Katrin Fischer 2018-10-11 05:59:27 UTC
(In reply to Martin Renvoize from comment #50)
> The bug description is really misleading.. this doesn't add a cash
> register/till at all, rather it's just a method of displaying change to give.

Hi Martin, do you have a good idea for a new bug title? I agree it might be a little misleading the way it is.
Comment 68 Katrin Fischer 2018-10-11 06:03:29 UTC
Hi Martin, I just tested: Overpaying and creating a credit by it is not possible right now. I think it was intentionally changed as the credits caused a bit of trouble and should be added intentionally.
Comment 69 Josef Moravec 2018-10-11 07:07:19 UTC
To add more: you missed the database part of adding a system preference, see https://wiki.koha-community.org/wiki/System_Preferences#Adding_a_new_system_preference
Comment 70 Katrin Fischer 2018-10-11 08:30:52 UTC
I think the overpaying note might not be valid, but the other remarks (input, db update) are, can you please check?
Comment 71 Martin Renvoize 2018-10-11 16:38:57 UTC
I'm not so sure this should be syspref driven.. I can't see any negative effects from it that one would want to disable it for (except, personally i'd like to be able to overpay if required and as such have this enhanced a little further to allow payment + keep the change as credit).
Comment 72 Martin Renvoize 2018-10-11 16:40:55 UTC
Created attachment 80414 [details]
Mockup
Comment 73 Katrin Fischer 2018-10-11 19:39:35 UTC
The initial implementation added more steps I think, which was part of the reason to ask to make it optional. I haven't tested tihs one yet, but I feel we shouldn't add more to it right now.
Comment 74 Pierre-Marc Thibault 2018-10-22 12:34:47 UTC
Right now, all the required fields are filled automatically. Moreover, if the change is null (0.00), submitting the form bring users back to the Pay fines page immediately. Users can click a single time to pay the full fine. I agree with the idea that adding a system preference is irrelevant in this context and, therefore, I am going to remove it.

I also agree that disabled inputs should be turned into spans to avoid confusion. I think that if the change is null the span should be blank and if not it should be filled in red.
Comment 75 Pierre-Marc Thibault 2018-10-22 17:38:36 UTC Comment hidden (obsolete)
Comment 76 Michal Denar 2018-11-07 15:28:37 UTC
Please rebase on master.
Comment 77 Pierre-Marc Thibault 2018-11-16 21:48:09 UTC Comment hidden (obsolete)
Comment 78 Michal Denar 2018-11-30 15:12:22 UTC
Created attachment 82797 [details] [review]
Bug 11373: Show the difference between the amount collected and the amount paid of a fine payment.

This patch adds a feature in the fine payment section. It allows to see the change due to patrons when the amount collected is higher than the amount paid.

Test plan :
- Apply patch.
- Select a patron with a fine.
- Go to Fines > Pay fines.
- Click the button pay.
- Choose the amount paid equal to the outstanding amount (exemple : 3$).
- Choose the amount collected to be more than the outstanding amount (exemple : 5$).
- Confirm that the change is correct (example : 5$ - 3$ = 2$).
- Click the button confirm.
- Click on Yes in the dialog box.
- Confirm that the payment has been made (example : last amount = 3$).

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 79 Owen Leonard 2019-01-11 16:09:17 UTC
Created attachment 83804 [details] [review]
Bug 11373: (follow-up) Add "change calculation" feature to the fine payment forms

This patch provides some follow-ups, including corrections to the
JavaScript and updates to the text for readability.

- Remove obsolete script "type" attribute
- Update the use of the obsolete jQuery method "bind"
- Use Bootstrap's built-in "show" option instead of simulated click
- Handle preventDoubleFormSubmit function's "waiting" class if modal is
  closed manually

To test, apply the patch and follow the original test plan. Everything
should work as expected.

Note that when the modal is shown, the cursor changes to the "waiting"
one when you hover over the body of the page. If you click "No" in the
modal, the cursor should return to normal.
Comment 80 Martin Renvoize 2019-01-11 17:05:20 UTC
Created attachment 83812 [details] [review]
Bug 11373: Show the difference between the amount collected and the amount paid of a fine payment.

This patch adds a feature in the fine payment section. It allows to see the change due to patrons when the amount collected is higher than the amount paid.

Test plan :
- Apply patch.
- Select a patron with a fine.
- Go to Fines > Pay fines.
- Click the button pay.
- Choose the amount paid equal to the outstanding amount (exemple : 3$).
- Choose the amount collected to be more than the outstanding amount (exemple : 5$).
- Confirm that the change is correct (example : 5$ - 3$ = 2$).
- Click the button confirm.
- Click on Yes in the dialog box.
- Confirm that the payment has been made (example : last amount = 3$).

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 81 Martin Renvoize 2019-01-11 17:05:26 UTC
Created attachment 83813 [details] [review]
Bug 11373: (follow-up) Add "change calculation" feature to the fine payment forms

This patch provides some follow-ups, including corrections to the
JavaScript and updates to the text for readability.

- Remove obsolete script "type" attribute
- Update the use of the obsolete jQuery method "bind"
- Use Bootstrap's built-in "show" option instead of simulated click
- Handle preventDoubleFormSubmit function's "waiting" class if modal is
  closed manually

To test, apply the patch and follow the original test plan. Everything
should work as expected.

Note that when the modal is shown, the cursor changes to the "waiting"
one when you hover over the body of the page. If you click "No" in the
modal, the cursor should return to normal.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 82 Martin Renvoize 2019-01-11 17:06:37 UTC
Works well for me, signing off
Comment 83 Josef Moravec 2019-02-02 08:28:49 UTC
Created attachment 84653 [details] [review]
Bug 11373: Show the difference between the amount collected and the amount paid of a fine payment.

This patch adds a feature in the fine payment section. It allows to see the change due to patrons when the amount collected is higher than the amount paid.

Test plan :
- Apply patch.
- Select a patron with a fine.
- Go to Fines > Pay fines.
- Click the button pay.
- Choose the amount paid equal to the outstanding amount (exemple : 3$).
- Choose the amount collected to be more than the outstanding amount (exemple : 5$).
- Confirm that the change is correct (example : 5$ - 3$ = 2$).
- Click the button confirm.
- Click on Yes in the dialog box.
- Confirm that the payment has been made (example : last amount = 3$).

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 84 Josef Moravec 2019-02-02 08:29:09 UTC
Created attachment 84654 [details] [review]
Bug 11373: (follow-up) Add "change calculation" feature to the fine payment forms

This patch provides some follow-ups, including corrections to the
JavaScript and updates to the text for readability.

- Remove obsolete script "type" attribute
- Update the use of the obsolete jQuery method "bind"
- Use Bootstrap's built-in "show" option instead of simulated click
- Handle preventDoubleFormSubmit function's "waiting" class if modal is
  closed manually

To test, apply the patch and follow the original test plan. Everything
should work as expected.

Note that when the modal is shown, the cursor changes to the "waiting"
one when you hover over the body of the page. If you click "No" in the
modal, the cursor should return to normal.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 85 Nick Clemens 2019-02-04 16:54:02 UTC
If I choose 'Pay amount' and set the amount paid to higher than the fines and collect more than is being paid I get the modal calculating the change and a confirm, but then the payment is rejected.

I feel like the Amount paid error should take precedence and prevent the modal and confirmation from appearing.

Does that seem reasonable?
Comment 86 Nick Clemens 2019-02-04 17:38:33 UTC
(In reply to Nick Clemens from comment #85)
> If I choose 'Pay amount' and set the amount paid to higher than the fines
> and collect more than is being paid I get the modal calculating the change
> and a confirm, but then the payment is rejected.
> 
> I feel like the Amount paid error should take precedence and prevent the
> modal and confirmation from appearing.
> 
> Does that seem reasonable?

I neglected to revert these so...I will file a dependent bug report
Comment 87 Nick Clemens 2019-02-04 17:39:07 UTC
Awesome work all!

Pushed to master for 19.05
Comment 88 Martin Renvoize 2019-02-11 13:50:13 UTC
Great enhancement, will not be backported however.
Comment 89 Katrin Fischer 2019-02-15 07:00:40 UTC
I have to say I am not a fan of the modal. It adds an extra step and has too much text.
Comment 90 Katrin Fischer 2019-02-17 12:28:29 UTC
(In reply to Katrin Fischer from comment #89)
> I have to say I am not a fan of the modal. It adds an extra step and has too
> much text.

I got some more feedback in chat - I've added bug 22359 for more discussion and ideas.