Merge branch 'feature/file-review-code' into 'master'

Mark files as viewed

See merge request gitlab-org/gitlab!51513

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

@@ -26,6 +26,7 @@ import CollapsedFilesWarning from './collapsed_files_warning.vue';
 
 import { diffsApp } from '../utils/performance';
 import { fileByFile } from '../utils/preferences';
+import { reviewStatuses } from '../utils/file_reviews';
 
 import {
   TREE_LIST_WIDTH_STORAGE_KEY,
@@ -169,12 +170,7 @@ export default {
       'hasConflicts',
       'viewDiffsFileByFile',
     ]),
-    ...mapGetters('diffs', [
-      'whichCollapsedTypes',
-      'isParallelView',
-      'currentDiffIndex',
-      'fileReviews',
-    ]),
+    ...mapGetters('diffs', ['whichCollapsedTypes', 'isParallelView', 'currentDiffIndex']),
     ...mapGetters(['isNotesFetched', 'getNoteableData']),
     diffs() {
       if (!this.viewDiffsFileByFile) {
@@ -232,6 +228,9 @@ export default {
 
       return visible;
     },
+    fileReviews() {
+      return reviewStatuses(this.diffFiles, this.mrReviews);
+    },
   },
   watch: {
     commit(newCommit, oldCommit) {

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

@@ -10,7 +10,9 @@ import notesEventHub from '../../notes/event_hub';
 import DiffFileHeader from './diff_file_header.vue';
 import DiffContent from './diff_content.vue';
 import { diffViewerErrors } from '~/ide/constants';
+
 import { collapsedType, isCollapsed } from '../utils/diff_file';
+
 import {
   DIFF_FILE_AUTOMATIC_COLLAPSE,
   DIFF_FILE_MANUAL_COLLAPSE,
@@ -144,6 +146,12 @@ export default {
     showContent() {
       return !this.isCollapsed && !this.isFileTooLarge;
     },
+    showLocalFileReviews() {
+      const loggedIn = Boolean(gon.current_user_id);
+      const featureOn = this.glFeatures.localFileReviews;
+
+      return loggedIn && featureOn;
+    },
   },
   watch: {
     'file.file_hash': {
@@ -181,6 +189,10 @@ export default {
     if (this.hasDiff) {
       this.postRender();
     }
+
+    if (this.reviewed && !this.isCollapsed && this.showLocalFileReviews) {
+      this.handleToggle();
+    }
   },
   beforeDestroy() {
     eventHub.$off(EVT_EXPAND_ALL_FILES, this.expandAllListener);
@@ -273,9 +285,11 @@ export default {
       :can-current-user-fork="canCurrentUserFork"
       :diff-file="file"
       :collapsible="true"
+      :reviewed="reviewed"
       :expanded="!isCollapsed"
       :add-merge-request-buttons="true"
       :view-diffs-file-by-file="viewDiffsFileByFile"
+      :show-local-file-reviews="showLocalFileReviews"
       class="js-file-title file-title gl-border-1 gl-border-solid gl-border-gray-100"
       :class="hasBodyClasses.header"
       @toggleFile="handleToggle"

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

 <script>
 import { escape } from 'lodash';
-import { mapActions, mapGetters } from 'vuex';
+import { mapActions, mapGetters, mapState } from 'vuex';
 import {
   GlTooltipDirective,
   GlSafeHtmlDirective,
@@ -10,16 +10,23 @@ import {
   GlDropdown,
   GlDropdownItem,
   GlDropdownDivider,
+  GlFormCheckbox,
   GlLoadingIcon,
 } from '@gitlab/ui';
+import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
 import ClipboardButton from '~/vue_shared/components/clipboard_button.vue';
 import FileIcon from '~/vue_shared/components/file_icon.vue';
 import { truncateSha } from '~/lib/utils/text_utility';
 import { __, s__, sprintf } from '~/locale';
-import { diffViewerModes } from '~/ide/constants';
 import DiffStats from './diff_stats.vue';
 import { scrollToElement } from '~/lib/utils/common_utils';
-import { isCollapsed } from '../utils/diff_file';
+
+import { collapsedType, isCollapsed } from '../utils/diff_file';
+import { reviewable } from '../utils/file_reviews';
+
+import { diffViewerModes } from '~/ide/constants';
+import { DIFF_FILE_AUTOMATIC_COLLAPSE } from '../constants';
+
 import { DIFF_FILE_HEADER } from '../i18n';
 
 export default {
@@ -33,12 +40,14 @@ export default {
     GlDropdown,
     GlDropdownItem,
     GlDropdownDivider,
+    GlFormCheckbox,
     GlLoadingIcon,
   },
   directives: {
     GlTooltip: GlTooltipDirective,
     SafeHtml: GlSafeHtmlDirective,
   },
+  mixins: [glFeatureFlagsMixin()],
   i18n: {
     ...DIFF_FILE_HEADER,
   },
@@ -76,6 +85,16 @@ export default {
       required: false,
       default: false,
     },
+    showLocalFileReviews: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+    reviewed: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   data() {
     return {
@@ -83,6 +102,7 @@ export default {
     };
   },
   computed: {
+    ...mapState('diffs', ['latestDiff']),
     ...mapGetters('diffs', ['diffHasExpandedDiscussions', 'diffHasDiscussions']),
     diffContentIDSelector() {
       return `#diff-content-${this.diffFile.file_hash}`;
@@ -170,6 +190,9 @@ export default {
         (this.diffFile.edit_path || this.diffFile.ide_edit_path)
       );
     },
+    isReviewable() {
+      return reviewable(this.diffFile);
+    },
   },
   methods: {
     ...mapActions('diffs', [
@@ -177,6 +200,8 @@ export default {
       'toggleFileDiscussionWrappers',
       'toggleFullDiff',
       'toggleActiveFileByHash',
+      'reviewFile',
+      'setFileCollapsedByUser',
     ]),
     handleToggleFile() {
       this.$emit('toggleFile');
@@ -204,6 +229,26 @@ export default {
     setMoreActionsShown(val) {
       this.moreActionsShown = val;
     },
+    toggleReview(newReviewedStatus) {
+      const autoCollapsed =
+        this.isCollapsed && collapsedType(this.diffFile) === DIFF_FILE_AUTOMATIC_COLLAPSE;
+      const open = this.expanded;
+      const closed = !open;
+      const reviewed = newReviewedStatus;
+
+      this.reviewFile({ file: this.diffFile, reviewed });
+
+      if (reviewed && autoCollapsed) {
+        this.setFileCollapsedByUser({
+          filePath: this.diffFile.file_path,
+          collapsed: true,
+        });
+      }
+
+      if ((open && reviewed) || (closed && !reviewed)) {
+        this.$emit('toggleFile');
+      }
+    },
   },
 };
 </script>
@@ -291,6 +336,19 @@ export default {
       class="file-actions d-flex align-items-center gl-ml-auto gl-align-self-start"
     >
       <diff-stats :added-lines="diffFile.added_lines" :removed-lines="diffFile.removed_lines" />
+      <gl-form-checkbox
+        v-if="isReviewable && showLocalFileReviews"
+        v-gl-tooltip.hover
+        data-testid="fileReviewCheckbox"
+        class="gl-mb-0"
+        :title="$options.i18n.fileReviewTooltip"
+        :checked="reviewed"
+        @change="toggleReview"
+      >
+        <span class="gl-line-height-20">
+          {{ $options.i18n.fileReviewLabel }}
+        </span>
+      </gl-form-checkbox>
       <gl-button-group class="gl-pt-0!">
         <gl-button
           v-if="diffFile.external_url"

app/assets/javascripts/diffs/i18n.js

@@ -4,6 +4,8 @@ export const GENERIC_ERROR = __('Something went wrong on our end. Please try aga
 
 export const DIFF_FILE_HEADER = {
   optionsDropdownTitle: __('Options'),
+  fileReviewLabel: __('Viewed'),
+  fileReviewTooltip: __('Collapses this file (only for you) until it’s changed again.'),
 };
 
 export const DIFF_FILE = {

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

@@ -749,12 +749,10 @@ export const setFileByFile = ({ commit }, { fileByFile }) => {
   );
 };
 
-export function reviewFile({ commit, state, getters }, { file, reviewed = true }) {
+export function reviewFile({ commit, state }, { file, reviewed = true }) {
   const { mrPath } = getDerivedMergeRequestInformation({ endpoint: file.load_collapsed_diff_url });
-  const reviews = setReviewsForMergeRequest(
-    mrPath,
-    markFileReview(getters.fileReviews(state), file, reviewed),
-  );
+  const reviews = markFileReview(state.mrReviews, file, reviewed);
 
+  setReviewsForMergeRequest(mrPath, reviews);
   commit(types.SET_MR_FILE_REVIEWS, reviews);
 }

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

 import { __, n__ } from '~/locale';
 import { parallelizeDiffLines } from './utils';
-import { isFileReviewed } from '../utils/file_reviews';
 import {
   PARALLEL_DIFF_VIEW_TYPE,
   INLINE_DIFF_VIEW_TYPE,
@@ -155,7 +154,3 @@ export const diffLines = (state) => (file, unifiedDiffComponents) => {
     state.diffViewType === INLINE_DIFF_VIEW_TYPE,
   );
 };
-
-export function fileReviews(state) {
-  return state.diffFiles.map((file) => isFileReviewed(state.mrReviews, file));
-}

app/assets/javascripts/diffs/store/modules/diff_state.js

@@ -47,4 +47,5 @@ export default () => ({
   showSuggestPopover: true,
   defaultSuggestionCommitMessage: '',
   mrReviews: {},
+  latestDiff: true,
 });

app/assets/javascripts/diffs/utils/file_reviews.js

@@ -2,6 +2,16 @@ function getFileReviewsKey(mrPath) {
   return `${mrPath}-file-reviews`;
 }
 
+export function isFileReviewed(reviews, file) {
+  const fileReviews = reviews[file.file_identifier_hash];
+
+  return file?.id && fileReviews?.length ? new Set(fileReviews).has(file.id) : false;
+}
+
+export function reviewStatuses(files, reviews) {
+  return files.map((file) => isFileReviewed(reviews, file));
+}
+
 export function getReviewsForMergeRequest(mrPath) {
   const reviewsForMr = localStorage.getItem(getFileReviewsKey(mrPath));
   let reviews = {};
@@ -23,23 +33,17 @@ export function setReviewsForMergeRequest(mrPath, reviews) {
   return reviews;
 }
 
-export function isFileReviewed(reviews, file) {
-  const fileReviews = reviews[file.file_identifier_hash];
-
-  return file?.id && fileReviews?.length ? new Set(fileReviews).has(file.id) : false;
-}
-
 export function reviewable(file) {
   return Boolean(file.id) && Boolean(file.file_identifier_hash);
 }
 
 export function markFileReview(reviews, file, reviewed = true) {
   const usableReviews = { ...(reviews || {}) };
-  let updatedReviews = usableReviews;
+  const updatedReviews = usableReviews;
   let fileReviews;
 
   if (reviewable(file)) {
-    fileReviews = new Set([...(usableReviews[file.file_identifier_hash] || [])]);
+    fileReviews = new Set(usableReviews[file.file_identifier_hash] || []);
 
     if (reviewed) {
       fileReviews.add(file.id);
@@ -47,10 +51,7 @@ export function markFileReview(reviews, file, reviewed = true) {
       fileReviews.delete(file.id);
     }
 
-    updatedReviews = {
-      ...usableReviews,
-      [file.file_identifier_hash]: Array.from(fileReviews),
-    };
+    updatedReviews[file.file_identifier_hash] = Array.from(fileReviews);
 
     if (updatedReviews[file.file_identifier_hash].length === 0) {
       delete updatedReviews[file.file_identifier_hash];

app/controllers/projects/merge_requests_controller.rb

@@ -42,6 +42,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     push_frontend_feature_flag(:diffs_gradual_load, @project, default_enabled: true)
     push_frontend_feature_flag(:codequality_mr_diff, @project)
     push_frontend_feature_flag(:suggestions_custom_commit, @project)
+    push_frontend_feature_flag(:local_file_reviews, default_enabled: :yaml)
 
     record_experiment_user(:invite_members_version_a)
     record_experiment_user(:invite_members_version_b)

changelogs/unreleased/feature-file-review.yml

+---
+title: Mark files as reviewed locally
+merge_request: 51513
+author:
+type: added

config/feature_flags/development/local_file_reviews.yml

+---
+name: local_file_reviews
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48976
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/296674
+milestone: '13.8'
+type: development
+group: group::code review
+default_enabled: false

locale/gitlab.pot

@@ -7081,6 +7081,9 @@ msgstr ""
 msgid "Collapse sidebar"
 msgstr ""
 
+msgid "Collapses this file (only for you) until it’s changed again."
+msgstr ""
+
 msgid "Collector hostname"
 msgstr ""
 
@@ -31401,6 +31404,9 @@ msgstr ""
 msgid "View users statistics"
 msgstr ""
 
+msgid "Viewed"
+msgstr ""
+
 msgid "Viewing commit"
 msgstr ""
 

spec/frontend/diffs/components/diff_file_spec.js

@@ -66,7 +66,7 @@ function markFileToBeRendered(store, index = 0) {
   });
 }
 
-function createComponent({ file, first = false, last = false }) {
+function createComponent({ file, first = false, last = false, options = {}, props = {} }) {
   const localVue = createLocalVue();
 
   localVue.use(Vuex);
@@ -89,7 +89,9 @@ function createComponent({ file, first = false, last = false }) {
       viewDiffsFileByFile: false,
       isFirstFile: first,
       isLastFile: last,
+      ...props,
     },
+    ...options,
   });
 
   return {
@@ -220,6 +222,53 @@ describe('DiffFile', () => {
     });
   });
 
+  describe('computed', () => {
+    describe('showLocalFileReviews', () => {
+      let gon;
+
+      function setLoggedIn(bool) {
+        window.gon.current_user_id = bool;
+      }
+
+      beforeAll(() => {
+        gon = window.gon;
+        window.gon = {};
+      });
+
+      afterEach(() => {
+        window.gon = gon;
+      });
+
+      it.each`
+        loggedIn | featureOn | bool
+        ${true}  | ${true}   | ${true}
+        ${false} | ${true}   | ${false}
+        ${true}  | ${false}  | ${false}
+        ${false} | ${false}  | ${false}
+      `(
+        'should be $bool when { userIsLoggedIn: $loggedIn, featureEnabled: $featureOn }',
+        ({ loggedIn, featureOn, bool }) => {
+          setLoggedIn(loggedIn);
+
+          ({ wrapper } = createComponent({
+            options: {
+              provide: {
+                glFeatures: {
+                  localFileReviews: featureOn,
+                },
+              },
+            },
+            props: {
+              file: store.state.diffs.diffFiles[0],
+            },
+          }));
+
+          expect(wrapper.vm.showLocalFileReviews).toBe(bool);
+        },
+      );
+    });
+  });
+
   describe('collapsing', () => {
     describe(`\`${EVT_EXPAND_ALL_FILES}\` event`, () => {
       beforeEach(() => {

spec/frontend/diffs/store/getters_spec.js

@@ -375,26 +375,4 @@ describe('Diffs Module Getters', () => {
       });
     });
   });
-
-  describe('fileReviews', () => {
-    const file1 = { id: '123', file_identifier_hash: 'abc' };
-    const file2 = { id: '098', file_identifier_hash: 'abc' };
-
-    it.each`
-      reviews                           | files             | fileReviews
-      ${{}}                             | ${[file1, file2]} | ${[false, false]}
-      ${{ abc: ['123'] }}               | ${[file1, file2]} | ${[true, false]}
-      ${{ abc: ['098'] }}               | ${[file1, file2]} | ${[false, true]}
-      ${{ def: ['123'] }}               | ${[file1, file2]} | ${[false, false]}
-      ${{ abc: ['123'], def: ['098'] }} | ${[]}             | ${[]}
-    `(
-      'returns $fileReviews based on the diff files in state and the existing reviews $reviews',
-      ({ reviews, files, fileReviews }) => {
-        localState.diffFiles = files;
-        localState.mrReviews = reviews;
-
-        expect(getters.fileReviews(localState)).toStrictEqual(fileReviews);
-      },
-    );
-  });
 });

spec/frontend/diffs/utils/file_reviews_spec.js

@@ -5,6 +5,7 @@ import {
   setReviewsForMergeRequest,
   isFileReviewed,
   markFileReview,
+  reviewStatuses,
   reviewable,
 } from '~/diffs/utils/file_reviews';
 
@@ -28,6 +29,39 @@ describe('File Review(s) utilities', () => {
     localStorage.clear();
   });
 
+  describe('isFileReviewed', () => {
+    it.each`
+      description                            | diffFile                      | fileReviews
+      ${'the file does not have an `id`'}    | ${{ ...file, id: undefined }} | ${getDefaultReviews()}
+      ${'there are no reviews for the file'} | ${file}                       | ${{ ...getDefaultReviews(), abc: undefined }}
+    `('returns `false` if $description', ({ diffFile, fileReviews }) => {
+      expect(isFileReviewed(fileReviews, diffFile)).toBe(false);
+    });
+
+    it("returns `true` for a file if it's available in the provided reviews", () => {
+      expect(isFileReviewed(reviews, file)).toBe(true);
+    });
+  });
+
+  describe('reviewStatuses', () => {
+    const file1 = { id: '123', file_identifier_hash: 'abc' };
+    const file2 = { id: '098', file_identifier_hash: 'abc' };
+
+    it.each`
+      mrReviews                         | files             | fileReviews
+      ${{}}                             | ${[file1, file2]} | ${[false, false]}
+      ${{ abc: ['123'] }}               | ${[file1, file2]} | ${[true, false]}
+      ${{ abc: ['098'] }}               | ${[file1, file2]} | ${[false, true]}
+      ${{ def: ['123'] }}               | ${[file1, file2]} | ${[false, false]}
+      ${{ abc: ['123'], def: ['098'] }} | ${[]}             | ${[]}
+    `(
+      'returns $fileReviews based on the diff files in state and the existing reviews $reviews',
+      ({ mrReviews, files, fileReviews }) => {
+        expect(reviewStatuses(files, mrReviews)).toStrictEqual(fileReviews);
+      },
+    );
+  });
+
   describe('getReviewsForMergeRequest', () => {
     it('fetches the appropriate stored reviews from localStorage', () => {
       getReviewsForMergeRequest(mrPath);
@@ -73,20 +107,6 @@ describe('File Review(s) utilities', () => {
     });
   });
 
-  describe('isFileReviewed', () => {
-    it.each`
-      description                            | diffFile                      | fileReviews
-      ${'the file does not have an `id`'}    | ${{ ...file, id: undefined }} | ${getDefaultReviews()}
-      ${'there are no reviews for the file'} | ${file}                       | ${{ ...getDefaultReviews(), abc: undefined }}
-    `('returns `false` if $description', ({ diffFile, fileReviews }) => {
-      expect(isFileReviewed(fileReviews, diffFile)).toBe(false);
-    });
-
-    it("returns `true` for a file if it's available in the provided reviews", () => {
-      expect(isFileReviewed(reviews, file)).toBe(true);
-    });
-  });
-
   describe('reviewable', () => {
     it.each`
       response | diffFile                                        | description