Merge branch '208388-design-removal-todos' into 'master'

Remove todos when a design is archived

See merge request gitlab-org/gitlab!69730

app/models/design_management/action.rb

@@ -17,6 +17,8 @@ module DesignManagement
 
     # we assume sequential ordering.
     scope :ordered, -> { order(version_id: :asc) }
+    scope :by_design, -> (design) { where(design: design) }
+    scope :by_event, -> (event) { where(event: event) }
 
     # For each design, only select the most recent action
     scope :most_recent, -> do

app/services/design_management/delete_designs_service.rb

@@ -17,6 +17,7 @@ module DesignManagement
       version = delete_designs!
       EventCreateService.new.destroy_designs(designs, current_user)
       Gitlab::UsageDataCounters::IssueActivityUniqueCounter.track_issue_designs_removed_action(author: current_user)
+      TodosDestroyer::DestroyedDesignsWorker.perform_async(designs.map(&:id))
 
       success(version: version)
     end

app/services/todos/destroy/design_service.rb

+# frozen_string_literal: true
+
+module Todos
+  module Destroy
+    # Service class for deleting todos that belongs to a deleted/archived design.
+    class DesignService
+      attr_reader :design_ids
+
+      def initialize(design_ids)
+        @design_ids = design_ids
+      end
+
+      def execute
+        todos.delete_all
+      end
+
+      private
+
+      def todos
+        Todo.for_target(deleted_designs.select(:design_id)).for_type(DesignManagement::Design)
+      end
+
+      def deleted_designs
+        DesignManagement::Action.by_design(design_ids).by_event(:deletion)
+      end
+    end
+  end
+end

app/workers/all_queues.yml

@@ -1681,6 +1681,15 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: todos_destroyer:todos_destroyer_destroyed_designs
+  :worker_name: TodosDestroyer::DestroyedDesignsWorker
+  :feature_category: :issue_tracking
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: todos_destroyer:todos_destroyer_destroyed_issuable
   :worker_name: TodosDestroyer::DestroyedIssuableWorker
   :feature_category: :issue_tracking

app/workers/todos_destroyer/destroyed_designs_worker.rb

+# frozen_string_literal: true
+
+module TodosDestroyer
+  class DestroyedDesignsWorker
+    include ApplicationWorker
+
+    data_consistency :always
+
+    sidekiq_options retry: 3
+    include TodosDestroyerQueue
+
+    idempotent!
+
+    def perform(design_ids)
+      ::Todos::Destroy::DesignService.new(design_ids).execute
+    end
+  end
+end

spec/models/design_management/action_spec.rb

@@ -8,37 +8,55 @@ RSpec.describe DesignManagement::Action do
   end
 
   describe 'scopes' do
-    describe '.most_recent' do
-      let_it_be(:design_a) { create(:design) }
-      let_it_be(:design_b) { create(:design) }
-      let_it_be(:design_c) { create(:design) }
+    let_it_be(:issue) { create(:issue) }
+    let_it_be(:design_a) { create(:design, issue: issue) }
+    let_it_be(:design_b) { create(:design, issue: issue) }
 
-      let(:designs) { [design_a, design_b, design_c] }
+    context 'with 3 designs' do
+      let_it_be(:design_c) { create(:design, issue: issue) }
 
-      before_all do
-        create(:design_version, designs: [design_a, design_b, design_c])
-        create(:design_version, designs: [design_a, design_b])
-        create(:design_version, designs: [design_a])
-      end
+      let_it_be(:action_a_1) { create(:design_action, design: design_a) }
+      let_it_be(:action_a_2) { create(:design_action, design: design_a, event: :deletion) }
+      let_it_be(:action_b)   { create(:design_action, design: design_b) }
+      let_it_be(:action_c)   { create(:design_action, design: design_c, event: :deletion) }
+
+      describe '.most_recent' do
+        let(:designs) { [design_a, design_b, design_c] }
+
+        before_all do
+          create(:design_version, designs: [design_a, design_b, design_c])
+          create(:design_version, designs: [design_a, design_b])
+          create(:design_version, designs: [design_a])
+        end
+
+        it 'finds the correct version for each design' do
+          dvs = described_class.where(design: designs)
+
+          expected = designs
+            .map(&:id)
+            .zip(dvs.order("version_id DESC").pluck(:version_id).uniq)
 
-      it 'finds the correct version for each design' do
-        dvs = described_class.where(design: designs)
+          actual = dvs.most_recent.map { |dv| [dv.design_id, dv.version_id] }
 
