Bug 11229

Summary: makepayment and makepartialpayment (C4::Accounts) don't log branch to statistics table
Product: Koha Reporter: Mathieu Saby <mathsabypro>
Component: CirculationAssignee: Katrin Fischer <katrin.fischer>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: cmurdock, gmcharlt, jonathan.druart, katrin.fischer, kyle.m.hall, mtompset, nengard, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11230
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 11229: branch missing from statistics of type 'payment'
Bug 11229: (regression tests) makepayment and makepartialpayment should log correctly
Bug 11229: makepayment doesn't log branch in statistics correctly
Bug 11229: (followup) makepartialpayment fix
Bug 11229: (regression tests) makepayment and makepartialpayment should log correctly
Bug 11229: makepayment doesn't log branch in statistics correctly
Bug 11229: (followup) makepartialpayment fix
Bug 11229: Make the tests pass even if issues exist in the DB
Bug 11229: Remove error rollback ineffective with AutoCommit enabled

Description Mathieu Saby 2013-11-09 21:50:48 UTC
C4::Accounts::makepartialpayment save the transaction in statistics table, by calling UpdateStats.
The fist given param is "$user".
It should be "$branch"

M. Saby
Comment 1 Mathieu Saby 2013-11-09 22:09:58 UTC
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
Comment 2 Mathieu Saby 2013-11-09 22:10:52 UTC
I meant "makepayment" ;-)
Comment 3 Katrin Fischer 2015-01-06 21:48:37 UTC
*** Bug 8876 has been marked as a duplicate of this bug. ***
Comment 4 Katrin Fischer 2015-01-06 21:56:23 UTC
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
Comment 5 Katrin Fischer 2015-06-08 02:01:23 UTC Comment hidden (obsolete)
Comment 6 Mark Tompsett 2015-06-18 02:46:17 UTC
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.
Comment 7 Tomás Cohen Arazi 2015-08-13 15:10:52 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2015-08-13 15:11:06 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2015-08-13 15:11:17 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-08-18 15:54:16 UTC
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>
Comment 11 Jonathan Druart 2015-08-18 15:54:22 UTC
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>
Comment 12 Jonathan Druart 2015-08-18 15:54:26 UTC
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>
Comment 13 Jonathan Druart 2015-08-18 15:54:30 UTC
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>
Comment 14 Jonathan Druart 2015-08-18 15:54:34 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2015-08-18 15:55:58 UTC
(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.
Comment 16 Jonathan Druart 2015-08-18 15:56:29 UTC
(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 17 Tomás Cohen Arazi 2015-08-18 16:15:21 UTC
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.
Comment 18 Tomás Cohen Arazi 2015-08-18 16:19:51 UTC
Patches pushed to master.

Thanks everyone!