#1597 RFE - Add a file attachment section on the Issue page
Closed: Fixed 7 years ago Opened 7 years ago by mreynolds.

It is common for many files/patches/images, etc, to be attached to a single Issue. Having a section listing all the attachments is a nice convenience, especially for issues with many comments.

Attaching patch for this RFE.


While reviewing the patch I started wondering how we could hide the Attachments section if there are no attachments.

While thinking about this, I wondered if it would make sense to move the logic to extract the url off the text from a jinja filter to a property of the ticket/comment.

What do you think? We could then just check ticket.attachments or comment.attachments in the template.

Note: I am also fine with merging as is and make the change myself in a following PR

While reviewing the patch I started wondering how we could hide the Attachments section if there are no attachments.

This should be very easy

While thinking about this, I wondered if it would make sense to move the logic to extract the url off the text from a jinja filter to a property of the ticket/comment.
What do you think? We could then just check ticket.attachments or comment.attachments in the template.

It's up to you. Right now the fix is simple and clean. I think to do what you are suggesting will require a more invasive approach (a new table column?, etc). If you want to change this, then I don't mind if you do it in a new PR, but I'm running out of free time to work on pagure issues :(

Also on a side note, what is the best way to reach you? I've been emailing you over the last few weeks and you never respond. I actually have a critical issue about tags I need your input on.

Thanks,
Mark

I've been emailing you over the last few weeks and you never respond.

Email should be fine, which email address did you try?

I have been moving last week so I was entirely afk and now got a weird error with the server hosting my emails making me unable to access them for 3 days now. But that's not a few weeks, so there is something odd there.

Otherwise, I am most often available on irc as pingou on freenode.

I've been emailing you over the last few weeks and you never respond.

Email should be fine, which email address did you try?

Ugh!!! I was accidentally using pagure@pagure.io

I'll use your redhat account, and resend it. Thanks!

I have been moving last week so I was entirely afk and now got a weird error with the server hosting my emails making me unable to access them for 3 days now. But that's not a few weeks, so there is something odd there.
Otherwise, I am most often available on irc as pingou on freenode.

Attaching new patch that does not display the "Attachment Section" if there are no attachments.

As for moving the attachments into the Issue, this adds complexity that we don't need. Also the way I do it now it's easy to get the comment link. Otherwise we would need to store that comment information in the Issue as well. Seems like overkill for adding a strictly UI feature. However, If you really think its better to maintain the attachment data in the Issue(and db) then I'll do it.

0001-Issue-1597-RFE-add-a-file-attachment-section-to-Issu.patch

This is working great, while reviewing I was wondering if we could merge the two loops into one.

Maybe just extract all the info needed in one loop and give that one to the show_attachments() which would then just worry about displaying them (if there are any).

What do you think? Worth trying?

This is working great, while reviewing I was wondering if we could merge the two loops into one.
Maybe just extract all the info needed in one loop and give that one to the show_attachments() which would then just worry about displaying them (if there are any).
What do you think? Worth trying?

Well I'd just be moving the one loop over to the macro, but there would still be two loops. I'm not sure how to make it one loop, and still have the ability to "remove" the attachments section if there are no attachments.

There will be two loops, one will go over all the comments (possibly a long list) and one over all the attachments (most likely a much shorter list), that was my idea a little.

We could even do without the change to pagure/ui/issues.py and just call {{ show_attachments(issue.attachments) }} in the template.

We could but then we would need to call .attachments twice, so your approach is better yes

arg, looks like this no longer applies cleanly on master, could you rebase?

Nevermind this last comment, I just learned about git am -3 which makes it easier to solve conflicts.

I added a small CSS fix to make the element of the list of little closer to each other and merged.

Thanks!

@pingou changed the status to Closed

7 years ago

Metadata Update from @lslebodn:
- Issue tagged with: IDM

7 years ago

Login to comment on this ticket.

Metadata