Bug 15407 - Move the patron categories related code to Koha::Patron::Categories - part 2
Summary: Move the patron categories related code to Koha::Patron::Categories - part 2
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
: 16877 (view as bug list)
Depends on: 14836 15629
Blocks: 15449 15758 16852 17384 17495 17548 17640 19921
  Show dependency treegraph
 
Reported: 2015-12-22 13:02 UTC by Jonathan Druart
Modified: 2018-01-05 19:36 UTC (History)
7 users (show)

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


Attachments
Bug 15407: Koha::Patron::Categories - remove sql queries in some pl and pm (18.63 KB, patch)
2015-12-22 13:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace C4::Category->all (31.34 KB, patch)
2015-12-22 13:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategory (10.90 KB, patch)
2015-12-22 13:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategoryList (11.35 KB, patch)
2015-12-22 13:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetborCatFromCatType (37.24 KB, patch)
2015-12-22 13:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategoryList (11.33 KB, patch)
2015-12-22 16:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
:x (37.24 KB, patch)
2015-12-22 16:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - remove sql queries in some pl and pm (18.74 KB, patch)
2016-01-21 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace C4::Category->all (31.34 KB, patch)
2016-01-21 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategory (10.88 KB, patch)
2016-01-21 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategoryList (11.35 KB, patch)
2016-01-21 10:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetborCatFromCatType (37.31 KB, patch)
2016-01-21 10:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - remove sql queries in some pl and pm (18.74 KB, patch)
2016-02-04 16:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace C4::Category->all (31.46 KB, patch)
2016-02-04 16:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategory (10.88 KB, patch)
2016-02-04 16:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategoryList (11.35 KB, patch)
2016-02-04 16:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetborCatFromCatType (37.28 KB, patch)
2016-02-04 16:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - remove sql queries in some pl and pm (17.64 KB, patch)
2016-02-08 17:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace C4::Category->all (31.56 KB, patch)
2016-02-08 17:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategory (10.88 KB, patch)
2016-02-08 17:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategoryList (11.35 KB, patch)
2016-02-08 17:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetborCatFromCatType (37.28 KB, patch)
2016-02-08 17:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - remove sql queries in some pl and pm (17.47 KB, patch)
2016-04-13 08:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace C4::Category->all (31.70 KB, patch)
2016-04-13 08:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategory (10.11 KB, patch)
2016-04-13 08:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategoryList (11.33 KB, patch)
2016-04-13 08:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetborCatFromCatType (37.32 KB, patch)
2016-04-13 08:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategory (9.27 KB, patch)
2016-06-03 14:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategoryList (11.33 KB, patch)
2016-06-03 14:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetborCatFromCatType (37.32 KB, patch)
2016-06-03 14:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - remove sql queries in some pl and pm (17.53 KB, patch)
2016-06-04 09:03 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace C4::Category->all (31.80 KB, patch)
2016-06-04 09:04 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategory (9.32 KB, patch)
2016-06-04 09:05 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategoryList (11.39 KB, patch)
2016-06-04 09:07 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetborCatFromCatType (37.37 KB, patch)
2016-06-04 09:07 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - remove sql queries in some pl and pm (17.54 KB, patch)
2016-07-15 12:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace C4::Category->all (31.80 KB, patch)
2016-07-15 12:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategory (9.32 KB, patch)
2016-07-15 12:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategoryList (11.39 KB, patch)
2016-07-15 12:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetborCatFromCatType (37.37 KB, patch)
2016-07-15 12:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetborCatFromCatType (37.36 KB, patch)
2016-08-02 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - remove sql queries in some pl and pm (17.54 KB, patch)
2016-08-12 08:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace C4::Category->all (31.82 KB, patch)
2016-08-12 08:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategory (9.22 KB, patch)
2016-08-12 08:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategoryList (11.39 KB, patch)
2016-08-12 08:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetborCatFromCatType (37.25 KB, patch)
2016-08-12 08:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: [QA Follow-up] Fix POD in reserves_stats.pl (805 bytes, patch)
2016-08-12 09:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15407: [QA Follow-up] Joubu, add your tests (818 bytes, patch)
2016-08-12 09:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15407: (follow-up) Koha::Patron::Categories - replace C4::Category->al (894 bytes, patch)
2016-08-12 10:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - remove sql queries in some pl and pm (17.65 KB, patch)
2016-08-12 11:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace C4::Category->all (31.95 KB, patch)
2016-08-12 11:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategory (9.33 KB, patch)
2016-08-12 11:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategoryList (11.50 KB, patch)
2016-08-12 11:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15407: Koha::Patron::Categories - replace GetborCatFromCatType (37.40 KB, patch)
2016-08-12 11:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15407: [QA Follow-up] Fix POD in reserves_stats.pl (805 bytes, patch)
2016-08-12 11:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15407: [QA Follow-up] Joubu, add your tests (818 bytes, patch)
2016-08-12 11:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15407: (follow-up) Koha::Patron::Categories - replace C4::Category->al (998 bytes, patch)
2016-08-12 11:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15407: [QA Follow-up] Confusing cathash removed (1.77 KB, patch)
2016-08-12 11:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15407: Uniformise calls in patron-attr-types (1.20 KB, patch)
2016-08-12 11:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15407: Remove 'No method selected!' warnings (1.35 KB, patch)
2016-09-13 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2015-12-22 13:02:17 UTC
This second part will move
- C4::Category
- the subroutines related to the patron categories in C4::Members (GetBorrowercategory, GetBorrowercategoryList and GetborCatFromCatType)
- the sql queries in some pl script

to Koha::Patron::Categories
Comment 1 Jonathan Druart 2015-12-22 13:13:19 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-12-22 13:13:22 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-12-22 13:13:26 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2015-12-22 13:13:29 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-12-22 13:13:32 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2015-12-22 13:14:20 UTC
QA note: A false positive is raised by the qa script
  *** ERROR: Spurious =cut command at line 1021 in file C4/Members.pm
It already exists before this patch set.
Comment 7 Marc Véron 2015-12-22 15:57:32 UTC
I went through all tests with all patches applied, no problems found.
Comment 8 Marc Véron 2015-12-22 16:10:12 UTC
Sorry, I missed one:

In Batch patron deletion / anonymization I see a branch restricted category in the dropdown.

Category restricted to Midway
Staff meber's library is set to Centerville.

With patches the category is visible in dropdown (should not)

Removed patches - now the category is no longer visible (as expected)
Comment 9 Jonathan Druart 2015-12-22 16:26:34 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-12-22 16:26:44 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2015-12-22 16:28:02 UTC
(In reply to Marc Véron from comment #8)
> Sorry, I missed one:
> 
> In Batch patron deletion / anonymization I see a branch restricted category
> in the dropdown.
> 
> Category restricted to Midway
> Staff meber's library is set to Centerville.
> 
> With patches the category is visible in dropdown (should not)
> 
> Removed patches - now the category is no longer visible (as expected)

Ho yes, the entire patch (replace GetBorrowercategoryList) was wrong actually.
I have updated it, to use the search_limited instead of search method.
Thanks for the catch!
Comment 12 Jonathan Druart 2016-01-21 10:11:39 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2016-01-21 10:11:42 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2016-01-21 10:11:53 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2016-01-21 10:12:22 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2016-01-21 10:12:28 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-01-21 10:13:05 UTC
patches rebased.
Comment 18 Jonathan Druart 2016-02-04 16:16:57 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2016-02-04 16:17:01 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2016-02-04 16:17:04 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2016-02-04 16:17:08 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2016-02-04 16:17:11 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2016-02-04 16:17:26 UTC
Just a rebase against master.
Comment 24 Marc Véron 2016-02-05 09:23:58 UTC
Just a note:
Testing, went through test plans from comment #1 and #2, both OK.
#3 points 1-3 are OK
Will later try to find time for more testing.
Comment 25 Jonathan Druart 2016-02-08 17:41:22 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2016-02-08 17:41:27 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2016-02-08 17:41:33 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2016-02-08 17:41:40 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2016-02-08 17:41:45 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2016-02-08 17:42:22 UTC
Patches rebased on top of bug 15629.
Comment 31 Jonathan Druart 2016-04-13 08:20:34 UTC Comment hidden (obsolete)
Comment 32 Jonathan Druart 2016-04-13 08:20:39 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2016-04-13 08:20:42 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2016-04-13 08:20:46 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2016-04-13 08:20:50 UTC Comment hidden (obsolete)
Comment 36 Chris Cormack 2016-06-03 10:46:49 UTC
I have applied and tested the first 2 patches, they work well but when I get to the third I'm hitting

Applying: Bug 15407: Koha::Patron::Categories - replace GetBorrowercategory
fatal: sha1 information is lacking or useless (tools/overduerules.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Comment 37 Jonathan Druart 2016-06-03 14:08:56 UTC Comment hidden (obsolete)
Comment 38 Jonathan Druart 2016-06-03 14:09:00 UTC Comment hidden (obsolete)
Comment 39 Jonathan Druart 2016-06-03 14:09:06 UTC Comment hidden (obsolete)
Comment 40 Chris Cormack 2016-06-04 09:03:29 UTC Comment hidden (obsolete)
Comment 41 Chris Cormack 2016-06-04 09:04:48 UTC Comment hidden (obsolete)
Comment 42 Chris Cormack 2016-06-04 09:05:05 UTC Comment hidden (obsolete)
Comment 43 Chris Cormack 2016-06-04 09:07:15 UTC Comment hidden (obsolete)
Comment 44 Chris Cormack 2016-06-04 09:07:38 UTC Comment hidden (obsolete)
Comment 45 Katrin Fischer 2016-07-11 21:41:38 UTC
*** Bug 16877 has been marked as a duplicate of this bug. ***
Comment 46 Kyle M Hall 2016-07-13 16:15:57 UTC
perl -c reports/issues_stats.pl
Global symbol "@patron_categories" requires explicit package name at reports/issues_stats.pl line 155.
Global symbol "@patron_categories" requires explicit package name at reports/issues_stats.pl line 326.
Global symbol "@patron_categories" requires explicit package name at reports/issues_stats.pl line 400.
Bareword "GetBorrowercategoryList" not allowed while "strict subs" in use at reports/issues_stats.pl line 79
Comment 47 Jonathan Druart 2016-07-15 12:49:50 UTC Comment hidden (obsolete)
Comment 48 Jonathan Druart 2016-07-15 12:49:54 UTC Comment hidden (obsolete)
Comment 49 Jonathan Druart 2016-07-15 12:49:58 UTC Comment hidden (obsolete)
Comment 50 Jonathan Druart 2016-07-15 12:50:04 UTC Comment hidden (obsolete)
Comment 51 Jonathan Druart 2016-07-15 12:50:09 UTC Comment hidden (obsolete)
Comment 52 Jonathan Druart 2016-07-15 12:51:13 UTC
(In reply to Kyle M Hall from comment #46)
> perl -c reports/issues_stats.pl
> Global symbol "@patron_categories" requires explicit package name at
> reports/issues_stats.pl line 155.
> Global symbol "@patron_categories" requires explicit package name at
> reports/issues_stats.pl line 326.
> Global symbol "@patron_categories" requires explicit package name at
> reports/issues_stats.pl line 400.
> Bareword "GetBorrowercategoryList" not allowed while "strict subs" in use at
> reports/issues_stats.pl line 79

Hi Kyle,
I do not recreate it, but I got a conflict on issues_stats.pl when applying the patches. I have rebased them against master.
Comment 53 Jonathan Druart 2016-08-02 12:39:50 UTC Comment hidden (obsolete)
Comment 54 Jonathan Druart 2016-08-12 08:26:30 UTC Comment hidden (obsolete)
Comment 55 Jonathan Druart 2016-08-12 08:26:35 UTC Comment hidden (obsolete)
Comment 56 Jonathan Druart 2016-08-12 08:26:40 UTC Comment hidden (obsolete)
Comment 57 Jonathan Druart 2016-08-12 08:26:45 UTC Comment hidden (obsolete)
Comment 58 Jonathan Druart 2016-08-12 08:26:51 UTC Comment hidden (obsolete)
Comment 59 Marcel de Rooy 2016-08-12 08:37:01 UTC
QA: OK Having another look
Comment 60 Marcel de Rooy 2016-08-12 09:04:38 UTC Comment hidden (obsolete)
Comment 61 Marcel de Rooy 2016-08-12 09:06:38 UTC
-   my %cathash = ($line eq 'categorycode' or $column eq 'categorycode') ? &catcodes_hash : ();
-   push @loopfilter, {debug=>1, crit=>"\%cathash", filter=>join(", ", map {$cathash{$_}} sort keys %cathash)};
+    my $patron_categories = Koha::Patron::Categories->search({}, {order_by => ['categorycode']});
+    push @loopfilter, {debug=>1, crit=>"\%cathash", filter=>join(", ", map { $_->categorycode . ' (' . ( $_->description || 'NO_DESCRIPTION' ) . ')'} $patron_categories->as_list )};

You removed cathash, but left it in the push
Comment 62 Marcel de Rooy 2016-08-12 09:22:10 UTC Comment hidden (obsolete)
Comment 63 Marcel de Rooy 2016-08-12 09:49:42 UTC
Please git grep on Categories.GetName

Koha/Template/Plugin/Categories.pm:[% Categories.GetName( categorycode ) %].
koha-tmpl/intranet-tmpl/prog/en/includes/circ-patron-search-results.inc:
koha-tmpl/intranet-tmpl/prog/en/modules/members/members-update.tt:
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:

Remove the pod and replace the three remaining calls please..
Comment 64 Jonathan Druart 2016-08-12 10:06:57 UTC Comment hidden (obsolete)
Comment 65 Marcel de Rooy 2016-08-12 10:12:03 UTC
Another thing :)

perl t/db_dependent/Reserves.t
1..74
ok 1 - use C4::Reserves;
Subroutine C4::Context::userenv redefined at t/db_dependent/Reserves.t line 45.
ok 2 - CheckReserves Test 1
ok 3 - CheckReserves() include reserve_id in its response
ok 4 - CheckReserves Test 2
ok 5 - CheckReserves Test 3
ok 6 - GetReservesControlBranch returns item home branch when set to ItemHomeLibrary
ok 7 - GetReservesControlBranch returns patron home branch when set to PatronLibrary
DBIx::Class::ResultSet::next(): [SQL::Abstract::__ANON__] Fatal: unexpected operator 'cancellationdate' with undef operand at /usr/share/koha/masterclone/Koha/Objects.pm line 168
# Looks like you planned 74 tests but ran 7.
# Looks like your test exited with 255 just after 7.
Comment 66 Jonathan Druart 2016-08-12 10:22:00 UTC
(In reply to Marcel de Rooy from comment #65)
> Another thing :)
> 
> perl t/db_dependent/Reserves.t
> 1..74
> ok 1 - use C4::Reserves;
> Subroutine C4::Context::userenv redefined at t/db_dependent/Reserves.t line
> 45.
> ok 2 - CheckReserves Test 1
> ok 3 - CheckReserves() include reserve_id in its response
> ok 4 - CheckReserves Test 2
> ok 5 - CheckReserves Test 3
> ok 6 - GetReservesControlBranch returns item home branch when set to
> ItemHomeLibrary
> ok 7 - GetReservesControlBranch returns patron home branch when set to
> PatronLibrary
> DBIx::Class::ResultSet::next(): [SQL::Abstract::__ANON__] Fatal: unexpected
> operator 'cancellationdate' with undef operand at
> /usr/share/koha/masterclone/Koha/Objects.pm line 168
> # Looks like you planned 74 tests but ran 7.
> # Looks like your test exited with 255 just after 7.

Exsts on master, see bug 17115.
Comment 67 Marcel de Rooy 2016-08-12 11:45:38 UTC
Created attachment 54361 [details] [review]
Bug 15407: Koha::Patron::Categories - remove sql queries in some pl and pm

This patch replaces sql queries done in some pl script and in
C4::Reports::Guided.
Since we have now a Koha::Patron::Categories module, we should use it
where it is possible.

Test plan:
- Prerequisite: Be sure you have several patron categories created, with
  different option enabled, and limit some to certain libraries.
- On the 'Circulation and fine rules' admin page (admin/smart-rules.pl),
  all the patron categories should be displayed (even the ones limited to
  another library), ordered by description. Try to add/update existing rules.
- On the overdue rules page (tools/overduerules.pl), all the patron
  categories with overduenoticerequired set should be displayed.
  Try to add/update existing rules.
- On the following reports:
    reports/borrowers_stats.pl
    reports/issues_avg_stats.pl
The patron categories should be displayed. Note that there is an
inconsistency with these 2 reports: the patron categories limited to
other libraries are displayed on them, when they are not on the other
reports. This should certainly be fixed (on another bug report).

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 68 Marcel de Rooy 2016-08-12 11:45:43 UTC
Created attachment 54362 [details] [review]
Bug 15407: Koha::Patron::Categories - replace C4::Category->all

The C4::Category module contained only 1 method to return the patron
categories available for the logged in user.
The new method Koha::Patron::Categories->search_limited does exactly the
same thing (see tests) and must be used in place of it.

Test plan:
- Same prerequisite as before
For the following pages, you should not see patron categories limited to
other libraries.
- On the 'Item circulation alerts' admin page
  (admin/item_circulation_alerts.pl), modify the settings for check-in
  and checkout (NOTE: Should not we display all patron categories on
  this page? If yes, it must be done in another bug report to ease
  backporting it).
- Search for patrons in the admin (budget) and acquisition (order) module.
- On the patron home page (search form in the header)

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 69 Marcel de Rooy 2016-08-12 11:45:48 UTC
Created attachment 54363 [details] [review]
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategory

The purpose of C4::Members::GetBorrowercategory was to return the patron
category for a given categorycode. This can be done easily with the
Koha::Patron::Categories->find method.

Test plan:
- Same prerequisite as before
- Edit a guarantor and confirm the information will be updated for his/her
  guarantee(s).
- Update a child to adult patron (Note: Should not we hide the patron
  categories limited to others libraries? If yes, it must be done on another
  bug report).
  There is a special behavior if there is only 1 adult category, the
  user does not need to select a category. So the same as before with
  only 1 adult patron category.
- Import a patron with a category_code which does not exist. You should
  be warned.
- Modify some patron using the batch patron modification tool. The
  patron category descriptions should correctly be displayed.

Note that the overduerules page has already been tested in a previous
patch.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 70 Marcel de Rooy 2016-08-12 11:45:53 UTC
Created attachment 54364 [details] [review]
Bug 15407: Koha::Patron::Categories - replace GetBorrowercategoryList

C4::Members::GetBorrowercategoryList returned all the patron categories,
ordered by description.
This can be done using the search method of the Koha::Patron::Categories
module.

Test plan:
- Same prerequisite as before
For the following pages, you should not see patron categories limited to other
libraries. They should be ordered as before this patch, by description.
- On the "patron attribute types" admin page (admin/patron-attr-types.pl).
- On the "Batch patron deletion/anonymization"
  (tools/cleanborrowers.pl), the dropdown list should be filled
  correctly.
- On the import patron tool
- On the following report:
    reports/reserves_stats.pl
Confirm that the patron categories are displayed and that the report
still works as before.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 71 Marcel de Rooy 2016-08-12 11:45:58 UTC
Created attachment 54365 [details] [review]
Bug 15407: Koha::Patron::Categories - replace GetborCatFromCatType

This unnecessary complicated subroutine returned an arrayref and an
hashref of the patron categories available for the logged in user, for a given
category_type, ordered by categorycode.
This can now be done with the search_limited method.

Test plan:
- Same prerequisite as before
For the following pages, you should not see patron categories limited to other
libraries. They should be ordered as before this patch, by categorycode.
- Add/edit a patron, change his/her patron category value.
- On the 3 following reports:
    reports/bor_issues_top.pl
    reports/borrowers_out.pl
    reports/cat_issues_top.pl
The display for these 3 reports are different than the 2 from the first
patch (borrowers_stats.pl issues_avg_stats.pl): they are ordered by
categorycode and the ones limited to other libraries are not displayed
(should certainly be fixed).

Note that the big part of this patch has already been tested before
(update child related: CATCODE_MULTI).

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 72 Marcel de Rooy 2016-08-12 11:46:03 UTC
Created attachment 54366 [details] [review]
Bug 15407: [QA Follow-up] Fix POD in reserves_stats.pl

Trivial fix.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 73 Marcel de Rooy 2016-08-12 11:46:07 UTC
Created attachment 54367 [details] [review]
Bug 15407: [QA Follow-up] Joubu, add your tests

Trivial fix (yes again).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 74 Marcel de Rooy 2016-08-12 11:46:12 UTC
Created attachment 54368 [details] [review]
Bug 15407: (follow-up) Koha::Patron::Categories - replace C4::Category->al

Categories.GetName should not have been removed in the previous patch
(merge conflict issue).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 75 Marcel de Rooy 2016-08-12 11:46:17 UTC
Created attachment 54369 [details] [review]
Bug 15407: [QA Follow-up] Confusing cathash removed

One of the patches removed %cathash. Some kind of silly remainder
was left in a string. Just used as a text field to explain the
filters used on borrowers_stats.pl. The filter name cathash is not
really easy and also was confusing in relation to the variable.

So just a string patch, replacing it with Patron category.

NOTE: The output of borrowers_stats.pl with the filters listed, is
not one of the most beautiful Koha screens.
Furthermore, translatibility of these filters is still an issue to
be handled on another report.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 76 Marcel de Rooy 2016-08-12 11:47:18 UTC
QA Comment:
Code looks good to me. Tested selectively. Especially checked the code carefully; hard to test everything in limited time. With a quick push we will have some time before release to fix one or two things if needed. The number of follow-ups here shows that we already found some. Bonus points for the rest.

Patch 3
It will probably work most of the time, but we assume often here that the find returned a record and access its columns rightaway without checking.

Patch 4
admin/patron-attr-types.pl: Why collecting first in scalar context and later in list context ? Seeing no difference, just slightly inconsistent..

Passed QA
Comment 77 Jonathan Druart 2016-08-12 11:58:22 UTC
Created attachment 54371 [details] [review]
Bug 15407: Uniformise calls in patron-attr-types

Does not make sense to call first in list context and later in scalar
context.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 78 Jonathan Druart 2016-08-12 12:00:11 UTC
(In reply to Marcel de Rooy from comment #76)
> Patch 3
> It will probably work most of the time, but we assume often here that the
> find returned a record and access its columns rightaway without checking.

Yes, and it was almost the same before. Except that now the app will explode :)
At least we able to catch if something is wrong elsewhere.
But you are right, we need to focus on these calls.

> Patch 4
> admin/patron-attr-types.pl: Why collecting first in scalar context and later
> in list context ? Seeing no difference, just slightly inconsistent..

Fixed in last follow-up.

> Passed QA

Thanks!
Comment 79 Kyle M Hall 2016-09-08 14:29:45 UTC
Pushed to master for 16.11, thanks Jonathan, Marcel!
Comment 80 Frédéric Demians 2016-09-08 17:58:11 UTC
Bugs 14836 and 15629 being in 16.05 branch, is there any insensitive to backport this one?
Comment 81 Jonathan Druart 2016-09-08 18:26:06 UTC
(In reply to Frédéric Demians from comment #80)
> Bugs 14836 and 15629 being in 16.05 branch, is there any insensitive to
> backport this one?

It adds a lot of changes, so I would wait a couple of months (at least 1) before backporting it and see if it does not produce to many side effects.

If there is no special need (hard conflicts), don't do it :)
Comment 82 Jonathan Druart 2016-09-13 08:48:48 UTC
Created attachment 55517 [details] [review]
Bug 15407: Remove 'No method selected!' warnings

From reserves_stats.pl, there is no library pre-selected.
Comment 83 Kyle M Hall 2016-09-13 09:54:57 UTC
(In reply to Jonathan Druart from comment #82)
> Created attachment 55517 [details] [review] [review]
> Bug 15407: Remove 'No method selected!' warnings
> 
> From reserves_stats.pl, there is no library pre-selected.

Followup pushed to master, thanks Jonathan!
Comment 84 Mason James 2016-11-30 08:18:28 UTC
enhancement, passing for 16.05