From d23e62e8cc2efe7eb8fbcc89ae2f538b6f78eaa7 Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Wed, 1 Apr 2015 03:47:32 +0300 Subject: [PATCH] #465: Updated specs for Api::V1::PullRequestsController --- .../api/v1/pull_requests_controller.rb | 22 ++- app/models/concerns/feed/issue.rb | 8 +- app/policies/pull_request_policy.rb | 4 + .../api/v1/pull_requests_controller.rb | 146 +++++++++--------- 4 files changed, 93 insertions(+), 87 deletions(-) diff --git a/app/controllers/api/v1/pull_requests_controller.rb b/app/controllers/api/v1/pull_requests_controller.rb index 9028c456a..7dc2a5faf 100644 --- a/app/controllers/api/v1/pull_requests_controller.rb +++ b/app/controllers/api/v1/pull_requests_controller.rb @@ -4,10 +4,10 @@ class Api::V1::PullRequestsController < Api::V1::BaseController before_action :authenticate_user! skip_before_action :authenticate_user!, only: %i(show index group_index commits files) if APP_CONFIG['anonymous_access'] - before_action :load_group, only: :group_index - before_action :load_project - before_action :load_issue, only: %i(show index commits files merge update) - before_action :load_pull, only: %i(show index commits files merge update) + before_action :load_group, only: %i(group_index) + before_action :load_project, except: %i(all_index user_index) + before_action :load_issue, only: %i(show index commits files merge update) + before_action :load_pull, only: %i(show index commits files merge update) def index @pulls = @project.pull_requests @@ -40,8 +40,7 @@ class Api::V1::PullRequestsController < Api::V1::BaseController end def show - redirect_to api_v1_project_issue_path(@project.id, @issue.serial_id) if @pull.nil? - respond_to :json + redirect_to api_v1_project_issue_path(@project.id, @issue.serial_id) and return if @pull.nil? end def create @@ -49,7 +48,7 @@ class Api::V1::PullRequestsController < Api::V1::BaseController from_project ||= @project authorize from_project, :show? - @pull = @project.pull_requests.new + @pull = @project.pull_requests.build @pull.build_issue title: pull_params[:title], body: pull_params[:body] @pull.from_project = from_project @pull.to_ref, @pull.from_ref = pull_params[:to_ref], pull_params[:from_ref] @@ -101,13 +100,11 @@ class Api::V1::PullRequestsController < Api::V1::BaseController def commits authorize @pull @commits = @pull.repo.commits_between(@pull.to_commit, @pull.from_commit).paginate(paginate_params) - respond_to :json end def files authorize @pull @stats = @pull.diff_stats.zip(@pull.diff).paginate(paginate_params) - respond_to :json end def merge @@ -124,7 +121,8 @@ class Api::V1::PullRequestsController < Api::V1::BaseController # Private: before_action hook which loads PullRequest. def load_pull - authorize @pull = @issue.pull_request, :show? + @pull = @issue.pull_request + authorize @pull, :show? if @pull end def render_pulls_list @@ -165,9 +163,7 @@ class Api::V1::PullRequestsController < Api::V1::BaseController @pulls = @pulls.where('issues.created_at >= to_timestamp(?)', params[:since]) if params[:since] =~ /\A\d+\z/ @pulls = @pulls.paginate(paginate_params) - respond_to do |format| - format.json { render :index } - end + render :index end def pull_params diff --git a/app/models/concerns/feed/issue.rb b/app/models/concerns/feed/issue.rb index f57f0fce0..5801ba22f 100644 --- a/app/models/concerns/feed/issue.rb +++ b/app/models/concerns/feed/issue.rb @@ -5,7 +5,7 @@ module Feed::Issue after_commit :new_issue_notifications, on: :create after_commit :send_assign_notifications, on: :create, if: ->(i) { i.assignee } - after_update -> { send_assign_notifications(:update) } + after_update :send_assign_notifications after_commit :send_hooks, on: :create after_update -> { send_hooks(:update) }, if: ->(i) { i.previous_changes['status'].present? } @@ -37,8 +37,10 @@ module Feed::Issue ::Comment.create_link_on_issues_from_item(self) end - def send_assign_notifications(action = :create) - if(action == :create && assignee_id) || previous_changes['assignee_id'].present? + def send_assign_notifications + return if @skip_assign_notifications + @skip_assign_notifications = true + if assignee_id && assignee_id_changed? if assignee.notifier.issue_assign && assignee.notifier.can_notify UserMailer.issue_assign_notification(self, assignee).deliver end diff --git a/app/policies/pull_request_policy.rb b/app/policies/pull_request_policy.rb index 74a075307..c788ac260 100644 --- a/app/policies/pull_request_policy.rb +++ b/app/policies/pull_request_policy.rb @@ -1,5 +1,9 @@ class PullRequestPolicy < ApplicationPolicy + def index? + true + end + def show? ProjectPolicy.new(user, record.to_project).show? end diff --git a/spec/controllers/api/v1/pull_requests_controller.rb b/spec/controllers/api/v1/pull_requests_controller.rb index b7dd40fb6..73e8b74de 100644 --- a/spec/controllers/api/v1/pull_requests_controller.rb +++ b/spec/controllers/api/v1/pull_requests_controller.rb @@ -1,15 +1,15 @@ 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 = project.pull_requests.build 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.check - pull + pull.reload end describe Api::V1::PullRequestsController, type: :controller do - before(:all) do + before do stub_symlink_methods @project = FactoryGirl.create(:project_with_commit) @@ -43,117 +43,117 @@ describe Api::V1::PullRequestsController, type: :controller do context 'read and accessible abilities' do context 'for user' do - before(:each) do + before 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 + expect(response).to 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') + expect(response).to render_template('api/v1/pull_requests/show') end it 'can show pull request in open project' do get :show, project_id: @another_project.id, id: @another_pull.serial_id, format: :json - response.should be_success + expect(response).to be_success end it 'can show pull request in own hidden project' do get :show, project_id: @own_hidden_project.id, id: @own_hidden_pull.serial_id, format: :json - response.should be_success + expect(response).to be_success end it 'cant show pull request in hidden project' do get :show, project_id: @hidden_project.id, id: @hidden_pull.serial_id, format: :json - response.status.should == 403 + expect(response.status).to eq 403 end it 'should return three pull requests' do 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) + expect(assigns[:pulls]).to include(@pull) + expect(assigns[:pulls]).to include(@own_hidden_pull) + expect(assigns[:pulls]).to 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') + expect(response).to render_template('api/v1/pull_requests/index') end it 'should return only assigned pull request' do get :user_index, format: :json - assigns[:pulls].should include(@own_hidden_pull) + expect(assigns[:pulls]).to include(@own_hidden_pull) expect(assigns[:pulls].count).to eq 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') + expect(response).to 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 + expect(response).to 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}") + expect(response).to 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 + expect(response).to_not be_success end end it 'should return 404' do get :show, project_id: @project.id, id: 999999, format: :json - response.status.should == 404 + expect(response.status).to eq 404 end it 'should redirect to issue page' do get :show, project_id: @project.id, id: @issue.serial_id, format: :json - response.should redirect_to(api_v1_project_issue_path(@project.id, @issue.serial_id)) + expect(response).to redirect_to(api_v1_project_issue_path(@project.id, @issue.serial_id)) 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 + expect(response).to 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 + expect(response.status).to eq 403 end it 'should not return any pull requests' do get :all_index, filter: 'all', format: :json - response.status.should == 401 + expect(response.status).to eq 401 end %w(commits files).each do |action| it "can show pull request #{action} in project", anonymous_access: true do get action, project_id: @project.id, id: @pull.serial_id, format: :json - response.should be_success + expect(response).to be_success end it "should render right template for commits action", anonymous_access: true do get action, project_id: @project.id, id: @pull.serial_id, format: :json - response.should render_template("api/v1/pull_requests/#{action}") + expect(response).to render_template("api/v1/pull_requests/#{action}") end it "can't show pull request #{action} in hidden project", anonymous_access: true do get action, project_id: @hidden_project.id, id: @hidden_pull.serial_id, format: :json - response.should_not be_success + expect(response).to_not be_success end end end @@ -166,33 +166,41 @@ describe Api::V1::PullRequestsController, type: :controller do end it 'can create pull request in own project' do - lambda { post :create, @create_params }.should change{ PullRequest.count }.by(1) + expect do + post :create, @create_params + end.to change(PullRequest, :count).by(1) end it 'can create pull request in own hidden project' do - lambda { post :create, @create_params.merge(project_id: @own_hidden_project.id) }.should - change{ PullRequest.count }.by(1) + expect do + post :create, @create_params.merge(project_id: @own_hidden_project.id) + end.to 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) + expect do + post :create, @create_params.merge(project_id: @another_project.id) + end.to change(PullRequest, :count).by(1) end it 'cant create pull request in hidden project' do - lambda { post :create, @create_params.merge(project_id: @hidden_project.id) }.should - change{ PullRequest.count }.by(0) + expect do + post :create, @create_params.merge(project_id: @hidden_project.id) + end.to_not change(PullRequest, :count) 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) + expect do + post :create, @create_params + end.to_not change(PullRequest, :count) end it 'cant create pull request in hidden project', anonymous_access: true do - lambda { post :create, @create_params.merge(project_id: @hidden_project.id) }.should - change{ PullRequest.count }.by(0) + expect do + post :create, @create_params.merge(project_id: @hidden_project.id) + end.to_not change(PullRequest, :count) end end end @@ -205,76 +213,76 @@ describe Api::V1::PullRequestsController, type: :controller do it 'can update pull request in own project' do put :update, @update_params - @pull.reload.title.should == 'new title' + expect(@pull.reload.title).to eq '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' + expect(@own_hidden_pull.reload.title).to eq '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' + expect(@another_pull.reload.title).to_not eq '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' + expect(@hidden_pull.reload.title).to_not eq '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 + expect(@pull.reload.status).to eq 'merged' + expect(response).to 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 + expect(@own_hidden_pull.reload.status).to eq 'merged' + expect(response).to 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 + expect(@another_pull.reload.status).to eq 'ready' + expect(response.status).to eq 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 + expect(@hidden_pull.reload.status).to eq 'ready' + expect(response.status).to eq 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 + expect(response.status).to eq 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 + expect(response.status).to eq 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 + expect(@another_pull.reload.status).to eq 'ready' + expect(response.status).to eq 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 + expect(@hidden_pull.reload.status).to eq 'ready' + expect(response.status).to eq 401 end end end context 'send email messages' do - before(:each) do + before do @project_reader = FactoryGirl.create :user create_relation(@project, @project_reader, 'reader') @project_admin = FactoryGirl.create :user @@ -288,36 +296,32 @@ describe Api::V1::PullRequestsController, type: :controller do it 'should send two email messages to project admins' do post :create, @create_params - @project.pull_requests.last.issue.send(:new_issue_notifications) - @project.pull_requests.last.issue.send(:send_assign_notifications) - ActionMailer::Base.deliveries.count.should == 2 + # @project.pull_requests.last.issue.send(:new_issue_notifications) + # @project.pull_requests.last.issue.send(:send_assign_notifications) + expect(ActionMailer::Base.deliveries.count).to eq 2 end it 'should send two email messages to admins and one to assignee' do post :create, @create_params.deep_merge(pull_request: {assignee_id: @project_reader.id}) - @project.pull_requests.last.issue.send(:new_issue_notifications) - @project.pull_requests.last.issue.send(:send_assign_notifications) - ActionMailer::Base.deliveries.count.should == 3 + # @project.pull_requests.last.issue.send(:new_issue_notifications) + # @project.pull_requests.last.issue.send(:send_assign_notifications) + expect(ActionMailer::Base.deliveries.count).to eq 3 end it 'should send email message to new assignee' do http_login(@project_admin) put :update, @update_params.deep_merge(pull_request: {assignee_id: @project_reader.id}) - @project.pull_requests.last.issue.send(:send_assign_notifications) - ActionMailer::Base.deliveries.count.should == 1 + # @project.pull_requests.last.issue.send(:send_assign_notifications) + expect(ActionMailer::Base.deliveries.count).to eq 1 end it 'should not duplicate email message' do post :create, @create_params.deep_merge(pull_request: {assignee_id: @project_admin.id}) - @project.pull_requests.last.issue.send(:new_issue_notifications) - @project.pull_requests.last.issue.send(:send_assign_notifications) - ActionMailer::Base.deliveries.count.should == 2 # send only to admins - ActionMailer::Base.deliveries.first.to != ActionMailer::Base.deliveries.last.to + # @project.pull_requests.last.issue.send(:new_issue_notifications) + # @project.pull_requests.last.issue.send(:send_assign_notifications) + expect(ActionMailer::Base.deliveries.count).to eq 2 # send only to admins + expect(ActionMailer::Base.deliveries.first.to).to_not eq ActionMailer::Base.deliveries.last.to end end - after(:all) do - User.destroy_all - Platform.destroy_all - end end