From 26dacf4739d12832995cf0885ea7bb8fba4f9017 Mon Sep 17 00:00:00 2001
From: Alex Dik <mail@alexdik.de>
Date: Tue, 1 Dec 2015 08:45:52 +0100
Subject: [PATCH 01/13] Disable WP-create button for create states universally

---
 .../wp-buttons/create-button/create-button.directive.js         | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/frontend/app/components/wp-buttons/create-button/create-button.directive.js b/frontend/app/components/wp-buttons/create-button/create-button.directive.js
index 6068267d74..c96230d6fe 100644
--- a/frontend/app/components/wp-buttons/create-button/create-button.directive.js
+++ b/frontend/app/components/wp-buttons/create-button/create-button.directive.js
@@ -53,7 +53,7 @@ function WorkPackageCreateButtonController($state, ProjectService) {
 
   vm.text = I18n.t('js.toolbar.unselected_title');
   vm.isDisabled = function () {
-    return !inProjectContext || !canCreate || $state.is('work-packages.list.new') || !vm.types;
+    return !inProjectContext || !canCreate || $state.includes('**.new') || !vm.types;
   };
 
   if (inProjectContext) {

From 77f5ce915d55b684b1bb2b4fe4809a6f87b7287c Mon Sep 17 00:00:00 2001
From: Alex Dik <mail@alexdik.de>
Date: Tue, 1 Dec 2015 08:56:17 +0100
Subject: [PATCH 02/13] Move types drop down menu to create button directory

---
 .../create-button.directive.html              |  2 +-
 .../types-drop-down-menu.service.js           | 39 +++++++++++++++++++
 .../types-drop-down-menu.template.html}       |  0
 .../work_packages/controllers/menus/index.js  |  9 -----
 4 files changed, 40 insertions(+), 10 deletions(-)
 create mode 100644 frontend/app/components/wp-buttons/create-button/types-drop-down-menu.service.js
 rename frontend/app/{templates/work_packages/menus/tasks_dropdown_menu.html => components/wp-buttons/create-button/types-drop-down-menu.template.html} (100%)

diff --git a/frontend/app/components/wp-buttons/create-button/create-button.directive.html b/frontend/app/components/wp-buttons/create-button/create-button.directive.html
index 12f8668192..ce6eb23361 100644
--- a/frontend/app/components/wp-buttons/create-button/create-button.directive.html
+++ b/frontend/app/components/wp-buttons/create-button/create-button.directive.html
@@ -1,5 +1,5 @@
 <div class="wp-create-button">
-  <button class="button -alt-highlight add-work-package" has-dropdown-menu target="TasksDropdownMenu"
+  <button class="button -alt-highlight add-work-package" has-dropdown-menu target="typesDropDownMenu"
           locals="vm" ng-disabled="vm.isDisabled()">
 
     <i class="button--icon icon-add"></i>
diff --git a/frontend/app/components/wp-buttons/create-button/types-drop-down-menu.service.js b/frontend/app/components/wp-buttons/create-button/types-drop-down-menu.service.js
new file mode 100644
index 0000000000..e6fa989ded
--- /dev/null
+++ b/frontend/app/components/wp-buttons/create-button/types-drop-down-menu.service.js
@@ -0,0 +1,39 @@
+// -- copyright
+// OpenProject is a project management system.
+// Copyright (C) 2012-2015 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.
+// ++
+
+angular
+  .module('openproject.workPackages')
+  .factory('typesDropDownMenu', typesDropDownMenu);
+
+
+function typesDropDownMenu(ngContextMenu) {
+  return ngContextMenu({
+    templateUrl: '/components/wp-buttons/create-button/types-drop-down-menu.template.html',
+    container: '.wp-create-button'
+  });
+}
diff --git a/frontend/app/templates/work_packages/menus/tasks_dropdown_menu.html b/frontend/app/components/wp-buttons/create-button/types-drop-down-menu.template.html
similarity index 100%
rename from frontend/app/templates/work_packages/menus/tasks_dropdown_menu.html
rename to frontend/app/components/wp-buttons/create-button/types-drop-down-menu.template.html
diff --git a/frontend/app/work_packages/controllers/menus/index.js b/frontend/app/work_packages/controllers/menus/index.js
index 3df80814fe..c1404d0aa3 100644
--- a/frontend/app/work_packages/controllers/menus/index.js
+++ b/frontend/app/work_packages/controllers/menus/index.js
@@ -86,15 +86,6 @@ angular.module('openproject.workPackages')
     'NotificationsService',
     require('./settings-dropdown-menu-controller')
   ])
-  .factory('TasksDropdownMenu', [
-    'ngContextMenu',
-    function(ngContextMenu) {
-      return ngContextMenu({
-        templateUrl: '/templates/work_packages/menus/tasks_dropdown_menu.html',
-        container: '.wp-create-button'
-      });
-    }
-  ])
   .constant('PERMITTED_CONTEXT_MENU_ACTIONS', [
     'edit', 'watch', 'log_time',
     'move', 'copy', 'delete'

From 603d0031aa6e5f7c61fe0cec52ce1f84524ab85f Mon Sep 17 00:00:00 2001
From: Alex Dik <mail@alexdik.de>
Date: Tue, 1 Dec 2015 09:02:39 +0100
Subject: [PATCH 03/13] Use ng-click directive to process state change

---
 .../wp-buttons/create-button/create-button.directive.js   | 8 ++++++++
 .../create-button/types-drop-down-menu.template.html      | 4 +---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/frontend/app/components/wp-buttons/create-button/create-button.directive.js b/frontend/app/components/wp-buttons/create-button/create-button.directive.js
index c96230d6fe..d4088a06ea 100644
--- a/frontend/app/components/wp-buttons/create-button/create-button.directive.js
+++ b/frontend/app/components/wp-buttons/create-button/create-button.directive.js
@@ -52,10 +52,18 @@ function WorkPackageCreateButtonController($state, ProjectService) {
       canCreate= false;
 
   vm.text = I18n.t('js.toolbar.unselected_title');
+
   vm.isDisabled = function () {
     return !inProjectContext || !canCreate || $state.includes('**.new') || !vm.types;
   };
 
+  vm.createWorkPackage = function (type) {
+    $state.go(vm.stateName, {
+      projectPath: vm.projectIdentifier,
+      type: type
+    })
+  };
+
   if (inProjectContext) {
     ProjectService.fetchProjectResource(vm.projectIdentifier).then(function(project) {
       canCreate = !!project.links.createWorkPackage;
diff --git a/frontend/app/components/wp-buttons/create-button/types-drop-down-menu.template.html b/frontend/app/components/wp-buttons/create-button/types-drop-down-menu.template.html
index d9d94c93b0..952f89267b 100644
--- a/frontend/app/components/wp-buttons/create-button/types-drop-down-menu.template.html
+++ b/frontend/app/components/wp-buttons/create-button/types-drop-down-menu.template.html
@@ -3,9 +3,7 @@
 
   <ul class="dropdown-menu">
     <li ng-repeat="type in vm.types">
-      <a role="menuitem" focus="{{ !$index }}"
-          ui-sref="{{ vm.stateName }}({projectPath: vm.projectIdentifier, type: type.id})">
-
+      <a role="menuitem" focus="{{ !$index }}" ng-click="vm.createWorkPackage(type.id)">
         {{type.name}}
       </a>
     </li>

From 64e9c20b47c2228a6f218863720103f03d6b267a Mon Sep 17 00:00:00 2001
From: Alex Dik <mail@alexdik.de>
Date: Tue, 1 Dec 2015 18:05:32 +0100
Subject: [PATCH 04/13] Add href attribute to drop down menu link

---
 .../create-button/types-drop-down-menu.template.html          | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/frontend/app/components/wp-buttons/create-button/types-drop-down-menu.template.html b/frontend/app/components/wp-buttons/create-button/types-drop-down-menu.template.html
index 952f89267b..bb4a1e3035 100644
--- a/frontend/app/components/wp-buttons/create-button/types-drop-down-menu.template.html
+++ b/frontend/app/components/wp-buttons/create-button/types-drop-down-menu.template.html
@@ -3,8 +3,8 @@
 
   <ul class="dropdown-menu">
     <li ng-repeat="type in vm.types">
-      <a role="menuitem" focus="{{ !$index }}" ng-click="vm.createWorkPackage(type.id)">
-        {{type.name}}
+      <a href="" role="menuitem" focus="{{ !$index }}" ng-click="vm.createWorkPackage(type.id)">
+        {{ type.name }}
       </a>
     </li>
   </ul>

From c7c6bc2e6a3fb62ffe6754541776ca95706a7dfd Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Tue, 1 Dec 2015 17:06:38 +0000
Subject: [PATCH 05/13] prevent unnecessary s3 access to see if file is
 readable

---
 app/models/attachment.rb           |  2 +-
 app/uploaders/file_uploader.rb     |  4 ++++
 app/uploaders/fog_file_uploader.rb | 13 +++++++++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/app/models/attachment.rb b/app/models/attachment.rb
index 19fe7d6c9f..521cec6e10 100644
--- a/app/models/attachment.rb
+++ b/app/models/attachment.rb
@@ -102,7 +102,7 @@ class Attachment < ActiveRecord::Base
 
   # Returns true if the file is readable
   def readable?
-    File.readable? file.local_file
+    file.readable?
   end
 
   # Bulk attaches a set of files to an object
diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb
index f93122479a..c5e065db9f 100644
--- a/app/uploaders/file_uploader.rb
+++ b/app/uploaders/file_uploader.rb
@@ -43,6 +43,10 @@ module FileUploader
     self.class.cache_dir
   end
 
+  def readable?
+    File.readable? local_file
+  end
+
   module ClassMethods
     def cache_dir
       @cache_dir ||= begin
diff --git a/app/uploaders/fog_file_uploader.rb b/app/uploaders/fog_file_uploader.rb
index af5ab51f8c..71419d6353 100644
--- a/app/uploaders/fog_file_uploader.rb
+++ b/app/uploaders/fog_file_uploader.rb
@@ -48,4 +48,17 @@ class FogFileUploader < CarrierWave::Uploader::Base
   def download_url
     remote_file.url
   end
+
+  ##
+  # Checks if this file exists and is readable in the remote storage.
+  #
+  # In the current version of carrierwave the call to #exists?
+  # throws an error if the file does not exist:
+  #
+  #   Excon::Errors::Forbidden: Expected(200) <=> Actual(403 Forbidden)
+  def readable?
+    remote_file.exists?
+  rescue Excon::Errors::Forbidden
+    false
+  end
 end

From 7f88ba5b223e370f6ec9fe200dbe7efbe9600bd0 Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Tue, 1 Dec 2015 17:08:06 +0000
Subject: [PATCH 06/13] removed attachments#show; only #download remains

---
 app/controllers/attachments_controller.rb     | 50 +++++++------------
 app/models/attachment.rb                      | 13 +++++
 app/views/attachments/diff.html.erb           | 39 ---------------
 app/views/attachments/file.html.erb           | 46 -----------------
 config/routes.rb                              |  8 +--
 .../v3/attachments/attachment_representer.rb  |  2 +-
 lib/api/v3/utilities/path_helper.rb           |  4 +-
 .../attachments_controller_spec.rb            | 12 +++--
 spec/lib/api/v3/utilities/path_helper_spec.rb | 10 +++-
 9 files changed, 55 insertions(+), 129 deletions(-)
 delete mode 100644 app/views/attachments/diff.html.erb
 delete mode 100644 app/views/attachments/file.html.erb

diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb
index a4585da78f..86326ee6d0 100644
--- a/app/controllers/attachments_controller.rb
+++ b/app/controllers/attachments_controller.rb
@@ -32,29 +32,14 @@ class AttachmentsController < ApplicationController
   before_filter :file_readable, :read_authorize, except: :destroy
   before_filter :delete_authorize, only: :destroy
 
-  def show
-    if @attachment.is_diff?
-      @diff = File.new(@attachment.diskfile, 'rb').read
-      render action: 'diff'
-    elsif @attachment.is_text? && @attachment.filesize <= Setting.file_max_size_displayed.to_i.kilobyte
-      @content = File.new(@attachment.diskfile, 'rb').read
-      render action: 'file'
-    else
-      redirect_to link_to_attachment(@attachment)
-    end
-  end
-
   def download
-    if @attachment.container.is_a?(Version) || @attachment.container.is_a?(Project)
-      @attachment.increment_download
-    end
+    url = @attachment.external_url
 
-    # browsers should not try to guess the content-type
-    response.headers['X-Content-Type-Options'] = 'nosniff'
-
-    send_file @attachment.diskfile, filename: filename_for_content_disposition(@attachment.filename),
-                                    type: @attachment.content_type,
-                                    disposition: @attachment.content_disposition
+    if url
+      redirect_to url.to_s
+    else
+      serve_attachment @attachment
+    end
   end
 
   def destroy
@@ -69,16 +54,6 @@ class AttachmentsController < ApplicationController
 
   private
 
-  def link_to_attachment(attachment)
-    url = URI.parse attachment.file.download_url
-
-    if url.host # check if URL or file path
-      url.to_s
-    else
-      download_attachment_url filename: attachment.filename, id: attachment.id
-    end
-  end
-
   def find_project
     @attachment = Attachment.find(params[:id])
     # Show 404 if the filename in the url is wrong
@@ -104,4 +79,17 @@ class AttachmentsController < ApplicationController
   def destroy_response_url(container)
     url_for(container.is_a?(WikiPage) ? [@project, container.wiki] : container)
   end
+
+  def serve_attachment(attachment)
+    if attachment.container.is_a?(Version) || attachment.container.is_a?(Project)
+      attachment.increment_download
+    end
+
+    # browsers should not try to guess the content-type
+    response.headers['X-Content-Type-Options'] = 'nosniff'
+
+    send_file attachment.diskfile, filename: filename_for_content_disposition(attachment.filename),
+                                   type: attachment.content_type,
+                                   disposition: attachment.content_disposition
+  end
 end
diff --git a/app/models/attachment.rb b/app/models/attachment.rb
index 521cec6e10..35dc3b8544 100644
--- a/app/models/attachment.rb
+++ b/app/models/attachment.rb
@@ -55,6 +55,19 @@ class Attachment < ActiveRecord::Base
     end
   end
 
+  ##
+  # Returns an URL if the attachment is stored in an external (fog) attachment storage
+  # or nil otherwise.
+  def external_url
+    url = URI.parse file.download_url # returns a path if local
+
+    url if url.host
+  end
+
+  def external_storage?
+    !external_url.nil?
+  end
+
   def increment_download
     increment!(:downloads)
   end
diff --git a/app/views/attachments/diff.html.erb b/app/views/attachments/diff.html.erb
deleted file mode 100644
index 762ded2aa3..0000000000
--- a/app/views/attachments/diff.html.erb
+++ /dev/null
@@ -1,39 +0,0 @@
-<%#-- copyright
-OpenProject is a project management system.
-Copyright (C) 2012-2015 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.
-
-++#%>
-<%= toolbar title: h(@attachment.filename) %>
-<div class="attachments">
-  <p><%= h("#{@attachment.description} - ") unless @attachment.description.blank? %>
-    <span class="author"><%= link_to_user(@attachment.author) %>, <%= format_time(@attachment.created_on) %></span></p>
-  <p><%= link_to_attachment @attachment, text: l(:button_download), download: true -%>
-    <span class="size">(<%= number_to_human_size @attachment.filesize %>)</span></p>
-</div>
-&nbsp;
-<%= render partial: 'common/diff', locals: {diff: @diff, diff_type: @diff_type} %>
-
-<% html_title h(@attachment.filename) %>
diff --git a/app/views/attachments/file.html.erb b/app/views/attachments/file.html.erb
deleted file mode 100644
index 64503b5180..0000000000
--- a/app/views/attachments/file.html.erb
+++ /dev/null
@@ -1,46 +0,0 @@
-<%#-- copyright
-OpenProject is a project management system.
-Copyright (C) 2012-2015 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.
-
-++#%>
-<%= toolbar title: h(@attachment.filename) %>
-
-<div class="attachments">
-  <p>
-    <% if @attachment.description.present? %>
-      <%= @attachment.description %> -
-    <% end %>
-    <span class="author"><%= link_to_user(@attachment.author) %>, <%= format_time(@attachment.created_on) %></span>
-  </p>
-  <p>
-    <%= link_to_attachment @attachment, text: t(:button_download), download: true -%>
-    <span class="size">(<%= number_to_human_size @attachment.filesize %>)</span>
-  </p>
-</div>
-&nbsp;
-<%= render partial: 'common/file', locals: {content: @content, filename: @attachment.filename} %>
-
-<% html_title @attachment.filename %>
diff --git a/config/routes.rb b/config/routes.rb
index a8674a18f5..6abcc0b7f1 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -496,14 +496,14 @@ OpenProject::Application.routes.draw do
     post :preview, on: :collection
   end
 
-  resources :attachments, only: [:show, :destroy], format: false do
+  resources :attachments, only: [:destroy], format: false do
     member do
-      scope via: :get,  constraints: { id: /\d+/, filename: /[^\/]*/ } do
-        match 'download(/:filename)' => 'attachments#download', as: 'download'
-        match ':filename' => 'attachments#show'
+      scope via: :get, constraints: { id: /\d+/, filename: /[^\/]*/ } do
+        match '(/:filename)' => 'attachments#download', as: 'download'
       end
     end
   end
+
   # redirect for backwards compatibility
   scope constraints: { id: /\d+/, filename: /[^\/]*/ } do
     get '/attachments/download/:id/:filename' => redirect("#{rails_relative_url_root}/attachments/%{id}/download/%{filename}"), format: false
diff --git a/lib/api/v3/attachments/attachment_representer.rb b/lib/api/v3/attachments/attachment_representer.rb
index 90b3117420..bbb9ebbdb3 100644
--- a/lib/api/v3/attachments/attachment_representer.rb
+++ b/lib/api/v3/attachments/attachment_representer.rb
@@ -46,7 +46,7 @@ module API
 
         link :downloadLocation do
           {
-            href: api_v3_paths.attachment_download(represented.id)
+            href: api_v3_paths.attachment_download(represented.id, represented.filename)
           }
         end
 
diff --git a/lib/api/v3/utilities/path_helper.rb b/lib/api/v3/utilities/path_helper.rb
index e18dbf5693..1965fc635b 100644
--- a/lib/api/v3/utilities/path_helper.rb
+++ b/lib/api/v3/utilities/path_helper.rb
@@ -48,8 +48,8 @@ module API
             "#{root}/attachments/#{id}"
           end
 
-          def self.attachment_download(id)
-            download_attachment_path(id)
+          def self.attachment_download(id, filename = nil)
+            download_attachment_path(id, filename)
           end
 
           def self.attachments_by_work_package(id)
diff --git a/spec/controllers/attachments_controller_spec.rb b/spec/controllers/attachments_controller_spec.rb
index 25d2daf172..a50db13e41 100644
--- a/spec/controllers/attachments_controller_spec.rb
+++ b/spec/controllers/attachments_controller_spec.rb
@@ -100,7 +100,7 @@ describe AttachmentsController, type: :controller do
     end
   end
 
-  describe '#show' do
+  describe '#download' do
     let(:file) { FileHelpers.mock_uploaded_file name: 'foobar.txt' }
     let(:work_package) { FactoryGirl.create :work_package, project: project }
     let(:uploader) { nil }
@@ -127,18 +127,22 @@ describe AttachmentsController, type: :controller do
     end
 
     before do
-      expect(Attachment).to receive(:find).with(attachment.id.to_s).and_return(attachment)
+      allow(Attachment).to receive(:find).with(attachment.id.to_s).and_return(attachment)
     end
 
     subject {
-      get :show, id: attachment.id
+      get :download, id: attachment.id
     }
 
     context 'with a local file' do
       let(:uploader) { LocalFileUploader }
       let(:url) { "http://test.host/attachments/#{attachment.id}/download/#{attachment.filename}" }
+      let(:headers) { subject.headers }
 
-      expect_it { to redirect_to(url) }
+      it 'serves the file' do
+        expect(subject.status).to eq 200
+        expect(headers['Content-Disposition']).to eq 'attachment; filename="foobar.txt"'
+      end
     end
 
     context 'with a remote file' do
diff --git a/spec/lib/api/v3/utilities/path_helper_spec.rb b/spec/lib/api/v3/utilities/path_helper_spec.rb
index 46e119baa2..ec3af98156 100644
--- a/spec/lib/api/v3/utilities/path_helper_spec.rb
+++ b/spec/lib/api/v3/utilities/path_helper_spec.rb
@@ -66,10 +66,16 @@ describe ::API::V3::Utilities::PathHelper do
     it_behaves_like 'api v3 path', '/attachments/1'
   end
 
-  describe '#attachment_download' do
+  describe '#attachment_download without file name' do
     subject { helper.attachment_download 1 }
 
-    it_behaves_like 'path', '/attachments/1/download'
+    it_behaves_like 'path', '/attachments/1'
+  end
+
+  describe '#attachment_download with file name' do
+    subject { helper.attachment_download 1, 'file.png' }
+
+    it_behaves_like 'path', '/attachments/1/file.png'
   end
 
   describe '#attachments_by_work_package' do

From f38c54655a2756d3e4aa3437527b39d7521f0c11 Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Wed, 2 Dec 2015 08:28:17 +0000
Subject: [PATCH 07/13] there's only download left

---
 lib/open_project/object_linking.rb | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/open_project/object_linking.rb b/lib/open_project/object_linking.rb
index c940958202..dbd2c5f0f7 100644
--- a/lib/open_project/object_linking.rb
+++ b/lib/open_project/object_linking.rb
@@ -67,12 +67,11 @@ module OpenProject
     # * :download - Force download (default: false)
     def link_to_attachment(attachment, options = {})
       text = options.delete(:text) || attachment.filename
-      action = options.delete(:download) ? 'download' : 'show'
       only_path = options.delete(:only_path) { true }
 
       link_to h(text),
               { controller: '/attachments',
-                action: action,
+                action: 'download',
                 id: attachment,
                 filename: attachment.filename,
                 host: Setting.host_name,

From 0ab709e846e0ec226779f88824a04efc0da7d9a7 Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Wed, 2 Dec 2015 08:41:59 +0000
Subject: [PATCH 08/13] updated routing specs to reflect removed show action

---
 spec_legacy/integration/routing_spec.rb | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/spec_legacy/integration/routing_spec.rb b/spec_legacy/integration/routing_spec.rb
index 866b3d8794..1c4f143965 100644
--- a/spec_legacy/integration/routing_spec.rb
+++ b/spec_legacy/integration/routing_spec.rb
@@ -71,26 +71,16 @@ describe 'routing', type: :routing do
   context 'attachments' do
     it {
       is_expected.to route(:get, '/attachments/1').to(controller: 'attachments',
-                                                      action: 'show',
+                                                      action: 'download',
                                                       id: '1')
     }
     it {
       is_expected.to route(:get, '/attachments/1/filename.ext').to(controller: 'attachments',
-                                                                   action: 'show',
+                                                                   action: 'download',
                                                                    id: '1',
                                                                    filename: 'filename.ext')
     }
-    it {
-      is_expected.to route(:get, '/attachments/1/download').to(controller: 'attachments',
-                                                               action: 'download',
-                                                               id: '1')
-    }
-    it {
-      is_expected.to route(:get, '/attachments/1/download/filename.ext').to(controller: 'attachments',
-                                                                            action: 'download',
-                                                                            id: '1',
-                                                                            filename: 'filename.ext')
-    }
+
     it 'should redirect /atttachments/download/1 to /attachments/1/download' do
       get '/attachments/download/1'
       assert_redirected_to '/attachments/1/download'

From 1d00e75c2036123ab3d19260e1b0620d2d69cda4 Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Wed, 2 Dec 2015 08:53:58 +0000
Subject: [PATCH 09/13] removed attachment#show specs

---
 .../functional/attachments_controller_spec.rb | 55 +------------------
 1 file changed, 2 insertions(+), 53 deletions(-)

diff --git a/spec_legacy/functional/attachments_controller_spec.rb b/spec_legacy/functional/attachments_controller_spec.rb
index 9b57c4bb51..d64342ceef 100644
--- a/spec_legacy/functional/attachments_controller_spec.rb
+++ b/spec_legacy/functional/attachments_controller_spec.rb
@@ -39,58 +39,7 @@ describe AttachmentsController, type: :controller do
     User.current = nil
   end
 
-  it 'should show diff' do
-    get :show, id: 14 # 060719210727_changeset_utf8.diff
-    assert_response :success
-    assert_template 'diff'
-    assert_equal 'text/html', response.content_type
-
-    assert_select 'th',
-               attributes: { class: /filename/ },
-               content: /issues_controller.rb\t\(révision 1484\)/
-    assert_select 'td',
-               attributes: { class: /line-code/ },
-               content: /Demande créée avec succès/
-  end
-
-  it 'should show diff should strip non utf8 content' do
-    get :show, id: 5 # 060719210727_changeset_iso8859-1.diff
-    assert_response :success
-    assert_template 'diff'
-    assert_equal 'text/html', response.content_type
-
-    assert_select 'th',
-               attributes: { class: /filename/ },
-               content: /issues_controller.rb\t\(rvision 1484\)/
-    assert_select 'td',
-               attributes: { class: /line-code/ },
-               content: /Demande cre avec succs/
-  end
-
-  it 'should show text file' do
-    get :show, id: 4
-    assert_response :success
-    assert_template 'file'
-    assert_equal 'text/html', response.content_type
-  end
-
-  it 'should show text file should send if too big' do
-    Setting.file_max_size_displayed = 512
-    Attachment.find(4).update_attribute :filesize, 754.kilobyte
-
-    get :show, id: 4
-    assert_redirected_to 'http://test.host/attachments/4/download/source.rb'
-
-    get :download, id: 4
-    assert_response :success
-    assert_equal 'text/x-ruby', response.content_type
-  end
-
-  it 'should show other' do
-    get :show, id: 6
-
-    assert_redirected_to 'http://test.host/attachments/6/download/archive.zip'
-
+  it 'should download other' do
     get :download, id: 6
     assert_equal 'application/zip', response.content_type
   end
@@ -108,7 +57,7 @@ describe AttachmentsController, type: :controller do
 
   it 'should anonymous on private private' do
     get :download, id: 7
-    assert_redirected_to '/login?back_url=http%3A%2F%2Ftest.host%2Fattachments%2F7%2Fdownload'
+    assert_redirected_to '/login?back_url=http%3A%2F%2Ftest.host%2Fattachments%2F7'
   end
 
   it 'should destroy without permission' do

From 5e6d657a350e4df029873095271a7d565f71008c Mon Sep 17 00:00:00 2001
From: Jens Ulferts <j.ulferts@finn.de>
Date: Wed, 2 Dec 2015 10:52:56 +0100
Subject: [PATCH 10/13] remove no longer required dynamic spec generation

---
 .../work_package_nested_set_spec.rb           | 406 +++++++++---------
 1 file changed, 201 insertions(+), 205 deletions(-)

diff --git a/spec/models/work_package/work_package_nested_set_spec.rb b/spec/models/work_package/work_package_nested_set_spec.rb
index 928e3264cb..e2f9796cb9 100644
--- a/spec/models/work_package/work_package_nested_set_spec.rb
+++ b/spec/models/work_package/work_package_nested_set_spec.rb
@@ -36,211 +36,207 @@ describe WorkPackage, type: :model do
   let(:work_package2) { FactoryGirl.build(:work_package, project: project) }
   let(:work_package3) { FactoryGirl.build(:work_package, project: project) }
 
-  [:work_package].each do |subclass|
-    describe "(#{subclass})" do
-      let(:instance) { send(subclass) }
-      let(:parent) { send(:"#{subclass}2") }
-      let(:parent2) { send(:"#{subclass}3") }
-
-      shared_examples_for 'root' do
-        it "should set root_id to the id of the #{subclass}" do
-          expect(instance.root_id).to eq(instance.id)
-        end
-
-        it 'should set lft to 1' do
-          expect(instance.lft).to eq(1)
-        end
-
-        it 'should set rgt to 2' do
-          expect(instance.rgt).to eq(2)
-        end
-      end
-
-      shared_examples_for 'first child' do
-        it "should set root_id to the id of the parent #{subclass}" do
-          expect(instance.root_id).to eq(parent.id)
-        end
-
-        it 'should set lft to 2' do
-          expect(instance.lft).to eq(2)
-        end
-
-        it 'should set rgt to 3' do
-          expect(instance.rgt).to eq(3)
-        end
-      end
-
-      describe 'creating a new instance without a parent' do
-        before do
-          instance.save!
-        end
-
-        it_should_behave_like 'root'
-      end
-
-      describe 'creating a new instance with a parent' do
-        before do
-          parent.save!
-          instance.parent = parent
-
-          instance.save!
-        end
-
-        it_should_behave_like 'first child'
-      end
-
-      describe 'an existant instance receives a parent' do
-        before do
-          parent.save!
-          instance.save!
-          instance.parent = parent
-          instance.save!
-        end
-
-        it_should_behave_like 'first child'
-      end
-
-      describe 'an existant instance becomes a root' do
-        before do
-          parent.save!
-          instance.parent = parent
-          instance.save!
-          instance.parent_id = nil
-          instance.save!
-        end
-
-        it_should_behave_like 'root'
-
-        it 'should set parent_id to nil' do
-          expect(instance.parent_id).to eq(nil)
-        end
-      end
-
-      describe 'an existant instance receives a new parent (new tree)' do
-        before do
-          parent.save!
-          parent2.save!
-          instance.parent_id = parent2.id
-          instance.save!
-
-          instance.parent = parent
-          instance.save!
-        end
-
-        it_should_behave_like 'first child'
-
-        it 'should set parent_id to new parent' do
-          expect(instance.parent_id).to eq(parent.id)
-        end
-      end
-
-      describe "an existant instance
-                with a right sibling receives a new parent" do
-        let(:other_child) { send(:"#{subclass}3") }
-
-        before do
-          parent.save!
-          instance.parent = parent
-          instance.save!
-          other_child.parent = parent
-          other_child.save!
-
-          instance.parent_id = nil
-          instance.save!
-        end
-
-        it "former roots's root_id should be unchanged" do
-          parent.reload
-          expect(parent.root_id).to eq(parent.id)
-        end
-
-        it "former roots's lft should be 1" do
-          parent.reload
-          expect(parent.lft).to eq(1)
-        end
-
-        it "former roots's rgt should be 4" do
-          parent.reload
-          expect(parent.rgt).to eq(4)
-        end
-
-        it "former right siblings's root_id should be unchanged" do
-          other_child.reload
-          expect(other_child.root_id).to eq(parent.id)
-        end
-
-        it "former right siblings's left should be 2" do
-          other_child.reload
-          expect(other_child.lft).to eq(2)
-        end
-
-        it "former right siblings's rgt should be 3" do
-          other_child.reload
-          expect(other_child.rgt).to eq(3)
-        end
-      end
-
-      describe 'an existant instance receives a new parent (same tree)' do
-        before do
-          parent.save!
-          parent2.save!
-          instance.parent_id = parent2.id
-          instance.save!
-
-          instance.parent = parent
-          instance.save!
-        end
-
-        it_should_behave_like 'first child'
-      end
-
-      describe 'an existant instance with children receives a new parent (itself)' do
-        let(:child) { send(:"#{subclass}3") }
-
-        before do
-          parent.save!
-          instance.parent = parent
-          instance.save!
-          child.parent_id = instance.id
-          child.save!
-
-          # reloading as instance's nested set attributes (lft, rgt) where
-          # updated by adding child to the set
-          instance.reload
-          instance.parent_id = nil
-          instance.save!
-        end
-
-        it "former parent's root_id should be unchanged" do
-          parent.reload
-          expect(parent.root_id).to eq(parent.id)
-        end
-
-        it "former parent's left should be 1" do
-          parent.reload
-          expect(parent.lft).to eq(1)
-        end
-
-        it "former parent's right should be 2" do
-          parent.reload
-          expect(parent.rgt).to eq(2)
-        end
-
-        it "the child should have the root_id of the parent #{subclass}" do
-          child.reload
-          expect(child.root_id).to eq(instance.id)
-        end
-
-        it 'the child should have a lft of 2' do
-          child.reload
-          expect(child.lft).to eq(2)
-        end
-
-        it 'the child should have a rgt of 3' do
-          child.reload
-          expect(child.rgt).to eq(3)
-        end
-      end
+  let(:instance) { work_package }
+  let(:parent) { work_package2 }
+  let(:parent2) { work_package3 }
+
+  shared_examples_for 'root' do
+    it 'should set root_id to the id of the work_package' do
+      expect(instance.root_id).to eq(instance.id)
+    end
+
+    it 'should set lft to 1' do
+      expect(instance.lft).to eq(1)
+    end
+
+    it 'should set rgt to 2' do
+      expect(instance.rgt).to eq(2)
+    end
+  end
+
+  shared_examples_for 'first child' do
+    it 'should set root_id to the id of the parent work_package' do
+      expect(instance.root_id).to eq(parent.id)
+    end
+
+    it 'should set lft to 2' do
+      expect(instance.lft).to eq(2)
+    end
+
+    it 'should set rgt to 3' do
+      expect(instance.rgt).to eq(3)
+    end
+  end
+
+  describe 'creating a new instance without a parent' do
+    before do
+      instance.save!
+    end
+
+    it_should_behave_like 'root'
+  end
+
+  describe 'creating a new instance with a parent' do
+    before do
+      parent.save!
+      instance.parent = parent
+
+      instance.save!
+    end
+
+    it_should_behave_like 'first child'
+  end
+
+  describe 'an existant instance receives a parent' do
+    before do
+      parent.save!
+      instance.save!
+      instance.parent = parent
+      instance.save!
+    end
+
+    it_should_behave_like 'first child'
+  end
+
+  describe 'an existant instance becomes a root' do
+    before do
+      parent.save!
+      instance.parent = parent
+      instance.save!
+      instance.parent_id = nil
+      instance.save!
+    end
+
+    it_should_behave_like 'root'
+
+    it 'should set parent_id to nil' do
+      expect(instance.parent_id).to eq(nil)
+    end
+  end
+
+  describe 'an existant instance receives a new parent (new tree)' do
+    before do
+      parent.save!
+      parent2.save!
+      instance.parent_id = parent2.id
+      instance.save!
+
+      instance.parent = parent
+      instance.save!
+    end
+
+    it_should_behave_like 'first child'
+
+    it 'should set parent_id to new parent' do
+      expect(instance.parent_id).to eq(parent.id)
+    end
+  end
+
+  describe "an existant instance
+            with a right sibling receives a new parent" do
+    let(:other_child) { work_package3 }
+
+    before do
+      parent.save!
+      instance.parent = parent
+      instance.save!
+      other_child.parent = parent
+      other_child.save!
+
+      instance.parent_id = nil
+      instance.save!
+    end
+
+    it "former roots's root_id should be unchanged" do
+      parent.reload
+      expect(parent.root_id).to eq(parent.id)
+    end
+
+    it "former roots's lft should be 1" do
+      parent.reload
+      expect(parent.lft).to eq(1)
+    end
+
+    it "former roots's rgt should be 4" do
+      parent.reload
+      expect(parent.rgt).to eq(4)
+    end
+
+    it "former right siblings's root_id should be unchanged" do
+      other_child.reload
+      expect(other_child.root_id).to eq(parent.id)
+    end
+
+    it "former right siblings's left should be 2" do
+      other_child.reload
+      expect(other_child.lft).to eq(2)
+    end
+
+    it "former right siblings's rgt should be 3" do
+      other_child.reload
+      expect(other_child.rgt).to eq(3)
+    end
+  end
+
+  describe 'an existant instance receives a new parent (same tree)' do
+    before do
+      parent.save!
+      parent2.save!
+      instance.parent_id = parent2.id
+      instance.save!
+
+      instance.parent = parent
+      instance.save!
+    end
+
+    it_should_behave_like 'first child'
+  end
+
+  describe 'an existant instance with children receives a new parent (itself)' do
+    let(:child) { work_package3 }
+
+    before do
+      parent.save!
+      instance.parent = parent
+      instance.save!
+      child.parent_id = instance.id
+      child.save!
+
+      # reloading as instance's nested set attributes (lft, rgt) where
+      # updated by adding child to the set
+      instance.reload
+      instance.parent_id = nil
+      instance.save!
+    end
+
+    it "former parent's root_id should be unchanged" do
+      parent.reload
+      expect(parent.root_id).to eq(parent.id)
+    end
+
+    it "former parent's left should be 1" do
+      parent.reload
+      expect(parent.lft).to eq(1)
+    end
+
+    it "former parent's right should be 2" do
+      parent.reload
+      expect(parent.rgt).to eq(2)
+    end
+
+    it 'the child should have the root_id of the parent work_package' do
+      child.reload
+      expect(child.root_id).to eq(instance.id)
+    end
+
+    it 'the child should have a lft of 2' do
+      child.reload
+      expect(child.lft).to eq(2)
+    end
+
+    it 'the child should have a rgt of 3' do
+      child.reload
+      expect(child.rgt).to eq(3)
     end
   end
 end

From b8c7d77683a7e88ff82d52776d871bbcda81b0ef Mon Sep 17 00:00:00 2001
From: Jens Ulferts <j.ulferts@finn.de>
Date: Wed, 2 Dec 2015 11:03:19 +0100
Subject: [PATCH 11/13] unpersisted WPs are considered leaves

---
 app/models/work_package.rb                     |  9 +++++++++
 .../work_package_nested_set_spec.rb            | 18 ++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/app/models/work_package.rb b/app/models/work_package.rb
index 1b2d73fc91..ceb5d17a82 100644
--- a/app/models/work_package.rb
+++ b/app/models/work_package.rb
@@ -451,6 +451,15 @@ class WorkPackage < ActiveRecord::Base
     type && type.is_milestone?
   end
 
+  # Overwriting awesome nested set here as it considers unpersisted work
+  # packages to not be leaves.
+  # https://github.com/collectiveidea/awesome_nested_set/blob/master/lib/awesome_nested_set/model.rb#L135
+  # The OP workflow however requires to first create a WP before children can
+  # be assigned to it. Unpersisted WPs are hence always leaves.
+  def leaf?
+    new_record? || super
+  end
+
   # Returns an array of status that user is able to apply
   def new_statuses_allowed_to(user, include_default = false)
     return [] if status.nil?
diff --git a/spec/models/work_package/work_package_nested_set_spec.rb b/spec/models/work_package/work_package_nested_set_spec.rb
index e2f9796cb9..3f5deec3b2 100644
--- a/spec/models/work_package/work_package_nested_set_spec.rb
+++ b/spec/models/work_package/work_package_nested_set_spec.rb
@@ -68,12 +68,22 @@ describe WorkPackage, type: :model do
     end
   end
 
+  describe 'instantiating a new instance' do
+    it 'is considered a leaf' do
+      expect(instance).to be_leaf
+    end
+  end
+
   describe 'creating a new instance without a parent' do
     before do
       instance.save!
     end
 
     it_should_behave_like 'root'
+
+    it 'is considered a leaf' do
+      expect(instance).to be_leaf
+    end
   end
 
   describe 'creating a new instance with a parent' do
@@ -85,6 +95,14 @@ describe WorkPackage, type: :model do
     end
 
     it_should_behave_like 'first child'
+
+    it 'is considered a leaf' do
+      expect(instance).to be_leaf
+    end
+
+    it 'the parent is not considered a leaf' do
+      expect(parent.reload).to_not be_leaf
+    end
   end
 
   describe 'an existant instance receives a parent' do

From 31732d818b1b3fd0010ff9bfebb8fd6423d96e95 Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Wed, 2 Dec 2015 10:52:05 +0000
Subject: [PATCH 12/13] updated specs

---
 spec_legacy/functional/user_mailer_spec.rb          | 6 +++---
 spec_legacy/unit/helpers/application_helper_spec.rb | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/spec_legacy/functional/user_mailer_spec.rb b/spec_legacy/functional/user_mailer_spec.rb
index bff600c00a..f025f73d06 100644
--- a/spec_legacy/functional/user_mailer_spec.rb
+++ b/spec_legacy/functional/user_mailer_spec.rb
@@ -100,7 +100,7 @@ describe UserMailer, type: :mailer do
       end
       # link to an attachment
       assert_select 'a[href=?]',
-                    "https://mydomain.foo/attachments/#{attachment.id}/download",
+                    "https://mydomain.foo/attachments/#{attachment.id}/#{attachment.filename}",
                     text: "#{attachment.filename}"
     end
   end
@@ -143,7 +143,7 @@ describe UserMailer, type: :mailer do
       end
       # link to an attachment
       assert_select 'a[href=?]',
-                    "http://mydomain.foo/rdm/attachments/#{attachment.id}/download",
+                    "http://mydomain.foo/rdm/attachments/#{attachment.id}/#{attachment.filename}",
                     text: "#{attachment.filename}"
     end
   end
@@ -190,7 +190,7 @@ describe UserMailer, type: :mailer do
         end
         # link to an attachment
         assert_select 'a[href=?]',
-                      "http://mydomain.foo/rdm/attachments/#{attachment.id}/download",
+                      "http://mydomain.foo/rdm/attachments/#{attachment.id}/#{attachment.filename}",
                       text: "#{attachment.filename}"
       end
     ensure
diff --git a/spec_legacy/unit/helpers/application_helper_spec.rb b/spec_legacy/unit/helpers/application_helper_spec.rb
index 37b1c1d465..8735806683 100644
--- a/spec_legacy/unit/helpers/application_helper_spec.rb
+++ b/spec_legacy/unit/helpers/application_helper_spec.rb
@@ -128,12 +128,12 @@ RAW
 
   it 'should attached images' do
     to_test = {
-      'Inline image: !logo.gif!' => "Inline image: <img src=\"/attachments/#{@attachment.id}/download\" title=\"This is a logo\" alt=\"This is a logo\" />",
-      'Inline image: !logo.GIF!' => "Inline image: <img src=\"/attachments/#{@attachment.id}/download\" title=\"This is a logo\" alt=\"This is a logo\" />",
+      'Inline image: !logo.gif!' => "Inline image: <img src=\"/attachments/#{@attachment.id}\" title=\"This is a logo\" alt=\"This is a logo\" />",
+      'Inline image: !logo.GIF!' => "Inline image: <img src=\"/attachments/#{@attachment.id}\" title=\"This is a logo\" alt=\"This is a logo\" />",
       'No match: !ogo.gif!' => 'No match: <img src="ogo.gif" alt="" />',
       'No match: !ogo.GIF!' => 'No match: <img src="ogo.GIF" alt="" />',
       # link image
-      '!logo.gif!:http://foo.bar/' => "<a href=\"http://foo.bar/\"><img src=\"/attachments/#{@attachment.id}/download\" title=\"This is a logo\" alt=\"This is a logo\" /></a>",
+      '!logo.gif!:http://foo.bar/' => "<a href=\"http://foo.bar/\"><img src=\"/attachments/#{@attachment.id}\" title=\"This is a logo\" alt=\"This is a logo\" /></a>",
     }
     to_test.each { |text, result| assert_dom_equal "<p>#{result}</p>", helper.format_text(text, attachments: [@attachment]) }
   end

From 840a36217796d47530ab99f6fdfbb32b0028f060 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Oliver=20G=C3=BCnther?= <mail@oliverguenther.de>
Date: Wed, 2 Dec 2015 14:13:20 +0100
Subject: [PATCH 13/13] Squish date to avoid start-of-month bugs

The date output in the `activity_tab_spec` today is `"December  1, 2015"` (mind the extra space),
which no longer matches the result when the date is a single digit.
---
 spec/features/work_packages/tabs/activity_tab_spec.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/spec/features/work_packages/tabs/activity_tab_spec.rb b/spec/features/work_packages/tabs/activity_tab_spec.rb
index bf2f10d285..380d743391 100644
--- a/spec/features/work_packages/tabs/activity_tab_spec.rb
+++ b/spec/features/work_packages/tabs/activity_tab_spec.rb
@@ -74,8 +74,10 @@ describe 'Activity tab', js: true, selenium: true do
       journals.each_with_index do |journal, idx|
         date_selector = ".work-package-details-activities-activity:nth-of-type(#{idx + 1}) " +
                         '.activity-date'
+        # .squish is required in order to match the date without double spaces
+        # on the first 9 days of the month
         expect(page).to have_selector(date_selector,
-                                      text: journal.created_at.to_date.to_s(:long))
+                                      text: journal.created_at.to_date.to_s(:long).squish)
 
         activity = page.find("#activity-#{idx + 1}")
         expect(activity).to have_selector('.user', text: journal.user.name)