From d863d86aeb1993c2032da0610b3662e61960eb38 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 1 Jun 2016 10:21:22 +0100 Subject: [PATCH 01/18] Add `sha` parameter to MR accept API The `sha` parameter is optional, and when present, must match the current HEAD SHA of the source branch. Otherwise, the API call fails with a 409 Conflict and a message containing the current HEAD for the source branch. Also tidy up some doc wording. --- CHANGELOG | 1 + doc/api/merge_requests.md | 11 +++++++---- lib/api/merge_requests.rb | 5 +++++ spec/requests/api/merge_requests_spec.rb | 13 +++++++++++++ 4 files changed, 26 insertions(+), 4 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 848aaa8506e..eb037e1ab8b 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -13,6 +13,7 @@ v 8.9.0 (unreleased) - Fix groups API to list only user's accessible projects - Redesign account and email confirmation emails - Use gitlab-shell v3.0.0 + - Add `sha` parameter to MR merge API, to ensure only reviewed changes are merged - Add DB index on users.state - Add rake task 'gitlab:db:configure' for conditionally seeding or migrating the database - Changed the Slack build message to use the singular duration if necessary (Aran Koning) diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 8217e30fe25..16b892dc3b7 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -413,11 +413,13 @@ curl -X DELETE -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.c Merge changes submitted with MR using this API. -If merge success you get `200 OK`. +If the merge succeeds you'll get a `200 OK`. -If it has some conflicts and can not be merged - you get 405 and error message 'Branch cannot be merged' +If it has some conflicts and can not be merged - you'll get a 405 and the error message 'Branch cannot be merged' -If merge request is already merged or closed - you get 405 and error message 'Method Not Allowed' +If merge request is already merged or closed - you'll get a 406 and the error message 'Method Not Allowed' + +If the `sha` parameter is passed and does not match the HEAD of the source - you'll get a 409 and the error message 'SHA does not match HEAD of source branch' If you don't have permissions to accept this merge request - you'll get a 401 @@ -431,7 +433,8 @@ Parameters: - `merge_request_id` (required) - ID of MR - `merge_commit_message` (optional) - Custom merge commit message - `should_remove_source_branch` (optional) - if `true` removes the source branch -- `merged_when_build_succeeds` (optional) - if `true` the MR is merge when the build succeeds +- `merged_when_build_succeeds` (optional) - if `true` the MR is merged when the build succeeds +- `sha` (optional) - if present, then this SHA must match the HEAD of the source branch, otherwise the merge will fail ```json { diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 4e7de8867b4..50baf4c09ad 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -218,6 +218,7 @@ module API # merge_commit_message (optional) - Custom merge commit message # should_remove_source_branch (optional) - When true, the source branch will be deleted if possible # merge_when_build_succeeds (optional) - When true, this MR will be merged when the build succeeds + # sha (optional) - When present, must have the HEAD SHA of the source branch # Example: # PUT /projects/:id/merge_requests/:merge_request_id/merge # @@ -233,6 +234,10 @@ module API render_api_error!('Branch cannot be merged', 406) unless merge_request.can_be_merged? + if params[:sha] && merge_request.source_sha != params[:sha] + render_api_error!("SHA does not match HEAD of source branch: #{merge_request.target_sha}", 409) + end + merge_params = { commit_message: params[:merge_commit_message], should_remove_source_branch: params[:should_remove_source_branch] diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 4b0111df149..5aa98ec4014 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -428,6 +428,19 @@ describe API::API, api: true do expect(json_response['message']).to eq('401 Unauthorized') end + it "returns 409 if the SHA parameter doesn't match" do + put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user), sha: merge_request.source_sha.succ + + expect(response.status).to eq(409) + expect(json_response['message']).to start_with('SHA does not match HEAD of source branch') + end + + it "succeeds if the SHA parameter matches" do + put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user), sha: merge_request.source_sha + + expect(response.status).to eq(200) + end + it "enables merge when build succeeds if the ci is active" do allow_any_instance_of(MergeRequest).to receive(:ci_commit).and_return(ci_commit) allow(ci_commit).to receive(:active?).and_return(true) From f680eca912f854373fc538ae1e9d0dcb60fcd310 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 1 Jun 2016 13:25:44 +0100 Subject: [PATCH 02/18] Don't allow merges with new commits Set a `sha` parameter on the MR form. If this doesn't match the HEAD of the source branch when the form is submitted, show a warning (like with a merge conflict) and don't merge the branch. --- CHANGELOG | 1 + .../projects/merge_requests_controller.rb | 5 ++ .../projects/merge_requests/merge.js.haml | 3 + .../widget/open/_accept.html.haml | 1 + .../open/_merge_when_build_succeeds.html.haml | 2 +- .../widget/open/_sha_mismatch.html.haml | 6 ++ .../merge_requests_controller_spec.rb | 86 +++++++++++++++++++ 7 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml diff --git a/CHANGELOG b/CHANGELOG index eb037e1ab8b..b998713e2f7 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -14,6 +14,7 @@ v 8.9.0 (unreleased) - Redesign account and email confirmation emails - Use gitlab-shell v3.0.0 - Add `sha` parameter to MR merge API, to ensure only reviewed changes are merged + - Don't allow MRs to be merged when commits were added since the last review / page load - Add DB index on users.state - Add rake task 'gitlab:db:configure' for conditionally seeding or migrating the database - Changed the Slack build message to use the singular duration if necessary (Aran Koning) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index d54284d7b20..3142fe5c767 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -190,6 +190,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController return end + if params[:sha] != @merge_request.source_sha + @status = :sha_mismatch + return + end + TodoService.new.merge_merge_request(merge_request, current_user) @merge_request.update(merge_error: nil) diff --git a/app/views/projects/merge_requests/merge.js.haml b/app/views/projects/merge_requests/merge.js.haml index 92ce479d463..84b6c9ebc5c 100644 --- a/app/views/projects/merge_requests/merge.js.haml +++ b/app/views/projects/merge_requests/merge.js.haml @@ -5,6 +5,9 @@ - when :merge_when_build_succeeds :plain $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/merge_when_build_succeeds'))}"); +- when :sha_mismatch + :plain + $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/sha_mismatch'))}"); - else :plain $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/reload'))}"); diff --git a/app/views/projects/merge_requests/widget/open/_accept.html.haml b/app/views/projects/merge_requests/widget/open/_accept.html.haml index cfdf4edac37..0d49b6471a9 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -2,6 +2,7 @@ = form_for [:merge, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post, html: { class: 'accept-mr-form js-quick-submit js-requires-input' } do |f| = hidden_field_tag :authenticity_token, form_authenticity_token + = hidden_field_tag :sha, @merge_request.source_sha .accept-merge-holder.clearfix.js-toggle-container .clearfix .accept-action diff --git a/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml b/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml index b83ddcab3a4..ad898ff153b 100644 --- a/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml +++ b/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml @@ -16,7 +16,7 @@ - if remove_source_branch_button || user_can_cancel_automatic_merge .clearfix.prepend-top-10 - if remove_source_branch_button - = link_to merge_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, merge_when_build_succeeds: true, should_remove_source_branch: true), remote: true, method: :post, class: "btn btn-grouped btn-primary btn-sm remove_source_branch" do + = link_to merge_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, merge_when_build_succeeds: true, should_remove_source_branch: true, sha: @merge_request.source_sha), remote: true, method: :post, class: "btn btn-grouped btn-primary btn-sm remove_source_branch" do = icon('times') Remove Source Branch When Merged diff --git a/app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml b/app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml new file mode 100644 index 00000000000..a78583b7c61 --- /dev/null +++ b/app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml @@ -0,0 +1,6 @@ +%h4 + = icon("exclamation-triangle") + This merge request has new commits since the page was loaded. + +%p + Please review the new commits before merging. diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 4f621a43d7e..8499bf07e9f 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -185,6 +185,92 @@ describe Projects::MergeRequestsController do end end + describe 'POST #merge' do + let(:base_params) do + { + namespace_id: project.namespace.path, + project_id: project.path, + id: merge_request.iid, + format: 'raw' + } + end + + context 'when the user does not have access' do + before do + project.team.truncate + project.team << [user, :reporter] + post :merge, base_params + end + + it 'returns not found' do + expect(response).to be_not_found + end + end + + context 'when the merge request is not mergeable' do + before do + merge_request.update_attributes(title: "WIP: #{merge_request.title}") + + post :merge, base_params + end + + it 'returns :failed' do + expect(assigns(:status)).to eq(:failed) + end + end + + context 'when the sha parameter does not match the source SHA' do + before { post :merge, base_params.merge(sha: 'foo') } + + it 'returns :sha_mismatch' do + expect(assigns(:status)).to eq(:sha_mismatch) + end + end + + context 'when the sha parameter matches the source SHA' do + def merge_with_sha + post :merge, base_params.merge(sha: merge_request.source_sha) + end + + it 'returns :success' do + merge_with_sha + + expect(assigns(:status)).to eq(:success) + end + + it 'starts the merge immediately' do + expect(MergeWorker).to receive(:perform_async).with(merge_request.id, anything, anything) + + merge_with_sha + end + + context 'when merge_when_build_succeeds is passed' do + def merge_when_build_succeeds + post :merge, base_params.merge(sha: merge_request.source_sha, merge_when_build_succeeds: '1') + end + + before do + create(:ci_empty_commit, project: project, sha: merge_request.source_sha, ref: merge_request.source_branch) + end + + it 'returns :merge_when_build_succeeds' do + merge_when_build_succeeds + + expect(assigns(:status)).to eq(:merge_when_build_succeeds) + end + + it 'sets the MR to merge when the build succeeds' do + service = double(:merge_when_build_succeeds_service) + + expect(MergeRequests::MergeWhenBuildSucceedsService).to receive(:new).with(project, anything, anything).and_return(service) + expect(service).to receive(:execute).with(merge_request) + + merge_when_build_succeeds + end + end + end + end + describe "DELETE #destroy" do it "denies access to users unless they're admin or project owner" do delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: merge_request.iid From 2fbfb85492401b2a8ac81f22b319b304afecf6c3 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 30 May 2016 15:55:11 +0200 Subject: [PATCH 03/18] Returning enums in ReferenceFilter#each_node This changes ReferenceFilter#each_node so that when it's called without a block an Enumerator is returned. --- lib/banzai/filter/reference_filter.rb | 2 ++ .../banzai/filter/reference_filter_spec.rb | 36 +++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 spec/lib/banzai/filter/reference_filter_spec.rb diff --git a/lib/banzai/filter/reference_filter.rb b/lib/banzai/filter/reference_filter.rb index 41ae0e1f9cc..5eace574ab4 100644 --- a/lib/banzai/filter/reference_filter.rb +++ b/lib/banzai/filter/reference_filter.rb @@ -68,6 +68,8 @@ module Banzai # by `ignore_ancestor_query`. Link tags are not processed if they have a # "gfm" class or the "href" attribute is empty. def each_node + return to_enum(__method__) unless block_given? + query = %Q{descendant-or-self::text()[not(#{ignore_ancestor_query})] | descendant-or-self::a[ not(contains(concat(" ", @class, " "), " gfm ")) and not(@href = "") diff --git a/spec/lib/banzai/filter/reference_filter_spec.rb b/spec/lib/banzai/filter/reference_filter_spec.rb new file mode 100644 index 00000000000..d3e7af0671c --- /dev/null +++ b/spec/lib/banzai/filter/reference_filter_spec.rb @@ -0,0 +1,36 @@ +require 'spec_helper' + +describe Banzai::Filter::ReferenceFilter, lib: true do + let(:project) { build(:project) } + + describe '#each_node' do + it 'iterates over the nodes in a document' do + document = Nokogiri::HTML.fragment('foo') + filter = described_class.new(document, project: project) + + expect { |b| filter.each_node(&b) }. + to yield_with_args(an_instance_of(Nokogiri::XML::Element)) + end + + it 'returns an Enumerator when no block is given' do + document = Nokogiri::HTML.fragment('foo') + filter = described_class.new(document, project: project) + + expect(filter.each_node).to be_an_instance_of(Enumerator) + end + + it 'skips links with a "gfm" class' do + document = Nokogiri::HTML.fragment('foo') + filter = described_class.new(document, project: project) + + expect { |b| filter.each_node(&b) }.not_to yield_control + end + + it 'skips text nodes in pre elements' do + document = Nokogiri::HTML.fragment('
foo
') + filter = described_class.new(document, project: project) + + expect { |b| filter.each_node(&b) }.not_to yield_control + end + end +end From 8a6c3f27e9dfea2c151657045e17fe66ad81b5e5 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 30 May 2016 15:56:05 +0200 Subject: [PATCH 04/18] Added ReferenceFilter#nodes This method returns an Array of the HTML nodes as yielded by ReferenceFilter#each_node. The method's return value is memoized to allow multiple calls without having to re-query the input document. --- lib/banzai/filter/reference_filter.rb | 5 +++++ spec/lib/banzai/filter/reference_filter_spec.rb | 9 +++++++++ 2 files changed, 14 insertions(+) diff --git a/lib/banzai/filter/reference_filter.rb b/lib/banzai/filter/reference_filter.rb index 5eace574ab4..2d6f34c9cd8 100644 --- a/lib/banzai/filter/reference_filter.rb +++ b/lib/banzai/filter/reference_filter.rb @@ -80,6 +80,11 @@ module Banzai end end + # Returns an Array containing all HTML nodes. + def nodes + @nodes ||= each_node.to_a + end + # Yields the link's URL and text whenever the node is a valid tag. def yield_valid_link(node) link = CGI.unescape(node.attr('href').to_s) diff --git a/spec/lib/banzai/filter/reference_filter_spec.rb b/spec/lib/banzai/filter/reference_filter_spec.rb index d3e7af0671c..55e681f6faf 100644 --- a/spec/lib/banzai/filter/reference_filter_spec.rb +++ b/spec/lib/banzai/filter/reference_filter_spec.rb @@ -33,4 +33,13 @@ describe Banzai::Filter::ReferenceFilter, lib: true do expect { |b| filter.each_node(&b) }.not_to yield_control end end + + describe '#nodes' do + it 'returns an Array of the HTML nodes' do + document = Nokogiri::HTML.fragment('foo') + filter = described_class.new(document, project: project) + + expect(filter.nodes).to eq([document.children[0]]) + end + end end From 01575e9966805fa4c12a7a56361f511b3b61e309 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 30 May 2016 15:56:50 +0200 Subject: [PATCH 05/18] Reduce Namespace queries in UserReferenceFilter This changes UserReferenceFilter so it operates using the following steps: 1. Grab all username references from the input document. 2. Query the corresponding Namespace objects using a single query. 3. Iterate over all nodes to build links while re-using the objects queried in step 2. The impact of these changes is that a comment mentioning 5 different usernames no longer runs 5 different queries (1 for every username), instead it only runs a single query. --- CHANGELOG | 1 + lib/banzai/filter/user_reference_filter.rb | 29 +++++++++++++++++-- .../filter/user_reference_filter_spec.rb | 19 ++++++++++++ 3 files changed, 47 insertions(+), 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index d1cde40c1c7..7e224060227 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -26,6 +26,7 @@ v 8.9.0 (unreleased) - Measure queue duration between gitlab-workhorse and Rails - Make authentication service for Container Registry to be compatible with < Docker 1.11 - Add Application Setting to configure Container Registry token expire delay (default 5min) + - Reduce number of SQL queries when rendering user references v 8.8.3 - Fix incorrect links on pipeline page when merge request created from fork diff --git a/lib/banzai/filter/user_reference_filter.rb b/lib/banzai/filter/user_reference_filter.rb index 331d8007257..5b0a6d8541b 100644 --- a/lib/banzai/filter/user_reference_filter.rb +++ b/lib/banzai/filter/user_reference_filter.rb @@ -29,7 +29,7 @@ module Banzai ref_pattern = User.reference_pattern ref_pattern_start = /\A#{ref_pattern}\z/ - each_node do |node| + nodes.each do |node| if text_node?(node) replace_text_when_pattern_matches(node, ref_pattern) do |content| user_link_filter(content) @@ -59,7 +59,7 @@ module Banzai self.class.references_in(text) do |match, username| if username == 'all' link_to_all(link_text: link_text) - elsif namespace = Namespace.find_by(path: username) + elsif namespace = namespaces[username] link_to_namespace(namespace, link_text: link_text) || match else match @@ -67,6 +67,31 @@ module Banzai end end + # Returns a Hash containing all Namespace objects for the username + # references in the current document. + # + # The keys of this Hash are the namespace paths, the values the + # corresponding Namespace objects. + def namespaces + @namespaces ||= + Namespace.where(path: usernames).each_with_object({}) do |row, hash| + hash[row.path] = row + end + end + + # Returns all usernames referenced in the current document. + def usernames + refs = Set.new + + nodes.each do |node| + node.to_html.scan(User.reference_pattern) do + refs << $~[:user] + end + end + + refs.to_a + end + private def urls diff --git a/spec/lib/banzai/filter/user_reference_filter_spec.rb b/spec/lib/banzai/filter/user_reference_filter_spec.rb index d7dfd6699ef..108b36a97cc 100644 --- a/spec/lib/banzai/filter/user_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/user_reference_filter_spec.rb @@ -136,4 +136,23 @@ describe Banzai::Filter::UserReferenceFilter, lib: true do expect(link.attr('data-user')).to eq user.namespace.owner_id.to_s end end + + describe '#namespaces' do + it 'returns a Hash containing all Namespaces' do + document = Nokogiri::HTML.fragment("

#{user.to_reference}

") + filter = described_class.new(document, project: project) + ns = user.namespace + + expect(filter.namespaces).to eq({ ns.path => ns }) + end + end + + describe '#usernames' do + it 'returns the usernames mentioned in a document' do + document = Nokogiri::HTML.fragment("

#{user.to_reference}

") + filter = described_class.new(document, project: project) + + expect(filter.usernames).to eq([user.username]) + end + end end From 04fdf4b9a926129d63d0483ad8cd7e748f3a4e07 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 2 Jun 2016 17:00:16 +0100 Subject: [PATCH 06/18] fixup! Add `sha` parameter to MR accept API --- lib/api/merge_requests.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 50baf4c09ad..db304abe1c3 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -235,7 +235,7 @@ module API render_api_error!('Branch cannot be merged', 406) unless merge_request.can_be_merged? if params[:sha] && merge_request.source_sha != params[:sha] - render_api_error!("SHA does not match HEAD of source branch: #{merge_request.target_sha}", 409) + render_api_error!("SHA does not match HEAD of source branch: #{merge_request.source_sha}", 409) end merge_params = { From 4f726683cb59da54f47302880d5c0c447638402a Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 2 Jun 2016 17:00:42 +0100 Subject: [PATCH 07/18] fixup! Don't allow merges with new commits --- .../merge_requests/widget/open/_sha_mismatch.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml b/app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml index a78583b7c61..499624f8dd8 100644 --- a/app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml +++ b/app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml @@ -1,6 +1,6 @@ %h4 = icon("exclamation-triangle") - This merge request has new commits since the page was loaded. + This merge request has received new commits since the page was loaded. %p - Please review the new commits before merging. + Please reload the page to review the new commits before merging. From 905e8b6b545dfb6bc408824889b66d47aa02eda2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 2 Jun 2016 18:14:33 +0200 Subject: [PATCH 08/18] Remove unused Issuable#is_assigned? method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/models/concerns/issuable.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 50f5b749e38..37124379c18 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -171,10 +171,6 @@ module Issuable today? && created_at == updated_at end - def is_assigned? - !!assignee_id - end - def is_being_reassigned? assignee_id_changed? end From 34007aa0dc61df80514cc0ff125eef8fcf57e35a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 2 Jun 2016 18:25:44 +0200 Subject: [PATCH 09/18] Fix deprecation warnings in spec/services/issues/bulk_update_service_spec.rb MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- spec/services/issues/bulk_update_service_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/services/issues/bulk_update_service_spec.rb b/spec/services/issues/bulk_update_service_spec.rb index 96f050bbd9b..454d5849495 100644 --- a/spec/services/issues/bulk_update_service_spec.rb +++ b/spec/services/issues/bulk_update_service_spec.rb @@ -18,7 +18,7 @@ describe Issues::BulkUpdateService, services: true do @issues = create_list(:issue, 5, project: @project) @params = { state_event: 'close', - issues_ids: @issues.map(&:id) + issues_ids: @issues.map(&:id).join(",") } end @@ -38,7 +38,7 @@ describe Issues::BulkUpdateService, services: true do @issues = create_list(:closed_issue, 5, project: @project) @params = { state_event: 'reopen', - issues_ids: @issues.map(&:id) + issues_ids: @issues.map(&:id).join(",") } end @@ -58,7 +58,7 @@ describe Issues::BulkUpdateService, services: true do before do @new_assignee = create :user @params = { - issues_ids: [issue.id], + issues_ids: issue.id.to_s, assignee_id: @new_assignee.id } end @@ -97,7 +97,7 @@ describe Issues::BulkUpdateService, services: true do before do @milestone = create(:milestone, project: @project) @params = { - issues_ids: [issue.id], + issues_ids: issue.id.to_s, milestone_id: @milestone.id } end From c3e923c496b7d1c344a5fa68cef4a80ce23c90d0 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Wed, 25 May 2016 18:52:10 -0700 Subject: [PATCH 10/18] Ensure we don't show TODOS for projects pending delete By joining the Todos on the project table. --- app/finders/todos_finder.rb | 2 +- spec/features/todos/todos_spec.rb | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index 4bd46a76087..f638b5bf91f 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -23,7 +23,7 @@ class TodosFinder end def execute - items = current_user.todos + items = current_user.todos.joins(:project).where(projects: { pending_delete: false }) items = by_action_id(items) items = by_author(items) items = by_project(items) diff --git a/spec/features/todos/todos_spec.rb b/spec/features/todos/todos_spec.rb index 4e627753cc7..c8c86a3ff47 100644 --- a/spec/features/todos/todos_spec.rb +++ b/spec/features/todos/todos_spec.rb @@ -98,5 +98,18 @@ describe 'Dashboard Todos', feature: true do end end end + + context 'User has a Todo in a project pending deletion' do + before do + deleted_project = create(:project, pending_delete: true) + create(:todo, :mentioned, user: user, project: deleted_project, target: issue, author: author) + login_as(user) + visit dashboard_todos_path + end + + it 'shows "All done" message' do + expect(page).to have_content "You're all done!" + end + end end end From 4ecc10fade61a1b45cd45ea4189e95a2acbea353 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Wed, 25 May 2016 21:31:36 -0700 Subject: [PATCH 11/18] Reduce the filters on the todos joins project query by being explicit about the join --- app/finders/todos_finder.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index f638b5bf91f..3243d62fc95 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -23,7 +23,13 @@ class TodosFinder end def execute - items = current_user.todos.joins(:project).where(projects: { pending_delete: false }) + items = current_user.todos + + # Filter out todos linked to project pending deletion + items = items.joins( + 'INNER JOIN projects ON projects.id = todos.project_id AND projects.pending_delete = false' + ) + items = by_action_id(items) items = by_author(items) items = by_project(items) From 4280575fc0888632196cf4483dcd777618c13390 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Mon, 30 May 2016 10:59:14 -0700 Subject: [PATCH 12/18] Move filtering todos by projects not pending deletion into a scope on the todo model --- app/finders/todos_finder.rb | 8 +------- app/models/todo.rb | 1 + 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index 3243d62fc95..5d7d55180e1 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -23,13 +23,7 @@ class TodosFinder end def execute - items = current_user.todos - - # Filter out todos linked to project pending deletion - items = items.joins( - 'INNER JOIN projects ON projects.id = todos.project_id AND projects.pending_delete = false' - ) - + items = current_user.todos.not_pending_delete items = by_action_id(items) items = by_author(items) items = by_project(items) diff --git a/app/models/todo.rb b/app/models/todo.rb index 3a091373329..f66755436ea 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -19,6 +19,7 @@ class Todo < ActiveRecord::Base scope :pending, -> { with_state(:pending) } scope :done, -> { with_state(:done) } + scope :not_pending_delete, -> { joins('INNER JOIN projects ON projects.id = todos.project_id AND projects.pending_delete = false') } state_machine :state, initial: :pending do event :done do From b173ea2bd4bbc65529b827f9afa5999f6f04579e Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Wed, 1 Jun 2016 16:44:35 -0700 Subject: [PATCH 13/18] Use the project finder in the todos finder to limit todos to just ones within projects you have access to. --- app/finders/todos_finder.rb | 14 +++++++++++++- app/models/todo.rb | 1 - spec/features/todos/todos_spec.rb | 2 +- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index 5d7d55180e1..6fbe68a720d 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -23,7 +23,7 @@ class TodosFinder end def execute - items = current_user.todos.not_pending_delete + items = current_user.todos items = by_action_id(items) items = by_author(items) items = by_project(items) @@ -78,6 +78,16 @@ class TodosFinder @project end + def projects + return @projects if defined?(@projects) + + if project? + @projects = project + else + @projects = ProjectsFinder.new.execute(current_user).reorder(nil) + end + end + def type? type.present? && ['Issue', 'MergeRequest'].include?(type) end @@ -105,6 +115,8 @@ class TodosFinder def by_project(items) if project? items = items.where(project: project) + elsif projects + items = items.merge(projects).joins(:project) end items diff --git a/app/models/todo.rb b/app/models/todo.rb index f66755436ea..3a091373329 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -19,7 +19,6 @@ class Todo < ActiveRecord::Base scope :pending, -> { with_state(:pending) } scope :done, -> { with_state(:done) } - scope :not_pending_delete, -> { joins('INNER JOIN projects ON projects.id = todos.project_id AND projects.pending_delete = false') } state_machine :state, initial: :pending do event :done do diff --git a/spec/features/todos/todos_spec.rb b/spec/features/todos/todos_spec.rb index c8c86a3ff47..c0a1cd64f32 100644 --- a/spec/features/todos/todos_spec.rb +++ b/spec/features/todos/todos_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe 'Dashboard Todos', feature: true do let(:user) { create(:user) } let(:author) { create(:user) } - let(:project) { create(:project) } + let(:project) { create(:project, visibility_level: Gitlab::VisibilityLevel::PUBLIC) } let(:issue) { create(:issue) } describe 'GET /dashboard/todos' do From 14a9b0d7dda78e88852644bd9a6c05922b5e367d Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Thu, 2 Jun 2016 12:06:24 -0700 Subject: [PATCH 14/18] Update target todo test to use a public project --- CHANGELOG | 3 +++ spec/features/todos/target_state_spec.rb | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 27f60c7de06..ef5b4aa79cd 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -31,6 +31,9 @@ v 8.9.0 (unreleased) - Cache assigned issue and merge request counts in sidebar nav - Cache project build count in sidebar nav +v 8.8.4 + - Fix todos page throwing errors when you have a project pending deletion + v 8.8.3 - Fix 404 page when viewing TODOs that contain milestones or labels in different projects. !4312 - Fixed JS error when trying to remove discussion form. !4303 diff --git a/spec/features/todos/target_state_spec.rb b/spec/features/todos/target_state_spec.rb index 72491ac7e61..32fa88a2b21 100644 --- a/spec/features/todos/target_state_spec.rb +++ b/spec/features/todos/target_state_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' feature 'Todo target states', feature: true do let(:user) { create(:user) } let(:author) { create(:user) } - let(:project) { create(:project) } + let(:project) { create(:project, visibility_level: Gitlab::VisibilityLevel::PUBLIC) } before do login_as user From f0ca487cd513d50c807e4226a1ee459586f16b08 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Thu, 2 Jun 2016 14:17:46 -0700 Subject: [PATCH 15/18] Reorder the todos because the use of the project finder attempts to order them differently --- app/finders/todos_finder.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index 6fbe68a720d..1d88116d7d2 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -30,7 +30,7 @@ class TodosFinder items = by_state(items) items = by_type(items) - items + items.reorder(id: :desc) end private @@ -84,7 +84,7 @@ class TodosFinder if project? @projects = project else - @projects = ProjectsFinder.new.execute(current_user).reorder(nil) + @projects = ProjectsFinder.new.execute(current_user) end end From 86675194aa44236c3e9acc4aa7ede143f718685e Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Thu, 2 Jun 2016 15:30:13 -0700 Subject: [PATCH 16/18] Fix failing todo tests --- spec/features/todos/todos_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/features/todos/todos_spec.rb b/spec/features/todos/todos_spec.rb index c0a1cd64f32..8e1833a069e 100644 --- a/spec/features/todos/todos_spec.rb +++ b/spec/features/todos/todos_spec.rb @@ -49,7 +49,7 @@ describe 'Dashboard Todos', feature: true do note1 = create(:note_on_issue, note: "Hello #{label1.to_reference(format: :name)}", noteable_id: issue.id, noteable_type: 'Issue', project: issue.project) create(:todo, :mentioned, project: project, target: issue, user: user, note_id: note1.id) - project2 = create(:project) + project2 = create(:project, visibility_level: Gitlab::VisibilityLevel::PUBLIC) label2 = create(:label, project: project2) issue2 = create(:issue, project: project2) note2 = create(:note_on_issue, note: "Test #{label2.to_reference(format: :name)}", noteable_id: issue2.id, noteable_type: 'Issue', project: project2) @@ -101,7 +101,7 @@ describe 'Dashboard Todos', feature: true do context 'User has a Todo in a project pending deletion' do before do - deleted_project = create(:project, pending_delete: true) + deleted_project = create(:project, visibility_level: Gitlab::VisibilityLevel::PUBLIC, pending_delete: true) create(:todo, :mentioned, user: user, project: deleted_project, target: issue, author: author) login_as(user) visit dashboard_todos_path From 3b4f03de8fb0de0b882d499c9259f053fa69d9e6 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 2 Jun 2016 19:48:11 -0300 Subject: [PATCH 17/18] Ensure branch cleanup regardless of whether the import process succeeds --- CHANGELOG | 3 +++ lib/gitlab/github_import/importer.rb | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 68ea866968d..6ead604b724 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -32,6 +32,9 @@ v 8.9.0 (unreleased) - Cache project build count in sidebar nav - Reduce number of queries needed to render issue labels in the sidebar +v 8.8.4 (unreleased) + - Ensure branch cleanup regardless of whether the GitHub import process succeeds + v 8.8.3 - Fix 404 page when viewing TODOs that contain milestones or labels in different projects. !4312 - Fixed JS error when trying to remove discussion form. !4303 diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb index 408d9b79632..9d077e79c39 100644 --- a/lib/gitlab/github_import/importer.rb +++ b/lib/gitlab/github_import/importer.rb @@ -89,11 +89,11 @@ module Gitlab end end - delete_refs(branches_removed) - true rescue ActiveRecord::RecordInvalid => e raise Projects::ImportService::Error, e.message + ensure + delete_refs(branches_removed) end def create_refs(branches) From 7cc897d74e72367ac782eb7cd469f4c26acc3ef0 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Fri, 3 Jun 2016 09:16:45 +0000 Subject: [PATCH 18/18] Let contributors know where to start gitlab-org/gitlab-ce/issues/14905#note_12235996 --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a15f8c4fec7..e952855fde1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -308,7 +308,7 @@ tests are least likely to receive timely feedback. The workflow to make a merge request is as follows: 1. Fork the project into your personal space on GitLab.com -1. Create a feature branch +1. Create a feature branch, branch away from `master`. 1. Write [tests](https://gitlab.com/gitlab-org/gitlab-development-kit#running-the-tests) and code 1. Add your changes to the [CHANGELOG](CHANGELOG) 1. If you are writing documentation, make sure to read the [documentation styleguide][doc-styleguide]