Bug 7595 - Add branchcode to accountlines
Summary: Add branchcode to accountlines
Status: RESOLVED DUPLICATE of bug 19066
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-24 20:20 UTC by Kyle M Hall
Modified: 2017-08-09 17:04 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7595 - Add branchcode to accountlines (9.56 KB, patch)
2012-02-24 20:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines (9.56 KB, patch)
2012-02-24 20:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines (8.72 KB, patch)
2012-02-24 21:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines (8.34 KB, patch)
2012-02-24 21:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7595 - Add branchcode to accountlines (8.59 KB, patch)
2012-02-24 21:13 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines (8.53 KB, patch)
2012-03-06 16:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7595 - Add branchcode to accountlines (8.78 KB, patch)
2012-03-11 19:49 UTC, Nicole C. Engard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7595 - Add branchcode to accountlines (8.65 KB, patch)
2012-09-20 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines - Followup (8.34 KB, patch)
2012-09-20 14:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
fredericd fixed 2 perlcritic errors (1.23 KB, patch)
2012-10-05 14:44 UTC, M. Tompsett
Details | Diff | Splinter Review
frediric patch already applied to master 2012-10-05 (61 bytes, text/plain)
2012-10-05 17:03 UTC, wajasu
Details
[SIGNED-OFF] Bug 7595 - Add branchcode to accountlines (8.60 KB, patch)
2013-01-24 12:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines - Followup (8.26 KB, patch)
2013-01-24 12:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines (9.58 KB, patch)
2013-09-05 17:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines - Followup (4.81 KB, patch)
2013-09-05 17:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines (9.34 KB, patch)
2013-12-04 17:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines (13.87 KB, patch)
2013-12-04 17:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines (13.83 KB, patch)
2014-04-21 05:18 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines (14.22 KB, patch)
2014-08-18 15:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines (14.72 KB, patch)
2014-08-18 16:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines (14.71 KB, patch)
2014-08-18 18:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines (14.19 KB, patch)
2015-04-20 05:24 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines (14.52 KB, patch)
2015-04-21 03:53 UTC, M. Tompsett
Details | Diff | Splinter Review
[Signed-off] Bug 7595 - Add branchcode to accountlines (14.85 KB, patch)
2015-04-21 09:24 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines (13.54 KB, patch)
2016-06-27 18:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Unit Tests (3.58 KB, patch)
2016-06-27 18:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Update Schema (1.39 KB, patch)
2016-06-27 18:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Add branchcode to accountlines (13.81 KB, patch)
2016-08-10 10:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Unit Tests (3.58 KB, patch)
2016-08-10 10:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7595 - Update Schema (1.39 KB, patch)
2016-08-10 10:50 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2012-02-24 20:20:21 UTC
For the purposes of statistics, it appears that it would help many libraries to have branchcode recorded in the accountlines table. For payments, the field would contain the code for the branch the payment was made at. For manual invoices, it would be the code of the library that created the invoice.
Comment 1 Kyle M Hall 2012-02-24 20:23:37 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-02-24 20:43:59 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2012-02-24 21:02:38 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2012-02-24 21:05:14 UTC Comment hidden (obsolete)
Comment 5 Nicole C. Engard 2012-02-24 21:13:53 UTC Comment hidden (obsolete)
Comment 6 Ian Walls 2012-03-01 20:09:46 UTC
Patch failed to apply, most likely due to a minor merge issue in updatedatabase.pl.  Easy rebase to do.

recordpayment() SQL is missing the branchcode column in the INSERT statement.

updatedatabase line should explicitly include where to insert the new branchcode column in accountlines (after manager_id).  Otherwise column order in kohastructure could mismatch with structure in DB after updatedatabase.pl
Comment 7 Kyle M Hall 2012-03-06 16:05:22 UTC Comment hidden (obsolete)
Comment 8 Nicole C. Engard 2012-03-11 19:49:55 UTC Comment hidden (obsolete)
Comment 9 Ian Walls 2012-03-19 20:56:55 UTC
There are also calls of "INSERT INTO accountlines..." in C4/Circulation.pm, C4/Reserves.pm and C4/Overdues.pm, as well as in members/pay.pl (for writeoffs).  Branchcode will need to be added to these areas, as well, in order to achieve complete and consistent coverage.

That is, unless under these circumstances, the branchcode should always be NULL.  If that's the use case, please let me know.  Otherwise, I'm going to mark this as Failed QA for now until those other 3 modules can be covered
Comment 10 Kyle M Hall 2012-09-20 14:16:35 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2012-09-20 14:17:10 UTC Comment hidden (obsolete)
Comment 12 M. Tompsett 2012-10-05 14:44:15 UTC Comment hidden (obsolete)
Comment 13 wajasu 2012-10-05 17:03:42 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2013-01-24 12:24:18 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2013-01-24 12:25:47 UTC Comment hidden (obsolete)
Comment 16 cedric.vita 2013-03-18 15:03:34 UTC
Something went wrong !
Applying: Bug 7595 - Add branchcode to accountlines
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Accounts.pm
CONFLICT (content): Merge conflict in C4/Accounts.pm
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 7595 - Add branchcode to accountlines
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 7595 - Add branchcode to accountlines

[SIGNED-OFF] Bug 7595 - Add branchcode to accountlines
Apply? [yn]
Patch left in /tmp/SIGNED-OFF-Bug-7595---Add-branchcode-to-accountlin-ViMI8t.patch
Comment 17 Kyle M Hall 2013-09-05 17:05:51 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2013-09-05 17:06:06 UTC Comment hidden (obsolete)
Comment 19 Katrin Fischer 2013-09-24 06:26:57 UTC
I have 2 problems with this:

1) (blocker) fines.pl is not applying fines for me after the patch was applied.
Running the script with -l it indicates there should be a fines for the patron I used for testing, but the fine is not created. Even the FINES log in action_logs show that there should be a fine entry on the account.

After running it repeatedly with different options an wondering why the fine would not appear on the patron account, it gave me an error:
Can't use an undefined value as a HASH reference at /home/katrin/kohaclone/C4/Overdues.pm line 598.

Which is the line changed by the patch.

2) I think we already track that information in the branch column of action_logs. But after applying the patch and paying a fine, I have an entry with an empty branchcode there.
Comment 20 Katrin Fischer 2013-09-24 06:34:14 UTC
Hm, the follow up was never signed off, but touches a lot of sensitive areas. Please switch back to "Needs Signoff" later.
Comment 21 Kyle M Hall 2013-12-04 17:35:20 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2013-12-04 17:46:31 UTC Comment hidden (obsolete)
Comment 23 M. Tompsett 2014-04-21 05:18:55 UTC Comment hidden (obsolete)
Comment 24 M. Tompsett 2014-04-21 05:28:09 UTC
Comment on attachment 27337 [details] [review]
Bug 7595 - Add branchcode to accountlines

Review of attachment 27337 [details] [review]:
-----------------------------------------------------------------

::: C4/Accounts.pm
@@ +407,4 @@
>      my ( $borrowernumber, $itemnum, $desc, $type, $amount, $note ) = @_;
>      my $manager_id = 0;
>      $manager_id = C4::Context->userenv->{'number'} if C4::Context->userenv;
> +    my $branchcode = C4::Context->userenv->{'branch'};

Why does this differ than line 359?
my $branchcode = C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef;
Comment 25 Kyle M Hall 2014-08-18 15:56:18 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2014-08-18 16:02:54 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2014-08-18 16:03:31 UTC
Good catch. I've updated all references to branchcode to check for the existance of userenv first.

(In reply to M. Tompsett from comment #24)
> Comment on attachment 27337 [details] [review] [review]
> Bug 7595 - Add branchcode to accountlines
> 
> Review of attachment 27337 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Accounts.pm
> @@ +407,4 @@
> >      my ( $borrowernumber, $itemnum, $desc, $type, $amount, $note ) = @_;
> >      my $manager_id = 0;
> >      $manager_id = C4::Context->userenv->{'number'} if C4::Context->userenv;
> > +    my $branchcode = C4::Context->userenv->{'branch'};
> 
> Why does this differ than line 359?
> my $branchcode = C4::Context->userenv ? C4::Context->userenv->{'branch'} :
> undef;
Comment 28 Kyle M Hall 2014-08-18 18:32:45 UTC Comment hidden (obsolete)
Comment 29 Marc Véron 2015-01-18 15:52:46 UTC
Does not apply.

Applying: Bug 7595 - Add branchcode to accountlines
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging C4/Reserves.pm
CONFLICT (content): Merge conflict in C4/Reserves.pm
Auto-merging C4/Overdues.pm
Auto-merging C4/Circulation.pm
Auto-merging C4/Accounts.pm
CONFLICT (content): Merge conflict in C4/Accounts.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 7595 - Add branchcode to accountlines
Comment 30 M. Tompsett 2015-04-20 05:24:39 UTC Comment hidden (obsolete)
Comment 31 M. Tompsett 2015-04-20 05:30:48 UTC
I'll triple check I didn't miss anything in the fix later.
Comment 32 M. Tompsett 2015-04-21 03:53:25 UTC Comment hidden (obsolete)
Comment 33 M. Tompsett 2015-04-21 04:01:25 UTC
There. Should be like it was. :)
Comment 34 Marc Véron 2015-04-21 09:24:55 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2015-04-21 12:36:52 UTC
Please provide tests for changes in modules.
Comment 36 M. Tompsett 2015-04-21 15:16:27 UTC
(In reply to Jonathan Druart from comment #35)
> Please provide tests for changes in modules.

True, most is covered:
t/db_dependent/Accounts.t
t/db_dependent/Circulation.t
t/db_dependent/Circulation_issue.t
t/db_dependent/Reserves.t

However, the makepayment, chargelostitem, manualinvoice, recordpayment_selectaccts, and makepartialpayment need test coverage.
I'm also not certain how to trigger _FixAccountForLostAndReturned, except that AddIssue does call it.
Comment 37 Kyle M Hall 2016-06-07 18:30:21 UTC
(In reply to M. Tompsett from comment #36)
> (In reply to Jonathan Druart from comment #35)
> > Please provide tests for changes in modules.
> 
> True, most is covered:
> t/db_dependent/Accounts.t
> t/db_dependent/Circulation.t
> t/db_dependent/Circulation_issue.t
> t/db_dependent/Reserves.t
> 
> However, the makepayment, chargelostitem, manualinvoice,
> recordpayment_selectaccts, and makepartialpayment need test coverage.
> I'm also not certain how to trigger _FixAccountForLostAndReturned, except
> that AddIssue does call it.

I'm going to argue that we should for the time being, pass on tests for the three payment handling subroutines, as they will be unified by bug 15894. If this is agreeable, I will write tests for chargelostitem, manualinvoice, and _FixAccountForLostAndReturned. Bug 15894 will include unit testing for payments.

How does that sound?
Comment 38 M. Tompsett 2016-06-07 19:46:15 UTC
[SEVERELY EDIT'D AND REORDERED]
(In reply to Kyle M Hall from comment #37)
> (In reply to M. Tompsett from comment #36)
> > chargelostitem,
> > manualinvoice,
> > _FixAccountForLostAndReturned
> > makepayment,
> > recordpayment_selectaccts,
> > makepartialpayment

> payment handling subroutines... unified by bug 15894.
> Bug 15894 will include unit testing for payments.
...
> chargelostitem,
> manualinvoice,
> _FixAccountForLostAndReturned
...
> How does that sound?

If 15894 will include testing for
> makepayment,
> recordpayment_selectaccts,
> makepartialpayment
then that would seem acceptable to me.
Comment 39 Kyle M Hall 2016-06-27 18:42:19 UTC Comment hidden (obsolete)
Comment 40 Kyle M Hall 2016-06-27 18:42:27 UTC Comment hidden (obsolete)
Comment 41 Kyle M Hall 2016-06-27 18:42:30 UTC Comment hidden (obsolete)
Comment 42 Kyle M Hall 2016-06-27 18:44:07 UTC
> If 15894 will include testing for
> > makepayment,
> > recordpayment_selectaccts,
> > makepartialpayment
> then that would seem acceptable to me.

Yes, that bug will remove those subs and unify them into one easily testable sub.

For the other subs, I've added a decent amount of testing above the branchcode testing alone, as none of them have unit tests yet!
Comment 43 Katrin Fischer 2016-07-12 21:32:50 UTC
There are no sign-off lines on these patches - Kyle & Marc, can you please check?
Comment 44 Katrin Fischer 2016-07-12 21:55:21 UTC
I have done some first tests - not so bad so far, but I worry about paying via SIP. Can you please verify that this works still as well?
Which branch should be logged? The branch of the SIP user from the config file?
Comment 45 Kyle M Hall 2016-07-13 13:14:55 UTC
(In reply to Katrin Fischer from comment #44)
> I have done some first tests - not so bad so far, but I worry about paying
> via SIP. Can you please verify that this works still as well?
> Which branch should be logged? The branch of the SIP user from the config
> file?

Yes, it should be the branch of the SIP user from the config file.
Comment 46 Katrin Fischer 2016-08-10 06:21:33 UTC
Looking at this again. 

Passes tests and QA test tools. 

I'd have preferred smaller steps here, it will take a while to work through this as it touches all of the fines and fee related features:

  - recordpayment
  - makepayment
  - chargelostitem
  - manualinvoice
  - recordpayment_selectaccts
  - FixAccountForLostAndReturned
  - AddRenewal (rentals)
  - AddIssuingCharge (rentals)
  - UpdateFine (overdues)
  - AddReserve (hold fee)
Comment 47 Kyle M Hall 2016-08-10 10:24:27 UTC
Created attachment 54262 [details] [review]
Bug 7595 - Add branchcode to accountlines

Adds a branchcode column to the accountlines table. For payments,
this inserts the branchcode of the library where the payment was
recorded. For manual invoices, it insert the branchcode of the
library where the manual invoice was created.

Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>

I tested with creating manual invoices, and paying partial fines,
entire fine lines, and writing off fines. All actions record the
branch where the action took place properly.

Signed-off-by: Marc Veron <veron@veron.ch>
Comment 48 Kyle M Hall 2016-08-10 10:50:19 UTC
Created attachment 54263 [details] [review]
Bug 7595 - Unit Tests
Comment 49 Kyle M Hall 2016-08-10 10:50:30 UTC
Created attachment 54264 [details] [review]
Bug 7595 - Update Schema
Comment 50 Katrin Fischer 2016-08-14 21:34:33 UTC
I am testing with a staff patron logged in at a different branch than their homebranch. The problems spotted so far concern writing off fines.

Some first test results: 

I GENERAL
- Passes QA test tools
- Passes perl t/db_dependent/Accounts.t
- Database update ok

II API
1) Koha offline circulation
- Payment is correctly applied.
  Logged-in branch of the staff user is recorded.
2) SIP
-  ( To be tested )

III PATRON ACCOUNT - STAFF
1) Reverse payment
   OK
2) Create manual invoice
   OK
3) Create manual credit
   OK
4) Pay (single fine)
   OK
5) Pay selected
   OK
