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 issue #5103 - Company/Accounts being duplicated on CRM Sync o… #5610

Merged

Conversation

stancel
Copy link
Contributor

@stancel stancel commented Jan 20, 2018

…f edited Company/Account

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) #5103
BC breaks? No
Deprecations? No

Description:

Like the reporter of #5103 we were seeing duplicate Mautic Company records being created when a SuiteCRM Account (Company) was edited. After debugging through this it was discovered that inside IdentifyCompanyHelper::identifyLeadsCompany class and method there was an else if statement for looking up a company name (not domain) through the MX records of the companyemail field. If anything was in that field it correctly skipped the next else if statement where the existing Mautic company would have been looked up by the 'companyname' field.

The proper ordering should be to first lookup my the name then companyname fields. I believe that the else if check put in to lookup the domain should be assigned to the $companyDomain variable instead of the $companyName variable and have made that adjustment in this pull request as well. Making these adjustments corrected the issue when testing in my local environment. Please let me know if you have any questions or feedback.

Steps to reproduce the bug:

  1. Activate SugarCRM integration (6.x) and map any applicable fields for account (companyname and companyemail at a minimum)
  2. Add a SugarCRM Account (company) record and trigger the mautic:integration:fetchleads --integration=Sugarcrm --time-interval="1 minute" command
  3. View that account/company added in Mautic
  4. Edit the same SugarCRM Account (company) just added and trigger the mautic:integration:fetchleads --integration=Sugarcrm --time-interval="1 minute" command again
  5. Observe that a duplicate Mautic Company is created versus updating the existing one.

Steps to test this PR:

  1. First test the bug using the steps above
  2. Pull the included commit into your repo
  3. Follow the same testing steps above
  4. Observe that the existing Mautic Company is properly updated versus a duplicate record being created

List deprecations along with the new alternative:

  1. N/A

List backwards compatibility breaks:

  1. None that I am aware of

…Sync of edited Company/Account - Adding correct syntax for elseif structure after Travis CI build failed for PHP 7.1
@npracht
Copy link
Member

npracht commented Jan 24, 2018

/label bug

@mautibot mautibot added the bug Issues or PR's relating to bugs label Jan 24, 2018
@npracht
Copy link
Member

npracht commented Jan 24, 2018

/label ready to test

@mautibot mautibot added the ready-to-test PR's that are ready to test label Jan 24, 2018
@npracht
Copy link
Member

npracht commented Jan 25, 2018

✅ Just tested and it saved my life, Sugar company fetch is working again!
We had to add a small fix that manages the fact that mautic company email is unique --> See @Noa83 PR #5637.

Consequently, it also corrected #5617

@npracht
Copy link
Member

npracht commented Jan 25, 2018

/unlabel ready to test

@mautibot mautibot removed the ready-to-test PR's that are ready to test label Jan 25, 2018
@npracht
Copy link
Member

npracht commented Jan 25, 2018

/label pending test confirmation

@mautibot mautibot added the pending-test-confirmation PR's that require one test before they can be merged label Jan 25, 2018
@dbhurley dbhurley added ready-to-test PR's that are ready to test and removed pending-test-confirmation PR's that require one test before they can be merged labels Jan 28, 2018
@stancel
Copy link
Contributor Author

stancel commented Feb 4, 2018

Please let me know if there is anything else that I can do on this issue/pull request to make sure it gets included in a future release. Thanks.

@KaKite
Copy link

KaKite commented Feb 9, 2018

Tested, working as expected. Companies are fetching WITH and WITHOUT email addresses, no duplicates and attaching contacts in Mautic - GREAT WORK!!!

@stancel
Copy link
Contributor Author

stancel commented Feb 12, 2018

@KaKite - thanks for testing and putting a comment here.

@dbhurley, @alanhartless - would there be any reason why this fix (and possibly #5615) could not be put into the 2.13.0 milestone? I ask because we are trying not to diverge from the standard Mautic releases and these are mission critical for us. Thanks in advance for your time and help in letting me know.

@mautibot mautibot added the code-review-needed PR's that require a code review before merging label Feb 12, 2018
@dbhurley dbhurley added this to the 2.13.0 milestone Feb 12, 2018
@dbhurley dbhurley modified the milestone: 2.13.0 Mar 4, 2018
@dbhurley
Copy link
Member

@stancel I'm going to have to push this to 2.14 (which shouldn't be too long in coming) - we're closing up 2.13.1 and this hasn't gotten any updates.

@dbhurley dbhurley modified the milestones: 2.13.1, 2.14.0 Apr 18, 2018
@stancel
Copy link
Contributor Author

stancel commented May 31, 2018

@dbhurley - please find the rebased updates pushed and let me know if anything else is needed. Thanks.

@npracht npracht added ready-to-test PR's that are ready to test and removed pending-feedback PR's and issues that are awaiting feedback from the author labels Jun 5, 2018
@alanhartless
Copy link
Contributor

@stancel I think something is still off here. It shouldn't be including commits already in master. What branch did you rebase to?

@npracht npracht added the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Jul 4, 2018
@dongilbert dongilbert modified the milestones: 2.14.0, 2.14.1 Jul 17, 2018
@heathdutton heathdutton added pending-test-confirmation PR's that require one test before they can be merged ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed has-conflicts Pull requests that cannot be merged until conflicts have been resolved ready-to-test PR's that are ready to test code-review-needed PR's that require a code review before merging pending-test-confirmation PR's that require one test before they can be merged labels Sep 4, 2018
@heathdutton heathdutton merged commit d43b1a5 into mautic:staging Sep 4, 2018
@escopecz escopecz removed ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged labels Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet