Issue #216, #239 & #240 - add order by support of multiple fields, ASC/DESC and NULLS FIRST/LAST; move orderby to context; fix to ensure recalc when order by field changes #241
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lds and move to context Issue SFDO-Community#216 - Add support for multiple order by fields and optional ASC/DESC and NULLS FIRST/LAST Issue SFDO-Community#239 - Move order by clause to LREngine Context
…rder by field changes
This was referenced Aug 30, 2015
|
Please see comments in #239 (comment) prior to merging this PR. Only 1 of the following PRs should be merged: #238, #241, #243, #244 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is an alternative to PR #238 (#238 or this PR should be merged, not both) and incorporates Issue #239 moving the order by to the context, eliminating any default orderby applied when none is specified (except for RelationshipField__c) and updating the helptext of FieldToOrderBy__c . It also includes #216 and #240.
Since this was all fresh in my mind, I decided to implement #239 in case it's decided to use that approach instead of keeping the order by on the RollupSummaryField. Based on working through #216 and the other issues identified that were addressed around orderby, I believe the approach in this PR should be chosen as it offers the benefits mentioned in #239. The downside to this PR as opposed to #238 is that it does change signatures of RollupSummaryField and Context constructors. However, since the orderby did not make it to LRE project, the surface area of the impact is minimized to DLRS internally and anyone using DLRS source only. The change to use the context will result in different rolled up results when no order by is specified compared to the ThenBy approach when RollupSummaryField contains the orderby. However, in both cases it is a non-deterministic result so the net effect is essentially the same. Moving to context seems to be a more natural way to manage the order by based on DLRS usage and avoids a lot of unnecessary processing.
Note