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

Issue #216 & #240 - add order by support of multiple fields, ASC/DESC and NULLS FIRST/LAST & fix to ensure recalc when order by field changes #238

Conversation

jondavis9898
Copy link
Contributor

I followed the preference (#223 (comment)) to leave the order by at the RollupSummaryField level rather than move it to the Context level and believe this provides as much backwards compat as possible.

The only place I could not provide 100% backwards compat is on the public member variable detailOrderBy in RollupSummaryField which changed from a Schema.DescribeField to a List<LREngine.Ordering>. It does not appear that the order by additions made in DLRS previously have made their way over to the LREngine project so this change would only effect internal DLRS usage (which I've handled in this PR) and anyone using the library source (and directly accessing this member) instead of as a package.

With all that said, I think it's worth discussing moving the order by to the context level for a couple of reasons. I'll open a new issue to explain in more detail.

@jondavis9898 jondavis9898 changed the title Issue #216 - add order by support of multiple fields, ASC/DESC and NULLS FIRST/LAST Issue #216 & #240 - add order by support of multiple fields, ASC/DESC and NULLS FIRST/LAST & fix to ensure recalc when order by field changes Aug 30, 2015
@jondavis9898
Copy link
Contributor Author

Please see comments in #239 (comment) prior to merging this PR.

Only 1 of the following PRs should be merged: #238, #241, #243, #244

@afawcett afawcett closed this Oct 17, 2015
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.

2 participants