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 for #7558 - PATCH loses data on contact merge #7560

Closed
wants to merge 1 commit into from

Conversation

pjeby
Copy link
Contributor

@pjeby pjeby commented May 24, 2019

Q A
Bug fix? Yes
New feature? No
Automated tests included? No
Related user documentation PR URL
Related developer documentation PR URL
Issues addressed (#s or URLs) #7558
BC breaks?
Deprecations?

Description:

Steps to reproduce the bug:

  1. Create a new anonymous contact via curl get to /mtc
  2. With curl, PATCH the contact to include first name, last name, or other fields, as well as email
  3. Create a second anonymous contact via curl get /mtc
  4. With curl, PATCH the second contact with the same email used in step 2

Result: the contacts are merged, but the field values created in step 2 will be missing.

Steps to test this PR:

  1. Load up this PR
  2. Same steps as above, but the field values should remain intact.

@npracht npracht added API Anything related to the API bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test labels Jun 25, 2019
@npracht npracht added this to the 2.16.0 milestone Jun 25, 2019
@npracht npracht added this to Ready to Test (first time) in Mautic 2 Aug 15, 2019
@npracht npracht modified the milestone: 2.16.0 Jan 23, 2020
@RCheesley RCheesley added this to the 2.16.1 milestone Mar 9, 2020
@npracht npracht added this to Ready to test in Mautic 2 Mar 10, 2020
@dennisameling dennisameling added the T2 Medium difficulty to fix (issue) or test (PR) label Mar 18, 2020
@dennisameling
Copy link
Member

dennisameling commented Mar 19, 2020

More detailed steps to test with Mautibox PR 8579 (a recent PR based on the latest commits for 2.16.1):

  1. Create a new anonymous contact via curl get to /mtc, so curl https://mautibox.com/8579/mtc
  2. Ensure the API is active and Basic Auth is enabled in Configuration > API settings > Enable Basic Auth
  3. With curl, PATCH the contact to include first name, last name, or other fields, as well as email, that is:
REPLACE {CONTACT_ID} with the contact ID that was just created (Contacts > Toggle anonymous)!
curl -X PATCH https://mautibox.com/8579/api/contacts/{CONTACT_ID}/edit -u admin:mautic --header "Content-Type: application/json" --data '{"firstname": "Dennis", "lastname": "Demo", "email": "dennis@test.org"}'
  1. Create a second anonymous contact via curl get /mtc, so curl https://mautibox.com/8579/mtc
  2. With curl, PATCH the second contact with the same email used in step 3

Step 1 created https://mautibox.com/8579/s/contacts/view/58, patched in step 3 and firstname + lastname + email are updated. Step 4 created https://mautibox.com/8579/s/contacts/view/59, patched in step 4 and firstname + lastname + email are updated. The contacts are merged into ID 55 (which I created earlier) and data is complete.

@pjeby are you sure this is still an issue in 2.16.0? Also, please provide more detailed testing steps next time so that testers who aren't familiar with the API know exactly what to do as well (e.g. provide example commands for curl).

@dennisameling
Copy link
Member

To do: after the 2.16.1-beta release, check if #7717 introduced a bug like @pjeby mentioned in #7717 (comment)

@pjeby
Copy link
Contributor Author

pjeby commented Mar 19, 2020

I haven't tested any version of mautic that doesn't have this patch, since it was necessary when I created it, and I hadn't seen anything re: the original bug report. Regarding #7717, I have now confirmed that it does not introduce the bug I thought it would... which is consistent with the hypothesis that the bug this PR fixes has already been fixed via some other change since last May. I will investigate further.

@dennisameling dennisameling removed this from the 2.16.1 milestone Mar 19, 2020
@npracht npracht added Triage M2/M3 and removed ready-to-test PR's that are ready to test labels Apr 4, 2020
@npracht
Copy link
Member

npracht commented Apr 4, 2020

Hi there!
It has been decided to not create any extra Mautic 2 versions (except for major security purpose) knowing that Mautic 3 is coming very soon.

We now want to integrate your contribution in the Mautic 3 roadmap as 3.0.1 candidate.

How to do?

  1. Check if the bug still present in Mautic 3
  2. If Yes: rebase your PR against the 3.x branch
  3. If No: please comment on your PR: “This PR only applies to Mautic 2.x”

Please report results by commenting on your PR to make us administration easier.

In case your bugfix only apply to Mautic 2, we'll consider adding it in an extra Mautic 2 version.


You can more information on how to do all of that on this blog post "Getting you PR ready for Mautic 3".

@npracht
Copy link
Member

npracht commented May 20, 2020

Hello @pjeby could you please test mautic 3.0.0-beta2 and tell me if the issue is still existing?

  • If no, i'll tag this issue at M2 only relevant
  • If yes, could you please rebase your branch against 3.x branch ?

Waiting for your feedback, i would very love to merge it in 3.0.1 if still relevant. Thanks !

@npracht npracht changed the base branch from staging to 3.x June 10, 2020 07:23
@npracht npracht added ready-to-test PR's that are ready to test and removed Triage M2/M3 labels Jun 10, 2020
@npracht npracht removed this from Ready to test in Mautic 2 Jun 10, 2020
@npracht npracht added this to the 3.0.1 milestone Jun 10, 2020
@RCheesley RCheesley changed the base branch from 3.x to staging June 17, 2020 11:36
@RCheesley
Copy link
Sponsor Member

I am not fully clear if this issue remains in 3.0 @pjeby ? Can you take a look (mautibox.com is now running Mautic 3.0) and let us know? Thanks!

@TravisBuddy
Copy link

Travis tests have failed

Hey @pjeby,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 3ac7d420-b643-11ea-aaf6-456b40a357ff

@RCheesley
Copy link
Sponsor Member

Moving to a later release due to unresolved Travis issues

@RCheesley RCheesley modified the milestones: 3.0.1, 3.0.2 Jun 26, 2020
@RCheesley RCheesley closed this Jul 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Anything related to the API bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test T2 Medium difficulty to fix (issue) or test (PR)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants