|
Description
Tomás Cohen Arazi (tcohen)
2019-07-05 18:50:20 UTC
Created attachment 94292 [details] [review] Bug 23271: Improve library limitation selection for item types Created attachment 94293 [details] [review] Bug 23271: Use Koha::Object::Limit::Library for Koha::Patron::Categories Created attachment 94294 [details] [review] Bug 23271: Replace search_limited with search_with_library_limits 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. (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. It would be great to have this in master ASAP and finish this cleanup. Can you please rebase? Created attachment 119503 [details] [review] Bug 23271: Improve library limitation selection for item types Created attachment 119504 [details] [review] Bug 23271: Use Koha::Object::Limit::Library for Koha::Patron::Categories Created attachment 119505 [details] [review] Bug 23271: Replace search_limited with search_with_library_limits 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.
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> Created attachment 119530 [details] [review] Bug 23271: Improve library limitation selection for item types Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> Created attachment 119532 [details] [review] Bug 23271: Replace search_limited with search_with_library_limits Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 119533 [details] [review] Bug 23271: (QA follow-up) Minor POD fix Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Jonathan, I submitted what I had in mind for you to ponder. 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. 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. 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> 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> 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> 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> 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> 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> All works as described and seems a solid code improvement... Signing off 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'
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 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> 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> 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> 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> 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> 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> 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. Pushed to master for 21.05, thanks to everybody involved! 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' Follow-up pushed to master. 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' Follow-up pushed to master. Created attachment 120054 [details] [review] Bug 23271: (follow-up) Fix typo 'limlits' Ooops, thanks Nick. Pushed to master! Enhancement not pushed to 20.11.x |