Description
Kyle M Hall
2012-02-24 20:20:21 UTC
Created attachment 7866 [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. Created attachment 7867 [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. Created attachment 7868 [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. Created attachment 7869 [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. Created attachment 7870 [details] [review] [SIGNED-OFF] 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. 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 Created attachment 8037 [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. Created attachment 8162 [details] [review] [SIGNED-OFF] 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. 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 Created attachment 12394 [details] [review] [SIGNED-OFF] 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. Created attachment 12395 [details] [review] Bug 7595 - Add branchcode to accountlines - Followup Created attachment 12717 [details] [review] fredericd fixed 2 perlcritic errors Originally posted on bug 8849, but unrelated to it. Wajasu suggested putting it here. Hope I transferred it okay. Created attachment 12719 [details]
frediric patch already applied to master 2012-10-05
obsoleted, since the perlcritic change has been done on master already
Created attachment 14814 [details] [review] [SIGNED-OFF] 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. Created attachment 14815 [details] [review] Bug 7595 - Add branchcode to accountlines - Followup 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 Created attachment 20808 [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. Created attachment 20809 [details] [review] Bug 7595 - Add branchcode to accountlines - Followup 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. Hm, the follow up was never signed off, but touches a lot of sensitive areas. Please switch back to "Needs Signoff" later. Created attachment 23306 [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. Created attachment 23308 [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. Created attachment 27337 [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. NOTE: Rebased 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; Created attachment 30946 [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. NOTE: Rebased Created attachment 30947 [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. 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; Created attachment 30950 [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. NOTE: Rebased 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 Created attachment 38150 [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. NOTE: Rebased I'll triple check I didn't miss anything in the fix later. Created attachment 38225 [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. NOTE: Rebased There. Should be like it was. :) Created attachment 38235 [details] [review] [Signed-off] 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. NOTE: Rebased --- How I tested: - Apply patch - Do payments, create manual invoices, create credits for patrons and verified result using mysql. Note: Resolved easy conflict in updatedatabase.pl Signed-off-by: Marc Véron <veron@veron.ch> Please provide tests for changes in modules. (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. (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? [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. Created attachment 52909 [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. NOTE: Rebased Created attachment 52910 [details] [review] Bug 7595 - Unit Tests Created attachment 52911 [details] [review] Bug 7595 - Update Schema > 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!
There are no sign-off lines on these patches - Kyle & Marc, can you please check? 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? (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. 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) 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> Created attachment 54263 [details] [review] Bug 7595 - Unit Tests Created attachment 54264 [details] [review] Bug 7595 - Update Schema 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. 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 This is too old to rework. Closing out. Will start fresh. *** This bug has been marked as a duplicate of bug 19066 *** |