Bug 28217 - Several non-repeatable attributes when merging patrons
Summary: Several non-repeatable attributes when merging patrons
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 21648
  Show dependency treegraph
 
Reported: 2021-04-26 08:02 UTC by Jonathan Druart
Modified: 2021-09-16 15: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,20.11.06,20.05.12


Attachments
Bug 28217: Prevent several non-repeatable attributes to be merged (6.92 KB, patch)
2021-04-26 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28217: Prevent several non-repeatable attributes to be merged (6.97 KB, patch)
2021-04-28 00:08 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28217: Prevent several non-repeatable attributes to be merged (7.03 KB, patch)
2021-04-30 08:18 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-04-26 08:02:39 UTC
When using the patron merge feature it's possible to generate a patron with several non-repeatable attributes.
Comment 1 Jonathan Druart 2021-04-26 08:04:33 UTC
Created attachment 120165 [details] [review]
Bug 28217: Prevent several non-repeatable attributes to be merged

When using the patron merge feature it's possible to generate a patron
with several non-repeatable attributes.

This patch prevents that.

Test plan:
Create 2 patron attribute types, one repeatable and one non-repeatable
Create 2 patrons and add them repeatable attributes
Add a non-repeatable attribute to one of them
Merge the 2 patrons
=> It succeeds, the resulting patron has all the repeatable attribute
and the non-repeatable one.

Do it again but this time add non-repeatable to both patrons
Merge them
=> It fails, you should see an error on the UI
  Merge failed! The following error was reported: Tried to add more than one non-repeatable attributes. type=TYPE value=VALUE.
Comment 2 Victor Grousset/tuxayo 2021-04-28 00:08:50 UTC
Created attachment 120248 [details] [review]
Bug 28217: Prevent several non-repeatable attributes to be merged

When using the patron merge feature it's possible to generate a patron
with several non-repeatable attributes.

This patch prevents that.

Test plan:
Create 2 patron attribute types, one repeatable and one non-repeatable
Create 2 patrons and add them repeatable attributes
Add a non-repeatable attribute to one of them
Merge the 2 patrons
=> It succeeds, the resulting patron has all the repeatable attribute
and the non-repeatable one.

Do it again but this time add non-repeatable to both patrons
Merge them
=> It fails, you should see an error on the UI
  Merge failed! The following error was reported: Tried to add more than one non-repeatable attributes. type=TYPE value=VALUE.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 3 Victor Grousset/tuxayo 2021-04-28 00:15:45 UTC
Works great, message quite clear :)
Comment 4 Julian Maurice 2021-04-30 08:18:13 UTC
Created attachment 120325 [details] [review]
Bug 28217: Prevent several non-repeatable attributes to be merged

When using the patron merge feature it's possible to generate a patron
with several non-repeatable attributes.

This patch prevents that.

Test plan:
Create 2 patron attribute types, one repeatable and one non-repeatable
Create 2 patrons and add them repeatable attributes
Add a non-repeatable attribute to one of them
Merge the 2 patrons
=> It succeeds, the resulting patron has all the repeatable attribute
and the non-repeatable one.

Do it again but this time add non-repeatable to both patrons
Merge them
=> It fails, you should see an error on the UI
  Merge failed! The following error was reported: Tried to add more than one non-repeatable attributes. type=TYPE value=VALUE.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 5 Jonathan Druart 2021-04-30 15:18:06 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 6 Fridolin Somers 2021-05-11 10:12:36 UTC
Pushed to 20.11.x for 20.11.06
Comment 7 Andrew Fuerste-Henry 2021-05-25 11:53:13 UTC
Pushed to 20.05.x for 20.05.12
Comment 8 Victor Grousset/tuxayo 2021-05-25 14:24:49 UTC
Can't backport to 19.11.x: can't solve a conflict on the tests.
Even after adding some missing variables
my $anonymous = $builder->build( { source => 'Borrower', }, );
my $anonymous_patron = Koha::Patrons->find( $anonymous->{borrowernumber} );

I get
#   Failed test 'Test Koha::Patrons::merge'
#   at t/db_dependent/Koha/Patrons.t line 1801.
The method Koha::Patron->extended_attributes is not covered by tests!


If there is an interest in having this backported, please submit a patch for 19.11.
Comment 9 Christopher Brannon 2021-05-25 14:37:23 UTC
The problem I have with this patch is that it makes the feature completely useless if you have non-repeatable attributes.  It doesn't offer a way to select attributes to keep.  There are no choices in how this functions.  You just get this feature taken away.
Comment 10 Jonathan Druart 2021-05-25 14:46:02 UTC
(In reply to Christopher Brannon from comment #9)
> The problem I have with this patch is that it makes the feature completely
> useless if you have non-repeatable attributes.  It doesn't offer a way to
> select attributes to keep.  There are no choices in how this functions.  You
> just get this feature taken away.

On bug 21648 you said you had to hide the feature because it caused data corruption.

With this patch the data integrity is preserved. In my opinion it goes to the right direction.
Comment 11 Christopher Brannon 2021-05-25 19:48:32 UTC
Yes.  It DOES prevent corruption.  That is a big issue.  Just wish this didn't stop there, and made it usable for EVERYONE.
Comment 12 Victor Grousset/tuxayo 2021-05-25 19:53:40 UTC
Hi,

It seems the remaining issue warrants a follow-up ticket with a test plan that match this use case.
Comment 14 Victor Grousset/tuxayo 2021-05-28 07:42:02 UTC
(In reply to Victor Grousset/tuxayo from comment #8)
> Can't backport to 19.11.x: can't solve a conflict on the tests.

Just retried with the idea of not backporting the newt tests and rely on manual testing. Turns out that the fix depends on bug 20443 which in not in 19.11.x

So 19.11.x still uses attribute() and C4::Members::Attributes::UpdateBorrowerAttribute()

But this patch relies on extended_attributes() and add_extended_attribute()

And changing the name don't do it :P
The existing tests don't pass anymore.

So here is some info in case someone need this for 19.11.x and bellow.
Comment 15 Mark Rogers 2021-09-16 13:25:31 UTC
Comment on attachment 120325 [details] [review]
Bug 28217: Prevent several non-repeatable attributes to be merged

Thank you for catching this issue of non-repeatable attributes being repeated upon merge. I didn't realize it had been an issue until trying to merge records recently and running into the exception thrown by this patch.

In my case, I was trying to merge two records for the same student; the non-repeatable attribute was the student's grade level.

Would it be possible to change the default behavior to one of keeping the non-repeatable attribute (if it exists) of the patron record that was selected as the patron to keep? This would make the behavior the same as that of the standard patron fields, which also are non-repeatable.

I apologize if this is not the right place to post this.

Thank you!
Comment 16 Christopher Brannon 2021-09-16 15:03:43 UTC
(In reply to Mark Rogers from comment #15)
> Comment on attachment 120325 [details] [review] [review]
> Bug 28217: Prevent several non-repeatable attributes to be merged
> 
> Thank you for catching this issue of non-repeatable attributes being
> repeated upon merge. I didn't realize it had been an issue until trying to
> merge records recently and running into the exception thrown by this patch.
> 
> In my case, I was trying to merge two records for the same student; the
> non-repeatable attribute was the student's grade level.
> 
> Would it be possible to change the default behavior to one of keeping the
> non-repeatable attribute (if it exists) of the patron record that was
> selected as the patron to keep? This would make the behavior the same as
> that of the standard patron fields, which also are non-repeatable.
> 
> I apologize if this is not the right place to post this.
> 
> Thank you!

Mark,
Since this bug is resolved, you will need to add your comments and votes on https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21648, where we are addressing this exact issue now.