CMS 5.2.0-beta1. Regression test: LinkField has "Draft / Modified" label even when Elemental Block was published

[sabina-talipova]

Description

Executed on: SC PHP 8.3 Chrome LinkField
Test scenario: https://studio.cucumber.io/projects/301855/test-runs/947336/folder-snapshots/11144442/scenario-snapshots/34759621/test-snapshots/46444047

After publishing block draft label won't go away, but it has been published on the front end. when the page refreshes only it will go away.

Steps to reproduce issue:

• I create a new Elemental block with LinkField;
• I create a new Link;
• I see "Draft" badge in LinkField;
• I publish the block (not a Page);
• I still see "Draft" badge in LinkField;
• I refresh the Page and then a badge disappears.

Acceptance criteria

• Manually publishing an elemental block with a owned Link relation update the status of the relevant linkfield.

PR

• FIX Update LinkField if Elemental version status was updated #268
• ENH Add element version to global state silverstripe-elemental#1161
• FIX Allow reloading react forms based on changes to a prop silverstripe-admin#1771
• FIX Reload element form when element version changes silverstripe-elemental#1200
• MNT Test publishing block updates link state #294

[sabina-talipova]

There is the similar problem for another nested versioned DataObject.
For example for files:

Update: Related issue was open silverstripe/silverstripe-asset-admin#1450

[emteknetnz]

@sabina-talipova I've closed the linked PRs because I don't think they're going about solving this the correct way. It's doing a couple of things I dislike:

• Creating a coupling between two optional modules
• Add client side form state thus creating a new source of truth. This adds complexity and can easily lead to creating regressions later on. It's FAR easier to only use the server as the source of truth.
• It's also not solving the asset-admin issue noted above which has the same symptom

Instead what I think we want to do is force the inline form (created using FormBuilder which uses redux-form) to refresh after a successful publish/unpublish so that linkfield will subsequently refresh its own data from the server.

This is non-trivial and has a pretty frustrating learning curve so feel free to move this issue back into the ready column if you don't want to tackle it

[sabina-talipova]

I put this ticket back to Ready column before blocking issue won't be merged.

[GuySartorelli]

When publishing the block, the linkfield component does get re-rendered, but because its value hasn't changed and it isn't being updated via the modal (which would update the editingID), and the forceFetch state hasn't changed, it doesn't fire off any requests and therefore doesn't know the state has updated.

I can think of a few ways to resolve this, but none of them feel particularly elegant.

1. Don't use that useEffect and just always request new data ever time the component re-renders.
   • Our setup results in components being re-rendered all the time, so this would be really inefficient at best and lead to weird behaviour at worst.
2. Introduce a new prop that always gets passed to all formfields when a publish/unpublish/archive/save event happens (maybe something like versionEventOccurred which is default false). Form fields can choose to ignore this or use it to update their version labels when relevant.
3. Similar to 2 above, but use either a global state in redux or a key in a context above the form.
4. Whenever we publish/unpublish/archive/save, cause some nasty custom global event fire or add something uniquely identifiable to the DOM. When that happens, the linkfield should be forced to refetch its data

I don't really like any of these but I lean towards option 3.

There's a hidden option 5 which is just do nothing. Refreshing the page or opening and closing a link modal refreshes its state so it's not wrong forever - only until further action is taken to update its display. I don't like this hidden option either though.

[emteknetnz]

I don't think 3 is viable if using context because you'd need to create something like <ElementContext.Provider> in elemental, however in linkfield we'd need to listen for that same context, and elemental may not be installed so we cannot import the ElementalContext. Also even if element is installed we may be in a non-elemental context editing a LinkField on another DataObject.

Redux is more doable, though you'd still end up looking for elemental keys within linkfield. Redux is a pretty confusing beast and I'm prefer to not use it here, especially since it's not used at all for LinkField

2 actually seems like the most viable, though I don't really like it. Seems like we creating some sort of quasi global API that will probably only get implemented for this one niche use case

[GuySartorelli]

The context idea would naturally rely on a context high enough up the chain that anything that needs to deal with it can deal with it.

Both that and the redux idea would use something that isn't specific to elemental.

[GuySartorelli]

@maxime-rainville can you please weigh in? This one may be worth leaving as is for now given it's not as simple as it first seemed.

[maxime-rainville]

I think the fundamental problem here is that there's not a clean way of invalidating <FormBuliderLoader> and forcing it to reload.

Basically, you want to detect a state change in <InlineEditForm /> by identifying some subset of props that reflect the need to refresh the <FormBuilderLoader/>

You can achieve this in a somewhat hackish way by passing some dummy GET parameters in the schema URL that reflect the versioned state of the block. This commit illustrates what that might look like: silverstripe/silverstripe-elemental@7cbde1e

This is what it looks like.
elemental-form-reload-2024-05-30_18.08.49.webm

I'm thinking the proper way to do this would be to add an extra prop to <FormBuilderLoader />. When that prop change, <FormBuilderLoader> should re-render and refetch the form schema URL.

[GuySartorelli]

Oh yeah, I didn't consider re rendering the whole form. It's a trade off of slightly bad UX waiting for the form to reload (which isn't needed for 90% of the time) vs worse bad UX of temporarily displaying incorrect information in the remaining 10% of the time.

I've got no problem making that tradeoff, I'll go that route.

[maxime-rainville]

While there is a trade off, I suspect this is not the only place where this bug might occur. (e.g: Publishing a block auto-publish a file).