Commit c4b6caf4 authored by Kushal Pandya's avatar Kushal Pandya

Merge branch '13382-include-description-change-diffs-when-querying-notes' into 'master'

Resolve "Include description change diffs when querying notes"

See merge request gitlab-org/gitlab!17670
parents 5ac7468c ae8b1240
......@@ -159,6 +159,7 @@ gem 'icalendar'
# Diffs
gem 'diffy', '~> 3.1.0'
gem 'diff_match_patch', '~> 0.1.0'
# Application server
gem 'rack', '~> 2.0.7'
......
......@@ -224,6 +224,7 @@ GEM
railties
rotp (~> 2.0)
diff-lcs (1.3)
diff_match_patch (0.1.0)
diffy (3.1.0)
discordrb-webhooks-blackst0ne (3.3.0)
rest-client (~> 2.0)
......@@ -1133,6 +1134,7 @@ DEPENDENCIES
device_detector
devise (~> 4.6)
devise-two-factor (~> 3.0.0)
diff_match_patch (~> 0.1.0)
diffy (~> 3.1.0)
discordrb-webhooks-blackst0ne (~> 3.3)
doorkeeper (~> 4.3)
......
......@@ -101,6 +101,7 @@ export default {
<time-ago-tooltip :time="createdAt" tooltip-placement="bottom" />
</a>
</template>
<slot name="extra-controls"></slot>
<i
class="fa fa-spinner fa-spin editing-spinner"
:aria-label="__('Comment is being updated')"
......
// Placeholder for GitLab FOSS
// Actual implementation: ee/app/assets/javascripts/notes/mixins/description_version_history.js
export default {
computed: {
canSeeDescriptionVersion() {},
shouldShowDescriptionVersion() {},
descriptionVersionToggleIcon() {},
},
methods: {
toggleDescriptionVersion() {},
},
};
......@@ -12,6 +12,7 @@ import service from '../services/notes_service';
import loadAwardsHandler from '../../awards_handler';
import sidebarTimeTrackingEventHub from '../../sidebar/event_hub';
import { isInViewport, scrollToElement, isInMRPage } from '../../lib/utils/common_utils';
import { mergeUrlParams } from '../../lib/utils/url_utility';
import mrWidgetEventHub from '../../vue_merge_request_widget/event_hub';
import { __ } from '~/locale';
import Api from '~/api';
......@@ -475,5 +476,20 @@ export const convertToDiscussion = ({ commit }, noteId) =>
export const removeConvertedDiscussion = ({ commit }, noteId) =>
commit(types.REMOVE_CONVERTED_DISCUSSION, noteId);
export const fetchDescriptionVersion = (_, { endpoint, startingVersion }) => {
let requestUrl = endpoint;
if (startingVersion) {
requestUrl = mergeUrlParams({ start_version_id: startingVersion }, requestUrl);
}
return axios
.get(requestUrl)
.then(res => res.data)
.catch(() => {
Flash(__('Something went wrong while fetching description changes. Please try again.'));
});
};
// prevent babel-plugin-rewire from generating an invalid default during karma tests
export default () => {};
import { n__, s__, sprintf } from '~/locale';
import { DESCRIPTION_TYPE } from '../constants';
/**
* Changes the description from a note, returns 'changed the description n number of times'
*/
export const changeDescriptionNote = (note, descriptionChangedTimes, timeDifferenceMinutes) => {
const descriptionNote = Object.assign({}, note);
descriptionNote.note_html = sprintf(
s__(`MergeRequest|
%{paragraphStart}changed the description %{descriptionChangedTimes} times %{timeDifferenceMinutes}%{paragraphEnd}`),
{
paragraphStart: '<p dir="auto">',
paragraphEnd: '</p>',
descriptionChangedTimes,
timeDifferenceMinutes: n__('within %d minute ', 'within %d minutes ', timeDifferenceMinutes),
},
false,
);
descriptionNote.times_updated = descriptionChangedTimes;
return descriptionNote;
};
/**
* Checks the time difference between two notes from their 'created_at' dates
* returns an integer
*/
export const getTimeDifferenceMinutes = (noteBeggining, noteEnd) => {
const descriptionNoteBegin = new Date(noteBeggining.created_at);
const descriptionNoteEnd = new Date(noteEnd.created_at);
......@@ -57,7 +32,6 @@ export const isDescriptionSystemNote = note => note.system && note.note === DESC
export const collapseSystemNotes = notes => {
let lastDescriptionSystemNote = null;
let lastDescriptionSystemNoteIndex = -1;
let descriptionChangedTimes = 1;
return notes.slice(0).reduce((acc, currentNote) => {
const note = currentNote.notes[0];
......@@ -70,32 +44,24 @@ export const collapseSystemNotes = notes => {
} else if (lastDescriptionSystemNote) {
const timeDifferenceMinutes = getTimeDifferenceMinutes(lastDescriptionSystemNote, note);
// are they less than 10 minutes apart?
if (timeDifferenceMinutes > 10) {
// reset counter
descriptionChangedTimes = 1;
// are they less than 10 minutes apart from the same user?
if (timeDifferenceMinutes > 10 || note.author.id !== lastDescriptionSystemNote.author.id) {
// update the previous system note
lastDescriptionSystemNote = note;
lastDescriptionSystemNoteIndex = acc.length;
} else {
// increase counter
descriptionChangedTimes += 1;
// set the first version to fetch grouped system note versions
note.start_description_version_id = lastDescriptionSystemNote.description_version_id;
// delete the previous one
acc.splice(lastDescriptionSystemNoteIndex, 1);
// replace the text of the current system note with the collapsed note.
currentNote.notes.splice(
0,
1,
changeDescriptionNote(note, descriptionChangedTimes, timeDifferenceMinutes),
);
// update the previous system note index
lastDescriptionSystemNoteIndex = acc.length;
}
}
}
acc.push(currentNote);
return acc;
}, []);
......
......@@ -17,9 +17,11 @@
* />
*/
import $ from 'jquery';
import { mapGetters } from 'vuex';
import { mapGetters, mapActions } from 'vuex';
import { GlSkeletonLoading } from '@gitlab/ui';
import noteHeader from '~/notes/components/note_header.vue';
import Icon from '~/vue_shared/components/icon.vue';
import descriptionVersionHistoryMixin from 'ee_else_ce/notes/mixins/description_version_history';
import TimelineEntryItem from './timeline_entry_item.vue';
import { spriteIcon } from '../../../lib/utils/common_utils';
import initMRPopovers from '~/mr_popover/';
......@@ -32,7 +34,9 @@ export default {
Icon,
noteHeader,
TimelineEntryItem,
GlSkeletonLoading,
},
mixins: [descriptionVersionHistoryMixin],
props: {
note: {
type: Object,
......@@ -75,13 +79,16 @@ export default {
mounted() {
initMRPopovers(this.$el.querySelectorAll('.gfm-merge_request'));
},
methods: {
...mapActions(['fetchDescriptionVersion']),
},
};
</script>
<template>
<timeline-entry-item
:id="noteAnchorId"
:class="{ target: isTargetNote }"
:class="{ target: isTargetNote, 'pr-0': shouldShowDescriptionVersion }"
class="note system-note note-wrapper"
>
<div class="timeline-icon" v-html="iconHtml"></div>
......@@ -89,14 +96,18 @@ export default {
<div class="note-header">
<note-header :author="note.author" :created-at="note.created_at" :note-id="note.id">
<span v-html="actionTextHtml"></span>
<template v-if="canSeeDescriptionVersion" slot="extra-controls">
&middot;
<button type="button" class="btn-blank btn-link" @click="toggleDescriptionVersion">
{{ __('Compare with previous version') }}
<icon :name="descriptionVersionToggleIcon" :size="12" class="append-left-5" />
</button>
</template>
</note-header>
</div>
<div class="note-body">
<div
:class="{
'system-note-commit-list': hasMoreCommits,
'hide-shade': expanded,
}"
:class="{ 'system-note-commit-list': hasMoreCommits, 'hide-shade': expanded }"
class="note-text md"
v-html="note.note_html"
></div>
......@@ -106,6 +117,12 @@ export default {
<span>{{ __('Toggle commit list') }}</span>
</div>
</div>
<div v-if="shouldShowDescriptionVersion" class="description-version pt-2">
<pre v-if="isLoadingDescriptionVersion" class="loading-state">
<gl-skeleton-loading />
</pre>
<pre v-else class="wrapper mt-2" v-html="descriptionVersion"></pre>
</div>
</div>
</div>
</timeline-entry-item>
......
......@@ -310,6 +310,17 @@ $note-form-margin-left: 72px;
.note-body {
overflow: hidden;
.description-version {
pre {
max-height: $dropdown-max-height-lg;
white-space: pre-wrap;
&.loading-state {
height: 94px;
}
}
}
.system-note-commit-list-toggler {
color: $blue-600;
padding: 10px 0 0;
......
......@@ -10,6 +10,10 @@ class DescriptionVersion < ApplicationRecord
%i(issue merge_request).freeze
end
def issuable
issue || merge_request
end
private
def exactly_one_issuable
......
......@@ -79,3 +79,5 @@ class NoteEntity < API::Entities::Note
request.current_user
end
end
NoteEntity.prepend_if_ee('EE::NoteEntity')
export default {
data() {
return {
isLoadingDescriptionVersion: false,
isDescriptionVersionExpanded: false,
descriptionVersion: '',
};
},
computed: {
canSeeDescriptionVersion() {
return Boolean(this.note.description_diff_path && this.note.description_version_id);
},
shouldShowDescriptionVersion() {
return this.canSeeDescriptionVersion && this.isDescriptionVersionExpanded;
},
descriptionVersionToggleIcon() {
return this.isDescriptionVersionExpanded ? 'chevron-up' : 'chevron-down';
},
},
methods: {
toggleDescriptionVersion() {
this.isDescriptionVersionExpanded = !this.isDescriptionVersionExpanded;
if (this.descriptionVersion) {
return false;
}
this.isLoadingDescriptionVersion = true;
const endpoint = this.note.description_diff_path;
const startingVersion = this.note.start_description_version_id;
return this.fetchDescriptionVersion({ endpoint, startingVersion }).then(diff => {
this.isLoadingDescriptionVersion = false;
this.descriptionVersion = diff;
});
},
},
};
# frozen_string_literal: true
module DescriptionDiffActions
extend ActiveSupport::Concern
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
return render_404 if previous_version.nil?
diff = Gitlab::Diff::CharDiff.new(previous_version.description, current_version.description)
diff.generate_diff
render html: diff.to_html
end
end
......@@ -7,6 +7,8 @@ module EE
extend ::Gitlab::Utils::Override
prepended do
include DescriptionDiffActions
# Specifying before_action :authenticate_user! multiple times
# doesn't work, since the last filter will override the previous
# ones.
......
......@@ -8,6 +8,8 @@ module EE
APPROVAL_RENDERING_ACTIONS = [:approve, :approvals, :unapprove].freeze
prepended do
include DescriptionDiffActions
before_action only: [:show] do
push_frontend_feature_flag(:sast_merge_request_report_api, default_enabled: true)
push_frontend_feature_flag(:dast_merge_request_report_api)
......
......@@ -7,6 +7,7 @@ class Groups::EpicsController < Groups::ApplicationController
include ToggleSubscriptionAction
include RendersNotes
include EpicsActions
include DescriptionDiffActions
before_action :check_epics_available!
before_action :epic, except: [:index, :create, :bulk_update]
......
......@@ -32,5 +32,16 @@ module EE
data
end
def description_diff_path(issuable, version_id)
case issuable
when Issue
description_diff_project_issue_path(issuable.project, issuable, version_id)
when MergeRequest
description_diff_project_merge_request_path(issuable.project, issuable, version_id)
when Epic
description_diff_group_epic_path(issuable.group, issuable, version_id)
end
end
end
end
......@@ -13,5 +13,19 @@ module EE
(super + %i(epic)).freeze
end
end
def issuable
epic || super
end
def previous_version
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
......@@ -61,6 +61,7 @@ class License < ApplicationRecord
default_project_deletion_protection
dependency_proxy
deploy_board
description_diffs
design_management
email_additional_text
extended_audit_events
......
# frozen_string_literal: true
module EE
module NoteEntity
extend ActiveSupport::Concern
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
end
private
def description_version_id
object.system_note_metadata&.description_version_id
end
end
end
end
......@@ -72,6 +72,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
resources :billings, only: [:index]
resources :epics, concerns: :awardable, constraints: { id: /\d+/ } do
member do
get '/descriptions/:version_id/diff', action: :description_diff, as: :description_diff
get :discussions, format: :json
get :realtime_changes
post :toggle_subscription
......
......@@ -69,8 +69,15 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
end
end
resources :issues, only: [], constraints: { id: /\d+/ } do
member do
get '/descriptions/:version_id/diff', action: :description_diff, as: :description_diff
end
end
resources :merge_requests, only: [], constraints: { id: /\d+/ } do
member do
get '/descriptions/:version_id/diff', action: :description_diff, as: :description_diff
get :metrics_reports
get :license_management_reports
get :container_scanning_reports
......
# frozen_string_literal: true
module Gitlab
module Diff
class CharDiff
include Gitlab::Utils::StrongMemoize
def initialize(old_string, new_string)
@old_string = old_string.to_s
@new_string = new_string.to_s
@changes = []
end
def generate_diff
@changes = diff_match_patch.diff_main(@old_string, @new_string)
diff_match_patch.diff_cleanupSemantic(@changes)
@changes
end
def to_html
@changes.map do |op, text|
%{<span class="#{html_class_names(op)}">#{ERB::Util.html_escape(text)}</span>}
end.join.html_safe
end
private
def diff_match_patch
strong_memoize(:diff_match_patch) { DiffMatchPatch.new }
end
def html_class_names(operation)
class_names = ['idiff']
case operation
when :insert
class_names << 'addition'
when :delete
class_names << 'deletion'
end
class_names.join(' ')
end
end
end
end
......@@ -537,4 +537,11 @@ describe Groups::EpicsController do
end
end
end
it_behaves_like DescriptionDiffActions do
let_it_be(:group) { create(:group, :public) }
let_it_be(:issuable) { create(:epic, group: group) }
let(:base_params) { { group_id: group, id: issuable } }
end
end
......@@ -329,4 +329,9 @@ describe Projects::IssuesController do
end
end
end
it_behaves_like DescriptionDiffActions do
let_it_be(:project) { create(:project_empty_repo, :public) }
let_it_be(:issuable) { create(:issue, project: project) }
end
end
......@@ -869,4 +869,9 @@ describe Projects::MergeRequestsController do
end
end
end
it_behaves_like DescriptionDiffActions do
let_it_be(:project) { create(:project, :repository, :public) }
let_it_be(:issuable) { create(:merge_request, source_project: project) }
end
end
# frozen_string_literal: true
FactoryBot.define do
factory :description_version do
description { generate(:title) }
after(:build) do |description_version|
description_version.issue = create(:issue) unless description_version.issuable
end
end
end
# frozen_string_literal: true
require 'fast_spec_helper'
require 'diff_match_patch'
describe Gitlab::Diff::CharDiff do
let(:old_string) { "Helo \n Worlld" }
let(:new_string) { "Hello \n World" }
subject { described_class.new(old_string, new_string) }
describe '#generate_diff' do
context 'when old string is nil' do
let(:old_string) { nil }
it 'does not raise an error' do
expect { subject.generate_diff }.not_to raise_error
end
it 'treats nil values as blank strings' do
changes = subject.generate_diff
expect(changes).to eq([
[:insert, "Hello \n World"]
])
end
end
it 'generates an array of changes' do
changes = subject.generate_diff
expect(changes).to eq([
[:equal, "Hel"],
[:insert, "l"],
[:equal, "o \n Worl"],
[:delete, "l"],
[:equal, "d"]
])
end
end
describe '#to_html' do
it 'returns an HTML representation of the diff' do
subject.generate_diff
expect(subject.to_html).to eq(
'<span class="idiff">Hel</span>' \
'<span class="idiff addition">l</span>' \
"<span class=\"idiff\">o \n Worl</span>" \
'<span class="idiff deletion">l</span>' \
'<span class="idiff">d</span>'
)
end
end
end
......@@ -12,4 +12,24 @@ describe DescriptionVersion do
expect(described_class.new(epic_id: 1)).to be_valid
end
end
describe '#previous_version' do
let(:issue) { create(:issue) }
let(:previous_version) { create(:description_version, issue: issue) }
let(:current_version) { create(:description_version, issue: issue) }
before do
create(:description_version, issue: issue)
create(:description_version)
previous_version
current_version
create(:description_version, issue: issue)
end
it 'returns the previous version for the same issuable' do
expect(current_version.previous_version).to eq(previous_version)
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe NoteEntity do
include Gitlab::Routing
let(:issue) { create(:issue) }
let(:description_version) { create(:description_version, issue: issue) }
let(:note) { create(:system_note, project: issue.project, noteable: issue, system_note_metadata: create(:system_note_metadata, description_version: description_version)) }
let(:request) { double('request', current_user: issue.author, noteable: issue) }
let(:entity) { described_class.new(note, request: request) }
subject { entity.as_json }
context 'when description_diffs license is available' do
before do
stub_licensed_features(description_diffs: true)
end
it 'includes version id and diff path' 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))
end
end
context 'when description_diffs license is not available' do
before do
stub_licensed_features(description_diffs: false)
end
it 'does not include version id and diff path' do
expect(subject[:description_version_id]).to be_nil
expect(subject[:description_diff_path]).to be_nil
end
end
end
# frozen_string_literal: true
require 'spec_helper'
shared_examples DescriptionDiffActions do
let(:base_params) { { namespace_id: project.namespace, project_id: project, id: issuable } }
describe 'GET description_diff' 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) }
def get_description_diff(extra_params = {})
get :description_diff, 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
get_description_diff(version_id: version_3)
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
get_description_diff(version_id: version_3, start_version_id: version_2)
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)
get_description_diff(version_id: other_version)
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
end
context 'when license is not available' do
before do
stub_licensed_features(epics: true, description_diffs: false)
end
it 'returns 404' do
get_description_diff(version_id: version_3)
expect(response.status).to eq(404)
end
end
end
end
......@@ -4386,6 +4386,9 @@ msgstr ""
msgid "Compare changes with the merge request target branch"
msgstr ""
msgid "Compare with previous version"
msgstr ""
msgid "CompareBranches|%{source_branch} and %{target_branch} are the same."
msgstr ""
......@@ -10714,9 +10717,6 @@ msgstr ""
msgid "MergeRequests|started a thread on commit %{linkStart}%{commitDisplay}%{linkEnd}"
msgstr ""
msgid "MergeRequest| %{paragraphStart}changed the description %{descriptionChangedTimes} times %{timeDifferenceMinutes}%{paragraphEnd}"
msgstr ""
msgid "MergeRequest|Error dismissing suggestion popover. Please try again."
msgstr ""
......@@ -15932,6 +15932,9 @@ msgstr ""
msgid "Something went wrong while fetching comments. Please try again."
msgstr ""
msgid "Something went wrong while fetching description changes. Please try again."
msgstr ""
msgid "Something went wrong while fetching group member contributions"
msgstr ""
......@@ -21199,10 +21202,5 @@ msgstr ""
msgid "with %{additions} additions, %{deletions} deletions."
msgstr ""
msgid "within %d minute "
msgid_plural "within %d minutes "
msgstr[0] ""
msgstr[1] ""
msgid "yaml invalid"
msgstr ""
......@@ -1094,8 +1094,9 @@ export const collapsedSystemNotes = [
noteable_type: 'Issue',
resolvable: false,
noteable_iid: 12,
start_description_version_id: undefined,
note: 'changed the description',
note_html: ' <p dir="auto">changed the description 2 times within 1 minute </p>',
note_html: '<p dir="auto">changed the description</p>',
current_user: { can_edit: false, can_award_emoji: true },
resolved: false,
resolved_by: null,
......@@ -1106,7 +1107,6 @@ export const collapsedSystemNotes = [
'/abuse_reports/new?ref_url=http%3A%2F%2Flocalhost%3A3000%2Fgitlab-org%2Fgitlab-shell%2Fissues%2F12%23note_905&user_id=1',
human_access: 'Owner',
path: '/gitlab-org/gitlab-shell/notes/905',
times_updated: 2,
},
],
individual_note: true,
......
......@@ -57,7 +57,7 @@ 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', () => {
expect(vm.$el.querySelector('.system-note-message').innerHTML).toEqual('<span>closed</span>');
expect(vm.$el.querySelector('.system-note-message').innerHTML).toContain('<span>closed</span>');
});
it('should initMRPopovers onMount', () => {
......
import {
isDescriptionSystemNote,
changeDescriptionNote,
getTimeDifferenceMinutes,
collapseSystemNotes,
} from '~/notes/stores/collapse_utils';
......@@ -24,15 +23,6 @@ describe('Collapse utils', () => {
);
});
it('changes the description to contain the number of changed times', () => {
const changedNote = changeDescriptionNote(mockSystemNote, 3, 5);
expect(changedNote.times_updated).toEqual(3);
expect(changedNote.note_html.trim()).toContain(
'<p dir="auto">changed the description 3 times within 5 minutes </p>',
);
});
it('gets the time difference between two notes', () => {
const anotherSystemNote = {
created_at: '2018-05-14T21:33:00.000Z',
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment