C4::Accounts::makepartialpayment save the transaction in statistics table, by calling UpdateStats. The fist given param is "$user". It should be "$branch" M. Saby
Same issue in makepartialpayment. Both subs are called by members/paycollect.pl We don't use fines, so I don't know if the branch to save is the homebranch of the user, or of the librarian. M. Saby
I meant "makepayment" ;-)
*** Bug 8876 has been marked as a duplicate of this bug. ***
I still see the missing branchcodes in my database: select branch, datetime, type, proccode from statistics where type= 'payment' order by datetime desc; This looks a bit suspicious: http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=C4/Accounts.pm;hb=b4e843615a76d3a9a8516f82a2ebad96aa9c4d1a#l278
Created attachment 39971 [details] [review] Bug 11229: branch missing from statistics of type 'payment' There are several ways of paying fines, not all of them recorded the branch the payment was made at correctly: 1) Pay button next to an individual fine 2) Checkbox + pay amount 3) Checkbox + pay selected For each of those, check if the payment registered in the table statistics contains the branchcode before and after the patch. The patch should fix 1). 2) and 3) are ok with and without the patch. Important: Don't change the amount, pay individual fines.
This patch is insufficient in my mind. Read the C4::Account code to which you linked. recordpayment sets $branch to logged in user's branch. Function seems to be SIP based on comments, so should be user paying own fine, right? recordpayment_selectaccts sets $branch to logged in user's branch. Function seems to be SIP based on comments, so should be user paying own fine, right? makepayment takes $branch as a parameter, but then uses $user in the UpdateStats call?! This patch fixes that. makepartialpayment takes $branch as a parameter, but then uses $user in the UpdateStats call?! This patch misses this. WriteOffFee takes $branch as a parameter, but if it is 0, '', or undef, it uses the logged in user's branch. It correctly uses $branch for UpdateStats. I think the handling used here should be duplicated in the makepayment and makepartialpayment functions.
Created attachment 41473 [details] [review] Bug 11229: (regression tests) makepayment and makepartialpayment should log correctly This patch introduces tests for makepayment and makepartialpayment to check they are correctly logging to the statistics table. To test: - Run $ prove t/db_dependent/Accounts.t => FAIL: tests fail because statistics are not correctly logged. Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Created attachment 41474 [details] [review] Bug 11229: makepayment doesn't log branch in statistics correctly This patch fixes makepayment() so it correctly logs the branch code to the statistics table. To test: - Run (with the regression tests applied) $ prove t/db_dependent/Accounts.t => FAIL: makepayment() tests fail because logging is wrong - Apply the patch - Run $ prove t/db_dependent/Accounts.t => SUCCESS: tests for makepayment now pass (Note: makepartialpayment tests still fail as they need the next patch) Original commit message relying on the UI to test: There are several ways of paying fines, not all of them recorded the branch the payment was made at correctly: 1) Pay button next to an individual fine 2) Checkbox + pay amount 3) Checkbox + pay selected For each of those, check if the payment registered in the table statistics contains the branchcode before and after the patch. The patch should fix 1). 2) and 3) are ok with and without the patch. Important: Don't change the amount, pay individual fines. Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar> Note: i fixed the commit message, because this is a fix for makepayment
Created attachment 41475 [details] [review] Bug 11229: (followup) makepartialpayment fix makepartialpayment() wasn't logging the branch either. This patch fixes that too. To test: - Run (with the regression tests applied) $ prove t/db_dependent/Accounts.t => FAIL: tests fail because makepartialpayment is not logging the branch code - Apply the patch - Run $ prove t/db_dependent/Accounts.t => SUCCESS: tests pass - Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Created attachment 41619 [details] [review] Bug 11229: (regression tests) makepayment and makepartialpayment should log correctly This patch introduces tests for makepayment and makepartialpayment to check they are correctly logging to the statistics table. To test: - Run $ prove t/db_dependent/Accounts.t => FAIL: tests fail because statistics are not correctly logged. Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41620 [details] [review] Bug 11229: makepayment doesn't log branch in statistics correctly This patch fixes makepayment() so it correctly logs the branch code to the statistics table. To test: - Run (with the regression tests applied) $ prove t/db_dependent/Accounts.t => FAIL: makepayment() tests fail because logging is wrong - Apply the patch - Run $ prove t/db_dependent/Accounts.t => SUCCESS: tests for makepayment now pass (Note: makepartialpayment tests still fail as they need the next patch) Original commit message relying on the UI to test: There are several ways of paying fines, not all of them recorded the branch the payment was made at correctly: 1) Pay button next to an individual fine 2) Checkbox + pay amount 3) Checkbox + pay selected For each of those, check if the payment registered in the table statistics contains the branchcode before and after the patch. The patch should fix 1). 2) and 3) are ok with and without the patch. Important: Don't change the amount, pay individual fines. Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar> Note: i fixed the commit message, because this is a fix for makepayment Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41621 [details] [review] Bug 11229: (followup) makepartialpayment fix makepartialpayment() wasn't logging the branch either. This patch fixes that too. To test: - Run (with the regression tests applied) $ prove t/db_dependent/Accounts.t => FAIL: tests fail because makepartialpayment is not logging the branch code - Apply the patch - Run $ prove t/db_dependent/Accounts.t => SUCCESS: tests pass - Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41622 [details] [review] Bug 11229: Make the tests pass even if issues exist in the DB This is not introduced by the previous patches of this patchset. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41623 [details] [review] Bug 11229: Remove error rollback ineffective with AutoCommit enabled The full error is: rollback ineffective with AutoCommit enabled at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1621. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Jonathan Druart from comment #14) > Created attachment 41623 [details] [review] [review] > Bug 11229: Remove error rollback ineffective with AutoCommit enabled > > The full error is: > rollback ineffective with AutoCommit enabled at > /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1621. > > Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> RM: Feel free to skip this one if you disagree.
(In reply to Jonathan Druart from comment #15) > (In reply to Jonathan Druart from comment #14) > > Created attachment 41623 [details] [review] [review] [review] > > Bug 11229: Remove error rollback ineffective with AutoCommit enabled > > > > The full error is: > > rollback ineffective with AutoCommit enabled at > > /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1621. > > > > Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> > > RM: Feel free to skip this one if you disagree. As usual ;)
Comment on attachment 41623 [details] [review] Bug 11229: Remove error rollback ineffective with AutoCommit enabled The problem is generated by how TestBuilder is coded. We should fix it instead.
Patches pushed to master. Thanks everyone!