refactor: share the answer filter between charts and raw data - #1503
Merged
renemadsen merged 2 commits intoJul 29, 2026
Conversation
ChartDataHelpers held two near-identical ~2100-line methods, each repeating the same answer filter: workflow state, date range, survey, filter question/answer, measured question, dashboard location/tag and ignored options. AnswerFilterHelper repeated it a third time for the raw data table, with only comments to stop the three drifting apart. The filter now lives once, in AnswerFilterHelper: - BuildFilteredAnswerValues covers the prefix every caller shares. - ApplyLocationFilter covers the site-wins-over-tag rule, used at four call sites (text and non-text, in both Calculate methods). - GetIgnoredOptionIds / ApplyIgnoredOptions cover the excluded options. - BuildAnswerQuery composes those, then adds the Answer.WorkflowState filter the charts deliberately do not apply. ChartDataHelpers drops from 4274 to 4120 lines and both Calculate methods now compose the same helpers, so the chart and the raw data table cannot disagree about which answers belong to an item. Two deliberate changes of substance: - The filter-question step becomes a correlated subquery instead of materialising answer ids and passing them back as IN(...). The selected set is identical and it saves a round trip. Because the subquery closes over the shared queryable, the Includes now attach after all Where clauses rather than before - a subquery carrying Includes is a translation hazard, and the original never created one because it materialised that step separately. - A leftover Console.WriteLine of the survey id is removed. Behaviour is otherwise unchanged, including the quirks: a location wins over a tag and they never combine; compared charts ignore the dashboard-level location; and with neither location nor tag set the non-compared branch still yields nothing. AnswerFilterHelperUTests covers the filter-question path directly, because every Dashboard*.data.json fixture has filterQuestionId null - so neither ChartDataUTests nor the raw data reconciliation test would have caught a regression in the rewritten step. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EnP42zmHAgo2NZQsa6zdhG
Code review caught the refactor contradicting its own comment. The shared filter warned that a subquery carrying Includes is a translation hazard, then attached four Includes to its return value - which BuildAnswerQuery feeds straight into an answer-id subquery and a Union. Stable's BuildAnswerQuery had no Includes at all, so this was a net-new EF translation surface on an already-shipped path, and would have logged NavigationBaseIncludeIgnored on every raw-data request. BuildFilteredAnswerValues now returns without Includes, and the two chart call sites attach their own, ending in AsQueryable() so the inferred type still allows the later reassignments. Both the chart query and the raw-data query now match stable exactly. Also from review: - IsComparedData goes back to private; widening it bought nothing. - The filter-question test only compared two distinct questions, missing the two cases where Contains-over-a-list and Any-over-a-subquery could actually diverge. Added one test where the filter question IS the measured question, and one where an answer carries repeated values for the filter question. - LocationFilter_PrefersSiteOverTag never passed a site and a tag together, so it never tested the precedence in its name. It does now. - Non-empty guard moved ahead of the comparison, so thin seed data reads as "cannot test" rather than "code is broken"; dropped a dead Where after a GroupBy; added the missing using. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EnP42zmHAgo2NZQsa6zdhG
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Follow-up to #1502, which deliberately left this out:
AnswerFilterHelpermirroredChartDataHelpers' answer selection by hand, with comments as the only thing keeping the two aligned.ChartDataHelpersheld two near-identical ~2100-line methods, each repeating the same filter — workflow state, date range, survey, filter question/answer, measured question, dashboard location/tag, ignored options. The raw data table repeated it a third time.The filter now lives once:
BuildFilteredAnswerValuesApplyLocationFilterGetIgnoredOptionIds/ApplyIgnoredOptionsBuildAnswerQueryChartDataHelpersdrops 4274 → 4120 lines, and the chart and raw data table can no longer disagree about which answers belong to an item.Two deliberate changes of substance
IN (...)list. Same set, one fewer round trip. Because the subquery closes over the shared queryable, theIncludes now attach after allWhereclauses — a subquery carrying Includes is a translation hazard, and the original never created one because it materialised that step separately.Console.WriteLineof the survey id is removed.Behaviour is otherwise unchanged, quirks included: a location wins over a tag and they never combine; compared charts ignore the dashboard-level location; and with neither location nor tag set the non-compared branch still yields nothing.
Testing
ChartDataUTestscompares chart output against golden JSON fixtures and is the primary regression net for this change.It has a gap, though, which is why this PR adds
AnswerFilterHelperUTests: everyDashboard*.data.jsonfixture hasfilterQuestionId: null, so neitherChartDataUTestsnor the raw data reconciliation test exercises the filter-question path — exactly the step this refactor rewrote. The new tests target it directly, asserting the correlated subquery selects precisely the answers the materialised id list did, and that the assertion is non-vacuous.Verified locally:
dotnet buildclean. The suite itself runs in CI, against a420_SDKseeded from420_SDK.sql— not against a dev database.🤖 Generated with Claude Code
https://claude.ai/code/session_01EnP42zmHAgo2NZQsa6zdhG