[Bug 27858] New: Make Koha::Patron::Attribute->store raise an exception on invalid type/code
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Bug ID: 27858 Summary: Make Koha::Patron::Attribute->store raise an exception on invalid type/code Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org Somehow, it goes through, and fails with "{UNKNOWN}: Can't call method "repeatable" on an undefined value" on the $self->_check_repeatable call. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |27833 Blocks| |27857 Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27833 [Bug 27833] Koha::Exceptions::Patron::Attribute::* should have parameters https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27857 [Bug 27857] Koha::Patron->extended_attributes skips checks -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 117678 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117678&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 117679 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117679&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 --- Comment #3 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 117680 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117680&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117678|0 |1 is obsolete| | --- Comment #4 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 117713 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117713&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117679|0 |1 is obsolete| | --- Comment #5 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 117714 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117714&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117680|0 |1 is obsolete| | --- Comment #6 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 117715 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117715&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |martin.renvoize@ptfs-europe | |.com --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Works as expected, code reads well and tests all pass. Signing off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com, | |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 --- Comment #8 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 118465 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118465&action=edit Bug 27858: (follow-up) Consistency in stringified exception Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joonas.kylmala@helsinki.fi -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |joonas.kylmala@helsinki.fi |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #9 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- 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'
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|joonas.kylmala@helsinki.fi |testopia@bugs.koha-communit | |y.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #118465|0 |1 is obsolete| | --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 118596 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118596&action=edit Bug 27858: (follow-up) Consistency in stringified exception Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 --- Comment #11 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |joonas.kylmala@helsinki.fi |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117713|0 |1 is obsolete| | Attachment #117714|0 |1 is obsolete| | Attachment #117715|0 |1 is obsolete| | Attachment #118596|0 |1 is obsolete| | --- Comment #12 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 118597 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118597&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 --- Comment #13 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 118598 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118598&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 --- Comment #14 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 118599 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118599&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 --- Comment #15 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 118600 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118600&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28031 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 [Bug 28031] Koha::Patron::Attribute->_check_repeatable doesn't exclude the object's ID -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28056 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28056 [Bug 28056] Add Koha::Exceptions::Patron::MissingMandatoryExtendedAttribute -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 --- Comment #17 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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.
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |21.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #19 from Fridolin Somers <fridolin.somers@biblibre.com> --- Enhancement not pushed to 20.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28220 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28220 [Bug 28220] Exception not caught when importing patrons -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org