🇬🇧United Kingdom @thoward216

Proposed solution updated with further information.

thoward216 → created an issue.

Carrying out some manual testing, I did do some back in #19 📌 Only allow deleting Code Components if there's 0 usages Active - to confirm; previously it was possible to 'delete' a code component that was in-use if not used on the current component tree as per comment #12 📌 Only allow deleting Code Components if there's 0 usages Active .

The updates in this MR to the access check now stops a user from doing this. This MR without any FE work displays a dialog like so:

With an error in the console: "This code component is in use and cannot be deleted."

All feedback should now have been addressed.

Moving back to needs work and assigning to myself. Had a review call with @wimleers & @f.mazeikis and have a some actionable feedback.

Marking as needs review for an initial review and a few questions on the MR.

I have implemented the following endpoints as per #16:

1. /canvas/api/v0/usage/{canvas_config_entity_type_id}/{canvas_config_entity} API, which provides a simple boolean true/false response
2. /canvas/api/v0/usage/{canvas_config_entity_type_id}/{canvas_config_entity}/details API, which provides detailed of information that \Drupal\canvas\Audit\ComponentAudit can
3. /canvas/api/v0/usage/{canvas_config_entity_type_id}?page=N which allows iterating over all is paginated-per-50 and lists an object like {id1: true, id2: false, …, id50: true}

I've updated the access check in VisibleWhenDisabledCanvasConfigEntityAccessControlHandler rather than in CanvasConfigEntityAccessControlHandler which may not be correct, but we'd have to inject component audit into the other handlers that extend this which I'm not sure makes sense as its quite specific at the moment.

The above update in the access check resolves the scenario where you have a Code component added to your library that is in use on a canvas page for example and attempt to delete it, previously this would delete the code component.

I've tested #7 and this scenario looks to have been accounted for before this MR.

Thanks @wimleers, its good to question and after reviewing #16, this seems clearer to me. I've started implementing as per #16.
My only thought is that we currently only have audit data for components - but is there a view to extend that in the future and the reason that we should keep these endpoints more general?

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

Assigning back to myself to make requested changes.

I've created an MR for adding weight to Segments, I haven't addressed anything in the Drupal UI for weighting as I saw this issue: 📌 Revisit segments UI: do we need a Drupal UI? Active and I assume it may get addressed there?

I've also not addressed discussion in #2/#3 as seems thats not currently confirmed. But we might need 📌 Provide a locked Default segment Active to land first? Or it get handled in that ticket after this lands?

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

thoward216 → changed the visibility of the branch 3541501-update-phpcs-config to hidden.

thoward216 → created an issue.

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

Moving to needs review to get some eyes on the one bit of feedback that I've attempted to implement and comment added on the MR, may need further work. All other feedback should be addressed.

I believe this would be happening because when you first create a new code component it will be using the name input by the user to generate the machine name of the component. Upon renaming the component, it already has a machine name and that is something that wouldn't change once created, so it's just updating the label at that point.

I've updated some tests that were failing, updated the access check to account for the new route and added to the test.

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

After rebasing and uncommenting the schema changes as mentioned in #13, there are a number of failing tests where the validation messages are not as expected.

An example: 'versioned_properties' => 'This value should satisfy at least one of the following constraints: [1] This value should be of the correct primitive type. [2] This value should be of the correct primitive type.'

To me this doesn't look right and the message is also duplicated. I spent some time investigating a bit further with @f.mazeikis we're not sure that the constraint AtLeastOneOf can be used with sequences? Looking at the core test AtLeastOneOfConstraintValidatorTest it only ever tests with strings or integers.

thoward216 → changed the visibility of the branch 3538306-update-phpcs-config to hidden.

thoward216 → created an issue.

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

Coming back around to this, as per #10.

A new components config with one version looks like this:

uuid: d973b1d2-df01-4f89-a309-fe83f34c9d9a
langcode: en
status: true
dependencies:
  config:
    - experience_builder.js_component.test
active_version: 8fe3be948e0194e1
versioned_properties:
  active:
    settings:
      prop_field_definitions: {  }
    fallback_metadata:
      slot_definitions: {  }
    label: test
    id: js.test
    provider: null
    source: js
    source_local_id: test
    category: '@todo'

New versions created adds them under `versioned_properties` as expected but as you can see above the hash `8fe3be948e0194e1` is not in `versioned _properties`. So either the above isn't structured as expected (though I'm sure there are a number of tests around this) OR the original constraint that was commented to add later maybe stale?

Started having a brief look into this but I can't seem to reproduce. If I create two patterns with the same name the second one then gets an id as per the code in #2.

Thanks @larowlan - I've manually tested this and everything is working as expected and the output of the component tree keys is what I was expecting to see also. I've just opened an MR to run the pipeline.

I've started some investigation into this and it doesn't look to be the same issue as #3534971 and looks to be due to #3526127 and the way that the function generateComponentTreeKeys() builds the keys. The array that is passed into the generateComponentTreeKeys() function contains all 10 items in the tree in my case, but when I go through the function I only end up with 8 items.

I did a test by commenting out the set() function in Pattern entity and everything saves as expected so that looks to prove where the root cause looks to be. Will continue to debug this further.

Moving back to needs work as rebased with latest 0.x and resolved conflicts but there are now some failing tests.

Updating issue summary.

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

I think this is failing as 'versioned_properties' does not contain the hash of the 'active_version' as its simply keyed as 'active'. So I'm not sure this is the correct constraint to be added here?

Had a look into this issue and got the library attached using similar code as shown in #2 but the preview still did not appear to update as expected. Requires further investigation.

Have updated the the MR and things are looking better. Now getting errors around the validation for the version constraint that was added/uncommented.
https://git.drupalcode.org/project/experience_builder/-/jobs/5827036#L4725

XB now requires 11.2 so I believe this can be actioned.

There we also some changes in Translation test where the 'article' tag selector needed to be removed. So these may also need to be reverted/reviewed.

See: https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...

thoward216 → created an issue.

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

Looks like theres now a number of config schema issues now occurring: https://git.drupalcode.org/project/experience_builder/-/jobs/5657671#L2402

These looked to be "ignored" before here

Not sure if these should be resolved in this MR.

Tests are now all passing on this.

Oh yes! That was a silly mistake, have fixed that now!

I've fixed those two PHPUnit test failures, commented out the failing e2e test for now and also added a bit to the components.md - let me know if this needs expanding.

thoward216 → created an issue.

I am able to reproduce this issue with SDCs, although I am also seeing an issue whereby if I toggle a boolean field from true to false, save the changes and then refresh the page the toggle has gone back to the true state. Looking at the data saved in the database it has saved correctly there so this looks to be a UI bug.

This can easily be seen with something like the Shoe Badge SDC and toggling the "pulse" boolean and saving the pulse effect in the preview does not happen but the toggle is showing as true.

I wonder if this is some of the root cause to the default values not working as expected here? Or could be something separate, if so I can create a new ticket for this.

As mentioned in #2 🐛 Setting default values does work for boolean props Active it does look like the default value used is the first value in the examples.

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

Understood, thanks for context here @berdir. It will likely need further investigation by the Site Studio team.

thoward216 → created an issue.

All the tests look good. Assigning to myself to look at adding a test for this next.

After merging the first one there was a conflict in the remaining MR, I have rebased that one again.

Both MRs have been rebased.

Moving back to needs work as theres a PHP unit test failing.

I've found the code that looks to have been causing this, it was recently changed. I've created an MR to run all the tests and will need further testing, its likely it'll need more logic rather than just removing the enable function.

Both MRs are now passing tests and ready for review.

I'm able to reproduce the same behaviour as @jatingupta40 - disabling an SDC component shows that component in both lists. Interestingly under "disabled components" the status for the component shows as "Incompatible" but the reason shows as "Manually disabled" I think that the status here maybe at least part of the problem and it should be marked as "Disabled" and not "Incompatible".