Merge pull request #6778 from dblessing/feature/mr_labels

Merge Request Labels

Merge pull request #6778 from dblessing/feature/mr_labels
Merge Request Labels
Dmitriy Zaporozhets
2 parents 0c621708 5f25cdfe
Showing 15 changed files with 142 additions and 17 deletions Show diff stats
app/assets/stylesheets/sections/merge_requests.scss
app/controllers/application_controller.rb
app/controllers/projects/labels_controller.rb
app/models/merge_request.rb
app/models/project.rb
app/views/projects/issues/index.html.haml
app/views/projects/merge_requests/_form.html.haml
app/views/projects/merge_requests/_merge_request.html.haml
app/views/projects/merge_requests/_show.html.haml
app/views/projects/merge_requests/index.html.haml
app/views/projects/merge_requests/show/_participants.html.haml
app/views/shared/_project_filter.html.haml
lib/api/entities.rb
lib/api/merge_requests.rb
spec/requests/api/projects_spec.rb
@@ -73,6 +73,10 @@
  
     .merge-request-info {
       color: #999;
+
+      .merge-request-labels {
+        display: inline-block;
+      }
     }
   }
 }
@@ -111,3 +115,7 @@
     }
   }
 }
+
+.merge-request-show-labels .label {
+  padding: 6px 10px;
+}
@@ -117,6 +117,11 @@ class ApplicationController &lt; ActionController::Base
     return access_denied! unless can?(current_user, :push_code, project)
   end
  
+  def authorize_labels!
+    # Labels should be accessible for issues and/or merge requests
+    authorize_read_issue! || authorize_read_merge_request!
+  end
+
   def access_denied!
     render "errors/access_denied", layout: "errors", status: 404
   end
 class Projects::LabelsController < Projects::ApplicationController
   before_filter :module_enabled
  
-  # Allow read any issue
-  before_filter :authorize_read_issue!
+  before_filter :authorize_labels!
  
   respond_to :js, :html
  
@@ -13,12 +12,18 @@ class Projects::LabelsController &lt; Projects::ApplicationController
   def generate
     Gitlab::IssuesLabels.generate(@project)
  
-    redirect_to project_issues_path(@project)
+    if params[:redirect] == 'issues'
+      redirect_to project_issues_path(@project)
+    elsif params[:redirect] == 'merge_requests'
+      redirect_to project_merge_requests_path(@project)
+    end
   end
  
   protected
  
   def module_enabled
-    return render_404 unless @project.issues_enabled
+    unless @project.issues_enabled || @project.merge_requests_enabled
+      return render_404
+    end
   end
 end
@@ -36,7 +36,9 @@ class MergeRequest &lt; ActiveRecord::Base
  
   delegate :commits, :diffs, :last_commit, :last_commit_short_sha, to: :merge_request_diff, prefix: nil
  
-  attr_accessible :title, :assignee_id, :source_project_id, :source_branch, :target_project_id, :target_branch, :milestone_id, :state_event, :description
+  attr_accessible :title, :assignee_id, :source_project_id, :source_branch,
+                  :target_project_id, :target_branch, :milestone_id,
+                  :state_event, :description, :label_list
  
   attr_accessor :should_remove_source_branch
  
@@ -44,6 +46,9 @@ class MergeRequest &lt; ActiveRecord::Base
   # It allows us to close or modify broken merge requests
   attr_accessor :allow_broken
  
+  ActsAsTaggableOn.strict_case_match = true
+  acts_as_taggable_on :labels
+
   state_machine :state, initial: :opened do
     event :close do
       transition [:reopened, :opened] => :closed
@@ -281,8 +281,11 @@ class Project &lt; ActiveRecord::Base
     self.id
   end
  
+  # Tags are shared by issues and merge requests
   def issues_labels
-    @issues_labels ||= (issues_default_labels + issues.tags_on(:labels)).uniq.sort_by(&:name)
+    @issues_labels ||= (issues_default_labels +
+                        merge_requests.tags_on(:labels) +
+                        issues.tags_on(:labels)).uniq.sort_by(&:name)
   end
  
   def issue_exists?(issue_id)
 = render "head"
 .row
   .col-md-3
-    = render 'shared/project_filter', project_entities_path: project_issues_path(@project), labels: true
+    = render 'shared/project_filter', project_entities_path: project_issues_path(@project),
+             labels: true, redirect: 'issues'
   .col-md-9.issues-holder
     = render "issues"
@@ -42,6 +42,15 @@
         .col-sm-10= f.select(:milestone_id, milestone_options(@merge_request), { include_blank: "Select milestone" }, {class: 'select2'})
  
  
+    - if @merge_request.persisted? # Only allow labels on edit to avoid fork vs upstream repo labels issue
+      .form-group
+        = f.label :label_list, class: 'control-label' do
+          %i.icon-tag
+          Labels
+        .col-sm-10
+          = f.text_field :label_list, maxlength: 2000, class: "form-control"
+          %p.hint Separate labels with commas.
+
   .form-actions
     - if @merge_request.new_record?
       = f.submit 'Submit merge request', class: "btn btn-create"
@@ -60,3 +69,32 @@
     $('#merge_request_assignee_id').val("#{current_user.id}").trigger("change");
     e.preventDefault();
   });
+
+  $("#merge_request_label_list")
+    .bind( "keydown", function( event ) {
+      if ( event.keyCode === $.ui.keyCode.TAB &&
+        $( this ).data( "autocomplete" ).menu.active ) {
+        event.preventDefault();
+      }
+    })
+    .bind("click", function(event) {
+      $(this).autocomplete("search", "");
+    })
+    .autocomplete({
+      minLength: 0,
+      source: function( request, response ) {
+        response( $.ui.autocomplete.filter(
+          #{raw labels_autocomplete_source}, extractLast( request.term ) ) );
+      },
+      focus: function() {
+        return false;
+      },
+      select: function(event, ui) {
+        var terms = split( this.value );
+        terms.pop();
+        terms.push( ui.item.value );
+        terms.push( "" );
+        this.value = terms.join( ", " );
+        return false;
+      }
+    });
@@ -31,3 +31,9 @@
  
     .pull-right
       %small updated #{time_ago_with_tooltip(merge_request.updated_at, 'bottom', 'merge_request_updated_ago')}
+
+    .merge-request-labels
+      - merge_request.labels.each do |label|
+        %span{class: "label #{label_css_class(label.name)}"}
+          %i.icon-tag
+          = label.name
@@ -4,6 +4,7 @@
   = render "projects/merge_requests/show/mr_box"
   = render "projects/merge_requests/show/state_widget"
   = render "projects/merge_requests/show/commits"
+  = render "projects/merge_requests/show/participants"
  
   - if @commits.present?
     %ul.nav.nav-tabs
@@ -8,7 +8,8 @@
 %hr
 .row
   .col-md-3
-    = render 'shared/project_filter', project_entities_path: project_merge_requests_path(@project)
+    = render 'shared/project_filter', project_entities_path: project_merge_requests_path(@project),
+             labels: true, redirect: 'merge_requests'
   .col-md-9
     .mr-filters.append-bottom-10
       .dropdown.inline
@@ -0,0 +1,11 @@
+.participants
+  %cite.cgray #{@merge_request.participants.count} participants
+  - @merge_request.participants.each do |participant|
+    = link_to_member(@project, participant, name: false, size: 24)
+
+  .merge-request-show-labels.pull-right
+    - @merge_request.labels.each do |label|
+      %span{class: "label #{label_css_class(label.name)}"}
+        %i.icon-tag
+        = label.name
+      &nbsp;
@@ -44,7 +44,7 @@
             .light-well
               Add first label to your issues
               %br
-              or #{link_to 'generate', generate_project_labels_path(@project), method: :post} default set of labels
+              or #{link_to 'generate', generate_project_labels_path(@project, redirect: redirect), method: :post} default set of labels
  
     %fieldset
       - if %w(state scope milestone_id assignee_id label_name).select { |k| params[k].present? }.any?
@@ -136,6 +136,7 @@ module API
       expose :target_branch, :source_branch, :upvotes, :downvotes
       expose :author, :assignee, using: Entities::UserBasic
       expose :source_project_id, :target_project_id
+      expose :label_list, as: :labels
     end
  
     class SSHKey < Grape::Entity
@@ -67,6 +67,7 @@ module API
       #   assignee_id              - Assignee user ID
       #   title (required)         - Title of MR
       #   description              - Description of MR
+      #   labels (optional)        - Labels for MR as a comma-separated list
       #
       # Example:
       #   POST /projects/:id/merge_requests
@@ -75,6 +76,7 @@ module API
         authorize! :write_merge_request, user_project
         required_attributes! [:source_branch, :target_branch, :title]
         attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :target_project_id, :description]
+        attrs[:label_list] = params[:labels] if params[:labels].present?
         merge_request = ::MergeRequests::CreateService.new(user_project, current_user, attrs).execute
  
         if merge_request.valid?
@@ -95,11 +97,13 @@ module API
       #   title                       - Title of MR
       #   state_event                 - Status of MR. (close|reopen|merge)
       #   description                 - Description of MR
+      #   labels (optional)           - Labels for a MR as a comma-separated list
       # Example:
       #   PUT /projects/:id/merge_request/:merge_request_id
       #
       put ":id/merge_request/:merge_request_id" do
         attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :state_event, :description]
+        attrs[:label_list] = params[:labels] if params[:labels].present?
         merge_request = user_project.merge_requests.find(params[:merge_request_id])
         authorize! :modify_merge_request, merge_request
         merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, attrs).execute(merge_request)
@@ -14,6 +14,12 @@ describe API::API, api: true  do
   let(:users_project) { create(:users_project, user: user, project: project, project_access: UsersProject::MASTER) }
   let(:users_project2) { create(:users_project, user: user3, project: project, project_access: UsersProject::DEVELOPER) }
   let(:issue_with_labels) { create(:issue, author: user, assignee: user, project: project, :label_list => "label1, label2") }
+  let(:merge_request_with_labels) do
+    create(:merge_request, :simple, author: user, assignee: user,
+           source_project: project, target_project: project, title: 'Test',
+           label_list: 'label3, label4')
+  end
+
  
   describe "GET /projects" do
     before { project }
@@ -634,15 +640,45 @@ describe API::API, api: true  do
     end
   end
  
-   describe "GET /projects/:id/labels" do
-    before { issue_with_labels }
+  describe 'GET /projects/:id/labels' do
+    context 'with an issue' do
+      before { issue_with_labels }
  
-    it "should return project labels" do
-      get api("/projects/#{project.id}/labels", user)
-      response.status.should == 200
-      json_response.should be_an Array
-      json_response.first['name'].should == issue_with_labels.labels.first.name
-      json_response.last['name'].should == issue_with_labels.labels.last.name
+      it 'should return project labels' do
+        get api("/projects/#{project.id}/labels", user)
+        response.status.should == 200
+        json_response.should be_an Array
+        json_response.first['name'].should == issue_with_labels.labels.first.name
+        json_response.last['name'].should == issue_with_labels.labels.last.name
+      end
+    end
+
+    context 'with a merge request' do
+      before { merge_request_with_labels }
+
+      it 'should return project labels' do
+        get api("/projects/#{project.id}/labels", user)
+        response.status.should == 200
+        json_response.should be_an Array
+        json_response.first['name'].should == merge_request_with_labels.labels.first.name
+        json_response.last['name'].should == merge_request_with_labels.labels.last.name
+      end
+    end
+
+    context 'with an issue and a merge request' do
+      before do
+        issue_with_labels
+        merge_request_with_labels
+      end
+
+      it 'should return project labels from both' do
+        get api("/projects/#{project.id}/labels", user)
+        response.status.should == 200
+        json_response.should be_an Array
+        all_labels = issue_with_labels.labels.map(&:name).to_a
+                       .concat(merge_request_with_labels.labels.map(&:name).to_a)
+        json_response.map { |e| e['name'] }.should =~ all_labels
+      end
     end
   end
 end
...	...	@@ -73,6 +73,10 @@
73	73
74	74	.merge-request-info {
75	75	color: #999;
	76	+
	77	+ .merge-request-labels {
	78	+ display: inline-block;
	79	+ }
76	80	}
77	81	}
78	82	}
...	...	@@ -111,3 +115,7 @@
111	115	}
112	116	}
113	117	}
	118	+
	119	+.merge-request-show-labels .label {
	120	+ padding: 6px 10px;
	121	+}
...	...
...	...	@@ -117,6 +117,11 @@ class ApplicationController < ActionController::Base
117	117	return access_denied! unless can?(current_user, :push_code, project)
118	118	end
119	119
	120	+ def authorize_labels!
	121	+ # Labels should be accessible for issues and/or merge requests
	122	+ authorize_read_issue! \|\| authorize_read_merge_request!
	123	+ end
	124	+
120	125	def access_denied!
121	126	render "errors/access_denied", layout: "errors", status: 404
122	127	end
...	...
1	1	class Projects::LabelsController < Projects::ApplicationController
2	2	before_filter :module_enabled
3	3
4		- # Allow read any issue
5		- before_filter :authorize_read_issue!
	4	+ before_filter :authorize_labels!
6	5
7	6	respond_to :js, :html
8	7
...	...	@@ -13,12 +12,18 @@ class Projects::LabelsController < Projects::ApplicationController
13	12	def generate
14	13	Gitlab::IssuesLabels.generate(@project)
15	14
16		- redirect_to project_issues_path(@project)
	15	+ if params[:redirect] == 'issues'
	16	+ redirect_to project_issues_path(@project)
	17	+ elsif params[:redirect] == 'merge_requests'
	18	+ redirect_to project_merge_requests_path(@project)
	19	+ end
17	20	end
18	21
19	22	protected
20	23
21	24	def module_enabled
22		- return render_404 unless @project.issues_enabled
	25	+ unless @project.issues_enabled \|\| @project.merge_requests_enabled
	26	+ return render_404
	27	+ end
23	28	end
24	29	end
...	...
...	...	@@ -36,7 +36,9 @@ class MergeRequest < ActiveRecord::Base
36	36
37	37	delegate :commits, :diffs, :last_commit, :last_commit_short_sha, to: :merge_request_diff, prefix: nil
38	38
39		- attr_accessible :title, :assignee_id, :source_project_id, :source_branch, :target_project_id, :target_branch, :milestone_id, :state_event, :description
	39	+ attr_accessible :title, :assignee_id, :source_project_id, :source_branch,
	40	+ :target_project_id, :target_branch, :milestone_id,
	41	+ :state_event, :description, :label_list
40	42
41	43	attr_accessor :should_remove_source_branch
42	44
...	...	@@ -44,6 +46,9 @@ class MergeRequest < ActiveRecord::Base
44	46	# It allows us to close or modify broken merge requests
45	47	attr_accessor :allow_broken
46	48
	49	+ ActsAsTaggableOn.strict_case_match = true
	50	+ acts_as_taggable_on :labels
	51	+
47	52	state_machine :state, initial: :opened do
48	53	event :close do
49	54	transition [:reopened, :opened] => :closed
...	...
...	...	@@ -281,8 +281,11 @@ class Project < ActiveRecord::Base
281	281	self.id
282	282	end
283	283
	284	+ # Tags are shared by issues and merge requests
284	285	def issues_labels
285		- @issues_labels \|\|= (issues_default_labels + issues.tags_on(:labels)).uniq.sort_by(&:name)
	286	+ @issues_labels \|\|= (issues_default_labels +
	287	+ merge_requests.tags_on(:labels) +
	288	+ issues.tags_on(:labels)).uniq.sort_by(&:name)
286	289	end
287	290
288	291	def issue_exists?(issue_id)
...	...
1	1	= render "head"
2	2	.row
3	3	.col-md-3
4		- = render 'shared/project_filter', project_entities_path: project_issues_path(@project), labels: true
	4	+ = render 'shared/project_filter', project_entities_path: project_issues_path(@project),
	5	+ labels: true, redirect: 'issues'
5	6	.col-md-9.issues-holder
6	7	= render "issues"
...	...
...	...	@@ -42,6 +42,15 @@
42	42	.col-sm-10= f.select(:milestone_id, milestone_options(@merge_request), { include_blank: "Select milestone" }, {class: 'select2'})
43	43
44	44
	45	+ - if @merge_request.persisted? # Only allow labels on edit to avoid fork vs upstream repo labels issue
	46	+ .form-group
	47	+ = f.label :label_list, class: 'control-label' do
	48	+ %i.icon-tag
	49	+ Labels
	50	+ .col-sm-10
	51	+ = f.text_field :label_list, maxlength: 2000, class: "form-control"
	52	+ %p.hint Separate labels with commas.
	53	+
45	54	.form-actions
46	55	- if @merge_request.new_record?
47	56	= f.submit 'Submit merge request', class: "btn btn-create"
...	...	@@ -60,3 +69,32 @@
60	69	$('#merge_request_assignee_id').val("#{current_user.id}").trigger("change");
61	70	e.preventDefault();
62	71	});
	72	+
	73	+ $("#merge_request_label_list")
	74	+ .bind( "keydown", function( event ) {
	75	+ if ( event.keyCode === $.ui.keyCode.TAB &&
	76	+ $( this ).data( "autocomplete" ).menu.active ) {
	77	+ event.preventDefault();
	78	+ }
	79	+ })
	80	+ .bind("click", function(event) {
	81	+ $(this).autocomplete("search", "");
	82	+ })
	83	+ .autocomplete({
	84	+ minLength: 0,
	85	+ source: function( request, response ) {
	86	+ response( $.ui.autocomplete.filter(
	87	+ #{raw labels_autocomplete_source}, extractLast( request.term ) ) );
	88	+ },
	89	+ focus: function() {
	90	+ return false;
	91	+ },
	92	+ select: function(event, ui) {
	93	+ var terms = split( this.value );
	94	+ terms.pop();
	95	+ terms.push( ui.item.value );
	96	+ terms.push( "" );
	97	+ this.value = terms.join( ", " );
	98	+ return false;
	99	+ }
	100	+ });
...	...
...	...	@@ -31,3 +31,9 @@
31	31
32	32	.pull-right
33	33	%small updated #{time_ago_with_tooltip(merge_request.updated_at, 'bottom', 'merge_request_updated_ago')}
	34	+
	35	+ .merge-request-labels
	36	+ - merge_request.labels.each do \|label\|
	37	+ %span{class: "label #{label_css_class(label.name)}"}
	38	+ %i.icon-tag
	39	+ = label.name
...	...
...	...	@@ -4,6 +4,7 @@
4	4	= render "projects/merge_requests/show/mr_box"
5	5	= render "projects/merge_requests/show/state_widget"
6	6	= render "projects/merge_requests/show/commits"
	7	+ = render "projects/merge_requests/show/participants"
7	8
8	9	- if @commits.present?
9	10	%ul.nav.nav-tabs
...	...
...	...	@@ -8,7 +8,8 @@
8	8	%hr
9	9	.row
10	10	.col-md-3
11		- = render 'shared/project_filter', project_entities_path: project_merge_requests_path(@project)
	11	+ = render 'shared/project_filter', project_entities_path: project_merge_requests_path(@project),
	12	+ labels: true, redirect: 'merge_requests'
12	13	.col-md-9
13	14	.mr-filters.append-bottom-10
14	15	.dropdown.inline
...	...