Description
Kyle M Hall (khall)
2016-02-24 15:57:09 UTC
Created attachment 48348 [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 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 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 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! 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 (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! 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) Please adjust this parameter: + lines => [$line], And check the comment on bug 15986 on the paypal script. Sorry 15896 (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. (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. (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 (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. 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... (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! Created attachment 58816 [details] [review] Bug 15907 [QA Followup] - Simplify code and call pay() only once 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> 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> 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> I have squashed the 2 patches. I did not test the paypal payment but the code looks good. Pushed to master for 17.05! This won't get ported back to 16.11.x as it is an enhancement. |