issue #222 - incorrect result when same parent/child with different order by #223
Conversation
…e 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
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.
|
Great work @jondavis9898, really really impressed with the depth of the testing here, for sure adds more stability for future changes. |
…ifferent-orderby issue #222 - incorrect result when same parent/child with different order by
|
Thanks @afawcett. I thought more about my earlier comments and I think the Context should really have the order by, not the individual rollupsummaryfields as this leads to the "Then By" approach. Do you think it's worth moving the order by to the context similar to WHERE clause (possible regression to anyone using source version of DLRS) adjusting any of that or just focus changes on core DLRS with the knowledge of how LREngine works? Personally, if no order by is specified, I don't think one should be applied but LREngine applies one when one isn't specified. |
|
Gut feel for now, leave it as is. |
|
no worries, will do. I'll start working on the multi-order by case with this now resolved. |
See commit jondavis9898@44f7a79 for tests that demonstrate issue.
See commit jondavis9898@4e8c39c for proposed fix.
While I tackled this from the DLRS side, I think it makes sense to address this within LREngine as well by not having LREngine apply the "Then By" approach and instead not adding to the order by when detailOrderBy is null. This PR works around that behavior when DLRS rollups have order by specified but still leaves open a gap when no order by is specified as when more rollups are added, order will change based on the way LREngine builds order by. It's still non-deterministic in that case but a quasi-controlled non-deterministic.
Interested in thoughts on whether LREngine should be adjusted per the above or if we let DLRS just work around this as best it can.