Bug 23830 - Koha::AuthorisedValues should use Koha::Objects::Limit::Library
Summary: Koha::AuthorisedValues should use Koha::Objects::Limit::Library
Status: RESOLVED 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: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 17755 23272 23281
Blocks:
  Show dependency treegraph
 
Reported: 2019-10-17 07:44 UTC by Martin Renvoize
Modified: 2021-06-14 21:35 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
This enhancement makes Koha::AuthorisedValues 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 23830: Make Koha::AuthorisedValues use Koha::Objects::Limit::Library (3.88 KB, patch)
2021-01-28 20:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23830: Adapt callers (6.07 KB, patch)
2021-01-28 20:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23830: Make Koha::AuthorisedValues use Koha::Objects::Limit::Library (3.94 KB, patch)
2021-02-02 08:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23830: Adapt callers (6.12 KB, patch)
2021-02-02 08:02 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2019-10-17 07:44:26 UTC
Koha::Patron::Attribute::Types should be used as an example.
Comment 1 Tomás Cohen Arazi 2021-01-28 20:40:39 UTC
Created attachment 115989 [details] [review]
Bug 23830: Make Koha::AuthorisedValues use Koha::Objects::Limit::Library

This patch makes Koha::AuthorisedValues inherit the
'search_with_library_limits' method from Koha::Objects::Limit::Library.
This was left out of the original implementation and this patch makes
this library have a similar implementation to that of Koha::ItemTypes,
etc.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/AuthorisedValues.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Notice Koha::AuthorisedValues now inherits this new library
4. Notice the overloaded ->search method is removed
5. Repeat (1)
=> SUCCESS: The behavior of the newly used method on the tests is the
same as before. We only changed the method name and how we pass the
library_id.
6. Sign off :-D
Comment 2 Tomás Cohen Arazi 2021-01-28 20:40:51 UTC
Created attachment 115990 [details] [review]
Bug 23830: Adapt callers

This patch is the result of making the same changes we did on the
t/db_dependent/AuthorisedValues.t file (replacing the calls to
Koha::AuthorisedValues->search with the tricky branchcode param, and
call ->search_with_library_limits, with the library_id as a third
parameter.

What I did was:
   $ git grep 'Koha::AuthorisedValues\->search'

and then revisited each of the grep results to check if they added the
'branchcode' parameter to the filters.

This patch changes the calls to ->search, for
->search_with_library_limits in all the places that require it in the
current codebase [1].

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Charges/Sales.t \
           t/db_dependent/Ill*
3. Verify the batchmod.pl script is working and filtering the authorised
   values keeps working

[1] Some places like the Koha/Template/Plugin/AuthorisedValues.pm plugin
don't seem to be tested, at first glance.
Comment 3 Martin Renvoize 2021-02-02 08:02:25 UTC
Created attachment 116185 [details] [review]
Bug 23830: Make Koha::AuthorisedValues use Koha::Objects::Limit::Library

This patch makes Koha::AuthorisedValues inherit the
'search_with_library_limits' method from Koha::Objects::Limit::Library.
This was left out of the original implementation and this patch makes
this library have a similar implementation to that of Koha::ItemTypes,
etc.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/AuthorisedValues.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Notice Koha::AuthorisedValues now inherits this new library
4. Notice the overloaded ->search method is removed
5. Repeat (1)
=> SUCCESS: The behavior of the newly used method on the tests is the
same as before. We only changed the method name and how we pass the
library_id.
6. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2021-02-02 08:02:29 UTC
Created attachment 116186 [details] [review]
Bug 23830: Adapt callers

This patch is the result of making the same changes we did on the
t/db_dependent/AuthorisedValues.t file (replacing the calls to
Koha::AuthorisedValues->search with the tricky branchcode param, and
call ->search_with_library_limits, with the library_id as a third
parameter.

What I did was:
   $ git grep 'Koha::AuthorisedValues\->search'

and then revisited each of the grep results to check if they added the
'branchcode' parameter to the filters.

This patch changes the calls to ->search, for
->search_with_library_limits in all the places that require it in the
current codebase [1].

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Charges/Sales.t \
           t/db_dependent/Ill*
3. Verify the batchmod.pl script is working and filtering the authorised
   values keeps working

[1] Some places like the Koha/Template/Plugin/AuthorisedValues.pm plugin
don't seem to be tested, at first glance.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2021-02-02 08:03:33 UTC
Oooh, thanks for this Tomas.. I had entirely forgotten I'd reported this one!

Code is trivial and good, all cases seem covered.. test suit passes, QA script passes (well, except for a false positive on POD removal).

I think we can safely go for a straight to PQA on this one.!
Comment 6 Jonathan Druart 2021-02-08 16:04:41 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 7 Fridolin Somers 2021-02-12 14:27:14 UTC
Enhancement not pushed to 20.11.x