Bug 5620

Summary: Capture Mode of payment
Product: Koha Reporter: Amit Gupta <kohapatch>
Component: PatronsAssignee: David Bourgault <david.bourgault>
Status: RESOLVED DUPLICATE QA Contact: Bugs List <koha-bugs>
Severity: enhancement    
Priority: P5 - low CC: alexbuckley, bouzid.fergani, charles.farmer, chris, david.bourgault, eric.begin, francois.charbonnier, gmcharlt, jonathan.druart, josef.moravec, katrin.fischer, kyle, liz, martin.renvoize, mtompset, philippe.blouin, rbit, veron
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13985
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug-5620 Capture Mode of payment, receipt number and notes in pay fines.
bug 5620 - Capture Mode of payment, receipt number and notes in pay fines.
bug 5620 - Capture Mode of payment, receipt number and notes in pay fines.
bug 5620 - Capture Mode of payment, receipt number and notes in pay fines.
Bug5620 - Add tests for sub GetKohaAuthorisedValues
bug 5620 - Capture Mode of payment, receipt number and notes in pay fines.
bug 5620 - Capture Mode of payment, receipt number and notes in pay fines.
Bug 5620 - Patch corrections, fixes to template files
Bug 5620 - Capture mode of payment.
Bug 5620 - Capture mode of payment in accounttype
Bug 5620 - Rebase and add Paypal to staff interface
Bug 5620 - (Rebase) - Collect payment mode in staff interface
Bug 5620 - (Rebase) - Collect payment mode in staff interface
Bug 5620 - (Rebase) - Collect payment mode in staff interface

Description Amit Gupta 2011-01-17 06:23:03 UTC
Capture Mode of payment, receipt number and notes in pay fines.
Comment 1 Amit Gupta 2011-01-17 10:59:24 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2013-01-01 22:48:30 UTC
This needs a rewrite, can't be rebased, it uses HTML::Template::Pro not Template::Toolkit
Comment 3 Eric Bégin 2013-10-03 17:22:25 UTC
It would be nice if an authorized value (PAYMODE?) was used in order to add other mode of payment
Comment 4 Eric Bégin 2014-07-17 17:38:42 UTC
Kyle, any plan to work on this feature ? We have something similar working for 3.14 that we could adapt for the master
Comment 5 Rémi Mayrand-Provencher 2016-05-06 13:22:20 UTC
Created attachment 51290 [details] [review]
bug 5620 - Capture Mode of payment, receipt number and notes in pay fines.
Comment 6 Mark Tompsett 2016-05-27 01:14:32 UTC
Comment on attachment 51290 [details] [review]
bug 5620 - Capture Mode of payment, receipt number and notes in pay fines.

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

::: C4/Accounts.pm
@@ +89,4 @@
>  sub recordpayment {
>  
>      #here we update the account lines
> +    my ( $borrowernumber, $data, $sip_paytype, $payment_note, $paymentmode ) = @_;

"PERL16: Hashrefs should be used as arguments"
Perhaps refactor is in order? There are already too many parameters.

::: installer/data/mysql/atomicupdate/bug5620_Add_Mode_Of_Payment.perl
@@ +10,5 @@
> +$dbh->do("alter table accountlines add column paymentmode text not null");
> +$dbh->do("insert into authorised_values (category, authorised_value, lib, lib_opac) values('PAYMODE','CASH','Cash', 'Cash')");
> +$dbh->do("insert into authorised_values (category, authorised_value, lib, lib_opac) values('PAYMODE','CC','Credit Card', 'Credit Card')");
> +$dbh->do("insert into authorised_values (category, authorised_value, lib, lib_opac) values('PAYMODE','DEBIG','Debit', 'Debit')");
> +print "bug5620 - add mode of payment";

Why not do this as a sql file?
Also, missing kohastructure.sql patch.

::: members/paycollect.pl
@@ +50,5 @@
> +my $sth = $dbh->prepare('SELECT * FROM authorised_values WHERE category = "PAYMODE"');
> +$sth->execute();
> +while ( my $row = $sth->fetchrow_hashref() ) {
> +    push @paymentmodes, $row;
> +}

This should probably be some nice function in some authorised value library somewhere.
Comment 7 Liz Rea 2016-09-12 02:33:12 UTC
FQA based on Marc's comment.
Comment 8 Bouzid Fergani 2016-10-04 14:16:18 UTC
Created attachment 56036 [details] [review]
bug 5620 - Capture Mode of payment, receipt number and notes in pay fines.

   Using sql file in atomiqueupdate
   using function GetKohaAuthorisedValues
Comment 9 Bouzid Fergani 2016-10-04 14:17:34 UTC
Created attachment 56037 [details] [review]
bug 5620 - Capture Mode of payment, receipt number and notes in pay fines.

Using sql file in atomiqueupdate
   using function GetKohaAuthorisedValues
Comment 10 Jonathan Druart 2016-10-05 07:34:17 UTC
The status is needs signoff when you want someone to test your patch!
Comment 11 Jonathan Druart 2016-10-05 07:37:08 UTC
And the status is failed qa and a patch modifies code without any test coverage.
You should also have a look at bug 17248 and bug 15894.
Comment 12 Bouzid Fergani 2016-10-07 16:41:53 UTC
Created attachment 56080 [details] [review]
Bug5620 - Add tests for sub GetKohaAuthorisedValues
Comment 13 Kyle M Hall 2017-02-14 17:53:46 UTC
I want to start by stating that I love this feature : )

