Merge branch '30782-store-issue-mr-description-changes' into 'master'

Store description versions

See merge request gitlab-org/gitlab!17147

app/models/concerns/issuable.rb

@@ -25,6 +25,7 @@ module Issuable
   include UpdatedAtFilterable
   include IssuableStates
   include ClosedAtFilterable
+  include VersionedDescription
 
   TITLE_LENGTH_MAX = 255
   TITLE_HTML_LENGTH_MAX = 800

app/models/concerns/versioned_description.rb

+# frozen_string_literal: true
+
+module VersionedDescription
+  extend ActiveSupport::Concern
+
+  included do
+    attr_accessor :saved_description_version
+
+    has_many :description_versions
+
+    after_update :save_description_version
+  end
+
+  private
+
+  def save_description_version
+    self.saved_description_version = nil
+
+    return unless Feature.enabled?(:save_description_versions, issuing_parent)
+    return unless saved_change_to_description?
+
+    unless description_versions.exists?
+      description_versions.create!(
+        description: description_before_last_save,
+        created_at: created_at
+      )
+    end
+
+    self.saved_description_version = description_versions.create!(description: description)
+  end
+end

app/models/description_version.rb

+# frozen_string_literal: true
+
+class DescriptionVersion < ApplicationRecord
+  belongs_to :issue
+  belongs_to :merge_request
+
+  validate :exactly_one_issuable
+
+  def self.issuable_attrs
+    %i(issue merge_request).freeze
+  end
+
+  private
+
+  def exactly_one_issuable
+    issuable_count = self.class.issuable_attrs.count { |attr| self["#{attr}_id"] }
+
+    errors.add(:base, "Exactly one of #{self.class.issuable_attrs.join(', ')} is required") if issuable_count != 1
+  end
+end
+
+DescriptionVersion.prepend_if_ee('EE::DescriptionVersion')

app/models/system_note_metadata.rb

@@ -23,6 +23,7 @@ class SystemNoteMetadata < ApplicationRecord
   validates :action, inclusion: { in: :icon_types }, allow_nil: true
 
   belongs_to :note
+  belongs_to :description_version
 
   def icon_types
     ICON_TYPES

app/services/issuable/clone/content_rewriter.rb

@@ -39,6 +39,10 @@ module Issuable
 
           if note.system_note_metadata
             new_params[:system_note_metadata] = note.system_note_metadata.dup
+
+            # TODO: Implement copying of description versions when an issue is moved
+            # https://gitlab.com/gitlab-org/gitlab/issues/32300
+            new_params[:system_note_metadata].description_version = nil
           end
 
           new_note.update(new_params)

app/services/note_summary.rb

@@ -10,6 +10,10 @@ class NoteSummary
               project: project, author: author, note: body }
     @metadata = { action: action, commit_count: commit_count }.compact
 
+    if action == 'description' && noteable.saved_description_version
+      @metadata[:description_version] = noteable.saved_description_version
+    end
+
     set_commit_params if note[:noteable].is_a?(Commit)
   end
 

db/migrate/20190927055500_create_description_versions.rb

+# frozen_string_literal: true
+
+class CreateDescriptionVersions < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    create_table :description_versions do |t|
+      t.timestamps_with_timezone
+      t.references :issue, index: false, foreign_key: { on_delete: :cascade }, type: :integer
+      t.references :merge_request, index: false, foreign_key: { on_delete: :cascade }, type: :integer
+      t.references :epic, index: false, foreign_key: { on_delete: :cascade }, type: :integer
+      t.text :description
+    end
+
+    add_index :description_versions, :issue_id, where: 'issue_id IS NOT NULL'
+    add_index :description_versions, :merge_request_id, where: 'merge_request_id IS NOT NULL'
+    add_index :description_versions, :epic_id, where: 'epic_id IS NOT NULL'
+
+    add_column :system_note_metadata, :description_version_id, :bigint
+  end
+
+  def down
+    remove_column :system_note_metadata, :description_version_id
+
+    drop_table :description_versions
+  end
+end

db/migrate/20190927055540_add_index_to_sytem_note_metadata_description_version_id.rb

+# frozen_string_literal: true
+
+class AddIndexToSytemNoteMetadataDescriptionVersionId < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :system_note_metadata, :description_version_id, unique: true, where: 'description_version_id IS NOT NULL'
+    add_concurrent_foreign_key :system_note_metadata, :description_versions, column: :description_version_id, on_delete: :nullify
+  end
+
+  def down
+    remove_foreign_key :system_note_metadata, column: :description_version_id
+    remove_concurrent_index :system_note_metadata, :description_version_id
+  end
+end

