Merge pull request #14120 from virgo47/feature/50849_change_work_pack…

…ages_status

Feature/50849 change work packages status

app/contracts/base_contract.rb

@@ -180,9 +180,10 @@ def ancestor_attribute_aliases
 
   # Traverse ancestor hierarchy to collect contract information.
   # This allows to define attributes on a common base class of two or more contracts.
+  # Reverse merge is important to use the more specific overrides from subclasses.
   def collect_ancestor_attributes(attribute_to_collect)
     combination_method, cleanup_method = if self.class.send(attribute_to_collect).is_a?(Hash)
-                                           %i[merge with_indifferent_access]
+                                           %i[reverse_merge! with_indifferent_access]
                                          else
                                            %i[concat uniq]
                                          end
@@ -198,6 +199,7 @@ def collect_ancestor_attributes_by(attribute_to_collect, combination_method, cle
     # similar object from the superclass, every call would alter the memoized object as an unwanted side effect.
     # Not only would that lead to the subclass now having all the attributes of the superclass,
     # but those attributes would also be duplicated so that performance suffers significantly.
+    # `dup` also enables usage of combination methods working in place, e.g. `reverse_merge!`
     attributes = klass.send(attribute_to_collect).dup
 
     while klass.superclass != ::BaseContract
@@ -246,8 +248,8 @@ def reduce_by_writable_permissions(attributes)
       canonical_attribute = attribute.delete_suffix('_id')
 
       permissions = attribute_permissions[canonical_attribute] ||
-                    attribute_permissions["#{canonical_attribute}_id"] ||
-                    attribute_permissions[:default_permission]
+        attribute_permissions["#{canonical_attribute}_id"] ||
+        attribute_permissions[:default_permission]
 
       next unless permissions
 

app/contracts/work_packages/base_contract.rb

@@ -34,6 +34,7 @@ class BaseContract < ::ModelContract
     attribute :subject
     attribute :description
     attribute :status_id,
+              permission: %i[edit_work_packages change_work_package_status],
               writable: ->(*) {
                 # If we did not change into the status,
                 # mark unwritable if status and version is closed

app/contracts/work_packages/create_contract.rb

@@ -34,6 +34,11 @@ class CreateContract < BaseContract
               writable: false do
       errors.add :author_id, :invalid if model.author != user
     end
+    attribute :status_id,
+              # Overriding permission from WP base contract to ignore change_work_package_status for creation,
+              # because we don't require that permission for writable status during WP creation.
+              # Note that nil would not override and [] would ignore the default permission, so we use the default here:
+              permission: :add_work_packages
 
     default_attribute_permission :add_work_packages
 

app/contracts/work_packages/update_contract.rb

@@ -52,6 +52,7 @@ def user_allowed_to_edit
       with_unchanged_project_id do
         next if @can.allowed?(model, :edit) ||
                 @can.allowed?(model, :assign_version) ||
+                @can.allowed?(model, :change_status) ||
                 @can.allowed?(model, :manage_subtasks) ||
                 @can.allowed?(model, :move)
         next if allowed_journal_addition?

app/policies/work_package_policy.rb

@@ -49,6 +49,7 @@ def allowed_hash(work_package)
       delete: delete_allowed?(work_package),
       manage_subtasks: manage_subtasks_allowed?(work_package),
       comment: comment_allowed?(work_package),
+      change_status: change_status_allowed?(work_package),
       assign_version: assign_version_allowed?(work_package)
     }
   end
@@ -94,4 +95,12 @@ def comment_allowed?(work_package)
   def assign_version_allowed?(work_package)
     user.allowed_in_project?(:assign_versions, work_package.project)
   end
+
+  def change_status_allowed?(work_package)
+    @change_status_cache ||= Hash.new do |hash, project|
+      hash[project] = user.allowed_in_project?(%i[edit_work_packages change_work_package_status], work_package.project)
+    end
+
+    @change_status_cache[work_package.project]
+  end
 end

app/services/authorization.rb

@@ -64,22 +64,21 @@ def roles(user, context = nil)
 
   # Normalizes the different types of permission arguments into Permission objects.
   # Possible arguments
-  #  - Symbol permission names (e.g. :view_work_packages)
+  #  - Symbol permission names (e.g. :view_work_packages, or [:edit_work_packages, :change_work_package_status])
   #  - Hash with :controller and :action (e.g. { controller: 'work_packages', action: 'show' })
   #
   # Exceptions
   #  - When there is no permission matching the +action+ parameter, either an empty array is returned
   #    or an +UnknownPermissionError+ is raised (depending on the +raise_on_unknown+ parameter).
   # .  Additionally a debugger message is logged.
   #    If the permission is disabled, it will not raise an error or log debug output.
-  def permissions_for(action, raise_on_unknown: false) # rubocop:disable Metrics/PerceivedComplexity, Metrics/AbcSize
-    return [action] if action.is_a?(OpenProject::AccessControl::Permission)
-    return action if action.is_a?(Array) && (action.empty? || action.all?(OpenProject::AccessControl::Permission))
+  def permissions_for(action, raise_on_unknown: false)
+    return Array(action) if Array(action).all?(OpenProject::AccessControl::Permission)
 
     perms = if action.is_a?(Hash)
               OpenProject::AccessControl.allow_actions(action)
             else
-              [OpenProject::AccessControl.permission(action)].compact
+              Array(action).filter_map { |a| OpenProject::AccessControl.permission(a) }
             end
 
     if perms.blank?

config/initializers/permissions.rb

@@ -323,6 +323,12 @@
                      permissible_on: :project,
                      dependencies: :view_work_packages
 
+      # WP status can be changed with :edit_work_packages, this permission allows it without Edit WP as well.
+      wpt.permission :change_work_package_status,
+                     {},
+                     permissible_on: :project,
+                     dependencies: :view_work_packages
+
       # A user having the following permission can become assignee and/or responsible of a work package.
       # This is a passive permission in the sense that a user having the permission isn't eligible to perform
       # actions but rather to have actions taken together with him/her.

config/locales/en.yml

@@ -2562,6 +2562,8 @@ en:
   permission_assign_versions: "Assign versions"
   permission_browse_repository: "Read-only access to repository (browse and checkout)"
   permission_change_wiki_parent_page: "Change parent wiki page"
+  permission_change_work_package_status: "Change work package status"
+  permission_change_work_package_status_explanation: "Allows changing status without Edit work packages permission"
   permission_comment_news: "Comment news"
   permission_commit_access: "Read/write access to repository (commit)"
   permission_copy_projects: "Copy projects"

docs/api/apiv3/paths/work_package.yml

@@ -169,7 +169,7 @@ patch:
       description: |-
         Returned if the client does not have sufficient permissions.
 
-        **Required permission:** edit work package, assign version, manage subtasks or move work package
+        **Required permission:** edit work package, assign version, change work package status, manage subtasks or move work package
     '404':
       content:
         application/hal+json:

docs/api/apiv3/paths/work_package_form.yml

@@ -42,7 +42,7 @@ post:
       description: |-
         Returned if the client does not have sufficient permissions.
 
-        **Required permission:** edit work package, assign version, manage subtasks or move work package
+        **Required permission:** edit work package, assign version, change work package status, manage subtasks or move work package
 
         *Note that you will only receive this error, if you are at least allowed to see the corresponding work package.*
       headers: {}

lib/api/v3/work_packages/work_package_representer.rb

@@ -583,6 +583,7 @@ def current_user_watcher?
         def current_user_update_allowed?
           @current_user_update_allowed ||=
             current_user.allowed_in_work_package?(:edit_work_packages, represented) ||
+              current_user.allowed_in_project?(:change_work_package_status, represented.project) ||
               current_user.allowed_in_project?(:assign_versions, represented.project)
         end
 

modules/boards/spec/features/board_management_spec.rb

@@ -236,7 +236,7 @@
     end
   end
 
-  context 'with view boards + edit work package permission' do
+  context 'with view boards + edit work package and change work package status permissions' do
     let(:permissions) { %i[show_board_views view_work_packages add_work_packages edit_work_packages] }
     let(:board_view) { create(:board_grid_with_queries, project:) }
 

spec/features/work_packages/edit_on_change_work_package_status_permission_spec.rb

@@ -0,0 +1,98 @@
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2024 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+require 'spec_helper'
+require 'features/page_objects/notification'
+
+RSpec.describe 'edit work package', :js do
+  let(:current_user) do
+    create(:user,
+           firstname: 'Dev',
+           lastname: 'Guy',
+           member_with_roles: { project => role })
+  end
+  let(:permissions) { %i[view_work_packages change_work_package_status] }
+  let(:role) { create(:project_role, permissions: permissions) }
+
+  let(:type) { create(:type) }
+  let(:project) { create(:project, types: [type]) }
+  let(:status_new) { create(:status, name: 'New') }
+  let(:status_done) { create(:status, name: 'Done') }
+  let(:workflow) do
+    create(:workflow,
+           type_id: type.id,
+           old_status: status_new,
+           new_status: status_done,
+           role: role)
+  end
+  let(:work_package) do
+    create(:work_package,
+           author: current_user,
+           status: status_new,
+           project:,
+           type:,
+           created_at: 5.days.ago.to_date.to_fs(:db))
+  end
+
+  let(:wp_page) { Pages::FullWorkPackage.new(work_package) }
+
+  before do
+    workflow
+
+    login_as(current_user)
+    wp_page.visit!
+    wp_page.ensure_page_loaded
+  end
+
+  context 'as a user having only the change_work_package_status permission' do
+    it 'can only change the status' do
+      status_field = wp_page.edit_field :status
+      status_field.expect_state_text status_new.name
+      status_field.update status_done.name
+
+      wp_page.expect_toast(message: 'Successful update')
+      status_field.expect_state_text status_done.name
+
+      subject_field = wp_page.work_package_field('subject')
+      subject_field.expect_read_only
+    end
+  end
+
+  context 'as a user having only the edit_work_packages permission' do
+    let(:permissions) { %i[view_work_packages edit_work_packages] }
+
+    it 'can change the status' do
+      status_field = wp_page.edit_field :status
+      status_field.expect_state_text status_new.name
+      status_field.update status_done.name
+
+      wp_page.expect_toast(message: 'Successful update')
+      status_field.expect_state_text status_done.name
+    end
+  end
+end

spec/lib/api/contracts/model_contract_spec.rb

@@ -37,6 +37,7 @@
            not_allowed: nil,
            changed: [],
            valid?: true,
+           new_record?: false,
            errors: ActiveModel::Errors.new(nil))
   end
 

spec/lib/api/v3/work_packages/schema/work_package_schema_representer_spec.rb

@@ -864,20 +864,39 @@
     end
 
     describe 'status' do
-      it_behaves_like 'has basic schema properties' do
-        let(:path) { 'status' }
-        let(:type) { 'Status' }
-        let(:name) { I18n.t('attributes.status') }
-        let(:required) { true }
-        let(:writable) { true }
-        let(:has_default) { true }
-        let(:location) { '_links' }
+      context 'if having the change_work_package_status permission' do
+        let(:permissions) { [:change_work_package_status] }
+
+        it_behaves_like 'has basic schema properties' do
+          let(:path) { 'status' }
+          let(:type) { 'Status' }
+          let(:name) { I18n.t('attributes.status') }
+          let(:required) { true }
+          let(:writable) { true }
+          let(:has_default) { true }
+          let(:location) { '_links' }
+        end
+
+        it_behaves_like 'has a collection of allowed values' do
+          let(:json_path) { 'status' }
+          let(:href_path) { 'statuses' }
+          let(:factory) { :status }
+        end
       end
 
-      it_behaves_like 'has a collection of allowed values' do
-        let(:json_path) { 'status' }
-        let(:href_path) { 'statuses' }
-        let(:factory) { :status }
+      # Just edit_work_packages without change_work_package_status still makes status writable:
+      context 'if having the edit_work_packages permission' do
+        let(:permissions) { [:edit_work_packages] }
+
+        it_behaves_like 'has basic schema properties' do
+          let(:path) { 'status' }
+          let(:type) { 'Status' }
+          let(:name) { I18n.t('attributes.status') }
+          let(:required) { true }
+          let(:writable) { true }
+          let(:has_default) { true }
+          let(:location) { '_links' }
+        end
       end
     end
 

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

@@ -466,7 +466,7 @@
           end
         end
 
-        context 'when user is lacks edit permission but has assign_versions' do
+        context 'when user lacks edit permission but has assign_versions' do
           let(:permissions) { all_permissions - [:edit_work_packages] + [:assign_versions] }
 
           it_behaves_like 'has an untitled link' do
@@ -479,6 +479,20 @@
             let(:href) { api_v3_paths.work_package(work_package.id) }
           end
         end
+
+        context 'when user lacks edit permission but has change_work_package_status' do
+          let(:permissions) { all_permissions - [:edit_work_packages] + [:change_work_package_status] }
+
+          it_behaves_like 'has an untitled link' do
+            let(:link) { 'update' }
+            let(:href) { api_v3_paths.work_package_form(work_package.id) }
+          end
+
+          it_behaves_like 'has an untitled link' do
+            let(:link) { 'updateImmediately' }
+            let(:href) { api_v3_paths.work_package(work_package.id) }
+          end
+        end
       end
 
       describe 'status' do

spec/models/mail_handler_spec.rb

@@ -324,9 +324,8 @@
   end
 
   shared_context 'with a new work package with attachment' do
-    # The edit_work_packages is currently wrongfully needed as the work package
-    # is created first and only then the attachment is added.
-    let(:permissions) { %i[add_work_packages edit_work_packages] }
+    # The work package is created first and only then the attachment is added.
+    let(:permissions) { %i[add_work_packages add_work_package_attachments] }
     let!(:user) do
       create(:user,
              mail: '[email protected]',
@@ -342,9 +341,8 @@
   end
 
   shared_context 'with a new work package with attachment in apple format' do
-    # The edit_work_packages is currently wrongfully needed as the work package
-    # is created first and only then the attachment is added.
-    let(:permissions) { %i[add_work_packages edit_work_packages] }
+    # The work package is created first and only then the attachment is added.
+    let(:permissions) { %i[add_work_packages add_work_package_attachments] }
     let!(:user) do
       create(:user,
              mail: '[email protected]',

spec/services/authorization_spec.rb

@@ -138,6 +138,17 @@
       end
     end
 
+    context 'when action is an array of permission names' do
+      let(:action) { %i[view_work_packages edit_work_packages] }
+
+      it 'returns the Permission object wrapped in an array' do
+        expect(subject).to eq([
+                                OpenProject::AccessControl.permission(:view_work_packages),
+                                OpenProject::AccessControl.permission(:edit_work_packages)
+                              ])
+      end
+    end
+
     context 'when there is a permission but it is disabled' do
       let(:permission_object) { OpenProject::AccessControl.permission(:manage_user) }
       let(:action) { permission_object.name }