🇬🇧United Kingdom @thoward216

Account created on 29 May 2018, over 7 years ago
  • Senior Software Engineer at Acquia 
#

Merge Requests

More

Recent comments

🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216
🇬🇧United Kingdom thoward216

Discussed this with @wimleers and @effulgentsia this looks to be an acceptable solution. MR has been created, will look at if there is a good way to test this.

🇬🇧United Kingdom thoward216

Would it be acceptable to catch the EnforcedResponseException in the renderComponent function in src/Element/RenderSafeComponentContainer.php before the current catch? That appears to resolve the issue from some testing, but I maybe missing something.

🇬🇧United Kingdom thoward216
🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216

thoward216 created an issue.

🇬🇧United Kingdom thoward216

#8 has now also need addressed in the current MR.

🇬🇧United Kingdom thoward216

Moving to "needs review" as I've addressed what has been described in the ticket summary.

Comment #8 hasn't been fully addressed in this so far as it looked to be fairly straight forward but quite disruptive to the tests and add more to the current MR and wonder if its better to do this in a follow up MR or ticket?

🇬🇧United Kingdom thoward216

Re #7 - The linked ticket looks to create a view page and no blocks AFAICT so should be okay.

Re #8 - It doesn't look like to be a simple case of updating those lines and has a knock on affect in lots of tests, and feels like its muddying the water a bit with this MR. When pairing with @jessebaker he suggested changing "@todo" to "Uncategorized" at least for now to be more user friendly. Happy to create a separate MR for this after this one has been merged to tackle this?

🇬🇧United Kingdom thoward216

I was thinking this when looking at 📌 Disable components by default sourced from block plugins provided by core in the Standard profile Active

I think the only problem with just changing the labels is currently you can "disable" a component manually and that will then appear in the "disabled components" list with the reason of "Manually disabled", though it does also stay in the "enabled components" list. If we just change the labels it means that a "valid" component that has been manually disable would end up in the "Invalid components" list...

Maybe the solution for that is to not store a reason when a component is manually disabled anymore?

🇬🇧United Kingdom thoward216

@laurii you get the following:

🇬🇧United Kingdom thoward216

I've made a start on this, by default it disables all block components apart from the ones listed. A user can navigate to the components list builder to enable one if they require it.

I think a number of the tests will fail as they might be expecting more components than is now available - so these will need updating, would that be enough test coverage or do we need something more specific?

🇬🇧United Kingdom thoward216

Have we tested what happens if user deletes a component and then reverts to a revision with that component?

I've just carried out the following test:

  1. Create a canvas page
  2. Create and add a code component to the page
  3. Save the page
  4. Edit the page, remove the code component and save the page
  5. Delete the code component
  6. Revert back to the previous revision
  7. View the page in the canvas editor - the code component that has been deleted shows as 'component' in the layers menu and on the preview
  8. Viewing the front end output of the page it renders without that component and no errors.


🇬🇧United Kingdom thoward216

Proposed solution updated with further information.

🇬🇧United Kingdom thoward216

thoward216 created an issue.

🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216

All feedback should now have been addressed.

🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216

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.

🇬🇧United Kingdom thoward216

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?

🇬🇧United Kingdom thoward216

Assigning back to myself to make requested changes.

🇬🇧United Kingdom thoward216

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?

🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216

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.

🇬🇧United Kingdom thoward216

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.

🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216

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.

🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216

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?

🇬🇧United Kingdom thoward216

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.

🇬🇧United Kingdom thoward216

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.

🇬🇧United Kingdom thoward216

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.

🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216

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?

🇬🇧United Kingdom thoward216

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.

🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216

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.

🇬🇧United Kingdom thoward216

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

🇬🇧United Kingdom thoward216

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.

Production build 0.71.5 2024