That said, there is one issue I think needs fixed. Instead of getting the authorised values in the script and passing to the template ( and altering a subroutine ) you should just use Koha::Template::Plugin::AuthorisedValues directly from the template.
Comment 14 Blou 2017-02-14 18:37:53 UTC
ha! Those young, hip, qa-ers...

When I was young, we walked barefoot in snowstorms to get to the public library, all the books were engraved on stone tablets, and those swell little plugin libraries didn't exist.
Comment 15 Bouzid Fergani 2017-03-10 15:26:35 UTC Comment hidden (obsolete)
Comment 16 Bouzid Fergani 2017-03-10 15:33:53 UTC
Created attachment 60996 [details] [review]
bug 5620 - Capture Mode of payment, receipt number and notes in pay fines.

- Replace all sub deleted
    - Using Koha::Template::Plugin::AuthorisedValues
    - Delete unit tests
Comment 17 Katrin Fischer 2017-05-01 16:05:55 UTC
Hi all, 

some thoughts while looking at this. 

At the moment we already have a way to display the mode of payment when the fine is paid via SIP. In this case different accounttype values are used:

From pay.tt:
[% SWITCH line.accounttype %]
 [% CASE 'Pay' %]Payment, thanks
 [% CASE 'Pay00' %]Payment, thanks (cash via SIP2)
 [% CASE 'Pay01' %]Payment, thanks (VISA via SIP2)
 [% CASE 'Pay02' %]Payment, thanks (credit card via SIP2)
...

I think we should try and not have 2 separate ways of doing this depending on payment at the desk or at the self check as this will lead to more confusion and also make it harder to wrote reports etc.

We could also discuss if this should be an authorised value or not. The advantage is, that the library can change it. The disadvantage is that the disadvantage is, that we can only display the values in one langauge at the moment. With the accounttype approach, we'd have to agree on a set of payment methods, but those would be translatable.

I am changing this to 'in Discussion'. Please note that this does also currently not apply.
Comment 18 David Bourgault 2017-09-26 14:59:05 UTC
Created attachment 67361 [details] [review]
Bug 5620 - Patch corrections, fixes to template files

Patch should now apply
Small fixes to use of template plugin AuthorisedValues
Comment 19 David Bourgault 2017-09-26 15:00:13 UTC
Comment on attachment 67361 [details] [review]
Bug 5620 - Patch corrections, fixes to template files

Woops, this didn't work
Comment 20 David Bourgault 2017-09-26 15:04:04 UTC
Created attachment 67362 [details] [review]
Bug 5620 - Capture mode of payment.

Correction to template files
Patch is rebased, should now apply.
Comment 21 David Bourgault 2017-09-26 15:08:07 UTC
I think using authorised values is the best option here, as it allows the library to customize the payment modes.

