Automatic merge of gitlab-org/gitlab-ce master

Gemfile.lock

@@ -801,7 +801,7 @@ GEM
     retriable (3.1.2)
     rinku (2.0.0)
     rotp (2.1.2)
-    rouge (3.3.0)
+    rouge (3.4.1)
     rqrcode (0.7.0)
       chunky_png
     rqrcode-rails3 (0.1.7)

app/controllers/projects/issues_controller.rb

@@ -33,7 +33,7 @@ class Projects::IssuesController < Projects::ApplicationController
   before_action :authorize_create_issue!, only: [:new, :create]
 
   # Allow modify issue
-  before_action :authorize_update_issuable!, only: [:edit, :update, :move]
+  before_action :authorize_update_issuable!, only: [:edit, :update, :move, :reorder]
 
   # Allow create a new branch and empty WIP merge request from current issue
   before_action :authorize_create_merge_request_from!, only: [:create_merge_request]
@@ -132,6 +132,16 @@ class Projects::IssuesController < Projects::ApplicationController
     render_conflict_response
   end
 
+  def reorder
+    service = Issues::ReorderService.new(project, current_user, reorder_params)
+
+    if service.execute(issue)
+      head :ok
+    else
+      head :unprocessable_entity
+    end
+  end
+
   def related_branches
     @related_branches = Issues::RelatedBranchesService.new(project, current_user).execute(issue)
 
@@ -239,6 +249,10 @@ class Projects::IssuesController < Projects::ApplicationController
     ] + [{ label_ids: [], assignee_ids: [], update_task: [:index, :checked, :line_number, :line_source] }]
   end
 
+  def reorder_params
+    params.permit(:move_before_id, :move_after_id, :group_full_path)
+  end
+
   def store_uri
     if request.get? && !request.xhr?
       store_location_for :user, request.fullpath

app/services/boards/issues/move_service.rb

@@ -79,9 +79,11 @@ module Boards
       # rubocop: enable CodeReuse/ActiveRecord
 
       def move_between_ids
-        return unless params[:move_after_id] || params[:move_before_id]
+        ids = [params[:move_after_id], params[:move_before_id]]
+                .map(&:to_i)
+                .map { |m| m.positive? ? m : nil }
 
-        [params[:move_after_id], params[:move_before_id]]
+        ids.any? ? ids : nil
       end
     end
   end

app/services/issues/reorder_service.rb

+# frozen_string_literal: true
+
+module Issues
+  class ReorderService < Issues::BaseService
+    def execute(issue)
+      return false unless can?(current_user, :update_issue, issue)
+      return false if group && !can?(current_user, :read_group, group)
+
+      attrs = issue_params(group)
+      return false if attrs.empty?
+
+      update(issue, attrs)
+    end
+
+    private
+
+    def group
+      return unless params[:group_full_path]
+
+      @group ||= Group.find_by_full_path(params[:group_full_path])
+    end
+
+    def update(issue, attrs)
+      ::Issues::UpdateService.new(project, current_user, attrs).execute(issue)
+    rescue ActiveRecord::RecordNotFound
+      false
+    end
+
+    def issue_params(group)
+      attrs = {}
+
+      if move_between_ids
+        attrs[:move_between_ids] = move_between_ids
+        attrs[:board_group_id]   = group&.id
+      end
+
+      attrs
+    end
+
+    def move_between_ids
+      ids = [params[:move_after_id], params[:move_before_id]]
+              .map(&:to_i)
+              .map { |m| m.positive? ? m : nil }
+
+      ids.any? ? ids : nil
+    end
+  end
+end

app/services/issues/update_service.rb

@@ -76,6 +76,7 @@ module Issues
 
       issue_before = get_issue_if_allowed(before_id, board_group_id)
       issue_after = get_issue_if_allowed(after_id, board_group_id)
+      raise ActiveRecord::RecordNotFound unless issue_before || issue_after
 
       issue.move_between(issue_before, issue_after)
     end

config/routes/project.rb

@@ -492,6 +492,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
           post :toggle_subscription
           post :mark_as_spam
           post :move
+          put :reorder
           get :related_branches
           get :can_create_branch
           get :realtime_changes

lib/gitlab/ci/ansi2html.rb

