From e8841e8dbc6a14af3ce98b7033136249dd9de265 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Thu, 18 May 2017 17:14:48 -0300 Subject: Drop merge_check endpoint and use only MR show instead --- .../services/mr_widget_service.js | 2 +- app/controllers/projects/merge_requests_controller.rb | 18 ++++++++++-------- .../projects/merge_requests_controller_spec.rb | 9 +++++++++ 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js b/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js index 42493be3372..79c3d335679 100644 --- a/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js +++ b/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js @@ -6,7 +6,7 @@ Vue.use(VueResource); export default class MRWidgetService { constructor(endpoints) { this.mergeResource = Vue.resource(endpoints.mergePath); - this.mergeCheckResource = Vue.resource(endpoints.mergeCheckPath); + this.mergeCheckResource = Vue.resource(endpoints.statusPath); this.cancelAutoMergeResource = Vue.resource(endpoints.cancelAutoMergePath); this.removeWIPResource = Vue.resource(endpoints.removeWIPPath); this.removeSourceBranchResource = Vue.resource(endpoints.sourceBranchPath); diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index b99ccd453b8..0352065998b 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -9,14 +9,15 @@ class Projects::MergeRequestsController < Projects::ApplicationController before_action :module_enabled before_action :merge_request, only: [ - :edit, :update, :show, :diffs, :commits, :conflicts, :conflict_for_path, :pipelines, :merge, :merge_check, + :edit, :update, :show, :diffs, :commits, :conflicts, :conflict_for_path, :pipelines, :merge, :pipeline_status, :ci_environments_status, :toggle_subscription, :cancel_merge_when_pipeline_succeeds, :remove_wip, :resolve_conflicts, :assign_related_issues, :commit_change_content ] before_action :validates_merge_request, only: [:show, :diffs, :commits, :pipelines] - before_action :define_show_vars, only: [:show, :diffs, :commits, :conflicts, :conflict_for_path, :builds, :pipelines] + before_action :define_show_vars, only: [:diffs, :commits, :conflicts, :conflict_for_path, :builds, :pipelines] before_action :define_commit_vars, only: [:diffs] before_action :ensure_ref_fetched, only: [:show, :diffs, :commits, :builds, :conflicts, :conflict_for_path, :pipelines] before_action :close_merge_request_without_source_project, only: [:show, :diffs, :commits, :builds, :pipelines] + before_action :check_if_can_be_merged, only: :show before_action :apply_diff_view_cookie!, only: [:new_diffs] before_action :build_merge_request, only: [:new, :new_diffs] @@ -75,9 +76,12 @@ class Projects::MergeRequestsController < Projects::ApplicationController respond_to do |format| format.html do define_discussion_vars + define_show_vars end format.json do + Gitlab::PollingInterval.set_header(response, interval: 10_000) + render json: serializer.represent(@merge_request, basic: params[:basic]) end @@ -309,12 +313,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController render json: serializer.represent(@merge_request) end - def merge_check - @merge_request.check_if_can_be_merged - - render json: serializer.represent(@merge_request) - end - def commit_change_content render partial: 'projects/merge_requests/widget/commit_change_content', layout: false end @@ -640,6 +638,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController private + def check_if_can_be_merged + @merge_request.check_if_can_be_merged + end + def merge! # Disable the CI check if merge_when_pipeline_succeeds is enabled since we have # to wait until CI completes to know diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 0b3492a8fed..7fe2f190790 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -119,6 +119,15 @@ describe Projects::MergeRequestsController do expect(response).to match_response_schema('entities/merge_request') end end + + context 'number of queries' do + it 'verifies number of queries' do + recorded = ActiveRecord::QueryRecorder.new { go(format: :json) } + + expect(recorded.count).to be_within(1).of(94) + expect(recorded.cached_count).to eq(0) + end + end end describe "as diff" do -- cgit v1.2.1 From 7efa0aa02c1590b943028ef12056717b0b078e28 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Thu, 18 May 2017 19:23:46 -0300 Subject: Remove unnecessary initial request --- app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js | 1 - spec/javascripts/vue_mr_widget/mr_widget_options_spec.js | 1 - 2 files changed, 2 deletions(-) diff --git a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js index 5452e19bd8e..99600b6664e 100644 --- a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js +++ b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js @@ -175,7 +175,6 @@ export default { }); }, handleMounted() { - this.checkStatus(); this.setFavicon(); this.initDeploymentsPolling(); }, diff --git a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js index 22ee7dcf0e7..e7adb54dc95 100644 --- a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js +++ b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js @@ -227,7 +227,6 @@ describe('mrWidgetOptions', () => { describe('handleMounted', () => { it('should call required methods to do the initial kick-off', () => { - spyOn(vm, 'checkStatus'); spyOn(vm, 'initDeploymentsPolling'); spyOn(vm, 'setFavicon'); -- cgit v1.2.1 From a355bcac4ba2fda77032f1cb7d88579f77a0ebe6 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Thu, 18 May 2017 20:05:47 -0300 Subject: Stop loading MergeRequestEntity data on sidebar request --- app/serializers/merge_request_basic_entity.rb | 1 + app/serializers/merge_request_entity.rb | 1 - app/views/shared/issuable/_sidebar.html.haml | 2 +- spec/fixtures/api/schemas/entities/merge_request.json | 1 - spec/fixtures/api/schemas/entities/merge_request_basic.json | 3 ++- 5 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/serializers/merge_request_basic_entity.rb b/app/serializers/merge_request_basic_entity.rb index 8771345c135..8461f158bb5 100644 --- a/app/serializers/merge_request_basic_entity.rb +++ b/app/serializers/merge_request_basic_entity.rb @@ -1,4 +1,5 @@ class MergeRequestBasicEntity < Grape::Entity + expose :assignee_id expose :merge_status expose :merge_error expose :state diff --git a/app/serializers/merge_request_entity.rb b/app/serializers/merge_request_entity.rb index a49f4d834cd..eb9d3861ee7 100644 --- a/app/serializers/merge_request_entity.rb +++ b/app/serializers/merge_request_entity.rb @@ -1,7 +1,6 @@ class MergeRequestEntity < IssuableEntity include RequestAwareEntity - expose :assignee_id expose :in_progress_merge_commit_sha expose :locked_at expose :merge_commit_sha diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml index ac84fffe831..e49bd5ebb13 100644 --- a/app/views/shared/issuable/_sidebar.html.haml +++ b/app/views/shared/issuable/_sidebar.html.haml @@ -140,7 +140,7 @@ :javascript gl.sidebarOptions = { - endpoint: "#{issuable_json_path(issuable)}", + endpoint: "#{issuable_json_path(issuable)}?basic=true", editable: #{can_edit_issuable ? true : false}, currentUser: #{current_user.to_json(only: [:username, :id, :name], methods: :avatar_url)}, rootPath: "#{root_path}" diff --git a/spec/fixtures/api/schemas/entities/merge_request.json b/spec/fixtures/api/schemas/entities/merge_request.json index e5df3e7b6d1..4afbb87453e 100644 --- a/spec/fixtures/api/schemas/entities/merge_request.json +++ b/spec/fixtures/api/schemas/entities/merge_request.json @@ -3,7 +3,6 @@ "properties" : { "id": { "type": "integer" }, "iid": { "type": "integer" }, - "assignee_id": { "type": ["integer", "null"] }, "author_id": { "type": "integer" }, "description": { "type": ["string", "null"] }, "lock_version": { "type": ["string", "null"] }, diff --git a/spec/fixtures/api/schemas/entities/merge_request_basic.json b/spec/fixtures/api/schemas/entities/merge_request_basic.json index ea6364b878c..6b14188582a 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_basic.json +++ b/spec/fixtures/api/schemas/entities/merge_request_basic.json @@ -8,7 +8,8 @@ "total_time_spent": { "type": "integer" }, "human_time_estimate": { "type": ["string", "null"] }, "human_total_time_spent": { "type": ["string", "null"] }, - "merge_error": { "type": ["string", "null"] } + "merge_error": { "type": ["string", "null"] }, + "assignee_id": { "type": ["integer", "null"] } }, "additionalProperties": false } -- cgit v1.2.1 From 92cdf78dcfa435673593088659790a5848152b9e Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Thu, 18 May 2017 21:15:17 -0300 Subject: Fix Karma expect --- spec/javascripts/vue_mr_widget/mr_widget_options_spec.js | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js index e7adb54dc95..bdc18243a15 100644 --- a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js +++ b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js @@ -232,7 +232,6 @@ describe('mrWidgetOptions', () => { vm.handleMounted(); - expect(vm.checkStatus).toHaveBeenCalled(); expect(vm.setFavicon).toHaveBeenCalled(); expect(vm.initDeploymentsPolling).toHaveBeenCalled(); }); -- cgit v1.2.1 From 3e08713c7975dc738b32e8b0e17651fbe3802ec2 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Thu, 18 May 2017 22:30:51 -0300 Subject: Remove route --- app/serializers/merge_request_entity.rb | 6 ------ config/routes/project.rb | 1 - spec/routing/project_routing_spec.rb | 5 ----- 3 files changed, 12 deletions(-) diff --git a/app/serializers/merge_request_entity.rb b/app/serializers/merge_request_entity.rb index eb9d3861ee7..b3247ae36dd 100644 --- a/app/serializers/merge_request_entity.rb +++ b/app/serializers/merge_request_entity.rb @@ -153,12 +153,6 @@ class MergeRequestEntity < IssuableEntity format: :json) end - expose :merge_check_path do |merge_request| - merge_check_namespace_project_merge_request_path(merge_request.project.namespace, - merge_request.project, - merge_request) - end - expose :ci_environments_status_path do |merge_request| ci_environments_status_namespace_project_merge_request_path(merge_request.project.namespace, merge_request.project, diff --git a/config/routes/project.rb b/config/routes/project.rb index c786cbdee1e..01b94f9f2b8 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -74,7 +74,6 @@ constraints(ProjectUrlConstrainer.new) do get :conflicts get :conflict_for_path get :pipelines - get :merge_check get :commit_change_content post :merge post :cancel_merge_when_pipeline_succeeds diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb index 50e96d56191..d5400bbaaf1 100644 --- a/spec/routing/project_routing_spec.rb +++ b/spec/routing/project_routing_spec.rb @@ -243,7 +243,6 @@ describe 'project routing' do # diffs_namespace_project_merge_request GET /:namespace_id/:project_id/merge_requests/:id/diffs(.:format) projects/merge_requests#diffs # commits_namespace_project_merge_request GET /:namespace_id/:project_id/merge_requests/:id/commits(.:format) projects/merge_requests#commits # merge_namespace_project_merge_request POST /:namespace_id/:project_id/merge_requests/:id/merge(.:format) projects/merge_requests#merge - # merge_check_namespace_project_merge_request GET /:namespace_id/:project_id/merge_requests/:id/merge_check(.:format) projects/merge_requests#merge_check # ci_status_namespace_project_merge_request GET /:namespace_id/:project_id/merge_requests/:id/ci_status(.:format) projects/merge_requests#ci_status # toggle_subscription_namespace_project_merge_request POST /:namespace_id/:project_id/merge_requests/:id/toggle_subscription(.:format) projects/merge_requests#toggle_subscription # branch_from_namespace_project_merge_requests GET /:namespace_id/:project_id/merge_requests/branch_from(.:format) projects/merge_requests#branch_from @@ -272,10 +271,6 @@ describe 'project routing' do ) end - it 'to #merge_check' do - expect(get('/gitlab/gitlabhq/merge_requests/1/merge_check')).to route_to('projects/merge_requests#merge_check', namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') - end - it 'to #branch_from' do expect(get('/gitlab/gitlabhq/merge_requests/branch_from')).to route_to('projects/merge_requests#branch_from', namespace_id: 'gitlab', project_id: 'gitlabhq') end -- cgit v1.2.1 From b3cf3d046b8a319c0dbca7cb9aeaea027c080e6f Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Fri, 19 May 2017 10:19:04 -0300 Subject: Make sure fixture creation does not affect query count test --- spec/controllers/projects/merge_requests_controller_spec.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 7fe2f190790..f0dc6df15ee 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -122,9 +122,12 @@ describe Projects::MergeRequestsController do context 'number of queries' do it 'verifies number of queries' do + # pre-create objects + merge_request + recorded = ActiveRecord::QueryRecorder.new { go(format: :json) } - expect(recorded.count).to be_within(1).of(94) + expect(recorded.count).to be_within(1).of(51) expect(recorded.cached_count).to eq(0) end end -- cgit v1.2.1