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

[dashboard] Fix save issue at Force_V2_Edit mode #5360

Merged

Conversation

graceguo-supercat
Copy link

@graceguo-supercat graceguo-supercat commented Jul 7, 2018

screen shot 2018-07-05 at 3 06 00 pm

Fix 2 issues for dashboard when set config
CAN_FALLBACK_TO_DASH_V1_EDIT_MODE = False

if dashboard is still in v1 mode:

  • for dashboard owner/viewer, can see/edit dashboard in v1 mode.
  • for dashboard owner/viewer, cannot save/save-as dashboard to v1 mode.
  • for dashboard owner, if they click on Edit dashboard button in V1 mode, we will show dashboard in v2 preview mode, and user can convert and save dashboard to v2 mode.

@@ -144,6 +144,7 @@ class Controls extends React.PureComponent {
}
/>
{dashboard.dash_save_perm &&
dashboard.force_v2_edit &&
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be negated? !dashboard.force_v2_edit

Copy link
Author

@graceguo-supercat graceguo-supercat Jul 7, 2018

Choose a reason for hiding this comment

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

force_v2_edit is true when CAN_FALLBACK_TO_DASH_V1_EDIT_MODE is false. Here is to overrule user level save_perm and hide save button.

@@ -43,25 +43,11 @@ class Header extends React.PureComponent {
showV2PromptModal: props.dashboard.promptV2Conversion,
};
this.toggleShowV2PromptModal = this.toggleShowV2PromptModal.bind(this);
this.handleConvertToV2 = this.handleConvertToV2.bind(this);
this.handleConvertToV2 = props.handleConvertToV2.bind(this);
Copy link
Contributor

@williaster williaster Jul 7, 2018

Choose a reason for hiding this comment

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

why do you need to bind a props func? it's already bound at the dashboard level right?

Copy link
Author

Choose a reason for hiding this comment

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

handleConvertToV2 is refactored out into Dashboard component

Copy link
Contributor

Choose a reason for hiding this comment

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

right but you don't need to bind a function that comes from props.

Copy link
Author

Choose a reason for hiding this comment

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

yes.... fixed.

@@ -2169,6 +2169,8 @@ def dashboard(self, dashboard_id):
else:
dashboard_view = 'v1'
prompt_v2_conversion = not force_v1
if force_v2_edit:
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed for removing the save as dropdown option in a v1 app? Does this still allow you to click edit when viewing a v1 dashboard to go to the v2 page?

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

thanks for working on a fix for this! I had a couple questions.

@codecov-io
Copy link

codecov-io commented Jul 7, 2018

Codecov Report

Merging #5360 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5360      +/-   ##
==========================================
- Coverage   61.32%   61.31%   -0.01%     
==========================================
  Files         369      369              
  Lines       23494    23497       +3     
  Branches     2714     2715       +1     
==========================================
  Hits        14407    14407              
- Misses       9075     9078       +3     
  Partials       12       12
Impacted Files Coverage Δ
.../src/dashboard/deprecated/v1/components/Header.jsx 0% <ø> (ø) ⬆️
...c/dashboard/deprecated/v1/components/Dashboard.jsx 0% <0%> (ø) ⬆️
superset/views/core.py 72.86% <0%> (-0.11%) ⬇️
...rc/dashboard/deprecated/v1/components/Controls.jsx 0% <0%> (ø) ⬆️

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 7222c6f...b392e6a. Read the comment docs.

}
handleSaveTitle(title) {
this.props.updateDashboardTitle(title);
}
handleConvertToV2(editMode) {
Copy link
Author

Choose a reason for hiding this comment

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

here, handleConvertToV2 is refactored out into Dashboard component, since this function is called from both Dashboard and Header component.

@@ -2169,6 +2169,8 @@ def dashboard(self, dashboard_id):
else:
dashboard_view = 'v1'
prompt_v2_conversion = not force_v1
if force_v2_edit:
dash_edit_perm = False
Copy link
Author

@graceguo-supercat graceguo-supercat Jul 7, 2018

Choose a reason for hiding this comment

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

this line disallow owner to save original v1 dashboard copy. Edit dashboard button in Header controls used dash_save_perm, which is not affected.

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

nice! thanks again for looking at this 🙌

@graceguo-supercat graceguo-supercat merged commit a17f714 into apache:master Jul 7, 2018
@graceguo-supercat graceguo-supercat deleted the gg-DisableDashV1Edit branch July 7, 2018 03:53
graceguo-supercat pushed a commit to graceguo-supercat/superset that referenced this pull request Jul 9, 2018
xumiao pushed a commit to xumiao/supernorm that referenced this pull request Jul 16, 2018
timifasubaa pushed a commit to airbnb/superset-fork that referenced this pull request Jul 25, 2018
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Jul 27, 2018
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Jul 31, 2018
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Aug 3, 2018
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Aug 3, 2018
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Aug 4, 2018
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Aug 4, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🍒 0.27.0 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 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 🍒 0.27.0 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants