Fix field values re-rendering for virtual tables (#35014)

e2e/test/scenarios/question/reproductions/34414-populate-field-values.cy.spec.js

+import {
+  filter,
+  openNotebook,
+  popover,
+  restore,
+  visitQuestionAdhoc,
+} from "e2e/support/helpers";
+import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
+import { SAMPLE_DB_ID } from "e2e/support/cypress_data";
+
+const { INVOICES_ID } = SAMPLE_DATABASE;
+
+const INVOICE_MODEL_DETAILS = {
+  name: "Invoices Model",
+  query: { "source-table": INVOICES_ID },
+  dataset: true,
+};
+
+describe("issue 34414", () => {
+  beforeEach(() => {
+    restore();
+    cy.signInAsNormalUser();
+  });
+
+  it("populate field values after re-adding filter on virtual table field (metabase#34414)", () => {
+    cy.createQuestion(INVOICE_MODEL_DETAILS).then(response => {
+      const modelId = response.body.id;
+
+      visitQuestionAdhoc({
+        dataset_query: {
+          type: "query",
+          database: SAMPLE_DB_ID,
+          query: { "source-table": `card__${modelId}` },
+        },
+      });
+    });
+
+    openNotebook();
+    filter({ mode: "notebook" });
+
+    popover().within(() => {
+      cy.findByText("Plan").click();
+      assertPlanFieldValues();
+
+      cy.log("Open filter again");
+      cy.findByTestId("sidebar-header-title").click();
+
+      cy.log("Open plan field again");
+      cy.findByText("Plan").click();
+
+      assertPlanFieldValues();
+    });
+  });
+});
+
+function assertPlanFieldValues() {
+  cy.findByText("Basic").should("be.visible");
+  cy.findByText("Business").should("be.visible");
+  cy.findByText("Premium").should("be.visible");
+}

frontend/src/metabase/components/FieldValuesWidget/FieldValuesWidget.tsx

@@ -212,7 +212,7 @@ export function FieldValuesWidgetInner({
 
       const results = await Promise.all(
         nonVirtualFields.map(field =>
-          dispatch(Fields.objectActions.fetchFieldValues({ id: field.id })),
+          dispatch(Fields.objectActions.fetchFieldValues(field)),
         ),
       );
 
@@ -222,7 +222,7 @@ export function FieldValuesWidgetInner({
         (field, index) =>
           results[index]?.payload?.values ??
           Fields.selectors.getFieldValues(results[index]?.payload, {
-            entityId: field.id,
+            entityId: field.getUniqueId(),
           }),
       );
 

frontend/src/metabase/components/FieldValuesWidget/FieldValuesWidget.unit.spec.tsx

@@ -95,15 +95,13 @@ describe("FieldValuesWidget", () => {
     });
 
     describe("has_field_values = list", () => {
-      const field = metadata.field(PRODUCTS.CATEGORY);
+      const field = checkNotNull(metadata.field(PRODUCTS.CATEGORY));
 
       it("should call fetchFieldValues", async () => {
         const { fetchFieldValues } = await setup({
           fields: [field],
         });
-        expect(fetchFieldValues).toHaveBeenCalledWith({
-          id: PRODUCTS.CATEGORY,
-        });
+        expect(fetchFieldValues).toHaveBeenCalledWith(field);
       });
 
       it("should not have 'Search the list' as the placeholder text for fields with less or equal than 10 values", async () => {
@@ -261,12 +259,18 @@ describe("FieldValuesWidget", () => {
       });
 
       expect(screen.getByText(LISTABLE_PK_FIELD_VALUE)).toBeInTheDocument();
-      expect(fetchFieldValues).toHaveBeenCalledWith({
-        id: LISTABLE_PK_FIELD_ID,
-      });
-      expect(fetchFieldValues).not.toHaveBeenCalledWith({
-        id: EXPRESSION_FIELD_ID,
-      });
+      expect(fetchFieldValues).toHaveBeenCalledWith(
+        expect.objectContaining({
+          id: LISTABLE_PK_FIELD_ID,
+          table_id: valuesField.table_id,
+        }),
+      );
+      expect(fetchFieldValues).not.toHaveBeenCalledWith(
+        expect.objectContaining({
+          id: EXPRESSION_FIELD_ID,
+          table_id: expressionField.table_id,
+        }),
+      );
     });
   });
 

frontend/src/metabase/components/FormCategoryInput/CategoryRadioPicker.tsx

@@ -5,6 +5,7 @@ import Radio from "metabase/core/components/Radio";
 import Fields from "metabase/entities/fields";
 
 import type { State } from "metabase-types/store";
+import type Field from "metabase-lib/metadata/Field";
 
 import type { CategoryWidgetProps as CategoryWidgetOwnProps } from "./types";
 
@@ -13,7 +14,7 @@ interface CategoryWidgetStateProps {
 }
 
 interface CategoryWidgetDispatchProps {
-  fetchFieldValues: (opts: { id: number }) => void;
+  fetchFieldValues: (field: Field) => void;
 }
 
 interface CategoryWidgetProps
@@ -43,7 +44,7 @@ function CategoryRadioPicker({
 }: CategoryWidgetProps) {
   useMount(() => {
     if (typeof field.id === "number") {
-      fetchFieldValues({ id: field.id });
+      fetchFieldValues(field);
     }
   });
 

frontend/src/metabase/core/components/AccordionList/AccordionList.jsx

@@ -353,7 +353,7 @@ export default class AccordionList extends Component {
 
     const searchRow = this.getRows().findIndex(row => row.type === "search");
 
-    if (searchRow >= 0) {
+    if (searchRow >= 0 && this.isVirtualized()) {
       this._list.scrollToRow(searchRow);
     }
   };

frontend/src/metabase/entities/fields.js

@@ -30,6 +30,7 @@ import {
 } from "metabase/lib/core";
 import { TYPE } from "metabase-lib/types/constants";
 import { getFieldValues } from "metabase-lib/queries/utils/field";
+import { getUniqueFieldId } from "metabase-lib/metadata/utils/fields";
 
 // ADDITIONAL OBJECT ACTIONS
 
@@ -71,21 +72,27 @@ const Fields = createEntity({
     fetchFieldValues: compose(
       withAction(FETCH_FIELD_VALUES),
       withCachedDataAndRequestState(
-        ({ id }) => [...Fields.getObjectStatePath(id)],
-        ({ id }) => [...Fields.getObjectStatePath(id), "values"],
-        entityQuery => Fields.getQueryKey(entityQuery),
+        ({ id, table_id }) => {
+          const uniqueId = getUniqueFieldId({ id, table_id });
+          return [...Fields.getObjectStatePath(uniqueId)];
+        },
+        ({ id, table_id }) => {
+          const uniqueId = getUniqueFieldId({ id, table_id });
+          return [...Fields.getObjectStatePath(uniqueId), "values"];
+        },
+        field => {
+          return Fields.getQueryKey({ id: field.id });
+        },
       ),
       withNormalize(FieldSchema),
-    )(({ id: fieldId, ...params }) => async (dispatch, getState) => {
-      const {
-        field_id: id,
-        values,
-        has_more_values,
-      } = await MetabaseApi.field_values({
-        fieldId,
-        ...params,
+    )(field => async () => {
+      const { field_id, ...data } = await MetabaseApi.field_values({
+        fieldId: field.id,
       });
-      return { id, values, has_more_values };
+      const table_id = field.table_id;
+
+      // table_id is required for uniqueFieldId as it's a way to know if field is virtual
+      return { id: field_id, ...data, ...(table_id && { table_id }) };
     }),
 
     updateField(field, values, opts) {
@@ -100,7 +107,7 @@ const Fields = createEntity({
 
         // field values needs to be fetched again once the field is updated metabase#16322
         await dispatch(
-          Fields.actions.fetchFieldValues({ id: field.id }, { reload: true }),
+          Fields.actions.fetchFieldValues(field, { reload: true }),
         );
 
         return result;

frontend/src/metabase/lib/entities.js

@@ -166,7 +166,7 @@ export function createEntity(def) {
 
   // normalize helpers
   entity.normalize = (object, schema = entity.schema) => ({
-    // include raw `object` (and alias under nameOne) for convienence
+    // include raw `object` (and alias under nameOne) for convenience
     object,
     [entity.nameOne]: object,
     // include standard normalizr properties, `result` and `entities`
@@ -174,7 +174,7 @@ export function createEntity(def) {
   });
 
   entity.normalizeList = (list, schema = entity.schema) => ({
-    // include raw `list` (and alias under nameMany) for convienence
+    // include raw `list` (and alias under nameMany) for convenience
     list,
     [entity.nameMany]: list,
     // include standard normalizr properties, `result` and `entities`

frontend/src/metabase/redux/metadata.js

@@ -99,11 +99,6 @@ export const fetchField = createThunkAction(
   },
 );
 
-export const fetchFieldValues = (id, reload = false) => {
-  deprecated("metabase/redux/metadata fetchFieldValues");
-  return Fields.actions.fetchFieldValues({ id }, reload);
-};
-
 export const updateFieldValues = (fieldId, fieldValuePairs) => {
   deprecated("metabase/redux/metadata updateFieldValues");
   return Fields.actions.updateFieldValues({ id: fieldId }, fieldValuePairs);