Koha::Patron::Attribute::Type should be used as example.
Created attachment 91368 [details] [review] Bug 23272: Make Koha::AuthorisedValue use Koha::Object::Limit::Library
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
Created attachment 91462 [details] [review] Bug 23272: Make Koha::AuthorisedValue use Koha::Object::Limit::Library Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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>
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'
(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!
(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.
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>
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>
Nice work! Pushed to master for 19.11.00
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>
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.
(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 ;)
Depends on Bug 23281 not in 19.05.x
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>
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);
(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
(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.
(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?