Bug 17755 - Introduce Koha::Patron::Attribute::Type(s)
Summary: Introduce Koha::Patron::Attribute::Type(s)
Status: CLOSED 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: Jonathan Druart
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 15449 17792 17825
  Show dependency treegraph
 
Reported: 2016-12-10 14:36 UTC by Tomás Cohen Arazi
Modified: 2017-12-07 22:16 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17755: Introduce Koha::Object::Library::Limit (6.47 KB, patch)
2016-12-12 17:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17755: Introduce Koha::Patron::Attribute::Type(s) (3.57 KB, patch)
2016-12-12 17:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17755: Unit tests (8.19 KB, patch)
2016-12-12 17:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17755: Introduce Koha::Patron::Attribute::Type(s) (3.54 KB, patch)
2016-12-12 17:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17755: (followup) Rename ::Library::Limit for ::Limit::Library (1.77 KB, patch)
2016-12-13 20:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17755: Introduce Koha::Object::Library::Limit (6.52 KB, patch)
2016-12-29 14:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17755: Unit tests (8.24 KB, patch)
2016-12-29 14:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17755: Introduce Koha::Patron::Attribute::Type(s) (3.59 KB, patch)
2016-12-29 14:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17755: (followup) Rename ::Library::Limit for ::Limit::Library (1.82 KB, patch)
2016-12-29 14:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17755: (QA followup) Return $self when appropriate (10.21 KB, patch)
2017-01-20 12:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17755: Introduce Koha::Object::Limit::Library (6.52 KB, patch)
2017-03-10 13:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17755: Unit tests (8.24 KB, patch)
2017-03-10 13:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17755: Introduce Koha::Patron::Attribute::Type(s) (3.59 KB, patch)
2017-03-10 13:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17755: (QA followup) Return $self when appropriate (10.16 KB, patch)
2017-03-10 13:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17755: (followup) Override ->search to allow filtering by branchcode (5.10 KB, patch)
2017-03-10 15:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17755: (QA followup) Return $self when appropriate (10.21 KB, patch)
2017-03-10 17:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17755: (followup) Override ->search to allow filtering by branchcode (5.15 KB, patch)
2017-03-10 17:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17755: Introduce Koha::Object::Limit::Library (6.60 KB, patch)
2017-03-23 15:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17755: Unit tests (8.31 KB, patch)
2017-03-23 15:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17755: Introduce Koha::Patron::Attribute::Type(s) (3.66 KB, patch)
2017-03-23 15:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17755: (QA followup) Return $self when appropriate (10.28 KB, patch)
2017-03-23 15:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17755: (followup) Override ->search to allow filtering by branchcode (5.23 KB, patch)
2017-03-23 15:54 UTC, Jonathan Druart
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 2016-12-10 14:36:26 UTC

    
Comment 1 Tomás Cohen Arazi 2016-12-12 17:28:42 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.
Comment 2 Tomás Cohen Arazi 2016-12-12 17:28:45 UTC
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.
Comment 3 Tomás Cohen Arazi 2016-12-12 17:28:48 UTC
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
Comment 4 Tomás Cohen Arazi 2016-12-12 17:50:24 UTC
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.
Comment 5 Tomás Cohen Arazi 2016-12-13 20:04:36 UTC
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.
Comment 6 Chris Kirby 2016-12-25 19:02:13 UTC
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"
Comment 7 Tomás Cohen Arazi 2016-12-26 15:39:56 UTC
(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.
Comment 8 Kyle M Hall 2016-12-29 14:49:41 UTC
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>
Comment 9 Kyle M Hall 2016-12-29 14:49:49 UTC
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>
Comment 10 Kyle M Hall 2016-12-29 14:49:52 UTC
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>
Comment 11 Kyle M Hall 2016-12-29 14:49:55 UTC
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>
Comment 12 Jonathan Druart 2017-01-11 09:13:01 UTC
Would not it make more sense to make library_limits returns Koha::Libraries instead of \@branchcodes?
Comment 13 Tomás Cohen Arazi 2017-01-11 10:43:25 UTC
(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.
Comment 14 Tomás Cohen Arazi 2017-01-18 22:01:06 UTC
(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?
Comment 15 Jonathan Druart 2017-01-19 07:51:56 UTC
To return $self I think.
Comment 16 Tomás Cohen Arazi 2017-01-20 12:45:06 UTC
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
Comment 17 Marcel de Rooy 2017-03-10 08:04:27 UTC
 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 ?
Comment 18 Tomás Cohen Arazi 2017-03-10 13:49:37 UTC
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>
Comment 19 Tomás Cohen Arazi 2017-03-10 13:49:47 UTC
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>
Comment 20 Tomás Cohen Arazi 2017-03-10 13:49:55 UTC
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>
Comment 21 Tomás Cohen Arazi 2017-03-10 13:50:02 UTC
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
Comment 22 Tomás Cohen Arazi 2017-03-10 15:33:09 UTC
Created attachment 60995 [details] [review]
Bug 17755: (followup) Override ->search to allow filtering by branchcode

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Kyle M Hall 2017-03-10 17:46:23 UTC
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>
Comment 24 Kyle M Hall 2017-03-10 17:46:30 UTC
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>
Comment 25 Jonathan Druart 2017-03-23 15:54:10 UTC
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>
Comment 26 Jonathan Druart 2017-03-23 15:54:14 UTC
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>
Comment 27 Jonathan Druart 2017-03-23 15:54:19 UTC
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>
Comment 28 Jonathan Druart 2017-03-23 15:54:22 UTC
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>
Comment 29 Jonathan Druart 2017-03-23 15:54:26 UTC
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>
Comment 30 Jonathan Druart 2017-03-23 16:02:04 UTC
QAed along withbug 17792 and bug 13757.
Please push them altogether.
Comment 31 Kyle M Hall 2017-03-24 18:43:27 UTC
Pushed to master for 17.05, thanks Tomas!
Comment 32 Katrin Fischer 2017-03-26 21:11:58 UTC
This won't get ported back to 16.11.x as it is an enhancement.