diff --git a/web-common/src/features/dashboards/filters/AdvancedFilter.svelte b/web-common/src/features/dashboards/filters/AdvancedFilter.svelte index ab62d0807cf..71a7563f037 100644 --- a/web-common/src/features/dashboards/filters/AdvancedFilter.svelte +++ b/web-common/src/features/dashboards/filters/AdvancedFilter.svelte @@ -1,17 +1,41 @@
+ {#if onRemove} + + + {/if} {m.filter_advanced_beta()} diff --git a/web-common/src/features/dashboards/filters/CanvasExpressionFilters.spec.ts b/web-common/src/features/dashboards/filters/CanvasExpressionFilters.spec.ts index 43fc4b6af0c..cf9b6052606 100644 --- a/web-common/src/features/dashboards/filters/CanvasExpressionFilters.spec.ts +++ b/web-common/src/features/dashboards/filters/CanvasExpressionFilters.spec.ts @@ -1,5 +1,6 @@ import { useCanvasFiltersVariant } from "@rilldata/web-common/features/dashboards/filters/test/canvas-filters-variant"; import { + testAdvancedFilters, testDimensionFilters, testMeasureFilters, } from "@rilldata/web-common/features/dashboards/filters/test/expression-filters-suite"; @@ -71,6 +72,7 @@ describe("CanvasExpressionFilters", () => { testDimensionFilters(variant); testMeasureFilters(variant); + testAdvancedFilters(variant); // We need to fix canvas navigation before enabling this. // Check CanvasInitialization.svelte for more details. // testURLNavigationFlows(variant); diff --git a/web-common/src/features/dashboards/filters/ExploreExpressionFilters.spec.ts b/web-common/src/features/dashboards/filters/ExploreExpressionFilters.spec.ts index 5c4d0eb894b..3b91ef010eb 100644 --- a/web-common/src/features/dashboards/filters/ExploreExpressionFilters.spec.ts +++ b/web-common/src/features/dashboards/filters/ExploreExpressionFilters.spec.ts @@ -1,5 +1,6 @@ import { useExploreFiltersVariant } from "@rilldata/web-common/features/dashboards/filters/test/explore-filters-variant"; import { + testAdvancedFilters, testDimensionFilters, testMeasureFilters, testUrlLengthLimit, @@ -75,4 +76,5 @@ describe("ExploreExpressionFilters", () => { testUrlLengthLimit(variant); testMeasureFilters(variant); testURLNavigationFlows(variant); + testAdvancedFilters(variant); }); diff --git a/web-common/src/features/dashboards/filters/ExpressionFilterManager.spec.ts b/web-common/src/features/dashboards/filters/ExpressionFilterManager.spec.ts index 86107a23281..dc01b02137f 100644 --- a/web-common/src/features/dashboards/filters/ExpressionFilterManager.spec.ts +++ b/web-common/src/features/dashboards/filters/ExpressionFilterManager.spec.ts @@ -1382,6 +1382,26 @@ describe("clear", () => { expect(filterManager.sortedFilterManagers.measures).toEqual([]); }); + it("resets the complex flag", () => { + const filterManager = createFilterManager(); + + // A top level OR has no chip, so the bar shows the read only advanced filter. + filterManager.storeSync.setUrlParams( + perMetricsViewParams({ + [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google') OR ${AD_BIDS_DOMAIN_DIMENSION} IN ('google.com')`, + }), + ); + expect(filterManager.isComplexFilter).toBe(true); + + filterManager.clear(); + + // The navigation after a clear does not parse the params again, so `clear` resets the flag itself. + expect(filterManager.isComplexFilter).toBe(false); + expect(filterManager.topLevelJoiner.expr).toEqual({}); + expect(filterManager.sortedFilterManagers.dimensions).toEqual([]); + expect(filterManager.sortedFilterManagers.measures).toEqual([]); + }); + it("keeps the required and pinned chips", () => { const yamlConfigProvider = new YAMLConfigProvider(); yamlConfigProvider.requiredFilters = { [AD_BIDS_DOMAIN_DIMENSION]: true }; diff --git a/web-common/src/features/dashboards/filters/ExpressionFilterManager.svelte.ts b/web-common/src/features/dashboards/filters/ExpressionFilterManager.svelte.ts index fd0a4682cf8..bc06ee9adac 100644 --- a/web-common/src/features/dashboards/filters/ExpressionFilterManager.svelte.ts +++ b/web-common/src/features/dashboards/filters/ExpressionFilterManager.svelte.ts @@ -316,6 +316,9 @@ export class ExpressionFilterManager implements UrlParamsStore { public clear() { this.temporaryFilterName = undefined; + // `setUrlParams` is the only other writer of this flag, + // and the navigation after a clear skips it because the params already match the cleared state. + this.isComplexFilter = false; this.topLevelJoiner.clear(); // Clearing goes through the param rather than the managers, so it has to report itself. this.events.emit("filter-changed", { source: undefined }); diff --git a/web-common/src/features/dashboards/filters/ExpressionFilters.svelte b/web-common/src/features/dashboards/filters/ExpressionFilters.svelte index fb9041af026..c3ce6c10ef4 100644 --- a/web-common/src/features/dashboards/filters/ExpressionFilters.svelte +++ b/web-common/src/features/dashboards/filters/ExpressionFilters.svelte @@ -89,7 +89,10 @@
{#if expressionFilterManager.isComplexFilter} {#each Object.entries(expressionFilterManager.exprByMetricsView) as [mv, expr] (mv)} - + expressionFilterManager.clear()} + /> {/each} {:else} {#if !hasFilters} @@ -135,14 +138,16 @@ {excludedDimensions} {excludedMeasures} /> + {/if} - - {#if hasClearableFilters} - - {/if} + + {#if expressionFilterManager.isComplexFilter || hasClearableFilters} + {/if}
diff --git a/web-common/src/features/dashboards/filters/StandaloneExpressionFilters.spec.ts b/web-common/src/features/dashboards/filters/StandaloneExpressionFilters.spec.ts index a43deb4212b..c26c4cde4c0 100644 --- a/web-common/src/features/dashboards/filters/StandaloneExpressionFilters.spec.ts +++ b/web-common/src/features/dashboards/filters/StandaloneExpressionFilters.spec.ts @@ -1,4 +1,5 @@ import { + testAdvancedFilters, testDimensionFilters, testMeasureFilters, } from "@rilldata/web-common/features/dashboards/filters/test/expression-filters-suite"; @@ -69,4 +70,5 @@ describe("StandaloneExpressionFilters", () => { testDimensionFilters(variant); testMeasureFilters(variant); + testAdvancedFilters(variant); }); diff --git a/web-common/src/features/dashboards/filters/VerticalExpressionFilters.svelte b/web-common/src/features/dashboards/filters/VerticalExpressionFilters.svelte index a707df4da9c..19dc1968c12 100644 --- a/web-common/src/features/dashboards/filters/VerticalExpressionFilters.svelte +++ b/web-common/src/features/dashboards/filters/VerticalExpressionFilters.svelte @@ -54,7 +54,10 @@
{#if expressionFilterManager.isComplexFilter} {#each Object.entries(expressionFilterManager.exprByMetricsView) as [mv, expr] (mv)} - + expressionFilterManager.clear()} + /> {/each} {:else} {#each sortedDimensionManagers as dimensionManager (dimensionManager.name)} @@ -83,7 +86,7 @@
- {#if hasFilters} + {#if hasFilters || expressionFilterManager.isComplexFilter} diff --git a/web-common/src/features/dashboards/filters/test/CanvasExpressionFiltersTest.svelte b/web-common/src/features/dashboards/filters/test/CanvasExpressionFiltersTest.svelte index 78c5dffdc29..91bc82cad5f 100644 --- a/web-common/src/features/dashboards/filters/test/CanvasExpressionFiltersTest.svelte +++ b/web-common/src/features/dashboards/filters/test/CanvasExpressionFiltersTest.svelte @@ -1,4 +1,5 @@ - - - -
Dashboard loaded!
-
-
+ + + + + +
Dashboard loaded!
+
+
+
diff --git a/web-common/src/features/dashboards/filters/test/ExploreExpressionFiltersTest.svelte b/web-common/src/features/dashboards/filters/test/ExploreExpressionFiltersTest.svelte index 508c8ebb1ca..c3f95156ba0 100644 --- a/web-common/src/features/dashboards/filters/test/ExploreExpressionFiltersTest.svelte +++ b/web-common/src/features/dashboards/filters/test/ExploreExpressionFiltersTest.svelte @@ -1,4 +1,5 @@ - - - -
Dashboard loaded!
-
-
+ + + + + +
Dashboard loaded!
+
+
+
diff --git a/web-common/src/features/dashboards/filters/test/StandaloneExpressionFiltersTest.svelte b/web-common/src/features/dashboards/filters/test/StandaloneExpressionFiltersTest.svelte index 30d7ef31fef..ad736f31429 100644 --- a/web-common/src/features/dashboards/filters/test/StandaloneExpressionFiltersTest.svelte +++ b/web-common/src/features/dashboards/filters/test/StandaloneExpressionFiltersTest.svelte @@ -1,4 +1,5 @@ - -{#if metricsViewsProvider.ready} -
Dashboard loaded!
-{/if} + + + + {#if metricsViewsProvider.ready} +
Dashboard loaded!
+ {/if} +
diff --git a/web-common/src/features/dashboards/filters/test/expression-filters-suite.ts b/web-common/src/features/dashboards/filters/test/expression-filters-suite.ts index 35990d8e07c..6c75c2ded11 100644 --- a/web-common/src/features/dashboards/filters/test/expression-filters-suite.ts +++ b/web-common/src/features/dashboards/filters/test/expression-filters-suite.ts @@ -13,6 +13,7 @@ import { getSelectAllButton, isMeasureFilterFormOpen, pressEnter, + removeAdvancedFilter, removeDimensionFilter, removeMeasureFilter, selectDimensionFilterMode, @@ -22,6 +23,7 @@ import { toggleMeasureFilter, toggleSelectAll, typeInDimensionFilterSearch, + waitForAdvancedFilter, waitForDimensionFilterResultCount, waitForDimensionFilterResults, waitForEmptyFilters, @@ -34,8 +36,10 @@ import { createInExpression, createLikeExpression, createSubQueryExpression, + getAllIdentifiers, } from "@rilldata/web-common/features/dashboards/stores/filter-utils"; import { + AD_BIDS_DOMAIN_DIMENSION, AD_BIDS_IMPRESSIONS_MEASURE, AD_BIDS_PUBLISHER_DIMENSION, } from "@rilldata/web-common/features/dashboards/stores/test-data/data"; @@ -1143,6 +1147,58 @@ export function testURLNavigationFlows(variant: ExpressionFiltersVariant) { }); } +/** + * A filter the chips cannot show falls back to the read only advanced pill. + * The pill cannot be edited, so clearing is the only way out of it. + */ +export function testAdvancedFilters(variant: ExpressionFiltersVariant) { + const { + initialUrlSearch, + urlSearchWithFilter, + assertWhereFilter, + assertUrlSearch, + } = variantAssertions(variant); + + describe("Advanced filters", () => { + // A top level OR has no chip of its own. + const orFilter = `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Facebook') OR ${AD_BIDS_DOMAIN_DIMENSION} IN ('google.com')`; + + it("Should clear an advanced filter with the clear button", async () => { + await variant.render(urlSearchWithFilter(orFilter)); + await waitForAdvancedFilter(orFilter); + + // The filter reaches the consumer even though no chip can show it. + // Only the identifiers are checked, since the explore state holds the OR as is while a + // standalone bar wraps it in an AND. + expect( + getAllIdentifiers(variant.expressionFilterManager.getWhereFilter()), + ).toEqual([AD_BIDS_PUBLISHER_DIMENSION, AD_BIDS_DOMAIN_DIMENSION]); + // Nothing can be added next to the pill. + expect( + screen.queryByLabelText("Add filter button"), + ).not.toBeInTheDocument(); + + await clearFilters(); + + expect(screen.queryByText("Advanced (BETA)")).not.toBeInTheDocument(); + expect(screen.getByLabelText("Add filter button")).toBeVisible(); + assertWhereFilter(createAndExpression([])); + assertUrlSearch(initialUrlSearch); + }); + + it("Should clear an advanced filter from its pill", async () => { + await variant.render(urlSearchWithFilter(orFilter)); + await waitForAdvancedFilter(orFilter); + + await removeAdvancedFilter(); + + await waitForEmptyFilters(); + assertWhereFilter(createAndExpression([])); + assertUrlSearch(initialUrlSearch); + }); + }); +} + /** The expression a measure filter on `impressions`, split by `publisher`, produces. */ export function measureFilterExpression(operation: V1Operation, value: number) { return createSubQueryExpression( diff --git a/web-common/src/features/dashboards/filters/test/filter-test-utils.ts b/web-common/src/features/dashboards/filters/test/filter-test-utils.ts index 6dea2a28298..00e4eacb8ec 100644 --- a/web-common/src/features/dashboards/filters/test/filter-test-utils.ts +++ b/web-common/src/features/dashboards/filters/test/filter-test-utils.ts @@ -311,6 +311,22 @@ export async function clearFilters() { await waitForEmptyFilters(); } +/** Waits for the advanced filter pill, which shows the filter param no chip can represent. */ +export async function waitForAdvancedFilter(filter: string) { + await waitFor(() => + expect(screen.getByText("Advanced (BETA)")).toBeVisible(), + ); + // The pill writes a nested expression with its parentheses, so match on the text within them. + expect(screen.getByText("Advanced (BETA)").parentElement).toHaveTextContent( + filter, + ); +} + +/** Removes the advanced filter pill, which clears the whole filter. */ +export async function removeAdvancedFilter() { + await act(() => screen.getByRole("button", { name: "Remove" }).click()); +} + /** * Whether the measure filter form is open. * diff --git a/web-common/src/features/dashboards/filters/test/standalone-filters-variant.ts b/web-common/src/features/dashboards/filters/test/standalone-filters-variant.ts index 971205ef1c4..691d3ae8c1c 100644 --- a/web-common/src/features/dashboards/filters/test/standalone-filters-variant.ts +++ b/web-common/src/features/dashboards/filters/test/standalone-filters-variant.ts @@ -73,10 +73,11 @@ export function useStandaloneFiltersVariant( getInListDimensions: () => expressionFilterManager!.inList, }, - render: async () => { + render: async (initUrlSearch?: string) => { render(StandaloneExpressionFiltersTest, { props: { metricsViewNames: [AD_BIDS_METRICS_NAME], + initUrlSearch, onManagerCreated: (manager: ExpressionFilterManager) => (expressionFilterManager = manager), }, diff --git a/web-common/src/lib/i18n/messages/en.json b/web-common/src/lib/i18n/messages/en.json index 6d6225bb744..d601fcb087a 100644 --- a/web-common/src/lib/i18n/messages/en.json +++ b/web-common/src/lib/i18n/messages/en.json @@ -1292,6 +1292,7 @@ "field_list_measures": "MEASURES", "field_list_time": "TIME", "filter_advanced_beta": "Advanced (BETA)", + "filter_advanced_remove": "Remove", "filter_advanced_warning": "Advanced filters are a bleeding edge feature! There may be bugs.", "filter_converted_to_select": "Converted filter type to Select", "filter_dimension": "dimension", diff --git a/web-common/src/lib/i18n/messages/es.json b/web-common/src/lib/i18n/messages/es.json index d49b726ac84..b3583ba8683 100644 --- a/web-common/src/lib/i18n/messages/es.json +++ b/web-common/src/lib/i18n/messages/es.json @@ -1301,6 +1301,7 @@ "field_list_measures": "MEDIDAS", "field_list_time": "TIEMPO", "filter_advanced_beta": "Avanzado (BETA)", + "filter_advanced_remove": "Eliminar", "filter_advanced_warning": "Los filtros avanzados son una funcionalidad experimental. Puede haber errores.", "filter_converted_to_select": "Tipo de filtro convertido a Seleccionar", "filter_dimension": "dimensión", diff --git a/web-local/tests/explores/advanced-filter.spec.ts b/web-local/tests/explores/advanced-filter.spec.ts new file mode 100644 index 00000000000..ad2d98f6e25 --- /dev/null +++ b/web-local/tests/explores/advanced-filter.spec.ts @@ -0,0 +1,34 @@ +import { expect } from "@playwright/test"; +import { test } from "../setup/base"; +import { waitForReconciliation } from "../utils/wait-for-reconciliation.ts"; + +// A filter the chips cannot show, such as a top level OR, falls back to the read only +// "Advanced (BETA)" pill. Clearing it has to work from the UI, and the cleared state has to be +// what the session restores afterwards, since an empty explore url loads the last state of the tab. +test.describe("advanced filters", () => { + test.use({ project: "AdBids" }); + + test("clears an advanced filter from a shared url and keeps it cleared on reload", async ({ + page, + }) => { + const { origin } = new URL(page.url()); + await waitForReconciliation(page); + + const filter = "publisher IN ('Facebook') OR domain IN ('google.com')"; + await page.goto( + `${origin}/explore/AdBids_metrics_explore?f=${encodeURIComponent(filter)}`, + ); + await expect(page.getByText("Advanced (BETA)")).toBeVisible(); + await expect(page.getByRole("button", { name: "Remove" })).toBeVisible(); + await expect(page.getByLabel("Add filter button")).toBeHidden(); + + await page.getByRole("button", { name: "Clear filters" }).click(); + await expect(page.getByText("No filters selected")).toBeVisible(); + await expect(page.getByLabel("Add filter button")).toBeVisible(); + await page.waitForURL((url) => !url.searchParams.has("f")); + + await page.reload(); + await expect(page.getByText("No filters selected")).toBeVisible(); + expect(new URL(page.url()).searchParams.has("f")).toBe(false); + }); +});