Video demonstration: http://screencast.com/t/Kqlja7z1F

Comment on attachment 24754 [details] [review]
Bug 11622 - Add ability to pay fees and fines from OPAC via PayPal

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

::: opac/opac-account-pay-paypal-return.pl
@@ +1,5 @@
> +#!/usr/bin/perl
> +
> +# Copyright Katipo Communications 2002
> +# Copyright Biblibre 2007,2010
> +#

This seems unlikely.

@@ +18,5 @@
> +# with Koha; if not, write to the Free Software Foundation, Inc.,
> +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> +
> +use strict;
> +use warnings;

Modern::Perl

@@ +20,5 @@
> +
> +use strict;
> +use warnings;
> +use utf8;
> +

I see no non-ASCII characters in the source.

::: opac/opac-account-pay.pl
@@ +1,4 @@
> +#!/usr/bin/perl
> +
> +# Copyright Katipo Communications 2002
> +# Copyright Biblibre 2007,2010

This seems unlikely.

@@ +18,5 @@
> +# with Koha; if not, write to the Free Software Foundation, Inc.,
> +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> +
> +use strict;
> +use warnings;

Modern::Perl

@@ +19,5 @@
> +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> +
> +use strict;
> +use warnings;
> +use utf8;

I see no non-ASCII characters in the source.

@@ +65,5 @@
> +
> +    my $opac_base_url = C4::Context->preference('OPACBaseURL');
> +
> +    my $return_url =
> +      URI->new( "http://"

This should be checking whether the OPAC is using SSL.

@@ +94,5 @@
> +        'PAYMENTREQUEST_0_PAYMENTACTION'        => 'Sale',
> +        'PAYMENTREQUEST_0_ALLOWEDPAYMENTMETHOD' => 'InstantPaymentOnly',
> +        'PAYMENTREQUEST_0_DESC' =>
> +          C4::Context->preference('PayPalChargeDescription'),
> +    };

It seems like using Business::PayPal::API might offer some protection against API changes.

fatal: sha1 information is lacking or useless (Koha/Accounts/CreditTypes.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 11622 - Add ability to pay fees and fines from OPAC via PayPa

(In reply to Galen Charlton from comment #5)
> Comment on attachment 24754 [details] [review] [review]
> Bug 11622 - Add ability to pay fees and fines from OPAC via PayPal
> 
> Review of attachment 24754 [details] [review] [review]:
> -----------------------------------------------------------------
[SNIP]
> 
> @@ +65,5 @@
> > +
> > +    my $opac_base_url = C4::Context->preference('OPACBaseURL');
> > +
> > +    my $return_url =
> > +      URI->new( "http://"
> 
> This should be checking whether the OPAC is using SSL.

Can someone please look at bug 5010 so we can avoid this mess of https/http concatentation/detection?

This latest version of this patch no longer depends on the accounts rewrite. I hadn't anticipated the length of time getting it into community Koha would take, so let's not hold up the opac paypal payments any longer!

(In reply to Galen Charlton from comment #5)
> Comment on attachment 24754 [details] [review] [review]
> Bug 11622 - Add ability to pay fees and fines from OPAC via PayPal
> 
> Review of attachment 24754 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: opac/opac-account-pay-paypal-return.pl
> @@ +20,5 @@
> > +
> > +use strict;
> > +use warnings;
> > +use utf8;
> > +

> Modern::Perl
> I see no non-ASCII characters in the source.

Still exists.

> ::: opac/opac-account-pay.pl
> @@ +19,5 @@
> > +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> > +
> > +use strict;
> > +use warnings;
> > +use utf8;

> Modern::Perl
> I see no non-ASCII characters in the source.

Still exists.

> @@ +94,5 @@
> > +        'PAYMENTREQUEST_0_PAYMENTACTION'        => 'Sale',
> > +        'PAYMENTREQUEST_0_ALLOWEDPAYMENTMETHOD' => 'InstantPaymentOnly',
> > +        'PAYMENTREQUEST_0_DESC' =>
> > +          C4::Context->preference('PayPalChargeDescription'),
> > +    };
> 
> It seems like using Business::PayPal::API might offer some protection
> against API changes.

Answer needed.


+use CGI;

Should be replaced by CGI qw( -utf8 ) unless you have a good reason.


> 3) Create a paypal developer account

How? Should I need a paypal account to do so?

(In reply to Jonathan Druart from comment #14)
> (In reply to Galen Charlton from comment #5)
> > Comment on attachment 24754 [details] [review] [review] [review]
> > Bug 11622 - Add ability to pay fees and fines from OPAC via PayPal
> > 
> > Review of attachment 24754 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: opac/opac-account-pay-paypal-return.pl
> > @@ +20,5 @@
> > > +
> > > +use strict;
> > > +use warnings;
> > > +use utf8;
> > > +
> 
> > Modern::Perl
> > I see no non-ASCII characters in the source.
> 
> Still exists.
> 

I'm not sure what this means. Can you explain?


> > ::: opac/opac-account-pay.pl
> > @@ +19,5 @@
> > > +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> > > +
> > > +use strict;
> > > +use warnings;
> > > +use utf8;
> 
> > Modern::Perl
> > I see no non-ASCII characters in the source.
> 
> Still exists.

Same as above

 
> > @@ +94,5 @@
> > > +        'PAYMENTREQUEST_0_PAYMENTACTION'        => 'Sale',
> > > +        'PAYMENTREQUEST_0_ALLOWEDPAYMENTMETHOD' => 'InstantPaymentOnly',
> > > +        'PAYMENTREQUEST_0_DESC' =>
> > > +          C4::Context->preference('PayPalChargeDescription'),
> > > +    };
> > 
> > It seems like using Business::PayPal::API might offer some protection
> > against API changes.
> 
> Answer needed.

That's what I tried first when I began writing this feature. It did not go well. PayPal doesn't change existing APIs anyway. They have an API versioning system so that applications don't break with changes to the API.
 
> 
> +use CGI;
> 
> Should be replaced by CGI qw( -utf8 ) unless you have a good reason.
> 

Fixed!

> 
> > 3) Create a paypal developer account
> 
> How? Should I need a paypal account to do so?

I don't think you need a regular paypal account, but you do need an account on https://developer.paypal.com/

(In reply to Kyle M Hall from comment #17)
> (In reply to Jonathan Druart from comment #14)
> > (In reply to Galen Charlton from comment #5)
> > > Comment on attachment 24754 [details] [review] [review] [review] [review]
> > > Bug 11622 - Add ability to pay fees and fines from OPAC via PayPal
> > > 
> > > Review of attachment 24754 [details] [review] [review] [review] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: opac/opac-account-pay-paypal-return.pl
> > > @@ +20,5 @@
> > > > +
> > > > +use strict;
> > > > +use warnings;
> > > > +use utf8;
> > > > +
> > 
> > > Modern::Perl
> > > I see no non-ASCII characters in the source.
> > 
> > Still exists.
> > 
> 
> I'm not sure what this means. Can you explain?

1/ use strict;use warnings; should be replaced with use Modern::Perl;
2/ You are using the utf8 pragma but there is no need of it, you don't have non-ASCII characters in the file.

> 1/ use strict;use warnings; should be replaced with use Modern::Perl;
> 2/ You are using the utf8 pragma but there is no need of it, you don't have
> non-ASCII characters in the file.

I see what you mean. Thanks!

1/ Trying to generate a debian packe for URL::Encode fails:

% dh-make-perl --pkg-perl --build --cpan URL::Encode
  [skip output]
% sudo dpkg -i liburl-encode-perl_0.03-1_all.deb
(Reading database ... 66746 files and directories currently installed.)
Preparing to unpack liburl-encode-perl_0.03-1_all.deb ...
Unpacking liburl-encode-perl (0.03-1) ...
dpkg: error processing archive liburl-encode-perl_0.03-1_all.deb (--install):
 trying to overwrite '/home/koha/perl5/lib/perl5/i586-linux-gnu-thread-multi-64int/perllocal.pod', which is also in package libtemplate-plugin-stash-perl 1.006-1
Errors were encountered while processing:
 liburl-encode-perl_0.03-1_all.deb


2/ Testing the feature:
click on "Make payment", I am redirected to the login paypal page, then I arrived on:
===
Modes de paiement
Mode de paiement
Solde PayPal
Taux de change PayPal au 17 sep 2015 : 1 Euro = 1,29364 Dollars USD
Carte cadeau, récompense ou autre remise PayPal
Coordonnées
jo.dru.pro-buyer@gmail.com
Vous avez presque terminé. Vous confirmerez votre paiement sur my library.
===
Sorry, in French...
But globally, alright, give me the money! (But how much??)

Then click continue and I am redirected to my Koha install with this message:

Error: there was an problem processing your payment
Unable to verify payment.
Please contact the library to verify your payment.

> 2/ Testing the feature:
> click on "Make payment", I am redirected to the login paypal page, then I
> arrived on:
> ===
> Modes de paiement
> Mode de paiement
> Solde PayPal
> Taux de change PayPal au 17 sep 2015 : 1 Euro = 1,29364 Dollars USD
> Carte cadeau, récompense ou autre remise PayPal
> Coordonnées
> jo.dru.pro-buyer@gmail.com
> Vous avez presque terminé. Vous confirmerez votre paiement sur my library.
> ===
> Sorry, in French...
> But globally, alright, give me the money! (But how much??)
> 
> Then click continue and I am redirected to my Koha install with this message:
> 
> Error: there was an problem processing your payment
> Unable to verify payment.
> Please contact the library to verify your payment.

Looks like the utf8 flag for cgi was breaking the feature. I've updated and squashed the followups.

Ok thanks, it's fixed.
But the amount is *never* displayed unless at the end of the payment process "Payment applied: your payment of 4.00 has been applied to your account".

It would be good to inform the patron how much he is going to pay :)

Something else: Cannot we avoid the screen opac-account-pay-paypal-return.pl? We would imagine the get the confirmation message on top of the fines page (opac-account.pl) and save 1 click :)

And what about the debian package build?

+ Execute the QA script on this patch set, there are 2 warnings.

The phrase "Payment applied: your payment of payment_error has been applied to your account" displays on the fines and charges page before you actually make a payment.

There are 2 major issues:

1/ The amount is not updated and the "Make payment" button is always disabled if javascript is disabled.
I think we should not allow to pay via Paypal (ie. not display "Pay selected fines and charges" block) in this case.

2/ The currency is never displayed and the user does not know how much he is going to pay.
From my testing, the currency used seems to be USD, even if my Paypal balance is in EUR. I did not find where we could change that, but this behavior is not acceptable for users outside the USD zone.

> 1/ The amount is not updated and the "Make payment" button is always
> disabled if javascript is disabled.
> I think we should not allow to pay via Paypal (ie. not display "Pay selected
> fines and charges" block) in this case.

Agreed. Followup added!
 
> 2/ The currency is never displayed and the user does not know how much he is
> going to pay.
> From my testing, the currency used seems to be USD, even if my Paypal
> balance is in EUR. I did not find where we could change that, but this
> behavior is not acceptable for users outside the USD zone.

Also agreed! The followup for this one worked for me using USD. Please give it a try!

Unfortunately, it does not work.
warn $response->decoded_content logs

opac-account-pay-paypal-return.pl: TIMESTAMP=2015%2d09%2d22T16%3a57%3a10Z&CORRELATIONID=e76fcad1465da&ACK=Failure&VERSION=82%2e0&BUILD=000000&L_ERRORCODE0=10444&L_SHORTMESSAGE0=Transaction%20refused%20because%20of%20an%20invalid%20argument%2e%20See%20additional%20error%20messages%20for%20details%2e&L_LONGMESSAGE0=The%20transaction%20currency%20specified%20must%20be%20the%20same%20as%20previously%20specified%2e&L_SEVERITYCODE0=Error at /home/koha/src/opac/opac-account-pay-paypal-return.pl line 84.

I also think that the Payment block should not be displayed if there is nothing to pay.

(In reply to Jonathan Druart from comment #37)
> Unfortunately, it does not work.
> warn $response->decoded_content logs
> 
> opac-account-pay-paypal-return.pl:
> TIMESTAMP=2015%2d09%2d22T16%3a57%3a10Z&CORRELATIONID=e76fcad1465da&ACK=Failur
> e&VERSION=82%2e0&BUILD=000000&L_ERRORCODE0=10444&L_SHORTMESSAGE0=Transaction%
> 20refused%20because%20of%20an%20invalid%20argument%2e%20See%20additional%20er
> ror%20messages%20for%20details%2e&L_LONGMESSAGE0=The%20transaction%20currency
> %20specified%20must%20be%20the%20same%20as%20previously%20specified%2e&L_SEVE
> RITYCODE0=Error at /home/koha/src/opac/opac-account-pay-paypal-return.pl
> line 84.

Fixed! Paypal wants the currency code both and initiation and at verification.

> I also think that the Payment block should not be displayed if there is
> nothing to pay.

Done!

Note for RM: Since only 2 patches have a signoff, feel free to ask for another one on the whole patch set, if you think it's needed.

(In reply to Kyle M Hall from comment #1)
> Created attachment 24753 [details] [review] [review]
> Bug 11622 - Add ability to pay fees and fines from OPAC via PayPal
> 
> This patch adds the ability for a logged in user to pay fines and
> fees from the OPAC via PayPal.

Kyle, if you're adding a dependency, you need to read this:

http://wiki.koha-community.org/wiki/Building_Debian_Dependencies/Dependency_Guidelines

or the email equivalent that went out in May: 

http://lists.koha-community.org/pipermail/koha-devel/2015-May/041464.html

This won't be able to go into master until I package URL::Encode and Debian allows it to be added, which may (or may not) take some time.

Would URI::Escape work, which is already a Koha dependency?

https://metacpan.org/pod/URI::Escape

(In reply to Robin Sheat from comment #61)
> Would URI::Escape work, which is already a Koha dependency?
> 
> https://metacpan.org/pod/URI::Escape

I'm afraid not. The note only uses url_encode, but also url_params_mixed which URI::Escape does not provide.

Unaware of this patch, I made Bug 15335 which is very similar, with the differences that Bug 15335 allows patron to select which fines to pay (i.e. as a patron you don't have to pay all your fines at once) and uses DIBS as a payment provider instead of Paypal.

do you think it's worth merging these patches to get the best of both?

(In reply to Martin Stenberg from comment #63)
> Unaware of this patch, I made Bug 15335 which is very similar, with the
> differences that Bug 15335 allows patron to select which fines to pay (i.e.
> as a patron you don't have to pay all your fines at once) and uses DIBS as a
> payment provider instead of Paypal.
> 
> do you think it's worth merging these patches to get the best of both?

Sure! I would recommend you add this bug as a 'depends on' for bug 15335. After that you can adjust your patch set to work on top of this patch set.

Created attachment 46143 [details] [review]
Bug 11622 - Add ability to pay fees and fines from OPAC via PayPal

This patch adds the ability for a logged in user to pay fines and
fees from the OPAC via PayPal.

Test Plan:
 1) Apply this patch
 2) Create a paypal developer account
 3) Create two test accounts, a Personal account and a Business account
 4) Enable PayPal in Sandbox mode via the system preferences.
 5) Enter the business account API credentials into the new system
    preferences.
 6) Create a new patron, add some fines/fees
 7) Log in as that patron in the OPAC
 8) Choose to pay via PayPal, log in as the sandbox Personal account
 9) Complete the transaction
10) Note the fee is now paid

Signed-off-by: Carol Corrales <ccorrales@losgatosca.gov>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 46144 [details] [review]
Bug 11622 [QA Followup]

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 46145 [details] [review]
Bug 11622 [QA Followup] - Show amount to be paid near make payment button

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 46146 [details] [review]
Bug 11622 [QA Followup] - Display payment messages directly on opac account view

Fixed spelling of "Receive" in preferences.

Signed-off-by: David Kuhn <techservspec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 46147 [details] [review]
Bug 11622 [QA Followup] - Payment message always displays on opac-account.pl

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 46148 [details] [review]
Bug 11622 [QA Followup] - Use Koha's active currency for PayPal

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 46149 [details] [review]
Bug 11622 [QA Followup] - Show ability to pay only if JavaScript is enabled

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 46150 [details] [review]
Bug 11622 [QA Followup] - Fix currency issue, add error template

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 46151 [details] [review]
Bug 11622 [QA Follouwp] - Display payment block only if there is something to pay

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Pushed to master!

I had pushed this to Master.  But there is an issue with the URL::Encode... 

An Assist from Galen suggests.

Please see http://stackoverflow.com/a/9690930, which lists some various ways of handling application/x-www-form-urlencoded content, several of which make use of modules that are already required by Koha.

Regards,

Galen 

Can we fix that up?  This will be a candidate for reverting if we don't solve the package issue.

Thanks,
Brendan

(In reply to Brendan Gallagher from comment #75)
> I had pushed this to Master.  But there is an issue with the URL::Encode... 
> 
> An Assist from Galen suggests.
> 
> Please see http://stackoverflow.com/a/9690930, which lists some various ways
> of handling application/x-www-form-urlencoded content, several of which make
> use of modules that are already required by Koha.
> 
> Regards,
> 
> Galen 
> 
> Can we fix that up?  This will be a candidate for reverting if we don't
> solve the package issue.
> 
> Thanks,
> Brendan

I've looked into that article and the only reasonable solution is to use URL::Encode. The good news is that the only dependency that URL::Encode has is Carp, so it should be very easy to package. Now we just need an easy to follow guide for the packaging ; )

*** Bug 15335 has been marked as a duplicate of this bug. ***