allow setting attachments through wp post/patch

app/contracts/attachments/validate_replacements.rb

@@ -0,0 +1,58 @@
+#-- encoding: UTF-8
+
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2018 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-2017 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 docs/COPYRIGHT.rdoc for more details.
+#++
+
+require 'model_contract'
+
+module Attachments
+  module ValidateReplacements
+    extend ActiveSupport::Concern
+
+    included do
+      validate :validate_attachments_replacements
+    end
+
+    private
+
+    def validate_attachments_replacements
+      model.attachments_replacements and model.attachments_replacements.each do |attachment|
+        error_if_attachment_assigned(attachment)
+        error_if_other_user_attachment(attachment)
+      end
+    end
+
+    def error_if_attachment_assigned(attachment)
+      errors.add :attachments, :unchangeable if attachment.container && attachment.container != model
+    end
+
+    def error_if_other_user_attachment(attachment)
+      errors.add :attachments, :does_not_exist if !attachment.container && attachment.author != user
+    end
+  end
+end

app/contracts/work_packages/base_contract.rb

@@ -32,6 +32,8 @@ require 'model_contract'
 
 module WorkPackages
   class BaseContract < ::ModelContract
+    include ::Attachments::ValidateReplacements
+
     def self.model
       WorkPackage
     end

app/controllers/application_controller.rb

@@ -557,8 +557,9 @@ class ApplicationController < ActionController::Base
 
   # Renders a warning flash if obj has unsaved attachments
   def render_attachment_warning_if_needed(obj)
-    if obj.unsaved_attachments.present?
-      flash[:warning] = l(:warning_attachments_not_saved, obj.unsaved_attachments.size)
+    unsaved_attachments = obj.attachments.select(&:new_record?)
+    if unsaved_attachments.any?
+      flash[:warning] = l(:warning_attachments_not_saved, unsaved_attachments.size)
     end
   end
 

app/controllers/messages_controller.rb

@@ -1,4 +1,5 @@
 #-- encoding: UTF-8
+
 #-- copyright
 # OpenProject is a project management system.
 # Copyright (C) 2012-2018 the OpenProject Foundation (OPF)
@@ -52,9 +53,9 @@ class MessagesController < ApplicationController
     end
 
     @replies = @topic.children.includes(:author, :attachments, board: :project)
-               .order("#{Message.table_name}.created_on ASC")
-               .page(page)
-               .per_page(per_page_param)
+                     .order("#{Message.table_name}.created_on ASC")
+                     .page(page)
+                     .per_page(per_page_param)
 
     @reply = Message.new(subject: "RE: #{@message.subject}")
     render action: 'show', layout: !request.xhr?
@@ -76,11 +77,11 @@ class MessagesController < ApplicationController
     end
 
     @message.attributes = permitted_params.message(@message)
-
     @message.attach_files(permitted_params.attachments.to_h)
 
     if @message.save
-      call_hook(:controller_messages_new_after_save,  params: params, message: @message)
+      render_attachment_warning_if_needed(@message)
+      call_hook(:controller_messages_new_after_save, params: params, message: @message)
 
       redirect_to topic_path(@message)
     else
@@ -96,31 +97,31 @@ class MessagesController < ApplicationController
     @reply.author = User.current
     @reply.board = @board
     @reply.attributes = permitted_params.reply
+    @reply.attach_files(permitted_params.attachments.to_h)
 
     @topic.children << @reply
-    if !@reply.new_record?
-      call_hook(:controller_messages_reply_after_save,  params: params, message: @reply)
-      attachments = Attachment.attach_files(@reply, permitted_params.attachments)
+    unless @reply.new_record?
       render_attachment_warning_if_needed(@reply)
+      call_hook(:controller_messages_reply_after_save, params: params, message: @reply)
     end
     redirect_to topic_path(@topic, r: @reply)
   end
 
   # Edit a message
   def edit
-    (render_403; return false) unless @message.editable_by?(User.current)
+    return render_403 unless @message.editable_by?(User.current)
     @message.attributes = permitted_params.message(@message)
   end
 
   # Edit a message
   def update
-    (render_403; return false) unless @message.editable_by?(User.current)
+    return render_403 unless @message.editable_by?(User.current)
 
     @message.attributes = permitted_params.message(@message)
-
     @message.attach_files(permitted_params.attachments.to_h)
 
     if @message.save
+      render_attachment_warning_if_needed(@message)
       flash[:notice] = l(:notice_successful_update)
       @message.reload
       redirect_to topic_path(@message.root, r: (@message.parent_id && @message.id))
@@ -131,12 +132,16 @@ class MessagesController < ApplicationController
 
   # Delete a messages
   def destroy
-    (render_403; return false) unless @message.destroyable_by?(User.current)
+    return render_403 unless @message.destroyable_by?(User.current)
     @message.destroy
     flash[:notice] = l(:notice_successful_delete)
-    redirect_to @message.parent.nil? ?
-      { controller: '/boards', action: 'show', project_id: @project, id: @board } :
-      { action: 'show', id: @message.parent, r: @message }
+    redirect_target = if @message.parent.nil?
+                        { controller: '/boards', action: 'show', project_id: @project, id: @board }
+                      else
+                        { action: 'show', id: @message.parent, r: @message }
+                      end
+
+    redirect_to redirect_target
   end
 
   def quote

app/controllers/wiki_controller.rb

@@ -42,26 +42,24 @@ require 'htmldiff'
 # * destroy - normal
 #
 # Other member and collection methods are also used
-#
-# TODO: still being worked on
 class WikiController < ApplicationController
   default_search_scope :wiki_pages
   before_action :find_wiki, :authorize
-  before_action :find_existing_page, only: [:edit_parent_page,
-                                            :update_parent_page,
-                                            :rename,
-                                            :protect,
-                                            :history,
-                                            :diff,
-                                            :annotate,
-                                            :add_attachment,
-                                            :list_attachments,
-                                            :destroy]
-  before_action :build_wiki_page_and_content, only: [:new, :create]
+  before_action :find_existing_page, only: %i[edit_parent_page
+                                              update_parent_page
+                                              rename
+                                              protect
+                                              history
+                                              diff
+                                              annotate
+                                              add_attachment
+                                              list_attachments
+                                              destroy]
+  before_action :build_wiki_page_and_content, only: %i[new create]
 
   verify method: :post, only: [:protect], redirect_to: { action: :show }
-  verify method: :get,  only: [:new, :new_child], render: { nothing: true, status: :method_not_allowed }
-  verify method: :post, only: :create,            render: { nothing: true, status: :method_not_allowed }
+  verify method: :get,  only: %i[new new_child], render: { nothing: true, status: :method_not_allowed }
+  verify method: :post, only: :create, render: { nothing: true, status: :method_not_allowed }
 
   include AttachmentsHelper
   include PaginationHelper
@@ -93,8 +91,7 @@ class WikiController < ApplicationController
     @pages_by_date = @pages.group_by { |p| p.updated_on.to_date }
   end
 
-  def new
-  end
+  def new; end
 
   def new_child
     find_existing_page
@@ -113,9 +110,9 @@ class WikiController < ApplicationController
 
     @content.attributes = permitted_params.wiki_content
     @content.author = User.current
+    @page.attach_files(permitted_params.attachments.to_h)
 
     if @page.save
-      attachments = Attachment.attach_files(@page, permitted_params.attachments.to_h)
       render_attachment_warning_if_needed(@page)
       call_hook(:controller_wiki_edit_after_save, params: params, page: @page)
       flash[:notice] = l(:notice_successful_create)
@@ -176,7 +173,6 @@ class WikiController < ApplicationController
     @content.lock_version = @page.content.lock_version
   end
 
-  verify method: :put, only: :update, render: { nothing: true, status: :method_not_allowed }
   # Creates a new page or updates an existing one
   def update
     @page = @wiki.find_or_new_page(wiki_page_title)
@@ -191,9 +187,9 @@ class WikiController < ApplicationController
     @content.text = initial_page_content(@page) if @content.text.blank?
     # don't keep previous comment
     @content.comments = nil
+    @page.attach_files(permitted_params.attachments.to_h)
 
     if !@page.new_record? && params[:content].present? && @content.text == params[:content][:text]
-      attachments = Attachment.attach_files(@page, permitted_params.attachments.to_h)
       render_attachment_warning_if_needed(@page)
       # don't save if text wasn't changed
       redirect_to_show
@@ -204,15 +200,13 @@ class WikiController < ApplicationController
     @content.add_journal User.current, params['content']['comments']
     # if page is new @page.save will also save content, but not if page isn't a new record
     if @page.new_record? ? @page.save : @content.save
-      attachments = Attachment.attach_files(@page, permitted_params.attachments.to_h)
       render_attachment_warning_if_needed(@page)
-      call_hook(:controller_wiki_edit_after_save,  params: params, page: @page)
+      call_hook(:controller_wiki_edit_after_save, params: params, page: @page)
       flash[:notice] = l(:notice_successful_update)
       redirect_to_show
     else
       render action: 'edit'
     end
-
   rescue ActiveRecord::StaleObjectError
     # Optimistic locking exception
     flash.now[:error] = l(:notice_locking_conflict)
@@ -286,7 +280,10 @@ class WikiController < ApplicationController
   # show page history
   def history
     # don't load text
-    @versions = @page.content.versions.select('id, user_id, notes, created_at, version')
+    @versions = @page
+                .content
+                .versions
+                .select(:id, :user_id, :notes, :created_at, :version)
                 .order('version DESC')
                 .page(params[:page])
                 .per_page(per_page_param)
@@ -295,7 +292,7 @@ class WikiController < ApplicationController
   end
 
   def diff
-    if @diff = @page.diff(params[:version], params[:version_from])
+    if (@diff = @page.diff(params[:version], params[:version_from]))
       @html_diff = HTMLDiff::DiffBuilder.new(@diff.content_from.data.text, @diff.content_to.data.text).build
     else
       render_404
@@ -360,8 +357,8 @@ class WikiController < ApplicationController
 
   def add_attachment
     return render_403 unless editable?
-    attachments = Attachment.attach_files(@page, permitted_params.attachments.to_h)
-    render_attachment_warning_if_needed(@page)
+    @page.attach_files(permitted_params.attachments.to_h)
+    @page.save
     redirect_to action: 'show', id: @page, project_id: @project
   end
 

app/models/attachment.rb

@@ -31,15 +31,16 @@
 require 'digest/md5'
 
 class Attachment < ActiveRecord::Base
-  ALLOWED_IMAGE_TYPES = %w[ image/gif image/jpeg image/png image/tiff image/bmp ]
+  ALLOWED_IMAGE_TYPES = %w[image/gif image/jpeg image/png image/tiff image/bmp].freeze
 
   belongs_to :container, polymorphic: true
   belongs_to :author, class_name: 'User', foreign_key: 'author_id'
 
-  validates_presence_of :container, :author, :content_type, :filesize
+  validates_presence_of :author, :content_type, :filesize
   validates_length_of :description, maximum: 255
 
-  validate :filesize_below_allowed_maximum
+  validate :filesize_below_allowed_maximum,
+           :container_changed_more_than_once
 
   acts_as_journalized
   acts_as_event title: -> { file.name },
@@ -51,12 +52,6 @@ class Attachment < ActiveRecord::Base
 
   after_commit :extract_fulltext, on: :create
 
-  def filesize_below_allowed_maximum
-    if filesize > Setting.attachment_max_size.to_i.kilobytes
-      errors.add(:file, :file_too_large, count: Setting.attachment_max_size.to_i.kilobytes)
-    end
-  end
-
   ##
   # Returns an URL if the attachment is stored in an external (fog) attachment storage
   # or nil otherwise.
@@ -86,11 +81,15 @@ class Attachment < ActiveRecord::Base
   end
 
   def visible?(user = User.current)
-    container.attachments_visible?(user)
+    allowed_or_author?(user) do
+      container.attachments_visible?(user)
+    end
   end
 
   def deletable?(user = User.current)
-    container.attachments_deletable?(user)
+    allowed_or_author?(user) do
+      container.attachments_deletable?(user)
+    end
   end
 
   # images are sent inline
@@ -122,37 +121,6 @@ class Attachment < ActiveRecord::Base
     file.readable?
   end
 
-  def cache_key
-    "#{super}-#{created_on.to_i}"
-  end
-
-  # Bulk attaches a set of files to an object
-  #
-  # Returns a Hash of the results:
-  # files: array of the attached files
-  # unsaved: array of the files that could not be attached
-  def self.attach_files(obj, attachments)
-    attached = []
-    if attachments
-      attachments.each_value do |attachment|
-        file = attachment['file']
-        next unless file && file.size > 0
-        a = Attachment.create(container: obj,
-                              file: file,
-                              description: attachment['description'].to_s.strip,
-                              author: User.current)
-
-        if a.new_record?
-          obj.unsaved_attachments ||= []
-          obj.unsaved_attachments << a
-        else
-          attached << a
-        end
-      end
-    end
-    { files: attached, unsaved: obj.unsaved_attachments }
-  end
-
   def diskfile
     file.local_file
   end
@@ -209,4 +177,31 @@ class Attachment < ActiveRecord::Base
       job.perform
     end
   end
+
+  private
+
+  def filesize_below_allowed_maximum
+    if filesize > Setting.attachment_max_size.to_i.kilobytes
+      errors.add(:file, :file_too_large, count: Setting.attachment_max_size.to_i.kilobytes)
+    end
+  end
+
+  def container_changed_more_than_once
+    if container_id_changed_more_than_once? || container_type_changed_more_than_once?
+      errors.add(:container, :unchangeable)
+    end
+  end
+
+  def container_id_changed_more_than_once?
+    container_id_changed? && container_id_was.present? && container_id_was != container_id
+  end
+
+  def container_type_changed_more_than_once?
+    container_type_changed? && container_type_was.present? && container_type_was != container_type
+  end
+
+  def allowed_or_author?(user)
+    containered? && yield ||
+      !containered? && author_id == user.id
+  end
 end

app/models/message.rb

@@ -35,7 +35,8 @@ class Message < ActiveRecord::Base
   belongs_to :author, class_name: 'User', foreign_key: 'author_id'
   acts_as_tree counter_cache: :replies_count, order: "#{Message.table_name}.created_on ASC"
   acts_as_attachable after_add: :attachments_changed,
-                     after_remove: :attachments_changed
+                     after_remove: :attachments_changed,
+                     add_permission: %i[edit_messages add_messages]
   belongs_to :last_reply, class_name: 'Message', foreign_key: 'last_reply_id'
 
   acts_as_journalized

app/models/work_package.rb

@@ -141,7 +141,9 @@ class WorkPackage < ActiveRecord::Base
   # test_destroying_root_projects_should_clear_data #
   # for details.                                    #
   ###################################################
-  acts_as_attachable after_remove: :attachments_changed, order: "#{Attachment.table_name}.filename"
+  acts_as_attachable after_remove: :attachments_changed,
+                     order: "#{Attachment.table_name}.filename",
+                     add_permission: %i[add_work_packages edit_work_packages]
 
   after_validation :set_attachments_error_details,
                    if: lambda { |work_package| work_package.errors.messages.has_key? :attachments }
@@ -321,7 +323,7 @@ class WorkPackage < ActiveRecord::Base
 
   # Overrides Redmine::Acts::Customizable::InstanceMethods#available_custom_fields
   def available_custom_fields
-    project && type ? (project.all_work_package_custom_fields & type.custom_fields) : []
+    WorkPackage::AvailableCustomFields.for(project, type)
   end
 
   # aliasing subject to name

app/models/work_package/available_custom_fields.rb

@@ -0,0 +1,33 @@
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2018 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-2017 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 docs/COPYRIGHT.rdoc for more details.
+#++
+
+module WorkPackage::AvailableCustomFields
+  def self.for(project, type)
+    project && type ? (project.all_work_package_custom_fields & type.custom_fields) : []
+  end
+end

app/services/work_packages/create_service.rb

@@ -54,7 +54,12 @@ class WorkPackages::CreateService
   def create(attributes, work_package)
     result = set_attributes(attributes, work_package)
 
-    result.success &&= work_package.save
+    result.success = if result.success
+                       work_package.attachments = work_package.attachments_replacements if work_package.attachments_replacements
+                       work_package.save
+                     else
+                       false
+                     end
 
     if result.success?
       result.merge!(reschedule_related(work_package))

app/services/work_packages/set_attributes_service.rb

@@ -59,7 +59,10 @@ class WorkPackages::SetAttributesService
   end
 
   def set_attributes(attributes)
-    work_package.attributes = attributes
+    if attributes.key?(:attachment_ids)
+      work_package.attachments_replacements = Attachment.where(id: attributes[:attachment_ids])
+    end
+    work_package.attributes = attributes.except(:attachment_ids)
 
     set_default_attributes
     unify_dates

app/services/work_packages/update_service.rb

@@ -54,6 +54,7 @@ class WorkPackages::UpdateService
     result = set_attributes(attributes)
 
     if result.success?
+      work_package.attachments = work_package.attachments_replacements if work_package.attachments_replacements
       result.merge!(update_dependent)
     end
 

app/views/attachments/_links.html.erb

@@ -45,7 +45,7 @@ See docs/COPYRIGHT.rdoc for more details.
                             title: l(:button_delete) } %>
             <% end %>
             <% if options[:author] %>
-              <span class="author"><%= h(attachment.author) %>, <%= format_time(attachment.created_on) %></span>
+              <span class="author"><%= h(attachment.author) %>, <%= format_time(attachment.created_at) %></span>
             <% end %>
           </td>
         </tr>

config/locales/crowdin/af.yml

@@ -1809,8 +1809,7 @@ af:
     text_show_again: You can restart this video from the help menu
     welcome: Welcome to OpenProject
   permission_add_work_package_notes: Add notes
-  permission_add_work_packages: Add work packages (also allows to add attachments
-    to all work packages)
+  permission_add_work_packages: Add work packages
   permission_add_messages: Post messages
   permission_add_project: Create project
   permission_add_subprojects: Create subprojects

config/locales/crowdin/ar.yml

@@ -1877,8 +1877,7 @@ ar:
     text_show_again: يمكنك إعادة تشغيل هذا الفيديو من قائمة المساعدة
     welcome: مرحبًا بكم في أوبِن بروجِكت
   permission_add_work_package_notes: إضافة ملاحظات
-  permission_add_work_packages: إضافة حزم العمل (كما يسمح لإضافة مرفقات إلى كافة حزم
-    العمل)
+  permission_add_work_packages: إضافة حزم العمل
   permission_add_messages: نشر الرسائل
   permission_add_project: إنشاء مشروع
   permission_add_subprojects: إنشاء مشاريع فرعية

config/locales/crowdin/bg.yml

@@ -1808,8 +1808,7 @@ bg:
     text_show_again: Можете да рестартирате това видео от менюто помощ
     welcome: Welcome to OpenProject
   permission_add_work_package_notes: Добавяне на бележки
-  permission_add_work_packages: Add work packages (also allows to add attachments
-    to all work packages)
+  permission_add_work_packages: Add work packages
   permission_add_messages: Post messages
   permission_add_project: Създаване на проект
   permission_add_subprojects: Create subprojects

config/locales/crowdin/ca.yml

@@ -1829,8 +1829,7 @@ ca:
     text_show_again: Pot reprendre aquest vídeo des del menú d'ajuda
     welcome: Welcome to OpenProject
   permission_add_work_package_notes: Afegir notes
-  permission_add_work_packages: Afegir paquets de treball (també permet afegir fitxers
-    adjunts a tots els paquets de treball)
+  permission_add_work_packages: Afegir paquets de treball
   permission_add_messages: Publicar missatges
   permission_add_project: Crear un projecte
   permission_add_subprojects: Crear subprojectes

config/locales/crowdin/cs.yml

@@ -1843,8 +1843,7 @@ cs:
     text_show_again: You can restart this video from the help menu
     welcome: Vítejte v OpenProject
   permission_add_work_package_notes: Add notes
-  permission_add_work_packages: Add work packages (also allows to add attachments
-    to all work packages)
+  permission_add_work_packages: Add work packages
   permission_add_messages: Post messages
   permission_add_project: Vytvořit projekt
   permission_add_subprojects: Vytvořit podprojekty

config/locales/crowdin/da.yml

@@ -1799,8 +1799,7 @@ da:
     text_show_again: You can restart this video from the help menu
     welcome: Velkommen til OpenProject
   permission_add_work_package_notes: Tilføj noter
-  permission_add_work_packages: Add work packages (also allows to add attachments
-    to all work packages)
+  permission_add_work_packages: Add work packages
   permission_add_messages: Send beskeder
   permission_add_project: Opret projekt
   permission_add_subprojects: Opret underprojekter

config/locales/crowdin/de.yml

@@ -1846,8 +1846,7 @@ de:
     text_show_again: Sie können dieses Video über das Hilfe-Menü erneut starten
     welcome: Willkommen bei OpenProject
   permission_add_work_package_notes: Kommentare hinzufügen
-  permission_add_work_packages: Arbeitspakete hinzufügen (enthält Recht Anhänge zu
-    Arbeitspaketen hinzuzufügen)
+  permission_add_work_packages: Arbeitspakete hinzufügen
   permission_add_messages: Forenbeiträge hinzufügen
   permission_add_project: Projekt erstellen
   permission_add_subprojects: Unterprojekte erstellen

config/locales/crowdin/es.yml

@@ -1849,8 +1849,7 @@ es:
     text_show_again: Puede reiniciar este vídeo en el menú ayuda
     welcome: Bienvenido a OpenProject
   permission_add_work_package_notes: Añadir notas
-  permission_add_work_packages: Añadir paquete de trabajo (Tambien habilita los adjuntos
-    para todos los paquetes de trabajo)
+  permission_add_work_packages: Añadir paquete de trabajo
   permission_add_messages: Publicar mensajes
   permission_add_project: Crear proyecto
   permission_add_subprojects: Crear subproyectos

config/locales/crowdin/et.yml

@@ -1793,8 +1793,7 @@ et:
     text_show_again: You can restart this video from the help menu
     welcome: Welcome to OpenProject
   permission_add_work_package_notes: Märkusi lisada
-  permission_add_work_packages: Add work packages (also allows to add attachments
-    to all work packages)
+  permission_add_work_packages: Add work packages
   permission_add_messages: Postitusi lisada
   permission_add_project: Projekte luua
   permission_add_subprojects: Alamprojekte luua

config/locales/crowdin/fa.yml

@@ -1797,8 +1797,7 @@ fa:
     text_show_again: You can restart this video from the help menu
     welcome: Welcome to OpenProject
   permission_add_work_package_notes: Add notes
-  permission_add_work_packages: Add work packages (also allows to add attachments
-    to all work packages)
+  permission_add_work_packages: Add work packages
   permission_add_messages: Post messages
   permission_add_project: Create project
   permission_add_subprojects: Create subprojects

config/locales/crowdin/fi.yml

@@ -1795,8 +1795,7 @@ fi:
     text_show_again: Voit käynnistää tämä videon uudelleen ohjeet-valikosta
     welcome: Tervetuloa OpenProjectiin
   permission_add_work_package_notes: Lisää muistiinpanoja
-  permission_add_work_packages: Add work packages (also allows to add attachments
-    to all work packages)
+  permission_add_work_packages: Add work packages
   permission_add_messages: Jätä viesti
   permission_add_project: Luo projekti
   permission_add_subprojects: Luoda aliprojekteja

config/locales/crowdin/fil.yml

@@ -1857,8 +1857,7 @@ fil:
     text_show_again: Maari mong i-restart amg video na ito mula sa tulong ng pagpipilian
     welcome: Maligayang pagdating sa OpenProject
   permission_add_work_package_notes: Magdagdag ng mga talaan
-  permission_add_work_packages: Magdagdag ng mga work packages (ito rin ay nagpapahintulot
-    na mag dagdag ng mga attachment sa lahat ng mga work packages)
+  permission_add_work_packages: Magdagdag ng mga work packages
   permission_add_messages: Mga post na mensahe
   permission_add_project: Lumikha ng proyekto
   permission_add_subprojects: Lumikha ng mga subproject

config/locales/crowdin/fr.yml

@@ -1840,8 +1840,7 @@ fr:
     text_show_again: Vous pouvez relancer cette vidéo au départ du menu d'aide
     welcome: Bienvenue dans OpenProject
   permission_add_work_package_notes: Ajouter des notes
-  permission_add_work_packages: Ajouter des lots de travaux (permet également d'ajouter
-    des pièces jointes à tous les lots de travaux)
+  permission_add_work_packages: Ajouter des lots de travaux
   permission_add_messages: Poster des messages
   permission_add_project: Créer un projet
   permission_add_subprojects: Créer des sous-projets

config/locales/crowdin/he.yml

@@ -1829,8 +1829,7 @@ he:
     text_show_again: You can restart this video from the help menu
     welcome: Welcome to OpenProject
   permission_add_work_package_notes: הוסף הערות
-  permission_add_work_packages: הוסף חבילות עבודה (גם מאפשר הוספה קבצים מצורפים לכל
-    חבילות עבודה)
+  permission_add_work_packages: הוסף חבילות עבודה
   permission_add_messages: Post messages
   permission_add_project: Create project
   permission_add_subprojects: Create subprojects

config/locales/crowdin/hr.yml

@@ -1836,8 +1836,7 @@ hr:
     text_show_again: You can restart this video from the help menu
     welcome: Welcome to OpenProject
   permission_add_work_package_notes: Dodaj bilješke
-  permission_add_work_packages: Dodaj radni paket (također omogućava dodavanje privitaka
-    svim radnim paketima)
+  permission_add_work_packages: Dodaj radni paket
   permission_add_messages: Pošalji poruke
   permission_add_project: Stvori projekt
   permission_add_subprojects: Kreiraj potprojekte

config/locales/crowdin/hu.yml

@@ -1810,8 +1810,7 @@ hu:
     text_show_again: Újra tudod indítani ezt a videót a segítség menüből
     welcome: Üdvözöl az OpenProject
   permission_add_work_package_notes: Megjegyzések hozzáadása
-  permission_add_work_packages: Munkacsomag hozzáadása (lehetővé teszi, hogy hozzáadjunk
-    mellékleteket minden munkacsomaghoz)
+  permission_add_work_packages: Munkacsomag hozzáadása
   permission_add_messages: Elküldött üzenetek
   permission_add_project: Projekt létrehozása
   permission_add_subprojects: Alprojektek létrehozása

config/locales/crowdin/id.yml

@@ -1795,8 +1795,7 @@ id:
     text_show_again: You can restart this video from the help menu
     welcome: Welcome to OpenProject
   permission_add_work_package_notes: Tambahkan catatan
-  permission_add_work_packages: Add work packages (also allows to add attachments
-    to all work packages)
+  permission_add_work_packages: Add work packages
   permission_add_messages: Kirim pesan
   permission_add_project: Buat Project
   permission_add_subprojects: Buat Sub Project

config/locales/crowdin/it.yml

@@ -1840,8 +1840,7 @@ it:
     text_show_again: È possibile riavviare questo video dal menu Guida
     welcome: Benvenuto in OpenProject
   permission_add_work_package_notes: Aggiungi nota
-  permission_add_work_packages: Aggiungi macro-attività (permette anche di aggiungere
-    allegati a tutte le macro-attività)
+  permission_add_work_packages: Aggiungi macro-attività
   permission_add_messages: Postare messaggi
   permission_add_project: Creare un progetto
   permission_add_subprojects: Creare sotto-progetti

config/locales/crowdin/ja.yml

@@ -1683,7 +1683,7 @@ ja:
     text_show_again: ヘルプメニューからこのビデオを再度開始することができます。
     welcome: OpenProjectへようこそ
   permission_add_work_package_notes: 注記の追加
-  permission_add_work_packages: 作業項目を追加 (すべての作業項目に添付ファイルを追加することもできます)
+  permission_add_work_packages: 作業項目を追加
   permission_add_messages: メッセージの投稿
   permission_add_project: プロジェクトの作成
   permission_add_subprojects: 子プロジェクトの追加

config/locales/crowdin/ko.yml

@@ -1748,7 +1748,7 @@ ko:
     text_show_again: 도움말 메뉴에서 이 비디오를 다시 시작할 수 있습니다.
     welcome: OpenProject에 오신 것을 환영합니다.
   permission_add_work_package_notes: 메모 추가
-  permission_add_work_packages: 작업 패키지 추가 (모든 작업 패키지에 파일 첨부 허가)
+  permission_add_work_packages: 작업 패키지 추가
   permission_add_messages: 메시지 게시
   permission_add_project: 프로젝트 만들기
   permission_add_subprojects: 하위 프로젝트 만들기

config/locales/crowdin/lt.yml

@@ -1852,8 +1852,7 @@ lt:
     text_show_again: Jūs galite paleisti šį video iš naujo iš pagalbos meniu
     welcome: Sveiki atvykę į OpenProject
   permission_add_work_package_notes: Pridėti pastabų
-  permission_add_work_packages: Pridėti darbų paketų (taip pat leidžia prisegti priedus
-    prie visų darbų paketų)
+  permission_add_work_packages: Pridėti darbų paketų
   permission_add_messages: Skelbti pranešimus
   permission_add_project: Sukurti projektą
   permission_add_subprojects: Sukurti sub-projektus

config/locales/crowdin/lv.yml

@@ -1812,8 +1812,7 @@ lv:
     text_show_again: You can restart this video from the help menu
     welcome: Welcome to OpenProject
   permission_add_work_package_notes: Add notes
-  permission_add_work_packages: Add work packages (also allows to add attachments
-    to all work packages)
+  permission_add_work_packages: Add work packages
   permission_add_messages: Post messages
   permission_add_project: Create project
   permission_add_subprojects: Apakšprojekta izveide

config/locales/crowdin/nl.yml

@@ -1827,8 +1827,7 @@ nl:
     text_show_again: U kunt deze video van het helpmenu opnieuw starten
     welcome: Welkom bij OpenProject
   permission_add_work_package_notes: Notities toevoegen
-  permission_add_work_packages: Werkpakketten toevoegen (staat ook toe bijlagen aan
-    alle werkpakketten toe te voegen)
+  permission_add_work_packages: Werkpakketten toevoegen
   permission_add_messages: Berichten posten
   permission_add_project: Project Aanmaken
   permission_add_subprojects: Subprojecten maken

config/locales/crowdin/no.yml

@@ -1796,8 +1796,7 @@
     text_show_again: You can restart this video from the help menu
     welcome: Welcome to OpenProject
   permission_add_work_package_notes: Legg til notater
-  permission_add_work_packages: Add work packages (also allows to add attachments
-    to all work packages)
+  permission_add_work_packages: Add work packages
   permission_add_messages: Skriv meldinger
   permission_add_project: Opprett prosjekt
   permission_add_subprojects: Opprett underprosjekt

config/locales/crowdin/pl.yml

@@ -1852,8 +1852,7 @@ pl:
     text_show_again: Możesz zresetować to wideo w menu pomocy
     welcome: Witamy w OpenProject
   permission_add_work_package_notes: Dodawanie notatek
-  permission_add_work_packages: Dodaj Zadania (zezwól także na dodawanie załączników
-    dla wszystkich zestawów Zadań)
+  permission_add_work_packages: Dodaj Zadania
   permission_add_messages: Wysyłanie wiadomości
   permission_add_project: Tworzenie projektu
   permission_add_subprojects: Tworzenie podprojektów

config/locales/crowdin/pt-BR.yml

@@ -1815,8 +1815,7 @@ pt-BR:
     text_show_again: Você pode reiniciar este vídeo através do menu ajuda
     welcome: Bem-vindo ao OpenProject
   permission_add_work_package_notes: Adicionar anotações
-  permission_add_work_packages: Adicionar pacotes de trabalho (também permite adicionar
-    anexos para todos os pacotes de trabalho)
+  permission_add_work_packages: Adicionar pacotes de trabalho
   permission_add_messages: Postar mensagens
   permission_add_project: Criar Projeto
   permission_add_subprojects: Criar subprojetos

config/locales/crowdin/pt.yml

@@ -1830,8 +1830,7 @@ pt:
     text_show_again: Pode reiniciar este vídeo através do menu de ajuda
     welcome: Bem-vindo ao OpenProject
   permission_add_work_package_notes: Adicionar notas
-  permission_add_work_packages: Adicionar pacotes de trabalho (também permite adicionar
-    anexos para todos os pacotes de trabalho)
+  permission_add_work_packages: Adicionar pacotes de trabalho
   permission_add_messages: Publicar mensagens
   permission_add_project: Criar Projeto
   permission_add_subprojects: Criar subprojetos

config/locales/crowdin/ro.yml

@@ -1833,8 +1833,7 @@ ro:
     text_show_again: Poate reporni acest videoclip din meniul de ajutor
     welcome: Welcome to OpenProject
   permission_add_work_package_notes: Adăugare note
-  permission_add_work_packages: Adăugare pachete de lucru (de asemenea, permite adăugarea
-    de atașamente la toate pachetele de lucru)
+  permission_add_work_packages: Adăugare pachete de lucru
   permission_add_messages: Publicare mesaje
   permission_add_project: Creare proiect
   permission_add_subprojects: Creare subproiecte

config/locales/crowdin/ru.yml

@@ -1855,8 +1855,7 @@ ru:
     text_show_again: Вы можете перезапустить это видео из меню «Справка»
     welcome: Добро пожаловать в OpenProject
   permission_add_work_package_notes: Добавить заметки
-  permission_add_work_packages: Добавление пакетов работ (включет добавление к ним
-    вложений)
+  permission_add_work_packages: Добавление пакетов работ
   permission_add_messages: Написать сообщения
   permission_add_project: Создать проект
   permission_add_subprojects: Создать подпроект

config/locales/crowdin/sk.yml

@@ -1858,8 +1858,7 @@ sk:
     text_show_again: Môžete spustiť toto video z ponuky Pomocník
     welcome: Welcome to OpenProject
   permission_add_work_package_notes: Pridať poznámky
-  permission_add_work_packages: Add work packages (also allows to add attachments
-    to all work packages)
+  permission_add_work_packages: Add work packages
   permission_add_messages: Post messages
   permission_add_project: Create project
   permission_add_subprojects: Create subprojects

config/locales/crowdin/sv-SE.yml

@@ -1797,7 +1797,7 @@ sv-SE:
     text_show_again: Du kan starta om denna video från hjälpmenyn
     welcome: Välkommen till OpenProject
   permission_add_work_package_notes: Lägg till anteckningar
-  permission_add_work_packages: Lägga till arbetspaket (även bilagor till arbetspaket)
+  permission_add_work_packages: Lägga till arbetspaket
   permission_add_messages: Publicera meddelanden
   permission_add_project: Skapa projekt
   permission_add_subprojects: Skapa delprojekt

config/locales/crowdin/th.yml

@@ -1771,8 +1771,7 @@ th:
     text_show_again: คุณสามารถเริ่มเล่นวิดีโอนี้ใหม่ได้จากเมนูวิธีใช้
     welcome: Welcome to OpenProject
   permission_add_work_package_notes: เพิ่มหมายเหตุ
-  permission_add_work_packages: Add work packages (also allows to add attachments
-    to all work packages)
+  permission_add_work_packages: Add work packages
   permission_add_messages: โพสต์ข้อความ
   permission_add_project: สร้างโครงการ
   permission_add_subprojects: สร้างโครงการย่อย

config/locales/crowdin/tr.yml

@@ -1800,8 +1800,7 @@ tr:
     text_show_again: Bu videoyu Yardım menüsünden yeniden başlatabilirsiniz
     welcome: OpenProject'e hoş geldiniz
   permission_add_work_package_notes: Not eklemek
-  permission_add_work_packages: İş paketleri ekleyin (ayrıca tüm iş paketlerine ek
-    eklemenizi sağlar)
+  permission_add_work_packages: İş paketleri ekleyin
   permission_add_messages: İleti göndermek
   permission_add_project: Proje oluşturmak
   permission_add_subprojects: Alt proje oluşturmak

config/locales/crowdin/uk.yml

@@ -1847,8 +1847,7 @@ uk:
     text_show_again: You can restart this video from the help menu
     welcome: Ласкаво просимо до OpenProject
   permission_add_work_package_notes: Add notes
-  permission_add_work_packages: Add work packages (also allows to add attachments
-    to all work packages)
+  permission_add_work_packages: Add work packages
   permission_add_messages: Post messages
   permission_add_project: Create project
   permission_add_subprojects: Create subprojects

config/locales/crowdin/vi.yml

@@ -1802,8 +1802,7 @@ vi:
     text_show_again: Bạn có thể khởi động lại video này từ trình đơn trợ giúp
     welcome: Welcome to OpenProject
   permission_add_work_package_notes: Add notes
-  permission_add_work_packages: Add work packages (also allows to add attachments
-    to all work packages)
+  permission_add_work_packages: Add work packages
   permission_add_messages: Đăng tin nhắn
   permission_add_project: Tạo Dự án
   permission_add_subprojects: Tạo Dự án con

config/locales/crowdin/zh-TW.yml

@@ -1685,7 +1685,7 @@ zh-TW:
     text_show_again: 您可以從 "説明" 功能表中重新開啟此影片
     welcome: 歡迎來到 OpenProject
   permission_add_work_package_notes: 增加註釋
-  permission_add_work_packages: 添加工作包 (也允許將附件添加到所有的工作包)
+  permission_add_work_packages: 添加工作包
   permission_add_messages: 張貼訊息
   permission_add_project: 建立專案
   permission_add_subprojects: 建立子專案

config/locales/crowdin/zh.yml

@@ -1683,7 +1683,7 @@ zh:
     text_show_again: 您可以从帮助菜单重新启动此视频
     welcome: 欢迎来到 OpenProject
   permission_add_work_package_notes: 添加注释
-  permission_add_work_packages: 添加工作包 (也允许将附件添加到所有的工作包)
+  permission_add_work_packages: 添加工作包
   permission_add_messages: 发布消息
   permission_add_project: 创建项目
   permission_add_subprojects: 创建子项目

config/locales/en.yml

@@ -462,6 +462,7 @@ en:
         before_or_equal_to: "must be before or equal to %{date}."
         could_not_be_copied: "could not be (fully) copied."
         regex_invalid: "could not be validated with the associated regular expression."
+        unchangeable: "cannot be changed."
       models:
         custom_field:
           at_least_one_custom_option: "At least one option needs to be available."
@@ -1734,7 +1735,7 @@ en:
     welcome: "Welcome to OpenProject"
 
   permission_add_work_package_notes: "Add notes"
-  permission_add_work_packages: "Add work packages (also allows to add attachments to all work packages)"
+  permission_add_work_packages: "Add work packages"
   permission_add_messages: "Post messages"
   permission_add_project: "Create project"
   permission_add_subprojects: "Create subprojects"

db/migrate/20180524084654_remove_non_null_container_on_attachments.rb

@@ -0,0 +1,52 @@
+#-- encoding: UTF-8
+
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2018 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-2017 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 docs/COPYRIGHT.rdoc for more details.
+#++
+
+class RemoveNonNullContainerOnAttachments < ActiveRecord::Migration[5.1]
+  def change
+    change_column_null :attachments, :container_id, true
+    change_column_null :attachments, :container_type, true
+
+    change_column_default :attachments, :container_id, from: 0, to: nil
+    change_column_default :attachments, :container_type, from: '', to: nil
+
+    change_column_null :attachment_journals, :container_id, true
+    change_column_null :attachment_journals, :container_type, true
+
+    change_column_default :attachment_journals, :container_id, from: 0, to: nil
+    change_column_default :attachment_journals, :container_type, from: '', to: nil
+
+    add_column :attachments, :updated_at, :datetime
+    rename_column :attachments, :created_on, :created_at
+
+    reversible do |change|
+      change.up { Attachment.update_all("updated_at = created_at") }
+    end
+  end
+end

docs/api/apiv3/endpoints/work-packages.apib

@@ -89,7 +89,7 @@ the human readable name of custom fields.*
                         "href": "/api/v3/work_packages/1528",
                         "title": "Develop API"
                     },
-                    "schema": { "href": "/api/v3/work_packages/schemas/1-1-2" },
+                    "schema": { "href": "/api/v3/work_packages/schemas/11-2" },
                     "update": {
                         "href": "/api/v3/work_packages/1528",
                         "method": "patch",
@@ -100,7 +100,7 @@ the human readable name of custom fields.*
                         "method": "delete",
                         "title": "Delete Develop API"
                     },
