Merge pull request #3959 from opf/release/5.0

Release/5.0

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

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
@@ -102,7 +115,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

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?

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

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

app/views/attachments/diff.html.erb

@@ -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) %>

app/views/attachments/file.html.erb

@@ -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 %>

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

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>

frontend/app/components/wp-buttons/create-button/create-button.directive.js

@@ -52,8 +52,16 @@ function WorkPackageCreateButtonController($state, ProjectService) {
       canCreate= false;
 
   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;
+  };
+
+  vm.createWorkPackage = function (type) {
+    $state.go(vm.stateName, {
+      projectPath: vm.projectIdentifier,
+      type: type
+    })
   };
 
   if (inProjectContext) {

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'
+  });
+}

frontend/app/components/wp-buttons/create-button/types-drop-down-menu.template.html

@@ -3,10 +3,8 @@
 
   <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})">
-
-        {{type.name}}
+      <a href="" role="menuitem" focus="{{ !$index }}" ng-click="vm.createWorkPackage(type.id)">
+        {{ type.name }}
       </a>
     </li>
   </ul>

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'

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
 

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)

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,

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

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)

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

spec/models/work_package/work_package_nested_set_spec.rb

@@ -36,211 +36,225 @@ 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 '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
+    before do
+      parent.save!
+      instance.parent = parent
+
+      instance.save!
+    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
+    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

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

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

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'

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