Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to overwrite the media #17113

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Allow to overwrite the media #17113

wants to merge 3 commits into from

Conversation

hishamco
Copy link
Member

@hishamco hishamco commented Dec 4, 2024

Fixes #6183

@hishamco hishamco requested a review from Piedone December 4, 2024 01:24
@hishamco
Copy link
Member Author

hishamco commented Dec 4, 2024

We configure it from within UI, but IMHO configuring it from with appsettings.json is more convenient

@Piedone
Copy link
Member

Piedone commented Dec 4, 2024

I don't think this is the right approach. Instead, we should do what every file manager does, and ask you to overwrite if the file already exists. This doesn't need configuration.

The cache invalidation problem mentioned in the issue is fixed now BTW.

@hishamco
Copy link
Member Author

hishamco commented Dec 4, 2024

I don't think this is the right approach. Instead, we should do what every file manager does, and ask you to overwrite if the file already exists. This doesn't need configuration.

This will be tedious if we upload multiple files at once, also think about chunking files.

We could configure this through UI with a warning message as we usually did

@hishamco
Copy link
Member Author

hishamco commented Dec 4, 2024

I might need to check a few CMSs as well to see what they did

@sebastienros
Copy link
Member

and ask you to overwrite if the file already exists

If the FS implementation doesn't even ask and overwrites, I would even use it as the default for Blob, to fix the issue.

@Piedone
Copy link
Member

Piedone commented Dec 4, 2024

This will be tedious if we upload multiple files at once, also think about chunking files.
File managers commonly use the "yes" - "yes to all" approach for this.

I don't think this is a big enough UX issue to worry about (compared to others), also because you can bulk delete files. But if we tackle it, we should do it properly, and for that a good indication is what the majority of file managers do. That again is confirming about overwrites, or sometimes indexing the new files and then keeping both versions, but I've never seen one silently and without asking the user overwrite files (configuring this behavior in appsettings would only be an appropriate user configuration for this if the app has a single user, and they're also configuring it).

If the FS implementation doesn't even ask and overwrites, I would even use it as the default for Blob, to fix the issue.

Overwriting without any indication or confirmation of it is dangerous and unexpected. Even what we have today with having to delete, then upload is better (note that there's no caching bug anymore, that part of the linked issue is not applicable anymore, this is purely about enhancing the use case of uploading files that would overwrite existing ones).

@hishamco
Copy link
Member Author

hishamco commented Dec 4, 2024

Overwriting without any indication or confirmation of it is dangerous and unexpected.

That's why it's OFF by default, so once you enable it you are ensuring that you don't have a problem with the overwriting behavior

@Piedone
Copy link
Member

Piedone commented Dec 4, 2024

It's still dangerous and unexpected :).

// Let users shoot themselves in the foot. We dare you not to enable it.
// "FootGun": "false

Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't overwrite media
3 participants