SalesforceFoundation / NPSP

The current version of the Salesforce.org Nonprofit Success Pack
http://www.salesforce.org/nonprofit/nonprofit-success-pack/
BSD 3-Clause "New" or "Revised" License
13 stars 1 forks source link

CON_ContactMerge_TDTM cannot be run from batch apex #2463

Closed stenebenau closed 5 years ago

stenebenau commented 7 years ago

Hi,

We are the developer of the app called Duplicate Check for Salesforce. This is a native force.com application to remove duplicates from a Salesforce org. Many NPSP users are using our application to deduplicate their Contact & Accounts.

Our merge has incorporated the logic from the CON_ContactMerge.cls so the contacts can be merged from our application. This works great as long as the merge is done via the UI or in a single transaction.

A lot of customers have issues with large amount of duplicate contacts and therefore we have developed a batch merge tool which uses batch apex to process all the merges.

The CON_ContactMerge_TDTM gives us problems here. Because this code always fires a future and unfortunately a future cannot be called from batch apex it throws an error and the merge is unsuccessful.

Could the CON_ContactMerge_TDTM be rewritten so that it is more aware of its context and fire a future when allowed and of not allowed directly processes de fixups?

I'm happy to code this myself and push it if needed.

Thanks, Sten

davidhabib commented 7 years ago

the amount of processing that handleContactMergeFixupsFuture() performs is quite large, including:

I'm not confident that we could perform all of this work within the same transaction as the initial merge. I'd certainly like more confidence that this could possibly work, before investing in making such a change. Can you provide any data that might help convince us this could be successful? thanks!

stenebenau commented 7 years ago

Hi David,

I have been running some tests with the change in my local develop environment. My solution works and all test classes keep on running smoothly.

However i don't have any clue on how many records could be affected with the fixups. Do you have any record count numbers for those rollups/opportunities/Affiliations so i can create some test data to test it more thoroughly. Or is there standardised test data available?

As a side note: when you change the CON_ContactMerge_TDTM execution to async in the TDTM tables it will fail with the current code. Hitting the same error i'm trying to fix.

Thanks, Sten

davidhabib commented 7 years ago

yes, we've found that our original idea of being able to make any trigger async was overly optimistic, and the potential permutations of async & sync was too much for us to even consider fully testing. I will let @mpusto, our product manager who prioritizes our backlog, help decide how we want to tackle this. thanks.

stenebenau commented 7 years ago

Thanks. Happy to push my changes if needed.

mpusto commented 7 years ago

**lurch: add

mpusto commented 7 years ago

@davidhabib I'd like to discuss further when you are back to see what our next steps on this might be.

LurchTheButler commented 7 years ago

Tracking W-017617

stenebenau commented 7 years ago

Hi Guys,

Did you already discuss this issue?

Sten

davidhabib commented 7 years ago

@stenebenau @mpusto sorry, we have not yet discussed.

davidhabib commented 7 years ago

So our contact merge code currently does a bunch of work in a future method, as described previously. For us to make this change to avoid the future call if we are already in a batch is trivial from a coding perspective, but if we do this so anyone can use contact merge from batch, we'd really need to fully test it first to make sure we weren't opening a whole can of worms. running our current Apex tests wouldn't exercise the new functionality. So to actually test it, we'd have to create an actual batch class to call it in a batch, and then write test code. @stenebenau is there any chance you would be interested in undertaking this testing work and submitting a pull request with a batch class and test code? that would certainly help get this change into the product sooner. thanks. @judisohn fyi.

stenebenau commented 7 years ago

Sure. Happy to help if that would speed up things. Can I just start coding this or is there a need for functional specs?

davidhabib commented 7 years ago

I'm thinking we just need a batch class that calls ContactMerge, and then a test class that exercises calling ContactMerge thru this batch class. Given a batch is typically given a list of objects to process, I'm not sure what this ContactMerge batch will take, however. thoughts?

stenebenau commented 7 years ago

I created a pull request #2504. Let me know what you think.

mrbelvedere commented 5 years ago

Included in beta release 3.151 (Beta 15)

mrbelvedere commented 5 years ago

Included in production release 3.151