Bug/73750 work packages can be assigned to closed sprints

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: -> { Agile::Sprint.for_project(@project) }
+                   scope: -> { @project.assignable_sprints }
                  )
                  .mount
         end

modules/backlogs/lib/open_project/backlogs/engine.rb

@@ -139,6 +139,7 @@ def self.settings
     patch_with_namespace :WorkPackages, :BaseContract
     patch_with_namespace :WorkPackages, :UpdateContract
     patch_with_namespace :API, :V3, :WorkPackages, :EagerLoading, :Checksum
+    patch_with_namespace :API, :V3, :WorkPackages, :Schema, :SpecificWorkPackageSchema
     patch_with_namespace :API, :V3, :Utilities, :ResourceLinkGenerator
 
     config.to_prepare do

modules/backlogs/lib/open_project/backlogs/patches/api/v3/work_packages/schema/specific_work_package_schema_patch.rb

@@ -0,0 +1,37 @@
+# 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 OpenProject::Backlogs::Patches::API::V3::WorkPackages::Schema::SpecificWorkPackageSchemaPatch
+  extend ActiveSupport::Concern
+
+  included do
+    delegate :assignable_sprints, to: :contract
+  end
+end

modules/backlogs/lib/open_project/backlogs/patches/api/work_package_schema_representer.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 #-- copyright
 # OpenProject is an open source project management software.
 # Copyright (C) the OpenProject GmbH

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

@@ -43,6 +43,11 @@ 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)
+    end
 
     private
 
@@ -59,6 +64,12 @@ def backlog_bucket_belongs_to_project
       errors.add :backlog_bucket, :backlog_bucket_from_another_project
     end
 
+    def validate_sprint_is_assignable
+      if model.sprint_id && model.assignable_sprints.map(&:id).exclude?(model.sprint_id)
+        errors.add :sprint_id, :inclusion
+      end
+    end
+
     def sprint_shared_with_project
       return if model.sprint.nil? ||
                 Agile::Sprint.for_project(model.project).exists?(id: model.sprint_id)

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

@@ -41,6 +41,10 @@ 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,6 +63,10 @@ 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

@@ -51,8 +51,14 @@
       view_work_packages
       add_work_packages
       manage_sprint_items
+      view_sprints
     ]
   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

@@ -125,6 +125,21 @@
       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
+    end
+
     context "when sprint is set while the user lacks the :manage_sprint_items permission" do
       let(:effective_permissions) { permissions - [:manage_sprint_items] }
 

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

@@ -48,10 +48,14 @@
       view_work_packages
       edit_work_packages
       manage_sprint_items
+      view_sprints
     ]
   end
 
   before do
+    allow(work_package_project).to receive(:assignable_sprints)
+                        .and_return(shared_sprints)
+
     visible_scope = instance_double(ActiveRecord::Relation)
 
     allow(WorkPackage)
@@ -68,7 +72,7 @@
     describe "validations" do
       context "when setting sprint and lock_version " \
               "and only having the manage_sprint_items permission but lacking edit_work_packages" do
-        let(:permissions) { %i[view_work_packages manage_sprint_items] }
+        let(:permissions) { %i[view_work_packages manage_sprint_items view_sprints] }
 
         before do
           # Reverting the change done in the setup
@@ -80,7 +84,7 @@
 
       context "when setting the sprint and another property " \
               "and only having the manage_sprint_items permission but lacking edit_work_packages" do
-        let(:permissions) { %i[view_work_packages manage_sprint_items] }
+        let(:permissions) { %i[view_work_packages manage_sprint_items view_sprints] }
 
         before do
           work_package.subject = "Some other subject"
@@ -94,7 +98,7 @@
 
     describe "writable_attributes" do
       context "when the user has only :manage_sprint_items permission but lacks :edit_work_packages" do
-        let(:permissions) { %i[view_work_packages manage_sprint_items] }
+        let(:permissions) { %i[view_work_packages manage_sprint_items view_sprints] }
 
         it "includes sprint, backlog_bucket and lock_version", :aggregate_failures do
           expect(contract.writable_attributes).to include("backlog_bucket", "sprint", "lock_version")

