Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Contact merges now correctly merge custom items #329

Open
wants to merge 1 commit into
base: staging
Choose a base branch
from

Conversation

driskell
Copy link

Not sure what branch to use as none of these branches mentioned exist, so using default.

Q A
Bug fix? (use the a.b branch) [x]
New feature/enhancement? (use the a.x branch) [ ]
Deprecations? [ ]
BC breaks? (use the c.x branch) [ ]
Automated tests included? [ ]
Issue(s) addressed Fixes #323

I haven't looked at tests as there is no coverage here currently. A unit test would be useless as there is no logic here it's just building a query so it would just end up testing method calls and failing every time someone modifies the code, rather than testing the outcome. I looked at adding a functional test but I need a bit of time to get a local database setup etc. to work from so I had to move on for now. If it's required let me know but I'm unsure my availability to do it.

Description:

When two contacts with custom item relationships merge, the custom item relationships are not merged, and so the losing contact's custom items are "lost" and get removed when that losing contact is removed.

Steps to test this PR:

Create two contacts. Set the email to the same. Ensure email field is "Unique". Create several different custom items and attach different custom items to both. Now run the deduplicate contacts command. Before this patch, the winning contact only has its original custom items, none of the ones from the losing contact. With this PR, the items for both contacts end up on the winning contact.

@acquia-stalebot-platauto
Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@escopecz escopecz added bug Something isn't working and removed stale labels Feb 13, 2025
@acquia-stalebot-platauto
Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please remove the stale label to avoid it being closed. Thank you for your contributions. More info: https://github.com/acquia/devops-github-administration/blob/main/docs/operations_related_to_repositories.md#acquia-stale-bot

@escopecz escopecz removed the stale label Mar 17, 2025
@PierreAmmeloot
Copy link

@escopecz what do I need to do to merge this proposal?

@escopecz
Copy link
Contributor

The conflict must be resolved and then reviewed and tested. Once that is done it has a high chance to get through the final quality assurance process of Acquia.

@PierreAmmeloot
Copy link

@driskell can you resolve this conflict please?

@driskell
Copy link
Author

driskell commented Mar 17, 2025

I am on leave for the next week and likely be busy after so it’s likely to be some time.

this issue is already fixed though as part of the v5 work but this PR I would say is more efficient and scalable for larger installations so definitely should be looked at - but it does mean the conflict is less a case of just resolving the conflict - the old code needs removing too

@PierreAmmeloot
Copy link

@escopecz Do you think this code should be integrated or rejected in view of what @driskell has just told us?

@escopecz
Copy link
Contributor

I'd leave it open in case @driskell or someone else would want to pick it up and finish it. However, the stale bot is annoying and if this won't be picked up till next Stalebot round I leave it closed. It can be reopened any time afterwards. Just ping me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merged contacts lose their custom objects
3 participants