Merge branch 'ph/209784/usePaginatedNotesEndpoint' into 'master'

Updated diffs notes app to use paginated notes

See merge request gitlab-org/gitlab!52089

app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js

@@ -67,13 +67,23 @@ export const publishReview = ({ commit, dispatch, getters }) => {
     .catch(() => commit(types.RECEIVE_PUBLISH_REVIEW_ERROR));
 };
 
-export const updateDiscussionsAfterPublish = ({ dispatch, getters, rootGetters }) =>
-  dispatch('fetchDiscussions', { path: getters.getNotesData.discussionsPath }, { root: true }).then(
-    () =>
-      dispatch('diffs/assignDiscussionsToDiff', rootGetters.discussionsStructuredByLineCode, {
-        root: true,
-      }),
-  );
+export const updateDiscussionsAfterPublish = async ({ dispatch, getters, rootGetters }) => {
+  if (window.gon?.features?.paginatedNotes) {
+    await dispatch('stopPolling', null, { root: true });
+    await dispatch('fetchData', null, { root: true });
+    await dispatch('restartPolling', null, { root: true });
+  } else {
+    await dispatch(
+      'fetchDiscussions',
+      { path: getters.getNotesData.discussionsPath },
+      { root: true },
+    );
+  }
+
+  dispatch('diffs/assignDiscussionsToDiff', rootGetters.discussionsStructuredByLineCode, {
+    root: true,
+  });
+};
 
 export const updateDraft = (
   { commit, getters },

app/assets/javascripts/notes/components/notes_app.vue

@@ -4,6 +4,7 @@ import OrderedLayout from '~/vue_shared/components/ordered_layout.vue';
 import highlightCurrentUser from '~/behaviors/markdown/highlight_current_user';
 import { __ } from '~/locale';
 import initUserPopovers from '~/user_popovers';
+import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
 import { getLocationHash, doesHashExistInUrl } from '../../lib/utils/url_utility';
 import { deprecatedCreateFlash as Flash } from '../../flash';
 import * as constants from '../constants';
@@ -30,6 +31,7 @@ export default {
     discussionFilterNote,
     OrderedLayout,
   },
+  mixins: [glFeatureFlagsMixin()],
   props: {
     noteableData: {
       type: Object,
@@ -57,7 +59,6 @@ export default {
   },
   data() {
     return {
-      isFetching: false,
       currentFilter: null,
     };
   },
@@ -68,6 +69,7 @@ export default {
       'convertedDisscussionIds',
       'getNotesDataByProp',
       'isLoading',
+      'isFetching',
       'commentsDisabled',
       'getNoteableData',
       'userCanReply',
@@ -103,6 +105,13 @@ export default {
     },
   },
   watch: {
+    async isFetching() {
+      if (!this.isFetching) {
+        await this.$nextTick();
+        await this.startTaskList();
+        await this.checkLocationHash();
+      }
+    },
     shouldShow() {
       if (!this.isNotesFetched) {
         this.fetchNotes();
@@ -153,6 +162,7 @@ export default {
   },
   methods: {
     ...mapActions([
+      'setFetchingState',
       'setLoadingState',
       'fetchDiscussions',
       'poll',
@@ -183,7 +193,11 @@ export default {
     fetchNotes() {
       if (this.isFetching) return null;
 
-      this.isFetching = true;
+      this.setFetchingState(true);
+
+      if (this.glFeatures.paginatedNotes) {
+        return this.initPolling();
+      }
 
       return this.fetchDiscussions(this.getFetchDiscussionsConfig())
         .then(this.initPolling)
@@ -191,11 +205,8 @@ export default {
           this.setLoadingState(false);
           this.setNotesFetchedState(true);
           eventHub.$emit('fetchedNotesData');
-          this.isFetching = false;
+          this.setFetchingState(false);
         })
-        .then(this.$nextTick)
-        .then(this.startTaskList)
-        .then(this.checkLocationHash)
         .catch(() => {
           this.setLoadingState(false);
           this.setNotesFetchedState(true);

app/assets/javascripts/notes/stores/actions.js

@@ -15,6 +15,7 @@ import loadAwardsHandler from '../../awards_handler';
 import sidebarTimeTrackingEventHub from '../../sidebar/event_hub';
 import { isInViewport, scrollToElement, isInMRPage } from '../../lib/utils/common_utils';
 import { mergeUrlParams } from '../../lib/utils/url_utility';
+import eventHub from '../event_hub';
 import mrWidgetEventHub from '../../vue_merge_request_widget/event_hub';
 import * as utils from './utils';
 import * as types from './mutation_types';
@@ -420,14 +421,25 @@ export const saveNote = ({ commit, dispatch }, noteData) => {
     .catch(processErrors);
 };
 
-const pollSuccessCallBack = (resp, commit, state, getters, dispatch) => {
+export const setFetchingState = ({ commit }, fetchingState) =>
+  commit(types.SET_NOTES_FETCHING_STATE, fetchingState);
+
+const pollSuccessCallBack = async (resp, commit, state, getters, dispatch) => {
   if (state.isResolvingDiscussion) {
     return null;
   }
 
+  if (window.gon?.features?.paginatedNotes && !resp.more && state.isFetching) {
+    eventHub.$emit('fetchedNotesData');
+    dispatch('setFetchingState', false);
+    dispatch('setNotesFetchedState', true);
+    dispatch('setLoadingState', false);
+  }
+
   if (resp.notes?.length) {
-    dispatch('updateOrCreateNotes', resp.notes);
+    await dispatch('updateOrCreateNotes', resp.notes);
     dispatch('startTaskList');
+    dispatch('updateResolvableDiscussionsCounts');
   }
 
   commit(types.SET_LAST_FETCHED_AT, resp.last_fetched_at);

app/assets/javascripts/notes/stores/getters.js

@@ -48,6 +48,8 @@ export const persistSortOrder = (state) => state.persistSortOrder;
 
 export const timelineEnabled = (state) => state.isTimelineEnabled;
 
+export const isFetching = (state) => state.isFetching;
+
 export const isLoading = (state) => state.isLoading;
 
 export const getNotesDataByProp = (state) => (prop) => state.notesData[prop];

app/assets/javascripts/notes/stores/modules/index.js

@@ -47,6 +47,7 @@ export default () => ({
     unresolvedDiscussionsCount: 0,
     descriptionVersions: {},
     isTimelineEnabled: false,
+    isFetching: false,
   },
   actions,
   getters,

app/assets/javascripts/notes/stores/mutation_types.js

@@ -14,6 +14,7 @@ export const UPDATE_NOTE = 'UPDATE_NOTE';
 export const UPDATE_DISCUSSION = 'UPDATE_DISCUSSION';
 export const UPDATE_DISCUSSION_POSITION = 'UPDATE_DISCUSSION_POSITION';
 export const SET_DISCUSSION_DIFF_LINES = 'SET_DISCUSSION_DIFF_LINES';
+export const SET_NOTES_FETCHING_STATE = 'SET_NOTES_FETCHING_STATE';
 export const SET_NOTES_FETCHED_STATE = 'SET_NOTES_FETCHED_STATE';
 export const SET_NOTES_LOADING_STATE = 'SET_NOTES_LOADING_STATE';
 export const DISABLE_COMMENTS = 'DISABLE_COMMENTS';

app/assets/javascripts/notes/stores/mutations.js

@@ -32,6 +32,20 @@ export default {
         }
       }
 
+      if (window.gon?.features?.paginatedNotes && note.base_discussion) {
+        if (discussion.diff_file) {
+          discussion.file_hash = discussion.diff_file.file_hash;
+
+          discussion.truncated_diff_lines = utils.prepareDiffLines(
+            discussion.truncated_diff_lines || [],
+          );
+        }
+
+        discussion.resolvable = note.resolvable;
+        discussion.expanded = note.base_discussion.expanded;
+        discussion.resolved = note.resolved;
+      }
+
       // note.base_discussion = undefined; // No point keeping a reference to this
       delete note.base_discussion;
       discussion.notes = [note];
@@ -323,6 +337,10 @@ export default {
     state.isLoading = value;
   },
 
+  [types.SET_NOTES_FETCHING_STATE](state, value) {
+    state.isFetching = value;
+  },
+
   [types.SET_DISCUSSION_DIFF_LINES](state, { discussionId, diffLines }) {
     const discussion = utils.findNoteObjectById(state.discussions, discussionId);
 

app/controllers/projects/merge_requests_controller.rb

@@ -44,6 +44,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     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)
+    push_frontend_feature_flag(:paginated_notes, @project, default_enabled: :yaml)
 
     record_experiment_user(:invite_members_version_a)
     record_experiment_user(:invite_members_version_b)

app/helpers/notes_helper.rb

@@ -175,7 +175,9 @@ module NotesHelper
     end
   end
 
-  def notes_data(issuable)
+  def notes_data(issuable, start_at_zero = false)
+    initial_last_fetched_at = start_at_zero ? 0 : Time.current.to_i * ::Gitlab::UpdatedNotesPaginator::MICROSECOND
+
     data = {
       discussionsPath: discussions_path(issuable),
       registerPath: new_session_path(:user, redirect_to_referer: 'yes', anchor: 'register-pane'),
@@ -186,7 +188,7 @@ module NotesHelper
       reopenPath: reopen_issuable_path(issuable),
       notesPath: notes_url,
       prerenderedNotesCount: issuable.capped_notes_count(MAX_PRERENDERED_NOTES),
-      lastFetchedAt: Time.now.to_i * ::Gitlab::UpdatedNotesPaginator::MICROSECOND
+      lastFetchedAt: initial_last_fetched_at
     }
 
     if issuable.is_a?(MergeRequest)

app/serializers/base_discussion_entity.rb

@@ -15,6 +15,7 @@ class BaseDiscussionEntity < Grape::Entity
   expose :for_commit?, as: :for_commit
   expose :individual_note?, as: :individual_note
   expose :resolvable?, as: :resolvable
+  expose :resolved_by_push?, as: :resolved_by_push
 
   expose :truncated_diff_lines, using: DiffLineEntity, if: -> (d, _) { d.diff_discussion? && d.on_text? && (d.expanded? || render_truncated_diff_lines?) }
 

app/views/projects/merge_requests/show.html.haml

@@ -56,10 +56,13 @@
               = render "projects/merge_requests/widget"
               = render "projects/merge_requests/awards_block"
               - if mr_action === "show"
-                - add_page_startup_api_call discussions_path(@merge_request)
+                - if Feature.enabled?(:paginated_notes, @project)
+                  - add_page_startup_api_call notes_url
+                - else
+                  - add_page_startup_api_call discussions_path(@merge_request)
                 - add_page_startup_api_call widget_project_json_merge_request_path(@project, @merge_request, format: :json)
                 - add_page_startup_api_call cached_widget_project_json_merge_request_path(@project, @merge_request, format: :json)
-              #js-vue-mr-discussions{ data: { notes_data: notes_data(@merge_request).to_json,
+              #js-vue-mr-discussions{ data: { notes_data: notes_data(@merge_request, Feature.enabled?(:paginated_notes, @project)).to_json,
                 noteable_data: serialize_issuable(@merge_request, serializer: 'noteable'),
                 noteable_type: 'MergeRequest',
                 target_type: 'merge_request',

spec/frontend/notes/stores/actions_spec.js

@@ -291,9 +291,45 @@ describe('Actions Notes Store', () => {
           [
             { type: 'updateOrCreateNotes', payload: discussionMock.notes },
             { type: 'startTaskList' },
+            { type: 'updateResolvableDiscussionsCounts' },
           ],
         ));
     });
+
+    describe('paginated notes feature flag enabled', () => {
+      const lastFetchedAt = '12358';
+
+      beforeEach(() => {
+        window.gon = { features: { paginatedNotes: true } };
+
+        axiosMock.onGet(notesDataMock.notesPath).replyOnce(200, {
+          notes: discussionMock.notes,
+          more: false,
+          last_fetched_at: lastFetchedAt,
+        });
+      });
+
+      afterEach(() => {
+        window.gon = null;
+      });
+
+      it('should dispatch setFetchingState, setNotesFetchedState, setLoadingState, updateOrCreateNotes, startTaskList and commit SET_LAST_FETCHED_AT', () => {
+        return testAction(
+          actions.fetchData,
+          null,
+          { notesData: notesDataMock, isFetching: true },
+          [{ type: 'SET_LAST_FETCHED_AT', payload: lastFetchedAt }],
+          [
+            { type: 'setFetchingState', payload: false },
+            { type: 'setNotesFetchedState', payload: true },
+            { type: 'setLoadingState', payload: false },
+            { type: 'updateOrCreateNotes', payload: discussionMock.notes },
+            { type: 'startTaskList' },
+            { type: 'updateResolvableDiscussionsCounts' },
+          ],
+        );
+      });
+    });
   });
 
   describe('poll', () => {
@@ -1355,4 +1391,17 @@ describe('Actions Notes Store', () => {
       );
     });
   });
+
+  describe('setFetchingState', () => {
+    it('commits SET_NOTES_FETCHING_STATE', (done) => {
+      testAction(
+        actions.setFetchingState,
+        true,
+        null,
+        [{ type: mutationTypes.SET_NOTES_FETCHING_STATE, payload: true }],
+        [],
+        done,
+      );
+    });
+  });
 });

spec/helpers/notes_helper_spec.rb

@@ -316,4 +316,15 @@ RSpec.describe NotesHelper do
       end
     end
   end
+
+  describe '#notes_data' do
+    let(:issue) { create(:issue, project: project) }
+
+    it 'sets last_fetched_at to 0 when start_at_zero is true' do
+      @project = project
+      @noteable = issue
+
+      expect(helper.notes_data(issue, true)[:lastFetchedAt]).to eq(0)
+    end
+  end
 end