I agree, this would be really useful.
--------------------------------------------------------------------------------
+1
--------------------------------------------------------------------------------
+1
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
+1
--------------------------------------------------------------------------------
+1
--------------------------------------------------------------------------------
In order to realize this feature, I thought of compressing and downloading Issue's attachment.
By applying this patch, you can download all of the attached files as a zip file. (using gem rubyzip)

Non-ascii file names are garbled in older versions (Windows 7, Windows Vista, Windows XP).
Windows 7 is scheduled to end support in 2020.
https://github.com/rubyzip/rubyzip/wiki/Files-with-non-ascii-filenames

Any feedback on this patch is welcome.
--------------------------------------------------------------------------------
Thank you for posting the patch. Could you explain why @attachments_to_zip@ method creates temporary files in @Attachment.storage_path@ instead of @tmp@ directory?
--------------------------------------------------------------------------------
I think the patch should consider the free space of the disk. Suppose that the total size of attachments of the issue is 1GB. If the free space of the disk is less than 1GB, the creation of a zip archive will fail and the server may stop working due to disk full.

Here are some ideas to avoid the problem.

* Show an error message indicates no sufficient disk space if the total size of attachments exceeds the free disk space
* Create and send a zip archive without creating a temporary file
--------------------------------------------------------------------------------
Go MAEDA wrote:
> Thank you for posting the patch. Could you explain why @attachments_to_zip@ method creates temporary files in @Attachment.storage_path@ instead of @tmp@ directory?

There was no particular reason. I changed the temporary file storage location to the tmp directory.

Go MAEDA wrote:
> I think the patch should consider the free space of the disk. Suppose that the total size of attachments of the issue is 1GB. If the free space of the disk is less than 1GB, the creation of a zip archive will fail and the server may stop working due to disk full.
>
> Here are some ideas to avoid the problem.
>
> * Show an error message indicates no sufficient disk space if the total size of attachments exceeds the free disk space
> * Create and send a zip archive without creating a temporary file

Thank you for your feedback.

Because I could not think of a good way to check disk space, I thought about limiting the maximum capacity of temporary files.

The maximum capacity of temporary file can be set in configration.yml.
By doing this setting, it is impossible to create a temporary file larger than the assumption of the server administrator.
!{width: 70%; border: 1px solid #ccc}error_message.png!
--------------------------------------------------------------------------------
Mizuki ISHIKAWA wrote:
> Non-ascii file names are garbled in older versions (Windows 7, Windows Vista, Windows XP).

I don't think this is a problem because Windows 7 is going to reach EOL next month (2020-01-14).
https://www.microsoft.com/en-us/microsoft-365/windows/end-of-windows-7-support
--------------------------------------------------------------------------------
I modified the patch to apply to the latest trunk.
--------------------------------------------------------------------------------
I have changed the patch:

* Add a label to the bulk download button
* Move the button to below the attachments list (like "usability plugin":https://www.redmine.org/plugins/usability)
* Updated messages
* Raised the default value for bulk_download_max_size to 512MB

!{border: 1px solid grey;}.screenshot-v4.png!
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
I have updated the patch. Moved bulk_download_max_size setting from configuration.yml to /settings GUI for consistency.
--------------------------------------------------------------------------------
Setting the target version to 4.2.0.
--------------------------------------------------------------------------------
Slightly updated the patch:

* Changed the method name @Attachment.attachments_to_zip@ to @Attachment.archive_attachments@ because @attachments_to_zip@ is too specific. No one knows if the feature uses ZIP format forever, so I think the method name should not depend on a specific archive format
* Changed the text for label_download_all_attachments from "Download all attached files" to simpler "Download all files"
--------------------------------------------------------------------------------
The patch may raise Zip::EntryExistsError when you try to download a ZIP file if a file that has "(2)" suffix in its basename is attached.

!{border: 1px solid grey; width: 445px;}.attachments-that-causes-EntryExistsError@2x.png!

<pre>
Started GET "/attachments/issues/32/download" for 127.0.0.1 at 2020-03-15 10:59:03 +0900
Processing by AttachmentsController#download_all as HTML
.
.
.
Completed 500 Internal Server Error in 28ms (ActiveRecord: 4.9ms)

Zip::EntryExistsError (add failed. Entry testfile(2).txt already exists):

app/models/attachment.rb:362:in `block (2 levels) in archive_attachments'
app/models/attachment.rb:355:in `each'
app/models/attachment.rb:355:in `block in archive_attachments'
app/models/attachment.rb:354:in `archive_attachments'
app/controllers/attachments_controller.rb:139:in `block in download_all'
app/controllers/attachments_controller.rb:138:in `download_all'
lib/redmine/sudo_mode.rb:64:in `sudo_mode'
</pre>

I have fixed the above issue. Also, I have changed the "(n)" suffix for duplicate filenames to start from 1 instead of 2. It is the same behavior as Chrome and Firefox do when downloading files with the same name.
--------------------------------------------------------------------------------
I like this feature.

What do you think if we show the link next to edit attached files icon (as Mizuki proposed first time) in order to keep the links grouped?
!download.png!

I don't find very UX friendly to have the Edit button to the right and the "Download all" button under attachments and thumbnails.

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Also, we should add a test for the new route added by this patch.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Marius BALTEANU wrote:
> What do you think if we show the link next to edit attached files icon (as Mizuki proposed first time) in order to keep the links grouped?

I prefer the current position becuase the position you suggest is very far from the list of files and many people may not notice this useful feature. But it is OK to move the button to the right if we can move this patch forward by doing that.
--------------------------------------------------------------------------------
Go MAEDA wrote:
> Marius BALTEANU wrote:
> > What do you think if we show the link next to edit attached files icon (as Mizuki proposed first time) in order to keep the links grouped?
>
> I prefer the current position becuase the position you suggest is very far from the list of files and many people may not notice this useful feature. But it is OK to move the button to the right if we can move this patch forward by doing that.

I agree with you that having all the action icons in the right corner is maybe not the best option, but this is in the current UI and we cannot just drop some action icons in other places. We should keep the consistency and if we do a change to resolve the distance, we should do it for all the icons because there is no difference between the Edit icon and Delete icon. In all other screens we have the icons one after each other. From my point of view, we can commit this if we move the icon and add the missing test.

--------------------------------------------------------------------------------
Thanks for contributions for releasing this feature.

I updated the patch.
* Add tests to test/integration/routing/attachments_test.rb
* Move icon position to original position
--------------------------------------------------------------------------------
Committed the patch. Thank you for improving Redmine.
--------------------------------------------------------------------------------
The attached patch @human-readable-file-size.patch@ fixes the format of file size in the message @error_bulk_download_size_too_big@ to human-readable.

*Before:*
!{width: 776px; border: 1px solid grey;}.file-size-before@2x.png!

*After:*
!{width: 776px; border: 1px solid grey;}.file-size-after@2x.png!

--------------------------------------------------------------------------------
Go MAEDA wrote:
> The attached patch @human-readable-file-size.patch@ fixes the format of file size in the message @error_bulk_download_size_too_big@ to human-readable.

Committed the patch in r19609.

--------------------------------------------------------------------------------
unfortunately, this feature doesn't work on Windows.

<pre>
Error:
AttachmentTest#test_archive_attachments:
Errno::EACCES: Permission denied @ rb_file_s_rename - (c:/redmine/tmp/attachments_zip20200324-12432-yknlax20200324-12432-1qjlddf, c:/redmine/tmp/attachments_zip20200324-12432-yknlax)
app/models/attachment.rb:355:in `archive_attachments'
test/unit/attachment_test.rb:284:in `block in test_archive_attachments'
test/unit/attachment_test.rb:283:in `test_archive_attachments'
</pre>

I've attached a quick fix.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Pavel Rosický wrote:
> unfortunately, this feature doesn't work on Windows.
>
> [...]

Thanks for reporting this problem.
I fixed it to create a zip file via stream. No temporary file is needed.

<pre><code class="diff">
diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb
index b69dcf983..0c76490b2 100644
--- a/app/controllers/attachments_controller.rb
+++ b/app/controllers/attachments_controller.rb
@@ -137,16 +137,16 @@ class AttachmentsController < ApplicationController
end

def download_all
- Tempfile.create('attachments_zip-', Rails.root.join('tmp')) do |tempfile|
- zip_file = Attachment.archive_attachments(tempfile, @attachments)
- if zip_file
- send_data(
- File.read(zip_file.path),
- :type => 'application/zip',
- :filename => "#{@container.class.to_s.downcase}-#{@container.id}-attachments.zip")
- else
- render_404
- end
+ zip_data = Attachment.archive_attachments(@attachments)
+ if zip_data
+ file_name = "#{@container.class.to_s.downcase}-#{@container.id}-attachments.zip"
+ send_data(
+ zip_data,
+ :type => Redmine::MimeType.of(file_name),
+ :filename => file_name
+ )
+ else
+ render_404
end
end

diff --git a/app/models/attachment.rb b/app/models/attachment.rb
index b5a3332ac..734b8f7fa 100644
--- a/app/models/attachment.rb
+++ b/app/models/attachment.rb
@@ -346,28 +346,28 @@ class Attachment < ActiveRecord::Base
Attachment.where("created_on < ? AND (container_type IS NULL OR container_type = '')", Time.now - age).destroy_all
end

- def self.archive_attachments(out_file, attachments)
+ def self.archive_attachments(attachments)
attachments = attachments.select(&:readable?)
return nil if attachments.blank?

Zip.unicode_names = true
archived_file_names = []
- Zip::File.open(out_file.path, Zip::File::CREATE) do |zip|
+ Zip::OutputStream.write_buffer do |zos|
attachments.each do |attachment|
filename = attachment.filename
# rename the file if a file with the same name already exists
dup_count = 0
while archived_file_names.include?(filename)
dup_count += 1
- basename = File.basename(attachment.filename, '.*')
extname = File.extname(attachment.filename)
+ basename = File.basename(attachment.filename, extname)
filename = "#{basename}(#{dup_count})#{extname}"
end
- zip.add(filename, attachment.diskfile)
+ zos.put_next_entry(filename)
+ zos << IO.binread(attachment.diskfile)
archived_file_names << filename
end
- end
- out_file
+ end.string
end

# Moves an existing attachment to its target directory
</code></pre>
--------------------------------------------------------------------------------
Yuichi HARADA thanks, it looks good!

but I'm getting a warning
<pre>
zlib(finalizer): the stream was freed prematurely.
</pre>

the output StreamIO should be closed
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Pavel Rosický wrote:
> Yuichi HARADA thanks, it looks good!
>
> but I'm getting a warning
> [...]
>
> the output StreamIO should be closed

Thanks for pointing it out.
I had forgotten to perform @StringIO#close@ . Fixed a patch.
--------------------------------------------------------------------------------
Pavel Rosický wrote:
> unfortunately, this feature doesn't work on Windows.
>
> [...]
>
> I've attached a quick fix.

Committed a fix for the issue in r19688. Thank you.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

relates,Reopened,8708,Provide a "download multiple files at once" feature
relates,Closed,35462,Download all attachments in a journal
duplicates,Closed,2662,Download a document