From 18be181946a3e5b7e651dde7a3e5fec46504dd08 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt <33317356+villebro@users.noreply.github.com> Date: Thu, 26 Aug 2021 06:24:33 +0300 Subject: [PATCH] fix(explore): update overwrite button on perm change (#16437) * fix(explore): update overwrite on perm change * remove redundant user_id prop * fix types * fix user type * fix tests * fix lint --- .../explore/components/SaveModal_spec.jsx | 8 +++++--- .../explore/components/ExploreViewContainer.jsx | 1 - .../explore/components/PropertiesModal/index.tsx | 2 +- .../src/explore/components/SaveModal.tsx | 16 +++++++++------- .../src/explore/reducers/exploreReducer.js | 1 + .../src/explore/reducers/getInitialState.ts | 13 ++++++++----- superset/connectors/sqla/models.py | 4 ++-- superset/models/slice.py | 4 +--- superset/views/core.py | 4 ---- 9 files changed, 27 insertions(+), 26 deletions(-) diff --git a/superset-frontend/spec/javascripts/explore/components/SaveModal_spec.jsx b/superset-frontend/spec/javascripts/explore/components/SaveModal_spec.jsx index 258c725bbc..b9d87eea6e 100644 --- a/superset-frontend/spec/javascripts/explore/components/SaveModal_spec.jsx +++ b/superset-frontend/spec/javascripts/explore/components/SaveModal_spec.jsx @@ -42,14 +42,16 @@ describe('SaveModal', () => { dashboards: [], }, explore: { - can_overwrite: true, - user_id: '1', datasource: {}, slice: { slice_id: 1, slice_name: 'title', + owners: [1], }, alert: null, + user: { + userId: 1, + }, }, }; const store = mockStore(initialState); @@ -104,7 +106,7 @@ describe('SaveModal', () => { it('disable overwrite option for non-owner', () => { const wrapperForNonOwner = getWrapper(); - wrapperForNonOwner.setProps({ can_overwrite: false }); + wrapperForNonOwner.setProps({ userId: 2 }); const overwriteRadio = wrapperForNonOwner.find('#overwrite-radio'); expect(overwriteRadio).toHaveLength(1); expect(overwriteRadio.prop('disabled')).toBe(true); diff --git a/superset-frontend/src/explore/components/ExploreViewContainer.jsx b/superset-frontend/src/explore/components/ExploreViewContainer.jsx index 27a6aa25ab..67682d12a8 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer.jsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer.jsx @@ -620,7 +620,6 @@ function mapStateToProps(state) { timeout: explore.common.conf.SUPERSET_WEBSERVER_TIMEOUT, ownState: dataMask[form_data.slice_id ?? 0]?.ownState, // 0 - unsaved chart impressionId, - userId: explore.user_id, user: explore.user, reports, }; diff --git a/superset-frontend/src/explore/components/PropertiesModal/index.tsx b/superset-frontend/src/explore/components/PropertiesModal/index.tsx index 0c46f6ba79..ab1e075a45 100644 --- a/superset-frontend/src/explore/components/PropertiesModal/index.tsx +++ b/superset-frontend/src/explore/components/PropertiesModal/index.tsx @@ -175,7 +175,7 @@ export default function PropertiesModal({ buttonStyle="primary" // @ts-ignore onClick={onSubmit} - disabled={!owners || submitting || !name} + disabled={submitting || !name} cta > {t('Save')} diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx index 5c93deb41c..e73a118ae4 100644 --- a/superset-frontend/src/explore/components/SaveModal.tsx +++ b/superset-frontend/src/explore/components/SaveModal.tsx @@ -34,11 +34,10 @@ const SK_DASHBOARD_ID = 'save_chart_recent_dashboard'; const SELECT_PLACEHOLDER = t('**Select** a dashboard OR **create** a new one'); type SaveModalProps = { - can_overwrite?: boolean; onHide: () => void; actions: Record; form_data?: Record; - userId: string; + userId: number; dashboards: Array; alert?: string; sliceName?: string; @@ -70,7 +69,7 @@ class SaveModal extends React.Component { saveToDashboardId: null, newSliceName: props.sliceName, alert: null, - action: props.can_overwrite ? 'overwrite' : 'saveas', + action: this.canOverwriteSlice() ? 'overwrite' : 'saveas', }; this.onDashboardSelectChange = this.onDashboardSelectChange.bind(this); this.onSliceNameChange = this.onSliceNameChange.bind(this); @@ -78,6 +77,10 @@ class SaveModal extends React.Component { this.saveOrOverwrite = this.saveOrOverwrite.bind(this); } + canOverwriteSlice(): boolean { + return this.props.slice?.owners?.includes(this.props.userId); + } + componentDidMount() { this.props.actions.fetchDashboards(this.props.userId).then(() => { const dashboardIds = this.props.dashboards.map( @@ -196,7 +199,7 @@ class SaveModal extends React.Component { disabled={!this.state.newSliceName} data-test="btn-modal-save" > - {!this.props.can_overwrite && this.props.slice + {!this.canOverwriteSlice() && this.props.slice ? t('Save as new chart') : t('Save')} @@ -225,7 +228,7 @@ class SaveModal extends React.Component { this.changeAction('overwrite')} data-test="save-overwrite-radio" @@ -289,8 +292,7 @@ function mapStateToProps({ return { datasource: explore.datasource, slice: explore.slice, - can_overwrite: explore.can_overwrite, - userId: explore.user_id, + userId: explore.user?.userId, dashboards: saveModal.dashboards, alert: saveModal.saveModalAlert, }; diff --git a/superset-frontend/src/explore/reducers/exploreReducer.js b/superset-frontend/src/explore/reducers/exploreReducer.js index d094b494ce..4c771971a0 100644 --- a/superset-frontend/src/explore/reducers/exploreReducer.js +++ b/superset-frontend/src/explore/reducers/exploreReducer.js @@ -219,6 +219,7 @@ export default function exploreReducer(state = {}, action) { slice: { ...state.slice, ...action.slice, + owners: action.slice.owners ?? null, }, sliceName: action.slice.slice_name ?? state.sliceName, }; diff --git a/superset-frontend/src/explore/reducers/getInitialState.ts b/superset-frontend/src/explore/reducers/getInitialState.ts index dea0ff7af4..28a270d2f9 100644 --- a/superset-frontend/src/explore/reducers/getInitialState.ts +++ b/superset-frontend/src/explore/reducers/getInitialState.ts @@ -22,7 +22,10 @@ import { ControlStateMapping, DatasourceMeta, } from '@superset-ui/chart-controls'; -import { CommonBootstrapData } from 'src/types/bootstrapTypes'; +import { + CommonBootstrapData, + UserWithPermissionsAndRoles, +} from 'src/types/bootstrapTypes'; import getToastsFromPyFlashMessages from 'src/messageToasts/utils/getToastsFromPyFlashMessages'; import { ChartState, Slice } from 'src/explore/types'; @@ -37,15 +40,15 @@ export interface ExlorePageBootstrapData extends JsonObject { can_add: boolean; can_download: boolean; can_overwrite: boolean; + common: CommonBootstrapData; datasource: DatasourceMeta; - form_data: QueryFormData; datasource_id: number; datasource_type: DatasourceType; + forced_height: string | null; + form_data: QueryFormData; slice: Slice | null; standalone: boolean; - user_id: number; - forced_height: string | null; - common: CommonBootstrapData; + user: UserWithPermissionsAndRoles; } export default function getInitialState( diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index c5c4662838..23ff4bb8dd 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -471,9 +471,9 @@ sqlatable_user = Table( ) -class SqlaTable( # pylint: disable=too-many-instance-attributes,too-many-public-methods +class SqlaTable( Model, BaseDatasource -): +): # pylint: disable=too-many-instance-attributes,too-many-public-methods """An ORM object for SqlAlchemy table references""" type = "table" diff --git a/superset/models/slice.py b/superset/models/slice.py index 28473d53d1..b4c7f6604f 100644 --- a/superset/models/slice.py +++ b/superset/models/slice.py @@ -201,9 +201,7 @@ class Slice( # pylint: disable=too-many-public-methods "form_data": self.form_data, "query_context": self.query_context, "modified": self.modified(), - "owners": [ - f"{owner.first_name} {owner.last_name}" for owner in self.owners - ], + "owners": [owner.id for owner in self.owners], "slice_id": self.id, "slice_name": self.slice_name, "slice_url": self.slice_url, diff --git a/superset/views/core.py b/superset/views/core.py index c538112937..29109f13c3 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -716,7 +716,6 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods def explore( # pylint: disable=too-many-locals self, datasource_type: Optional[str] = None, datasource_id: Optional[int] = None ) -> FlaskResponse: - user_id = g.user.get_id() if g.user else None form_data, slc = get_form_data(use_slice_data=True) query_context = request.form.get("query_context") # Flash the SIP-15 message if the slice is owned by the current user and has not @@ -843,14 +842,12 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods bootstrap_data = { "can_add": slice_add_perm, "can_download": slice_download_perm, - "can_overwrite": slice_overwrite_perm, "datasource": datasource_data, "form_data": form_data, "datasource_id": datasource_id, "datasource_type": datasource_type, "slice": slc.data if slc else None, "standalone": standalone_mode, - "user_id": user_id, "user": bootstrap_user_data(g.user, include_perms=True), "forced_height": request.args.get("height"), "common": common_bootstrap_payload(), @@ -1020,7 +1017,6 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods response = { "can_add": slice_add_perm, "can_download": slice_download_perm, - "can_overwrite": is_owner(slc, g.user), "form_data": slc.form_data, "slice": slc.data, "dashboard_url": dash.url if dash else None,