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

Deleting an image that is used as a cover in an album fails to delete the database entry #108

Open
Jokler opened this issue Jun 21, 2022 · 6 comments
Assignees
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@Jokler
Copy link
Member

Jokler commented Jun 21, 2022

As a result the image is deleted but not removed from the list or any albums.

I am not sure what the best way of doing this is but I am starting to think that not deleting it and letting the user know why is the best way to handle this.

@Jokler Jokler added the bug Something isn't working label Jun 21, 2022
@Jokler Jokler self-assigned this Jun 21, 2022
@Jokler Jokler added the help wanted Extra attention is needed label Jun 22, 2022
@dolanske
Copy link
Member

dolanske commented Jul 5, 2022

Would a flow like this work?

  • Check every album coverKey
  • If any album has it set to our image's id, go to said album and change coveryKey to the first image of that album
  • Delete our image (no longer set as coverKey anywhere)

or did I misunderstand the issue?

@Jokler
Copy link
Member Author

Jokler commented Jul 5, 2022

This works until you try to delete an image which is the only image in an album.

@Jokler
Copy link
Member Author

Jokler commented Jul 5, 2022

Maybe we could have a default image which gets set as a cover when the previous one gets deleted. I am not sure how I feel about that though.

@dolanske
Copy link
Member

dolanske commented Jul 6, 2022

I would say, if you delete the last image of an album, we set it as a draft and user gets a notification about it? And we can link to the album edit page where they can either delete it or upload images in it.

@dolanske
Copy link
Member

dolanske commented Jul 7, 2022

My possible solution

When deleting an image, we query the albums it is part of, for each album we have to perform a few checks

  1. If an image is the coverKey of said album
    1. If is more than 1 image left in the album, we replace coverKey with the id of the first image. And delete the original image
    2. If the image we are deleting is the last image on the album, we return an error with a message similar to this example "You can't delete the last picture in an album, please add more pictures or delete the album itself"
  2. If an image is not a coverKey of said album we can simply remove it

If an image is part of multiple albums, we should keep a variable, something like canDelete which by default is set to true and if image doesn't pass any of the album checks for deletion, at the end of the loop, we can return the same error as in 1.ii.


This operation could be pretty heavy, if for each image we query an album(s). Because in most cases, I think people would remove images in bulk. On the front-end, no matter how many selections there are, I always have an array, over which I iterate and call the delete endpoint.

I propose we allow to send in an array of strings, and handle the looping on the back-end. We could then optimize fetching all the albums, as we can scan the ID's and fetch all the relevant albums just once.

@dolanske
Copy link
Member

What if, we simply don't allow deleting images which are used as coverKey? Backend returns an error stating said image can not be deleted because it is used as an album cover image.

If user wanted to remove the picture anyway. they can change the cover image and them remove it without any problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants