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

Embedding Dashboard with Locked parameters does not allow numeric values #25031

Closed
nemanjaglumac opened this issue Aug 26, 2022 · 5 comments · Fixed by #26969
Closed

Embedding Dashboard with Locked parameters does not allow numeric values #25031

nemanjaglumac opened this issue Aug 26, 2022 · 5 comments · Fixed by #26969
Assignees
Labels
.Backend Embedding/Static Static embedding, previously known as signed embedding Priority:P2 Average run of the mill bug Querying/Parameters & Variables Filter widgets, field filters, variables etc. .Regression Bugs that were previously fixed and/or bugs unintentionally shipped with new features. Reporting/Dashboards .Reproduced Issues reproduced in test (usually Cypress) Type:Bug Product defects
Milestone

Comments

@nemanjaglumac
Copy link
Member

nemanjaglumac commented Aug 26, 2022

Describe the bug
This is the repetition of #20845, but for the dashboard scenario.

To Reproduce
Steps to reproduce the behavior:

  1. Native Question > Sample > select count(*) from orders where true [[AND quantity={{qty_locked}}]] - set variable as Number
  2. Add question to dashboard
  3. Add filter to the dashboard (Number > Equal to) and connect it to the card/question on qty_locked
    image
  4. Save the dashboard
  5. Enable embedding and set the dashboard filter "Equal to" to locked

Note
From this point on it is not possible to reproduce this issue merely using the UI. You have to generate the JWT token with the numeric value. Frontend is passing a string, even though the filter is numeric.

{
  resource: { dashboard: dashboard_id },
  params: {
    equal_to: 15 // IMPORTANT: integer
  },
}

If we now visit the embedded dashboard URL, it will error with "There was a problem displaying this chart":
image

As with the previous issue, backend error is not helpful, either:
Don't know how to create ISeq from: java.lang.Integer

Expected behavior
Allow numeric parameters.
While we're at it, would be nice if we allowed the resource as string, since I see that as a common mistake as well.

Information about your Metabase Installation:
local dev, master, 9335035, H2, Sample Database

@nemanjaglumac nemanjaglumac added Type:Bug Product defects .Needs Triage Embedding/Static Static embedding, previously known as signed embedding labels Aug 26, 2022
nemanjaglumac added a commit that referenced this issue Aug 26, 2022
@flamber flamber added Priority:P2 Average run of the mill bug Reporting/Dashboards Querying/Parameters & Variables Filter widgets, field filters, variables etc. .Backend .Regression Bugs that were previously fixed and/or bugs unintentionally shipped with new features. and removed .Needs Triage labels Aug 26, 2022
@flamber
Copy link
Contributor

flamber commented Aug 26, 2022

While we're at it, let's make sure that we allow resources to be strings too.
The logic is currently that resources has to be numeric, while the params has to be string, which seems off.

resource: { dashboard: 123 }, //works
resource: { dashboard: "123" }, //fails

@nemanjaglumac
Copy link
Member Author

While we're at it, let's make sure that we allow resources to be strings too. The logic is currently that resources has to be numeric, while the params has to be string, which seems off.

resource: { dashboard: 123 }, //works
resource: { dashboard: "123" }, //fails

Yeah, added that to the "Expected behavior", or rather copied it over from your original issue.
The repro is checking for both strings and integers.

@laura-radaelli
Copy link

Hi guys, thanks for adding this issue, I will follow the progress. I am not sure whether it is useful, but I wanted to add another 2 scenarios where this happens for me:

scenario 1:
1.-4. same as you described
5. Enable embedding and set the dashboard filter to editable

scenario 2:

  1. Query builder > from a table that has a Numeric field
  2. -5. same as you described

Both of those lead me to the same "There was a problem displaying this chart" and same backend error.

nemanjaglumac added a commit that referenced this issue Aug 27, 2022
…ow numeric values (#25032)

* Add repro for #25031

* Rename file

* Clean up issue references
github-actions bot pushed a commit that referenced this issue Aug 27, 2022
…ow numeric values (#25032)

* Add repro for #25031

* Rename file

* Clean up issue references
@nemanjaglumac nemanjaglumac added the .Reproduced Issues reproduced in test (usually Cypress) label Aug 28, 2022
nemanjaglumac added a commit that referenced this issue Aug 31, 2022
…ow numeric values (#25032) (#25058)

* Add repro for #25031

* Rename file

* Clean up issue references

Co-authored-by: Nemanja Glumac <31325167+nemanjaglumac@users.noreply.github.com>
@ranquild ranquild self-assigned this Sep 12, 2022
@dpsutton
Copy link
Contributor

dpsutton commented Dec 5, 2022

While we're at it, let's make sure that we allow resources to be strings too. The logic is currently that resources has to be numeric, while the params has to be string, which seems off.

   resource: { dashboard: 123 }, //works
   resource: { dashboard: "123" }, //fails

I'm not sure I follow what's going on here and why we would expect string ids to work? Dashboards have numeric ids (eg. 123). I don't see why we would expect a string key to work here. It seems like we should be more careful with what values we put into tokens and the expectations when we take it from the jwt, not that we should loosen our parsing.

@dpsutton dpsutton self-assigned this Dec 5, 2022
@flamber flamber added this to the 0.45.2 milestone Dec 8, 2022
@nemanjaglumac

This comment was marked as resolved.

nemanjaglumac added a commit that referenced this issue Dec 8, 2022
nemanjaglumac added a commit that referenced this issue Dec 8, 2022
github-actions bot pushed a commit that referenced this issue Dec 8, 2022
metabase-bot bot added a commit that referenced this issue Dec 8, 2022
Co-authored-by: Nemanja Glumac <31325167+nemanjaglumac@users.noreply.github.com>
This was referenced Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.Backend Embedding/Static Static embedding, previously known as signed embedding Priority:P2 Average run of the mill bug Querying/Parameters & Variables Filter widgets, field filters, variables etc. .Regression Bugs that were previously fixed and/or bugs unintentionally shipped with new features. Reporting/Dashboards .Reproduced Issues reproduced in test (usually Cypress) Type:Bug Product defects
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants