https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23272 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23272 --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 91369 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91369&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23272 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |23281 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23281 [Bug 23281] Add Koha::Objects::Limit::Library -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23272 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #91368|0 |1 is obsolete| | --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 91462 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91462&action=edit Bug 23272: Make Koha::AuthorisedValue use Koha::Object::Limit::Library Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23272 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de Status|Signed Off |Failed QA --- Comment #5 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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' -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23272 --- Comment #7 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23272 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23272 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #91463|0 |1 is obsolete| | --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 92769 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=92769&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23272 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |19.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23272 --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 92775 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=92775&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23272 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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 ;) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23272 --- Comment #15 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 93197 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93197&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23272 --- Comment #17 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23272 --- Comment #18 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23272 --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Tomás Cohen Arazi from comment #6)

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!

Thats too easy. This has nothing to do with QA tools. This is a regular perl warning about a rattling? object hierarchy? -- You are receiving this mail because: You are watching all bug changes.