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: alert & reports active toggle optimistic update #20402

Conversation

diegomedina248
Copy link
Contributor

SUMMARY

For the alert and reports, the active toggle could lag and give the user the impression nothing is happening.
This PR just uses an optimistic update instead, and revert the change if the API fails.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

Screen.Recording.2022-06-15.at.16.50.44.mov

After:

new.mov

TESTING INSTRUCTIONS

  1. go to the alerts page
  2. toggle the alert on and off

The expected result is that the user sees the toggle pretty quickly.

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

@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Merging #20402 (3f3efb9) into master (86f146e) will increase coverage by 0.00%.
The diff coverage is 6.66%.

@@           Coverage Diff           @@
##           master   #20402   +/-   ##
=======================================
  Coverage   66.52%   66.53%           
=======================================
  Files        1739     1739           
  Lines       65141    65142    +1     
  Branches     6900     6900           
=======================================
+ Hits        43336    43343    +7     
+ Misses      20052    20046    -6     
  Partials     1753     1753           
Flag Coverage Δ
javascript 51.73% <6.66%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
superset-frontend/src/views/CRUD/hooks.ts 46.56% <0.00%> (-0.50%) ⬇️
...perset-frontend/src/views/CRUD/alert/AlertList.tsx 62.50% <10.00%> (-3.16%) ⬇️
...gins/plugin-chart-echarts/src/Pie/controlPanel.tsx 28.57% <0.00%> (-4.77%) ⬇️
...c/views/CRUD/data/database/DatabaseModal/index.tsx 32.16% <0.00%> (-0.18%) ⬇️
...frontend/src/SqlLab/components/SqlEditor/index.jsx 51.36% <0.00%> (ø)
.../src/views/CRUD/data/savedquery/SavedQueryList.tsx 63.30% <0.00%> (ø)
...dashboard/components/SliceHeaderControls/index.tsx 64.28% <0.00%> (ø)
...qlLab/components/EstimateQueryCostButton/index.tsx 5.26% <0.00%> (ø)
...es/superset-ui-core/src/query/buildQueryContext.ts 100.00% <0.00%> (ø)
...ugin-chart-echarts/src/Timeseries/controlPanel.tsx 28.57% <0.00%> (ø)
... and 16 more

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 86f146e...3f3efb9. Read the comment docs.


updateResource(update_id, { active: checked }, false, false)
.then()
.catch(() => setResourceCollection(original));
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we display a toast for the user like "An error occurred while changing the active state"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are. The updateResource comes from the useSingleViewResource hook, which accepts a function to handle errors and we're passing addDangerToast to it.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM

@michael-s-molina michael-s-molina merged commit 4dc3044 into apache:master Jun 23, 2022
michael-s-molina pushed a commit that referenced this pull request Jun 23, 2022
michael-s-molina pushed a commit that referenced this pull request Jun 28, 2022
@mistercrunch mistercrunch added 🍒 2.0.0 🍒 2.0.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels Preset-Patch size/M v2.0 🍒 2.0.0 🍒 2.0.1 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants