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: View Results Doesn't Match Chart After Refreshing Cache #19540

Conversation

diegomedina248
Copy link
Contributor

SUMMARY

When the data cache is configured, after a cache invalidation in the explore, the results view is not impacted.
So, if we modify the underlying dataset while using the explore, and we try to invalidate the cache, then the chart correctly displays the new data, but the view results doesn't.

This PR ensures that, after a cache invalidation, both the chart and results reflect the latest data.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

Screen.Recording.2022-04-05.at.20.33.35.mov

After:

new.mov

TESTING INSTRUCTIONS

  1. Create a table chart with raw records, pointing to a dataset that is easy to update (Gsheets for example)
  2. Add or remove some rows from your dataset
  3. Refresh the cache on the chart

Ensure that, after cache invalidation, both the chart and data results view reflect the latest data.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@diegomedina248 diegomedina248 force-pushed the fix/view-results-does-not-match-chart-after-refreshing-cache branch from 25d4280 to edf9773 Compare April 6, 2022 00:00
@codecov
Copy link

codecov bot commented Apr 6, 2022

Codecov Report

Merging #19540 (f3692a1) into master (ad71542) will increase coverage by 0.00%.
The diff coverage is 80.00%.

@@           Coverage Diff           @@
##           master   #19540   +/-   ##
=======================================
  Coverage   66.55%   66.55%           
=======================================
  Files        1692     1692           
  Lines       64801    64806    +5     
  Branches     6657     6657           
=======================================
+ Hits        43128    43132    +4     
- Misses      19973    19974    +1     
  Partials     1700     1700           
Flag Coverage Δ
javascript 51.25% <80.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rset-frontend/src/components/Chart/chartReducer.ts 25.00% <ø> (ø)
...ntend/src/explore/components/ExploreChartPanel.jsx 71.66% <ø> (ø)
...t-frontend/src/explore/reducers/getInitialState.ts 0.00% <ø> (ø)
...erset-frontend/src/components/Chart/chartAction.js 53.74% <71.42%> (-0.06%) ⬇️
...nd/src/explore/components/DataTablesPane/index.tsx 72.32% <100.00%> (+0.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad71542...f3692a1. Read the comment docs.

@zhaoyongjie zhaoyongjie self-requested a review April 6, 2022 06:26
Comment on lines 261 to 289
return getChartDataRequest({
formData: queryFormData,
resultFormat: 'json',
force: !!latestQueryCacheInvalidatedRef.current,
resultType,
ownState,
})
Copy link
Member

Choose a reason for hiding this comment

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

I remember that the force field is in the explore state, but I can not get a True value even though I clicked the cached button. Could we resolve this issue through change the force value?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, that force is used for something else, which is tracked in the ExploreViewContainer.
I also cannot find where in the exploreReducer that is being modified, but updating that value there will have other consequences

Copy link
Member

Choose a reason for hiding this comment

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

@diegomedina248 I pushed a branch in my repository to fix this issue. discussion welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zhaoyongjie looks good to me! feel free to close this PR once yours lands here. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

@zhaoyongjie were you indeed opening a PR to resolve this, or just providing some advice/examples?

Copy link
Member

Choose a reason for hiding this comment

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

@rusackas @diegomedina248 I will open a new PR today. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

opened PR at #19932.

@diegomedina248 diegomedina248 force-pushed the fix/view-results-does-not-match-chart-after-refreshing-cache branch from edf9773 to 083ec4b Compare April 21, 2022 20:10
@rusackas
Copy link
Member

rusackas commented May 3, 2022

Closing in favor of #19932

@rusackas rusackas closed this May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants