diff --git a/Gemfile b/Gemfile index bfa5a17e43..2a40776e07 100644 --- a/Gemfile +++ b/Gemfile @@ -14,6 +14,7 @@ gem "roadie", "~> 3.2.1" gem "mimemagic" gem "mail", "~> 2.6.4" gem "csv", "~> 1.0.2" if RUBY_VERSION >= "2.3" +gem "rubyzip", "~> 1.2.1" gem "nokogiri", "~> 1.8.0" gem "i18n", "~> 0.7.0" diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb index 9c0bf15baa..a7d9a93a53 100644 --- a/app/controllers/attachments_controller.rb +++ b/app/controllers/attachments_controller.rb @@ -17,6 +17,7 @@ class AttachmentsController < ApplicationController before_action :find_attachment, :only => [:show, :download, :thumbnail, :update, :destroy] + before_action :find_container, :only => [:edit_all, :update_all, :download_all] before_action :find_editable_attachments, :only => [:edit_all, :update_all] before_action :file_readable, :read_authorize, :only => [:show, :download, :thumbnail] before_action :update_authorize, :only => :update @@ -129,6 +130,20 @@ class AttachmentsController < ApplicationController render :action => 'edit_all' end + def download_all + begin + temp_file = Attachment.attachments_to_zip(@container.attachments) + render_404 and return if temp_file.nil? + send_data(File.read(temp_file.path), :type => 'application/zip', :filename => "#{@container.class.to_s.downcase}-#{@container.id}-attachments.zip") + ensure + # Close and delete the temp file + unless temp_file.nil? + temp_file.close + temp_file.unlink + end + end + end + def update @attachment.safe_attributes = params[:attachment] saved = @attachment.save @@ -190,6 +205,11 @@ class AttachmentsController < ApplicationController end def find_editable_attachments + @attachments = @container.attachments.select(&:editable?) + render_404 if @attachments.empty? + end + + def find_container klass = params[:object_type].to_s.singularize.classify.constantize rescue nil unless klass && klass.reflect_on_association(:attachments) render_404 @@ -201,11 +221,9 @@ class AttachmentsController < ApplicationController render_403 return end - @attachments = @container.attachments.select(&:editable?) if @container.respond_to?(:project) @project = @container.project end - render_404 if @attachments.empty? rescue ActiveRecord::RecordNotFound render_404 end diff --git a/app/helpers/attachments_helper.rb b/app/helpers/attachments_helper.rb index 385f3b8a4f..0c2b5869bb 100644 --- a/app/helpers/attachments_helper.rb +++ b/app/helpers/attachments_helper.rb @@ -27,6 +27,10 @@ module AttachmentsHelper object_attachments_path container.class.name.underscore.pluralize, container.id end + def container_attachments_download_path(container) + object_attachments_download_path container.class.name.underscore.pluralize, container.id + end + # Displays view/delete links to the attachments of the given object # Options: # :author -- author names are not displayed if set to false diff --git a/app/models/attachment.rb b/app/models/attachment.rb index cc9b5f930f..b790755a37 100644 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -17,6 +17,7 @@ require "digest" require "fileutils" +require "zip" class Attachment < ActiveRecord::Base include Redmine::SafeAttributes @@ -333,6 +334,26 @@ class Attachment < ActiveRecord::Base Attachment.where("created_on < ? AND (container_type IS NULL OR container_type = '')", Time.now - age).destroy_all end + def self.attachments_to_zip(attachments) + attachments = attachments.select{|attachment| File.readable?(attachment.diskfile) } + return nil if attachments.blank? + temp_file = Tempfile.new('attachments_zip', Rails.root.join(Attachment.storage_path)) + Zip.unicode_names = true + existing_file_names = [] + Zip::File.open(temp_file.path, Zip::File::CREATE) do |zip| + attachments.each do |attachment| + filename = attachment.filename + existing_file_names << filename + # If a file with the same name already exists, change the file name. + if existing_file_names.count(filename) > 1 + filename = "#{File.basename(filename, ".*")}(#{existing_file_names.count(filename)})#{File.extname(filename)}" + end + zip.add(filename, attachment.diskfile) + end + end + temp_file + end + # Moves an existing attachment to its target directory def move_to_target_directory! return unless !new_record? & readable? diff --git a/app/views/attachments/_links.html.erb b/app/views/attachments/_links.html.erb index 0a9f5e3ebf..88593f990a 100644 --- a/app/views/attachments/_links.html.erb +++ b/app/views/attachments/_links.html.erb @@ -5,6 +5,11 @@ :title => l(:label_edit_attachments), :class => 'icon-only icon-edit' ) if options[:editable] %> + <%= link_to(l(:label_download_attachments), + container_attachments_download_path(container), + :title => l(:label_download_attachments), + :class => 'icon-only icon-download' + ) %> <% for attachment in attachments %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 6e2cd75c51..25e59e3cc6 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -988,6 +988,7 @@ en: label_users_visibility_all: All active users label_users_visibility_members_of_visible_projects: Members of visible projects label_edit_attachments: Edit attached files + label_download_attachments: Download attached files label_link_copied_issue: Link copied issue label_ask: Ask label_search_attachments_yes: Search attachment filenames and descriptions diff --git a/config/routes.rb b/config/routes.rb index 0344982085..9bddb8ad0c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -275,6 +275,7 @@ Rails.application.routes.draw do resources :attachments, :only => [:show, :update, :destroy] get 'attachments/:object_type/:object_id/edit', :to => 'attachments#edit_all', :as => :object_attachments_edit patch 'attachments/:object_type/:object_id', :to => 'attachments#update_all', :as => :object_attachments + get 'attachments/:object_type/:object_id/download', :to => 'attachments#download_all', :as => :object_attachments_download resources :groups do resources :memberships, :controller => 'principal_memberships' diff --git a/test/functional/attachments_controller_test.rb b/test/functional/attachments_controller_test.rb index a5d4c17053..e3eaa7e2f5 100644 --- a/test/functional/attachments_controller_test.rb +++ b/test/functional/attachments_controller_test.rb @@ -540,6 +540,30 @@ class AttachmentsControllerTest < Redmine::ControllerTest assert_equal 'This is a Ruby source file', attachment.description end + def test_download_all_with_valid_container + @request.session[:user_id] = 2 + # Check to delete tempfile + Tempfile.any_instance.expects(:close) + Tempfile.any_instance.expects(:unlink) + + get :download_all, :params => { + :object_type => 'issues', + :object_id => '2' + } + assert_response 200 + assert_equal response.headers['Content-Type'], 'application/zip' + assert_match (/issue-2-attachments.zip/), response.headers['Content-Disposition'] + end + + def test_download_all_with_invalid_container + @request.session[:user_id] = 2 + get :download_all, :params => { + :object_type => 'issues', + :object_id => '999' + } + assert_response 404 + end + def test_destroy_issue_attachment set_tmp_attachments_directory issue = Issue.find(3) diff --git a/test/unit/attachment_test.rb b/test/unit/attachment_test.rb index 2bee67ed2a..7a58e7cfcb 100644 --- a/test/unit/attachment_test.rb +++ b/test/unit/attachment_test.rb @@ -277,6 +277,30 @@ class AttachmentTest < ActiveSupport::TestCase end end + def test_attachments_to_zip_with_attachments + attachment = Attachment.create!(:file => uploaded_test_file("testfile.txt", ""), :author_id => 1) + tempfile = Attachment.attachments_to_zip([attachment]) + assert_instance_of Tempfile, tempfile + end + + def test_attachments_to_zip_without_attachments + tempfile = Attachment.attachments_to_zip([]) + assert_nil tempfile + end + + def test_attachments_to_zip_should_not_duplicate_file_names + attachment_1 = Attachment.create!(:file => uploaded_test_file("testfile.txt", ""), :author_id => 1) + attachment_2 = Attachment.create!(:file => uploaded_test_file("testfile.txt", ""), :author_id => 1) + zip_file = Attachment.attachments_to_zip([attachment_1, attachment_2]) + zip_file_names = ['testfile.txt', 'testfile(2).txt'] + + Zip::File.open(zip_file.path) do |zip_file| + zip_file.each_with_index do |entry, i| + assert_includes zip_file_names[i], entry.name + end + end + end + def test_move_from_root_to_target_directory_should_move_root_files a = Attachment.find(20) assert a.disk_directory.blank?