As for traduction, I think very few library actually support multiple languages at once in their production servers, so this would be a very minor inconvenience.
Comment 22 Katrin Fischer 2017-09-26 22:08:37 UTC
(In reply to David Bourgault from comment #21)
> I think using authorised values is the best option here, as it allows the
> library to customize the payment modes.
> 
> As for traduction, I think very few library actually support multiple
> languages at once in their production servers, so this would be a very minor
> inconvenience.

I respectfully disagree here. - it's by far not just a few.
Comment 23 Chris Cormack 2017-09-26 22:11:30 UTC
Yep, a lot do. And more should, so lets not make it harder
Comment 24 Jonathan Druart 2017-09-27 13:02:43 UTC
We should at least make translatable the 3 choices you add by default.
Comment 25 David Bourgault 2017-09-27 13:08:25 UTC
Didn't know it was that frequent... Like Karin suggested, the next option is to use hardcoded options in accountypes. In addition to the ones existing (* via SIP2) I propose the following:

Pay03 : Payment, thanks (cash)
Pay04 : Payment, thanks (credit card)
Pay05 : Payment, thanks (debit)
Pay06 : Payment, thanks (check)
Pay07 : Payment, thanks (bitcoin)

Would that be ok? I'll attach a new patch today.
Comment 26 David Bourgault 2017-09-27 14:30:27 UTC
Created attachment 67394 [details] [review]
Bug 5620 - Capture mode of payment in accounttype

Means of payment are now captured into accountlines.accounttype. Staff users can select the mode in a dropdown menu in paycollect.tt.
OPAC Paypal payment have been changed from 'note=Paypal" to mode=08

To reuse SIP logic, payment modes are represented as two digits codes, appended to the 'Pay' string. Codes are mapped as follows

00 : cash via SIP2
01 : credit card via SIP2
02 : visa via SIP2
03 : cash
04 : credit card
05 : debit
06 : check
07 : cryptocurrency
08 : paypal

00 - 02 are already in master, this patches only adds 03 - 08. Payments with no specified mode default to 'Pay', which is displayed as "Payment, thanks".

Test plan:
0) Apply patch
1) Visit a test patron's profile in the staff interface
2) Add an invoice if patron has no fines
3) In the pay fines tab, pay off either all, selected or individual (they should all work)
4) Select a payment mode in the dropdown menu when asked
5) Check that payment mode is correctly displayed in staff interface and OPAC user's profile ('your fines')

This obsoletes previous patches because the solution is completly different.
Comment 27 Alex Buckley 2017-10-04 01:13:47 UTC
Hi David 

