feat(explore): Show confirmation modal if user exits Explore without saving changes

[kgabryje]

SUMMARY

This PR adds a browser confirmation modal (similar to the one that we have in dashboard edit mode) when user makes a change in control panel and then tries to close Explore (either by closing the tab or moving to another page) without clicking Save button.
Currently, that modal is not customizable. However, when we implement Single Page Application in Superset, we'll probably be able to customize the modal when navigating within Superset.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Changes made without saving:

1. Open a chart
2. Make some changes in control panel
3. Try to leave the page (close tab or navigate to another page)
4. Verify that browser confirmation modal appears

Changes made and saved:

1. Open a chart
2. Make some changes in control panel and save.
3. Leave the page, verify that the modal didn't appear

Changes made and then reverted:

1. Open a chart
2. Make some changes in control panel.
3. Revert those changes (for example add and remove a metric)
4. Leave the page, verify that the modal didn't appear

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

[michael-s-molina]

@kgabryje If I do more than one change, then the confirmation is not appearing. The beforeunload is being unregistered.

Screen.Recording.2022-05-09.at.11.14.29.AM.mov

[codecov]

Codecov Report

Merging #19993 (33690f9) into master (62e1c34) will decrease coverage by 0.00%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master   #19993      +/-   ##
==========================================
- Coverage   66.28%   66.28%   -0.01%     
==========================================
  Files        1712     1712              
  Lines       63968    64003      +35     
  Branches     6731     6736       +5     
==========================================
+ Hits        42404    42424      +20     
- Misses      19853    19866      +13     
- Partials     1711     1713       +2     

Flag	Coverage Δ
javascript	51.28% <75.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../explore/components/ExploreViewContainer/index.jsx	52.10% <46.66%> (-0.47%)	⬇️
...rset-frontend/src/explore/exploreUtils/formData.ts	88.88% <91.66%> (+3.17%)	⬆️
...-frontend/src/components/AlteredSliceTag/index.jsx	87.23% <100.00%> (-1.01%)	⬇️
superset-frontend/src/SqlLab/actions/sqlLab.js	60.39% <0.00%> (ø)
...c/views/CRUD/data/database/DatabaseModal/index.tsx	32.69% <0.00%> (+0.18%)	⬆️
...et-frontend/src/explore/reducers/exploreReducer.js	28.75% <0.00%> (+1.98%)	⬆️
...mponents/nativeFilters/FilterCard/useTruncation.ts	50.00% <0.00%> (+6.52%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62e1c34...33690f9. Read the comment docs.

[kgabryje]

Well spotted @michael-s-molina! The problem was that the cleanup function in use effect was being called on each change of its dependencies. I moved the cleanup to a separate useEffect that gets called only on unmount.

Also I fixed an old bug that caused the "Altered" pill to stay visible when there was an adhoc metric added and then you added some metric and deleted it immediately (so effectively there was no change, but "Altered" stayed). The reason for that was that in initial form data the adhoc metric is represented by a plain JSON object, but later it's transformed to AdhocMetric instance, so isEqual is false even if the field values are the same. I fixed it by omitting __proto__ before calling isEqual so that we ignore the difference between plain object and class instance

[kgabryje]

/testenv up

[github-actions]

@kgabryje Ephemeral environment spinning up at http://54.244.56.255:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[michael-s-molina]

Found two more problems:
1 - It's showing the confirmation when navigating using the main header without any changes.
2 - It's showing the confirmation when dropping a filter and canceling

Screen.Recording.2022-05-11.at.8.47.13.AM.mov

[kgabryje]

Found two more problems: 1 - It's showing the confirmation when navigating using the main header without any changes. 2 - It's showing the confirmation when dropping a filter and canceling

Screen.Recording.2022-05-11.at.8.47.13.AM.mov

Thanks for more testing 🙂

1. It looks like the algorithm for calculating diffs for Altered table is faulty, because the confirmation modal uses the same logic. Altered pill probably shouldn't popup here as you just opened a saved chart, so the problem might be related. Will investigate.
2. I don't think it's related to dropping and cancelling filter. The Altered pill is visible and it's most likely that that's why the confirmation modal is showing.

[kgabryje]

@michael-s-molina It looks like sliceFormData which was used as a reference for "initial" form data didn't include values of controls that are "trigger renders" (so mostly controls in Customize section). I fixed it by using actual initial form data instead of form data from chart.
The takeaway is that in case the user does the following steps:

1. Open a chart
2. Change something in form data
3. Run query
4. Reload the page (confirmation modal appears)
5. Close the page without saving - confirmation modal doesn't appear

On the other hand such flow might be expected - when user changes something in the chart and then sends a link to someone else, that other user shouldn't see the confirmation modal before closing the tab.
I think that current solution is optimal, but tagging @lauderbaugh @kasiazjc @rusackas in case you guys have different opinion.

A similar issue with sliceFormData as described above occurs in calculating diffs Altered pill - I think it similarly should be replaced with initial form_data. I'm not going to fix it in this PR though in case the problem is more complex.

[kgabryje]

/testenv up

[github-actions]

@kgabryje Ephemeral environment spinning up at http://52.12.219.96:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[michael-s-molina]

LGTM

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.