kotovalexarian-likes-gitlab
/
gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Commenting on image diffs

This commit is contained in:
Felipe Artur 2017-10-07 04:25:17 +00:00 committed by Jacob Schatz
parent b4f9dc4816
commit b54203f0ad
77 changed files with 3314 additions and 266 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
app/assets/images/icon_image_comment.svg Normal file
View File

@ -0,0 +1 @@
<svg width="24" height="30" viewBox="0 0 24 30" xmlns="http://www.w3.org/2000/svg"><title>cursor</title><g fill="none" fill-rule="evenodd"><path d="M24 12.105c0 6.686-5.74 11.58-12 17.895C5.74 23.684 0 18.79 0 12.105 0 5.42 5.373 0 12 0s12 5.42 12 12.105z" fill="#1F78D1" fill-rule="nonzero"/><path d="M15.28 25.249c1.458-1.475 2.539-2.635 3.474-3.747 2.851-3.394 4.203-6.265 4.203-9.397 0-6.111-4.908-11.062-10.957-11.062-6.05 0-10.957 4.951-10.957 11.062 0 3.132 1.352 6.003 4.203 9.397.935 1.112 2.016 2.272 3.474 3.747.511.517 2.216 2.213 3.28 3.275 1.064-1.062 2.769-2.758 3.28-3.275z" fill="#FFF"/><path d="M14.551 8.256A6.874 6.874 0 0 0 12 7.787c-.91 0-1.763.156-2.558.469-.79.308-1.42.725-1.888 1.252-.465.527-.697 1.096-.697 1.708 0 .5.159.977.476 1.433.321.45.772.841 1.352 1.172l.583.334-.181.643c-.107.407-.263.79-.469 1.152a6.604 6.604 0 0 0 1.842-1.145l.288-.254.381.04c.309.035.599.053.871.053.91 0 1.761-.154 2.551-.462.795-.312 1.424-.732 1.889-1.259.468-.526.703-1.096.703-1.707 0-.612-.235-1.181-.703-1.708-.465-.527-1.094-.944-1.889-1.252zm2.645.81c.536.656.804 1.373.804 2.15 0 .776-.268 1.495-.804 2.156-.535.656-1.263 1.176-2.183 1.56-.92.38-1.924.57-3.013.57a9.16 9.16 0 0 1-.971-.054 7.32 7.32 0 0 1-3.08 1.62 5.044 5.044 0 0 1-.764.148h-.033a.26.26 0 0 1-.181-.074.324.324 0 0 1-.107-.18v-.007c-.014-.018-.016-.045-.007-.08.014-.037.018-.059.014-.068 0-.009.01-.031.033-.067a.645.645 0 0 0 .04-.06 1.73 1.73 0 0 0 .047-.054l.054-.06a53.034 53.034 0 0 1 .435-.489c.049-.049.118-.136.207-.26.094-.126.168-.24.221-.342.054-.103.114-.235.181-.395.067-.161.125-.33.174-.51-.7-.397-1.254-.888-1.66-1.473A3.261 3.261 0 0 1 6 11.216c0-.777.268-1.494.804-2.15.535-.66 1.263-1.18 2.183-1.56.92-.384 1.924-.576 3.013-.576 1.09 0 2.094.192 3.013.576.92.38 1.648.9 2.183 1.56z" fill="#1F78D1" fill-rule="nonzero"/></g></svg>
Side by Side

After

Width:  |  Height:  |  Size: 1.8 KiB

1
app/assets/images/icon_image_comment@2x.svg Normal file
View File

@ -0,0 +1 @@
<svg width="48" height="60" viewBox="0 0 48 60" xmlns="http://www.w3.org/2000/svg"><title>cursor_2x</title><g fill="none" fill-rule="evenodd"><path d="M48 24.21C48 37.583 36.522 47.369 24 60 11.478 47.368 0 37.582 0 24.21 0 10.84 10.745 0 24 0s24 10.84 24 24.21z" fill="#1F78D1" fill-rule="nonzero"/><path d="M30.56 50.497c2.915-2.95 5.078-5.268 6.947-7.493 5.703-6.788 8.406-12.53 8.406-18.793 0-12.223-9.815-22.124-21.913-22.124S2.087 11.988 2.087 24.211c0 6.263 2.703 12.005 8.406 18.793 1.87 2.225 4.032 4.544 6.947 7.493 1.022 1.035 4.432 4.426 6.56 6.55 2.128-2.124 5.538-5.515 6.56-6.55z" fill="#FFF"/><path d="M29.103 16.512c-1.58-.625-3.282-.938-5.103-.938-1.821 0-3.527.313-5.116.938-1.58.616-2.84 1.45-3.777 2.504-.928 1.054-1.393 2.192-1.393 3.415 0 1 .317 1.956.951 2.866.643.902 1.545 1.684 2.706 2.344l1.165.67-.362 1.286a9.603 9.603 0 0 1-.937 2.303 13.208 13.208 0 0 0 3.683-2.29l.576-.509.763.08c.616.072 1.196.108 1.741.108 1.821 0 3.522-.308 5.103-.925 1.589-.625 2.848-1.464 3.776-2.517.938-1.054 1.407-2.192 1.407-3.416 0-1.223-.469-2.361-1.407-3.415-.928-1.053-2.187-1.888-3.776-2.504zm5.29 1.62c1.071 1.313 1.607 2.746 1.607 4.3 0 1.553-.536 2.99-1.607 4.312-1.072 1.312-2.527 2.353-4.366 3.12-1.84.76-3.848 1.139-6.027 1.139a18.32 18.32 0 0 1-1.942-.107c-1.768 1.562-3.821 2.643-6.16 3.24-.438.126-.947.224-1.527.295h-.067a.521.521 0 0 1-.362-.147.649.649 0 0 1-.214-.362v-.013c-.027-.036-.032-.09-.014-.16.027-.072.036-.117.027-.135 0-.017.022-.062.067-.133a1.29 1.29 0 0 0 .08-.121c.01-.009.04-.045.094-.107a106.068 106.068 0 0 1 .522-.59c.215-.232.367-.401.456-.508.098-.099.236-.273.415-.523.188-.25.335-.477.442-.683.107-.205.228-.468.362-.79.134-.321.25-.66.348-1.018-1.402-.794-2.51-1.777-3.322-2.946C12.402 25.025 12 23.77 12 22.43c0-1.553.536-2.986 1.607-4.299 1.072-1.321 2.527-2.361 4.366-3.12 1.84-.768 3.848-1.152 6.027-1.152 2.179 0 4.188.384 6.027 1.152 1.84.759 3.294 1.799 4.366 3.12z" fill="#1F78D1" fill-rule="nonzero"/></g></svg>
Side by Side

After

Width:  |  Height:  |  Size: 1.9 KiB

12
app/assets/javascripts/commit.js
View File

@ -1,12 +0,0 @@
/* eslint-disable func-names, space-before-function-paren, wrap-iife */
/* global CommitFile */
window.Commit = (function() {
function Commit() {
$('.files .diff-file').each(function() {
return new CommitFile(this);
});
}
return Commit;
})();

14
app/assets/javascripts/commit/file.js
View File

@ -1,14 +0,0 @@
/* eslint-disable func-names, space-before-function-paren, wrap-iife, no-new */
/* global ImageFile */
(function() {
this.CommitFile = (function() {
function CommitFile(file) {
if ($('.image', file).length) {
new gl.ImageFile(file);
}
}
return CommitFile;
})();
}).call(window);

13
app/assets/javascripts/commit/image_file.js
View File

@ -1,4 +1,6 @@
/* eslint-disable func-names, space-before-function-paren, wrap-iife, no-var, no-use-before-define, prefer-arrow-callback, no-else-return, consistent-return, prefer-template, quotes, one-var, one-var-declaration-per-line, no-unused-vars, no-return-assign, comma-dangle, quote-props, no-unused-expressions, no-sequences, object-shorthand, max-len */
import 'vendor/jquery.waitforimages';
(function() {
gl.ImageFile = (function() {
var prepareFrames;
@ -17,15 +19,10 @@
// Load two-up view after images are loaded
// so that we can display the correct width and height information
const images = $('.two-up.view img', _this.file);
let loadedCount = 0;
const $images = $('.two-up.view img', _this.file);
images.on('load', () => {
loadedCount += 1;
if (loadedCount === images.length) {
_this.initView('two-up');
}
$images.waitForImages(function() {
_this.initView('two-up');
});
});
};

5
app/assets/javascripts/diff.js
View File

@ -3,6 +3,7 @@
import './lib/utils/url_utility';
import FilesCommentButton from './files_comment_button';
import SingleFileDiff from './single_file_diff';
import imageDiffHelper from './image_diff/helpers/index';
const UNFOLD_COUNT = 20;
let isBound = false;
@ -20,7 +21,9 @@ class Diff {
const tab = document.getElementById('diffs');
if (!tab || (tab && tab.dataset && tab.dataset.isLocked !== '')) FilesCommentButton.init($diffFile);
$diffFile.each((index, file) => new gl.ImageFile(file));
const firstFile = $('.files').first().get(0);
const canCreateNote = firstFile && firstFile.hasAttribute('data-can-create-note');
$diffFile.each((index, file) => imageDiffHelper.initImageDiff(file, canCreateNote));
if (!isBound) {
$(document)

9
app/assets/javascripts/diff_notes/components/jump_to_discussion.js
View File

@ -171,7 +171,14 @@ const JumpToDiscussion = Vue.extend({
// When jumping between unresolved discussions on the diffs tab, we show them.
$target.closest(".content").show();
$target = $target.closest("tr.notes_holder");
const $notesHolder = $target.closest("tr.notes_holder");
// Image diff discussions does not use notes_holder
// so we should keep original $target value in those cases
if ($notesHolder.length > 0) {
$target = $notesHolder;
}
$target.show();
// If we are on the diffs tab, we don't scroll to the discussion itself, but to

2
app/assets/javascripts/dispatcher.js
View File

@ -7,7 +7,6 @@
/* global IssuableForm */
/* global LabelsSelect */
/* global MilestoneSelect */
/* global Commit */
/* global CommitsList */
/* global NewBranchForm */
/* global NotificationsForm */
@ -316,7 +315,6 @@ import { ajaxGet, convertPermissionToBoolean } from './lib/utils/common_utils';
new gl.Activities();
break;
case 'projects:commit:show':
new Commit();
new gl.Diff();
new ZenMode();
shortcut_handler = new ShortcutsNavigation();

38
app/assets/javascripts/image_diff/helpers/badge_helper.js Normal file
View File

@ -0,0 +1,38 @@
export function createImageBadge(noteId, { x, y }, classNames = []) {
const buttonEl = document.createElement('button');
const classList = classNames.concat(['js-image-badge']);
classList.forEach(className => buttonEl.classList.add(className));
buttonEl.setAttribute('type', 'button');
buttonEl.setAttribute('disabled', true);
buttonEl.dataset.noteId = noteId;
buttonEl.style.left = `${x}px`;
buttonEl.style.top = `${y}px`;
return buttonEl;
}
export function addImageBadge(containerEl, { coordinate, badgeText, noteId }) {
const buttonEl = createImageBadge(noteId, coordinate, ['badge']);
buttonEl.innerText = badgeText;
containerEl.appendChild(buttonEl);
}
export function addImageCommentBadge(containerEl, { coordinate, noteId }) {
const buttonEl = createImageBadge(noteId, coordinate, ['image-comment-badge', 'inverted']);
const iconEl = document.createElement('i');
iconEl.className = 'fa fa-comment-o';
iconEl.setAttribute('aria-label', 'comment');
buttonEl.appendChild(iconEl);
containerEl.appendChild(buttonEl);
}
export function addAvatarBadge(el, event) {
const { noteId, badgeNumber } = event.detail;
// Add badge to new comment
const avatarBadgeEl = el.querySelector(`#${noteId} .badge`);
avatarBadgeEl.innerText = badgeNumber;
avatarBadgeEl.classList.remove('hidden');
}

58
app/assets/javascripts/image_diff/helpers/comment_indicator_helper.js Normal file
View File

@ -0,0 +1,58 @@
export function addCommentIndicator(containerEl, { x, y }) {
const buttonEl = document.createElement('button');
buttonEl.classList.add('btn-transparent');
buttonEl.classList.add('comment-indicator');
buttonEl.setAttribute('type', 'button');
buttonEl.style.left = `${x}px`;
buttonEl.style.top = `${y}px`;
buttonEl.innerHTML = gl.utils.spriteIcon('image-comment-dark');
containerEl.appendChild(buttonEl);
}
export function removeCommentIndicator(imageFrameEl) {
const commentIndicatorEl = imageFrameEl.querySelector('.comment-indicator');
const imageEl = imageFrameEl.querySelector('img');
const willRemove = !!commentIndicatorEl;
let meta = {};
if (willRemove) {
meta = {
x: parseInt(commentIndicatorEl.style.left, 10),
y: parseInt(commentIndicatorEl.style.top, 10),
image: {
width: imageEl.width,
height: imageEl.height,
},
};
commentIndicatorEl.remove();
}
return Object.assign({}, meta, {
removed: willRemove,
});
}
export function showCommentIndicator(imageFrameEl, coordinate) {
const { x, y } = coordinate;
const commentIndicatorEl = imageFrameEl.querySelector('.comment-indicator');
if (commentIndicatorEl) {
commentIndicatorEl.style.left = `${x}px`;
commentIndicatorEl.style.top = `${y}px`;
} else {
addCommentIndicator(imageFrameEl, coordinate);
}
}
export function commentIndicatorOnClick(event) {
// Prevent from triggering onAddImageDiffNote in notes.js
event.stopPropagation();
const buttonEl = event.currentTarget;
const diffViewerEl = buttonEl.closest('.diff-viewer');
const textareaEl = diffViewerEl.querySelector('.note-container .note-textarea');
textareaEl.focus();
}

44
app/assets/javascripts/image_diff/helpers/dom_helper.js Normal file
View File

@ -0,0 +1,44 @@
export function setPositionDataAttribute(el, options) {
// Update position data attribute so that the
// new comment form can use this data for ajax request
const { x, y, width, height } = options;
const position = el.dataset.position;
const positionObject = Object.assign({}, JSON.parse(position), {
x,
y,
width,
height,
});
el.setAttribute('data-position', JSON.stringify(positionObject));
}
export function updateDiscussionAvatarBadgeNumber(discussionEl, newBadgeNumber) {
const avatarBadgeEl = discussionEl.querySelector('.image-diff-avatar-link .badge');
avatarBadgeEl.innerText = newBadgeNumber;
}
export function updateDiscussionBadgeNumber(discussionEl, newBadgeNumber) {
const discussionBadgeEl = discussionEl.querySelector('.badge');
discussionBadgeEl.innerText = newBadgeNumber;
}
export function toggleCollapsed(event) {
const toggleButtonEl = event.currentTarget;
const discussionNotesEl = toggleButtonEl.closest('.discussion-notes');
const formEl = discussionNotesEl.querySelector('.discussion-form');
const isCollapsed = discussionNotesEl.classList.contains('collapsed');
if (isCollapsed) {
discussionNotesEl.classList.remove('collapsed');
} else {
discussionNotesEl.classList.add('collapsed');
}
// Override the inline display style set in notes.js
if (formEl && !isCollapsed) {
formEl.style.display = 'none';
} else if (formEl && isCollapsed) {
formEl.style.display = 'block';
}
}

25
app/assets/javascripts/image_diff/helpers/index.js Normal file
View File

@ -0,0 +1,25 @@
import * as badgeHelper from './badge_helper';
import * as commentIndicatorHelper from './comment_indicator_helper';
import * as domHelper from './dom_helper';
import * as utilsHelper from './utils_helper';
export default {
addCommentIndicator: commentIndicatorHelper.addCommentIndicator,
removeCommentIndicator: commentIndicatorHelper.removeCommentIndicator,
showCommentIndicator: commentIndicatorHelper.showCommentIndicator,
commentIndicatorOnClick: commentIndicatorHelper.commentIndicatorOnClick,
addImageBadge: badgeHelper.addImageBadge,
addImageCommentBadge: badgeHelper.addImageCommentBadge,
addAvatarBadge: badgeHelper.addAvatarBadge,
setPositionDataAttribute: domHelper.setPositionDataAttribute,
updateDiscussionAvatarBadgeNumber: domHelper.updateDiscussionAvatarBadgeNumber,
updateDiscussionBadgeNumber: domHelper.updateDiscussionBadgeNumber,
toggleCollapsed: domHelper.toggleCollapsed,
resizeCoordinatesToImageElement: utilsHelper.resizeCoordinatesToImageElement,
generateBadgeFromDiscussionDOM: utilsHelper.generateBadgeFromDiscussionDOM,
getTargetSelection: utilsHelper.getTargetSelection,
initImageDiff: utilsHelper.initImageDiff,
};

95
app/assets/javascripts/image_diff/helpers/utils_helper.js Normal file
View File

@ -0,0 +1,95 @@
import ImageBadge from '../image_badge';
import ImageDiff from '../image_diff';
import ReplacedImageDiff from '../replaced_image_diff';
import '../../commit/image_file';
export function resizeCoordinatesToImageElement(imageEl, meta) {
const { x, y, width, height } = meta;
const imageWidth = imageEl.width;
const imageHeight = imageEl.height;
const widthRatio = imageWidth / width;
const heightRatio = imageHeight / height;
return {
x: Math.round(x * widthRatio),
y: Math.round(y * heightRatio),
width: imageWidth,
height: imageHeight,
};
}
export function generateBadgeFromDiscussionDOM(imageFrameEl, discussionEl) {
const position = JSON.parse(discussionEl.dataset.position);
const firstNoteEl = discussionEl.querySelector('.note');
const badge = new ImageBadge({
actual: position,
imageEl: imageFrameEl.querySelector('img'),
noteId: firstNoteEl.id,
discussionId: discussionEl.dataset.discussionId,
});
return badge;
}
export function getTargetSelection(event) {
const containerEl = event.currentTarget;
const imageEl = containerEl.querySelector('img');
const x = event.offsetX;
const y = event.offsetY;
const width = imageEl.width;
const height = imageEl.height;
const actualWidth = imageEl.naturalWidth;
const actualHeight = imageEl.naturalHeight;
const widthRatio = actualWidth / width;
const heightRatio = actualHeight / height;
// Browser will include the frame as a clickable target,
// which would result in potential 1px out of bounds value
// This bound the coordinates to inside the frame
const normalizedX = Math.max(0, x) && Math.min(x, width);
const normalizedY = Math.max(0, y) && Math.min(y, height);
return {
browser: {
x: normalizedX,
y: normalizedY,
width,
height,
},
actual: {
// Round x, y so that we don't need to deal with decimals
x: Math.round(normalizedX * widthRatio),
y: Math.round(normalizedY * heightRatio),
width: actualWidth,
height: actualHeight,
},
};
}
export function initImageDiff(fileEl, canCreateNote, renderCommentBadge) {
const options = {
canCreateNote,
renderCommentBadge,
};
let diff;
// ImageFile needs to be invoked before initImageDiff so that badges
// can mount to the correct location
new gl.ImageFile(fileEl); // eslint-disable-line no-new
if (fileEl.querySelector('.diff-file .js-single-image')) {
diff = new ImageDiff(fileEl, options);
diff.init();
} else if (fileEl.querySelector('.diff-file .js-replaced-image')) {
diff = new ReplacedImageDiff(fileEl, options);
diff.init();
}
return diff;
}

23
app/assets/javascripts/image_diff/image_badge.js Normal file
View File

@ -0,0 +1,23 @@
import imageDiffHelper from './helpers/index';
const defaultMeta = {
x: 0,
y: 0,
width: 0,
height: 0,
};
export default class ImageBadge {
constructor(options) {
const { noteId, discussionId } = options;
this.actual = options.actual || defaultMeta;
this.browser = options.browser || defaultMeta;
this.noteId = noteId;
this.discussionId = discussionId;
if (options.imageEl && !options.browser) {
this.browser = imageDiffHelper.resizeCoordinatesToImageElement(options.imageEl, this.actual);
}
}
}

143
app/assets/javascripts/image_diff/image_diff.js Normal file
View File

@ -0,0 +1,143 @@
import imageDiffHelper from './helpers/index';
import ImageBadge from './image_badge';
import { isImageLoaded } from '../lib/utils/image_utility';
export default class ImageDiff {
constructor(el, options) {
this.el = el;
this.canCreateNote = !!(options && options.canCreateNote);
this.renderCommentBadge = !!(options && options.renderCommentBadge);
this.$noteContainer = $('.note-container', this.el);
this.imageBadges = [];
}
init() {
this.imageFrameEl = this.el.querySelector('.diff-file .js-image-frame');
this.imageEl = this.imageFrameEl.querySelector('img');
this.bindEvents();
}
bindEvents() {
this.imageClickedWrapper = this.imageClicked.bind(this);
this.imageBlurredWrapper = imageDiffHelper.removeCommentIndicator.bind(null, this.imageFrameEl);
this.addBadgeWrapper = this.addBadge.bind(this);
this.removeBadgeWrapper = this.removeBadge.bind(this);
this.renderBadgesWrapper = this.renderBadges.bind(this);
// Render badges
if (isImageLoaded(this.imageEl)) {
this.renderBadges();
} else {
this.imageEl.addEventListener('load', this.renderBadgesWrapper);
}
// jquery makes the event delegation here much simpler
this.$noteContainer.on('click', '.js-diff-notes-toggle', imageDiffHelper.toggleCollapsed);
$(this.el).on('click', '.comment-indicator', imageDiffHelper.commentIndicatorOnClick);
if (this.canCreateNote) {
this.el.addEventListener('click.imageDiff', this.imageClickedWrapper);
this.el.addEventListener('blur.imageDiff', this.imageBlurredWrapper);
this.el.addEventListener('addBadge.imageDiff', this.addBadgeWrapper);
this.el.addEventListener('removeBadge.imageDiff', this.removeBadgeWrapper);
}
}
imageClicked(event) {
const customEvent = event.detail;
const selection = imageDiffHelper.getTargetSelection(customEvent);
const el = customEvent.currentTarget;
imageDiffHelper.setPositionDataAttribute(el, selection.actual);
imageDiffHelper.showCommentIndicator(this.imageFrameEl, selection.browser);
}
renderBadges() {
const discussionsEls = this.el.querySelectorAll('.note-container .discussion-notes .notes');
[...discussionsEls].forEach(this.renderBadge.bind(this));
}
renderBadge(discussionEl, index) {
const imageBadge = imageDiffHelper
.generateBadgeFromDiscussionDOM(this.imageFrameEl, discussionEl);
this.imageBadges.push(imageBadge);
const options = {
coordinate: imageBadge.browser,
noteId: imageBadge.noteId,
};
if (this.renderCommentBadge) {
imageDiffHelper.addImageCommentBadge(this.imageFrameEl, options);
} else {
const numberBadgeOptions = Object.assign({}, options, {
badgeText: index + 1,
});
imageDiffHelper.addImageBadge(this.imageFrameEl, numberBadgeOptions);
}
}
addBadge(event) {
const { x, y, width, height, noteId, discussionId } = event.detail;
const badgeText = this.imageBadges.length + 1;
const imageBadge = new ImageBadge({
actual: {
x,
y,
width,
height,
},
imageEl: this.imageFrameEl.querySelector('img'),
noteId,
discussionId,
});
this.imageBadges.push(imageBadge);
imageDiffHelper.addImageBadge(this.imageFrameEl, {
coordinate: imageBadge.browser,
badgeText,
noteId,
});
imageDiffHelper.addAvatarBadge(this.el, {
detail: {
noteId,
badgeNumber: badgeText,
},
});
const discussionEl = this.el.querySelector(`#discussion_${discussionId}`);
imageDiffHelper.updateDiscussionBadgeNumber(discussionEl, badgeText);
}
removeBadge(event) {
const { badgeNumber } = event.detail;
const indexToRemove = badgeNumber - 1;
const imageBadgeEls = this.imageFrameEl.querySelectorAll('.badge');
if (this.imageBadges.length !== badgeNumber) {
// Cascade badges count numbers for (avatar badges + image badges)
this.imageBadges.forEach((badge, index) => {
if (index > indexToRemove) {
const { discussionId } = badge;
const updatedBadgeNumber = index;
const discussionEl = this.el.querySelector(`#discussion_${discussionId}`);
imageBadgeEls[index].innerText = updatedBadgeNumber;
imageDiffHelper.updateDiscussionBadgeNumber(discussionEl, updatedBadgeNumber);
imageDiffHelper.updateDiscussionAvatarBadgeNumber(discussionEl, updatedBadgeNumber);
}
});
}
this.imageBadges.splice(indexToRemove, 1);
const imageBadgeEl = imageBadgeEls[indexToRemove];
imageBadgeEl.remove();
}
}

12
app/assets/javascripts/image_diff/init_discussion_tab.js Normal file
View File

@ -0,0 +1,12 @@
import imageDiffHelper from './helpers/index';
export default () => {
// Always pass can-create-note as false because a user
// cannot place new badge markers on discussion tab
const canCreateNote = false;
const renderCommentBadge = true;
const diffFileEls = document.querySelectorAll('.timeline-content .diff-file.js-image-file');
[...diffFileEls].forEach(diffFileEl =>
imageDiffHelper.initImageDiff(diffFileEl, canCreateNote, renderCommentBadge));
};

92
app/assets/javascripts/image_diff/replaced_image_diff.js Normal file
View File

@ -0,0 +1,92 @@
import imageDiffHelper from './helpers/index';
import { viewTypes, isValidViewType } from './view_types';
import ImageDiff from './image_diff';
export default class ReplacedImageDiff extends ImageDiff {
init(defaultViewType = viewTypes.TWO_UP) {
this.imageFrameEls = {
[viewTypes.TWO_UP]: this.el.querySelector('.two-up .js-image-frame'),
[viewTypes.SWIPE]: this.el.querySelector('.swipe .js-image-frame'),
[viewTypes.ONION_SKIN]: this.el.querySelector('.onion-skin .js-image-frame'),
};
const viewModesEl = this.el.querySelector('.view-modes-menu');
this.viewModesEls = {
[viewTypes.TWO_UP]: viewModesEl.querySelector('.two-up'),
[viewTypes.SWIPE]: viewModesEl.querySelector('.swipe'),
[viewTypes.ONION_SKIN]: viewModesEl.querySelector('.onion-skin'),
};
this.currentView = defaultViewType;
this.generateImageEls();
this.bindEvents();
}
generateImageEls() {
this.imageEls = {};
const viewTypeNames = Object.getOwnPropertyNames(viewTypes);
viewTypeNames.forEach((viewType) => {
this.imageEls[viewType] = this.imageFrameEls[viewType].querySelector('img');
});
}
bindEvents() {
super.bindEvents();
this.changeToViewTwoUp = this.changeView.bind(this, viewTypes.TWO_UP);
this.changeToViewSwipe = this.changeView.bind(this, viewTypes.SWIPE);
this.changeToViewOnionSkin = this.changeView.bind(this, viewTypes.ONION_SKIN);
this.viewModesEls[viewTypes.TWO_UP].addEventListener('click', this.changeToViewTwoUp);
this.viewModesEls[viewTypes.SWIPE].addEventListener('click', this.changeToViewSwipe);
this.viewModesEls[viewTypes.ONION_SKIN].addEventListener('click', this.changeToViewOnionSkin);
}
get imageEl() {
return this.imageEls[this.currentView];
}
get imageFrameEl() {
return this.imageFrameEls[this.currentView];
}
changeView(newView) {
if (!isValidViewType(newView)) {
return;
}
const indicator = imageDiffHelper.removeCommentIndicator(this.imageFrameEl);
this.currentView = newView;
// Clear existing badges on new view
const existingBadges = this.imageFrameEl.querySelectorAll('.badge');
[...existingBadges].map(badge => badge.remove());
// Remove existing references to old view image badges
this.imageBadges = [];
// Image_file.js has a fade animation of 200ms for loading the view
// Need to wait an additional 250ms for the images to be displayed
// on window in order to re-normalize their dimensions
setTimeout(this.renderNewView.bind(this, indicator), 250);
}
renderNewView(indicator) {
// Generate badge coordinates on new view
this.renderBadges();
// Re-render indicator in new view
if (indicator.removed) {
const normalizedIndicator = imageDiffHelper
.resizeCoordinatesToImageElement(this.imageEl, {
x: indicator.x,
y: indicator.y,
width: indicator.image.width,
height: indicator.image.height,
});
imageDiffHelper.showCommentIndicator(this.imageFrameEl, normalizedIndicator);
}
}
}

9
app/assets/javascripts/image_diff/view_types.js Normal file
View File

@ -0,0 +1,9 @@
export const viewTypes = {
TWO_UP: 'TWO_UP',
SWIPE: 'SWIPE',
ONION_SKIN: 'ONION_SKIN',
};
export function isValidViewType(validate) {
return !!Object.getOwnPropertyNames(viewTypes).find(viewType => viewType === validate);
}

5
app/assets/javascripts/lib/utils/image_utility.js Normal file
View File

@ -0,0 +1,5 @@
/* eslint-disable import/prefer-default-export */
export function isImageLoaded(element) {
return element.complete && element.naturalHeight !== 0;
}

3
app/assets/javascripts/main.js
View File

@ -35,8 +35,6 @@ import './shortcuts_network';
import './templates/issuable_template_selector';
import './templates/issuable_template_selectors';
// commit
import './commit/file';
import './commit/image_file';
// lib/utils
@ -70,7 +68,6 @@ import './build';
import './build_artifacts';
import './build_variables';
import './ci_lint_editor';
import './commit';
import './commits';
import './compare';
import './compare_autocomplete';

4
app/assets/javascripts/merge_request_tabs.js
View File

@ -13,6 +13,8 @@ import {
isMetaClick,
} from './lib/utils/common_utils';
import initDiscussionTab from './image_diff/init_discussion_tab';
/* eslint-disable max-len */
// MergeRequestTabs
//
@ -154,6 +156,8 @@ import {
}
this.resetViewContainer();
this.destroyPipelinesView();
initDiscussionTab();
}
if (this.setUrl) {
this.setCurrentAction(action);

128
app/assets/javascripts/notes.js
View File

@ -24,6 +24,7 @@ import './autosave';
import './dropzone_input';
import TaskList from './task_list';
import { ajaxPost, isInViewport, getPagePath, scrollToElement, isMetaKey } from './lib/utils/common_utils';
import imageDiffHelper from './image_diff/helpers/index';
window.autosize = autosize;
window.Dropzone = Dropzone;
@ -42,6 +43,7 @@ export default class Notes {
this.visibilityChange = this.visibilityChange.bind(this);
this.cancelDiscussionForm = this.cancelDiscussionForm.bind(this);
this.onAddDiffNote = this.onAddDiffNote.bind(this);
this.onAddImageDiffNote = this.onAddImageDiffNote.bind(this);
this.setupDiscussionNoteForm = this.setupDiscussionNoteForm.bind(this);
this.onReplyToDiscussionNote = this.onReplyToDiscussionNote.bind(this);
this.removeNote = this.removeNote.bind(this);
@ -114,6 +116,8 @@ export default class Notes {
$(document).on('click', '.js-discussion-reply-button', this.onReplyToDiscussionNote);
// add diff note
$(document).on('click', '.js-add-diff-note-button', this.onAddDiffNote);
// add diff note for images
$(document).on('click', '.js-add-image-diff-note-button', this.onAddImageDiffNote);
// hide diff note form
$(document).on('click', '.js-close-discussion-note-form', this.cancelDiscussionForm);
// toggle commit list
@ -140,6 +144,7 @@ export default class Notes {
$(document).off('click', '.js-note-attachment-delete');
$(document).off('click', '.js-discussion-reply-button');
$(document).off('click', '.js-add-diff-note-button');
$(document).off('click', '.js-add-image-diff-note-button');
$(document).off('visibilitychange');
$(document).off('keyup input', '.js-note-text');
$(document).off('click', '.js-note-target-reopen');
@ -412,6 +417,11 @@ export default class Notes {
this.note_ids.push(noteEntity.id);
form = $form || $(`.js-discussion-note-form[data-discussion-id="${noteEntity.discussion_id}"]`);
row = form.closest('tr');
if (noteEntity.on_image) {
row = form;
}
lineType = this.isParallelView() ? form.find('#line_type').val() : 'old';
diffAvatarContainer = row.prevAll('.line_holder').first().find('.js-avatar-container.' + lineType + '_line');
// is this the first note of discussion?
@ -423,7 +433,7 @@ export default class Notes {
if (noteEntity.diff_discussion_html) {
var $discussion = $(noteEntity.diff_discussion_html).renderGFM();
if (!this.isParallelView() || row.hasClass('js-temp-notes-holder')) {
if (!this.isParallelView() || row.hasClass('js-temp-notes-holder') || noteEntity.on_image) {
// insert the note and the reply button after the temp row
row.after($discussion);
} else {
@ -449,6 +459,7 @@ export default class Notes {
if (typeof gl.diffNotesCompileComponents !== 'undefined' && noteEntity.discussion_resolvable) {
gl.diffNotesCompileComponents();
this.renderDiscussionAvatar(diffAvatarContainer, noteEntity);
}
@ -561,7 +572,7 @@ export default class Notes {
form.find('#note_line_code').val(),
// DiffNote
form.find('#note_position').val()
form.find('#note_position').val(),
];
return new Autosave(textarea, key);
}
@ -783,9 +794,22 @@ export default class Notes {
$(`.js-diff-avatars-${discussionId}`).trigger('remove.vue');
// The notes tr can contain multiple lists of notes, like on the parallel diff
if (notesTr.find('.discussion-notes').length > 1) {
// notesTr does not exist for image diffs
if (notesTr.find('.discussion-notes').length > 1 || notesTr.length === 0) {
const $diffFile = $notes.closest('.diff-file');
if ($diffFile.length > 0) {
const removeBadgeEvent = new CustomEvent('removeBadge.imageDiff', {
detail: {
// badgeNumber's start with 1 and index starts with 0
badgeNumber: $notes.index() + 1,
},
});
$diffFile[0].dispatchEvent(removeBadgeEvent);
}
$notes.remove();
} else {
} else if (notesTr.length > 0) {
notesTr.remove();
}
}
@ -841,7 +865,11 @@ export default class Notes {
*/
setupDiscussionNoteForm(dataHolder, form) {
// setup note target
const diffFileData = dataHolder.closest('.text-file');
let diffFileData = dataHolder.closest('.text-file');
if (diffFileData.length === 0) {
diffFileData = dataHolder.closest('.image');
}
var discussionID = dataHolder.data('discussionId');
@ -907,6 +935,31 @@ export default class Notes {
});
}
onAddImageDiffNote(e) {
const $link = $(e.currentTarget || e.target);
const $diffFile = $link.closest('.diff-file');
const clickEvent = new CustomEvent('click.imageDiff', {
detail: e,
});
$diffFile[0].dispatchEvent(clickEvent);
// Setup comment form
let newForm;
const $noteContainer = $link.closest('.diff-viewer').find('.note-container');
const $form = $noteContainer.find('> .discussion-form');
if ($form.length === 0) {
newForm = this.cleanForm(this.formClone.clone());
newForm.appendTo($noteContainer);
} else {
newForm = $form;
}
this.setupDiscussionNoteForm($link, newForm);
}
toggleDiffNote({
target,
lineType,
@ -999,10 +1052,25 @@ export default class Notes {
}
cancelDiscussionForm(e) {
var form;
e.preventDefault();
form = $(e.target).closest('.js-discussion-note-form');
return this.removeDiscussionNoteForm(form);
const $form = $(e.target).closest('.js-discussion-note-form');
const $discussionNote = $(e.target).closest('.discussion-notes');
if ($discussionNote.length === 0) {
// Only send blur event when the discussion form
// is not part of a discussion note
const $diffFile = $form.closest('.diff-file');
if ($diffFile.length > 0) {
const blurEvent = new CustomEvent('blur.imageDiff', {
detail: e,
});
$diffFile[0].dispatchEvent(blurEvent);
}
}
return this.removeDiscussionNoteForm($form);
}
/**
@ -1414,6 +1482,15 @@ export default class Notes {
// Submission successful! remove placeholder
$notesContainer.find(`#${noteUniqueId}`).remove();
const $diffFile = $form.closest('.diff-file');
if ($diffFile.length > 0) {
const blurEvent = new CustomEvent('blur.imageDiff', {
detail: e,
});
$diffFile[0].dispatchEvent(blurEvent);
}
// Reset cached commands list when command is applied
if (hasQuickActions) {
$form.find('textarea.js-note-text').trigger('clear-commands-cache.atwho');
@ -1436,7 +1513,28 @@ export default class Notes {
}
// Show final note element on UI
this.addDiscussionNote($form, note, $notesContainer.length === 0);
const isNewDiffComment = $notesContainer.length === 0;
this.addDiscussionNote($form, note, isNewDiffComment);
if (isNewDiffComment) {
// Add image badge, avatar badge and toggle discussion badge for new image diffs
const notePosition = $form.find('#note_position').val();
if ($diffFile.length > 0 && notePosition.length > 0) {
const { x, y, width, height } = JSON.parse(notePosition);
const addBadgeEvent = new CustomEvent('addBadge.imageDiff', {
detail: {
x,
y,
width,
height,
noteId: `note_${note.id}`,
discussionId: note.discussion_id,
},
});
$diffFile[0].dispatchEvent(addBadgeEvent);
}
}
// append flash-container to the Notes list
if ($notesContainer.length) {
@ -1457,6 +1555,16 @@ export default class Notes {
// Submission failed, remove placeholder note and show Flash error message
$notesContainer.find(`#${noteUniqueId}`).remove();
const blurEvent = new CustomEvent('blur.imageDiff', {
detail: e,
});
const closestDiffFile = $form.closest('.diff-file');
if (closestDiffFile.length) {
closestDiffFile[0].dispatchEvent(blurEvent);
}
if (hasQuickActions) {
$notesContainer.find(`#${systemNoteUniqueId}`).remove();
}
@ -1500,6 +1608,8 @@ export default class Notes {
const $noteBody = $editingNote.find('.js-task-list-container');
const $noteBodyText = $noteBody.find('.note-text');
const { formData, formContent, formAction } = this.getFormData($form);
const $diffFile = $form.closest('.diff-file');
const $notesContainer = $form.closest('.notes');
// Cache original comment content
const cachedNoteBodyText = $noteBodyText.html();

7
app/assets/javascripts/single_file_diff.js
View File

@ -1,6 +1,7 @@
/* eslint-disable func-names, prefer-arrow-callback, space-before-function-paren, no-var, prefer-rest-params, wrap-iife, one-var, one-var-declaration-per-line, consistent-return, no-param-reassign, max-len */
import FilesCommentButton from './files_comment_button';
import imageDiffHelper from './image_diff/helpers/index';
const WRAPPER = '<div class="diff-content"></div>';
const LOADING_HTML = '<i class="fa fa-spinner fa-spin"></i>';
@ -74,7 +75,11 @@ export default class SingleFileDiff {
gl.diffNotesCompileComponents();
}
FilesCommentButton.init($(_this.file));
const $file = $(_this.file);
FilesCommentButton.init($file);
const canCreateNote = $file.closest('.files').is('[data-can-create-note]');
imageDiffHelper.initImageDiff($file[0], canCreateNote);
if (cb) cb();
};

22
app/assets/stylesheets/framework/buttons.scss
View File

@ -1,3 +1,25 @@
@mixin btn-comment-icon {
border-radius: 50%;
background: $white-light;
padding: 1px 5px;
font-size: 12px;
color: $blue-500;
width: 23px;
height: 23px;
border: 1px solid $blue-500;
&:hover,
&.inverted {
background: $blue-500;
border-color: $blue-600;
color: $white-light;
}
&:active {
outline: 0;
}
}
@mixin btn-default {
border-radius: 3px;
font-size: $gl-font-size;

6
app/assets/stylesheets/framework/timeline.scss
View File

@ -17,15 +17,19 @@
.diff-file {
border: 1px solid $border-color;
border-bottom: none;
margin: 0;
}
&.text-file .diff-file {
border-bottom: none;
}
}
.timeline-entry {
border-color: $white-normal;
color: $gl-text-color;
border-bottom: 1px solid $border-white-light;
background: $white-light;
.timeline-entry-inner {
position: relative;

7
app/assets/stylesheets/framework/variables.scss
View File

@ -323,6 +323,7 @@ $diff-image-info-color: grey;
$diff-swipe-border: #999;
$diff-view-modes-color: grey;
$diff-view-modes-border: #c1c1c1;
$diff-jagged-border-gradient-color: darken($white-normal, 8%);
/*
* Fonts
@ -712,3 +713,9 @@ Issuable warning
*/
$issuable-warning-size: 24px;
$issuable-warning-icon-margin: 4px;
/*
Image Commenting cursor
*/
$image-comment-cursor-left-offset: 12;
$image-comment-cursor-top-offset: 30;

170
app/assets/stylesheets/pages/diff.scss
View File

@ -297,6 +297,7 @@
.drag-track {
display: block;
position: absolute;
top: 0;
left: 12px;
height: 10px;
width: 276px;
@ -547,16 +548,23 @@
}
.diff-notes-collapse {
width: 19px;
height: 19px;
width: 24px;
height: 24px;
border-radius: 50%;
padding: 0;
transition: transform .1s ease-out;
z-index: 100;
svg {
vertical-align: text-top;
.collapse-icon {
height: 50%;
width: 100%;
}
svg {
vertical-align: middle;
}
.collapse-icon,
path {
fill: $white-light;
}
@ -644,3 +652,157 @@
text-overflow: ellipsis;
white-space: nowrap;
}
.note-container {
background-color: $gray-light;
border-top: 1px solid $white-normal;
// double jagged line divider
.discussion-notes + .discussion-notes::before,
.discussion-notes + .discussion-form::before {
content: '';
position: relative;
display: block;
width: 100%;
height: 10px;
background-color: $white-light;
background-image: linear-gradient(45deg, transparent, transparent 73%, $diff-jagged-border-gradient-color 75%, $white-light 80%),
linear-gradient(225deg, transparent, transparent 73%, $diff-jagged-border-gradient-color 75%, $white-light 80%),
linear-gradient(135deg, transparent, transparent 73%, $diff-jagged-border-gradient-color 75%, $white-light 80%),
linear-gradient(-45deg, transparent, transparent 73%, $diff-jagged-border-gradient-color 75%, $white-light 80%);
background-position: 5px 5px,0 5px,0 5px,5px 5px;
background-size: 10px 10px;
background-repeat: repeat;
}
.notes {
position: relative;
}
.diff-notes-collapse {
position: absolute;
left: -12px;
}
}
.diff-file .note-container > .new-note,
.note-container .discussion-notes {
margin-left: 100px;
border-left: 1px solid $white-normal;
}
.notes.active {
.diff-file .note-container > .new-note,
.note-container .discussion-notes {
// Override our margin and border (set for diff tab)
// when user is on the discussion tab for MR
margin-left: inherit;
border-left: inherit;
}
}
.files:not([data-can-create-note]) .frame {
cursor: auto;
}
.frame.click-to-comment {
position: relative;
cursor: url(icon_image_comment.svg)
$image-comment-cursor-left-offset $image-comment-cursor-top-offset, auto;
// Retina cursor
cursor: -webkit-image-set(url(icon_image_comment.svg) 1x, url(icon_image_comment@2x.svg) 2x)
$image-comment-cursor-left-offset $image-comment-cursor-top-offset, auto;
.comment-indicator {
position: absolute;
padding: 0;
width: (2px * $image-comment-cursor-left-offset);
height: (1px * $image-comment-cursor-top-offset);
// center the indicator to match the top left click region
margin-top: (-1px * $image-comment-cursor-top-offset) + 2;
margin-left: (-1px * $image-comment-cursor-left-offset) + 1;
svg {
width: 100%;
height: 100%;
}
&:focus {
outline: none;
}
}
}
.frame .badge,
.image-diff-avatar-link .badge,
.notes > .badge {
position: absolute;
background-color: $blue-400;
color: $white-light;
border: $white-light 1px solid;
min-height: $gl-padding;
padding: 5px 8px;
border-radius: 12px;
&:focus {
outline: none;
}
}
.frame .badge,
.frame .image-comment-badge {
// Center align badges on the frame
transform: translate3d(-50%, -50%, 0);
}
.image-comment-badge {
@include btn-comment-icon;
position: absolute;
&.inverted {
border-color: $white-light;
}
}
.image-diff-avatar-link {
position: relative;
.badge,
.image-comment-badge {
top: 25px;
right: 8px;
}
}
.notes > .badge {
display: none;
left: -13px;
}
.discussion-notes {
min-height: 35px;
&:first-child {
// First child does not have the jagged borders
min-height: 25px;
}
&.collapsed {
background-color: $white-light;
.diff-notes-collapse,
.note,
.discussion-reply-holder, {
display: none;
}
.notes > .badge {
display: block;
}
}
}
.discussion-body .image .frame {
position: relative;
}

5
app/assets/stylesheets/pages/note_form.scss
View File

@ -161,10 +161,13 @@
}
.discussion-form {
padding: $gl-padding-top $gl-padding $gl-padding;
background-color: $white-light;
}
.discussion-form-container {
padding: $gl-padding-top $gl-padding $gl-padding;
}
.discussion-notes .disabled-comment {
padding: 6px 0;
}

19
app/assets/stylesheets/pages/notes.scss
View File

@ -650,29 +650,12 @@ ul.notes {
}
.add-diff-note {
@include btn-comment-icon;
opacity: 0;
margin-top: -2px;
border-radius: 50%;
background: $white-light;
padding: 1px 5px;
font-size: 12px;
color: $blue-500;
margin-left: -55px;
position: absolute;
z-index: 10;
width: 23px;
height: 23px;
border: 1px solid $blue-500;
&:hover {
background: $blue-500;
border-color: $blue-600;
color: $white-light;
}
&:active {
outline: 0;
}
}
.discussion-body,

11
app/controllers/concerns/notes_actions.rb
View File

@ -96,7 +96,8 @@ module NotesActions
id: note.id,
discussion_id: note.discussion_id(noteable),
html: note_html(note),
note: note.note
note: note.note,
on_image: note.try(:on_image?)
)
discussion = note.to_discussion(noteable)
@ -122,7 +123,9 @@ module NotesActions
def diff_discussion_html(discussion)
return unless discussion.diff_discussion?
if params[:view] == 'parallel'
on_image = discussion.on_image?
if params[:view] == 'parallel' && !on_image
template = "discussions/_parallel_diff_discussion"
locals =
if params[:line_type] == 'old'
@ -132,7 +135,9 @@ module NotesActions
end
else
template = "discussions/_diff_discussion"
locals = { discussions: [discussion] }
@fresh_discussion = true
locals = { discussions: [discussion], on_image: on_image }
end
render_to_string(

4
app/models/concerns/discussion_on_diff.rb
View File

@ -28,6 +28,10 @@ module DiscussionOnDiff
true
end
def file_new_path
first_note.position.new_path
end
# Returns an array of at most 16 highlighted lines above a diff note
def truncated_diff_lines(highlight: true)
lines = highlight ? highlighted_diff_lines : diff_lines

2
app/models/diff_discussion.rb
View File

@ -11,6 +11,8 @@ class DiffDiscussion < Discussion
delegate :position,
:original_position,
:change_position,
:on_text?,
:on_image?,
to: :first_note

14
app/models/diff_note.rb
View File

@ -12,8 +12,8 @@ class DiffNote < Note
validates :original_position, presence: true
validates :position, presence: true
validates :diff_line, presence: true
validates :line_code, presence: true, line_code: true
validates :diff_line, presence: true, if: :on_text?
validates :line_code, presence: true, line_code: true, if: :on_text?
validates :noteable_type, inclusion: { in: NOTEABLE_TYPES }
validate :positions_complete
validate :verify_supported
@ -43,6 +43,14 @@ class DiffNote < Note
end
end
def on_text?
position.position_type == "text"
end
def on_image?
position.position_type == "image"
end
def diff_file
@diff_file ||= self.original_position.diff_file(self.project.repository)
end
@ -56,6 +64,8 @@ class DiffNote < Note
end
def original_line_code
return unless on_text?
self.diff_file.line_code(self.diff_line)
end

4
app/models/discussion.rb
View File

@ -66,6 +66,10 @@ class Discussion
@context_noteable = context_noteable
end
def on_image?
false
end
def ==(other)
other.class == self.class &&
other.context_noteable == self.context_noteable &&

8
app/models/legacy_diff_discussion.rb
View File

@ -17,6 +17,14 @@ class LegacyDiffDiscussion < Discussion
true
end
def on_image?
false
end
def on_text?
true
end
def active?(*args)
return @active if @active.present?

14
app/models/note.rb
View File

@ -134,14 +134,22 @@ class Note < ActiveRecord::Base
Discussion.build(notes)
end
# Group diff discussions by line code or file path.
# It is not needed to group by line code when comment is
# on an image.
def grouped_diff_discussions(diff_refs = nil)
groups = {}
diff_notes.fresh.discussions.each do |discussion|
line_code = discussion.line_code_in_diffs(diff_refs)
group_key =
if discussion.on_image?
discussion.file_new_path
else
discussion.line_code_in_diffs(diff_refs)
end
if line_code
discussions = groups[line_code] ||= []
if group_key
discussions = groups[group_key] ||= []
discussions << discussion
end
end

16
app/views/discussions/_diff_discussion.html.haml
View File

@ -1,6 +1,10 @@
- expanded = local_assigns.fetch(:expanded, true)
%tr.notes_holder{ class: ('hide' unless expanded) }
%td.notes_line{ colspan: 2 }
%td.notes_content
.content{ class: ('hide' unless expanded) }
= render partial: "discussions/notes", collection: discussions, as: :discussion
- if local_assigns[:on_image]
= render partial: "discussions/notes", collection: discussions, as: :discussion
- else
-# Text diff discussions
- expanded = local_assigns.fetch(:expanded, true)
%tr.notes_holder{ class: ('hide' unless expanded) }
%td.notes_line{ colspan: 2 }
%td.notes_content
.content{ class: ('hide' unless expanded) }
= render partial: "discussions/notes", collection: discussions, as: :discussion

31
app/views/discussions/_diff_with_notes.html.haml
View File

@ -1,18 +1,27 @@
- diff_file = discussion.diff_file
- blob = discussion.blob
- discussions = { discussion.original_line_code => [discussion] }
- diff_file_class = diff_file.text? ? 'text-file' : 'js-image-file'
.diff-file.file-holder
.diff-file.file-holder{ class: diff_file_class }
.js-file-title.file-title.file-title-flex-parent
.file-header-content
= render "projects/diffs/file_header", diff_file: diff_file, url: discussion_path(discussion), show_toggle: false
.diff-content.code.js-syntax-highlight
%table
- discussions = { discussion.original_line_code => [discussion] }
= render partial: "projects/diffs/line",
collection: discussion.truncated_diff_lines,
as: :line,
locals: { diff_file: diff_file,
discussions: discussions,
discussion_expanded: true,
plain: true }
- if diff_file.text?
.diff-content.code.js-syntax-highlight
%table
= render partial: "projects/diffs/line",
collection: discussion.truncated_diff_lines,
as: :line,
locals: { diff_file: diff_file,
discussions: discussions,
discussion_expanded: true,
plain: true }
- else
- partial = (diff_file.new_file? || diff_file.deleted_file?) ? 'single_image_diff' : 'replaced_image_diff'
= render partial: "projects/diffs/#{partial}", locals: { diff_file: diff_file, position: discussion.position.to_json, click_to_comment: false }
.note-container
= render partial: "discussions/notes", locals: { discussion: discussion, show_toggle: false, show_image_comment_badge: true, disable_collapse: true }

19
app/views/discussions/_notes.html.haml
View File

@ -1,6 +1,19 @@
.discussion-notes
%ul.notes{ data: { discussion_id: discussion.id } }
= render partial: "shared/notes/note", collection: discussion.notes, as: :note
- disable_collapse = local_assigns.fetch(:disable_collapse, false)
- collapsed_class = 'collapsed' if discussion.resolved? && !disable_collapse
- badge_counter = discussion_counter + 1 if local_assigns[:discussion_counter]
- show_toggle = local_assigns.fetch(:show_toggle, true)
- show_image_comment_badge = local_assigns.fetch(:show_image_comment_badge, false)
.discussion-notes{ class: collapsed_class }
-# Save the first note position data so that we have a reference and can go
-# to the first note position when we click on a badge diff discussion
%ul.notes{ id: "discussion_#{discussion.id}", data: { discussion_id: discussion.id, position: discussion.notes[0].position.to_json } }
- if discussion.try(:on_image?) && show_toggle
%button.diff-notes-collapse.js-diff-notes-toggle{ type: 'button' }
= sprite_icon('collapse', css_class: 'collapse-icon')
%button.btn-transparent.badge.js-diff-notes-toggle{ type: 'button' }
= badge_counter
= render partial: "shared/notes/note", collection: discussion.notes, as: :note, locals: { badge_counter: badge_counter, show_image_comment_badge: show_image_comment_badge }
.flash-container

5
app/views/projects/diffs/_image_diff_frame.html.haml Normal file
View File

@ -0,0 +1,5 @@
- class_name = local_assigns.fetch(:class_name, '')
- note_type = local_assigns.fetch(:note_type, '')
.frame{ class: class_name, data: { position: position, note_type: note_type } }
= image_tag(image_path, alt: alt, draggable: false, lazy: false)

61
app/views/projects/diffs/_replaced_image_diff.html.haml Normal file
View File

@ -0,0 +1,61 @@
- blob = diff_file.blob
- old_blob = diff_file.old_blob
- blob_raw_path = diff_file_blob_raw_path(diff_file)
- old_blob_raw_path = diff_file_old_blob_raw_path(diff_file)
- click_to_comment = local_assigns.fetch(:click_to_comment, true)
- diff_view_data = local_assigns.fetch(:diff_view_data, '')
- class_name = ''
- if click_to_comment
- class_name = 'js-add-image-diff-note-button click-to-comment'
.image.js-replaced-image{ data: diff_view_data }
.two-up.view
.wrap
.frame.deleted
= image_tag(old_blob_raw_path, alt: diff_file.old_path, lazy: false)
%p.image-info.hide
%span.meta-filesize= number_to_human_size(old_blob.size)
|
%strong W:
%span.meta-width
|
%strong H:
%span.meta-height
.wrap
= render partial: "projects/diffs/image_diff_frame", locals: { class_name: "added js-image-frame #{class_name}", position: position, note_type: DiffNote.name, image_path: blob_raw_path, alt: diff_file.new_path }
%p.image-info.hide
%span.meta-filesize= number_to_human_size(blob.size)
|
%strong W:
%span.meta-width
|
%strong H:
%span.meta-height
.swipe.view.hide
.swipe-frame
.frame.deleted
= image_tag(old_blob_raw_path, alt: diff_file.old_path, lazy: false)
.swipe-wrap
= render partial: "projects/diffs/image_diff_frame", locals: { class_name: "added js-image-frame #{class_name}", position: position, note_type: DiffNote.name, image_path: blob_raw_path, alt: diff_file.new_path }
%span.swipe-bar
%span.top-handle
%span.bottom-handle
.onion-skin.view.hide
.onion-skin-frame
.frame.deleted
= image_tag(old_blob_raw_path, alt: diff_file.old_path, lazy: false)
= render partial: "projects/diffs/image_diff_frame", locals: { class_name: "added js-image-frame #{class_name}", position: position, note_type: DiffNote.name, image_path: blob_raw_path, alt: diff_file.new_path }
.controls
.transparent
.drag-track
.dragger{ :style => "left: 0px;" }
.opaque
.view-modes.hide
%ul.view-modes-menu
%li.two-up{ data: { mode: 'two-up' } } 2-up
%li.swipe{ data: { mode: 'swipe' } } Swipe
%li.onion-skin{ data: { mode: 'onion-skin' } } Onion skin

16
app/views/projects/diffs/_single_image_diff.html.haml Normal file
View File

@ -0,0 +1,16 @@
- blob = diff_file.blob
- old_blob = diff_file.old_blob
- blob_raw_path = diff_file_blob_raw_path(diff_file)
- old_blob_raw_path = diff_file_old_blob_raw_path(diff_file)
- click_to_comment = local_assigns.fetch(:click_to_comment, true)
- diff_view_data = local_assigns.fetch(:diff_view_data, '')
- class_name = ''
- if click_to_comment
- class_name = 'js-add-image-diff-note-button click-to-comment'
.image.js-single-image{ data: diff_view_data }
.wrap
- single_class_name = diff_file.deleted_file? ? 'deleted' : 'added'
= render partial: "projects/diffs/image_diff_frame", locals: { class_name: "#{single_class_name} #{class_name} js-image-frame", position: position, note_type: DiffNote.name, image_path: blob_raw_path, alt: diff_file.file_path }
%p.image-info= number_to_human_size(blob.size)

70
app/views/projects/diffs/viewers/_image.html.haml
View File

@ -1,67 +1,13 @@
- diff_file = viewer.diff_file
- blob = diff_file.blob
- old_blob = diff_file.old_blob
- blob_raw_path = diff_file_blob_raw_path(diff_file)
- old_blob_raw_path = diff_file_old_blob_raw_path(diff_file)
- image_point = Gitlab::Diff::ImagePoint.new(nil, nil, nil, nil)
- discussions = @grouped_diff_discussions[diff_file.new_path] if @grouped_diff_discussions
- locals = { diff_file: diff_file, position: diff_file.position(image_point, position_type: :image).to_json, click_to_comment: true, diff_view_data: diff_view_data }
- if diff_file.new_file? || diff_file.deleted_file?
.image
%span.wrap
.frame{ class: (diff_file.deleted_file? ? 'deleted' : 'added') }
= image_tag(blob_raw_path, alt: diff_file.file_path)
%p.image-info= number_to_human_size(blob.size)
= render partial: "projects/diffs/single_image_diff", locals: locals
- else
.image
.two-up.view
%span.wrap
.frame.deleted
= image_tag(old_blob_raw_path, alt: diff_file.old_path)
%p.image-info.hide
%span.meta-filesize= number_to_human_size(old_blob.size)
|
%b W:
%span.meta-width
|
%b H:
%span.meta-height
%span.wrap
.frame.added
= image_tag(blob_raw_path, alt: diff_file.new_path)
%p.image-info.hide
%span.meta-filesize= number_to_human_size(blob.size)
|
%b W:
%span.meta-width
|
%b H:
%span.meta-height
= render partial: "projects/diffs/replaced_image_diff", locals: locals
.swipe.view.hide
.swipe-frame
.frame.deleted
= image_tag(old_blob_raw_path, alt: diff_file.old_path, lazy: false)
.swipe-wrap
.frame.added
= image_tag(blob_raw_path, alt: diff_file.new_path, lazy: false)
%span.swipe-bar
%span.top-handle
%span.bottom-handle
.onion-skin.view.hide
.onion-skin-frame
.frame.deleted
= image_tag(old_blob_raw_path, alt: diff_file.old_path, lazy: false)
.frame.added
= image_tag(blob_raw_path, alt: diff_file.new_path, lazy: false)
.controls
.transparent
.drag-track
.dragger{ :style => "left: 0px;" }
.opaque
.view-modes.hide
%ul.view-modes-menu
%li.two-up{ data: { mode: 'two-up' } } 2-up
%li.swipe{ data: { mode: 'swipe' } } Swipe
%li.onion-skin{ data: { mode: 'onion-skin' } } Onion skin
.note-container
= render partial: "discussions/notes", collection: discussions, as: :discussion

29
app/views/shared/notes/_form.html.haml
View File

@ -24,20 +24,21 @@
-# DiffNote
= f.hidden_field :position
= render layout: 'projects/md_preview', locals: { url: preview_url, referenced_users: true } do
= render 'projects/zen', f: f,
attr: :note,
classes: 'note-textarea js-note-text',
placeholder: "Write a comment or drag your files here...",
supports_quick_actions: supports_quick_actions,
supports_autocomplete: supports_autocomplete
= render 'shared/notes/hints', supports_quick_actions: supports_quick_actions
.error-alert
.discussion-form-container
= render layout: 'projects/md_preview', locals: { url: preview_url, referenced_users: true } do
= render 'projects/zen', f: f,
attr: :note,
classes: 'note-textarea js-note-text',
placeholder: "Write a comment or drag your files here...",
supports_quick_actions: supports_quick_actions,
supports_autocomplete: supports_autocomplete
= render 'shared/notes/hints', supports_quick_actions: supports_quick_actions
.error-alert
.note-form-actions.clearfix
= render partial: 'shared/notes/comment_button'
.note-form-actions.clearfix
= render partial: 'shared/notes/comment_button'
= yield(:note_actions)
= yield(:note_actions)
%a.btn.btn-cancel.js-note-discard{ role: "button", data: {cancel_text: "Cancel" } }
Discard draft
%a.btn.btn-cancel.js-note-discard{ role: "button", data: {cancel_text: "Cancel" } }
Discard draft

15
app/views/shared/notes/_note.html.haml
View File

@ -1,7 +1,10 @@
- return unless note.author
- return if note.cross_reference_not_visible_for?(current_user)
- show_image_comment_badge = local_assigns.fetch(:show_image_comment_badge, false)
- note_editable = note_editable?(note)
- note_counter = local_assigns.fetch(:note_counter, 0)
%li.timeline-entry{ id: dom_id(note),
class: ["note", "note-row-#{note.id}", ('system-note' if note.system)],
data: { author_id: note.author.id,
@ -12,8 +15,18 @@
- if note.system
= icon_for_system_note(note)
- else
%a{ href: user_path(note.author) }
%a.image-diff-avatar-link{ href: user_path(note.author) }
= image_tag avatar_icon(note.author), alt: '', class: 'avatar s40'
- if note.is_a?(DiffNote) && note.on_image?
- if show_image_comment_badge && note_counter == 0
-# Only show this for the first comment in the discussion
%span.image-comment-badge.inverted
= icon('comment-o')
- elsif note_counter == 0
- counter = badge_counter if local_assigns[:badge_counter]
- badge_class = "hidden" if @fresh_discussion || counter.nil?
%span.badge{ class: badge_class }
= counter
.timeline-content
.note-header
.note-header-info

5
changelogs/unreleased/issue_35873.yml Normal file
View File

@ -0,0 +1,5 @@
---
title: Commenting on image diffs
merge_request: 14061
author:
type: added

29
lib/gitlab/diff/file.rb
View File

@ -27,16 +27,23 @@ module Gitlab
@fallback_diff_refs = fallback_diff_refs
end
def position(line)
def position(position_marker, position_type: :text)
return unless diff_refs
Position.new(
data = {
diff_refs: diff_refs,
position_type: position_type.to_s,
old_path: old_path,
new_path: new_path,
old_line: line.old_line,
new_line: line.new_line,
diff_refs: diff_refs
)
new_path: new_path
}
if position_type == :text
data.merge!(text_position_properties(position_marker))
else
data.merge!(image_position_properties(position_marker))
end
Position.new(data)
end
def line_code(line)
@ -228,6 +235,14 @@ module Gitlab
private
def text_position_properties(line)
{ old_line: line.old_line, new_line: line.new_line }
end
def image_position_properties(image_point)
image_point.to_h
end
def blobs_changed?
old_blob && new_blob && old_blob.id != new_blob.id
end

61
lib/gitlab/diff/formatters/base_formatter.rb Normal file
View File

@ -0,0 +1,61 @@
module Gitlab
module Diff
module Formatters
class BaseFormatter
attr_reader :old_path
attr_reader :new_path
attr_reader :base_sha
attr_reader :start_sha
attr_reader :head_sha
attr_reader :position_type
def initialize(attrs)
if diff_file = attrs[:diff_file]
attrs[:diff_refs] = diff_file.diff_refs
attrs[:old_path] = diff_file.old_path
attrs[:new_path] = diff_file.new_path
end
if diff_refs = attrs[:diff_refs]
attrs[:base_sha] = diff_refs.base_sha
attrs[:start_sha] = diff_refs.start_sha
attrs[:head_sha] = diff_refs.head_sha
end
@old_path = attrs[:old_path]
@new_path = attrs[:new_path]
@base_sha = attrs[:base_sha]
@start_sha = attrs[:start_sha]
@head_sha = attrs[:head_sha]
end
def key
[base_sha, start_sha, head_sha, Digest::SHA1.hexdigest(old_path || ""), Digest::SHA1.hexdigest(new_path || "")]
end
def to_h
{
base_sha: base_sha,
start_sha: start_sha,
head_sha: head_sha,
old_path: old_path,
new_path: new_path,
position_type: position_type
}
end
def position_type
raise NotImplementedError
end
def ==(other)
raise NotImplementedError
end
def complete?
raise NotImplementedError
end
end
end
end
end

43
lib/gitlab/diff/formatters/image_formatter.rb Normal file
View File

@ -0,0 +1,43 @@
module Gitlab
module Diff
module Formatters
class ImageFormatter < BaseFormatter
attr_reader :width
attr_reader :height
attr_reader :x
attr_reader :y
def initialize(attrs)
@x = attrs[:x]
@y = attrs[:y]
@width = attrs[:width]
@height = attrs[:height]
super(attrs)
end
def key
@key ||= super.push(x, y)
end
def complete?
x && y && width && height
end
def to_h
super.merge(width: width, height: height, x: x, y: y)
end
def position_type
"image"
end
def ==(other)
other.is_a?(self.class) &&
x == other.x &&
y == other.y
end
end
end
end
end

49
lib/gitlab/diff/formatters/text_formatter.rb Normal file
View File

@ -0,0 +1,49 @@
module Gitlab
module Diff
module Formatters
class TextFormatter < BaseFormatter
attr_reader :old_line
attr_reader :new_line
def initialize(attrs)
@old_line = attrs[:old_line]
@new_line = attrs[:new_line]
super(attrs)
end
def key
@key ||= super.push(old_line, new_line)
end
def complete?
old_line || new_line
end
def to_h
super.merge(old_line: old_line, new_line: new_line)
end
def line_age
if old_line && new_line
nil
elsif new_line
'new'
else
'old'
end
end
def position_type
"text"
end
def ==(other)
other.is_a?(self.class) &&
new_line == other.new_line &&
old_line == other.old_line
end
end
end
end
end

23
lib/gitlab/diff/image_point.rb Normal file
View File

@ -0,0 +1,23 @@
module Gitlab
module Diff
class ImagePoint
attr_reader :width, :height, :x, :y
def initialize(width, height, x, y)
@width = width
@height = height
@x = x
@y = y
end
def to_h
{
width: width,
height: height,
x: x,
y: y
}
end
end
end
end

88
lib/gitlab/diff/position.rb
View File

@ -1,37 +1,25 @@
# Defines a specific location, identified by paths and line numbers,
# Defines a specific location, identified by paths line numbers and image coordinates,
# within a specific diff, identified by start, head and base commit ids.
module Gitlab
module Diff
class Position
attr_reader :old_path
attr_reader :new_path
attr_reader :old_line
attr_reader :new_line
attr_reader :base_sha
attr_reader :start_sha
attr_reader :head_sha
attr_accessor :formatter
delegate :old_path,
:new_path,
:base_sha,
:start_sha,
:head_sha,
:old_line,
:new_line,
:position_type, to: :formatter
# A position can belong to a text line or to an image coordinate
# it depends of the position_type argument.
# Text position will have: new_line and old_line
# Image position will have: width, height, x, y
def initialize(attrs = {})
if diff_file = attrs[:diff_file]
attrs[:diff_refs] = diff_file.diff_refs
attrs[:old_path] = diff_file.old_path
attrs[:new_path] = diff_file.new_path
end
if diff_refs = attrs[:diff_refs]
attrs[:base_sha] = diff_refs.base_sha
attrs[:start_sha] = diff_refs.start_sha
attrs[:head_sha] = diff_refs.head_sha
end
@old_path = attrs[:old_path]
@new_path = attrs[:new_path]
@base_sha = attrs[:base_sha]
@start_sha = attrs[:start_sha]
@head_sha = attrs[:head_sha]
@old_line = attrs[:old_line]
@new_line = attrs[:new_line]
@formatter = get_formatter_class(attrs[:position_type]).new(attrs)
end
# `Gitlab::Diff::Position` objects are stored as serialized attributes in
@ -46,7 +34,11 @@ module Gitlab
end
def encode_with(coder)
coder['attributes'] = self.to_h
coder['attributes'] = formatter.to_h
end
def key
formatter.key
end
def ==(other)
@ -54,20 +46,11 @@ module Gitlab
other.diff_refs == diff_refs &&
other.old_path == old_path &&
other.new_path == new_path &&
other.old_line == old_line &&
other.new_line == new_line
other.formatter == formatter
end
def to_h
{
old_path: old_path,
new_path: new_path,
old_line: old_line,
new_line: new_line,
base_sha: base_sha,
start_sha: start_sha,
head_sha: head_sha
}
formatter.to_h
end
def inspect
@ -75,23 +58,15 @@ module Gitlab
end
def complete?
file_path.present? &&
(old_line || new_line) &&
diff_refs.complete?
file_path.present? && formatter.complete? && diff_refs.complete?
end
def to_json(opts = nil)
JSON.generate(self.to_h, opts)
JSON.generate(formatter.to_h, opts)
end
def type
if old_line && new_line
nil
elsif new_line
'new'
else
'old'
end
formatter.line_age
end
def unchanged?
@ -150,6 +125,17 @@ module Gitlab
diff_refs.compare_in(repository.project).diffs(paths: paths, expanded: true).diff_files.first
end
def get_formatter_class(type)
type ||= "text"
case type
when 'image'
Gitlab::Diff::Formatters::ImageFormatter
else
Gitlab::Diff::Formatters::TextFormatter
end
end
end
end
end

5
spec/factories/merge_requests.rb
View File

@ -22,6 +22,11 @@ FactoryGirl.define do
trait :with_diffs do
end
trait :with_image_diffs do
source_branch "add_images_and_changes"
target_branch "master"
end
trait :without_diffs do
source_branch "improve/awesome"
target_branch "master"

8
spec/features/merge_requests/diffs_spec.rb
View File

@ -42,8 +42,12 @@ feature 'Diffs URL', js: true do
visit "#{diffs_project_merge_request_path(project, merge_request)}#{fragment}"
end
it 'shows expanded note' do
expect(page).to have_selector(fragment, visible: true)
it 'shows collapsed note' do
wait_for_requests
expect(page).to have_selector('.discussion-notes.collapsed') do |note_container|
expect(note_container).to have_selector(fragment, visible: false)
end
end
end
end

196
spec/features/merge_requests/image_diff_notes.rb Normal file
View File

@ -0,0 +1,196 @@
require 'spec_helper'
feature 'image diff notes', js: true do
include NoteInteractionHelpers
let(:user) { create(:user) }
let(:project) { create(:project, :public, :repository) }
before do
project.team << [user, :master]
sign_in user
page.driver.set_cookie('sidebar_collapsed', 'true')
# Stub helper to return any blob file as image from public app folder.
# This is necessary to run this specs since we don't display repo images in capybara.
allow_any_instance_of(DiffHelper).to receive(:diff_file_blob_raw_path).and_return('/apple-touch-icon.png')
end
context 'create commit diff notes' do
commit_id = '2f63565e7aac07bcdadb654e253078b727143ec4'
describe 'create a new diff note' do
before do
visit project_commit_path(project, commit_id)
create_image_diff_note
end
it 'shows indicator badge on image diff' do
indicator = find('.js-image-badge')
expect(indicator).to have_content('1')
end
it 'shows the avatar badge on the new note' do
badge = find('.image-diff-avatar-link .badge')
expect(badge).to have_content('1')
end
it 'allows collapsing/expanding the discussion notes' do
find('.js-diff-notes-toggle', :first).click
expect(page).not_to have_content('image diff test comment')
find('.js-diff-notes-toggle').click
expect(page).to have_content('image diff test comment')
end
end
describe 'render commit diff notes' do
let(:path) { "files/images/6049019_460s.jpg" }
let(:commit) { project.commit('2f63565e7aac07bcdadb654e253078b727143ec4') }
let(:note1_position) do
Gitlab::Diff::Position.new(
old_path: path,
new_path: path,
width: 100,
height: 100,
x: 10,
y: 10,
position_type: "image",
diff_refs: commit.diff_refs
)
end
let(:note2_position) do
Gitlab::Diff::Position.new(
old_path: path,
new_path: path,
width: 100,
height: 100,
x: 20,
y: 20,
position_type: "image",
diff_refs: commit.diff_refs
)
end
let!(:note1) { create(:diff_note_on_commit, commit_id: commit.id, project: project, position: note1_position, note: 'my note 1') }
let!(:note2) { create(:diff_note_on_commit, commit_id: commit.id, project: project, position: note2_position, note: 'my note 2') }
before do
visit project_commit_path(project, commit.id)
wait_for_requests
end
it 'render diff indicators within the image diff frame' do
expect(page).to have_css('.js-image-badge', count: 2)
end
it 'shows the diff notes' do
expect(page).to have_css('.diff-content .note', count: 2)
end
it 'shows the diff notes with correct avatar badge numbers' do
expect(page).to have_css('.image-diff-avatar-link', text: 1)
expect(page).to have_css('.image-diff-avatar-link', text: 2)
end
end
end
%w(inline parallel).each do |view|
context "#{view} view" do
let(:merge_request) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, author: user) }
let(:path) { "files/images/ee_repo_logo.png" }
let(:position) do
Gitlab::Diff::Position.new(
old_path: path,
new_path: path,
width: 100,
height: 100,
x: 1,
y: 1,
position_type: "image",
diff_refs: merge_request.diff_refs
)
end
let!(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) }
describe 'creating a new diff note' do
before do
visit diffs_project_merge_request_path(project, merge_request, view: view)
create_image_diff_note
end
it 'shows indicator badge on image diff' do
indicator = find('.js-image-badge', match: :first)
expect(indicator).to have_content('1')
end
it 'shows the avatar badge on the new note' do
badge = find('.image-diff-avatar-link .badge', match: :first)
expect(badge).to have_content('1')
end
it 'allows expanding/collapsing the discussion notes' do
page.all('.js-diff-notes-toggle')[0].trigger('click')
page.all('.js-diff-notes-toggle')[1].trigger('click')
expect(page).not_to have_content('image diff test comment')
page.all('.js-diff-notes-toggle')[0].trigger('click')
page.all('.js-diff-notes-toggle')[1].trigger('click')
expect(page).to have_content('image diff test comment')
end
end
end
end
describe 'discussion tab polling', :js do
let(:merge_request) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, author: user) }
let(:path) { "files/images/ee_repo_logo.png" }
let(:position) do
Gitlab::Diff::Position.new(
old_path: path,
new_path: path,
width: 100,
height: 100,
x: 50,
y: 50,
position_type: "image",
diff_refs: merge_request.diff_refs
)
end
before do
visit project_merge_request_path(project, merge_request)
end
it 'render diff indicators within the image frame' do
diff_note = create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position)
wait_for_requests
expect(page).to have_selector('.image-comment-badge')
expect(page).to have_content(diff_note.note)
end
end
end
def create_image_diff_note
find('.js-add-image-diff-note-button', match: :first).click
page.all('.js-add-image-diff-note-button')[0].trigger('click')
find('.diff-content .note-textarea').native.send_keys('image diff test comment')
click_button 'Comment'
wait_for_requests
end

1
spec/features/merge_requests/user_posts_diff_notes_spec.rb
View File

@ -227,6 +227,7 @@ feature 'Merge requests > User posts diff notes', :js do
write_comment_on_line(line_holder, diff_side)
click_button 'Comment'
wait_for_requests
assert_comment_persistence(line_holder, asset_form_reset: asset_form_reset)

132
spec/javascripts/image_diff/helpers/badge_helper_spec.js Normal file
View File

@ -0,0 +1,132 @@
import * as badgeHelper from '~/image_diff/helpers/badge_helper';
import * as mockData from '../mock_data';
describe('badge helper', () => {
const { coordinate, noteId, badgeText, badgeNumber } = mockData;
let containerEl;
let buttonEl;
beforeEach(() => {
containerEl = document.createElement('div');
});
describe('createImageBadge', () => {
beforeEach(() => {
buttonEl = badgeHelper.createImageBadge(noteId, coordinate);
});
it('should create button', () => {
expect(buttonEl.tagName).toEqual('BUTTON');
expect(buttonEl.getAttribute('type')).toEqual('button');
});
it('should set disabled attribute', () => {
expect(buttonEl.hasAttribute('disabled')).toEqual(true);
});
it('should set noteId', () => {
expect(buttonEl.dataset.noteId).toEqual(noteId);
});
it('should set coordinate', () => {
expect(buttonEl.style.left).toEqual(`${coordinate.x}px`);
expect(buttonEl.style.top).toEqual(`${coordinate.y}px`);
});
describe('classNames', () => {
it('should set .js-image-badge by default', () => {
expect(buttonEl.className).toEqual('js-image-badge');
});
it('should add additional class names if parameter is passed', () => {
const classNames = ['first-class', 'second-class'];
buttonEl = badgeHelper.createImageBadge(noteId, coordinate, classNames);
expect(buttonEl.className).toEqual(classNames.concat('js-image-badge').join(' '));
});
});
});
describe('addImageBadge', () => {
beforeEach(() => {
badgeHelper.addImageBadge(containerEl, {
coordinate,
badgeText,
noteId,
});
buttonEl = containerEl.querySelector('button');
});
it('should appends button to container', () => {
expect(buttonEl).toBeDefined();
});
it('should set the badge text', () => {
expect(buttonEl.innerText).toEqual(badgeText);
});
it('should set the button coordinates', () => {
expect(buttonEl.style.left).toEqual(`${coordinate.x}px`);
expect(buttonEl.style.top).toEqual(`${coordinate.y}px`);
});
it('should set the button noteId', () => {
expect(buttonEl.dataset.noteId).toEqual(noteId);
});
});
describe('addImageCommentBadge', () => {
beforeEach(() => {
badgeHelper.addImageCommentBadge(containerEl, {
coordinate,
noteId,
});
buttonEl = containerEl.querySelector('button');
});
it('should append icon button to container', () => {
expect(buttonEl).toBeDefined();
});
it('should create icon comment button', () => {
const iconEl = buttonEl.querySelector('i');
expect(iconEl).toBeDefined();
expect(iconEl.classList.contains('fa')).toEqual(true);
expect(iconEl.classList.contains('fa-comment-o')).toEqual(true);
});
it('should have .image-comment-badge.inverted in button class', () => {
expect(buttonEl.classList.contains('image-comment-badge')).toEqual(true);
expect(buttonEl.classList.contains('inverted')).toEqual(true);
});
});
describe('addAvatarBadge', () => {
let avatarBadgeEl;
beforeEach(() => {
containerEl.innerHTML = `
<div id="${noteId}">
<div class="badge hidden">
</div>
</div>
`;
badgeHelper.addAvatarBadge(containerEl, {
detail: {
noteId,
badgeNumber,
},
});
avatarBadgeEl = containerEl.querySelector(`#${noteId} .badge`);
});
it('should update badge number', () => {
expect(avatarBadgeEl.innerText).toEqual(badgeNumber.toString());
});
it('should remove hidden class', () => {
expect(avatarBadgeEl.classList.contains('hidden')).toEqual(false);
});
});
});

139
spec/javascripts/image_diff/helpers/comment_indicator_helper_spec.js Normal file
View File

@ -0,0 +1,139 @@
import * as commentIndicatorHelper from '~/image_diff/helpers/comment_indicator_helper';
import * as mockData from '../mock_data';
describe('commentIndicatorHelper', () => {
const { coordinate } = mockData;
let containerEl;
beforeEach(() => {
containerEl = document.createElement('div');
});
describe('addCommentIndicator', () => {
let buttonEl;
beforeEach(() => {
commentIndicatorHelper.addCommentIndicator(containerEl, coordinate);
buttonEl = containerEl.querySelector('button');
});
it('should append button to container', () => {
expect(buttonEl).toBeDefined();
});
describe('button', () => {
it('should set coordinate', () => {
expect(buttonEl.style.left).toEqual(`${coordinate.x}px`);
expect(buttonEl.style.top).toEqual(`${coordinate.y}px`);
});
it('should contain image-comment-dark svg', () => {
const svgEl = buttonEl.querySelector('svg');
expect(svgEl).toBeDefined();
const svgLink = svgEl.querySelector('use').getAttribute('xlink:href');
expect(svgLink.indexOf('image-comment-dark') !== -1).toEqual(true);
});
});
});
describe('removeCommentIndicator', () => {
it('should return removed false if there is no comment-indicator', () => {
const result = commentIndicatorHelper.removeCommentIndicator(containerEl);
expect(result.removed).toEqual(false);
});
describe('has comment indicator', () => {
let result;
beforeEach(() => {
containerEl.innerHTML = `
<div class="comment-indicator" style="left:${coordinate.x}px; top: ${coordinate.y}px;">
<img src="${gl.TEST_HOST}/image.png">
</div>
`;
result = commentIndicatorHelper.removeCommentIndicator(containerEl);
});
it('should remove comment indicator', () => {
expect(containerEl.querySelector('.comment-indicator')).toBeNull();
});
it('should return removed true', () => {
expect(result.removed).toEqual(true);
});
it('should return indicator meta', () => {
expect(result.x).toEqual(coordinate.x);
expect(result.y).toEqual(coordinate.y);
expect(result.image).toBeDefined();
expect(result.image.width).toBeDefined();
expect(result.image.height).toBeDefined();
});
});
});
describe('showCommentIndicator', () => {
describe('commentIndicator exists', () => {
beforeEach(() => {
containerEl.innerHTML = `
<button class="comment-indicator"></button>
`;
commentIndicatorHelper.showCommentIndicator(containerEl, coordinate);
});
it('should set commentIndicator coordinates', () => {
const commentIndicatorEl = containerEl.querySelector('.comment-indicator');
expect(commentIndicatorEl.style.left).toEqual(`${coordinate.x}px`);
expect(commentIndicatorEl.style.top).toEqual(`${coordinate.y}px`);
});
});
describe('commentIndicator does not exist', () => {
beforeEach(() => {
commentIndicatorHelper.showCommentIndicator(containerEl, coordinate);
});
it('should addCommentIndicator', () => {
const buttonEl = containerEl.querySelector('.comment-indicator');
expect(buttonEl).toBeDefined();
expect(buttonEl.style.left).toEqual(`${coordinate.x}px`);
expect(buttonEl.style.top).toEqual(`${coordinate.y}px`);
});
});
});
describe('commentIndicatorOnClick', () => {
let event;
let textAreaEl;
beforeEach(() => {
containerEl.innerHTML = `
<div class="diff-viewer">
<button></button>
<div class="note-container">
<textarea class="note-textarea"></textarea>
</div>
</div>
`;
textAreaEl = containerEl.querySelector('textarea');
event = {
stopPropagation: () => {},
currentTarget: containerEl.querySelector('button'),
};
spyOn(event, 'stopPropagation');
spyOn(textAreaEl, 'focus');
commentIndicatorHelper.commentIndicatorOnClick(event);
});
it('should stopPropagation', () => {
expect(event.stopPropagation).toHaveBeenCalled();
});
it('should focus textAreaEl', () => {
expect(textAreaEl.focus).toHaveBeenCalled();
});
});
});

118
spec/javascripts/image_diff/helpers/dom_helper_spec.js Normal file
View File

@ -0,0 +1,118 @@
import * as domHelper from '~/image_diff/helpers/dom_helper';
import * as mockData from '../mock_data';
describe('domHelper', () => {
const { imageMeta, badgeNumber } = mockData;
describe('setPositionDataAttribute', () => {
let containerEl;
let attributeAfterCall;
const position = {
myProperty: 'myProperty',
};
beforeEach(() => {
containerEl = document.createElement('div');
containerEl.dataset.position = JSON.stringify(position);
domHelper.setPositionDataAttribute(containerEl, imageMeta);
attributeAfterCall = JSON.parse(containerEl.dataset.position);
});
it('should set x, y, width, height', () => {
expect(attributeAfterCall.x).toEqual(imageMeta.x);
expect(attributeAfterCall.y).toEqual(imageMeta.y);
expect(attributeAfterCall.width).toEqual(imageMeta.width);
expect(attributeAfterCall.height).toEqual(imageMeta.height);
});
it('should not override other properties', () => {
expect(attributeAfterCall.myProperty).toEqual('myProperty');
});
});
describe('updateDiscussionAvatarBadgeNumber', () => {
let discussionEl;
beforeEach(() => {
discussionEl = document.createElement('div');
discussionEl.innerHTML = `
<a href="#" class="image-diff-avatar-link">
<div class="badge"></div>
</a>
`;
domHelper.updateDiscussionAvatarBadgeNumber(discussionEl, badgeNumber);
});
it('should update avatar badge number', () => {
expect(discussionEl.querySelector('.badge').innerText).toEqual(badgeNumber.toString());
});
});
describe('updateDiscussionBadgeNumber', () => {
let discussionEl;
beforeEach(() => {
discussionEl = document.createElement('div');
discussionEl.innerHTML = `
<div class="badge"></div>
`;
domHelper.updateDiscussionBadgeNumber(discussionEl, badgeNumber);
});
it('should update discussion badge number', () => {
expect(discussionEl.querySelector('.badge').innerText).toEqual(badgeNumber.toString());
});
});
describe('toggleCollapsed', () => {
let element;
let discussionNotesEl;
beforeEach(() => {
element = document.createElement('div');
element.innerHTML = `
<div class="discussion-notes">
<button></button>
<form class="discussion-form"></form>
</div>
`;
discussionNotesEl = element.querySelector('.discussion-notes');
});
describe('not collapsed', () => {
beforeEach(() => {
domHelper.toggleCollapsed({
currentTarget: element.querySelector('button'),
});
});
it('should add collapsed class', () => {
expect(discussionNotesEl.classList.contains('collapsed')).toEqual(true);
});
it('should force formEl to display none', () => {
const formEl = element.querySelector('.discussion-form');
expect(formEl.style.display).toEqual('none');
});
});
describe('collapsed', () => {
beforeEach(() => {
discussionNotesEl.classList.add('collapsed');
domHelper.toggleCollapsed({
currentTarget: element.querySelector('button'),
});
});
it('should remove collapsed class', () => {
expect(discussionNotesEl.classList.contains('collapsed')).toEqual(false);
});
it('should force formEl to display block', () => {
const formEl = element.querySelector('.discussion-form');
expect(formEl.style.display).toEqual('block');
});
});
});
});

207
spec/javascripts/image_diff/helpers/utils_helper_spec.js Normal file
View File

@ -0,0 +1,207 @@
import * as utilsHelper from '~/image_diff/helpers/utils_helper';
import ImageDiff from '~/image_diff/image_diff';
import ReplacedImageDiff from '~/image_diff/replaced_image_diff';
import ImageBadge from '~/image_diff/image_badge';
import * as mockData from '../mock_data';
describe('utilsHelper', () => {
const {
noteId,
discussionId,
image,
imageProperties,
imageMeta,
} = mockData;
describe('resizeCoordinatesToImageElement', () => {
let result;
beforeEach(() => {
result = utilsHelper.resizeCoordinatesToImageElement(image, imageMeta);
});
it('should return x based on widthRatio', () => {
expect(result.x).toEqual(imageMeta.x * 0.5);
});
it('should return y based on heightRatio', () => {
expect(result.y).toEqual(imageMeta.y * 0.5);
});
it('should return image width', () => {
expect(result.width).toEqual(image.width);
});
it('should return image height', () => {
expect(result.height).toEqual(image.height);
});
});
describe('generateBadgeFromDiscussionDOM', () => {
let discussionEl;
let result;
beforeEach(() => {
const imageFrameEl = document.createElement('div');
imageFrameEl.innerHTML = `
<img src="${gl.TEST_HOST}/image.png">
`;
discussionEl = document.createElement('div');
discussionEl.dataset.discussionId = discussionId;
discussionEl.innerHTML = `
<div class="note" id="${noteId}"></div>
`;
discussionEl.dataset.position = JSON.stringify(imageMeta);
result = utilsHelper.generateBadgeFromDiscussionDOM(imageFrameEl, discussionEl);
});
it('should return actual image properties', () => {
const { actual } = result;
expect(actual.x).toEqual(imageMeta.x);
expect(actual.y).toEqual(imageMeta.y);
expect(actual.width).toEqual(imageMeta.width);
expect(actual.height).toEqual(imageMeta.height);
});
it('should return browser image properties', () => {
const { browser } = result;
expect(browser.x).toBeDefined();
expect(browser.y).toBeDefined();
expect(browser.width).toBeDefined();
expect(browser.height).toBeDefined();
});
it('should return instance of ImageBadge', () => {
expect(result instanceof ImageBadge).toEqual(true);
});
it('should return noteId', () => {
expect(result.noteId).toEqual(noteId);
});
it('should return discussionId', () => {
expect(result.discussionId).toEqual(discussionId);
});
});
describe('getTargetSelection', () => {
let containerEl;
beforeEach(() => {
containerEl = {
querySelector: () => imageProperties,
};
});
function generateEvent(offsetX, offsetY) {
return {
currentTarget: containerEl,
offsetX,
offsetY,
};
}
it('should return browser properties', () => {
const event = generateEvent(25, 25);
const result = utilsHelper.getTargetSelection(event);
const { browser } = result;
expect(browser.x).toEqual(event.offsetX);
expect(browser.y).toEqual(event.offsetY);
expect(browser.width).toEqual(imageProperties.width);
expect(browser.height).toEqual(imageProperties.height);
});
it('should return resized actual image properties', () => {
const event = generateEvent(50, 50);
const result = utilsHelper.getTargetSelection(event);
const { actual } = result;
expect(actual.x).toEqual(100);
expect(actual.y).toEqual(100);
expect(actual.width).toEqual(imageProperties.naturalWidth);
expect(actual.height).toEqual(imageProperties.naturalHeight);
});
describe('normalize coordinates', () => {
it('should return x = 0 if x < 0', () => {
const event = generateEvent(-5, 50);
const result = utilsHelper.getTargetSelection(event);
expect(result.browser.x).toEqual(0);
});
it('should return x = width if x > width', () => {
const event = generateEvent(1000, 50);
const result = utilsHelper.getTargetSelection(event);
expect(result.browser.x).toEqual(imageProperties.width);
});
it('should return y = 0 if y < 0', () => {
const event = generateEvent(50, -10);
const result = utilsHelper.getTargetSelection(event);
expect(result.browser.y).toEqual(0);
});
it('should return y = height if y > height', () => {
const event = generateEvent(50, 1000);
const result = utilsHelper.getTargetSelection(event);
expect(result.browser.y).toEqual(imageProperties.height);
});
});
});
describe('initImageDiff', () => {
let glCache;
let fileEl;
beforeEach(() => {
window.gl = window.gl || (window.gl = {});
glCache = window.gl;
window.gl.ImageFile = () => {};
fileEl = document.createElement('div');
fileEl.innerHTML = `
<div class="diff-file"></div>
`;
spyOn(ImageDiff.prototype, 'init').and.callFake(() => {});
spyOn(ReplacedImageDiff.prototype, 'init').and.callFake(() => {});
});
afterEach(() => {
window.gl = glCache;
});
it('should initialize gl.ImageFile', () => {
spyOn(window.gl, 'ImageFile');
utilsHelper.initImageDiff(fileEl, false, false);
expect(gl.ImageFile).toHaveBeenCalled();
});
it('should initialize ImageDiff if js-single-image', () => {
const diffFileEl = fileEl.querySelector('.diff-file');
diffFileEl.innerHTML = `
<div class="js-single-image">
</div>
`;
const imageDiff = utilsHelper.initImageDiff(fileEl, true, false);
expect(ImageDiff.prototype.init).toHaveBeenCalled();
expect(imageDiff.canCreateNote).toEqual(true);
expect(imageDiff.renderCommentBadge).toEqual(false);
});
it('should initialize ReplacedImageDiff if js-replaced-image', () => {
const diffFileEl = fileEl.querySelector('.diff-file');
diffFileEl.innerHTML = `
<div class="js-replaced-image">
</div>
`;
const replacedImageDiff = utilsHelper.initImageDiff(fileEl, false, true);
expect(ReplacedImageDiff.prototype.init).toHaveBeenCalled();
expect(replacedImageDiff.canCreateNote).toEqual(false);
expect(replacedImageDiff.renderCommentBadge).toEqual(true);
});
});
});

84
spec/javascripts/image_diff/image_badge_spec.js Normal file
View File

@ -0,0 +1,84 @@
import ImageBadge from '~/image_diff/image_badge';
import imageDiffHelper from '~/image_diff/helpers/index';
import * as mockData from './mock_data';
describe('ImageBadge', () => {
const { noteId, discussionId, imageMeta } = mockData;
const options = {
noteId,
discussionId,
};
it('should save actual property', () => {
const imageBadge = new ImageBadge(Object.assign({}, options, {
actual: imageMeta,
}));
const { actual } = imageBadge;
expect(actual.x).toEqual(imageMeta.x);
expect(actual.y).toEqual(imageMeta.y);
expect(actual.width).toEqual(imageMeta.width);
expect(actual.height).toEqual(imageMeta.height);
});
it('should save browser property', () => {
const imageBadge = new ImageBadge(Object.assign({}, options, {
browser: imageMeta,
}));
const { browser } = imageBadge;
expect(browser.x).toEqual(imageMeta.x);
expect(browser.y).toEqual(imageMeta.y);
expect(browser.width).toEqual(imageMeta.width);
expect(browser.height).toEqual(imageMeta.height);
});
it('should save noteId', () => {
const imageBadge = new ImageBadge(options);
expect(imageBadge.noteId).toEqual(noteId);
});
it('should save discussionId', () => {
const imageBadge = new ImageBadge(options);
expect(imageBadge.discussionId).toEqual(discussionId);
});
describe('default values', () => {
let imageBadge;
beforeEach(() => {
imageBadge = new ImageBadge(options);
});
it('should return defaultimageMeta if actual property is not provided', () => {
const { actual } = imageBadge;
expect(actual.x).toEqual(0);
expect(actual.y).toEqual(0);
expect(actual.width).toEqual(0);
expect(actual.height).toEqual(0);
});
it('should return defaultimageMeta if browser property is not provided', () => {
const { browser } = imageBadge;
expect(browser.x).toEqual(0);
expect(browser.y).toEqual(0);
expect(browser.width).toEqual(0);
expect(browser.height).toEqual(0);
});
});
describe('imageEl property is provided and not browser property', () => {
beforeEach(() => {
spyOn(imageDiffHelper, 'resizeCoordinatesToImageElement').and.returnValue(true);
});
it('should generate browser property', () => {
const imageBadge = new ImageBadge(Object.assign({}, options, {
imageEl: document.createElement('img'),
}));
expect(imageDiffHelper.resizeCoordinatesToImageElement).toHaveBeenCalled();
expect(imageBadge.browser).toEqual(true);
});
});
});

361
spec/javascripts/image_diff/image_diff_spec.js Normal file
View File

@ -0,0 +1,361 @@
import ImageDiff from '~/image_diff/image_diff';
import * as imageUtility from '~/lib/utils/image_utility';
import imageDiffHelper from '~/image_diff/helpers/index';
import * as mockData from './mock_data';
describe('ImageDiff', () => {
let element;
let imageDiff;
beforeEach(() => {
setFixtures(`
<div id="element">
<div class="diff-file">
<div class="js-image-frame">
<img src="${gl.TEST_HOST}/image.png">
<div class="comment-indicator"></div>
<div id="badge-1" class="badge">1</div>
<div id="badge-2" class="badge">2</div>
<div id="badge-3" class="badge">3</div>
</div>
<div class="note-container">
<div class="discussion-notes">
<div class="js-diff-notes-toggle"></div>
<div class="notes"></div>
</div>
<div class="discussion-notes">
<div class="js-diff-notes-toggle"></div>
<div class="notes"></div>
</div>
</div>
</div>
</div>
`);
element = document.getElementById('element');
});
describe('constructor', () => {
beforeEach(() => {
imageDiff = new ImageDiff(element, {
canCreateNote: true,
renderCommentBadge: true,
});
});
it('should set el', () => {
expect(imageDiff.el).toEqual(element);
});
it('should set canCreateNote', () => {
expect(imageDiff.canCreateNote).toEqual(true);
});
it('should set renderCommentBadge', () => {
expect(imageDiff.renderCommentBadge).toEqual(true);
});
it('should set $noteContainer', () => {
expect(imageDiff.$noteContainer[0]).toEqual(element.querySelector('.note-container'));
});
describe('default', () => {
beforeEach(() => {
imageDiff = new ImageDiff(element);
});
it('should set canCreateNote as false', () => {
expect(imageDiff.canCreateNote).toEqual(false);
});
it('should set renderCommentBadge as false', () => {
expect(imageDiff.renderCommentBadge).toEqual(false);
});
});
});
describe('init', () => {
beforeEach(() => {
spyOn(ImageDiff.prototype, 'bindEvents').and.callFake(() => {});
imageDiff = new ImageDiff(element);
imageDiff.init();
});
it('should set imageFrameEl', () => {
expect(imageDiff.imageFrameEl).toEqual(element.querySelector('.diff-file .js-image-frame'));
});
it('should set imageEl', () => {
expect(imageDiff.imageEl).toEqual(element.querySelector('.diff-file .js-image-frame img'));
});
it('should call bindEvents', () => {
expect(imageDiff.bindEvents).toHaveBeenCalled();
});
});
describe('bindEvents', () => {
let imageEl;
beforeEach(() => {
spyOn(imageDiffHelper, 'toggleCollapsed').and.callFake(() => {});
spyOn(imageDiffHelper, 'commentIndicatorOnClick').and.callFake(() => {});
spyOn(imageDiffHelper, 'removeCommentIndicator').and.callFake(() => {});
spyOn(ImageDiff.prototype, 'imageClicked').and.callFake(() => {});
spyOn(ImageDiff.prototype, 'addBadge').and.callFake(() => {});
spyOn(ImageDiff.prototype, 'removeBadge').and.callFake(() => {});
spyOn(ImageDiff.prototype, 'renderBadges').and.callFake(() => {});
imageEl = element.querySelector('.diff-file .js-image-frame img');
});
describe('default', () => {
beforeEach(() => {
spyOn(imageUtility, 'isImageLoaded').and.returnValue(false);
imageDiff = new ImageDiff(element);
imageDiff.imageEl = imageEl;
imageDiff.bindEvents();
});
it('should register click event delegation to js-diff-notes-toggle', () => {
element.querySelector('.js-diff-notes-toggle').click();
expect(imageDiffHelper.toggleCollapsed).toHaveBeenCalled();
});
it('should register click event delegation to comment-indicator', () => {
element.querySelector('.comment-indicator').click();
expect(imageDiffHelper.commentIndicatorOnClick).toHaveBeenCalled();
});
});
describe('image loaded', () => {
beforeEach(() => {
spyOn(imageUtility, 'isImageLoaded').and.returnValue(true);
imageDiff = new ImageDiff(element);
imageDiff.imageEl = imageEl;
});
it('should renderBadges', () => {});
});
describe('image not loaded', () => {
beforeEach(() => {
spyOn(imageUtility, 'isImageLoaded').and.returnValue(false);
imageDiff = new ImageDiff(element);
imageDiff.imageEl = imageEl;
imageDiff.bindEvents();
});
it('should registers load eventListener', () => {
const loadEvent = new Event('load');
imageEl.dispatchEvent(loadEvent);
expect(imageDiff.renderBadges).toHaveBeenCalled();
});
});
describe('canCreateNote', () => {
beforeEach(() => {
spyOn(imageUtility, 'isImageLoaded').and.returnValue(false);
imageDiff = new ImageDiff(element, {
canCreateNote: true,
});
imageDiff.imageEl = imageEl;
imageDiff.bindEvents();
});
it('should register click.imageDiff event', () => {
const event = new CustomEvent('click.imageDiff');
element.dispatchEvent(event);
expect(imageDiff.imageClicked).toHaveBeenCalled();
});
it('should register blur.imageDiff event', () => {
const event = new CustomEvent('blur.imageDiff');
element.dispatchEvent(event);
expect(imageDiffHelper.removeCommentIndicator).toHaveBeenCalled();
});
it('should register addBadge.imageDiff event', () => {
const event = new CustomEvent('addBadge.imageDiff');
element.dispatchEvent(event);
expect(imageDiff.addBadge).toHaveBeenCalled();
});
it('should register removeBadge.imageDiff event', () => {
const event = new CustomEvent('removeBadge.imageDiff');
element.dispatchEvent(event);
expect(imageDiff.removeBadge).toHaveBeenCalled();
});
});
describe('canCreateNote is false', () => {
beforeEach(() => {
spyOn(imageUtility, 'isImageLoaded').and.returnValue(false);
imageDiff = new ImageDiff(element);
imageDiff.imageEl = imageEl;
imageDiff.bindEvents();
});
it('should not register click.imageDiff event', () => {
const event = new CustomEvent('click.imageDiff');
element.dispatchEvent(event);
expect(imageDiff.imageClicked).not.toHaveBeenCalled();
});
});
});
describe('imageClicked', () => {
beforeEach(() => {
spyOn(imageDiffHelper, 'getTargetSelection').and.returnValue({
actual: {},
browser: {},
});
spyOn(imageDiffHelper, 'setPositionDataAttribute').and.callFake(() => {});
spyOn(imageDiffHelper, 'showCommentIndicator').and.callFake(() => {});
imageDiff = new ImageDiff(element);
imageDiff.imageClicked({
detail: {
currentTarget: {},
},
});
});
it('should call getTargetSelection', () => {
expect(imageDiffHelper.getTargetSelection).toHaveBeenCalled();
});
it('should call setPositionDataAttribute', () => {
expect(imageDiffHelper.setPositionDataAttribute).toHaveBeenCalled();
});
it('should call showCommentIndicator', () => {
expect(imageDiffHelper.showCommentIndicator).toHaveBeenCalled();
});
});
describe('renderBadges', () => {
beforeEach(() => {
spyOn(ImageDiff.prototype, 'renderBadge').and.callFake(() => {});
imageDiff = new ImageDiff(element);
imageDiff.renderBadges();
});
it('should call renderBadge for each discussionEl', () => {
const discussionEls = element.querySelectorAll('.note-container .discussion-notes .notes');
expect(imageDiff.renderBadge.calls.count()).toEqual(discussionEls.length);
});
});
describe('renderBadge', () => {
let discussionEls;
beforeEach(() => {
spyOn(imageDiffHelper, 'addImageBadge').and.callFake(() => {});
spyOn(imageDiffHelper, 'addImageCommentBadge').and.callFake(() => {});
spyOn(imageDiffHelper, 'generateBadgeFromDiscussionDOM').and.returnValue({
browser: {},
noteId: 'noteId',
});
discussionEls = element.querySelectorAll('.note-container .discussion-notes .notes');
imageDiff = new ImageDiff(element);
imageDiff.renderBadge(discussionEls[0], 0);
});
it('should populate imageBadges', () => {
expect(imageDiff.imageBadges.length).toEqual(1);
});
describe('renderCommentBadge', () => {
beforeEach(() => {
imageDiff.renderCommentBadge = true;
imageDiff.renderBadge(discussionEls[0], 0);
});
it('should call addImageCommentBadge', () => {
expect(imageDiffHelper.addImageCommentBadge).toHaveBeenCalled();
});
});
describe('renderCommentBadge is false', () => {
it('should call addImageBadge', () => {
expect(imageDiffHelper.addImageBadge).toHaveBeenCalled();
});
});
});
describe('addBadge', () => {
beforeEach(() => {
spyOn(imageDiffHelper, 'addImageBadge').and.callFake(() => {});
spyOn(imageDiffHelper, 'addAvatarBadge').and.callFake(() => {});
spyOn(imageDiffHelper, 'updateDiscussionBadgeNumber').and.callFake(() => {});
imageDiff = new ImageDiff(element);
imageDiff.imageFrameEl = element.querySelector('.diff-file .js-image-frame');
imageDiff.addBadge({
detail: {
x: 0,
y: 1,
width: 25,
height: 50,
noteId: 'noteId',
discussionId: 'discussionId',
},
});
});
it('should add imageBadge to imageBadges', () => {
expect(imageDiff.imageBadges.length).toEqual(1);
});
it('should call addImageBadge', () => {
expect(imageDiffHelper.addImageBadge).toHaveBeenCalled();
});
it('should call addAvatarBadge', () => {
expect(imageDiffHelper.addAvatarBadge).toHaveBeenCalled();
});
it('should call updateDiscussionBadgeNumber', () => {
expect(imageDiffHelper.updateDiscussionBadgeNumber).toHaveBeenCalled();
});
});
describe('removeBadge', () => {
beforeEach(() => {
const { imageMeta } = mockData;
spyOn(imageDiffHelper, 'updateDiscussionBadgeNumber').and.callFake(() => {});
spyOn(imageDiffHelper, 'updateDiscussionAvatarBadgeNumber').and.callFake(() => {});
imageDiff = new ImageDiff(element);
imageDiff.imageBadges = [imageMeta, imageMeta, imageMeta];
imageDiff.imageFrameEl = element.querySelector('.diff-file .js-image-frame');
imageDiff.removeBadge({
detail: {
badgeNumber: 2,
},
});
});
describe('cascade badge count', () => {
it('should update next imageBadgeEl value', () => {
const imageBadgeEls = imageDiff.imageFrameEl.querySelectorAll('.badge');
expect(imageBadgeEls[0].innerText).toEqual('1');
expect(imageBadgeEls[1].innerText).toEqual('2');
expect(imageBadgeEls.length).toEqual(2);
});
it('should call updateDiscussionBadgeNumber', () => {
expect(imageDiffHelper.updateDiscussionBadgeNumber).toHaveBeenCalled();
});
it('should call updateDiscussionAvatarBadgeNumber', () => {
expect(imageDiffHelper.updateDiscussionAvatarBadgeNumber).toHaveBeenCalled();
});
});
it('should remove badge from imageBadges', () => {
expect(imageDiff.imageBadges.length).toEqual(2);
});
it('should remove imageBadgeEl', () => {
expect(imageDiff.imageFrameEl.querySelector('#badge-2')).toBeNull();
});
});
});

37
spec/javascripts/image_diff/init_discussion_tab_spec.js Normal file
View File

@ -0,0 +1,37 @@
import initDiscussionTab from '~/image_diff/init_discussion_tab';
import imageDiffHelper from '~/image_diff/helpers/index';
describe('initDiscussionTab', () => {
beforeEach(() => {
setFixtures(`
<div class="timeline-content">
<div class="diff-file js-image-file"></div>
<div class="diff-file js-image-file"></div>
</div>
`);
});
it('should pass canCreateNote as false to initImageDiff', (done) => {
spyOn(imageDiffHelper, 'initImageDiff').and.callFake((diffFileEl, canCreateNote) => {
expect(canCreateNote).toEqual(false);
done();
});
initDiscussionTab();
});
it('should pass renderCommentBadge as true to initImageDiff', (done) => {
spyOn(imageDiffHelper, 'initImageDiff').and.callFake((diffFileEl, canCreateNote, renderCommentBadge) => {
expect(renderCommentBadge).toEqual(true);
done();
});
initDiscussionTab();
});
it('should call initImageDiff for each diffFileEls', () => {
spyOn(imageDiffHelper, 'initImageDiff').and.callFake(() => {});
initDiscussionTab();
expect(imageDiffHelper.initImageDiff.calls.count()).toEqual(2);
});
});

28
spec/javascripts/image_diff/mock_data.js Normal file
View File

@ -0,0 +1,28 @@
export const noteId = 'noteId';
export const discussionId = 'discussionId';
export const badgeText = 'badgeText';
export const badgeNumber = 5;
export const coordinate = {
x: 100,
y: 100,
};
export const image = {
width: 100,
height: 100,
};
export const imageProperties = {
width: image.width,
height: image.height,
naturalWidth: image.width * 2,
naturalHeight: image.height * 2,
};
export const imageMeta = {
x: coordinate.x,
y: coordinate.y,
width: imageProperties.naturalWidth,
height: imageProperties.naturalHeight,
};

312
spec/javascripts/image_diff/replaced_image_diff_spec.js Normal file
View File

@ -0,0 +1,312 @@
import ReplacedImageDiff from '~/image_diff/replaced_image_diff';
import ImageDiff from '~/image_diff/image_diff';
import { viewTypes } from '~/image_diff/view_types';
import imageDiffHelper from '~/image_diff/helpers/index';
describe('ReplacedImageDiff', () => {
let element;
let replacedImageDiff;
beforeEach(() => {
setFixtures(`
<div id="element">
<div class="two-up">
<div class="js-image-frame">
<img src="${gl.TEST_HOST}/image.png">
</div>
</div>
<div class="swipe">
<div class="js-image-frame">
<img src="${gl.TEST_HOST}/image.png">
</div>
</div>
<div class="onion-skin">
<div class="js-image-frame">
<img src="${gl.TEST_HOST}/image.png">
</div>
</div>
<div class="view-modes-menu">
<div class="two-up">2-up</div>
<div class="swipe">Swipe</div>
<div class="onion-skin">Onion skin</div>
</div>
</div>
`);
element = document.getElementById('element');
});
function setupImageFrameEls() {
replacedImageDiff.imageFrameEls = [];
replacedImageDiff.imageFrameEls[viewTypes.TWO_UP] = element.querySelector('.two-up .js-image-frame');
replacedImageDiff.imageFrameEls[viewTypes.SWIPE] = element.querySelector('.swipe .js-image-frame');
replacedImageDiff.imageFrameEls[viewTypes.ONION_SKIN] = element.querySelector('.onion-skin .js-image-frame');
}
function setupViewModesEls() {
replacedImageDiff.viewModesEls = [];
replacedImageDiff.viewModesEls[viewTypes.TWO_UP] = element.querySelector('.view-modes-menu .two-up');
replacedImageDiff.viewModesEls[viewTypes.SWIPE] = element.querySelector('.view-modes-menu .swipe');
replacedImageDiff.viewModesEls[viewTypes.ONION_SKIN] = element.querySelector('.view-modes-menu .onion-skin');
}
function setupImageEls() {
replacedImageDiff.imageEls = [];
replacedImageDiff.imageEls[viewTypes.TWO_UP] = element.querySelector('.two-up img');
replacedImageDiff.imageEls[viewTypes.SWIPE] = element.querySelector('.swipe img');
replacedImageDiff.imageEls[viewTypes.ONION_SKIN] = element.querySelector('.onion-skin img');
}
it('should extend ImageDiff', () => {
replacedImageDiff = new ReplacedImageDiff(element);
expect(replacedImageDiff instanceof ImageDiff).toEqual(true);
});
describe('init', () => {
beforeEach(() => {
spyOn(ReplacedImageDiff.prototype, 'bindEvents').and.callFake(() => {});
spyOn(ReplacedImageDiff.prototype, 'generateImageEls').and.callFake(() => {});
replacedImageDiff = new ReplacedImageDiff(element);
replacedImageDiff.init();
});
it('should set imageFrameEls', () => {
const { imageFrameEls } = replacedImageDiff;
expect(imageFrameEls).toBeDefined();
expect(imageFrameEls[viewTypes.TWO_UP]).toEqual(element.querySelector('.two-up .js-image-frame'));
expect(imageFrameEls[viewTypes.SWIPE]).toEqual(element.querySelector('.swipe .js-image-frame'));
expect(imageFrameEls[viewTypes.ONION_SKIN]).toEqual(element.querySelector('.onion-skin .js-image-frame'));
});
it('should set viewModesEls', () => {
const { viewModesEls } = replacedImageDiff;
expect(viewModesEls).toBeDefined();
expect(viewModesEls[viewTypes.TWO_UP]).toEqual(element.querySelector('.view-modes-menu .two-up'));
expect(viewModesEls[viewTypes.SWIPE]).toEqual(element.querySelector('.view-modes-menu .swipe'));
expect(viewModesEls[viewTypes.ONION_SKIN]).toEqual(element.querySelector('.view-modes-menu .onion-skin'));
});
it('should generateImageEls', () => {
expect(ReplacedImageDiff.prototype.generateImageEls).toHaveBeenCalled();
});
it('should bindEvents', () => {
expect(ReplacedImageDiff.prototype.bindEvents).toHaveBeenCalled();
});
describe('currentView', () => {
it('should set currentView', () => {
replacedImageDiff.init(viewTypes.ONION_SKIN);
expect(replacedImageDiff.currentView).toEqual(viewTypes.ONION_SKIN);
});
it('should default to viewTypes.TWO_UP', () => {
expect(replacedImageDiff.currentView).toEqual(viewTypes.TWO_UP);
});
});
});
describe('generateImageEls', () => {
beforeEach(() => {
spyOn(ReplacedImageDiff.prototype, 'bindEvents').and.callFake(() => {});
replacedImageDiff = new ReplacedImageDiff(element, {
canCreateNote: false,
renderCommentBadge: false,
});
setupImageFrameEls();
});
it('should set imageEls', () => {
replacedImageDiff.generateImageEls();
const { imageEls } = replacedImageDiff;
expect(imageEls).toBeDefined();
expect(imageEls[viewTypes.TWO_UP]).toEqual(element.querySelector('.two-up img'));
expect(imageEls[viewTypes.SWIPE]).toEqual(element.querySelector('.swipe img'));
expect(imageEls[viewTypes.ONION_SKIN]).toEqual(element.querySelector('.onion-skin img'));
});
});
describe('bindEvents', () => {
beforeEach(() => {
spyOn(ImageDiff.prototype, 'bindEvents').and.callFake(() => {});
replacedImageDiff = new ReplacedImageDiff(element);
setupViewModesEls();
});
it('should call super.bindEvents', () => {
replacedImageDiff.bindEvents();
expect(ImageDiff.prototype.bindEvents).toHaveBeenCalled();
});
it('should register click eventlistener to 2-up view mode', (done) => {
spyOn(ReplacedImageDiff.prototype, 'changeView').and.callFake((viewMode) => {
expect(viewMode).toEqual(viewTypes.TWO_UP);
done();
});
replacedImageDiff.bindEvents();
replacedImageDiff.viewModesEls[viewTypes.TWO_UP].click();
});
it('should register click eventlistener to swipe view mode', (done) => {
spyOn(ReplacedImageDiff.prototype, 'changeView').and.callFake((viewMode) => {
expect(viewMode).toEqual(viewTypes.SWIPE);
done();
});
replacedImageDiff.bindEvents();
replacedImageDiff.viewModesEls[viewTypes.SWIPE].click();
});
it('should register click eventlistener to onion skin view mode', (done) => {
spyOn(ReplacedImageDiff.prototype, 'changeView').and.callFake((viewMode) => {
expect(viewMode).toEqual(viewTypes.SWIPE);
done();
});
replacedImageDiff.bindEvents();
replacedImageDiff.viewModesEls[viewTypes.SWIPE].click();
});
});
describe('getters', () => {
describe('imageEl', () => {
beforeEach(() => {
replacedImageDiff = new ReplacedImageDiff(element);
replacedImageDiff.currentView = viewTypes.TWO_UP;
setupImageEls();
});
it('should return imageEl based on currentView', () => {
expect(replacedImageDiff.imageEl).toEqual(element.querySelector('.two-up img'));
replacedImageDiff.currentView = viewTypes.SWIPE;
expect(replacedImageDiff.imageEl).toEqual(element.querySelector('.swipe img'));
});
});
describe('imageFrameEl', () => {
beforeEach(() => {
replacedImageDiff = new ReplacedImageDiff(element);
replacedImageDiff.currentView = viewTypes.TWO_UP;
setupImageFrameEls();
});
it('should return imageFrameEl based on currentView', () => {
expect(replacedImageDiff.imageFrameEl).toEqual(element.querySelector('.two-up .js-image-frame'));
replacedImageDiff.currentView = viewTypes.ONION_SKIN;
expect(replacedImageDiff.imageFrameEl).toEqual(element.querySelector('.onion-skin .js-image-frame'));
});
});
});
describe('changeView', () => {
beforeEach(() => {
replacedImageDiff = new ReplacedImageDiff(element);
spyOn(imageDiffHelper, 'removeCommentIndicator').and.returnValue({
removed: false,
});
setupImageFrameEls();
});
describe('invalid viewType', () => {
beforeEach(() => {
replacedImageDiff.changeView('some-view-name');
});
it('should not call removeCommentIndicator', () => {
expect(imageDiffHelper.removeCommentIndicator).not.toHaveBeenCalled();
});
});
describe('valid viewType', () => {
beforeEach(() => {
jasmine.clock().install();
spyOn(ReplacedImageDiff.prototype, 'renderNewView').and.callFake(() => {});
replacedImageDiff.changeView(viewTypes.ONION_SKIN);
});
afterEach(() => {
jasmine.clock().uninstall();
});
it('should call removeCommentIndicator', () => {
expect(imageDiffHelper.removeCommentIndicator).toHaveBeenCalled();
});
it('should update currentView to newView', () => {
expect(replacedImageDiff.currentView).toEqual(viewTypes.ONION_SKIN);
});
it('should clear imageBadges', () => {
expect(replacedImageDiff.imageBadges.length).toEqual(0);
});
it('should call renderNewView', () => {
jasmine.clock().tick(251);
expect(replacedImageDiff.renderNewView).toHaveBeenCalled();
});
});
});
describe('renderNewView', () => {
beforeEach(() => {
replacedImageDiff = new ReplacedImageDiff(element);
});
it('should call renderBadges', () => {
spyOn(ReplacedImageDiff.prototype, 'renderBadges').and.callFake(() => {});
replacedImageDiff.renderNewView({
removed: false,
});
expect(replacedImageDiff.renderBadges).toHaveBeenCalled();
});
describe('removeIndicator', () => {
const indicator = {
removed: true,
x: 0,
y: 1,
image: {
width: 50,
height: 100,
},
};
beforeEach(() => {
setupImageEls();
setupImageFrameEls();
});
it('should pass showCommentIndicator normalized indicator values', (done) => {
spyOn(imageDiffHelper, 'showCommentIndicator').and.callFake(() => {});
spyOn(imageDiffHelper, 'resizeCoordinatesToImageElement').and.callFake((imageEl, meta) => {
expect(meta.x).toEqual(indicator.x);
expect(meta.y).toEqual(indicator.y);
expect(meta.width).toEqual(indicator.image.width);
expect(meta.height).toEqual(indicator.image.height);
done();
});
replacedImageDiff.renderNewView(indicator);
});
it('should call showCommentIndicator', (done) => {
const normalized = {
normalized: true,
};
spyOn(imageDiffHelper, 'resizeCoordinatesToImageElement').and.returnValue(normalized);
spyOn(imageDiffHelper, 'showCommentIndicator').and.callFake((imageFrameEl, normalizedIndicator) => {
expect(normalizedIndicator).toEqual(normalized);
done();
});
replacedImageDiff.renderNewView(indicator);
});
});
});
});

24
spec/javascripts/image_diff/view_types_spec.js Normal file
View File

@ -0,0 +1,24 @@
import { viewTypes, isValidViewType } from '~/image_diff/view_types';
describe('viewTypes', () => {
describe('isValidViewType', () => {
it('should return true for TWO_UP', () => {
expect(isValidViewType(viewTypes.TWO_UP)).toEqual(true);
});
it('should return true for SWIPE', () => {
expect(isValidViewType(viewTypes.SWIPE)).toEqual(true);
});
it('should return true for ONION_SKIN', () => {
expect(isValidViewType(viewTypes.ONION_SKIN)).toEqual(true);
});
it('should return false for non view types', () => {
expect(isValidViewType('some-view-type')).toEqual(false);
expect(isValidViewType(null)).toEqual(false);
expect(isValidViewType(undefined)).toEqual(false);
expect(isValidViewType('')).toEqual(false);
});
});
});

32
spec/javascripts/lib/utils/image_utility_spec.js Normal file
View File

@ -0,0 +1,32 @@
import * as imageUtility from '~/lib/utils/image_utility';
describe('imageUtility', () => {
describe('isImageLoaded', () => {
it('should return false when image.complete is false', () => {
const element = {
complete: false,
naturalHeight: 100,
};
expect(imageUtility.isImageLoaded(element)).toEqual(false);
});
it('should return false when naturalHeight = 0', () => {
const element = {
complete: true,
naturalHeight: 0,
};
expect(imageUtility.isImageLoaded(element)).toEqual(false);
});
it('should return true when image.complete and naturalHeight != 0', () => {
const element = {
complete: true,
naturalHeight: 100,
};
expect(imageUtility.isImageLoaded(element)).toEqual(true);
});
});
});

20
spec/lib/gitlab/diff/formatters/image_formatter_spec.rb Normal file
View File

@ -0,0 +1,20 @@
require 'spec_helper'
describe Gitlab::Diff::Formatters::ImageFormatter do
it_behaves_like "position formatter" do
let(:base_attrs) do
{
base_sha: 123,
start_sha: 456,
head_sha: 789,
old_path: 'old_image.png',
new_path: 'new_image.png',
position_type: 'image'
}
end
let(:attrs) do
base_attrs.merge(width: 100, height: 100, x: 1, y: 2)
end
end
end

42
spec/lib/gitlab/diff/formatters/text_formatter_spec.rb Normal file
View File

@ -0,0 +1,42 @@
require 'spec_helper'
describe Gitlab::Diff::Formatters::TextFormatter do
let!(:base) do
{
base_sha: 123,
start_sha: 456,
head_sha: 789,
old_path: 'old_path.txt',
new_path: 'new_path.txt'
}
end
let!(:complete) do
base.merge(old_line: 1, new_line: 2)
end
it_behaves_like "position formatter" do
let(:base_attrs) { base }
let(:attrs) { complete }
end
# Specific text formatter examples
let!(:formatter) { described_class.new(attrs) }
describe '#line_age' do
subject { formatter.line_age }
context ' when there is only new_line' do
let(:attrs) { base.merge(new_line: 1) }
it { is_expected.to eq('new') }
end
context ' when there is only old_line' do
let(:attrs) { base.merge(old_line: 1) }
it { is_expected.to eq('old') }
end
end
end

85
spec/lib/gitlab/diff/position_spec.rb
View File

@ -5,7 +5,7 @@ describe Gitlab::Diff::Position do
let(:project) { create(:project, :repository) }
describe "position for an added file" do
describe "position for an added text file" do
let(:commit) { project.commit("2ea1f3dec713d940208fb5ce4a38765ecb5d3f73") }
subject do
@ -47,6 +47,31 @@ describe Gitlab::Diff::Position do
end
end
describe "position for an added image file" do
let(:commit) { project.commit("33f3729a45c02fc67d00adb1b8bca394b0e761d9") }
subject do
described_class.new(
old_path: "files/images/6049019_460s.jpg",
new_path: "files/images/6049019_460s.jpg",
width: 100,
height: 100,
x: 1,
y: 100,
diff_refs: commit.diff_refs,
position_type: "image"
)
end
it "returns the correct diff file" do
diff_file = subject.diff_file(project.repository)
expect(diff_file.new_file?).to be true
expect(diff_file.new_path).to eq(subject.new_path)
expect(diff_file.diff_refs).to eq(subject.diff_refs)
end
end
describe "position for a changed file" do
let(:commit) { project.commit("570e7b2abdd848b95f2f578043fc23bd6f6fd24d") }
@ -468,26 +493,54 @@ describe Gitlab::Diff::Position do
end
describe "#to_json" do
let(:hash) do
{
old_path: "files/ruby/popen.rb",
new_path: "files/ruby/popen.rb",
old_line: nil,
new_line: 14,
base_sha: nil,
head_sha: nil,
start_sha: nil
}
shared_examples "diff position json" do
it "returns the position as JSON" do
expect(JSON.parse(diff_position.to_json)).to eq(hash.stringify_keys)
end
it "works when nested under another hash" do
expect(JSON.parse(JSON.generate(pos: diff_position))).to eq('pos' => hash.stringify_keys)
end
end
let(:diff_position) { described_class.new(hash) }
context "for text positon" do
let(:hash) do
{
old_path: "files/ruby/popen.rb",
new_path: "files/ruby/popen.rb",
old_line: nil,
new_line: 14,
base_sha: nil,
head_sha: nil,
start_sha: nil,
position_type: "text"
}
end
it "returns the position as JSON" do
expect(JSON.parse(diff_position.to_json)).to eq(hash.stringify_keys)
let(:diff_position) { described_class.new(hash) }
it_behaves_like "diff position json"
end
it "works when nested under another hash" do
expect(JSON.parse(JSON.generate(pos: diff_position))).to eq('pos' => hash.stringify_keys)
context "for image positon" do
let(:hash) do
{
old_path: "files/any.img",
new_path: "files/any.img",
base_sha: nil,
head_sha: nil,
start_sha: nil,
width: 100,
height: 100,
x: 1,
y: 100,
position_type: "image"
}
end
let(:diff_position) { described_class.new(hash) }
it_behaves_like "diff position json"
end
end
end

16
spec/lib/gitlab/diff/position_tracer_spec.rb
View File

@ -71,6 +71,10 @@ describe Gitlab::Diff::PositionTracer do
Gitlab::Diff::DiffRefs.new(base_sha: base_commit.id, head_sha: head_commit.id)
end
def text_position_attrs
[:old_line, :new_line]
end
def position(attrs = {})
attrs.reverse_merge!(
diff_refs: old_diff_refs
@ -91,7 +95,11 @@ describe Gitlab::Diff::PositionTracer do
expect(new_position.diff_refs).to eq(new_diff_refs)
attrs.each do |attr, value|
expect(new_position.send(attr)).to eq(value)
if text_position_attrs.include?(attr)
expect(new_position.formatter.send(attr)).to eq(value)
else
expect(new_position.send(attr)).to eq(value)
end
end
end
end
@ -110,7 +118,11 @@ describe Gitlab::Diff::PositionTracer do
expect(change_position.diff_refs).to eq(change_diff_refs)
attrs.each do |attr, value|
expect(change_position.send(attr)).to eq(value)
if text_position_attrs.include?(attr)
expect(change_position.formatter.send(attr)).to eq(value)
else
expect(change_position.send(attr)).to eq(value)
end
end
end
end

40
spec/models/diff_note_spec.rb
View File

@ -3,7 +3,7 @@ require 'spec_helper'
describe DiffNote do
include RepoHelpers
let(:merge_request) { create(:merge_request) }
let!(:merge_request) { create(:merge_request) }
let(:project) { merge_request.project }
let(:commit) { project.commit(sample_commit.id) }
@ -98,14 +98,14 @@ describe DiffNote do
diff_line = subject.diff_line
expect(diff_line.added?).to be true
expect(diff_line.new_line).to eq(position.new_line)
expect(diff_line.new_line).to eq(position.formatter.new_line)
expect(diff_line.text).to eq("+ vars = {")
end
end
describe "#line_code" do
it "returns the correct line code" do
line_code = Gitlab::Diff::LineCode.generate(position.file_path, position.new_line, 15)
line_code = Gitlab::Diff::LineCode.generate(position.file_path, position.formatter.new_line, 15)
expect(subject.line_code).to eq(line_code)
end
@ -255,4 +255,38 @@ describe DiffNote do
end
end
end
describe "image diff notes" do
let(:path) { "files/images/any_image.png" }
let!(:position) do
Gitlab::Diff::Position.new(
old_path: path,
new_path: path,
width: 10,
height: 10,
x: 1,
y: 1,
diff_refs: merge_request.diff_refs,
position_type: "image"
)
end
describe "validations" do
subject { build(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) }
it { is_expected.not_to validate_presence_of(:line_code) }
it "does not validate diff line" do
diff_line = subject.diff_line
expect(diff_line).to be nil
expect(subject).to be_valid
end
end
it "returns true for on_image?" do
expect(subject.on_image?).to be_truthy
end
end
end

50
spec/models/note_spec.rb
View File

@ -314,6 +314,56 @@ describe Note do
expect(subject[active_diff_note1.line_code].first.id).to eq(active_diff_note1.discussion_id)
expect(subject[active_diff_note3.line_code].first.id).to eq(active_diff_note3.discussion_id)
end
context 'with image discussions' do
let(:merge_request2) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, title: "Added images and changes") }
let(:image_path) { "files/images/ee_repo_logo.png" }
let(:text_path) { "bar/branch-test.txt" }
let!(:image_note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request2, position: image_position) }
let!(:text_note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request2, position: text_position) }
let(:image_position) do
Gitlab::Diff::Position.new(
old_path: image_path,
new_path: image_path,
width: 100,
height: 100,
x: 1,
y: 1,
position_type: "image",
diff_refs: merge_request2.diff_refs
)
end
let(:text_position) do
Gitlab::Diff::Position.new(
old_path: text_path,
new_path: text_path,
old_line: nil,
new_line: 2,
position_type: "text",
diff_refs: merge_request2.diff_refs
)
end
it "groups image discussions by file identifier" do
diff_discussion = DiffDiscussion.new([image_note])
discussions = merge_request2.notes.grouped_diff_discussions
expect(discussions.size).to eq(2)
expect(discussions[image_note.diff_file.new_path]).to include(diff_discussion)
end
it "groups text discussions by line code" do
diff_discussion = DiffDiscussion.new([text_note])
discussions = merge_request2.notes.grouped_diff_discussions
expect(discussions.size).to eq(2)
expect(discussions[text_note.line_code]).to include(diff_discussion)
end
end
end
context 'diff discussions for older diff refs' do

6
spec/services/discussions/update_diff_position_service_spec.rb
View File

@ -164,8 +164,8 @@ describe Discussions::UpdateDiffPositionService do
change_position = discussion.change_position
expect(change_position.start_sha).to eq(old_diff_refs.head_sha)
expect(change_position.head_sha).to eq(new_diff_refs.head_sha)
expect(change_position.old_line).to eq(9)
expect(change_position.new_line).to be_nil
expect(change_position.formatter.old_line).to eq(9)
expect(change_position.formatter.new_line).to be_nil
end
it 'creates a system discussion' do
@ -184,7 +184,7 @@ describe Discussions::UpdateDiffPositionService do
expect(discussion.original_position).to eq(old_position)
expect(discussion.position).not_to eq(old_position)
expect(discussion.position.new_line).to eq(22)
expect(discussion.position.formatter.new_line).to eq(22)
end
context 'when the resolve_outdated_diff_discussions setting is set' do

2
spec/support/features/discussion_comments_shared_example.rb
View File

@ -71,7 +71,7 @@ shared_examples 'discussion comments' do |resource_name|
expect(page).not_to have_selector menu_selector
find(toggle_selector).click
find('body').click
find('body').trigger 'click'
expect(page).not_to have_selector menu_selector
end

43
spec/support/shared_examples/position_formatters.rb Normal file
View File

@ -0,0 +1,43 @@
shared_examples_for "position formatter" do
let(:formatter) { described_class.new(attrs) }
describe '#key' do
let(:key) { [123, 456, 789, Digest::SHA1.hexdigest(formatter.old_path), Digest::SHA1.hexdigest(formatter.new_path), 1, 2] }
subject { formatter.key }
it { is_expected.to eq(key) }
end
describe '#complete?' do
subject { formatter.complete? }
context 'when there are missing key attributes' do
it { is_expected.to be_truthy }
end
context 'when old_line and new_line are nil' do
let(:attrs) { base_attrs }
it { is_expected.to be_falsy }
end
end
describe '#to_h' do
let(:formatter_hash) do
attrs.merge(position_type: base_attrs[:position_type] || 'text' )
end
subject { formatter.to_h }
it { is_expected.to eq(formatter_hash) }
end
describe '#==' do
subject { formatter }
let(:other_formatter) { described_class.new(attrs) }
it { is_expected.to eq(other_formatter) }
end
end

3
spec/support/test_env.rb
View File

@ -46,7 +46,8 @@ module TestEnv
'v1.1.0' => 'b83d6e3',
'add-ipython-files' => '93ee732',
'add-pdf-file' => 'e774ebd',
'add-pdf-text-binary' => '79faa7b'
'add-pdf-text-binary' => '79faa7b',
'add_images_and_changes' => '010d106'
}.freeze
# gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily