Concurrent editing of layouts is very confusing

Thanks for the patch. I reviewed the code for clarity and code formatting and didn't see anything obviously wrong. I compared the code against some of the Views lock code and when looking at this:

  /**
   * If this view is locked for editing.
   *
   * If this view is locked it will contain the result of
   * \Drupal\Core\TempStore\SharedTempStore::getMetadata(). Which can be a stdClass or
   * NULL.
   *
   * @var object
   */
  public $lock;

I'm wondering why the Lock class was created rather than using the same approach as Views. And, if the Lock class is better, then should Views be refactored to use that?

I'll do some testing.

I tested this by:

1. Created "anotheradmin" user (so there would be two admins)
2. Enabled the layout modules
3. Configured the basic page content type to use layout per node
4. Created node/1
5. Went to layout page for node/1 as admin and made some changes but didn't save
6. Went to layout page for node/1 as anotheradmin and saw the locked warning
7. Clicked "break this lock" and continued with form to break the lock which was successful
8. Then I reversed the process and started as anotheradmin and then tried as admin and saw the locked warning for the admin account
9. As admin, I broke the lock and made changes and they were saved successfully

One oddity I found was after I broke the lock with the form and got back to the layout page, if I reloaded the page, it would show a message that there were unsaved changes even when there weren't any changes. This might not be due to this patch but it seems like it might be. Given that and my previous comment in #9 I'll mark this back to needs work for feedback.

Given comments from #11, marking RTBC.

+++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
@@ -344,21 +386,4 @@ public function saveLayout(SectionStorageInterface $section_storage) {
-  /**
-   * Cancels the layout.
-   *
-   * @param \Drupal\layout_builder\SectionStorageInterface $section_storage
-   *   The section storage.
-   *
-   * @return \Symfony\Component\HttpFoundation\RedirectResponse
-   *   A redirect response.
-   */
-  public function cancelLayout(SectionStorageInterface $section_storage) {
-    $this->layoutTempstoreRepository->delete($section_storage);
-
-    $this->messenger->addMessage($this->t('The changes to the layout have been discarded.'));
-
-    return new RedirectResponse($section_storage->getRedirectUrl()->setAbsolute()->toString());
-  }

At a first read, this seems out of scope?

So given the impact on user input (where the user gets seemingly broken and irrelevant data on editing a layout the same time as someone else), this is a major bug.

I discussed this issue with @tim.plunkett a bit. There are two parts to the fix:

1. Converting the "cancel layout" link to use a confirmation form.
2. Adding the same locking pattern that Views uses to fix the concurrent editing behavior.

I suggest that we split the first off into its own blocking issue, since it's a good change on its own. For the second, reviewing it here is a little difficult because we have a slightly forked, slightly improved version of the API that the Views tempstore uses, which I don't think we should do. @tim.plunkett indicated that his main concern was using a stdClass object to pass around the data the way that Views currently does. We agreed to work on an upstream patch to fix that. At a minimum, knowing what the upstream patch will look like will make this issue easier to review, and ideally, we can just make that upstream fix without adding technical debt for this issue on top of what already exists in views.

Thanks!

+++ b/core/modules/layout_builder/src/Form/DiscardLayoutChangesForm.php
@@ -84,6 +103,8 @@ public function getCancelUrl() {
+    $lock = $this->layoutTempstoreRepository->getLock($section_storage);
+    $this->isOwnerCurrentUser = $lock && $lock->getOwnerId() === $this->currentUser()->id();

It's not a big deal, but I'm not clear why we need to store this in a boolean property just for getQuestion(). Perhaps a new protected method would be better?

Other than that, I think I have no complaints. Patch seems very straightforward and consistent with the Views UI locking mechanism.

Thought about it a bit, and since the class is relatively sparse, I think this is probably fine. I'd be less comfortable with it if this were a more complex form, but for what it is, I don't think there are too many risks here. RTBC.

Does this need to be manually tested again?

FWIW I would end up manually testing this myself before commit if the testing were not repeated, so if #10 were repeated with the final patch that would save committer time. :)

I tried to test the patch on simplytest.me a few times but it kept bombing out. I'll try to remember to try again tomorrow.

One oddity I found was after I broke the lock with the form and got back to the layout page, if I reloaded the page, it would show a message that there were unsaved changes even when there weren't any changes. This might not be due to this patch but it seems like it might be. Given that and my previous comment in #9 I'll mark this back to needs work for feedback.

I tested with defaults (instead of overrides) using #10's steps and did not get this message with the patch applied. However, I did get redirected back to the "Manage display" page when I broke the lock, instead of the form I was on, which is confusing. Marking NW for that.

I briefly tested this, using defaults (not overrides), and was redirected back to the layout editing page when I broke the lock. Based on the last few comments, that seems to be the desired behavior. It worked for me, then, so I guess this is good to go.

+++ b/core/modules/layout_builder/src/Form/DiscardLayoutChangesForm.php
@@ -69,14 +88,14 @@ public function getFormId() {
-    return $this->t('Are you sure you want to discard your layout changes?');
+    return $this->isOwnerCurrentUser ? $this->t('Are you sure you want to discard your layout changes?') : $this->t('Are you sure you want to break the lock on the layout changes?');

I think the message or description here should indicate that "breaking the lock" means throwing away the other user's changes. (Edit: The "break this lock" part of the message on the UI itself should probably indicate that as well.)

I know the original string message about the existence of the lock is from the Views UI, but the confirm form saying "Are you sure you want to break this lock?" does not give the user enough information. It just says "Are you sure you want to break this lock? This action cannot be undone." Which sounds fairly harmless. But it's deleting another person's work and data from the database.

Tagging for UX review.

I am talking about core/modules/layout_builder/src/Form/DiscardLayoutChangesForm.php, a change made to it in this patch. See #31. It's not in Drupal\Core.

Also, let's please not use Views UI as our standard of acceptable UX for LB (or anything). :P Views UI is very very admin-facing; LB is OTOH available to content authors.

Screenshots for the UX review. The user clicks the "break this lock" link in this message:

That link takes them to this form:

The result of the action is that another user's unsaved changes to the layout are lost forever, including their unsaved changes if they are on the form making edits right now. I don't think "break the lock on layout changes" sufficiently conveys to the user that the in-progress changes from @otheruser are going to be immediately and permanently lost.

Just testing with defaults but presumably this would happen with overrides.

1. If the layout is locked by another user the the user who sees the lock message still sees the "Save" and "Discard" links
   
   These links also work so the other locked out user can actually discard or save the active users work with these links.

   We don't want to add an access check for locks in \Drupal\layout_builder\Access\LayoutBuilderAccessCheck::access() because this would stop the user from even seeing the Manage layout page with the lock message.

   But we could make another another access checker service and tag routes like we are tagging with '_layout_builder_access'. Maybe '_layout_builder_locked_access'. Then check if the section storage is locked by another user so we don't allow access Save and Discard links.

2. This is more of an edge case but if 2 users open up the Manage Layout page before either one actually saves a change they will both be able to make changes. This is because the layout builder is loaded via ajax and the lock message will never show.

   I think if we add the new access tag '_layout_builder_locked_access' to all the "add section" "add block", etc routes then this would stop the both users from making changes at the same time. It would be good if we could detect the 403 on the dilag and reroute the page back to manage layout where they would then see the lock message.

This will need to be updated with:

diff --git a/core/modules/layout_builder/src/LayoutTempstoreRepository.php b/core/modules/layout_builder/src/LayoutTempstoreRepository.php
index a2fad4d5f0..ed61a975c4 100644
--- a/core/modules/layout_builder/src/LayoutTempstoreRepository.php
+++ b/core/modules/layout_builder/src/LayoutTempstoreRepository.php
@@ -49,7 +49,7 @@ public function get(SectionStorageInterface $section_storage) {
    * {@inheritdoc}
    */
   public function getLock(SectionStorageInterface $section_storage) {
-    $id = $section_storage->getStorageId();
+    $id = $this->getKey($section_storage);
     return $this->getTempstore($section_storage)->getMetadata($id);
   }
 

following #3026698: Allow section storage to provide a more granular ID for tempstore.

Edit: Forgot to mention the above is from @tim.plunkett telling me what to type. :D

1. +++ b/core/modules/layout_builder/src/Access/LayoutBuilderLockAccessCheck.php
   @@ -0,0 +1,54 @@
   +  public function __construct(LayoutTempstoreRepositoryInterface $layout_tempstore_repository) {
   ...
   +  protected $layoutTempstoreRepository;
   

   The properties should probably be in top of the class.

2. +++ b/core/modules/layout_builder/src/Access/LayoutBuilderLockAccessCheck.php
   @@ -0,0 +1,54 @@
   +   * Checks for a lock from another user on the layout.
   

   Maybe change to: Checks for a lock on the layout

+++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
@@ -99,6 +112,27 @@ public function preRender($element) {
+    return \Drupal::service('renderer')->render($message_array);

Should be injected as well(?)

#56 Those changes looks great!

Did some manual testing. The base functionality works as expected. Awesome!

I think it would be more nice if the message was in the content area, and the other UI elements (i.e. revisions) was hidden and maybe also the "You are editing message", because you aren't really editing it, right? IMO, this can be in a follow-up.

Did some edge case testing as well:

Assume two user's has the same layout opened, but they have not yet done any changes. If one of the user adds the block, they are now the owner. If the second user now tries to add a block they will get a 403 as expected, yay! Though there are no message explaining why, so it just looks like the UI is broken. Should we adress that? Maybe in follow-up?

+++ b/core/modules/layout_builder/tests/src/Functional/Update/TempstoreKeyUpdatePathTest.php
@@ -34,18 +35,28 @@ public function testRunUpdates() {
+    if ($account->id() == 1) {
...
+    else {

Why do we have a if-else here?

  /**
   * Tests the upgrade path for Layout Builder extra fields.
   */

Not showing in patch, but the comment for the test is wrong.

1. +++ b/core/modules/layout_builder/src/Access/LayoutBuilderLockAccessCheck.php
   @@ -0,0 +1,54 @@
   +/**
   + * Checks access based on whether the layout is locked.
   

   The term "locked" could use a bit of explanation here. How about "...is locked (i.e., changed by another user)"?

2. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -57,11 +74,21 @@ class LayoutBuilder extends RenderElement implements ContainerFactoryPluginInter
   +   * @param \Drupal\Core\Session\AccountInterface $current_user
   +   *   The current user.
   +   * @param \Drupal\Core\Render\RendererInterface|null $renderer
   +   *   The renderer.
   

   Even though these are technically optional, not passing them triggers deprecation errors, so they're not truly optional. Therefore, +1 on not starting the descriptions with "(optional)".

3. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -57,11 +74,21 @@ class LayoutBuilder extends RenderElement implements ContainerFactoryPluginInter
   +    $this->currentUser = $current_user;
   

   Why doesn't this parameter also trigger a deprecation notice?

4. +++ b/core/modules/layout_builder/src/Form/DiscardLayoutChangesForm.php
   @@ -87,6 +106,10 @@ public function getCancelUrl() {
   +    $this->isOwnerCurrentUser = !$lock ? TRUE : $lock->getOwnerId() === $this->currentUser()->id();
   

   I find this logic jarring; there's a weird double-negative introduced by switching on !$lock. I guess this is a nitpick, but ternary expressions really read better (to me, anyway; I'm not sure if I'm in the majority here) if the thing being tested isn't inverted. If it's not too much of an eye-roller, can this be changed? :)

5. +++ b/core/modules/layout_builder/src/Form/DiscardLayoutChangesForm.php
   @@ -98,7 +121,10 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    // If the user is discarding their own changes, redirect as usual. If they
   +    // are breaking the lock of another user's changes, redirect them back to
   +    // the Layout Builder UI to make their own changes.
   +    $form_state->setRedirectUrl($this->isOwnerCurrentUser ? $this->sectionStorage->getRedirectUrl() : $this->sectionStorage->getLayoutBuilderUrl());
   

   Nice, clear comment. Thanks!

6. +++ b/core/modules/layout_builder/src/LayoutTempstoreRepositoryInterface.php
   @@ -22,6 +22,17 @@ interface LayoutTempstoreRepositoryInterface {
   +  /**
   +   * Returns the lock object.
   +   *
   +   * @param \Drupal\layout_builder\SectionStorageInterface $section_storage
   +   *   The section storage.
   +   *
   +   * @return \Drupal\Core\TempStore\Lock|null
   +   *   The lock object, or NULL if none exists.
   +   */
   +  public function getLock(SectionStorageInterface $section_storage);
   

   This is a good API addition, but I think it also condemns this patch to 8.8.x and beyond only. So 8.7.x users will need to deal with confusing concurrency when editing layouts. :(

@tim.plunkett also asked me for my opinion on the discussion that took place between #31 and #35. I completely agree with @xjm -- we should not use the terminology of Views UI. In my ideal world, we wouldn't even use the word "lock" in this warning; that's a technical term we'd be exposing to content editors. My preferred wording would be something like this: "This layout is being edited by $other_user. If you edit this layout now, their changes will be lost. [link]Continue editing the layout anyway[/link]."

I like that the break_lock_link stuff is abstracted into a render element, but I don't really like the rigidity of its phrasing. So what if we added a new property to it with a default value that keeps the current wording? Something like:

'#question' => ''This @label is being edited by user @user, and is therefore locked from editing by others. This lock is @age old. Click here to <a href=":url">break this lock</a>.'

Using predefined placeholders (like formatPlural() does), calling code could easily customize the phrasing of the link:

$link = [
  '#type' => 'break_lock_link',
  '#question' => 'This layout is being edited by @user. If you edit this layout now, their changes will be lost. <a href=":url">Continue editing the layout anyway</a>.',
];

In #59 there were test failures in LayoutBuilderUiTest and LayoutBuilderTest... fixed half of them

Fixed LayoutBuilderUiTest by changing LayoutRebuildTrait to refresh the entire entity form, not just the layout builder render element. This required a decent amount of refactoring, which is what the majority of changes in this patch came from. It also surfaced a problem reported in #3045171: Form blocks rendered inside layout builder break save, where adding the search block to a layout would prevent the layout from being saveable. The solution in this patch fixes the issue without removing the form from the Layout Builder UI.

I was not able to fix LayoutBuilderTest::testConcurrentEditing(), but I was able to narrow down it down enough that someone may be able to identify the problem. The test fails when a user unlocks the layout to make it editable, but when they return to the layout it is still locked by the other user. This does not happen with manual testing, and I added assertions to the test that confirm the layout's temp storage has been cleared. To rule out a timing issue, I tried refreshing the page and adding a pause before going to the Layout Builder UI, but the test still thinks the layout is locked.

There are also some changes where I moved code shared by DefaultSectionStorage and OverridesSectionStorage into a shared trait.

This addresses all the items in #62

#62.1 created a FormEditableSectionStorageInterface interface with a method for getting the entity directly from section storage. For section storage instances that don't implement the interface, the rebuild process after an ajax update will rebuild layout builder UI as opposed to the entire entity form

#62.3 the weird double quotes were just the byproduct of fatigue -- changed to single.

#62

Can't this use getLayoutBuilderUrl in both?

yeppp... (see above)

Still not sure how to address the failure in \Drupal\Tests\layout_builder\Functional\LayoutBuilderTest::testConcurrentEditing, another set of eyes on that would be appreciated. Manual tests work, and the test confirms the tempstore is emptied, but the UI within the test does not reflect that.

Tests were failing because Layout Builder element was set to be uncacheable, but not the entity forms. Dynamic page cache was retaining the markup generated while locked.

I also expanded the concurrent editing test to cover overrides in addition to defaults and removed a method I added in a previous patch that wasn't supposed to be there to begin with (artifact of some earlier attempts at changing the AJAX refresh)

Corrected unit tests to reflect the changed cache config for layout entity form routes.

#67 is a mistake as the branch was not rebased. Here is the corrected patch, interdiffed from #65

I haven't read or tested this patch yet to know if it applies to the Layout tab of an entity that's using overrides. Under the assumption that it does, I added #3063373: Add content locking (anti-concurrent editing) to core? to the Ideas queue.

Rerolled patch for 9.1.x, please review.

Little bit busy in next couple of days. Later will plan to work on it.

@tim.plunkett It was a reroll.

Re-rolling #72, as patch was failing against 8.9.x.

Patch #84 works with 8.9.x as well, please ignore #88 re-roll.

I have applied #84 and it seems to be working, however if we use this patch along with layout_builder_modal module, it introduces an issue where modal popup on layout while adding or updating block does not close.

Steps to reproduce:

• On fresh drupal install apply the patch #84
• Install and enable content_lock and layout_builder_modal module
• You can enable content lock for layout by visiting admin/config/content/content_lock.
• Go to layout builder page of a node and add a block ,a modal popup will open
• Try to save the modal, the data in the background saved but modal popup does not close.

Re-roll patch for 9.2

Looks like the MR needs a reroll (rebase).

Does this still need tests? Can that tag be removed.

For the re-roller, please try to debug the test fails too rather than a straight re-roll.

We also need type-hints and return type-hints here on the new code.

Didn't comment on each line that needed it in the MR, because it applies to all new methods.

Creating patch for 10.1

try to fix #105

@timplunkett
Can you pls change the target branch to 10.1.x for this MR #322 rerolled it to 10.1.x branch.

As it has review comments.