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.
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
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.
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
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?)
# 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.
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.
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.
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>
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>
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>
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>
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>
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>
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>
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>
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>
I'm happy to PQA on Tomas's patches but would like him to verify my final followup.
(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!
Excellent :).. PQA
Aren't we going to adjust the controllers?
(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.
It does not convince me. members/memberentry.pl 592 $patron->extended_attributes($extended_patron_attributes); may raise an uncaught exceptions.
(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.
(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 ?
(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?
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 :)
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>
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
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>
Pushed to master for 21.05, thanks to everybody involved!
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.
Last patch pushed to master.
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
Created attachment 119938 [details] [review] Bug 27857: Remove unecessary âttribute_type3 var It could be created with mandatory=1 and will make tests fail
Last two patches pushed to master.
Depends on Bug 27858 and Bug 28056 both not in 20.11.x
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.
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.
Master_D9_My8 586 Docker_7
Random failure reported on bug 28516