modules/backlogs/spec/features/inbox_column_spec.rb

@@ -279,6 +279,28 @@
         planning_page.expect_story_in_sprint(inbox_wp1, sprint)
         planning_page.expect_work_packages_in_sprint_in_order(sprint, work_packages: [sprint_wp, inbox_wp1])
       end
+
+      context "when the target sprint is completed (race condition #73750)" do
+        it "shows an error and does not move the item" do
+          planning_page.click_in_inbox_move_menu(inbox_wp1, "Move to sprint")
+
+          within("#move-to-sprint-dialog") do
+            expect(page).to have_select("target_id", with_options: ["Sprint 1", "Sprint 2"])
+            select sprint.name, from: "target_id"
+
+            # Before saving the selection, simulate that another user completed the sprint
+            sprint.completed!
+
+            click_button "Move"
+          end
+
+          planning_page.expect_and_dismiss_error("Update failed: Sprint is not set to one of the allowed values.")
+
+          # Item was *not* moved:
+          planning_page.expect_inbox_item(inbox_wp1)
+          planning_page.expect_story_not_in_sprint(inbox_wp1, sprint)
+        end
+      end
     end
 
     describe "moving backlog items to a sprint via drag-and-drop" do

modules/backlogs/spec/requests/api/v3/work_packages/create_resource_spec.rb

@@ -40,10 +40,11 @@
   shared_let(:status) { create(:status, is_default: true) }
   shared_let(:priority) { create(:priority, is_default: true) }
   shared_let(:sprint) { create(:agile_sprint, project:) }
+  shared_let(:completed_sprint) { create(:agile_sprint, project:, status: :completed) }
   shared_let(:outside_sprint) { create(:agile_sprint, project: create(:project)) }
 
   let(:role) { create(:project_role, permissions:) }
-  let(:permissions) { %i[add_work_packages view_work_packages manage_sprint_items] }
+  let(:permissions) { %i[add_work_packages view_work_packages manage_sprint_items view_sprints] }
 
   current_user do
     create(:user, member_with_roles: { project => role })
@@ -91,11 +92,43 @@
     end
 
     context "when the user does not have permission to manage sprint items" do
-      let(:permissions) { %i[add_work_packages view_work_packages] }
+      let(:permissions) { %i[add_work_packages view_work_packages view_sprints] }
 
       it_behaves_like "read-only violation", "sprint", WorkPackage
     end
 
+    context "when the user does not have permission to view sprints" do
+      let(:permissions) { %i[add_work_packages view_work_packages manage_sprint_items] }
+
+      it_behaves_like "constraint violation" do
+        let(:message) { "Sprint is not set to one of the allowed values." }
+      end
+    end
+
+    context "when attempting to create the work package on a completed sprint" do
+      let(:parameters) do
+        {
+          subject: "new work packages",
+          storyPoints: 5,
+          _links: {
+            type: {
+              href: api_v3_paths.type(type.id)
+            },
+            project: {
+              href: api_v3_paths.project(project.id)
+            },
+            sprint: {
+              href: api_v3_paths.sprint(completed_sprint.id)
+            }
+          }
+        }
+      end
+
+      it_behaves_like "constraint violation" do
+        let(:message) { "Sprint is not set to one of the allowed values." }
+      end
+    end
+
     context "when attempting to create the work package on a non valid sprint" do
       let(:parameters) do
         {
@@ -115,8 +148,11 @@
         }
       end
 
-      it_behaves_like "constraint violation" do
-        let(:message) { "Sprint is not shared with the project the work package is in." }
+      it_behaves_like "multiple errors of the same type with messages" do
+        let(:message) do
+          ["Sprint is not shared with the project the work package is in.",
+           "Sprint is not set to one of the allowed values."]
+        end
       end
     end
   end

modules/backlogs/spec/requests/api/v3/work_packages/update_resource_spec.rb

@@ -35,19 +35,23 @@
                content_type: :json do
   include API::V3::Utilities::PathHelper
 
