Somehow, it goes through, and fails with "{UNKNOWN}: Can't call method "repeatable" on an undefined value" on the $self->_check_repeatable call.
Created attachment 117678 [details] [review] Bug 27858: Reorganize tests This patch renames the tests file, and also groups subtests by method. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Patron/Attribute.t => SUCCESS: Tests pass! 3. Sign off :-D
Created attachment 117679 [details] [review] Bug 27858: Unit tests This patch introduces unit tests for the new exception to be thrown. It generates a new extended attribute type, keeps the randomly generated 'code' and deletes the object. This way we can be sure the code doesn't exist on the DB. It then tries to generate an attribute of that type, to force the exception to be thrown.
Created attachment 117680 [details] [review] Bug 27858: Make Koha::Patron::Attribute->store raise an exception on invalid type/code This patch adds type validation to the ->store method. And exception should be thrown if the type is invalid. Things currently explode in such situations, so this doesn't change the behavior (just adding more informative failure information). To test: 1. Apply the previous patches 2. Run: $ kshell k$ prove t/db_dependent/Koha/Patron/Attribute.t => FAIL: No exception is thrown on the bad case, weird error 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! Exception thrown! 5. Sign off :-D
Created attachment 117713 [details] [review] Bug 27858: Reorganize tests This patch renames the tests file, and also groups subtests by method. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Patron/Attribute.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 117714 [details] [review] Bug 27858: Unit tests This patch introduces unit tests for the new exception to be thrown. It generates a new extended attribute type, keeps the randomly generated 'code' and deletes the object. This way we can be sure the code doesn't exist on the DB. It then tries to generate an attribute of that type, to force the exception to be thrown. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 117715 [details] [review] Bug 27858: Make Koha::Patron::Attribute->store raise an exception on invalid type/code This patch adds type validation to the ->store method. And exception should be thrown if the type is invalid. Things currently explode in such situations, so this doesn't change the behavior (just adding more informative failure information). To test: 1. Apply the previous patches 2. Run: $ kshell k$ prove t/db_dependent/Koha/Patron/Attribute.t => FAIL: No exception is thrown on the bad case, weird error 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! Exception thrown! 5. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Works as expected, code reads well and tests all pass. Signing off
Created attachment 118465 [details] [review] Bug 27858: (follow-up) Consistency in stringified exception Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Tomás, please redo the last follow-up patch, it breaks the tests: > # got: 'Your action breaks a unique constraint on the attribute. type=UPAc_e value=Foo' > # expected: 'Your action breaks a unique constraint on the attribute. code=UPAc_e attribute=Foo'
Created attachment 118596 [details] [review] Bug 27858: (follow-up) Consistency in stringified exception Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Joonas Kylmälä from comment #9) > Tomás, please redo the last follow-up patch, it breaks the tests: > > > # got: 'Your action breaks a unique constraint on the attribute. type=UPAc_e value=Foo' > > # expected: 'Your action breaks a unique constraint on the attribute. code=UPAc_e attribute=Foo' Good catch, sorry for that. I fixed it now.
Created attachment 118597 [details] [review] Bug 27858: Reorganize tests This patch renames the tests file, and also groups subtests by method. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Patron/Attribute.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 118598 [details] [review] Bug 27858: Unit tests This patch introduces unit tests for the new exception to be thrown. It generates a new extended attribute type, keeps the randomly generated 'code' and deletes the object. This way we can be sure the code doesn't exist on the DB. It then tries to generate an attribute of that type, to force the exception to be thrown. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 118599 [details] [review] Bug 27858: Make Koha::Patron::Attribute->store raise an exception on invalid type/code This patch adds type validation to the ->store method. And exception should be thrown if the type is invalid. Things currently explode in such situations, so this doesn't change the behavior (just adding more informative failure information). To test: 1. Apply the previous patches 2. Run: $ kshell k$ prove t/db_dependent/Koha/Patron/Attribute.t => FAIL: No exception is thrown on the bad case, weird error 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! Exception thrown! 5. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 118600 [details] [review] Bug 27858: (follow-up) Consistency in stringified exception Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
What are we exactly trying to do here? Does it mean we are going to have one exception for each our relations/FKs? We already have the ORM/DBI that is telling us what's happening if we are doing that.
(In reply to Jonathan Druart from comment #16) > What are we exactly trying to do here? > > Does it mean we are going to have one exception for each our relations/FKs? > We already have the ORM/DBI that is telling us what's happening if we are > doing that. No, here it is only needed because before saving the attribute to the DB we want to make sure the Attribute Type is marked as repeatable (if adding more than 1 instance of this attribute) so we try to fetch that Attribute Types info from the DB and if it doesn't exists the code would fail. It is done in the subroutine that is called in the store function here, _check_repeatable(). It is important to catch this because e.g. via REST API somebody might try to add attribute with non-existent attribute type. See example usage in https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118701 > Can't locate object method "type" via package "Can't call method "repeatable" on > an undefined value at /kohadevbox/koha/Koha/Patron/Attribute.pm line 145. > " (perhaps you forgot to load "Can't call method "repeatable" on an undefined > value at /kohadevbox/koha/Koha/Patron/Attribute.pm line 145.
Pushed to master for 21.05, thanks to everybody involved!
Enhancement not pushed to 20.11.x