Add rudimentary conflict prevention to the Config Auto-save end-point

Postponing on 📌 [PP-1] Enforce conflict enforcement outside of tests and e2e tests Postponed because that issue end up having some back-end changes we would likely use here

I brought in some more changes from 📌 [PP-1] Enforce conflict enforcement outside of tests and e2e tests Postponed just so it is ready to go once that issue is in. but that issue should still be fixed first

📌 [PP-1] Enforce conflict enforcement outside of tests and e2e tests Postponed is in🎉

Need to bring in the latest changes from 0.x now

@jessebaker, this is the MR we chatted about.

On the client side I used claude code to hopefully handle the auto-save conflict detection in the same way you set up in 📌 Add rudimentary conflict prevention to the preview end-point Active . Hopefully this better than the first attempt with claude on the other issue because it was using the existing pattern and creating something from scratch.

For the `/xb/api/v0/config/auto-save/{entityTypeId}/{configEntityId}` pathes that client calls to save the auto-save for JS Components and the Asset library I have changed the request to nest the current entity data under a new top level `data` key, so that allows a sibling autoSaves like we used in #3490565

let me know what you think

@effulgentsia just raised with @balintbrews what he thought about test coverage would be appropriate for the experience_builder.api.config.auto-save.(get|patch) routes supporting conflict detection/prevention. @balintbrews recommended (and @effulgentsia +1'd) that no end-to-end tests nor functional tests are needed, because it'd require writing all of those from scratch.

So they recommend: Kernel tests + manual testing of the inability to lose data in the UI.

IOW: the scope here is ONLY avoiding data loss on back-end, out of scope is informing the user of this.

re #10

So they recommend: Kernel tests + manual testing of the inability to lose data in the UI.

Sounds good. I have manually tested it and it work and it should be easy to add a kernel test like \Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPostTest::testOutdatedAutoSave

re

IOW: the scope here is ONLY avoiding data loss on back-end, out of scope is informing the user of this.

I understand the first part, I don't understand what "out of scope is informing the user of this." is suppose to mean. Are we just suppose to error in the auto-save but user isn't suppose to know this? Do they just keep on trying to make changes but they never take affect?

FWIW I implement the same error response we added in 📌 [PP-1] Enforce conflict enforcement outside of tests and e2e tests Postponed were we just tell them

Your latest change was not saved because the content was modified
elsewhere since you loaded the page. Please refresh your browser to
receive the latest changes and continue.

See ui/src/features/editor/ConflictWarning.tsx

One thing I noticed is that if I am editing a JS component whenever an auto-save is triggered for the component an auto-save request is also triggered for the global asset library, regardless of the fact that I haven't made any changes to global asset library CSS.

This will probably mean there would be many more conflicts than if we only triggered an auto-save for the global asset library when it changed in the editor. The chance that another user has edited the same JS Component since my last GET request is a lot less then the chance that another user has edited the global asset library, since there could be many JS components but always only one global asset library which everyone is editing.

We also might want to look into reloading the global asset library auto-save when the user moves to that editor. I could be editing a JS component for 30 minutes and never touched the global asset library then switch to it. In that 30 minutes any user could editing any other JS component could have edited the global asset library

again back to #10 and

IOW: the scope here is ONLY avoiding data loss on back-end, out of scope is informing the user of this.

I tested locally reverting show the same error warning on conflict as with the layout and the 409 conflict is just logged to the console. So the user is not informed. The effect is that the user can just keep editing the Javascript because they
don't know to reload their browser but the Preview on right does update with their changes

I would argue this actually is data loss because we are letting the user continue spend time on the JS with no indication they should do anything, so they will lose this work. The user would have no way of knowing that their work did not make it to the auto-save

Assigning back to myself because I need to make the kernel test I mentioned in #11. I have started on that

@jessebaker you could still review the front-end changes.

Added a new kernel test

I closed 📌 For config entities add client support conflict detection via the `autoSaves' key Active because we will likely need to do the front-end changes here

Assiging back to @jessebaker see my MR comments specifically https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... . 2 of the failing e2e test point to the problem

I am confused by the e2e gitlab CI fails.

Getting errors like

cy:command ✘ uncaught exception Error: {"error":{"status":500,"data":{"message":"Body does not match schema for content-type \"application/json\" for Request [post /xb/api/v0/layout/{entityTypeId}/{entityId}]. [Keyword validation failed: Required property 'autoSaves' must be present in the object in autoSaves]"}},"isUnhandledError":false,"meta":{"request":{},"response":{}}}

and

{
"status": "500",
"errors": {
"message": "Body does not match schema for content-type \"application/json\" for Request [patch /xb/api/v0/layout/{entityTypeId}/{entityId}]. [Keyword validation failed: Required property 'autoSaves' must be present in the object in autoSaves]"
},

so it appears the client logic to autoSaves to the post and patch requests is not working and the logic that enforces our openapi.yml rule is catching it.

but when I run these tests locally they do pass. I tested locally altering the openapi.yml file to make sure these e2e will fail for openapi.yml violations and they do locally.

@jessebaker any possibility the new JS to add autoSaves to the requests somehow is not working on gitlab CI? that is the only thing I can think of

have manually tested this again

1. create a code component
2. make an edit <FormattedText>{title} 1</FormattedText>
3. copy the url and open another browser tab
4. 2nd tab make an edit <FormattedText>{title} 2</FormattedText>
5. 1st tab make an edit <FormattedText>{title} 3</FormattedText>
6. 1st tab errors because it doesn't have the latest changes: this is expected and correct
7. Refresh browser by clicking on "Refresh" link in error messages
8. 1st tab show latest changes <FormattedText>{title} 2</FormattedText>
9. 2nd tab make an edit <FormattedText>{title} 4</FormattedText>
10. Pause to ensure auto-save
11. 2nd tab make an edit <FormattedText>{title} 2</FormattedText>: important this brings back auto-save state back the value before the last edit and to the value that tab 1 also has
12. 1st tab make an edit <FormattedText>{title} 5</FormattedText>
13. 1st tab does not error: This this is expected and correct, even though the latest save was on tab 2 the 1st tab has code that matches the latest auto-save state, so it should not error

There is one open thread for @jessebaker from @lauriii on the front-end code but I think this is ready for review

Thanks for the updated issue summary, made it much easier to dive right in!

Hope my refinements to the issue summary make it easier for the next person.

Impressive MR!

2 main concerns:

1. Cacheability regression: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
2. The clarity and maintainability of both \Drupal\Tests\experience_builder\Kernel\AutoSave\AutoSaveConflictTestTrait::testOutdatedAutoSave() and the code that powers it: \Drupal\experience_builder\Controller\AutoSaveValidateTrait::validateAutoSaves(). Tried to self-address that in https://git.drupalcode.org/project/experience_builder/-/merge_requests/1..., up to @tedbow to review 👍

Follow-up for #25.1: turns out it's misleading because all of this is in a kernel test. We'll need a stable blocker follow-up to convert all these to functional tests. 🙏 See https://git.drupalcode.org/project/experience_builder/-/merge_requests/1....

I'd just like to see https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... removed or clarified now, assuming @tedbow is +1 to the commits I pushed in #25.

Added follow-up 📌 Convert Autosave conflict tests using AutoSaveConflictTestTrait into functional test to test caching Active

I merged in 0.x. there were a lot of JS changes which I think I got right, but will shall see 🤞🏻

Went through the manual testing again. everything ok

Re #26

1. I have create the follow-up 📌 Convert Autosave conflict tests using AutoSaveConflictTestTrait into functional test to test caching Active and added todos https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
2. reviewed changes made that were referenced in #25

   Mostly it look good, updated some comments https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

3. Only problem with the expanded test coverage @wim leers added was that auto-save entry does not get deleted for non-config entities when the entity is saved, because that happens via \Drupal\experience_builder\AutoSave\AutoSaveManager::onXbConfigEntitySave(). fixed in https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

#30:

1. 👍 Thanks!
2. Yay, thanks for the rectification, hope this will help the next reviewer/maintainer 😊
3. 🤔 Isn't that a bug then? How does an auto-save entry targeting a no-longer-existing-as-it-was content entity (revision) make sense?

Going through this one last time 🤓🤞

#31 3)
I think maybe we should be calculating autoSaveStartingPoint in \Drupal\experience_builder\AutoSave\AutoSaveManager::saveEntity() in but only in the case where $original_hash === NULL, where know there wasn't an existing auto-save entry so we actually do know the starting point.

Calculating in \Drupal\experience_builder\Controller\AutoSaveValidateTrait::getClientAutoSaveData might actually be problematic.

This looks solid now, thanks @tedbow! 🥳

Two follow-ups needed:

1. #30.3 is AFAICT a bug that needs follow-up — it means garbage can pile up in auto-save? I just tested it and it allows me to continue editing, but then I end up being stuck with this validation error:
   

   The content has either been modified by another user, or you have already submitted modifications. As a result, your changes cannot be saved.

   So: it's nice that this keeps your existing auto-save, but then you kinda can't do anything with it? 😅 I think we need a follow-up to make this more granular for content entities: it should be possible to discard all changes , which would then allow you to "re-target" your existing work to the latest content entity revision.

   This is of course a pre-existing problem, so not blocking this MR over it. It's just that the new test coverage here and https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... in particular made much more obvious.

2. I suspect a bunch of complexity in this issue could have been avoided if we'd updated \Drupal\experience_builder\AutoSaveEntity instead of sprinkling extra cacheability logic all over. But this too is not being introduced here. Described in some more detail what I think would significantly simplify all this: 📌 Add methods to `AutoSaveEntity` value object to simplify auto-save infrastructure Active .

IOW: I created 50% of the needed follow-ups already :D

@wim leers ok https://git.drupalcode.org/project/experience_builder/-/merge_requests/1270 might be a starting point for 📌 Add methods to `AutoSaveEntity` value object to simplify auto-save infrastructure Active though I did it slight differently

Awesome!

Can you still create a follow-up for #33.1 though? 🙏

Tested changes on branch 0.x , for following scenarios :

Thanks for the good news, @neha_bawankar!

Wim here is the follow-up for #30.3 🐛 Content being edited XB will error on publishing if saved outside of XB Active

wim leers → closed merge request !1270

Thanks!

It'd be nice to also still add a screenshot/GIF showing this in action 😇

Also closed https://git.drupalcode.org/project/experience_builder/-/merge_requests/1270 since ~1204 was merged.

Automatically closed - issue fixed for 2 weeks with no activity.