Incubator-superset: Saving metadata in Dashboard properties is broken

[zuzana-vej]

Summary:
Saving metadata in Dashboard properties is broken. 

Steps to Reproduce:

1. Edit dashboard  --> Edit Dashboard Properties --> Advanced
2. Change properties JSON, for example, totally remove default_filters or filter_scopes section
3. Click Save button to close edit Modal
4. Click Save button in dashboard header to finish edit mode

Investigation:
The root problem is, when you click the first save, new dashboard metadata send to backend, but after this save, this change is not updated in the dashboard redux store. So that when you click 2nd save, other old dashboard metadata sent to backend again, which overwrite the first request. The last change is overwritten.

Workaround:
There is a workaround that currently works (but it's not intuitive and likely we will have to explain it to all our users as they raise this issue): Click save only on modal, and not again on dashboard. Leave dashboard without saving the changes. The changes made on the Advanced Properties modal will actually get saved.

Checklist

Make sure these boxes are checked before submitting your issue - thank you!

• [x] I have checked the superset logs for python stacktraces and included it here as text if there are any.
• [x] I have reproduced the issue with at least the latest released version of superset.
• [x] I have checked the issue tracker for the same issue and I haven't found one similar.

[issue-label-bot[bot]]

Issue-Label Bot is automatically applying the label #bug to this issue, with a confidence of 1.00. Please mark this comment with :thumbsup: or :thumbsdown: to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

[dpgaspar]

@zuzana-vej thank you for reporting this.

@nytai can you take a looks at this

[lilykuang]

@zuzana-vej thank you for for reporting this. I wasn't able to reproduce this bug based on the step provided on commit hash a95e5c9327738489fd9fb547134f7ff4ea8c37af. Is ENABLE_REACT_CRUD_VIEWS set to True in superset/config.py?

Here is the video I tried to reproduce the bug:
https://www.loom.com/share/32547542dee24eb5bb5029b232c7e694

[nytai]

@lilykuang I think @zuzana-vej is referring to editing on the actual dashboard

[zuzana-vej]

Yes on the actual dashboard. @graceguo-supercat can confirm, she did some debugging too. It should be reproduceable on dashboard, if not let me know I can add a video.

[mistercrunch]

This seems related to the recent work that I did in this area. There's clearly components / modals that fetch and save straight to the endpoints without going through redux/actions here. This is convenient for the dashboard list view, but breaks down in the context where this is embedded in an app that does its own state management and triggers its own (delayed) save event.

The deep fix involves things like:

• merging all "properties" / modals into a single modal with all properties (CSS, properties, filter mapping, auto-refresh)
• "Ok" on that screen should just merge into the redux store, and not commit to database until user hits save
• look into the implications that this has on the list view's "edit action" modal. My vote there would be to NOT allow the modal there and force the user to open the dash, get in edit mode and do the changes there.

[nytai]

I think ideally these modals should be more "dumb" to maximize reuse. OnSave should just delegate to a callback. This callback can be a redux action that just pushes the changes to state or a handler that fires a patch request... or both.

I'd be more in favor of only allowing dashboard edits on the dashboard page if it loaded faster and we could jump straight into edit mode. Having to click through, wait for the dashboard to load, toggle edit mode, then click into the properties modal is quite a journey for a quick edit.

[eschutho]

I talked with a few folks about expected behavior and found some places where we could make improvements here. I've started work that is in progress that I've linked above, and should have something ready next week.

[nytai]

I'll be consolidating some of the modals and fixing the save not delegating to redux issue sometime next week

[eschutho]

I know that there are some usability features that we're discussing to improve this flow, but do you all think that https://github.com/apache/incubator-superset/pull/11220 has fixed this particular issue?

[zuzana-vej]

No, it didn't. Right now, (after #11220 merged) this is what happens if user tries to edit dashboard metadata (e.g. best way to test this on your test dashboard is to change color of one of the lines on your chart in the dashboard metadata tab).

Right now, what happens, is:

1. user edits metatada (e.g. change color of a line on a chart)
2. user clicks save on modal (all good)
3. user clicks save on dashboard --> and gets the message "this dashboard has changed, please reload"
4. when user reloads, user can see the change actually got applied

Steps 3+4 will confuse them currently.

So in a way it solves the issue - change is applied, but user currently gets a warning message which will confuse them. Actually if we want to be really precise, this issue is resolved, but with #11220 there is new issue now. I am not sure if this open PR would solve what is happening now?

this is how I test - by changing one of these values:

cc: @graceguo-supercat

[zuzana-vej]

I am going to close this because looks like https://github.com/apache/incubator-superset/pull/10834 resolved the issue. Thanks @eschutho!