Bug 16443 - C4::Members::Statistics is not plack safe
Summary: C4::Members::Statistics is not plack safe
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 7172
  Show dependency treegraph
 
Reported: 2016-05-04 19:08 UTC by Jonathan Druart
Modified: 2019-06-27 09:24 UTC (History)
6 users (show)

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


Attachments
Bug 16443: Make C4::Members::Statistics plack safe (1.19 KB, patch)
2016-05-04 19:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16443: Make C4::Members::Statistics plack safe (1.24 KB, patch)
2016-05-09 10:02 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 16443: Make C4::Members::Statistics plack safe (1.34 KB, patch)
2016-06-10 10:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16443: [QA Follow-up] Add two tests for get_fields (1.45 KB, patch)
2016-06-10 10:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-05-04 19:08:46 UTC
If the prefs is updated, the fields won't be.
To make sure we already fetch updated values, we should remove the package variable and define it in the subroutine.
Comment 1 Jonathan Druart 2016-05-04 19:09:55 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2016-05-09 10:02:58 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2016-05-09 10:04:11 UTC
This works, I haven't tested if there is a performance hit, but even if there was we need to make the change anyway. But if there is, we may want to put some caching in get_fields
Comment 4 Marcel de Rooy 2016-06-04 13:18:24 UTC
(In reply to Chris Cormack from comment #3)
> This works, I haven't tested if there is a performance hit, but even if
> there was we need to make the change anyway. But if there is, we may want to
> put some caching in get_fields

Yes it is definitely slower. (You could speed up somewhat by returning to dbh, but we chose for DBIx..)
Could we cache the item columns here instead of fetching them every time?
get_fields needs the item columns and the pref.
Comment 5 Marcel de Rooy 2016-06-10 10:04:30 UTC
(In reply to Marcel de Rooy from comment #4)
> (In reply to Chris Cormack from comment #3)
> > This works, I haven't tested if there is a performance hit, but even if
> > there was we need to make the change anyway. But if there is, we may want to
> > put some caching in get_fields
> 
> Yes it is definitely slower. (You could speed up somewhat by returning to
> dbh, but we chose for DBIx..)
> Could we cache the item columns here instead of fetching them every time?
> get_fields needs the item columns and the pref.

Hm. Actually not too bad after all..
Comment 6 Marcel de Rooy 2016-06-10 10:05:51 UTC
Created attachment 52219 [details] [review]
Bug 16443: Make C4::Members::Statistics plack safe

If the prefs is updated, the fields won't be.
To make sure we already fetch updated values, we should remove the
package variable and define it in the subroutine.

There is not test plan, just make sure the changes are consistent.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2016-06-10 10:05:55 UTC
Created attachment 52220 [details] [review]
Bug 16443: [QA Follow-up] Add two tests for get_fields

Adds t/db_dependent/Members/Statistics.t.

Test plan:
Run the test.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Kyle M Hall 2016-06-10 16:37:10 UTC
Pushed to master for Koha 16.11, thanks Jonathan, Marcel!
Comment 9 Frédéric Demians 2016-06-15 07:40:26 UTC
Pushed in 16.05. Will be in 16.05.01.
Comment 10 Julian Maurice 2016-06-16 08:32:18 UTC
Patches pushed to 3.22.x, will be in 3.22.8