Summary: | action_logs not tracking payments | ||
---|---|---|---|
Product: | Koha | Reporter: | Nicole C. Engard <nengard> |
Component: | Transaction logs | Assignee: | Kyle M Hall <kyle> |
Status: | CLOSED FIXED | QA Contact: | Jonathan Druart <jonathan.druart> |
Severity: | major | ||
Priority: | P5 - low | CC: | gmcharlt, jonathan.druart, kyle, liz, mjr |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Attachments: |
Bug 9293 - action_logs not tracking payments
Bug 9293 - action_logs not tracking payments Bug 9293 - action_logs not tracking payments Bug 9293 - action_logs not tracking payments Bug 9293 - action_logs not tracking payments Bug 9293 - action_logs not tracking payments Bug 9293 - action_logs not tracking payments Bug 9293 - action_logs not tracking payments Bug 9293 - action_logs not tracking payments |
Description
Nicole C. Engard
2012-12-14 19:52:32 UTC
Created attachment 14664 [details] [review] Bug 9293 - action_logs not tracking payments Add all fine actions to action_logs via FinesLog syspref Test Plan: 1) Enable FinesLog 2) Apply patch 3) Try all the various fine related actions possible 4) Verify that each action creates an action_logs entry Created attachment 14667 [details] [review] Bug 9293 - action_logs not tracking payments Add all fine actions to action_logs via FinesLog syspref Test Plan: 1) Enable FinesLog 2) Apply patch 3) Try all the various fine related actions possible 4) Verify that each action creates an action_logs entry This patch certainly works, but I think the output in the log needs to be clearer. Where money is involved, you do not want *any* ambiguity as to what happened, and you average librarian is going to look at that and probably have trouble understanding what happened. I know that the same output appears in other logs, but the other logs don't involve money and financial "paper trail." Please make the output a bit easier to understand, something like "borrower: <name> Action: <paid, writeoff, etc> Amount: <amount> It doesn't have to be exactly like that, but it should have those elements. Created attachment 15066 [details] [review] Bug 9293 - action_logs not tracking payments Add all fine actions to action_logs via FinesLog syspref Test Plan: 1) Enable FinesLog 2) Apply patch 3) Try all the various fine related actions possible 4) Verify that each action creates an action_logs entry Created attachment 15067 [details] [review] Bug 9293 - action_logs not tracking payments Add all fine actions to action_logs via FinesLog syspref Test Plan: 1) Enable FinesLog 2) Apply patch 3) Try all the various fine related actions possible 4) Verify that each action creates an action_logs entry Created attachment 15068 [details] [review] Bug 9293 - action_logs not tracking payments Add all fine actions to action_logs via FinesLog syspref Test Plan: 1) Enable FinesLog 2) Apply patch 3) Try all the various fine related actions possible 4) Verify that each action creates an action_logs entry I think this new version will be more to your liking. Kyle (In reply to comment #3) > This patch certainly works, but I think the output in the log needs to be > clearer. > > Where money is involved, you do not want *any* ambiguity as to what > happened, and you average librarian is going to look at that and probably > have trouble understanding what happened. > > I know that the same output appears in other logs, but the other logs don't > involve money and financial "paper trail." > > Please make the output a bit easier to understand, something like "borrower: > <name> Action: <paid, writeoff, etc> Amount: <amount> > > It doesn't have to be exactly like that, but it should have those elements. The patch has a *lot* of extraneous whitespace changes. If you want to clean up whitespace in C4/Accounts.pm as part of this patch series, please break it up into two patches so that your functionality change (adding FinesLog) can be evaluated separately from whitespace cleanup. Also, the patch includes this: @@ -396,19 +460,37 @@ sub manualinvoice { if ( $itemnum ) { $desc .= ' ' . $itemnum; - my $sth = $dbh->prepare( - 'INSERT INTO accountlines - (borrowernumber, accountno, date, amount, description, accounttype, amountoutstanding, itemnumber,notify_ - VALUES (?, ?, now(), ?,?, ?,?,?,?,?,?)'); - $sth->execute($borrowernumber, $accountno, $amount, $desc, $type, $amountleft, $itemnum,$notifyid, $note, $manager_id) || re - } else { - my $sth=$dbh->prepare("INSERT INTO accountlines + my $sth = $dbh->prepare(" + INSERT INTO accountlines + (borrowernumber, accountno, date, amount, description, accounttype, amountoutstanding, itemnumber,notify_id, note, ma + VALUES (?, ?, now(), ?,?, ?,?,?,?,?,?) + "); + $sth->execute($borrowernumber, $accountno, $amount, $desc, $type, $amountleft, $itemnum,$notifyid, $note, $manager_id) || + } else { + my $sth=$dbh->prepare(" + INSERT INTO accountlines I really wish that whatever is going on here wasn't changing the single quotes to double quotes in the $dbh->prepare() statement -- that makes it marginally more vulnerable to an SQL injection if somebody slips up and introduces a variable inside the quotes. Created attachment 15135 [details] [review] Bug 9293 - action_logs not tracking payments Add all fine actions to action_logs via FinesLog syspref Test Plan: 1) Enable FinesLog 2) Apply patch 3) Try all the various fine related actions possible 4) Verify that each action creates an action_logs entry Created attachment 15136 [details] [review] Bug 9293 - action_logs not tracking payments Add all fine actions to action_logs via FinesLog syspref Test Plan: 1) Enable FinesLog 2) Apply patch 3) Try all the various fine related actions possible 4) Verify that each action creates an action_logs entry Comment on attachment 15136 [details] [review] Bug 9293 - action_logs not tracking payments Review of attachment 15136 [details] [review]: ----------------------------------------------------------------- ::: C4/Accounts.pm @@ +25,5 @@ > use C4::Members; > use C4::Circulation qw(ReturnLostItem); > +use C4::Log qw(logaction); > + > +use Data::Dumper qw(Dumper); This works, but since we already make use of Module::Load::Conditional in Koha, we could use it, and can_load to load Data::Dumper only if we need it ie if (C4::Context->preference("FinesLog") ) { can_load(Data::Dumper); } Probably not needed here, but every module we don't use that we load is a small performance hit Created attachment 15782 [details] [review] Bug 9293 - action_logs not tracking payments Add all fine actions to action_logs via FinesLog syspref Test Plan: 1) Enable FinesLog 2) Apply patch 3) Try all the various fine related actions possible 4) Verify that each action creates an action_logs entry Signed-off-by: MJ Ray <mjr@phonecoop.coop> I might not have tested all the possible actions, but all the ones I tried seemed to work, and it's been waiting three weeks, so I'll sign it off. Hi Kyle, Don't you think JSON::to_json is more adapted than Data::Dumper::Dumper here? (In reply to comment #14) > Hi Kyle, > Don't you think JSON::to_json is more adapted than Data::Dumper::Dumper here? I think that's a great idea, but Dumper is currently the way most everything is stored for the action logs. Perhaps we should submit a bug to switch everything to that and have it depend on this bug. We would have to have updatedatabase convert everything that is already in the action logs as well. Kyle, in master, Dumper is used only 1 time with logaction. But it is not blocker. QA comment: This patch adds some logs for payments, it is clean and seems to work as expected. I am not familiar with this module (Accounts.pm) so I don't know if I tested all cases. It is a small patch and it will not introduce regression so I mark it as Passed QA. Created attachment 15839 [details] [review] Bug 9293 - action_logs not tracking payments Add all fine actions to action_logs via FinesLog syspref Test Plan: 1) Enable FinesLog 2) Apply patch 3) Try all the various fine related actions possible 4) Verify that each action creates an action_logs entry Signed-off-by: MJ Ray <mjr@phonecoop.coop> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> This patch has been pushed to master. |