diff --git a/modules/backlogs/lib/api/v3/sprints/sprints_by_project_api.rb b/modules/backlogs/lib/api/v3/sprints/sprints_by_project_api.rb index f5acfb5bfd5d..fe798ccc80c5 100644 --- a/modules/backlogs/lib/api/v3/sprints/sprints_by_project_api.rb +++ b/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 diff --git a/modules/backlogs/lib/open_project/backlogs/engine.rb b/modules/backlogs/lib/open_project/backlogs/engine.rb index e7d12298d045..9e72211079b7 100644 --- a/modules/backlogs/lib/open_project/backlogs/engine.rb +++ b/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 diff --git a/modules/backlogs/lib/open_project/backlogs/patches/api/v3/work_packages/schema/specific_work_package_schema_patch.rb b/modules/backlogs/lib/open_project/backlogs/patches/api/v3/work_packages/schema/specific_work_package_schema_patch.rb new file mode 100644 index 000000000000..4ed668650293 --- /dev/null +++ b/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 diff --git a/modules/backlogs/lib/open_project/backlogs/patches/api/work_package_schema_representer.rb b/modules/backlogs/lib/open_project/backlogs/patches/api/work_package_schema_representer.rb index 92f5a58f949f..1ec826a1c4e4 100644 --- a/modules/backlogs/lib/open_project/backlogs/patches/api/work_package_schema_representer.rb +++ b/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 diff --git a/modules/backlogs/lib/open_project/backlogs/patches/base_contract_patch.rb b/modules/backlogs/lib/open_project/backlogs/patches/base_contract_patch.rb index f805c1d5d8f3..3d5be1e80147 100644 --- a/modules/backlogs/lib/open_project/backlogs/patches/base_contract_patch.rb +++ b/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) diff --git a/modules/backlogs/lib/open_project/backlogs/patches/project_patch.rb b/modules/backlogs/lib/open_project/backlogs/patches/project_patch.rb index 7ef4c750f4ff..d7780f790990 100644 --- a/modules/backlogs/lib/open_project/backlogs/patches/project_patch.rb +++ b/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 diff --git a/modules/backlogs/lib/open_project/backlogs/patches/work_package_patch.rb b/modules/backlogs/lib/open_project/backlogs/patches/work_package_patch.rb index 2a8532a6e129..7c36d10a703c 100644 --- a/modules/backlogs/lib/open_project/backlogs/patches/work_package_patch.rb +++ b/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 diff --git a/modules/backlogs/spec/contracts/work_packages/create_contract_spec.rb b/modules/backlogs/spec/contracts/work_packages/create_contract_spec.rb index 7dcfc025236f..7766276f3e81 100644 --- a/modules/backlogs/spec/contracts/work_packages/create_contract_spec.rb +++ b/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 diff --git a/modules/backlogs/spec/contracts/work_packages/shared_contract_examples.rb b/modules/backlogs/spec/contracts/work_packages/shared_contract_examples.rb index 1fb898ddc57c..7a9492ec5bf4 100644 --- a/modules/backlogs/spec/contracts/work_packages/shared_contract_examples.rb +++ b/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] } diff --git a/modules/backlogs/spec/contracts/work_packages/update_contract_spec.rb b/modules/backlogs/spec/contracts/work_packages/update_contract_spec.rb index 19751a8ba1e7..e945861bf77b 100644 --- a/modules/backlogs/spec/contracts/work_packages/update_contract_spec.rb +++ b/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") diff --git a/modules/backlogs/spec/features/inbox_column_spec.rb b/modules/backlogs/spec/features/inbox_column_spec.rb index 27e87674e1ae..07a1d7e4592d 100644 --- a/modules/backlogs/spec/features/inbox_column_spec.rb +++ b/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 diff --git a/modules/backlogs/spec/requests/api/v3/work_packages/create_resource_spec.rb b/modules/backlogs/spec/requests/api/v3/work_packages/create_resource_spec.rb index 0618bb113be2..d697a95fd4f0 100644 --- a/modules/backlogs/spec/requests/api/v3/work_packages/create_resource_spec.rb +++ b/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 diff --git a/modules/backlogs/spec/requests/api/v3/work_packages/update_resource_spec.rb b/modules/backlogs/spec/requests/api/v3/work_packages/update_resource_spec.rb index e63c108ada65..fe151c809344 100644 --- a/modules/backlogs/spec/requests/api/v3/work_packages/update_resource_spec.rb +++ b/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