From 27ece220f6e368727029ec3a4233964bab2e296b Mon Sep 17 00:00:00 2001 From: kikito Date: Mon, 31 Aug 2015 19:02:50 +0200 Subject: [PATCH] Transform Flag.flag! & unflag! into non-raising methods They now return false and do nothing instead --- app/controllers/comments_controller.rb | 4 ++-- app/controllers/debates_controller.rb | 4 ++-- app/models/flag.rb | 22 ++++------------------ spec/factories.rb | 4 ++-- spec/features/comments_spec.rb | 2 +- spec/features/debates_spec.rb | 8 ++++---- spec/models/ability_spec.rb | 4 ++-- spec/models/flag_spec.rb | 26 +++++++++++++------------- 8 files changed, 30 insertions(+), 44 deletions(-) diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index c342b8d10..e792240f8 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -23,12 +23,12 @@ class CommentsController < ApplicationController end def flag - Flag.flag!(current_user, @comment) + Flag.flag(current_user, @comment) respond_with @comment, template: 'comments/_refresh_flag_actions' end def unflag - Flag.unflag!(current_user, @comment) + Flag.unflag(current_user, @comment) respond_with @comment, template: 'comments/_refresh_flag_actions' end diff --git a/app/controllers/debates_controller.rb b/app/controllers/debates_controller.rb index 9e5247134..2f660c1c7 100644 --- a/app/controllers/debates_controller.rb +++ b/app/controllers/debates_controller.rb @@ -55,12 +55,12 @@ class DebatesController < ApplicationController end def flag - Flag.flag!(current_user, @debate) + Flag.flag(current_user, @debate) respond_with @debate, template: 'debates/_refresh_flag_actions' end def unflag - Flag.unflag!(current_user, @debate) + Flag.unflag(current_user, @debate) respond_with @debate, template: 'debates/_refresh_flag_actions' end diff --git a/app/models/flag.rb b/app/models/flag.rb index d2f30d284..8d54c2dc7 100644 --- a/app/models/flag.rb +++ b/app/models/flag.rb @@ -9,28 +9,14 @@ class Flag < ActiveRecord::Base flaggable_id: flaggable.id) end) - - class AlreadyFlaggedError < StandardError - def initialize - super "The flaggable was already flagged by this user" - end - end - - class NotFlaggedError < StandardError - def initialize - super "The flaggable was not flagged by this user" - end - end - - - def self.flag!(user, flaggable) - raise AlreadyFlaggedError if flagged?(user, flaggable) + def self.flag(user, flaggable) + return false if flagged?(user, flaggable) create(user: user, flaggable: flaggable) end - def self.unflag!(user, flaggable) + def self.unflag(user, flaggable) flags = by_user_and_flaggable(user, flaggable) - raise NotFlaggedError if flags.empty? + return false if flags.empty? flags.destroy_all end diff --git a/spec/factories.rb b/spec/factories.rb index 7a85a0193..344b178a8 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -68,7 +68,7 @@ FactoryGirl.define do trait :flagged do after :create do |debate| - Flag.flag!(FactoryGirl.create(:user), debate) + Flag.flag(FactoryGirl.create(:user), debate) end end end @@ -101,7 +101,7 @@ FactoryGirl.define do trait :flagged do after :create do |debate| - Flag.flag!(FactoryGirl.create(:user), debate) + Flag.flag(FactoryGirl.create(:user), debate) end end end diff --git a/spec/features/comments_spec.rb b/spec/features/comments_spec.rb index 9814a4b33..c40863534 100644 --- a/spec/features/comments_spec.rb +++ b/spec/features/comments_spec.rb @@ -155,7 +155,7 @@ feature 'Comments' do user = create(:user) debate = create(:debate) comment = create(:comment, commentable: debate) - Flag.flag!(user, comment) + Flag.flag(user, comment) login_as(user) visit debate_path(debate) diff --git a/spec/features/debates_spec.rb b/spec/features/debates_spec.rb index da966f7a3..f6e07b844 100644 --- a/spec/features/debates_spec.rb +++ b/spec/features/debates_spec.rb @@ -335,7 +335,7 @@ feature 'Debates' do scenario "Unflagging", :js do user = create(:user) debate = create(:debate) - Flag.flag!(user, debate) + Flag.flag(user, debate) login_as(user) visit debate_path(debate) @@ -432,9 +432,9 @@ feature 'Debates' do expect(current_url).to include('tag=Deporte') expect(page).to have_selector('#debates .debate', count: 2) - expect(page).to_not have_content(debate3.title) - expect(page).to have_content(debate1.title) - expect(page).to have_content(debate2.title) + expect(page).to_not have_content(debate3.title) + expect(page).to have_content(debate1.title) + expect(page).to have_content(debate2.title) end end end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 06e2a9a62..43dc06a23 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -52,14 +52,14 @@ describe Ability do end describe "already-flagged comments" do - before(:each) { Flag.flag!(user, comment) } + before(:each) { Flag.flag(user, comment) } it { should_not be_able_to(:flag, comment) } it { should be_able_to(:unflag, comment) } end describe "already-flagged debates" do - before(:each) { Flag.flag!(user, debate) } + before(:each) { Flag.flag(user, debate) } it { should_not be_able_to(:flag, debate) } it { should be_able_to(:unflag, debate) } diff --git a/spec/models/flag_spec.rb b/spec/models/flag_spec.rb index a5725e7c8..248bd04e1 100644 --- a/spec/models/flag_spec.rb +++ b/spec/models/flag_spec.rb @@ -5,41 +5,41 @@ describe Flag do let(:user) { create(:user) } let(:comment) { create(:comment) } - describe '.flag!' do + describe '.flag' do it 'creates a flag when there is none' do - expect { described_class.flag!(user, comment) }.to change{ Flag.count }.by(1) + expect { described_class.flag(user, comment) }.to change{ Flag.count }.by(1) expect(Flag.last.user).to eq(user) expect(Flag.last.flaggable).to eq(comment) end - it 'raises an error if the flag has already been created' do - described_class.flag!(user, comment) - expect { described_class.flag!(user, comment) }.to raise_error(Flag::AlreadyFlaggedError) + it 'does nothing if the flag already exists' do + described_class.flag(user, comment) + expect(described_class.flag(user, comment)).to eq(false) + expect(Flag.by_user_and_flaggable(user, comment).count).to eq(1) end it 'increases the flag count' do - expect { described_class.flag!(user, comment) }.to change{ comment.reload.flags_count }.by(1) + expect { described_class.flag(user, comment) }.to change{ comment.reload.flags_count }.by(1) end end - describe '.unflag!' do + describe '.unflag' do it 'raises an error if the flag does not exist' do - expect { described_class.unflag!(user, comment) }.to raise_error(Flag::NotFlaggedError) + expect(described_class.unflag(user, comment)).to eq(false) end describe 'when the flag already exists' do - before(:each) { described_class.flag!(user, comment) } + before(:each) { described_class.flag(user, comment) } it 'removes an existing flag' do - expect { described_class.unflag!(user, comment) }.to change{ Flag.count }.by(-1) + expect { described_class.unflag(user, comment) }.to change{ Flag.count }.by(-1) end it 'decreases the flag count' do - expect { described_class.unflag!(user, comment) }.to change{ comment.reload.flags_count }.by(-1) + expect { described_class.unflag(user, comment) }.to change{ comment.reload.flags_count }.by(-1) end end - end describe '.flagged?' do @@ -48,7 +48,7 @@ describe Flag do end it 'returns true when the user has flagged the comment' do - described_class.flag!(user, comment) + described_class.flag(user, comment) expect(described_class.flagged?(user, comment)).to be end end