Bug 27858 - Make Koha::Patron::Attribute->store raise an exception on invalid type/code
Summary: Make Koha::Patron::Attribute->store raise an exception on invalid type/code
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on: 27833
Blocks: 27857 28031 28056 28220
  Show dependency treegraph
 
Reported: 2021-03-04 11:06 UTC by Tomás Cohen Arazi
Modified: 2021-08-13 08:50 UTC (History)
6 users (show)

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


Attachments
Bug 27858: Reorganize tests (15.14 KB, patch)
2021-03-04 11:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27858: Unit tests (2.24 KB, patch)
2021-03-04 11:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27858: Make Koha::Patron::Attribute->store raise an exception on invalid type/code (1.25 KB, patch)
2021-03-04 11:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27858: Reorganize tests (15.20 KB, patch)
2021-03-04 12:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27858: Unit tests (2.30 KB, patch)
2021-03-04 12:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27858: Make Koha::Patron::Attribute->store raise an exception on invalid type/code (1.32 KB, patch)
2021-03-04 12:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27858: (follow-up) Consistency in stringified exception (2.40 KB, patch)
2021-03-18 15:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27858: (follow-up) Consistency in stringified exception (3.52 KB, patch)
2021-03-22 13:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27858: Reorganize tests (15.36 KB, patch)
2021-03-22 14:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27858: Unit tests (2.46 KB, patch)
2021-03-22 14:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27858: Make Koha::Patron::Attribute->store raise an exception on invalid type/code (1.46 KB, patch)
2021-03-22 14:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27858: (follow-up) Consistency in stringified exception (3.66 KB, patch)
2021-03-22 14:00 UTC, Joonas Kylmälä
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 2021-03-04 11:06:26 UTC
Somehow, it goes through, and fails with "{UNKNOWN}: Can't call method "repeatable" on an undefined value" on the $self->_check_repeatable call.
Comment 1 Tomás Cohen Arazi 2021-03-04 11:32:29 UTC
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
Comment 2 Tomás Cohen Arazi 2021-03-04 11:32:32 UTC
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.
Comment 3 Tomás Cohen Arazi 2021-03-04 11:32:36 UTC
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
Comment 4 Martin Renvoize 2021-03-04 12:46:13 UTC
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>
Comment 5 Martin Renvoize 2021-03-04 12:46:16 UTC
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>
Comment 6 Martin Renvoize 2021-03-04 12:46:19 UTC
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>
Comment 7 Martin Renvoize 2021-03-04 12:46:48 UTC
Works as expected, code reads well and tests all pass.

Signing off
Comment 8 Tomás Cohen Arazi 2021-03-18 15:07:26 UTC
Created attachment 118465 [details] [review]
Bug 27858: (follow-up) Consistency in stringified exception

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Joonas Kylmälä 2021-03-22 08:42:06 UTC
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'
Comment 10 Tomás Cohen Arazi 2021-03-22 13:54:25 UTC
Created attachment 118596 [details] [review]
Bug 27858: (follow-up) Consistency in stringified exception

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2021-03-22 13:55:02 UTC
(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.
Comment 12 Joonas Kylmälä 2021-03-22 14:00:06 UTC
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>
Comment 13 Joonas Kylmälä 2021-03-22 14:00:09 UTC
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>
Comment 14 Joonas Kylmälä 2021-03-22 14:00:13 UTC
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>
Comment 15 Joonas Kylmälä 2021-03-22 14:00:16 UTC
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>
Comment 16 Jonathan Druart 2021-04-09 08:15:36 UTC
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.
Comment 17 Joonas Kylmälä 2021-04-09 08:40:33 UTC
(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.
Comment 18 Jonathan Druart 2021-04-09 14:41:08 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 19 Fridolin Somers 2021-04-19 10:14:51 UTC
Enhancement not pushed to 20.11.x