[#73389] Align shared sprint behavior

myabc · myabc · commit 852206c0b6f4 · 2026-03-24T23:42:44.000Z
Align shared sprint behavior with the new controller scoping. This applies the review-driven follow-ups around shared sprint visibility, sprint planning routing, and controller behavior while keeping the legacy and Agile paths consistent. https://community.openproject.org/wp/73389
diff --git a/modules/backlogs/app/components/backlogs/backlog_component.rb b/modules/backlogs/app/components/backlogs/backlog_component.rb
@@ -82,7 +82,7 @@ def draggable_item_config(story)
       {
         draggable_id: story.id,
         draggable_type: "story",
-        drop_url: move_legacy_backlogs_project_sprint_story_path(project, sprint, story)
+        drop_url: move_backlogs_project_sprint_story_path(project, sprint, story)
       }
     end
   end
diff --git a/modules/backlogs/app/controllers/agile/sprint_planning_controller.rb b/modules/backlogs/app/controllers/agile/sprint_planning_controller.rb
@@ -39,7 +39,7 @@ class SprintPlanningController < BaseController
 
     def show
       if turbo_frame_request?
-        render partial: "sprint_planning_list", layout: false
+        render partial: "list", layout: false
       else
         render :show
       end
diff --git a/modules/backlogs/app/controllers/agile/sprints_controller.rb b/modules/backlogs/app/controllers/agile/sprints_controller.rb
@@ -126,13 +126,15 @@ def finish
 
     private
 
-    # Member actions receive :id, not :sprint_id
+    # Member actions receive :id, not :sprint_id.
+    # Scoped to sprint_source (not for_project) to prevent mutation of
+    # sprints that are merely visible via work-package references.
     def load_sprint
       sprint_id = params[:sprint_id] || params[:id]
       return unless sprint_id
 
       @sprint = ::Agile::Sprint
-        .for_project(@project)
+        .where(project: @project.sprint_source)
         .visible
         .find(sprint_id)
     end
diff --git a/modules/backlogs/app/controllers/agile/stories_controller.rb b/modules/backlogs/app/controllers/agile/stories_controller.rb
@@ -60,7 +60,7 @@ def load_project_sprint_and_story
         .for_project(@project)
         .visible
         .find(params[:sprint_id])
-      @story = ::Story.visible.find(params[:id])
+      @story = WorkPackage.visible.find(params[:id])
     end
 
     def move_story(move_attributes)
diff --git a/modules/backlogs/app/controllers/rb_sprints_controller.rb b/modules/backlogs/app/controllers/rb_sprints_controller.rb
@@ -81,7 +81,7 @@ def update_header_component_via_turbo_stream(state: :show)
   # Overrides load_sprint_and_project to load the sprint from :id instead of :sprint_id
   def load_sprint_and_project
     load_project
-    @sprint = Sprint.visible.find(params[:id])
+    @sprint = Sprint.visible.apply_to(@project).find(params[:id])
   end
 
   def sprint_params
diff --git a/modules/backlogs/app/views/agile/sprint_planning/_list.html.erb b/modules/backlogs/app/views/agile/sprint_planning/_list.html.erb
diff --git a/modules/backlogs/config/routes.rb b/modules/backlogs/config/routes.rb
@@ -87,8 +87,6 @@
               as: :details,
               work_package_split_view: true,
               defaults: { tab: :overview }
-
-          get :sprint_planning
         end
       end
 
@@ -107,7 +105,7 @@
 
         resources :stories, controller: :rb_stories, only: [] do
           member do
-            put :move_legacy
+            put :move
             post :reorder
           end
         end
diff --git a/modules/backlogs/lib/open_project/backlogs/engine.rb b/modules/backlogs/lib/open_project/backlogs/engine.rb
@@ -68,7 +68,7 @@ def self.settings
 
       project_module :backlogs, dependencies: :work_package_tracking do
         permission :view_sprints,
-                   { rb_master_backlogs: %i[index sprint_planning details],
+                   { rb_master_backlogs: %i[index details],
                      rb_sprints: %i[index show show_name],
                      rb_wikis: :show,
                      rb_stories: %i[index show],
@@ -90,23 +90,22 @@ def self.settings
                    require: :member
 
         permission :create_sprints,
-                   { rb_sprints: %i[new_dialog refresh_form create edit_name update edit_dialog update_agile_sprint],
+                   { rb_sprints: %i[edit_name update],
                      rb_wikis: %i[edit update],
                      "agile/sprints": %i[new_dialog refresh_form create edit_dialog update] },
                    permissible_on: :project,
                    require: :member,
                    dependencies: :view_sprints
 
         permission :start_complete_sprint,
-                   { rb_sprints: %i[start finish],
-                     "agile/sprints": %i[start finish] },
+                   { "agile/sprints": %i[start finish] },
                    permissible_on: :project,
                    require: :member,
                    dependencies: %i[view_sprints manage_board_views],
                    visible: -> { OpenProject::FeatureDecisions.scrum_projects_active? }
 
         permission :manage_sprint_items,
-                   { rb_stories: %i[move move_legacy reorder],
+                   { rb_stories: %i[move reorder],
                      "agile/stories": :move },
                    permissible_on: :project,
                    require: :member,
@@ -123,15 +122,15 @@ def self.settings
       # Menu items that are there when feature flag is active
       menu :project_menu,
            :backlogs,
-           { controller: "/rb_master_backlogs", action: :sprint_planning },
+           { controller: "/agile/sprint_planning", action: :show },
            if: Proc.new { |project| project.module_enabled?(:backlogs) && OpenProject::FeatureDecisions.scrum_projects_active? },
            caption: :project_module_backlogs,
            after: :work_packages,
            icon: "op-backlogs"
 
       menu :project_menu,
            :sprint_planning,
-           { controller: "/rb_master_backlogs", action: :sprint_planning },
+           { controller: "/agile/sprint_planning", action: :show },
            if: Proc.new { |project| project.module_enabled?(:backlogs) && OpenProject::FeatureDecisions.scrum_projects_active? },
            caption: :label_sprint_planning,
            parent: :backlogs
diff --git a/modules/backlogs/spec/components/backlogs/backlog_component_spec.rb b/modules/backlogs/spec/components/backlogs/backlog_component_spec.rb
@@ -129,7 +129,7 @@ def render_component
         story_row = page.find(".Box-row[id='story_#{story1.id}']")
         expect(story_row["data-draggable-id"]).to eq(story1.id.to_s)
         expect(story_row["data-draggable-type"]).to eq("story")
-        expect(story_row["data-drop-url"]).to end_with(move_legacy_backlogs_project_sprint_story_path(project, sprint, story1))
+        expect(story_row["data-drop-url"]).to end_with(move_backlogs_project_sprint_story_path(project, sprint, story1))
       end
 
       it "renders story rows with proper classes" do
diff --git a/modules/backlogs/spec/controllers/rb_sprints_controller_spec.rb b/modules/backlogs/spec/controllers/rb_sprints_controller_spec.rb
@@ -38,7 +38,7 @@
     current_user { user }
 
     let(:visible_projects_scope) { instance_double(ActiveRecord::Relation) }
-    let(:visible_sprints_scope) { instance_double(ActiveRecord::Relation) }
+    let(:visible_sprints_scope) { double("Sprint::ActiveRecord_Relation") } # rubocop:disable RSpec/VerifiedDoubles
 
     before do
       allow(Setting)
@@ -58,6 +58,11 @@
         .to receive(:visible)
         .and_return(visible_sprints_scope)
 
+      allow(visible_sprints_scope)
+        .to receive(:apply_to)
+        .with(project)
+        .and_return(visible_sprints_scope)
+
       allow(visible_sprints_scope)
         .to receive(:find)
         .with(sprint.id.to_s)
diff --git a/modules/backlogs/spec/controllers/rb_stories_controller_spec.rb b/modules/backlogs/spec/controllers/rb_stories_controller_spec.rb
@@ -51,19 +51,19 @@
             .and_return({ "story_types" => [type_feature.id], "task_type" => type_task.id })
   end
 
-  describe "PUT #move_legacy" do
+  describe "PUT #move" do
     context "with a user lacking project permission" do
       let(:user) { create(:user) }
 
       it "responds with 403" do
-        put :move_legacy, params: {
-                            project_id: project.id,
-                            sprint_id: version_sprint.id,
-                            id: story.id,
-                            target_id: "foo",
-                            position: 1
-                          },
-                          format: :turbo_stream
+        put :move, params: {
+                     project_id: project.id,
+                     sprint_id: version_sprint.id,
+                     id: story.id,
+                     target_id: "foo",
+                     position: 1
+                   },
+                   format: :turbo_stream
 
         expect(response).not_to be_successful
         expect(response).to have_http_status :not_found
@@ -74,14 +74,14 @@
       let(:other_version_sprint) { create(:sprint, name: "Sprint 2", project:) }
 
       it "responds with success", :aggregate_failures do
-        put :move_legacy, params: {
-                            project_id: project.id,
-                            sprint_id: version_sprint.id,
-                            id: story.id,
-                            target_id: "version:#{other_version_sprint.id}",
-                            position: 1
-                          },
-                          format: :turbo_stream
+        put :move, params: {
+                     project_id: project.id,
+                     sprint_id: version_sprint.id,
+                     id: story.id,
+                     target_id: "version:#{other_version_sprint.id}",
+                     position: 1
+                   },
+                   format: :turbo_stream
 
         expect(response).to be_successful
         expect(response).to have_http_status :ok
@@ -103,14 +103,14 @@
       let(:story) { create(:story, status:, version: other_version_sprint, project:) }
 
       it "responds with success", :aggregate_failures do
-        put :move_legacy, params: {
-                            project_id: project.id,
-                            sprint_id: other_version_sprint.id,
-                            id: story.id,
-                            target_id: "version:#{version_sprint.id}",
-                            position: 1
-                          },
-                          format: :turbo_stream
+        put :move, params: {
+                     project_id: project.id,
+                     sprint_id: other_version_sprint.id,
+                     id: story.id,
+                     target_id: "version:#{version_sprint.id}",
+                     position: 1
+                   },
+                   format: :turbo_stream
 
         expect(response).to be_successful
         expect(response).to have_http_status :ok
@@ -126,35 +126,6 @@
       end
     end
 
-    context "with an Agile::Sprint as target", with_flag: { scrum_projects: true } do
-      let(:agile_sprint) { create(:agile_sprint, name: "Agile Sprint 1", project:) }
-
-      it "responds with success and moves story to Agile::Sprint, removing the association to the version", :aggregate_failures do
-        put :move_legacy, params: {
-                            project_id: project.id,
-                            sprint_id: version_sprint.id,
-                            id: story.id,
-                            target_id: "sprint:#{agile_sprint.id}",
-                            position: 1
-                          },
-                          format: :turbo_stream
-
-        expect(response).to be_successful
-        expect(response).to have_http_status :ok
-        expect(response).to have_turbo_stream action: "replace", target: "backlogs-backlog-component-#{version_sprint.id}"
-        expect(response).to have_turbo_stream action: "replace", target: "backlogs-sprint-component-#{agile_sprint.id}"
-        assert_select %(turbo-stream[action="replace"][target="backlogs-backlog-component-#{version_sprint.id}"][method="morph"])
-        assert_select %(turbo-stream[action="replace"][target="backlogs-sprint-component-#{agile_sprint.id}"])
-        expect(response).to have_turbo_stream action: "flash", target: "op-primer-flash-component"
-        expect(assigns(:project)).to eq(project)
-        expect(assigns(:sprint)).to eq(version_sprint)
-        expect(assigns(:story)).to eq(story)
-
-        # It will remove the association to the version since the version is used as backlog:
-        expect(story.reload.version).to be_nil
-      end
-    end
-
     context "when service call fails" do
       let(:other_version_sprint) { create(:sprint, name: "Sprint 2", project:) }
       let(:service_result) { ServiceResult.failure(message: "Something went wrong") }
@@ -168,14 +139,14 @@
       end
 
       it "renders an error flash with 422", :aggregate_failures do
-        put :move_legacy, params: {
-                            project_id: project.id,
-                            sprint_id: version_sprint.id,
-                            id: story.id,
-                            target_id: "version:#{other_version_sprint.id}",
-                            position: 1
-                          },
-                          format: :turbo_stream
+        put :move, params: {
+                     project_id: project.id,
+                     sprint_id: version_sprint.id,
+                     id: story.id,
+                     target_id: "version:#{other_version_sprint.id}",
+                     position: 1
+                   },
+                   format: :turbo_stream
 
         expect(response).to have_http_status :unprocessable_entity
         expect(response).to have_turbo_stream action: "flash", target: "op-primer-flash-component"
diff --git a/modules/backlogs/spec/features/sprints/create_spec.rb b/modules/backlogs/spec/features/sprints/create_spec.rb
@@ -188,6 +188,23 @@
   end
 
   context "with the feature flag inactive" do
+    # Sprint planning URL doesn't exist when the flag is off — visit the legacy index
+    let(:planning_page) do
+      page_class = Class.new(Pages::Page) do
+        include Rails.application.routes.url_helpers
+
+        attr_reader :project
+
+        def initialize(project)
+          super()
+          @project = project
+        end
+
+        def path = backlogs_project_backlogs_path(project)
+      end
+      page_class.new(project)
+    end
+
     it "is missing the 'new sprint' button" do
       expect(page).to have_no_button "Create"
       expect(page).not_to have_test_selector("op-sprints--new-sprint-button")
diff --git a/modules/backlogs/spec/lib/open_project/backlogs/permissions_spec.rb b/modules/backlogs/spec/lib/open_project/backlogs/permissions_spec.rb
@@ -63,7 +63,7 @@
     end
 
     it "covers both start and finish sprint actions" do
-      expect(subject.controller_actions).to include("rb_sprints/start", "rb_sprints/finish")
+      expect(subject.controller_actions).to include("agile/sprints/start", "agile/sprints/finish")
     end
 
     context "when scrum_projects feature flag is active", with_flag: { scrum_projects: true } do
diff --git a/modules/backlogs/spec/routing/agile/sprint_planning_routing_spec.rb b/modules/backlogs/spec/routing/agile/sprint_planning_routing_spec.rb
@@ -64,12 +64,8 @@
     end
 
     context "with the feature flag inactive", with_flag: { scrum_projects: false } do
-      it "routes sprint_planning to legacy controller" do
-        expect(get("/projects/project_42/backlogs/sprint_planning")).to route_to(
-          controller: "rb_master_backlogs",
-          action: "sprint_planning",
-          project_id: "project_42"
-        )
+      it "does not route sprint_planning" do
+        expect(get("/projects/project_42/backlogs/sprint_planning")).not_to be_routable
       end
     end
   end
diff --git a/modules/backlogs/spec/routing/agile/stories_routing_spec.rb b/modules/backlogs/spec/routing/agile/stories_routing_spec.rb
@@ -45,7 +45,15 @@
     end
 
     context "with the feature flag inactive", with_flag: { scrum_projects: false } do
-      it { expect(put("/projects/project_42/sprints/21/stories/85/move")).not_to be_routable }
+      it "routes to legacy controller" do
+        expect(put("/projects/project_42/sprints/21/stories/85/move")).to route_to(
+          controller: "rb_stories",
+          action: "move",
+          project_id: "project_42",
+          sprint_id: "21",
+          id: "85"
+        )
+      end
     end
   end
 end
diff --git a/modules/backlogs/spec/routing/rb_master_backlogs_routing_spec.rb b/modules/backlogs/spec/routing/rb_master_backlogs_routing_spec.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 #-- copyright
 # OpenProject is an open source project management software.
 # Copyright (C) the OpenProject GmbH
@@ -36,13 +38,6 @@
                                                                project_id: "project_42")
     }
 
-    it {
-      route = "/projects/project_42/backlogs/sprint_planning"
-      expect(get(route)).to route_to(controller: "rb_master_backlogs",
-                                     action: "sprint_planning",
-                                     project_id: "project_42")
-    }
-
     it {
       expect(get("/projects/project_42/backlogs/details/33")).to route_to(
         controller: "rb_master_backlogs",
diff --git a/modules/backlogs/spec/routing/rb_stories_routing_spec.rb b/modules/backlogs/spec/routing/rb_stories_routing_spec.rb
@@ -33,9 +33,9 @@
 RSpec.describe RbStoriesController do
   describe "routing" do
     it {
-      expect(put("/projects/project_42/sprints/21/stories/85/move_legacy")).to route_to(
+      expect(put("/projects/project_42/sprints/21/stories/85/move")).to route_to(
         controller: "rb_stories",
-        action: "move_legacy",
+        action: "move",
         project_id: "project_42",
         sprint_id: "21",
         id: "85"

Original file line number	Diff line number	Diff line change
`@@ -82,7 +82,7 @@ def draggable_item_config(story)`
`82`	`82`	`{`
`83`	`83`	`draggable_id: story.id,`
`84`	`84`	`draggable_type: "story",`
`85`		`- drop_url: move_legacy_backlogs_project_sprint_story_path(project, sprint, story)`
	`85`	`+ drop_url: move_backlogs_project_sprint_story_path(project, sprint, story)`
`86`	`86`	`}`
`87`	`87`	`end`
`88`	`88`	`end`