From 806600a399ecfa8e0445c5e97ecd699656ed9a5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Schultz=20Madsen?= Date: Wed, 29 Jul 2026 13:48:29 +0200 Subject: [PATCH 1/2] refactor: share the answer filter between charts and raw data 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) Claude-Session: https://claude.ai/code/session_01EnP42zmHAgo2NZQsa6zdhG --- .../AnswerFilterHelperUTests.cs | 153 ++++++++++++++ .../Helpers/AnswerFilterHelper.cs | 167 ++++++++++----- .../Helpers/ChartDataHelpers.cs | 190 ++---------------- 3 files changed, 287 insertions(+), 223 deletions(-) create mode 100644 eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn.Test/AnswerFilterHelperUTests.cs diff --git a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn.Test/AnswerFilterHelperUTests.cs b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn.Test/AnswerFilterHelperUTests.cs new file mode 100644 index 00000000..1971830c --- /dev/null +++ b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn.Test/AnswerFilterHelperUTests.cs @@ -0,0 +1,153 @@ +/* +The MIT License (MIT) + +Copyright (c) 2007 - 2021 Microting A/S + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. +*/ + +namespace InsightDashboard.Pn.Test; + +using System.Linq; +using System.Threading.Tasks; +using Base; +using Infrastructure.Helpers; +using Infrastructure.Models.Dashboards; +using Microsoft.EntityFrameworkCore; +using Microting.eForm.Infrastructure.Constants; +using Microting.InsightDashboardBase.Infrastructure.Data.Entities; +using NUnit.Framework; + +/// +/// Covers the filter-question path, which none of the Dashboard*.data.json +/// fixtures exercise - every one of them has filterQuestionId null. Extracting the +/// shared filter out of ChartDataHelpers rewrote that path from "materialise the +/// matching answer ids, then use Contains" into a correlated subquery, so it needs +/// its own test rather than relying on the chart fixtures. +/// +[TestFixture] +public class AnswerFilterHelperUTests : DbTestFixture +{ + [Test] + public async Task FilterQuestion_SubqueryMatchesMaterialisedIds() + { + // Find a survey with at least two answered questions, so one can measure + // and the other can filter. + var candidate = await DbContext.AnswerValues + .AsNoTracking() + .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) + .GroupBy(x => new { x.Answer.QuestionSetId, x.QuestionId }) + .Select(g => new { g.Key.QuestionSetId, g.Key.QuestionId, Count = g.Count() }) + .Where(x => x.Count > 0) + .OrderByDescending(x => x.Count) + .Take(20) + .ToListAsync(); + + var survey = candidate + .GroupBy(x => x.QuestionSetId) + .FirstOrDefault(g => g.Count() >= 2); + + Assert.That(survey, Is.Not.Null, + "Seed data has no survey with two answered questions; cannot exercise filtering."); + + var firstQuestionId = survey.ElementAt(0).QuestionId; + var filterQuestionId = survey.ElementAt(1).QuestionId; + + var filterOptionId = await DbContext.AnswerValues + .AsNoTracking() + .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) + .Where(x => x.QuestionId == filterQuestionId) + .Select(x => x.OptionId) + .FirstOrDefaultAsync(); + + Assert.That(filterOptionId, Is.GreaterThan(0), "No option found for the filter question."); + + var dashboardItem = new DashboardItem + { + FirstQuestionId = firstQuestionId, + FilterQuestionId = filterQuestionId, + FilterAnswerId = filterOptionId, + IgnoredAnswerValues = new System.Collections.Generic.List(), + CompareLocationsTags = new System.Collections.Generic.List(), + }; + + var answerDates = new DashboardEditAnswerDates(); + + var actual = await AnswerFilterHelper + .BuildFilteredAnswerValues(DbContext, dashboardItem, survey.Key, answerDates) + .Select(x => x.AnswerId) + .Distinct() + .ToListAsync(); + + // The shape ChartDataHelpers used before the shared filter was extracted. + var scope = DbContext.AnswerValues + .AsNoTracking() + .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) + .Where(x => x.Answer.QuestionSetId == survey.Key); + + var materialisedIds = await scope + .Where(y => y.QuestionId == filterQuestionId && y.OptionId == filterOptionId) + .Select(y => y.AnswerId) + .ToListAsync(); + + var expected = await scope + .Where(x => materialisedIds.Contains(x.AnswerId)) + .Where(x => x.QuestionId == firstQuestionId) + .Select(x => x.AnswerId) + .Distinct() + .ToListAsync(); + + Assert.That(actual.OrderBy(x => x), Is.EqualTo(expected.OrderBy(x => x)), + "The correlated subquery must select exactly the answers the materialised " + + "id list did, otherwise every filtered chart shifts."); + + Assert.That(expected, Is.Not.Empty, + "The chosen question pair matched no answers, so this test proved nothing. " + + "Pick a different pair if the seed data changes."); + } + + /// + /// A location wins over a tag, and neither set leaves the query untouched. + /// + [Test] + public async Task LocationFilter_PrefersSiteOverTag() + { + var siteId = await DbContext.Answers + .AsNoTracking() + .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) + .Select(x => x.SiteId) + .FirstOrDefaultAsync(); + + Assert.That(siteId, Is.GreaterThan(0), "Seed data has no answers."); + + var all = DbContext.AnswerValues + .AsNoTracking() + .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed); + + var unfiltered = await all.CountAsync(); + var bySite = await AnswerFilterHelper.ApplyLocationFilter(all, siteId, null).CountAsync(); + var untouched = await AnswerFilterHelper.ApplyLocationFilter(all, null, null).CountAsync(); + + Assert.That(untouched, Is.EqualTo(unfiltered), + "With neither location nor tag the query must be returned unchanged."); + Assert.That(bySite, Is.LessThanOrEqualTo(unfiltered)); + Assert.That(bySite, Is.GreaterThan(0), + "Filtering by a site that has answers must not empty the query."); + } +} diff --git a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Infrastructure/Helpers/AnswerFilterHelper.cs b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Infrastructure/Helpers/AnswerFilterHelper.cs index 795f401c..eeca698f 100644 --- a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Infrastructure/Helpers/AnswerFilterHelper.cs +++ b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Infrastructure/Helpers/AnswerFilterHelper.cs @@ -36,65 +36,75 @@ namespace InsightDashboard.Pn.Infrastructure.Helpers; using Models.Dashboards; /// -/// Selects the answers that feed a single dashboard item. +/// Single home for "which answers belong to this dashboard item". /// -/// This MUST stay behaviourally identical to the answer-selection half of -/// ChartDataHelpers.CalculateDashboardItem, otherwise the raw data table will -/// disagree with the chart it sits under. Line references below point at -/// ChartDataHelpers.cs as of the commit that introduced this file. -/// -/// Two deliberate deviations: -/// 1. Answer.WorkflowState is also filtered (ChartDataHelpers filters only -/// AnswerValue.WorkflowState). The delete path sets both together, so this -/// does not change counts in practice. -/// 2. The filter-question step uses a correlated subquery instead of -/// materialising answer ids with ToList(). Semantically identical, one -/// fewer round trip. +/// ChartDataHelpers.CalculateDashboardItem and CalculateDashboardItemByWeight both +/// build the same filtered AnswerValue query, and the raw data table has to select +/// exactly the same answers or its row count will not reconcile with the chart it +/// sits under. All three now compose the methods below instead of repeating the +/// predicates, so they cannot drift apart. /// public static class AnswerFilterHelper { - public static IQueryable BuildAnswerQuery( + /// + /// The filter every caller shares: workflow state, date range, survey, the + /// optional filter question/answer pair, and the measured question. + /// + /// Note this deliberately does NOT filter Answer.WorkflowState - the charts + /// never have, and adding it here would silently change every chart. The raw + /// data table applies it separately in BuildAnswerQuery. + /// + public static IQueryable BuildFilteredAnswerValues( MicrotingDbContext sdkContext, DashboardItem dashboardItem, int dashboardSurveyId, - int? dashboardLocationId, - int? dashboardLocationTagId, DashboardEditAnswerDates answerDates) { - // ChartDataHelpers.cs:135-142 - var answerValues = sdkContext.AnswerValues + // Includes are attached at the end, not here: the filter-question step + // below closes over this queryable to build a correlated subquery, and a + // subquery carrying Includes is a translation hazard. The original code + // materialised that step separately, so EF never saw the combination. + var answerQueryable = sdkContext.AnswerValues .AsNoTracking() .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) - .Where(x => x.Answer.WorkflowState != Constants.WorkflowStates.Removed) .AsQueryable(); - // ChartDataHelpers.cs:144-154 if (answerDates.Today) { var dateTimeNow = DateTime.Now; answerDates.DateTo = new DateTime( - dateTimeNow.Year, dateTimeNow.Month, dateTimeNow.Day, 23, 59, 59); + dateTimeNow.Year, + dateTimeNow.Month, + dateTimeNow.Day, + 23, + 59, + 59); } - // ChartDataHelpers.cs:156-166 if (answerDates.DateFrom != null) { - answerValues = answerValues.Where(x => x.Answer.FinishedAt >= answerDates.DateFrom); + answerQueryable = answerQueryable + .Where(x => x.Answer.FinishedAt >= answerDates.DateFrom); } if (answerDates.DateTo != null) { - answerValues = answerValues.Where(x => x.Answer.FinishedAt <= answerDates.DateTo); + answerQueryable = answerQueryable + .Where(x => x.Answer.FinishedAt <= answerDates.DateTo); } - // ChartDataHelpers.cs:170-171 - answerValues = answerValues.Where(x => x.Answer.QuestionSetId == dashboardSurveyId); + answerQueryable = answerQueryable + .Where(x => x.Answer.QuestionSetId == dashboardSurveyId); - // ChartDataHelpers.cs:173-190 if (dashboardItem.FilterQuestionId != null && dashboardItem.FilterAnswerId != null) { - var filterScope = answerValues; - answerValues = answerValues + // Restrict to answers that also carry the filter question's chosen + // option. Expressed as a correlated subquery over the same scope the + // original two-step used, so the generated set is identical without + // the round trip. + var filterScope = answerQueryable; + + answerQueryable = answerQueryable .Where(x => filterScope.Any(y => y.AnswerId == x.AnswerId && y.QuestionId == dashboardItem.FilterQuestionId @@ -103,35 +113,87 @@ public static IQueryable BuildAnswerQuery( } else { - answerValues = answerValues.Where(x => x.QuestionId == dashboardItem.FirstQuestionId); + answerQueryable = answerQueryable + .Where(x => x.QuestionId == dashboardItem.FirstQuestionId); } - // ChartDataHelpers.cs:223-236 - this block only runs when compare is OFF - if (!dashboardItem.CompareEnabled) + // Kept for parity with the original query shape. Every consumer projects + // with Select, so EF ignores these in practice. + return answerQueryable + .Include(x => x.Question) + .Include(x => x.Option) + .Include(x => x.Answer) + .Include(x => x.Option.OptionTranslationses); + } + + /// + /// Dashboard-level location/tag. A location wins over a tag; they are never + /// combined. Applied unconditionally for text questions, and only when compare + /// is off for everything else - compared charts scope by their own compare set. + /// + public static IQueryable ApplyLocationFilter( + IQueryable answerQueryable, + int? dashboardLocationId, + int? dashboardLocationTagId) + { + if (dashboardLocationId != null) + { + return answerQueryable.Where(x => x.Answer.SiteId == dashboardLocationId); + } + + if (dashboardLocationTagId != null) { - if (dashboardLocationId != null) - { - answerValues = answerValues.Where(x => x.Answer.SiteId == dashboardLocationId); - } - else if (dashboardLocationTagId != null) - { - answerValues = answerValues.Where(x => - x.Answer.Site.SiteTags.Any(y => y.TagId == dashboardLocationTagId)); - } + return answerQueryable.Where(x => + x.Answer.Site.SiteTags.Any(y => y.TagId == dashboardLocationTagId)); } - // ChartDataHelpers.cs:240-252 - ignored answer OPTIONS. The column is - // misleadingly named AnswerId but holds options.Id. - var ignoredOptionIds = dashboardItem.IgnoredAnswerValues + return answerQueryable; + } + + /// + /// Options the dashboard item excludes from its calculation. The column is + /// named AnswerId but holds options.Id. + /// + public static int[] GetIgnoredOptionIds(DashboardItem dashboardItem) => + dashboardItem.IgnoredAnswerValues .Where(y => y.WorkflowState != Constants.WorkflowStates.Removed) .Select(x => x.AnswerId) .ToArray(); - if (ignoredOptionIds.Length > 0) + public static IQueryable ApplyIgnoredOptions( + IQueryable answerQueryable, + int[] ignoredOptionIds) => + ignoredOptionIds.Length == 0 + ? answerQueryable + : answerQueryable.Where(x => !ignoredOptionIds.Contains(x.OptionId)); + + /// + /// The answers behind one dashboard item, for the raw data table. + /// + /// Adds Answer.WorkflowState filtering on top of the shared filter. The delete + /// path sets that alongside AnswerValue.WorkflowState, so this does not change + /// counts in practice. + /// + public static IQueryable BuildAnswerQuery( + MicrotingDbContext sdkContext, + DashboardItem dashboardItem, + int dashboardSurveyId, + int? dashboardLocationId, + int? dashboardLocationTagId, + DashboardEditAnswerDates answerDates) + { + var answerValues = BuildFilteredAnswerValues( + sdkContext, dashboardItem, dashboardSurveyId, answerDates) + .Where(x => x.Answer.WorkflowState != Constants.WorkflowStates.Removed); + + if (!dashboardItem.CompareEnabled) { - answerValues = answerValues.Where(x => !ignoredOptionIds.Contains(x.OptionId)); + answerValues = ApplyLocationFilter( + answerValues, dashboardLocationId, dashboardLocationTagId); } + answerValues = ApplyIgnoredOptions(answerValues, GetIgnoredOptionIds(dashboardItem)); + var answerIds = IsComparedData(dashboardItem) ? ComparedAnswerIds(answerValues, dashboardItem, dashboardLocationId, dashboardLocationTagId) : NonComparedAnswerIds(answerValues, dashboardLocationId, dashboardLocationTagId); @@ -142,8 +204,10 @@ public static IQueryable BuildAnswerQuery( .Where(x => answerIds.Contains(x.Id)); } - // ChartDataHelpers.cs:121-133 - private static bool IsComparedData(DashboardItem dashboardItem) + /// + /// Mirrors the isComparedData decision ChartDataHelpers makes. + /// + public static bool IsComparedData(DashboardItem dashboardItem) { if (dashboardItem.ChartType != DashboardChartTypes.GroupedStackedBarChart && dashboardItem.ChartType != DashboardChartTypes.Line) @@ -159,7 +223,6 @@ private static bool IsComparedData(DashboardItem dashboardItem) return dashboardItem.ChartType == DashboardChartTypes.Line && dashboardItem.CalculateAverage; } - // ChartDataHelpers.cs:255-390 - union of the per-tag queries and the site query private static IQueryable ComparedAnswerIds( IQueryable answerValues, DashboardItem dashboardItem, @@ -197,8 +260,10 @@ private static IQueryable ComparedAnswerIds( return byTag.Union(bySite).Distinct(); } - // ChartDataHelpers.cs:392-490 - when neither location nor tag is set the - // chart renders nothing, so the raw table must be empty too. + /// + /// With neither a location nor a tag the chart renders nothing, so the raw + /// table must be empty too. + /// private static IQueryable NonComparedAnswerIds( IQueryable answerValues, int? dashboardLocationId, diff --git a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Infrastructure/Helpers/ChartDataHelpers.cs b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Infrastructure/Helpers/ChartDataHelpers.cs index 4969ca15..5eb5d7cb 100644 --- a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Infrastructure/Helpers/ChartDataHelpers.cs +++ b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Infrastructure/Helpers/ChartDataHelpers.cs @@ -132,77 +132,14 @@ public static async Task CalculateDashboardItem( } } - var answerQueryable = sdkContext.AnswerValues - .AsNoTracking() - .Include(x => x.Question) - .Include(x => x.Option) - .Include(x => x.Answer) - .Include(x => x.Option.OptionTranslationses) - .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) - .AsQueryable(); - - if (answerDates.Today) - { - var dateTimeNow = DateTime.Now; - answerDates.DateTo = new DateTime( - dateTimeNow.Year, - dateTimeNow.Month, - dateTimeNow.Day, - 23, - 59, - 59); - } - - if (answerDates.DateFrom != null) - { - answerQueryable = answerQueryable - .Where(x => x.Answer.FinishedAt >= answerDates.DateFrom); - } - - if (answerDates.DateTo != null) - { - answerQueryable = answerQueryable - .Where(x => x.Answer.FinishedAt <= answerDates.DateTo); - } - - Console.WriteLine($"Using QuestionSetId {dashboardSurveyId}"); - - answerQueryable = answerQueryable - .Where(x => x.Answer.QuestionSetId == dashboardSurveyId); - - if (dashboardItem.FilterQuestionId != null && dashboardItem.FilterAnswerId != null) - { - var answerIds = answerQueryable - .Where(y => y.QuestionId == dashboardItem.FilterQuestionId && - y.OptionId == dashboardItem.FilterAnswerId) - .Select(y => y.AnswerId) - .ToList(); - - answerQueryable = answerQueryable - .Where(x => answerIds - .Contains(x.AnswerId)) - .Where(x => x.QuestionId == dashboardItem.FirstQuestionId); - } - else - { - answerQueryable = answerQueryable - .Where(x => x.QuestionId == dashboardItem.FirstQuestionId); - } + var answerQueryable = AnswerFilterHelper.BuildFilteredAnswerValues( + sdkContext, dashboardItem, dashboardSurveyId, answerDates); // Question type == Text if (dashboardItemModel.FirstQuestionType == Constants.QuestionTypes.Text) { - if (dashboardLocationId != null) - { - answerQueryable = answerQueryable - .Where(x => x.Answer.SiteId == dashboardLocationId); - } - else if (dashboardLocationTagId != null) - { - answerQueryable = answerQueryable - .Where(x => x.Answer.Site.SiteTags.Any( - y => y.TagId == dashboardLocationTagId)); - } + answerQueryable = AnswerFilterHelper.ApplyLocationFilter( + answerQueryable, dashboardLocationId, dashboardLocationTagId); var textData = await answerQueryable .Select(x => new DashboardItemTextQuestionDataModel @@ -222,31 +159,16 @@ public static async Task CalculateDashboardItem( // Question type != Text if (!dashboardItem.CompareEnabled) { - if (dashboardLocationId != null) - { - answerQueryable = answerQueryable - .Where(x => x.Answer.SiteId == dashboardLocationId); - } - else if (dashboardLocationTagId != null) - { - answerQueryable = answerQueryable - .Where(x => x.Answer.Site.SiteTags.Any( - y => y.TagId == dashboardLocationTagId)); - } + answerQueryable = AnswerFilterHelper.ApplyLocationFilter( + answerQueryable, dashboardLocationId, dashboardLocationTagId); } var ignoreOptions = new List