mozilla / sumo

Project management board for SUMO and Community properties.
Mozilla Public License 2.0
13 stars 5 forks source link

[Accounts] The user can't delete a uploaded image #304

Closed ovidiuboca1 closed 3 years ago

ovidiuboca1 commented 4 years ago

Prerequisites: You need to ask a question and upload an image. This is the link that I'm using - https://support.allizom.org/en-US/questions/1207026

STR:

  1. Try to delete the uploaded image by hovering over it and click on the "X" button

ER: The image is deleted. AR: The image is still there. Screen recording - https://streamable.com/fydes

NOTE: For this test, I've used the account that doesn't have the admin privileges. I've also tested this using the admin account and I was able to delete the picture.

mirunacurtean commented 4 years ago

Re-tested this in Stage on Windows 10 and Android 9.0 across FF and Chrome browsers and it still occurs. Thread containing uploaded image: https://support.allizom.org/en-US/questions/1207081 I've tried deleting the image with both the reply's owner account and also with a moderator account and the result was the same; the image is not deleted. A '404 Not found' is the request's result.

Stage Cannot delete image

In Production I could only try to delete the image using the reply's owner account, lacking a moderator one, and the results is slightly different. On the first try, the image appears to be deleted initially, but after a page refresh it can be seen that it's still posted. Subsequent tries have resulted in the same behavior as in stage. Thread from production: https://support.mozilla.org/en-US/questions/1291721#answer-1330674

Related Bugzilla issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1650824

mirunacurtean commented 3 years ago

The initial test case has been verified fixed across Windows pro 10, android 9.0.0, iPhone 13.4.1 and macOS Mojave 10.14.6 on Firefox Release/ Fenix and also on Safari and Chrome. Other test cases can be viewed here.

@LeoMcA I have been investigating a few potential issues:

  1. first of all, should an image still be removed from the question or replies of it was deleted in edit mode but then the edit was cancelled? image is removed even though edit was cancelled

  2. second, should an image that was uploaded but not posted (in edit mode) still show up as uploaded when editing again? like this: image is still uploaded

  3. I've been trying very hard to reproduce this an I think I've finally figured it out. The steps are:

    • edit question
    • upload image and then delete it
    • upload image again
    • cancel edit
    • notice that the image is posted in the question thread despite that.

Image uploaded despite cancelling update

This is the example https://support.allizom.org/en-US/questions/1207100. I cannot manage to reproduce this in production - could it be a regression? Should I create a separate issue for it?

LeoMcA commented 3 years ago

@LeoMcA I have been investigating a few potential issues

Ah, these are very interesting, and stem from the way our attachments work, so yes these should (mostly) be happening insofar as that's how the architecture currently is. As for whether our UX should work this way, I'll defer to @jess-cook03.

A technical explanation for why these things are happening:

  1. first of all, should an image still be removed from the question or replies if it was deleted in edit mode but then the edit was cancelled?

This happens because attachments to a question/answer are treated as their own objects. So the "x" button on the image in edit mode directly sends a request to delete the image from the server. Cancelling the edit cancels any changes to the post itself, but the image has already been deleted.

We could do something like, in edit mode, just queue up those deletions, and then actually execute them when save is clicked. However I'm not sure how that would change the behaviour with attachments where we can delete them outside of edit mode - when a user with sufficient permissions views a post they can delete those attachments directly.

Perhaps copy will help here, indicating that changes to images are done immediately?

  1. second, should an image that was uploaded but not posted (in edit mode) still show up as uploaded when editing again? like this:

When creating a new question/answer, the images are associated with the user - rather than the post, because this doesn't exist yet. However that means if you start creating a post, then cancel it, the image is still associated with the user and so will appear the next time that user starts creating a post.

We could clear these images associated with the user when cancel is clicked when creating a new post.

  1. I've been trying very hard to reproduce this and I think I've finally figured it out. The steps are: edit question, upload image and then delete it, upload image again, cancel edit, notice that the image is posted in the question thread despite that.

This is a very interesting one. So, actually, following on from everything I've said before, these steps should be possible:

However, the last step here won't happen, because the cache won't have been invalidated. So, despite the uploaded attachment actually being attached to the post, the image won't appear because the previous set of images is still cached.

What you're doing with your repro steps is clearing the post's image cache by deleting an image. So actually reordering those steps a little should also work:

I agree that this seems pretty confusing to users: "I cancelled my edit, but my image was still attached!". I guess some copy could also help here, but it might be a little much to explain to users.

mirunacurtean commented 3 years ago

We could do something like, in edit mode, just queue up those deletions, and then actually execute them when save is clicked. However I'm not sure how that would change the behavior with attachments where we can delete them outside of edit mode - when a user with sufficient permissions views a post they can delete those attachments directly.

Perhaps copy will help here, indicating that changes to images are done immediately?

I agree that copy would be a solution that would not impact the image deletion much. Depending on the time we have to get something like that done, I am also willing to create another issue for us to deal with this at a later date.

We could clear these images associated with the user when cancel is clicked when creating a new post.

This behavior has been here for a while, but yes, your proposed solution is more inline with what I was expecting and if done would also probably impact the repro of issue number 3.

What you're doing with your repro steps is clearing the post's image cache by deleting an image. So actually reordering those steps a little should also work:

  • edit post
  • upload an attachment
  • delete it
  • upload one or more attachments
  • cancel editing the post
  • notice the attachments uploaded after deleting one are posted despite that

I agree that this seems pretty confusing to users: "I cancelled my edit, but my image was still attached!". I guess some copy could also help here, but it might be a little much to explain to users.

I quit looking for a possible user flow for this one and just focused on getting it reproduced in the end, but the repro steps used in your last example are something that I can imagine has happened a few times, however, I don't imagine there's an easy way to provide a helpful message for this. Luckily deleting an image added to a post by the post owner or an account with elevated permissions is very easy so we can count on that.

LeoMcA commented 3 years ago

I've opened https://github.com/mozilla/kitsune/pull/4589 to add an explanatory note, and fix the cache not being invalidated when a question/answer edit is cancelled.

And I've created https://github.com/mozilla/sumo-project/issues/691 to track the (rather-involved) work around changing the UX here.

mirunacurtean commented 3 years ago

Re-tested. Results are passed. Added Other user is unable to delete through editing the images posted by a superuser/ moderator account as a separate issue. Regarding https://github.com/mozilla/kitsune/pull/4589 right now there's an image/text overlap that needs to be tied up and then this will be ready for release.

mirunacurtean commented 3 years ago

Info text overlap has been fixed. Tested across Firefox and Chrome on Windows pro 10 and Android 9.0.0. image