#465: Updated specs for Api::V1::PullRequestsController

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_group,   only:   %i(group_index)
-  before_action :load_project
+  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_issue,   only:   %i(show index commits files merge update)
-  before_action :load_pull,  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?
+    redirect_to api_v1_project_issue_path(@project.id, @issue.serial_id) and return if @pull.nil?
-    respond_to :json
   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|
+    render :index
-      format.json { render :index }
-    end
   end
 
   def pull_params

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)
+  def send_assign_notifications
-    if(action == :create && assignee_id) || previous_changes['assignee_id'].present?
+    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

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

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)
+        expect(assigns[:pulls]).to include(@pull)
-        assigns[:pulls].should include(@own_hidden_pull)
+        expect(assigns[:pulls]).to include(@own_hidden_pull)
-        assigns[:pulls].should include(@membered_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
+        expect do
-          change{ PullRequest.count }.by(1)
+          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
+        expect do
-          change{ PullRequest.count }.by(1)
+          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
+        expect do
-          change{ PullRequest.count }.by(0)
+          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
+        expect do
-          change{ PullRequest.count }.by(0)
+          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'
+        expect(@pull.reload.status).to eq 'merged'
-        response.should be_success
+        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'
+        expect(@own_hidden_pull.reload.status).to eq 'merged'
-        response.should be_success
+        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'
+        expect(@another_pull.reload.status).to eq 'ready'
-        response.status.should == 403
+        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'
+        expect(@hidden_pull.reload.status).to eq 'ready'
-        response.status.should == 403
+        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'
+        expect(@another_pull.reload.status).to eq 'ready'
-        response.status.should == 401
+        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'
+        expect(@hidden_pull.reload.status).to eq 'ready'
-        response.status.should == 401
+        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(:new_issue_notifications)
-      @project.pull_requests.last.issue.send(:send_assign_notifications)
+      # @project.pull_requests.last.issue.send(:send_assign_notifications)
-      ActionMailer::Base.deliveries.count.should == 2
+      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(:new_issue_notifications)
-      @project.pull_requests.last.issue.send(:send_assign_notifications)
+      # @project.pull_requests.last.issue.send(:send_assign_notifications)
-      ActionMailer::Base.deliveries.count.should == 3
+      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)
+      # @project.pull_requests.last.issue.send(:send_assign_notifications)
-      ActionMailer::Base.deliveries.count.should == 1
+      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(:new_issue_notifications)
-      @project.pull_requests.last.issue.send(:send_assign_notifications)
+      # @project.pull_requests.last.issue.send(:send_assign_notifications)
-      ActionMailer::Base.deliveries.count.should == 2 # send only to admins
+      expect(ActionMailer::Base.deliveries.count).to eq 2 # send only to admins
-      ActionMailer::Base.deliveries.first.to != ActionMailer::Base.deliveries.last.to
+      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