Bug 15907

Summary: Remove use of makepayment in opac/opac-account-pay-paypal-return.pl
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Architecture, internals, and plumbingAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, josef.moravec, katrin.fischer, m.de.rooy
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Who signed the patch off: Text to go in the release notes:
Version(s) released in:
Bug Depends on: 15897    
Bug Blocks: 15905    
Attachments: Bug 15907 - Remove use of makepayment in opac/opac-account-pay-paypal-return.pl
Bug 15907 - Remove use of makepayment in opac/opac-account-pay-paypal-return.pl
Bug 15907 - Remove use of makepayment in opac/opac-account-pay-paypal-return.pl
Bug 15907 [QA Followup] - Simplify code and call pay() only once
[SIGNED-OFF] Bug 15907 - Remove use of makepayment in opac/opac-account-pay-paypal-return.pl
[SIGNED-OFF] Bug 15907 [QA Followup] - Simplify code and call pay() only once
Bug 15907 - Remove use of makepayment in opac/opac-account-pay-paypal-return.pl

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.