Bug 27857 - Koha::Patron->extended_attributes skips checks
Summary: Koha::Patron->extended_attributes skips checks
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: Testopia
URL:
Keywords:
Depends on: 27858 28056
Blocks: 23666 27853 27956 28009 28516
  Show dependency treegraph
 
Reported: 2021-03-04 10:02 UTC by Tomás Cohen Arazi
Modified: 2021-06-04 13:03 UTC (History)
8 users (show)

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


Attachments
Bug 27857: Move extended_attributes tests to Koha/Patron.t (15.46 KB, patch)
2021-03-04 12:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27857: Regression tests (10.15 KB, patch)
2021-03-04 12:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27857: Make Koha::Patron->extended_attributes validate data (1.62 KB, patch)
2021-03-04 12:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27857: Add handling for globally mandatory attributes (3.56 KB, patch)
2021-03-08 17:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27857: Move extended_attributes tests to Koha/Patron.t (15.52 KB, patch)
2021-03-08 18:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 27857: Regression tests (10.21 KB, patch)
2021-03-08 18:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 27857: Make Koha::Patron->extended_attributes validate data (1.67 KB, patch)
2021-03-08 18:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 27857: Add handling for globally mandatory attributes (3.61 KB, patch)
2021-03-08 18:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 27857: Add handling for globally mandatory attributes (3.74 KB, patch)
2021-03-09 08:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27857: Move extended_attributes tests to Koha/Patron.t (15.57 KB, patch)
2021-03-09 08:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27857: Regression tests (10.26 KB, patch)
2021-03-09 08:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27857: Make Koha::Patron->extended_attributes validate data (1.73 KB, patch)
2021-03-09 08:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27857: Add handling for globally mandatory attributes (3.74 KB, patch)
2021-03-09 08:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27857: (use-it-in-real-life follow-up) Use a more fine-grained exception for mandatory attributes (1.75 KB, patch)
2021-03-30 14:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27857: Get the right column (2.89 KB, patch)
2021-03-31 11:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27857: (follow-up) Use a more fine-grained exception for mandatory attributes (2.16 KB, patch)
2021-03-31 11:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27857: Fix Pseudonymization.t (1.73 KB, patch)
2021-04-12 09:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27857: Start a transaction foreach subtest (3.19 KB, patch)
2021-04-21 07:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27857: Remove unecessary âttribute_type3 var (1.07 KB, patch)
2021-04-21 07:48 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-04 10:02:57 UTC
Patron attributes can have some requirements:
- Be unique across the DB
- (Not) bd repeatable

This is correctly handled by both
- Koha::Patron::Attribute->store
- Koha::Patron->add_extended_attribute

But then, there's Koha::Patron->extended_attributes, that can overwrite them all at once, but doesn't rely on those methods, and uses DBIC relations instead, so no checks on attribute constraints are made.

We need to fix it.
Comment 1 Tomás Cohen Arazi 2021-03-04 12:43:14 UTC
Created attachment 117709 [details] [review]
Bug 27857: Move extended_attributes tests to Koha/Patron.t

They really belong here. both Patron.t and Patrons.t got lengthy and it
is not intuitive to look for tests for a Koha::Patron method in the
Patrons.t file.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t \
           t/db_dependent/Koha/Patrons.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass, no new stuff
4. Sign off :-D
Comment 2 Tomás Cohen Arazi 2021-03-04 12:43:18 UTC
Created attachment 117710 [details] [review]
Bug 27857: Regression tests

This patch adds regression tests for the changed behavior.
Koha::Patron->extended_attributes will now throw exceptions and rollback
on invalid attributes.

The existing tests are amended so they don't try to deal with skipping
bad attributes.

Specific subtests are added for error situations.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t
=> FAIL: Tests fail because the new behavior is not implemented.
Comment 3 Tomás Cohen Arazi 2021-03-04 12:43:24 UTC
Created attachment 117711 [details] [review]
Bug 27857: Make Koha::Patron->extended_attributes validate data

