From 328eef6ea28ec5df7e441ca2a5cc2f3d1e7e2f51 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Tue, 14 Apr 2015 16:06:44 +0500 Subject: [PATCH] #465 fix subscribes; add/fix subscribe specs --- .../projects/commit_subscribes_controller.rb | 2 - app/models/subscribe.rb | 1 + app/policies/subscribe_policy.rb | 2 + .../subscribes/_subscribe_status.html.slim | 2 +- config/routes.rb | 3 +- .../commit_subscribes_controller_spec.rb | 117 ++++++++++++++++++ .../projects/subscribes_controller_spec.rb | 18 +-- spec/models/subscribe_spec.rb | 97 +++------------ 8 files changed, 146 insertions(+), 96 deletions(-) create mode 100644 spec/controllers/projects/commit_subscribes_controller_spec.rb diff --git a/app/controllers/projects/commit_subscribes_controller.rb b/app/controllers/projects/commit_subscribes_controller.rb index f17ba3027..440e52a47 100644 --- a/app/controllers/projects/commit_subscribes_controller.rb +++ b/app/controllers/projects/commit_subscribes_controller.rb @@ -3,7 +3,6 @@ class Projects::CommitSubscribesController < Projects::BaseController before_action :find_commit def create - authorize @commit.subscribes.build(user_id: current_user.id) if Subscribe.subscribe_to_commit(@options) flash[:notice] = I18n.t("flash.subscribe.commit.saved") # TODO js @@ -15,7 +14,6 @@ class Projects::CommitSubscribesController < Projects::BaseController end def destroy - authorize @commit.subscribes.build(user_id: current_user.id) Subscribe.unsubscribe_from_commit(@options) flash[:notice] = t("flash.subscribe.commit.destroyed") redirect_to commit_path(@project, @commit) diff --git a/app/models/subscribe.rb b/app/models/subscribe.rb index 3c08e8393..5b00693de 100644 --- a/app/models/subscribe.rb +++ b/app/models/subscribe.rb @@ -4,6 +4,7 @@ class Subscribe < ActiveRecord::Base belongs_to :project attr_accessible :status, :user_id + validates :user, presence: true def commit_subscribe? subscribeable_type == 'Grit::Commit' diff --git a/app/policies/subscribe_policy.rb b/app/policies/subscribe_policy.rb index 455946ac0..67d3debf3 100644 --- a/app/policies/subscribe_policy.rb +++ b/app/policies/subscribe_policy.rb @@ -2,11 +2,13 @@ class SubscribePolicy < ApplicationPolicy def create? return false if user.guest? + return true if record.subscribeable.is_a?(Grit::Commit) !record.subscribeable.subscribes.exists?(user_id: user.id) end def destroy? return false if user.guest? + return true if record.subscribeable.is_a?(Grit::Commit) record.subscribeable.subscribes.exists?(user_id: user.id) end end diff --git a/app/views/projects/subscribes/_subscribe_status.html.slim b/app/views/projects/subscribes/_subscribe_status.html.slim index 76fbc8f84..169bbad6f 100644 --- a/app/views/projects/subscribes/_subscribe_status.html.slim +++ b/app/views/projects/subscribes/_subscribe_status.html.slim @@ -1,6 +1,6 @@ - if Comment.issue_comment?(commentable.class) - is_subscribed = commentable.subscribes.exists?(user_id: current_user.id) - - subscribe_path = is_subscribed ? project_issue_subscribe_path(project, commentable, current_user.id) : project_issue_subscribes_path(project, commentable) + - subscribe_path = is_subscribed ? project_issue_unsubscribe_path(project, commentable) : project_issue_subscribe_path(project, commentable) - else Comment.commit_comment?(commentable.class) - is_subscribed = Subscribe.subscribed_to_commit?(project, current_user, commentable) - subscribe_path = is_subscribed ? unsubscribe_commit_path(project, commentable) : subscribe_commit_path(project, commentable) diff --git a/config/routes.rb b/config/routes.rb index cf5d28b50..efc30ae4d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -324,7 +324,8 @@ Rosa::Application.routes.draw do end resources :issues, except: [:destroy, :edit] do resources :comments, only: [:edit, :create, :update, :destroy] - resources :subscribes, only: [:create, :destroy] + post '/subscribe' => "subscribes#create", as: :subscribe + delete '/unsubscribe' => "subscribes#destroy", as: :unsubscribe collection do post :create_label get :search_collaborators diff --git a/spec/controllers/projects/commit_subscribes_controller_spec.rb b/spec/controllers/projects/commit_subscribes_controller_spec.rb new file mode 100644 index 000000000..1abcfd5f4 --- /dev/null +++ b/spec/controllers/projects/commit_subscribes_controller_spec.rb @@ -0,0 +1,117 @@ +require 'spec_helper' + +def subscribe_to_commit + Subscribe.subscribe_to_commit(project_id: @project.id, + subscribeable_id: @commit.id.hex, + subscribeable_type: @commit.class.name, + user_id: @user.id) +end + +shared_examples_for 'can subscribe' do + it 'should be able to perform create action' do + post :create, @create_params + expect(response).to redirect_to(commit_path(@project, @commit)) + end + + it 'should create subscribe object into db' do + expect { post :create, @create_params }.to change(Subscribe, :count).by(1) + end +end + +shared_examples_for 'can not subscribe' do + it 'should not be able to perform create action' do + post :create, @create_params + expect(response).to redirect_to(commit_path(@project, @commit)) + end + + it 'should not create subscribe object into db' do + expect { post :create, @create_params }.to_not change(Subscribe, :count) + end +end + +shared_examples_for 'can unsubscribe' do + it 'should be able to perform destroy action' do + delete :destroy, @destroy_params + expect(response).to redirect_to(commit_path(@project, @commit)) + end + + it 'should reduce subscribes count' do + delete :destroy, @destroy_params + expect(Subscribe.subscribed_to_commit?(@project, @user, @commit)).to be_falsy + end +end + +shared_examples_for 'can not unsubscribe' do + it 'should not be able to perform destroy action' do + delete :destroy, @destroy_params + + expect(response).to redirect_to(commit_path(@project, @commit)) + end + + it 'should not reduce subscribes count' do + expect { delete :destroy, @destroy_params }.to_not change(Subscribe, :count) + end +end + +describe Projects::CommitSubscribesController, type: :controller do + before(:each) do + stub_symlink_methods + + @project = FactoryGirl.create(:project_with_commit) + @commit = @project.repo.commits.first + + @create_params = { commit_id: @commit.sha, name_with_owner: @project.name_with_owner } + @destroy_params = { commit_id: @commit.sha, name_with_owner: @project.name_with_owner } + + allow_any_instance_of(Project).to receive(:versions).and_return(%w(v1.0 v2.0)) + end + + context 'for global admin user' do + before(:each) do + @user = FactoryGirl.create(:admin) + set_session_for(@user) + end + + context 'subscribed' do + before(:each) { subscribe_to_commit } + it_should_behave_like 'can unsubscribe' + it_should_behave_like 'can not subscribe' + end + + context 'not subscribed' do + it_should_behave_like 'can subscribe' + end + end + + context 'for simple user' do + before(:each) do + @user = FactoryGirl.create(:user) + set_session_for(@user) + end + + context 'subscribed' do + before(:each) { subscribe_to_commit } + + it_should_behave_like 'can unsubscribe' + it_should_behave_like 'can not subscribe' + end + + context 'not subscribed' do + it_should_behave_like 'can subscribe' + end + end + + context 'for guest' do + before(:each) { set_session_for(User.new) } + + it 'should not be able to perform create action' do + post :create, @create_params + expect(response).to redirect_to(new_user_session_path) + end + + it 'should not be able to perform destroy action' do + delete :destroy, @destroy_params + expect(response).to redirect_to(new_user_session_path) + end + end +end diff --git a/spec/controllers/projects/subscribes_controller_spec.rb b/spec/controllers/projects/subscribes_controller_spec.rb index c4b626468..83f71404d 100644 --- a/spec/controllers/projects/subscribes_controller_spec.rb +++ b/spec/controllers/projects/subscribes_controller_spec.rb @@ -7,9 +7,7 @@ shared_examples_for 'can subscribe' do end it 'should create subscribe object into db' do - expect do - post :create, @create_params - end.to change(Subscribe, :count).by(1) + expect { post :create, @create_params }.to change(Subscribe, :count).by(1) end end @@ -20,9 +18,7 @@ shared_examples_for 'can not subscribe' do end it 'should not create subscribe object into db' do - expect do - post :create, @create_params - end.to_not change(Subscribe, :count) + expect { post :create, @create_params }.to_not change(Subscribe, :count) end end @@ -34,9 +30,7 @@ shared_examples_for 'can unsubscribe' do end it 'should reduce subscribes count' do - expect do - delete :destroy, @destroy_params - end.to change(Subscribe, :count).by(-1) + expect { delete :destroy, @destroy_params }.to change(Subscribe, :count).by(-1) end end @@ -48,9 +42,7 @@ shared_examples_for 'can not unsubscribe' do end it 'should not reduce subscribes count' do - expect do - delete :destroy, @destroy_params - end.to_not change(Subscribe, :count) + expect { delete :destroy, @destroy_params }.to_not change(Subscribe, :count) end end @@ -74,7 +66,6 @@ describe Projects::SubscribesController, type: :controller do @user = FactoryGirl.create(:admin) set_session_for(@user) create_relation(@project, @user, 'admin') - @destroy_params = @destroy_params.merge({id: @user.id}) end context 'subscribed' do @@ -115,5 +106,4 @@ describe Projects::SubscribesController, type: :controller do it_should_behave_like 'can subscribe' end end - end diff --git a/spec/models/subscribe_spec.rb b/spec/models/subscribe_spec.rb index 15b164db6..cacf7b9ba 100644 --- a/spec/models/subscribe_spec.rb +++ b/spec/models/subscribe_spec.rb @@ -1,78 +1,19 @@ -# require 'spec_helper' -# require "cancan/matchers" -# -# def set_testable_data -# @ability = Ability.new(@user) -# -# @project = FactoryGirl.create(:project) -# @issue = FactoryGirl.create(:issue, project_id: @project.id) -# -# allow_any_instance_of(Project).to receive(:versions).and_return(%w(v1.0 v2.0)) -# end -# -# describe Subscribe do -# before { stub_symlink_methods } -# context 'for global admin user' do -# before(:each) do -# @user = FactoryGirl.create(:admin) -# @stranger = FactoryGirl.create(:user) -# -# set_testable_data -# end -# -# it 'should create subscribe' do -# @ability.should be_able_to(:create, FactoryGirl.build(:subscribe, subscribeable: @issue, user: @user)) -# end -# -# context 'destroy' do -# before(:each) do -# @subscribe = FactoryGirl.create(:subscribe, subscribeable: @issue, user: @user) -# @stranger_subscribe = FactoryGirl.create(:subscribe, subscribeable: @issue, user: @stranger) -# end -# -# context 'own subscribe' do -# it 'should destroy subscribe' do -# @ability.should be_able_to(:destroy, @subscribe) -# end -# end -# -# context 'stranger subscribe' do -# it 'should not destroy subscribe' do -# @ability.should_not be_able_to(:destroy, @stranger_subscribe) -# end -# end -# end -# end -# -# context 'for simple user' do -# before(:each) do -# @user = FactoryGirl.create(:user) -# @stranger = FactoryGirl.create(:user) -# -# set_testable_data -# end -# -# it 'should create subscribe' do -# @ability.should be_able_to(:create, FactoryGirl.build(:subscribe, subscribeable: @issue, user: @user)) -# end -# -# context 'destroy' do -# before(:each) do -# @subscribe = FactoryGirl.create(:subscribe, subscribeable: @issue, user: @user) -# @stranger_subscribe = FactoryGirl.create(:subscribe, subscribeable: @issue, user: @stranger) -# end -# -# context 'own subscribe' do -# it 'should destroy subscribe' do -# @ability.should be_able_to(:destroy, @subscribe) -# end -# end -# -# context 'stranger subscribe' do -# it 'should not destroy subscribe' do -# @ability.should_not be_able_to(:destroy, @stranger_subscribe) -# end -# end -# end -# end -# end +require 'spec_helper' + +describe Subscribe do + before(:each) { stub_symlink_methods } + + context 'validates that subscribe contains user' do + + it 'when subscribe contains user' do + s = FactoryGirl.build(:subscribe) + expect(s.valid?).to be_truthy + end + + it 'when subscribe does not contains user' do + s = FactoryGirl.build(:subscribe) + s.user = nil + expect(s.valid?).to be_falsy + end + end +end