[Bug 28031] New: Koha::Patron::Attribute->_check_repeatable doesn't exclude the object's ID
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 Bug ID: 28031 Summary: Koha::Patron::Attribute->_check_repeatable doesn't exclude the object's ID Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major 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 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 :-/ -- 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=28031 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |17828 Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17828 [Bug 17828] Koha::Patron::Attribute->store should raise an exception if unique_id is being broken -- 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=28031 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org, | |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=28031 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |27858 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27858 [Bug 27858] Make Koha::Patron::Attribute->store raise an exception on invalid type/code -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 118792 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118792&action=edit Bug 28031: Regression tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 118793 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118793&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 --- Comment #3 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 118794 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118794&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch CC| |tomascohen@gmail.com Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |23666 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23666 [Bug 23666] Add API route for additional patron attributes -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #118792|0 |1 is obsolete| | --- Comment #4 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 118828 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118828&action=edit Bug 28031: Regression tests 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=28031 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #118793|0 |1 is obsolete| | --- Comment #5 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 118829 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118829&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #118794|0 |1 is obsolete| | --- Comment #6 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 118830 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118830&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 118831 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118831&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 --- Comment #8 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 118833 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118833&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 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=28031 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |kyle@bywatersolutions.com |y.org | CC| |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 Kyle M Hall <kyle@bywatersolutions.com> 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=28031 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #118828|0 |1 is obsolete| | Attachment #118829|0 |1 is obsolete| | Attachment #118830|0 |1 is obsolete| | Attachment #118831|0 |1 is obsolete| | Attachment #118833|0 |1 is obsolete| | --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 118859 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118859&action=edit Bug 28031: Regression tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 118860 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118860&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 --- Comment #11 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 118861 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118861&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 118862 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118862&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 118863 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118863&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 --- Comment #15 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #118863|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 --- Comment #16 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 119377 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119377&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 119378 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119378&action=edit Bug 28031: test remove type from parameter -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #119377|0 |1 is obsolete| | Attachment #119378|0 |1 is obsolete| | --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 119379 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119379&action=edit Bug 28031: Fix patron.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 119380 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119380&action=edit Bug 28031: Remove type from parameter of *_ok methods -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 119382 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119382&action=edit Bug 28031: Add missing txn_begin in Attribute.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 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=28031 --- Comment #22 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=28031 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28162 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28162 [Bug 28162] Self registration fails if patron extended attributes are editable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |magnus@libriotech.no --- Comment #23 from Magnus Enger <magnus@libriotech.no> --- I am having trouble with this on 20.05, I think. Any chance of a backport? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28031 --- Comment #24 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org