The new Koha::Patron::Attribute should check the unique_id property and raise an exception if storing the new attribute would break it.
Created attachment 58526 [details] [review] Bug 17828: Unit tests for Koha::Patron::Attribute->store 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. Test plan on the implementing patch.
Created attachment 58527 [details] [review] Bug 17828: Overload Koha::Patron::Attribute->store to check for unique_id 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. This patch introduces an exception definition 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
This patch was wrong, re-doing it.
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.
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
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>
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>
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>
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.
Created attachment 61644 [details] [review] Bug 17828: (QA followup) Add ->type and reuse it Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 61662 [details] [review] Bug 17828: Use Koha::Patron::Attributes instead of _resultset
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
(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!
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>
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>
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>
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>
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>
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>
Created attachment 61709 [details] [review] Bug 17828: return $self to be consistent Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Pushed to master for 17.05, thanks Tomas, Jonathan!
Dependency is not in 16.11.x, so this should not be needed there.