michael-s-molina commented on code in PR #20448:
URL: https://github.com/apache/superset/pull/20448#discussion_r902960704
##########
superset-frontend/src/dashboard/util/getDashboardPermission.ts:
##########
@@ -16,24 +16,13 @@
* specific language governing permissions and limitations
* under the License.
*/
-import memoizeOne from 'memoize-one';
import {
- UserRoles,
isUserWithPermissionsAndRoles,
UndefinedUser,
UserWithPermissionsAndRoles,
} from 'src/types/bootstrapTypes';
import Dashboard from 'src/types/Dashboard';
-
-const findPermission = memoizeOne(
- (perm: string, view: string, roles?: UserRoles | null) =>
- !!roles &&
- Object.values(roles).some(permissions =>
- permissions.some(([perm_, view_]) => perm_ === perm && view_ === view),
- ),
-);
-
-export default findPermission;
+import { findPermission } from 'src/utils/findPermission';
// this should really be a config value,
Review Comment:
I think a better name for this file would be something like `permissions.ts`
or `permissionUtils.ts` because it exports a bunch of permission-related
functions. `getDashboardPermission` looks like it only exports that function.
##########
superset-frontend/src/dashboard/components/Header/index.jsx:
##########
@@ -31,6 +31,7 @@ import {
import Icons from 'src/components/Icons';
import Button from 'src/components/Button';
import { AntdButton } from 'src/components/';
+import { findPermission } from 'src/utils/findPermission';
Review Comment:
Nice! Way better off the dashboard folder!
##########
superset-frontend/src/explore/actions/datasourcesActions.test.ts:
##########
@@ -0,0 +1,59 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { DatasourceType } from '@superset-ui/core';
+import * as actions from 'src/explore/actions/datasourcesActions';
+import datasourcesReducer from '../reducers/datasourcesReducer';
+
+const defaultState = {
+ '1__table': {
+ id: 1,
+ uid: '1__table',
+ type: DatasourceType.Table,
+ columns: [],
+ metrics: [],
+ column_format: {},
+ verbose_map: {},
+ main_dttm_col: '__timestamp',
+ // eg. ['["ds", true]', 'ds [asc]']
+ datasource_name: 'test datasource',
+ description: null,
+ },
+};
+
+describe('reducers', () => {
+ it('sets new datasource', () => {
+ const NEW_DATASOURCE = {
+ id: 2,
+ type: DatasourceType.Table,
+ columns: [],
+ metrics: [],
+ column_format: {},
+ verbose_map: {},
+ main_dttm_col: '__timestamp',
+ // eg. ['["ds", true]', 'ds [asc]']
+ datasource_name: 'test datasource',
+ description: null,
+ };
+ const newState = datasourcesReducer(
+ defaultState,
+ actions.setDatasource(NEW_DATASOURCE),
Review Comment:
```suggestion
setDatasource(NEW_DATASOURCE),
```
##########
superset-frontend/src/explore/actions/datasourcesActions.test.ts:
##########
@@ -0,0 +1,59 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { DatasourceType } from '@superset-ui/core';
+import * as actions from 'src/explore/actions/datasourcesActions';
Review Comment:
I think is a good practice to explicitly declare the dependencies.
```suggestion
import { setDatasource } from 'src/explore/actions/datasourcesActions';
```
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -60,6 +60,7 @@ import {
LOG_ACTIONS_CHANGE_EXPLORE_CONTROLS,
} from '../../../logger/LogUtils';
import ConnectedExploreChartHeader from '../ExploreChartHeader';
+import { datasourcesActions } from '../../actions/datasourcesActions';
Review Comment:
Can we use the full path?
##########
superset-frontend/src/explore/actions/datasourcesActions.test.ts:
##########
@@ -0,0 +1,59 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { DatasourceType } from '@superset-ui/core';
+import * as actions from 'src/explore/actions/datasourcesActions';
+import datasourcesReducer from '../reducers/datasourcesReducer';
+
+const defaultState = {
+ '1__table': {
+ id: 1,
+ uid: '1__table',
+ type: DatasourceType.Table,
+ columns: [],
+ metrics: [],
+ column_format: {},
+ verbose_map: {},
+ main_dttm_col: '__timestamp',
+ // eg. ['["ds", true]', 'ds [asc]']
+ datasource_name: 'test datasource',
+ description: null,
+ },
+};
+
+describe('reducers', () => {
Review Comment:
[Avoid nesting when you're
testing](https://github.com/apache/superset/wiki/Testing-Guidelines-and-Best-Practices#avoid-nesting-when-youre-testing)
##########
superset-frontend/src/explore/reducers/index.js:
##########
@@ -17,22 +17,41 @@
* under the License.
*/
import { combineReducers } from 'redux';
+import shortid from 'shortid';
+import { bootstrapData } from 'src/preamble';
import reports from 'src/reports/reducers/reports';
import charts from 'src/components/Chart/chartReducer';
import dataMask from 'src/dataMask/reducer';
import messageToasts from 'src/components/MessageToasts/reducers';
+import datasources from './datasourcesReducer';
import saveModal from './saveModalReducer';
import explore from './exploreReducer';
-const impressionId = (state = '') => state;
+// noopReducer, userReducer temporarily copied from src/views/store.ts
+// when SPA work is done, we'll be able to reuse those instead of copying
Review Comment:
```suggestion
// TODO: when SPA work is done, we'll be able to reuse those instead of
copying
```
##########
superset-frontend/src/explore/reducers/getInitialState.ts:
##########
@@ -32,13 +37,17 @@ import {
getFormDataFromControls,
applyMapStateToPropsToControl,
} from 'src/explore/controlUtils';
+import { findPermission } from '../../utils/findPermission';
+import { getDatasourceUid } from '../../utils/getDatasourceUid';
+import { getUrlParam } from '../../utils/urlUtils';
+import { URL_PARAMS } from '../../constants';
Review Comment:
```suggestion
import { findPermission } from 'src/utils/findPermission';
import { getDatasourceUid } from 'src/utils/getDatasourceUid';
import { getUrlParam } from 'src/utils/urlUtils';
import { URL_PARAMS } from 'src/constants';
```
##########
superset-frontend/src/utils/findPermission.test.ts:
##########
@@ -0,0 +1,67 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { findPermission } from './findPermission';
+
+describe('findPermission', () => {
Review Comment:
[Avoid nesting when you're
testing](https://github.com/apache/superset/wiki/Testing-Guidelines-and-Best-Practices#avoid-nesting-when-youre-testing)
##########
superset-frontend/src/explore/reducers/getInitialState.ts:
##########
@@ -32,13 +37,17 @@ import {
getFormDataFromControls,
applyMapStateToPropsToControl,
} from 'src/explore/controlUtils';
+import { findPermission } from '../../utils/findPermission';
+import { getDatasourceUid } from '../../utils/getDatasourceUid';
+import { getUrlParam } from '../../utils/urlUtils';
+import { URL_PARAMS } from '../../constants';
export interface ExplorePageBootstrapData extends JsonObject {
can_add: boolean;
can_download: boolean;
can_overwrite: boolean;
common: CommonBootstrapData;
- datasource: Dataset;
+ datasource: Dataset & { uid?: string };
Review Comment:
Could the `uid` be part of the `Dataset`?
##########
superset-frontend/src/explore/reducers/getInitialState.ts:
##########
@@ -52,40 +61,49 @@ export interface ExplorePageBootstrapData extends
JsonObject {
export default function getInitialState(
bootstrapData: ExplorePageBootstrapData,
) {
- const { form_data: initialFormData } = bootstrapData;
- const { slice } = bootstrapData;
- const sliceName = slice ? slice.slice_name : null;
+ const {
+ form_data: initialFormData,
+ common,
+ user,
+ datasource,
+ slice,
+ } = bootstrapData;
const exploreState = {
// note this will add `form_data` to state,
// which will be manipulatable by future reducers.
- ...bootstrapData,
- sliceName,
- common: {
- flash_messages: bootstrapData.common.flash_messages,
- conf: bootstrapData.common.conf,
- },
+ can_add: findPermission('can_write', 'Chart', user?.roles),
+ can_download: findPermission('can_csv', 'Superset', user?.roles),
+ can_overwrite: ensureIsArray(slice?.owners).includes(
+ user?.userId as number,
Review Comment:
Shouldn't this be something like `user?.userId || 0` since the user can be
`undefined`?
##########
superset-frontend/src/explore/reducers/exploreReducer.js:
##########
@@ -49,17 +49,18 @@ export default function exploreReducer(state = {}, action) {
isDatasourceMetaLoading: true,
};
},
- [actions.SET_DATASOURCE]() {
+ [actions.UPDATE_FORM_DATA_BY_DATASOURCE]() {
const newFormData = { ...state.form_data };
- if (action.datasource.type !== state.datasource.type) {
- if (action.datasource.type === 'table') {
- newFormData.granularity_sqla = action.datasource.granularity_sqla;
- newFormData.time_grain_sqla = action.datasource.time_grain_sqla;
+ if (action.prevDatasource.type !== action.newDatasource.type) {
Review Comment:
Here we could extract `prevDatasource` and `newDatasource` as constants to
improve readability like:
`const { prevDatasource, newDatasource } = action;`
##########
superset-frontend/src/utils/getDatasourceUid.test.ts:
##########
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { DatasourceType } from '@superset-ui/core';
+import { getDatasourceUid } from './getDatasourceUid';
+
+const TEST_DATASOURCE = {
+ id: 2,
+ type: DatasourceType.Table,
+ columns: [],
+ metrics: [],
+ column_format: {},
+ verbose_map: {},
+ main_dttm_col: '__timestamp',
+ // eg. ['["ds", true]', 'ds [asc]']
+ datasource_name: 'test datasource',
+ description: null,
+};
+
+const TEST_DATASOURCE_WITH_UID = {
+ ...TEST_DATASOURCE,
+ uid: 'dataset_uid',
+};
+
+test('creates uid from id and type when dataset doesnt have uid field', () => {
Review Comment:
```suggestion
test('creates uid from id and type when dataset does not have uid field', ()
=> {
```
##########
superset-frontend/src/utils/getDatasourceUid.test.ts:
##########
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { DatasourceType } from '@superset-ui/core';
+import { getDatasourceUid } from './getDatasourceUid';
+
+const TEST_DATASOURCE = {
+ id: 2,
+ type: DatasourceType.Table,
+ columns: [],
+ metrics: [],
+ column_format: {},
+ verbose_map: {},
+ main_dttm_col: '__timestamp',
+ // eg. ['["ds", true]', 'ds [asc]']
+ datasource_name: 'test datasource',
+ description: null,
+};
+
+const TEST_DATASOURCE_WITH_UID = {
+ ...TEST_DATASOURCE,
+ uid: 'dataset_uid',
+};
+
+test('creates uid from id and type when dataset doesnt have uid field', () => {
+ expect(getDatasourceUid(TEST_DATASOURCE)).toEqual('2__table');
+});
+
+test('returns uid when dataset has uid field', () => {
+ expect(getDatasourceUid(TEST_DATASOURCE_WITH_UID)).toEqual('dataset_uid');
Review Comment:
```suggestion
expect(getDatasourceUid(TEST_DATASOURCE_WITH_UID)).toEqual(TEST_DATASOURCE_WITH_UID.uid);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
