Bug 28031 - Koha::Patron::Attribute->_check_repeatable doesn't exclude the object's ID
Summary: Koha::Patron::Attribute->_check_repeatable doesn't exclude the object's ID
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 major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 27858 17828
Blocks: 23666 28162
  Show dependency treegraph
 
Reported: 2021-03-25 12:23 UTC by Tomás Cohen Arazi
Modified: 2021-04-29 13:19 UTC (History)
6 users (show)

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


Attachments
Bug 28031: Regression tests (1.92 KB, patch)
2021-03-25 13:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28031: Avoid wrong exception on saving updated attribute (1.68 KB, patch)
2021-03-25 13:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28031: (follow-up) Retrieve type only once (2.72 KB, patch)
2021-03-25 13:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28031: Regression tests (1.99 KB, patch)
2021-03-25 17:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28031: Avoid wrong exception on saving updated attribute (1.75 KB, patch)
2021-03-25 17:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28031: (follow-up) Retrieve type only once (2.78 KB, patch)
2021-03-25 17:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28031: (follow-up) Clarify check methods (4.73 KB, patch)
2021-03-25 17:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28031: (follow-up) Cache type instead of passing (2.04 KB, patch)
2021-03-25 17:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28031: Regression tests (2.06 KB, patch)
2021-03-26 14:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28031: Avoid wrong exception on saving updated attribute (1.82 KB, patch)
2021-03-26 14:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28031: (follow-up) Retrieve type only once (2.85 KB, patch)
2021-03-26 14:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28031: (follow-up) Clarify check methods (4.80 KB, patch)
2021-03-26 14:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28031: (follow-up) Cache type instead of passing (2.11 KB, patch)
2021-03-26 14:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28031: (QA follow-up) Adapt controllers and class (6.83 KB, patch)
2021-04-09 12:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28031: test remove type from parameter (3.96 KB, patch)
2021-04-09 13:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28031: Fix patron.t (975 bytes, patch)
2021-04-09 13:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28031: Remove type from parameter of *_ok methods (1.92 KB, patch)
2021-04-09 13:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28031: Add missing txn_begin in Attribute.t (1.04 KB, patch)
2021-04-09 13:37 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 2021-03-25 12:23:59 UTC
In the case of non-repeatable attributes, if you are adding a new attribute, the check works correctly.

But if you are editing an existing attribute, it fails, because it doesn't exclude it's own id from the search :-/
Comment 1 Tomás Cohen Arazi 2021-03-25 13:08:17 UTC
Created attachment 118792 [details] [review]
Bug 28031: Regression tests
Comment 2 Tomás Cohen Arazi 2021-03-25 13:08:22 UTC
Created attachment 118793 [details] [review]
Bug 28031: Avoid wrong exception on saving updated attribute

In the case a non-repeatable attribute for a patron is being updated
(its value) the routines that check the type is repeatable should
exclude its own id from the search for things to work.

This patch solves that.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron/Attribute.t
=> FAIL: Tests explode! An unexpected exception is thrown!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D
Comment 3 Tomás Cohen Arazi 2021-03-25 13:08:26 UTC
Created attachment 118794 [details] [review]
Bug 28031: (follow-up) Retrieve type only once

The current implementation of store+check_repeatable+check_unique_id
notably retrieves the related Koha::Patron::Attribute::Type object three
times.

This can be easily solved by retrieving it once and reusing. This patch
does that.

This changes the signature for the helper methods.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron/Attribute.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Verify the old _check_repeatable method is not used anywhere
   $ git grep _check_repeatable
=> SUCCESS: It is not!
5. Verify check_unique_id is not used anywhere, so no risk changing the
   signature
   $ git grep check_unique_id
=> SUCCESS: It is safe to update it!
6. Sign off :-D
Comment 4 Martin Renvoize 2021-03-25 17:26:09 UTC
Created attachment 118828 [details] [review]
Bug 28031: Regression tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2021-03-25 17:26:12 UTC
Created attachment 118829 [details] [review]
Bug 28031: Avoid wrong exception on saving updated attribute

