Merge commit 'ca3c5c29' into rename-ci-commit

CHANGELOG

@@ -14,6 +14,8 @@ 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
+  - 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)
@@ -34,6 +36,13 @@ v 8.9.0 (unreleased)
   - Improve error handling importing projects
   - Put project Files and Commits tabs under Code tab
 
+v 8.8.4
+  - Fix todos page throwing errors when you have a project pending deletion
+  - Reduce number of SQL queries when rendering user references
+
+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

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]

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)

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
@@ -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)
+    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

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

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'))}");

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

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
 

app/views/projects/merge_requests/widget/open/_sha_mismatch.html.haml

+%h4
+  = icon("exclamation-triangle")
+  This merge request has received new commits since the page was loaded.
+
+%p
+  Please reload the page to review the new commits before merging.

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
 {

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.source_sha}", 409)
+          end
+
           merge_params = {
             commit_message: params[:merge_commit_message],
             should_remove_source_branch: params[:should_remove_source_branch]

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 = "")
@@ -78,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 <a> tag.
       def yield_valid_link(node)
         link = CGI.unescape(node.attr('href').to_s)

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

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)

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

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

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
@@ -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)
@@ -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, 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
+      end
+
+      it 'shows "All done" message' do
+        expect(page).to have_content "You're all done!"
+      end
+    end
   end
 end

spec/lib/banzai/filter/reference_filter_spec.rb

+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('<a href="foo">foo</a>')
+      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('<a href="foo">foo</a>')
+      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('<a href="foo" class="gfm">foo</a>')
+      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('<pre>foo</pre>')
+      filter = described_class.new(document, project: project)
+
+      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('<a href="foo">foo</a>')
+      filter = described_class.new(document, project: project)
+
+      expect(filter.nodes).to eq([document.children[0]])
+    end
+  end
+end

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("<p>#{user.to_reference}</p>")
+      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("<p>#{user.to_reference}</p>")
+      filter = described_class.new(document, project: project)
+
+      expect(filter.usernames).to eq([user.username])
+    end
+  end
 end

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(:pipeline).and_return(pipeline)
       allow(ci_commit).to receive(:active?).and_return(true)

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