-        expected = designs
-          .map(&:id)
-          .zip(dvs.order("version_id DESC").pluck(:version_id).uniq)
+          expect(actual).to eq(expected)
+        end
+      end
 
-        actual = dvs.most_recent.map { |dv| [dv.design_id, dv.version_id] }
+      describe '.by_design' do
+        it 'returns the actions by design_id' do
+          expect(described_class.by_design([design_a.id, design_b.id]))
+            .to match_array([action_a_1, action_a_2, action_b])
+        end
+      end
 
-        expect(actual).to eq(expected)
+      describe '.by_event' do
+        it 'returns the actions by event type' do
+          expect(described_class.by_event(:deletion)).to match_array([action_a_2, action_c])
+        end
       end
     end
 
     describe '.up_to_version' do
-      let_it_be(:issue) { create(:issue) }
-      let_it_be(:design_a) { create(:design, issue: issue) }
-      let_it_be(:design_b) { create(:design, issue: issue) }
-
       # let bindings are not available in before(:all) contexts,
       # so we need to redefine the array on each construction.
       let_it_be(:oldest) { create(:design_version, designs: [design_a, design_b]) }

spec/services/design_management/delete_designs_service_spec.rb

@@ -149,6 +149,12 @@ RSpec.describe DesignManagement::DeleteDesignsService do
           expect { run_service }
             .to change { designs.first.deleted? }.from(false).to(true)
         end
+
+        it 'schedules deleting todos for that design' do
+          expect(TodosDestroyer::DestroyedDesignsWorker).to receive(:perform_async).with([designs.first.id])
+
+          run_service
+        end
       end
 
       context 'more than one design is passed' do
@@ -168,6 +174,12 @@ RSpec.describe DesignManagement::DeleteDesignsService do
             .and change { Event.destroyed_action.for_design.count }.by(2)
         end
 
+        it 'schedules deleting todos for that design' do
+          expect(TodosDestroyer::DestroyedDesignsWorker).to receive(:perform_async).with(designs.map(&:id))
+
+          run_service
+        end
+
         it_behaves_like "a success"
 
         context 'after executing the service' do

spec/services/todos/destroy/design_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Todos::Destroy::DesignService do
+  let_it_be(:user)     { create(:user) }
+  let_it_be(:user_2)   { create(:user) }
+  let_it_be(:design)   { create(:design) }
+  let_it_be(:design_2) { create(:design) }
+  let_it_be(:design_3) { create(:design) }
+
+  let_it_be(:create_action)   { create(:design_action, design: design)}
+  let_it_be(:create_action_2) { create(:design_action, design: design_2)}
+
+  describe '#execute' do
+    before do
+      create(:todo, user: user, target: design)
+      create(:todo, user: user_2, target: design)
+      create(:todo, user: user, target: design_2)
+      create(:todo, user: user, target: design_3)
+    end
+
+    subject { described_class.new([design.id, design_2.id, design_3.id]).execute }
+
+    context 'when the design has been archived' do
+      let_it_be(:archive_action) { create(:design_action, design: design, event: :deletion)}
+      let_it_be(:archive_action_2) { create(:design_action, design: design_3, event: :deletion)}
+
+      it 'removes todos for that design' do
+        expect { subject }.to change { Todo.count }.from(4).to(1)
+      end
+    end
+
+    context 'when no design has been archived' do
+      it 'does not remove any todos' do
+        expect { subject }.not_to change { Todo.count }.from(4)
+      end
+    end
+  end
+end

spec/workers/every_sidekiq_worker_spec.rb

@@ -436,6 +436,7 @@ RSpec.describe 'Every Sidekiq worker' do
         'TodosDestroyer::ConfidentialEpicWorker' => 3,
         'TodosDestroyer::ConfidentialIssueWorker' => 3,
         'TodosDestroyer::DestroyedIssuableWorker' => 3,
+        'TodosDestroyer::DestroyedDesignsWorker' => 3,
         'TodosDestroyer::EntityLeaveWorker' => 3,
         'TodosDestroyer::GroupPrivateWorker' => 3,
         'TodosDestroyer::PrivateFeaturesWorker' => 3,

spec/workers/todos_destroyer/destroyed_designs_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe TodosDestroyer::DestroyedDesignsWorker do
+  let(:service) { double }
+
+  it 'calls the Todos::Destroy::DesignService with design_ids parameter' do
+    expect(::Todos::Destroy::DesignService).to receive(:new).with([1, 5]).and_return(service)
+    expect(service).to receive(:execute)
+
+    described_class.new.perform([1, 5])
+  end
+end