@@ -200,7 +200,9 @@ module Gitlab
             css_classes = %w[section line] + sections.map { |section| "s_#{section}" }
           end
 
-          write_in_tag %{<br/>}
+          ensure_open_new_tag
+          write_raw %{<br/>}
+          close_open_tags
           write_raw %{<span class="#{css_classes.join(' ')}"></span>} if css_classes.any?
           @lineno_in_section += 1
           open_new_tag

spec/controllers/projects/issues_controller_spec.rb

@@ -320,6 +320,90 @@ describe Projects::IssuesController do
     end
   end
 
+  describe 'PUT #reorder' do
+    let(:group)   { create(:group, projects: [project]) }
+    let!(:issue1) { create(:issue, project: project, relative_position: 10) }
+    let!(:issue2) { create(:issue, project: project, relative_position: 20) }
+    let!(:issue3) { create(:issue, project: project, relative_position: 30) }
+
+    before do
+      sign_in(user)
+    end
+
+    context 'when user has access' do
+      before do
+        project.add_developer(user)
+      end
+
+      context 'with valid params' do
+        it 'reorders issues and returns a successful 200 response' do
+          reorder_issue(issue1,
+            move_after_id: issue2.id,
+            move_before_id: issue3.id,
+            group_full_path: group.full_path)
+
+          [issue1, issue2, issue3].map(&:reload)
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(issue1.relative_position)
+            .to be_between(issue2.relative_position, issue3.relative_position)
+        end
+      end
+
+      context 'with invalid params' do
+        it 'returns a unprocessable entity 422 response for invalid move ids' do
+          reorder_issue(issue1, move_after_id: 99, move_before_id: 999)
+
+          expect(response).to have_gitlab_http_status(422)
+        end
+
+        it 'returns a not found 404 response for invalid issue id' do
+          reorder_issue(object_double(issue1, iid: 999),
+            move_after_id: issue2.id,
+            move_before_id: issue3.id)
+
+          expect(response).to have_gitlab_http_status(404)
+        end
+
+        it 'returns a unprocessable entity 422 response for issues not in group' do
+          another_group = create(:group)
+
+          reorder_issue(issue1,
+            move_after_id: issue2.id,
+            move_before_id: issue3.id,
+            group_full_path: another_group.full_path)
+
+          expect(response).to have_gitlab_http_status(422)
+        end
+      end
+    end
+
+    context 'with unauthorized user' do
+      before do
+        project.add_guest(user)
+      end
+
+      it 'responds with 404' do
+        reorder_issue(issue1, move_after_id: issue2.id, move_before_id: issue3.id)
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    def reorder_issue(issue, move_after_id: nil, move_before_id: nil, group_full_path: nil)
+      put :reorder,
+           params: {
+               namespace_id: project.namespace.to_param,
+               project_id: project,
+               id: issue.iid,
+               move_after_id: move_after_id,
+               move_before_id: move_before_id,
+               group_full_path: group_full_path
+           },
+           format: :json
+    end
+  end
+
   describe 'PUT #update' do
     subject do
       put :update,

spec/lib/gitlab/ci/ansi2html_spec.rb

@@ -141,11 +141,11 @@ describe Gitlab::Ci::Ansi2html do
   end
 
   it "replaces newlines with line break tags" do
-    expect(convert_html("\n")).to eq('<span class=""><br/><span class=""></span></span>')
+    expect(convert_html("\n")).to eq('<span class=""></span><br/><span class=""></span>')
   end
 
   it "groups carriage returns with newlines" do
-    expect(convert_html("\r\n")).to eq('<span class=""><br/><span class=""></span></span>')
+    expect(convert_html("\r\n")).to eq('<span class=""></span><br/><span class=""></span>')
   end
 
   describe "incremental update" do
@@ -193,7 +193,7 @@ describe Gitlab::Ci::Ansi2html do
       let(:pre_text) { "Hello\r" }
       let(:pre_html) { "<span class=\"\">Hello\r</span>" }
       let(:text) { "\nWorld" }
-      let(:html) { "<span class=\"\"><br/><span class=\"\">World</span></span>" }
+      let(:html) { "<span class=\"\"></span><br/><span class=\"\">World</span>" }
 
       it_behaves_like 'stateable converter'
     end
