🇺🇸United States @tedbow

benjifisher → credited tedbow → .

quietone → credited tedbow → .

I think to find the number of pages or content entities in general that use a specific component we might need 📌 Ensure querying JSON nested values when parent keys are unknown is possible in all supported databases Active or more likely 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active

I will research this further

I am wondering if it would make sense to start an MR hear that would add a ComponentUsageCalculator service. Because we are going to need some way to find how many usage particular component has. I guess this will be across content entities using a Json field query, but also we would need to loop take into account instances inside `PageTemplate` .

For PageTemplate entities is it just important show a list of which regions would be affected? Would this only for the active theme? I can imagine in some sites they make start with 1 theme, places some code components in regions but then decide to switch to another theme and not bother to uninstall the other theme. But in other sites they may have custom code or a module like https://www.drupal.org/project/theme_switcher → switches themes.

Clicking view swaps the modal to a deeper view of which
pages are affected

What about if

1. You have 2 existing code components, A & B
2. the code component B is used inside code component A
3. Code Component B has been edited but Code component A has not

Would the "deeper view " show that Code Component A has been affected? Would it should all pages where code component A is used but not B directly? Because these pages would have changes resulting from B changes
Would the number 46 in the example image take into account all Component B placements?

This number shows the total number of instances that will be affected by the basic card component update.

So is this if you are making a new component or editing an existing it shows how many times the JS component is place

So for example:

1. New or existing Component, but no instances in Unpublished Changes or already published = 0
2. Existing Component
   Unpublished changes: 2 instances added in Page A, 1 instance added in Page B
   Already Published: 3 instances in Page C,
   = 6
3. Existing Component:
   Unpublished changes, 1 instance added in Page A, 1 instance removed in Page C
   Already published 3 instances in Page C,

= 3? because there will be instances once "Publish all" is submitted?

I made issue to specifically deal with 📌 Code Components should render with their auto-saved state(if any) when rendered in the XB UI Active

Just a start but I will unassign myself as others will be starting there day tomorrow before me.

I bumped it to major only because I think we should decide sooner rather than later if this going to need changes to the system outside of Code Components. I made possible way we could do this that might just apply to the Code Components code

tedbow → created an issue.

Will work on this

I was just on call with @balintbrews and @lauri regarding auto-saving Code Component. I just wanted to get my understanding down in regard to the usage of auto-saved components, because this would intersect with out parts of XB.

This is my understanding at least for 🌱 Milestone 0.2.0: Experience Builder-rendered nodes Active

1. If a code component is only in an auto-save state and has never been published, it will not be available in the component list in the library of the XB ui. Meaning you can't place an code component on the page/node if it is only in auto-save
2. if a code component has been published, it will be available in the component list in the library of the XB ui, it can be placed
3. if a code component has been published but the editor is currently making changes that are in auto-save, the version available in the component list in the library of the XB ui that can be placed and the version for instances that are already placed, is the published version and is not affected by the auto-save state. This auto-saved version of Code Component also would not affect any instances on page/nodes/global regions that are already been "Published"(in the XB process of "publish all")
4. for an already published version of a Code Component, if the editor has an auto-saved version, once the auto-saved Code Component is published all versions in both pending XB page/nodes/global regions, and already published XB page/nodes/global regions would use the updated version. There would no option to use the previous version

@Wim Leers re #54 ok. yeah I reread the issue in 📌 [PP-2] Don't store page template model data in auto-save for an entity Postponed . I missed this part

The values are filtered out in \Drupal\experience_builder\Controller\ClientServerConversionTrait::clientModelToServerProps but we are storing more in the autosave/tempstore than we need to.

I thought there would be conflict in "Publish All" phase because each entity could have its own copy of the global regions that could conflict. Now I see it is just extra data we don't actually use. So +1 on moving it to "Polish" section

Finished going through issue queue for 6. Save (draft) content section

I think we are close but we mind unknown issues once ✨ [PP-1] Implement the "Publish All" button Postponed is completed as they will allow use to do the whole process. For this to include entity fields we will need the 2.1. Content editing of meta fields section to be done too

As far as know is 🐛 Some components cannot be used in XB because UI prevents SDC props being named `name` Active isn't required for 0.3.0

FYI 📌 CONTRIBUTING.MD is nearly impossible to follow without insider drupal knowledge Active . I am not following either of these but so not sure if they are dealing with the same sections of the file

I merged this because it was random fail in another e2e test. Since this MR adds 1 new e2e test, not failing, I think it is fine to go in.

Will make an issue listing known random e2e tests

Renaming the title case in body sees this in the queue, to give a better idea of what it does

component-operations.cy.js failed but since we just added a new test I can't see how this is related

retesting

Added issue for the "Save (draft) content" section. I am still looking for more issues to add

Assigning to myself for reminder but if anyone else wants to make follow-up MR please assign yourself. You can assign back to me for review

I assigned @traviscarden to review because we needed openapi.yml changes but now those have been fixed in 🐛 open #/components/schemas/LayoutPreview should require properties Active

So @jessebaker and myself already approved this

ok might have been something local

tedbow → created an issue.

I am not not really sure why this pipeline which just has new expection thrown https://git.drupalcode.org/project/experience_builder/-/jobs/4018805

Does report the error response

where as this one https://git.drupalcode.org/project/experience_builder/-/jobs/4018805
which was from the other issue and the reason I made this issue does not

Anyways bumping down to minor because this just might be me not understanding our e2e jobs will log errors for requests(not tests) that fail

wim leers → credited tedbow → .

rerunning e2e test. Hopefully it was a random fail

tedbow → created an issue.

tedbow → created an issue.

It was not failing locally because the requirements for validating openapi.yml are not installed, composer require league/openapi-psr7-validator webflo/drupal-finder devizzent/cebe-php-openapi --dev
if I install them locally it fails. These are installed on gitlab ci

I fixed this and am opening a follow-up issue

@callumharrod I pushed an idea I think could work but I don't have much experience with Cypress. will comment on the MR

I chatted to @callumharrod about this I am going to push up some debug cod to see what is failing on the server side

phenaproxima → credited tedbow → .

phenaproxima → credited tedbow → .

I am 86.75% sure this will now pass tests

Figure out the problem, working on it

@hooroomoo thanks looks good to me!

I think this is good now, I started this and @larowlan did a lot of the work too, so I can't approve

Thanks

The tests failed and I fixed the formatting.

1 e2e test failed after, maybe it was random, retesting

tedbow → created an issue.

I updated the issue summary to change the scope of the test to be more inline with my new comments on the MR

#7

I don't think is particularly valuable to have an end to end test that ACTUALLY drags and drops elements (e.g. mousedown, 
mousemove, mouseover, mouseup etc) . It is a lot of work, is prone to failure etc

@jessebaker and/or @hooroomoo are the same actions possible through keyboard actions. If so would this be easier to test?

I added a suggestion to use the description field to give a clue to manual testers as to why 2 of the components don't show in the UI. I think this will save manual testers time

PHPUnit tests are passing now, the e2e contextual-panel.cy.js test failed, Hoping was just a fluke so retesting

I know this issue was about testing for dragging components into slots but since we skip components-slots.cy.js completely doesn't that mean we also don't have e2e coverage for dragging a component with slots on the canvas? Could we do just the part of the test where we add the "Two column" component on to the canvas and then fix the rest later?

I think we would not have had the regression in 🐛 Unable to add SDCs with slots to the page Active if we at least had that.

If seems like from the issue summary that problem is testing placing items in the slot not placing an item with slots the canvas, correct?

I think we should add test coverage soon for what we can

We still might not want to do this right now but I think we would not have hit 🐛 Unable to add SDCs with slots to the page Active if we had the test.

It seems like "slots" is a very basic functionality for XB and we don't know if any commit will break that should be consider "major" priority to fix it.
could we at least add back the part of the test that drags the "two column" element on?

Tested it manually and it does fix the problem but we should try to get 📌 Write end-to-end test for dragging components into slots Postponed fixed

Does this point to the fact that we don't have e2e test coverage for adding components that have slots?

I guess we had it in ui/tests/e2e/components-slots.cy.js but decided to skip the test in 🐛 Allow dragging components to top/bottom of page and in between adjacent components with slots Fixed . We going to added it back in 📌 Write end-to-end test for dragging components into slots Postponed . I changed the issue to "major" because I think with any commit we could break "slots" functionality and won't know it

I have idea why the test is failing. Will work on it

I have fixed the bugs I found in manual tests. Now in manual testing change update the page data fields(at least have tried title and body) the "Publish" button works, and when I view the node outside of experience builder I see both the XB and other field changes.🎉

Assigning back to @wim leers since @larowlan assigned to him but I will continue to review.

working this

tedbow → created an issue.

tedbow → created an issue.

Created related 📌 Missing e2e for Publish button hides bugs Active

tedbow → created an issue.

This looks good but when I try to manually test it I get errors. Investigating

re "Save (draft) content:"

we already have

and a Publish button to trigger a real entity save:

but I guessing what we want for 0.3.0 is the "Publish all" and we will remove the individual entity save button.

@effulgentsia if that is correct I will add the issues that get use to "Publish all" to the summary

re #10, Yeah I agree the benefit of having something like `violationWithPropertyPathReplacePrefix` on ConstraintViolationException seem more having our own exception class

Looks good to me. Assign to @wimleers because he or @f.mazeikis needs to approve this MR.

Didn't even use ValidationFailedException, we already have a specific ConstraintViolationException in this codebase!

don't have to do this in this issue but do we actually need `ConstraintViolationException`? couldn't we just useSymfony\Component\Validator\Exception\ValidationFailedException?

Looks good, I can't merge it until @wim leers or @f.mazeikis approves the merge request

Just merged 📌 Move SDC specific logic out of ClientsideConversionTrait into component source plugins Active so will need to resolve conflicts

Thanks @longwave!

I also feel like the source plugins are now doing way too much, SingleDirectoryComponent has 11 dependencies and 41 use statements and we're not really close to finishing it.

I agree that is a lot. But also seems like this issue doesn't make it much works. Adds 1 dependency, entity type manager, but maybe the would removed in 📌 Click image with ML enabled should open that image in props form Active which is linked from \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\SingleDirectoryComponent::findTargetForProps

also seems like findTargetForProps could be replace by generic getTargetForSrc(string $src, string $type) somewhere but maybe it will go away completely.

I had 1 question on the MR but think it is find the way it is

reviewing

I didn't know about Symfony\Component\Validator\Exception\ValidationFailedException. seems like a good idea

@amateescu re #8

The design of workspace reverts in general is to allow users to revert workspace publications going back as far as possible, but only one at a time,

would it desirable for XB to actually hide this limitation from the user? could we allow the user to pick the workspace publish point they want to go back to and behind the scenes revert all the ones since then until we got to the point they wanted? Or would we not know this possible until we tried to revert each workspace published.

thanks @traviscarden

@longwave thanks for figuring this out!

I assigned to @traviscarden because he needs to approve the MR.

@arunsahijpal thanks for testing.

experience_builder_system_info_alter() was removed in 📌 Introduce the `xb_stark` theme that uses the Semi-Coupled theme engine, removes the need for taking over the Twig theme engine for the whole site Active so I am not sure what the problem was but I think we can safely close this.

Test are failing because of 📌 Since twig/twig 3.12: Twig Filter "spaceless" is deprecated Active which has a fix awaiting approval

@wim leers thanks for the feedback and @larowlan thanks for addressing those problems,.

@wim leers see the update summary for the issue scope

Added more ideas to 📌 [PP-1] Add entity access checks to routes that deal with entities Postponed while I was thinking about it. There is todo in the code for this

Another complication with access is right now as 📌 Save metadata(page data) field with the entity revision Active is done field access is checked in the process of `\Drupal\experience_builder\ClientDataToEntityConverter::convert()`. This make sense as this will be use to set entity fields we shouldn't allow setting field the user does not have access to. But we don't run `ClientDataToEntityConverter::convert()` on auto-save. We couldn't really call `convert()` to determine access though because the EntityConstraintViolationListInterface it returns mixes non-access violations and access violations.

So either we could either

1. extract out something ClientDataToEntityConverter::ensureAcess()` that would be called on autosave, but then access would still be checking in ClientDataToEntityConverter::convert()` during "Publish all"
2. Introduce a new AccessAwareEntityConstraintViolationList extends EntityConstraintViolationList with additional addAccessViolation(ConstraintViolation $v), getAccessViolationsList() and getNonAccessViolationsList() and have `ClientDataToEntityConverter::convert()` return this.

   So then on autosave we could ensure getAccessViolationsList()->count() === 0
   and then on Publish all we could ensure getNonAccessViolationsList()->count() === 0

@larowlan thanks for making this issue.

I think we also might need a temporary generic publish XB changes permissions

The account that submits "Publish All" might not have done the changes to all of the entities(or any?) that would be published. Are we going to require that account to have the permissions to make all the changes across all the entities.

I think having a simple publish XB changes should be good enough for 0.2.0 because once we use Workspaces we want use Workspaces related permissions to determine access to publishing changes to the live site.

Related to 📌 Save metadata(page data) field with the entity revision Active If we went with publish XB changes we would probably want to check access to field changes at the time the changes went into the auto-save. The other option I guess would be to check access as the person who the auto-save considers it's "owner" when submitting "Publish all". The problem with that is that multiple accounts may have edited the same entity. Also the account could have different permissions at that time or been deactivated, hopefully for pre-workspaces, 0.2.0 we won't to worry about these edge case.

But again this may change with Workspace integration so I don't think we should spend that development time on it since 0.2.0 is not for production sites. For example we don't know that once we use Workspaces the final "Publish all" step will still be the point at which we convert auto-saves to real entity saves. We might just use auto-saves at that point to handle: 1) invalid entities, 2) speed or DB bloat concerns from many revisions. We could still require real entity revisions(whether user is aware of them or not tbd) before entities could be move to the live Workspace.

Not completely done but probably could use other eyes on to make this is going the right direction.

Assigning to @wim leers but @larowlan if you happen to review this first and have feedback I can address feel free to assign it back to me and set to Needs work. Obviously also welcome to work on your self.

looking at the openapi related fails in 3494388-force-fail and the fact that 3494388-debug-request-validation doesn't have any openapi fails I think the problem is just requests

So this MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/485 proves at least validation is being triggered for responses.

Opened up https://git.drupalcode.org/project/experience_builder/-/merge_requests/486 to see if the problem is just that requests are not being validated.

📌 Change ApiContentUpdateForDemoController to save from auto-save instead of request data Active should have triggered a request validation error. I can confirm locally it does

re #5 I am pretty sure it is duplicate but @larowlan could correct me. But just to have 1 place to discuss this now let reopen this one. I was really getting confused between the 2 issues

copying my comment from 📌 See why OpenAPI mistake for API error didn't cause test failures Active

@TravisCarden Why not instead of having isProd at all we just leave the other checks in \Drupal\experience_builder\EventSubscriber\ApiMessageValidatorBase::shouldValidate but remove the isProd() check. Then in we could

1. catch the OpenApi validation errors and save the error info in Drupal state under xb_openapi_errors
2. log the errors in \Drupal\experience_builder\EventSubscriber\ApiMessageValidatorBase::logFailure
3. In \Drupal\Tests\BrowserTestBase::tearDown in our base test class(es?) and check the state there and throw an exception. We could have 1 trait for this across all test types

On advantage of this is that you won't fail on the first Open Validation error but could report on all the validation errors in the test run. Another advantage is it won't stop the test so you would also see another failure that would fail the test even if Open Validation failed.

This would also allow say if you were adding a new route locally but were sure on the return structure yet to not do the changes to openapi.yml or not complete the changes until you were sure on the structure, even if you had league/openapi-psr7-validator installed. It wouldn't stop you from working on the test for the new route. Openapi failures would still cause the test to fail but only at the very end so you would know if the rest of the test would pass.

I guess the con would be errors would not stop requests even if you had league/openapi-psr7-validator installed locally during manual testing, only automated tests. But we would also display the errors on the page.

We do something like this in \Drupal\Tests\package_manager\Kernel\PackageManagerKernelTestBase::tearDown but in that case we check the logs for specific errors.

I closed 📌 See why OpenAPI mistake for API error didn't cause test failures Active and pushed the MR there to here

closing and pushed this branch to the other issue