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.

Sign up for GitHub

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: preventing save button from flickering in SQL Lab #25106

Merged
merged 6 commits into from
Sep 26, 2023
Merged

fix: preventing save button from flickering in SQL Lab #25106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9b95ca8
preventing save button from rendering prior to evaluating canExploreD…
fisjac Aug 25, 2023
cc6ee44
changing null to false
fisjac Aug 25, 2023
2aa0080
adding back hide logic after erroneously removed
fisjac Aug 25, 2023
97a92a4
adjusting logic to prevent render only when allows_virtual_table_expl…
fisjac Aug 31, 2023
513f8eb
fixing conditional expect error in test case
fisjac Aug 31, 2023
57327bf
changing canShowSaveButton to shouldShowSaveButton
fisjac Sep 2, 2023
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
21 changes: 20 additions & 1 deletion superset-frontend/src/SqlLab/components/SaveQuery/SaveQuery.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import { initialState, databases } from 'src/SqlLab/fixtures';
const mockedProps = {
queryEditorId: '123',
animation: false,
database: databases.result[0],
database: { ...databases.result[0], allows_virtual_table_explore: false },
onUpdate: () => {},
onSave: () => {},
saveQueryWarning: null,
Expand Down Expand Up @@ -61,6 +61,25 @@ const middlewares = [thunk];
const mockStore = configureStore(middlewares);

describe('SavedQuery', () => {
it('doesnt render save button when allows_virtual_table_explore is undefined', async () => {
const noRenderProps = {
...mockedProps,
database: {
...mockedProps.database,
allows_virtual_table_explore: undefined,
},
};
render(<SaveQuery {...noRenderProps} />, {
useRedux: true,
store: mockStore(mockState),
});
expect(() => {
screen.getByRole('button', { name: /save/i });
}).toThrow(
'Unable to find an accessible element with the role "button" and name `/save/i`',
);
});

it('renders a non-split save button when allows_virtual_table_explore is not enabled', () => {
render(<SaveQuery {...mockedProps} />, {
useRedux: true,
Expand Down
12 changes: 8 additions & 4 deletions superset-frontend/src/SqlLab/components/SaveQuery/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ const SaveQuery = ({
const [showSaveDatasetModal, setShowSaveDatasetModal] = useState(false);
const isSaved = !!query.remoteId;
const canExploreDatabase = !!database?.allows_virtual_table_explore;
const shouldShowSaveButton =
database?.allows_virtual_table_explore !== undefined;
Copy link
Member

Choose a reason for hiding this comment

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

what should the value of canShowSaveButton be if database is null or database.allows_virtual_table_explore is False?

Copy link
Member

Choose a reason for hiding this comment

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

Basically, what's the difference between canExploreDatabase and canShowSaveButton? Also, just a naming nit, I would change canShowSaveButton to something like shouldShowSaveButton, to signify that 1) it's a boolean like you have it, but also 2) that it's not related to permissions but rather state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

database should always be set since the parent component returns an component if database is not defined.
The allows_virtual_table_explore property is added to the store when dispatch(getDatabases) action is called inside of the , and then the component gets rerendered with databases.allows_virtual_table_explore as a boolean.

While the getDatabases action is awaiting a response databases.allows_virtual_table_explore will be undefined, and canShowSaveButton prevents the save button from being rendered. Once the additional databases data has been added to the redux store, the save button will either render a split button, or the original button based upon the value canExploreDatabase.


const overlayMenu = (
<Menu>
Expand Down Expand Up @@ -180,10 +182,12 @@ const SaveQuery = ({

return (
<Styles className="SaveQuery">
<SaveDatasetActionButton
setShowSave={setShowSave}
overlayMenu={canExploreDatabase ? overlayMenu : null}
/>
{shouldShowSaveButton && (
<SaveDatasetActionButton
setShowSave={setShowSave}
overlayMenu={canExploreDatabase ? overlayMenu : null}
/>
)}
<SaveDatasetModal
visible={showSaveDatasetModal}
onHide={() => setShowSaveDatasetModal(false)}
Expand Down