From 5f25cdfe19c7c0a8c1ada592307e9017e2a754e1 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Mon, 14 Apr 2014 20:12:07 -0500 Subject: [PATCH] Implement Merge Request Labels --- .../stylesheets/sections/merge_requests.scss | 8 +++ app/controllers/application_controller.rb | 5 ++ app/controllers/projects/labels_controller.rb | 13 +++-- app/models/merge_request.rb | 7 ++- app/models/project.rb | 5 +- app/views/projects/issues/index.html.haml | 3 +- .../projects/merge_requests/_form.html.haml | 38 ++++++++++++++ .../merge_requests/_merge_request.html.haml | 6 +++ .../projects/merge_requests/_show.html.haml | 1 + .../projects/merge_requests/index.html.haml | 3 +- .../show/_participants.html.haml | 11 ++++ app/views/shared/_project_filter.html.haml | 2 +- lib/api/entities.rb | 1 + lib/api/merge_requests.rb | 4 ++ spec/requests/api/projects_spec.rb | 52 ++++++++++++++++--- 15 files changed, 142 insertions(+), 17 deletions(-) create mode 100644 app/views/projects/merge_requests/show/_participants.html.haml diff --git a/app/assets/stylesheets/sections/merge_requests.scss b/app/assets/stylesheets/sections/merge_requests.scss index 790496a1a5a..82a8ff48333 100644 --- a/app/assets/stylesheets/sections/merge_requests.scss +++ b/app/assets/stylesheets/sections/merge_requests.scss @@ -74,6 +74,10 @@ .merge-request-info { color: #999; + + .merge-request-labels { + display: inline-block; + } } } } @@ -112,3 +116,7 @@ } } } + +.merge-request-show-labels .label { + padding: 6px 10px; +} diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index a3f39c23e08..2730e9942ec 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -117,6 +117,11 @@ def authorize_push! 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 diff --git a/app/controllers/projects/labels_controller.rb b/app/controllers/projects/labels_controller.rb index 0166ca9ff00..b037cf56502 100644 --- a/app/controllers/projects/labels_controller.rb +++ b/app/controllers/projects/labels_controller.rb @@ -1,8 +1,7 @@ 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 @@ def index 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 diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 8c885b70a48..a55f3a61398 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -36,7 +36,9 @@ class MergeRequest < 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 < 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 diff --git a/app/models/project.rb b/app/models/project.rb index 7ddcc73cf2a..45e950f4807 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -281,8 +281,11 @@ def project_id 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) diff --git a/app/views/projects/issues/index.html.haml b/app/views/projects/issues/index.html.haml index 5e899d412c6..51a8c911af8 100644 --- a/app/views/projects/issues/index.html.haml +++ b/app/views/projects/issues/index.html.haml @@ -1,6 +1,7 @@ = 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" diff --git a/app/views/projects/merge_requests/_form.html.haml b/app/views/projects/merge_requests/_form.html.haml index 0fe2d1d9801..e385fba6afb 100644 --- a/app/views/projects/merge_requests/_form.html.haml +++ b/app/views/projects/merge_requests/_form.html.haml @@ -52,6 +52,15 @@ = f.text_area :description, class: "form-control js-gfm-input", rows: 14 %p.hint Description is parsed with #{link_to "GitLab Flavored Markdown", help_markdown_path, target: '_blank'}. + - 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" @@ -83,3 +92,32 @@ target_branch.on("change", function() { $.get("#{branch_to_project_merge_requests_path(@source_project)}", {target_project_id: target_project.val(),ref: $(this).val() }); }); + + $("#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; + } + }); diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml index 980ac126742..25cf489b688 100644 --- a/app/views/projects/merge_requests/_merge_request.html.haml +++ b/app/views/projects/merge_requests/_merge_request.html.haml @@ -35,3 +35,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 diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml index e36a48f62cf..193c600f774 100644 --- a/app/views/projects/merge_requests/_show.html.haml +++ b/app/views/projects/merge_requests/_show.html.haml @@ -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 diff --git a/app/views/projects/merge_requests/index.html.haml b/app/views/projects/merge_requests/index.html.haml index 34faebf619c..12a72edb224 100644 --- a/app/views/projects/merge_requests/index.html.haml +++ b/app/views/projects/merge_requests/index.html.haml @@ -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 diff --git a/app/views/projects/merge_requests/show/_participants.html.haml b/app/views/projects/merge_requests/show/_participants.html.haml new file mode 100644 index 00000000000..0dabd965e52 --- /dev/null +++ b/app/views/projects/merge_requests/show/_participants.html.haml @@ -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 +   diff --git a/app/views/shared/_project_filter.html.haml b/app/views/shared/_project_filter.html.haml index d82b08eeaa2..7936a038be3 100644 --- a/app/views/shared/_project_filter.html.haml +++ b/app/views/shared/_project_filter.html.haml @@ -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? diff --git a/lib/api/entities.rb b/lib/api/entities.rb index abe6fceff14..1fd29acefe1 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -135,6 +135,7 @@ class MergeRequest < ProjectEntity 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 diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 4b88b0f84c1..5fac2a3ea19 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -67,6 +67,7 @@ def handle_merge_request_errors!(errors) # 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 @@ def handle_merge_request_errors!(errors) 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 @@ def handle_merge_request_errors!(errors) # 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) diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 4b8f41a4683..81e6abbb0d7 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -14,6 +14,12 @@ 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 @@ 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 -- GitLab