Bug 17828 - Koha::Patron::Attribute->store should raise an exception if unique_id is being broken
Summary: Koha::Patron::Attribute->store should raise an exception if unique_id is bein...
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:
Keywords:
Depends on: 13757
Blocks: 18339
  Show dependency treegraph
 
Reported: 2016-12-29 14:50 UTC by Tomás Cohen Arazi
Modified: 2017-12-07 22:16 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17828: Unit tests for Koha::Patron::Attribute->store (3.15 KB, patch)
2016-12-30 14:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17828: Overload Koha::Patron::Attribute->store to check for unique_id (2.83 KB, patch)
2016-12-30 14:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17828: Unit tests for Koha::Patron::Attribute->store (5.34 KB, patch)
2017-01-02 19:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17828: Overload Koha::Patron::Attribute->store to check for unique_id and repeatable (4.43 KB, patch)
2017-01-02 19:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17828: Unit tests for Koha::Patron::Attribute->store (5.40 KB, patch)
2017-03-23 08:38 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17828: Overload Koha::Patron::Attribute->store to check for unique_id and repeatable (4.48 KB, patch)
2017-03-23 08:38 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17828: (followup) Add Test::Exception as dependency (4.06 KB, patch)
2017-03-23 15:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17828: (QA followup) Add ->type and reuse it (3.36 KB, patch)
2017-03-27 15:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17828: Use Koha::Patron::Attributes instead of _resultset (1.30 KB, patch)
2017-03-28 14:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17828: Unit tests for Koha::Patron::Attribute->store (5.46 KB, patch)
2017-03-29 19:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17828: Unit tests for Koha::Patron::Attribute->store (5.46 KB, patch)
2017-03-29 19:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17828: Overload Koha::Patron::Attribute->store to check for unique_id and repeatable (4.54 KB, patch)
2017-03-29 19:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17828: (followup) Add Test::Exception as dependency (4.13 KB, patch)
2017-03-29 19:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17828: (QA followup) Add ->type and reuse it (3.43 KB, patch)
2017-03-29 19:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17828: Use Koha::Patron::Attributes instead of _resultset (1.37 KB, patch)
2017-03-29 19:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17828: return $self to be consistent (741 bytes, patch)
2017-03-29 19:49 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-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.