Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dashboard parameter values in embed preview not working #37972

Conversation

WiNloSt
Copy link
Member

@WiNloSt WiNloSt commented Jan 22, 2024

Closes #37914

Description

When we preview embed, we have some conditional logics that make sure the preview works e.g. we want to always allow previewing the dashboard even without publishing it first.

This has a consequence on the dashboard values that makes it calls an unavailable API. It calls the dashboard value with UUID instead of the number ID.

How to verify

Follow the reproduce steps from #37914, and make sure the dashboard preview and embedded dashboard parameters behaviors are the same.

Demo

Upload a demo video or before/after screenshots if sensible or remove the section

Checklist

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

@WiNloSt WiNloSt marked this pull request as ready for review January 22, 2024 13:44
Copy link

replay-io bot commented Jan 22, 2024

StatusComplete ↗︎
Commit3170b2c
Results
⚠️ 3 Flaky
  • should display the toast indefinitely unless dismissing manually
  • should validate approved email domains for a dashboard subscription in the audit app
  • should work for longitude
2232 Passed

@WiNloSt WiNloSt added the backport Automatically create PR on current release branch on merge label Jan 22, 2024
@WiNloSt WiNloSt force-pushed the 37914-fix-previewed-embed-dashboard-parameter-values-not-working branch 2 times, most recently from 692f5c3 to 2ca1202 Compare January 24, 2024 14:09
@WiNloSt WiNloSt force-pushed the 37914-fix-previewed-embed-dashboard-parameter-values-not-working branch from 2ca1202 to e2f07d8 Compare January 24, 2024 14:11
query: { "source-table": PRODUCTS_ID },
};

const filter3 = {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I declare filter3 first because I want to use filter2's id in filter, and I don't want to hardcode that, so I need to declare filter2 before filter1, and I figure I should put filter3 first for consistency.

@@ -276,7 +277,7 @@ export const fetchDashboard = createAsyncThunk(
return {
entities,
dashboard: result,
dashboardId: dashId,
dashboardId: result.id,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken a look at all the dashboard types above.

  • embed: Embedded dashboard
  • transient: x-ray dashboard
  • inline: I don't think this is used anymore. Asked a question in Slack.
  • and a normal dashboard.

All have result.id === dashId except the embed dashboard since I change that to IS_EMBED_PREVIEW ? result.id : dashId in this PR. I don't want to repeat the same condition again, so I figure using result.id would make more sense.

@@ -196,7 +197,7 @@ export const fetchDashboard = createAsyncThunk(
);
result = {
...result,
id: dashId,
id: IS_EMBED_PREVIEW ? result.id : dashId,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is essentially the fix. Previously dashId in this case (dashboardType === "embed") would be a UUID, and that makes calling the parameter values API with the dashboard as a UUID

But that means we'd be calling /api/dashboard/:uuid which is wrong, we're supposed to call /api/dashboard/:dashboard-id instead. The thing is that in embed preview we don't call /api/embed/* endpoints because it's possible for the resource to not have been embedded yet, calling /api/embed/* in that case will not return anything.

You can see that we have a logic that changes the dashboard and question parameter values endpoints to have /api/embed prefix for embedding, but we don't do that in embed preview

export function setEmbedQuestionEndpoints(token) {
if (!IS_EMBED_PREVIEW) {
setCardEndpoints("/api/embed/card/:token", { token });
}
}
export function setEmbedDashboardEndpoints() {
if (!IS_EMBED_PREVIEW) {
setDashboardEndpoints("/api/embed");
}
}

const lockedParameters = getLockedPreviewParameters(
resourceParameters,
embeddingParams,
);

const iframeUrl = useMemo(() => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the problem where the iframe is rendered twice.

https://www.loom.com/share/c082515a29724bcdb7d8a67535f21a1c

@WiNloSt WiNloSt requested a review from a team January 24, 2024 14:39
@WiNloSt WiNloSt merged commit a504c8b into master Jan 25, 2024
107 checks passed
@WiNloSt WiNloSt deleted the 37914-fix-previewed-embed-dashboard-parameter-values-not-working branch January 25, 2024 09:37
Copy link

@WiNloSt Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

WiNloSt added a commit that referenced this pull request Jan 25, 2024
* Fix dashboard parameter values in embed preview not working

* Fix iframe rerendering unnecessarily

* Add E2E reproduction test

* Prevent iframe from rerendering unnecessarily
WiNloSt added a commit that referenced this pull request Jan 25, 2024
…38120)

* Fix dashboard parameter values in embed preview not working

* Fix iframe rerendering unnecessarily

* Add E2E reproduction test

* Prevent iframe from rerendering unnecessarily

Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev>
sloansparger pushed a commit that referenced this pull request Feb 5, 2024
* Fix dashboard parameter values in embed preview not working

* Fix iframe rerendering unnecessarily

* Add E2E reproduction test

* Prevent iframe from rerendering unnecessarily
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Automatically create PR on current release branch on merge .Team/Embedding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Text filters shown as input box in embed instead as dropdown
2 participants