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

Multi-question cards filter data incorrectly in email / Slack dashboard subscriptions #39083

Closed
zbodi74 opened this issue Feb 22, 2024 · 0 comments · Fixed by #43428
Closed
Assignees
Labels
.Correctness Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness Reporting/Pulses Now called Subscriptions .Team/DashViz Dashboard and Viz team Type:Bug Product defects
Milestone

Comments

@zbodi74
Copy link

zbodi74 commented Feb 22, 2024

Describe the bug

When a card has multiple questions added filters set in the dashboard subscription will only filter the first series of the card.

To Reproduce

  1. Create a dashboard
  2. Add a question
  3. Add another question to the first card
  4. Set up a dashboard filter and wire it to both questions of the first card
  5. Set up an email or Slack subscription for the dashboard, set a filter value, and send a test email
  6. (!) Only one of the series will be filtered

Dashboard filter with unfiltered data:
image

Output of email subscription when data is filtered (should only return for 1 month for both series)
image

Expected behavior

Similarly as it works on a dashboard, the filter should be applied to each series of the card

Logs

No response

Information about your Metabase installation

{
  "browser-info": {
    "language": "en-GB",
    "platform": "MacIntel",
    "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/121.0.0.0 Safari/537.36",
    "vendor": "Google Inc."
  },
  "system-info": {
    "file.encoding": "UTF-8",
    "java.runtime.name": "OpenJDK Runtime Environment",
    "java.runtime.version": "11.0.22+7",
    "java.vendor": "Eclipse Adoptium",
    "java.vendor.url": "https://adoptium.net/",
    "java.version": "11.0.22",
    "java.vm.name": "OpenJDK 64-Bit Server VM",
    "java.vm.version": "11.0.22+7",
    "os.name": "Linux",
    "os.version": "5.10.209-198.812.amzn2.x86_64",
    "user.language": "en",
    "user.timezone": "GMT"
  },
  "metabase-info": {
    "databases": [
      "snowflake",
      "bigquery-cloud-sdk",
      "postgres",
      "h2"
    ],
    "hosting-env": "unknown",
    "application-database": "postgres",
    "application-database-details": {
      "database": {
        "name": "PostgreSQL",
        "version": "14.10"
      },
      "jdbc-driver": {
        "name": "PostgreSQL JDBC Driver",
        "version": "42.6.0"
      }
    },
    "run-mode": "prod",
    "version": {
      "date": "2024-02-06",
      "tag": "v1.48.5",
      "hash": "dab12cf"
    },
    "settings": {
      "report-timezone": "America/Los_Angeles"
    }
  }
}

Severity

P1 (correctness)

Additional context

No response

@zbodi74 zbodi74 added Type:Bug Product defects Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness Reporting/Pulses Now called Subscriptions .Correctness labels Feb 22, 2024
@cdeweyx cdeweyx added the .Team/DashViz Dashboard and Viz team label Feb 26, 2024
@adam-james-v adam-james-v self-assigned this May 23, 2024
adam-james-v added a commit that referenced this issue May 31, 2024
Fixes: #39083

Multi-series dashcards now properly render when a subscription is set up with filters that impact the values of each
card in the multi-series card.
adam-james-v added a commit that referenced this issue Jun 6, 2024
)

* Multi Series Dashcards Render with Filters Applied To All Series

Fixes: #39083

Multi-series dashcards now properly render when a subscription is set up with filters that impact the values of each
card in the multi-series card.

* Small refactor for executing dashcards in a pulse context.

- move execute-dashboard-subscription to pulse.util
- this fn now will also execute all card series cards
  and put those results into a list on the key :series-results in the dashcard
  which is then used in the render multimethod to send the correct data to the javascript context

A small todo was taken care of as well, we can now use :dashboard-subscription in the :context key

* Fix up render fn

The shape of hte data going into the javascript was a bit unclear before. This keeps things a little more
understandable.

Next step is probably to create some Malli schemas inside the js_svg namespace to document and enforce the needed data shapes

* pull :data up a level from :result in the series cards

* Update src/metabase/pulse/render/body.clj

Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>

* Remove unneeded let

* set/rename-keys won't work here because we raise the :data key

I've realised a suggested change to just use set/rename-keys won't do what we need. We need instead to raise :data
from within the result key up to the same level (and dissoc result).

I've renamed the helper function to make its purpose a bit clearer

---------

Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>
github-automation-metabase pushed a commit that referenced this issue Jun 6, 2024
)

* Multi Series Dashcards Render with Filters Applied To All Series

Fixes: #39083

Multi-series dashcards now properly render when a subscription is set up with filters that impact the values of each
card in the multi-series card.

* Small refactor for executing dashcards in a pulse context.

- move execute-dashboard-subscription to pulse.util
- this fn now will also execute all card series cards
  and put those results into a list on the key :series-results in the dashcard
  which is then used in the render multimethod to send the correct data to the javascript context

A small todo was taken care of as well, we can now use :dashboard-subscription in the :context key

* Fix up render fn

The shape of hte data going into the javascript was a bit unclear before. This keeps things a little more
understandable.

Next step is probably to create some Malli schemas inside the js_svg namespace to document and enforce the needed data shapes

* pull :data up a level from :result in the series cards

* Update src/metabase/pulse/render/body.clj

Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>

* Remove unneeded let

* set/rename-keys won't work here because we raise the :data key

I've realised a suggested change to just use set/rename-keys won't do what we need. We need instead to raise :data
from within the result key up to the same level (and dissoc result).

I've renamed the helper function to make its purpose a bit clearer

---------

Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>
@adam-james-v adam-james-v added this to the 0.50 milestone Jun 6, 2024
github-automation-metabase added a commit that referenced this issue Jun 7, 2024
) (#43758)

* Multi Series Dashcards Render with Filters Applied To All Series

Fixes: #39083

Multi-series dashcards now properly render when a subscription is set up with filters that impact the values of each
card in the multi-series card.

* Small refactor for executing dashcards in a pulse context.

- move execute-dashboard-subscription to pulse.util
- this fn now will also execute all card series cards
  and put those results into a list on the key :series-results in the dashcard
  which is then used in the render multimethod to send the correct data to the javascript context

A small todo was taken care of as well, we can now use :dashboard-subscription in the :context key

* Fix up render fn

The shape of hte data going into the javascript was a bit unclear before. This keeps things a little more
understandable.

Next step is probably to create some Malli schemas inside the js_svg namespace to document and enforce the needed data shapes

* pull :data up a level from :result in the series cards

* Update src/metabase/pulse/render/body.clj



* Remove unneeded let

* set/rename-keys won't work here because we raise the :data key

I've realised a suggested change to just use set/rename-keys won't do what we need. We need instead to raise :data
from within the result key up to the same level (and dissoc result).

I've renamed the helper function to make its purpose a bit clearer

---------

Co-authored-by: adam-james <21064735+adam-james-v@users.noreply.github.com>
Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.Correctness Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness Reporting/Pulses Now called Subscriptions .Team/DashViz Dashboard and Viz team Type:Bug Product defects
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants