Bug 11229 - makepayment and makepartialpayment (C4::Accounts) don't log branch to statistics table
Summary: makepayment and makepartialpayment (C4::Accounts) don't log branch to statist...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Katrin Fischer
QA Contact: Testopia
URL:
Keywords:
: 8876 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-11-09 21:50 UTC by Mathieu Saby
Modified: 2016-12-05 21:23 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 11229: branch missing from statistics of type 'payment' (1.13 KB, patch)
2015-06-08 02:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11229: (regression tests) makepayment and makepartialpayment should log correctly (16.14 KB, patch)
2015-08-13 15:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11229: makepayment doesn't log branch in statistics correctly (1.72 KB, patch)
2015-08-13 15:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11229: (followup) makepartialpayment fix (2.00 KB, patch)
2015-08-13 15:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11229: (regression tests) makepayment and makepartialpayment should log correctly (16.22 KB, patch)
2015-08-18 15:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11229: makepayment doesn't log branch in statistics correctly (1.79 KB, patch)
2015-08-18 15:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11229: (followup) makepartialpayment fix (2.08 KB, patch)
2015-08-18 15:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11229: Make the tests pass even if issues exist in the DB (1.04 KB, patch)
2015-08-18 15:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11229: Remove error rollback ineffective with AutoCommit enabled (799 bytes, patch)
2015-08-18 15:54 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!