Add a route for PATCHing both a config entity and its auto-saved version together

How do we know which config entity properties this is safe to do for?

Would it be okay to start with only status? 🙏

A way to think of this is: we want to apply a "hot fix" to both the production branch (the published entity) and the development branch (the auto-saved draft).

Helpful metaphor 👍

Create a 3rd route

Why? Why not:

diff --git a/src/Controller/ApiConfigControllers.php b/src/Controller/ApiConfigControllers.php
index e308597fe..c489692e3 100644
--- a/src/Controller/ApiConfigControllers.php
+++ b/src/Controller/ApiConfigControllers.php
@@ -182,6 +182,7 @@ final class ApiConfigControllers extends ApiControllerBase {
     }
 
     // Save the XB config entity, respond with a 200.
+    // @todo Also update the auto-save version with the changed fields. Do this using a \Drupal\Core\Config\ConfigEvents::SAVE subscriber.
     $xb_config_entity->save();
     $xb_config_entity_type = $xb_config_entity->getEntityType();
     assert($xb_config_entity_type instanceof ConfigEntityTypeInterface);

Crafting a draft MR.

Another reason to do what I proposed in #4 is to prevent auto-saves being wiped when doing a configuration sync: imagine that the label of a JavaScriptComponent config entity is modified by the config sync, then you don't want to lose your auto-saved changes to the actual JS code.

Outline of MR pushed.

What I'm proposing is essentially a subset of 📌 META: Conflict free concurrent editing Active .

This blocks 🐛 Adding component to component library results in component code and configuration being lost Active .

wim leers → credited larowlan → .

Confirmed by @hooroomoo at #3513147-13: [PP-1] Using actions from the contextual menu from the sidebar list overrides code component with its latest non-autosaved version → that this fixes the problem. (But there's another bug they surface there. So we can't merge this until @hooroomoo clarifies at #3513147-14: [PP-1] Using actions from the contextual menu from the sidebar list overrides code component with its latest non-autosaved version → how to reproduce that bug.)

Needs sign-off from an auto-save maintainer. @larowlan is out, so @tedbow, over to you!

Looks good! will merge on green tests

Quoting #12:

(But there's another bug they surface there. So we can't merge this until @hooroomoo clarifies at #3513147-14: [PP-1] Using actions from the contextual menu from the sidebar list overrides code component with its latest non-autosaved version → how to reproduce that bug.)

So just to preface, all my testing has been with sending PATCH requests to /xb/api/config/js_component/${id}, not the auto-save because that's what's currently in the code.

Could you confirm if client side PATCH requests to change the status or the label should still be sent to /xb/api/config/js_component/${id} or does it need to change to send it to

/xb.api/config/auto-save/js_component/${id}?

<strong>1. WITHOUT frontend change</strong>
This issue I was seeing on this MR (with no frontend changes) turned out to also exist in 0.x. The bug (let's call this Bug #1) is

<ol>
  <li>Create and add a code component to your library (using top right Add to components button in code editor)</li>
  <li>Make an edit to the code from the library</li>
  <li>Exit out of the code editor</li>
  <li>All the styling (CSS) is gone from the component in the layout preview. </li>
</ol>

Since the bug also exists in 0.x, that means this MR is not the cause of it.

<strong>2.) WITH frontend change to PATCH request to only send necessary changes.(Currently in 0.x, PATCH requests are still sending the entire component )</strong>

<ol>
  <li>Create a new code component</li>
  <li>Trigger an auto-save and then click "Add to components" from the top right in the code editor</li>
  <li>Add the component to your page preview. It doesn't render anything. </li>
  <li>If you look in your <code>/sites/default/files/astro-island

nothing is generated from your newly created code component

This behavior exists on both 0.x and this MR, WITH the frontend change to the PATCH request of only sending wanted changes.
----------------------------------------

So my conclusion: I don't think merging in this MR will cause any regressions that don't already exist in 0.x, HOWEVER, we would need to open a second bug report for the 2nd issue I described because the frontend needs to eventually change the PATCH requests to only send wanted changes.

Could you confirm if client side PATCH requests to change the status or the label should still be sent to /xb/api/config/js_component/${id} or does it need to change to send it to /xb.api/config/auto-save/js_component/${id}?

The answer depends:

• If you need the changes to first appear under "view changes" prior to publishing everything together: /xb/api/config/auto-save/js_component/…
• If you need the changes to NOT first appear there but be "immediate" (e.g. the "add to components" button), then: /xb/api/config/js_component/…

@tedbow If you have time, could you please investigate #16?

Investigating

Investigated #16.1 with @hooroomoo , created a new issue 🐛 Auto-saved Javascript Components CSS changes do not work with CSS aggregation Active ,

For #16.2 I think we figured out that it is NOT an issue with the current MR.

which makes sense because in 0.x we would be deleting the auto-saved entry of the component if you just sent the status, while in 0.x we would just updating the status in the auto-save entry

@hooroomoo can you confirm?

I think the issue i was seeing in 16.2 had to do with my css aggregation being on or something else weird because I tested it again with css aggregation OFF, and I'm able to correctly see the autosaved component in my page preview. Also renders correctly after publishing.

I think this MR is good to go. I added the frontend changes to change the PATCH request to only send wanted changes and it works as expected.

This MR should fix both:

🐛 Adding component to component library results in component code and configuration being lost Active 🐛 Cannot add component to the component library using the contextual menu Active

This looks good to me, but there's a failing E2E test that looks like it could be related to this MR, not just a flaky test.

I've seen the same assertion fail on different MRs as well as on 0.x so I think it might just be the flaky test and there was an issue filed for it 📌 Cypress test publish-validation is flaky Active .

The fail is definitely unrelated to the frontend change. But maybe @tedbow could confirm if the fail is unrelated to the backend changes

experience_builder.auto_save.inc doesn't exist in 0.x anymore since 📌 Move to OO hooks now we require 11.x Active got in so this needs a rebase

re #24 and #25 I ran the failing tests locally on 0.x and the MR branch.

publish-validation.cy.js consistently passes for me locally on both branches

component-operations.cy.js consistently passes for me on 0.x and consistently fails on the MR branch 😢. I haven't looked into why yet

@hooroomoo have you run the tests locally? Would be good to double check if someone else gets the same result

Assigning back to @tedbow to fix the merge conflicts with 0.x and hoping for a successful test run post-rebase

I'll tackle that since I worked on 📌 Move to OO hooks now we require 11.x Active .

The e2e test failures have been fixed in 🐛 [pending] In progress Active .

Merging given the approvals by @hooroomoo and @tedbow — thanks both of you for the intense collaboration on this issue cluster! 😄🙏