fix: Disallow sort by hidden measure in leaderboard (#6425)

* Disallow sort by hidden measure in leaderboard * Validate for url params * Fix tests * Rename to getSimpleMeasures
rilldata · Jan 23, 2025 · a9b38cd · a9b38cd · github-actions · Jan 23, 2025
1 parent bf28d85
commit a9b38cd
Show file tree

Hide file tree

Showing 8 changed files with 53 additions and 13 deletions.
diff --git a/web-common/src/features/dashboards/leaderboard/LeaderboardControls.svelte b/web-common/src/features/dashboards/leaderboard/LeaderboardControls.svelte
@@ -3,18 +3,15 @@
   import DashboardVisibilityDropdown from "@rilldata/web-common/components/menu/shadcn/DashboardVisibilityDropdown.svelte";
   import * as Select from "@rilldata/web-common/components/select";
   import { LeaderboardContextColumn } from "@rilldata/web-common/features/dashboards/leaderboard-context-column";
+  import { getSimpleMeasures } from "@rilldata/web-common/features/dashboards/state-managers/selectors/measures";
   import { metricsExplorerStore } from "web-common/src/features/dashboards/stores/dashboard-stores";
   import { getStateManagers } from "../state-managers/state-managers";
 
   export let exploreName: string;
 
   const {
     selectors: {
-      measures: {
-        filteredSimpleMeasures,
-        leaderboardMeasureName,
-        getMeasureByName,
-      },
+      measures: { leaderboardMeasureName, getMeasureByName, visibleMeasures },
       dimensions: { visibleDimensions, allDimensions },
     },
     actions: {
@@ -26,7 +23,7 @@
 
   let active = false;
 
-  $: measures = $filteredSimpleMeasures();
+  $: measures = getSimpleMeasures($visibleMeasures);
 
   $: metricsExplorer = $metricsExplorerStore.entities[exploreName];
 

diff --git a/web-common/src/features/dashboards/state-managers/actions/measures.ts b/web-common/src/features/dashboards/state-managers/actions/measures.ts
@@ -9,6 +9,13 @@ export const toggleMeasureVisibility = (
     const deleted = dashboard.visibleMeasureKeys.delete(measureName);
     if (!deleted) {
       dashboard.visibleMeasureKeys.add(measureName);
+    } else if (
+      dashboard.leaderboardMeasureName === measureName &&
+      dashboard.visibleMeasureKeys.size > 0
+    ) {
+      dashboard.leaderboardMeasureName = dashboard.visibleMeasureKeys
+        .keys()
+        .next().value;
     }
   } else {
     const allSelected =

diff --git a/web-common/src/features/dashboards/state-managers/selectors/measures.ts b/web-common/src/features/dashboards/state-managers/selectors/measures.ts
@@ -175,6 +175,16 @@ export const getIndependentMeasures = (
   return [...measures];
 };
 
+export const getSimpleMeasures = (measures: MetricsViewSpecMeasureV2[]) => {
+  return (
+    measures?.filter(
+      (m) =>
+        !m.window &&
+        m.type !== MetricsViewSpecMeasureType.MEASURE_TYPE_TIME_COMPARISON,
+    ) ?? []
+  );
+};
+
 export const measureSelectors = {
   /**
    * Get all measures in the dashboard.

diff --git a/web-common/src/features/dashboards/stores/test-data/data.ts b/web-common/src/features/dashboards/stores/test-data/data.ts
@@ -241,7 +241,7 @@ export const AD_BIDS_PRESET: V1ExplorePreset = {
   compareTimeRange: "rill-PP",
   measures: [AD_BIDS_IMPRESSIONS_MEASURE],
   dimensions: [AD_BIDS_PUBLISHER_DIMENSION],
-  exploreSortBy: AD_BIDS_BID_PRICE_MEASURE,
+  exploreSortBy: AD_BIDS_IMPRESSIONS_MEASURE,
   exploreSortAsc: true,
   exploreSortType: V1ExploreSortType.EXPLORE_SORT_TYPE_PERCENT,
 };

diff --git a/web-common/src/features/dashboards/stores/test-data/store-mutations.ts b/web-common/src/features/dashboards/stores/test-data/store-mutations.ts
@@ -147,11 +147,20 @@ export const AD_BIDS_SORT_DESC_BY_IMPRESSIONS: TestDashboardMutation = (
   setLeaderboardMeasureName(mut, AD_BIDS_IMPRESSIONS_MEASURE);
   setSortDescending(mut);
 };
+export const AD_BIDS_SORT_ASC_BY_IMPRESSIONS: TestDashboardMutation = (mut) => {
+  setLeaderboardMeasureName(mut, AD_BIDS_IMPRESSIONS_MEASURE);
+  setSortDescending(mut);
+  toggleSort(mut, mut.dashboard.dashboardSortType);
+};
 export const AD_BIDS_SORT_ASC_BY_BID_PRICE: TestDashboardMutation = (mut) => {
   setLeaderboardMeasureName(mut, AD_BIDS_BID_PRICE_MEASURE);
   setSortDescending(mut);
   toggleSort(mut, mut.dashboard.dashboardSortType);
 };
+export const AD_BIDS_SORT_DESC_BY_BID_PRICE: TestDashboardMutation = (mut) => {
+  setLeaderboardMeasureName(mut, AD_BIDS_BID_PRICE_MEASURE);
+  setSortDescending(mut);
+};
 export const AD_BIDS_SORT_BY_VALUE: TestDashboardMutation = (mut) => {
   toggleSort(mut, DashboardState_LeaderboardSortType.VALUE);
 };

diff --git a/web-common/src/features/dashboards/url-state/convertURLToExplorePreset.ts b/web-common/src/features/dashboards/url-state/convertURLToExplorePreset.ts
@@ -390,7 +390,16 @@ function fromExploreUrlParams(
   if (searchParams.has(ExploreStateURLParams.SortBy)) {
     const sortBy = searchParams.get(ExploreStateURLParams.SortBy) as string;
     if (measures.has(sortBy)) {
-      preset.exploreSortBy = sortBy;
+      if (
+        (preset.measures && preset.measures.includes(sortBy)) ||
+        !preset.measures
+      ) {
+        preset.exploreSortBy = sortBy;
+      } else {
+        errors.push(
+          getSingleFieldError("sort by measure", sortBy, "It is hidden."),
+        );
+      }
     } else {
       errors.push(getSingleFieldError("sort by measure", sortBy));
     }

diff --git a/web-common/src/features/dashboards/url-state/error-message-helpers.ts b/web-common/src/features/dashboards/url-state/error-message-helpers.ts
@@ -1,5 +1,11 @@
-export function getSingleFieldError(fieldLabel: string, field: string) {
-  return new Error(`Selected ${fieldLabel}: "${field}" is not valid.`);
+export function getSingleFieldError(
+  fieldLabel: string,
+  field: string,
+  suffix?: string,
+) {
+  return new Error(
+    `Selected ${fieldLabel}: "${field}" is not valid.${suffix ? " " + suffix : ""}`,
+  );
 }
 
 export function getMultiFieldError(fieldLabel: string, fields: string[]) {

diff --git a/web-common/src/features/dashboards/url-state/url-state-variations.spec.ts b/web-common/src/features/dashboards/url-state/url-state-variations.spec.ts
@@ -29,9 +29,11 @@ import {
   AD_BIDS_SET_PREVIOUS_PERIOD_COMPARE_TIME_RANGE_FILTER,
   AD_BIDS_SET_PREVIOUS_WEEK_COMPARE_TIME_RANGE_FILTER,
   AD_BIDS_SORT_ASC_BY_BID_PRICE,
+  AD_BIDS_SORT_ASC_BY_IMPRESSIONS,
   AD_BIDS_SORT_BY_DELTA_ABS_VALUE,
   AD_BIDS_SORT_BY_PERCENT_VALUE,
   AD_BIDS_SORT_BY_VALUE,
+  AD_BIDS_SORT_DESC_BY_BID_PRICE,
   AD_BIDS_SORT_DESC_BY_IMPRESSIONS,
   AD_BIDS_SORT_PIVOT_BY_IMPRESSIONS_DESC,
   AD_BIDS_SORT_PIVOT_BY_TIME_DAY_ASC,
@@ -198,7 +200,7 @@ const TestCases: {
   {
     title:
       "Leaderboard configs with preset and leaderboard sort measure in state same as preset",
-    mutations: [AD_BIDS_SORT_BY_PERCENT_VALUE, AD_BIDS_SORT_ASC_BY_BID_PRICE],
+    mutations: [AD_BIDS_SORT_BY_PERCENT_VALUE, AD_BIDS_SORT_ASC_BY_IMPRESSIONS],
     preset: AD_BIDS_PRESET,
     expectedUrl: "http://localhost/",
   },
@@ -207,11 +209,11 @@ const TestCases: {
       "Leaderboard configs with preset and leaderboard sort measure in state different than preset",
     mutations: [
       AD_BIDS_SORT_BY_DELTA_ABS_VALUE,
-      AD_BIDS_SORT_DESC_BY_IMPRESSIONS,
+      AD_BIDS_SORT_DESC_BY_BID_PRICE,
     ],
     preset: AD_BIDS_PRESET,
     expectedUrl:
-      "http://localhost/?sort_by=impressions&sort_type=delta_abs&sort_dir=DESC",
+      "http://localhost/?sort_by=bid_price&sort_type=delta_abs&sort_dir=DESC",
   },
 
   {