Edit attachments after upload (#1326).

git-svn-id: http://svn.redmine.org/redmine/trunk@13665 e93f8b46-1217-0410-a6f0-8f06a7374b81

13 files changed, 236 insertions, 5 deletions

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 @@
 <div class="attachments">
+<div class="contextual">
+  <%= link_to image_tag('edit.png'), container_attachments_edit_path(container) if attachments.any?(&:editable?) %>
+</div>
 <% for attachment in attachments %>
 <p><%= 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 @@
+<h2><%= l(:label_attachment_plural) %></h2> 
+
+<%= error_messages_for *@attachments %>
+
+<%= form_tag(container_attachments_path(@container), :method => 'patch') do %>
+  <%= back_url_hidden_field_tag %>
+  <div class="box attachments">
+  <table>
+  <% @attachments.each do |attachment| %>
+    <tr>
+      <td colspan="2">
+        <span class="icon icon-attachment"><%= attachment.filename_was %></span>
+        <span class="size">(<%= number_to_human_size attachment.filesize %>)</span>
+        <span class="author"><%= h(attachment.author) %>, <%= format_time(attachment.created_on) %></span>
+      </td>
+    </tr>
+    <tr id="attachment-<%= attachment.id %>">
+      <td><%= text_field_tag "attachments[#{attachment.id}][filename]", attachment.filename, :size => 40 %></td>
+      <td>
+        <%= text_field_tag "attachments[#{attachment.id}][description]", attachment.description, :size => 80, :placeholder => l(:label_optional_description) %>
+      </td>
+    </tr>
+  <% end %>
+  </table>
+  </div>
+  <p>
+    <%= submit_tag l(:button_save) %>
+    <%= link_to l(:button_cancel), back_url if back_url.present? %>
+  </p>
+<% 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)