Merge pull request #508 from opf/bug/attachment_delete_closes_work_package_edit

[BUG] Attachment delete closes work package edit

app/controllers/attachments_controller.rb

@@ -59,17 +59,15 @@ class AttachmentsController < ApplicationController
   def destroy
     # Make sure association callbacks are called
     @attachment.container.attachments.delete(@attachment)
-    redirect_to :back
-  rescue ::ActionController::RedirectBackError
-    # we cannot be sure the container actually has a project (example: LandingPage)
-    if @project
-      redirect_to :controller => '/projects', :action => 'show', :id => @project
-    else
-      redirect_to home_url
+
+    respond_to do |format|
+      format.html { redirect_to url_for(destroy_response_url(@attachment.container)) }
+      format.js {}
     end
   end
 
 private
+
   def find_project
     @attachment = Attachment.find(params[:id])
     # Show 404 if the filename in the url is wrong
@@ -99,4 +97,8 @@ private
     end
     content_type.to_s
   end
+
+  def destroy_response_url(container)
+    url_for(container.kind_of?(WikiPage) ? [@project, container.wiki] : container)
+  end
 end

app/views/attachments/_links.html.erb

@@ -28,21 +28,31 @@ See doc/COPYRIGHT.rdoc for more details.
 ++#%>
 
 <div class="attachments">
-<% for attachment in attachments %>
-<p><%= link_to_attachment attachment, :class => 'icon icon-attachment' -%>
-<%= h(" - #{attachment.description}") unless attachment.description.blank? %>
-  <span class="size">(<%= number_to_human_size attachment.filesize %>)</span>
-  <% if options[:deletable] %>
-    <%= link_to image_tag('webalys/delete.png', :alt => l(:button_delete), :title => l(:button_delete)),
-                                        attachment_path(attachment),
-                                        { :confirm => l(:text_are_you_sure),
-                                          :method => :delete,
-                                          :class => 'delete',
-                                          :title => l(:button_delete) } %>
-  <% end %>
-  <% if options[:author] %>
-    <span class="author"><%= h(attachment.author) %>, <%= format_time(attachment.created_on) %></span>
-  <% end %>
-  </p>
-<% end %>
+  <table>
+    <tbody>
+      <% for attachment in attachments %>
+        <tr id="attachment_<%= attachment.id %>">
+          <td>
+            <%= link_to_attachment attachment, :class => 'icon icon-attachment' -%>
+            <%= h(" - #{attachment.description}") unless attachment.description.blank? %>
+            <span class="size">(<%= number_to_human_size attachment.filesize %>)</span>
+            <% if options[:deletable] %>
+              <%= link_to image_tag('webalys/delete.png',
+                                    alt: l(:button_delete),
+                                    title: l(:button_delete)),
+                          attachment,
+                          { confirm: l(:text_are_you_sure),
+                            method: :delete,
+                            class: 'delete',
+                            title: l(:button_delete),
+                            remote: true } %>
+            <% end %>
+            <% if options[:author] %>
+              <span class="author"><%= h(attachment.author) %>, <%= format_time(attachment.created_on) %></span>
+            <% end %>
+          </td>
+        </tr>
+      <% end %>
+    </tbody>
+  </table>
 </div>

app/views/attachments/destroy.js.erb

