Don't rerun dashcard queries when switching tabs without parameter changes #40783
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kulyk
added
support/Repro
backport
ranquild
approved these changes
Mar 29, 2024
uladzimirdev
approved these changes
Mar 29, 2024
| cy.wait("@dashcardQuery"); | ||
| cy.get("@dashcardQuery.all").should("have.length", 3); | ||
|
|
||
| // Rerun 2nd tab query with new parameters |
There was a problem hiding this comment.
nit: instead of comments, use cy.log
| // Rerun 1st tab query with new parameters | ||
| setDateFilter(); | ||
| cy.wait("@dashcardQuery"); | ||
| cy.get("@dashcardQuery.all").should("have.length", 3); |
There was a problem hiding this comment.
nit: I found that way of testing number of requests as very unreliable (number can vary from run to run), but I hope it works in your case
|
kulyk
force-pushed
the
39863-fix-redundant-query-reruns
branch
from
1f1bb41
to
ee9decb
Compare
|
@metabase-bot backport release-x.48.x |
|
@kulyk Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
github-actions bot
pushed a commit
that referenced
this pull request
Mar 29, 2024
…anges (#40783) * Add E2E test * Move `getDatasetQueryParams` to `data-fetching.js` * Use `null` as a parameter value when missing * Fix missing default `datasetQuery`
github-actions bot
pushed a commit
that referenced
this pull request
Mar 29, 2024
…anges (#40783) * Add E2E test * Move `getDatasetQueryParams` to `data-fetching.js` * Use `null` as a parameter value when missing * Fix missing default `datasetQuery`
metabase-bot bot
added a commit
that referenced
this pull request
Mar 30, 2024
…t parameter changes" (#40824) * Don't rerun dashcard queries when switching tabs without parameter changes (#40783) * Add E2E test * Move `getDatasetQueryParams` to `data-fetching.js` * Use `null` as a parameter value when missing * Fix missing default `datasetQuery` * Fix E2E test --------- Co-authored-by: Anton Kulyk <kuliks.anton@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #39863
There are a few places in the
fetchCardDatadashboard action where we're checking if queries or parameters have changed since the last run (here is an example) to decide if we need to refresh data. For versions 49 and lower, there's an unhandled case with parameters without a value. They don't have avalueproperty in the last run object, but they havevalue: nullin the other object we're using for comparison. We triggerfetchCardDatawhen switching the dashboard tab, and this comparison logic should prevent us from redundant reruns when neither queries or parameters change, but this wasn't working as expected.On
masterthis was fixed by #35465, the BE started to returnvalue: nullin query responses. This PR adds an E2E test to ensure the problem won't come back and adds a FE safety net (to be backported to 48 and 49)To reproduce