Commit 1b86dda4 authored by David O'Regan's avatar David O'Regan

Merge branch '296769-fix-sourcegraph-unified-diff' into 'master'

Fix Sourcegraph with MR diffs

See merge request gitlab-org/gitlab!58746
parents abd17934 718093e5
......@@ -13,6 +13,11 @@ import {
CONFLICT_THEIR,
CONFLICT_MARKER,
} from '../constants';
import {
getInteropInlineAttributes,
getInteropOldSideAttributes,
getInteropNewSideAttributes,
} from '../utils/interoperability';
import DiffGutterAvatars from './diff_gutter_avatars.vue';
import * as utils from './diff_row_utils';
......@@ -116,6 +121,16 @@ export default {
isLeftConflictMarker() {
return [CONFLICT_MARKER_OUR, CONFLICT_MARKER_THEIR].includes(this.line.left?.type);
},
interopLeftAttributes() {
if (this.inline) {
return getInteropInlineAttributes(this.line.left);
}
return getInteropOldSideAttributes(this.line.left);
},
interopRightAttributes() {
return getInteropNewSideAttributes(this.line.right);
},
},
mounted() {
this.scrollToLineIfNeededParallel(this.line);
......@@ -181,6 +196,7 @@ export default {
<div
data-testid="left-side"
class="diff-grid-left left-side"
v-bind="interopLeftAttributes"
@dragover.prevent
@dragenter="onDragEnter(line.left, index)"
@dragend="onDragEnd"
......@@ -286,6 +302,7 @@ export default {
v-if="!inline"
data-testid="right-side"
class="diff-grid-right right-side"
v-bind="interopRightAttributes"
@dragover.prevent
@dragenter="onDragEnter(line.right, index)"
@dragend="onDragEnd"
......
......@@ -2,6 +2,7 @@
import { GlTooltipDirective, GlIcon, GlSafeHtmlDirective as SafeHtml } from '@gitlab/ui';
import { mapActions, mapGetters, mapState } from 'vuex';
import { CONTEXT_LINE_CLASS_NAME } from '../constants';
import { getInteropInlineAttributes } from '../utils/interoperability';
import DiffGutterAvatars from './diff_gutter_avatars.vue';
import {
isHighlighted,
......@@ -96,6 +97,9 @@ export default {
shouldShowAvatarsOnGutter() {
return this.line.hasDiscussions;
},
interopAttrs() {
return getInteropInlineAttributes(this.line);
},
},
mounted() {
this.scrollToLineIfNeededInline(this.line);
......@@ -124,6 +128,7 @@ export default {
:id="inlineRowId"
:class="classNameMap"
class="line_holder"
v-bind="interopAttrs"
@mouseover="handleMouseMove"
@mouseout="handleMouseMove"
>
......
......@@ -3,6 +3,10 @@ import { GlTooltipDirective, GlIcon, GlSafeHtmlDirective as SafeHtml } from '@gi
import $ from 'jquery';
import { mapActions, mapGetters, mapState } from 'vuex';
import { CONTEXT_LINE_CLASS_NAME, PARALLEL_DIFF_VIEW_TYPE } from '../constants';
import {
getInteropOldSideAttributes,
getInteropNewSideAttributes,
} from '../utils/interoperability';
import DiffGutterAvatars from './diff_gutter_avatars.vue';
import * as utils from './diff_row_utils';
......@@ -108,6 +112,12 @@ export default {
this.line.hasDiscussionsRight,
);
},
interopLeftAttributes() {
return getInteropOldSideAttributes(this.line.left);
},
interopRightAttributes() {
return getInteropNewSideAttributes(this.line.right);
},
},
mounted() {
this.scrollToLineIfNeededParallel(this.line);
......@@ -217,6 +227,7 @@ export default {
:key="line.left.line_code"
v-safe-html="line.left.rich_text"
:class="parallelViewLeftLineType"
v-bind="interopLeftAttributes"
class="line_content with-coverage parallel left-side"
@mousedown="handleParallelLineMouseDown"
></td>
......@@ -283,6 +294,7 @@ export default {
hll: isHighlighted,
},
]"
v-bind="interopRightAttributes"
class="line_content with-coverage parallel right-side"
@mousedown="handleParallelLineMouseDown"
></td>
......
......@@ -9,7 +9,8 @@ import {
import { fileByFile } from '../../utils/preferences';
import { getDefaultWhitespace } from '../utils';
const viewTypeFromQueryString = getParameterValues('view')[0];
const getViewTypeFromQueryString = () => getParameterValues('view')[0];
const viewTypeFromCookie = Cookies.get(DIFF_VIEW_COOKIE_NAME);
const defaultViewType = INLINE_DIFF_VIEW_TYPE;
const whiteSpaceFromQueryString = getParameterValues('w')[0];
......@@ -31,7 +32,7 @@ export default () => ({
coverageFiles: {},
mergeRequestDiffs: [],
mergeRequestDiff: null,
diffViewType: viewTypeFromQueryString || viewTypeFromCookie || defaultViewType,
diffViewType: getViewTypeFromQueryString() || viewTypeFromCookie || defaultViewType,
tree: [],
treeEntries: {},
showTreeList: true,
......
const OLD = 'old';
const NEW = 'new';
const ATTR_PREFIX = 'data-interop-';
export const ATTR_TYPE = `${ATTR_PREFIX}type`;
export const ATTR_LINE = `${ATTR_PREFIX}line`;
export const ATTR_NEW_LINE = `${ATTR_PREFIX}new-line`;
export const ATTR_OLD_LINE = `${ATTR_PREFIX}old-line`;
export const getInteropInlineAttributes = (line) => {
if (!line) {
return null;
}
const interopType = line.type?.startsWith(OLD) ? OLD : NEW;
const interopLine = interopType === OLD ? line.old_line : line.new_line;
return {
[ATTR_TYPE]: interopType,
[ATTR_LINE]: interopLine,
[ATTR_NEW_LINE]: line.new_line,
[ATTR_OLD_LINE]: line.old_line,
};
};
export const getInteropOldSideAttributes = (line) => {
if (!line) {
return null;
}
return {
[ATTR_TYPE]: OLD,
[ATTR_LINE]: line.old_line,
[ATTR_OLD_LINE]: line.old_line,
};
};
export const getInteropNewSideAttributes = (line) => {
if (!line) {
return null;
}
return {
[ATTR_TYPE]: NEW,
[ATTR_LINE]: line.new_line,
[ATTR_NEW_LINE]: line.new_line,
};
};
......@@ -211,19 +211,20 @@ describe('CompareVersions', () => {
});
describe('prev commit', () => {
const { location } = window;
beforeAll(() => {
delete window.location;
window.location = { href: `${TEST_HOST}?commit_id=${mrCommit.id}` };
global.jsdom.reconfigure({
url: `${TEST_HOST}?commit_id=${mrCommit.id}`,
});
});
beforeEach(() => {
jest.spyOn(wrapper.vm, 'moveToNeighboringCommit').mockImplementation(() => {});
afterAll(() => {
global.jsdom.reconfigure({
url: TEST_HOST,
});
});
afterAll(() => {
window.location = location;
beforeEach(() => {
jest.spyOn(wrapper.vm, 'moveToNeighboringCommit').mockImplementation(() => {});
});
it('uses the correct href', () => {
......@@ -253,19 +254,20 @@ describe('CompareVersions', () => {
});
describe('next commit', () => {
const { location } = window;
beforeAll(() => {
delete window.location;
window.location = { href: `${TEST_HOST}?commit_id=${mrCommit.id}` };
global.jsdom.reconfigure({
url: `${TEST_HOST}?commit_id=${mrCommit.id}`,
});
});
beforeEach(() => {
jest.spyOn(wrapper.vm, 'moveToNeighboringCommit').mockImplementation(() => {});
afterAll(() => {
global.jsdom.reconfigure({
url: TEST_HOST,
});
});
afterAll(() => {
window.location = location;
beforeEach(() => {
jest.spyOn(wrapper.vm, 'moveToNeighboringCommit').mockImplementation(() => {});
});
it('uses the correct href', () => {
......
......@@ -4,6 +4,7 @@ import Vuex from 'vuex';
import DiffRow from '~/diffs/components/diff_row.vue';
import { mapParallel } from '~/diffs/components/diff_row_utils';
import diffsModule from '~/diffs/store/modules';
import { findInteropAttributes } from '../find_interop_attributes';
import diffFileMockData from '../mock_data/diff_file';
describe('DiffRow', () => {
......@@ -211,4 +212,20 @@ describe('DiffRow', () => {
expect(coverage.classes('no-coverage')).toBeFalsy();
});
});
describe('interoperability', () => {
it.each`
desc | line | inline | leftSide | rightSide
${'with inline and new_line'} | ${{ left: { old_line: 3, new_line: 5, type: 'new' } }} | ${true} | ${{ type: 'new', line: '5', oldLine: '3', newLine: '5' }} | ${null}
${'with inline and no new_line'} | ${{ left: { old_line: 3, type: 'old' } }} | ${true} | ${{ type: 'old', line: '3', oldLine: '3' }} | ${null}
${'with parallel and no right side'} | ${{ left: { old_line: 3, new_line: 5 } }} | ${false} | ${{ type: 'old', line: '3', oldLine: '3' }} | ${null}
${'with parallel and no left side'} | ${{ right: { old_line: 3, new_line: 5 } }} | ${false} | ${null} | ${{ type: 'new', line: '5', newLine: '5' }}
${'with parallel and right side'} | ${{ left: { old_line: 3 }, right: { new_line: 5 } }} | ${false} | ${{ type: 'old', line: '3', oldLine: '3' }} | ${{ type: 'new', line: '5', newLine: '5' }}
`('$desc, sets interop data attributes', ({ line, inline, leftSide, rightSide }) => {
const wrapper = createWrapper({ props: { line, inline } });
expect(findInteropAttributes(wrapper, '[data-testid="left-side"]')).toEqual(leftSide);
expect(findInteropAttributes(wrapper, '[data-testid="right-side"]')).toEqual(rightSide);
});
});
});
......@@ -3,6 +3,7 @@ import DiffGutterAvatars from '~/diffs/components/diff_gutter_avatars.vue';
import { mapInline } from '~/diffs/components/diff_row_utils';
import InlineDiffTableRow from '~/diffs/components/inline_diff_table_row.vue';
import { createStore } from '~/mr_notes/stores';
import { findInteropAttributes } from '../find_interop_attributes';
import discussionsMockData from '../mock_data/diff_discussions';
import diffFileMockData from '../mock_data/diff_file';
......@@ -310,4 +311,16 @@ describe('InlineDiffTableRow', () => {
});
});
});
describe('interoperability', () => {
it.each`
desc | line | expectation
${'with type old'} | ${{ ...thisLine, type: 'old', old_line: 3, new_line: 5 }} | ${{ type: 'old', line: '3', oldLine: '3', newLine: '5' }}
${'with type new'} | ${{ ...thisLine, type: 'new', old_line: 3, new_line: 5 }} | ${{ type: 'new', line: '5', oldLine: '3', newLine: '5' }}
`('$desc, sets interop data attributes', ({ line, expectation }) => {
createComponent({ line });
expect(findInteropAttributes(wrapper)).toEqual(expectation);
});
});
});
......@@ -5,6 +5,7 @@ import DiffGutterAvatars from '~/diffs/components/diff_gutter_avatars.vue';
import { mapParallel } from '~/diffs/components/diff_row_utils';
import ParallelDiffTableRow from '~/diffs/components/parallel_diff_table_row.vue';
import { createStore } from '~/mr_notes/stores';
import { findInteropAttributes } from '../find_interop_attributes';
import discussionsMockData from '../mock_data/diff_discussions';
import diffFileMockData from '../mock_data/diff_file';
......@@ -418,5 +419,27 @@ describe('ParallelDiffTableRow', () => {
});
});
});
describe('interoperability', () => {
beforeEach(() => {
createComponent();
});
it('adds old side interoperability data attributes', () => {
expect(findInteropAttributes(wrapper, '.line_content.left-side')).toEqual({
type: 'old',
line: thisLine.left.old_line.toString(),
oldLine: thisLine.left.old_line.toString(),
});
});
it('adds new side interoperability data attributes', () => {
expect(findInteropAttributes(wrapper, '.line_content.right-side')).toEqual({
type: 'new',
line: thisLine.right.new_line.toString(),
newLine: thisLine.right.new_line.toString(),
});
});
});
});
});
export const findInteropAttributes = (parent, sel) => {
const target = sel ? parent.find(sel) : parent;
if (!target.exists()) {
return null;
}
const type = target.attributes('data-interop-type');
if (!type) {
return null;
}
return {
type,
line: target.attributes('data-interop-line'),
oldLine: target.attributes('data-interop-old-line'),
newLine: target.attributes('data-interop-new-line'),
};
};
import {
getInteropInlineAttributes,
getInteropNewSideAttributes,
getInteropOldSideAttributes,
ATTR_TYPE,
ATTR_LINE,
ATTR_NEW_LINE,
ATTR_OLD_LINE,
} from '~/diffs/utils/interoperability';
describe('~/diffs/utils/interoperability', () => {
describe('getInteropInlineAttributes', () => {
it.each([
['with null input', { input: null, output: null }],
[
'with type=old input',
{
input: { type: 'old', old_line: 3, new_line: 5 },
output: { [ATTR_TYPE]: 'old', [ATTR_LINE]: 3, [ATTR_OLD_LINE]: 3, [ATTR_NEW_LINE]: 5 },
},
],
[
'with type=old-nonewline input',
{
input: { type: 'old-nonewline', old_line: 3, new_line: 5 },
output: { [ATTR_TYPE]: 'old', [ATTR_LINE]: 3, [ATTR_OLD_LINE]: 3, [ATTR_NEW_LINE]: 5 },
},
],
[
'with type=new input',
{
input: { type: 'new', old_line: 3, new_line: 5 },
output: { [ATTR_TYPE]: 'new', [ATTR_LINE]: 5, [ATTR_OLD_LINE]: 3, [ATTR_NEW_LINE]: 5 },
},
],
[
'with type=bogus input',
{
input: { type: 'bogus', old_line: 3, new_line: 5 },
output: { [ATTR_TYPE]: 'new', [ATTR_LINE]: 5, [ATTR_OLD_LINE]: 3, [ATTR_NEW_LINE]: 5 },
},
],
])('%s', (desc, { input, output }) => {
expect(getInteropInlineAttributes(input)).toEqual(output);
});
});
describe('getInteropOldSideAttributes', () => {
it.each`
input | output
${null} | ${null}
${{ old_line: 2 }} | ${{ [ATTR_TYPE]: 'old', [ATTR_LINE]: 2, [ATTR_OLD_LINE]: 2 }}
`('with input=$input', ({ input, output }) => {
expect(getInteropOldSideAttributes(input)).toEqual(output);
});
});
describe('getInteropNewSideAttributes', () => {
it.each`
input | output
${null} | ${null}
${{ new_line: 2 }} | ${{ [ATTR_TYPE]: 'new', [ATTR_LINE]: 2, [ATTR_NEW_LINE]: 2 }}
`('with input=$input', ({ input, output }) => {
expect(getInteropNewSideAttributes(input)).toEqual(output);
});
});
});
......@@ -25,6 +25,10 @@ RSpec.describe Projects::MergeRequests::DiffsController, '(JavaScript fixtures)'
end
before do
# Create a user that matches the project.commit author
# This is so that the "author" information will be populated
create(:user, email: project.commit.author_email, name: project.commit.author_name)
sign_in(user)
end
......@@ -33,17 +37,21 @@ RSpec.describe Projects::MergeRequests::DiffsController, '(JavaScript fixtures)'
end
it 'merge_request_diffs/with_commit.json' do
# Create a user that matches the project.commit author
# This is so that the "author" information will be populated
create(:user, email: project.commit.author_email, name: project.commit.author_name)
render_merge_request(merge_request, commit_id: project.commit.sha)
end
it 'merge_request_diffs/diffs_metadata.json' do
render_merge_request(merge_request, action: :diffs_metadata)
end
it 'merge_request_diffs/diffs_batch.json' do
render_merge_request(merge_request, action: :diffs_batch, page: 1, per_page: 30)
end
private
def render_merge_request(merge_request, view: 'inline', **extra_params)
get :show, params: {
def render_merge_request(merge_request, action: :show, view: 'inline', **extra_params)
get action, params: {
namespace_id: project.namespace.to_param,
project_id: project,
id: merge_request.to_param,
......
/**
* This helper module contains the API expectation of the diff output HTML.
*
* This helps simulate what third-party HTML scrapers, such as Sourcegraph,
* should be looking for.
*/
export const getDiffCodePart = (codeElement) => {
const el = codeElement.closest('[data-interop-type]');
return el.dataset.interopType === 'old' ? 'base' : 'head';
};
export const getCodeElementFromLineNumber = (codeView, line, part) => {
const type = part === 'base' ? 'old' : 'new';
const el = codeView.querySelector(`[data-interop-${type}-line="${line}"]`);
return el ? el.querySelector('span.line') : null;
};
export const getLineNumberFromCodeElement = (codeElement) => {
const el = codeElement.closest('[data-interop-line]');
return parseInt(el.dataset.interopLine || '', 10);
};
import { waitFor } from '@testing-library/dom';
import { TEST_HOST } from 'helpers/test_constants';
import initDiffsApp from '~/diffs';
import { createStore } from '~/mr_notes/stores';
import {
getDiffCodePart,
getLineNumberFromCodeElement,
getCodeElementFromLineNumber,
} from './diffs_interopability_api';
jest.mock('~/vue_shared/mixins/gl_feature_flags_mixin', () => () => ({
inject: {
glFeatures: {
from: 'window.gon.features',
default: () => global.window.gon?.features,
},
},
}));
const TEST_PROJECT_PATH = 'gitlab-org/gitlab-test';
const TEST_BASE_URL = `/${TEST_PROJECT_PATH}/-/merge_requests/1/`;
const TEST_DIFF_FILE = 'files/js/commit.coffee';
const EXPECT_INLINE = [
['head', 1],
['head', 2],
['head', 3],
['base', 4],
['head', 4],
null,
['base', 6],
['head', 6],
null,
];
const EXPECT_PARALLEL_LEFT_SIDE = [
['base', 1],
['base', 2],
['base', 3],
['base', 4],
null,
['base', 6],
null,
];
const EXPECT_PARALLEL_RIGHT_SIDE = [
['head', 1],
['head', 2],
['head', 3],
['head', 4],
null,
['head', 6],
null,
];
const startDiffsApp = () => {
const el = document.createElement('div');
el.id = 'js-diffs-app';
document.body.appendChild(el);
Object.assign(el.dataset, {
endpoint: TEST_BASE_URL,
endpointMetadata: `${TEST_BASE_URL}diffs_metadata.json`,
endpointBatch: `${TEST_BASE_URL}diffs_batch.json`,
projectPath: TEST_PROJECT_PATH,
helpPagePath: '/help',
currentUserData: 'null',
changesEmptyStateIllustration: '',
isFluidLayout: 'false',
dismissEndpoint: '',
showSuggestPopover: 'false',
showWhitespaceDefault: 'true',
viewDiffsFileByFile: 'false',
defaultSuggestionCommitMessage: 'Lorem ipsum',
});
const store = createStore();
const vm = initDiffsApp(store);
store.dispatch('setActiveTab', 'diffs');
return vm;
};
describe('diffs third party interoperability', () => {
let vm;
afterEach(() => {
vm.$destroy();
document.body.innerHTML = '';
});
const tryOrErrorMessage = (fn) => (...args) => {
try {
return fn(...args);
} catch (e) {
return e.message;
}
};
const findDiffFile = () => document.querySelector(`.diff-file[data-path="${TEST_DIFF_FILE}"]`);
const hasLines = (sel = 'tr.line_holder') => findDiffFile().querySelectorAll(sel).length > 0;
const findLineElements = (sel = 'tr.line_holder') =>
Array.from(findDiffFile().querySelectorAll(sel));
const findCodeElements = (lines, sel = 'td.line_content') => {
return lines.map((x) => x.querySelector(`${sel} span.line`));
};
const getCodeElementsInteropModel = (codeElements) =>
codeElements.map(
(x) =>
x && [
tryOrErrorMessage(getDiffCodePart)(x),
tryOrErrorMessage(getLineNumberFromCodeElement)(x),
],
);
describe.each`
desc | unifiedDiffComponents | view | rowSelector | codeSelector | expectation
${'inline view'} | ${false} | ${'inline'} | ${'tr.line_holder'} | ${'td.line_content'} | ${EXPECT_INLINE}
${'parallel view left side'} | ${false} | ${'parallel'} | ${'tr.line_holder'} | ${'td.line_content.left-side'} | ${EXPECT_PARALLEL_LEFT_SIDE}
${'parallel view right side'} | ${false} | ${'parallel'} | ${'tr.line_holder'} | ${'td.line_content.right-side'} | ${EXPECT_PARALLEL_RIGHT_SIDE}
${'inline view'} | ${true} | ${'inline'} | ${'.diff-tr.line_holder'} | ${'.diff-td.line_content'} | ${EXPECT_INLINE}
${'parallel view left side'} | ${true} | ${'parallel'} | ${'.diff-tr.line_holder'} | ${'.diff-td.line_content.left-side'} | ${EXPECT_PARALLEL_LEFT_SIDE}
${'parallel view right side'} | ${true} | ${'parallel'} | ${'.diff-tr.line_holder'} | ${'.diff-td.line_content.right-side'} | ${EXPECT_PARALLEL_RIGHT_SIDE}
`(
'$desc (unifiedDiffComponents=$unifiedDiffComponents)',
({ unifiedDiffComponents, view, rowSelector, codeSelector, expectation }) => {
beforeEach(async () => {
global.jsdom.reconfigure({
url: `${TEST_HOST}/${TEST_BASE_URL}/diffs?view=${view}`,
});
window.gon.features = { unifiedDiffComponents };
vm = startDiffsApp();
await waitFor(() => expect(hasLines(rowSelector)).toBe(true));
});
it('should match diff model', () => {
const lines = findLineElements(rowSelector);
const codes = findCodeElements(lines, codeSelector);
expect(getCodeElementsInteropModel(codes)).toEqual(expectation);
});
it.each`
lineNumber | part | expectedText
${4} | ${'base'} | ${'new CommitFile(this)'}
${4} | ${'head'} | ${'new CommitFile(@)'}
${2} | ${'base'} | ${'constructor: ->'}
${2} | ${'head'} | ${'constructor: ->'}
`(
'should find code element lineNumber=$lineNumber part=$part',
({ lineNumber, part, expectedText }) => {
const codeElement = getCodeElementFromLineNumber(findDiffFile(), lineNumber, part);
expect(codeElement.textContent.trim()).toBe(expectedText);
},
);
},
);
});
......@@ -40,6 +40,12 @@ export const getMergeRequestVersions = factory.json(() =>
export const getRepositoryFiles = factory.json(() =>
require('test_fixtures/projects_json/files.json'),
);
export const getDiffsMetadata = factory.json(() =>
require('test_fixtures/merge_request_diffs/diffs_metadata.json'),
);
export const getDiffsBatch = factory.json(() =>
require('test_fixtures/merge_request_diffs/diffs_batch.json'),
);
export const getPipelinesEmptyResponse = factory.json(() =>
require('test_fixtures/projects_json/pipelines_empty.json'),
);
......
import { getDiffsMetadata, getDiffsBatch } from 'test_helpers/fixtures';
import { withValues } from 'test_helpers/utils/obj';
export default (server) => {
server.get('/:namespace/:project/-/merge_requests/:mrid/diffs_metadata.json', () => {
return getDiffsMetadata();
});
server.get('/:namespace/:project/-/merge_requests/:mrid/diffs_batch.json', () => {
const { pagination, ...result } = getDiffsBatch();
return {
...result,
pagination: withValues(pagination, {
current_page: null,
next_page: null,
total_pages: 1,
next_page_href: null,
}),
};
});
};
......@@ -5,6 +5,7 @@ export default (server) => {
require('./projects'),
require('./repository'),
require('./ci'),
require('./diffs'),
require('./404'),
].forEach(({ default: setup }) => {
setup(server);
......
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