Follow these steps: Add a manual invoice with a note Add a second manual invoice with a different note Pay the first invoice using the 'pay' tab the note carries over but is not published on the 'pay' line Pay the second invoice and put a different note in the note isn't put on the payment line, but the note on the charge is changed Logically, there should be a note for the payment line and a note for the charge line, they don't have to be the same. As it is now you can't add a note on a payment line at all, but the field is there.
Example video: http://screencast.com/t/bPYEhgtAj
Looking into this
Its working as designed, it allows you to modify the note on the charge when you save. It was never designed to add a new note on the payment line. So changing it would be changing behaviour, which would make it impossible for libraries to change the note on a charge when the pay it off. Is that desirable? I think it would have to be done in a way that allows current behaviour to still continue, but also allow someone to add a note on the payment line.
I honestly don't know. It sounds like if that's how it's supposed to be then maybe we shouldn't consider what I'm asking for a bug. Instead this is a feature request to be able to add a note to payments that is separate from the fine line. Anyone else have any opinions? Maybe a way to make this clearer to the librarian what's going to happen because it looks like the note is going on the payment line because that's where the form is appearing. Nicole
The ability to add a note when paying a fine appears to have been removed in the patch for bug 3498 Allow partial payments of fines (507c7f422485aaf7ae601b0a84bacdbaedac061c). Is adding it back in desirable? If so, is the ability to also edit the notes on fines still required? Currently it is not possible so not having it is not reverting, however it could be added in via an edit button on the fines page or something similar.
I've had several libraries ask to have that ability brought back - for paying and for writing off fines. I'm not sure about whether they want to be able to edit that note after the fact, but if it were possible I'm sure no one would complain. Most important thing is allowing people to make notes/memos on payments/writeoffs. Nicole
Created attachment 7333 [details] [review] Added ability to add notes when paying or writing off fines On the Account tab of the fines page there is a new 'note' column beside each entry. When paying a fine a 'payment note' box appears beside each fine, this note will be set for that payment rather than the fine itself. Both notes on fines and notes on payments/write-offs cannot be changed. Using either the 'pay' or 'write off' buttons next to an entry will save the note for that entry. Using the 'writeoff all' button will save the notes for all entries. Please note however that using either the 'pay amount' or 'pay selected' does NOT save the notes. This is due to the current way in which this is handled between pay.pl and paycollect.pl
Attempted testing of this patch, does not appear to work completely. Applies cleanly. 1) I created a series of manual invoices with notes, those notes appear in the Note column on boraccount.pl 2) Switch to pay.pl. Here, the Payment Note column appears with a text box, but does not pull over the fine note ( I'm not sure if that is supposed to be part of the functionality or not, but I thought I'd mention it ). 4) Tested with pay, writeoff, and writeoff all buttons, works as stated. HOWEVER, everything gets broken and messed up if special characters are used in *any* of the notes fields. The data being passed is not being url encoded properly. For example, if I make the payment note "Test Payment #12 Note", the payment note will be "Test Payment ". It gets truncated at the special character. If you look at the passed params, you will see "payment_note=Test Payment #12 Note" rather than "payment_note=Test%20Payment%20%2312%20Note", as it should be. It gets worse if you used special characters in the Description of charges, or the Note field. In that case, no part of the note will show up, as everything is truncated at the special character even earlier. So, patch essentially works, but everything needs to be url encoded properly or librarians will not understand why their notes aren't showing up correctly.
Created attachment 7496 [details] [review] Added ability to add notes when paying or writing off fines Cheers for that Kyle. There was 2 issues. One was with the note fields for payments needing url encoding, the other was to do with description$line_no needing to be escaped, but as I cant see where it is being used I have removed it (if it needs to be put back in, it must be uri_escaped). Hope this patch works better.
Applying: Bug 6413 Added ability to add a note when paying or writing off a fine Code will also respect notes when using the "Writeoff All" button but WILL NOT when using either the "Pay Amount" or "Pay Selected" buttons Fixed uri encoding of arguments /home/koha/kohaclone/.git/rebase-apply/patch:77: trailing whitespace. <th>Payment Note</th> /home/koha/kohaclone/.git/rebase-apply/patch:217: trailing whitespace. if ( $total_paid == $total_due ) { error: patch failed: C4/Accounts.pm:197 error: C4/Accounts.pm: patch does not apply
Created attachment 9107 [details] [review] Added ability to add notes when paying or writing off fines Corrected patch so it would apply against master.
nengard@kohavm:~/kohaclone$ git bz apply 6413 Bug 6413 - Notes in Fines doing wonky things Added ability to add notes when paying or writing off fines Apply? [yn] y Applying: Bug 6413 Added ability to add a note when paying or writing off a fine Code will also respect notes when using the "Writeoff All" button but WILL NOT when using either the "Pay Amount" or "Pay Selected" buttons Fixed uri encoding of arguments /home/nengard/kohaclone/.git/rebase-apply/patch:121: trailing whitespace. <th>Payment Note</th> /home/nengard/kohaclone/.git/rebase-apply/patch:231: trailing whitespace. if ( $total_paid == $total_due ) { error: patch failed: koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt:47 error: koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt: patch does not apply Patch failed at 0001 Bug 6413 Added ability to add a note when paying or writing off a fine Code will also respect notes when using the "Writeoff All" button but WILL NOT when using either the "Pay Amount" or "Pay Selected" buttons Fixed uri encoding of arguments When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Patch left in /tmp/Added-ability-to-add-notes-when-paying-or-writing--sDFuBJ.patch nengard@kohavm:~/kohaclone$
Did you remember to try applying it with a 3 way merge git bz is cool, but without the patch from paul, it doesnt try to merge. git am --abort git am -iu3 /tmp/Added-ability-to-add-notes-when-paying-or-writing--E9jlEe.patch Found out it was a tiny conflict because of the capitalisation change, in one file. Fixed that, reattached. But if git bz fails, please try git am -iu3 before marking does not apply, if that still fails, then that's fine. But a lot of the times it passes
Created attachment 9129 [details] [review] Bug 6413 Added ability to add a note when paying or writing off a fine Code will also respect notes when using the "Writeoff All" button but WILL NOT when using either the "Pay Amount" or "Pay Selected" buttons Fixed uri encoding of arguments
Created attachment 9144 [details] [review] [SIGNED-OFF] Bug 6413 Added ability to add a note when paying or writing off a fine Code will also respect notes when using the "Writeoff All" button but WILL NOT when using either the "Pay Amount" or "Pay Selected" buttons Fixed uri encoding of arguments Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com> Paid full fines, partial fines and wrote off fines - all with notes everything worked as expected.
A future dev for this would be to allow notes when using the buttons at the bottom .. but this works as explained and is a great step in the right direction!
Adds payment_note param to makepayment(), makepartialpayment() and WriteOffFee(). Additional param is at the end, and is safely added (defined to '' if nothing is passed) Introduces a new string to members/pay.tt... RM, your call as to whether that's a blocker for 3.8 or not. Otherwise, looks good, marking Passed QA.
Created attachment 9220 [details] [review] Added ability to add notes when paying or writing off fines I noticed that somehow a typo got merged into my patch that wasn't in my original version (if you look at the latest patch before mine it says "Account ype" whereas my patch says "Account Type"). Should apply cleanly against master (also fixed white space issue). Sorry to make you guys sign off again.
Created attachment 9221 [details] [review] [SIGNED-OFF] Bug 6413 Added ability to add a note when paying or writing off a fine Code will also respect notes when using the "Writeoff All" button but WILL NOT when using either the "Pay Amount" or "Pay Selected" buttons Fixed uri encoding of arguments Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
signed off on typo fix.
This patch changes some string, so i'm not inclined to push it for 3.8, plus there's a (tiny) failed QA problem: Payment Note => should be Payment note. I continue checking other patches that passed QA, will decide later if it's better to push it
(note that "Payment note" string does not exist yet in the translation tables, so it won't be translated if I push)
QA comment: if you use the 'pay amount' button, any comment you've entered is lost. I think this is quite inconsistent. Any explanation welcomed, maybe that's an ergonomic problem (mixing pay line by line, with comment added, and pay all at once is illogic) Marking failed QA for now, not that I refuse the patch, but fixes a problem and I think introduces another one...
Created attachment 9374 [details] [review] Bug 6413 follow-up: fixing capital case & tabs * switching from "Payment Note" to "Payment note" * replaced tabs by 4 spaces in lines just before and after this fix, for consistent indenting
I agree that when paying all or paying selected the notes should stay - but it's not a new bug as much as it's a feature that never existed. So I'd love to see that feature added :)
Hey Guys I agree that all the buttons should behave in a similar way and that this could be confusing. However for both pay amount and pay selected the way they are handled is quite differently to the other buttons, everything (either everything selected or everything) in the form is encoded into a one dimensional array that is then encoded as a single string and this is then passed back and forth between multiple scripts. This is quite messy to deal with and get right and would significantly increase the size and complexity of the patch.
Like I said, I'm with Chris, I don't think this should fail QA. The reason you're failing it is a feature that never existed, Chris's patch just fixes a feature that did exist and was broken.
(In reply to comment #13) > Did you remember to try applying it with a 3 way merge > > git bz is cool, but without the patch from paul, it doesnt try to merge. Just as a reminder, the patch is here: https://depot.biblibre.com/ppoulain/0001-Add-3-parameter-to-git-am.patch (otherwise, checking this patch again, to see if I was wrong refusing QA, or arguing a little bit more to justify)
After testing again, I confirm my failed QA: * add 2 manual invoices, say 1€ and 2€ * head to "Pay fine" (pay.pl) tab * you can: - check or uncheck a line - add a comment on each line * uncheck line 2€, keep line 1€ checked, add a note to line 1€ * clic "Pay selected": you are asked to pay 1€, if you save,the comment you've entered is not saved I also tried to reproduce the behaviour shown on http://screencast.com/t/bPYEhgtAj, but since the bug 3498 Allow partial payments of fines, things have changed, and I can't reproduce the buggy behaviour.
*** Bug 7004 has been marked as a duplicate of this bug. ***
Chris, Can you submit a new patch that will pass QA on this? Nicole
This patch no longer applies :(
Created attachment 12658 [details] [review] Bug 6413 Added ability to add a note when paying or writing off a fine Code will also respect notes when using the "Writeoff All" button but WILL NOT when using either the "Pay Amount" or "Pay Selected" buttons Fixed uri encoding of arguments Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
Created attachment 12661 [details] [review] Bug 6413 Added ability to add a note when paying or writing off a fine Code will also respect notes when using the "Writeoff All" button but WILL NOT when using either the "Pay Amount" or "Pay Selected" buttons Fixed uri encoding of arguments Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
Created attachment 12668 [details] [review] Bug 6413 follow-up: fixing capital case & tabs * switching from "Payment Note" to "Payment note" * replaced tabs by 4 spaces in lines just before and after this fix, for consistent indenting
Created attachment 12669 [details] [review] Bug 6413 Added ability to add a note when paying or writing off a fine Code will also respect notes when using the "Writeoff All" button but WILL NOT when using either the "Pay Amount" or "Pay Selected" buttons Fixed uri encoding of arguments Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
(In reply to comment #36) > Created attachment 12669 [details] [review] > Bug 6413 Added ability to add a note when paying or writing off a fine > > Code will also respect notes when using the "Writeoff All" button but WILL > NOT when using either the "Pay Amount" or "Pay Selected" buttons Fixed uri > encoding of arguments > > Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com> 2 patches test as expected, passing QA... $ koha-qa.pl -c 2 testing 2 commit(s) (applied to commit 6c18204) * c1c9900 Bug 6413 follow-up: fixing capital case & tabs koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt * 66cfbba Bug 6413 Added ability to add a note when paying or writing off a fine C4/Accounts.pm koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt koha-tmpl/intranet-tmpl/prog/en/modules/members/paycollect.tt members/pay.pl members/paycollect.pl * C4/Accounts.pm OK * members/pay.pl OK * members/paycollect.pl OK * koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt OK * koha-tmpl/intranet-tmpl/prog/en/modules/members/paycollect.tt OK
Sorry guys (& girls), but the problem I've explained in comment 29 is still here: (In reply to comment #29) > After testing again, I confirm my failed QA: > * add 2 manual invoices, say 1€ and 2€ > * head to "Pay fine" (pay.pl) tab > * you can: > - check or uncheck a line > - add a comment on each line > * uncheck line 2€, keep line 1€ checked, add a note to line 1€ > * clic "Pay selected": you are asked to pay 1€, if you save,the comment > you've entered is not saved I also see wrong HTML: + <td><input type="text" name="payment_note_[% line.accountno %]"></input></td> should be written <td><input type="text" name="payment_note_[% line.accountno %]" /></td> (self closed html tag)
Just a nudge - is anyone working on a new patch that addresses Paul's QA comments, by chance?
(In reply to comment #39) > Just a nudge - is anyone working on a new patch that addresses Paul's QA > comments, by chance? seems no...
Created attachment 14832 [details] [review] [SIGNED-OFF] Bug 6413 Added ability to add a note when paying or writing off a fine Code will also respect notes when using the "Writeoff All" button but WILL NOT when using either the "Pay Amount" or "Pay Selected" buttons Fixed uri encoding of arguments Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
Created attachment 14833 [details] [review] Bug 6413 - QA Followup
Is this what you were asking for? ^ (In reply to comment #40) > (In reply to comment #39) > > Just a nudge - is anyone working on a new patch that addresses Paul's QA > > comments, by chance? > > seems no...
Created attachment 14834 [details] [review] Bug 6413 follow-up: fixing capital case & tabs * switching from "Payment Note" to "Payment note" * replaced tabs by 4 spaces in lines just before and after this fix, for consistent indenting
Followup is super trivial, switching to signed-off. QA can decide if another SO is necessary.
Created attachment 14840 [details] [review] Bug 6413 - QA Followup - Add notes for 'Pay selected'
Created attachment 14846 [details] [review] Bug 6413 - QA Followup - Add notes for 'Pay selected'
Can someone provide a test plan for this? Can the last three patches be squashed?
Also, is the title of this bug accurate with regard to what the patches are for?
Created attachment 14895 [details] [review] [SIGNED-OFF] Bug 6413 follow-up: fixing capital case & tabs * switching from "Payment Note" to "Payment note" * replaced tabs by 4 spaces in lines just before and after this fix, for consistent indenting Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 14896 [details] [review] Bug 6413 - QA Followup - Add notes for 'Pay selected'
(In reply to comment #49) > Also, is the title of this bug accurate with regard to what the patches are > for? The middle one is from Paul, but I squashed my two together. Nicole mentioned the lack of the note field in her initial comment, but it might be better to change the title to something like "Add note fields for fine payments". Test plan: 1) Apply the patches 2) Create some test fees 3) Pick two or more to use for the 'Pay selected' option, add notes for each 4) Click 'pay selected' 5) The notes for each selected fee should be pulled into the new 'notes' text field on paycollect.pl. Here you can modify the imported notes if you wish before clicking "Confirm" 6) Confirm the note for the payment is now displayed on boraccount.pl for the payment.
I'm ready to sign off except for one small bug: The table columns are now out of sync with the footer. Sub total and total due should be aligned with the last column.
Created attachment 14899 [details] [review] Bug 6413 - QA Followup - Add notes for 'Pay selected'
Fixed! (In reply to comment #53) > I'm ready to sign off except for one small bug: The table columns are now > out of sync with the footer. Sub total and total due should be aligned with > the last column.
Created attachment 14900 [details] [review] [SIGNED-OFF] Bug 6413 - QA Followup - Add notes for 'Pay selected' Signed-off-by: Owen Leonard <oleonard@myacpl.org>
The patch conflicts now because the fix for recording payments in the logs. Kyle or Chris - can you please resolve the conflicts?
Created attachment 15232 [details] [review] Bug 6413 Added ability to add a note when paying or writing off a fine Code will also respect notes when using the "Writeoff All" button but WILL NOT when using either the "Pay Amount" or "Pay Selected" buttons Fixed uri encoding of arguments Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
Created attachment 15233 [details] [review] Bug 6413 follow-up: fixing capital case & tabs * switching from "Payment Note" to "Payment note" * replaced tabs by 4 spaces in lines just before and after this fix, for consistent indenting Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 15234 [details] [review] Bug 6413 - QA Followup - Add notes for 'Pay selected' Signed-off-by: Owen Leonard <oleonard@myacpl.org>
QA comment : * small conflict i've fixed * still passes koha-qa * the bugguy behaviour I saw in comment 29 is fixed * no other comment passed QA !
Created attachment 16486 [details] [review] [PASSED QA]Bug 6413 Added ability to add a note when paying or writing off a fine Code will also respect notes when using the "Writeoff All" button but WILL NOT when using either the "Pay Amount" or "Pay Selected" buttons Fixed uri encoding of arguments Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Created attachment 16487 [details] [review] [PASSED QA]Bug 6413 Added ability to add a note when paying or writing off a fine Code will also respect notes when using the "Writeoff All" button but WILL NOT when using either the "Pay Amount" or "Pay Selected" buttons Fixed uri encoding of arguments Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Created attachment 16488 [details] [review] [PASSED QA]Bug 6413 follow-up: fixing capital case & tabs * switching from "Payment Note" to "Payment note" * replaced tabs by 4 spaces in lines just before and after this fix, for consistent indenting Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Created attachment 16489 [details] [review] [PASSED QA]Bug 6413 - QA Followup - Add notes for 'Pay selected' Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
This patch breaks the table on the Pay fines tab: http://screencast.com/t/YgcrEjHqBCU
Created attachment 18035 [details] [review] Bug 6413 - QA Followup - Remove extra table column header Trivial fix, switching back to passed qa.
This patch has string changes and will therefore be held for 3.14.
Pushed to master. Thanks, Chris, Paul, and Kyle!