You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "hughhhh (via GitHub)" <gi...@apache.org> on 2023/08/29 16:29:20 UTC
[GitHub] [superset] hughhhh opened a new pull request, #25112: fix: permalink save/overwrites in explore
hughhhh opened a new pull request, #25112:
URL: https://github.com/apache/superset/pull/25112
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
When using a permalink, users will be redirected to a bad url with no context. Now we've updated the logic to always add the slice id before redirecting
Related PR: https://github.com/apache/superset/pull/23627
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on a diff in pull request #25112: fix: permalink save/overwrites in explore
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #25112:
URL: https://github.com/apache/superset/pull/25112#discussion_r1320540913
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -265,10 +265,10 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
searchParams.set('save_action', this.state.action);
if (this.state.action !== 'overwrite') {
searchParams.delete('form_data_key');
- }
- if (this.state.action === 'saveas') {
+ } else {
searchParams.set('slice_id', value.id.toString());
Review Comment:
@justinpark + @geido so i've spent literally tons of time trying to get this test work with the current testing pattern and been having no luck. I did find that this is a known issue in enzyme when using `shallow` vs `mount`
https://github.com/enzymejs/enzyme/issues/2086
What i suggest is we merge this to fix the current issue, and I can investigate on rewriting this file to work properly with RTL and i'll take full ownership over this
```
const defaultPropsTwo = {
addDangerToast: jest.fn(),
onHide: () => ({}),
actions: {
saveDataset: jest.fn().mockReturnValue({ id: 42 }),
setFormData: jest.fn(),
updateSlice: jest.fn().mockReturnValue({ id: 42 }),
},
form_data: { datasource: '107__table', url_params: { foo: 'bar' } },
history: {
replace: jest.fn(),
}
};
const fetchChartEndpoint = `glob:*/api/v1/chart/${1}?q=(columns:!(dashboards.id))`;
beforeAll(() => {
fetchMock.get(fetchDashboardsEndpoint, mockDashboardData)
fetchMock.get(fetchChartEndpoint, { dashboards: [mockDashboardData]})
});
test('make sure saveOverwrite function always has slice_id attached to the url', () => {
const wrapper = getWrapper(defaultPropsTwo, queryStore);
const footerWrapper = shallow(wrapper.find(StyledModal).props().footer);
wrapper.setState({
action: 'overwrite',
});
const overwriteRadio = wrapper.find('#overwrite-radio');
overwriteRadio.simulate('click');
expect(wrapper.state().action).toBe('overwrite');
const save = footerWrapper.find('#btn_modal_save');
save.simulate('click');
expect(defaultPropsTwo.history.replace).toBeCalled()
});
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on pull request #25112: fix: permalink save/overwrites in explore
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on PR #25112:
URL: https://github.com/apache/superset/pull/25112#issuecomment-1712486450
@john-bodley + @michael-s-molina I updated the PR with more context information
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #25112: fix: permalink save/overwrites in explore
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #25112:
URL: https://github.com/apache/superset/pull/25112#issuecomment-1699037755
@hughhhh If I recall correctly, permalinks don't use the `slice_id`, they have the form of `superset/explore/p/BwdQzab51P6/`. Can you explain a little bit more the problem? How are permalinks failing?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] justinpark commented on a diff in pull request #25112: fix: permalink save/overwrites in explore
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #25112:
URL: https://github.com/apache/superset/pull/25112#discussion_r1309227123
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -265,10 +265,10 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
searchParams.set('save_action', this.state.action);
if (this.state.action !== 'overwrite') {
searchParams.delete('form_data_key');
- }
- if (this.state.action === 'saveas') {
+ } else {
searchParams.set('slice_id', value.id.toString());
Review Comment:
+1
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: permalink save/overwrites in explore [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25112:
URL: https://github.com/apache/superset/pull/25112#discussion_r1358483857
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -159,6 +159,19 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
this.props.dispatch(setSaveChartModalVisibility(false));
}
+ handleRedirect = (windowLocationSearch: string, chart: any) => {
+ console.log(windowLocationSearch);
Review Comment:
```suggestion
```
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -159,6 +159,19 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
this.props.dispatch(setSaveChartModalVisibility(false));
}
+ handleRedirect = (windowLocationSearch: string, chart: any) => {
+ console.log(windowLocationSearch);
+ const searchParams = new URLSearchParams(windowLocationSearch);
+ searchParams.set('save_action', this.state.action);
+ if (this.state.action !== 'overwrite') {
+ searchParams.delete('form_data_key');
+ }
+
+ searchParams.set('slice_id', chart.id.toString());
+ console.log(searchParams);
Review Comment:
```suggestion
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #25112: fix: permalink save/overwrites in explore
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #25112:
URL: https://github.com/apache/superset/pull/25112#issuecomment-1697814355
@hughhhh would you mind adding a unit test?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #25112: fix: permalink save/overwrites in explore
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #25112:
URL: https://github.com/apache/superset/pull/25112#issuecomment-1699046274
> @hughhhh If I recall correctly, permalinks don't use the `slice_id`, they have the form of `superset/explore/p/BwdQzab51P6/`. Can you explain a little bit more the problem? How are permalinks failing? If you can add a video with the problem it would be great!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: permalink save/overwrites in explore [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25112:
URL: https://github.com/apache/superset/pull/25112#discussion_r1358786107
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -518,3 +522,7 @@ function mapStateToProps({
}
export default withRouter(connect(mapStateToProps)(SaveModal));
+
+// todo(hughhhh): For testing
+// - need to revisit once we convert this to functional component
Review Comment:
I think you can remove the todo and just say that you're exporting this for testing.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido commented on a diff in pull request #25112: fix: permalink save/overwrites in explore
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #25112:
URL: https://github.com/apache/superset/pull/25112#discussion_r1309098441
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -265,10 +265,10 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
searchParams.set('save_action', this.state.action);
if (this.state.action !== 'overwrite') {
searchParams.delete('form_data_key');
- }
- if (this.state.action === 'saveas') {
+ } else {
searchParams.set('slice_id', value.id.toString());
Review Comment:
Should we add a small check in the tests to make sure that the slice_id always appears?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: permalink save/overwrites in explore [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25112:
URL: https://github.com/apache/superset/pull/25112#discussion_r1358487880
##########
superset-frontend/src/explore/components/SaveModal.test.jsx:
##########
@@ -226,3 +233,28 @@ test('set dataset name when chart source is query', () => {
expect(wrapper.find('[data-test="new-dataset-name"]')).toExist();
expect(wrapper.state().datasetName).toBe('test');
});
+
+test('make sure slice_id in the URLSearchParams before the redirect', () => {
+ const myProps = {
+ ...defaultProps,
+ slice: { slice_id: 1, slice_name: 'title', owners: [1] },
+ actions: {
+ setFormData: jest.fn(),
+ updateSlice: jest.fn(() => Promise.resolve({ id: 1 })),
+ getSliceDashboards: jest.fn(),
+ },
+ user: { userId: 1 },
+ history: {
+ replace: jest.fn(),
+ },
+ dispatch: jest.fn(),
+ };
+
+ const saveModal = new PureSaveModal(myProps);
+ saveModal.setState = jest.fn();
Review Comment:
do you need this still?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: permalink save/overwrites in explore [superset]
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh merged PR #25112:
URL: https://github.com/apache/superset/pull/25112
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #25112: fix: permalink save/overwrites in explore
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #25112:
URL: https://github.com/apache/superset/pull/25112#issuecomment-1699534199
@hughhhh given this is a fix, would you mind adding some unit tests?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org