Merge branch 'issue-337190-sync_requirements_and_requirements_issues_state' into 'master'

Sync requirement and requirement issues state

See merge request gitlab-org/gitlab!70607

app/services/issues/close_service.rb

@@ -3,8 +3,8 @@
 module Issues
   class CloseService < Issues::BaseService
     # Closes the supplied issue if the current user is able to do so.
-    def execute(issue, commit: nil, notifications: true, system_note: true)
-      return issue unless can?(current_user, :update_issue, issue) || issue.is_a?(ExternalIssue)
+    def execute(issue, commit: nil, notifications: true, system_note: true, skip_authorization: false)
+      return issue unless can_close?(issue, skip_authorization: skip_authorization)
 
       close_issue(issue,
                   closed_via: commit,
@@ -24,7 +24,7 @@ module Issues
         return issue
       end
 
-      if project.issues_enabled? && issue.close(current_user)
+      if perform_close(issue)
         event_service.close_issue(issue, current_user)
         create_note(issue, closed_via) if system_note
 
@@ -51,6 +51,15 @@ module Issues
 
     private
 
+    # Overridden on EE
+    def perform_close(issue)
+      issue.close(current_user)
+    end
+
+    def can_close?(issue, skip_authorization: false)
+      skip_authorization || can?(current_user, :update_issue, issue) || issue.is_a?(ExternalIssue)
+    end
+
     def perform_incident_management_actions(issue)
       resolve_alert(issue)
     end

app/services/issues/reopen_service.rb

@@ -2,10 +2,10 @@
 
 module Issues
   class ReopenService < Issues::BaseService
-    def execute(issue)
-      return issue unless can?(current_user, :reopen_issue, issue)
+    def execute(issue, skip_authorization: false)
+      return issue unless can_reopen?(issue, skip_authorization: skip_authorization)
 
-      if issue.reopen
+      if perform_reopen(issue)
         event_service.reopen_issue(issue, current_user)
         create_note(issue, 'reopened')
         notification_service.async.reopen_issue(issue, current_user)
@@ -22,6 +22,15 @@ module Issues
 
     private
 
+    # Overriden on EE
+    def perform_reopen(issue)
+      issue.reopen
+    end
+
+    def can_reopen?(issue, skip_authorization: false)
+      skip_authorization || can?(current_user, :reopen_issue, issue)
+    end
+
     def perform_incident_management_actions(issue)
     end
 

ee/app/models/requirements_management/requirement.rb

@@ -86,7 +86,7 @@ module RequirementsManagement
       end
 
       def sync_params
-        [:title, :description]
+        [:title, :description, :state]
       end
     end
 

ee/app/services/ee/issues/base_service.rb

@@ -107,6 +107,36 @@ module EE
           raise EpicAssignmentError, result[:message]
         end
       end
+
+      # This is part IIb(sync state updates) of the migration from
+      # Requirement (the first class object) to Issue/Work Item (of type Requirement).
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/323779
+      def sync_requirement_state(issue, state, &block)
+        return yield unless issue.requirement?
+
+        requirement = issue.requirement
+
+        return yield unless requirement # no need to use transaction if there is no requirement to sync
+
+        ::Issue.transaction do
+          requirement.state = state
+          requirement.save!
+
+          state == 'archived' ? issue.close!(current_user) : issue.reopen!
+        rescue StandardError => e
+          ::Gitlab::AppLogger.info(
+            message: 'Requirement-Issue state Sync: Associated requirement could not be saved',
+            error: e.message,
+            project_id: project.id,
+            user_id: current_user.id,
+            requirement_id: requirement.id,
+            issue_id: issue.id,
+            state: state
+          )
+
+          false
+        end
+      end
     end
   end
 end

ee/app/services/ee/issues/close_service.rb

@@ -10,6 +10,13 @@ module EE
         super
         update_issuable_sla(issue)
       end
+
+      override :perform_close
+      def perform_close(issue)
+        sync_requirement_state(issue, 'archived') do
+          super
+        end
+      end
     end
   end
 end

ee/app/services/ee/issues/reopen_service.rb

@@ -10,6 +10,13 @@ module EE
         super
         update_issuable_sla(issue)
       end
+
+      override :perform_reopen
+      def perform_reopen(issue)
+        sync_requirement_state(issue, 'opened') do
+          super
+        end
+      end
     end
   end
 end

ee/app/services/ee/issues/update_service.rb

@@ -8,7 +8,6 @@ module EE
 
       override :filter_params
       def filter_params(issue)
-        params.delete(:skip_auth)
         params.delete(:sprint_id) unless can_admin_issuable?(issue)
 
         filter_epic(issue)
@@ -32,11 +31,6 @@ module EE
         result
       end
 
-      override :can_admin_issuable?
-      def can_admin_issuable?(issuable)
-        skip_auth || super
-      end
-
       override :handle_changes
       def handle_changes(issue, _options)
         super
@@ -79,7 +73,7 @@ module EE
 
       private
 
-      attr_accessor :skip_auth, :requirement_to_sync
+      attr_accessor :requirement_to_sync
 
       def handle_iteration_change(issue)
         return unless issue.previous_changes.include?('sprint_id')

ee/app/services/requirements_management/update_requirement_service.rb

@@ -5,7 +5,7 @@ module RequirementsManagement
     def execute(requirement)
       raise Gitlab::Access::AccessDeniedError unless can?(current_user, :update_requirement, project)
 
-      attrs = whitelisted_requirement_params
+      attrs = allowlisted_requirement_params
 
       requirement.assign_attributes(attrs)
 
@@ -40,7 +40,7 @@ module RequirementsManagement
       TestReport.build_report(requirement: requirement, state: params[:last_test_report_state], author: current_user).save!
     end
 
-    def whitelisted_requirement_params
+    def allowlisted_requirement_params
       params.slice(:title, :description, :state)
     end
 
@@ -54,11 +54,31 @@ module RequirementsManagement
 
       requirement_issue = requirement.requirement_issue
 
-      # Skip authorisation so we don't risk a permissions mismatch while still getting the advantages
-      # of the service, such as system notes.
-      params = sync_attrs.merge(skip_auth: true)
-      ::Issues::UpdateService.new(project: project, current_user: current_user, params: params)
+      state_change = sync_attrs.delete(:state)
+      update_requirement_issue_title_and_description(requirement_issue, sync_attrs)
+      update_requirement_issue_state(requirement_issue, state_change)
+    end
+
+    def update_requirement_issue_title_and_description(requirement_issue, params)
+      return requirement_issue unless params.any?
+
+      title_and_description = params.with_indifferent_access.slice(:title, :description)
+
+      ::Issues::UpdateService.new(project: project, current_user: current_user, params: title_and_description)
         .execute(requirement_issue)
     end
+
+    def update_requirement_issue_state(requirement_issue, new_state)
+      return requirement_issue unless new_state
+
+      service =
+        if new_state.to_sym == :opened
+          ::Issues::ReopenService
+        else
+          ::Issues::CloseService
+        end
+
+      service.new(project: project, current_user: current_user).execute(requirement_issue, skip_authorization: true)
+    end
   end
 end

ee/spec/services/ee/issues/close_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Issues::CloseService do
+  context 'sync requirement' do
+    let(:requirement_initial_state) { 'opened' }
+    let(:requirement_expected_state) { 'archived' }
+    let(:issue_initial_state) { 'opened' }
+    let(:issue_expected_state) { 'closed' }
+
+    it_behaves_like 'sync requirement with issue state'
+  end
+end

ee/spec/services/ee/issues/reopen_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Issues::ReopenService do
+  context 'sync requirement' do
+    let(:requirement_initial_state) { 'archived' }
+    let(:requirement_expected_state) { 'opened' }
+    let(:issue_initial_state) { 'closed' }
+    let(:issue_expected_state) { 'opened' }
+
+    it_behaves_like 'sync requirement with issue state'
+  end
+end

ee/spec/services/requirements_management/update_requirement_service_spec.rb

@@ -11,7 +11,8 @@ RSpec.describe RequirementsManagement::UpdateRequirementService do
 
   let_it_be(:project) { create(:project)}
   let_it_be(:user) { create(:user) }
-  let_it_be_with_reload(:requirement) { create(:requirement, project: project, title: title, description: description) }
+
+  let!(:requirement) { create(:requirement, project: project, title: title, description: description, state: :opened) }
 
   let(:params) do
     {
@@ -47,7 +48,7 @@ RSpec.describe RequirementsManagement::UpdateRequirementService do
         )
       end
 
-      context 'when updating title or description' do
+      context 'when updating title, description or state' do
         shared_examples 'keeps requirement and its requirement_issue in sync' do
           it 'keeps title and description in sync' do
             subject
@@ -58,6 +59,12 @@ RSpec.describe RequirementsManagement::UpdateRequirementService do
             expect(requirement).to have_attributes(
               title: requirement.requirement_issue.title,
               description: requirement.requirement_issue.description)
+
+            # Both objects (Requirement | Requirement Issue) state enums have the same integers
+            # but on Requirement 'closed' means 'archived'.
+            # requirement: enum state: { opened: 1, archived: 2 }
+            # issue:       STATE_ID_MAP = { opened: 1, closed: 2, ...
+            expect(requirement.read_attribute_before_type_cast(:state)). to eq(requirement.requirement_issue.state_id)
           end
         end
 
@@ -72,7 +79,7 @@ RSpec.describe RequirementsManagement::UpdateRequirementService do
         end
 
         context 'if there is an associated requirement_issue' do
-          let_it_be_with_reload(:requirement_issue) { create(:requirement_issue, requirement: requirement, title: title, description: description) }
+          let!(:requirement_issue) { create(:requirement_issue, requirement: requirement, title: title, description: description, state: :opened) }
 
           let(:params) do
             { title: new_title, description: new_description }
@@ -84,30 +91,52 @@ RSpec.describe RequirementsManagement::UpdateRequirementService do
               .and change { requirement.requirement_issue.title }.from(title).to(new_title)
           end
 
-          context 'when updating only title' do
+          context 'when updating title' do
             let(:params) do
               { title: new_title }
             end
 
-            it "updates requirement's title" do
+            it "updates requirement's issue title" do
               expect { subject }.to change { requirement.requirement_issue.reload.title }.from(title).to(new_title)
             end
 
             it_behaves_like 'keeps requirement and its requirement_issue in sync'
           end
 
-          context "updates requirement's description" do
+          context 'when updating description' do
             let(:params) do
               { description: new_description }
             end
 
-            it 'updates description' do
+            it "updates requirement's issue description" do
               expect { subject }.to change { requirement.requirement_issue.reload.description }.from(description).to(new_description)
             end
 
             it_behaves_like 'keeps requirement and its requirement_issue in sync'
           end
 
+          context 'when updating state' do
+            context 'to archived' do
+              let(:params) do
+                { state: 'archived' }
+              end
+
+              it_behaves_like 'keeps requirement and its requirement_issue in sync'
+            end
+
+            context 'to opened' do
+              let(:params) do
+                { state: 'opened' }
+              end
+
+              before do
+                requirement.update!(state: 'archived')
+              end
+
+              it_behaves_like 'keeps requirement and its requirement_issue in sync'
+            end
+          end
+
           context 'if update fails' do
             let(:params) do
               { title: nil }
@@ -126,6 +155,15 @@ RSpec.describe RequirementsManagement::UpdateRequirementService do
                   allow(service).to receive(:execute).and_return(requirement_issue)
                 end
 
+                allow_next_instance_of(::Issues::ReopenService) do |service|
+                  allow(service).to receive(:execute).and_return(requirement_issue)
+                end
+
+                allow_next_instance_of(::Issues::CloseService) do |service|
+                  allow(service).to receive(:execute).and_return(requirement_issue)
+                end
+
+                allow(requirement).to receive(:requirement_issue).and_return(requirement_issue)
                 allow(requirement_issue).to receive(:invalid?).and_return(true).at_least(:once)
               end
 
@@ -141,18 +179,6 @@ RSpec.describe RequirementsManagement::UpdateRequirementService do
               end
             end
           end
-
-          context 'when updating some unrelated field' do
-            let(:params) do
-              { state: :archived }
-            end
-
-            it 'does not update the associated requirement issue' do
-              expect { subject }.not_to change { requirement.requirement_issue.state }
-            end
-
-            it_behaves_like 'keeps requirement and its requirement_issue in sync'
-          end
         end
 
         context 'if there is no requirement_issue' do

ee/spec/support/shared_examples/services/sync_issue_and_requirement_state_shared_examples.rb

+# frozen_string_literal: true
+
+shared_examples 'sync requirement with issue state' do
+  let_it_be(:not_member) { create(:user) }
+  let_it_be(:project) { create(:project, :private) }
+
+  let(:issue) { create(:issue, issue_type: :requirement, project: project, state: issue_initial_state) }
+  let!(:requirement) { create(:requirement, project: project, requirement_issue: issue, state: requirement_initial_state) }
+
+  subject { described_class.new(project: project, current_user: not_member).execute(issue, skip_authorization: skip_authorization) }
+
+  context 'when skip_authorization is false' do
+    let(:skip_authorization) { false }
+
+    it 'does not change issue state' do
+      subject
+
+      expect(issue.reload.state).to eq(issue_initial_state)
+      expect(requirement.reload.state).to eq(requirement_initial_state)
+    end
+  end
+
+  context 'when skip_authorization is true' do
+    let(:skip_authorization) { true }
+
+    context 'when issue is not of requirement_type' do
+      before do
+        issue.update!(issue_type: :incident)
+      end
+
+      it 'does not sync state' do
+        subject
+
+        expect(issue.reload.state).to eq(issue_expected_state)
+        expect(requirement.reload.state).to eq(requirement_initial_state)
+      end
+    end
+
+    it 'keeps requirement and requirement issue in sync' do
+      subject
+
+      expect(issue.reload.state).to eq(issue_expected_state)
+      expect(requirement.reload.state).to eq(requirement_expected_state)
+    end
+
+    context 'when saving requirement fails' do
+      before do
+        allow(issue).to receive(:requirement).and_return(requirement)
+        allow(requirement).to receive(:save!).and_raise(ActiveRecord::StatementTimeout, 'time is out')
+      end
+
+      it 'does not change requirement and issue states' do
+        subject
+
+        expect(issue.reload.state).to eq(issue_initial_state)
+        expect(requirement.reload.state).to eq(requirement_initial_state)
+      end
+
+      it 'logs error' do
+        expect(Gitlab::AppLogger).to receive(:info).with(
+          message: 'Requirement-Issue state Sync: Associated requirement could not be saved',
+          error: 'time is out',
+          project_id: project.id,
+          user_id: not_member.id,
+          requirement_id: requirement.id,
+          issue_id: issue.id,
+          state: requirement_expected_state
+        )
+
+        subject
+      end
+    end
+  end
+end

spec/services/issues/close_service_spec.rb

@@ -22,6 +22,18 @@ RSpec.describe Issues::CloseService do
   describe '#execute' do
     let(:service) { described_class.new(project: project, current_user: user) }
 
+    context 'when skip_authorization is true' do
+      it 'does close the issue even if user is not authorized' do
+        non_authorized_user = create(:user)
+
+        service = described_class.new(project: project, current_user: non_authorized_user)
+
+        expect do
+          service.execute(issue, skip_authorization: true)
+        end.to change { issue.reload.state }.from('opened').to('closed')
+      end
+    end
+
     it 'checks if the user is authorized to update the issue' do
       expect(service).to receive(:can?).with(user, :update_issue, issue)
         .and_call_original

spec/services/issues/reopen_service_spec.rb

@@ -8,18 +8,26 @@ RSpec.describe Issues::ReopenService do
 
   describe '#execute' do
     context 'when user is not authorized to reopen issue' do
-      before do
+      it 'does not reopen the issue' do
         guest = create(:user)
         project.add_guest(guest)
 
-        perform_enqueued_jobs do
         described_class.new(project: project, current_user: guest).execute(issue)
-        end
-      end
 
-      it 'does not reopen the issue' do
         expect(issue).to be_closed
       end
+
+      context 'when skip_authorization is true' do
+        it 'does close the issue even if user is not authorized' do
+          non_authorized_user = create(:user)
+
+          service = described_class.new(project: project, current_user: non_authorized_user)
+
+          expect do
+            service.execute(issue, skip_authorization: true)
+          end.to change { issue.reload.state }.from('closed').to('opened')
+        end
+      end
     end
 
     context 'when user is authorized to reopen issue' do