@@ -232,7 +232,7 @@ describe Gitlab::Ci::Ansi2html do
       it 'prints light red' do
         text = "#{section_start}\e[91mHello\e[0m\n#{section_end}"
         header = %{<span class="term-fg-l-red section js-section-header section-header js-s-#{class_name(section_name)}">Hello</span>}
-        line_break = %{<span class="section js-section-header section-header js-s-#{class_name(section_name)}"><br/></span>}
+        line_break = %{<span class="section js-section-header section-header js-s-#{class_name(section_name)}"></span><br/>}
         line = %{<span class="section line s_#{class_name(section_name)}"></span>}
         empty_line = %{<span class="section js-s-#{class_name(section_name)}"></span>}
         html = "#{section_start_html}#{header}#{line_break}#{line}#{empty_line}#{section_end_html}"

spec/lib/gitlab/ci/trace/stream_spec.rb

@@ -65,9 +65,9 @@ describe Gitlab::Ci::Trace::Stream, :clean_gitlab_redis_cache do
           result = stream.html
 
           expect(result).to eq(
-            "<span class=\"\">ヾ(´༎ຶД༎ຶ`)ﾉ<br/><span class=\"\"></span></span>"\
-            "<span class=\"term-fg-green\">許功蓋</span><span class=\"\"><br/>"\
-            "<span class=\"\"></span></span>")
+            "<span class=\"\">ヾ(´༎ຶД༎ຶ`)ﾉ</span><br/><span class=\"\"></span>"\
+            "<span class=\"term-fg-green\">許功蓋</span><span class=\"\"></span><br/>"\
+            "<span class=\"\"></span>")
           expect(result.encoding).to eq(Encoding.default_external)
         end
       end
@@ -306,8 +306,8 @@ describe Gitlab::Ci::Trace::Stream, :clean_gitlab_redis_cache do
     shared_examples_for 'htmls' do
       it "returns html" do
         expect(stream.html).to eq(
-          "<span class=\"\">12<br/><span class=\"\">34<br/>"\
-          "<span class=\"\">56</span></span></span>")
+          "<span class=\"\">12</span><br/><span class=\"\">34</span><br/>"\
+          "<span class=\"\">56</span>")
       end
 
       it "returns html for last line only" do

spec/lib/gitlab/highlight_spec.rb

@@ -18,9 +18,10 @@ describe Gitlab::Highlight do
   end
 
   describe '#highlight' do
+    let(:plain_text_file_name) { "test.txt" }
+    let(:plain_text_content) { "plain text contents" }
     let(:file_name) { 'test.lisp' }
-    let(:no_context_content) { ":type \"assem\"))" }
-    let(:content) { "(make-pathname :defaults name\n#{no_context_content}" }
+    let(:content) { "(make-pathname :defaults name\n:type \"assem\")" }
     let(:multiline_content) do
       %q(
       def test(input):
@@ -32,22 +33,22 @@ describe Gitlab::Highlight do
 
     it 'highlights' do
       expected = %Q[<span id="LC1" class="line" lang="common_lisp"><span class="p">(</span><span class="nb">make-pathname</span> <span class="ss">:defaults</span> <span class="nv">name</span></span>
-<span id="LC2" class="line" lang="common_lisp"><span class="ss">:type</span> <span class="s">"assem"</span><span class="p">))</span></span>]
+<span id="LC2" class="line" lang="common_lisp"><span class="ss">:type</span> <span class="s">"assem"</span><span class="p">)</span></span>]
 
       expect(described_class.highlight(file_name, content)).to eq(expected)
     end
 
     it 'returns plain version for unknown lexer context' do
-      result = described_class.highlight(file_name, no_context_content)
+      result = described_class.highlight(plain_text_file_name, plain_text_content)
 
-      expect(result).to eq(%[<span id="LC1" class="line" lang="">:type "assem"))</span>])
+      expect(result).to eq(%[<span id="LC1" class="line" lang="plaintext">plain text contents</span>])
     end
 
     it 'returns plain version for long content' do
       stub_const('Gitlab::Highlight::MAXIMUM_TEXT_HIGHLIGHT_SIZE', 1)
       result = described_class.highlight(file_name, content)
 
-      expect(result).to eq(%[<span id="LC1" class="line" lang="">(make-pathname :defaults name</span>\n<span id="LC2" class="line" lang="">:type "assem"))</span>])
+      expect(result).to eq(%[<span id="LC1" class="line" lang="">(make-pathname :defaults name</span>\n<span id="LC2" class="line" lang="">:type "assem")</span>])
     end
 
     it 'highlights multi-line comments' do

spec/services/issues/reorder_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Issues::ReorderService do
+  set(:user)    { create(:user) }
+  set(:project) { create(:project) }
+  set(:group)   { create(:group) }
+
+  shared_examples 'issues reorder service' do
+    context 'when reordering issues' do
+      it 'returns false with no params' do
+        expect(service({}).execute(issue1)).to be_falsey
+      end
+
+      it 'returns false with both invalid params' do
+        params = { move_after_id: nil, move_before_id: 1 }
+
+        expect(service(params).execute(issue1)).to be_falsey
+      end
+
+      it 'sorts issues' do
+        params = { move_after_id: issue2.id, move_before_id: issue3.id }
+
+        service(params).execute(issue1)
+
+        expect(issue1.relative_position)
+          .to be_between(issue2.relative_position, issue3.relative_position)
+      end
+    end
+  end
+
+  describe '#execute' do
+    let(:issue1) { create(:issue, project: project, relative_position: 10) }
+    let(:issue2) { create(:issue, project: project, relative_position: 20) }
+    let(:issue3) { create(:issue, project: project, relative_position: 30) }
+
+    context 'when ordering issues in a project' do
+      let(:parent) { project }
+
+      before do
+        parent.add_developer(user)
+      end
+
+      it_behaves_like 'issues reorder service'
+    end
+
+    context 'when ordering issues in a group' do
+      let(:project) { create(:project, namespace: group) }
+
+      before do
+        group.add_developer(user)
+      end
+
+      it_behaves_like 'issues reorder service'
+
+      context 'when ordering in a group issue list' do
+        let(:params) { { move_after_id: issue2.id, move_before_id: issue3.id, group_full_path: group.full_path } }
+
+        subject { service(params) }
+
+        it 'sends the board_group_id parameter' do
+          match_params = { move_between_ids: [issue2.id, issue3.id], board_group_id: group.id }
+
+          expect(Issues::UpdateService)
+            .to receive(:new).with(project, user, match_params)
+            .and_return(double(execute: build(:issue)))
+
+          subject.execute(issue1)
+        end
+
+        it 'sorts issues' do
+          project2 = create(:project, namespace: group)
+          issue4   = create(:issue, project: project2)
+
+          subject.execute(issue4)
+
+          expect(issue4.relative_position)
+            .to be_between(issue2.relative_position, issue3.relative_position)
+        end
+      end
+    end
+  end
+
+  def service(params)
+    described_class.new(project, user, params)
+  end
+end

spec/services/issues/update_service_spec.rb

@@ -687,6 +687,22 @@ describe Issues::UpdateService, :mailer do
       end
     end
 
+    context 'when moving an issue ', :nested_groups do
+      it 'raises an error for invalid move ids within a project' do
+        opts = { move_between_ids: [9000, 9999] }
+
+        expect { described_class.new(issue.project, user, opts).execute(issue) }
+            .to raise_error(ActiveRecord::RecordNotFound)
+      end
+
+      it 'raises an error for invalid move ids within a group' do
+        opts = { move_between_ids: [9000, 9999], board_group_id: create(:group).id }
+
+        expect { described_class.new(issue.project, user, opts).execute(issue) }
+            .to raise_error(ActiveRecord::RecordNotFound)
+      end
+    end
+
     include_examples 'issuable update service' do
       let(:open_issuable) { issue }
       let(:closed_issuable) { create(:closed_issue, project: project) }

spec/support/shared_examples/ci_trace_shared_examples.rb

@@ -5,7 +5,7 @@ shared_examples_for 'common trace features' do
     end
 
     it "returns formatted html" do
-      expect(trace.html).to eq("<span class=\"\">12<br/><span class=\"\">34</span></span>")
+      expect(trace.html).to eq("<span class=\"\">12</span><br/><span class=\"\">34</span>")
     end
 
     it "returns last line of formatted html" do