From 288c014aa7aa608751dbafeb2c8b358f2fec5c22 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Sat, 29 Nov 2014 13:41:53 +0000 Subject: [PATCH] Edit attachments after upload (#1326). git-svn-id: http://svn.redmine.org/redmine/trunk@13665 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- app/controllers/attachments_controller.rb | 42 +++++++++++++- app/helpers/attachments_helper.rb | 16 ++++- app/models/attachment.rb | 37 ++++++++++++ app/models/news.rb | 3 +- app/models/project.rb | 1 + app/models/version.rb | 1 + app/views/attachments/_links.html.erb | 3 + app/views/attachments/edit.html.erb | 30 ++++++++++ config/routes.rb | 2 + .../lib/acts_as_attachable.rb | 6 ++ .../functional/attachments_controller_test.rb | 58 +++++++++++++++++++ test/integration/routing/attachments_test.rb | 3 + test/unit/attachment_test.rb | 39 +++++++++++++ 13 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 app/views/attachments/edit.html.erb diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb index b7f856a1a..924e9a186 100644 --- a/app/controllers/attachments_controller.rb +++ b/app/controllers/attachments_controller.rb @@ -16,7 +16,8 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. class AttachmentsController < ApplicationController - before_filter :find_project, :except => :upload + before_filter :find_attachment, :only => [:show, :download, :thumbnail, :destroy] + before_filter :find_editable_attachments, :only => [:edit, :update] before_filter :file_readable, :read_authorize, :only => [:show, :download, :thumbnail] before_filter :delete_authorize, :only => :destroy before_filter :authorize_global, :only => :upload @@ -99,6 +100,19 @@ class AttachmentsController < ApplicationController end end + def edit + end + + def update + if params[:attachments].is_a?(Hash) + if Attachment.update_attachments(@attachments, params[:attachments]) + redirect_back_or_default home_path + return + end + end + render :action => 'edit' + end + def destroy if @attachment.container.respond_to?(:init_journal) @attachment.container.init_journal(User.current) @@ -116,8 +130,9 @@ class AttachmentsController < ApplicationController end end -private - def find_project + private + + def find_attachment @attachment = Attachment.find(params[:id]) # Show 404 if the filename in the url is wrong raise ActiveRecord::RecordNotFound if params[:filename] && params[:filename] != @attachment.filename @@ -126,6 +141,27 @@ private render_404 end + def find_editable_attachments + klass = params[:object_type].to_s.singularize.classify.constantize rescue nil + unless klass && klass.reflect_on_association(:attachments) + render_404 + return + end + + @container = klass.find(params[:object_id]) + if @container.respond_to?(:visible?) && !@container.visible? + 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 + # Checks that the file exists and is readable def file_readable if @attachment.readable? diff --git a/app/helpers/attachments_helper.rb b/app/helpers/attachments_helper.rb index d86fe0dbe..387ed99d7 100644 --- a/app/helpers/attachments_helper.rb +++ b/app/helpers/attachments_helper.rb @@ -18,6 +18,15 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. module AttachmentsHelper + + def container_attachments_edit_path(container) + object_attachments_edit_path container.class.name.underscore.pluralize, container.id + end + + def container_attachments_path(container) + object_attachments_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 @@ -28,7 +37,12 @@ module AttachmentsHelper if container.attachments.any? options = {:deletable => container.attachments_deletable?, :author => true}.merge(options) render :partial => 'attachments/links', - :locals => {:attachments => container.attachments, :options => options, :thumbnails => (options[:thumbnails] && Setting.thumbnails_enabled?)} + :locals => { + :container => container, + :attachments => container.attachments, + :options => options, + :thumbnails => (options[:thumbnails] && Setting.thumbnails_enabled?) + } end end diff --git a/app/models/attachment.rb b/app/models/attachment.rb index da82088f2..42690912a 100644 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -166,6 +166,14 @@ class Attachment < ActiveRecord::Base end end + def editable?(user=User.current) + if container_id + container && container.attachments_editable?(user) + else + author == user + end + end + def deletable?(user=User.current) if container_id container && container.attachments_deletable?(user) @@ -254,6 +262,35 @@ class Attachment < ActiveRecord::Base result end + # Updates the filename and description of a set of attachments + # with the given hash of attributes. Returns true if all + # attachments were updated. + # + # Example: + # Attachment.update_attachments(attachments, { + # 4 => {:filename => 'foo'}, + # 7 => {:filename => 'bar', :description => 'file description'} + # }) + # + def self.update_attachments(attachments, params) + params = params.transform_keys {|key| key.to_i} + + saved = true + transaction do + attachments.each do |attachment| + if p = params[attachment.id] + attachment.filename = p[:filename] if p.key?(:filename) + attachment.description = p[:description] if p.key?(:description) + saved &&= attachment.save + end + end + unless saved + raise ActiveRecord::Rollback + end + end + saved + end + def self.latest_attach(attachments, filename) attachments.sort_by(&:created_on).reverse.detect do |att| att.filename.downcase == filename.downcase diff --git a/app/models/news.rb b/app/models/news.rb index 19a55bda4..6ade4f4f1 100644 --- a/app/models/news.rb +++ b/app/models/news.rb @@ -26,7 +26,8 @@ class News < ActiveRecord::Base validates_length_of :summary, :maximum => 255 attr_protected :id - acts_as_attachable :delete_permission => :manage_news + acts_as_attachable :edit_permission => :manage_news, + :delete_permission => :manage_news acts_as_searchable :columns => ['title', 'summary', "#{table_name}.description"], :scope => preload(:project) acts_as_event :url => Proc.new {|o| {:controller => 'news', :action => 'show', :id => o.id}} diff --git a/app/models/project.rb b/app/models/project.rb index 1a3534217..c21f6ef7e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -60,6 +60,7 @@ class Project < ActiveRecord::Base acts_as_nested_set :dependent => :destroy acts_as_attachable :view_permission => :view_files, + :edit_permission => :manage_files, :delete_permission => :manage_files acts_as_customizable diff --git a/app/models/version.rb b/app/models/version.rb index c8ac6ca0e..e97512c0c 100644 --- a/app/models/version.rb +++ b/app/models/version.rb @@ -22,6 +22,7 @@ class Version < ActiveRecord::Base has_many :fixed_issues, :class_name => 'Issue', :foreign_key => 'fixed_version_id', :dependent => :nullify acts_as_customizable acts_as_attachable :view_permission => :view_files, + :edit_permission => :manage_files, :delete_permission => :manage_files VERSION_STATUSES = %w(open locked closed) diff --git a/app/views/attachments/_links.html.erb b/app/views/attachments/_links.html.erb index d2ab22ced..a8dbd3584 100644 --- a/app/views/attachments/_links.html.erb +++ b/app/views/attachments/_links.html.erb @@ -1,4 +1,7 @@
+
+ <%= link_to image_tag('edit.png'), container_attachments_edit_path(container) if attachments.any?(&:editable?) %> +
<% for attachment in attachments %>

<%= link_to_attachment attachment, :class => 'icon icon-attachment', :download => true -%> <% if attachment.is_text? %> diff --git a/app/views/attachments/edit.html.erb b/app/views/attachments/edit.html.erb new file mode 100644 index 000000000..065488c67 --- /dev/null +++ b/app/views/attachments/edit.html.erb @@ -0,0 +1,30 @@ +

<%= l(:label_attachment_plural) %>

+ +<%= error_messages_for *@attachments %> + +<%= form_tag(container_attachments_path(@container), :method => 'patch') do %> + <%= back_url_hidden_field_tag %> +
+ + <% @attachments.each do |attachment| %> + + + + + + + + <% end %> +
+ <%= attachment.filename_was %> + (<%= number_to_human_size attachment.filesize %>) + <%= h(attachment.author) %>, <%= format_time(attachment.created_on) %> +
<%= text_field_tag "attachments[#{attachment.id}][filename]", attachment.filename, :size => 40 %> + <%= text_field_tag "attachments[#{attachment.id}][description]", attachment.description, :size => 80, :placeholder => l(:label_optional_description) %> +
+
+

+ <%= submit_tag l(:button_save) %> + <%= link_to l(:button_cancel), back_url if back_url.present? %> +

+<% end %> diff --git a/config/routes.rb b/config/routes.rb index 1b57a6ecb..6a3bc023c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -266,6 +266,8 @@ Rails.application.routes.draw do get 'attachments/download/:id', :to => 'attachments#download', :id => /\d+/ get 'attachments/thumbnail/:id(/:size)', :to => 'attachments#thumbnail', :id => /\d+/, :size => /\d+/, :as => 'thumbnail' resources :attachments, :only => [:show, :destroy] + get 'attachments/:object_type/:object_id/edit', :to => 'attachments#edit', :as => :object_attachments_edit + patch 'attachments/:object_type/:object_id', :to => 'attachments#update', :as => :object_attachments resources :groups do resources :memberships, :controller => 'principal_memberships' diff --git a/lib/plugins/acts_as_attachable/lib/acts_as_attachable.rb b/lib/plugins/acts_as_attachable/lib/acts_as_attachable.rb index e65c68c6f..e86b83eb3 100644 --- a/lib/plugins/acts_as_attachable/lib/acts_as_attachable.rb +++ b/lib/plugins/acts_as_attachable/lib/acts_as_attachable.rb @@ -27,6 +27,7 @@ module Redmine cattr_accessor :attachable_options self.attachable_options = {} attachable_options[:view_permission] = options.delete(:view_permission) || "view_#{self.name.pluralize.underscore}".to_sym + attachable_options[:edit_permission] = options.delete(:edit_permission) || "edit_#{self.name.pluralize.underscore}".to_sym attachable_options[:delete_permission] = options.delete(:delete_permission) || "edit_#{self.name.pluralize.underscore}".to_sym has_many :attachments, lambda {order("#{Attachment.table_name}.created_on ASC, #{Attachment.table_name}.id ASC")}, @@ -46,6 +47,11 @@ module Redmine user.allowed_to?(self.class.attachable_options[:view_permission], self.project) end + def attachments_editable?(user=User.current) + (respond_to?(:visible?) ? visible?(user) : true) && + user.allowed_to?(self.class.attachable_options[:edit_permission], self.project) + end + def attachments_deletable?(user=User.current) (respond_to?(:visible?) ? visible?(user) : true) && user.allowed_to?(self.class.attachable_options[:delete_permission], self.project) diff --git a/test/functional/attachments_controller_test.rb b/test/functional/attachments_controller_test.rb index 19e4d0c09..c1b27c64e 100644 --- a/test/functional/attachments_controller_test.rb +++ b/test/functional/attachments_controller_test.rb @@ -327,6 +327,64 @@ class AttachmentsControllerTest < ActionController::TestCase puts '(ImageMagick convert not available)' end + def test_edit + @request.session[:user_id] = 2 + get :edit, :object_type => 'issues', :object_id => '3' + assert_response :success + assert_template 'edit' + + container = Issue.find(3) + assert_equal container, assigns(:container) + assert_equal container.attachments.size, assigns(:attachments).size + + assert_select 'form[action=?]', '/attachments/issues/3' do + assert_select 'tr#attachment-4' do + assert_select 'input[name=?][value=?]', 'attachments[4][filename]', 'source.rb' + assert_select 'input[name=?][value=?]', 'attachments[4][description]', 'This is a Ruby source file' + end + end + end + + def test_edit_invalid_container_class_should_return_404 + get :edit, :object_type => 'nuggets', :object_id => '3' + assert_response 404 + end + + def test_edit_for_object_that_is_not_visible_should_return_403 + get :edit, :object_type => 'issues', :object_id => '4' + assert_response 403 + end + + def test_update + @request.session[:user_id] = 2 + patch :update, :object_type => 'issues', :object_id => '3', :attachments => { + '1' => {:filename => 'newname.text', :description => ''}, + '4' => {:filename => 'newname.rb', :description => 'Renamed'}, + } + + assert_response 302 + attachment = Attachment.find(4) + assert_equal 'newname.rb', attachment.filename + assert_equal 'Renamed', attachment.description + end + + def test_update_with_failure + @request.session[:user_id] = 2 + patch :update, :object_type => 'issues', :object_id => '3', :attachments => { + '1' => {:filename => '', :description => ''}, + '4' => {:filename => 'newname.rb', :description => 'Renamed'}, + } + + assert_response :success + assert_template 'edit' + assert_select_error /file #{ESCAPED_CANT} be blank/i + + # The other attachment should not be updated + attachment = Attachment.find(4) + assert_equal 'source.rb', attachment.filename + assert_equal 'This is a Ruby source file', attachment.description + end + def test_destroy_issue_attachment set_tmp_attachments_directory issue = Issue.find(3) diff --git a/test/integration/routing/attachments_test.rb b/test/integration/routing/attachments_test.rb index d6d278001..9727f9ee3 100644 --- a/test/integration/routing/attachments_test.rb +++ b/test/integration/routing/attachments_test.rb @@ -29,5 +29,8 @@ class RoutingAttachmentsTest < Redmine::RoutingTest should_route 'GET /attachments/thumbnail/1/200' => 'attachments#thumbnail', :id => '1', :size => '200' should_route 'DELETE /attachments/1' => 'attachments#destroy', :id => '1' + + should_route 'GET /attachments/issues/1/edit' => 'attachments#edit', :object_type => 'issues', :object_id => '1' + should_route 'PATCH /attachments/issues/1' => 'attachments#update', :object_type => 'issues', :object_id => '1' end end diff --git a/test/unit/attachment_test.rb b/test/unit/attachment_test.rb index 6a077fcc2..29c433f7b 100644 --- a/test/unit/attachment_test.rb +++ b/test/unit/attachment_test.rb @@ -250,6 +250,45 @@ class AttachmentTest < ActiveSupport::TestCase assert_equal 'text/plain', attachment.content_type end + def test_update_attachments + attachments = Attachment.where(:id => [2, 3]).to_a + + assert Attachment.update_attachments(attachments, { + '2' => {:filename => 'newname.txt', :description => 'New description'}, + 3 => {:filename => 'othername.txt'} + }) + + attachment = Attachment.find(2) + assert_equal 'newname.txt', attachment.filename + assert_equal 'New description', attachment.description + + attachment = Attachment.find(3) + assert_equal 'othername.txt', attachment.filename + end + + def test_update_attachments_with_failure + attachments = Attachment.where(:id => [2, 3]).to_a + + assert !Attachment.update_attachments(attachments, { + '2' => {:filename => '', :description => 'New description'}, + 3 => {:filename => 'othername.txt'} + }) + + attachment = Attachment.find(3) + assert_equal 'logo.gif', attachment.filename + end + + def test_update_attachments_should_sanitize_filename + attachments = Attachment.where(:id => 2).to_a + + assert Attachment.update_attachments(attachments, { + 2 => {:filename => 'newname?.txt'}, + }) + + attachment = Attachment.find(2) + assert_equal 'newname_.txt', attachment.filename + end + def test_latest_attach set_fixtures_attachments_directory a1 = Attachment.find(16) -- 2.39.5