This patch makes the extended_attributes method use
Koha::Patron->add_extended_attribute inside the loop, instead of direct
DB access. This way, it takes advantage of the validation that is done
there.

All happens inside a transaction, so if 'add_extended_attribute' throws
an exception, everything is rolled back.

To test:
1. Apply the previous patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t
=> FAIL: The behavior is not the correct one, tests fail
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D
Comment 4 Martin Renvoize 2021-03-04 13:27:05 UTC
Whilst I'm reviewing this..

* We need to add handling for mandatory attributes - Working on this
* Why do we filter the delete by the logged in users branch (not the patrons branch.. or no filtering at all?)
Comment 5 Martin Renvoize 2021-03-05 15:49:01 UTC
# Get a list of required attributes
my $required_attribute_types =
    Koha::Patron::Attribute::Types->search_with_library_limits(
        { mandatory => 1 },
        {}, $self->library->branchcode )->get_column('type');

We got stuck at deciding what mandatory actually means.. right now we delete all attributes for the patron at the library the staff member is logged in at (the patron may well have a different home library).

Should we be checking against their home library for mandatory attributes.. or the staff users library?

The above query gets the patrons home libraries mandatory attribute list.. but until we know the answer to this intermediary question it's hard to know how/when/what error to throw.
Comment 6 Martin Renvoize 2021-03-08 16:48:26 UTC
The more I think about this, the more I think perhaps that 'mandatory' is a purely UI concept in this case.

I can see all sorts of reasons you may have added mandatory attributes.. but actually enforcing them would likely need to happen at whatever point the attribute should be getting looked at rather than at the point at which it's entered.

Examples

* An attribute that denotes permission to access certain resources at a certain branch.
- This attribute is 'mandatory' as such at the point at which the patron wants to gain access to the resource.. they're either given permission or not.  This is at a branch level and is mandatory in so much as it's required prior to being able to access the resource. We don't tie any logic to that mandatory for that action so in reality, it's only at the data entry via the existing UI that will take any notice of the mandatory flag.. I think the API can and probably should safely ignore the mandatory cases when they're not set as globally mandatory.
Comment 7 Martin Renvoize 2021-03-08 17:25:30 UTC
Created attachment 117938 [details] [review]
Bug 27857: Add handling for globally mandatory attributes

This patch adds handling for globally mandatory extended attributes and
the corresponding unit tests.
Comment 8 David Nind 2021-03-08 18:51:25 UTC
Created attachment 117945 [details] [review]
Bug 27857: Move extended_attributes tests to Koha/Patron.t

They really belong here. both Patron.t and Patrons.t got lengthy and it
is not intuitive to look for tests for a Koha::Patron method in the
Patrons.t file.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t \
           t/db_dependent/Koha/Patrons.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass, no new stuff
4. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2021-03-08 18:51:29 UTC
Created attachment 117946 [details] [review]
Bug 27857: Regression tests

This patch adds regression tests for the changed behavior.
Koha::Patron->extended_attributes will now throw exceptions and rollback
on invalid attributes.

The existing tests are amended so they don't try to deal with skipping
bad attributes.

Specific subtests are added for error situations.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t
=> FAIL: Tests fail because the new behavior is not implemented.

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2021-03-08 18:51:33 UTC
Created attachment 117947 [details] [review]
Bug 27857: Make Koha::Patron->extended_attributes validate data

This patch makes the extended_attributes method use
Koha::Patron->add_extended_attribute inside the loop, instead of direct
DB access. This way, it takes advantage of the validation that is done
there.

All happens inside a transaction, so if 'add_extended_attribute' throws
an exception, everything is rolled back.

To test:
1. Apply the previous patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t
=> FAIL: The behavior is not the correct one, tests fail
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 David Nind 2021-03-08 18:51:37 UTC
Created attachment 117948 [details] [review]
Bug 27857: Add handling for globally mandatory attributes

This patch adds handling for globally mandatory extended attributes and
the corresponding unit tests.

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 Martin Renvoize 2021-03-09 08:04:11 UTC
Created attachment 117953 [details] [review]
Bug 27857: Add handling for globally mandatory attributes

This patch adds handling for globally mandatory extended attributes and
the corresponding unit tests.

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 Martin Renvoize 2021-03-09 08:05:12 UTC
Created attachment 117954 [details] [review]
Bug 27857: Move extended_attributes tests to Koha/Patron.t

They really belong here. both Patron.t and Patrons.t got lengthy and it
is not intuitive to look for tests for a Koha::Patron method in the
Patrons.t file.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t \
           t/db_dependent/Koha/Patrons.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass, no new stuff
4. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2021-03-09 08:05:16 UTC
Created attachment 117955 [details] [review]
Bug 27857: Regression tests

This patch adds regression tests for the changed behavior.
Koha::Patron->extended_attributes will now throw exceptions and rollback
on invalid attributes.

The existing tests are amended so they don't try to deal with skipping
bad attributes.

Specific subtests are added for error situations.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t
=> FAIL: Tests fail because the new behavior is not implemented.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2021-03-09 08:05:19 UTC
Created attachment 117956 [details] [review]
Bug 27857: Make Koha::Patron->extended_attributes validate data

This patch makes the extended_attributes method use
Koha::Patron->add_extended_attribute inside the loop, instead of direct
DB access. This way, it takes advantage of the validation that is done
there.

All happens inside a transaction, so if 'add_extended_attribute' throws
an exception, everything is rolled back.

To test:
1. Apply the previous patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t
=> FAIL: The behavior is not the correct one, tests fail
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2021-03-09 08:05:22 UTC
Created attachment 117957 [details] [review]
Bug 27857: Add handling for globally mandatory attributes

