Bug 21648

Summary: Patron Merge duplicates patron attributes
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: PatronsAssignee: Bugs List <koha-bugs>
Status: RESOLVED MOVED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: bob, bugzilla, cbrannon, clopez, gmcharlt, irma, jonathan.druart, josef.moravec, katrin.fischer, kyle.m.hall, lisettepalouse+koha, marcos.rogers, mfuerst, mtj, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9302
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29059
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 9302, 28217    
Bug Blocks:    
Attachments: Bug 21648: Prevent several non-repeatable attributes to be merged

Description Andrew Fuerste-Henry 2018-10-24 15:34:36 UTC
The patron merge tool introduced in 18.05 is a great step, but creates some issues. Contact information for the discarded patron is lost even when the kept patron has null values for those fields. Merging patrons with additional patron attributes results in a patron with multiple values for an attribute that is not supposed to be repeatable. The screen that confirms the merge has completed reports counts of transferred data but doesn't allow one to see details of what was transferred from the discarded patron to the kept patron.

A good next step here would be an interface similar to what we see when merging bibs -- the opportunity to see values for both patron records and select which will be carried forward to the final record, at least for all fields in the borrowers table and for additional patron attributes.
Comment 1 Carlos Lopez 2019-04-01 05:06:33 UTC
Verified: When we've merged borrower records we've found that the resulting record contains multiple patron attributes (even when these are set as not repeatable).
Comment 2 Christopher Brannon 2020-07-01 17:32:59 UTC
This wreaks havoc with our records because of duplicated patron attributes and no easy way to clean up.  This should be considered a bug, not an enhancement, IMO.
Comment 3 Andrew Fuerste-Henry 2020-07-01 18:12:55 UTC
Renaming and refocusing this bug. I agree the attributes are a more pressing aspect of this, so let's make this specific and therefore more easily addressed.
Comment 4 Christopher Brannon 2020-07-01 18:22:49 UTC
(In reply to Andrew Fuerste-Henry from comment #3)
> Renaming and refocusing this bug. I agree the attributes are a more pressing
> aspect of this, so let's make this specific and therefore more easily
> addressed.

+1
Comment 5 Martha Fuerst 2020-08-12 16:11:26 UTC
It makes sense that these attributes would also merge. Thanks for filing this bug!
Comment 6 Christopher Brannon 2020-11-10 16:46:48 UTC
Hey folks, I changed the importance on this.  We would really like to use this feature, but we have to hide it because it causes problems with the database.  This really needs to be addressed!
Comment 7 Jonathan Druart 2020-11-11 13:14:40 UTC
This is only a problem when the patron's attributes cannot be repeatable, right?
Comment 8 Christopher Brannon 2020-11-12 15:45:22 UTC
(In reply to Jonathan Druart from comment #7)
> This is only a problem when the patron's attributes cannot be repeatable,
> right?

Correct.  I don't know if there can be some kind of Duplicates allowed checkbox in each attribute that can be created that would facilitate this, or if the merge option should review items fields being duplicated.
Comment 9 Jonathan Druart 2021-04-23 13:41:27 UTC
Created attachment 120103 [details] [review]
Bug 21648: 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 10 Jonathan Druart 2021-04-23 13:42:10 UTC
Lowering severity, there is no data loss. Please test!
Comment 11 Christopher Brannon 2021-04-23 14:48:10 UTC
I really don't like the direction of this plan.  It looks like you have it fail the merge rather than review and select one of the non-repeatable attributes, as suggested.  As is, this leaves A LOT of work for staff to try and resolve issues.  At our library, some of these non-repeatable attributes are required.  So, it is not as simple as going to one of the two records and deleting the non-repeatable attribute.

This should be resolvable at the time of the merge.  Otherwise, the merge is pointless and useless for some libraries.  For us, it would fail every single attempt because of required attributes.
Comment 12 Jonathan Druart 2021-04-23 15:11:10 UTC
Christopher, yes I take the easy path, to prevent corrupted data.

What you are suggesting is, to me, considered as a new enhancement, not a bug fix. The changes required to implement this behaviour are much more important and can be done on a separate bug report. It will certainly need funding if you want to see it implemented.
Comment 13 Christopher Brannon 2021-04-23 18:33:17 UTC
I'm not picky about whether it is a bug or an enhancement.  If you want to charge to do this, then so be it.  It isn't anything we've slated to sponsor at this time.  If you want to give an estimate for the work, feel free to put it on the bug, so that we know how much it will cost to sponsor.  I just ask that the nature of this request be honored and not changed.
Comment 14 Jonathan Druart 2021-04-26 08:01:05 UTC
(In reply to Christopher Brannon from comment #13)
> I'm not picky about whether it is a bug or an enhancement.  If you want to
> charge to do this, then so be it.  It isn't anything we've slated to sponsor
> at this time.  If you want to give an estimate for the work, feel free to
> put it on the bug, so that we know how much it will cost to sponsor.  I just
> ask that the nature of this request be honored and not changed.

In my understanding the scope of the bug report was not correctly defined when first created and has been refined later. I also thought that the main point was about the issue leading to corrupted data (patron with several non-repeatable attributes). As the severity was "critical" I provided a patch to fix this specific issue, as small as possible to ease backport into stable branches.
We improved the code in this area recently and it was easy to develop, I wrote the patch in about one hour.

The way you want to fix the issue requires a much bigger change, and I agree with you it will be a nice one to have. However we are one month left before the release and it's definitely not my job to provide a new enhancement for this specific need.

That's why I talked about funding, but I should not have and I regret it. My point was not to tell you "pay to get it", I don't like either when it's the only answer provided, but this bug report has been opened for a long time already and it is something you should consider if you really need it.

I will move the patch to a separate bug report and change the status of this bug report to "new enhancement".
Comment 15 Jonathan Druart 2021-04-26 08:03:35 UTC
(In reply to Jonathan Druart from comment #14)
> I will move the patch to a separate bug report and change the status of this
> bug report to "new enhancement".

See bug 28217.
Comment 16 Tomás Cohen Arazi 2021-04-26 12:59:49 UTC
(In reply to Jonathan Druart from comment #12)
> Christopher, yes I take the easy path, to prevent corrupted data.
> 
> What you are suggesting is, to me, considered as a new enhancement, not a
> bug fix. The changes required to implement this behaviour are much more
> important and can be done on a separate bug report. It will certainly need
> funding if you want to see it implemented.

I agree with Jonathan's plan to prevent corrupted data, and then see how we continue with this.

I also agree the feature, to be useful, should provide better feedback and options to solve conflicts.
Comment 17 Mark Rogers 2021-09-16 15:38:34 UTC
I agree that prevention of data loss/corruption is important. It sounds like the implementation of Andrew's suggestion to have a full interface to merge both patrons (though great) is currently a barrier to getting a more functional solution for those of us who have required non-repeating patron attributes.

Would it be possible to simply treat the non-repeating patron attributes the same way as the standard patron fields? That is, the default behavior would be to retain those non-repeating attributes belonging to the patron record that was chosen when initiating the merge rather than the values for those attributes provided by the non-selected patron. The screen where the merge is confirmed already has links to both patron records, allowing the differences of patron records and additional patron attributes to be compared, albeit somewhat manually.
Comment 18 Jonathan Druart 2021-09-20 14:58:25 UTC
Mark, I've opened bug 29059 to implement your suggestion.
Comment 19 Andrew Fuerste-Henry 2022-03-16 12:18:49 UTC
I'm closing this bug in the interest of clarity.
Bug 28217 (pushed to Koha) made it so that Koha will block a patron merge rather than end up with two values for a non-repeatable attribute.
Bug 29059 (signed off) would adjust this behavior so that Koha keeps the attribute value from the retained patron, rather than blocking the merge.
Bug 30303 (just filed) is for the larger enhancement request of a pull patron merge interface to let one select which values to retain from each patron.