fix: permalink save/overwrites in explore by hughhhh · Pull Request #25112 · apache/superset

hughhhh · 2023-08-29T16:29:01Z

SUMMARY

Fixing the bug whenever a user is trying to do an overwrite action from permalink state. The main issue is when using a permalink the explore SaveModal doesn't properly pass the slice_id to allow the reload of the update chart. To fix this i've added logic to make sure the slice_id is always present before redirecting.

Screen.Recording.2023-09-07.at.1.26.31.PM.mov

Related PR: #23627

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- 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

geido · 2023-08-29T16:42:10Z

superset-frontend/src/explore/components/SaveModal.tsx

-      }
-      if (this.state.action === 'saveas') {
+      } else {
        searchParams.set('slice_id', value.id.toString());


Should we add a small check in the tests to make sure that the slice_id always appears?

@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
enzymejs/enzyme#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() });

john-bodley · 2023-08-29T16:52:57Z

@hughhhh given this is a fix, would you mind adding some unit tests?

michael-s-molina · 2023-08-30T12:04:29Z

@hughhhh If I recall correctly, permalinks don't use the slice_id, they have the form of superset/explore/p/BwdQzab51P6/. How are permalinks failing? Can you add a video demonstrating the problem?

hughhhh · 2023-09-09T11:12:49Z

@john-bodley + @michael-s-molina I updated the PR with more context information

…ix-permlink-save

superset-frontend/src/explore/components/SaveModal.tsx

eschutho · 2023-10-13T16:28:49Z

superset-frontend/src/explore/components/SaveModal.test.jsx

+  };
+
+  const saveModal = new PureSaveModal(myProps);
+  saveModal.setState = jest.fn();


do you need this still?

Co-authored-by: Elizabeth Thompson <[email protected]>

eschutho · 2023-10-13T20:18:56Z

superset-frontend/src/explore/components/SaveModal.tsx

 export default withRouter(connect(mapStateToProps)(SaveModal));
+
+// todo(hughhhh): For testing
+// - need to revisit once we convert this to functional component


I think you can remove the todo and just say that you're exporting this for testing.

eschutho

LGTM. I just left a small nit comment.

Co-authored-by: Elizabeth Thompson <[email protected]> (cherry picked from commit e58a3ab)

