#8003 Bugs in attachments to comments
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)
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
Nlinks 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?
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.
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.
Closed #877.
ib/8003Note 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 settingETag, 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 changingattach.url()to append?<last-changed-date>will break those and probably the right way will be to add separate method toAttachmentclass forthumb_urland 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
I think this is a good enough improvement for now to merge it. Cache busting the thumbnails can be separate.