This patch adds handling for globally mandatory extended attributes and
the corresponding unit tests.

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 Martin Renvoize 2021-03-09 08:09:57 UTC
I'm happy to PQA on Tomas's patches but would like him to verify my final followup.
Comment 18 Tomás Cohen Arazi 2021-03-09 11:43:46 UTC
(In reply to Martin Renvoize from comment #17)
> I'm happy to PQA on Tomas's patches but would like him to verify my final
> followup.

I like the follow-up. Go ahead!
Comment 19 Martin Renvoize 2021-03-09 11:52:25 UTC
Excellent :).. PQA
Comment 20 Jonathan Druart 2021-03-11 09:54:38 UTC
Aren't we going to adjust the controllers?
Comment 21 Martin Renvoize 2021-03-11 14:05:52 UTC
(In reply to Jonathan Druart from comment #20)
> Aren't we going to adjust the controllers?

I had a brief check of these at the time.. I believe currently the case is that the controller already check these things prior to passing use the data and so don't need changes to try/catch the exceptions... as such I felt it made sense to not touch them at this time and rather do that as part of migrating UI to using the API's.
Comment 22 Jonathan Druart 2021-03-11 14:12:00 UTC
It does not convince me.

members/memberentry.pl
592             $patron->extended_attributes($extended_patron_attributes);

may raise an uncaught exceptions.
Comment 23 Tomás Cohen Arazi 2021-03-11 14:23:41 UTC
(In reply to Jonathan Druart from comment #22)
> It does not convince me.
> 
> members/memberentry.pl
> 592             $patron->extended_attributes($extended_patron_attributes);
> 
> may raise an uncaught exceptions.

I'll take a look now.
Comment 24 Tomás Cohen Arazi 2021-03-11 14:26:26 UTC
(In reply to Tomás Cohen Arazi from comment #18)
> (In reply to Martin Renvoize from comment #17)
> > I'm happy to PQA on Tomas's patches but would like him to verify my final
> > followup.
> 
> I like the follow-up. Go ahead!

Should we rename the id => patron_extended_attribute_id ?
Comment 25 Tomás Cohen Arazi 2021-03-11 15:40:12 UTC
(In reply to Jonathan Druart from comment #22)
> It does not convince me.
> 
> members/memberentry.pl
> 592             $patron->extended_attributes($extended_patron_attributes);
> 
> may raise an uncaught exceptions.

Do you think we should wrap everything inside a transaction?
Comment 26 Jonathan Druart 2021-03-11 16:02:13 UTC
I don't know exactly what needs to be done, but there are check above, see the block with "ERROR_extended_unique_id_failed_code".

Also the repeatable is not checked in the controller, only from the template. Now that we have it we should deal with it.
Basically it's:
- insert
- catch "must unique" => propagate and display nice to the template
- catch other failures => "something went wrong" is enough (they hack the form)
And remove the existing block.

We should continue on a dedicated bug report :)
Comment 27 Tomás Cohen Arazi 2021-03-30 14:03:35 UTC
Created attachment 119009 [details] [review]
Bug 27857: (use-it-in-real-life follow-up) Use a more fine-grained exception for mandatory attributes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi 2021-03-31 11:06:43 UTC
Created attachment 119038 [details] [review]
Bug 27857: Get the right column

This patch makes the search for mandatory attribute types actually use
'code' instead of 'class' for comparisson.

Tests are added to cover this behavior.

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

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

https://bugs.koha-community.org/show_bug.cgi?id=27587
Comment 29 Tomás Cohen Arazi 2021-03-31 11:17:26 UTC
Created attachment 119040 [details] [review]
Bug 27857: (follow-up) Use a more fine-grained exception for mandatory attributes

On using this dev for the API, I found that the original FKConstraint
exception was too generic, and could be confused with those risen by
Koha::Patron->store. So this patch changes the exception that is thrown
in Koha::Patron->extended_attributes

The change is reflected on the test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t
=> SUCCESS: Tests pass, and they include the new exception
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Jonathan Druart 2021-04-09 14:41:03 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 31 Jonathan Druart 2021-04-12 09:11:25 UTC
Created attachment 119443 [details] [review]
Bug 27857: Fix Pseudonymization.t

t/db_dependent/Koha/Pseudonymization.t .. 2/3
    #   Failed test 'Attribute 1 should be retrieved correctly'
    #   at t/db_dependent/Koha/Pseudonymization.t line 198.
    #     Structures begin differing at:
    #          $got->{borrowernumber} = Does not exist
    #     $expected->{borrowernumber} = '58'

    #   Failed test 'Attribute 2 should be retrieved correctly'
    #   at t/db_dependent/Koha/Pseudonymization.t line 204.
    #     Structures begin differing at:
    #          $got->{borrowernumber} = Does not exist
    #     $expected->{borrowernumber} = '58'
    # Looks like you failed 2 tests of 3.
t/db_dependent/Koha/Pseudonymization.t .. 3/3

Highlighted by
  commit 7b40ee22035210e3fbdfe75785d68ca314285409
  Bug 27857: Make Koha::Patron->extended_attributes validate data

Koha::Patron->add_extended_attributes should not modify the $attribute
hashref parameter we passed.
Comment 32 Jonathan Druart 2021-04-12 09:12:03 UTC
Last patch pushed to master.
Comment 33 Jonathan Druart 2021-04-21 07:48:13 UTC
Created attachment 119937 [details] [review]
Bug 27857: Start a transaction foreach subtest

There is a random failure

     # Looks like you planned 15 tests but ran 2.

 #   Failed test 'extended_attributes'
 #   at t/db_dependent/Koha/Patron.t line 664.
 Missing mandatory extended attribute (type=B0vCQxlI)# Looks like your test exited with 255 just after 7.
 [17:00:24] t/db_dependent/Koha/Patron.t
Comment 34 Jonathan Druart 2021-04-21 07:48:17 UTC
Created attachment 119938 [details] [review]
Bug 27857: Remove unecessary âttribute_type3 var

It could be created with mandatory=1 and will make tests fail
Comment 35 Jonathan Druart 2021-04-21 08:46:48 UTC
Last two patches pushed to master.
Comment 36 Fridolin Somers 2021-04-29 09:45:02 UTC
Depends on Bug 27858 and Bug 28056 both not in 20.11.x
Comment 37 Jonathan Druart 2021-05-25 14:41:14 UTC
Hum, failure on Jenkins

Master_D9 run 1663

11:41:11 koha_1       | [0m[92mKoha::Exceptions::Patron::MissingMandatoryExtendedAttribute[0m  {
11:41:11 koha_1       |     Parents       [92mKoha::Exceptions::Patron[0m
11:41:11 koha_1       |     public methods (3) : [92mdescription[0m, [92mFields[0m, [92mtype[0m
11:41:11 koha_1       |     private methods (0)
11:41:11 koha_1       |     internals: {
11:41:11 koha_1       |         [35megid[0m      "[93m1000 1000[0m",
11:46:27 koha_1       |         [35meuid[0m      [94m1001[0m,
11:46:27 koha_1       |         [35mgid[0m       "[93m1000 1000[0m",
11:46:27 koha_1       |         [35mmessage[0m   "[93m[0m",
11:46:27 koha_1       |         [35mpid[0m       [94m4522[0m,
11:46:27 koha_1       |         [35mtime[0m      [94m1621935671[0m,
11:46:27 koha_1       |         [35mtrace[0m     [92mDevel::StackTrace[0m,
11:46:27 koha_1       |         [35mtype[0m      "[93mVfu0SZ[0m",
11:46:27 koha_1       |         [35muid[0m       [94m1001[0m
11:46:27 koha_1       |     }
11:46:27 koha_1       | }
11:46:27 koha_1       | Missing mandatory extended attribute (type=Vfu0SZ) at /kohadevbox/koha/Koha/Patrons/Import.pm line 420, <$fh> line 2.
11:46:27 koha_1       | 
11:46:27 koha_1       |     #   Failed test at t/db_dependent/Koha/Patrons/Import.t line 902.
11:46:27 koha_1       |     #          got: '0'
11:46:27 koha_1       |     #     expected: '1'
11:46:27 koha_1       |     # Looks like you planned 17 tests but ran 1.
11:46:27 koha_1       |     # Looks like you failed 1 test of 1 run.
11:46:27 koha_1       | 
11:46:27 koha_1       | #   Failed test 'patron_attributes'
11:46:27 koha_1       | #   at t/db_dependent/Koha/Patrons/Import.t line 1114.
11:46:27 koha_1       | Can't call method "extended_attributes" on an undefined value at t/db_dependent/Koha/Patrons/Import.t line 905, <$fh> line 2.
Comment 38 Jonathan Druart 2021-05-28 12:42:09 UTC
Got it locally

t/db_dependent/Koha/Patrons.t ........................................... 28/42         # No tests run!                                                           
                                                                                                                                                                  
    #   Failed test 'No tests run for subtest "extended attributes"'                                                                                              
    #   at t/db_dependent/Koha/Patrons.t line 1770.                                                                                                               
    # Looks like you failed 1 test of 110.                                                                                                                        
t/db_dependent/Koha/Patrons.t ........................................... 33/42                                                                                   
#   Failed test 'Test Koha::Patrons::merge'                                                                                                                       
#   at t/db_dependent/Koha/Patrons.t line 1774.                                                                                                                   
Missing mandatory extended attribute (type=hGA0QWrb6)# Looks like your test exited with 2 just after 33.
Comment 39 Jonathan Druart 2021-05-28 15:05:05 UTC
Master_D9_My8 586 Docker_7
Comment 40 Jonathan Druart 2021-06-04 13:03:32 UTC
Random failure reported on bug 28516