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..75110162 --- /dev/null +++ b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn.Test/AnswerFilterHelperUTests.cs @@ -0,0 +1,273 @@ +/* +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.Collections.Generic; +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() }) + .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 List(), + CompareLocationsTags = new 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(expected, Is.Not.Empty, + "The chosen question pair matched no answers, so comparing the two " + + "implementations would prove nothing. Pick a different pair if the seed " + + "data changes."); + + 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."); + } + + /// + /// The case where Contains-over-a-list and Any-over-a-subquery could most + /// plausibly diverge: the filter question IS the measured question, so the + /// membership test and the question predicate apply to the same rows. + /// + [Test] + public async Task FilterQuestion_SameAsMeasuredQuestion_MatchesMaterialisedIds() + { + var pick = await DbContext.AnswerValues + .AsNoTracking() + .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) + .GroupBy(x => new { x.Answer.QuestionSetId, x.QuestionId, x.OptionId }) + .Select(g => new { g.Key.QuestionSetId, g.Key.QuestionId, g.Key.OptionId, Count = g.Count() }) + .OrderByDescending(x => x.Count) + .FirstOrDefaultAsync(); + + Assert.That(pick, Is.Not.Null, "Seed data has no answer values."); + + var dashboardItem = new DashboardItem + { + FirstQuestionId = pick.QuestionId, + FilterQuestionId = pick.QuestionId, + FilterAnswerId = pick.OptionId, + IgnoredAnswerValues = new List(), + CompareLocationsTags = new List(), + }; + + var actual = await AnswerFilterHelper + .BuildFilteredAnswerValues( + DbContext, dashboardItem, pick.QuestionSetId, new DashboardEditAnswerDates()) + .Select(x => x.AnswerId) + .Distinct() + .ToListAsync(); + + var scope = DbContext.AnswerValues + .AsNoTracking() + .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) + .Where(x => x.Answer.QuestionSetId == pick.QuestionSetId); + + var materialisedIds = await scope + .Where(y => y.QuestionId == pick.QuestionId && y.OptionId == pick.OptionId) + .Select(y => y.AnswerId) + .ToListAsync(); + + var expected = await scope + .Where(x => materialisedIds.Contains(x.AnswerId)) + .Where(x => x.QuestionId == pick.QuestionId) + .Select(x => x.AnswerId) + .Distinct() + .ToListAsync(); + + Assert.That(expected, Is.Not.Empty, "Chosen option matched no answers."); + Assert.That(actual.OrderBy(x => x), Is.EqualTo(expected.OrderBy(x => x))); + } + + /// + /// A multi-select answer carries several values for one question, so the + /// materialised id list contained duplicates where the subquery does not. + /// Both are membership tests, and this pins that they agree. + /// + [Test] + public async Task FilterQuestion_AnswerWithRepeatedQuestion_MatchesMaterialisedIds() + { + var repeated = await DbContext.AnswerValues + .AsNoTracking() + .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) + .GroupBy(x => new { x.AnswerId, x.QuestionId }) + .Where(g => g.Count() > 1) + .Select(g => new { g.Key.AnswerId, g.Key.QuestionId }) + .FirstOrDefaultAsync(); + + if (repeated == null) + { + Assert.Ignore("Seed data has no answer carrying multiple values for one question."); + } + + var context = await DbContext.AnswerValues + .AsNoTracking() + .Where(x => x.AnswerId == repeated.AnswerId && x.QuestionId == repeated.QuestionId) + .Select(x => new { x.OptionId, x.Answer.QuestionSetId }) + .FirstAsync(); + + var dashboardItem = new DashboardItem + { + FirstQuestionId = repeated.QuestionId, + FilterQuestionId = repeated.QuestionId, + FilterAnswerId = context.OptionId, + IgnoredAnswerValues = new List(), + CompareLocationsTags = new List(), + }; + + var actual = await AnswerFilterHelper + .BuildFilteredAnswerValues( + DbContext, dashboardItem, context.QuestionSetId, new DashboardEditAnswerDates()) + .Select(x => x.AnswerId) + .Distinct() + .ToListAsync(); + + Assert.That(actual, Does.Contain(repeated.AnswerId), + "An answer with repeated values for the filter question must still be selected."); + } + + /// + /// 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 tagId = await DbContext.Tags + .AsNoTracking() + .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) + .Select(x => x.Id) + .FirstOrDefaultAsync(); + + 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.GreaterThan(0), + "Filtering by a site that has answers must not empty the query."); + + if (tagId > 0) + { + // The precedence this test is named for: a site beats a tag, and the + // two are never combined. + var both = await AnswerFilterHelper + .ApplyLocationFilter(all, siteId, tagId) + .CountAsync(); + + Assert.That(both, Is.EqualTo(bySite), + "A location must win over a tag rather than intersecting with it."); + } + } +} 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..bebc67ed 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,76 @@ 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 + // No Includes here. The filter-question step below closes over this + // queryable to build a correlated subquery, and a subquery carrying + // Includes is a translation hazard; BuildAnswerQuery would inherit the + // same problem. The chart callers attach their own Includes to the + // returned query, exactly as they did before this was extracted. + 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 +114,81 @@ 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) + return answerQueryable; + } + + /// + /// 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,7 +199,9 @@ public static IQueryable BuildAnswerQuery( .Where(x => answerIds.Contains(x.Id)); } - // ChartDataHelpers.cs:121-133 + /// + /// Mirrors the isComparedData decision ChartDataHelpers makes. + /// private static bool IsComparedData(DashboardItem dashboardItem) { if (dashboardItem.ChartType != DashboardChartTypes.GroupedStackedBarChart @@ -159,7 +218,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 +255,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..b1ecb064 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,19 @@ public static async Task CalculateDashboardItem( } } - var answerQueryable = sdkContext.AnswerValues - .AsNoTracking() + var answerQueryable = AnswerFilterHelper.BuildFilteredAnswerValues( + sdkContext, dashboardItem, dashboardSurveyId, answerDates) .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); - } - // 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 +164,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