6) Write off (single fine)
   NOT OK - doesn't record a branchcode.
7) Pay an amount (partial)
   OK
8) Write off all
   NOT OK - doesn't record a branchcode.
Comment 51 Katrin Fischer 2016-08-14 22:14:12 UTC
Continued testing, but didn't get much further:

IV CIRCULATION
1) Checkout with rental fine
   OK
2) Renew with rental fine
   NOT OK - no rental fine recorded

From the logs:

[Mon Aug 15 00:09:28.378180 2016] [cgi:error] [pid 20899] [client 127.0.0.1:48043] AH01215: [Mon Aug 15 00:09:28 2016] renew: DBD::mysql::st execute failed: Unknown column 'branch' in 'field list' [for Statement "INSERT INTO accountlines, referer: http://localhost:8080/cgi-bin/koha/circ/circulation.pl?borrowernumber=1
[Mon Aug 15 00:09:28.378352 2016] [cgi:error] [pid 20899] [client 127.0.0.1:48043] AH01215: [Mon Aug 15 00:09:28 2016] renew:                     (date, borrowernumber, accountno, amount, manager_id,, referer: http://localhost:8080/cgi-bin/koha/circ/circulation.pl?borrowernumber=1
[Mon Aug 15 00:09:28.378447 2016] [cgi:error] [pid 20899] [client 127.0.0.1:48043] AH01215: [Mon Aug 15 00:09:28 2016] renew:                     description,accounttype, amountoutstanding, itemnumber, branch), referer: http://localhost:8080/cgi-bin/koha/circ/circulation.pl?borrowernumber=1
[Mon Aug 15 00:09:28.378810 2016] [cgi:error] [pid 20899] [client 127.0.0.1:48043] AH01215: [Mon Aug 15 00:09:28 2016] renew:                     VALUES (now(),?,?,?,?,?,?,?,?,?)" with ParamValues: 0="1", 1='43', 2=0.1000, 3="1", 4="Renewal of Rental Item Something", 5='Rent', 6=0.1000, 7="751", 8="TPL"] at /home/katrin/kohaclone/C4/Circulation.pm line 3120., referer: http://localhost:8080/cgi-bin/koha/circ/circulation.pl?borrowernumber=1
Comment 52 Kyle M Hall 2017-08-09 13:46:48 UTC
This is too old to rework. Closing out. Will start fresh.
Comment 53 Kyle M Hall 2017-08-09 17:04:47 UTC

*** This bug has been marked as a duplicate of bug 19066 ***