From e343ba90aa83c487178c437b9a9a2a4c790341bb Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Wed, 19 Jun 2013 01:45:40 +0600 Subject: [PATCH 01/17] [#105] pulls api v1 --- .../api/v1/pull_requests_controller.rb | 151 ++++++++++++++++++ app/models/pull_request.rb | 2 + .../api/v1/pull_requests/_pull.json.jbuilder | 24 +++ .../api/v1/pull_requests/index.json.jbuilder | 5 + .../api/v1/pull_requests/show.json.jbuilder | 13 ++ config/routes.rb | 4 + 6 files changed, 199 insertions(+) create mode 100644 app/controllers/api/v1/pull_requests_controller.rb create mode 100644 app/views/api/v1/pull_requests/_pull.json.jbuilder create mode 100644 app/views/api/v1/pull_requests/index.json.jbuilder create mode 100644 app/views/api/v1/pull_requests/show.json.jbuilder diff --git a/app/controllers/api/v1/pull_requests_controller.rb b/app/controllers/api/v1/pull_requests_controller.rb new file mode 100644 index 000000000..df9dcf147 --- /dev/null +++ b/app/controllers/api/v1/pull_requests_controller.rb @@ -0,0 +1,151 @@ +# -*- encoding : utf-8 -*- +class Api::V1::PullRequestsController < Api::V1::BaseController + respond_to :json + + before_filter :authenticate_user! + skip_before_filter :authenticate_user!, :only => [:show] if APP_CONFIG['anonymous_access'] + + load_resource :group, :only => :group_index, :find_by => :id, :parent => false + load_and_authorize_resource :project + load_resource :issue, :through => :project, :find_by => :serial_id, :parent => false, :only => [:show, :index] + load_and_authorize_resource :instance_name => :pull, :through => :issue, :singleton => true, :only => [:show, :index] + + def index + @pull_requests = @project.pull_requests + @pulls_url = api_v1_project_pull_requests_path(@project, :format => :json) + render_pulls_list + end + + def all_index + project_ids = get_all_project_ids Project.accessible_by(current_ability, :membered).uniq.pluck(:id) + @pulls = PullRequest.where('pull_requests.to_project_id IN (?)', project_ids) + @pulls_url = api_v1_pull_requests_path :format => :json + render_pulls_list + end + + def user_index + project_ids = get_all_project_ids current_user.projects.select('distinct projects.id').pluck(:id) + @pulls = PullRequest.where('pull_requests.to_project_id IN (?)', project_ids) + @pulls_url = pull_requests_api_v1_user_path :format => :json + render_pulls_list + end + + def group_index + project_ids = @group.projects.select('distinct projects.id').pluck(:id) + project_ids = Project.accessible_by(current_ability, :membered).where(:id => project_ids).uniq.pluck(:id) + @pulls = PullRequest.where(:to_project_id => project_ids) + @pulls_url = pull_requests_api_v1_group_path + render_pulls_list + end + + def show + end + + def create + from_project = Project.find params[:pull_request].try('[]', :from_project) + authorize! :read, from_project + + @pull = @project.pull_requests.new + @pull.build_issue :title => pull_params[:title], :body => pull_params[:body] + @pull.from_project = @project + @pull.to_ref, @pull.from_ref = pull_params[:to_ref], pull_params[:from_ref] + @pull.issue.assignee_id = pull_params[:assignee] if can?(:write, @project) + @pull.issue.user, @pull.issue.project = current_user, @project + + render_validation_error(@pull, "#{@pull.class.name} has not been created") && return unless @pull.valid? + + @pull.save # set pull id + @pull.check(false) # don't make event transaction + if @pull.already? + @pull.destroy + error_message = I18n.t('projects.pull_requests.up_to_date', :to_ref => @pull.to_ref, :from_ref => @pull.from_ref) + render_json_response(@pull, error_message, 422) + else + @pull.send(@pull.status == 'blocked' ? 'block' : @pull.status) + render_json_response @pull, "#{@pull.class.name} has been created successfully" + end + end + + def update + @pull = @project.pull_requests.includes(:issue).where(:issues => {:serial_id => params[:id]}).first + authorize! :update, @pull + + if pull_params.present? + attrs = pull_params.slice(:title, :body) + attrs.merge(:assignee_id => pull_params[:assignee_id]) if can?(:write, @project) + + if (action = pull_params[:status]) && %w(close reopen).include?(pull_params[:status]) + if @pull.send("can_#{action}?") + @pull.set_user_and_time current_user + need_check = true if action == 'reopen' + end + end + end + + class_name = @pull.class.name + if @pull.issue.update_attributes(attrs) + @pull.send(action) if action.present? + @pull.check if need_check + render_json_response @pull, "#{class_name} has been updated successfully" + else + render_validation_error @pull, "#{class_name} has not been updated" + end + end + + private + + def render_pulls_list + @pulls = @pulls.includes(:issue => [:user, :assignee]) + if params[:status] == 'closed' + @pulls = @pulls.closed_or_merged + else + @pulls = @pulls.not_closed_or_merged + end + + if action_name == 'index' && params[:assignee].present? + case params[:assignee] + when 'none' + @pulls = @pulls.where('issues.assigned_id IS NULL') + when '*' + @pulls = @pulls.where('issues.assigned_id IS NOT NULL') + else + @pulls = @pulls.where('assignees_issues.uname = ?', params[:assignee]) + end + end + + if %w[all_index user_index group_index].include?(action_name) + case params[:filter] + when 'created' + @pulls = @pulls.where('issues.user_id = ?', current_user.id) + when 'all' + else + @pulls = @pulls.where('issues.assignee_id = ?', current_user.id) + end + else + @pulls.where('users.uname = ?', params[:creator]) if params[:creator].present? + end + + sort = params[:sort] == 'updated' ? 'issues.updated_at' : 'issues.created_at' + direction = params[:direction] == 'asc' ? 'ASC' : 'DESC' + @pulls = @pulls.order("#{sort} #{direction}") + + @pulls = @pulls.where('issues.created_at >= to_timestamp(?)', params[:since]) if params[:since] =~ /\A\d+\z/ + @pulls.paginate(paginate_params) + render :index + end + + def get_all_project_ids default_project_ids + project_ids = [] + if ['created', 'all'].include? params[:filter] + # add own pulls + project_ids = Project.accessible_by(current_ability, :show).joins(:issues). + where(:issues => {:user_id => current_user.id}).uniq.pluck('projects.id') + end + project_ids |= default_project_ids + end + + + def pull_params + @pull_params ||= params[:pull_request] || {} + end +end diff --git a/app/models/pull_request.rb b/app/models/pull_request.rb index f47e05880..858715ea6 100644 --- a/app/models/pull_request.rb +++ b/app/models/pull_request.rb @@ -20,6 +20,8 @@ class PullRequest < ActiveRecord::Base attr_accessible :issue_attributes, :to_ref, :from_ref scope :needed_checking, includes(:issue).where(:issues => {:status => ['open', 'blocked', 'ready']}) + scope :not_closed_or_merged, needed_checking + scope :closed_or_merged, where(:issues => {:status => ['closed', 'merged']}) state_machine :status, :initial => :open do event :ready do diff --git a/app/views/api/v1/pull_requests/_pull.json.jbuilder b/app/views/api/v1/pull_requests/_pull.json.jbuilder new file mode 100644 index 000000000..ae919757b --- /dev/null +++ b/app/views/api/v1/pull_requests/_pull.json.jbuilder @@ -0,0 +1,24 @@ +json.number pull.serial_id +json.(pull, :title, :status) +json.to_ref do |json_ref| + json_ref.ref pull.to_ref + json_ref.sha pull.to_commit.id + json_ref.project do |json_project| + json_project.partial! 'api/v1/projects/project', :project => pull.to_project, :json => json + end +end +json.from_ref do |json_ref| + json_ref.ref pull.from_ref + json_ref.sha pull.from_commit.id + json_ref.project do |json_project| + json_project.partial! 'api/v1/projects/project', :project => pull.from_project, :json => json + end +end +json.partial! 'api/v1/shared/owner', :owner => pull.user +json.assignee do |json_assignee| + json.partial! 'api/v1/shared/member', :member => pull.issue.assignee, :tag => json_assignee +end if pull.issue.assignee +json.mergeable pull.can_merging? +json.merged_at pull.issue.closed_at.to_i if pull.merged? + +json.url api_v1_project_pull_request_path(pull.to_project.id, pull.id, :format => :json) diff --git a/app/views/api/v1/pull_requests/index.json.jbuilder b/app/views/api/v1/pull_requests/index.json.jbuilder new file mode 100644 index 000000000..eebf94bce --- /dev/null +++ b/app/views/api/v1/pull_requests/index.json.jbuilder @@ -0,0 +1,5 @@ +json.pulls @pulls do |json, pull| + json.partial! 'pull', :pull => pull, :json => json +end + +json.url @pulls_url diff --git a/app/views/api/v1/pull_requests/show.json.jbuilder b/app/views/api/v1/pull_requests/show.json.jbuilder new file mode 100644 index 000000000..1131da5e9 --- /dev/null +++ b/app/views/api/v1/pull_requests/show.json.jbuilder @@ -0,0 +1,13 @@ +json.issue do |json| + json.partial! 'pull', :pull => @pull, :json => json + json.body @pull.body + json.closed_at pull.issue.closed_at.to_i if @pull.merged? || @pull.closed? + json.closed_by do |json_user| + json.partial! 'api/v1/shared/member', :member => @pull.issue.closer, :tag => json_user + end if @pull.issue.closer + json.merged_by do |json_user| + json.partial! 'api/v1/shared/member', :member => @pull.issue.closer, :tag => json_user + end if @pull.merged? + json.created_at @pull.issue.created_at.to_i + json.updated_at @pull.issue.updated_at.to_i +end diff --git a/config/routes.rb b/config/routes.rb index 85b4d3d37..b5ab16fd7 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -59,6 +59,7 @@ Rosa::Application.routes.draw do } resources :build_lists, :only => :index resources :issues, :only => [:index, :create, :show, :update] + resources :pull_requests, :only => [:index, :create, :show, :update] end resources :users, :only => [:show] get 'user' => 'users#show_current_user' @@ -67,6 +68,7 @@ Rosa::Application.routes.draw do get :notifiers put :notifiers get '/issues' => 'issues#user_index' + get '/pull_requests' => 'pull_requests#user_index' } end resources :groups, :only => [:index, :show, :update, :create, :destroy] do @@ -76,6 +78,7 @@ Rosa::Application.routes.draw do delete :remove_member put :update_member get '/issues' => 'issues#group_index' + get '/pull_requests' => 'pull_requests#group_index' } end resources :products, :only => [:show, :update, :create, :destroy] do @@ -86,6 +89,7 @@ Rosa::Application.routes.draw do end #resources :ssh_keys, :only => [:index, :create, :destroy] get 'issues' => 'issues#all_index' + get 'pull_requests' => 'pull_requests#all_index' end end From 20056fbc319722cc64cf7e718b22607693e7f0b0 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Wed, 19 Jun 2013 02:00:01 +0600 Subject: [PATCH 02/17] many issue/pull fixes --- app/controllers/api/v1/issues_controller.rb | 22 +++++++++++++------ .../api/v1/pull_requests_controller.rb | 2 +- .../projects/pull_requests_controller.rb | 2 -- app/models/ability.rb | 3 ++- config/routes.rb | 2 +- 5 files changed, 19 insertions(+), 12 deletions(-) diff --git a/app/controllers/api/v1/issues_controller.rb b/app/controllers/api/v1/issues_controller.rb index d6c712c02..794b43f3f 100644 --- a/app/controllers/api/v1/issues_controller.rb +++ b/app/controllers/api/v1/issues_controller.rb @@ -5,9 +5,9 @@ class Api::V1::IssuesController < Api::V1::BaseController before_filter :authenticate_user! skip_before_filter :authenticate_user!, :only => [:show] if APP_CONFIG['anonymous_access'] - load_resource :group, :only => :group_index, :find_by => :id, :parent => false - load_resource :project - load_and_authorize_resource :issue, :through => :project, :find_by => :serial_id, :only => [:show, :update, :destroy, :create, :index] + load_and_authorize_resource :group, :only => :group_index, :find_by => :id, :parent => false + load_and_authorize_resource :project + load_and_authorize_resource :issue, :through => :project, :find_by => :serial_id, :only => [:show, :update, :create, :index] def index @issues = @project.issues @@ -42,13 +42,21 @@ class Api::V1::IssuesController < Api::V1::BaseController end def update + unless can?(:write, @project) + params.delete :update_labels + [:assignee_id, :labelings, :labelings_attributes].each do |k| + params[:issue].delete k + end if params[:issue] + end + @issue.labelings.destroy_all if params[:update_labels] + if params[:issue] && status = params[:issue][:status] + @issue.set_close(current_user) if status == 'closed' + @issue.set_open if status == 'open' + params[:issue].delete :status + end update_subject @issue end - def destroy - destroy_subject @issue - end - private def render_issues_list diff --git a/app/controllers/api/v1/pull_requests_controller.rb b/app/controllers/api/v1/pull_requests_controller.rb index df9dcf147..4c135fa11 100644 --- a/app/controllers/api/v1/pull_requests_controller.rb +++ b/app/controllers/api/v1/pull_requests_controller.rb @@ -5,7 +5,7 @@ class Api::V1::PullRequestsController < Api::V1::BaseController before_filter :authenticate_user! skip_before_filter :authenticate_user!, :only => [:show] if APP_CONFIG['anonymous_access'] - load_resource :group, :only => :group_index, :find_by => :id, :parent => false + load_and_authorize_resource :group, :only => :group_index, :find_by => :id, :parent => false load_and_authorize_resource :project load_resource :issue, :through => :project, :find_by => :serial_id, :parent => false, :only => [:show, :index] load_and_authorize_resource :instance_name => :pull, :through => :issue, :singleton => true, :only => [:show, :index] diff --git a/app/controllers/projects/pull_requests_controller.rb b/app/controllers/projects/pull_requests_controller.rb index c1d5e4d45..4265ac670 100644 --- a/app/controllers/projects/pull_requests_controller.rb +++ b/app/controllers/projects/pull_requests_controller.rb @@ -40,8 +40,6 @@ class Projects::PullRequestsController < Projects::BaseController @pull = to_project.pull_requests.new pull_params @pull.issue.assignee_id = (params[:issue] || {})[:assignee_id] if can?(:write, to_project) @pull.issue.user, @pull.issue.project, @pull.from_project = current_user, to_project, @project - @pull.from_project_owner_uname = @pull.from_project.owner.uname - @pull.from_project_name = @pull.from_project.name if @pull.valid? # FIXME more clean/clever logics @pull.save # set pull id diff --git a/app/models/ability.rb b/app/models/ability.rb index 85eebe6be..5f3982d06 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -144,7 +144,8 @@ class Ability can :read, PullRequest, :to_project => {:owner_type => 'Group', :owner_id => user.group_ids} can(:read, PullRequest, read_relations_for('pull_requests', 'to_projects')) {|pull| can? :read, pull.to_project rescue nil} can :create, PullRequest - can([:update, :merge], PullRequest) {|pull| pull.user_id == user.id or local_admin?(pull.to_project)} + can(:update, PullRequest) {|pull| pull.user_id == user.id or local_admin?(pull.to_project)} + can(:merge, PullRequest) {|pull| local_admin?(pull.to_project)} can([:create, :new_line], Comment) {|comment| can? :read, comment.project} can([:update, :destroy], Comment) {|comment| comment.user == user or comment.project.owner == user or local_admin?(comment.project)} diff --git a/config/routes.rb b/config/routes.rb index b5ab16fd7..313bb6937 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -275,7 +275,7 @@ Rosa::Application.routes.draw do match 'compare/:versions' => 'wiki#compare', :versions => /([a-f0-9\^]{6,40})(\.\.\.[a-f0-9\^]{6,40})/, :as => :compare_versions, :via => :get end end - resources :issues, :except => :edit do + resources :issues, :except => [:destroy, :edit] do resources :comments, :only => [:edit, :create, :update, :destroy] resources :subscribes, :only => [:create, :destroy] collection do From f16c84bb585fd153686cb7130ee2800b55543964 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Fri, 21 Jun 2013 19:45:28 +0600 Subject: [PATCH 03/17] [#105] add commits action --- .../api/v1/pull_requests_controller.rb | 15 +++++++---- app/models/ability.rb | 8 +++--- .../v1/pull_requests/commits.json.jbuilder | 25 +++++++++++++++++++ config/initializers/a_app_config.rb | 2 ++ config/routes.rb | 7 +++++- 5 files changed, 47 insertions(+), 10 deletions(-) create mode 100644 app/views/api/v1/pull_requests/commits.json.jbuilder diff --git a/app/controllers/api/v1/pull_requests_controller.rb b/app/controllers/api/v1/pull_requests_controller.rb index 4c135fa11..1be40786c 100644 --- a/app/controllers/api/v1/pull_requests_controller.rb +++ b/app/controllers/api/v1/pull_requests_controller.rb @@ -3,12 +3,12 @@ class Api::V1::PullRequestsController < Api::V1::BaseController respond_to :json before_filter :authenticate_user! - skip_before_filter :authenticate_user!, :only => [:show] if APP_CONFIG['anonymous_access'] + skip_before_filter :authenticate_user!, :only => [:show, :commits, :files] if APP_CONFIG['anonymous_access'] - load_and_authorize_resource :group, :only => :group_index, :find_by => :id, :parent => false - load_and_authorize_resource :project - load_resource :issue, :through => :project, :find_by => :serial_id, :parent => false, :only => [:show, :index] - load_and_authorize_resource :instance_name => :pull, :through => :issue, :singleton => true, :only => [:show, :index] + load_resource :group, :only => :group_index, :find_by => :id, :parent => false + load_resource :project + load_resource :issue, :through => :project, :find_by => :serial_id, :parent => false, :only => [:show, :index, :commits, :files] + load_and_authorize_resource :instance_name => :pull, :through => :issue, :singleton => true, :only => [:show, :index, :commits, :files] def index @pull_requests = @project.pull_requests @@ -92,6 +92,11 @@ class Api::V1::PullRequestsController < Api::V1::BaseController end end + def commits + @commits = @pull.repo.commits_between(@pull.to_commit, @pull.from_commit) + @commits.paginate(paginate_params) + end + private def render_pulls_list diff --git a/app/models/ability.rb b/app/models/ability.rb index 5f3982d06..44ce42279 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -17,7 +17,7 @@ class Ability can :get_id, Project, :visibility => 'open' # api can :archive, Project, :visibility => 'open' can :read, Issue, :project => {:visibility => 'open'} - can :read, PullRequest, :to_project => {:visibility => 'open'} + can [:read, :commits, :files], PullRequest, :to_project => {:visibility => 'open'} can :search, BuildList can [:read, :log, :everything], BuildList, :project => {:visibility => 'open'} can [:read, :log], ProductBuildList#, :product => {:platform => {:visibility => 'open'}} # double nested hash don't work @@ -140,9 +140,9 @@ class Ability can(:update, Issue) {|issue| issue.user_id == user.id or local_admin?(issue.project)} cannot :manage, Issue, :project => {:has_issues => false} # switch off issues - can :read, PullRequest, :to_project => {:owner_type => 'User', :owner_id => user.id} - can :read, PullRequest, :to_project => {:owner_type => 'Group', :owner_id => user.group_ids} - can(:read, PullRequest, read_relations_for('pull_requests', 'to_projects')) {|pull| can? :read, pull.to_project rescue nil} + can [:read, :commits, :files], PullRequest, :to_project => {:owner_type => 'User', :owner_id => user.id} + can [:read, :commits, :files], PullRequest, :to_project => {:owner_type => 'Group', :owner_id => user.group_ids} + can([:read, :commits, :files], PullRequest, read_relations_for('pull_requests', 'to_projects')) {|pull| can? :read, pull.to_project} can :create, PullRequest can(:update, PullRequest) {|pull| pull.user_id == user.id or local_admin?(pull.to_project)} can(:merge, PullRequest) {|pull| local_admin?(pull.to_project)} diff --git a/app/views/api/v1/pull_requests/commits.json.jbuilder b/app/views/api/v1/pull_requests/commits.json.jbuilder new file mode 100644 index 000000000..0baf00714 --- /dev/null +++ b/app/views/api/v1/pull_requests/commits.json.jbuilder @@ -0,0 +1,25 @@ +json.commits @commits do |json_commit, commit| + json_commit.sha commit.id + json_commit.https_url commit_path(@project, commit.id) + json.author do |json_author| + json_author.name commit.author.name + json_author.email commit.author.email + json_author.date commit.authored_date.to_i + end + json.committer do |json_committer| + json_committer.name commit.committer.name + json_committer.email commit.committer.email + json_committer.date commit.committed_date.to_i + end + json.message commit.message + json.tree do |json_tree| + json_tree.sha commit.id + json_tree.https_url commit_path(@project, commit.id) + end + json.parents commit.parents do |json, parent| + json.sha parent.id + json.https_url commit_path(@project, parent.id) + end +end + +json.url commits_api_v1_project_pull_request_path(:format => :json) diff --git a/config/initializers/a_app_config.rb b/config/initializers/a_app_config.rb index 04cb06bb5..a9e711aff 100644 --- a/config/initializers/a_app_config.rb +++ b/config/initializers/a_app_config.rb @@ -2,3 +2,5 @@ APP_CONFIG = YAML.load_file("#{Rails.root}/config/application.yml")[Rails.env] # Remove '/' from the end of url APP_CONFIG.keys.select {|key| key =~ /_url\Z/}.each {|key| APP_CONFIG[key] = APP_CONFIG[key].chomp('/') if APP_CONFIG[key].respond_to?(:chomp)} +# Paginates a static array +require 'will_paginate/array' diff --git a/config/routes.rb b/config/routes.rb index 313bb6937..aa8484ee9 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -59,7 +59,12 @@ Rosa::Application.routes.draw do } resources :build_lists, :only => :index resources :issues, :only => [:index, :create, :show, :update] - resources :pull_requests, :only => [:index, :create, :show, :update] + resources :pull_requests, :only => [:index, :create, :show, :update] do + member { + get :commits + get :files + } + end end resources :users, :only => [:show] get 'user' => 'users#show_current_user' From 926c4d604d04a3e7747934a19228115184d41d50 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Fri, 21 Jun 2013 19:49:12 +0600 Subject: [PATCH 04/17] [#105] restore deleted code in error --- app/controllers/projects/pull_requests_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/projects/pull_requests_controller.rb b/app/controllers/projects/pull_requests_controller.rb index 4265ac670..c1d5e4d45 100644 --- a/app/controllers/projects/pull_requests_controller.rb +++ b/app/controllers/projects/pull_requests_controller.rb @@ -40,6 +40,8 @@ class Projects::PullRequestsController < Projects::BaseController @pull = to_project.pull_requests.new pull_params @pull.issue.assignee_id = (params[:issue] || {})[:assignee_id] if can?(:write, to_project) @pull.issue.user, @pull.issue.project, @pull.from_project = current_user, to_project, @project + @pull.from_project_owner_uname = @pull.from_project.owner.uname + @pull.from_project_name = @pull.from_project.name if @pull.valid? # FIXME more clean/clever logics @pull.save # set pull id From dcde3aa369aa7afeeb4df97f7f641b079e8bf16f Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Mon, 24 Jun 2013 20:54:22 +0600 Subject: [PATCH 05/17] [#105] add files action --- .../api/v1/pull_requests_controller.rb | 4 ++++ .../api/v1/pull_requests/files.json.jbuilder | 24 +++++++++++++++++++ 2 files changed, 28 insertions(+) create mode 100644 app/views/api/v1/pull_requests/files.json.jbuilder diff --git a/app/controllers/api/v1/pull_requests_controller.rb b/app/controllers/api/v1/pull_requests_controller.rb index 1be40786c..39505d28f 100644 --- a/app/controllers/api/v1/pull_requests_controller.rb +++ b/app/controllers/api/v1/pull_requests_controller.rb @@ -97,6 +97,10 @@ class Api::V1::PullRequestsController < Api::V1::BaseController @commits.paginate(paginate_params) end + def files + @stats = @pull.diff_stats.zip(@pull.diff) + end + private def render_pulls_list diff --git a/app/views/api/v1/pull_requests/files.json.jbuilder b/app/views/api/v1/pull_requests/files.json.jbuilder new file mode 100644 index 000000000..8070b0271 --- /dev/null +++ b/app/views/api/v1/pull_requests/files.json.jbuilder @@ -0,0 +1,24 @@ +json.files @stats do |json_stat, stat| + fstat, diff = stat + commit_id = diff.deleted_file ? @pull.to_commit.id : @pull.from_commit.id + json_stat.sha commit_id + json_stat.filename diff.b_path + status = case + when diff.new_file + 'added' + when diff.deleted_file + 'deleted' + when diff.renamed_file + 'renamed' + else + 'modified' + end + json_stat.status status + json_stat.additions fstat.additions + json_stat.deletions fstat.deletions + json_stat.changes fstat.additions + fstat.deletions + json_stat.blob_https_url blob_path(@project, commit_id, diff.b_path) + json_stat.raw_https_url raw_path(@project, commit_id, diff.b_path) +end + +json.url files_api_v1_project_pull_request_path(:format => :json) From eb9ce477237f1468a2d147b0e7733c1e5b5b73fc Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Mon, 24 Jun 2013 20:54:50 +0600 Subject: [PATCH 06/17] pull diff small refactoring --- .gitignore | 1 + app/views/projects/pull_requests/_pull_diff.html.haml | 5 ++--- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index 69fc58944..f98b0074d 100644 --- a/.gitignore +++ b/.gitignore @@ -20,3 +20,4 @@ crash.log config/newrelic.yml config/deploy/*.rb config/deploy.rb +*.swo diff --git a/app/views/projects/pull_requests/_pull_diff.html.haml b/app/views/projects/pull_requests/_pull_diff.html.haml index 60fbe55ff..baf8b9ece 100644 --- a/app/views/projects/pull_requests/_pull_diff.html.haml +++ b/app/views/projects/pull_requests/_pull_diff.html.haml @@ -2,9 +2,8 @@ .file %a{:name => "diff-#{pull_diff_counter}"} .top - .l= h(pull_diff.renamed_file ? "#{pull_diff.a_path.rtruncate 60} -> #{pull_diff.b_path.rtruncate 60}" : pull_diff.a_path.rtruncate(120)) - - if pull_diff.b_path.present? - .r= link_to "view file @ #{short_hash_id(commit_id)}", blob_path(@project, commit_id, pull_diff.b_path) + .l= h(pull_diff.renamed_file ? "#{pull_diff.a_path.rtruncate 60}=>#{pull_diff.b_path.rtruncate 60}" : pull_diff.b_path.rtruncate(120)) + .r= link_to "view file @ #{short_hash_id(commit_id)}", blob_path(@project, commit_id, pull_diff.b_path) .clear -if pull_diff.diff.present? && !(@pull.repo.tree(commit_id) / pull_diff.b_path).binary? .diff_data=render_diff(pull_diff, :diff_counter => pull_diff_counter, :comments => @comments) From ebc278d65aa9d6e0879f07d920bf343066ec8a35 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Mon, 24 Jun 2013 21:18:17 +0600 Subject: [PATCH 07/17] [#105] add merge action --- app/controllers/api/v1/pull_requests_controller.rb | 13 +++++++++++-- config/routes.rb | 1 + 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/v1/pull_requests_controller.rb b/app/controllers/api/v1/pull_requests_controller.rb index 39505d28f..3cc459cd4 100644 --- a/app/controllers/api/v1/pull_requests_controller.rb +++ b/app/controllers/api/v1/pull_requests_controller.rb @@ -7,8 +7,8 @@ class Api::V1::PullRequestsController < Api::V1::BaseController load_resource :group, :only => :group_index, :find_by => :id, :parent => false load_resource :project - load_resource :issue, :through => :project, :find_by => :serial_id, :parent => false, :only => [:show, :index, :commits, :files] - load_and_authorize_resource :instance_name => :pull, :through => :issue, :singleton => true, :only => [:show, :index, :commits, :files] + load_resource :issue, :through => :project, :find_by => :serial_id, :parent => false, :only => [:show, :index, :commits, :files, :merge] + load_and_authorize_resource :instance_name => :pull, :through => :issue, :singleton => true, :only => [:show, :index, :commits, :files, :merge] def index @pull_requests = @project.pull_requests @@ -101,6 +101,15 @@ class Api::V1::PullRequestsController < Api::V1::BaseController @stats = @pull.diff_stats.zip(@pull.diff) end + def merge + class_name = @pull.class.name + if @pull.merge!(current_user) + render_json_response @pull, "#{class_name} has been merged successfully" + else + render_validation_error @pull, "#{class_name} has not been merged" + end + end + private def render_pulls_list diff --git a/config/routes.rb b/config/routes.rb index aa8484ee9..5b276e3f6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -63,6 +63,7 @@ Rosa::Application.routes.draw do member { get :commits get :files + put :merge } end end From 8e189821bd8b6abad7ce55f73bb611e10cd09839 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Mon, 24 Jun 2013 21:18:52 +0600 Subject: [PATCH 08/17] remove extra pull check --- app/controllers/projects/pull_requests_controller.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/controllers/projects/pull_requests_controller.rb b/app/controllers/projects/pull_requests_controller.rb index c1d5e4d45..38e373d0a 100644 --- a/app/controllers/projects/pull_requests_controller.rb +++ b/app/controllers/projects/pull_requests_controller.rb @@ -78,7 +78,6 @@ class Projects::PullRequestsController < Projects::BaseController end def merge - @pull.check unless @pull.merge!(current_user) flash.now[:error] = t('flash.pull_request.save_error') flash.now[:warning] = @pull.errors.full_messages.join('. ') From 20cb49c3188f45ef797288e4ea7fcf3468acd2b6 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Wed, 26 Jun 2013 23:58:32 +0600 Subject: [PATCH 09/17] [#105] many fixes by @avokhmin comments --- app/controllers/api/v1/issues_controller.rb | 14 ++++++------- .../api/v1/pull_requests_controller.rb | 20 +++++++++---------- .../api/v1/pull_requests/index.json.jbuilder | 2 +- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/app/controllers/api/v1/issues_controller.rb b/app/controllers/api/v1/issues_controller.rb index 794b43f3f..a69027bc2 100644 --- a/app/controllers/api/v1/issues_controller.rb +++ b/app/controllers/api/v1/issues_controller.rb @@ -3,7 +3,7 @@ class Api::V1::IssuesController < Api::V1::BaseController respond_to :json before_filter :authenticate_user! - skip_before_filter :authenticate_user!, :only => [:show] if APP_CONFIG['anonymous_access'] + skip_before_filter :authenticate_user!, :only => [:index, :group_index, :show] if APP_CONFIG['anonymous_access'] load_and_authorize_resource :group, :only => :group_index, :find_by => :id, :parent => false load_and_authorize_resource :project @@ -15,20 +15,20 @@ class Api::V1::IssuesController < Api::V1::BaseController end def all_index - project_ids = get_all_project_ids Project.accessible_by(current_ability, :membered).uniq.pluck(:id) + project_ids = get_all_project_ids Project.accessible_by(current_ability, :membered).pluck(:id) @issues = Issue.where('issues.project_id IN (?)', project_ids) render_issues_list end def user_index - project_ids = get_all_project_ids current_user.projects.select('distinct projects.id').pluck(:id) + project_ids = get_all_project_ids current_user.projects.pluck(:id) @issues = Issue.where('issues.project_id IN (?)', project_ids) render_issues_list end def group_index - project_ids = @group.projects.select('distinct projects.id').pluck(:id) - project_ids = Project.accessible_by(current_ability, :membered).where(:id => project_ids).uniq.pluck(:id) + project_ids = @group.projects.pluck(:id) + project_ids = Project.accessible_by(current_ability, :membered).where(:id => project_ids).pluck(:id) @issues = Issue.where(:project_id => project_ids) render_issues_list end @@ -100,7 +100,7 @@ class Api::V1::IssuesController < Api::V1::BaseController @issues = @issues.order("#{sort} #{direction}") @issues = @issues.where('issues.created_at >= to_timestamp(?)', params[:since]) if params[:since] =~ /\A\d+\z/ - @issues.paginate(paginate_params) + @issues = @issues.paginate(paginate_params) render :index end @@ -109,7 +109,7 @@ class Api::V1::IssuesController < Api::V1::BaseController if ['created', 'all'].include? params[:filter] # add own issues project_ids = Project.accessible_by(current_ability, :show).joins(:issues). - where(:issues => {:user_id => current_user.id}).uniq.pluck('projects.id') + where(:issues => {:user_id => current_user.id}).pluck('projects.id') end project_ids |= default_project_ids end diff --git a/app/controllers/api/v1/pull_requests_controller.rb b/app/controllers/api/v1/pull_requests_controller.rb index 3cc459cd4..beaf594fb 100644 --- a/app/controllers/api/v1/pull_requests_controller.rb +++ b/app/controllers/api/v1/pull_requests_controller.rb @@ -3,7 +3,7 @@ class Api::V1::PullRequestsController < Api::V1::BaseController respond_to :json before_filter :authenticate_user! - skip_before_filter :authenticate_user!, :only => [:show, :commits, :files] if APP_CONFIG['anonymous_access'] + skip_before_filter :authenticate_user!, :only => [:show, :index, :group_index, :commits, :files] if APP_CONFIG['anonymous_access'] load_resource :group, :only => :group_index, :find_by => :id, :parent => false load_resource :project @@ -11,28 +11,28 @@ class Api::V1::PullRequestsController < Api::V1::BaseController load_and_authorize_resource :instance_name => :pull, :through => :issue, :singleton => true, :only => [:show, :index, :commits, :files, :merge] def index - @pull_requests = @project.pull_requests + @pulls = @project.pull_requests @pulls_url = api_v1_project_pull_requests_path(@project, :format => :json) render_pulls_list end def all_index - project_ids = get_all_project_ids Project.accessible_by(current_ability, :membered).uniq.pluck(:id) + project_ids = get_all_project_ids Project.accessible_by(current_ability, :membered).pluck(:id) @pulls = PullRequest.where('pull_requests.to_project_id IN (?)', project_ids) @pulls_url = api_v1_pull_requests_path :format => :json render_pulls_list end def user_index - project_ids = get_all_project_ids current_user.projects.select('distinct projects.id').pluck(:id) + project_ids = get_all_project_ids current_user.projects.pluck(:id) @pulls = PullRequest.where('pull_requests.to_project_id IN (?)', project_ids) @pulls_url = pull_requests_api_v1_user_path :format => :json render_pulls_list end def group_index - project_ids = @group.projects.select('distinct projects.id').pluck(:id) - project_ids = Project.accessible_by(current_ability, :membered).where(:id => project_ids).uniq.pluck(:id) + project_ids = @group.projects.pluck(:id) + project_ids = Project.accessible_by(current_ability, :membered).where(:id => project_ids).pluck(:id) @pulls = PullRequest.where(:to_project_id => project_ids) @pulls_url = pull_requests_api_v1_group_path render_pulls_list @@ -42,7 +42,8 @@ class Api::V1::PullRequestsController < Api::V1::BaseController end def create - from_project = Project.find params[:pull_request].try('[]', :from_project) + from_project = Project.find(pull_params[:from_project]) if pull_params.try('[]', :from_project).present? + from_project ||= @project authorize! :read, from_project @pull = @project.pull_requests.new @@ -51,7 +52,6 @@ class Api::V1::PullRequestsController < Api::V1::BaseController @pull.to_ref, @pull.from_ref = pull_params[:to_ref], pull_params[:from_ref] @pull.issue.assignee_id = pull_params[:assignee] if can?(:write, @project) @pull.issue.user, @pull.issue.project = current_user, @project - render_validation_error(@pull, "#{@pull.class.name} has not been created") && return unless @pull.valid? @pull.save # set pull id @@ -148,7 +148,7 @@ class Api::V1::PullRequestsController < Api::V1::BaseController @pulls = @pulls.order("#{sort} #{direction}") @pulls = @pulls.where('issues.created_at >= to_timestamp(?)', params[:since]) if params[:since] =~ /\A\d+\z/ - @pulls.paginate(paginate_params) + @pulls = @pulls.paginate(paginate_params) render :index end @@ -157,7 +157,7 @@ class Api::V1::PullRequestsController < Api::V1::BaseController if ['created', 'all'].include? params[:filter] # add own pulls project_ids = Project.accessible_by(current_ability, :show).joins(:issues). - where(:issues => {:user_id => current_user.id}).uniq.pluck('projects.id') + where(:issues => {:user_id => current_user.id}).pluck('projects.id') end project_ids |= default_project_ids end diff --git a/app/views/api/v1/pull_requests/index.json.jbuilder b/app/views/api/v1/pull_requests/index.json.jbuilder index eebf94bce..607678bee 100644 --- a/app/views/api/v1/pull_requests/index.json.jbuilder +++ b/app/views/api/v1/pull_requests/index.json.jbuilder @@ -1,4 +1,4 @@ -json.pulls @pulls do |json, pull| +json.pull_requests @pulls do |json, pull| json.partial! 'pull', :pull => pull, :json => json end From fd93f472d7847765ff85fe83c84664993a3d4ee8 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Wed, 26 Jun 2013 23:59:07 +0600 Subject: [PATCH 10/17] [#105] first part pull requests api specs --- .../api/v1/pull_requests_controller.rb | 161 ++++++++++++++++++ 1 file changed, 161 insertions(+) create mode 100644 spec/controllers/api/v1/pull_requests_controller.rb diff --git a/spec/controllers/api/v1/pull_requests_controller.rb b/spec/controllers/api/v1/pull_requests_controller.rb new file mode 100644 index 000000000..d22db3eb8 --- /dev/null +++ b/spec/controllers/api/v1/pull_requests_controller.rb @@ -0,0 +1,161 @@ +# -*- encoding : utf-8 -*- +require 'spec_helper' + +def create_pull to_ref, from_ref, owner, project = @project + pull = project.pull_requests.new :issue_attributes => {:title => 'test', :body => 'testing'} + pull.issue.user, pull.issue.project = owner, pull.to_project + pull.to_ref, pull.from_ref, pull.from_project = to_ref, from_ref, project + pull.save + pull +end + +describe Api::V1::PullRequestsController do + before(:each) do + stub_symlink_methods + stub_redis + @project = FactoryGirl.create(:project_with_commit) + @pull = create_pull 'master', 'non_conflicts', @project.owner + @create_params = {:pull_request => {:title => 'title', :body => 'body', + :from_ref => 'conflicts', :to_ref => 'master'}, + :project_id => @project.id, :format => :json} + end + + context 'read and accessible abilities' do + context 'for user' do + before(:each) do + http_login(@project.owner) + end + + it 'can show pull request in own project' do + get :show, :project_id => @project.id, :id => @pull.serial_id, :format => :json + response.should be_success + end + + it 'should render right template for show action' do + get :show, :project_id => @project.id, :id => @pull.serial_id, :format => :json + response.should render_template('api/v1/pull_requests/show') + end + + it 'can show pull request in open project' do + another_project = FactoryGirl.create(:project_with_commit) + pull = create_pull 'master', 'non_conflicts', another_project.owner, another_project + get :show, :project_id => another_project.id, :id => pull.serial_id, :format => :json + response.should be_success + end + + it 'can show pull request in own hidden project' do + @project.update_column :visibility, 'hidden' + get :show, :project_id => @project.id, :id => @pull.serial_id, :format => :json + response.should be_success + end + + it 'cant show pull request in hidden project' do + another_project = FactoryGirl.create(:project_with_commit) + pull = create_pull 'master', 'non_conflicts', another_project.owner, another_project + another_project.update_column :visibility, 'hidden' + + get :show, :project_id => another_project.id, :id => pull.serial_id, :format => :json + response.status.should == 403 + end + + it 'should return three pull requests' do + own_hidden_project = FactoryGirl.create(:project_with_commit, :owner => @project.owner) + own_hidden_project.update_column :visibility, 'hidden' + own_hidden_pull = create_pull 'master', 'non_conflicts', own_hidden_project.owner, own_hidden_project + membered_project = FactoryGirl.create(:project_with_commit) + membered_pull = create_pull 'master', 'non_conflicts', membered_project.owner, membered_project + membered_project.relations.create(:role => 'reader', :actor => @pull.user) + + get :all_index, :filter => 'all', :format => :json + assigns[:pulls].should include(@pull) + assigns[:pulls].should include(own_hidden_pull) + assigns[:pulls].should include(membered_pull) + end + + it 'should render right template for all index action' do + get :all_index, :format => :json + response.should render_template('api/v1/pull_requests/index') + end + + it 'should return only assigned pull request' do + own_hidden_project = FactoryGirl.create(:project_with_commit, :owner => @project.owner) + own_hidden_project.update_column :visibility, 'hidden' + own_hidden_pull = create_pull 'master', 'non_conflicts', own_hidden_project.owner, own_hidden_project + own_hidden_pull.issue.update_column :assignee_id, @pull.user + + get :user_index, :format => :json + assigns[:pulls].should include(own_hidden_pull) + assigns[:pulls].count.should == 1 + end + + it 'should render right template for user index action' do + get :user_index, :format => :json + response.should render_template('api/v1/pull_requests/index') + end + end + + context 'for anonymous user' do + it 'can show pull request in open project', :anonymous_access => true do + get :show, :project_id => @project.id, :id => @pull.serial_id, :format => :json + response.should be_success + end + + it 'cant show pull request in hidden project', :anonymous_access => true do + @project.update_column :visibility, 'hidden' + get :show, :project_id => @project.id, :id => @pull.serial_id, :format => :json + response.status.should == 403 + end + + it 'should not return any pull requests' do + get :all_index, :filter => 'all', :format => :json + response.status.should == 401 + end + end + end + + context 'create accessibility' do + context 'for user' do + before(:each) do + http_login(@pull.user) + end + + it 'can create pull request in own project' do + lambda { post :create, @create_params }.should change{ PullRequest.count }.by(1) + end + + it 'can create pull request in own hidden project' do + own_hidden_project = FactoryGirl.create(:project_with_commit, :owner => @project.owner) + own_hidden_project.update_column :visibility, 'hidden' + lambda { post :create, @create_params.merge(:project_id => own_hidden_project.id) }.should + change{ PullRequest.count }.by(1) + end + + it 'can create pull request in open project' do + lambda { post :create, @create_params.merge(:project_id => @another_project.id) }.should + change{ PullRequest.count }.by(1) + end + + it 'cant create pull request in hidden project' do + hidden_project = FactoryGirl.create(:project_with_commit) + hidden_project.update_column :visibility, 'hidden' + + lambda { post :create, @create_params.merge(:project_id => hidden_project.id) }.should + change{ PullRequest.count }.by(0) + + end + end + + context 'for anonymous user' do + it 'cant create pull request in project', :anonymous_access => true do + lambda { post :create, @create_params }.should change{ PullRequest.count }.by(0) + end + + it 'cant create pull request in hidden project', :anonymous_access => true do + hidden_project = FactoryGirl.create(:project_with_commit) + hidden_project.update_column :visibility, 'hidden' + lambda { post :create, @create_params.merge(:project_id => hidden_project.id) }.should + change{ PullRequest.count }.by(0) + end + end + end +end From 3fd99a42d220b2bf40f78118a0af2a51fed5345a Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Thu, 27 Jun 2013 00:01:33 +0600 Subject: [PATCH 11/17] [#105] fix paginate commits & add paginate to files --- app/controllers/api/v1/pull_requests_controller.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/controllers/api/v1/pull_requests_controller.rb b/app/controllers/api/v1/pull_requests_controller.rb index beaf594fb..77e5f2ae8 100644 --- a/app/controllers/api/v1/pull_requests_controller.rb +++ b/app/controllers/api/v1/pull_requests_controller.rb @@ -93,12 +93,11 @@ class Api::V1::PullRequestsController < Api::V1::BaseController end def commits - @commits = @pull.repo.commits_between(@pull.to_commit, @pull.from_commit) - @commits.paginate(paginate_params) + @commits = @pull.repo.commits_between(@pull.to_commit, @pull.from_commit).paginate(paginate_params) end def files - @stats = @pull.diff_stats.zip(@pull.diff) + @stats = @pull.diff_stats.zip(@pull.diff).paginate(paginate_params) end def merge From dad840f973599339d444c2edca938dc2dae35a51 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Fri, 28 Jun 2013 19:10:22 +0600 Subject: [PATCH 12/17] [#105] add missing specs --- .../api/v1/pull_requests_controller.rb | 184 ++++++++++++++---- 1 file changed, 146 insertions(+), 38 deletions(-) diff --git a/spec/controllers/api/v1/pull_requests_controller.rb b/spec/controllers/api/v1/pull_requests_controller.rb index d22db3eb8..e7bcd8aad 100644 --- a/spec/controllers/api/v1/pull_requests_controller.rb +++ b/spec/controllers/api/v1/pull_requests_controller.rb @@ -5,19 +5,39 @@ def create_pull to_ref, from_ref, owner, project = @project pull = project.pull_requests.new :issue_attributes => {:title => 'test', :body => 'testing'} pull.issue.user, pull.issue.project = owner, pull.to_project pull.to_ref, pull.from_ref, pull.from_project = to_ref, from_ref, project - pull.save + pull.save; pull.check pull end describe Api::V1::PullRequestsController do - before(:each) do + before(:all) do stub_symlink_methods stub_redis @project = FactoryGirl.create(:project_with_commit) @pull = create_pull 'master', 'non_conflicts', @project.owner + + @another_project = FactoryGirl.create(:project_with_commit) + @another_pull = create_pull 'master', 'non_conflicts', @another_project.owner, @another_project + + @hidden_project = FactoryGirl.create(:project_with_commit) + @hidden_project.update_column :visibility, 'hidden' + @hidden_pull = create_pull 'master', 'non_conflicts', @hidden_project.owner, @hidden_project + + @own_hidden_project = FactoryGirl.create(:project_with_commit, :owner => @project.owner) + @own_hidden_project.update_column :visibility, 'hidden' + @own_hidden_pull = create_pull 'master', 'non_conflicts', @own_hidden_project.owner, @own_hidden_project + @own_hidden_pull.issue.update_column :assignee_id, @project.owner.id + + @membered_project = FactoryGirl.create(:project_with_commit) + @membered_pull = create_pull 'master', 'non_conflicts', @membered_project.owner, @membered_project + @membered_project.relations.create(:role => 'reader', :actor => @pull.user) + @create_params = {:pull_request => {:title => 'title', :body => 'body', :from_ref => 'conflicts', :to_ref => 'master'}, :project_id => @project.id, :format => :json} + + @update_params = {:pull_request => {:title => 'new title'}, + :project_id => @project.id, :id => @pull.serial_id, :format => :json} end context 'read and accessible abilities' do @@ -37,39 +57,25 @@ describe Api::V1::PullRequestsController do end it 'can show pull request in open project' do - another_project = FactoryGirl.create(:project_with_commit) - pull = create_pull 'master', 'non_conflicts', another_project.owner, another_project - get :show, :project_id => another_project.id, :id => pull.serial_id, :format => :json + get :show, :project_id => @another_project.id, :id => @another_pull.serial_id, :format => :json response.should be_success end it 'can show pull request in own hidden project' do - @project.update_column :visibility, 'hidden' - get :show, :project_id => @project.id, :id => @pull.serial_id, :format => :json + get :show, :project_id => @own_hidden_project.id, :id => @own_hidden_pull.serial_id, :format => :json response.should be_success end it 'cant show pull request in hidden project' do - another_project = FactoryGirl.create(:project_with_commit) - pull = create_pull 'master', 'non_conflicts', another_project.owner, another_project - another_project.update_column :visibility, 'hidden' - - get :show, :project_id => another_project.id, :id => pull.serial_id, :format => :json + get :show, :project_id => @hidden_project.id, :id => @hidden_pull.serial_id, :format => :json response.status.should == 403 end it 'should return three pull requests' do - own_hidden_project = FactoryGirl.create(:project_with_commit, :owner => @project.owner) - own_hidden_project.update_column :visibility, 'hidden' - own_hidden_pull = create_pull 'master', 'non_conflicts', own_hidden_project.owner, own_hidden_project - membered_project = FactoryGirl.create(:project_with_commit) - membered_pull = create_pull 'master', 'non_conflicts', membered_project.owner, membered_project - membered_project.relations.create(:role => 'reader', :actor => @pull.user) - get :all_index, :filter => 'all', :format => :json assigns[:pulls].should include(@pull) - assigns[:pulls].should include(own_hidden_pull) - assigns[:pulls].should include(membered_pull) + assigns[:pulls].should include(@own_hidden_pull) + assigns[:pulls].should include(@membered_pull) end it 'should render right template for all index action' do @@ -78,13 +84,8 @@ describe Api::V1::PullRequestsController do end it 'should return only assigned pull request' do - own_hidden_project = FactoryGirl.create(:project_with_commit, :owner => @project.owner) - own_hidden_project.update_column :visibility, 'hidden' - own_hidden_pull = create_pull 'master', 'non_conflicts', own_hidden_project.owner, own_hidden_project - own_hidden_pull.issue.update_column :assignee_id, @pull.user - get :user_index, :format => :json - assigns[:pulls].should include(own_hidden_pull) + assigns[:pulls].should include(@own_hidden_pull) assigns[:pulls].count.should == 1 end @@ -92,6 +93,23 @@ describe Api::V1::PullRequestsController do get :user_index, :format => :json response.should render_template('api/v1/pull_requests/index') end + + %w(commits files).each do |action| + it "can show pull request #{action} in own project" do + get action, :project_id => @project.id, :id => @pull.serial_id, :format => :json + response.should be_success + end + + it "should render right template for commits action" do + get action, :project_id => @project.id, :id => @pull.serial_id, :format => :json + response.should render_template("api/v1/pull_requests/#{action}") + end + + it "can't show pull request #{action} in hidden project" do + get action, :project_id => @hidden_project.id, :id => @hidden_pull.serial_id, :format => :json + response.should_not be_success + end + end end context 'for anonymous user' do @@ -110,6 +128,23 @@ describe Api::V1::PullRequestsController do get :all_index, :filter => 'all', :format => :json response.status.should == 401 end + + %w(commits files).each do |action| + it "can show pull request #{action} in project" do + get action, :project_id => @project.id, :id => @pull.serial_id, :format => :json + response.should be_success + end + + it "should render right template for commits action" do + get action, :project_id => @project.id, :id => @pull.serial_id, :format => :json + response.should render_template("api/v1/pull_requests/#{action}") + end + + it "can't show pull request #{action} in hidden project" do + get action, :project_id => @hidden_project.id, :id => @hidden_pull.serial_id, :format => :json + response.should_not be_success + end + end end end @@ -124,9 +159,7 @@ describe Api::V1::PullRequestsController do end it 'can create pull request in own hidden project' do - own_hidden_project = FactoryGirl.create(:project_with_commit, :owner => @project.owner) - own_hidden_project.update_column :visibility, 'hidden' - lambda { post :create, @create_params.merge(:project_id => own_hidden_project.id) }.should + lambda { post :create, @create_params.merge(:project_id => @own_hidden_project.id) }.should change{ PullRequest.count }.by(1) end @@ -136,12 +169,8 @@ describe Api::V1::PullRequestsController do end it 'cant create pull request in hidden project' do - hidden_project = FactoryGirl.create(:project_with_commit) - hidden_project.update_column :visibility, 'hidden' - - lambda { post :create, @create_params.merge(:project_id => hidden_project.id) }.should + lambda { post :create, @create_params.merge(:project_id => @hidden_project.id) }.should change{ PullRequest.count }.by(0) - end end @@ -151,11 +180,90 @@ describe Api::V1::PullRequestsController do end it 'cant create pull request in hidden project', :anonymous_access => true do - hidden_project = FactoryGirl.create(:project_with_commit) - hidden_project.update_column :visibility, 'hidden' - lambda { post :create, @create_params.merge(:project_id => hidden_project.id) }.should + lambda { post :create, @create_params.merge(:project_id => @hidden_project.id) }.should change{ PullRequest.count }.by(0) end end end + + context 'update accessibility' do + context 'for user' do + before(:each) do + http_login(@project.owner) + end + + it 'can update pull request in own project' do + put :update, @update_params + @pull.reload.title.should == 'new title' + end + + it 'can update pull request in own hidden project' do + put :update, @update_params.merge(:project_id => @own_hidden_project.id, :id => @own_hidden_pull.serial_id) + @own_hidden_pull.reload.title.should == 'new title' + end + + it 'cant update pull request in open project' do + put :update, @update_params.merge(:project_id => @another_project.id, :id => @another_pull.serial_id) + @another_pull.reload.title.should_not == 'new title' + end + + it 'cant update pull request in hidden project' do + put :update, @update_params.merge(:project_id => @hidden_project.id, :id => @hidden_pull.serial_id) + @hidden_pull.reload.title.should_not == 'title' + end + + it 'can merge pull request in own project' do + put :merge, :project_id => @project.id, :id => @pull.serial_id, :format => :json + @pull.reload.status.should == 'merged' + response.should be_success + end + + it 'can merge pull request in own hidden project' do + put :merge, :project_id => @own_hidden_project.id, :id => @own_hidden_pull.serial_id, :format => :json + @own_hidden_pull.reload.status.should == 'merged' + response.should be_success + end + + it 'cant merge pull request in open project' do + put :merge, :project_id => @another_project.id, :id => @another_pull.serial_id, :format => :json + @another_pull.reload.status.should == 'ready' + response.status.should == 403 + end + + it 'cant merge pull request in hidden project' do + put :merge, :project_id => @hidden_project.id, :id => @hidden_pull.serial_id, :format => :json + @hidden_pull.reload.status.should == 'ready' + response.status.should == 403 + end + end + + context 'for anonymous user' do + it 'cant update pull request in project', :anonymous_access => true do + put :update, @update_params + response.status.should == 401 + end + + it 'cant update pull request in hidden project', :anonymous_access => true do + put :update, @update_params.merge(:project_id => @hidden_project.id, :id => @hidden_pull.serial_id) + response.status.should == 401 + end + + it 'cant merge pull request in open project' do + put :merge, :project_id => @another_project.id, :id => @another_pull.serial_id, :format => :json + @another_pull.reload.status.should == 'ready' + response.status.should == 401 + end + + it 'cant merge pull request in hidden project' do + put :merge, :project_id => @hidden_project.id, :id => @hidden_pull.serial_id, :format => :json + @hidden_pull.reload.status.should == 'ready' + response.status.should == 401 + end + end + end + + after(:all) do + User.destroy_all + Platform.destroy_all + end end From 680c4f2a5a4b3569137c973bed76cb2f463ab778 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Fri, 28 Jun 2013 19:20:10 +0600 Subject: [PATCH 13/17] refactoring issues specs --- .../api/v1/issues_controller_spec.rb | 24 ++++++------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/spec/controllers/api/v1/issues_controller_spec.rb b/spec/controllers/api/v1/issues_controller_spec.rb index 8e307660a..710b5410c 100644 --- a/spec/controllers/api/v1/issues_controller_spec.rb +++ b/spec/controllers/api/v1/issues_controller_spec.rb @@ -73,7 +73,6 @@ describe Api::V1::IssuesController do end it 'should return only assigned issue' do - http_login(@issue.user) get :user_index, :format => :json assigns[:issues].should include(@own_hidden_issue) assigns[:issues].count.should == 1 @@ -107,42 +106,32 @@ describe Api::V1::IssuesController do context 'for user' do before(:each) do http_login(@issue.user) - @count = Issue.count end it 'can create issue in own project' do - post :create, @create_params - Issue.count.should == @count+1 + lambda { post :create, @create_params}.should change{ Issue.count }.by(1) end it 'can create issue in own hidden project' do - post :create, @create_params.merge(:project_id => @own_hidden_project.id) - Issue.count.should == @count+1 + lambda { post :create, @create_params.merge(:project_id => @own_hidden_project.id)}.should change{ Issue.count }.by(1) end it 'can create issue in open project' do - post :create, @create_params.merge(:project_id => @open_project.id) - Issue.count.should == @count+1 + lambda { post :create, @create_params.merge(:project_id => @open_project.id)}.should change{ Issue.count }.by(1) end it 'cant create issue in hidden project' do - post :create, @create_params.merge(:project_id => @hidden_project.id) - Issue.count.should == @count + lambda { post :create, @create_params.merge(:project_id => @hidden_project.id)}.should change{ Issue.count }.by(0) end end context 'for anonymous user' do - before(:each) do - @count = Issue.count - end it 'cant create issue in project', :anonymous_access => true do - post :create, @create_params - Issue.count.should == @count + lambda { post :create, @create_params}.should change{ Issue.count }.by(0) end it 'cant create issue in hidden project', :anonymous_access => true do - post :create, @create_params.merge(:project_id => @hidden_project.id) - Issue.count.should == @count + lambda { post :create, @create_params.merge(:project_id => @hidden_project.id)}.should change{ Issue.count }.by(0) end end end @@ -189,6 +178,7 @@ describe Api::V1::IssuesController do end end end + after(:all) do User.destroy_all Platform.destroy_all From b5cc162763596a5b0707ec854231834e9bd26972 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Fri, 28 Jun 2013 19:20:34 +0600 Subject: [PATCH 14/17] [#105] fix authorize resouce --- app/controllers/api/v1/pull_requests_controller.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/v1/pull_requests_controller.rb b/app/controllers/api/v1/pull_requests_controller.rb index 77e5f2ae8..72b1746c1 100644 --- a/app/controllers/api/v1/pull_requests_controller.rb +++ b/app/controllers/api/v1/pull_requests_controller.rb @@ -7,8 +7,8 @@ class Api::V1::PullRequestsController < Api::V1::BaseController load_resource :group, :only => :group_index, :find_by => :id, :parent => false load_resource :project - load_resource :issue, :through => :project, :find_by => :serial_id, :parent => false, :only => [:show, :index, :commits, :files, :merge] - load_and_authorize_resource :instance_name => :pull, :through => :issue, :singleton => true, :only => [:show, :index, :commits, :files, :merge] + load_resource :issue, :through => :project, :find_by => :serial_id, :parent => false, :only => [:show, :index, :commits, :files, :merge, :update] + load_and_authorize_resource :instance_name => :pull, :through => :issue, :singleton => true, :only => [:show, :index, :commits, :files, :merge, :update] def index @pulls = @project.pull_requests From 5bb598ec817e4852478d2ef76f5c6b27b548c050 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Fri, 28 Jun 2013 19:38:58 +0600 Subject: [PATCH 15/17] [#105] fix assignee --- app/controllers/api/v1/pull_requests_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/api/v1/pull_requests_controller.rb b/app/controllers/api/v1/pull_requests_controller.rb index 72b1746c1..ef9bc0a15 100644 --- a/app/controllers/api/v1/pull_requests_controller.rb +++ b/app/controllers/api/v1/pull_requests_controller.rb @@ -50,7 +50,7 @@ class Api::V1::PullRequestsController < Api::V1::BaseController @pull.build_issue :title => pull_params[:title], :body => pull_params[:body] @pull.from_project = @project @pull.to_ref, @pull.from_ref = pull_params[:to_ref], pull_params[:from_ref] - @pull.issue.assignee_id = pull_params[:assignee] if can?(:write, @project) + @pull.issue.assignee_id = pull_params[:assignee_id] if can?(:write, @project) @pull.issue.user, @pull.issue.project = current_user, @project render_validation_error(@pull, "#{@pull.class.name} has not been created") && return unless @pull.valid? From 8a6214d0966fc17579398e34cc6c2344cd0cc2d9 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Fri, 28 Jun 2013 20:02:02 +0600 Subject: [PATCH 16/17] small refactoring of the issue controller --- app/controllers/api/v1/issues_controller.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/api/v1/issues_controller.rb b/app/controllers/api/v1/issues_controller.rb index a69027bc2..b59e39cc5 100644 --- a/app/controllers/api/v1/issues_controller.rb +++ b/app/controllers/api/v1/issues_controller.rb @@ -49,10 +49,9 @@ class Api::V1::IssuesController < Api::V1::BaseController end if params[:issue] end @issue.labelings.destroy_all if params[:update_labels] - if params[:issue] && status = params[:issue][:status] + if params[:issue] && status = params[:issue].delete(:status) @issue.set_close(current_user) if status == 'closed' @issue.set_open if status == 'open' - params[:issue].delete :status end update_subject @issue end From d1555fa1775100535cb059c51dc75ecaff96b27c Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Fri, 28 Jun 2013 22:15:17 +0600 Subject: [PATCH 17/17] [#105] small spec refactoring --- spec/controllers/api/v1/issues_controller_spec.rb | 2 +- spec/controllers/api/v1/pull_requests_controller.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/controllers/api/v1/issues_controller_spec.rb b/spec/controllers/api/v1/issues_controller_spec.rb index 710b5410c..4b60910f7 100644 --- a/spec/controllers/api/v1/issues_controller_spec.rb +++ b/spec/controllers/api/v1/issues_controller_spec.rb @@ -75,7 +75,7 @@ describe Api::V1::IssuesController do it 'should return only assigned issue' do get :user_index, :format => :json assigns[:issues].should include(@own_hidden_issue) - assigns[:issues].count.should == 1 + assigns[:issues].should have(1).item end it 'should render right template for user index action' do diff --git a/spec/controllers/api/v1/pull_requests_controller.rb b/spec/controllers/api/v1/pull_requests_controller.rb index e7bcd8aad..f7e1c9ea9 100644 --- a/spec/controllers/api/v1/pull_requests_controller.rb +++ b/spec/controllers/api/v1/pull_requests_controller.rb @@ -86,7 +86,7 @@ describe Api::V1::PullRequestsController do it 'should return only assigned pull request' do get :user_index, :format => :json assigns[:pulls].should include(@own_hidden_pull) - assigns[:pulls].count.should == 1 + assigns[:pulls].should have(1).item end it 'should render right template for user index action' do