From e343ba90aa83c487178c437b9a9a2a4c790341bb Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Wed, 19 Jun 2013 01:45:40 +0600 Subject: [PATCH 01/26] [#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/26] 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/26] [#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/26] [#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/26] [#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/26] 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/26] [#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/26] 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/26] [#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/26] [#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/26] [#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 eeed2a49b75e11ca3f173e28be13f9da89d8b2ad Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Thu, 27 Jun 2013 17:06:42 +0400 Subject: [PATCH 12/26] #200: moved observer of Issue to lib --- Gemfile | 1 + Gemfile.lock | 2 + app/models/activity_feed_observer.rb | 78 ++++++++++--------- app/models/issue.rb | 1 + lib/modules/observers/activity_feed/issue.rb | 59 ++++++++++++++ .../lib/observers/activity_feed/issue_spec.rb | 30 +++++++ spec/mailers/user_mailer_spec.rb | 1 - spec/models/activity_feed_observer_spec.rb | 6 -- 8 files changed, 134 insertions(+), 44 deletions(-) create mode 100644 lib/modules/observers/activity_feed/issue.rb create mode 100644 spec/lib/observers/activity_feed/issue_spec.rb delete mode 100644 spec/models/activity_feed_observer_spec.rb diff --git a/Gemfile b/Gemfile index c9423e923..e55eacfff 100644 --- a/Gemfile +++ b/Gemfile @@ -95,5 +95,6 @@ group :test do gem 'rr', '~> 1.0.4' gem 'shoulda' gem 'mock_redis', '0.6.2' + gem 'test_after_commit', '0.2.0' gem 'rake' end diff --git a/Gemfile.lock b/Gemfile.lock index dfe789d3c..b5c710783 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -363,6 +363,7 @@ GEM state_machine (1.1.2) stringex (1.4.0) systemu (2.5.2) + test_after_commit (0.2.0) therubyracer (0.10.2) libv8 (~> 3.3.10) thin (1.5.1) @@ -466,6 +467,7 @@ DEPENDENCIES shotgun shoulda state_machine + test_after_commit (= 0.2.0) therubyracer (~> 0.10.2) therubyrhino (~> 1.73.1) trinidad (~> 1.0.2) diff --git a/app/models/activity_feed_observer.rb b/app/models/activity_feed_observer.rb index 85ca47126..ae3c1ae05 100644 --- a/app/models/activity_feed_observer.rb +++ b/app/models/activity_feed_observer.rb @@ -1,6 +1,6 @@ # -*- encoding : utf-8 -*- class ActivityFeedObserver < ActiveRecord::Observer - observe :issue, :comment, :user, :build_list + observe :comment, :user, :build_list BUILD_LIST_STATUSES = [ BuildList::BUILD_PUBLISHED, @@ -10,7 +10,11 @@ class ActivityFeedObserver < ActiveRecord::Observer BuildList::TESTS_FAILED ].freeze - def after_create(record) + def after_commit(record) + # See: + # - http://rails-bestpractices.com/posts/695-use-after_commit + # - https://coderwall.com/p/f5-vlq + return unless record.send(:transaction_include_action?, :create) case record.class.to_s when 'User' ActivityFeed.create( @@ -19,29 +23,29 @@ class ActivityFeedObserver < ActiveRecord::Observer :data => {:user_name => record.user_appeal, :user_email => record.email} ) - when 'Issue' - record.collect_recipients.each do |recipient| - next if record.user_id == recipient.id - UserMailer.new_issue_notification(record, recipient).deliver if recipient.notifier.can_notify && recipient.notifier.new_issue - ActivityFeed.create( - :user => recipient, - :kind => 'new_issue_notification', - :data => {:user_name => record.user.name, :user_email => record.user.email, :user_id => record.user_id,:issue_serial_id => record.serial_id, - :issue_title => record.title, :project_id => record.project.id, :project_name => record.project.name, :project_owner => record.project.owner.uname} - ) - end + # when 'Issue' + # record.collect_recipients.each do |recipient| + # next if record.user_id == recipient.id + # UserMailer.new_issue_notification(record, recipient).deliver if recipient.notifier.can_notify && recipient.notifier.new_issue + # ActivityFeed.create( + # :user => recipient, + # :kind => 'new_issue_notification', + # :data => {:user_name => record.user.name, :user_email => record.user.email, :user_id => record.user_id,:issue_serial_id => record.serial_id, + # :issue_title => record.title, :project_id => record.project.id, :project_name => record.project.name, :project_owner => record.project.owner.uname} + # ) + # end - if record.assignee_id_changed? - UserMailer.new_issue_notification(record, record.assignee).deliver if record.assignee.notifier.issue_assign && record.assignee.notifier.can_notify - ActivityFeed.create( - :user => record.user, - :kind => 'issue_assign_notification', - :data => {:user_name => record.user.name, :user_email => record.user.email, :user_id => record.user_id, :issue_serial_id => record.serial_id, - :project_id => record.project.id, :issue_title => record.title, :project_name => record.project.name, :project_owner => record.project.owner.uname} - ) - end - record.project.hooks.each{ |h| h.receive_issues(record, :create) } - Comment.create_link_on_issues_from_item(record) + # if record.assignee_id_changed? + # UserMailer.new_issue_notification(record, record.assignee).deliver if record.assignee.notifier.issue_assign && record.assignee.notifier.can_notify + # ActivityFeed.create( + # :user => record.user, + # :kind => 'issue_assign_notification', + # :data => {:user_name => record.user.name, :user_email => record.user.email, :user_id => record.user_id, :issue_serial_id => record.serial_id, + # :project_id => record.project.id, :issue_title => record.title, :project_name => record.project.name, :project_owner => record.project.owner.uname} + # ) + # end + # record.project.hooks.each{ |h| h.receive_issues(record, :create) } + # Comment.create_link_on_issues_from_item(record) when 'Comment' return if record.automatic if record.issue_comment? @@ -138,19 +142,19 @@ class ActivityFeedObserver < ActiveRecord::Observer def after_update(record) case record.class.to_s - when 'Issue' - if record.assignee_id && record.assignee_id_changed? - UserMailer.issue_assign_notification(record, record.assignee).deliver if record.assignee.notifier.issue_assign && record.assignee.notifier.can_notify - ActivityFeed.create( - :user => record.assignee, - :kind => 'issue_assign_notification', - :data => {:user_name => record.assignee.name, :user_email => record.assignee.email, :issue_serial_id => record.serial_id, :issue_title => record.title, - :project_id => record.project.id, :project_name => record.project.name, :project_owner => record.project.owner.uname} - ) - end - record.project.hooks.each{ |h| h.receive_issues(record, :update) } if record.status_changed? - # dont remove outdated issues link - Comment.create_link_on_issues_from_item(record) + # when 'Issue' + # if record.assignee_id && record.assignee_id_changed? + # UserMailer.issue_assign_notification(record, record.assignee).deliver if record.assignee.notifier.issue_assign && record.assignee.notifier.can_notify + # ActivityFeed.create( + # :user => record.assignee, + # :kind => 'issue_assign_notification', + # :data => {:user_name => record.assignee.name, :user_email => record.assignee.email, :issue_serial_id => record.serial_id, :issue_title => record.title, + # :project_id => record.project.id, :project_name => record.project.name, :project_owner => record.project.owner.uname} + # ) + # end + # record.project.hooks.each{ |h| h.receive_issues(record, :update) } if record.status_changed? + # # dont remove outdated issues link + # Comment.create_link_on_issues_from_item(record) when 'BuildList' if record.mass_build.blank? && ( # Do not show mass build activity in activity feeds record.status_changed? && BUILD_LIST_STATUSES.include?(record.status) || diff --git a/app/models/issue.rb b/app/models/issue.rb index c9236a489..9e043eb0a 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -1,5 +1,6 @@ # -*- encoding : utf-8 -*- class Issue < ActiveRecord::Base + include Modules::Observers::ActivityFeed::Issue STATUSES = ['open', 'closed'] belongs_to :project diff --git a/lib/modules/observers/activity_feed/issue.rb b/lib/modules/observers/activity_feed/issue.rb new file mode 100644 index 000000000..d2ed1d016 --- /dev/null +++ b/lib/modules/observers/activity_feed/issue.rb @@ -0,0 +1,59 @@ +# -*- encoding : utf-8 -*- +module Modules::Observers::ActivityFeed::Issue + extend ActiveSupport::Concern + + included do + after_commit :new_issue_notifications, :on => :create + after_update -> { send_assign_notifications(:update) } + end + + private + + def new_issue_notifications + collect_recipients.each do |recipient| + next if user_id == recipient.id + UserMailer.new_issue_notification(self, recipient).deliver if recipient.notifier.can_notify && recipient.notifier.new_issue + ActivityFeed.create( + :user => recipient, + :kind => 'new_issue_notification', + :data => { + :user_name => user.name, + :user_email => user.email, + :user_id => user_id, + :issue_serial_id => serial_id, + :issue_title => title, + :project_id => project.id, + :project_name => project.name, + :project_owner => project.owner.uname + } + ) + end + send_assign_notifications + end + + def send_assign_notifications(action = :create) + if assignee_id && assignee_id_changed? + if assignee.notifier.issue_assign && assignee.notifier.can_notify + user_mailer_action = action == :create ? :new_issue_notification : :issue_assign_notification + UserMailer.send(user_mailer_action, self, assignee).deliver + end + ActivityFeed.create( + :user => assignee, + :kind => 'issue_assign_notification', + :data => { + :user_name => assignee.name, + :user_email => assignee.email, + :issue_serial_id => serial_id, + :issue_title => title, + :project_id => project.id, + :project_name => project.name, + :project_owner => project.owner.uname + } + ) + end + project.hooks.each{ |h| h.receive_issues(self, action) } if action == :create || status_changed? + # dont remove outdated issues link + Comment.create_link_on_issues_from_item(self) + end + +end \ No newline at end of file diff --git a/spec/lib/observers/activity_feed/issue_spec.rb b/spec/lib/observers/activity_feed/issue_spec.rb new file mode 100644 index 000000000..37e9d2730 --- /dev/null +++ b/spec/lib/observers/activity_feed/issue_spec.rb @@ -0,0 +1,30 @@ +# -*- encoding : utf-8 -*- +require 'spec_helper' + +describe Modules::Observers::ActivityFeed::Issue do + before { stub_symlink_methods } + + it 'sends a notification email after create' do + issue = FactoryGirl.build(:issue, :assignee => nil) + mailer = mock!.deliver + mock(UserMailer).new_issue_notification(issue, issue.project.owner) { mailer } + issue.save + end + + it 'does not send a notification email after update' do + issue = FactoryGirl.create(:issue, :assignee => nil) + issue.title = 'new title' + dont_allow(UserMailer).new_issue_notification + issue.save + end + + it 'sends a notification email after a assignee of issue has been changed' do + user = FactoryGirl.create(:user) + issue = FactoryGirl.build(:issue, :assignee => nil) + issue.assignee = user + mailer = mock!.deliver + mock(UserMailer).issue_assign_notification(issue, user) { mailer } + issue.save + end + +end diff --git a/spec/mailers/user_mailer_spec.rb b/spec/mailers/user_mailer_spec.rb index 72c8674ec..17dacb5a8 100644 --- a/spec/mailers/user_mailer_spec.rb +++ b/spec/mailers/user_mailer_spec.rb @@ -2,7 +2,6 @@ require "spec_helper" describe UserMailer do - pending "add some examples to (or delete) #{__FILE__}" context 'On Issue create' do before(:each) do diff --git a/spec/models/activity_feed_observer_spec.rb b/spec/models/activity_feed_observer_spec.rb deleted file mode 100644 index f008468e9..000000000 --- a/spec/models/activity_feed_observer_spec.rb +++ /dev/null @@ -1,6 +0,0 @@ -# -*- encoding : utf-8 -*- -require 'spec_helper' - -describe ActivityFeedObserver do - pending "add some examples to (or delete) #{__FILE__}" -end From 8782129985e6d03628b10fa80482f7c433280498 Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Thu, 27 Jun 2013 18:37:41 +0400 Subject: [PATCH 13/26] #200: moved User and BuildList observers --- app/models/activity_feed_observer.rb | 128 +----------------- app/models/build_list.rb | 1 + app/models/comment.rb | 6 +- app/models/user.rb | 1 + .../observers/activity_feed/build_list.rb | 46 +++++++ .../observers/activity_feed/comment.rb | 59 ++++++++ lib/modules/observers/activity_feed/user.rb | 19 +++ spec/factories/comments.rb | 1 + .../observers/activity_feed/comment_spec.rb | 14 ++ 9 files changed, 144 insertions(+), 131 deletions(-) create mode 100644 lib/modules/observers/activity_feed/build_list.rb create mode 100644 lib/modules/observers/activity_feed/comment.rb create mode 100644 lib/modules/observers/activity_feed/user.rb create mode 100644 spec/lib/observers/activity_feed/comment_spec.rb diff --git a/app/models/activity_feed_observer.rb b/app/models/activity_feed_observer.rb index ae3c1ae05..532b945dc 100644 --- a/app/models/activity_feed_observer.rb +++ b/app/models/activity_feed_observer.rb @@ -1,88 +1,8 @@ # -*- encoding : utf-8 -*- class ActivityFeedObserver < ActiveRecord::Observer - observe :comment, :user, :build_list - BUILD_LIST_STATUSES = [ - BuildList::BUILD_PUBLISHED, - BuildList::SUCCESS, - BuildList::BUILD_ERROR, - BuildList::FAILED_PUBLISH, - BuildList::TESTS_FAILED - ].freeze - - def after_commit(record) - # See: - # - http://rails-bestpractices.com/posts/695-use-after_commit - # - https://coderwall.com/p/f5-vlq - return unless record.send(:transaction_include_action?, :create) + def after_create(record) case record.class.to_s - when 'User' - ActivityFeed.create( - :user => record, - :kind => 'new_user_notification', - :data => {:user_name => record.user_appeal, :user_email => record.email} - ) - - # when 'Issue' - # record.collect_recipients.each do |recipient| - # next if record.user_id == recipient.id - # UserMailer.new_issue_notification(record, recipient).deliver if recipient.notifier.can_notify && recipient.notifier.new_issue - # ActivityFeed.create( - # :user => recipient, - # :kind => 'new_issue_notification', - # :data => {:user_name => record.user.name, :user_email => record.user.email, :user_id => record.user_id,:issue_serial_id => record.serial_id, - # :issue_title => record.title, :project_id => record.project.id, :project_name => record.project.name, :project_owner => record.project.owner.uname} - # ) - # end - - # if record.assignee_id_changed? - # UserMailer.new_issue_notification(record, record.assignee).deliver if record.assignee.notifier.issue_assign && record.assignee.notifier.can_notify - # ActivityFeed.create( - # :user => record.user, - # :kind => 'issue_assign_notification', - # :data => {:user_name => record.user.name, :user_email => record.user.email, :user_id => record.user_id, :issue_serial_id => record.serial_id, - # :project_id => record.project.id, :issue_title => record.title, :project_name => record.project.name, :project_owner => record.project.owner.uname} - # ) - # end - # record.project.hooks.each{ |h| h.receive_issues(record, :create) } - # Comment.create_link_on_issues_from_item(record) - when 'Comment' - return if record.automatic - if record.issue_comment? - subscribes = record.commentable.subscribes - subscribes.each do |subscribe| - if record.user_id != subscribe.user_id - UserMailer.new_comment_notification(record, subscribe.user).deliver if record.can_notify_on_new_comment?(subscribe) - ActivityFeed.create( - :user => subscribe.user, - :kind => 'new_comment_notification', - :data => {:user_name => record.user.name, :user_email => record.user.email, :user_id => record.user_id, :comment_body => record.body, - :issue_title => record.commentable.title, :issue_serial_id => record.commentable.serial_id, :project_id => record.commentable.project.id, - :comment_id => record.id, :project_name => record.project.name, :project_owner => record.project.owner.uname} - ) - end - end - elsif record.commit_comment? - subscribes = Subscribe.comment_subscribes(record).where(:status => true) - subscribes.each do |subscribe| - next if record.own_comment?(subscribe.user) - if subscribe.user.notifier.can_notify and - ( (subscribe.project.owner?(subscribe.user) && subscribe.user.notifier.new_comment_commit_repo_owner) or - (subscribe.user.commentor?(record.commentable) && subscribe.user.notifier.new_comment_commit_commentor) or - (subscribe.user.committer?(record.commentable) && subscribe.user.notifier.new_comment_commit_owner) ) - UserMailer.new_comment_notification(record, subscribe.user).deliver - end - ActivityFeed.create( - :user => subscribe.user, - :kind => 'new_comment_commit_notification', - :data => {:user_name => record.user.name, :user_email => record.user.email, :user_id => record.user_id, :comment_body => record.body, - :commit_message => record.commentable.message, :commit_id => record.commentable.id, - :project_id => record.project.id, :comment_id => record.id, :project_name => record.project.name, :project_owner => record.project.owner.uname} - ) - end - end - Comment.create_link_on_issues_from_item(record) - when 'GitHook' return unless record.project PullRequest.where("from_project_id = ? OR to_project_id = ?", record.project, record.project).needed_checking.each {|pull| pull.check} @@ -140,50 +60,4 @@ class ActivityFeedObserver < ActiveRecord::Observer end end - def after_update(record) - case record.class.to_s - # when 'Issue' - # if record.assignee_id && record.assignee_id_changed? - # UserMailer.issue_assign_notification(record, record.assignee).deliver if record.assignee.notifier.issue_assign && record.assignee.notifier.can_notify - # ActivityFeed.create( - # :user => record.assignee, - # :kind => 'issue_assign_notification', - # :data => {:user_name => record.assignee.name, :user_email => record.assignee.email, :issue_serial_id => record.serial_id, :issue_title => record.title, - # :project_id => record.project.id, :project_name => record.project.name, :project_owner => record.project.owner.uname} - # ) - # end - # record.project.hooks.each{ |h| h.receive_issues(record, :update) } if record.status_changed? - # # dont remove outdated issues link - # Comment.create_link_on_issues_from_item(record) - when 'BuildList' - if record.mass_build.blank? && ( # Do not show mass build activity in activity feeds - record.status_changed? && BUILD_LIST_STATUSES.include?(record.status) || - record.status == BuildList::BUILD_PENDING && record.bs_id_changed? - ) - - record.project.admins.each do |recipient| - user = record.publisher || record.user - ActivityFeed.create( - :user => recipient, - :kind => 'build_list_notification', - :data => { - :task_num => record.bs_id, - :build_list_id => record.id, - :status => record.status, - :updated_at => record.updated_at, - :project_id => record.project_id, - :project_name => record.project.name, - :project_owner => record.project.owner.uname, - :user_name => user.name, - :user_email => user.email, - :user_id => user.id - } - ) - end - end - when 'Comment' - # dont remove outdated issues link - Comment.create_link_on_issues_from_item(record) - end - end end diff --git a/app/models/build_list.rb b/app/models/build_list.rb index 7ab46c41c..adf967c07 100644 --- a/app/models/build_list.rb +++ b/app/models/build_list.rb @@ -3,6 +3,7 @@ class BuildList < ActiveRecord::Base include Modules::Models::CommitAndVersion include Modules::Models::FileStoreClean include AbfWorker::ModelHelper + include Modules::Observers::ActivityFeed::BuildList belongs_to :project belongs_to :arch diff --git a/app/models/comment.rb b/app/models/comment.rb index f78148b86..7272d6a9b 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -1,5 +1,7 @@ # -*- encoding : utf-8 -*- class Comment < ActiveRecord::Base + include Modules::Observers::ActivityFeed::Comment + # regexp take from http://code.google.com/p/concerto-platform/source/browse/v3/cms/lib/CodeMirror/mode/gfm/gfm.js?spec=svn861&r=861#71 # User/Project#Num # User#Num @@ -54,10 +56,6 @@ class Comment < ActiveRecord::Base user_id == user.id end - def can_notify_on_new_comment?(subscribe) - User.find(subscribe.user).notifier.new_comment && User.find(subscribe.user).notifier.can_notify - end - def actual_inline_comment?(diff = nil, force = false) unless force raise "This is not inline comment!" if data.blank? # for debug diff --git a/app/models/user.rb b/app/models/user.rb index a53f530e1..1c4322cc2 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -60,6 +60,7 @@ class User < Avatar include Modules::Models::PersonalRepository include Modules::Models::ActsLikeMember + include Modules::Observers::ActivityFeed::User def admin? role == 'admin' diff --git a/lib/modules/observers/activity_feed/build_list.rb b/lib/modules/observers/activity_feed/build_list.rb new file mode 100644 index 000000000..ca904a788 --- /dev/null +++ b/lib/modules/observers/activity_feed/build_list.rb @@ -0,0 +1,46 @@ +# -*- encoding : utf-8 -*- +module Modules::Observers::ActivityFeed::BuildList + extend ActiveSupport::Concern + + included do + after_update :build_list_notifications + end + + private + + def build_list_notifications + if mass_build.blank? && ( # Do not show mass build activity in activity feeds + status_changed? && [ + BUILD_PUBLISHED, + SUCCESS, + BUILD_ERROR, + FAILED_PUBLISH, + TESTS_FAILED + ].include?(status) || + status == BUILD_PENDING && bs_id_changed? + ) + + updater = publisher || user + project.admins.each do |recipient| + ActivityFeed.create( + :user => recipient, + :kind => 'build_list_notification', + :data => { + :task_num => bs_id, + :build_list_id => id, + :status => status, + :updated_at => updated_at, + :project_id => project_id, + :project_name => project.name, + :project_owner => project.owner.uname, + :user_name => updater.name, + :user_email => updater.email, + :user_id => updater.id + } + ) + end + + end + end + +end \ No newline at end of file diff --git a/lib/modules/observers/activity_feed/comment.rb b/lib/modules/observers/activity_feed/comment.rb new file mode 100644 index 000000000..920169b4d --- /dev/null +++ b/lib/modules/observers/activity_feed/comment.rb @@ -0,0 +1,59 @@ +# -*- encoding : utf-8 -*- +module Modules::Observers::ActivityFeed::Comment + extend ActiveSupport::Concern + + included do + after_commit :new_comment_notifications, :on => :create, :unless => :automatic? + # dont remove outdated issues link + after_update -> { Comment.create_link_on_issues_from_item(self) } + end + + private + + def new_comment_notifications + if issue_comment? + commentable.subscribes.each do |subscribe| + if user_id != subscribe.user_id + UserMailer.new_comment_notification(self, subscribe.user).deliver if can_notify_on_new_comment?(subscribe) + send_new_comment_commit_notification(subscribe) + end + end + elsif commit_comment? + Subscribe.comment_subscribes(self).where(:status => true).each do |subscribe| + next if own_comment?(subscribe.user) + if subscribe.user.notifier.can_notify and + ( (subscribe.project.owner?(subscribe.user) && subscribe.user.notifier.new_comment_commit_repo_owner) or + (subscribe.user.commentor?(self.commentable) && subscribe.user.notifier.new_comment_commit_commentor) or + (subscribe.user.committer?(self.commentable) && subscribe.user.notifier.new_comment_commit_owner) ) + UserMailer.new_comment_notification(self, subscribe.user).deliver + end + send_new_comment_commit_notification(subscribe) + end + end + Comment.create_link_on_issues_from_item(self) + end + + def send_new_comment_commit_notification(subscribe) + ActivityFeed.create( + :user => subscribe.user, + :kind => 'new_comment_commit_notification', + :data => { + :user_name => user.name, + :user_email => user.email, + :user_id => user_id, + :comment_body => body, + :commit_message => commentable.message, + :commit_id => commentable.id, + :project_id => project.id, + :comment_id => id, + :project_name => project.name, + :project_owner => project.owner.uname + } + ) + end + + def can_notify_on_new_comment?(subscribe) + User.find(subscribe.user).notifier.new_comment && User.find(subscribe.user).notifier.can_notify + end + +end \ No newline at end of file diff --git a/lib/modules/observers/activity_feed/user.rb b/lib/modules/observers/activity_feed/user.rb new file mode 100644 index 000000000..971378b30 --- /dev/null +++ b/lib/modules/observers/activity_feed/user.rb @@ -0,0 +1,19 @@ +# -*- encoding : utf-8 -*- +module Modules::Observers::ActivityFeed::User + extend ActiveSupport::Concern + + included do + after_commit :new_user_notification, :on => :create + end + + private + + def new_user_notification + ActivityFeed.create( + :user => self, + :kind => 'new_user_notification', + :data => {:user_name => self.user_appeal, :user_email => self.email} + ) + end + +end \ No newline at end of file diff --git a/spec/factories/comments.rb b/spec/factories/comments.rb index 145b50ca6..76119494c 100644 --- a/spec/factories/comments.rb +++ b/spec/factories/comments.rb @@ -4,5 +4,6 @@ FactoryGirl.define do body { FactoryGirl.generate(:string) } association :user, :factory => :user association :commentable, :factory => :issue + project { |c| c.commentable.project } end end diff --git a/spec/lib/observers/activity_feed/comment_spec.rb b/spec/lib/observers/activity_feed/comment_spec.rb new file mode 100644 index 000000000..139e9e114 --- /dev/null +++ b/spec/lib/observers/activity_feed/comment_spec.rb @@ -0,0 +1,14 @@ +# -*- encoding : utf-8 -*- +require 'spec_helper' + +describe Modules::Observers::ActivityFeed::Comment do + before { stub_symlink_methods } + + it 'sends a notification email after create a issue comment' do + comment = FactoryGirl.build(:comment) + mailer = mock!.deliver + mock(UserMailer).new_comment_notification(comment, comment.commentable.assignee) { mailer } + comment.save + end + +end From 66fe5c11cfb2d567527dc4cd18ebca7b3282bb10 Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Thu, 27 Jun 2013 18:40:19 +0400 Subject: [PATCH 14/26] #200: removed activity_feed_observer --- app/models/git_hook.rb | 2 +- config/application.rb | 2 +- lib/modules/models/git.rb | 2 +- .../modules/observers/activity_feed/git.rb | 8 +++++--- 4 files changed, 8 insertions(+), 6 deletions(-) rename app/models/activity_feed_observer.rb => lib/modules/observers/activity_feed/git.rb (96%) diff --git a/app/models/git_hook.rb b/app/models/git_hook.rb index 0aef7df8a..d13829002 100644 --- a/app/models/git_hook.rb +++ b/app/models/git_hook.rb @@ -65,7 +65,7 @@ class GitHook end def self.process(*args) - ActivityFeedObserver.instance.after_create(args.size > 1 ? GitHook.new(*args) : args.first) + Modules::Observers::ActivityFeed::Git.create_notifications(args.size > 1 ? GitHook.new(*args) : args.first) end def find_user(user) diff --git a/config/application.rb b/config/application.rb index 13f0a15c1..c51dde67b 100644 --- a/config/application.rb +++ b/config/application.rb @@ -34,7 +34,7 @@ module Rosa # config.plugins = [ :exception_notification, :ssl_requirement, :all ] # Activate observers that should always be running. - config.active_record.observers = :event_log_observer, :activity_feed_observer, :build_list_observer + config.active_record.observers = :event_log_observer, :build_list_observer # Set Time.zone default to the specified zone and make Active Record auto-convert to this zone. # Run "rake -D time" for a list of tasks for finding time zone names. Default is UTC. diff --git a/lib/modules/models/git.rb b/lib/modules/models/git.rb index 43cc16965..6d96bd851 100644 --- a/lib/modules/models/git.rb +++ b/lib/modules/models/git.rb @@ -150,7 +150,7 @@ module Modules module ClassMethods def process_hook(owner_uname, repo, newrev, oldrev, ref, newrev_type, user = nil, message = nil) rec = GitHook.new(owner_uname, repo, newrev, oldrev, ref, newrev_type, user, message) - ActivityFeedObserver.instance.after_create rec + Modules::Observers::ActivityFeed::Git.create_notifications rec end end end diff --git a/app/models/activity_feed_observer.rb b/lib/modules/observers/activity_feed/git.rb similarity index 96% rename from app/models/activity_feed_observer.rb rename to lib/modules/observers/activity_feed/git.rb index 532b945dc..cf5b19ebd 100644 --- a/app/models/activity_feed_observer.rb +++ b/lib/modules/observers/activity_feed/git.rb @@ -1,7 +1,8 @@ # -*- encoding : utf-8 -*- -class ActivityFeedObserver < ActiveRecord::Observer +module Modules::Observers::ActivityFeed::Git + + def self.create_notifications(record) - def after_create(record) case record.class.to_s when 'GitHook' return unless record.project @@ -58,6 +59,7 @@ class ActivityFeedObserver < ActiveRecord::Observer ) end end + end -end +end \ No newline at end of file From 0bd7b042c43ce3f9f7f15d2b47f7d3b6f138ee1f Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Thu, 27 Jun 2013 19:00:56 +0400 Subject: [PATCH 15/26] #200: fixed: uninitialized constant --- lib/modules/observers/activity_feed/build_list.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/modules/observers/activity_feed/build_list.rb b/lib/modules/observers/activity_feed/build_list.rb index ca904a788..1873f01e4 100644 --- a/lib/modules/observers/activity_feed/build_list.rb +++ b/lib/modules/observers/activity_feed/build_list.rb @@ -11,13 +11,13 @@ module Modules::Observers::ActivityFeed::BuildList def build_list_notifications if mass_build.blank? && ( # Do not show mass build activity in activity feeds status_changed? && [ - BUILD_PUBLISHED, - SUCCESS, - BUILD_ERROR, - FAILED_PUBLISH, - TESTS_FAILED + BuildList::BUILD_PUBLISHED, + BuildList::SUCCESS, + BuildList::BUILD_ERROR, + BuildList::FAILED_PUBLISH, + BuildList::TESTS_FAILED ].include?(status) || - status == BUILD_PENDING && bs_id_changed? + status == BuildList::BUILD_PENDING && bs_id_changed? ) updater = publisher || user From 48bd3455737e218b4fc6f62808501ad00046e6cf Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Thu, 27 Jun 2013 20:18:53 +0400 Subject: [PATCH 16/26] #200: rollback some changes --- .../observers/activity_feed/comment.rb | 52 +++++++++++-------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/lib/modules/observers/activity_feed/comment.rb b/lib/modules/observers/activity_feed/comment.rb index 920169b4d..8a12429a5 100644 --- a/lib/modules/observers/activity_feed/comment.rb +++ b/lib/modules/observers/activity_feed/comment.rb @@ -15,7 +15,22 @@ module Modules::Observers::ActivityFeed::Comment commentable.subscribes.each do |subscribe| if user_id != subscribe.user_id UserMailer.new_comment_notification(self, subscribe.user).deliver if can_notify_on_new_comment?(subscribe) - send_new_comment_commit_notification(subscribe) + ActivityFeed.create( + :user => subscribe.user, + :kind => 'new_comment_notification', + :data => { + :user_name => user.name, + :user_email => user.email, + :user_id => user_id, + :comment_body => body, + :issue_title => commentable.title, + :issue_serial_id => commentable.serial_id, + :project_id => commentable.project.id, + :comment_id => id, + :project_name => project.name, + :project_owner => project.owner.uname + } + ) end end elsif commit_comment? @@ -27,31 +42,26 @@ module Modules::Observers::ActivityFeed::Comment (subscribe.user.committer?(self.commentable) && subscribe.user.notifier.new_comment_commit_owner) ) UserMailer.new_comment_notification(self, subscribe.user).deliver end - send_new_comment_commit_notification(subscribe) + ActivityFeed.create( + :user => subscribe.user, + :kind => 'new_comment_commit_notification', + :data => { + :user_name => user.name, + :user_email => user.email, + :user_id => user_id, + :comment_body => body, + :commit_message => commentable.message, + :commit_id => commentable.id, + :project_id => project.id, + :comment_id => id, + :project_name => project.name, + :project_owner => project.owner.uname} + ) end end Comment.create_link_on_issues_from_item(self) end - def send_new_comment_commit_notification(subscribe) - ActivityFeed.create( - :user => subscribe.user, - :kind => 'new_comment_commit_notification', - :data => { - :user_name => user.name, - :user_email => user.email, - :user_id => user_id, - :comment_body => body, - :commit_message => commentable.message, - :commit_id => commentable.id, - :project_id => project.id, - :comment_id => id, - :project_name => project.name, - :project_owner => project.owner.uname - } - ) - end - def can_notify_on_new_comment?(subscribe) User.find(subscribe.user).notifier.new_comment && User.find(subscribe.user).notifier.can_notify end From 8d3d3c36f9e2ecd1e47398c83bdd4eb88e1aea54 Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Thu, 27 Jun 2013 20:53:14 +0400 Subject: [PATCH 17/26] #200: updated specs --- Gemfile | 1 - Gemfile.lock | 2 -- spec/lib/observers/activity_feed/comment_spec.rb | 2 ++ spec/lib/observers/activity_feed/issue_spec.rb | 2 ++ 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index e55eacfff..c9423e923 100644 --- a/Gemfile +++ b/Gemfile @@ -95,6 +95,5 @@ group :test do gem 'rr', '~> 1.0.4' gem 'shoulda' gem 'mock_redis', '0.6.2' - gem 'test_after_commit', '0.2.0' gem 'rake' end diff --git a/Gemfile.lock b/Gemfile.lock index b5c710783..dfe789d3c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -363,7 +363,6 @@ GEM state_machine (1.1.2) stringex (1.4.0) systemu (2.5.2) - test_after_commit (0.2.0) therubyracer (0.10.2) libv8 (~> 3.3.10) thin (1.5.1) @@ -467,7 +466,6 @@ DEPENDENCIES shotgun shoulda state_machine - test_after_commit (= 0.2.0) therubyracer (~> 0.10.2) therubyrhino (~> 1.73.1) trinidad (~> 1.0.2) diff --git a/spec/lib/observers/activity_feed/comment_spec.rb b/spec/lib/observers/activity_feed/comment_spec.rb index 139e9e114..1c027a3e3 100644 --- a/spec/lib/observers/activity_feed/comment_spec.rb +++ b/spec/lib/observers/activity_feed/comment_spec.rb @@ -2,6 +2,7 @@ require 'spec_helper' describe Modules::Observers::ActivityFeed::Comment do + self.use_transactional_fixtures = false before { stub_symlink_methods } it 'sends a notification email after create a issue comment' do @@ -11,4 +12,5 @@ describe Modules::Observers::ActivityFeed::Comment do comment.save end + before(:all) { User.destroy_all } end diff --git a/spec/lib/observers/activity_feed/issue_spec.rb b/spec/lib/observers/activity_feed/issue_spec.rb index 37e9d2730..f67179655 100644 --- a/spec/lib/observers/activity_feed/issue_spec.rb +++ b/spec/lib/observers/activity_feed/issue_spec.rb @@ -2,6 +2,7 @@ require 'spec_helper' describe Modules::Observers::ActivityFeed::Issue do + self.use_transactional_fixtures = false before { stub_symlink_methods } it 'sends a notification email after create' do @@ -27,4 +28,5 @@ describe Modules::Observers::ActivityFeed::Issue do issue.save end + before(:all) { User.destroy_all } end From 391d5af18d4ccf1c9ae0fe9485c68803d55126e2 Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Thu, 27 Jun 2013 23:10:11 +0400 Subject: [PATCH 18/26] #200: fixed specs --- spec/lib/observers/activity_feed/comment_spec.rb | 2 -- spec/lib/observers/activity_feed/issue_spec.rb | 2 -- spec/models/comment_for_commit_spec.rb | 1 + spec/models/comment_spec.rb | 1 + spec/models/issue_spec.rb | 1 + spec/spec_helper.rb | 1 + 6 files changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/lib/observers/activity_feed/comment_spec.rb b/spec/lib/observers/activity_feed/comment_spec.rb index 1c027a3e3..84b5065d5 100644 --- a/spec/lib/observers/activity_feed/comment_spec.rb +++ b/spec/lib/observers/activity_feed/comment_spec.rb @@ -11,6 +11,4 @@ describe Modules::Observers::ActivityFeed::Comment do mock(UserMailer).new_comment_notification(comment, comment.commentable.assignee) { mailer } comment.save end - - before(:all) { User.destroy_all } end diff --git a/spec/lib/observers/activity_feed/issue_spec.rb b/spec/lib/observers/activity_feed/issue_spec.rb index f67179655..be287e73b 100644 --- a/spec/lib/observers/activity_feed/issue_spec.rb +++ b/spec/lib/observers/activity_feed/issue_spec.rb @@ -27,6 +27,4 @@ describe Modules::Observers::ActivityFeed::Issue do mock(UserMailer).issue_assign_notification(issue, user) { mailer } issue.save end - - before(:all) { User.destroy_all } end diff --git a/spec/models/comment_for_commit_spec.rb b/spec/models/comment_for_commit_spec.rb index 9972a699a..1d779ef0d 100644 --- a/spec/models/comment_for_commit_spec.rb +++ b/spec/models/comment_for_commit_spec.rb @@ -37,6 +37,7 @@ def should_not_send_email(args={}) end describe Comment do + self.use_transactional_fixtures = false before { stub_symlink_methods } context 'for global admin user' do before(:each) do diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb index 2226a8c13..51b7eb5a9 100644 --- a/spec/models/comment_spec.rb +++ b/spec/models/comment_spec.rb @@ -19,6 +19,7 @@ def create_comment_in_issue issue, body end describe Comment do + self.use_transactional_fixtures = false before { stub_symlink_methods } context 'for global admin user' do before(:each) do diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 1777fcc92..a8635a625 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -13,6 +13,7 @@ def create_issue issue_owner end describe Issue do + self.use_transactional_fixtures = false before do stub_symlink_methods any_instance_of(Project, :versions => ['v1.0', 'v2.0']) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index c233dd750..f5c167bfa 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -30,6 +30,7 @@ RSpec.configure do |config| config.before(:all) { init_test_root } config.after(:all) { clear_test_root } + config.after(:all) { User.destroy_all } end def set_session_for(user=nil) From 8a83a4cefa773780f059f2b2749b39ef0c527938 Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Thu, 27 Jun 2013 23:43:44 +0400 Subject: [PATCH 19/26] #200: removed duplicates of specs --- .../observers/activity_feed/comment.rb | 3 +- spec/factories/comments.rb | 1 + spec/factories/issues.rb | 1 + spec/factories/users.rb | 1 + .../observers/activity_feed/comment_spec.rb | 14 --------- .../lib/observers/activity_feed/issue_spec.rb | 30 ------------------- spec/models/comment_for_commit_spec.rb | 1 - spec/models/comment_spec.rb | 1 - spec/models/issue_spec.rb | 1 - spec/spec_helper.rb | 1 - 10 files changed, 5 insertions(+), 49 deletions(-) delete mode 100644 spec/lib/observers/activity_feed/comment_spec.rb delete mode 100644 spec/lib/observers/activity_feed/issue_spec.rb diff --git a/lib/modules/observers/activity_feed/comment.rb b/lib/modules/observers/activity_feed/comment.rb index 8a12429a5..0327fcb12 100644 --- a/lib/modules/observers/activity_feed/comment.rb +++ b/lib/modules/observers/activity_feed/comment.rb @@ -3,7 +3,7 @@ module Modules::Observers::ActivityFeed::Comment extend ActiveSupport::Concern included do - after_commit :new_comment_notifications, :on => :create, :unless => :automatic? + after_commit :new_comment_notifications, :on => :create # dont remove outdated issues link after_update -> { Comment.create_link_on_issues_from_item(self) } end @@ -11,6 +11,7 @@ module Modules::Observers::ActivityFeed::Comment private def new_comment_notifications + return if automatic? if issue_comment? commentable.subscribes.each do |subscribe| if user_id != subscribe.user_id diff --git a/spec/factories/comments.rb b/spec/factories/comments.rb index 76119494c..fdfb51a9c 100644 --- a/spec/factories/comments.rb +++ b/spec/factories/comments.rb @@ -5,5 +5,6 @@ FactoryGirl.define do association :user, :factory => :user association :commentable, :factory => :issue project { |c| c.commentable.project } + after(:create) { |c| c.send(:new_comment_notifications) } end end diff --git a/spec/factories/issues.rb b/spec/factories/issues.rb index a76caea56..1bacc7763 100644 --- a/spec/factories/issues.rb +++ b/spec/factories/issues.rb @@ -7,5 +7,6 @@ FactoryGirl.define do association :user, :factory => :user association :assignee, :factory => :user status "open" + after(:create) { |i| i.send(:new_issue_notifications) } end end diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 8f8d138a9..37dab7ef1 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -7,6 +7,7 @@ FactoryGirl.define do password '123456' password_confirmation {|u| u.password} confirmed_at { Time.now.utc } + after(:create) { |u| u.send(:new_user_notification) } end factory :admin, :parent => :user do diff --git a/spec/lib/observers/activity_feed/comment_spec.rb b/spec/lib/observers/activity_feed/comment_spec.rb deleted file mode 100644 index 84b5065d5..000000000 --- a/spec/lib/observers/activity_feed/comment_spec.rb +++ /dev/null @@ -1,14 +0,0 @@ -# -*- encoding : utf-8 -*- -require 'spec_helper' - -describe Modules::Observers::ActivityFeed::Comment do - self.use_transactional_fixtures = false - before { stub_symlink_methods } - - it 'sends a notification email after create a issue comment' do - comment = FactoryGirl.build(:comment) - mailer = mock!.deliver - mock(UserMailer).new_comment_notification(comment, comment.commentable.assignee) { mailer } - comment.save - end -end diff --git a/spec/lib/observers/activity_feed/issue_spec.rb b/spec/lib/observers/activity_feed/issue_spec.rb deleted file mode 100644 index be287e73b..000000000 --- a/spec/lib/observers/activity_feed/issue_spec.rb +++ /dev/null @@ -1,30 +0,0 @@ -# -*- encoding : utf-8 -*- -require 'spec_helper' - -describe Modules::Observers::ActivityFeed::Issue do - self.use_transactional_fixtures = false - before { stub_symlink_methods } - - it 'sends a notification email after create' do - issue = FactoryGirl.build(:issue, :assignee => nil) - mailer = mock!.deliver - mock(UserMailer).new_issue_notification(issue, issue.project.owner) { mailer } - issue.save - end - - it 'does not send a notification email after update' do - issue = FactoryGirl.create(:issue, :assignee => nil) - issue.title = 'new title' - dont_allow(UserMailer).new_issue_notification - issue.save - end - - it 'sends a notification email after a assignee of issue has been changed' do - user = FactoryGirl.create(:user) - issue = FactoryGirl.build(:issue, :assignee => nil) - issue.assignee = user - mailer = mock!.deliver - mock(UserMailer).issue_assign_notification(issue, user) { mailer } - issue.save - end -end diff --git a/spec/models/comment_for_commit_spec.rb b/spec/models/comment_for_commit_spec.rb index 1d779ef0d..9972a699a 100644 --- a/spec/models/comment_for_commit_spec.rb +++ b/spec/models/comment_for_commit_spec.rb @@ -37,7 +37,6 @@ def should_not_send_email(args={}) end describe Comment do - self.use_transactional_fixtures = false before { stub_symlink_methods } context 'for global admin user' do before(:each) do diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb index 51b7eb5a9..2226a8c13 100644 --- a/spec/models/comment_spec.rb +++ b/spec/models/comment_spec.rb @@ -19,7 +19,6 @@ def create_comment_in_issue issue, body end describe Comment do - self.use_transactional_fixtures = false before { stub_symlink_methods } context 'for global admin user' do before(:each) do diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index a8635a625..1777fcc92 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -13,7 +13,6 @@ def create_issue issue_owner end describe Issue do - self.use_transactional_fixtures = false before do stub_symlink_methods any_instance_of(Project, :versions => ['v1.0', 'v2.0']) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index f5c167bfa..c233dd750 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -30,7 +30,6 @@ RSpec.configure do |config| config.before(:all) { init_test_root } config.after(:all) { clear_test_root } - config.after(:all) { User.destroy_all } end def set_session_for(user=nil) From b7b2a0b2ff6800e8efd9d9b466a9fc61a1c30ea9 Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Fri, 28 Jun 2013 16:32:40 +0400 Subject: [PATCH 20/26] #202: added highlighting --- app/assets/javascripts/extra/log-wrapper.js | 7 ++---- app/assets/stylesheets/design/custom.scss | 24 +++++++-------------- app/views/shared/_log.html.haml | 16 ++++++-------- 3 files changed, 16 insertions(+), 31 deletions(-) diff --git a/app/assets/javascripts/extra/log-wrapper.js b/app/assets/javascripts/extra/log-wrapper.js index 0204e9275..d449d233c 100644 --- a/app/assets/javascripts/extra/log-wrapper.js +++ b/app/assets/javascripts/extra/log-wrapper.js @@ -50,7 +50,7 @@ function initLogWrapper() { var hScroll = l.scrollLeft; var onBottom = Math.abs((l.clientHeight + vScroll - l.scrollHeight)) < getLineHeight(l); - $logCont.text(data.log); + CodeMirror.runMode(data.log.replace(/&/gi, '&'), "text/x-sh", document.getElementById("output")); $logCont.scrollLeft(hScroll); $logCont.scrollTop((onBottom || first_open) ? l.scrollHeight - l.clientHeight : vScroll); @@ -95,7 +95,7 @@ function initLogWrapper() { $autoload.on('change', reloadChange); $('#word_wrap').on('change', function() { - $logCont.attr({'wrap': ($(this).is(':checked')) ? 'soft' : 'off'}); + $logCont.css('white-space', ($(this).is(':checked')) ? 'normal' : 'pre'); }); $('#reload_interval').on('change', function() { @@ -104,8 +104,5 @@ function initLogWrapper() { t = setInterval($(this).val()); } }); - $('#load_lines').on('change', function() { - $logCont.data('load_lines', $(this).val()); - }).trigger('change'); loadLog(); } \ No newline at end of file diff --git a/app/assets/stylesheets/design/custom.scss b/app/assets/stylesheets/design/custom.scss index afa3c83d4..b25e15380 100644 --- a/app/assets/stylesheets/design/custom.scss +++ b/app/assets/stylesheets/design/custom.scss @@ -1457,18 +1457,18 @@ div.reloader { float: right; } -div.log-wrapper { +.log-wrapper { width: 565px; - div.log-header { + .log-header { margin-bottom: 10px; - div.text-wrap, span { + .text-wrap, span { height: 16px; margin: 0; padding: 0; } - div.text-wrap { + .text-wrap { width: 545px; float: left; @@ -1492,13 +1492,13 @@ div.log-wrapper { } } - div.log-body { + .log-body { - div.reloader, .log { + .reloader, .log { border: 1px solid #ccc; } - div.reloader { + .reloader { padding: 5px 10px; border-bottom: none; border-radius: 5px 5px 0 0; @@ -1523,15 +1523,6 @@ div.log-wrapper { border-left: 1px solid #ccc; } } - - tr.bottom { - td { - padding-left: 5px; - } - td.first { - border-top: 1px solid #ccc; - } - } } } @@ -1539,6 +1530,7 @@ div.log-wrapper { background: #f8f8f8; height: 500px; width: 543px; + white-space: pre; min-width: 543px; border-radius: 5px 0 5px 5px; overflow: auto; diff --git a/app/views/shared/_log.html.haml b/app/views/shared/_log.html.haml index 6ff0518d7..30bc027ec 100644 --- a/app/views/shared/_log.html.haml +++ b/app/views/shared/_log.html.haml @@ -28,19 +28,15 @@ = check_box_tag :autoreload, true, build_started = t("layout.build_lists.log.autoreload") = select_tag :reload_interval, log_reload_time_options - %tr.bottom - %td.first - - if download_log_url - = link_to t("layout.build_lists.log.download"), download_log_url, :id => :log_url - %td.last{ :class => build_started ? nil : :hidden } - = label_tag :load_lines do - = raw t("layout.build_lists.log.load_lines", :count => select_tag(:load_lines, log_reload_lines_options)) + + .both + #output.cm-s-default.log{ :readonly => :readonly, + :data => { :url => get_log_path, :log_type => :build } } + %pre#code .both - %textarea.log{ :readonly => :readonly, :wrap => 'off', - :data => { :url => get_log_path, :log_type => :build } } - = t("layout.build_lists.log.not_available") .both + :javascript $(function() { initLogWrapper(); From dad840f973599339d444c2edca938dc2dae35a51 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Fri, 28 Jun 2013 19:10:22 +0600 Subject: [PATCH 21/26] [#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 22/26] 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 23/26] [#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 24/26] [#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 25/26] 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 26/26] [#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