Merge branch 'issue_32894' into 'master'

Allow to soft delete issuables description history

See merge request gitlab-org/gitlab!21439

app/models/note.rb

@@ -124,7 +124,7 @@ class Note < ApplicationRecord
   scope :inc_author, -> { includes(:author) }
   scope :inc_relations_for_view, -> do
     includes(:project, { author: :status }, :updated_by, :resolved_by, :award_emoji,
-             :system_note_metadata, :note_diff_file, :suggestions)
+             { system_note_metadata: :description_version }, :note_diff_file, :suggestions)
   end
 
   scope :with_notes_filter, -> (notes_filter) do

db/migrate/20191209143606_add_deleted_at_to_description_versions.rb

+# frozen_string_literal: true
+
+class AddDeletedAtToDescriptionVersions < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def change
+    add_column :description_versions, :deleted_at, :datetime_with_timezone
+  end
+end

db/schema.rb

@@ -1410,6 +1410,7 @@ ActiveRecord::Schema.define(version: 2020_01_21_132641) do
     t.integer "merge_request_id"
     t.integer "epic_id"
     t.text "description"
+    t.datetime_with_timezone "deleted_at"
     t.index ["epic_id"], name: "index_description_versions_on_epic_id", where: "(epic_id IS NOT NULL)"
     t.index ["issue_id"], name: "index_description_versions_on_issue_id", where: "(issue_id IS NOT NULL)"
     t.index ["merge_request_id"], name: "index_description_versions_on_merge_request_id", where: "(merge_request_id IS NOT NULL)"

ee/app/controllers/concerns/description_diff_actions.rb

@@ -2,22 +2,55 @@
 
 module DescriptionDiffActions
   extend ActiveSupport::Concern
+  include Gitlab::Utils::StrongMemoize
 
-  def description_diff
-    return render_404 unless issuable.resource_parent.feature_available?(:description_diffs)
-
-    current_version = issuable.description_versions.find(params[:version_id])
-    previous_version = if params[:start_version_id].present?
-                         issuable.description_versions.find(params[:start_version_id]).previous_version
-                       else
-                         current_version.previous_version
-                       end
+  included do
+    before_action :verify_description_diffs_enabled!, only: [:description_diff, :delete_description_version]
+    before_action :authorize_delete_description_version!, only: :delete_description_version
+  end
 
-    return render_404 if previous_version.nil?
+  def description_diff
+    return render_404 if previous_description_version.nil?
 
-    diff = Gitlab::Diff::CharDiff.new(previous_version.description, current_version.description)
+    diff = Gitlab::Diff::CharDiff.new(previous_description_version.description, description_version.description)
     diff.generate_diff
 
     render html: diff.to_html
   end
+
+  def delete_description_version
+    description_version.delete!(start_id: params[:start_version_id])
+
+    head :ok
+  rescue ActiveRecord::RecordNotFound
+    render_404
+  end
+
+  private
+
+  def previous_description_version
+    strong_memoize(:previous_description_version) do
+      if params[:start_version_id].present?
+        issuable.description_versions.visible.find(params[:start_version_id]).previous_version
+      else
+        description_version.previous_version
+      end
+    end
+  end
+
+  def description_version
+    strong_memoize(:description_version) do
+      issuable.description_versions.visible.find(params[:version_id])
+    end
+  end
+
+  def verify_description_diffs_enabled!
+    return render_404 unless issuable.resource_parent.feature_available?(:description_diffs)
+  end
+
+  def authorize_delete_description_version!
+    rule = "admin_#{issuable.class.to_ability_name}"
+
+    return render_404 unless can?(current_user, rule, issuable)
+  end
 end

ee/app/helpers/ee/notes_helper.rb

@@ -43,5 +43,16 @@ module EE
         description_diff_group_epic_path(issuable.group, issuable, version_id)
       end
     end
+
+    def delete_description_version_path(issuable, version_id)
+      case issuable
+      when Issue
+        delete_description_version_project_issue_path(issuable.project, issuable, version_id)
+      when MergeRequest
+        delete_description_version_project_merge_request_path(issuable.project, issuable, version_id)
+      when Epic
+        delete_description_version_group_epic_path(issuable.group, issuable, version_id)
+      end
+    end
   end
 end

ee/app/models/ee/description_version.rb

@@ -6,6 +6,10 @@ module EE
 
     prepended do
       belongs_to :epic
+
+      # This scope is using `deleted_at` column which is not indexed.
+      # Prevent using it in not scoped contexts.
+      scope :visible, -> { where(deleted_at: nil) }
     end
 
     class_methods do
@@ -19,13 +23,36 @@ module EE
     end
 
     def previous_version
+      issuable_description_versions
+        .where('created_at < ?', created_at)
+        .order(created_at: :desc, id: :desc)
+        .first
+    end
+
+    # Soft deletes a description version.
+    # If start_id is given it soft deletes current version
+    # up to start_id of the same issuable.
+    def delete!(start_id: nil)
+      start_id ||= self.id
+
+      description_versions =
+        issuable_description_versions.where('id BETWEEN ? AND ?', start_id, self.id)
+
+      description_versions.update_all(deleted_at: Time.now)
+    end
+
+    def deleted?
+      self.deleted_at.present?
+    end
+
+    private
+
+    def issuable_description_versions
       self.class.where(
         issue_id: issue_id,
         merge_request_id: merge_request_id,
         epic_id: epic_id
-      ).where('created_at < ?', created_at)
-      .order(created_at: :desc, id: :desc)
-      .first
+      )
     end
   end
 end

ee/app/serializers/ee/note_entity.rb

@@ -7,9 +7,22 @@ module EE
     prepended do
       with_options if: -> (note, _) { note.system? && note.resource_parent.feature_available?(:description_diffs) } do
         expose :description_version_id
+
         expose :description_diff_path, if: -> (_) { description_version_id } do |note|
           description_diff_path(note.noteable, description_version_id)
         end
+
+        expose :delete_description_version_path, if: -> (_) { description_version_id } do |note|
+          delete_description_version_path(note.noteable, description_version_id)
+        end
+
+        expose :can_delete_description_version do |note|
+          rule = "admin_#{object.noteable.class.to_ability_name}"
+
+          Ability.allowed?(current_user, rule, object.noteable.resource_parent)
+        end
+
+        expose :description_version_deleted
       end
 
       private
@@ -17,6 +30,10 @@ module EE
       def description_version_id
         object.system_note_metadata&.description_version_id
       end
+
+      def description_version_deleted
+        object.system_note_metadata&.description_version&.deleted?
+      end
     end
   end
 end

ee/changelogs/unreleased/issue_32894.yml

+---
+title: Allow to soft delete issuables description history
+merge_request: 21439
+author:
+type: added

ee/config/routes/group.rb

@@ -74,6 +74,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
     resources :epics, concerns: :awardable, constraints: { id: /\d+/ } do
       member do
         get '/descriptions/:version_id/diff', action: :description_diff, as: :description_diff
+        delete '/descriptions/:version_id', action: :delete_description_version, as: :delete_description_version
         get :discussions, format: :json
         get :realtime_changes
         post :toggle_subscription

ee/config/routes/merge_requests_ee.rb

@@ -3,6 +3,7 @@
 resources :merge_requests, only: [], constraints: { id: /\d+/ } do
   member do
     get '/descriptions/:version_id/diff', action: :description_diff, as: :description_diff
+    delete '/descriptions/:version_id', action: :delete_description_version, as: :delete_description_version
     get :metrics_reports
     get :license_management_reports
     get :container_scanning_reports

ee/config/routes/project.rb

@@ -126,6 +126,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
       resources :issues, only: [], constraints: { id: /\d+/ } do
         member do
           get '/descriptions/:version_id/diff', action: :description_diff, as: :description_diff
+          delete '/descriptions/:version_id', action: :delete_description_version, as: :delete_description_version
           get '/designs(/*vueroute)', to: 'issues#designs', as: :designs, format: false
         end
 

ee/spec/models/ee/description_version_spec.rb

@@ -32,4 +32,51 @@ describe DescriptionVersion do
       expect(current_version.previous_version).to eq(previous_version)
     end
   end
+
+  describe '#delete!' do
+    let_it_be(:issue) { create(:issue) }
+    let_it_be(:merge_request) { create(:merge_request) }
+    let_it_be(:epic) { create(:epic) }
+
+    before_all do
+      2.times do
+        create(:description_version, issue: issue)
+        create_list(:description_version, 2, epic: epic)
+        create(:description_version, merge_request: merge_request)
+      end
+    end
+
+    def deleted_count
+      DescriptionVersion
+        .where('issue_id = ? or epic_id = ? or merge_request_id = ?', issue.id, epic.id, merge_request.id)
+        .where('deleted_at IS NOT NULL')
+        .count
+    end
+
+    context 'when start_id is not present' do
+      it 'only soft deletes description_version' do
+        version = epic.description_versions.last
+
+        version.delete!
+
+        expect(version.reload.deleted_at).to be_present
+        expect(deleted_count).to eq(1)
+      end
+    end
+
+    context 'when start_id is present' do
+      it 'soft deletes description versions of same issuable up to start_id' do
+        description_version = epic.description_versions.last.previous_version
+        starting_version = epic.description_versions.second
+
+        description_version.delete!(start_id: starting_version.id)
+
+        expect(epic.description_versions.first.deleted_at).to be_nil
+        expect(epic.description_versions.second.deleted_at).to be_present
+        expect(epic.description_versions.third.deleted_at).to be_present
+        expect(epic.description_versions.fourth.deleted_at).to be_nil
+        expect(deleted_count).to eq(2)
+      end
+    end
+  end
 end

ee/spec/serializers/ee/note_entity_spec.rb

@@ -19,9 +19,11 @@ describe NoteEntity do
       stub_licensed_features(description_diffs: true)
     end
 
-    it 'includes version id and diff path' do
+    it 'includes description versions attributes' do
       expect(subject[:description_version_id]).to eq(description_version.id)
       expect(subject[:description_diff_path]).to eq(description_diff_project_issue_path(issue.project, issue, description_version.id))
+      expect(subject[:delete_description_version_path]).to eq(delete_description_version_project_issue_path(issue.project, issue, description_version.id))
+      expect(subject[:can_delete_description_version]).to eq(true)
     end
   end
 
@@ -30,9 +32,11 @@ describe NoteEntity do
       stub_licensed_features(description_diffs: false)
     end
 
-    it 'does not include version id and diff path' do
+    it 'does not include description versions attributes' do
       expect(subject[:description_version_id]).to be_nil
       expect(subject[:description_diff_path]).to be_nil
+      expect(subject[:delete_description_version_path]).to be_nil
+      expect(subject[:can_delete_description_version]).to be_nil
     end
   end
 end

ee/spec/support/shared_examples/controllers/concerns/description_diff_actions_shared_examples.rb

@@ -3,7 +3,7 @@
 RSpec.shared_examples DescriptionDiffActions do
   let(:base_params) { { namespace_id: project.namespace, project_id: project, id: issuable } }
 
-  describe 'GET description_diff' do
+  describe do
     let_it_be(:version_1) { create(:description_version, issuable.class.name.underscore => issuable) }
     let_it_be(:version_2) { create(:description_version, issuable.class.name.underscore => issuable) }
     let_it_be(:version_3) { create(:description_version, issuable.class.name.underscore => issuable) }
@@ -12,44 +12,119 @@ RSpec.shared_examples DescriptionDiffActions do
       get :description_diff, params: base_params.merge(extra_params)
     end
 
+    def delete_description_version(extra_params = {})
+      delete :delete_description_version, params: base_params.merge(extra_params)
+    end
+
     context 'when license is available' do
       before do
         stub_licensed_features(epics: true, description_diffs: true)
       end
 
-      it 'returns the diff with the previous version' do
-        expect(Gitlab::Diff::CharDiff).to receive(:new).with(version_2.description, version_3.description).and_call_original
+      context 'GET description_diff' do
+        it 'returns the diff with the previous version' do
+          expect(Gitlab::Diff::CharDiff).to receive(:new).with(version_2.description, version_3.description).and_call_original
 
-        get_description_diff(version_id: version_3)
+          get_description_diff(version_id: version_3)
 
