Bug 14788 - Move opac-topissues.pl code into C4::Circulation
Summary: Move opac-topissues.pl code into C4::Circulation
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 14749 11139
  Show dependency treegraph
 
Reported: 2015-09-04 14:22 UTC by Julian Maurice
Modified: 2016-06-21 21:40 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14788: Move opac-topissues.pl code into C4::Circulation (13.60 KB, patch)
2015-09-04 14:23 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 14788: Move opac-topissues.pl code into C4::Circulation (13.84 KB, patch)
2015-10-02 09:19 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14788: Add unit tests for GetTopIssues (3.64 KB, patch)
2015-10-05 09:03 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 14788: Move opac-topissues.pl code into C4::Circulation (13.92 KB, patch)
2015-10-07 07:39 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 14788: Add unit tests for GetTopIssues (3.64 KB, patch)
2015-10-07 07:39 UTC, Julian Maurice
Details | Diff | Splinter Review
[PASSED QA] Bug 14788: Move opac-topissues.pl code into C4::Circulation (13.99 KB, patch)
2015-10-22 06:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14788: Add unit tests for GetTopIssues (3.70 KB, patch)
2015-10-22 06:11 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2015-09-04 14:22:33 UTC
Move opac-topissues.pl code into C4::Circulation
Comment 1 Julian Maurice 2015-09-04 14:23:27 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2015-10-02 06:29:08 UTC
Hi Julian,

With the patch, the checkouts on the result table do no longer sort numerically but alpabetically, something like:
1 -14 - 2- 26- 89
instead of
1 -2 -14 -26- 89
Comment 3 Julian Maurice 2015-10-02 06:49:27 UTC
Hi Marc,

I have the same behaviour on master, I don't think the patch is responsible for that.
Comment 4 Julian Maurice 2015-10-02 07:33:05 UTC
Created a new bug: Bug 14940

Switching back to Needs signoff
Comment 5 Marc Véron 2015-10-02 09:19:08 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2015-10-02 09:32:26 UTC
Julian, please provide tests.
Comment 7 Julian Maurice 2015-10-05 09:03:17 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2015-10-06 13:03:36 UTC
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-topissues.tt
Comment 9 Julian Maurice 2015-10-07 07:39:11 UTC Comment hidden (obsolete)
Comment 10 Julian Maurice 2015-10-07 07:39:13 UTC Comment hidden (obsolete)
Comment 11 Julian Maurice 2015-10-07 07:40:14 UTC
Patches rebased on master.
Minor conflict resolved, switching back to Signed Off
Comment 12 Katrin Fischer 2015-10-22 06:10:53 UTC
Created attachment 43730 [details] [review]
[PASSED QA] Bug 14788: Move opac-topissues.pl code into C4::Circulation

Tested with syspref 'AdvancedSearchTypes' set to itemtypes an ccode (one at a time).
No problems found.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Katrin Fischer 2015-10-22 06:11:04 UTC
Created attachment 43731 [details] [review]
[PASSED QA] Bug 14788: Add unit tests for GetTopIssues

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2015-10-22 06:12:42 UTC
I think this is an improvement over the current code, but needs to be pushed together with bug 11139 that will actually make it work nicely :)

If you add multiple values in AdvancedSearchTypes with only this patch, it doesn't work quite right yet - but tested with 11139 things look alright.
Comment 15 Tomás Cohen Arazi 2015-10-22 15:02:07 UTC
Patches pushed to master.

Good job Julian!
Comment 16 Jonathan Druart 2015-10-23 14:45:06 UTC
]% prove t/db_dependent/Circulation/GetTopIssues.t
t/db_dependent/Circulation/GetTopIssues.t .. DBD::mysql::st execute failed: Column 'biblioitemnumber' cannot be null at C4/Items.pm line 2196.
DBD::mysql::st execute failed: Column 'biblioitemnumber' cannot be null at C4/Items.pm line 2196.
# Looks like your test exited with 255 before it could output anything.
t/db_dependent/Circulation/GetTopIssues.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 14/14 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation/GetTopIssues.t (Wstat: 65280 Tests: 0 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 14 tests but ran 0.
Files=1, Tests=0,  2 wallclock secs ( 0.02 usr  0.00 sys +  1.40 cusr  0.02 csys =  1.44 CPU)
Result: FAIL
Comment 17 Jonathan Druart 2015-10-23 14:56:23 UTC
Ok...
The AI on my biblio and biblioitems tables differ.
I don't really know why...