Koha::Account::pay will fail if $userenv is not set. The context would be a payment plugin for the opac, in which the payment system will call a callback URL to confirm the payment has been made. In this situation, no user is logged in when the plugin uses Koha::Account::pay. The problem is that the following line in Koha::Account::pay: my $manager_id = $userenv ? $userenv->{number} : 0; will result in an SQL error, since $userenv->{number} is not set, and 0 is not an acceptable value because of foreign key constraint: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_hkr`.`accountlines`, CONSTRAINT `accountlines_ibfk_borrowers_2` FOREIGN KEY (`manager_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE) By the way, if you have a look at C4/Accounts.pm, in the manualinvoice function: my $manager_id = C4::Context->userenv ? C4::Context->userenv->{'number'} : undef; $manager_id will be set to undef, which will not fail, since undef is an acceptable value regarding to SQL foreign key constraints. It's hard to provide a test plan, since it relies on a plugin, sorry, but I will try to answer to any question :)
Created attachment 110552 [details] [review] Bug 26506: Fix Koha::Account::pay if called when no user is logged-in Currently, Koha::Account::pay will fail if no user is logged-in when it is called. In this case, this patch will use undef instead of 0 for $manager_id in order to comply with the foreign key constraint on manager_id.
Created attachment 110581 [details] [review] Bug 26506: Fix Koha::Account::pay if called when no user is logged-in Currently, Koha::Account::pay will fail if no user is logged-in when it is called. In this case, this patch will use undef instead of 0 for $manager_id in order to comply with the foreign key constraint on manager_id. Signed-off-by: Magnus Enger <magnus@libriotech.no> Biblibre and Libriotech have created a OPAC payment plugin that was having problems because of this bug. Applying the fix in this patch to a live server solved the problem.
I have signed off the patch because I know it solves a problem. However, I have been involved in the work that Biblibre has done to uncover and fix the bug, so if my signoff is considered to be "not independent enough" please set the status back to NSO.
Exactly the same patch as attached to bug 26332. As this one is signed off, I think I can safely QA this version even though I wrote the duplicate.
Whilst I agree with this patch and will PQA it once back at a computer, I do feel it perhaps highlight some issues with the plugin code in question (as I highlighted on my version of the bug and it prompted me to fix my own payments plugin). Lots of Koha functions still exist that expect a userenv to always be present so in general I feel developers should always attempt to set at least the minimum of branch code and interface in a skeleton userenv if they can't catch an existing environment. Just some food for though Matthias [U+1F609]. For an example of this see my commit on one of the payment plugins we maintain: https://github.com/PTFS-Europe/koha-plugin-wpm-online_payments/commit/0a0ee1ae7dd2477d1b55d40f974a2c0dcf4e621a
Don't forget to write a test.
*** Bug 26332 has been marked as a duplicate of this bug. ***
Good catch, it needs a unit test.
Created attachment 111045 [details] [review] Bug 26506: Fix Koha::Account::pay if called when no user is logged-in Currently, Koha::Account::pay will fail if no user is logged-in when it is called. In this case, this patch will use undef instead of 0 for $manager_id in order to comply with the foreign key constraint on manager_id. Signed-off-by: Magnus Enger <magnus@libriotech.no> Biblibre and Libriotech have created a OPAC payment plugin that was having problems because of this bug. Applying the fix in this patch to a live server solved the problem. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 111046 [details] [review] Bug 26506: (QA follow-up) Add Unit Test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Pushed to master for 20.11, thanks to everybody involved!
backported to 20.05.x for 20.05.06
tests fail on 19.11.x. not backported. $ prove t/db_dependent/Koha/Account.t t/db_dependent/Koha/Account.t .. 7/11 # Looks like you planned 5 tests but ran 4. # Failed test 'pay() tests' # at t/db_dependent/Koha/Account.t line 724. Can't use string ("71") as a HASH ref while "strict refs" in use at t/db_dependent/Koha/Account.t line 720. # Looks like your test exited with 255 just after 8. t/db_dependent/Koha/Account.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 4/11 subtests Test Summary Report ------------------- t/db_dependent/Koha/Account.t (Wstat: 65280 Tests: 8 Failed: 1) Failed test: 8 Non-zero exit status: 255 Parse errors: Bad plan. You planned 11 tests but ran 8. Files=1, Tests=8, 3 wallclock secs ( 0.02 usr 0.01 sys + 2.42 cusr 0.84 csys = 3.29 CPU) Result: FAIL if needed, please fix. There also doesn't seem to be a test plan here so I'm not sure how to test this.