Merge branch 'image-comment-reviews' into 'master'

Fixed start review not working on images

Closes #10072

See merge request gitlab-org/gitlab-ee!10673

app/assets/javascripts/diffs/components/diff_content.vue

 <script>
 import { mapActions, mapGetters, mapState } from 'vuex';
+import diffLineNoteFormMixin from 'ee_else_ce/notes/mixins/diff_line_note_form';
+import draftCommentsMixin from 'ee_else_ce/diffs/mixins/draft_comments';
 import DiffViewer from '~/vue_shared/components/diff_viewer/diff_viewer.vue';
 import NotDiffableViewer from '~/vue_shared/components/diff_viewer/viewers/not_diffable.vue';
 import NoPreviewViewer from '~/vue_shared/components/diff_viewer/viewers/no_preview.vue';
@@ -22,7 +24,9 @@ export default {
     ImageDiffOverlay,
     NotDiffableViewer,
     NoPreviewViewer,
+    DiffFileDrafts: () => import('ee_component/batch_comments/components/diff_file_drafts.vue'),
   },
+  mixins: [diffLineNoteFormMixin, draftCommentsMixin],
   props: {
     diffFile: {
       type: Object,
@@ -58,10 +62,13 @@ export default {
       return this.diffViewerMode === diffViewerModes.not_diffable;
     },
     diffFileCommentForm() {
-      return this.getCommentFormForDiffFile(this.diffFile.file_hash);
+      return this.getCommentFormForDiffFile(this.diffFileHash);
     },
     showNotesContainer() {
-      return this.diffFile.discussions.length || this.diffFileCommentForm;
+      return this.imageDiscussions.length || this.diffFileCommentForm;
+    },
+    diffFileHash() {
+      return this.diffFile.file_hash;
     },
   },
   methods: {
@@ -112,15 +119,15 @@ export default {
         :new-sha="diffFile.diff_refs.head_sha"
         :old-path="diffFile.old_path"
         :old-sha="diffFile.diff_refs.base_sha"
-        :file-hash="diffFile.file_hash"
+        :file-hash="diffFileHash"
         :project-path="projectPath"
         :a-mode="diffFile.a_mode"
         :b-mode="diffFile.b_mode"
       >
         <image-diff-overlay
           slot="image-overlay"
-          :discussions="diffFile.discussions"
-          :file-hash="diffFile.file_hash"
+          :discussions="imageDiscussions"
+          :file-hash="diffFileHash"
           :can-comment="getNoteableData.current_user.can_create_note"
         />
         <div v-if="showNotesContainer" class="note-container">
@@ -131,14 +138,16 @@ export default {
             :should-collapse-discussions="true"
             :render-avatar-badge="true"
           />
+          <diff-file-drafts :file-hash="diffFileHash" class="diff-file-discussions" />
           <note-form
             v-if="diffFileCommentForm"
             ref="noteForm"
             :is-editing="false"
             :save-button-title="__('Comment')"
             class="diff-comment-form new-note discussion-form discussion-form-container"
+            @handleFormUpdateAddToReview="addToReview"
             @handleFormUpdate="handleSaveNote"
-            @cancelForm="closeDiffFileCommentForm(diffFile.file_hash)"
+            @cancelForm="closeDiffFileCommentForm(diffFileHash)"
           />
         </div>
       </diff-viewer>

app/assets/javascripts/diffs/components/image_diff_overlay.vue

 <script>
 import { mapActions, mapGetters } from 'vuex';
 import _ from 'underscore';
+import imageDiffMixin from 'ee_else_ce/diffs/mixins/image_diff';
 import Icon from '~/vue_shared/components/icon.vue';
 
 export default {
@@ -8,6 +9,7 @@ export default {
   components: {
     Icon,
   },
+  mixins: [imageDiffMixin],
   props: {
     discussions: {
       type: [Array, Object],
@@ -48,7 +50,6 @@ export default {
     },
   },
   methods: {
-    ...mapActions(['toggleDiscussion']),
     ...mapActions('diffs', ['openDiffFileCommentForm']),
     getImageDimensions() {
       return {
@@ -105,15 +106,15 @@ export default {
       v-for="(discussion, index) in allDiscussions"
       :key="discussion.id"
       :style="getPosition(discussion)"
-      :class="badgeClass"
+      :class="[badgeClass, { 'is-draft': discussion.isDraft }]"
       :disabled="!shouldToggleDiscussion"
       class="js-image-badge"
       type="button"
-      @click="toggleDiscussion({ discussionId: discussion.id })"
+      @click="clickedToggle(discussion)"
     >
       <icon v-if="showCommentIcon" name="image-comment-dark" />
       <template v-else>
-        {{ index + 1 }}
+        {{ toggleText(discussion, index) }}
       </template>
     </button>
     <button

app/assets/javascripts/diffs/mixins/draft_comments.js

@@ -3,5 +3,8 @@ export default {
     shouldRenderDraftRow: () => () => false,
     shouldRenderParallelDraftRow: () => () => false,
     draftForLine: () => () => ({}),
+    imageDiscussions() {
+      return this.diffFile.discussions;
+    },
   },
 };

app/assets/javascripts/diffs/mixins/image_diff.js

+import { mapActions } from 'vuex';
+
+export default {
+  methods: {
+    ...mapActions(['toggleDiscussion']),
+    clickedToggle(discussion) {
+      this.toggleDiscussion({ discussionId: discussion.id });
+    },
+    toggleText(discussion, index) {
+      return index + 1;
+    },
+  },
+};

app/assets/javascripts/diffs/store/mutations.js

@@ -160,7 +160,9 @@ export default {
         }
 
         if (!file.parallel_diff_lines || !file.highlighted_diff_lines) {
-          file.discussions = (file.discussions || []).concat(discussion);
+          file.discussions = (file.discussions || [])
+            .filter(d => d.id !== discussion.id)
+            .concat(discussion);
         }
 
         return file;

ee/app/assets/javascripts/batch_comments/components/diff_file_drafts.vue

+<script>
+import { mapGetters } from 'vuex';
+import imageDiff from '~/diffs/mixins/image_diff';
+import DraftNote from './draft_note.vue';
+
+export default {
+  components: {
+    DraftNote,
+  },
+  mixins: [imageDiff],
+  props: {
+    fileHash: {
+      type: String,
+      required: true,
+    },
+  },
+  computed: {
+    ...mapGetters('batchComments', ['draftsForFile']),
+    drafts() {
+      return this.draftsForFile(this.fileHash);
+    },
+  },
+};
+</script>
+
+<template>
+  <div>
+    <div
+      v-for="(draft, index) in drafts"
+      :key="draft.id"
+      class="discussion-notes diff-discussions position-relative"
+    >
+      <div class="notes">
+        <span class="d-block btn-transparent badge badge-pill is-draft js-diff-notes-index">
+          {{ toggleText(draft, index) }}
+        </span>
+        <draft-note :draft="draft" />
+      </div>
+    </div>
+  </div>
+</template>

ee/app/assets/javascripts/batch_comments/components/preview_item.vue

 <script>
 import { mapActions, mapGetters } from 'vuex';
+import { IMAGE_DIFF_POSITION_TYPE } from '~/diffs/constants';
 import { sprintf, __ } from '~/locale';
 import Icon from '~/vue_shared/components/icon.vue';
 import resolvedStatusMixin from '../mixins/resolved_status';
@@ -43,6 +44,10 @@ export default {
       });
     },
     linePosition() {
+      if (this.draft.position && this.draft.position.position_type === IMAGE_DIFF_POSITION_TYPE) {
+        return `${this.draft.position.x}x ${this.draft.position.y}y`;
+      }
+
       const position = this.discussion ? this.discussion.position : this.draft.position;
 
       return position.new_line || position.old_line;
@@ -78,7 +83,7 @@ export default {
   >
     <span class="review-preview-item-header">
       <icon class="append-right-8 flex-shrink-0" :name="iconName" />
-      <span class="bold">
+      <span class="bold text-nowrap">
         <span class="review-preview-item-header-text block-truncated"> {{ titleText }} </span>
         <template v-if="showLinePosition">
           :{{ linePosition }}

ee/app/assets/javascripts/batch_comments/stores/modules/batch_comments/getters.js

@@ -61,6 +61,9 @@ export const draftForLine = (state, getters) => (diffFileSha, line, side = null)
   return {};
 };
 
+export const draftsForFile = state => diffFileSha =>
+  state.drafts.filter(draft => draft.file_hash === diffFileSha);
+
 export const isPublishingDraft = state => draftId =>
   state.currentlyPublishingDrafts.indexOf(draftId) !== -1;
 

ee/app/assets/javascripts/diffs/mixins/draft_comments.js

@@ -6,6 +6,10 @@ export default {
       'shouldRenderDraftRow',
       'shouldRenderParallelDraftRow',
       'draftForLine',
+      'draftsForFile',
     ]),
+    imageDiscussions() {
+      return this.diffFile.discussions.concat(this.draftsForFile(this.diffFile.file_hash));
+    },
   },
 };

ee/app/assets/javascripts/diffs/mixins/image_diff.js

+import { mapActions } from 'vuex';
+
+export default {
+  computed: {
+    diffFileDiscussions() {
+      return this.allDiscussions.filter(d => !d.isDraft);
+    },
+  },
+  methods: {
+    ...mapActions(['toggleDiscussion']),
+    clickedToggle(discussion) {
+      if (!discussion.isDraft) {
+        this.toggleDiscussion({ discussionId: discussion.id });
+      }
+    },
+    toggleText(discussion, index) {
+      const count = index + 1;
+
+      return discussion.isDraft ? count - this.diffFileDiscussions.length : count;
+    },
+  },
+};

ee/app/assets/javascripts/notes/mixins/diff_line_note_form.js

 import { mapActions, mapGetters, mapState } from 'vuex';
+import { TEXT_DIFF_POSITION_TYPE, IMAGE_DIFF_POSITION_TYPE } from '~/diffs/constants';
 import { getDraftReplyFormData, getDraftFormData } from 'ee/batch_comments/utils';
 import createFlash from '~/flash';
 import { s__ } from '~/locale';
@@ -6,6 +7,7 @@ import { s__ } from '~/locale';
 export default {
   computed: {
     ...mapState({
+      noteableData: state => state.notes.noteableData,
       notesData: state => state.notes.notesData,
       withBatchComments: state => state.batchComments && state.batchComments.withBatchComments,
     }),
@@ -41,6 +43,9 @@ export default {
         });
     },
     addToReview(note) {
+      const positionType = this.diffFileCommentForm
+        ? IMAGE_DIFF_POSITION_TYPE
+        : TEXT_DIFF_POSITION_TYPE;
       const selectedDiffFile = this.getDiffFileByHash(this.diffFileHash);
       const postData = getDraftFormData({
         note,
@@ -51,11 +56,17 @@ export default {
         diffViewType: this.diffViewType,
         diffFile: selectedDiffFile,
         linePosition: this.position,
+        positionType,
+        ...this.diffFileCommentForm,
       });
 
       return this.saveDraft(postData)
         .then(() => {
-          this.handleClearForm(this.line.line_code);
+          if (positionType === IMAGE_DIFF_POSITION_TYPE) {
+            this.closeDiffFileCommentForm(this.diffFileHash);
+          } else {
+            this.handleClearForm(this.line.line_code);
+          }
         })
         .catch(() => {
           createFlash(s__('MergeRequests|An error occurred while saving the draft comment.'));

ee/app/assets/stylesheets/components/batch_comments/draft_note.scss

@@ -81,3 +81,10 @@ button[disabled] {
     }
   }
 }
+
+.frame,
+.diff-discussions {
+  .badge.is-draft {
+    background-color: $orange-600;
+  }
+}

ee/changelogs/unreleased/image-comment-reviews.yml

+---
+title: Fixed starting a review on images
+merge_request:
+author:
+type: fixed

ee/spec/features/merge_request/user_reviews_image_spec.rb

+require 'rails_helper'
+
+describe 'Merge request > image review', :js do
+  include MergeRequestDiffHelpers
+  include RepoHelpers
+
+  let(:user) { project.owner }
+  let(:project) { create(:project, :repository) }
+  let(:merge_request) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, author: user) }
+
+  before do
+    stub_licensed_features(batch_comments: true)
+
+    sign_in(user)
+
+    allow_any_instance_of(DiffHelper).to receive(:diff_file_blob_raw_url).and_return('/apple-touch-icon.png')
+    allow_any_instance_of(DiffHelper).to receive(:diff_file_old_blob_raw_url).and_return('/favicon.png')
+
+    visit diffs_project_merge_request_path(merge_request.project, merge_request)
+
+    wait_for_requests
+  end
+
+  it 'leaves review' do
+    find('.js-add-image-diff-note-button', match: :first).click
+
+    find('.diff-content .note-textarea').native.send_keys('image diff test comment')
+
+    click_button('Start a review')
+
+    wait_for_requests
+
+    page.within(find('.draft-note-component')) do
+      expect(page).to have_content('image diff test comment')
+    end
+  end
+end

ee/spec/frontend/batch_comments/stores/modules/batch_comments/getters_spec.js

+import * as getters from 'ee/batch_comments/stores/modules/batch_comments/getters';
+
+describe('Batch comments store getters', () => {
+  describe('draftsForFile', () => {
+    it('returns drafts for a file hash', () => {
+      const state = {
+        drafts: [
+          {
+            file_hash: 'filehash',
+            comment: 'testing 123',
+          },
+          {
+            file_hash: 'filehash2',
+            comment: 'testing 1234',
+          },
+        ],
+      };
+
+      expect(getters.draftsForFile(state)('filehash')).toEqual([
+        {
+          file_hash: 'filehash',
+          comment: 'testing 123',
+        },
+      ]);
+    });
+  });
+});

ee/spec/javascripts/batch_comments/components/diff_file_drafts_spec.js

+import { shallowMount, createLocalVue } from '@vue/test-utils';
+import Vuex from 'vuex';
+import DiffFileDrafts from 'ee/batch_comments/components/diff_file_drafts.vue';
+import DraftNote from 'ee/batch_comments/components/draft_note.vue';
+
+const localVue = createLocalVue();
+
+localVue.use(Vuex);
+
+describe('Batch comments diff file drafts component', () => {
+  let vm;
+
+  function factory() {
+    const store = new Vuex.Store({
+      modules: {
+        batchComments: {
+          namespaced: true,
+          getters: {
+            draftsForFile: () => () => [{ id: 1 }, { id: 2 }],
+          },
+        },
+      },
+    });
+
+    vm = shallowMount(DiffFileDrafts, { store, localVue, propsData: { fileHash: 'filehash' } });
+  }
+
+  afterEach(() => {
+    vm.destroy();
+  });
+
+  it('renders list of draft notes', () => {
+    factory();
+
+    expect(vm.findAll(DraftNote).length).toEqual(2);
+  });
+
+  it('renders index of draft note', () => {
+    factory();
+
+    expect(vm.findAll('.js-diff-notes-index').length).toEqual(2);
+
+    expect(
+      vm
+        .findAll('.js-diff-notes-index')
+        .at(0)
+        .text(),
+    ).toEqual('1');
+
+    expect(
+      vm
+        .findAll('.js-diff-notes-index')
+        .at(1)
+        .text(),
+    ).toEqual('2');
+  });
+});

ee/spec/javascripts/batch_comments/components/preview_item_spec.js

@@ -87,6 +87,16 @@ describe('Batch comments draft preview item component', () => {
 
       expect(vm.$el.querySelector('.bold').textContent).toContain(':2');
     });
+
+    it('renders image position', () => {
+      createComponent(false, {
+        file_path: 'index.js',
+        file_hash: 'abc',
+        position: { position_type: 'image', x: 10, y: 20 },
+      });
+
+      expect(vm.$el.querySelector('.bold').textContent).toContain('10x 20y');
+    });
   });
 
   describe('for discussion', () => {