From 44f7a793b435faad4d635a8b12397d714ca41a1a Mon Sep 17 00:00:00 2001 From: Jon Davis Date: Thu, 13 Aug 2015 00:27:27 -0700 Subject: [PATCH 1/2] issue #222 - Multi rolllup order by tests to demonstrate issue Tests for ensuring rolled-up value is correct for each rollup and corresponds to its order by. testMultiRollupOfDifferentTypesDifferentOrderBy will fail intentionally to demonstrate the issue --- rolluptool/src/classes/RollupServiceTest.cls | 106 +++++++++++++++++++ rolluptool/src/classes/TestLREngine.cls | 68 ++++++++++++ 2 files changed, 174 insertions(+) diff --git a/rolluptool/src/classes/RollupServiceTest.cls b/rolluptool/src/classes/RollupServiceTest.cls index 70f4001c..68595923 100644 --- a/rolluptool/src/classes/RollupServiceTest.cls +++ b/rolluptool/src/classes/RollupServiceTest.cls @@ -1102,6 +1102,112 @@ private with sharing class RollupServiceTest System.assertEquals(expectedResultB, accountResult.Description); } + /** + * Current default behavior of LREngine is to order by relationship field, then by each + * rollup summary detail field (FieldToAggregate__c) in the order specified (e.g. AccountId, StageName, Name) + * within the context (RollupSummaryField order). If no order by is specified on a rollupsummaryfield + * the detail field is used for the order by following the Then By approach based on order + * in the context. + * + * Current default behavior of DLRS is to build the context with all rollupsummaries + * retrieving them ordered by ParentObject__c (Account) and then by RelationshipField__c (e.g. AccountId) + * which results in non-deterministic result so a test cannot reliabily be written against + * multiple rollups on same parent/child relationship when no order by is specified. + */ + private static Id setupMultiRollupDifferentTypes(RollupSummaries.AggregateOperation operationA, Schema.DescribeFieldResult orderByFieldA, RollupSummaries.AggregateOperation operationB, Schema.DescribeFieldResult orderByFieldB) + { + // Test data + // OpportunityName => Amount;CloseDateAddMonthsToToday;StageName + Map opportunityData = new Map { + 'Joe' => '250;0;Open', + 'Steve' => '50;1;Prospecting', + 'Kim' => '100;-2;Closed Won', + 'Charlie' => '225;-1;Needs Analysis'}; + + // Configure rollup A + LookupRollupSummary__c rollupSummaryA = new LookupRollupSummary__c(); + rollupSummaryA.Name = 'First Opportunity Name into Sic on Account'; + rollupSummaryA.ParentObject__c = 'Account'; + rollupSummaryA.ChildObject__c = 'Opportunity'; + rollupSummaryA.RelationShipField__c = 'AccountId'; + rollupSummaryA.RelationShipCriteria__c = null; + rollupSummaryA.FieldToAggregate__c = 'StageName'; + rollupSummaryA.FieldToOrderBy__c = orderByFieldA != null ? orderByFieldA.getName() : null; + rollupSummaryA.AggregateOperation__c = operationA.name(); + rollupSummaryA.AggregateResultField__c = 'Sic'; + rollupSummaryA.Active__c = true; + rollupSummaryA.CalculationMode__c = 'Realtime'; + + // Configure rollup B + LookupRollupSummary__c rollupSummaryB = new LookupRollupSummary__c(); + rollupSummaryB.Name = 'Concatenate Opportunities Stage Name into Description on Account'; + rollupSummaryB.ParentObject__c = 'Account'; + rollupSummaryB.ChildObject__c = 'Opportunity'; + rollupSummaryB.RelationShipField__c = 'AccountId'; + rollupSummaryB.RelationShipCriteria__c = null; + rollupSummaryB.FieldToAggregate__c = 'Name'; + rollupSummaryB.FieldToOrderBy__c = orderByFieldB != null ? orderByFieldB.getName() : null; + rollupSummaryB.AggregateOperation__c = operationB.name(); + rollupSummaryB.AggregateResultField__c = 'Description'; + rollupSummaryB.ConcatenateDelimiter__c = ','; + rollupSummaryB.Active__c = true; + rollupSummaryB.CalculationMode__c = 'Realtime'; + + // Insert rollup definitions + insert new List { rollupSummaryA, rollupSummaryB }; + + // Test data + Account account = new Account(); + account.Name = 'Test Account'; + account.AnnualRevenue = 0; + insert account; + + Date today = System.today(); + List opps = new List(); + for (String opportunityName :opportunityData.keySet()) + { + List oppFieldValues = opportunityData.get(opportunityName).split(';'); + Opportunity opp = new Opportunity(); + opp.Name = opportunityName; + opp.AccountId = account.Id; + opp.Amount = Decimal.valueOf(oppFieldValues[0]); + opp.CloseDate = today.addMonths(Integer.valueOf(oppFieldValues[1])); + opp.StageName = oppFieldValues[2]; + opps.add(opp); + } + insert opps; + + return account.Id; + } + + /** + * Test default behavior with different order by on each rollup + */ + private testmethod static void testMultiRollupOfDifferentTypesDifferentOrderBy() + { + // Test supported? + if(!TestContext.isSupported()) + return; + + // Test data for rollup A + String expectedResultA = 'Closed Won'; + RollupSummaries.AggregateOperation operationA = RollupSummaries.AggregateOperation.First; + Schema.DescribeFieldResult orderByFieldA = Schema.SObjectType.Opportunity.fields.CloseDate; + + // Test data for rollup B + String expectedResultB = 'Steve,Kim,Charlie,Joe'; + RollupSummaries.AggregateOperation operationB = RollupSummaries.AggregateOperation.Concatenate; + Schema.DescribeFieldResult orderByFieldB = Schema.SObjectType.Opportunity.fields.Amount; + + // generate rollups and data + Id accountId = setupMultiRollupDifferentTypes(operationA, orderByFieldA, operationB, orderByFieldB); + + // Assert rollup + Account accountResult = Database.query('select Sic, Description from Account where Id = :accountId'); + System.assertEquals(expectedResultA, accountResult.Sic); + System.assertEquals(expectedResultB, accountResult.Description); + } + private testmethod static void testPicklistRollup() { // Test supported? diff --git a/rolluptool/src/classes/TestLREngine.cls b/rolluptool/src/classes/TestLREngine.cls index 592eb529..0e2d80ba 100644 --- a/rolluptool/src/classes/TestLREngine.cls +++ b/rolluptool/src/classes/TestLREngine.cls @@ -712,6 +712,74 @@ private class TestLREngine { 'Won,Won,Lost'); } + /** + * Current default behavior of LREngine is to build the order by clause based on the following + * 1) LookupField then by + * 2) For each RollupSummaryField in context (in the order specified in the context) + * a) if detailOrderBy is specified use detailOrderBy.getName() + * b) else use detail.getName() + * + * This results in all queries having an order by even if one is not specified. + * Also, results in summary fields after the first having their order by influenced by previous summary fields + * + * For example, if two rollup summary fields are in the context as follows: + * 1) Order By Amount + * 2) Order By CloseDate + * + * A single SOQL will be executed with an order by of AccountId, Amount, CloseDate + */ + static testMethod void testMultipleRollupsDifferentFieldWithDifferentOrderBy() { + // create seed data + prepareData(); + + // force the 'Lost' Opportunity to be the oldest to demonstrate that + // even when ordering by CloseDate, order by will be based on AccountId, Amount, CloseDate + // since summaries are in the same context and context applies order by fields + // using "Then By" approach + Opportunity makeOldest = [SELECT Id, CloseDate FROM Opportunity WHERE AccountId = :acc2.Id AND Name = 'o2Acc2' LIMIT 1]; + makeOldest.CloseDate = System.today().addMonths(-24); + update makeOldest; + + // assert that the oldest opportunity is the 400 one that we just changed and that its + // stage name is lost + Opportunity assertOldest = [SELECT Id, Amount, StageName FROM Opportunity WHERE AccountId = :acc2.Id ORDER BY AccountId,CloseDate LIMIT 1]; + System.assertEquals(400, assertOldest.Amount); + System.assertEquals('Lost', assertOldest.StageName); + + LREngine.Context ctx = new LREngine.Context(Account.SobjectType, + Opportunity.SobjectType, + Schema.SObjectType.Opportunity.fields.AccountId); + + LREngine.RollupSummaryField rollupField1 = + new LREngine.RollupSummaryField( + Schema.SObjectType.Account.fields.Description, + Schema.SObjectType.Opportunity.fields.StageName, + Schema.SObjectType.Opportunity.fields.Amount, + LREngine.RollupOperation.Concatenate, ',' + ); + ctx.add(rollupField1); + LREngine.RollupSummaryField rollupField2 = + new LREngine.RollupSummaryField( + Schema.SObjectType.Account.fields.Sic, + Schema.SObjectType.Opportunity.fields.StageName, + Schema.SObjectType.Opportunity.fields.CloseDate, + LREngine.RollupOperation.First, null + ); + ctx.add(rollupField2); + + SObject[] masters = LREngine.rollUp(ctx, detailRecords); + + Map mastersById = new Map(masters); + Account reloadedAcc1 = (Account)mastersById.get(acc1.Id); + Account reloadedAcc2 = (Account)mastersById.get(acc2.Id); + System.assertEquals(2, masters.size()); + System.assertEquals('test,test,test', reloadedAcc1.get(rollupField1.master.getName())); + System.assertEquals('test', reloadedAcc1.get(rollupField2.master.getName())); + System.assertEquals('Won,Won,Lost', reloadedAcc2.get(rollupField1.master.getName())); + // the oldest is 'Lost' but due to Then By approach, the oldest should be 'Won' + System.assertEquals('Won', reloadedAcc2.get(rollupField2.master.getName())); + } + static testMethod void testRollupConcatenateNoDelimiter() { testRollup( new LREngine.RollupSummaryField( From 4e8c39c2bce37f6780a8420304c8c108c3df1de4 Mon Sep 17 00:00:00 2001 From: Jon Davis Date: Thu, 13 Aug 2015 00:31:59 -0700 Subject: [PATCH 2/2] issue #222 - Ensure order by differentiates context used Even though this addresses the issue, this could appear to cause a regression to an unsuspecting user/admin/dev since the issue this addresses could influence existing rollups that were not respecting proper order by but still could have "appeared" to be working. --- rolluptool/src/classes/RollupService.cls | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/rolluptool/src/classes/RollupService.cls b/rolluptool/src/classes/RollupService.cls index 5e5ae7e3..056a134d 100644 --- a/rolluptool/src/classes/RollupService.cls +++ b/rolluptool/src/classes/RollupService.cls @@ -694,8 +694,10 @@ global with sharing class RollupService LREngine.SharingMode.User : LREngine.SharingMode.System_x; // Determine if an LREngine Context has been created for this parent child relationship, filter combination or underlying query type and sharing mode? - String rsfType = rsf.isAggregateBasedRollup() ? 'aggregate' : 'query'; - String contextKey = lookup.ParentObject__c + '#' + lookup.RelationshipField__c + '#' + lookup.RelationShipCriteria__c + '#' + rsfType + '#' + sharingMode; + String rsfType = rsf.isAggregateBasedRollup() ? 'aggregate' : 'query'; + String orderBy = String.isBlank(Lookup.FieldToOrderBy__c) ? '' : Lookup.FieldToOrderBy__c; + String contextKey = lookup.ParentObject__c + '#' + lookup.RelationshipField__c + '#' + lookup.RelationShipCriteria__c + '#' + rsfType + '#' + sharingMode + '#' + orderBy; + LREngine.Context lreContext = engineCtxByParentRelationship.get(contextKey); if(lreContext==null) {