Fix timeline events filtering in the query builder (#44355)

* Add repro test * Fix timeline events filtering * Don't include first date of next interval
metabase · Jun 18, 2024 · 1b78f5a · 1b78f5a
1 parent bdbaee5
commit 1b78f5a
Show file tree

Hide file tree

Showing 2 changed files with 132 additions and 8 deletions.
diff --git a/e2e/test/scenarios/organization/timelines-question.cy.spec.js b/e2e/test/scenarios/organization/timelines-question.cy.spec.js
@@ -7,6 +7,7 @@ import {
   rightSidebar,
   visitQuestionAdhoc,
   echartsIcon,
+  popover,
 } from "e2e/support/helpers";
 
 const { ORDERS, ORDERS_ID } = SAMPLE_DATABASE;
@@ -438,6 +439,74 @@ describe("scenarios > organization > timelines > question", () => {
         "4px solid rgba(0, 0, 0, 0)",
       );
     });
+
+    it("should not filter out events in last period (metabase#23336)", () => {
+      cy.createTimelineWithEvents({
+        events: [
+          { name: "Last week", timestamp: "2026-04-21T12:00:00Z" },
+          { name: "Last month", timestamp: "2026-04-27T12:00:00Z" },
+          { name: "Last quarter", timestamp: "2026-05-10T12:00:00Z" },
+          { name: "Last year", timestamp: "2026-09-10T12:00:00Z" },
+        ],
+      });
+
+      visitQuestionAdhoc({
+        dataset_query: {
+          type: "query",
+          database: SAMPLE_DB_ID,
+          query: {
+            aggregation: [["count"]],
+            breakout: [
+              ["field", ORDERS.CREATED_AT, { "temporal-unit": "week" }],
+            ],
+            "source-table": ORDERS_ID,
+          },
+        },
+      });
+
+      cy.icon("calendar").click();
+
+      // Week
+      rightSidebar().within(() => {
+        cy.findByText("Last week").should("exist");
+        cy.findByText("Last month").should("not.exist");
+        cy.findByText("Last quarter").should("not.exist");
+        cy.findByText("Last year").should("not.exist");
+      });
+
+      // Month
+      cy.findByTestId("timeseries-chrome").findByText("Week").click();
+      popover().findByText("Month").click();
+      cy.wait("@dataset");
+      rightSidebar().within(() => {
+        cy.findByText("Last week").should("exist");
+        cy.findByText("Last month").should("exist");
+        cy.findByText("Last quarter").should("not.exist");
+        cy.findByText("Last year").should("not.exist");
+      });
+
+      // Quarter
+      cy.findByTestId("timeseries-chrome").findByText("Month").click();
+      popover().findByText("Quarter").click();
+      cy.wait("@dataset");
+      rightSidebar().within(() => {
+        cy.findByText("Last week").should("exist");
+        cy.findByText("Last month").should("exist");
+        cy.findByText("Last quarter").should("exist");
+        cy.findByText("Last year").should("not.exist");
+      });
+
+      // Year
+      cy.findByTestId("timeseries-chrome").findByText("Quarter").click();
+      popover().findByText("Year").click();
+      cy.wait("@dataset");
+      rightSidebar().within(() => {
+        cy.findByText("Last week").should("exist");
+        cy.findByText("Last month").should("exist");
+        cy.findByText("Last quarter").should("exist");
+        cy.findByText("Last year").should("exist");
+      });
+    });
   });
 
   describe("as readonly user", () => {

diff --git a/frontend/src/metabase/query_builder/selectors.js b/frontend/src/metabase/query_builder/selectors.js
@@ -28,11 +28,15 @@ import { getEmbedOptions, getIsEmbedded } from "metabase/selectors/embed";
 import { getMetadata } from "metabase/selectors/metadata";
 import { getSetting } from "metabase/selectors/settings";
 import { getMode as getQuestionMode } from "metabase/visualizations/click-actions/lib/modes";
+import {
+  computeTimeseriesDataInverval,
+  minTimeseriesUnit,
+} from "metabase/visualizations/echarts/cartesian/utils/timeseries";
 import {
   getXValues,
   isTimeseries,
 } from "metabase/visualizations/lib/renderer_utils";
-
+import { isAbsoluteDateTimeUnit } from "metabase-types/guards/date-time";
 import { isAdHocModelQuestion } from "metabase-lib/v1/metadata/utils/models";
 import { getCardUiParameters } from "metabase-lib/v1/parameters/utils/cards";
 import {
@@ -42,6 +46,7 @@ import {
 import Question from "metabase-lib/v1/Question";
 import { getIsPKFromTablePredicate } from "metabase-lib/v1/types/utils/isa";
 import { LOAD_COMPLETE_FAVICON } from "metabase/hoc/Favicon";
+import { isNotNull } from "metabase/lib/types";
 import { getQuestionWithDefaultVisualizationSettings } from "./actions/core/utils";
 
 export const getUiControls = state => state.qb.uiControls;
@@ -758,10 +763,6 @@ const getNativeEditorSelectedRange = createSelector(
   uiControls => uiControls && uiControls.nativeEditorSelectedRange,
 );
 
-function isEventWithinDomain(event, xDomain) {
-  return event.timestamp.isBetween(xDomain[0], xDomain[1], undefined, "[]");
-}
-
 export const getIsTimeseries = createSelector(
   [getVisualizationSettings],
   settings => settings && isTimeseries(settings),
@@ -773,6 +774,34 @@ export const getTimeseriesXValues = createSelector(
     isTimeseries && series && settings && getXValues({ series, settings }),
 );
 
+const getTimeseriesDataInterval = createSelector(
+  [
+    getTransformedSeries,
+    getVisualizationSettings,
+    getIsTimeseries,
+    getTimeseriesXValues,
+  ],
+  (series, settings, isTimeseries, xValues) => {
+    if (!isTimeseries || !xValues) {
+      return null;
+    }
+    const columns = series[0]?.data?.cols ?? [];
+    const dimensions = settings?.["graph.dimensions"] ?? [];
+    const dimensionColumns = dimensions.map(dimension =>
+      columns.find(column => column.name === dimension),
+    );
+    const columnUnits = dimensionColumns
+      .map(column =>
+        isAbsoluteDateTimeUnit(column?.unit) ? column.unit : null,
+      )
+      .filter(isNotNull);
+    return computeTimeseriesDataInverval(
+      xValues,
+      minTimeseriesUnit(columnUnits),
+    );
+  },
+);
+
 export const getTimeseriesXDomain = createSelector(
   [getIsTimeseries, getTimeseriesXValues],
   (isTimeseries, xValues) => xValues && isTimeseries && d3.extent(xValues),
@@ -799,14 +828,40 @@ export const getTransformedTimelines = createSelector(
   },
 );
 
+function isEventWithinDomain(event, xDomain) {
+  return event.timestamp.isBetween(xDomain[0], xDomain[1], undefined, "[]");
+}
+
+function getXDomainForTimelines(xDomain, dataInterval) {
+  // When looking at, let's say, count of orders over years, last year value is Jan 1, 2024
+  // If we filter timeline events up until Jan 1, 2024, we won't see any events from 2024,
+  // so we need to extend xDomain by dataInterval.count * dataInterval.unit to include them
+  if (xDomain && isAbsoluteDateTimeUnit(dataInterval?.unit)) {
+    let maxValue = xDomain[1]
+      .clone()
+      .add(dataInterval.count, dataInterval.unit);
+
+    if (dataInterval.unit !== "hour" && dataInterval.unit !== "minute") {
+      maxValue = maxValue.subtract(1, "day");
+    }
+
+    return [xDomain[0], maxValue];
+  }
+
+  return xDomain;
+}
+
 export const getFilteredTimelines = createSelector(
-  [getTransformedTimelines, getTimeseriesXDomain],
-  (timelines, xDomain) => {
+  [getTransformedTimelines, getTimeseriesXDomain, getTimeseriesDataInterval],
+  (timelines, xDomain, dataInterval) => {
+    const timelineXDomain = getXDomainForTimelines(xDomain, dataInterval);
     return timelines
       .map(timeline =>
         updateIn(timeline, ["events"], events =>
           xDomain
-            ? events.filter(event => isEventWithinDomain(event, xDomain))
+            ? events.filter(event =>
+                isEventWithinDomain(event, timelineXDomain),
+              )
             : events,
         ),
       )