In the case a non-repeatable attribute for a patron is being updated
(its value) the routines that check the type is repeatable should
exclude its own id from the search for things to work.

This patch solves that.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron/Attribute.t
=> FAIL: Tests explode! An unexpected exception is thrown!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2021-03-25 17:26:15 UTC
Created attachment 118830 [details] [review]
Bug 28031: (follow-up) Retrieve type only once

The current implementation of store+check_repeatable+check_unique_id
notably retrieves the related Koha::Patron::Attribute::Type object three
times.

This can be easily solved by retrieving it once and reusing. This patch
does that.

This changes the signature for the helper methods.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron/Attribute.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Verify the old _check_repeatable method is not used anywhere
   $ git grep _check_repeatable
=> SUCCESS: It is not!
5. Verify check_unique_id is not used anywhere, so no risk changing the
   signature
   $ git grep check_unique_id
=> SUCCESS: It is safe to update it!
6. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2021-03-25 17:26:18 UTC
Created attachment 118831 [details] [review]
Bug 28031: (follow-up) Clarify check methods

The check methods were positioned under the 'Internal methods' section
of the meodule but are used externally.

It also felt strange to have a noop or die method. Instead, I propose
renaming them to `repeatable_ok` and `unique_ok` and returning a
boolean denoting their state.
Comment 8 Martin Renvoize 2021-03-25 17:49:39 UTC
Created attachment 118833 [details] [review]
Bug 28031: (follow-up) Cache type instead of passing

We should cache the 'type' instead of passing it around. That way we're
not changing the signature further for external users of the methods.
Comment 9 Kyle M Hall 2021-03-26 14:32:21 UTC
Created attachment 118859 [details] [review]
Bug 28031: Regression tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall 2021-03-26 14:32:34 UTC
Created attachment 118860 [details] [review]
Bug 28031: Avoid wrong exception on saving updated attribute

In the case a non-repeatable attribute for a patron is being updated
(its value) the routines that check the type is repeatable should
exclude its own id from the search for things to work.

This patch solves that.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron/Attribute.t
=> FAIL: Tests explode! An unexpected exception is thrown!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Kyle M Hall 2021-03-26 14:32:37 UTC
Created attachment 118861 [details] [review]
Bug 28031: (follow-up) Retrieve type only once

The current implementation of store+check_repeatable+check_unique_id
notably retrieves the related Koha::Patron::Attribute::Type object three
times.

This can be easily solved by retrieving it once and reusing. This patch
does that.

This changes the signature for the helper methods.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron/Attribute.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Verify the old _check_repeatable method is not used anywhere
   $ git grep _check_repeatable
=> SUCCESS: It is not!
5. Verify check_unique_id is not used anywhere, so no risk changing the
   signature
   $ git grep check_unique_id
=> SUCCESS: It is safe to update it!
6. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Kyle M Hall 2021-03-26 14:32:40 UTC
Created attachment 118862 [details] [review]
Bug 28031: (follow-up) Clarify check methods

The check methods were positioned under the 'Internal methods' section
of the meodule but are used externally.

It also felt strange to have a noop or die method. Instead, I propose
renaming them to `repeatable_ok` and `unique_ok` and returning a
boolean denoting their state.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Kyle M Hall 2021-03-26 14:32:43 UTC
Created attachment 118863 [details] [review]
Bug 28031: (follow-up) Cache type instead of passing

We should cache the 'type' instead of passing it around. That way we're
not changing the signature further for external users of the methods.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Jonathan Druart 2021-04-09 08:12:47 UTC
1. 
Patches applied on top of the whole tree

    #   Failed test 'globally mandatory attributes tests'
    #   at t/db_dependent/Koha/Patron.t line 661.
    # Looks like you failed 1 test of 15.
t/db_dependent/Koha/Patron.t .. 7/7 
#   Failed test 'extended_attributes'
#   at t/db_dependent/Koha/Patron.t line 664.
Can't locate object method "value" via package "Koha::Exceptions::Patron::MissingMandatoryExtendedAttribute" at t/db_dependent/Koha/Patron.t line 650.


2.
Why Koha::Patron::Attribute->type does not look like

  return Koha::Patron::Attribute::Type->_new_from_dbic( $self->_result->code );

3. Additionally I don't agree with the "cache", simply because the following code will be wrong:

  $attr->type($new_code)->store;
  my $type = $attr->type; # will get the old type
Comment 15 Tomás Cohen Arazi 2021-04-09 10:40:39 UTC
(In reply to Jonathan Druart from comment #14)
> 1. 
> Patches applied on top of the whole tree
> 
>     #   Failed test 'globally mandatory attributes tests'
>     #   at t/db_dependent/Koha/Patron.t line 661.
>     # Looks like you failed 1 test of 15.
> t/db_dependent/Koha/Patron.t .. 7/7 
> #   Failed test 'extended_attributes'
> #   at t/db_dependent/Koha/Patron.t line 664.
> Can't locate object method "value" via package
> "Koha::Exceptions::Patron::MissingMandatoryExtendedAttribute" at
> t/db_dependent/Koha/Patron.t line 650.

Will check now.

> 2.
> Why Koha::Patron::Attribute->type does not look like
> 
>   return Koha::Patron::Attribute::Type->_new_from_dbic( $self->_result->code
> );

I didn't intend to change the method inline, I was tempted to, though. I thought of doing it on a separate bug, also adding the 'type' relationship so it can be followed through on embedding (API).

> 3. Additionally I don't agree with the "cache", simply because the following
> code will be wrong:
> 
>   $attr->type($new_code)->store;
>   my $type = $attr->type; # will get the old type

I agree caching is not as trivial. I changed the methods signature to accept the type to avoid fetching things twice, but then Martin found some uses of the methods in a controller so it felt like changing the signature would required changes outside.

Maybe an alternate patch making the param optional is a better approach?

my ($self, $type) = @_;
my $THE_type = $type // $self->type;
Comment 16 Tomás Cohen Arazi 2021-04-09 12:54:59 UTC
Created attachment 119377 [details] [review]
Bug 28031: (QA follow-up) Adapt controllers and class

This patch adapts the *memberentry.pl controllers so they pass the $type
parameter when calling unique_ok. The parameter would still be retrieved
from the DB if not passed, and then fetched again for reporting back
errors... So it makes sense to do it once and stick with this approach.

The $type parameter is made mandatory, and exceptions are thrown if
missing. Tests added only for this exception situation, as the other
cases are already covered in the store() tests.

Bonus: this patch also fixes a mistake made on a late follow-up for bug
27857 on the tests.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t \
           t/db_dependent/Koha/Patron/Attribute*
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Jonathan Druart 2021-04-09 13:11:55 UTC
Created attachment 119378 [details] [review]
Bug 28031: test remove type from parameter
Comment 18 Jonathan Druart 2021-04-09 13:28:11 UTC
Created attachment 119379 [details] [review]
Bug 28031: Fix patron.t
Comment 19 Jonathan Druart 2021-04-09 13:28:15 UTC
Created attachment 119380 [details] [review]
Bug 28031: Remove type from parameter of *_ok methods
Comment 20 Jonathan Druart 2021-04-09 13:37:55 UTC
Created attachment 119382 [details] [review]
Bug 28031: Add missing txn_begin in Attribute.t
Comment 21 Jonathan Druart 2021-04-09 14:09:43 UTC
The code looks much better without the "type" paramter passed to the *_ok methods IMO and I don't think we should care much about the additional fetch.
Comment 22 Jonathan Druart 2021-04-09 14:41:13 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 23 Magnus Enger 2021-04-29 12:37:08 UTC
I am having trouble with this on 20.05, I think. Any chance of a backport?
Comment 24 Jonathan Druart 2021-04-29 13:19:34 UTC
(In reply to Magnus Enger from comment #23)
> I am having trouble with this on 20.05, I think. Any chance of a backport?

This is depending on a several changes that are not in stable branches. What problem do you face exactly?