* fix: is_select with UNION (apache#25290) (cherry picked from commit bb002d6) * fix: Add explicit ON DELETE CASCADE for dashboard_roles (apache#25320) (cherry picked from commit d54e827) * fix(chart): Supporting custom SQL as temporal x-axis column with filter (apache#25126) Co-authored-by: Kamil Gabryjelski <[email protected]> * fix: Use RLS clause instead of ID for cache key (apache#25229) (cherry picked from commit fba66c6) * fix: Improve the reliability of alerts & reports (apache#25239) (cherry picked from commit f672d5d) * fix: DashboardRoles cascade operation (apache#25349) (cherry picked from commit a971a28) * fix: datetime with timezone excel export (apache#25318) Co-authored-by: Michael S. Molina <[email protected]> (cherry picked from commit 5ebcd2a) * fix: Workaround for Cypress ECONNRESET error (apache#25399) (cherry picked from commit d76ff39) * fix(sqllab): invalid persisted tab state (apache#25308) (apache#25398) * fix: Rename on_delete parameter to ondelete (apache#25424) (cherry picked from commit 893b45f) * fix: preventing save button from flickering in SQL Lab (apache#25106) (cherry picked from commit 296ff17) * fix: chart import (apache#25425) (cherry picked from commit a4d8f36) * fix: swagger UI CSP error (apache#25368) (cherry picked from commit 1716b9f) * fix: smarter date formatter (apache#25404) (cherry picked from commit f0080f9) * fix(sqllab): invalid start date (apache#25437) * fix(nativeFilters): Speed up native filters by removing unnecessary rerenders (apache#25282) Co-authored-by: JUST.in DO IT <[email protected]> (cherry picked from commit a0eeb4d) * fix(SqlLab): make icon placement even (apache#25372) (cherry picked from commit 11b49a6) * fix: Duplicate items when pasting into Select (apache#25447) (cherry picked from commit 7cf96cd) * fix: update the SQLAlchemy model definition at json column for Log table (apache#25445) (cherry picked from commit e83a76a) * fix(helm chart): set chart appVersion to 3.0.0 (apache#25373) * fix(mysql): handle string typed decimal results (apache#24241) (cherry picked from commit 7eab59a) * fix: Styles not loading because of faulty CSP setting (apache#25468) (cherry picked from commit 0cebffd) * fix(sqllab): error with lazy_gettext for tab titles (apache#25469) (cherry picked from commit ddde178) * fix: Address Mypy issue which is causing CI to fail (apache#25494) (cherry picked from commit 36ed617) * chore: Adds 3.0.1 CHANGELOG * fix: Unable to sync columns when database or dataset name contains `+` (apache#25390) (cherry picked from commit dbe0838) * fix(sqllab): Broken query containing 'children' (apache#25490) (cherry picked from commit b92957e) * chore: Expand error detail on screencapture (apache#25519) (cherry picked from commit ba541e8) * fix: tags permissions error message (apache#25516) (cherry picked from commit 50b0816) * fix: Apply normalization to all dttm columns (apache#25147) (cherry picked from commit 58fcd29) * fix: REST API CSRF exempt list (apache#25590) (cherry picked from commit 549abb5) * fix(RLS): Fix Info Tooltip + Button Alignment on RLS Modal (apache#25400) (cherry picked from commit a6d0e6f) * fix: thubmnails loading - Talisman default config (apache#25486) (cherry picked from commit 52f631a) * fix(Presto): catch DatabaseError when testing Presto views (apache#25559) Co-authored-by: Rui Zhao <[email protected]> (cherry picked from commit be3714e) * fix(Charts): Set max row limit + removed the option to use an empty row limit value (apache#25579) (cherry picked from commit f556ef5) * fix(window): unavailable localStorage and sessionStorage (apache#25599) * fix: finestTemporalGrainFormatter (apache#25618) (cherry picked from commit 62bffaf) * fix: revert fix(sqllab): Force trino client async execution (apache#24859) (apache#25541) (cherry picked from commit e56e0de) * chore: Updates 3.0.1 CHANGELOG * fix(sqllab): Mistitled for new tab after rename (apache#25523) (cherry picked from commit a520124) * fix(sqllab): template validation error within comments (apache#25626) (cherry picked from commit b370c66) * fix: avoid 500 errors with SQLLAB_BACKEND_PERSISTENCE (apache#25553) (cherry picked from commit 99f79f5) * fix(import): Make sure query context is overwritten for overwriting imports (apache#25493) (cherry picked from commit a0a0d80) * fix: permalink save/overwrites in explore (apache#25112) Co-authored-by: Elizabeth Thompson <[email protected]> (cherry picked from commit e58a3ab) * fix(header navlinks): link navlinks to path prefix (apache#25495) (cherry picked from commit 51c56dd) * fix: improve upload ZIP file validation (apache#25658) * fix: warning of nth-child (apache#23638) (cherry picked from commit 16cc089) * fix(dremio): Fixes issue with Dremio SQL generation for Charts with Series Limit (apache#25657) (cherry picked from commit be82657) --------- Co-authored-by: Beto Dealmeida <[email protected]> Co-authored-by: John Bodley <[email protected]> Co-authored-by: Zef Lin <[email protected]> Co-authored-by: Kamil Gabryjelski <[email protected]> Co-authored-by: Jack Fragassi <[email protected]> Co-authored-by: Michael S. Molina <[email protected]> Co-authored-by: JUST.in DO IT <[email protected]> Co-authored-by: Jack <[email protected]> Co-authored-by: Daniel Vaz Gaspar <[email protected]> Co-authored-by: Stepan <[email protected]> Co-authored-by: Corbin Bullard <[email protected]> Co-authored-by: Gyuil Han <[email protected]> Co-authored-by: Celalettin Calis <[email protected]> Co-authored-by: Ville Brofeldt <[email protected]> Co-authored-by: ʈᵃᵢ <[email protected]> Co-authored-by: Michael S. Molina <[email protected]> Co-authored-by: mapledan <[email protected]> Co-authored-by: Igor Khrol <[email protected]> Co-authored-by: Rui Zhao <[email protected]> Co-authored-by: Fabien <[email protected]> Co-authored-by: Hugh A. Miles II <[email protected]> Co-authored-by: OskarNS <[email protected]>

Co-authored-by: Elizabeth Thompson <[email protected]>

make sure slice_id is in params if not overwrite

0bd7778

hughhhh requested a review from justinpark August 29, 2023 16:29

pull-request-size bot added the size/XS label Aug 29, 2023

geido reviewed Aug 29, 2023

View reviewed changes

hughhhh added 2 commits August 30, 2023 14:54

always make sure slce_id is added

6227169

save

11f8ec5

Merge branch 'master' of https://github.com/preset-io/superset into f…

4c6083e

…ix-permlink-save

pull-request-size bot added size/M and removed size/XS labels Sep 9, 2023

hughhhh added 4 commits October 10, 2023 18:49

Merge branch 'master' of https://github.com/preset-io/superset into f…

7833211

…ix-permlink-save

get test to work properly

83fc411

ok

7632855

ok

b9b6d59

eschutho reviewed Oct 13, 2023

View reviewed changes

superset-frontend/src/explore/components/SaveModal.tsx Outdated Show resolved Hide resolved

eschutho reviewed Oct 13, 2023

View reviewed changes

superset-frontend/src/explore/components/SaveModal.tsx Outdated Show resolved Hide resolved

eschutho reviewed Oct 13, 2023

View reviewed changes

hughhhh and others added 3 commits October 13, 2023 13:14

Update superset-frontend/src/explore/components/SaveModal.tsx

89e5e06

Co-authored-by: Elizabeth Thompson <[email protected]>

Update superset-frontend/src/explore/components/SaveModal.tsx

526329a

Co-authored-by: Elizabeth Thompson <[email protected]>

remove setSate

6152399

eschutho reviewed Oct 13, 2023

View reviewed changes

eschutho approved these changes Oct 13, 2023

View reviewed changes

Update SaveModal.tsx

6deb900

hughhhh merged commit e58a3ab into apache:master Oct 16, 2023

hughhhh deleted the fix-permlink-save branch October 16, 2023 18:00

jinghua-qa pushed a commit to preset-io/superset that referenced this pull request Oct 16, 2023

fix: permalink save/overwrites in explore (apache#25112)

999e9b3

Co-authored-by: Elizabeth Thompson <[email protected]> (cherry picked from commit e58a3ab)

michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Oct 17, 2023

michael-s-molina pushed a commit that referenced this pull request Oct 19, 2023

fix: permalink save/overwrites in explore (#25112)

236aef8

Co-authored-by: Elizabeth Thompson <[email protected]> (cherry picked from commit e58a3ab)

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024

fix: permalink save/overwrites in explore (apache#25112)

506415b

Co-authored-by: Elizabeth Thompson <[email protected]>

mistercrunch added 🍒 3.0.2 Cherry-picked to 3.0.2 🍒 3.0.3 Cherry-picked to 3.0.3 🍒 3.0.4 Cherry-picked to 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 First shipped in 3.1.0 labels Mar 8, 2024

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

fix: permalink save/overwrites in explore (apache#25112)

33610f2

Co-authored-by: Elizabeth Thompson <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: permalink save/overwrites in explore#25112

fix: permalink save/overwrites in explore#25112
hughhhh merged 12 commits intoapache:masterfrom
preset-io:fix-permlink-save

hughhhh commented Aug 29, 2023 •

edited

Loading

Uh oh!

geido Aug 29, 2023

Uh oh!

justinpark Aug 29, 2023

Uh oh!

hughhhh Sep 9, 2023

Uh oh!

john-bodley commented Aug 29, 2023 •

edited

Loading

Uh oh!

michael-s-molina commented Aug 30, 2023 •

edited

Loading

Uh oh!

hughhhh commented Sep 9, 2023

Uh oh!

Uh oh!

Uh oh!

eschutho Oct 13, 2023

Uh oh!

eschutho Oct 13, 2023

Uh oh!

eschutho left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

hughhhh commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

geido Aug 29, 2023

Choose a reason for hiding this comment

Uh oh!

justinpark Aug 29, 2023

Choose a reason for hiding this comment

Uh oh!

hughhhh Sep 9, 2023

Choose a reason for hiding this comment

Uh oh!

john-bodley commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-s-molina commented Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hughhhh commented Sep 9, 2023

Uh oh!

Uh oh!

Uh oh!

eschutho Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

eschutho Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

eschutho left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hughhhh commented Aug 29, 2023 •

edited

Loading

john-bodley commented Aug 29, 2023 •

edited

Loading

michael-s-molina commented Aug 30, 2023 •

edited

Loading