db/schema.rb

@@ -1266,6 +1266,18 @@ ActiveRecord::Schema.define(version: 2019_10_16_220135) do
     t.index ["project_id", "status"], name: "index_deployments_on_project_id_and_status"
   end
 
+  create_table "description_versions", force: :cascade do |t|
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
+    t.integer "issue_id"
+    t.integer "merge_request_id"
+    t.integer "epic_id"
+    t.text "description"
+    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)"
+  end
+
   create_table "design_management_designs", force: :cascade do |t|
     t.integer "project_id", null: false
     t.integer "issue_id"
@@ -3494,6 +3506,8 @@ ActiveRecord::Schema.define(version: 2019_10_16_220135) do
     t.string "action"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
+    t.bigint "description_version_id"
+    t.index ["description_version_id"], name: "index_system_note_metadata_on_description_version_id", unique: true, where: "(description_version_id IS NOT NULL)"
     t.index ["note_id"], name: "index_system_note_metadata_on_note_id", unique: true
   end
 
@@ -4089,6 +4103,9 @@ ActiveRecord::Schema.define(version: 2019_10_16_220135) do
   add_foreign_key "deploy_keys_projects", "projects", name: "fk_58a901ca7e", on_delete: :cascade
   add_foreign_key "deployments", "clusters", name: "fk_289bba3222", on_delete: :nullify
   add_foreign_key "deployments", "projects", name: "fk_b9a3851b82", on_delete: :cascade
+  add_foreign_key "description_versions", "epics", on_delete: :cascade
+  add_foreign_key "description_versions", "issues", on_delete: :cascade
+  add_foreign_key "description_versions", "merge_requests", on_delete: :cascade
   add_foreign_key "design_management_designs", "issues", on_delete: :cascade
   add_foreign_key "design_management_designs", "projects", on_delete: :cascade
   add_foreign_key "design_management_designs_versions", "design_management_designs", column: "design_id", name: "fk_03c671965c", on_delete: :cascade
@@ -4325,6 +4342,7 @@ ActiveRecord::Schema.define(version: 2019_10_16_220135) do
   add_foreign_key "software_license_policies", "software_licenses", on_delete: :cascade
   add_foreign_key "subscriptions", "projects", on_delete: :cascade
   add_foreign_key "suggestions", "notes", on_delete: :cascade
+  add_foreign_key "system_note_metadata", "description_versions", name: "fk_fbd87415c9", on_delete: :nullify
   add_foreign_key "system_note_metadata", "notes", name: "fk_d83a918cb1", on_delete: :cascade
   add_foreign_key "term_agreements", "application_setting_terms", column: "term_id"
   add_foreign_key "term_agreements", "users", on_delete: :cascade

ee/app/models/ee/description_version.rb

+# frozen_string_literal: true
+
+module EE
+  module DescriptionVersion
+    extend ActiveSupport::Concern
+
+    prepended do
+      belongs_to :epic
+    end
+
+    class_methods do
+      def issuable_attrs
+        (super + %i(epic)).freeze
+      end
+    end
+  end
+end

ee/spec/models/ee/description_version_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DescriptionVersion do
+  describe 'associations' do
+    it { is_expected.to belong_to :epic }
+  end
+
+  describe 'validations' do
+    it 'is valid when epic_id is set' do
+      expect(described_class.new(epic_id: 1)).to be_valid
+    end
+  end
+end

ee/spec/models/epic_spec.rb

@@ -794,4 +794,6 @@ describe Epic do
       let(:default_params) { {} }
     end
   end
+
+  it_behaves_like 'versioned description'
 end

spec/lib/gitlab/import_export/all_models.yml

@@ -25,6 +25,7 @@ issues:
 - epic
 - designs
 - design_versions
+- description_versions
 - prometheus_alerts
 - prometheus_alert_events
 - self_managed_prometheus_alert_events
@@ -132,6 +133,7 @@ merge_requests:
 - blocks_as_blockee
 - blocking_merge_requests
 - blocked_merge_requests
+- description_versions
 external_pull_requests:
 - project
 merge_request_diff:

