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 Sugar/Suite CRM sync for email addresses with an uppercase char #7401

Merged
merged 4 commits into from May 19, 2019

Conversation

tsummerer
Copy link
Contributor

Please be sure you are submitting this against the staging branch.

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

Description:

Currently the code will not properly keep Mautic and Sugar/SuiteCRM in sync when a Lead's email address has any uppercase characters.

Within the SugarcrmIntegration::pushLeads function, a $checkEmailsInSugar array is created. Its keys are forced to be lowercase:

//line 1297
$checkEmailsInSugar[$object][mb_strtolower($lead['email'])] = $lead;

Later, this is compared to a list of CRM records. However, the CRM records are NOT normalized to lowercase. If the system does not find a match, then it marks the IntegrationEntity relationship as deleted

//line 1497
foreach ($sugarLeadRecords as $sugarLeadRecord) {
    if ((isset($sugarLeadRecord) && $sugarLeadRecord)) {
        $email           = $sugarLeadRecord['email1'];
        $key             = mb_strtolower($email);
        $leadOwnerEmails = [];
        foreach ($checkEmailsInSugar as $emailKey => $mauticRecord) {
            if ($email == $emailKey) {     <------

The fix was simple, I just changed $email to $key.

Steps to reproduce the bug:

  1. Configure the SugarCRM plugin. Be sure to have at least one field pointing from Mautic to CRM
  2. Create a Lead in Mautic - it should have uppercase characters in the email address
  3. Run the mautic:integration:synccontacts -i Sugarcrm (or fetchleads (they are synonymous)) command
    This should create the Lead in CRM.
  4. Now, edit your Mautic->CRM field in Mautic.
  5. Run the command again.

Expected: The field updates in CRM
Actual : The field does not update. Additionally, the integration_entity record has changed from 'lead' to 'lead-deleted'

Steps to test this PR:

  1. Repeat steps above after installing PR.

List deprecations along with the new alternative:

  1. n/a

List backwards compatibility breaks:

  1. n/a

@npracht
Copy link
Member

npracht commented Apr 10, 2019

Hi @tsummerer is it duplicate of #6694 ?

@npracht
Copy link
Member

npracht commented Apr 10, 2019

@kuzmany ?

@npracht npracht added the pending-feedback PR's and issues that are awaiting feedback from the author label Apr 10, 2019
Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about?
Both cases convert to lower and compare?

if ($key == mb_strtolower($emailKey))

@tsummerer
Copy link
Contributor Author

tsummerer commented Apr 10, 2019

@npracht - this is completely different from #6694 - that PR is related to case sensitive URL's/module names. This one is focused on Email Address case sensitivity

@tsummerer
Copy link
Contributor Author

@kuzmany - that would be, I guess, more clear. But it's completely redundant. The $emailKey was already moved to lowercase (see my description and line ~1297). So there's no real reason to do it again.

Given that we strtolower the $email variable four lines above, I think that the original developers used $email when they meant to use $key

@kuzmany
Copy link
Member

kuzmany commented Apr 10, 2019

@tsummere Okay, then it looks good for me.

@npracht npracht removed the pending-feedback PR's and issues that are awaiting feedback from the author label Apr 11, 2019
@npracht npracht requested a review from kuzmany April 11, 2019 06:51
@npracht npracht added bug Issues or PR's relating to bugs pending-test-confirmation PR's that require one test before they can be merged plugin Anything related to plugins labels Apr 11, 2019
@npracht npracht modified the milestones: 2.16.0, 2.15.2 Apr 11, 2019
@npracht npracht added this to Ready to Test (confirmation) in Mautic 2 Apr 11, 2019
* @return array The first element is made up of records that exist in Mautic, but which no longer have a match in CRM.
* We therefore assume that they've been deleted in CRM and will mark them as deleted in the pushLeads function (~line 1320).
* The second element contains Ids of records that were explicitly marked as deleted in CRM. ATM, nothing is done with this data.
*
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*

Please remove this line to make Travis CI happy with code style.

@escopecz escopecz added the pending-feedback PR's and issues that are awaiting feedback from the author label Apr 24, 2019
@escopecz escopecz moved this from Ready to Test (confirmation) to Changes Requested / Review in Mautic 2 Apr 24, 2019
Co-Authored-By: tsummerer <tsummerer@gmail.com>
@escopecz escopecz removed the pending-feedback PR's and issues that are awaiting feedback from the author label May 1, 2019
@escopecz escopecz moved this from Changes Requested / Review to Ready to Test (confirmation) in Mautic 2 May 1, 2019
@escopecz escopecz changed the title Fix #7138 and #6140 Fix Sugar/Suite CRM sync for email addresses with an uppercase char May 7, 2019
Mautic 2 automation moved this from Ready to Test (confirmation) to Ready to Test (first time) May 19, 2019
@kuzmany kuzmany added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged labels May 19, 2019
@kuzmany kuzmany merged commit 1b1996a into mautic:staging May 19, 2019
Mautic 2 automation moved this from Ready to Test (first time) to Merged May 19, 2019
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 plugin Anything related to plugins ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged
Projects
No open projects
Mautic 2
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

4 participants