Bug 15907 - Remove use of makepayment in opac/opac-account-pay-paypal-return.pl
Summary: Remove use of makepayment in opac/opac-account-pay-paypal-return.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 15897
Blocks: 15905
  Show dependency treegraph
 
Reported: 2016-02-24 15:57 UTC by Kyle M Hall
Modified: 2017-12-07 22:16 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15907 - Remove use of makepayment in opac/opac-account-pay-paypal-return.pl (1.45 KB, patch)
2016-02-24 16:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15907 - Remove use of makepayment in opac/opac-account-pay-paypal-return.pl (1.45 KB, patch)
2016-06-07 15:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15907 - Remove use of makepayment in opac/opac-account-pay-paypal-return.pl (1.58 KB, patch)
2016-11-04 13:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15907 [QA Followup] - Simplify code and call pay() only once (1.46 KB, patch)
2017-01-11 14:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15907 - Remove use of makepayment in opac/opac-account-pay-paypal-return.pl (1.65 KB, patch)
2017-01-11 16:07 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15907 [QA Followup] - Simplify code and call pay() only once (1.53 KB, patch)
2017-01-11 16:07 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15907 - Remove use of makepayment in opac/opac-account-pay-paypal-return.pl (1.81 KB, patch)
2017-01-11 18:25 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2016-02-24 15:57:09 UTC

    
Comment 1 Kyle M Hall 2016-02-24 16:00:06 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2016-06-07 15:12:26 UTC
Created attachment 52149 [details] [review]
Bug 15907 - Remove use of makepayment in opac/opac-account-pay-paypal-return.pl

Test Plan:
1) Apply this patch
2) Make a payment via PayPal in sandbox mode
3) Note the payment succeeds
Comment 3 Josef Moravec 2016-10-13 11:30:50 UTC
Kyle, I think there is amount missing in calling Koha::Account->pay

I tried via paypal sandbox and the pay accountline was added, but with amount of 0
Comment 4 Josef Moravec 2016-11-04 09:26:31 UTC
Could you please look at this? It should be trivial to fix it (I hope), but better done by you, as I can then test and sign-off and not have to wait to another tester... Thanks!
Comment 5 Kyle M Hall 2016-11-04 13:32:50 UTC
Created attachment 57194 [details] [review]
Bug 15907 - Remove use of makepayment in opac/opac-account-pay-paypal-return.pl

