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

v1.2.0 - Grand(and greater)parent rollups #57

Merged
merged 33 commits into from
Mar 19, 2021

Conversation

jamessimone
Copy link
Owner

  • grandparent rollups started from the child have no practical limit - you can rollup values to a child's great-great-grandparent, for example
  • grandparent rollups started from the parent respect SOQL's map relationship-field hopping of 5 levels:

In each specified relationship, no more than five levels can be specified in a child-to-parent relationship. For example, Contact.Account.Owner.FirstName (three levels)

  • Deprecated RollupControl__mdt.MaximumRollupRetries__c in favor of RollupControl__mdt.MaxRollupRetries__c to keep naming consistent
  • Updated query architecture for rollups initiated from parents, in general - bind variables are now used to prevent query size limits from being reached

…h to the grand (or greater) parent, finishing the cleanup of this POC because it was a good time
…f RollupControl__mdt.MaxParentRowsUpdatedAtOnce__c. Previously an exception was being thrown if there were too many parent records getting updated, but we can just chunk the updates. Started investigating why MaximumRollupRetries__c is not being used. Current status - fixing shouldDeferUpdateWhenMaxParentRowsLessThanCurrentUpdateRows() test
…overflow for full recalculation query string
…se up the object chain to retrieve the ultimate parent records. Needs more testing
… to determine if the ultimate parent of a record has changed
…ng that it uses the where clause effectively (may still need more testing here), and that a singular parent tied to multiple children is tracked correctly
…ed the reflection issue with flow engine and SObject collections
…do related to re-queueing and avoiding duplicate queries
…llups, updated Readme documentation, bumped medium version, replaced local variables where they were shadowing class variables
…arenting when the values have also changed - yikes!
… that grandparent items can eventually compare whether or not the old value changed during reparenting; it will also be necessary to test that this works on the non-reparenting route
…t these have to conform to the SOQL max object jumps of 5
…hipFieldFinder, and added test proving it works
@jamessimone jamessimone requested a review from jongpie March 17, 2021 15:31
@jamessimone jamessimone temporarily deployed to Test March 17, 2021 15:31 Inactive
@codecov
Copy link

codecov bot commented Mar 17, 2021

Codecov Report

Merging #57 (3dd60c6) into master (f1b984c) will increase coverage by 0.73%.
The diff coverage is 95.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
+ Coverage   94.43%   95.17%   +0.73%     
==========================================
  Files           7        8       +1     
  Lines        2032     2340     +308     
==========================================
+ Hits         1919     2227     +308     
  Misses        113      113              
Impacted Files Coverage Δ
.../default/classes/RollupRelationshipFieldFinder.cls 95.27% <95.27%> (ø)
rollup/main/default/classes/Rollup.cls 95.93% <95.61%> (+1.21%) ⬆️
rollup/main/default/classes/RollupCalculator.cls 94.94% <100.00%> (+0.02%) ⬆️
rollup/main/default/classes/RollupEvaluator.cls 92.06% <100.00%> (+0.28%) ⬆️
...p/main/default/classes/RollupFlowBulkProcessor.cls 97.77% <100.00%> (+0.05%) ⬆️
...in/default/classes/RollupFullBatchRecalculator.cls 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1b984c...3dd60c6. Read the comment docs.

@jamessimone jamessimone temporarily deployed to Test March 17, 2021 20:23 Inactive
Copy link
Collaborator

@jongpie jongpie left a comment

Choose a reason for hiding this comment

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

I did an initial review of the code - things look great overall, I don't have any major changes to your approach for handling grandparents. But, I added a few comments related to naming conventions and a few clarifying questions. I'm planning to do some more testing in a scratch org to see if I find any bugs - I'll let ya know if I find anything else!

rollup/main/default/classes/RollupEvaluatorTests.cls Outdated Show resolved Hide resolved
@@ -0,0 +1,13 @@
<?xml version="1.0" encoding="UTF-8"?>
<CustomField xmlns="http://soap.sforce.com/2006/04/metadata">
<fullName>GrandparentRelationshipFieldPath__c</fullName>
Copy link
Collaborator

Choose a reason for hiding this comment

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

So far, you've been using naming conventions like CalcItem__c and LookupObject__c (instead of Parent and Child), but now you're incorporating the term Grandparent into some of your fields - any thoughts on updating the naming convention to consistently have names like GrandparentRelationshipFieldPath__c, ParentObject__c and ChildObject__c?

Renaming existing fields & methods could obviously lead to some headaches (especially for existing orgs trying to upgrade), so not a big deal if you want to stick with the current naming convention.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think I'll stick to it for now, but this is good food for thought in the pre-managed package era. Since the next big thing I want to do is re-do the folder structure, I'll 📍this - I love the idea, overall, and think I'll probably update them long-term

@@ -0,0 +1,14 @@
<?xml version="1.0" encoding="UTF-8"?>
<CustomField xmlns="http://soap.sforce.com/2006/04/metadata">
<fullName>MaxParentRowsUpdatedAtOnce__c</fullName>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Related to my other comment on naming convention, here you're using parent in this field name, which seems a little inconsistent with other fields like CalcItem__c and LookupObject__c that are (more or less) referring to the same concept, but don't currently use terms like parent and child.

Your other fields on RollupControl__mdt are also using "Max Lookup Rows..." but this new field now has "Max Parent Rows...", so it seems inconsistent.

image

Copy link
Owner Author

Choose a reason for hiding this comment

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

See my response above - I've definitely already started unconsciously (as a result of working on this feature) incorporated what will eventually amount to taking you up on your stellar comment above. I don't want to further bloat this change by introducing a ton of other renamings (some of which are already noise in this PR introduced by the use of apex-assist helpfully pointing our where I was shadowing variable names).

rollup/main/default/classes/RollupTests.cls Outdated Show resolved Hide resolved
…, added better clarity surrounding when RollupRelationshipFieldFinder uses the simple out of lookup ids changing for reparenting, reverted an unnecessary change from null to empty string in RollupTests, and removed the unnecessary System namespace prefix from the Formula method in RollupEvaluatorTests
@jamessimone jamessimone temporarily deployed to Test March 18, 2021 01:21 Inactive
…re legitimately updated, Rollup now properly recalculates grandparent rollups
@jamessimone jamessimone temporarily deployed to Test March 19, 2021 02:32 Inactive
@@ -311,4 +311,54 @@ private class RollupIntegrationTests {
System.assertEquals(secondChild.Name, updatedGreatGrandparent.Name, 'CONCAT_DISTINCT and reparenting should have worked');
System.assertEquals(child.Name, updatedGreatGrandparentTwo.Name, 'CONCAT_DISTINCT and reparenting should have worked again');
}

@isTest
static void shouldRunGrandparentRollupsWhenIntermediateObjectsAreUpdated() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Niiiiiice

Copy link
Owner Author

Choose a reason for hiding this comment

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

So glad you tested that scenario

String.isNotBlank(rollupInfo.GrandparentRelationshipFieldPath__c) &&
calcItems.isEmpty() == false
) {
// it's a grandparent rollup triggered by an intermediate object
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's beautiful

@jamessimone jamessimone temporarily deployed to Test March 19, 2021 03:55 Inactive
@jamessimone jamessimone temporarily deployed to Test March 19, 2021 13:26 Inactive
@jamessimone jamessimone merged commit 0cd4f62 into master Mar 19, 2021
@jamessimone jamessimone deleted the feature/great-grandchildren-rollups branch March 19, 2021 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for grandchild to grandparent rollups (and potentially more)
2 participants