Bug 11230 - Refactor C4::Stats::UpdateStats and add UT
Summary: Refactor C4::Stats::UpdateStats and add UT
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Mathieu Saby
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-10 15:23 UTC by Mathieu Saby
Modified: 2015-06-04 23:33 UTC (History)
5 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:


Attachments
[PATCH] Bug 11230 - Refactor C4::Stats::UpdateStats and add UT (19.84 KB, patch)
2013-11-10 15:34 UTC, Mathieu Saby
Details | Diff | Splinter Review
[PATCH] Bug 11230 - Refactor C4::Stats::UpdateStats and add UT (19.82 KB, patch)
2014-04-03 16:34 UTC, Mathieu Saby
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11230 - Refactor C4::Stats::UpdateStats and add UT (20.12 KB, patch)
2014-06-23 02:22 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[PATCH 2/2] Bug 11230: dereference hashes in keys (QA followup) (1.67 KB, patch)
2014-07-13 22:18 UTC, Mathieu Saby
Details | Diff | Splinter Review
[PASSED QA] Bug 11230 - Refactor C4::Stats::UpdateStats and add UT (20.17 KB, patch)
2014-07-25 14:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 11230: dereference hashes in keys (QA followup) (1.69 KB, patch)
2014-07-25 14:40 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 Mathieu Saby 2013-11-10 15:23:11 UTC
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
Comment 1 Mathieu Saby 2013-11-10 15:34:49 UTC Comment hidden (obsolete)
Comment 2 Mathieu Saby 2014-04-03 16:34:36 UTC Comment hidden (obsolete)
Comment 3 Mathieu Saby 2014-04-03 16:37:08 UTC
I think this bug should be tested before improving stats (bug 7021)

Mathieu
Comment 4 Mathieu Saby 2014-04-26 12:45:31 UTC
Patch still applies in master.
Comment 5 Bernardo Gonzalez Kriegel 2014-06-23 02:22:05 UTC Comment hidden (obsolete)
Comment 6 Mathieu Saby 2014-06-23 10:56:35 UTC
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
Comment 7 Bernardo Gonzalez Kriegel 2014-06-24 20:18:38 UTC
(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.
Comment 8 Mathieu Saby 2014-07-08 05:54:36 UTC
In fact I already made a bug for this issue in 2013 : 
Bug 11229
Comment 9 Kyle M Hall 2014-07-11 17:21:03 UTC
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
Comment 10 Mathieu Saby 2014-07-13 22:18:38 UTC Comment hidden (obsolete)
Comment 11 Mathieu Saby 2014-07-13 22:21:28 UTC
I hope I fixed it. I'm afraid beeing too far from Koha is making me forget my Perl...

Mathieu
Comment 12 Kyle M Hall 2014-07-25 14:39:51 UTC
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>
Comment 13 Kyle M Hall 2014-07-25 14:40:02 UTC
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>
Comment 14 Tomás Cohen Arazi 2014-07-27 23:01:48 UTC
Patches pushed to master.

Thanks Mathieu!