Merge branch 'id-etag-caching-for-mr-widget-ee' into 'master'

Send only necessary fields on mr-widget auto-refresh

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

app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js

@@ -87,9 +87,6 @@ export default class MergeRequestStore {
     this.allowCollaboration = data.allow_collaboration;
     this.sourceProjectId = data.source_project_id;
     this.targetProjectId = data.target_project_id;
-    this.mergePipelinesEnabled = Boolean(data.merge_pipelines_enabled);
-    this.mergeTrainsCount = data.merge_trains_count || 0;
-    this.mergeTrainIndex = data.merge_train_index;
 
     // CI related
     this.hasCI = data.has_ci;

app/serializers/merge_request_poll_widget_entity.rb

@@ -141,4 +141,4 @@ class MergeRequestPollWidgetEntity < IssuableEntity
   end
 end
 
-MergeRequestPollWidgetEntity.prepend_if_ee('EE::MergeRequestWidgetEntity')
+MergeRequestPollWidgetEntity.prepend_if_ee('EE::MergeRequestPollWidgetEntity')

app/serializers/merge_request_widget_entity.rb

@@ -84,8 +84,12 @@ class MergeRequestWidgetEntity < Grape::Entity
 
   private
 
+  delegate :current_user, to: :request
+
   def presenter(merge_request)
     @presenters ||= {}
-    @presenters[merge_request] ||= MergeRequestPresenter.new(merge_request, current_user: request.current_user) # rubocop: disable CodeReuse/Presenter
+    @presenters[merge_request] ||= MergeRequestPresenter.new(merge_request, current_user: current_user) # rubocop: disable CodeReuse/Presenter
   end
 end
+
+MergeRequestWidgetEntity.prepend_if_ee('EE::MergeRequestWidgetEntity')

ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js

@@ -39,11 +39,21 @@ export default class MergeRequestStore extends CEMergeRequestStore {
     this.metricsReportsPath = data.metrics_reports_path;
 
     this.blockingMergeRequests = data.blocking_merge_requests;
+
+    this.hasApprovalsAvailable = data.has_approvals_available;
+    this.apiApprovalsPath = data.api_approvals_path;
+    this.apiApprovalSettingsPath = data.api_approval_settings_path;
+    this.apiApprovePath = data.api_approve_path;
+    this.apiUnapprovePath = data.api_unapprove_path;
   }
 
   setData(data, isRebased) {
     this.initGeo(data);
-    this.initApprovals(data);
+    this.initApprovals();
+
+    this.mergePipelinesEnabled = Boolean(data.merge_pipelines_enabled);
+    this.mergeTrainsCount = data.merge_trains_count || 0;
+    this.mergeTrainIndex = data.merge_train_index;
 
     super.setData(data, isRebased);
   }
@@ -53,17 +63,10 @@ export default class MergeRequestStore extends CEMergeRequestStore {
     this.geoSecondaryHelpPath = this.geoSecondaryHelpPath || data.geo_secondary_help_path;
   }
 
-  initApprovals(data) {
+  initApprovals() {
     this.isApproved = this.isApproved || false;
     this.approvals = this.approvals || null;
     this.approvalRules = this.approvalRules || [];
-    this.hasApprovalsAvailable = Boolean(
-      data.has_approvals_available || this.hasApprovalsAvailable,
-    );
-    this.apiApprovalsPath = data.api_approvals_path || this.apiApprovalsPath;
-    this.apiApprovalSettingsPath = data.api_approval_settings_path || this.apiApprovalSettingsPath;
-    this.apiApprovePath = data.api_approve_path || this.apiApprovePath;
-    this.apiUnapprovePath = data.api_unapprove_path || this.apiUnapprovePath;
   }
 
   setApprovals(data) {

ee/app/presenters/ee/merge_request_presenter.rb

@@ -57,10 +57,6 @@ module EE
       ::ApproverGroup.filtered_approver_groups(merge_request.approver_groups, current_user)
     end
 
-    def vulnerability_feedback_path
-      project_vulnerability_feedback_index_path(merge_request.project)
-    end
-
     private
 
     def expose_mr_approval_path?

ee/app/serializers/ee/merge_request_poll_widget_entity.rb

+# frozen_string_literal: true
+
+module EE
+  module MergeRequestPollWidgetEntity
+    include ::API::Helpers::RelatedResourcesHelpers
+    extend ActiveSupport::Concern
+
+    prepended do
+      expose :merge_pipelines_enabled?, as: :merge_pipelines_enabled do |merge_request|
+        merge_request.target_project.merge_pipelines_enabled?
+      end
+
+      expose :merge_trains_count, if: -> (merge_request) { merge_request.target_project.merge_trains_enabled? } do |merge_request|
+        MergeTrain.total_count_in_train(merge_request)
+      end
+
+      expose :merge_train_index, if: -> (merge_request) { merge_request.on_train? } do |merge_request|
+        merge_request.merge_train.index
+      end
+    end
+  end
+end

ee/app/serializers/ee/merge_request_widget_entity.rb

@@ -6,8 +6,6 @@ module EE
     extend ActiveSupport::Concern
 
     prepended do
-      expose :approvals_required, as: :approvals_before_merge
-
       expose :blob_path do
         expose :head_path, if: -> (mr, _) { mr.head_pipeline_sha } do |merge_request|
           project_blob_path(merge_request.project, merge_request.head_pipeline_sha)
@@ -113,7 +111,7 @@ module EE
       end
 
       expose :vulnerability_feedback_path do |merge_request|
-        presenter(merge_request).vulnerability_feedback_path
+        project_vulnerability_feedback_index_path(merge_request.project)
       end
 
       expose :create_vulnerability_feedback_issue_path do |merge_request|
@@ -128,39 +126,26 @@ module EE
         presenter(merge_request).create_vulnerability_feedback_dismissal_path(merge_request.project)
       end
 
-      expose :merge_pipelines_enabled?, as: :merge_pipelines_enabled do |merge_request|
-        merge_request.target_project.merge_pipelines_enabled?
-      end
-
-      expose :merge_trains_count, if: -> (*) { merge_trains_enabled? } do |merge_request|
-        MergeTrain.total_count_in_train(merge_request)
-      end
-
-      expose :merge_train_index, if: -> (merge_request) { merge_request.on_train? } do |merge_request|
-        merge_request.merge_train.index
-      end
-
       expose :has_approvals_available do |merge_request|
         merge_request.approval_feature_available?
       end
-      expose :rebase_path do |merge_request|
-        presenter(merge_request).rebase_path
-      end
-      expose :approvals_path do |merge_request|
-        presenter(merge_request).approvals_path
-      end
+
       expose :api_approvals_path do |merge_request|
         presenter(merge_request).api_approvals_path
       end
+
       expose :api_approval_settings_path do |merge_request|
         presenter(merge_request).api_approval_settings_path
       end
+
       expose :api_approve_path do |merge_request|
         presenter(merge_request).api_approve_path
       end
+
       expose :api_unapprove_path do |merge_request|
         presenter(merge_request).api_unapprove_path
       end
+
       expose :merge_train_when_pipeline_succeeds_docs_path do |merge_request|
         presenter(merge_request).merge_train_when_pipeline_succeeds_docs_path
       end
@@ -199,9 +184,5 @@ module EE
       object.base_pipeline&.present(current_user: current_user)
         &.downloadable_path_for_report_type(file_type)
     end
-
-    def merge_trains_enabled?
-      object.target_project.merge_trains_enabled?
-    end
   end
 end

ee/changelogs/unreleased/id-etag-caching-for-mr-widget-ee.yml

+---
+title: Send only necessary fields on mr-widget auto-refresh
+merge_request: 15495
+author:
+type: performance

ee/spec/fixtures/api/schemas/entities/merge_request_widget.json

@@ -4,22 +4,19 @@
     { "$ref": "../../../../../../spec/fixtures/api/schemas/entities/merge_request_widget.json" },
     {
       "properties": {
-        "has_approvals_available": { "type": ["boolean"] },
-        "approvals_before_merge": { "type": ["integer", "null"] },
-        "approved": { "type": "boolean" },
-        "approvals_path": { "type": ["string", "null"] },
-        "api_approvals_path": { "type": ["string", "null"] },
-        "api_approval_settings_path": { "type": ["string", "null"] },
-        "api_approve_path": { "type": ["string", "null"] },
-        "api_unapprove_path": { "type": ["string", "null"] },
-        "codeclimate": {
+        "blob_path": {
           "head_path": { "type": "string" },
           "base_path": { "type": "string" }
         },
-        "blob_path": {
+        "codeclimate": {
           "head_path": { "type": "string" },
           "base_path": { "type": "string" }
         },
+        "has_approvals_available": { "type": ["boolean"] },
+        "api_approvals_path": { "type": ["string", "null"] },
+        "api_approval_settings_path": { "type": ["string", "null"] },
+        "api_approve_path": { "type": ["string", "null"] },
+        "api_unapprove_path": { "type": ["string", "null"] },
         "vulnerability_feedback_path": { "type": "string" },
         "create_vulnerability_feedback_issue_path": { "type": ["string", "null"] },
         "create_vulnerability_feedback_merge_request_path": { "type": ["string", "null"] },

ee/spec/javascripts/vue_mr_widget/stores/mr_widget_store_spec.js

@@ -60,4 +60,48 @@ describe('MergeRequestStore', () => {
       expect(store.isNothingToMergeState).toEqual(false);
     });
   });
+
+  describe('setData', () => {
+    describe('mergePipelinesEnabled', () => {
+      it('should set mergePipelinesEnabled = true when merge_pipelines_enabled is true', () => {
+        store.setData({ ...mockData, merge_pipelines_enabled: true });
+
+        expect(store.mergePipelinesEnabled).toBe(true);
+      });
+
+      it('should set mergePipelinesEnabled = false when merge_pipelines_enabled is not provided', () => {
+        store.setData({ ...mockData, merge_pipelines_enabled: undefined });
+
+        expect(store.mergePipelinesEnabled).toBe(false);
+      });
+    });
+
+    describe('mergeTrainsCount', () => {
+      it('should set mergeTrainsCount when merge_trains_count is provided', () => {
+        store.setData({ ...mockData, merge_trains_count: 3 });
+
+        expect(store.mergeTrainsCount).toBe(3);
+      });
+
+      it('should set mergeTrainsCount = 0 when merge_trains_count is not provided', () => {
+        store.setData({ ...mockData, merge_trains_count: undefined });
+
+        expect(store.mergeTrainsCount).toBe(0);
+      });
+    });
+
+    describe('mergeTrainIndex', () => {
+      it('should set mergeTrainIndex when merge_train_index is provided', () => {
+        store.setData({ ...mockData, merge_train_index: 3 });
+
+        expect(store.mergeTrainIndex).toBe(3);
+      });
+
+      it('should not set mergeTrainIndex when merge_train_index is not provided', () => {
+        store.setData({ ...mockData, merge_train_index: undefined });
+
+        expect(store.mergeTrainIndex).toBeUndefined();
+      });
+    });
+  });
 });

ee/spec/presenters/merge_request_presenter_spec.rb

@@ -119,12 +119,6 @@ describe MergeRequestPresenter do
     end
   end
 
-  describe '#vulnerability_feedback_path' do
-    subject { described_class.new(merge_request, current_user: user).vulnerability_feedback_path }
-
-    it { is_expected.to eq("/#{merge_request.project.full_path}/vulnerability_feedback") }
-  end
-
   describe 'create vulnerability feedback paths' do
     where(:create_feedback_path) do
       [

ee/spec/serializers/merge_request_widget_entity_spec.rb

@@ -29,10 +29,6 @@ describe MergeRequestWidgetEntity do
     expect(subject.as_json[:blob_path]).to include(:head_path)
   end
 
-  it 'sets approvals_before_merge to 0 if nil' do
-    expect(subject.as_json[:approvals_before_merge]).to eq(0)
-  end
-
   def create_all_artifacts
     artifacts = %i(codequality sast dependency_scanning container_scanning dast license_management performance)
 
@@ -186,7 +182,9 @@ describe MergeRequestWidgetEntity do
   end
 
   it 'has vulnerability feedback paths' do
-    expect(subject.as_json).to include(:vulnerability_feedback_path)
+    expect(subject.as_json[:vulnerability_feedback_path]).to eq(
+      "/#{merge_request.project.full_path}/vulnerability_feedback"
+    )
     expect(subject.as_json).to include(:create_vulnerability_feedback_issue_path)
     expect(subject.as_json).to include(:create_vulnerability_feedback_merge_request_path)
     expect(subject.as_json).to include(:create_vulnerability_feedback_dismissal_path)

spec/javascripts/vue_mr_widget/stores/mr_widget_store_spec.js

@@ -82,47 +82,5 @@ describe('MergeRequestStore', () => {
         expect(store.isNothingToMergeState).toEqual(false);
       });
     });
-
-    describe('mergePipelinesEnabled', () => {
-      it('should set mergePipelinesEnabled = true when merge_pipelines_enabled is true', () => {
-        store.setData({ ...mockData, merge_pipelines_enabled: true });
-
-        expect(store.mergePipelinesEnabled).toBe(true);
-      });
-
-      it('should set mergePipelinesEnabled = false when merge_pipelines_enabled is not provided', () => {
-        store.setData({ ...mockData, merge_pipelines_enabled: undefined });
-
-        expect(store.mergePipelinesEnabled).toBe(false);
-      });
-    });
-
-    describe('mergeTrainsCount', () => {
-      it('should set mergeTrainsCount when merge_trains_count is provided', () => {
-        store.setData({ ...mockData, merge_trains_count: 3 });
-
-        expect(store.mergeTrainsCount).toBe(3);
-      });
-
-      it('should set mergeTrainsCount = 0 when merge_trains_count is not provided', () => {
-        store.setData({ ...mockData, merge_trains_count: undefined });
-
-        expect(store.mergeTrainsCount).toBe(0);
-      });
-    });
-
-    describe('mergeTrainIndex', () => {
-      it('should set mergeTrainIndex when merge_train_index is provided', () => {
-        store.setData({ ...mockData, merge_train_index: 3 });
-
-        expect(store.mergeTrainIndex).toBe(3);
-      });
-
-      it('should not set mergeTrainIndex when merge_train_index is not provided', () => {
-        store.setData({ ...mockData, merge_train_index: undefined });
-
-        expect(store.mergeTrainIndex).toBeUndefined();
-      });
-    });
   });
 });