#8003 Bugs in attachments to comments

v1.4.0
closed
General
2016-01-11
2015-10-14
No

Editing a comment to modify its attachments has some bugs, compared to what you can do for top-level artifact attachments.

  • no way to delete an attachment
  • uploading with the same name duplicates the file, rather than replace it (see [#4350] which could be considered a feature)

Related

Tickets: #4350

Discussion

  • Igor Bondarenko

    Igor Bondarenko - 2015-12-09
    • labels: --> 42cc
    • status: open --> in-progress
    • assigned_to: Igor Bondarenko
     
  • Igor Bondarenko

    Igor Bondarenko - 2015-12-11

    Seems like somethig changed since this ticket was created. Now you can delete attachments for comments just fine.

    We need to decide regarding second point though. What behavior do we want?

    In fact, I just tested and top-level artifact editing also duplicates a file but not replaces it (tested on tickets). And it does it in a weird way. The file stays the same as the first uploaded version, but you have N links to it (which are the same) in the UI.

    So, should we make all attachments replace each other if name matches, or should we fix duplication to generate different links and save all version of the files?

     
    • Dave Brondsema

      Dave Brondsema - 2015-12-14

      I think replacing it would be good. It's easier, and its often the expected behavior, for example if you are attaching some script or a screenshot, then upload with a new name you probably mean to update it and don't need anyone to access the previous version.

       
  • Dave Brondsema

    Dave Brondsema - 2015-12-14
    • labels: 42cc --> 42cc, sf-2, sf-current
     
  • Igor Bondarenko

    Igor Bondarenko - 2015-12-24

    Regarding attachments removal, there is actually a bug. You can't delete attachments to other user's comment even if you're a moderator and can edit the comment itself and add/rewrite new attachments. Fix is underway.

     
  • Igor Bondarenko

    Igor Bondarenko - 2015-12-25
    • status: in-progress --> review
     
  • Igor Bondarenko

    Igor Bondarenko - 2015-12-25

    Closed #877.

    ib/8003

    Note that if you're replacing image attachment thumbnail will not change rightaway. It will change only if you reload a page manually. That's because of browser caching. At least in Chrome, after adding the attachment and redirecting back to the, let's say, tickets page, browser will not issue another request for thumbnail, but will use one in cache. To avoid that we'll need to change thumb/attach url whenever image is changed (e.g. append ?<last-changed-date> to the url). We're setting ETag, but it's not sufficient because browser doesn't make request to check if image changed in some cases as I described above.

    I didn't change url right now, since there's quite a few places that need to be changed. E.g. thumb url always constructed manually from attach.url() in templates, so changing attach.url() to append ?<last-changed-date> will break those and probably the right way will be to add separate method to Attachment class for thumb_url and use that everywhere.

    Anyway, wanted to consult you guys before doing that. Check out the new code and let me know what do you think. Thanks

     
  • Dave Brondsema

    Dave Brondsema - 2016-01-05
    • status: review --> closed
    • Reviewer: Dave Brondsema
     
  • Dave Brondsema

    Dave Brondsema - 2016-01-05

    I think this is a good enough improvement for now to merge it. Cache busting the thumbnails can be separate.

     
  • Dave Brondsema

    Dave Brondsema - 2016-01-11
    • labels: 42cc, sf-2, sf-current --> 42cc, sf-2
     
  • Dave Brondsema

    Dave Brondsema - 2016-04-11
    • Milestone: unreleased --> v1.4.0
     

Log in to post a comment.