Description
Tomás Cohen Arazi
2016-12-10 14:36:26 UTC
Created attachment 58142 [details] [review] Bug 17755: Introduce Koha::Object::Library::Limit This patch introduces a new class for extending Koha::Object using multiple inheritance. It cannot be used standalone, it needs to be used in Koha::Object implementations like this: use base qw( Koha::Object Koha::Object::Library::Limit ); Its goal is to provide a single way and place to deal with this common pattern in Koha's codebase. As it happened with Koha::Object, that needed to be tested in a real object class, this work was done on top of Koha::Patron::Attribute::Type implementation and it is fully covered by the tests that are introduced for it. Created attachment 58143 [details] [review] Bug 17755: Introduce Koha::Patron::Attribute::Type(s) This patch introduces the Koha::Object-based classes for handling patron attribute types. It also adds branch limitation handling to the Koha::Patron::Attribute::Type class. It is built on top of the new Koha::Object::Library::Limit class that extends Koha::Object so it handles library limits. Created attachment 58144 [details] [review] Bug 17755: Unit tests This patch introduces unit tests for Koha::Object::Library::Limit. It is done this way because it needs to be instantiated to be usable. To test: - Run: $ prove t/db_dependent/Koha/Patron/Attribute/Types.t => SUCCESS: Tests pass - Sign off :-D Created attachment 58145 [details] [review] Bug 17755: Introduce Koha::Patron::Attribute::Type(s) This patch introduces the Koha::Object-based classes for handling patron attribute types. It also adds branch limitation handling to the Koha::Patron::Attribute::Type class. It is built on top of the new Koha::Object::Library::Limit class that extends Koha::Object so it handles library limits. Created attachment 58167 [details] [review] Bug 17755: (followup) Rename ::Library::Limit for ::Limit::Library It makes sense to rename it, as there might be other limits defined, not just library-wise. Error: "Can't locate object method 'throw' via package 'Koha::Exceptions::CannotAddLibraryLimit' (perhaps you forgot to load 'Koha::Exceptions::CannotAddLibraryLimit'?) at Koha/Object/Limit/Library.pm" (In reply to Chris Kirby from comment #6) > Error: "Can't locate object method 'throw' via package > 'Koha::Exceptions::CannotAddLibraryLimit' (perhaps you forgot to load > 'Koha::Exceptions::CannotAddLibraryLimit'?) at Koha/Object/Limit/Library.pm" Chris, please find me on IRC or email to check what's going on. Things work fine for me. Created attachment 58495 [details] [review] Bug 17755: Introduce Koha::Object::Library::Limit This patch introduces a new class for extending Koha::Object using multiple inheritance. It cannot be used standalone, it needs to be used in Koha::Object implementations like this: use base qw( Koha::Object Koha::Object::Library::Limit ); Its goal is to provide a single way and place to deal with this common pattern in Koha's codebase. As it happened with Koha::Object, that needed to be tested in a real object class, this work was done on top of Koha::Patron::Attribute::Type implementation and it is fully covered by the tests that are introduced for it. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 58496 [details] [review] Bug 17755: Unit tests This patch introduces unit tests for Koha::Object::Library::Limit. It is done this way because it needs to be instantiated to be usable. To test: - Run: $ prove t/db_dependent/Koha/Patron/Attribute/Types.t => SUCCESS: Tests pass - Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 58497 [details] [review] Bug 17755: Introduce Koha::Patron::Attribute::Type(s) This patch introduces the Koha::Object-based classes for handling patron attribute types. It also adds branch limitation handling to the Koha::Patron::Attribute::Type class. It is built on top of the new Koha::Object::Library::Limit class that extends Koha::Object so it handles library limits. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 58498 [details] [review] Bug 17755: (followup) Rename ::Library::Limit for ::Limit::Library It makes sense to rename it, as there might be other limits defined, not just library-wise. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Would not it make more sense to make library_limits returns Koha::Libraries instead of \@branchcodes? (In reply to Jonathan Druart from comment #12) > Would not it make more sense to make library_limits returns Koha::Libraries > instead of \@branchcodes? I'm not sure. It takes its data from borrower_attribute_types_branch Maybe you can check how it is used and see if the overhead is worth. Once I get to the office we can discuss it, but I'm not against this. I'm not specially attached to this api. (In reply to Jonathan Druart from comment #12) > Would not it make more sense to make library_limits returns Koha::Libraries > instead of \@branchcodes? What behaviour would you expect when assigning library_limits? To return $self I think. Created attachment 59354 [details] [review] Bug 17755: (QA followup) Return $self when appropriate As failure situations raise exceptions that should be handled outside the object class, methods should return $self so successive calls can be chained nicely. This patch makes methods return $self and adjusts the tests to reflect this change. Make sure tests pass: - Run: $ prove t/db_dependent/Koha/Patron/Attribute/Types.t => SUCCESS: Tests return green - Sign off :-D FAIL Koha/Object/Library/Limit.pm OK critic OK forbidden patterns FAIL git manipulation The file has been added and deleted in the same patchset OK pod SKIP spelling OK valid FAIL Koha/Object/Limit/Library.pm FAIL valid Can't locate Data/Printer.pm in @INC Debugging leftover ? Created attachment 60989 [details] [review] Bug 17755: Introduce Koha::Object::Limit::Library This patch introduces a new class for extending Koha::Object using multiple inheritance. It cannot be used standalone, it needs to be used in Koha::Object implementations like this: use base qw( Koha::Object Koha::Object::Limit::Library ); Its goal is to provide a single way and place to deal with this common pattern in Koha's codebase. As it happened with Koha::Object, that needed to be tested in a real object class, this work was done on top of Koha::Patron::Attribute::Type implementation and it is fully covered by the tests that are introduced for it. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 60990 [details] [review] Bug 17755: Unit tests This patch introduces unit tests for Koha::Object::Library::Limit. It is done this way because it needs to be instantiated to be usable. To test: - Run: $ prove t/db_dependent/Koha/Patron/Attribute/Types.t => SUCCESS: Tests pass - Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 60991 [details] [review] Bug 17755: Introduce Koha::Patron::Attribute::Type(s) This patch introduces the Koha::Object-based classes for handling patron attribute types. It also adds branch limitation handling to the Koha::Patron::Attribute::Type class. It is built on top of the new Koha::Object::Library::Limit class that extends Koha::Object so it handles library limits. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 60992 [details] [review] Bug 17755: (QA followup) Return $self when appropriate As failure situations raise exceptions that should be handled outside the object class, methods should return $self so successive calls can be chained nicely. This patch makes methods return $self and adjusts the tests to reflect this change. Make sure tests pass: - Run: $ prove t/db_dependent/Koha/Patron/Attribute/Types.t => SUCCESS: Tests return green - Sign off :-D Created attachment 60995 [details] [review] Bug 17755: (followup) Override ->search to allow filtering by branchcode Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 61000 [details] [review] Bug 17755: (QA followup) Return $self when appropriate As failure situations raise exceptions that should be handled outside the object class, methods should return $self so successive calls can be chained nicely. This patch makes methods return $self and adjusts the tests to reflect this change. Make sure tests pass: - Run: $ prove t/db_dependent/Koha/Patron/Attribute/Types.t => SUCCESS: Tests return green - Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 61001 [details] [review] Bug 17755: (followup) Override ->search to allow filtering by branchcode Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 61548 [details] [review] Bug 17755: Introduce Koha::Object::Limit::Library This patch introduces a new class for extending Koha::Object using multiple inheritance. It cannot be used standalone, it needs to be used in Koha::Object implementations like this: use base qw( Koha::Object Koha::Object::Limit::Library ); Its goal is to provide a single way and place to deal with this common pattern in Koha's codebase. As it happened with Koha::Object, that needed to be tested in a real object class, this work was done on top of Koha::Patron::Attribute::Type implementation and it is fully covered by the tests that are introduced for it. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 61549 [details] [review] Bug 17755: Unit tests This patch introduces unit tests for Koha::Object::Library::Limit. It is done this way because it needs to be instantiated to be usable. To test: - Run: $ prove t/db_dependent/Koha/Patron/Attribute/Types.t => SUCCESS: Tests pass - Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 61550 [details] [review] Bug 17755: Introduce Koha::Patron::Attribute::Type(s) This patch introduces the Koha::Object-based classes for handling patron attribute types. It also adds branch limitation handling to the Koha::Patron::Attribute::Type class. It is built on top of the new Koha::Object::Library::Limit class that extends Koha::Object so it handles library limits. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 61551 [details] [review] Bug 17755: (QA followup) Return $self when appropriate As failure situations raise exceptions that should be handled outside the object class, methods should return $self so successive calls can be chained nicely. This patch makes methods return $self and adjusts the tests to reflect this change. Make sure tests pass: - Run: $ prove t/db_dependent/Koha/Patron/Attribute/Types.t => SUCCESS: Tests return green - Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 61552 [details] [review] Bug 17755: (followup) Override ->search to allow filtering by branchcode Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> QAed along withbug 17792 and bug 13757. Please push them altogether. Pushed to master for 17.05, thanks Tomas! This won't get ported back to 16.11.x as it is an enhancement. perl -cw Koha/Patron/Attribute/Type.pm Inconsistent hierarchy during C3 merge of class 'Koha::Patron::Attribute::Type': current merge results [ Koha::Patron::Attribute::Type, ] merging failed on 'Koha::Object' at /usr/lib/x86_64-linux-gnu/perl-base/base.pm line 186. BEGIN failed--compilation aborted at Koha/Patron/Attribute/Type.pm line 23. |