simplify assignability check in contract

By extracting the assignable scope from the contract, the setup becomes simpler. This then lead to merging two validations into one.

It also contains a drive by fix in the sprints api where only non completed sprints would be visible.

Author: ulferts

modules/backlogs/app/models/agile/sprint.rb

@@ -45,7 +45,8 @@ class Sprint < ApplicationRecord
              inverse_of: :linked,
              dependent: :nullify
 
-    scopes :for_project,
+    scopes :assignable,
+           :for_project,
            :not_completed,
            :order_by_date,
            :receiving_projects,

modules/backlogs/app/models/agile/sprints/scopes/assignable.rb

@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+# -- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 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 Agile::Sprints::Scopes::Assignable
+  extend ActiveSupport::Concern
+
+  class_methods do
+    def assignable(project:, user: User.current)
+      for_project(project)
+        .visible(user)
+        .not_completed
+    end
+  end
+end

modules/backlogs/config/locales/en.yml

@@ -80,7 +80,7 @@ en:
             version_id:
               task_version_must_be_the_same_as_story_version: "must be the same as the parent story's version."
             sprint:
-              not_shared_with_project: "is not shared with the project the work package is in."
+              not_assignable: "is not assignable since it is either not shared with the project or already finished."
               not_eligible_for_moving: "is not an active sprint in the project which holds the sprint the work package is moved out of."
         agile/sprint:
           attributes:

modules/backlogs/lib/api/v3/sprints/sprints_by_project_api.rb

@@ -40,7 +40,7 @@ class SprintsByProjectAPI < ::API::OpenProjectAPI
           get &::API::V3::Utilities::Endpoints::Index
                  .new(
                    model: Agile::Sprint,
-                   scope: -> { @project.assignable_sprints }
+                   scope: -> { Agile::Sprint.for_project(@project).visible }
                  )
                  .mount
         end

modules/backlogs/lib/open_project/backlogs/patches/base_contract_patch.rb

@@ -42,11 +42,14 @@ module OpenProject::Backlogs::Patches::BaseContractPatch
 
     validate :backlog_bucket_xor_sprint
     validate :backlog_bucket_belongs_to_project
-    validate :sprint_shared_with_project
     validate :validate_sprint_is_assignable
 
     def assignable_sprints
-      model.try(:assignable_sprints)
+      if model.project
+        Agile::Sprint.assignable(project: model.project, user:)
+      else
+        Agile::Sprint.none
+      end
     end
 
     private
@@ -65,16 +68,11 @@ def backlog_bucket_belongs_to_project
     end
 
     def validate_sprint_is_assignable
-      if model.sprint_id && model.sprint_id_changed? && model.assignable_sprints.map(&:id).exclude?(model.sprint_id)
-        errors.add :sprint_id, :inclusion
+      if model.sprint_id &&
+         model.sprint_id_changed? &&
+         !assignable_sprints.exists?(id: model.sprint_id)
+        errors.add :sprint, :not_assignable
       end
     end
-
-    def sprint_shared_with_project
-      return if model.sprint.nil? ||
-                Agile::Sprint.for_project(model.project).exists?(id: model.sprint_id)
-
-      errors.add :sprint, :not_shared_with_project
-    end
   end
 end

modules/backlogs/lib/open_project/backlogs/patches/project_patch.rb

@@ -41,10 +41,6 @@ module OpenProject::Backlogs::Patches::ProjectPatch
   def backlogs_enabled?
     module_enabled? "backlogs"
   end
-
-  def assignable_sprints
-    @assignable_sprints ||= Agile::Sprint.for_project(self).visible.not_completed
-  end
 end
 
 Project.include OpenProject::Backlogs::Patches::ProjectPatch

modules/backlogs/lib/open_project/backlogs/patches/work_package_patch.rb

@@ -63,10 +63,6 @@ def done?
     def backlogs_enabled?
       project&.backlogs_enabled?
     end
-
-    def assignable_sprints
-      project.try(:assignable_sprints)
-    end
   end
 end
 

modules/backlogs/spec/contracts/work_packages/create_contract_spec.rb

@@ -55,10 +55,5 @@
     ]
   end
 
-  before do
-    allow(work_package_project).to receive(:assignable_sprints)
-                                     .and_return(shared_sprints)
-  end
-
   it_behaves_like "work package contract with backlogs extensions"
 end

modules/backlogs/spec/contracts/work_packages/shared_contract_examples.rb

@@ -39,7 +39,7 @@
   let(:work_package_story_points) { 5 }
   let(:work_package_sprint) { build_stubbed(:agile_sprint) }
   let(:work_package_position) { 5 }
-  let(:shared_sprints) { [work_package_sprint] }
+  let(:assignable_sprints) { [work_package_sprint] }
   let(:backlogs_enabled) { true }
   let(:work_package_project) do
     build_stubbed(:project, types: [work_package_type]) do |project|
@@ -59,16 +59,16 @@
   let(:effective_permissions) { permissions }
 
   before do
-    shared_sprints_scope = instance_double(ActiveRecord::Relation)
+    assignable_sprints_scope = instance_double(ActiveRecord::Relation)
 
     allow(Agile::Sprint)
-      .to receive(:for_project)
-            .with(work_package.project)
-            .and_return(shared_sprints_scope)
+      .to receive(:assignable)
+            .with(project: work_package.project, user:)
+            .and_return(assignable_sprints_scope)
 
-    allow(shared_sprints_scope)
+    allow(assignable_sprints_scope)
       .to receive(:exists?) do |id:|
-      shared_sprints.map(&:id).include?(id.to_i)
+      assignable_sprints.map(&:id).include?(id.to_i)
     end
   end
 
@@ -119,25 +119,10 @@
       it_behaves_like "contract is valid"
     end
 
-    context "when sprint is set to a sprint not shared with the wp's project" do
-      let(:shared_sprints) { [] }
+    context "when sprint is set to a sprint not assignable" do
+      let(:assignable_sprints) { [] }
 
-      it_behaves_like "contract is invalid", sprint: :not_shared_with_project
-    end
-
-    context "when sprint is completed (shared with project but not assignable)" do
-      let(:completed_sprint) { build_stubbed(:agile_sprint, status: :completed) }
-      let(:work_package_sprint) { completed_sprint }
-
-      before do
-        # The sprint is still shared with the project (the outer before mock makes
-        # `Agile::Sprint.for_project.exists?` return true), so `sprint_shared_with_project`
-        # passes. We stub assignable_sprints to exclude it, simulating the `.not_completed`
-        # scope, so only `validate_sprint_is_assignable` fires.
-        allow(work_package_project).to receive(:assignable_sprints).and_return([])
-      end
-
-      it_behaves_like "contract is invalid", sprint_id: :inclusion
+      it_behaves_like "contract is invalid", sprint: :not_assignable
     end
 
     context "when sprint is set while the user lacks the :manage_sprint_items permission" do

modules/backlogs/spec/contracts/work_packages/update_contract_spec.rb

@@ -53,9 +53,6 @@
   end
 
   before do
-    allow(work_package_project).to receive(:assignable_sprints)
-                        .and_return(shared_sprints)
-
     visible_scope = instance_double(ActiveRecord::Relation)
 
     allow(WorkPackage)
@@ -95,16 +92,14 @@
                         story_points: :error_readonly
       end
 
-      context "when sprint is completed (shared with project but not assignable) but the assignment did not change" do
+      context "when sprint is not assignable but the assignment did not change" do
         let(:completed_sprint) { build_stubbed(:agile_sprint, status: :completed) }
         let(:work_package_sprint) { completed_sprint }
+        let(:assignable_sprints) { [] }
 
         before do
           # So that the changes look like they came out of the database
           work_package.clear_changes_information
-
-          # The mocks in the shared context need to be taken into account.
-          allow(work_package_project).to receive(:assignable_sprints).and_return([])
         end
 
         it_behaves_like "contract is valid"