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

Fixed bug in SF that did not use the nextUrl when syncing more than 2K and use SystemModStamp instead of LastModifiedDate #5841

Merged
merged 2 commits into from Mar 21, 2018

Conversation

alanhartless
Copy link
Contributor

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

Description:

Syncing more than 2K records from SF to Mautic failed because the next URL was not used. This fixes that. This also replaces querying by LastModifiedDate, which can be manipulated causing differing results in the query, to SystemModStamp which is read only.

Steps to reproduce the bug:

  1. Hack in https://github.com/mautic/mautic/compare/staging...alanhartless:bug-sf-person-batching?expand=1#diff-6ce39f45c72f8ef1c78d747ec6426817R328 so that it'll batch in 200 (dev SF only allows 2000 records and the default batch size is 2000 so you won't be able to reproduce this in a dev SF account without this hack)
  2. Import/update more than 200 records in SF
  3. Run the php app/console m:i:s -i Salesforce command
  4. Notice that it will run forever until it's past 100% (it's processing the same batch over and over)

Steps to test this PR:

  1. Apply the PR and run the command again
  2. All records should sync and the script completes

…K records and changed use of LastModifiedDate which can be manipulated to SystemModStamp which cannot
@alanhartless alanhartless added bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test labels Mar 21, 2018
@alanhartless alanhartless added this to the 2.13.0 milestone Mar 21, 2018
@mautibot mautibot added the code-review-needed PR's that require a code review before merging label Mar 21, 2018
@javjim
Copy link

javjim commented Mar 21, 2018

So was able to reproduce bug and fix looks ok. But found something interesting. I had to create and delete some data on SF and at the end I had 2000 contacts and had 14k+ deleted contacts on SF.

After running the Sync command, it looks like I have 14k contacts in mautic instead of just the 2k, so it looks like its syncing data that is on SF recycle bin. Does that make any sense? is this intended behaviour???

@javjim javjim added pending-test-confirmation PR's that require one test before they can be merged pending-feedback PR's and issues that are awaiting feedback from the author and removed ready-to-test PR's that are ready to test labels Mar 21, 2018
@escopecz escopecz self-assigned this Mar 21, 2018
Copy link
Sponsor Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

This PR fixes the issue 👍

@mautibot mautibot removed the code-review-needed PR's that require a code review before merging label Mar 21, 2018
@escopecz escopecz removed pending-test-confirmation PR's that require one test before they can be merged review labels Mar 21, 2018
@escopecz escopecz merged commit 6ac4820 into mautic:staging Mar 21, 2018
@alanhartless alanhartless deleted the bug-sf-person-batching branch May 24, 2018 22:42
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 pending-feedback PR's and issues that are awaiting feedback from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants