Bug 23271 - Koha::Patron::Category should use Koha::Object::Limit::Library
Summary: Koha::Patron::Category should use Koha::Object::Limit::Library
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 17755 23281
Blocks:
  Show dependency treegraph
 
Reported: 2019-07-05 18:50 UTC by Tomás Cohen Arazi
Modified: 2021-05-21 15:04 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
This enhancement makes Koha::Patron::Categories inherit the 'search_with_library_limits' method from Koha::Objects::Limit::Library and thus makes it consistent with other locations where filtering by branch is required.
Version(s) released in:
21.05.00


Attachments
Bug 23271: Improve library limitation selection for item types (7.66 KB, patch)
2019-10-16 12:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23271: Use Koha::Object::Limit::Library for Koha::Patron::Categories (11.39 KB, patch)
2019-10-16 12:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23271: Replace search_limited with search_with_library_limits (16.29 KB, patch)
2019-10-16 12:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23271: Improve library limitation selection for item types (6.71 KB, patch)
2021-04-13 07:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23271: Use Koha::Object::Limit::Library for Koha::Patron::Categories (11.21 KB, patch)
2021-04-13 07:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23271: Replace search_limited with search_with_library_limits (16.39 KB, patch)
2021-04-13 07:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23271: (QA follow-up) Make search_with_library_limits fallback to userenv if required (5.62 KB, patch)
2021-04-13 12:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23271: Improve library limitation selection for item types (6.77 KB, patch)
2021-04-13 12:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23271: Use Koha::Object::Limit::Library for Koha::Patron::Categories (11.27 KB, patch)
2021-04-13 12:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23271: Replace search_limited with search_with_library_limits (16.45 KB, patch)
2021-04-13 12:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23271: (QA follow-up) Minor POD fix (849 bytes, patch)
2021-04-13 12:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23271: Remove 'selecteds' from Branches (5.79 KB, patch)
2021-04-13 13:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23271: (QA follow-up) Make search_with_library_limits fallback to userenv if required (5.68 KB, patch)
2021-04-14 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23271: Improve library limitation selection for item types (6.83 KB, patch)
2021-04-14 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23271: Use Koha::Object::Limit::Library for Koha::Patron::Categories (11.32 KB, patch)
2021-04-14 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23271: Replace search_limited with search_with_library_limits (16.47 KB, patch)
2021-04-14 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23271: (QA follow-up) Minor POD fix (912 bytes, patch)
2021-04-14 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23271: Remove 'selecteds' from Branches (5.85 KB, patch)
2021-04-14 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23271: (QA follow-up) Make search_with_library_limits fallback to userenv if required (5.75 KB, patch)
2021-04-16 17:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23271: Improve library limitation selection for item types (6.90 KB, patch)
2021-04-16 17:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23271: Use Koha::Object::Limit::Library for Koha::Patron::Categories (11.39 KB, patch)
2021-04-16 17:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23271: Replace search_limited with search_with_library_limits (16.54 KB, patch)
2021-04-16 17:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23271: (QA follow-up) Minor POD fix (984 bytes, patch)
2021-04-16 17:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23271: Remove 'selecteds' from Branches (5.92 KB, patch)
2021-04-16 17:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23271: Prevent crash if called without parameters (1.69 KB, patch)
2021-04-21 09:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23271: Fix Template/Plugin/Branches.t (1.44 KB, patch)
2021-04-21 14:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23271: Fix random failure from Categories.t (1.72 KB, patch)
2021-04-21 15:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23271: (follow-up) Fix typo 'limlits' (920 bytes, patch)
2021-04-23 11:19 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2019-07-05 18:50:20 UTC
Koha::Patron::Attribute::Type should be used as example.
Comment 1 Jonathan Druart 2019-10-16 12:57:30 UTC
Created attachment 94292 [details] [review]
Bug 23271: Improve library limitation selection for item types
Comment 2 Jonathan Druart 2019-10-16 12:57:33 UTC
Created attachment 94293 [details] [review]
Bug 23271: Use Koha::Object::Limit::Library for Koha::Patron::Categories
Comment 3 Jonathan Druart 2019-10-16 12:57:44 UTC
Created attachment 94294 [details] [review]
Bug 23271: Replace search_limited with search_with_library_limits
Comment 4 Jonathan Druart 2019-10-16 12:59:00 UTC
Tomas, about the last patch I am wondering if it makes sense. Should not we call search_with_library_limits from search_limited and keep the search_limited calls?
I can imagine having other limitations and so a generic method names makes more sense.
Comment 5 Tomás Cohen Arazi 2019-10-16 13:05:37 UTC
(In reply to Jonathan Druart from comment #4)
> Tomas, about the last patch I am wondering if it makes sense. Should not we
> call search_with_library_limits from search_limited and keep the
> search_limited calls?
> I can imagine having other limitations and so a generic method names makes
> more sense.

Maybe, but we could do it in Koha::Objects if such was the case. The idea was not to 'detect' environment state inside de libs, but do it in the controllers and explicitly pass the information as parameter in the call.
Comment 6 Tomás Cohen Arazi 2021-04-12 20:53:53 UTC
It would be great to have this in master ASAP and finish this cleanup.
Can you please rebase?
Comment 7 Jonathan Druart 2021-04-13 07:36:37 UTC
Created attachment 119503 [details] [review]
Bug 23271: Improve library limitation selection for item types
Comment 8 Jonathan Druart 2021-04-13 07:36:41 UTC
Created attachment 119504 [details] [review]
Bug 23271: Use Koha::Object::Limit::Library for Koha::Patron::Categories
Comment 9 Jonathan Druart 2021-04-13 07:36:45 UTC
Created attachment 119505 [details] [review]
Bug 23271: Replace search_limited with search_with_library_limits
Comment 10 Tomás Cohen Arazi 2021-04-13 11:19:57 UTC
The changes assume ->search_with_library_limits implicitly falls back to C4::Context->userenv->{branch} if no library_id is passed.

While that could be convenient, it is not the case right now. So failing it until it passes the right library_id or this/a new bug introduces the fallback.
Comment 11 Tomás Cohen Arazi 2021-04-13 12:14:58 UTC
Created attachment 119529 [details] [review]
Bug 23271: (QA follow-up) Make search_with_library_limits fallback to userenv if required

This patch makes  the generic method rely on C4::Context->userenv if the
library_id is not passed.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron/Attribute/Types.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2021-04-13 12:15:03 UTC
Created attachment 119530 [details] [review]
Bug 23271: Improve library limitation selection for item types

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Tomás Cohen Arazi 2021-04-13 12:15:08 UTC
Created attachment 119531 [details] [review]
Bug 23271: Use Koha::Object::Limit::Library for Koha::Patron::Categories

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2021-04-13 12:15:13 UTC
Created attachment 119532 [details] [review]
Bug 23271: Replace search_limited with search_with_library_limits

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2021-04-13 12:15:21 UTC
Created attachment 119533 [details] [review]
Bug 23271: (QA follow-up) Minor POD fix

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Tomás Cohen Arazi 2021-04-13 12:21:23 UTC
Jonathan, I submitted what I had in mind for you to ponder.
Comment 17 Jonathan Druart 2021-04-13 13:52:43 UTC
Created attachment 119537 [details] [review]
Bug 23271: Remove 'selecteds' from Branches

Restoring the first version of this from the first patch and reverting
what hit master first.
Comment 18 Jonathan Druart 2021-04-13 13:54:27 UTC
Test plan:
1. Create a patron category that is not limited to any libraries
2. Create a patron category limited to a couple a libraries
3. Edit it and confirm that the select is selected with the libraries you picked
4. Modify the choice and save again, confirm it has been saved correctly
5. Confirm that the patron's category is visible only from those libraries.
Comment 19 Martin Renvoize 2021-04-14 14:48:27 UTC
Created attachment 119579 [details] [review]
Bug 23271: (QA follow-up) Make search_with_library_limits fallback to userenv if required

This patch makes  the generic method rely on C4::Context->userenv if the
library_id is not passed.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron/Attribute/Types.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2021-04-14 14:48:31 UTC
Created attachment 119580 [details] [review]
Bug 23271: Improve library limitation selection for item types

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2021-04-14 14:48:34 UTC
Created attachment 119581 [details] [review]
Bug 23271: Use Koha::Object::Limit::Library for Koha::Patron::Categories

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2021-04-14 14:48:38 UTC
Created attachment 119582 [details] [review]
Bug 23271: Replace search_limited with search_with_library_limits

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2021-04-14 14:48:42 UTC
Created attachment 119583 [details] [review]
Bug 23271: (QA follow-up) Minor POD fix

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2021-04-14 14:48:46 UTC
Created attachment 119584 [details] [review]
Bug 23271: Remove 'selecteds' from Branches

Restoring the first version of this from the first patch and reverting
what hit master first.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2021-04-14 14:49:37 UTC
All works as described and seems a solid code improvement... Signing off
Comment 26 Martin Renvoize 2021-04-14 14:53:58 UTC
Humm.. there is a funky QA script warning however: 

FAIL   Koha/Patron/Category.pm
   FAIL   pod coverage
                POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
   FAIL   valid
                Inconsistent hierarchy during C3 merge of class 'Koha::Patron::Category':
                        current merge results [
                                Koha::Patron::Category,
                        ]
                        merging failed on 'Koha::Object'
Comment 27 Jonathan Druart 2021-04-15 08:04:37 UTC
We already have it for most of the other modules based on Koha::Object::Limit::Library:

Koha::AuthorisedValue, Koha::Account::CreditType, Koha::Account::CreditType and Koha::Patron::Attribute::Type

Not Koha::ItemType.

In my understanding it's coming from this circular dependency:

Koha:AuthorisedValues inherits from Koha::Object and Koha::Object::Limit::Library
Koha::Object::Limit::Library uses Koha::Libraries -> Koha::Biblios -> Koha::Biblio -> Koha::Suggestions -> Koha::Patrons -> Koha::Patron -> Koha::Holds -> Koha::Hold -> Koha::AuthorisedValues -> ...

There is certainly a shorter path to find the loop
Comment 28 Kyle M Hall 2021-04-16 17:41:20 UTC
Created attachment 119772 [details] [review]
Bug 23271: (QA follow-up) Make search_with_library_limits fallback to userenv if required

This patch makes  the generic method rely on C4::Context->userenv if the
library_id is not passed.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron/Attribute/Types.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Kyle M Hall 2021-04-16 17:41:32 UTC
Created attachment 119773 [details] [review]
Bug 23271: Improve library limitation selection for item types

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Kyle M Hall 2021-04-16 17:41:36 UTC
Created attachment 119774 [details] [review]
Bug 23271: Use Koha::Object::Limit::Library for Koha::Patron::Categories

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 31 Kyle M Hall 2021-04-16 17:41:40 UTC
Created attachment 119775 [details] [review]
Bug 23271: Replace search_limited with search_with_library_limits

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 32 Kyle M Hall 2021-04-16 17:41:44 UTC
Created attachment 119776 [details] [review]
Bug 23271: (QA follow-up) Minor POD fix

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 33 Kyle M Hall 2021-04-16 17:41:48 UTC
Created attachment 119777 [details] [review]
Bug 23271: Remove 'selecteds' from Branches

Restoring the first version of this from the first patch and reverting
what hit master first.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Jonathan Druart 2021-04-21 09:54:41 UTC
Created attachment 119939 [details] [review]
Bug 23271: Prevent crash if called without parameters

Can't use an undefined value as a HASH reference at /kohadevbox/koha/Koha/Objects/Limit/Library.pm line 87.
Comment 35 Jonathan Druart 2021-04-21 13:29:44 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 36 Jonathan Druart 2021-04-21 14:58:04 UTC
Created attachment 119956 [details] [review]
Bug 23271: Fix Template/Plugin/Branches.t

#   Failed test 'Without selected parameter, my library should be preselected'
    #   at t/db_dependent/Template/Plugin/Branches.t line 81.
    #          got: '0'
    #     expected: '1'
Comment 37 Jonathan Druart 2021-04-21 14:58:25 UTC
Follow-up pushed to master.
Comment 38 Jonathan Druart 2021-04-21 15:26:04 UTC
Created attachment 119957 [details] [review]
Bug 23271: Fix random failure from Categories.t

#   Failed test 'The branch limitation should have been stored and retrieved'
 #   at t/db_dependent/Koha/Patron/Categories.t line 57.
 #     Structures begin differing at:
 #          $got->[0] = 'fm0A1a'
 #     $expected->[0] = 'vNquBvCa'
Comment 39 Jonathan Druart 2021-04-21 15:26:43 UTC
Follow-up pushed to master.
Comment 40 Nick Clemens 2021-04-23 11:19:40 UTC
Created attachment 120054 [details] [review]
Bug 23271: (follow-up) Fix typo 'limlits'
Comment 41 Jonathan Druart 2021-04-23 11:56:07 UTC
Ooops, thanks Nick. Pushed to master!
Comment 42 Fridolin Somers 2021-04-29 13:10:00 UTC
Enhancement not pushed to 20.11.x