spec/models/description_version_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DescriptionVersion do
+  describe 'associations' do
+    it { is_expected.to belong_to :issue }
+    it { is_expected.to belong_to :merge_request }
+  end
+
+  describe 'validations' do
+    describe 'exactly_one_issuable' do
+      using RSpec::Parameterized::TableSyntax
+
+      subject { described_class.new(issue_id: issue_id, merge_request_id: merge_request_id).valid? }
+
+      where(:issue_id, :merge_request_id, :valid?) do
+        nil | 1   | true
+        1   | nil | true
+        nil | nil | false
+        1   | 1   | false
+      end
+
+      with_them do
+        it { is_expected.to eq(valid?) }
+      end
+    end
+  end
+end

spec/models/issue_spec.rb

@@ -899,4 +899,6 @@ describe Issue do
       let(:default_params) { { project: project } }
     end
   end
+
+  it_behaves_like 'versioned description'
 end

spec/models/merge_request_spec.rb

@@ -3331,4 +3331,6 @@ describe MergeRequest do
 
     it { expect(query).to contain_exactly(merge_request1, merge_request2) }
   end
+
+  it_behaves_like 'versioned description'
 end

spec/models/system_note_metadata_spec.rb

@@ -5,6 +5,7 @@ require 'spec_helper'
 describe SystemNoteMetadata do
   describe 'associations' do
     it { is_expected.to belong_to(:note) }
+    it { is_expected.to belong_to(:description_version) }
   end
 
   describe 'validation' do

spec/services/note_summary_spec.rb

@@ -46,5 +46,17 @@ describe NoteSummary do
     it 'returns metadata hash' do
       expect(create_note_summary.metadata).to eq(action: 'icon', commit_count: 5)
     end
+
+    context 'description action and noteable has saved_description_version' do
+      before do
+        noteable.saved_description_version = 1
+      end
+
+      subject { described_class.new(noteable, project, user, 'note', action: 'description') }
+
+      it 'sets the description_version metadata' do
+        expect(subject.metadata).to include(description_version: 1)
+      end
+    end
   end
 end

spec/services/system_notes/issuables_service_spec.rb

@@ -212,6 +212,15 @@ describe ::SystemNotes::IssuablesService do
       it 'sets the note text' do
         expect(subject.note).to eq('changed the description')
       end
+
+      it 'associates the related description version' do
+        noteable.update!(description: 'New description')
+
+        description_version_id = subject.system_note_metadata.description_version_id
+
+        expect(description_version_id).not_to be_nil
+        expect(description_version_id).to eq(noteable.saved_description_version.id)
+      end
     end
   end
 

spec/support/shared_examples/versioned_description_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'versioned description' do
+  describe 'associations' do
+    it { is_expected.to have_many(:description_versions) }
+  end
+
+  describe 'save_description_version' do
+    let(:factory_name) { described_class.name.underscore.to_sym }
+    let!(:model) { create(factory_name, description: 'Original description') }
+
+    context 'when feature is enabled' do
+      before do
+        stub_feature_flags(save_description_versions: true)
+      end
+
+      context 'when description was changed' do
+        before do
+          model.update!(description: 'New description')
+        end
+
+        it 'saves the old and new description for the first update' do
+          expect(model.description_versions.first.description).to eq('Original description')
+          expect(model.description_versions.last.description).to eq('New description')
+        end
+
+        it 'only saves the new description for subsequent updates' do
+          expect { model.update!(description: 'Another description') }.to change { model.description_versions.count }.by(1)
+
+          expect(model.description_versions.last.description).to eq('Another description')
+        end
+
+        it 'sets the new description version to `saved_description_version`' do
+          expect(model.saved_description_version).to eq(model.description_versions.last)
+        end
+
+        it 'clears `saved_description_version` after another save that does not change description' do
+          model.save!
+
+          expect(model.saved_description_version).to be_nil
+        end
+      end
+
+      context 'when description was not changed' do
+        it 'does not save any description version' do
+          expect { model.save! }.not_to change { model.description_versions.count }
+
+          expect(model.saved_description_version).to be_nil
+        end
+      end
+    end
+
+    context 'when feature is disabled' do
+      before do
+        stub_feature_flags(save_description_versions: false)
+      end
+
+      it 'does not save any description version' do
+        expect { model.update!(description: 'New description') }.not_to change { model.description_versions.count }
+
+        expect(model.saved_description_version).to be_nil
+      end
+    end
+  end
+end