Always update schema data in redux

[stevie-mayhew]

As the underlying data may have changed regardless of its ID set matching

Fixes #960

[stevie-mayhew]

Further to this, there are a few sections of code here which compare ID's and whatnot to see if the data has "changed"

I'd like to have a look at removing all of that stuff and start assuming that we're passing around immutable things which could change at any point, it'll remove some of the edge cases like this one which seem to pop up in this module.

[ScopeyNZ]

Hmmmn. My initial reaction to this is that we're breaking forms that have been changed but unmounted from the DOM. This happens now with form fields in elemental, when the form is "collapsed" the form state is saved in redux but the DOM elements are actually unmounted. I don't think it's "correct" to assume that redux is always stale.

I'll do a more detailed review soon.

[stevie-mayhew]

@ScopeyNZ I don't think that we should care if it is stale or not ;)

In this case, the alternative path was to perform more detailed checks of the "deep" copy of the data, which makes the comparators much more complex than they are currently and IMHO they are a bit useless - if we're using a store then we should push everything into the store all the time and let it sort itself out, right?

[ScopeyNZ]

I don't think that we should care if it is stale or not

Either the initial state (usually given in the form of a data attribute or a "form schema" request) is stale or redux is stale. I think we do have to care if it's stale. The issue linked is probably caused by the fact that the component is remounted with the correct "initial" data - but really it should've been added to redux and the component would have updated. It's probably not doing that because we're hitting our weird "oh it's entwine but it's also React/redux" ecosystem.

If we merge this we'll almost certainly reintroduce a bug where edited file details are removed when the UploadField is hidden (removed from the DOM) and then re-shown. Perhaps what we should do is bust redux stores when the CMS form is saved.

I'm not really sure what's the best idea here. It's all a bit messy.

[stevie-mayhew]

Yeah it is a problem with the entwine re-population of the form on a PJAX request on save. I agree that it is really messy :)

When you say "hidden and then re-shown" what context is that in? As far as I was aware editing of files now happens within the assets section, no longer on the page where this component is shown?

[ScopeyNZ]

When you say "hidden and then re-shown" what context is that in? As far as I was aware editing of files now happens within the assets section, no longer on the page where this component is shown?

The case I'm thinking of is a file block in elemental (^4). You can upload a file, or adjust details on the file (using the asset-admin modal that appears when you click the UploadField). Then you may collapse the form again before either saving or re-expanding the form.

Now that you mention it you might not be able to actually edit meta-data on the file this way, but you can definitely change the chosen file or upload one without saving it, and then use the page level save to actually persist the changes.

[stevie-mayhew]

It might be that a lot of the code that makes no sense to me in this module has been written to help with that elemental case rather than its stock standard functionality?

Should we instead move some of that required functionality up a level and implement a UploadField which suits those specific needs for elemental? I'd be happy to have a look at doing that if you agreed @ScopeyNZ

[ScopeyNZ]

Think of it as a lot of the code has been rewritten to support use of this component with "FormBuilder" - the JS/React implementation to render a getCMSFields. We are getting a heap of bugs trying to make things work with both entwine and this new "FormBuilder" context. I don't deny that the bug your fixing is a problem in entwine. It just feels frustrating accepting a PR that I know will break it in another context.

Implementing a new higher level (order 😉 ) component is a good idea and makes a lot of sense, but I would prefer that we implement that to support the entwine context (not the other way around) as the goal future state means that everything is React and we can delete all the entwine compatibility code.

[dnsl48]

I've spent some time looking into what might get broken and I cannot find anything at the moment.
I also cannot find any behat tests covering UploadField, neither jest has anything broken after this patch.
I suggest we just merge this patch as is and deal with whatever comes up later when we do regression tests. @ScopeyNZ what do you reckon?

[ScopeyNZ]

I don't think it's a good idea to merge this and deal with a problem that might show up when I'm pretty confident one will show up. If you checkout this patch and play around with upload forms in elemental I think you'll probably see an issue relatively quickly where state is lost (or incorrectly retained) in the UI.

I haven't had any opportunity to test this yet (I'm not working with SS much right now) but I can still probably find a bit of time to play around with this soonish

[ScopeyNZ]

I also want to mention that I think the problem that I'm pretty sure will occur is (in my opinion) higher impact than the bug that is being fixed here.

[dnsl48]

If you checkout this patch and play around with upload forms in elemental I think you'll probably see an issue relatively quickly where state is lost

that's my point. I actually been trying to find what's broken anywhere with this patch applied on my localhost - neither tests nor elemental blocks nor standalone upload field seem to get broken with it. Everything seems to be working just fine to me.
At this point I don't see many other options as how we can figure out what relies on this code except to remove it and see what may get broken.

I also want to mention that I think the problem that I'm pretty sure will occur is (in my opinion) higher impact than the bug that is being fixed here.

Good point. I think if that's critical enough, then it should come up in regression testing quite quickly, then we would be able to rethink our approach to how we fix it or return this code with more in-depth comparison, rather than just checking IDs.

[emteknetnz]

This pull request hasn't had any activity for a while. Are you going to be doing further work on it, or would you prefer to close it now?

[stevie-mayhew]

I won't be, I still think that this fixes the issue but the higher order components aren't really part of the concern for this. I'd prefer that we merged it and fixed the issue thats been around for a long time, or look at actually fixing the issue upstream rather than blocking a fix.

[dnsl48]

@ScopeyNZ do you feel strongly about blocking this, or should we just merge and move on?

[emteknetnz]

I'm thinking hold old merging this for now, there's other related issues around files and state not being in sync and they kind of all need to be fixed together. I believe these two issues are also related

• Replacing an asset. Asset admin doesn't show the new filename. #1010
• STATE: Files embedded in TinyMCE remain marked as draft after Page is published #1001

Unfortunately the whole props.files along with props.data.files kind of means there's basically two sources of truth and that doing any kind of work here is difficult. Regressions are inevitable so it would kind of reckless to merge this right now without it being one of our internal priorities to actually fixing all of these issues.

[kinglozzer]

I’ve been testing this this afternoon, and I can recreate the issue that @ScopeyNZ was afraid of: updates being lost when un-mounting and re-mounting an UploadField (in inline edited blocks). I have an alternative proposal here: #1263

[stevie-mayhew]

Closing in favour of #1263