BeaconCMS / beacon

Open-source content management system (CMS) built with Phoenix LiveView. Faster render times to boost SEO performance, even for the most content-heavy pages.
https://beaconcms.org
MIT License
1.04k stars 101 forks source link

Feature: add customizable asset delete callbacks #601

Open ddink opened 1 month ago

ddink commented 1 month ago

What will it allow you to do that you can't do today?

It will allow users to customize a soft_delete/1 callback that's used when Beacon.MediaLibrary.soft_delete/1 gets called.

Why do you need this feature and how will it benefit other users?

This feature is necessary to be able to programmatically delete stored assets where they are stored when utilizing custom provider modules for the media library's asset upload functionality.

Are there any drawbacks to this feature?

This feature does not standardize the customizable callback. As many or as few callbacks can be added. In other words, this callback is not a required implementation.

leandrocp commented 1 month ago

Hey @ddink thanks for the PR! I believe we can actually move that logic into the provider itself, similar to send_to_cdn/1 that providers already implement. So we would not introduce a new global lifecycle, but rather a new Provider callback to soft delete an asset.

So MediaLibrary.soft_delete/1 can call Lifecycle.Asset.delete_uploaded_asset(asset) which fetches the list of providers from the media type config, and call Beacon.MediaLibrary.Provider.soft_delete/1 (to be implemented, similar to Provider.send_to_cdns/1)

And also move that repo(asset).update_all from MediaLibrary.soft_delete/1 into the Repo.soft_delete/1 provider function.

There are still some improvements and fixes we need to do on the Media Library API, but this PR is moving into that direction, giving more capabilities to the providers, which is great!

ddink commented 1 month ago

Hey @leandrocp! Of course, happy to be able to help 🫡 

I think I have now moved around things the way that you want. The only thing that sticks out to me is the way we’re updating the asset before the soft delete.

In order to have the media library updated when the asset is soft deleted, I also had to add the repo(asset).update_all block into my custom provider’s soft_delete callback. It’s not a big deal, but I’m wondering if there’s a way to do that without having to include it into the custom provider’s logic. What do you think about putting the repo(asset).update_all block in Beacon.MediaLibrary.Provider.soft_delete/2 before the list of providers call their own soft_delete/1 callbacks?

leandrocp commented 1 month ago

I also had to add the repo(asset).update_all block into my custom provider’s soft_delete callback

Yeah that's not ideal. The current design assume the Repo is optional but part of it is still necessary, although I'm not sure yet how to improve it.

What do you think about putting the repo(asset).update_all block in Beacon.MediaLibrary.Provider.soft_delete/2 before the list of providers call their own soft_delete/1 callbacks?

I think that will work for now.