Merge branch 'ph/24468/outdatedDiscussionDiffLines' into 'master'

Displays change that made a diff line outdated

See merge request gitlab-org/gitlab!72597

app/assets/javascripts/vue_shared/components/notes/system_note.vue

@@ -26,6 +26,7 @@ import {
 import $ from 'jquery';
 import { mapGetters, mapActions, mapState } from 'vuex';
 import descriptionVersionHistoryMixin from 'ee_else_ce/notes/mixins/description_version_history';
+import axios from '~/lib/utils/axios_utils';
 import { __ } from '~/locale';
 import initMRPopovers from '~/mr_popover/';
 import noteHeader from '~/notes/components/note_header.vue';
@@ -61,6 +62,9 @@ export default {
   data() {
     return {
       expanded: false,
+      lines: [],
+      showLines: false,
+      loadingDiff: false,
     };
   },
   computed: {
@@ -94,10 +98,25 @@ export default {
   },
   methods: {
     ...mapActions(['fetchDescriptionVersion', 'softDeleteDescriptionVersion']),
+    async toggleDiff() {
+      this.showLines = !this.showLines;
+
+      if (!this.lines.length) {
+        this.loadingDiff = true;
+        const { data } = await axios.get(this.note.outdated_line_change_path);
+
+        this.lines = data.map((l) => ({
+          ...l,
+          rich_text: l.rich_text.replace(/^[+ -]/, ''),
+        }));
+        this.loadingDiff = false;
+      }
+    },
   },
   safeHtmlConfig: {
     ADD_TAGS: ['use'], // to support icon SVGs
   },
+  userColorSchemeClass: window.gon.user_color_scheme,
 };
 </script>
 
@@ -112,15 +131,28 @@ export default {
       <div class="note-header">
         <note-header :author="note.author" :created-at="note.created_at" :note-id="note.id">
           <span v-safe-html="actionTextHtml"></span>
-          <template v-if="canSeeDescriptionVersion" #extra-controls>
+          <template
+            v-if="canSeeDescriptionVersion || note.outdated_line_change_path"
+            #extra-controls
+          >
             &middot;
             <gl-button
+              v-if="canSeeDescriptionVersion"
               variant="link"
               :icon="descriptionVersionToggleIcon"
               data-testid="compare-btn"
               @click="toggleDescriptionVersion"
               >{{ __('Compare with previous version') }}</gl-button
             >
+            <gl-button
+              v-if="note.outdated_line_change_path"
+              :icon="showLines ? 'chevron-up' : 'chevron-down'"
+              variant="link"
+              data-testid="outdated-lines-change-btn"
+              @click="toggleDiff"
+            >
+              {{ __('Compare changes') }}
+            </gl-button>
           </template>
         </note-header>
       </div>
@@ -154,6 +186,37 @@ export default {
             @click="deleteDescriptionVersion"
           />
         </div>
+        <div
+          v-if="lines.length && showLines"
+          class="diff-content gl-border-solid gl-border-1 gl-border-gray-200 gl-mt-4 gl-rounded-small gl-overflow-hidden"
+        >
+          <table
+            :class="$options.userColorSchemeClass"
+            class="code js-syntax-highlight"
+            data-testid="outdated-lines"
+          >
+            <tr v-for="line in lines" v-once :key="line.line_code" class="line_holder">
+              <td
+                :class="line.type"
+                class="diff-line-num old_line gl-border-bottom-0! gl-border-top-0!"
+              >
+                {{ line.old_line }}
+              </td>
+              <td
+                :class="line.type"
+                class="diff-line-num new_line gl-border-bottom-0! gl-border-top-0!"
+              >
+                {{ line.new_line }}
+              </td>
+              <td
+                :class="line.type"
+                class="line_content gl-display-table-cell!"
+                v-html="line.rich_text /* eslint-disable-line vue/no-v-html */"
+              ></td>
+            </tr>
+          </table>
+        </div>
+        <gl-skeleton-loading v-else-if="showLines" class="gl-mt-4" />
       </div>
     </div>
   </timeline-entry-item>

app/controllers/projects/notes_controller.rb

@@ -55,6 +55,14 @@ class Projects::NotesController < Projects::ApplicationController
     end
   end
 
+  def outdated_line_change
+    diff_lines = Rails.cache.fetch(['note', note.id, 'oudated_line_change'], expires_in: 7.days) do
+      ::MergeRequests::OutdatedDiscussionDiffLinesService.new(project: @project, note: note).execute.to_json
+    end
+
+    render json: diff_lines
+  end
+
   private
 
   def render_json_with_notes_serializer

app/serializers/note_entity.rb

@@ -51,6 +51,10 @@ class NoteEntity < API::Entities::Note
     SystemNoteHelper.system_note_icon_name(note)
   end
 
+  expose :outdated_line_change_path, if: -> (note, _) { note.system? && note.change_position&.line_range && Feature.enabled?(:display_outdated_line_diff, note.project, default_enabled: :yaml) } do |note|
+    outdated_line_change_namespace_project_note_path(namespace_id: note.project.namespace, project_id: note.project, id: note)
+  end
+
   expose :is_noteable_author do |note|
     note.noteable_author?(request.noteable)
   end

app/services/merge_requests/outdated_discussion_diff_lines_service.rb

+# frozen_string_literal: true
+
+module MergeRequests
+  class OutdatedDiscussionDiffLinesService
+    include Gitlab::Utils::StrongMemoize
+
+    attr_reader :project, :note
+
+    OVERFLOW_LINES_COUNT = 2
+
+    def initialize(project:, note:)
+      @project = project
+      @note = note
+    end
+
+    def execute
+      end_position = position.line_range["end"]
+      diff_line_index = diff_lines.find_index { |l| l.new_line == end_position["new_line"] || l.old_line == end_position["old_line"] }
+      initial_line_index = [diff_line_index - OVERFLOW_LINES_COUNT, 0].max
+      last_line_index = [diff_line_index + OVERFLOW_LINES_COUNT, diff_lines.length].min
+
+      prev_lines = []
+
+      diff_lines[initial_line_index..last_line_index].each do |line|
+        if line.meta?
+          prev_lines.clear
+        else
+          prev_lines << line
+        end
+      end
+
+      prev_lines
+    end
+
+    private
+
+    def position
+      note.change_position
+    end
+
+    def repository
+      project.repository
+    end
+
+    def diff_file
+      position.diff_file(repository)
+    end
+
+    def diff_lines
+      strong_memoize(:diff_lines) do
+        diff_file.highlighted_diff_lines
+      end
+    end
+  end
+end

config/feature_flags/development/display_outdated_line_diff.yml

+---
+name: display_outdated_line_diff
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/72597
+rollout_issue_url:
+milestone: '14.5'
+type: development
+group: group::code review
+default_enabled: false

config/routes/project.rb

@@ -540,6 +540,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
           delete :delete_attachment # rubocop:todo Cop/PutProjectRoutesUnderScope
           post :resolve # rubocop:todo Cop/PutProjectRoutesUnderScope
           delete :resolve, action: :unresolve # rubocop:todo Cop/PutProjectRoutesUnderScope
+          get :outdated_line_change # rubocop:todo Cop/PutProjectRoutesUnderScope
         end
       end
 

lib/gitlab/diff/position_tracer/line_strategy.rb

@@ -104,7 +104,7 @@ module Gitlab
                 # the current state on the CD diff, so we treat it as outdated.
                 ac_diff = ac_diffs.diff_file_with_new_path(c_path, c_mode)
 
-                { position: new_position(ac_diff, nil, c_line), outdated: true }
+                { position: new_position(ac_diff, nil, c_line, position.line_range), outdated: true }
               end
             else
               # If the line is still in D and not in C, it is still added.
@@ -112,7 +112,7 @@ module Gitlab
             end
           else
             # If the line is no longer in D, it has been removed from the MR.
-            { position: new_position(bd_diff, b_line, nil), outdated: true }
+            { position: new_position(bd_diff, b_line, nil, position.line_range), outdated: true }
           end
         end
 
@@ -140,14 +140,14 @@ module Gitlab
               # removed line into an unchanged one.
               bd_diff = bd_diffs.diff_file_with_new_path(d_path, d_mode)
 
-              { position: new_position(bd_diff, nil, d_line), outdated: true }
+              { position: new_position(bd_diff, nil, d_line, position.line_range), outdated: true }
             else
               # If the line is still in C and not in D, it is still removed.
               { position: new_position(cd_diff, c_line, nil, position.line_range), outdated: false }
             end
           else
             # If the line is no longer in C, it has been removed outside of the MR.
-            { position: new_position(ac_diff, a_line, nil), outdated: true }
+            { position: new_position(ac_diff, a_line, nil, position.line_range), outdated: true }
           end
         end
 

spec/controllers/projects/notes_controller_spec.rb

@@ -1007,6 +1007,35 @@ RSpec.describe Projects::NotesController do
     end
   end
 
+  describe 'GET outdated_line_change' do
+    let(:request_params) do
+      {
+        namespace_id: project.namespace,
+        project_id: project,
+        id: note,
+        format: 'json'
+      }
+    end
+
+    before do
+      service = double
+      allow(service).to receive(:execute).and_return([{ line_text: 'Test' }])
+      allow(MergeRequests::OutdatedDiscussionDiffLinesService).to receive(:new).once.and_return(service)
+
+      sign_in(user)
+      project.add_developer(user)
+    end
+
+    it "successfully renders expected JSON response" do
+      get :outdated_line_change, params: request_params
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(json_response).to be_an(Array)
+      expect(json_response.count).to eq(1)
+      expect(json_response.first).to include({ "line_text" => "Test" })
+    end
+  end
+
   # Convert a time to an integer number of microseconds
   def microseconds(time)
     (time.to_i * 1_000_000) + time.usec

spec/frontend/vue_shared/components/notes/system_note_spec.js

+import MockAdapter from 'axios-mock-adapter';
 import { mount } from '@vue/test-utils';
+import waitForPromises from 'helpers/wait_for_promises';
 import initMRPopovers from '~/mr_popover/index';
 import createStore from '~/notes/stores';
 import IssueSystemNote from '~/vue_shared/components/notes/system_note.vue';
+import axios from '~/lib/utils/axios_utils';
 
 jest.mock('~/mr_popover/index', () => jest.fn());
 
 describe('system note component', () => {
   let vm;
   let props;
+  let mock;
+
+  function createComponent(propsData = {}) {
+    const store = createStore();
+    store.dispatch('setTargetNoteHash', `note_${props.note.id}`);
+
+    vm = mount(IssueSystemNote, {
+      store,
+      propsData,
+    });
+  }
 
   beforeEach(() => {
     props = {
@@ -27,28 +41,29 @@ describe('system note component', () => {
       },
     };
 
-    const store = createStore();
-    store.dispatch('setTargetNoteHash', `note_${props.note.id}`);
-
-    vm = mount(IssueSystemNote, {
-      store,
-      propsData: props,
-    });
+    mock = new MockAdapter(axios);
   });
 
   afterEach(() => {
     vm.destroy();
+    mock.restore();
   });
 
   it('should render a list item with correct id', () => {
+    createComponent(props);
+
     expect(vm.attributes('id')).toEqual(`note_${props.note.id}`);
   });
 
   it('should render target class is note is target note', () => {
+    createComponent(props);
+
     expect(vm.classes()).toContain('target');
   });
 
   it('should render svg icon', () => {
+    createComponent(props);
+
     expect(vm.find('.timeline-icon svg').exists()).toBe(true);
   });
 
@@ -56,10 +71,31 @@ describe('system note component', () => {
   // we need to strip them because they break layout of commit lists in system notes:
   // https://gitlab.com/gitlab-org/gitlab-foss/uploads/b07a10670919254f0220d3ff5c1aa110/jqzI.png
   it('removes wrapping paragraph from note HTML', () => {
+    createComponent(props);
+
     expect(vm.find('.system-note-message').html()).toContain('<span>closed</span>');
   });
 
   it('should initMRPopovers onMount', () => {
+    createComponent(props);
+
     expect(initMRPopovers).toHaveBeenCalled();
   });
+
+  it('renders outdated code lines', async () => {
+    mock
+      .onGet('/outdated_line_change_path')
+      .reply(200, [
+        { rich_text: 'console.log', type: 'new', line_code: '123', old_line: null, new_line: 1 },
+      ]);
+
+    createComponent({
+      note: { ...props.note, outdated_line_change_path: '/outdated_line_change_path' },
+    });
+
+    await vm.find("[data-testid='outdated-lines-change-btn']").trigger('click');
+    await waitForPromises();
+
+    expect(vm.find("[data-testid='outdated-lines']").exists()).toBe(true);
+  });
 });

spec/lib/gitlab/diff/position_tracer/line_strategy_spec.rb

@@ -581,13 +581,16 @@ RSpec.describe Gitlab::Diff::PositionTracer::LineStrategy, :clean_gitlab_redis_c
                   )
                 end
 
-                it "returns the new position but drops line_range information" do
+                it "returns the new position" do
                   expect_change_position(
                     old_path: file_name,
                     new_path: file_name,
                     old_line: nil,
                     new_line: 2,
-                    line_range: nil
+                    line_range: {
+                      "start_line_code" => 1,
+                      "end_line_code"   => 2
+                    }
                   )
                 end
               end