Bug 23272 - Koha::AuthorisedValue should use Koha::Object::Limit::Library
Summary: Koha::AuthorisedValue should use Koha::Object::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: Marcel de Rooy
URL:
Keywords:
Depends on: 17755 23281
Blocks: 23830
  Show dependency treegraph
 
Reported: 2019-07-05 18:51 UTC by Tomás Cohen Arazi
Modified: 2019-10-17 07:45 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00


Attachments
Bug 23272: Make Koha::AuthorisedValue use Koha::Object::Limit::Library (3.56 KB, patch)
2019-07-05 19:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23272: Adapt controller and tests (5.42 KB, patch)
2019-07-05 19:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23272: Make Koha::AuthorisedValue use Koha::Object::Limit::Library (3.63 KB, patch)
2019-07-11 10:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23272: Adapt controller and tests (5.49 KB, patch)
2019-07-11 10:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23272: Make Koha::AuthorisedValue use Koha::Object::Limit::Library (3.72 KB, patch)
2019-09-13 06:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23272: Adapt controller and tests (5.57 KB, patch)
2019-09-13 06:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23272: (RM followup) Fix failing tests (1.49 KB, patch)
2019-09-13 10:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23272: (RM followup) Fix edit action (1.27 KB, patch)
2019-09-27 11:56 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 Tomás Cohen Arazi 2019-07-05 18:51:07 UTC
Koha::Patron::Attribute::Type should be used as example.
Comment 1 Tomás Cohen Arazi 2019-07-05 19:26:27 UTC
Created attachment 91368 [details] [review]
Bug 23272: Make Koha::AuthorisedValue use Koha::Object::Limit::Library
Comment 2 Tomás Cohen Arazi 2019-07-05 19:26:33 UTC
Created attachment 91369 [details] [review]
Bug 23272: Adapt controller and tests

This patch adapts the controller script for handling authorised values,
and the tests to reflect the method names changes.

To test:
- Verify you can add/delete/edit authorised values with/without branch
limitations, as usual
=> SUCCESS: All works as usual!
- Run:
  $ kshell
 k$ prove t/db_dependent/AuthorisedValues.t
=> SUCCESS: Tests pass!
- Sign off :-D
Comment 3 Kyle M Hall 2019-07-11 10:29:29 UTC
Created attachment 91462 [details] [review]
Bug 23272: Make Koha::AuthorisedValue use Koha::Object::Limit::Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 Kyle M Hall 2019-07-11 10:29:38 UTC
Created attachment 91463 [details] [review]
Bug 23272: Adapt controller and tests

This patch adapts the controller script for handling authorised values,
and the tests to reflect the method names changes.

To test:
- Verify you can add/delete/edit authorised values with/without branch
limitations, as usual
=> SUCCESS: All works as usual!
- Run:
  $ kshell
 k$ prove t/db_dependent/AuthorisedValues.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Katrin Fischer 2019-08-17 19:26:00 UTC
Hi Tomas,

can you please check this one?

 FAIL	Koha/AuthorisedValue.pm
   FAIL	  valid
		Inconsistent hierarchy during C3 merge of class 'Koha::AuthorisedValue':
			current merge results [
				Koha::AuthorisedValue,
			]
			merging failed on 'Koha::Object'
Comment 6 Tomás Cohen Arazi 2019-08-20 19:09:41 UTC
(In reply to Katrin Fischer from comment #5)
> Hi Tomas,
> 
> can you please check this one?
> 
>  FAIL	Koha/AuthorisedValue.pm
>    FAIL	  valid
> 		Inconsistent hierarchy during C3 merge of class 'Koha::AuthorisedValue':
> 			current merge results [
> 				Koha::AuthorisedValue,
> 			]
> 			merging failed on 'Koha::Object'

I have no clue about it. The change is pretty trivial IMHO, just removing many things, and changing the use base line. I'd say this is a bug in the QA tools, that shouldn't prevent this patches from making into the QA process!
Comment 7 Tomás Cohen Arazi 2019-08-22 18:20:22 UTC
(In reply to Katrin Fischer from comment #5)
> Hi Tomas,
> 
> can you please check this one?
> 
>  FAIL	Koha/AuthorisedValue.pm
>    FAIL	  valid
> 		Inconsistent hierarchy during C3 merge of class 'Koha::AuthorisedValue':
> 			current merge results [
> 				Koha::AuthorisedValue,
> 			]
> 			merging failed on 'Koha::Object'

It is a false positive, Katrin.
Comment 8 Marcel de Rooy 2019-09-13 06:03:56 UTC
Created attachment 92768 [details] [review]
Bug 23272: Make Koha::AuthorisedValue use Koha::Object::Limit::Library

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2019-09-13 06:04:01 UTC
Created attachment 92769 [details] [review]
Bug 23272: Adapt controller and tests

This patch adapts the controller script for handling authorised values,
and the tests to reflect the method names changes.

To test:
- Verify you can add/delete/edit authorised values with/without branch
limitations, as usual
=> SUCCESS: All works as usual!
- Run:
  $ kshell
 k$ prove t/db_dependent/AuthorisedValues.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Martin Renvoize 2019-09-13 07:05:06 UTC
Nice work!

Pushed to master for 19.11.00
Comment 11 Martin Renvoize 2019-09-13 10:22:17 UTC
Created attachment 92775 [details] [review]
Bug 23272: (RM followup) Fix failing tests

This bug introduced a regression whereby if any authorized value has no
branch limitations then the authorized value administration page would
fail to display at all.

A simple ternary corrects the issue, but I'm now wondering about the
logic of returning 'undef' from 'get_branch_limits'.. perhaps an empty
(or full) resultset might be a better option to prevent us from
requireing ternaries before calling chained methods on such objects.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2019-09-13 10:25:11 UTC
Just added a followup here to fix the failing tests.. as the commit says however I took the quick option of adding a ternary.. perhaps a longer discussion is in order to settle on a returns guideline for koha object methods.  To me, if an object is expected as the return we should always return an object (even if it's an empty/full object) rather than ever returning an implicit 'undef'. That would prevent the need for ternaries throughout the codebase checking the return value before chaining methods.

Thoughts.
Comment 13 Marcel de Rooy 2019-09-13 10:28:21 UTC
(In reply to Martin Renvoize from comment #12)
> Just added a followup here to fix the failing tests.. as the commit says
> however I took the quick option of adding a ternary.. perhaps a longer
> discussion is in order to settle on a returns guideline for koha object
> methods.  To me, if an object is expected as the return we should always
> return an object (even if it's an empty/full object) rather than ever
> returning an implicit 'undef'. That would prevent the need for ternaries
> throughout the codebase checking the return value before chaining methods.
> 
> Thoughts.

Good catch. Changing the returns of Koha objects is a nice exercise now ;)
Comment 14 Fridolin SOMERS 2019-09-13 13:20:24 UTC
Depends on Bug 23281 not in 19.05.x
Comment 15 Martin Renvoize 2019-09-27 11:56:41 UTC
Created attachment 93197 [details] [review]
Bug 23272: (RM followup) Fix edit action

This bug introduced a regression whereby if an authorized value has no
branch limitations then the authorized value cannot be edited once it
has been created. The edit page would display an error and not load.

This is the second case of a simple ternary correcting the issue, but
the logic of returning 'undef' from 'get_branch_limits' being called
into question.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Jonathan Druart 2019-10-16 06:37:32 UTC
Why do not we have Koha::Koha::AuthorisedValues inherit from Koha::Objects::Limit::Library (like Koha::ItemTypes and Koha::Patron::Attribute::Types?

% git grep "use.*Koha::Objects::Limit::Library"
Koha/ItemTypes.pm:use base qw(Koha::Objects Koha::Objects::Limit::Library);
Koha/Patron/Attribute/Types.pm:use base qw(Koha::Objects Koha::Objects::Limit::Library);
Comment 17 Tomás Cohen Arazi 2019-10-16 12:53:21 UTC
(In reply to Jonathan Druart from comment #16)
> Why do not we have Koha::Koha::AuthorisedValues inherit from
> Koha::Objects::Limit::Library (like Koha::ItemTypes and
> Koha::Patron::Attribute::Types?
> 
> % git grep "use.*Koha::Objects::Limit::Library"
> Koha/ItemTypes.pm:use base qw(Koha::Objects Koha::Objects::Limit::Library);
> Koha/Patron/Attribute/Types.pm:use base qw(Koha::Objects
> Koha::Objects::Limit::Library);

That's because it was added later... And I couldn't spend the time on that lately
Comment 18 Martin Renvoize 2019-10-17 07:45:49 UTC
(In reply to Jonathan Druart from comment #16)
> Why do not we have Koha::Koha::AuthorisedValues inherit from
> Koha::Objects::Limit::Library (like Koha::ItemTypes and
> Koha::Patron::Attribute::Types?
> 
> % git grep "use.*Koha::Objects::Limit::Library"
> Koha/ItemTypes.pm:use base qw(Koha::Objects Koha::Objects::Limit::Library);
> Koha/Patron/Attribute/Types.pm:use base qw(Koha::Objects
> Koha::Objects::Limit::Library);

I've added a bug for this part, bug 23830. Agreed, it would be great to get that case sorted aswell.