🇺🇸United States @tedbow

@utkarsh_33 take another look. I change to new MR since you last reviewed.
Basically switched the approach to using XB's own validators rather than having to add specific validation in xb_ai class
for SetAIGeneratedComponentStructure this mean a conversion first to a `ComponentTreeItemList` and then just calling validate() on that. Basically the same for CreateComponent but there isn't conversion needed beyond what was already in the class

This has the advantage of running the actual validation that XB will run later on publish so less surprises later. Also we don't have to maintain duplication validation logic

I am currently looking at refactoring it to use XB own validators. I hoping this will work so we don't have duplicate the logic as there is my current MR

Not sure why EntityFormControllerTest is failing. Since the MR only affects xb_ai I don't think it could be related

re #13 change the errors to show the actual paths for the yaml structure. you can see the changes made to the test expectations in this commit https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

@utkarsh_33 take another look and let me know what you think

@utkarsh_33 thanks for the detailed response and testing!
re

are the errors descriptive enough to provide the AI with sufficient details about the reason for failure?

I do have idea to provide better information.

Leaving to assigned to myself to update the summary with the basics of the actual fix.

@penyaskito thanks for the clarification. I will update the issue summary and then merge in a bit

Needs review for my general approach

I think there is just 1 change Wim asked for that has not been addressed. Otherwise looks good to me.

The summary does not reflect the actual solution though

I pushed up a MR that takes a slightly different approach.

The title of the issue says "validate changes before publishing" which I think would imply that the validation would happen after components are added to the page

In my MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/1381 it validates the structure of the component before they are added to the page. It will make some tests fail in `SetAIGeneratedComponentStructureTest` because they do not use valid components

I am just getting to know this module so it is hard to hard to test manually, the AI seems to just make valid components. But I locally I did hardcode some exceptions is like "The cta2 for component %s contains a URL in for cta2 that uses "Click". Please use "Pop"'for the header link I can see the AI being creating a first version using "Click" and then remaking the component using "Pop" , So it does appear that `SetAIGeneratedComponentStructure` is a place we can throw exceptions and it triggers the AI to take the feedback an fix the components, neat!

@utkarsh_33 thanks, I will take a look

#19 looks like @lauriii provided the info on the MR

Thanks @xjm and @smustgrave

re

Fond memories (and also stressful ones!).

Indeed, I will sign-off with this fond memory from #2784443-171: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it → 😜

This error is handled on the back-end but the UI doesn't give any indication, so #2 the transaction actually is rolled back.

I think to 2 child issues that would get us closer to concurrent editing are
📌 [PP-1] For less conflict errors only validate region auto-save on layout PATCH Postponed

📌 Replace the postPreview action with atomic equivalents Active

I am not sure how much work 3492065 would be but 3532056 is small change and just now needs test coverage.

3532056 will not get use all the way to concurrent editing but at least it would mean that if you were editing a component in 1 region you would not error if there were any changes to any other region. I think practically this would cause there to be many fewer conflicts

I am currently working on other issues so I am unassigning myself. I still think this would a good thing to get done

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

I chatted with @wimleers about this before making the issue we agreed that probably

The user should get a message that the entity was saved outside of XB and everything except the XB canvas has been reset to the saved version

Is the best solution for the time being

tedbow → created an issue.

I don't think it has anything to do with error about the require field. Instead I think is actually because when Page is already checked in "review x changes" then the client doesn't request the update list after there are new changes. It just happens it was discovered because when you get the error about the require property the UI leaves the page checked in "review X changes"

It should be reproducible via

1. Open XB with no pending changes
2. create a new page
3. make a change to the title
4. wait till "review 1 change" to show
5. click "review 1 change"
6. select the new page in the "unpublished changes" list but do not publish
7. close the "unpublished changes"
8. make another change to the title
9. wait a few seconds, to ensure this not a problem with a race condition
10. click "review 1 change",
11. observe that page is still selected
12. wait a few seconds, to ensure this not a problem with a race condition
13. click publish
14. you should get the error

It appears that if you have something selected in the "unpublished changes" list then even when you close the list and make a new change the client does not request the pending changes from /xb/api/v0/auto-saves/pending

If you do the same steps but do not select the page for publishing until after you have viewed and closed the "unpublished changes" list 1x time and only select it right before publishing you will not get the problem

There may be other race conditions still but I think the problem described is actually a race condition it doesn't matter how long you wait between the steps you will still run into the problem.

I think this needs a front-end fix

Might have been introduced in 📌 Create UI for selective publishing of changes Active because that is where the ability to select individual items to publish was added

Assigining to @mred9 because I think he does most of the front-end work there

Updated the summary based on this MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/1351

It doesn't require any front-end changes but it would probably be good idea for the front-end to also stop sending changed but it could be in follow-up

I was thinking of test assertions like we have in XbConfigEntityHttpApiTest

Were we have comments like this

// Add missing crucial data, but leave a required shape violation: 500,
    // courtesy of OpenAPI.

So sending the wrong shape and confirming the error

Thanks all!

Code looks good. I have to update summary because we are not solving the initial problem reported and no using the solution in the summary

but I will then RTBC and merge

xjm → credited tedbow → .

@utkarsh_33 can you push up the local work you have for #3 to branch? It would really helpful for me and/or others to help. Thanks

I think issue summary needs to be updated. I can do that tomorrow but until then this is where I think it stands

I detailed in the problem and why @bnjmnm's MR could still result in random fails in test in comments here https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

I do think now the proper solution is ignore the "changed" value from the client because was always being ignore except to determine if access was going to be checked. That is done in my MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/1351. I have explained the reasoning of why I think it is ok in #15

It would be possible to write a test by mocking the request time, through a new class like \Drupal\update_test\Datetime\TestTime::getRequestTime and have the client send in the same timestamp. This should result in access error in 0.x but not in https://git.drupalcode.org/project/experience_builder/-/merge_requests/1351

I closed my MR. I think @larowlan's or @wimleers is probably the way to go. I think I understand how @wimleers' https://git.drupalcode.org/project/experience_builder/-/merge_requests/1346 works and it does solve the current problem

@larowlan's https://git.drupalcode.org/project/experience_builder/-/merge_requests/1352 seems promising because it gets to root of the problem, we consider required to mean not blank. but right now there are test failing around validation so assume it makes a bunch of other validation fail.

Here is little debug module to get around the problem in 🐛 Revision created timestamp not updated when editing in XB Active so you can see when testing the changed time is actually still updated

@bnjmnm thanks for the investigation.

I originally wrote the logic around "changed" I think I made it overly complex to cover edge cases that probably won't happen

1. It assumed that it might be valid at some point to used the "changed" value sent from the client
2. It assumed that some entity types might have overridden entity forms that would not have the logic in \Drupal\Core\Entity\ContentEntityForm::updateChangedTime
3. It didn't consider the implications of allowing the client to send a "changed" value if 1) and 2) actually happened. Basically for the same entity some edits could be made inside XB where different clients would be the authority on revision timestamps and some edits would be made outside of XB where the server would the authority on revision timestamps

I think basically 2) is very unlikely to happen so1) is not needed. Even if 1 was needed 3) means there would probably other implications to consider

So made an MR to ignore "changed" from the client. https://git.drupalcode.org/issue/experience_builder-3536040/-/tree/35360...

#9 is probably unrelated. opened another issue 🐛 Revision created timestamp not updated when editing in XB Active

I pushed up a debug module xb_revision_debug. You can go to /revision-info/node/1 or http://exp-d-core.test/revision-info/xb_page/1

to see the difference between "changed" and the revision timestamp. Apparently on the node and page history "changed" is not what is displayed

tedbow → created an issue.

One thing I noticed in 0.x is that it does not seem that change time is ever updated

With node or page

• create the entity - note the time
• wait a minute
• make an edit in XB
• wait a minute
• Publish the entity

If you look at the entity revisions list and the time is the same for all revisions

I started a branch locally to better handle the error, for example info about the entity that was being processed but I think basic fix is for ui/src/services/pendingChangesApi.ts to return error message to the UI instead of console and to not show the "Published" message

I don't why gitlab was showing me so many file changes an hour or so ago🤔

@isholgueras assigning back to you I think there is some merge problem with the MR it now has 63 files changed

Thanks!

addressing the feedback

Started a rough MR. Manually testing it also stops publishing an empty string prop. Added basic test coverage

I am looking into solutions in #5

I did some debugging on this.

I think what is happening is that "Required" string property does not error if an empty string is sent back

Tracked this down to \JsonSchema\Constraints\UndefinedConstraint::validateCommonProperties

/ Verify required values
        if ($this->getTypeCheck()->isObject($value)) {
            if (!($value instanceof self) && isset($schema->required) && is_array($schema->required)) {
                // Draft 4 - Required is an array of strings - e.g. "required": ["foo", ...]
                foreach ($schema->required as $required) {
                    if (!$this->getTypeCheck()->propertyExists($value, $required)) {
                        $this->addError(
                            ConstraintError::REQUIRED(),
                            $this->incrementPath($path, $required), [
                                'property' => $required
                            ]
                        );
                    }
                }
            } 

So it just checks if the property exists.
I checked and added minLength: 1 to heading.component.yml for "text" then it does fail validation on publish with an empty string.

Not sure if this is the expected for SDC validation

I tested tests/modules/xb_test_sdc/components/containers/two_column/two_column.component.yml and got rid of the `$ref: json-schema-definitions://experience_builder.module/column-width` for "width" so that it would just be treated as regular integer which would allow not filling out this field. I did get a "The property width is required." error when trying to publish in this case.

I am not sure what the correct solution is here. We could do some shenanigans on the server-side to unset string properties that are empty and required.
Or we could add minLength: 1 to all string properties we want to be non-empty
Or we could add a new json-schema-definitions://experience_builder.module/non-empty-string` schema
Or we could add some sort of higher level validator that would make sure all require strings are not empty

@mayur-rose thanks for the detailed testing report.

I can't reproduce the problem in TC8

"Set as homepage" action is hidden. After page reload option is visible again.

what exactly failed? What I am seeing with 2 pages is, if I set Page 1 as home page, "Set as homepage" action is hidden on Page 1. That is expected. I reload and it is still hidden on Page 1. If I set Page 2 as home page then the actions disappears from Page 2 but appears again from Page 2

phpunit test failures

@wim leers and @jessebaker, thanks for the reviews!

Crediting @wim leers for detailing the solution in 📌 Allow CMS Author to set site's homepage from navigation Postponed

tedbow → created an issue.

I think this issue is good. Re #66 We need follow-ups but since they are front-end and I don't exactly what what the problems are I don't think could make even much of placeholder issues

@larowlan left to comments https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... but chatted with him and these just suggestions for things to do if we have other simple config updates, which we don't currently have planned.

See #65 about the phpstan failure, I don't think we need to fix that here

global-regions-interact.cy.js is failing but passes for me locally but not consistently. I don't see how that could be related

It would be good to get approval of the front-end code on the MR, assigning to @jessebaker, but others could review, assigned in gitlab

I fixed #63 1 and 3

I re-ran the gitlabci job on 0.x and it is not getting the same phpstan file as this MR
see https://git.drupalcode.org/project/experience_builder/-/pipelines/550539

I opened 🐛 phpstan ci job is failing Active

tedbow → created an issue.

tedbow → changed the visibility of the branch 3536247-detect-collisions-autosave to hidden.

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

tedbow → created an issue.

tedbow → created an issue.

@penyaskito amazing work here!🎉

Also does it happen for others with less permissions but who are still able to publish

@mayur-sose This is random error correct? Or does this happen on all nodes all the time?

See MR comments

created 🐛 DxRouteConsistencyTest::ignoreDynamicPathPartNames regex does not preserve the number of path arguments Active . though I am not sure that is correct now. I put a fix in for \Drupal\Tests\experience_builder\Unit\DxRouteConsistencyTest::ignoreDynamicPathPartNames in this MR

tedbow → created an issue.

Looks good if tests pass. Thanks @penyaskito!

tedbow → created an issue.

media library e2e test failed. Guessing not related.

📌 Create UI for selective publishing of changes Active and this issue both passed tests on their own but in combo make \Drupal\Tests\experience_builder\Kernel\ApiAutoSaveControllerTest::testDelete fail.

Will open up follow-up MR

see my MR comment https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... I will implement this

re #8 I created 🐛 New XB pages also create a new revision on publish even though they are intended not to Active . I didn't add a todo in this issue's MR because I don't think we would change what we are doing here

tedbow → created an issue.

I think I found a bug that is 0.x and this branch.

$use_existing_revision_id = AutoSaveManager::contentEntityIsConsideredNew($entity);

        if ($entity instanceof EntityPublishedInterface) {
          $entity->setPublished();
        }
        // If the entity is new, the autosaved data is considered to be part
        // of the first revision. Therefore, do not create a new revision
        // for new entities.
        if ($use_existing_revision_id) {
          $entity->setNewRevision(FALSE);
        }

Even if $use_existing_revision_id === true a new revision is always created.

I think the intention of this code, from reading the comment, is that if you

1. create a new page inside XB
2. make some changes to the page
3. publish the page
4. There should be only 1 revision in the log after publishing

The last item does not work. there are always 2 items. Presumably for every XB page there will always be the same first revision with the same data, excepted for created and changed timestamps, because no changes have been made for this revision.

I am not sure why this happens

This relates to this issue because that first revision will always show as "anonymous"

But that might be a bigger problem that we could fix in a follow-up. If someone one knows why this is happening it and it is a small fix it might make sense to fix it here

I am still getting the anonymous if I don't change the title. Also I do think we want

$use_existing_revision_id = AutoSaveManager::contentEntityIsConsideredNew($original_entity);

Because otherwise if you first create a page that hasn't been published and change the title it will never be considered new which I don't think is the intention

and that would leave the first revision showing as anonymous, which I don't think is the intention.

see MR comment

I pushed up the changes I had started on the other issue.

Instead of adding AutoSaveEntity::getLiveStartingPoint() I added AutoSaveManager::calculateStartingPoint()

I think the starting point needs to calculated at 2 points

1. In AutoSaveManager::saveEntity() if there is no existing auto-save entry
2. In \Drupal\experience_builder\Controller\AutoSaveValidateTrait::getClientAutoSaveData() there is no auto-save entry

@wim leers thanks for creating this follow-up. I started an MR on the other issue to show an similar idea https://git.drupalcode.org/project/experience_builder/-/merge_requests/1270

I will move some of that work here

@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

#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.

I noticed the if I get error in publishing then all the boxes are unchecked.

I tested this by trying publish a code component without publishing the global CSS asset, since #3534989: Error if the user tries to publish Code Component without the Global CSS → this will cause an error

Before I submitted here I had other entities besides the global CSS selected. With just 3 entities it is not a big deal to reselect but if I had many it would be pain to have to reselect, especially if I was only selecting pages after viewing them another browser tab to make sure they were ready publish. If looked at 20 pages and checked the 10 I thought were ready to go I would have to do the process over again if I got this error.

Now that we have 📌 Add field access check on `ApiAutoSaveController::post()` Active and 📌 Add field and entity access check on `ApiAutoSaveController::post()` Active there are more ways to get errors on publish

This could be handled in a follow-up

re #55 @neha_bawankar wow thanks for the detailed testing report!!!

If this comes back green I will merge