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: should update last_modified_time in client-side after save dash #11305

Merged

Conversation

graceguo-supercat
Copy link

SUMMARY

This PR is to fix an issue related to #11220 . To reproduce:

  1. open a dashboard and edit,
  2. save change, successfully,
  3. in the same browser window, edit same dashboard again,
  4. when you try to save the 2nd change, you will see error message.

The root cause is, after user's first update, the client-side still hold the previous value for dashboard last_updated_time. On user submit the second edit, server-side compare the last_updated_time with newer one and will reject the 2nd edit request.

This PR adds solution for this issue: update last_updated_time in client-side when the first save is successful.

TEST PLAN

CI and new unit test.

ADDITIONAL INFORMATION

@@ -138,6 +139,7 @@ class HeaderActionsDropdown extends React.PureComponent {
isLoading,
refreshLimit,
refreshWarning,
lastModifiedTime,
Copy link
Member

Choose a reason for hiding this comment

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

This thing is getting a little too bloated. I'm wondering whether we can refactor and just pass a dashboard object to this component.

Copy link
Author

Choose a reason for hiding this comment

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

yes. dashboardInfo is read only dashboard metadata. I will consider group other edit-able dashboard metadata into another object.

@codecov-io
Copy link

codecov-io commented Oct 16, 2020

Codecov Report

Merging #11305 into master will decrease coverage by 1.33%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11305      +/-   ##
==========================================
- Coverage   62.71%   61.38%   -1.34%     
==========================================
  Files         441      834     +393     
  Lines       14721    39562   +24841     
  Branches     3610     3610              
==========================================
+ Hits         9233    24286   +15053     
- Misses       5307    15095    +9788     
  Partials      181      181              
Flag Coverage Δ
#javascript 62.71% <ø> (ø)
#python 60.59% <ø> (?)

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

Impacted Files Coverage Δ
...erset-frontend/src/dashboard/components/Header.jsx 35.65% <ø> (ø)
...src/dashboard/components/HeaderActionsDropdown.jsx 62.22% <ø> (ø)
...et-frontend/src/dashboard/components/SaveModal.jsx 40.90% <ø> (ø)
...ntend/src/dashboard/containers/DashboardHeader.jsx 100.00% <ø> (ø)
...-frontend/src/dashboard/reducers/dashboardState.js 74.54% <ø> (ø)
...frontend/src/dashboard/reducers/getInitialState.js 0.00% <ø> (ø)
superset/utils/dashboard_import_export.py 60.71% <0.00%> (ø)
superset/views/filters.py 100.00% <0.00%> (ø)
...et/migrations/versions/12d55656cbca_is_featured.py 0.00% <0.00%> (ø)
superset/examples/birth_names.py 97.59% <0.00%> (ø)
... and 389 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 9856f99...4109ead. Read the comment docs.

@graceguo-supercat graceguo-supercat merged commit 8863c93 into apache:master Oct 16, 2020
graceguo-supercat pushed a commit to airbnb/superset-fork that referenced this pull request Oct 16, 2020
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 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 size/M 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants