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

Text filters shown as input box in embed instead as dropdown #37914

Closed
albertoperdomo opened this issue Jan 19, 2024 · 4 comments · Fixed by #37972 or #41342
Closed

Text filters shown as input box in embed instead as dropdown #37914

albertoperdomo opened this issue Jan 19, 2024 · 4 comments · Fixed by #37972 or #41342
Assignees
Labels
Embedding/Static Static embedding, previously known as signed embedding .Escalation Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness .Regression Bugs that were previously fixed and/or bugs unintentionally shipped with new features. .Team/Embedding Type:Bug Product defects
Milestone

Comments

@albertoperdomo
Copy link
Member

albertoperdomo commented Jan 19, 2024

Describe the bug

Decided to open this issue after spending some time reproducing #31347 and noticing the behavior has changed a lot.
Essentially, you expect filters to look the same in the embed, in the embed preview and in the dashboard inside of Metabase.

Findings:

  • filter set as dropdown in dashboard, no default value for filter in dashboard, editable in embed -> filter is rendered as input box in embed
  • filter set as dropdown in dashboard, default value set for filter in dashboard, locked in embed -> filter is rendered as input box in embed
  • filter set as droddown in dashboard, default value set for filter in dashboard, editable in embed -> filter is rendered as dropdown in embed as it should, but it's rendered differently in the preview

The preview features and linking of the filters seems to work properly, unlike reported in #31347.

To Reproduce

  1. Go to New -> Question -> Products -> Save
  2. Create New Dashboard -> Add 3 Filters and Add the above question then link them to column Category
image 3. Link the first filter to the second one -> Save image 4. Confirm that the link is working by selecting Doohickey for Second Filter and Gadget,Gizmo for Third filter ... Notice that on metabase works as expected and First filter only shows Doohickey as an option image 5. Go to Sharing -> Embed your application -> Make the filters editable -> Publish 6. The filters are rendered in the embed and preview as input boxes instead of as dropdown image
  1. Edit the dashboard, set the default value for text 1 as Doohickey -> Save

  2. Go to Sharing -> Embed your application -> Preview

  3. In the preview Text filter is still rendered as input box but text 1 is rendered as multi-select, in the embed it is a proper dropdown

Text:
image

Text 1 in preview inside of UI:
image

Text 1 opening iframe from preview URL in incognito mode:
image

  1. Copy the iframe URL from the preview using inspect and open it in incognito mode. You will see that text and text1 are now rendered as dropdown and also that the linking of the filters is working as expected. Text2 is still rendered as input box.

  2. Set text1 to locked, set a preview value for it as "Doohickey", publish, copy the iframe URL from the preview using inspect, open it in incognito mode. The results are properly filtered (Doohickey only), but text filter shows again as input box even though there is a default value

Expected behavior

The filters should look the same across dashboard in MB, embed preview and real life embed. The combination of setting a default value in the dashboard and setting the filter as editable triggers this consistent behavior but it should not be needed

Logs

No response

Information about your Metabase installation

Metabase v1.48.1

Severity

Might be blocking in certain use cases

Additional context

No response

@albertoperdomo
Copy link
Member Author

Setting a default value for both filters and embedding triggers the correct behavior and rendering on the embed:

image (12)

@Tony-metabase
Copy link
Contributor

There was already some research on linked filters by @WiNloSt which is good to have it here for reference:

#31347 (comment)

@WiNloSt
Copy link
Member

WiNloSt commented Jan 22, 2024

@albertoperdomo I might found 2 issues in this single issue.

  1. The preview embed on Metabase instance and the preview embed when viewing it directly (copy the iframe URL to a new tab) will result in parameters behaving differently because the embed preview will call an unexistent API. This should be solved in this PR Fix dashboard parameter values in embed preview not working #37972

  2. When we set a parameter to locked, all other filters that map to the same locked parameter won't be working in embedding anymore. I think this might be a BE issue, so I'd need some BE engineer aid here.

    in embedding, when calling the dashboard endpoint when all parameters are set to editable we'll get param_fields (1) in the response which makes FE show the dropdown. But when we set any of the parameters to anything except editible (locked, or disabled), we'll not get any value in param_fields (2)

    1. image
    2. image

@Tony-metabase
Copy link
Contributor

Tony-metabase commented Mar 26, 2024

I am reopening this one since I am testing to make sure the embedding behaves the same as metabase and I am getting a text filter everywhere so I cannot even test if linked filters are working as expected on embedding (i.e. similar to the metabase dashboard)

Both when filters are set to Disabled or Locked

@Tony-metabase Tony-metabase reopened this Mar 26, 2024
@Tony-metabase Tony-metabase added .Regression Bugs that were previously fixed and/or bugs unintentionally shipped with new features. .Escalation and removed .Escalation labels Mar 26, 2024
@Tony-metabase Tony-metabase removed this from the 0.48.4 milestone Mar 28, 2024
@deniskaber deniskaber mentioned this issue Apr 2, 2024
1 task
@perivamsi perivamsi assigned adam-james-v and unassigned WiNloSt Apr 7, 2024
@cbalusek cbalusek added Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness and removed Priority:P2 Average run of the mill bug labels Apr 10, 2024
adam-james-v added a commit that referenced this issue Apr 12, 2024
Fixes: #37914

Previously, we removed locked parameters entirely. This is to prevent leaking potentially sensitive values.

However, for the situation where the same field backs 1 locked or disabled paramter and 1 enabled parameter, we do
still want to send the paramater values, because the enabled parameter implies that the values are permissible to see
in the embed.

So, this change will still remove parameters based on their 'disabled/locked/enabled' status, but will NOT remove the
linked field ids if they're also being used by 'enabled' parameters.

This results in the backend correctly sending necessary parameter values to the embed, where the frontend can then
render the appropriate UI instead of falling back to just text filters.
adam-james-v added a commit that referenced this issue Apr 12, 2024
Fixes: #37914

Previously, we removed locked parameters entirely. This is to prevent leaking potentially sensitive values.

However, for the situation where the same field backs 1 locked or disabled paramter and 1 enabled parameter, we do
still want to send the paramater values, because the enabled parameter implies that the values are permissible to see
in the embed.

So, this change will still remove parameters based on their 'disabled/locked/enabled' status, but will NOT remove the
linked field ids if they're also being used by 'enabled' parameters.

This results in the backend correctly sending necessary parameter values to the embed, where the frontend can then
render the appropriate UI instead of falling back to just text filters.
adam-james-v added a commit that referenced this issue Apr 12, 2024
* Keep Filter Values for enabled Parameters in Embedding

Fixes: #37914

Previously, we removed locked parameters entirely. This is to prevent leaking potentially sensitive values.

However, for the situation where the same field backs 1 locked or disabled paramter and 1 enabled parameter, we do
still want to send the paramater values, because the enabled parameter implies that the values are permissible to see
in the embed.

So, this change will still remove parameters based on their 'disabled/locked/enabled' status, but will NOT remove the
linked field ids if they're also being used by 'enabled' parameters.

This results in the backend correctly sending necessary parameter values to the embed, where the frontend can then
render the appropriate UI instead of falling back to just text filters.

* test the case where a locked and enabled param share same field

* Address feedback. Added comment to explain classify fn
github-actions bot pushed a commit that referenced this issue Apr 12, 2024
* Keep Filter Values for enabled Parameters in Embedding

Fixes: #37914

Previously, we removed locked parameters entirely. This is to prevent leaking potentially sensitive values.

However, for the situation where the same field backs 1 locked or disabled paramter and 1 enabled parameter, we do
still want to send the paramater values, because the enabled parameter implies that the values are permissible to see
in the embed.

So, this change will still remove parameters based on their 'disabled/locked/enabled' status, but will NOT remove the
linked field ids if they're also being used by 'enabled' parameters.

This results in the backend correctly sending necessary parameter values to the embed, where the frontend can then
render the appropriate UI instead of falling back to just text filters.

* test the case where a locked and enabled param share same field

* Address feedback. Added comment to explain classify fn
@adam-james-v adam-james-v added this to the 0.49.6 milestone Apr 12, 2024
metabase-bot bot added a commit that referenced this issue Apr 12, 2024
* Keep Filter Values for enabled Parameters in Embedding

Fixes: #37914

Previously, we removed locked parameters entirely. This is to prevent leaking potentially sensitive values.

However, for the situation where the same field backs 1 locked or disabled paramter and 1 enabled parameter, we do
still want to send the paramater values, because the enabled parameter implies that the values are permissible to see
in the embed.

So, this change will still remove parameters based on their 'disabled/locked/enabled' status, but will NOT remove the
linked field ids if they're also being used by 'enabled' parameters.

This results in the backend correctly sending necessary parameter values to the embed, where the frontend can then
render the appropriate UI instead of falling back to just text filters.

* test the case where a locked and enabled param share same field

* Address feedback. Added comment to explain classify fn

Co-authored-by: adam-james <21064735+adam-james-v@users.noreply.github.com>
crisptrutski pushed a commit that referenced this issue Apr 15, 2024
* Keep Filter Values for enabled Parameters in Embedding

Fixes: #37914

Previously, we removed locked parameters entirely. This is to prevent leaking potentially sensitive values.

However, for the situation where the same field backs 1 locked or disabled paramter and 1 enabled parameter, we do
still want to send the paramater values, because the enabled parameter implies that the values are permissible to see
in the embed.

So, this change will still remove parameters based on their 'disabled/locked/enabled' status, but will NOT remove the
linked field ids if they're also being used by 'enabled' parameters.

This results in the backend correctly sending necessary parameter values to the embed, where the frontend can then
render the appropriate UI instead of falling back to just text filters.

* test the case where a locked and enabled param share same field

* Address feedback. Added comment to explain classify fn
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Embedding/Static Static embedding, previously known as signed embedding .Escalation Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness .Regression Bugs that were previously fixed and/or bugs unintentionally shipped with new features. .Team/Embedding Type:Bug Product defects
Projects
None yet
5 participants