Bug 28217

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.

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>

Works great, message quite clear :)

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>

Pushed to master for 21.05, thanks to everybody involved!

Pushed to 20.11.x for 20.11.06

Pushed to 20.05.x for 20.05.12

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.

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.

(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.

Yes.  It DOES prevent corruption.  That is a big issue.  Just wish this didn't stop there, and made it usable for EVERYONE.

Hi,

It seems the remaining issue warrants a follow-up ticket with a test plan that match this use case.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21648

(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 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!

(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.