-        expect(response.status).to eq(200)
-      end
+          expect(response.status).to eq(200)
+        end
 
-      it 'returns the diff with the previous version of the specified start_version_id' do
-        expect(Gitlab::Diff::CharDiff).to receive(:new).with(version_1.description, version_3.description).and_call_original
+        it 'returns the diff with the previous version of the specified start_version_id' do
+          expect(Gitlab::Diff::CharDiff).to receive(:new).with(version_1.description, version_3.description).and_call_original
 
-        get_description_diff(version_id: version_3, start_version_id: version_2)
+          get_description_diff(version_id: version_3, start_version_id: version_2)
 
-        expect(response.status).to eq(200)
-      end
+          expect(response.status).to eq(200)
+        end
 
-      context 'when description version is from another issuable' do
-        it 'returns 404' do
-          other_version = create(:description_version)
+        context 'when description version is from another issuable' do
+          it 'returns 404' do
+            other_version = create(:description_version)
 
-          get_description_diff(version_id: other_version)
+            get_description_diff(version_id: other_version)
 
-          expect(response.status).to eq(404)
+            expect(response.status).to eq(404)
+          end
+        end
+
+        context 'when start_version_id is from another issuable' do
+          it 'returns 404' do
+            other_version = create(:description_version)
+
+            get_description_diff(version_id: version_3, start_version_id: other_version)
+
+            expect(response.status).to eq(404)
+          end
+        end
+
+        context 'when start_version_id is deleted' do
+          it 'returns 404' do
+            version_2.delete!
+
+            get_description_diff(version_id: version_3, start_version_id: version_2)
+
+            expect(response.status).to eq(404)
+          end
+        end
+
+        context 'when description version is deleted' do
+          it 'returns 404' do
+            version_3.delete!
+
+            delete_description_version(version_id: version_3)
+
+            expect(response.status).to eq(404)
+          end
         end
       end
 
-      context 'when start_version_id is from another issuable' do
-        it 'returns 404' do
-          other_version = create(:description_version)
+      context 'DELETE description_diff' do
+        before do
+          developer_user = create(:user)
+          issuable.resource_parent.add_developer(developer_user)
+          sign_in(developer_user)
+        end
 
-          get_description_diff(version_id: version_3, start_version_id: other_version)
+        it 'returns 200' do
+          delete_description_version(version_id: version_3)
 
-          expect(response.status).to eq(404)
+          expect(response.status).to eq(200)
+          expect(version_3.reload.deleted_at).to be_present
+        end
+
+        context 'when start_version_id is present' do
+          it 'returns 200' do
+            delete_description_version(version_id: version_3, start_version_id: version_1)
+
+            expect(response.status).to eq(200)
+            expect(version_1.reload.deleted_at).to be_present
+            expect(version_2.reload.deleted_at).to be_present
+            expect(version_3.reload.deleted_at).to be_present
+          end
+        end
+
+        context 'when version is already deleted' do
+          it 'returns 404' do
+            version_3.delete!
+
+            delete_description_version(version_id: version_3)
+
+            expect(response.status).to eq(404)
+          end
+        end
+
+        context 'when user cannot admin issuable' do
+          it 'returns 404' do
+            guest_user = create(:user)
+            issuable.resource_parent.add_guest(guest_user)
+            sign_in(guest_user)
+
+            delete_description_version(version_id: version_3)
+
+            expect(response.status).to eq(404)
+            expect(version_3.reload.deleted_at).to be_nil
+          end
         end
       end
     end
@@ -59,10 +134,20 @@ RSpec.shared_examples DescriptionDiffActions do
         stub_licensed_features(epics: true, description_diffs: false)
       end
 
-      it 'returns 404' do
-        get_description_diff(version_id: version_3)
+      context 'GET description_diff' do
+        it 'returns 404' do
+          get_description_diff(version_id: version_3)
+
+          expect(response.status).to eq(404)
+        end
+      end
+
+      context 'DELETE description_diff' do
+        it 'returns 404' do
+          delete_description_version(version_id: version_3)
 
-        expect(response.status).to eq(404)
+          expect(response.status).to eq(404)
+        end
       end
     end
   end