Only allow deleting Code Components if there's 0 usages

wim leers → credited catch → .

Closed 🐛 Trying to remove code component from page immediately after attempting to edit it gives 404 error Active as a duplicate of this.

Scenario that needs to be addressed with this fix :

• Create code component named {code01}
• Create code component named {code02}
• Import code01 to code02
• Delete code01 able to delete code01
• Code02 gives error as below

Could we make the soft delete as an explicit feature? I.e. hiding modules from the UI. I think this would be really useful in the beginning until we can associate which users can use which components. Tagging with needs follow-up for that.

We could still offer the delete as an option because there are scenarios where it's desired to delete a component and all of its uses altogether. However, there's a difference between content vs components referring to components. I'd say that uses of a component in another component should prevent deleting, but deleting a component used in content should not prevent the deletion (especially because even past revisions would prevent deletion).

What would be extremely useful is having a list of usages of the component. This is not only useful for the delete use case but also when you want to assess how components are being adopted.

I tested #7.

To clarify, this is not an issue when the components are stored (e.g. I publish them, even without "adding to components" aka status=1), only on the transient time when they are in auto-save, not published, not status=1, as we do this validation on config entity dependencies. That part IMHO is Works as designed, as the preview message is informative enough to fix the situation.

The original purpose of this issue still remains though.

It seems this is kind of a duplicate of ✨ Usage info for code components with unpublished changes Postponed (because it's hard to find).

@lauriii in #8:

What would be extremely useful is having a list of usages of the component.

We already have that infrastructure and information, since 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active . We just don't have it in the modal dialog, which is what I suspect you're hinting at?

If so, that makes this even more closely related to ✨ Usage info for code components with unpublished changes Postponed !

Could we make the soft delete as an explicit feature?

Yes, but what does "explicit feature" mean? 👈 confused me even more, perhaps you meant "components"?

Moving to critical due to this causing data integrity issues.

Because of the data integrity issues, I wonder if we could prioritize the simplest possible change before we settle on the final solution, including soft-deletion etc.

I think it would be the following:

When users try to delete or make a component internal (i.e. remove from components), we currently do that check to see if the component is being used in the current page's component tree. We can now do that properly thanks to 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active . I don't see an HTTP API for the audit info, so unless I'm mistaken, that would need to happen before we update the check on the UI.

Do we already account for first-party code component imports (JavaScriptComponent dependencies) in the audit logic?

I think component imports should be handled via \Drupal\experience_builder\Audit\ComponentAudit::getConfigEntityDependenciesUsingComponent in the audit logic.

I'm sorry, but I can't help but question the current MR direction (which I've not been involved in). 🙈

Questions

I don't quite understand 2 fundamentals:

1. Why we need a dry-run operation. Because that would then imply we would want to do this for many (config) entities Canvas provides! Do we really want to build such infrastructure multiple times over?
2. How would such a dry-run operation allow the "delete" operation to be available or not in the UI? Is the UI going to do a "Delete (checking…)" disabled contextual menu first, fire a request to the server, wait for the response, and then either change it to "Delete" or "Delete (not possible, see usage)"? 🤔

Potential alternative

Why not:

1. Update access check logic at \Drupal\canvas\EntityHandlers\CanvasConfigEntityAccessControlHandler::checkAccess() to prevent deletion if there's >0 usages returned by \Drupal\canvas\Audit\ComponentAudit::hasUsages()
2. 👈 presumably too slow?
3. Provide 3 internal HTTP APIs:
   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 not a simple boolean true/false response, but the full breadth and depth of information that \Drupal\canvas\Audit\ComponentAudit can
   3. a sibling /canvas/api/v0/usage/{canvas_config_entity_type_id}/{canvas_config_entity_ids} which accepts a comma-separated list of config entity IDs (say, up to 50), to bulk-check?
   4. OR a sibling /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} aka booleans-per-known config entity of this type, to bulk-check?

   Either of the 2 bulk-check ones could then be used by the UI to solve problem 2 above, and the detailed one could be used to (in the future) provide detailed information to the person trying to delete it.

   This could then be generalized to work for all config entity types automatically: JavaScriptComponent, ContentTemplate, etc.

   This proposed alternative is AFAICT what @balintbrews proposed in #12.

   The MR does not tackle the issue summary, which is about soft-deleting, so we need the issue summary to be overhauled to reflect the updated direction — also to ensure we don't forget about the original scope.

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?

Not every stable blocker needs to block an RC, but this one does.

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.

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