This patch looks really good but one point that needs to be fixed is that on the paycollect.tt page there is no 'PayPal' option in the dropdown box however you have got a case statement for displaying 'Payment, thanks (PayPal)' in the boraccount.tt and opac-account.tt. Can you please add the 'PayPal' option in the paycollect.tt dropdown input. Thanks
Comment 28 Katrin Fischer 2017-10-04 05:51:11 UTC
Maybe it's excluded on purpose like the SIP payment methods as you are not supposed to set it manually? That would make sense to me.
Comment 29 David Bourgault 2017-10-04 12:28:22 UTC
Yes, I excluded it because usually PayPal payments are done through the OPAC. Should it be added to the staff interface anyway?
Comment 30 Josef Moravec 2017-10-04 12:38:42 UTC
(In reply to David Bourgault from comment #29)
> Yes, I excluded it because usually PayPal payments are done through the
> OPAC. Should it be added to the staff interface anyway?

Maybe it does make sense for some administration purposes?
Comment 31 David Bourgault 2017-10-04 13:37:33 UTC
Created attachment 67603 [details] [review]
Bug 5620 - Rebase and add Paypal to staff interface

Previous patch failed to apply for me, so I remade it. 

+ Paypal should now be an option in the staff interface when collecting from patrons.
Comment 32 Alex Buckley 2017-10-04 22:56:34 UTC
Hi David

Testing this patch again it fails to apply for me, the issue is in members/paycollect.pl, can you please rebase against master and then I will be happy to retest:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 5620 - Rebase and add Paypal to staff interface
/home/vagrant/kohaclone/.git/rebase-apply/patch:155: trailing whitespace.
                Koha::Account->new( { patron_id => $borrowernumber } )->pay( 
/home/vagrant/kohaclone/.git/rebase-apply/patch:156: trailing whitespace.
                    { 
/home/vagrant/kohaclone/.git/rebase-apply/patch:157: trailing whitespace.
                        amount => $total_paid, 
/home/vagrant/kohaclone/.git/rebase-apply/patch:160: trailing whitespace.
                    } 
warning: 4 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-account.tt
M	members/paycollect.pl
<stdin>:155: trailing whitespace.
                Koha::Account->new( { patron_id => $borrowernumber } )->pay( 
<stdin>:156: trailing whitespace.
                    { 
<stdin>:157: trailing whitespace.
                        amount => $total_paid, 
<stdin>:160: trailing whitespace.
                    } 
warning: 4 lines applied after fixing whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging members/paycollect.pl
CONFLICT (content): Merge conflict in members/paycollect.pl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-account.tt
Failed to merge in the changes.
Patch failed at 0001 Bug 5620 - Rebase and add Paypal to staff interface
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
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-5620---Rebase-and-add-Paypal-to-staff-interfac-SeWTyY.patch
Comment 33 David Bourgault 2017-10-05 12:35:21 UTC
Created attachment 67634 [details] [review]
Bug 5620 - (Rebase) - Collect payment mode in staff interface

Rebase of previous patch, nothing changed. Same test plan as before.
Comment 34 Alex Buckley 2017-10-05 22:39:39 UTC
Created attachment 67661 [details] [review]
Bug 5620 - (Rebase) - Collect payment mode in staff interface

Followed test plan, patch worked as described. Also ran QA test tool
everything passed

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 35 David Bourgault 2017-11-07 16:49:23 UTC
Created attachment 69002 [details] [review]
Bug 5620 - (Rebase) - Collect payment mode in staff interface

Followed test plan, patch worked as described. Also ran QA test tool
everything passed

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 36 David Bourgault 2017-11-07 16:50:48 UTC
Rebased on master, since patch did not apply. The corrections were trivial so I will not change the status.
Comment 37 Katrin Fischer 2017-11-15 20:00:38 UTC
Bug 18786 seems to be a duplicate development
Comment 38 Charles Farmer 2017-12-28 19:28:11 UTC
Is there any consensus on how to move this bug forward?

One of the implementation touches on SIP, the other does not. Are we sure they're really duplicates?
Comment 39 Kyle M Hall 2018-01-16 19:49:19 UTC
(In reply to Charles Farmer from comment #38)
> Is there any consensus on how to move this bug forward?
> 
> One of the implementation touches on SIP, the other does not. Are we sure
> they're really duplicates?

I don't consider these bugs to be duplicates. The big issue I think we have with this one is that it hard codes a set up SIP payment values that are not in the SIP2 protocol specification.
Comment 40 Katrin Fischer 2018-03-12 15:04:53 UTC
Comment on attachment 69002 [details] [review]
Bug 5620 - (Rebase) - Collect payment mode in staff interface

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

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/paycollect.tt
@@ +156,5 @@
> +            <option value="04">Credit card</option>
> +            <option value="05">Debit</option>
> +            <option value="08">Paypal</option>
> +            <option value="06">Check</option>
> +            <option value="07">Cryptocurrency</option>\

The dash here at the end is superfluous.
Comment 41 Katrin Fischer 2018-03-12 15:12:16 UTC
I've tested paying in different variations, but it only appears to work for one option:

1) Pay button next to fine = pull down works
2) Pay selected = doesn't offer the pull down (FAIL)
3) Pay amount = doesn't offer the pull down (FAIL)

Am I confusing something here?

Other things:

- account_offsets only lists "Payment" - should we list the type of payment there? What about partial payments where one part is paid with one mode of payment and the other part with another?
- Should we consider preselecting Cash or make Other = Not specified or similar?
Comment 42 David Bourgault 2018-04-12 14:38:27 UTC
I'm not sure what to do with this bug.
Bug 18786 achieves what I tried to do, but uses AV which I tried to avoid after Katrin's comment

Comment 17
>At the moment we already have a way to display the mode of payment when the fine is paid via SIP. In this case different accounttype values are used:
>[...]
>We could also discuss if this should be an authorised value or not. The advantage is, that the library can change it. The disadvantage is that the disadvantage is, that we can only display the values in one langauge at the moment. With the accounttype approach, we'd have to agree on a set of payment methods, but those would be translatable.

and the follow-up in Comment 22, 23 and 24 saying that AVs would not be a good choice since they need to be translatable.

I tried using SIP-like values (PayXX) not in an effort to be SIP-compliant but to keep the code uniform. As it stands, I don't consider this to be an improvement to Koha's SIP2 implentation since the values are not standard (the protocol only defines 00,01,02).

I don't plan on updating this patch since Bug 18786 does the same thing, unless we decide that we really don't want to use AVs.
Comment 43 Martin Renvoize 2019-03-29 16:35:20 UTC

*** This bug has been marked as a duplicate of bug 18786 ***