Fix card filter connected to a field not causing an update #36206

oleggromov · 2023-11-29T18:08:19Z

Description

The problem was that for some reason in Dashboard.componentDidUpdate, the method that decides whether to fetch new data, only parameter values where checked — but not parameters themselves.

I have fixed that, removing garbage code along the way, and added "unit tests" for the Dashboard component. Net change (without the test code) is -46 sloc, which I think is worth it.

Test approach evolution

I have tried to implement an e2e test suite, which is pure madness in my opinion — I would've to recreate the entire page in a browser and click through 10 times to cover a tiny conditional way down in the function call tree.
Then I tried to extract a real unit from a react component (which hardly is a "unit" itself), but realised that I would still need to invoke the component itself to make sure the integration of the component and this unit with conditionals is covered too.
Finally I have found a happy middle ground, which I am quite satisfied with, but the setup part is ugly anyways.

PS Certainly the tests we produce with react testing library aren't unit tests because they require a rich environment, so much mocking, and are slow.

How to verify

Repeat after what I do in the screencast.

Before it didn't work

287385786-a0544210-58b4-486f-95ca-c81d8b26e5e8.mov

Now it works

287386102-4a8010f3-09e5-4660-b07e-09076d263189.mov

Checklist

Tests have been added/updated to cover changes in this PR

deploysentinel · 2023-11-30T18:55:56Z

No failed tests 🎉

oleggromov · 2023-12-01T21:10:57Z

frontend/src/metabase/dashboard/components/Dashboard/Dashboard.jsx


    if (
      !_.isEqual(prevProps.parameterValues, this.props.parameterValues) ||
-      (!prevProps.dashboard && this.props.dashboard)


We do initially render Dashboard with dashboard={null}. However, this conditional never works as it's superseded by the first one in this method: prevProps.dashboardId !== this.props.dashboardId.

I was wrong, so reverting this. Won't be committing after working hours anymore.

oleggromov · 2023-12-01T21:11:39Z

frontend/src/metabase/dashboard/components/Dashboard/Dashboard.jsx

    if (
      !_.isEqual(prevProps.parameterValues, this.props.parameterValues) ||
-      (!prevProps.dashboard && this.props.dashboard)
+      !_.isEqual(prevProps.parameters, this.props.parameters)


This is the actual bug fix.

oleggromov · 2023-12-01T21:12:02Z

frontend/src/metabase/dashboard/components/Dashboard/Dashboard.jsx

-    const canWrite = dashboard?.can_write ?? false;
-
-    const dashboardHasCards = dashboard?.dashcards.length > 0 ?? false;
+  shouldRenderAsNightMode() {


I have extracted this as a method as it was used in two different methods.

oleggromov · 2023-12-01T21:12:53Z

frontend/src/metabase/dashboard/components/Dashboard/Dashboard.jsx

+  }

+  renderContent = () => {
+    const p = this.props;


Everything that's happening here is removing destructured props as they are essentially slag.

oleggromov · 2023-12-01T21:13:51Z

frontend/src/metabase/dashboard/components/Dashboard/Dashboard.jsx

-    const shouldRenderAsNightMode = isNightMode && isFullscreen;
-
-    const visibleParameters = getVisibleParameters(parameters);
+    const p = this.props;


Same here: instead of wasting lines on avoiding writing this.props, I am putting it into a one-char variable (which is still ugly) that's almost invisible to the eye and removes this unnecessary cruft.

oleggromov · 2023-12-01T21:15:25Z

frontend/src/metabase/dashboard/containers/DashboardApp/DashboardApp.unit.spec.tsx

-
-const TEST_CARD = createMockCard();
-
-const TEST_TABLE = createMockTable();


Same cruft as everywhere else. We don't need "constants" when they're not exported or at least used multiple times, and absolutely certainly we don't need them at the top of the file — it's ugly and useless albeit quite common.

oleggromov · 2023-12-01T21:15:57Z

frontend/src/metabase/dashboard/containers/DashboardApp/DashboardApp.unit.spec.tsx

 }

 async function setup({ dashboard }: Options = {}) {
+  const TEST_COLLECTION = createMockCollection();


This is where they belong. I don't use this SHOUT_CASE for every constant but here I left it as is.

Convert to regular camel case straight away?

ranquild

Good overall. Somewhat inconsistent with other integration tests (naming etc).

ranquild · 2023-12-04T13:43:58Z

frontend/src/metabase/dashboard/components/Dashboard/Dashboard.unit.spec.tsx

nit: We usually call it SetupOpts

ranquild · 2023-12-04T13:44:18Z

frontend/src/metabase/dashboard/components/Dashboard/Dashboard.unit.spec.tsx

nit: We usually omit the return type in setup function

ranquild · 2023-12-04T13:44:50Z

frontend/src/metabase/dashboard/components/Dashboard/Dashboard.unit.spec.tsx

nit: We usually name callbacks as onFetch... and return them as it is

I am not sure it's correct in this case since these aren't really "event handlers", which on* would suggest, but rather callbacks or action dispatchers or whatever you call them. So I'll leave as is if you don't object. I think it reads well.

ranquild · 2023-12-04T13:45:45Z

frontend/src/metabase/dashboard/containers/DashboardApp/DashboardApp.unit.spec.tsx

 }

 async function setup({ dashboard }: Options = {}) {
+  const TEST_COLLECTION = createMockCollection();


Convert to regular camel case straight away?

ranquild · 2023-12-04T13:46:26Z

frontend/src/metabase/dashboard/components/Dashboard/Dashboard.unit.spec.tsx

We usually use these props in setup function so we don't need to deal with spreading

That's actually a very good catch, thanks!

oleggromov · 2023-12-05T15:48:41Z

frontend/src/metabase/dashboard/components/Dashboard/Dashboard.jsx

+        p.addCardToDashboard({
+          dashId: p.dashboardId,
+          cardId: p.addCardOnLoad,
+          tabId: p.dashboard?.tabs[0]?.id ?? null,


Notice ? after dashboard. This was causing exceptions (and failed e2e tests) when dashboard is null.

* Rename DashboardHeaderView to match filename * Fix non updating cards on filter mapping to a field * Clean up DashboardApp tests a bit * Clean up Dashboard.jsx cruft * Add Dashboard componentDidUpdate "unit tests" * Simplify setup code as per feedback * Add test for changing dashboard prop in componentDidMount * Change constant names * Fix optional chaining in Dashboard

…36419) * Rename DashboardHeaderView to match filename * Fix non updating cards on filter mapping to a field * Clean up DashboardApp tests a bit * Clean up Dashboard.jsx cruft * Add Dashboard componentDidUpdate "unit tests" * Simplify setup code as per feedback * Add test for changing dashboard prop in componentDidMount * Change constant names * Fix optional chaining in Dashboard Co-authored-by: Oleg Gromov <mail@oleggromov.com>

…36206)" This reverts commit 2af32bb.

* Rename DashboardHeaderView to match filename * Fix non updating cards on filter mapping to a field * Clean up DashboardApp tests a bit * Clean up Dashboard.jsx cruft * Add Dashboard componentDidUpdate "unit tests" * Simplify setup code as per feedback * Add test for changing dashboard prop in componentDidMount * Change constant names * Fix optional chaining in Dashboard

metabase-bot bot assigned oleggromov Nov 29, 2023

metabase-bot bot added the .Team/Querying label Nov 29, 2023

oleggromov added the backport Automatically create PR on current release branch on merge label Nov 30, 2023

oleggromov added this to the 0.47.9 milestone Nov 30, 2023

oleggromov force-pushed the bugfix-35503-new branch 2 times, most recently from 8661ca2 to 25dc512 Compare November 30, 2023 18:42

perivamsi removed this from the 0.47.9 milestone Nov 30, 2023

perivamsi removed the backport Automatically create PR on current release branch on merge label Nov 30, 2023

oleggromov force-pushed the bugfix-35503-new branch 2 times, most recently from 997cd59 to 5e49fc2 Compare December 1, 2023 21:08

oleggromov commented Dec 1, 2023

View reviewed changes

oleggromov added the backport Automatically create PR on current release branch on merge label Dec 1, 2023

oleggromov added this to the 0.47.10 milestone Dec 1, 2023

oleggromov requested review from alxnddr and ranquild December 1, 2023 21:28

oleggromov changed the title ~~WIP Fix card filter connected to a field not causing an update~~ Fix card filter connected to a field not causing an update Dec 1, 2023

oleggromov force-pushed the bugfix-35503-new branch from 5e49fc2 to be4b7b0 Compare December 4, 2023 11:43

ranquild approved these changes Dec 4, 2023

View reviewed changes

oleggromov force-pushed the bugfix-35503-new branch from be4b7b0 to f0a9240 Compare December 4, 2023 18:06

oleggromov added 4 commits December 5, 2023 14:58

Rename DashboardHeaderView to match filename

60b62dd

Fix non updating cards on filter mapping to a field

3eb7024

Clean up DashboardApp tests a bit

698d7d1

Clean up Dashboard.jsx cruft

7f52759

oleggromov added 6 commits December 5, 2023 14:58

Fix typos

85196c5

Add Dashboard componentDidUpdate "unit tests"

70577c9

Restore previous conditional

47b5d55

Simplify setup code as per feedback

4ba2f48

Add test for changing dashboard prop in componentDidMount

5258450

Change constant names

d5de9b7

oleggromov force-pushed the bugfix-35503-new branch from 6682b22 to d5de9b7 Compare December 5, 2023 15:11

Fix optional chaining in Dashboard

e53f824

oleggromov commented Dec 5, 2023

View reviewed changes

oleggromov merged commit 2af32bb into master Dec 5, 2023

oleggromov deleted the bugfix-35503-new branch December 5, 2023 16:27

metabase-bot bot mentioned this pull request Dec 5, 2023

🤖 backported " Fix card filter connected to a field not causing an update" #36419

Merged

kulyk added a commit that referenced this pull request Dec 7, 2023

Revert " Fix card filter connected to a field not causing an update (#…

2aaae11

…36206)" This reverts commit 2af32bb.

kulyk mentioned this pull request Dec 7, 2023

Revert "Fix card filter connected to a field not causing an update" #36534

Merged


		const TEST_CARD = createMockCard();

		const TEST_TABLE = createMockTable();

Fix card filter connected to a field not causing an update #36206

Fix card filter connected to a field not causing an update #36206

Uh oh!

Conversation

oleggromov commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test approach evolution

How to verify

Before it didn't work

Now it works

Checklist

Uh oh!

deploysentinel bot commented Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleggromov Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranquild left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleggromov Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

oleggromov commented Nov 29, 2023 •

edited

Loading

deploysentinel bot commented Nov 30, 2023 •

edited

Loading

oleggromov Dec 1, 2023 •

edited

Loading

oleggromov Dec 4, 2023 •

edited

Loading