I am not sure of the right Component for this bug. I want to improve Statistics recording (adding some new fields, adding custom fields, etc). The first preliminary step is in this patch, which improves current code. An other patch will provide new features. This patch - refactors C4::Stats::UpdateStats (it now takes a hashref as unique parameter, instead of a list of parameters) - add UT for it - change the calls made to this sub in C4::Accounts and C4::Circulation Also : - adds POD to C4::Stats::TotalPaid - adds some comments to C4::Stats::TotalPaid (I think I found some errors in it) M. Saby
Created attachment 22850 [details] [review] [PATCH] Bug 11230 - Refactor C4::Stats::UpdateStats and add UT This patch - refactors C4::Stats::UpdateStats (it now takes a hashref as unique parameter, instead of a list of parameters) - add UT for it - change the calls made to this sub in C4::Accounts and C4::Circulation Additionnaly it also - adds POD to C4::Stats::TotalPaid - adds some comments to C4::Stats::TotalPaid (I think I found some errors in it) To test : 1. run "prove t/db_dependant/Stats.t -v" 2. make some circulation operations (checkout, checkin, renew, localuse) check the operations are rightly recorded in Statistics table (with a SQL query like "SELECT * FROM statistics WHERE datetime LIKE "2013-11-15%", if you run your test on the 15th november) 3. make some fine payments operations (writeoff, payment) check the operations are rightly recorded in Statistics table (with a SQL query like "SELECT * FROM statistics WHERE datetime LIKE "2013-11-15%", if you run your test on the 15th november) Note that there is probably an issue to fix in Accounts.pm : the user is saved instead of the branch. But this is not the purpose of this patch, so I kept the previous behavior for the moment.
Created attachment 26798 [details] [review] [PATCH] Bug 11230 - Refactor C4::Stats::UpdateStats and add UT fix trailing whitespaces
I think this bug should be tested before improving stats (bug 7021) Mathieu
Patch still applies in master.
Created attachment 29158 [details] [review] [SIGNED-OFF] Bug 11230 - Refactor C4::Stats::UpdateStats and add UT This patch - refactors C4::Stats::UpdateStats (it now takes a hashref as unique parameter, instead of a list of parameters) - add UT for it - change the calls made to this sub in C4::Accounts and C4::Circulation Additionnaly it also - adds POD to C4::Stats::TotalPaid - adds some comments to C4::Stats::TotalPaid (I think I found some errors in it) To test : 1. run "prove t/db_dependant/Stats.t -v" 2. make some circulation operations (checkout, checkin, renew, localuse) check the operations are rightly recorded in Statistics table (with a SQL query like "SELECT * FROM statistics WHERE datetime LIKE "2013-11-15%", if you run your test on the 15th november) 3. make some fine payments operations (writeoff, payment) check the operations are rightly recorded in Statistics table (with a SQL query like "SELECT * FROM statistics WHERE datetime LIKE "2013-11-15%", if you run your test on the 15th november) Note that there is probably an issue to fix in Accounts.pm : the user is saved instead of the branch. But this is not the purpose of this patch, so I kept the previous behavior for the moment. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Work, test pass, isse/return/writeoff recorded on statistics Removed a "=back" to make happy koha-qa No other errors
Thanks for signing What do you think of the point "Note that there is probably an issue to fix in Accounts.pm : the user is saved instead of the branch. But this is not the purpose of this patch, so I kept the previous behavior for the moment." Do you agree ? If so, I suppose it deserves a new bug. It could be easy to fix, but I did not try to write a patch, because I did not know if it would be necessary and possible to fix the wrong data already stored in statistics table. (and additionnaly I've got no time for that...) Mathieu
(In reply to mathieu saby from comment #6) > Thanks for signing > > What do you think of the point > "Note that there is probably an issue to fix in Accounts.pm : the user is > saved instead of the branch. But this is not the purpose of this patch, so I > kept the previous behavior for the moment." > > Do you agree ? If so, I suppose it deserves a new bug > It could be easy to fix, but I did not try to write a patch, because I did > not know if it would be necessary and possible to fix the wrong data already > stored in statistics table. > (and additionnaly I've got no time for that...) Hi Mathieu, You are right, and the removed comment said so. But looking into the code, seems that the logic was to record somehow what staff user did the payment. But there is a 'usercode' column in statistics table, that seems to be the right place for that info. A new bug is the right thing to do, maybe a mail to koha-devel first. Old data... is unfixable.
In fact I already made a bug for this issue in 2013 : Bug 11229
I do believe all instances of "keys $params" need to be changed to "keys %$params". koha-qa -v 2 -c 1 testing 1 commit(s) (applied to cb8b0fe 'Bug 12508: Follow up - corrects capit') FAIL C4/Accounts.pm OK pod OK forbidden patterns FAIL valid Type of arg 1 to keys must be hash (not private variable) Compilation failed in require OK critic FAIL C4/Circulation.pm OK pod OK forbidden patterns FAIL valid Type of arg 1 to keys must be hash (not private variable) Compilation failed in require OK critic FAIL C4/Stats.pm OK pod OK forbidden patterns FAIL valid C4/Stats.pm had compilation errors. Type of arg 1 to keys must be hash (not private variable) OK critic FAIL t/db_dependent/Stats.t OK pod OK forbidden patterns FAIL valid Type of arg 1 to keys must be hash (not private variable) Compilation failed in require OK critic
Created attachment 29666 [details] [review] [PATCH 2/2] Bug 11230: dereference hashes in keys (QA followup) This followup replaces $params with %$params in keys function It also fixes some wording
I hope I fixed it. I'm afraid beeing too far from Koha is making me forget my Perl... Mathieu
Created attachment 30123 [details] [review] [PASSED QA] Bug 11230 - Refactor C4::Stats::UpdateStats and add UT This patch - refactors C4::Stats::UpdateStats (it now takes a hashref as unique parameter, instead of a list of parameters) - add UT for it - change the calls made to this sub in C4::Accounts and C4::Circulation Additionnaly it also - adds POD to C4::Stats::TotalPaid - adds some comments to C4::Stats::TotalPaid (I think I found some errors in it) To test : 1. run "prove t/db_dependant/Stats.t -v" 2. make some circulation operations (checkout, checkin, renew, localuse) check the operations are rightly recorded in Statistics table (with a SQL query like "SELECT * FROM statistics WHERE datetime LIKE "2013-11-15%", if you run your test on the 15th november) 3. make some fine payments operations (writeoff, payment) check the operations are rightly recorded in Statistics table (with a SQL query like "SELECT * FROM statistics WHERE datetime LIKE "2013-11-15%", if you run your test on the 15th november) Note that there is probably an issue to fix in Accounts.pm : the user is saved instead of the branch. But this is not the purpose of this patch, so I kept the previous behavior for the moment. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Work, test pass, isse/return/writeoff recorded on statistics Removed a "=back" to make happy koha-qa No other errors Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 30124 [details] [review] [PASSED QA] Bug 11230: dereference hashes in keys (QA followup) This followup replaces $params with %$params in keys function It also fixes some wording Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Patches pushed to master. Thanks Mathieu!