🇺🇸United States @tedbow

There was e2e test failure on global-regions.cy.js but I ran locally and passed

re #47 ok working on it

Back-end changes look good

This looks good to me but publish-validation.cy.js has a merge conflict I tried merge locally but then this test fails. Not sure if was merge conflict resolution or something we the actual test. I am not pushing my merge because it would probably just make thing worse

Either update ApiAutoSaveController::post() add a new route requirement-level access check (TBD which of the two is best), which must:

• iterate over all content + config entities in the auto-save store
• throw a CacheableAccessDeniedHttpException if any of them fail on $entity->access(operation: 'update', return_as_object: TRUE)
• if a content entity, iterate over all changed fields, throw a CacheableAccessDeniedHttpException if any of them fail on $entity->get($field_name)->access(operation: 'edit', return_as_object: TRUE)

This has not been done.

I double checked changing \Drupal\Tests\experience_builder\Kernel\ApiAutoSaveControllerTest::testApiAutoSaveControllerPost

To check if user with no entity specific permission could publish entities and they could

$this->setUpCurrentUser(permissions: [
      'access administration pages'
    ]);
    $response = $this->makePublishAllRequest();
    $json = json_decode($response->getContent() ?: '', TRUE);
    self::assertEquals(Response::HTTP_OK, $response->getStatusCode());
    self::assertEquals(['message' => \sprintf('Successfully published %d items.', $withGlobal ? 6 : 5)], $json);

Seems like we need a new issue for that

sound right

re #5 Test fail did happen

Not clear on why

Creating 🐛 Fields values the user has view but not edit access can be saved to AutoSave Needs work for #41.1

Ok. I am guessing the change will make entity-form-field-types-test.cy.js fail for the field field_xbt_comment see https://git.drupalcode.org/project/experience_builder/-/merge_requests/9...

Not sure it if is related but I did see problem with field_xbt_comment in 🐛 Radio button boolean fields can sometimes can be set in page data form Active

Update the summary, not need to start the MR from the work on 📌 [PP-1] Add entity access checks to routes that deal with entities Postponed

Leaving assigned to myself. See not in summary

NOT DONE WITH SUMMARY PLEASE LET ME FINISH BEFORE ISSUE IS WORKED ON ETC(🙏tedbow)

Will finish later today, there are few steps I want to explain. Then I will bring over changes from 📌 [PP-1] Add entity access checks to routes that deal with entities Postponed to start the MR

tedbow → created an issue.

I am making the issue for #41.1 now

Needs work for phpstan and probably improve the tests. I think the e2e test are broken in HEAD but since this is only phpunit test changes, it could probably committed while they are still broken

tedbow → created an issue.

re #38
I agree the scope has been blurred in this issue.

I think we should make 2 new issues out of the tests in 3494915-field-access

1. \Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPostTest::testEntityAccessRequired This is meant to prove that user without access to "sticky" field cannot get it set in the auto-save. Right now this test fails in 0.x but passes in this branch. Hence we know it needs the changes in \Drupal\experience_builder\Controller\ApiLayoutController::buildPreviewRenderable because those are the only non-changes in non-test code.

   I think the problem is that this will cause an e2e test to fail but right now I can't e2e test to run this MR and I can't the tests to pass on my local even on 0.x. But we can figure it out in the issue we create.

2. \Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPostTest::testEntityFieldAccessRequiredThrowsConstraintViolation passes in both 0.x and the MR. So we know it doesn't need any non-test changes.

   also we adds test coverage that we are missing for \Drupal\experience_builder\ClientDataToEntityConverter::checkPatchFieldAccess where it does throw new AccessException("The current user is not allowed to update the field '$field_name'.");. This seems like something we should really test.

   Not sure if ,\Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPostTest::testEntityFieldAccessRequiredThrowsConstraintViolation is exactly the right way of it should be added to ClientDataToEntityConverterTest or if we should have both but the new issue can figure that out.

   there is the 2nd problem that as it is not testEntityFieldAccessRequiredThrowsConstraintViolation also sets "sticky" and should throw an exception is that but we could add a todo to the first issue as to why we aren't using sticky

tedbow → created an issue.

The only 2 e2e tests that are failing now are `entity-form-field-types-test.cy.js` which does pass for me locally and `media-library.cy.js` which did fail locally but I didn't see an 409 request errors in the log. So maybe these are unrelated random test errors? Not sure, retesting

I brought in the changes from 📌 Add rudimentary conflict prevention to the preview end-point Active because I made progress getting the e2e test to pass, so I thought I would the current state

chatted with @wim leers myself or others(or wim) could review. I will look at this tomorrow if it still needs review

To address @larowlan's feedback I have made this a backend only issue and follow-up handle the Frontend work. I create 📌 [PP-1] Enforce conflict enforcement outside of tests and e2e tests Postponed as follow-up

In that issue I pointed to the commits that remove the /ui if the person tackling that wants to start from my work

tedbow → created an issue.

so I don't forget

The 3473761-empty-slot MR has phpunit test failures I will look into.

I re-running the failed e2e tests on the other MR

#13 sounds like @wim leers was ready to merge this, so assigning back to him

I think this could probably use some other eyes on before I work more on it.

So far it is the back-end implementation I mentioned in #6.

I gave a first pass(with AI help at the front). The front-end changes work with the caveat I mention in the MR about rapid fire requests.
The last test that is failing for me locally is entity-form-field-types-test.cy.js when the test loop through many different fields, so I am pretty sure this is because of the same problem. I could probably get the tests passing with the correct waiting on requests. I have fixed a few test this way in this issue already.

re #12 and #13 I actually realized that modules/contrib/experience_builder/src/Controller/ApiAutoSaveController.php:107 already returns hash of auto-save entries so this is not new problem.

So I have reverted back to using the hash

To address #12 I am just going to use UUID that will get saved with auto-save each time.

I didn't want to use an auto-incremented number because a malicious user could just guess the current ones to purposefully wipe of another users changes and save there's.

I am wondering if there will be any security concerns with sending the actual hash of the auto-save to the client.

Right now I don't think there are because we pretty much send the contents of the auto save to the client via \Drupal\experience_builder\Controller\ApiLayoutController::get anyways, so having the hash is not giving any more information.

but after 🐛 If user with less/different permissions edits a page after a user with more/different permissions data lost could occur Active we may store fields in the auto-save that user does not view or edit access to. We will then probably fitler what's in the auto-save before we send it to the client.

In that case imagine this scenario

1. the only field the user does not have access to is a boolean field.
2. the user knows the name of the field
3. the knowledge from the source code the hash is actually of whole auto-save including the field they don't have access to.

Would it be possible for the user to reconstruct the complete auto-save data from the information they have andgenerate the has themselves, 1 time with the boolean field FALSE and 1 time with the boolean field TRUE and then just check which one matches the hash they were sent?
I know it might be an edge case but this could also work with any field that has limited number of possible values

My other thought is to just send a auto-save version number.

Initially in this issue @larowlan suggested

For each item in the model, keep a version number, initialize this to 1 when we first write to the auto-save entry for that entity.

I am not sure he was considering this or he just suggested using a version number because of security concerns or just he was referring to sub-items of the auto-save we don't have hashes already for.

This is still very rough. I will pick it back up tomorrow

Right now in the MR I have only worked on getting the 3 \Drupal\Tests\experience_builder\Kernel\ApiLayoutController*Test classes to pass

a bunch of other things should break because they are not sending the new 'autoSaves' key back to the server. Probably all of the e2e tests should fail

working on this

I need to update the summary more and figure out the relationship to 📌 [PP-1] Add entity access checks to routes that deal with entities Postponed which I also need to review

I tagged this "Needs tests" because someone could actually write a test that confirms the problem even if don't know that actually solution yet.

tedbow → created an issue.

This might be fixed by 📌 Publish checks validation constraints, but not form validation Active

re #4 and #5
I chatted with @lauriii, @effulgentsia and @balintbrews about this on yesterday about this.

The decision was that as first pass we should only check that the user has the latest version of auto-save, probably by comparing that last known auto-save.

so re #5

The auto save hashes are at the whole entity level. But we need them at the component level

I agree this is not an as ideal as what you have suggested, it still would allow us to add more advanced conflict detection later.

Do we need to update `\Drupal\experience_builder\PropSource\DynamicPropSource::calculateDependencies` or create another issue for that? Maybe this will become obvious as I review this more. But thought I would put it here not to forgot or if someone else knows

tedbow → made their first commit to this issue’s fork.

This looks to me. I guess generally if people are ok with the user setting paths to executables then I am ok with it. 'administer software updates' is already very powerful permission and should not be given to many people

Just went over this again with @phenaproxima, looks good!

This is rough POC but I think it solves the problem in 🐛 JS component slots don't appear in the preview canvas until published Active while allowing the client to only send status: true when adding the code component to the library.

the other thing this would allow is adding new props and slots after a component has been added to the library. there are probably other hurdles to overcome for this but this does allow the component list use the auto-save version of code components which would not affect currently published pages with published code components in them

tedbow → created an issue.

Looks good

component-operations.cy.js is failing on 1 test but it does fail randomly on CI. Checked locally and it passes for me

tedbow → made their first commit to this issue’s fork.

I merged this as is because there some issues I think I would like to merge and I don't feel qualified to make the core follow-up. I would rather not merged the other issue with failing phpunit tests.

so setting back to Needs Work and assigning to @longwave to create another MR for the core follow-up link

tedbow → made their first commit to this issue’s fork.

phpunit test failure is 🐛 JavaScriptComponentValidationTest::testInvalidSlotIdentifiedByConfigSchema fails Active

retesting e2e tests

This looks good to me @thoward216 did the test changes @wim leers asked for in #18

phpunit test failure is because of 🐛 JavaScriptComponentValidationTest::testInvalidSlotIdentifiedByConfigSchema fails Active . retesting e2e tests to see if it was random

@bnjmnm thanks for the review. I implemented your suggestions, though one in slightly different way, see MR comments.

Let me know

the phpunit test failures is from 🐛 JavaScriptComponentValidationTest::testInvalidSlotIdentifiedByConfigSchema fails Active , so unrelated

working on the fix. I think for now we should allow e2e test to opt of aggregation

@wim leers actually I have another idea that I push up in a MR to use `EntityConstraintViolationList`. See my comments on the as to why it is hard to test right now. If you think the MR is good idea though I could look at testing it.

🐛 Global AssetLibrary should render with its auto-saved state (if any) when rendered in the XB UI Active is fixed

🎉

Thanks everyone!

re #12 I created 🐛 Create loadAutoSave() helper function Active as follow-up for 2) and refactored for 1)

tedbow → created an issue.

2 e2e test fails, passed locally. PHPUnit tests passed

re #13.2 that seems to be known side effect of 📌 Compile Tailwind CSS globally for code components Active . It will be fixed by 🐛 Global AssetLibrary should render with its auto-saved state (if any) when rendered in the XB UI Active

Postponing on #3508937 because it will be easier to test after that

phpunit test fails

1. re #12

   Instead of enabling aggregation for XbPageVariantTest I enabled aggregation in \Drupal\Tests\experience_builder\Functional\FunctionalTestBase::setUp which is the base test that XbPageVariantTest extends. This way all our functional tests will run with CSS aggregation on not just XbPageVariantTest.

   I since found the only test that fails in this situation is \Drupal\Tests\experience_builder\Functional\AssetLibraryAttachmentTest because we are checking for CSS and JS file paths in the HTML, so I disabled aggregation just for this test

2. During manual testing I was finding weird behavior where the code component do not look like do in the editor preview. This affects user 1 also with aggregation off. I have tried testing on 0.x and still see the problem. so I am investigating that.

re #8

also enable CSS + JS aggregation for \Drupal\Tests\experience_builder\Functional\XbPageVariantTest

Since XbPageVariantTest extends FunctionalTestBase enabled it in FunctionalTestBase so we don't write functional tests that only work aggregation off. But if there is fail I can switch to XbPageVariantTest is needed.

Doing the manual testing now

tedbow → made their first commit to this issue’s fork.

see #3521819-12: Hovered preview for JS components in library not working anymore → for small MR that reverts a bit of 📌 Add a route for PATCHing both a config entity and its auto-saved version together Active that solve the problem on this issue

Since 📌 Compile Tailwind CSS globally for code components Active might take a while to fix I propose we just revert the small change ui/src/features/code-editor/dialogs/AddToComponentsDialog.tsx that was made in 📌 Add a route for PATCHing both a config entity and its auto-saved version together Active with todo to remove this in 📌 Compile Tailwind CSS globally for code components Active . This doesn't solve the problem I documented in 2) of the "Proposed solution" but that was an existing problem

@thoward216 nope I assume this should be able to be fixed here. Just related to the other problem. in manual testing of this MR I do see it fixing the problem of hover preview not being updated

Looks good to be. Needs /ui approval

tedbow → made their first commit to this issue’s fork.

It sounds like 📌 Compile Tailwind CSS globally for code components Active will solve this

Lets postpone on that and see if this is still an issue

crediting myself and hooroomoo

You can test this by following the directions in the summary and then requesting

1. http://exp-d-core.test/xb/api/v0/config/js_component/component_name This should have empty slots(and compiled_js)
2. /xb/api/v0/config/auto-save/js_component/component_name this should have slots filled

If you git checkout e72f0d0b538ef6abefae89f0a8f14409dfd3ec03^ which is the commit before 📌 Add a route for PATCHing both a config entity and its auto-saved version together Active

1. http://exp-d-core.test/xb/api/v0/config/js_component/component_name should have slots filled (and compiled_js)
2. /xb/api/v0/config/auto-save/js_component/component_name this should have slots filled

The difference is the client now never actually sends a request to /xb/api/v0/config/js_component/component_name with slots filled.

re #10

Let's start with a failing test. I'm assuming this is a back-end bug.

I don't think so. I think the root cause is the same as 🐛 Hovered preview for JS components in library not working Active .

basically before 📌 Add a route for PATCHing both a config entity and its auto-saved version together Active when clicked "add to components" it would send up the whole entity including compiled_js and slots and props. After #3519634 the client never actually makes a request to add the slots to the save config and probably the "layers" menu was always using the saved config and never using the auto-save. It just wasn't noticeable because you can't change slots after you add the component to the library.

This makes the problem in 🐛 JS component slots don't appear in the preview canvas until published Active worse because since it no longer showed the saved config compiled js for hovering and since auto-save doesn't work either there is no preview on hover

I chatted with @hooroomoo about this. Updating the summary with more details

re the 1st step in the summary

Create a new code component and make a JS change

I don't think it makes difference if you make a JS change or not the hover will not work

Looking into this

Needs work for https://git.drupalcode.org/project/experience_builder/-/merge_requests/9...

Since 🐛 Regression after #3500386: import map scope mismatch when auto-saved code component's JS sends a 307 Active just landed another merge request I test this again because it does relate to auto-save. Both situations I described in #20 still work

Tested locally and the 1 e2e test that failed passed

Investigating.

@mayur-sose, small point but the video shows the steps in slightly different order than the summary

From the summary

1. Ensure xb_dev_js_blocks as this provide block overrides which you can't make from the UI yet
2. Open XB and open code editor for one of the overrides (e.g., branding)
3. Make changes to the override
4. Drag the dynamic component being overridden to the page
5. Observe that the modified overrides are not applied
6. Publish the project, and then note that the override changes finally appear

In the video, step 4, dragging the component on, was done before step 2 and 3, editing the override.

In any case both orders should work.

I tested both orders and they worked for me. I have attached screen recordings.

It very well could be that something in the last 18 days since @mayur-sose posted his screen recording that some other commit fixed this.

@mayur-sose can you test this again with both editing the override and then dragging it on, as the summary says and dragging it on and then editing it.

Thanks

Marking back as RTBC since I just changed to variable names for clarity. When tests are green I will merge

To be practical, I am going to implement my last MR comment then mark it back it as RTBC, since it is only naming comment changes.