-  shared_let(:project) { create(:project, public: false) }
+  shared_let(:project) { create(:project, public: false, enabled_module_names: %w[work_package_tracking backlogs]) }
+  shared_let(:other_project) { create(:project, enabled_module_names: %w[work_package_tracking backlogs]) }
   shared_let(:type) { project.types.first }
   shared_let(:status) { create(:status, is_default: true) }
   shared_let(:priority) { create(:priority, is_default: true) }
   shared_let(:sprint) { create(:agile_sprint, project:) }
-  shared_let(:outside_sprint) { create(:agile_sprint, project: create(:project)) }
+  shared_let(:completed_sprint) { create(:agile_sprint, project:, status: :completed) }
+  shared_let(:outside_sprint) { create(:agile_sprint, project: other_project) }
   shared_let(:work_package) { create(:work_package, project:, type:, status:, priority:) }
 
   let(:role) { create(:project_role, permissions:) }
-  let(:permissions) { %i[edit_work_packages view_work_packages manage_sprint_items] }
+  let(:permissions) { %i[edit_work_packages view_work_packages manage_sprint_items view_sprints] }
+  let(:other_role) { create(:project_role, permissions: other_permissions) }
+  let(:other_permissions) { permissions }
 
   current_user do
-    create(:user, member_with_roles: { project => role })
+    create(:user, member_with_roles: { project => role, other_project => other_role })
   end
 
   describe "PATCH /api/v3/work_packages/:id" do
@@ -82,13 +86,21 @@
     end
 
     context "when the user does not have permission to manage sprint items" do
-      let(:permissions) { %i[edit_work_packages view_work_packages] }
+      let(:permissions) { %i[edit_work_packages view_work_packages view_sprints] }
 
       it_behaves_like "read-only violation", "sprint", WorkPackage
     end
 
-    context "when the user has only the permission to manage sprint items and changes only the sprint" do
-      let(:permissions) { %i[view_work_packages manage_sprint_items] }
+    context "when the user does not have permission to view sprints" do
+      let(:permissions) { %i[edit_work_packages view_work_packages manage_sprint_items] }
+
+      it_behaves_like "constraint violation" do
+        let(:message) { "Sprint is not set to one of the allowed values." }
+      end
+    end
+
+    context "when the user has only the permission to manage sprint items/view sprints and changes only the sprint" do
+      let(:permissions) { %i[view_work_packages manage_sprint_items view_sprints] }
 
       let(:parameters) do
         {
@@ -109,8 +121,8 @@
       end
     end
 
-    context "when the user has only the permission to manage sprint items and changes more than the sprint" do
-      let(:permissions) { %i[view_work_packages manage_sprint_items] }
+    context "when the user has only the permission to manage sprint items/view sprints and changes more than the sprint" do
+      let(:permissions) { %i[view_work_packages manage_sprint_items view_sprints] }
 
       let(:parameters) do
         {
@@ -127,7 +139,26 @@
       it_behaves_like "read-only violation", "subject", WorkPackage
     end
 
+    context "when attempting to assign the work package to a completed sprint" do
+      let(:parameters) do
+        {
+          storyPoints: 5,
+          lockVersion: work_package.lock_version,
+          _links: {
+            sprint: {
+              href: api_v3_paths.sprint(completed_sprint.id)
+            }
+          }
+        }
+      end
+
+      it_behaves_like "constraint violation" do
+        let(:message) { "Sprint is not set to one of the allowed values." }
+      end
+    end
+
     context "when attempting to assign the work package to a non valid sprint" do
+      let(:other_permissions) { [] }
       let(:parameters) do
         {
           storyPoints: 5,
@@ -140,8 +171,11 @@
         }
       end
 
-      it_behaves_like "constraint violation" do
-        let(:message) { "Sprint is not shared with the project the work package is in." }
+      it_behaves_like "multiple errors of the same type with messages" do
+        let(:message) do
+          ["Sprint is not shared with the project the work package is in.",
+           "Sprint is not set to one of the allowed values."]
+        end
       end
     end
   end