#465 fix subscribes; add/fix subscribe specs

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)

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'

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

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)

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

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

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
+    expect { post :create, @create_params }.to change(Subscribe, :count).by(1)
-      post :create, @create_params
-    end.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
+    expect { post :create, @create_params }.to_not change(Subscribe, :count)
-      post :create, @create_params
-    end.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
+    expect { delete :destroy, @destroy_params }.to change(Subscribe, :count).by(-1)
-      delete :destroy, @destroy_params
-    end.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
+    expect { delete :destroy, @destroy_params }.to_not change(Subscribe, :count)
-      delete :destroy, @destroy_params
-    end.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

spec/models/subscribe_spec.rb

@@ -1,78 +1,19 @@
-# require 'spec_helper'
+require 'spec_helper'
-# require "cancan/matchers"
+
-#
+describe Subscribe do
-# def set_testable_data
+  before(:each) { stub_symlink_methods }
-#   @ability = Ability.new(@user)
+
-#
+  context 'validates that subscribe contains user' do
-#   @project = FactoryGirl.create(:project)
+
-#   @issue = FactoryGirl.create(:issue, project_id: @project.id)
+    it 'when subscribe contains user' do
-#
+      s = FactoryGirl.build(:subscribe)
-#   allow_any_instance_of(Project).to receive(:versions).and_return(%w(v1.0 v2.0))
+      expect(s.valid?).to be_truthy
-# end
+    end
-#
+
-# describe Subscribe do
+    it 'when subscribe does not contains user' do
-#   before { stub_symlink_methods }
+      s = FactoryGirl.build(:subscribe)
-#   context 'for global admin user' do
+      s.user = nil
-#     before(:each) do
+      expect(s.valid?).to be_falsy
-#       @user = FactoryGirl.create(:admin)
+    end
-#       @stranger = FactoryGirl.create(:user)
+  end
-#
+end
-#       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