Merge pull request #23464 from opf/a-bit-of-cleanug-in-backlog-module

A bit of cleaning in backlog module

modules/backlogs/app/components/backlogs/backlog_component.html.erb

@@ -49,7 +49,7 @@ See COPYRIGHT and LICENSE files for more details.
         )
       end
 
-      if allow_backlog_bucket_creation?(project)
+      if backlog_bucket_creation_allowed?
         head.with_actions do
           render Primer::Beta::Button.new(
             tag: :a,
@@ -64,7 +64,7 @@ See COPYRIGHT and LICENSE files for more details.
             BacklogBucket.human_model_name
           end
         end
-      elsif allow_sprint_creation?(project)
+      elsif sprint_creation_allowed?
         # Ugly hack:
         # This button will not be rendered, but still takes up space to push the left side down a bit.
         # Remove as soon as the `+ Sprint` button moves from the Subhead into a SubHeader.

modules/backlogs/app/components/backlogs/inbox_component.rb

@@ -30,6 +30,7 @@
 
 module Backlogs
   class InboxComponent < ApplicationComponent
+    include OpPrimer::ComponentHelpers
     include OpTurbo::Streamable
     include CommonHelper
 

modules/backlogs/app/components/backlogs/sprint_component.rb

@@ -120,9 +120,5 @@ module Backlogs
     def show_burndown_link?
       sprint.active?
     end
-
-    def user_allowed?(permission)
-      current_user.allowed_in_project?(permission, project)
-    end
   end
 end

modules/backlogs/app/components/backlogs/sprints_component.html.erb

@@ -31,7 +31,7 @@ See COPYRIGHT and LICENSE files for more details.
   render(Primer::Beta::Subhead.new(hide_border: true, pb: 0)) do |head|
     head.with_heading(tag: :h3, size: :medium, font_weight: :bold) { Sprint.human_model_name.pluralize }
 
-    if allow_sprint_creation?(project)
+    if sprint_creation_allowed?
       head.with_actions do
         render Primer::Beta::Button.new(
           tag: :a,

modules/backlogs/app/components/backlogs/sprints_component.rb

@@ -52,7 +52,7 @@ module Backlogs
     private
 
     def blankslate_description
-      if allow_sprint_management?(project)
+      if sprint_management_allowed?
         description_with_settings_link
       else
         description
@@ -67,7 +67,7 @@ module Backlogs
 
       if project.receive_shared_sprints?
         t(".blankslate.receive_and_manage_description_html", settings_link:)
-      elsif allow_sprint_creation?(project)
+      elsif sprint_creation_allowed?
         t(".blankslate.create_and_manage_description_html", settings_link:)
       else
         t(".blankslate.manage_description_html", settings_link:)
@@ -77,7 +77,7 @@ module Backlogs
     def description
       if project.receive_shared_sprints?
         t(".blankslate.receive_description_text")
-      elsif allow_sprint_creation?(project)
+      elsif sprint_creation_allowed?
         t(".blankslate.create_description_text")
       else
         t(".blankslate.no_actions_description_text")

modules/backlogs/app/components/backlogs/work_package_card_list_item_component.rb

@@ -30,6 +30,8 @@
 
 module Backlogs
   class WorkPackageCardListItemComponent < OpenProject::Common::BorderBoxListComponent::WorkPackageItem
+    include CommonHelper
+
     private
 
     def build_card
@@ -37,7 +39,7 @@ module Backlogs
     end
 
     def draggable?
-      current_user.allowed_in_project?(:manage_sprint_items, project)
+      user_allowed?(:manage_sprint_items)
     end
 
     def split_url

modules/backlogs/app/components/backlogs/work_package_card_menu_component.rb

@@ -75,7 +75,7 @@ module Backlogs
     end
 
     def allowed_to_manage_sprint_items?
-      current_user.allowed_in_project?(:manage_sprint_items, project)
+      user_allowed?(:manage_sprint_items)
     end
 
     def build_move_menu(menu)

modules/backlogs/app/helpers/backlogs/common_helper.rb

@@ -30,17 +30,21 @@
 
 module Backlogs
   module CommonHelper
-    def allow_backlog_bucket_creation?(project)
-      current_user.allowed_in_project?(:create_sprints, project)
+    def user_allowed?(permission)
+      current_user.allowed_in_project?(permission, project)
     end
 
-    def allow_sprint_creation?(project)
-      allow_backlog_bucket_creation?(project) &&
+    def backlog_bucket_creation_allowed?
+      user_allowed?(:create_sprints)
+    end
+
+    def sprint_creation_allowed?
+      user_allowed?(:create_sprints) &&
         !project.receive_shared_sprints?
     end
 
-    def allow_sprint_management?(project)
-      current_user.allowed_in_project?(:share_sprint, project)
+    def sprint_management_allowed?
+      user_allowed?(:share_sprint)
     end
 
     def show_all_backlog

modules/backlogs/spec/services/backlogs/sprints/finish_service_spec.rb

@@ -68,7 +68,7 @@ RSpec.describe Backlogs::Sprints::FinishService do
     it "completes the sprint ignoring the closed work package", :aggregate_failures do
       expect(result).to be_success
       expect(sprint.reload).to be_completed
-      expect(closed_wp.reload.sprint).to eq(sprint)
+      expect(closed_wp.reload).to have_attributes(sprint:)
     end
   end
 
@@ -86,7 +86,7 @@ RSpec.describe Backlogs::Sprints::FinishService do
       expect(result).to be_success
       expect(sprint.reload).to be_completed
       # The WP is not moved — it stays in the completed sprint.
-      expect(done_like_wp.reload.sprint).to eq(sprint)
+      expect(done_like_wp.reload).to have_attributes(sprint:)
     end
   end
 
@@ -100,7 +100,7 @@ RSpec.describe Backlogs::Sprints::FinishService do
         expect(result).not_to be_success
         expect(result.includes_error?(:base, :unfinished_work_packages)).to be true
         expect(sprint.reload).to be_active
-        expect(open_wp.reload.sprint).to eq(sprint)
+        expect(open_wp.reload).to have_attributes(sprint:)
       end
     end
 
@@ -112,7 +112,7 @@ RSpec.describe Backlogs::Sprints::FinishService do
       it "moves the open work packages and completes the sprint", :aggregate_failures do
         expect(result).to be_success
         expect(sprint.reload).to be_completed
-        expect(open_wp.reload.sprint).to eq(target_sprint)
+        expect(open_wp.reload).to have_attributes(sprint: target_sprint)
       end
     end
 
@@ -125,7 +125,7 @@ RSpec.describe Backlogs::Sprints::FinishService do
       it "returns failure on the work package update and leaves the sprint active", :aggregate_failures do
         expect(result).not_to be_success
         expect(sprint.reload).to be_active
-        expect(open_wp.reload.sprint).to eq(sprint)
+        expect(open_wp.reload).to have_attributes(sprint:)
       end
     end
 
@@ -139,8 +139,7 @@ RSpec.describe Backlogs::Sprints::FinishService do
       it "unassigns from sprint, completes the sprint, and places WP before existing backlog items", :aggregate_failures do
         expect(result).to be_success
         expect(sprint.reload).to be_completed
-        expect(open_wp.reload.sprint).to be_nil
-        expect(open_wp.reload.position).to be < existing_backlog_wp.reload.position
+        expect(open_wp.reload).to have_attributes(sprint: nil, position: be < existing_backlog_wp.reload.position)
       end
     end
 
@@ -154,8 +153,7 @@ RSpec.describe Backlogs::Sprints::FinishService do
       it "unassigns from sprint, completes the sprint, and places WP after existing backlog items", :aggregate_failures do
         expect(result).to be_success
         expect(sprint.reload).to be_completed
-        expect(open_wp.reload.sprint).to be_nil
-        expect(open_wp.reload.position).to be > existing_backlog_wp.reload.position
+        expect(open_wp.reload).to have_attributes(sprint: nil, position: be > existing_backlog_wp.reload.position)
       end
     end
   end
@@ -201,57 +199,21 @@ RSpec.describe Backlogs::Sprints::FinishService do
         expect(sprint.reload).to be_completed
 
         # In the project's sprint (the one the work packages were moved to)
-
-        open_wp1.reload
-        expect(open_wp1.sprint).to eq(target_sprint)
-        expect(open_wp1.position).to eq(1)
-        expect(open_wp1.project).to eq(project)
-
-        open_wp2.reload
-        expect(open_wp2.sprint).to eq(target_sprint)
-        expect(open_wp2.position).to eq(2)
-        expect(open_wp2.project).to eq(project)
-
-        open_wp3_target_sprint.reload
-        expect(open_wp3_target_sprint.sprint).to eq(target_sprint)
-        expect(open_wp3_target_sprint.position).to eq(3)
-        expect(open_wp3_target_sprint.project).to eq(project)
-
-        open_wp4_target_sprint.reload
-        expect(open_wp4_target_sprint.sprint).to eq(target_sprint)
-        expect(open_wp4_target_sprint.position).to eq(4)
-        expect(open_wp4_target_sprint.project).to eq(project)
+        expect(open_wp1.reload).to have_attributes(sprint: target_sprint, position: 1, project:)
+        expect(open_wp2.reload).to have_attributes(sprint: target_sprint, position: 2, project:)
+        expect(open_wp3_target_sprint.reload).to have_attributes(sprint: target_sprint, position: 3, project:)
+        expect(open_wp4_target_sprint.reload).to have_attributes(sprint: target_sprint, position: 4, project:)
 
         # In the project's backlog
-
-        open_wp5_backlog.reload
-        expect(open_wp5_backlog.sprint).to be_nil
-        expect(open_wp5_backlog.position).to eq(1)
-        expect(open_wp5_backlog.project).to eq(project)
+        expect(open_wp5_backlog.reload).to have_attributes(sprint: nil, position: 1, project:)
 
         # In the project's sprint
-
-        closed_wp.reload.sprint
-        expect(closed_wp.sprint).to eq(sprint)
-        expect(closed_wp.position).to eq(1)
-        expect(closed_wp.project).to eq(project)
+        expect(closed_wp.reload).to have_attributes(sprint:, position: 1, project:)
 
         # In the other project's target_sprint (newly added)
-
-        open_wp1_other_project.reload
-        expect(open_wp1_other_project.sprint).to eq(target_sprint)
-        expect(open_wp1_other_project.position).to eq(1)
-        expect(open_wp1_other_project.project).to eq(other_project)
-
-        open_wp2_other_project.reload
-        expect(open_wp2_other_project.sprint).to eq(target_sprint)
-        expect(open_wp2_other_project.position).to eq(2)
-        expect(open_wp2_other_project.project).to eq(other_project)
-
-        open_wp3_other_project_backlog.reload
-        expect(open_wp3_other_project_backlog.sprint).to be_nil
-        expect(open_wp3_other_project_backlog.position).to eq(1)
-        expect(open_wp3_other_project_backlog.project).to eq(other_project)
+        expect(open_wp1_other_project.reload).to have_attributes(sprint: target_sprint, position: 1, project: other_project)
+        expect(open_wp2_other_project.reload).to have_attributes(sprint: target_sprint, position: 2, project: other_project)
+        expect(open_wp3_other_project_backlog.reload).to have_attributes(sprint: nil, position: 1, project: other_project)
       end
     end
 
@@ -264,57 +226,21 @@ RSpec.describe Backlogs::Sprints::FinishService do
         expect(sprint.reload).to be_completed
 
         # In the project's backlog
-
-        open_wp1.reload
-        expect(open_wp1.sprint).to be_nil
-        expect(open_wp1.position).to eq(1)
-        expect(open_wp1.project).to eq(project)
-
-        open_wp2.reload
-        expect(open_wp2.sprint).to be_nil
-        expect(open_wp2.position).to eq(2)
-        expect(open_wp2.project).to eq(project)
-
-        open_wp5_backlog.reload
-        expect(open_wp5_backlog.sprint).to be_nil
-        expect(open_wp5_backlog.position).to eq(3)
-        expect(open_wp5_backlog.project).to eq(project)
+        expect(open_wp1.reload).to have_attributes(sprint: nil, position: 1, project:)
+        expect(open_wp2.reload).to have_attributes(sprint: nil, position: 2, project:)
+        expect(open_wp5_backlog.reload).to have_attributes(sprint: nil, position: 3, project:)
 
         # In the project's other sprint
-
-        open_wp3_target_sprint.reload
-        expect(open_wp3_target_sprint.sprint).to eq(target_sprint)
-        expect(open_wp3_target_sprint.position).to eq(1)
-        expect(open_wp3_target_sprint.project).to eq(project)
-
-        open_wp4_target_sprint.reload
-        expect(open_wp4_target_sprint.sprint).to eq(target_sprint)
-        expect(open_wp4_target_sprint.position).to eq(2)
-        expect(open_wp4_target_sprint.project).to eq(project)
+        expect(open_wp3_target_sprint.reload).to have_attributes(sprint: target_sprint, position: 1, project:)
+        expect(open_wp4_target_sprint.reload).to have_attributes(sprint: target_sprint, position: 2, project:)
 
         # In the project's sprint
-
-        closed_wp.reload.sprint
-        expect(closed_wp.sprint).to eq(sprint)
-        expect(closed_wp.position).to eq(1)
-        expect(closed_wp.project).to eq(project)
+        expect(closed_wp.reload).to have_attributes(sprint:, position: 1, project:)
 
         # In the other project's backlog
-
-        open_wp1_other_project.reload
-        expect(open_wp1_other_project.sprint).to be_nil
-        expect(open_wp1_other_project.position).to eq(1)
-        expect(open_wp1_other_project.project).to eq(other_project)
-
-        open_wp2_other_project.reload
-        expect(open_wp2_other_project.sprint).to be_nil
-        expect(open_wp2_other_project.position).to eq(2)
-        expect(open_wp2_other_project.project).to eq(other_project)
-
-        open_wp3_other_project_backlog.reload
-        expect(open_wp3_other_project_backlog.sprint).to be_nil
-        expect(open_wp3_other_project_backlog.position).to eq(3)
-        expect(open_wp3_other_project_backlog.project).to eq(other_project)
+        expect(open_wp1_other_project.reload).to have_attributes(sprint: nil, position: 1, project: other_project)
+        expect(open_wp2_other_project.reload).to have_attributes(sprint: nil, position: 2, project: other_project)
+        expect(open_wp3_other_project_backlog.reload).to have_attributes(sprint: nil, position: 3, project: other_project)
       end
     end
 
@@ -327,59 +253,22 @@ RSpec.describe Backlogs::Sprints::FinishService do
         expect(sprint.reload).to be_completed
 
         # In the project's backlog
-
-        open_wp5_backlog.reload
-        expect(open_wp5_backlog.sprint).to be_nil
-        expect(open_wp5_backlog.position).to eq(1)
-        expect(open_wp5_backlog.project).to eq(project)
-
-        open_wp1.reload
-        expect(open_wp1.sprint).to be_nil
-        expect(open_wp1.position).to eq(2)
-        expect(open_wp1.project).to eq(project)
-
-        open_wp2.reload
-        expect(open_wp2.sprint).to be_nil
-        expect(open_wp2.position).to eq(3)
-        expect(open_wp2.project).to eq(project)
+        expect(open_wp5_backlog.reload).to have_attributes(sprint: nil, position: 1, project:)
+        expect(open_wp1.reload).to have_attributes(sprint: nil, position: 2, project:)
+        expect(open_wp2.reload).to have_attributes(sprint: nil, position: 3, project:)
 
         # In the project's other sprint
-
-        open_wp3_target_sprint.reload
-        expect(open_wp3_target_sprint.sprint).to eq(target_sprint)
-        expect(open_wp3_target_sprint.position).to eq(1)
-        expect(open_wp3_target_sprint.project).to eq(project)
-
-        open_wp4_target_sprint.reload
-        expect(open_wp4_target_sprint.sprint).to eq(target_sprint)
-        expect(open_wp4_target_sprint.position).to eq(2)
-        expect(open_wp4_target_sprint.project).to eq(project)
+        expect(open_wp3_target_sprint.reload).to have_attributes(sprint: target_sprint, position: 1, project:)
+        expect(open_wp4_target_sprint.reload).to have_attributes(sprint: target_sprint, position: 2, project:)
 
         # In the project's sprint
-
-        closed_wp.reload.sprint
-        expect(closed_wp.sprint).to eq(sprint)
-        # This should be 1 but is 2
-        # expect(closed_wp.position).to eq(1)
-        expect(closed_wp.position).to eq(2)
-        expect(closed_wp.project).to eq(project)
+        # Position should be 1 but it is 2
+        expect(closed_wp.reload).to have_attributes(sprint:, position: 2, project:)
 
         # In the other project's backlog
-
-        open_wp3_other_project_backlog.reload
-        expect(open_wp3_other_project_backlog.sprint).to be_nil
-        expect(open_wp3_other_project_backlog.position).to eq(1)
-        expect(open_wp3_other_project_backlog.project).to eq(other_project)
-
-        open_wp1_other_project.reload
-        expect(open_wp1_other_project.sprint).to be_nil
-        expect(open_wp1_other_project.position).to eq(2)
-        expect(open_wp1_other_project.project).to eq(other_project)
-
-        open_wp2_other_project.reload
-        expect(open_wp2_other_project.sprint).to be_nil
-        expect(open_wp2_other_project.position).to eq(3)
-        expect(open_wp2_other_project.project).to eq(other_project)
+        expect(open_wp3_other_project_backlog.reload).to have_attributes(sprint: nil, position: 1, project: other_project)
+        expect(open_wp1_other_project.reload).to have_attributes(sprint: nil, position: 2, project: other_project)
+        expect(open_wp2_other_project.reload).to have_attributes(sprint: nil, position: 3, project: other_project)
       end
     end
   end

modules/backlogs/spec/services/backlogs/work_packages/rebuild_positions_service_integration_spec.rb

@@ -32,9 +32,11 @@ require "spec_helper"
 
 RSpec.describe Backlogs::WorkPackages::RebuildPositionsService, "integration", type: :model do
   def create_work_package(options)
-    create(:work_package, options.reverse_merge(project: options[:sprint]&.project || options[:backlog_bucket]&.project,
-                                                type_id: type.id)) do |wp|
-      wp.update_column(:position, options[:position]) if options.key?(:position)
+    container = options[:sprint] || options[:backlog_bucket]
+    project = container&.project
+
+    WorkPackage.acts_as_list_no_update do
+      create(:work_package, options.reverse_merge(project:, type_id: type.id))
     end
   end
 
@@ -50,49 +52,24 @@ RSpec.describe Backlogs::WorkPackages::RebuildPositionsService, "integration", t
   shared_let(:sprint1_wp1) { create_work_package(subject: "Sprint 1 WorkPackage 1", sprint: sprint1, position: nil) }
   shared_let(:sprint1_wp2) { create_work_package(subject: "Sprint 1 WorkPackage 2", sprint: sprint1, position: 1) }
   shared_let(:sprint1_wp3) { create_work_package(subject: "Sprint 1 WorkPackage 3", sprint: sprint1, position: 2) }
-  shared_let(:sprint1_wp4) do
-    create_work_package(subject: "Sprint 1 WorkPackage 4", sprint: sprint1, position: 2).tap do
-      # Force wp3 back to position 2 so that wp3 and wp4 are genuinely
-      # duplicated — the service must break the tie via created_at.
-      sprint1_wp3.update_column(:position, 2)
-    end
-  end
+  shared_let(:sprint1_wp4) { create_work_package(subject: "Sprint 1 WorkPackage 4", sprint: sprint1, position: 2) }
   shared_let(:sprint1_wp5) { create_work_package(subject: "Sprint 1 WorkPackage 5", sprint: sprint1, position: nil) }
 
   shared_let(:sprint2_wp1) { create_work_package(subject: "Sprint 2 WorkPackage 1", sprint: sprint2, position: 3) }
   shared_let(:sprint2_wp2) { create_work_package(subject: "Sprint 2 WorkPackage 2", sprint: sprint2, position: 2) }
-  shared_let(:sprint2_wp3) do
-    create_work_package(subject: "Sprint 2 WorkPackage 3", sprint: sprint2, position: 1).tap do
-      # acts_as_list inserts shift earlier siblings; restore them so
-      # the DB starts from the positions the test actually sets.
-      sprint2_wp1.update_column(:position, 3)
-      sprint2_wp2.update_column(:position, 2)
-    end
-  end
+  shared_let(:sprint2_wp3) { create_work_package(subject: "Sprint 2 WorkPackage 3", sprint: sprint2, position: 1) }
 
   shared_let(:sprint3_wp1) { create_work_package(subject: "Sprint 3 WorkPackage 1", sprint: sprint3, position: nil) }
   shared_let(:sprint3_wp2) { create_work_package(subject: "Sprint 3 WorkPackage 2", sprint: sprint3, position: nil) }
   shared_let(:sprint3_wp3) { create_work_package(subject: "Sprint 3 WorkPackage 3", sprint: sprint3, position: nil) }
 
-  shared_let(:inbox_wp1) do
-    create_work_package(subject: "Inbox WorkPackage 1", project: project1, position: nil)
-  end
-  shared_let(:inbox_wp2) do
-    create_work_package(subject: "Inbox WorkPackage 2", project: project1, position: nil)
-  end
-  shared_let(:inbox_wp3) do
-    create_work_package(subject: "Inbox WorkPackage 3", project: project1, position: nil)
-  end
+  shared_let(:inbox_wp1) { create_work_package(subject: "Inbox WorkPackage 1", project: project1, position: nil) }
+  shared_let(:inbox_wp2) { create_work_package(subject: "Inbox WorkPackage 2", project: project1, position: nil) }
+  shared_let(:inbox_wp3) { create_work_package(subject: "Inbox WorkPackage 3", project: project1, position: nil) }
 
   shared_let(:bucket1_wp1) { create_work_package(subject: "Bucket 1 WorkPackage 1", backlog_bucket: bucket1, position: nil) }
   shared_let(:bucket1_wp2) { create_work_package(subject: "Bucket 1 WorkPackage 2", backlog_bucket: bucket1, position: 2) }
-  shared_let(:bucket1_wp3) do
-    create_work_package(subject: "Bucket 1 WorkPackage 3", backlog_bucket: bucket1, position: 1).tap do
-      # acts_as_list inserts shift earlier siblings; restore them so
-      # the DB starts from the positions the test actually sets.
-      bucket1_wp2.update_column(:position, 2)
-    end
-  end
+  shared_let(:bucket1_wp3) { create_work_package(subject: "Bucket 1 WorkPackage 3", backlog_bucket: bucket1, position: 1) }
 
   shared_let(:bucket2_wp1) { create_work_package(subject: "Bucket 2 WorkPackage 1", backlog_bucket: bucket2, position: nil) }
   shared_let(:bucket2_wp2) { create_work_package(subject: "Bucket 2 WorkPackage 2", backlog_bucket: bucket2, position: nil) }