Edgewall Software
Modify

Opened 12 years ago

Closed 10 years ago

Last modified 10 years ago

#7078 closed enhancement (fixed)

[PATCH] Download/save all attachments as .zip file

Reported by: mark@… Owned by: Remy Blank
Priority: normal Milestone: 1.0
Component: attachment Version: 0.10.4
Severity: normal Keywords: zip
Cc: Thijs Triemstra Branch:
Release Notes:

attachment: Added the possibility to download all attachments in a list as a .zip.

API Changes:
Internal Changes:

Description

We often have several attachments to each ticket. Downloading each of them separately can be quite tedious, and to ease this process I added a new "Save all as ZIP-file" button to the attachments list.

Attached is a patch file for patching Trac 0.10.4. Feel free to use it or possibly include it in a future version of Trac if you find it useful.

Attachments (5)

trac-10.4-get-attachments-as-zipfile.patch (2.5 KB ) - added by mark@… 12 years ago.
trac-0.11.0-get-attachments-as-zipfile.patch (2.9 KB ) - added by mark@… 12 years ago.
Patch adapted for trac 0.11.0
7078-download-all-r10322.patch (7.8 KB ) - added by Remy Blank 10 years ago.
Allow downloading all attachments of a resource as a ZIP file.
7078-download-all-r10500.patch (8.9 KB ) - added by Remy Blank 10 years ago.
Updated patch.
7078-limit-zip-download-r10551.patch (3.6 KB ) - added by Remy Blank 10 years ago.
Limit the total size of attachments to be downloadable as a .zip.

Download all attachments as: .zip

Change History (21)

comment:1 by Christian Boos, 12 years ago

Component: generalattachment
Milestone: 1.0
Owner: changed from Jonas Borgström to Christian Boos

Interesting, thanks for the patch!

by mark@…, 12 years ago

Patch adapted for trac 0.11.0

comment:2 by Christian Boos, 10 years ago

Milestone: 1.0unscheduled

Milestone 1.0 deleted

comment:3 by Remy Blank, 10 years ago

Milestone: triaging0.13
Owner: changed from Christian Boos to Remy Blank

by Remy Blank, 10 years ago

Allow downloading all attachments of a resource as a ZIP file.

comment:4 by Remy Blank, 10 years ago

7078-download-all-r10322.patch adds "Download all" links next to the "Attachments" title, and allows downloading all attachments of a resource as a ZIP file.

I'm not happy with the placement of the "Download all" link. It's nice on the ticket page, but rather ugly on wiki pages. I haven't found a way to place it right next to the "Attachments" title (i.e. not at the far right) without interfering with folding.

The links are marked with rel="nofollow, in the same way as the alternate format links. Should we also add that attribute to the raw download icons for attachment and source links? Ideally, we should prevent following the links (which is not what rel="nofollow" is about, unfortunately), but this doesn't seem to be possible on a per-link basis.

Also, there is currently no limit on the size of the resulting ZIP, which is built in-memory. Should I add an [attachment] max_download_all_size option for that, and remove the link if the total size of the attachments exceeds that limit?

Comments and suggestions welcome (as always).

comment:5 by AdrianFritz, 10 years ago

Summary: Download/save all attachments as .zip file[PATCH] Download/save all attachments as .zip file

in reply to:  4 comment:6 by Christian Boos, 10 years ago

Replying to rblank:

I'm not happy with the placement of the "Download all" link. It's nice on the ticket page, but rather ugly on wiki pages. I haven't found a way to place it right next to the "Attachments" title (i.e. not at the far right) without interfering with folding.

I haven't tried the patch yet, but I would expect to see the Download all at the bottom of the list (same idea as for normal pages "Download in alternate formats:" at the bottom).

e.g.

Attachments

Also, rather than ?action=download we should use /zip-attachment/... as this is more robots.txt friendly.

Last edited 10 years ago by Christian Boos (previous) (diff)

comment:7 by Remy Blank, 10 years ago

All good ideas, I'll implement them.

by Remy Blank, 10 years ago

Updated patch.

comment:8 by Remy Blank, 10 years ago

7078-download-all-r10500.patch implements the ideas from comment:6.

comment:9 by Thijs Triemstra, 10 years ago

Cc: Thijs Triemstra added
Keywords: zip added

comment:10 by Remy Blank, 10 years ago

Last patch applied in [10551].

One last question: currently, no checks are done as to the size of the resulting .zip. We limit the maximum size of a single attachment with [attachment] max_size, but we don't limit the number of attachments, so theoretically a malicious user could create lots of attachments, and try to download the .zip, in the hope of generating a DoS.

Should I add a [attachment] zip_max_attachments option to limit the number of attachments allowed for a .zip? Or maybe rather a [attachment] max_zip_size option to limit the maximum total size of attachments downloadable as a .zip? I tend to prefer the latter, suggested patch coming shortly.

by Remy Blank, 10 years ago

Limit the total size of attachments to be downloadable as a .zip.

comment:11 by Remy Blank, 10 years ago

7078-limit-zip-download-r10551.patch adds an option [attachment] max_zip_size to specify the maximum total size of a list of attachments to be downloadable as a .zip. It can be set to -1 to disable the functionality.

comment:12 by Christian Boos, 10 years ago

I've read the patch, it's perfect (well, except for the nitpick that I personally try to use "…" strings for messages and '…' for string constants, but I'm probably the only one to find that useful ;-) ).

in reply to:  12 comment:13 by Remy Blank, 10 years ago

Replying to cboos:

except for the nitpick that I personally try to use "…" strings for messages and '…' for string constants, but I'm probably the only one to find that useful ;-)

You know, it's funny you should mention that. I started doing exactly that in my plugin. In core, there's no clear tendency about that, both are equally present.

comment:14 by Remy Blank, 10 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: newclosed

Patch applied in [10553]. Thanks for reviewing!

comment:15 by bsheridan@…, 10 years ago

I believe the code added for the total_size check breaks the functionality. Since Attachment.select is implemented as a generator, I believe the line

total_size = sum(attachment.size for attachment in attachments)

consumes it and iterating it later…

for attachment in attachments:
    ...

is empty.

Collapsing the generator into a list first seems to correct the problem for us.

  • attachment.py

     
    719721
    720722    def _download_as_zip(self, req, parent, attachments=None):
    721723        if attachments is None:
    722             attachments = Attachment.select(self.env, parent.realm, parent.id)
     724            attachments = [a for a in Attachment.select(self.env, parent.realm, parent.id)]
    723725        total_size = sum(attachment.size for attachment in attachments)
    724726        if total_size > self.max_zip_size:
    725727            raise TracError(_("Maximum total attachment size: %(num)s bytes",

in reply to:  15 comment:16 by Remy Blank, 10 years ago

Replying to bsheridan@…:

I believe the code added for the total_size check breaks the functionality.

This has been fixed already by Christian in [10588]. Please update!

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.