Test Plan:
1) Apply this patch
2) Make a payment via PayPal in sandbox mode
3) Note the payment succeeds
Comment 6 Kyle M Hall 2016-11-04 13:34:47 UTC
(In reply to Josef Moravec from comment #4)
> Could you please look at this? It should be trivial to fix it (I hope), but
> better done by you, as I can then test and sign-off and not have to wait to
> another tester... Thanks!

Good catch!
Comment 7 Josef Moravec 2016-11-04 14:56:57 UTC
I didn't noticed before, but as you are passing account lines to pay sub as list, it looks this is based on bug 15897, which does need rebase (so this will probably too)
Comment 8 Marcel de Rooy 2016-11-04 16:19:10 UTC
Please adjust this parameter:

+                    lines  => [$line],

And check the comment on bug 15986 on the paypal script.
Comment 9 Marcel de Rooy 2016-11-04 16:19:43 UTC
Sorry 15896
Comment 10 Kyle M Hall 2016-12-09 18:11:35 UTC
(In reply to Marcel de Rooy from comment #8)
> Please adjust this parameter:
> 
> +                    lines  => [$line],
> 
> And check the comment on bug 15986 on the paypal script.

Not sure what you mean by this. Koha::Account::Pay accepts a 'lines' parameter.
Comment 11 Marcel de Rooy 2016-12-12 07:36:34 UTC
(In reply to Kyle M Hall from comment #10)
> (In reply to Marcel de Rooy from comment #8)
> > Please adjust this parameter:
> > 
> > +                    lines  => [$line],
> > 
> > And check the comment on bug 15986 on the paypal script.
> 
> Not sure what you mean by this. Koha::Account::Pay accepts a 'lines'
> parameter.

Could you please double-check?
I cannot find it.
Comment 12 Josef Moravec 2016-12-13 11:01:43 UTC
(In reply to Marcel de Rooy from comment #11)
> (In reply to Kyle M Hall from comment #10)
> > (In reply to Marcel de Rooy from comment #8)
> > > Please adjust this parameter:
> > > 
> > > +                    lines  => [$line],
> > > 
> > > And check the comment on bug 15986 on the paypal script.
> > 
> > Not sure what you mean by this. Koha::Account::Pay accepts a 'lines'
> > parameter.
> 
> Could you please double-check?
> I cannot find it.

It was added by bug 15897 I think
Comment 13 Marcel de Rooy 2016-12-13 13:17:17 UTC
(In reply to Josef Moravec from comment #12)
> (In reply to Marcel de Rooy from comment #11)
> > (In reply to Kyle M Hall from comment #10)
> > > (In reply to Marcel de Rooy from comment #8)
> > > > Please adjust this parameter:
> > > > 
> > > > +                    lines  => [$line],
> > > > 
> > > > And check the comment on bug 15986 on the paypal script.
> > > 
> > > Not sure what you mean by this. Koha::Account::Pay accepts a 'lines'
> > > parameter.
> > 
> > Could you please double-check?
> > I cannot find it.
> 
> It was added by bug 15897 I think

OK That's clear.
Comment 14 Josef Moravec 2017-01-11 08:42:29 UTC
Kyle, I could be wrong, but, shouldn't it be better (just for being on the safe side) to make the array of account lines objects and then call the Koha::Account->pay just once on top of it? Because this way you call pay method with whole amount on each of the accountline...
Comment 15 Kyle M Hall 2017-01-11 14:07:47 UTC
(In reply to Josef Moravec from comment #14)
> Kyle, I could be wrong, but, shouldn't it be better (just for being on the
> safe side) to make the array of account lines objects and then call the
> Koha::Account->pay just once on top of it? Because this way you call pay
> method with whole amount on each of the accountline...

I see what you mean. Yes, what I did here seems a bit silly, I'll put up a followup asap!
Comment 16 Kyle M Hall 2017-01-11 14:15:42 UTC
Created attachment 58816 [details] [review]
Bug 15907 [QA Followup] - Simplify code and call pay() only once
Comment 17 Josef Moravec 2017-01-11 16:07:24 UTC
Created attachment 58829 [details] [review]
[SIGNED-OFF] Bug 15907 - Remove use of makepayment in opac/opac-account-pay-paypal-return.pl

Test Plan:
1) Apply this patch
2) Make a payment via PayPal in sandbox mode
3) Note the payment succeeds

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 18 Josef Moravec 2017-01-11 16:07:32 UTC
Created attachment 58830 [details] [review]
[SIGNED-OFF] Bug 15907 [QA Followup] - Simplify code and call pay() only once

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 19 Jonathan Druart 2017-01-11 18:25:10 UTC
Created attachment 58840 [details] [review]
Bug 15907 - Remove use of makepayment in opac/opac-account-pay-paypal-return.pl

Test Plan:
1) Apply this patch
2) Make a payment via PayPal in sandbox mode
3) Note the payment succeeds

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Bug 15907 [QA Followup] - Simplify code and call pay() only once

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 20 Jonathan Druart 2017-01-11 18:26:13 UTC
I have squashed the 2 patches.
I did not test the paypal payment but the code looks good.
Comment 21 Kyle M Hall 2017-01-12 13:42:20 UTC
Pushed to master for 17.05!
Comment 22 Katrin Fischer 2017-01-15 16:08:45 UTC
This won't get ported back to 16.11.x as it is an enhancement.