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

chore(sqllab): Relocate user in SqlLab to root #25010

Merged
merged 6 commits into from
Aug 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 2 additions & 11 deletions superset-frontend/src/SqlLab/App.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
isFeatureEnabled,
} from '@superset-ui/core';
import { GlobalStyles } from 'src/GlobalStyles';
import { setupStore } from 'src/views/store';
import { setupStore, userReducer } from 'src/views/store';
import setupExtensions from 'src/setup/setupExtensions';
import getBootstrapData from 'src/utils/getBootstrapData';
import { tableApiUtil } from 'src/hooks/apiResources/tables';
Expand Down Expand Up @@ -78,12 +78,6 @@ const sqlLabPersistStateConfig = {
}
});

if (subset.sqlLab?.user) {
// Don't persist the user.
// User should really not be stored under the "sqlLab" field. Oh well.
delete subset.sqlLab.user;
}

const data = JSON.stringify(subset);
// 2 digit precision
const currentSize =
Expand All @@ -105,17 +99,14 @@ const sqlLabPersistStateConfig = {
...initialState.sqlLab,
},
};
// Filter out any user data that may have been persisted in an older version.
Copy link
Member

Choose a reason for hiding this comment

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

Should we also remove?

if (subset.sqlLab?.user) {
  // Don't persist the user.
  // User should really not be stored under the "sqlLab" field. Oh well.
  delete subset.sqlLab.user;
}

// Get user from bootstrap data instead, every time
result.sqlLab.user = initialState.sqlLab.user;
return result;
},
},
};

export const store = setupStore({
initialState,
rootReducers: reducers,
rootReducers: { ...reducers, user: userReducer },
Copy link
Member

Choose a reason for hiding this comment

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

Could you add the user reducer to src/SqlLab/reducers/index.js instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

First I intended to do so but SqlLab/reducers/index.js will be consolidated to SPA store which is already has the userReducer (as well as it will make a circular ref issue)
I just add like this since this will be gone after SPA work.

...(!isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE) && {
enhancers: [
persistState(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ describe('QueryTable', () => {
it('renders a proper table', () => {
const mockStore = configureStore([thunk]);
const store = mockStore({
sqlLab: user,
user,
});

const wrapper = mount(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ const QueryTable = ({
[columns],
);

const user = useSelector<SqlLabRootState, User>(state => state.sqlLab.user);
const user = useSelector<SqlLabRootState, User>(state => state.user);

const data = useMemo(() => {
const restoreSql = (query: QueryResponse) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ import {
DatasetRadioState,
EXPLORE_CHART_DEFAULT,
DatasetOwner,
SqlLabExploreRootState,
getInitialState,
SqlLabRootState,
} from 'src/SqlLab/types';
import { mountExploreUrl } from 'src/explore/exploreUtils';
Expand Down Expand Up @@ -177,9 +175,7 @@ export const SaveDatasetModal = ({
>(undefined);
const [loading, setLoading] = useState<boolean>(false);

const user = useSelector<SqlLabExploreRootState, User>(user =>
getInitialState(user),
);
const user = useSelector<SqlLabRootState, User>(state => state.user);
const dispatch = useDispatch<(dispatch: any) => Promise<JsonObject>>();

const createWindow = (url: string) => {
Expand Down
4 changes: 2 additions & 2 deletions superset-frontend/src/SqlLab/components/SouthPane/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ const SouthPane = ({
const dispatch = useDispatch();

const { editorQueries, dataPreviewQueries, databases, offline, user } =
useSelector(({ sqlLab }: SqlLabRootState) => {
const { databases, offline, user, queries, tables } = sqlLab;
useSelector(({ user, sqlLab }: SqlLabRootState) => {
const { databases, offline, queries, tables } = sqlLab;
const dataPreviewQueries = tables
.filter(
({ dataPreviewQueryId, queryEditorId: qeId }) =>
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/SqlLab/fixtures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -660,10 +660,10 @@ export const initialState = {
workspaceQueries: [],
queriesLastUpdate: 0,
activeSouthPaneTab: 'Results',
user: { user },
unsavedQueryEditor: {},
},
messageToasts: [],
user,
common: {
conf: {
DEFAULT_SQLLAB_LIMIT: 1000,
Expand Down
4 changes: 1 addition & 3 deletions superset-frontend/src/SqlLab/reducers/getInitialState.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ export default function getInitialState({
tab_state_ids: tabStateIds = [],
databases,
queries: queries_,
requested_query: requestedQuery,
user,
}) {
/**
Expand Down Expand Up @@ -200,11 +199,9 @@ export default function getInitialState({
tabHistory: dedupeTabHistory(tabHistory),
tables: Object.values(tables),
queriesLastUpdate: Date.now(),
user,
unsavedQueryEditor,
queryCostEstimates: {},
},
requestedQuery,
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove requestedQuery since it's no longer used by #12086

messageToasts: getToastsFromPyFlashMessages(
(common || {}).flash_messages || [],
),
Expand All @@ -213,5 +210,6 @@ export default function getInitialState({
flash_messages: common.flash_messages,
conf: common.conf,
},
user,
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ const apiDataWithTabState = {
};
describe('getInitialState', () => {
it('should output the user that is passed in', () => {
expect(getInitialState(apiData).sqlLab.user.userId).toEqual(1);
expect(getInitialState(apiData).user.userId).toEqual(1);
});
it('should return undefined instead of null for templateParams', () => {
expect(
Expand Down
18 changes: 2 additions & 16 deletions superset-frontend/src/SqlLab/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import { JsonObject, QueryResponse } from '@superset-ui/core';
import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';
import { ToastType } from 'src/components/MessageToasts/types';
import { RootState } from 'src/dashboard/types';
import { DropdownButtonProps } from 'src/components/DropdownButton';
import { ButtonProps } from 'src/components/Button';

Expand Down Expand Up @@ -66,33 +65,20 @@ export type SqlLabRootState = {
tabHistory: string[]; // default is activeTab ? [activeTab.id.toString()] : []
tables: Record<string, any>[];
queriesLastUpdate: number;
user: UserWithPermissionsAndRoles;
errorMessage: string | null;
unsavedQueryEditor: Partial<QueryEditor>;
queryCostEstimates?: Record<string, QueryCostEstimate>;
editorTabLastUpdatedAt?: number;
};
localStorageUsageInKilobytes: number;
messageToasts: toastState[];
user: UserWithPermissionsAndRoles;
common: {
flash_messages: string[];
conf: JsonObject;
};
};

export type SqlLabExploreRootState = SqlLabRootState | RootState;
Copy link
Member Author

@justinpark justinpark Aug 16, 2023

Choose a reason for hiding this comment

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

@lyndsiWilliams I removed this user selector logic since it's under the same path.


export const getInitialState = (state: SqlLabExploreRootState) => {
if (state.hasOwnProperty('sqlLab')) {
const {
sqlLab: { user },
} = state as SqlLabRootState;
return user;
}

const { user } = state as RootState;
return user as UserWithPermissionsAndRoles;
};

export enum DatasetRadioState {
SAVE_NEW = 1,
OVERWRITE_DATASET = 2,
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/views/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export type UserLoadedAction = {
user: UserWithPermissionsAndRoles;
};

const userReducer = (
export const userReducer = (
user = bootstrapData.user || {},
action: UserLoadedAction,
): BootstrapUser | UndefinedUser => {
Expand Down
Loading