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

✨ Add delete media functionality #502

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

Conversation

JMicheli
Copy link
Collaborator

@JMicheli JMicheli commented Nov 7, 2024

This pull request addresses #110 by adding the requested feature.

I added a new endpoint to the api::v1::media::individual module that allows deletion of files by users with the ManageLibrary permission. The endpoint takes an optional query parameter to allow deletion of the underlying file.

For the UI, I added a button to the management screen for deleting books. Clicking it opens a confirmation modal that allows a user to check a box to delete the underlying file.

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

Attention: Patch coverage is 0% with 76 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
apps/server/src/routers/api/v1/media/individual.rs 0.00% 70 Missing ⚠️
apps/server/src/routers/api/v1/media/mod.rs 0.00% 5 Missing ⚠️
apps/server/src/routers/api/mod.rs 0.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
apps/server/src/routers/api/mod.rs 0.00% <0.00%> (ø)
apps/server/src/routers/api/v1/media/mod.rs 0.00% <0.00%> (ø)
apps/server/src/routers/api/v1/media/individual.rs 0.00% <0.00%> (ø)

🚨 Try these New Features:

@aaronleopold
Copy link
Collaborator

There are two big things I'd like to touch on before digging into this feature: Deletion from the perspective of the DB vs deletion of files directly

I'll create explicit sections to better organize my thoughts

Deletion (DB)

It isn't actually used today, but media records do have a deleted_at field. There is an issue that, after rereading I realize is a duplicate of the file deletion lol, references soft vs hard deletion: #336.

This does have some implications though, primarily that most queries would need to be refactored as to exclude any deleted books by default.

I thought I had more to discuss here, but I'm coming up blank. I think the section below is more relevant to the conversation at hand, and it touches on the tentative interactions between the DB and file deletion. I'll leave this here for the useful links and move on

Deletion (files)

Originally, in the referenced issue, I expressed the preference for some kind of "trash" functionality:

I'm personally not against adding some utilities in the stump_core package to generalize trashing files per user request, so long as it's trashing them and not completely, hard deleting them

I think this might be a more palatable first step into the deletion realm for users. Programmatic deletion is generally non-recoverable unless someone has their own backup system, which is somewhat scary. The elephant in the room is that "trashing" a file in Docker doesn't necessarily make sense. I think a viable route would be to implement some kind of custom trash system where:

  • When in docker, move files to a mounted /trash path. Develop an API for emptying the trash, which does the full file removal
    • This extra barrier to deletion IMO is a good, inherent safety net
  • When outside docker, use the existing trash crate to push things into the native trash for the operating system

I also think we need to add something like file:delete to the user permission enum. This functionality extends beyond just managing a library IMO.


OK. Rant ended. All of this to say, I'm not entirely sure what the best combination of these deletion strategies is. A potentially helpful use-case to consider could be:

  • A user reads a book, decides they don't like it, soft-deletes it from the library which moves it to the trash and sets the deleted_at field
  • A few weeks later, a different user wants the book. The original user can restore the book from the trash, which moves it back to the library and unsets the deleted_at field. Since the file was never actually deleted, the book is still available for the second user. Additionally, since the record was never deleted, the reading history is still intact
  • If the second point never happened, the trash could be emptied instead. The file would be deleted, the record dropped, and any relations (e.g. reading progress) should automatically cascade and get deleted.

I can't enumerate all the potential use-cases, and this is just the first I could think of, but I think this is a good starting point for how I see it in my head at least. I'm okay making broad strokes / decisions here since, generally speaking, if people are actually using Stump they aren't very vocal about what they want vs definitely don't want.

Once we land on a concrete path forward, I think we can move the more granular and implementation-specific conversation to Discord for convenience. But please let me know if you have any thoughts or differing opinions! I'm happy to discuss further

@JMicheli
Copy link
Collaborator Author

JMicheli commented Nov 7, 2024

Okay so the deleted_at part makes sense to me, and adding a trash space should be easy using that. I do think there are two use cases not discussed above:

  1. The user made a mistake when structuring their library and wants to delete a book from the database, then immediately rescan the library to get things right. This would be a situation where the trash option probably makes sense, since they could forget about it and simply rescan
  2. The user made a mistake when uploading a book and wants to remove the file immediately and re-upload. I think this is probably something that should be accommodated, since people will make mistakes with the new upload functionality. In this instance, having it go to the trash and only be soft deleted might not make sense.

@aaronleopold
Copy link
Collaborator

aaronleopold commented Nov 7, 2024

The user made a mistake when structuring their library and wants to delete a book from the database, then immediately rescan the library to get things right.

They wouldn't have to rescan at all, assuming it is implemented as I described above:

This does have some implications though, primarily that most queries would need to be refactored as to exclude any deleted books by default.

So if I were to delete a book this way, the UI should refetch the current query which doesn't include the book anymore. Let me know if I'm not explaining that bit well

The user made a mistake when uploading a book and wants to remove the file immediately and re-upload

I don't see much difference in this scenario, but I do understand your point. The problem I see is that to accomodate this would mean two separate-ish code paths:

  • Delete when the record is "fresh" or "new"
  • Delete when the record is "stale" or "old enough"

What would those relative time descriptors be? Should it be X minutes? Would Y minutes be too much? Etc. The UI would also need to present what would happen in the same manner, since there would be an inconsistency in what an action does between these two scenarios.

If the difference wasn't there, it might be a little more tedious to empty the trash for an accidental upload for sure. But I am not sure if the break in consistency would be worth the saved click. Edited since this probably doesn't really matter at the end of the day.

If we can come up with acceptable definitions for these kinds of things and document them accordingly, I'd be okay with it though 🙂

@JMicheli JMicheli marked this pull request as draft November 11, 2024 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants