Fix archive project through API

The `Projects::UpdateService` used by the API endpoint was duplicating
the checks done in `ArchiveContract` and `UnarchiveContract`. Now the
relevant contract is called and used instead. As the duplication has
been removed, the class `Projects::Archiver` is not needed anymore and
its methods have been inlined.

Also, as project is not active when being archived, all permission
checks would fail because no action is allowed on archived projects. So
the condition was relaxed a little to also allow permission check on
projects being archived.

app/contracts/concerns/requires_admin_guard.rb

@@ -30,15 +30,11 @@ module RequiresAdminGuard
   extend ActiveSupport::Concern
 
   included do
-    validate { validate_admin_only(user, errors) }
+    validate :validate_admin_only
   end
 
-  module_function
-
   # Adds an error if user is archived or not an admin.
-  #
+  def validate_admin_only
-  # Can be used from outside like +RequiresAdminGuard.validate_admin_only(user, errors)+
-  def validate_admin_only(user, errors)
     unless user.admin? && user.active?
       errors.add :base, :error_unauthorized
     end

app/contracts/projects/archive_contract.rb

@@ -27,15 +27,24 @@
 #++
 
 module Projects
-  class ArchiveContract < ModelContract
+  class ArchiveContract < ::BaseContract
-    include Projects::Archiver
-
     validate :validate_no_foreign_wp_references
     validate :validate_has_archive_project_permission
-    validate :validate_has_archive_project_permission
 
     protected
 
+    # Check that there is no wp of a non descendant project that is assigned
+    # to one of the project or descendant versions
+    def validate_no_foreign_wp_references
+      version_ids = model.rolled_up_versions.select(:id)
+
+      exists = WorkPackage
+                 .where.not(project_id: model.self_and_descendants.select(:id))
+                 .exists?(version_id: version_ids)
+
+      errors.add :base, :foreign_wps_reference_version if exists
+    end
+
     def validate_has_archive_project_permission
       validate_can_archive_project
       validate_can_archive_subprojects
@@ -57,9 +66,5 @@ module Projects
         errors.add :base, :archive_permission_missing_on_subprojects
       end
     end
-
-    def validate_model?
-      false
-    end
   end
 end

app/contracts/projects/archiver.rb

@@ -1,50 +0,0 @@
-#-- copyright
-# OpenProject is an open source project management software.
-# Copyright (C) 2012-2023 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.
-#++
-
-module Projects
-  module Archiver
-    # Check that there is no wp of a non descendant project that is assigned
-    # to one of the project or descendant versions
-    def validate_no_foreign_wp_references
-      version_ids = model.rolled_up_versions.select(:id)
-
-      exists = WorkPackage
-                 .where.not(project_id: model.self_and_descendants.select(:id))
-                 .where(version_id: version_ids)
-                 .exists?
-
-      errors.add :base, :foreign_wps_reference_version if exists
-    end
-
-    def validate_all_ancestors_active
-      if model.ancestors.any?(&:archived?)
-        errors.add :base, :archived_ancestor
-      end
-    end
-  end
-end

app/contracts/projects/base_contract.rb

@@ -30,7 +30,6 @@ module Projects
   class BaseContract < ::ModelContract
     include AssignableValuesContract
     include AssignableCustomFieldValues
-    include Projects::Archiver
 
     attribute :name
     attribute :identifier
@@ -131,14 +130,11 @@ module Projects
     def validate_changing_active
       return unless model.active_changed?
 
-      RequiresAdminGuard.validate_admin_only(user, errors)
+      contract_klass = model.being_archived? ? ArchiveContract : UnarchiveContract
+      contract = contract_klass.new(model, user)
+      contract.validate
 
-      if model.active?
+      errors.merge!(contract.errors)
-        # switched to active -> unarchiving
-        validate_all_ancestors_active
-      else
-        validate_no_foreign_wp_references
-      end
     end
   end
 end

app/contracts/projects/unarchive_contract.rb

@@ -27,16 +27,17 @@
 #++
 
 module Projects
-  class UnarchiveContract < ModelContract
+  class UnarchiveContract < ::BaseContract
     include RequiresAdminGuard
-    include Projects::Archiver
 
     validate :validate_all_ancestors_active
 
     protected
 
-    def validate_model?
+    def validate_all_ancestors_active
-      false
+      if model.ancestors.any?(&:archived?)
+        errors.add :base, :archived_ancestor
+      end
     end
   end
 end

app/models/project.rb

@@ -162,6 +162,10 @@ class Project < ApplicationRecord
     !active?
   end
 
+  def being_archived?
+    (active == false) && (active_was == true)
+  end
+
   def copy_allowed?
     User.current.allowed_to?(:copy_projects, self)
   end

app/models/users/project_role_cache.rb

@@ -40,8 +40,9 @@ class Users::ProjectRoleCache
   private
 
   def roles(project)
-    # No role on archived projects
+    # Project is nil if checking global role
-    return [] unless !project || project&.active?
+    # No roles on archived projects, unless the active state is being changed
+    return [] if project && archived?(project)
 
     # Return all roles if user is admin
     return all_givable_roles if user.admin?
@@ -56,4 +57,11 @@ class Users::ProjectRoleCache
   def all_givable_roles
     @all_givable_roles ||= Role.givable.to_a
   end
+
+  def archived?(project)
+    # project for which activity is being changed is still considered active
+    return false if project.being_archived?
+
+    project.archived?
+  end
 end

app/services/authorization/user_allowed_service.rb

@@ -79,7 +79,7 @@ class Authorization::UserAllowedService
     end
 
     # No action allowed on archived projects
-    return false unless project.active?
+    return false unless project.active? || project.being_archived?
     # No action allowed on disabled modules
     return false unless project.allows_to?(action)
     # Inactive users are never authorized

app/services/projects/update_service.rb

@@ -90,17 +90,19 @@ module Projects
     def handle_archiving
       return unless model.saved_change_to_active?
 
-      if model.active?
+      service_class =
-        # was unarchived
+        if model.active?
-        Projects::UnarchiveService
+          # was unarchived
-          .new(user:, model:)
+          Projects::UnarchiveService
-          .call
+        else
-      else
+          # was archived
-        # as archived
+          Projects::ArchiveService
-        Projects::ArchiveService
+        end
-          .new(user:, model:)
+
-          .call
+      # EmptyContract is used because archive/unarchive conditions have
-      end
+      # already been checked in Projects::UpdateContract
+      service = service_class.new(user:, model:, contract_class: EmptyContract)
+      service.call
     end
   end
 end

spec/models/project_spec.rb

@@ -55,18 +55,54 @@ describe Project do
   end
 
   describe '#archived?' do
-    context 'if archived' do
+    subject { project.archived? }
-      it 'is true' do
+
-        expect(project).not_to be_archived
+    context 'if active is true' do
+      let(:active) { true }
+
+      it { is_expected.to be false }
+    end
+
+    context 'if active is false' do
+      let(:active) { false }
+
+      it { is_expected.to be true }
+    end
+  end
+
+  describe '#being_archived?' do
+    subject { project.being_archived? }
+
+    context 'if active is true' do
+      let(:active) { true }
+
+      it { is_expected.to be false }
+    end
+
+    context 'if active was true and changes to false (marking as archived)' do
+      let(:active) { true }
+
+      before do
+        project.active = false
       end
+
+      it { is_expected.to be true }
     end
 
-    context 'if not archived' do
+    context 'if active is false' do
       let(:active) { false }
 
-      it 'is false' do
+      it { is_expected.to be false }
-        expect(project).to be_archived
+    end
+
+    context 'if active was false and changes to true (marking as active)' do
+      let(:active) { false }
+
+      before do
+        project.active = true
       end
+
+      it { is_expected.to be false }
     end
   end
 

spec/requests/api/v3/projects/update_resource_spec.rb

@@ -268,6 +268,12 @@ describe 'API v3 Project resource update', type: :request, content_type: :json d
   end
 
   context 'when deactivating (archiving) the project' do
+    let(:body) do
+      {
+        active: false
+      }
+    end
+
     context 'for an admin' do
       let(:current_user) do
         create(:admin)
@@ -281,12 +287,6 @@ describe 'API v3 Project resource update', type: :request, content_type: :json d
         create(:project)
       end
 
-      let(:body) do
-        {
-          active: false
-        }
-      end
-
       it 'responds with 200 OK' do
         expect(last_response.status)
           .to be(200)
@@ -304,12 +304,6 @@ describe 'API v3 Project resource update', type: :request, content_type: :json d
     end
 
     context 'for a non admin' do
-      let(:body) do
-        {
-          active: false
-        }
-      end
-
       it 'responds with 403' do
         expect(last_response.status)
           .to be(403)

spec/services/authorization/user_allowed_service_spec.rb

@@ -137,6 +137,7 @@ describe Authorization::UserAllowedService do
       before do
         Array(context).each do |project|
           project.active = false
+          project.clear_changes_information
         end
       end
 
@@ -151,6 +152,18 @@ describe Authorization::UserAllowedService do
       end
     end
 
+    context 'with the project being archived' do
+      before do
+        Array(context).each do |project|
+          project.active = false
+        end
+      end
+
+      it 'is true' do
+        expect(subject).to be_truthy
+      end
+    end
+
     context 'with the project not having the action enabled' do
       let(:project_allows_to) { false }