@@ -0,0 +1,30 @@
+<%#-- copyright
+OpenProject is a project management system.
+Copyright (C) 2012-2013 the OpenProject Foundation (OPF)
+
+This program is free software; you can redistribute it and/or
+modify it under the terms of the GNU General Public License version 3.
+
+OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+Copyright (C) 2006-2013 Jean-Philippe Lang
+Copyright (C) 2010-2013 the ChiliProject Team
+
+This program is free software; you can redistribute it and/or
+modify it under the terms of the GNU General Public License
+as published by the Free Software Foundation; either version 2
+of the License, or (at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program; if not, write to the Free Software
+Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+
+See doc/COPYRIGHT.rdoc for more details.
+
+++#%>
+
+jQuery('#attachment_<%= @attachment.id %>').remove();

doc/CHANGELOG.md

@@ -30,6 +30,7 @@ See doc/COPYRIGHT.rdoc for more details.
 # Changelog
 
 * `#959` Too many available responsibles returned for filtering in timelines
+* `#1916` Work package update screen is closed when attached file is deleted
 * `#1935` Fixed bug: Default submenu for wiki pages is wrong (Configure menu item)
 
 ## 3.0.0pre22

features/work_packages/attachments.feature

@@ -48,16 +48,29 @@ Feature: Attachments on work packages
       | name        | is_closed | is_default |
       | New         | false     | true       |
     Given the user "bob" has 1 issue with the following:
-      | subject     | issue1 |
+      | subject     | work package 1 |
       | type        | Bug    |
-    Given the issue "issue1" has an attachment "logo.gif"
+    Given the issue "work package 1" has an attachment "logo.gif"
     And I am already logged in as "bob"
 
   Scenario: A work package's attachment is listed
-    When I go to the page for the issue "issue1"
+    When I go to the page for the issue "work package 1"
     Then I should see "logo.gif" within ".icon-attachment"
 
   Scenario: Deleting a work package's attachment is possible
-    When I go to the page for the issue "issue1"
+    When I go to the page for the issue "work package 1"
     When I click the first delete attachment link
     Then I should not see ".icon-attachment"
+
+  # see ticket #1916 on OpenProject.org
+  @javascript
+  Scenario: Deleting attachment while editing a work package
+    When I go to the page for the work package "work package 1"
+     And I select "Update" from the action menu
+     And I fill in "Notes" with "Note message"
+
+    When I click the first delete attachment link
+     And I accept the alert dialog
+
+    Then I should not see ".icon-attachment"
+     And the "Notes" field should contain "Note message"

spec/controllers/attachments_controller_spec.rb

@@ -0,0 +1,92 @@
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2013 the OpenProject Foundation (OPF)
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See doc/COPYRIGHT.rdoc for more details.
+#++
+
+require 'spec_helper'
+
+describe AttachmentsController do
+  let(:user) { FactoryGirl.create(:user) }
+  let(:project) { FactoryGirl.create(:project) }
+  let(:role) { FactoryGirl.create(:role,
+                                  permissions: [:edit_work_packages,
+                                                :view_work_packages,
+                                                :delete_wiki_pages_attachments]) }
+  let!(:member) { FactoryGirl.create(:member,
+                                     project: project,
+                                     principal: user,
+                                     roles: [role]) }
+
+  before { User.stub(:current).and_return user }
+
+  describe :destroy do
+    let(:attachment) { FactoryGirl.create(:attachment,
+                                          container: container) }
+
+    shared_examples_for :deleted do
+      subject { Attachment.find_by_id(attachment.id) }
+
+      it { should be_nil }
+    end
+
+    shared_examples_for :redirected do
+      subject { response }
+
+      it { should be_redirect }
+
+      it { should redirect_to(redirect_path) }
+    end
+
+    context :work_package do
+      let(:container) { FactoryGirl.create(:work_package,
+                                           author: user,
+                                           project: project) }
+      let(:redirect_path) { work_package_path(container) }
+
+      before { delete :destroy, id: attachment.id }
+
+      it_behaves_like :deleted
+
+      it_behaves_like :redirected
+    end
+
+    context :wiki do
+      let(:container) { FactoryGirl.create(:wiki_page,
+                                           wiki: project.wiki) }
+      let(:redirect_path) { project_wiki_path(project, project.wiki) }
+
+      before do
+        project.reload # get wiki
+
+        delete :destroy, id: attachment.id
+      end
+
+      it_behaves_like :deleted
+
+      it_behaves_like :redirected
+    end
+  end
+end

test/functional/attachments_controller_test.rb

@@ -120,34 +120,6 @@ class AttachmentsControllerTest < ActionController::TestCase
     assert_redirected_to '/login?back_url=http%3A%2F%2Ftest.host%2Fattachments%2F7%2Fdownload'
   end
 
-  def test_destroy_issue_attachment
-    issue = WorkPackage.find(3)
-    issue.recreate_initial_journal!
-
-    @request.session[:user_id] = 2
-
-    assert_difference 'issue.attachments.count', -1 do
-      delete :destroy, :id => 1
-    end
-    # no referrer
-    issue.reload
-    assert_redirected_to '/projects/ecookbook'
-    assert_nil Attachment.find_by_id(1)
-
-    j = issue.journals.last
-
-    assert_equal [:attachments_1], j.details.keys
-    assert_equal 'error281.txt', j.details[:attachments_1].first
-  end
-
-  def test_destroy_wiki_page_attachment
-    @request.session[:user_id] = 2
-    assert_difference 'Attachment.count', -1 do
-      delete :destroy, :id => 3
-      assert_response 302
-    end
-  end
-
   def test_destroy_without_permission
     delete :destroy, :id => 3
     assert_redirected_to '/login?back_url=http%3A%2F%2Ftest.host%2Fattachments%2F3'