-                    "log_time": {
+                    "logTime": {
                         "href": "/work_packages/1528/time_entries/new",
                         "type": "text/html",
                         "title": "Log time on Develop API"
@@ -132,7 +132,7 @@ the human readable name of custom fields.*
                             "method": "post",
                             "title": "Forward to accounting"
                         }
-                    ]
+                    ],
                     "responsible": {
                         "href": "/api/v3/users/23",
                         "title": "Laron Leuschke - Alaina5788"
@@ -162,7 +162,7 @@ the human readable name of custom fields.*
                     },
                     "type": {
                         "href": "/api/v3/types/1",
-                        "title": "New"
+                        "title": "A Type"
                     },
                     "version": {
                         "href": "/api/v3/versions/1",

frontend/app/components/common/path-helper/apiv3/apiv3-paths.ts

@@ -41,6 +41,9 @@ export class ApiV3Paths {
   // Base path
   public readonly apiV3Base  = this.appBasePath + '/api/v3';
 
+  // /api/v3/attachments
+  public readonly attachments = new SimpleResource(this.apiV3Base, 'attachments');
+
   // /api/v3/configuration
   public readonly configuration = new SimpleResource(this.apiV3Base, 'configuration');
 

frontend/app/components/common/path-helper/path-helper.service.ts

@@ -45,6 +45,16 @@ export class PathHelperService {
     return this.appBasePath;
   }
 
+  public attachmentDownloadPath(attachmentIdentifier:string, slug:string|undefined) {
+    let path = this.staticBase + '/attachments/' + attachmentIdentifier;
+
+    if (slug) {
+      return path + "/" + slug;
+    } else {
+      return path;
+    }
+  }
+
   public boardPath(projectIdentifier:string, boardIdentifier:string) {
     return this.projectBoardsPath(projectIdentifier) + '/' + boardIdentifier;
   }

frontend/app/components/work-packages/wp-attachments-formattable-field/wp-attachments-formattable.directive.ts

@@ -110,9 +110,6 @@ export class WpAttachmentsFormattableController {
 
   protected uploadAndInsert(files:UploadFile[], model:EditorModel | WorkPackageFieldModel) {
     const wp = this.$scope.workPackage as WorkPackageResource;
-    if (wp.isNew) {
-      return this.insertDelayedAttachments(files, model, wp);
-    }
 
     wp
       .uploadAttachments(files)
@@ -160,19 +157,6 @@ export class WpAttachmentsFormattableController {
     }
   }
 
-  protected insertDelayedAttachments(files:UploadFile[], description:any, workPackage:WorkPackageResource):void {
-    for (var i = 0; i < files.length; i++) {
-      var currentFile = new SingleAttachmentModel(files[i]);
-      var insertMode = currentFile.isAnImage ? InsertMode.INLINE : InsertMode.ATTACHMENT;
-      const name = files[i].customName || files[i].name;
-
-      description.insertAttachmentLink(name.replace(/ /g, '_'), insertMode, true);
-      workPackage.pendingAttachments.push((files[i]));
-    }
-
-    description.save();
-  }
-
   protected insertUrls(dropData:DropModel, description:any):void {
     const insertUrl:string = dropData.isAttachmentOfCurrentWp() ? dropData.removeHostInformationFromUrl() : dropData.webLinkUrl;
     const insertAlternative:InsertMode = dropData.isWebImage() ? InsertMode.INLINE : InsertMode.LINK;

frontend/app/components/wp-attachments/wp-attachment-list/wp-attachment-list-item.component.ts

@@ -31,6 +31,7 @@ import {WorkPackageNotificationService} from '../../wp-edit/wp-notification.serv
 import {Component, Inject, Input} from '@angular/core';
 import {I18nToken} from 'core-app/angular4-transition-utils';
 import {HalResource} from 'core-app/modules/hal/resources/hal-resource';
+import {PathHelperService} from "core-components/common/path-helper/path-helper.service";
 
 @Component({
   template: require('!!raw-loader!./wp-attachment-list-item.html'),
@@ -43,12 +44,17 @@ export class WorkPackageAttachmentListItemComponent {
   public text = {
     destroyConfirmation: this.I18n.t('js.text_attachment_destroy_confirmation'),
     removeFile: (arg:any) => this.I18n.t('js.label_remove_file', arg)
-  }
+  };
 
   constructor(protected wpNotificationsService:WorkPackageNotificationService,
+              readonly pathHelper:PathHelperService,
               @Inject(I18nToken) readonly I18n:op.I18n) {
   }
 
+  public get downloadPath() {
+    return this.pathHelper.attachmentDownloadPath(this.attachment.id, this.attachment.name);
+  }
+
   public confirmRemoveAttachment($event:JQueryEventObject) {
     if (!window.confirm(this.text.destroyConfirmation)) {
       $event.stopImmediatePropagation();

frontend/app/components/wp-attachments/wp-attachment-list/wp-attachment-list-item.html

@@ -3,21 +3,20 @@
     <op-icon icon-classes="icon-context icon-attachment"></op-icon>
     <a
         class="work-package--attachments--filename"
-        [attr.href]="attachment.downloadLocation.href || '#'"
+        [attr.href]="downloadPath || '#'"
         download>
 
       {{ attachment.fileName || attachment.customName || attachment.name }}
       <authoring class="work-package--attachments--info"
                  [createdOn]="attachment.createdAt"
                  [author]="attachment.author"
-                 [showAuthorAsLink]="false"
-                 *ngIf="!workPackage.isNew"></authoring>
+                 [showAuthorAsLink]="false"></authoring>
     </a>
   </span>
   <a
       href=""
       class="form--selected-value--remover work-package--atachments--delete-button"
-      *ngIf="!!attachment.$links.delete || workPackage.isNew"
+      *ngIf="!!attachment.$links.delete"
       (click)="confirmRemoveAttachment($event)">
     <op-icon icon-classes="icon-delete"
              [icon-title]="text.removeFile({fileName: attachment.fileName})"></op-icon>

frontend/app/components/wp-attachments/wp-attachment-list/wp-attachment-list.html

@@ -1,8 +1,4 @@
 <div class="work-package--attachments--files">
-  <ul class="form--selected-value--list"
-      *ngFor="let attachment of workPackage.pendingAttachments">
-    <wp-attachment-list-item [attachment]="attachment" [workPackage]="workPackage"></wp-attachment-list-item>
-  </ul>
   <ul class="form--selected-value--list"
       *ngFor="let attachment of workPackage.attachments.elements">
     <wp-attachment-list-item [attachment]="attachment" [workPackage]="workPackage"></wp-attachment-list-item>

frontend/app/components/wp-attachments/wp-attachments-upload/wp-attachments-upload.directive.ts

@@ -32,9 +32,9 @@ import {UploadFile} from '../../api/op-file-upload/op-file-upload.service';
 import IDirective = angular.IDirective;
 
 export class WorkPackageUploadDirectiveController {
-  public workPackage: WorkPackageResource;
-  public text: any;
-  public maxFileSize: number;
+  public workPackage:WorkPackageResource;
+  public text:any;
+  public maxFileSize:number;
 
   constructor(protected $q:ng.IQService, ConfigurationService:any, protected I18n:op.I18n) {
     this.text = {
@@ -47,16 +47,11 @@ export class WorkPackageUploadDirectiveController {
     });
   }
 
-  public uploadFiles(files: UploadFile[]):void {
+  public uploadFiles(files:UploadFile[]):void {
     if (files === undefined || files.length === 0) {
       return;
     }
 
-    if (this.workPackage.isNew) {
-      this.workPackage.pendingAttachments.push(...files);
-      return;
-    }
-
     this.workPackage.uploadAttachments(files);
   }
 }

frontend/app/components/wp-edit-form/work-package-changeset.ts

@@ -38,6 +38,7 @@ import {WorkPackageResource} from 'core-app/modules/hal/resources/work-package-r
 import {FormResource} from 'core-app/modules/hal/resources/form-resource';
 import {HalResourceService} from 'core-app/modules/hal/services/hal-resource.service';
 import {WorkPackagesActivityService} from 'core-components/wp-single-view-tabs/activity-panel/wp-activity.service';
+import {HalResource} from 'core-app/modules/hal/resources/hal-resource';
 
 export class WorkPackageChangeset {
   // Injections
@@ -185,7 +186,6 @@ export class WorkPackageChangeset {
 
                 if (wasNew) {
                   this.workPackage.overriddenSchema = undefined;
-                  this.workPackage.uploadAttachmentsAndReload();
                   this.wpCreate.newWorkPackageCreated(this.workPackage);
                 }
 
@@ -258,6 +258,11 @@ export class WorkPackageChangeset {
       } else {
         payload = this.workPackage.$source;
       }
+
+      // Add attachments to be assigned.
+      // They will already be created on the server but now
+      // we need to claim them for the newly created work package.
+      payload['_links']['attachments'] = this.workPackage.attachments.elements.map((a:HalResource) => { return { href: a.href }; });
     } else {
       // Otherwise, simply use the bare minimum, which is the lock version.
       payload = this.minimalPayload;

frontend/app/components/wp-edit-form/work-package-edit-form.ts

@@ -179,13 +179,13 @@ export class WorkPackageEditForm {
    * @return {any}
    */
   public submit():Promise<WorkPackageResource> {
-    if (this.changeset.empty && !this.workPackage.isNew) {
+    const isInitial = this.workPackage.isNew;
+
+    if (this.changeset.empty && !isInitial) {
       this.closeEditFields();
       return Promise.resolve(this.workPackage);
     }
 
-    const isInitial = this.workPackage.isNew;
-
     // Reset old error notifcations
     this.errorsPerAttribute = {};
 

frontend/app/modules/hal/resources/attachment-collection-resource.ts

@@ -30,6 +30,11 @@ import {HalResource} from 'core-app/modules/hal/resources/hal-resource';
 import {CollectionResource} from 'core-app/modules/hal/resources/collection-resource';
 
 export class AttachmentCollectionResource extends CollectionResource {
+  public $initialize(source:any) {
+    super.$initialize(source);
+
+    this.elements = this.elements || [];
+  }
 
 }
 

frontend/app/modules/hal/resources/work-package-resource.ng2.test.ts

@@ -158,36 +158,6 @@ describe('WorkPackage', () => {
     });
   });
 
-  describe('when using uploadPendingAttachments', () => {
-    let uploadAttachmentsStub:sinon.SinonStub;
-
-    beforeEach(() => {
-      workPackage.pendingAttachments.push({} as any, {} as any);
-      uploadAttachmentsStub = sinon
-        .stub(workPackage, 'uploadAttachments')
-        .returns(Promise.resolve());
-    });
-
-    beforeEach(() => {
-      workPackage.$source.id = 1234;
-      workPackage.uploadPendingAttachments();
-    });
-
-    afterEach(() => {
-      uploadAttachmentsStub.restore();
-    })
-
-    it('should call the uploadAttachments method with the pendingAttachments', () => {
-      expect(uploadAttachmentsStub.calledWith([{}, {}])).to.be.true;
-    });
-
-    describe('when the upload succeeds', () => {
-      it('should reset the pending attachments', () => {
-        expect(workPackage.pendingAttachments).to.have.length(0);
-      });
-    });
-  });
-
   describe('when using removeAttachment', () => {
     let file:any;
     let attachment:any;
@@ -201,14 +171,6 @@ describe('WorkPackage', () => {
 
       createWorkPackage();
       workPackage.attachments.elements = [attachment];
-      workPackage.pendingAttachments.push(file);
-    });
-
-    describe('when the attachment is a regular file', () => {
-      it('should be removed from the pending attachments', () => {
-        workPackage.removeAttachment(file);
-        expect(workPackage.pendingAttachments).to.have.length(0);
-      });
     });
 
     describe('when the attachment is an attachment resource', () => {

frontend/app/modules/hal/resources/work-package-resource.ts

@@ -117,7 +117,6 @@ export class WorkPackageResource extends HalResource {
   public activities:CollectionResource;
   public attachments:AttachmentCollectionResource;
 
-  public pendingAttachments:UploadFile[] = [];
   public overriddenSchema?:SchemaResource;
 
   readonly I18n:op.I18n = this.injector.get(I18nToken);
@@ -190,7 +189,6 @@ export class WorkPackageResource extends HalResource {
    */
   public removeAttachment(attachment:any):Promise<any> {
     _.pull(this.attachments.elements, attachment);
-    _.pull(this.pendingAttachments, attachment);
 
     if (attachment.$isHal) {
       return attachment.delete()
@@ -205,20 +203,6 @@ export class WorkPackageResource extends HalResource {
     return Promise.resolve();
   }
 
-  /**
-   * Upload the pending attachments if the work package exists.
-   * Do nothing, if the work package is being created.
-   */
-  public uploadPendingAttachments():Promise<any>|void {
-    if (!this.pendingAttachments.length) {
-      return undefined;
-    }
-
-    const attachments = this.pendingAttachments;
-    this.pendingAttachments = [];
-    return this.uploadAttachments(attachments);
-  }
-
   /**
    * Upload the given attachments, update the resource and notify the user.
    * Return an updated AttachmentCollectionResource.
@@ -232,7 +216,20 @@ export class WorkPackageResource extends HalResource {
     return finished
       .then((result:any[]) => {
         setTimeout(() => this.NotificationsService.remove(notification), 700);
-        this.updateAttachments();
+        if (!this.isNew) {
+          this.updateAttachments();
+        } else {
+          result.forEach(r => {
+            let attachment = new HalResource(this.injector,
+                                             r.data,
+                                             false,
+                                             this.halInitializer,
+                                             'HalResource');
+
+
+            this.attachments.elements.push(attachment);
+          });
+        }
         return result.map(el => { return { response: el.data, uploadUrl: el.data._links.downloadLocation.href }; });
       })
       .catch((error:any) => {
@@ -241,29 +238,20 @@ export class WorkPackageResource extends HalResource {
   }
 
   private performUpload(files:UploadFile[]) {
-    const href = this.attachments.$href!;
+    let href = '';
+
+    if (this.isNew) {
+      href = this.pathHelper.api.v3.attachments.path;
+    } else {
+      href = this.attachments.$href!;
+    }
+
     // TODO upgrade
     const opFileUpload:OpenProjectFileUploadService = angular.element('body').injector().get('opFileUpload');
 
     return opFileUpload.upload(href, files);
   }
 
-  /**
-   * Uploads the attachments and reloads the work package when done
-   * Reloading is skipped if no attachment is added
-   */
-  public uploadAttachmentsAndReload() {
-    const attachmentUpload = this.uploadPendingAttachments();
-
-    if (attachmentUpload) {
-      attachmentUpload.then((attachmentsResource) => {
-        if (attachmentsResource.count > 0) {
-          this.wpCacheService.loadWorkPackage(this.id, true);
-        }
-      });
-    }
-  }
-
   public getSchemaName(name:string):string {
     if (this.isMilestone && (name === 'startDate' || name === 'dueDate')) {
       return 'date';
@@ -332,7 +320,7 @@ export class WorkPackageResource extends HalResource {
   public $initialize(source:any) {
     super.$initialize(source);
 
-    let attachments = this.attachments || { $source: {} };
+    let attachments = this.attachments || { $source: {}, elements: [] };
     this.attachments = new AttachmentCollectionResource(
       this.injector,
       attachments,

lib/api/utilities/property_name_converter.rb

@@ -60,7 +60,8 @@ module API
           mail: 'email',
           column_names: 'columns',
           is_public: 'public',
-          sort_criteria: 'sortBy'
+          sort_criteria: 'sortBy',
+          message: 'post'
         }.freeze
 
         # Converts the attribute name as refered to by ActiveRecord to a corresponding API-conform

lib/api/v3/attachments/attachment_representer.rb

@@ -44,39 +44,24 @@ module API
                             v3_path: :user,
                             representer: ::API::V3::Users::UserRepresenter
 
+        cached_representer key_parts: %i[author container]
+
         def self.associated_container_getter
           ->(*) do
             next unless embed_links
 
-            representer = case represented.container
-                          when WorkPackage
-                            ::API::V3::WorkPackages::WorkPackageRepresenter
-                          when WikiPage
-                            ::API::V3::WikiPages::WikiPageRepresenter
-                          when Message
-                            ::API::V3::Posts::PostRepresenter
-                          end
-
-            representer.new(represented.container, current_user: current_user)
+            container_representer
+              .new(represented.container, current_user: current_user)
           end
         end
 
         def self.associated_container_link
           ->(*) do
-            path, title_attribute = case represented.container
-                                    when WorkPackage
-                                      %i[work_package subject]
-                                    when WikiPage
-                                      %i[wiki_page title]
-                                    when Message
-                                      %i[post subject]
-                                    end
-
             ::API::Decorators::LinkObject
               .new(represented,
-                   path: path,
+                   path: v3_container_name,
                    property_name: :container,
-                   title_attribute: title_attribute)
+                   title_attribute: container_title_attribute)
               .to_hash
           end
         end
@@ -96,7 +81,6 @@ module API
           }
         end
 
-        # visibility of this link is also work_package specific!
         link :delete,
              cache_if: -> { represented.deletable?(current_user) } do
           {
@@ -121,14 +105,27 @@ module API
                    ::API::Decorators::Digest.new(digest, algorithm: 'md5')
                  },
                  render_nil: true
-        property :created_on,
-                 as: 'createdAt',
+        property :created_at,
                  exec_context: :decorator,
-                 getter: ->(*) { datetime_formatter.format_datetime(represented.created_on) }
+                 getter: ->(*) { datetime_formatter.format_datetime(represented.created_at) }
 
         def _type
           'Attachment'
         end
+
+        def container_representer
+          name = v3_container_name.camelcase
+
+          "::API::V3::#{name.pluralize}::#{name}Representer".constantize
+        end
+
+        def v3_container_name
+          ::API::Utilities::PropertyNameConverter.from_ar_name(represented.container.class.name.underscore).underscore
+        end
+
+        def container_title_attribute
+          represented.container.respond_to?(:subject) ? :subject : :title
+        end
       end
     end
   end

lib/api/v3/attachments/attachments_api.rb

@@ -42,8 +42,7 @@ module API
           end
 
           post do
-            # TODO: get real permissions preferably via acts_as_attachable
-            authorize_any %i[add_work_packages edit_wiki_pages], global: true
+            raise API::Errors::Unauthorized if Redmine::Acts::Attachable.attachables.none?(&:attachments_addable?)
 
             ::API::V3::Attachments::AttachmentRepresenter.new(parse_and_create,
                                                               current_user: current_user)
@@ -66,7 +65,12 @@ module API
             delete do
               raise API::Errors::Unauthorized unless @attachment.deletable?(current_user)
 
-              @attachment.container.attachments.delete(@attachment)
+              if @attachment.container
+                @attachment.container.attachments.delete(@attachment)
+              else
+                @attachment.destroy
+              end
+
               status 204
             end
 

lib/api/v3/attachments/attachments_by_container_api.rb

@@ -106,9 +106,13 @@ module API
           end
         end
 
-        def self.create(permissions)
+        def self.create(permissions = [])
           -> do
-            authorize_any permissions, projects: container.project
+            if permissions.empty?
+              raise API::Errors::Unauthorized unless container.attachments_addable?(current_user)
+            else
+              authorize_any(permissions, projects: container.project)
+            end
 
             ::API::V3::Attachments::AttachmentRepresenter.new(parse_and_create,
                                                               current_user: current_user)

lib/api/v3/attachments/attachments_by_post_api.rb

@@ -44,7 +44,7 @@ module API
           end
 
           get &API::V3::Attachments::AttachmentsByContainerAPI.read
-          post &API::V3::Attachments::AttachmentsByContainerAPI.create(%i[edit_messages add_messages])
+          post &API::V3::Attachments::AttachmentsByContainerAPI.create([:edit_messages])
         end
       end
     end

lib/api/v3/attachments/attachments_by_wiki_page_api.rb

@@ -44,7 +44,7 @@ module API
           end
 
           get &API::V3::Attachments::AttachmentsByContainerAPI.read
-          post &API::V3::Attachments::AttachmentsByContainerAPI.create([:edit_wiki_pages])
+          post &API::V3::Attachments::AttachmentsByContainerAPI.create
         end
       end
     end

lib/api/v3/attachments/attachments_by_work_package_api.rb

@@ -46,7 +46,14 @@ module API
           end
 
           get &API::V3::Attachments::AttachmentsByContainerAPI.read
-          post &API::V3::Attachments::AttachmentsByContainerAPI.create(%i[edit_work_packages add_work_packages])
+
+          # while attachments are #addable? when the user has the :add_work_packages permission or
+          # the :edit_work_packages permission, we cannot differentiate here between adding to a newly
+          # created work package (for which :add_work_package would be required) and adding to an older
+          # work package (for which :edit_work_packages would be required). We thus only allow
+          # :edit_work_packages in this endpoint and require clients to upload uncontainered work packages
+          # first and attach them on wp creation.
+          post &API::V3::Attachments::AttachmentsByContainerAPI.create([:edit_work_packages])
         end
       end
     end

lib/api/v3/posts/post_representer.rb

@@ -41,8 +41,7 @@ module API
         end
 
         link :addAttachment do
-          next unless current_user_allowed_to(:edit_messages, context: represented.project) ||
-                      current_user_allowed_to(:add_messages, context: represented.project)
+          next unless current_user_allowed_to(:edit_messages, context: represented.project)
 
           {
             href: api_v3_paths.attachments_by_post(represented.id),

lib/api/v3/utilities/custom_field_injector.rb

@@ -292,7 +292,7 @@ module API
                     else
                       fragment
                     end
-            self.custom_field_values = { custom_field.id => value }
+            send(:"custom_field_#{custom_field.id}=", value)
           }
         end
 

lib/api/v3/work_packages/create_form_api.rb

@@ -26,8 +26,7 @@
 # See docs/COPYRIGHT.rdoc for more details.
 #++
 
-require 'api/v3/work_packages/work_packages_shared_helpers'
-require 'create_work_package_service'
+require 'api/v3/work_packages/form_helper'
 require 'work_packages/create_contract'
 
 module API
@@ -35,17 +34,13 @@ module API
     module WorkPackages
       class CreateFormAPI < ::API::OpenProjectAPI
         resource :form do
-          helpers ::API::V3::WorkPackages::WorkPackagesSharedHelpers
+          helpers ::API::V3::WorkPackages::FormHelper
 
           post do
-            work_package = merge_hash_into_work_package!(request_body, WorkPackage.new)
-            work_package = WorkPackage.new(author: current_user,
-                                           project: work_package.project)
-
-            create_work_package_form(work_package,
-                                     contract_class: ::WorkPackages::CreateContract,
-                                     form_class: CreateFormRepresenter,
-                                     action: :create)
+            respond_with_work_package_form(WorkPackage.new(author: current_user),
+                                           contract_class: ::WorkPackages::CreateContract,
+                                           form_class: CreateFormRepresenter,
+                                           action: :create)
           end
         end
       end

lib/api/v3/work_packages/create_project_form_api.rb

@@ -26,7 +26,7 @@
 # See docs/COPYRIGHT.rdoc for more details.
 #++
 
-require 'api/v3/work_packages/work_packages_shared_helpers'
+require 'api/v3/work_packages/form_helper'
 require 'create_work_package_service'
 require 'work_packages/create_contract'
 
@@ -35,14 +35,14 @@ module API
     module WorkPackages
       class CreateProjectFormAPI < ::API::OpenProjectAPI
         resource :form do
-          helpers ::API::V3::WorkPackages::WorkPackagesSharedHelpers
+          helpers ::API::V3::WorkPackages::FormHelper
 
           post do
             work_package = WorkPackage.new(project: @project)
-            create_work_package_form(work_package,
-                                     contract_class: ::WorkPackages::CreateContract,
-                                     form_class: CreateProjectFormRepresenter,
-                                     action: :create)
+            respond_with_work_package_form(work_package,
+                                           contract_class: ::WorkPackages::CreateContract,
+                                           form_class: CreateProjectFormRepresenter,
+                                           action: :create)
           end
         end
       end

lib/api/v3/work_packages/create_work_packages.rb

@@ -39,10 +39,13 @@ module API
           work_package = WorkPackage.new
           yield(work_package) if block_given?
 
-          work_package = write_work_package_attributes(work_package, request_body || {})
+          parameters = ::API::V3::WorkPackages::ParseParamsService
+                       .new(current_user)
+                       .call(request_body)
 
           result = create_work_package(current_user,
                                        work_package,
+                                       parameters,
                                        notify_according_to_params)
 
           represent_create_result(result, current_user)
@@ -62,10 +65,11 @@ module API
           end
         end
 
-        def create_work_package(current_user, work_package, send_notification)
+        def create_work_package(current_user, work_package, attributes, send_notification)
           create_service = ::WorkPackages::CreateService.new(user: current_user)
 
-          create_service.call(work_package: work_package,
+          create_service.call(attributes: attributes,
+                              work_package: work_package,
                               send_notifications: send_notification)
         end
       end

lib/api/v3/work_packages/form_helper.rb

@@ -0,0 +1,72 @@
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2018 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-2017 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 docs/COPYRIGHT.rdoc for more details.
+#++
+
+require 'api/v3/work_packages/work_package_payload_representer'
+
+module API
+  module V3
+    module WorkPackages
+      module FormHelper
+        extend Grape::API::Helpers
+
+        def respond_with_work_package_form(work_package, contract_class:, form_class:, action: :update)
+          parameters = parse_body
+
+          result = ::WorkPackages::SetAttributesService
+                   .new(user: current_user, work_package: work_package, contract: contract_class)
+                   .call(parameters)
+
+          api_errors = ::API::Errors::ErrorBase.create_errors(result.errors)
+
+          # errors for invalid data (e.g. validation errors) are handled inside the form
+          if only_validation_errors(api_errors)
+            status 200
+            form_class.new(work_package,
+                           current_user: current_user,
+                           errors: api_errors,
+                           action: action)
+          else
+            fail ::API::Errors::MultipleErrors.create_if_many(api_errors)
+          end
+        end
+
+        private
+
+        def only_validation_errors(errors)
+          errors.all? { |error| error.code == 422 }
+        end
+
+        def parse_body
+          ::API::V3::WorkPackages::ParseParamsService
+            .new(current_user)
+            .call(request_body)
+        end
+      end
+    end
+  end
+end

lib/api/v3/work_packages/parse_params_service.rb

@@ -0,0 +1,66 @@
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2018 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-2017 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 docs/COPYRIGHT.rdoc for more details.
+#++
+
+module API
+  module V3
+    module WorkPackages
+      class ParseParamsService
+        def initialize(user)
+          @current_user = user
+        end
+
+        def call(request_body)
+          return {} unless request_body
+
+          parse_attributes(request_body)
+        end
+
+        private
+
+        attr_accessor :current_user
+
+        def parse_attributes(request_body)
+          struct = ParsingStruct.new
+
+          ::API::V3::WorkPackages::WorkPackagePayloadRepresenter
+            .create_class(struct)
+            .new(struct, current_user: current_user)
+            .from_hash(Hash(request_body))
+            .to_h
+            .reverse_merge(lock_version: nil)
+        end
+
+        class ParsingStruct < OpenStruct
+          def available_custom_fields
+            @available_custom_fields ||= WorkPackageCustomField.all.to_a
+          end
+        end
+      end
+    end
+  end
+end

lib/api/v3/work_packages/update_form_api.rb

@@ -26,19 +26,19 @@
 # See docs/COPYRIGHT.rdoc for more details.
 #++
 
-require 'api/v3/work_packages/work_packages_shared_helpers'
+require 'api/v3/work_packages/form_helper'
 
 module API
   module V3
     module WorkPackages
       class UpdateFormAPI < ::API::OpenProjectAPI
         resource :form do
-          helpers ::API::V3::WorkPackages::WorkPackagesSharedHelpers
+          helpers ::API::V3::WorkPackages::FormHelper
 
           post do
-            create_work_package_form(@work_package,
-                                     contract_class: ::WorkPackages::UpdateContract,
-                                     form_class: UpdateFormRepresenter)
+            respond_with_work_package_form(@work_package,
+                                           contract_class: ::WorkPackages::UpdateContract,
+                                           form_class: UpdateFormRepresenter)
           end
         end
       end

lib/api/v3/work_packages/work_package_payload_representer.rb

@@ -37,7 +37,30 @@ module API
         cached_representer disabled: true
 
         def writeable_attributes
-          super + ["date"]
+          super + %w[date attachments]
+        end
+
+        property :attachments,
+                 exec_context: :decorator,
+                 getter: ->(*) {},
+                 setter: ->(fragment:, **) do
+                   ids = fragment.map do |link|
+                     ::API::Utilities::ResourceLinkParser.parse_id link['href'],
+                                                                   property: :attachment,
+                                                                   expected_version: '3',
+                                                                   expected_namespace: :attachments
+                   end
+
+                   represented.attachment_ids = ids
+                 end,
+                 skip_render: ->(*) { true },
+                 linked_resource: true,
+                 uncacheable: true
+
+        links :attachments do
+          represented.attachments.map do |attachment|
+            { href: api_v3_paths.attachment(attachment.id) }
+          end
         end
 
         def load_complete_model(model)

lib/api/v3/work_packages/work_package_representer.rb

@@ -176,8 +176,7 @@ module API
 
         link :addAttachment,
              cache_if: -> do
-               current_user_allowed_to(:edit_work_packages, context: represented.project) ||
-                 current_user_allowed_to(:add_work_packages, context: represented.project)
+               current_user_allowed_to(:edit_work_packages, context: represented.project)
              end do
           {
             href: api_v3_paths.attachments_by_work_package(represented.id),
@@ -355,8 +354,8 @@ module API
                    datetime_formatter.format_date(represented.start_date, allow_nil: true)
                  end,
                  render_nil: true,
-                 if: ->(_) {
-                   !represented.milestone?
+                 skip_render: ->(_) {
+                   represented.milestone?
                  }
 
         property :due_date,
@@ -365,8 +364,8 @@ module API
                    datetime_formatter.format_date(represented.due_date, allow_nil: true)
                  end,
                  render_nil: true,
-                 if: ->(_) {
-                   !represented.milestone?
+                 skip_render: ->(_) {
+                   represented.milestone?
                  }
 
         property :date,
@@ -375,8 +374,8 @@ module API
                    datetime_formatter.format_date(represented.due_date, allow_nil: true)
                  end,
                  render_nil: true,
-                 if: ->(*) {
-                   represented.milestone?
+                 skip_render: ->(*) {
+                   !represented.milestone?
                  }
 
         property :estimated_time,

lib/api/v3/work_packages/work_packages_api.rb

@@ -86,14 +86,16 @@ module API
             end
 
             patch do
-              write_work_package_attributes(@work_package, request_body, reset_lock_version: true)
+              parameters = ::API::V3::WorkPackages::ParseParamsService
+                           .new(current_user)
+                           .call(request_body)
 
               call = ::WorkPackages::UpdateService
                      .new(
                        user: current_user,
                        work_package: @work_package
                      )
-                     .call(send_notifications: notify_according_to_params)
+                     .call(attributes: parameters, send_notifications: notify_according_to_params)
 
               if call.success?
                 @work_package.reload

lib/api/v3/work_packages/work_packages_shared_helpers.rb

@@ -35,50 +35,6 @@ module API
       module WorkPackagesSharedHelpers
         extend Grape::API::Helpers
 
-        def merge_hash_into_work_package!(hash, work_package)
-          payload = ::API::V3::WorkPackages::WorkPackagePayloadRepresenter.create(work_package, current_user: current_user)
-          payload.from_hash(Hash(hash))
-        end
-
-        def write_work_package_attributes(work_package, request_body, reset_lock_version: false)
-          if request_body
-            work_package.lock_version = nil if reset_lock_version
-            # we need to merge the JSON two times:
-            # In Pass 1 the representer only has custom fields for the current WP type/project
-            # After Pass 1 the correct type/project information is merged into the WP
-            # In Pass 2 the representer is created with the new type/project info and will be able
-            # to also parse custom fields successfully
-            work_package = merge_hash_into_work_package!(request_body, work_package)
-
-            if custom_field_context_changed?(work_package)
-              work_package = merge_hash_into_work_package!(request_body, work_package)
-            end
-
-            work_package
-          end
-        end
-
-        def create_work_package_form(work_package, contract_class:, form_class:, action: :update)
-          write_work_package_attributes(work_package, request_body, reset_lock_version: true)
-
-          result = ::WorkPackages::SetAttributesService
-                   .new(user: current_user, work_package: work_package, contract: contract_class)
-                   .call({})
-
-          api_errors = ::API::Errors::ErrorBase.create_errors(result.errors)
-
-          # errors for invalid data (e.g. validation errors) are handled inside the form
-          if only_validation_errors(api_errors)
-            status 200
-            form_class.new(work_package,
-                           current_user: current_user,
-                           errors: api_errors,
-                           action: action)
-          else
-            fail ::API::Errors::MultipleErrors.create_if_many(api_errors)
-          end
-        end
-
         def work_package_representer(work_package = @work_package)
           ::API::V3::WorkPackages::WorkPackageRepresenter.create(
             work_package,
@@ -119,15 +75,6 @@ module API
           errors
         end
 
-        def custom_field_context_changed?(work_package)
-          work_package.type_id_changed? ||
-            work_package.project_id_changed?
-        end
-
-        def only_validation_errors(errors)
-          errors.all? { |error| error.code == 422 }
-        end
-
         def notify_according_to_params
           params[:notify] != 'false'
         end

lib/open_project/text_formatting/formatters/textile/formatter.rb

@@ -206,7 +206,7 @@ module OpenProject::TextFormatting::Formatters
             ext = $2
             alt = $3
             alttext = $4
-            attachments ||= (options[:attachments] || obj.attachments).sort_by(&:created_on).reverse
+            attachments ||= (options[:attachments] || obj.attachments).sort_by(&:created_at).reverse
             # search for the picture in attachments
             if found = attachments.detect { |att| att.filename.downcase == filename }
               image_url = url_for only_path: only_path, controller: '/attachments', action: 'download', id: found

lib/plugins/acts_as_attachable/lib/acts_as_attachable.rb

@@ -33,22 +33,34 @@ module Redmine
         base.extend ClassMethods
       end
 
+      def self.attachables
+        @attachables ||= []
+      end
+
       module ClassMethods
         def acts_as_attachable(options = {})
+          Redmine::Acts::Attachable.attachables.push(self)
           cattr_accessor :attachable_options
-          self.attachable_options = {}
-          attachable_options[:view_permission] = options.delete(:view_permission) || "view_#{name.pluralize.underscore}".to_sym
-          attachable_options[:delete_permission] = options.delete(:delete_permission) || "edit_#{name.pluralize.underscore}".to_sym
+          set_acts_as_attachable_options(options)
 
-          attachments_order = options.delete(:order) || "#{Attachment.table_name}.created_on"
+          attachments_order = options.delete(:order) || "#{Attachment.table_name}.created_at"
           has_many :attachments, -> {
             order(attachments_order)
           }, options.reverse_merge!(as: :container, dependent: :destroy)
 
-          attr_accessor :unsaved_attachments
-          after_initialize :initialize_unsaved_attachments
+          attr_accessor :attachments_replacements
           send :include, Redmine::Acts::Attachable::InstanceMethods
         end
+
+        private
+
+        def set_acts_as_attachable_options(options)
+          name_default = name.pluralize.underscore
+          self.attachable_options = {}
+          attachable_options[:view_permission] = options.delete(:view_permission) || "view_#{name_default}".to_sym
+          attachable_options[:delete_permission] = options.delete(:delete_permission) || "edit_#{name_default}".to_sym
+          attachable_options[:add_permission] = options.delete(:add_permission) || "edit_#{name_default}".to_sym
+        end
       end
 
       module InstanceMethods
@@ -57,15 +69,15 @@ module Redmine
         end
 
         def attachments_visible?(user = User.current)
-          user.allowed_to?(self.class.attachable_options[:view_permission], project)
+          allowed_to_on_attachment?(user, self.class.attachable_options[:view_permission])
         end
 
         def attachments_deletable?(user = User.current)
-          user.allowed_to?(self.class.attachable_options[:delete_permission], project)
+          allowed_to_on_attachment?(user, self.class.attachable_options[:delete_permission])
         end
 
-        def initialize_unsaved_attachments
-          @unsaved_attachments ||= []
+        def attachments_addable?(user = User.current)
+          allowed_to_on_attachment?(user, self.class.attachable_options[:add_permission])
         end
 
         # Bulk attaches a set of files to an object
@@ -73,7 +85,7 @@ module Redmine
           if attachments && attachments.is_a?(Hash)
             attachments.each_value do |attachment|
               file = attachment['file']
-              next unless file && file.size > 0
+              next if !file || file.size.zero?
               self.attachments.build(file: file,
                                      container: self,
                                      description: attachment['description'].to_s.strip,
@@ -82,7 +94,20 @@ module Redmine
           end
         end
 
+        private
+
+        def allowed_to_on_attachment?(user, permissions)
+          Array(permissions).any? do |permission|
+            user.allowed_to?(permission, project)
+          end
+        end
+
         module ClassMethods
+          def attachments_addable?(user = User.current)
+            Array(attachable_options[:add_permission]).any? do |permission|
+              user.allowed_to_globally?(permission)
+            end
+          end
         end
       end
     end

spec/features/work_packages/table/milestones_spec.rb

@@ -7,15 +7,15 @@ describe 'Inline editing milestones', js: true do
   let(:project) { FactoryBot.create(:project, types: [type]) }
   let!(:work_package) {
     FactoryBot.create(:work_package,
-                       project: project,
-                       type:    type,
-                       subject: 'Foobar')
+                      project: project,
+                      type:    type,
+                      subject: 'Foobar')
   }
 
   let!(:wp_table) { Pages::WorkPackagesTable.new(project) }
   let!(:query) do
     query              = FactoryBot.build(:query, user: user, project: project)
-    query.column_names = ['subject', 'start_date', 'due_date']
+    query.column_names = %w(subject start_date due_date)
     query.filters.clear
     query.show_hierarchies = false
 

spec/features/work_packages/table/queries/filter_spec.rb

@@ -252,11 +252,15 @@ describe 'filter work packages', js: true do
   end
 
   context 'by attachment content' do
-    let(:attachment_a) { FactoryBot.create(:attachment, filename: 'attachment-first.pdf') }
-    let(:attachment_b) { FactoryBot.create(:attachment, filename: 'attachment-second.pdf') }
-    let(:wp_with_attachment_a) { FactoryBot.create :work_package, subject: 'WP attachment A', project: project, attachments: [attachment_a] }
-    let(:wp_with_attachment_b) { FactoryBot.create :work_package, subject: 'WP attachment B', project: project, attachments: [attachment_b] }
-    let(:wp_without_attachment) { FactoryBot.create :work_package, subject: 'WP no attachment', project: project}
+    let(:attachment_a) { FactoryBot.build(:attachment, filename: 'attachment-first.pdf') }
+    let(:attachment_b) { FactoryBot.build(:attachment, filename: 'attachment-second.pdf') }
+    let(:wp_with_attachment_a) do
+      FactoryBot.create :work_package, subject: 'WP attachment A', project: project, attachments: [attachment_a]
+    end
+    let(:wp_with_attachment_b) do
+      FactoryBot.create :work_package, subject: 'WP attachment B', project: project, attachments: [attachment_b]
+    end
+    let(:wp_without_attachment) { FactoryBot.create :work_package, subject: 'WP no attachment', project: project }
     let(:wp_table) { ::Pages::WorkPackagesTable.new }
 
     before do

spec/features/work_packages/table/switch_types_spec.rb

@@ -4,38 +4,38 @@ describe 'Switching types in work package table', js: true do
   let(:user) { FactoryBot.create :admin }
 
   describe 'switching to required CF' do
-    let(:cf_req_text) {
+    let(:cf_req_text) do
       FactoryBot.create(
         :work_package_custom_field,
         field_format: 'string',
         is_required: true,
         is_for_all: false
       )
-    }
-    let(:cf_text) {
+    end
+    let(:cf_text) do
       FactoryBot.create(
         :work_package_custom_field,
         field_format: 'string',
         is_required: false,
         is_for_all: false
       )
-    }
+    end
 
     let(:type_task) { FactoryBot.create(:type_task, custom_fields: [cf_text]) }
     let(:type_bug) { FactoryBot.create(:type_bug, custom_fields: [cf_req_text]) }
 
-    let(:project) {
+    let(:project) do
       FactoryBot.create(
         :project,
         types: [type_task, type_bug],
         work_package_custom_fields: [cf_text, cf_req_text]
       )
-    }
+    end
     let(:work_package) do
       FactoryBot.create(:work_package,
-                         subject: 'Foobar',
-                         type: type_task,
-                         project: project)
+                        subject: 'Foobar',
+                        type: type_task,
+                        project: project)
     end
     let(:wp_table) { Pages::WorkPackagesTable.new(project) }
 
@@ -173,30 +173,30 @@ describe 'Switching types in work package table', js: true do
   end
 
   describe 'switching to required bool CF with default value' do
-    let(:cf_req_bool) {
+    let(:cf_req_bool) do
       FactoryBot.create(
         :work_package_custom_field,
         field_format: 'bool',
         is_required: true,
         default_value: false
       )
-    }
+    end
 
     let(:type_task) { FactoryBot.create(:type_task) }
     let(:type_bug) { FactoryBot.create(:type_bug, custom_fields: [cf_req_bool]) }
 
-    let(:project) {
+    let(:project) do
       FactoryBot.create(
         :project,
         types: [type_task, type_bug],
         work_package_custom_fields: [cf_req_bool]
       )
-    }
+    end
     let(:work_package) do
       FactoryBot.create(:work_package,
-                         subject: 'Foobar',
-                         type: type_task,
-                         project: project)
+                        subject: 'Foobar',
+                        type: type_task,
+                        project: project)
     end
     let(:wp_page) { Pages::FullWorkPackage.new(work_package) }
     let(:type_field) { wp_page.edit_field :type }
@@ -232,8 +232,8 @@ describe 'Switching types in work package table', js: true do
     let(:role) { FactoryBot.create :role, permissions: permissions }
     let(:user) do
       FactoryBot.create :user,
-                         member_in_project: project,
-                         member_through_role: role
+                        member_in_project: project,
+                        member_through_role: role
     end
 
     before do
@@ -260,10 +260,10 @@ describe 'Switching types in work package table', js: true do
     let!(:status) { FactoryBot.create(:default_status) }
     let!(:workflow) do
       FactoryBot.create :workflow,
-                         type_id: type.id,
-                         old_status: status,
-                         new_status: FactoryBot.create(:status),
-                         role: role
+                        type_id: type.id,
+                        old_status: status,
+                        new_status: FactoryBot.create(:status),
+                        role: role
     end
 
     let!(:priority) { FactoryBot.create :priority, is_default: true }

spec/lib/api/v3/attachments/attachment_representer_spec.rb

@@ -38,10 +38,11 @@ describe ::API::V3::Attachments::AttachmentRepresenter do
   let(:permissions) { all_permissions }
 
   let(:container) { FactoryBot.build_stubbed(:stubbed_work_package) }
+  let(:author) { current_user }
   let(:attachment) do
     FactoryBot.build_stubbed(:attachment,
                              container: container,
-                             created_on: DateTime.now) do |attachment|
+                             author: author) do |attachment|
       allow(attachment)
         .to receive(:filename)
         .and_return('some_file_of_mine.txt')
@@ -79,7 +80,7 @@ describe ::API::V3::Attachments::AttachmentRepresenter do
   end
 
   it_behaves_like 'has UTC ISO 8601 date and time' do
-    let(:date) { attachment.created_on }
+    let(:date) { attachment.created_at }
     let(:json_path) { 'createdAt' }
   end
 
@@ -108,6 +109,15 @@ describe ::API::V3::Attachments::AttachmentRepresenter do
       end
     end
 
+    context 'without a container' do
+      let(:container) { nil }
+
+      it_behaves_like 'has an untitled link' do
+        let(:link) { 'container' }
+        let(:href) { nil }
+      end
+    end
+
     describe 'downloadLocation link' do
       context 'for a local attachment' do
         it_behaves_like 'has an untitled link' do
@@ -158,6 +168,25 @@ describe ::API::V3::Attachments::AttachmentRepresenter do
           let(:link) { 'delete' }
         end
       end
+
+      context 'attachment has no container' do
+        let(:container) { nil }
+
+        context 'user is the author' do
+          it_behaves_like 'has an untitled link' do
+            let(:link) { 'delete' }
+            let(:href) { api_v3_paths.attachment(attachment.id) }
+          end
+        end
+
+        context 'user is not the author' do
+          let(:author) { FactoryBot.build_stubbed(:user) }
+
+          it_behaves_like 'has no link' do
+            let(:link) { 'delete' }
+          end
+        end
+      end
     end
   end
 
@@ -187,9 +216,7 @@ describe ::API::V3::Attachments::AttachmentRepresenter do
       end
 
       it 'changes when the attachment is changed (has no update)' do
-        allow(attachment)
-          .to receive(:cache_key)
-          .and_return('blubs')
+        attachment.updated_at = Time.now + 10.seconds
 
         expect(representer.json_cache_key)
           .not_to eql former_cache_key

spec/lib/api/v3/utilities/custom_field_injector_spec.rb

@@ -244,7 +244,9 @@ describe ::API::V3::Utilities::CustomFieldInjector do
 
       it 'on writing it sets on the represented' do
         expected = { custom_field.id => expected_setter }
-        expect(represented).to receive(:custom_field_values=).with(expected)
+        expect(represented)
+          .to receive(:"custom_field_#{custom_field.id}=")
+          .with(expected_setter)
         modified_class
           .new(represented, current_user: nil)
           .from_json({ cf_path => json_value }.to_json)

spec/lib/api/v3/work_packages/form_helper_spec.rb

@@ -28,17 +28,17 @@
 
 require 'spec_helper'
 
-describe ::API::V3::WorkPackages::WorkPackagesSharedHelpers do
+describe ::API::V3::WorkPackages::FormHelper do
   let(:project) { FactoryBot.create(:project, is_public: false) }
   let(:work_package) { FactoryBot.create(:work_package, project: project) }
   let(:user) { FactoryBot.create(:user, member_in_project: project, member_through_role: role) }
   let(:role) { FactoryBot.create(:role, permissions: permissions) }
-  let(:permissions) { [:view_work_packages, :add_work_packages] }
+  let(:permissions) { %i[view_work_packages add_work_packages] }
   let(:env) { { 'api.request.body' => { 'subject' => 'foo' } } }
 
   let(:helper_class) {
     Class.new do
-      include ::API::V3::WorkPackages::WorkPackagesSharedHelpers
+      include ::API::V3::WorkPackages::FormHelper
 
       def initialize(user, env)
         @user = user
@@ -57,15 +57,14 @@ describe ::API::V3::WorkPackages::WorkPackagesSharedHelpers do
         @user
       end
 
-      def status(_code)
-      end
+      def status(_code); end
     end
   }
   let(:helper) { helper_class.new(user, env) }
 
-  describe '#create_work_package_form' do
+  describe '#respond_with_work_package_form' do
     subject do
-      helper.create_work_package_form(
+      helper.respond_with_work_package_form(
         work_package,
         contract_class: ::WorkPackages::CreateContract,
         form_class: ::API::V3::WorkPackages::CreateProjectFormRepresenter

spec/lib/api/v3/work_packages/work_package_representer_spec.rb

@@ -534,25 +534,7 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do
         end
 
         context 'user is not allowed to edit work packages' do
-          let(:permissions) { all_permissions - [:edit_work_packages] }
-
-          it_behaves_like 'has an untitled link' do
-            let(:link) { 'addAttachment' }
-            let(:href) { api_v3_paths.attachments_by_work_package(work_package.id) }
-          end
-        end
-
-        context 'user is not allowed to add work packages' do
-          let(:permissions) { all_permissions - [:add_work_packages] }
-
-          it_behaves_like 'has an untitled link' do
-            let(:link) { 'addAttachment' }
-            let(:href) { api_v3_paths.attachments_by_work_package(work_package.id) }
-          end
-        end
-
-        context 'user is neither allowed to edit work packages nor to add them' do
-          let(:permissions) { all_permissions - %i[edit_work_packages add_work_packages] }
+          let(:permissions) { all_permissions - %i[edit_work_packages] }
 
           it_behaves_like 'has no link' do
             let(:link) { 'addAttachment' }

spec/models/attachment_spec.rb

@@ -28,86 +28,159 @@
 require 'spec_helper'
 
 describe Attachment, type: :model do
-  let(:author)           { FactoryBot.create :user }
+  let(:stubbed_author) { FactoryBot.build_stubbed(:user) }
+  let(:author) { FactoryBot.create :user }
   let(:long_description) { 'a' * 300 }
-  let(:work_package)     { FactoryBot.create :work_package, description: '' }
-  let(:file)             { FactoryBot.create :uploaded_jpg, name: 'test.jpg' }
+  let(:work_package) { FactoryBot.create :work_package }
+  let(:stubbed_work_package) { FactoryBot.build_stubbed :stubbed_work_package }
+  let(:file) { FactoryBot.create :uploaded_jpg, name: 'test.jpg' }
+  let(:second_file) { FactoryBot.create :uploaded_jpg, name: 'test2.jpg' }
+  let(:container) { stubbed_work_package }
 
   let(:attachment) do
     FactoryBot.build(
       :attachment,
       author:       author,
-      container:    work_package,
+      container:    container,
       content_type: nil, # so that it is detected
-      file:         file)
+      file:         file
+    )
   end
   let(:stubbed_attachment) do
     FactoryBot.build_stubbed(
       :attachment,
-      author:       author,
-      container:    work_package,
-      content_type: nil, # so that it is detected
-      file:         file)
+      author:       stubbed_author,
+      container:    container
+    )
+  end
+
+  describe 'validations' do
+    it 'is valid' do
+      expect(stubbed_attachment)
+        .to be_valid
+    end
+
+    context 'with a long description' do
+      before do
+        stubbed_attachment.description = long_description
+        stubbed_attachment.valid?
+      end
+
+      it 'raises an error regarding description length' do
+        expect(stubbed_attachment.errors[:description])
+          .to match_array [I18n.t('activerecord.errors.messages.too_long', count: 255)]
+      end
+    end
+
+    context 'without a container' do
+      let(:container) { nil }
+
+      it 'is valid' do
+        expect(stubbed_attachment)
+          .to be_valid
+      end
+    end
+
+    context 'without a container first and then setting a container' do
+      let(:container) { nil }
+
+      before do
+        stubbed_attachment.container = work_package
+      end
+
+      it 'is valid' do
+        expect(stubbed_attachment)
+          .to be_valid
+      end
+    end
+
+    context 'with a container first and then removing the container' do
+      before do
+        stubbed_attachment.container = nil
+      end
+
+      it 'notes the field as unchangeable' do
+        stubbed_attachment.valid?
+
+        expect(stubbed_attachment.errors.symbols_for(:container))
+          .to match_array [:unchangeable]
+      end
+    end
+
+    context 'with a container first and then changing the container_id' do
+      before do
+        stubbed_attachment.container_id = stubbed_attachment.container_id + 1
+      end
+
+      it 'notes the field as unchangeable' do
+        stubbed_attachment.valid?
+
+        expect(stubbed_attachment.errors.symbols_for(:container))
+          .to match_array [:unchangeable]
+      end
+    end
+
+    context 'with a container first and then changing the container_type' do
+      before do
+        stubbed_attachment.container_type = 'WikiPage'
+      end
+
+      it 'notes the field as unchangeable' do
+        stubbed_attachment.valid?
+
+        expect(stubbed_attachment.errors.symbols_for(:container))
+          .to match_array [:unchangeable]
+      end
+    end
   end
 
   describe 'create' do
-    context 'save' do
-      before do
-        attachment.description = long_description
-        attachment.valid?
-      end
-
-      it 'should validate description length' do
-        expect(attachment.errors[:description]).not_to be_empty
-      end
-
-      it 'should raise an error regarding description length' do
-        expect(attachment.errors.full_messages[0]).to include I18n.t('activerecord.errors.messages.too_long', count: 255)
-      end
-    end
-
-    it('should create a jpg file called test') do
+    it('creates a jpg file called test') do
       expect(File.exists?(attachment.diskfile.path)).to eq true
     end
 
-    it('have the content type "image/jpeg"') do
+    it('has the content type "image/jpeg"') do
       expect(attachment.content_type).to eq 'image/jpeg'
     end
 
     context 'with wrong content-type' do
       let(:file) { FactoryBot.create :uploaded_jpg, content_type: 'text/html' }
 
-      it 'should detect the correct content-type' do
+      it 'detects the correct content-type' do
         expect(attachment.content_type).to eq 'image/jpeg'
       end
     end
-  end
 
-  describe 'update' do
-    before do
-      attachment.save!
+    it 'has the correct filesize' do
+      expect(attachment.filesize)
+        .to eql file.size
     end
 
-    context 'update' do
-      before do
-        attachment.description = long_description
-        attachment.valid?
-      end
+    it 'creates an md5 digest' do
+      expect(attachment.digest)
+        .to eql Digest::MD5.file(file.path).hexdigest
+    end
+  end
 
-      it 'should validate description length' do
-        expect(attachment.errors[:description]).not_to be_empty
-      end
+  describe 'two attachments with same file name' do
+    let(:second_file) { FactoryBot.create :uploaded_jpg, name: file.original_filename }
+    it 'does not interfere' do
+      a1 = Attachment.create!(container: work_package,
+                              file: file,
+                              author: author)
+      a2 = Attachment.create!(container: work_package,
+                              file: second_file,
+                              author: author)
 
-      it 'should raise an error regarding description length' do
-        expect(attachment.errors.full_messages[0]).to include I18n.t('activerecord.errors.messages.too_long', count: 255)
-      end
+      expect(a1.diskfile.path)
+        .not_to eql a2.diskfile.path
     end
   end
 
   ##
   # The tests assumes the default, file-based storage is configured and tests against that.
   # I.e. it does not test fog attachments being deleted from the cloud storage (such as S3).
-  describe 'destroy' do
+  describe '#destroy' do
     before do
       attachment.save!
 
@@ -123,18 +196,4 @@ describe Attachment, type: :model do
       expect(File.exists?(attachment.file.path)).to eq false
     end
   end
-
-  # Made necessary as attachments only have the created_on field which is not factored
-  # into the cache_key. While it shouldn't be a problem in production, as attachments cannot be
-  # altered, it is a problem in the tests.
-  describe '#cache_key' do
-    before do
-      stubbed_attachment.created_on = Time.now
-    end
-
-    it 'factors in id and created_on' do
-      expect(stubbed_attachment.cache_key)
-        .to eql("attachments/#{stubbed_attachment.id}-#{stubbed_attachment.created_on.to_i}")
-    end
-  end
 end

spec/requests/api/v3/attachments/attachment_resource_spec.rb

@@ -37,6 +37,9 @@ describe 'API v3 Attachment resource', type: :request, content_type: :json do
   let(:current_user) do
     FactoryBot.create(:user, member_in_project: project, member_through_role: role)
   end
+  let(:author) do
+    current_user
+  end
   let(:project) { FactoryBot.create(:project, is_public: false) }
   let(:role) { FactoryBot.create(:role, permissions: permissions) }
   let(:permissions) do
@@ -44,7 +47,7 @@ describe 'API v3 Attachment resource', type: :request, content_type: :json do
        edit_work_packages edit_wiki_pages edit_messages]
   end
   let(:work_package) { FactoryBot.create(:work_package, author: current_user, project: project) }
-  let(:attachment) { FactoryBot.create(:attachment, container: container) }
+  let(:attachment) { FactoryBot.create(:attachment, container: container, author: author) }
   let(:wiki) { FactoryBot.create(:wiki, project: project) }
   let(:wiki_page) { FactoryBot.create(:wiki_page, wiki: wiki) }
   let(:board) { FactoryBot.create(:board, project: project) }
@@ -185,18 +188,32 @@ describe 'API v3 Attachment resource', type: :request, content_type: :json do
 
     subject(:response) { last_response }
 
+    shared_examples_for 'deletes the attachment' do
+      it 'responds with HTTP No Content' do
+        expect(subject.status).to eq 204
+      end
+
+      it 'removes the attachment from the DB' do
+        expect(Attachment.exists?(attachment.id)).to be_falsey
+      end
+    end
+
+    shared_examples_for 'does not delete the attachment' do |status = 403|
+      it "responds with #{status}" do
+        expect(subject.status).to eq status
+      end
+
+      it 'does not delete the attachment' do
+        expect(Attachment.exists?(attachment.id)).to be_truthy
+      end
+    end
+
     %i[wiki_page work_package board_message].each do |attachment_type|
       context "with a #{attachment_type} attachment" do
         let(:container) { send(attachment_type) }
 
         context 'with required permissions' do
-          it 'responds with HTTP No Content' do
-            expect(subject.status).to eq 204
-          end
-
-          it 'deletes the attachment' do
-            expect(Attachment.exists?(attachment.id)).not_to be_truthy
-          end
+          it_behaves_like 'deletes the attachment'
 
           context 'for a non-existent attachment' do
             let(:path) { api_v3_paths.attachment 1337 }
@@ -211,16 +228,24 @@ describe 'API v3 Attachment resource', type: :request, content_type: :json do
         context 'without required permissions' do
           let(:permissions) { %i[view_work_packages view_wiki_pages] }
 
-          it 'responds with 403' do
-            expect(subject.status).to eq 403
-          end
-
-          it 'does not delete the attachment' do
-            expect(Attachment.exists?(attachment.id)).to be_truthy
-          end
+          it_behaves_like 'does not delete the attachment'
         end
       end
     end
+
+    context "with an uncontainered attachment" do
+      let(:container) { nil }
+
+      context 'with the user being the author' do
+        it_behaves_like 'deletes the attachment'
+      end
+
+      context 'with the user not being the author' do
+        let(:author) { FactoryBot.create(:user) }
+
+        it_behaves_like 'does not delete the attachment', 404
+      end
+    end
   end
 
   describe '#content' do

spec/requests/api/v3/attachments/attachments_by_post_resource_spec.rb

@@ -134,9 +134,7 @@ describe 'API v3 Attachments by post resource', type: :request do
     context 'only allowed to add messages, but no edit permission' do
       let(:permissions) { %i[view_messages add_messages] }
 
-      it 'should respond with HTTP Created' do
-        expect(subject.status).to eq(201)
-      end
+      it_behaves_like 'unauthorized access'
     end
 
     context 'only allowed to view messages' do

spec/requests/api/v3/attachments/attachments_by_work_package_resource_spec.rb

@@ -133,9 +133,7 @@ describe 'API v3 Attachments by work package resource', type: :request do
     context 'only allowed to add work packages, but no edit permission' do
       let(:permissions) { %i[view_work_packages add_work_packages] }
 
-      it 'should respond with HTTP Created' do
-        expect(subject.status).to eq(201)
-      end
+      it_behaves_like 'unauthorized access'
     end
 
     context 'only allowed to view work packages' do

spec/requests/api/v3/work_package_resource_spec.rb

@@ -36,16 +36,16 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
 
   let(:closed_status) { FactoryBot.create(:closed_status) }
 
-  let(:work_package) {
+  let(:work_package) do
     FactoryBot.create(:work_package, project_id: project.id,
-                                      description: 'lorem ipsum'
+                                     description: 'lorem ipsum'
                       )
-  }
+  end
   let(:project) do
     FactoryBot.create(:project, identifier: 'test_project', is_public: false)
   end
   let(:role) { FactoryBot.create(:role, permissions: permissions) }
-  let(:permissions) { [:view_work_packages, :edit_work_packages] }
+  let(:permissions) { %i[view_work_packages edit_work_packages] }
   let(:current_user) do
     user = FactoryBot.create(:user, member_in_project: project, member_through_role: role)
 
@@ -133,16 +133,15 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
 
       describe 'response body' do
         subject(:parsed_response) { JSON.parse(last_response.body) }
-        let!(:other_wp)    {
+        let!(:other_wp) do
           FactoryBot.create(:work_package, project_id: project.id,
-                                            status: closed_status)
-        }
-        let(:work_package) {
+                                           status: closed_status)
+        end
+        let(:work_package) do
           FactoryBot.create(:work_package, project_id: project.id,
-                                            description: description
-                            )
-        }
-        let(:description) {
+                                           description: description)
+        end
+        let(:description) do
           %{
       {{>toc}}
 
@@ -162,7 +161,7 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
       * Relaxed
       * Debonaire
 
-      }}
+      } end
 
         it 'should respond with work package in HAL+JSON format' do
           expect(parsed_response['id']).to eq(work_package.id)
@@ -245,11 +244,11 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
 
       context 'no permission to edit the work package' do
         let(:role) { FactoryBot.create(:role, permissions: [:view_work_packages]) }
-        let(:current_user) {
+        let(:current_user) do
           FactoryBot.create(:user,
-                             member_in_project: work_package.project,
-                             member_through_role: role)
-        }
+                            member_in_project: work_package.project,
+                            member_through_role: role)
+        end
         let(:params) { valid_params }
 
         include_context 'patch request'
@@ -339,9 +338,9 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
 
         context 'with value' do
           let(:raw) { '*Some text* _describing_ *something*...' }
-          let(:html) {
+          let(:html) do
             '<p><strong>Some text</strong> <em>describing</em> <strong>something</strong>...</p>'
-          }
+          end
           let(:params) { valid_params.merge(description: { raw: raw }) }
 
           include_context 'patch request'
@@ -388,16 +387,16 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
         let(:status_parameter) { { _links: { status: { href: status_link } } } }
         let(:params) { valid_params.merge(status_parameter) }
 
-        before do allow(User).to receive(:current).and_return current_user end
+        before { allow(User).to receive(:current).and_return current_user }
 
         context 'valid status' do
-          let!(:workflow) {
+          let!(:workflow) do
             FactoryBot.create(:workflow,
-                               type_id: work_package.type.id,
-                               old_status: work_package.status,
-                               new_status: target_status,
-                               role: current_user.memberships[0].roles[0])
-          }
+                              type_id: work_package.type.id,
+                              old_status: work_package.status,
+                              new_status: target_status,
+                              role: current_user.memberships[0].roles[0])
+          end
 
           include_context 'patch request'
 
@@ -415,10 +414,10 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
           include_context 'patch request'
 
           it_behaves_like 'constraint violation' do
-            let(:message) {
+            let(:message) do
               'Status ' + I18n.t('activerecord.errors.models.' \
                           'work_package.attributes.status_id.status_transition_invalid')
-            }
+            end
           end
         end
 
@@ -428,12 +427,12 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
           include_context 'patch request'
 
           it_behaves_like 'invalid resource link' do
-            let(:message) {
+            let(:message) do
               I18n.t('api_v3.errors.invalid_resource',
                      property: 'status',
                      expected: '/api/v3/statuses/:id',
                      actual: status_link)
-            }
+            end
           end
         end
       end
@@ -444,7 +443,7 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
         let(:type_parameter) { { _links: { type: { href: type_link } } } }
         let(:params) { valid_params.merge(type_parameter) }
 
-        before do allow(User).to receive(:current).and_return current_user end
+        before { allow(User).to receive(:current).and_return current_user }
 
         context 'valid type' do
           before do
@@ -497,12 +496,12 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
           include_context 'patch request'
 
           it_behaves_like 'invalid resource link' do
-            let(:message) {
+            let(:message) do
               I18n.t('api_v3.errors.invalid_resource',
                      property: 'type',
                      expected: '/api/v3/types/:id',
                      actual: type_link)
-            }
+            end
           end
         end
       end
@@ -517,9 +516,9 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
 
         before do
           FactoryBot.create :member,
-                             user: current_user,
-                             project: target_project,
-                             roles: [FactoryBot.create(:role, permissions: [:move_work_packages])]
+                            user: current_user,
+                            project: target_project,
+                            roles: [FactoryBot.create(:role, permissions: [:move_work_packages])]
 
           allow(User).to receive(:current).and_return current_user
         end
@@ -565,24 +564,24 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
       context 'assignee and responsible' do
         let(:user) { FactoryBot.create(:user, member_in_project: project) }
         let(:params) { valid_params.merge(user_parameter) }
-        let(:work_package) {
+        let(:work_package) do
           FactoryBot.create(:work_package,
-                             project: project,
-                             assigned_to: current_user,
-                             responsible: current_user)
-        }
+                            project: project,
+                            assigned_to: current_user,
+                            responsible: current_user)
+        end
 
-        before do allow(User).to receive(:current).and_return current_user end
+        before { allow(User).to receive(:current).and_return current_user }
 
         shared_context 'setup group membership' do |group_assignment|
           let(:group) { FactoryBot.create(:group) }
           let(:group_role) { FactoryBot.create(:role) }
-          let(:group_member) {
+          let(:group_member) do
             FactoryBot.create(:member,
-                               principal: group,
-                               project: project,
-                               roles: [group_role])
-          }
+                              principal: group,
+                              project: project,
+                              roles: [group_role])
+          end
 
           before do
             allow(Setting).to receive(:work_package_group_assignment?).and_return(group_assignment)
@@ -650,11 +649,11 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
               let(:user_href) { api_v3_paths.user 909090 }
 
               it_behaves_like 'constraint violation' do
-                let(:message) {
+                let(:message) do
                   I18n.t('api_v3.errors.validation.' \
                                      'invalid_user_assigned_to_work_package',
                          property: property.capitalize)
-                }
+                end
               end
             end
 
@@ -663,10 +662,10 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
               let(:user_href) { api_v3_paths.user invalid_user.id }
 
               it_behaves_like 'constraint violation' do
-                let(:message) {
+                let(:message) do
                   I18n.t('api_v3.errors.validation.invalid_user_assigned_to_work_package',
                          property: property.capitalize)
-                }
+                end
               end
             end
 
@@ -676,12 +675,12 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
               include_context 'patch request'
 
               it_behaves_like 'invalid resource link' do
-                let(:message) {
+                let(:message) do
                   I18n.t('api_v3.errors.invalid_resource',
                          property: property,
                          expected: '/api/v3/users/:id',
                          actual: user_href)
-                }
+                end
               end
             end
 
@@ -692,10 +691,10 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
               include_context 'patch request'
 
               it_behaves_like 'constraint violation' do
-                let(:message) {
+                let(:message) do
                   I18n.t('api_v3.errors.validation.invalid_user_assigned_to_work_package',
                          property: "#{property.capitalize}")
-                }
+                end
               end
             end
           end
@@ -716,7 +715,7 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
         let(:version_parameter) { { _links: { version: { href: version_link } } } }
         let(:params) { valid_params.merge(version_parameter) }
 
-        before do allow(User).to receive(:current).and_return current_user end
+        before { allow(User).to receive(:current).and_return current_user }
 
         context 'valid' do
           include_context 'patch request'
@@ -738,7 +737,7 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
         let(:category_parameter) { { _links: { category: { href: category_link } } } }
         let(:params) { valid_params.merge(category_parameter) }
 
-        before do allow(User).to receive(:current).and_return current_user end
+        before { allow(User).to receive(:current).and_return current_user }
 
         context 'valid' do
           include_context 'patch request'
@@ -760,7 +759,7 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
         let(:priority_parameter) { { _links: { priority: { href: priority_link } } } }
         let(:params) { valid_params.merge(priority_parameter) }
 
-        before do allow(User).to receive(:current).and_return current_user end
+        before { allow(User).to receive(:current).and_return current_user }
 
         context 'valid' do
           include_context 'patch request'
@@ -913,6 +912,34 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
           it_behaves_like 'update conflict'
         end
       end
+
+      context 'claiming attachments' do
+        let(:old_attachment) { FactoryBot.create(:attachment, container: work_package) }
+        let(:attachment) { FactoryBot.create(:attachment, container: nil, author: current_user) }
+        let(:params) do
+          {
+            lockVersion: work_package.lock_version,
+            _links: {
+              attachments: [
+                href: api_v3_paths.attachment(attachment.id)
+              ]
+            }
+          }
+        end
+
+        before do
+          old_attachment
+        end
+
+        include_context 'patch request'
+
+        it 'replaces the current with the provided attachments' do
+          work_package.reload
+
+          expect(work_package.attachments)
+            .to match_array(attachment)
+        end
+      end
     end
   end
 
@@ -926,7 +953,7 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
     subject { last_response }
 
     context 'with required permissions' do
-      let(:permissions) { [:view_work_packages, :delete_work_packages] }
+      let(:permissions) { %i[view_work_packages delete_work_packages] }
 
       it 'responds with HTTP No Content' do
         expect(subject.status).to eq 204
@@ -965,7 +992,7 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
 
   describe '#post' do
     let(:path) { api_v3_paths.work_packages }
-    let(:permissions) { [:add_work_packages, :view_project] }
+    let(:permissions) { %i[add_work_packages view_project] }
     let(:status) { FactoryBot.build(:status, is_default: true) }
     let(:priority) { FactoryBot.build(:priority, is_default: true) }
     let(:type) { project.types.first }
@@ -992,7 +1019,7 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
     end
 
     context 'notifications' do
-      let(:permissions) { [:add_work_packages, :view_project, :view_work_packages] }
+      let(:permissions) { %i[add_work_packages view_project view_work_packages] }
 
       it 'sends a mail by default' do
         expect(ActionMailer::Base.deliveries.count).to eq(1)
@@ -1110,5 +1137,34 @@ describe 'API v3 Work package resource', type: :request, content_type: :json do
         expect(WorkPackage.all.count).to eq(0)
       end
     end
+
+    context 'claiming attachments' do
+      let(:attachment) { FactoryBot.create(:attachment, container: nil, author: current_user) }
+      let(:parameters) do
+        {
+          subject: 'subject',
+          _links: {
+            type: {
+              href: api_v3_paths.type(project.types.first.id)
+            },
+            project: {
+              href: api_v3_paths.project(project.id)
+            },
+            attachments: [
+              href: api_v3_paths.attachment(attachment.id)
+            ]
+          }
+        }
+      end
+
+      it 'creates the work package and assigns the attachments' do
+        expect(WorkPackage.all.count).to eq(1)
+
+        work_package = WorkPackage.last
+
+        expect(work_package.attachments)
+          .to match_array(attachment)
+      end
+    end
   end
 end

spec/services/work_packages/create_service_integration_spec.rb

@@ -31,12 +31,12 @@ require 'spec_helper'
 describe WorkPackages::CreateService, 'integration', type: :model do
   let(:user) do
     FactoryBot.create(:user,
-                       member_in_project: project,
-                       member_through_role: role)
+                      member_in_project: project,
+                      member_through_role: role)
   end
   let(:role) do
     FactoryBot.create(:role,
-                       permissions: permissions)
+                      permissions: permissions)
   end
 
   let(:permissions) do
@@ -45,7 +45,7 @@ describe WorkPackages::CreateService, 'integration', type: :model do
 
   let(:type) do
     FactoryBot.create(:type,
-                       custom_fields: [custom_field])
+                      custom_fields: [custom_field])
   end
   let(:default_type) do
     FactoryBot.create(:type_standard)
@@ -53,8 +53,8 @@ describe WorkPackages::CreateService, 'integration', type: :model do
   let(:project) { FactoryBot.create(:project, types: [type, default_type]) }
   let(:parent) do
     FactoryBot.create(:work_package,
-                       project: project,
-                       type: type)
+                      project: project,
+                      type: type)
   end
   let(:instance) { described_class.new(user: user) }
   let(:custom_field) { FactoryBot.create(:work_package_custom_field) }
@@ -128,5 +128,42 @@ describe WorkPackages::CreateService, 'integration', type: :model do
       expect(parent.due_date)
         .to eql attributes[:due_date]
     end
+
+    describe 'setting the attachments' do
+      let!(:other_users_attachment) do
+        FactoryBot.create(:attachment, container: nil, author: FactoryBot.create(:user))
+      end
+      let!(:users_attachment) do
+        FactoryBot.create(:attachment, container: nil, author: user)
+      end
+
+      it 'reports on invalid attachments and sets the new if everything is valid' do
+        result = instance.call(attributes: attributes.merge(attachment_ids: [other_users_attachment.id]))
+
+        expect(result)
+          .to be_failure
+
+        expect(result.errors.symbols_for(:attachments))
+          .to match_array [:does_not_exist]
+
+        # The parent work package
+        expect(WorkPackage.count)
+          .to eql 1
+
+        expect(other_users_attachment.reload.container)
+          .to be_nil
+
+        result = instance.call(attributes: attributes.merge(attachment_ids: [users_attachment.id]))
+
+        expect(result)
+          .to be_success
+
+        expect(result.result.attachments)
+          .to match_array [users_attachment]
+
+        expect(users_attachment.reload.container)
+          .to eql result.result
+      end
+    end
   end
 end

spec/services/work_packages/update_service_integration_spec.rb

@@ -33,8 +33,8 @@ require 'spec_helper'
 describe WorkPackages::UpdateService, 'integration tests', type: :model do
   let(:user) do
     FactoryBot.create(:user,
-                       member_in_project: project,
-                       member_through_role: role)
+                      member_in_project: project,
+                      member_through_role: role)
   end
   let(:role) { FactoryBot.create(:role, permissions: permissions) }
   let(:permissions) do
@@ -55,18 +55,18 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
   end
   let(:work_package) do
     FactoryBot.create(:work_package,
-                       work_package_attributes)
+                      work_package_attributes)
   end
   let(:parent_work_package) do
     FactoryBot.create(:work_package,
-                       work_package_attributes).tap do |w|
+                      work_package_attributes).tap do |w|
       w.children << work_package
       work_package.reload
     end
   end
   let(:grandparent_work_package) do
     FactoryBot.create(:work_package,
-                       work_package_attributes).tap do |w|
+                      work_package_attributes).tap do |w|
       w.children << parent_work_package
     end
   end
@@ -78,25 +78,25 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
   end
   let(:sibling1_work_package) do
     FactoryBot.create(:work_package,
-                       sibling1_attributes)
+                      sibling1_attributes)
   end
   let(:sibling2_work_package) do
     FactoryBot.create(:work_package,
-                       sibling2_attributes)
+                      sibling2_attributes)
   end
   let(:child_attributes) do
     work_package_attributes.merge(parent: work_package)
   end
   let(:child_work_package) do
     FactoryBot.create(:work_package,
-                       child_attributes)
+                      child_attributes)
   end
   let(:grandchild_attributes) do
     work_package_attributes.merge(parent: child_work_package)
   end
   let(:grandchild_work_package) do
     FactoryBot.create(:work_package,
-                       grandchild_attributes)
+                      grandchild_attributes)
   end
   let(:instance) do
     described_class.new(user: user,
@@ -124,13 +124,13 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
     describe 'updating project' do
       let(:target_project) do
         p = FactoryBot.create(:project,
-                               types: target_types,
-                               parent: target_parent)
+                              types: target_types,
+                              parent: target_parent)
 
         FactoryBot.create(:member,
-                           user: user,
-                           project: p,
-                           roles: [FactoryBot.create(:role, permissions: target_permissions)])
+                          user: user,
+                          project: p,
+                          roles: [FactoryBot.create(:role, permissions: target_permissions)])
 
         p
       end
@@ -151,11 +151,11 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
       describe 'time_entries' do
         let!(:time_entries) do
           [FactoryBot.create(:time_entry,
-                              project: project,
-                              work_package: work_package),
+                             project: project,
+                             work_package: work_package),
            FactoryBot.create(:time_entry,
-                              project: project,
-                              work_package: work_package)]
+                             project: project,
+                             work_package: work_package)]
         end
 
         it 'moves the time entries along' do
@@ -169,7 +169,7 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
         describe 'categories' do
           let(:category) do
             FactoryBot.create(:category,
-                               project: project)
+                              project: project)
           end
 
           before do
@@ -180,8 +180,8 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
           context 'with equally named category' do
             let!(:target_category) do
               FactoryBot.create(:category,
-                                 name: category.name,
-                                 project: target_project)
+                                name: category.name,
+                                project: target_project)
             end
 
             it 'replaces the current category by the equally named one' do
@@ -196,7 +196,7 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
           context 'w/o target category' do
             let!(:other_category) do
               FactoryBot.create(:category,
-                                 project: target_project)
+                                project: target_project)
             end
 
             it 'removes the category' do
@@ -213,14 +213,14 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
           let(:sharing) { 'none' }
           let(:version) do
             FactoryBot.create(:version,
-                               status: 'open',
-                               project: project,
-                               sharing: sharing)
+                              status: 'open',
+                              project: project,
+                              sharing: sharing)
           end
           let(:work_package) do
             FactoryBot.create(:work_package,
-                               fixed_version: version,
-                               project: project)
+                              fixed_version: version,
+                              project: project)
           end
 
           context 'unshared version' do
@@ -509,17 +509,17 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
     describe 'closing duplicates on closing status' do
       let(:status_closed) do
         FactoryBot.create(:status,
-                           is_closed: true).tap do |status_closed|
+                          is_closed: true).tap do |status_closed|
           FactoryBot.create(:workflow,
-                             old_status: status,
-                             new_status: status_closed,
-                             type: type,
-                             role: role)
+                            old_status: status,
+                            new_status: status_closed,
+                            type: type,
+                            role: role)
         end
       end
       let(:duplicate_work_package) do
         FactoryBot.create(:work_package,
-                           work_package_attributes).tap do |wp|
+                          work_package_attributes).tap do |wp|
           wp.duplicated << work_package
         end
       end
@@ -569,7 +569,7 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
       end
       let(:following_work_package) do
         FactoryBot.create(:work_package,
-                           following_attributes).tap do |wp|
+                          following_attributes).tap do |wp|
           wp.follows << work_package
         end
       end
@@ -580,7 +580,7 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
       end
       let(:following_parent_work_package) do
         FactoryBot.create(:work_package,
-                           following_parent_attributes)
+                          following_parent_attributes)
       end
       let(:following2_attributes) do
         work_package_attributes.merge(parent: following2_parent_work_package,
@@ -590,7 +590,7 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
       end
       let(:following2_work_package) do
         FactoryBot.create(:work_package,
-                           following2_attributes)
+                          following2_attributes)
       end
       let(:following2_parent_attributes) do
         work_package_attributes.merge(subject: 'following2_parent',
@@ -599,7 +599,7 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
       end
       let(:following2_parent_work_package) do
         FactoryBot.create(:work_package,
-                           following2_parent_attributes).tap do |wp|
+                          following2_parent_attributes).tap do |wp|
           wp.follows << following_parent_work_package
         end
       end
@@ -611,7 +611,7 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
       end
       let(:following3_work_package) do
         FactoryBot.create(:work_package,
-                           following3_attributes).tap do |wp|
+                          following3_attributes).tap do |wp|
           wp.follows << following2_work_package
         end
       end
@@ -622,7 +622,7 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
       end
       let(:following3_parent_work_package) do
         FactoryBot.create(:work_package,
-                           following3_parent_attributes)
+                          following3_parent_attributes)
       end
       let(:following3_sibling_attributes) do
         work_package_attributes.merge(parent: following3_parent_work_package,
@@ -632,7 +632,7 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
       end
       let(:following3_sibling_work_package) do
         FactoryBot.create(:work_package,
-                           following3_sibling_attributes)
+                          following3_sibling_attributes)
       end
 
       before do
@@ -743,7 +743,7 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
       end
       let(:following_work_package) do
         FactoryBot.create(:work_package,
-                           following_attributes).tap do |wp|
+                          following_attributes).tap do |wp|
           wp.follows << work_package
         end
       end
@@ -754,7 +754,7 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
       end
       let(:following_parent_work_package) do
         FactoryBot.create(:work_package,
-                           following_parent_attributes)
+                          following_parent_attributes)
       end
       let(:other_attributes) do
         work_package_attributes.merge(subject: 'other',
@@ -763,7 +763,7 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
       end
       let(:other_work_package) do
         FactoryBot.create(:work_package,
-                           other_attributes)
+                          other_attributes)
       end
       let(:following2_attributes) do
         work_package_attributes.merge(parent: following2_parent_work_package,
@@ -773,7 +773,7 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
       end
       let(:following2_work_package) do
         FactoryBot.create(:work_package,
-                           following2_attributes)
+                          following2_attributes)
       end
       let(:following2_parent_attributes) do
         work_package_attributes.merge(subject: 'following2_parent',
@@ -782,15 +782,15 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
       end
       let(:following2_parent_work_package) do
         following2 = FactoryBot.create(:work_package,
-                                        following2_parent_attributes).tap do |wp|
+                                       following2_parent_attributes).tap do |wp|
           wp.follows << following_parent_work_package
         end
 
         FactoryBot.create(:relation,
-                           relation_type: Relation::TYPE_FOLLOWS,
-                           from: following2,
-                           to: other_work_package,
-                           delay: 3)
+                          relation_type: Relation::TYPE_FOLLOWS,
+                          from: following2,
+                          to: other_work_package,
+                          delay: 3)
 
         following2
       end
@@ -801,7 +801,7 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
       end
       let(:following3_work_package) do
         FactoryBot.create(:work_package,
-                           following3_attributes).tap do |wp|
+                          following3_attributes).tap do |wp|
           wp.follows << following2_work_package
         end
       end
@@ -1123,6 +1123,61 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model do
           .to eql sibling_attributes[:due_date]
       end
     end
+
+    describe 'replacing the attachments' do
+      let!(:old_attachment) do
+        FactoryBot.create(:attachment, container: work_package)
+      end
+      let!(:other_users_attachment) do
+        FactoryBot.create(:attachment, container: nil, author: FactoryBot.create(:user))
+      end
+      let!(:new_attachment) do
+        FactoryBot.create(:attachment, container: nil, author: user)
+      end
+
+      it 'reports on invalid attachments and replaces the existent with the new if everything is valid' do
+        work_package.attachments.reload
+
+        result = instance.call(attributes: { attachment_ids: [other_users_attachment.id] })
+
+        expect(result)
+          .to be_failure
+
+        expect(result.errors.symbols_for(:attachments))
+          .to match_array [:does_not_exist]
+
+        expect(work_package.attachments.reload)
+          .to match_array [old_attachment]
+
+        expect(other_users_attachment.reload.container)
+          .to be_nil
+
+        result = instance.call(attributes: { attachment_ids: [new_attachment.id] })
+
+        expect(result)
+          .to be_success
+
+        expect(work_package.attachments.reload)
+          .to match_array [new_attachment]
+
+        expect(new_attachment.reload.container)
+          .to eql work_package
+
+        expect(Attachment.find_by(id: old_attachment.id))
+          .to be_nil
+
+        result = instance.call(attributes: { attachment_ids: [] })
+
+        expect(result)
+          .to be_success
+
+        expect(work_package.attachments.reload)
+          .to be_empty
+
+        expect(Attachment.all)
+          .to match_array [other_users_attachment]
+      end
+    end
   end
 
   ##