Skip to content

Commit

Permalink
Merge pull request #223 from jondavis9898/issue222-incorrect-result-d…
Browse files Browse the repository at this point in the history 
…ifferent-orderby

issue #222 - incorrect result when same parent/child with different order by
  • Loading branch information
@afawcett
afawcett committed Aug 14, 2015
2 parents 3b5e4f6 + 4e8c39c commit cc448fb
Show file tree
Hide file tree
Showing 3 changed files with 178 additions and 2 deletions.
6 changes: 4 additions & 2 deletions rolluptool/src/classes/RollupService.cls
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
106 changes: 106 additions & 0 deletions rolluptool/src/classes/RollupServiceTest.cls
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, String> opportunityData = new Map<String, String> {
'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<LookupRollupSummary__c> { rollupSummaryA, rollupSummaryB };

// Test data
Account account = new Account();
account.Name = 'Test Account';
account.AnnualRevenue = 0;
insert account;

Date today = System.today();
List<Opportunity> opps = new List<Opportunity>();
for (String opportunityName :opportunityData.keySet())
{
List<String> 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?
Expand Down
68 changes: 68 additions & 0 deletions rolluptool/src/classes/TestLREngine.cls
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Id, SObject> mastersById = new Map<Id, SObject>(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(
Expand Down

0 comments on commit cc448fb

Please sign in to comment.