Site Settings: Fix lost site settings dirty fields when site icon is updated

[youknowriad]

This is still WIP but will allow us to discuss more for the best way to fix #10412

Testing instructions

• Navigate to the general settings form
• Edit the title of the blog
• Update the site icon
• Try to navigate away from the form
• You should be prompted with the "Discard local changes" modal

[matticbot]

Test live: https://calypso.live/?branch=fix/dirty-fields-lost

[youknowriad]

While these changes are not too complex, having these duplicate selectors seems weird. Maybe it could be better if would be simpler if we just store the last request id, without storing saveStatus for the different request Ids

[youknowriad]

I updated to store the last id only and I'm way more satisfied here

[aduth]

If we were to go this route, would it make sense to extend the existing saveSiteSettings action creator separately using the extendAction utility (either as separate action creators or in the connect binding) rather than adding more arguments to the action creator? Maybe it's meta that the requesting reducer can track and getSiteSettingsSaveRequestStatus and related selectors could optionally accept as an argument?

[youknowriad]

would it make sense to extend the existing saveSiteSettings action creator separately using the extendAction utility (either as separate action creators or in the connect binding)

@aduth Yes, we could. I prefer a separate action creator, other forms could benefit from it.

Maybe it's meta that the requesting reducer can track and getSiteSettingsSaveRequestStatus and related selectors could optionally accept as an argument?

The first approach I tried in 8095254 is storing the request by id and having the id as an argument in the selectors but I think it was way more complex. I don't think having the argument is necessary now since I'm only storing the last save request id.

--

@aduth This is a proposal for now. Let me know if you strongly disagree with this and you prefer to the "clearing dirty fields when props change" approach instead.

[aduth]

There's something about the idea of tracking individual requests that doesn't sit well with me. Maybe it's that we're convoluting the idea of saving settings with needs of the UI? There might be ways to better separate the two while staying within Redux state (e.g. extended action creators, separate UI state, maybe even form state).

The thought with considering dirty fields as props was an attempt in framing it in high-level requirements: under what conditions should we prompt the user of unsaved changes? To me, it seems to be at the point a user tries to leave a page and we determine that there are form values out of sync with what we consider to be the persisted values. This is what led me to consider how we achieve this in posts state and wondering if we could take a similar approach. I'll admit I've not thought much further than that, so perhaps there are blocking considerations to making this work in the context of settings.

[youknowriad]

Closing, this has been fixed by #10808