Bug 17828

Summary: Koha::Patron::Attribute->store should raise an exception if unique_id is being broken
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: enhancement    
Priority: P5 - low CC: katrin.fischer, kyle, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 13757    
Bug Blocks: 18339, 27833, 28031    
Attachments: Bug 17828: Unit tests for Koha::Patron::Attribute->store
Bug 17828: Overload Koha::Patron::Attribute->store to check for unique_id
Bug 17828: Unit tests for Koha::Patron::Attribute->store
Bug 17828: Overload Koha::Patron::Attribute->store to check for unique_id and repeatable
[SIGNED-OFF] Bug 17828: Unit tests for Koha::Patron::Attribute->store
[SIGNED-OFF] Bug 17828: Overload Koha::Patron::Attribute->store to check for unique_id and repeatable
Bug 17828: (followup) Add Test::Exception as dependency
Bug 17828: (QA followup) Add ->type and reuse it
Bug 17828: Use Koha::Patron::Attributes instead of _resultset
Bug 17828: Unit tests for Koha::Patron::Attribute->store
Bug 17828: Unit tests for Koha::Patron::Attribute->store
Bug 17828: Overload Koha::Patron::Attribute->store to check for unique_id and repeatable
Bug 17828: (followup) Add Test::Exception as dependency
Bug 17828: (QA followup) Add ->type and reuse it
Bug 17828: Use Koha::Patron::Attributes instead of _resultset
Bug 17828: return $self to be consistent

Description Tomás Cohen Arazi 2016-12-29 14:50:09 UTC
The new Koha::Patron::Attribute should check the unique_id property and raise an exception if storing the new attribute would break it.
Comment 1 Tomás Cohen Arazi 2016-12-30 14:17:33 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2016-12-30 14:17:37 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2017-01-02 18:17:23 UTC
This patch was wrong, re-doing it.
Comment 4 Tomás Cohen Arazi 2017-01-02 19:02:59 UTC
Created attachment 58557 [details] [review]
Bug 17828: Unit tests for Koha::Patron::Attribute->store

Unit tests for an overloaded Koha::Patron::Attribute->store that
checks attribute type's uniqueness property and raises an exception
conveniently.

It also tests for repeatable attribute type's property.

Test plan on the implementing patch.
Comment 5 Tomás Cohen Arazi 2017-01-02 19:03:11 UTC
Created attachment 58558 [details] [review]
Bug 17828: Overload Koha::Patron::Attribute->store to check for unique_id and repeatable

This patch introduces a Koha::Patron::Attribute->store method that does
the required check for the corresponding Koha::Patron::Attribute::Type
and raises an exception if trying to store more than one unique
attribute code/value combination, and another exception for
non-repeatable attribute types being stored while already existing on
the DB.

This patch introduces exception definitions for that purpose.

To test:
- Apply the unit tests patches
- Run:
  $ prove t/db_dependent/Koha/Patron/Attributes.t
=> FAIL: Tests fail
- Apply this patch
- Run:
  $ prove t/db_dependent/Koha/Patron/Attributes.t
=> SUCCESS: Tests pass!
- Sign off :-D
Comment 6 Josef Moravec 2017-03-23 08:38:24 UTC
Created attachment 61505 [details] [review]
[SIGNED-OFF] Bug 17828: Unit tests for Koha::Patron::Attribute->store

Unit tests for an overloaded Koha::Patron::Attribute->store that
checks attribute type's uniqueness property and raises an exception
conveniently.

It also tests for repeatable attribute type's property.

Test plan on the implementing patch.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 7 Josef Moravec 2017-03-23 08:38:32 UTC
Created attachment 61506 [details] [review]
[SIGNED-OFF] Bug 17828: Overload Koha::Patron::Attribute->store to check for unique_id and repeatable

This patch introduces a Koha::Patron::Attribute->store method that does
the required check for the corresponding Koha::Patron::Attribute::Type
and raises an exception if trying to store more than one unique
attribute code/value combination, and another exception for
non-repeatable attribute types being stored while already existing on
the DB.

This patch introduces exception definitions for that purpose.

To test:
- Apply the unit tests patches
- Run:
  $ prove t/db_dependent/Koha/Patron/Attributes.t
=> FAIL: Tests fail
- Apply this patch
- Run:
  $ prove t/db_dependent/Koha/Patron/Attributes.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 8 Tomás Cohen Arazi 2017-03-23 15:09:25 UTC
Created attachment 61547 [details] [review]
Bug 17828: (followup) Add Test::Exception as dependency

Test::Exception is available on Jessie and already installed when
koha-common gets installed

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Jonathan Druart 2017-03-23 20:53:08 UTC
The 2 Koha::Database->new->schema->resultset( $self->_type )->search call sounds weird to me.
It looks like you need to create a new method to return the type of the attribute (so return a the Koha::Patron::Attribute::Type related to $self->code).
Don't you think? That will simplify the code and make it more readable.

Switch back to SO if you disagree or if I am missing something.
Comment 10 Tomás Cohen Arazi 2017-03-27 15:49:00 UTC
Created attachment 61644 [details] [review]
Bug 17828: (QA followup) Add ->type and reuse it

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Jonathan Druart 2017-03-28 14:39:33 UTC
Created attachment 61662 [details] [review]
Bug 17828: Use Koha::Patron::Attributes instead of _resultset
Comment 12 Jonathan Druart 2017-03-28 14:44:53 UTC
Not sure about _check_repeatable and _check_unique_id, I will think a bit more about it.
Note that one returns $self not the other one
Comment 13 Tomás Cohen Arazi 2017-03-28 15:28:22 UTC
(In reply to Jonathan Druart from comment #12)
> Not sure about _check_repeatable and _check_unique_id, I will think a bit
> more about it.
> Note that one returns $self not the other one

Boo, I forgot that line!
Comment 14 Jonathan Druart 2017-03-29 19:43:12 UTC
Created attachment 61703 [details] [review]
Bug 17828: Unit tests for Koha::Patron::Attribute->store

Unit tests for an overloaded Koha::Patron::Attribute->store that
checks attribute type's uniqueness property and raises an exception
conveniently.

It also tests for repeatable attribute type's property.

Test plan on the implementing patch.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Jonathan Druart 2017-03-29 19:49:15 UTC
Created attachment 61704 [details] [review]
Bug 17828: Unit tests for Koha::Patron::Attribute->store

Unit tests for an overloaded Koha::Patron::Attribute->store that
checks attribute type's uniqueness property and raises an exception
conveniently.

It also tests for repeatable attribute type's property.

Test plan on the implementing patch.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Jonathan Druart 2017-03-29 19:49:29 UTC
Created attachment 61705 [details] [review]
Bug 17828: Overload Koha::Patron::Attribute->store to check for unique_id and repeatable

This patch introduces a Koha::Patron::Attribute->store method that does
the required check for the corresponding Koha::Patron::Attribute::Type
and raises an exception if trying to store more than one unique
attribute code/value combination, and another exception for
non-repeatable attribute types being stored while already existing on
the DB.

This patch introduces exception definitions for that purpose.

To test:
- Apply the unit tests patches
- Run:
  $ prove t/db_dependent/Koha/Patron/Attributes.t
=> FAIL: Tests fail
- Apply this patch
- Run:
  $ prove t/db_dependent/Koha/Patron/Attributes.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2017-03-29 19:49:36 UTC
Created attachment 61706 [details] [review]
Bug 17828: (followup) Add Test::Exception as dependency

Test::Exception is available on Jessie and already installed when
koha-common gets installed

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2017-03-29 19:49:45 UTC
Created attachment 61707 [details] [review]
Bug 17828: (QA followup) Add ->type and reuse it

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Jonathan Druart 2017-03-29 19:49:49 UTC
Created attachment 61708 [details] [review]
Bug 17828: Use Koha::Patron::Attributes instead of _resultset

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Jonathan Druart 2017-03-29 19:49:54 UTC
Created attachment 61709 [details] [review]
Bug 17828: return $self to be consistent

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Kyle M Hall 2017-03-31 15:17:30 UTC
Pushed to master for 17.05, thanks Tomas, Jonathan!
Comment 22 Katrin Fischer 2017-04-02 17:50:55 UTC
Dependency is not in 16.11.x, so this should not be needed there.