From 0aac2e0706cd767993148826d723aa3641cbb2a4 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 14 Nov 2016 20:42:22 -0200 Subject: Allow subscriptions to be created without a project --- app/models/concerns/subscribable.rb | 21 ++++-- app/models/subscription.rb | 6 +- spec/models/concerns/subscribable_spec.rb | 109 ++++++++++++++++++++++------- spec/models/subscription_spec.rb | 1 - spec/services/notification_service_spec.rb | 24 +++++-- 5 files changed, 119 insertions(+), 42 deletions(-) diff --git a/app/models/concerns/subscribable.rb b/app/models/concerns/subscribable.rb index 5f2725a6d7f..0723db548d8 100644 --- a/app/models/concerns/subscribable.rb +++ b/app/models/concerns/subscribable.rb @@ -12,7 +12,7 @@ module Subscribable has_many :subscriptions, dependent: :destroy, as: :subscribable end - def subscribed?(user, project) + def subscribed?(user, project = nil) if subscription = subscriptions.find_by(user: user, project: project) subscription.subscribed else @@ -27,20 +27,22 @@ module Subscribable end def subscribers(project) - subscriptions.where(project: project, subscribed: true).map(&:user) + subscriptions_available(project). + where(subscribed: true). + map(&:user) end - def toggle_subscription(user, project) + def toggle_subscription(user, project = nil) find_or_initialize_subscription(user, project). update(subscribed: !subscribed?(user, project)) end - def subscribe(user, project) + def subscribe(user, project = nil) find_or_initialize_subscription(user, project). update(subscribed: true) end - def unsubscribe(user, project) + def unsubscribe(user, project = nil) find_or_initialize_subscription(user, project). update(subscribed: false) end @@ -49,6 +51,13 @@ module Subscribable def find_or_initialize_subscription(user, project) subscriptions. - find_or_initialize_by(user_id: user.id, project_id: project.id) + find_or_initialize_by(user_id: user.id, project_id: project.try(:id)) + end + + def subscriptions_available(project) + t = Subscription.arel_table + + subscriptions. + where(t[:project_id].eq(nil).or(t[:project_id].eq(project.try(:id)))) end end diff --git a/app/models/subscription.rb b/app/models/subscription.rb index f881d999384..17869c8bac2 100644 --- a/app/models/subscription.rb +++ b/app/models/subscription.rb @@ -3,9 +3,7 @@ class Subscription < ActiveRecord::Base belongs_to :project belongs_to :subscribable, polymorphic: true - validates :user, :project, :subscribable, presence: true + validates :user, :subscribable, presence: true - validates :project_id, - uniqueness: { scope: [:subscribable_id, :subscribable_type, :user_id] }, - presence: true + validates :project_id, uniqueness: { scope: [:subscribable_id, :subscribable_type, :user_id] } end diff --git a/spec/models/concerns/subscribable_spec.rb b/spec/models/concerns/subscribable_spec.rb index 0a96cbbe166..58f5c164116 100644 --- a/spec/models/concerns/subscribable_spec.rb +++ b/spec/models/concerns/subscribable_spec.rb @@ -3,23 +3,43 @@ require 'spec_helper' describe Subscribable, 'Subscribable' do let(:project) { create(:empty_project) } let(:resource) { create(:issue, project: project) } - let(:user) { create(:user) } + let(:user_1) { create(:user) } describe '#subscribed?' do - it 'returns false when no subcription exists' do - expect(resource.subscribed?(user, project)).to be_falsey - end + context 'without project' do + it 'returns false when no subscription exists' do + expect(resource.subscribed?(user_1)).to be_falsey + end + + it 'returns true when a subcription exists and subscribed is true' do + resource.subscriptions.create(user: user_1, subscribed: true) + + expect(resource.subscribed?(user_1)).to be_truthy + end - it 'returns true when a subcription exists and subscribed is true' do - resource.subscriptions.create(user: user, project: project, subscribed: true) + it 'returns false when a subcription exists and subscribed is false' do + resource.subscriptions.create(user: user_1, subscribed: false) - expect(resource.subscribed?(user, project)).to be_truthy + expect(resource.subscribed?(user_1)).to be_falsey + end end - it 'returns false when a subcription exists and subscribed is false' do - resource.subscriptions.create(user: user, project: project, subscribed: false) + context 'with project' do + it 'returns false when no subscription exists' do + expect(resource.subscribed?(user_1, project)).to be_falsey + end + + it 'returns true when a subcription exists and subscribed is true' do + resource.subscriptions.create(user: user_1, project: project, subscribed: true) + + expect(resource.subscribed?(user_1, project)).to be_truthy + end - expect(resource.subscribed?(user, project)).to be_falsey + it 'returns false when a subcription exists and subscribed is false' do + resource.subscriptions.create(user: user_1, project: project, subscribed: false) + + expect(resource.subscribed?(user_1, project)).to be_falsey + end end end @@ -29,41 +49,80 @@ describe Subscribable, 'Subscribable' do end it 'returns the subscribed users' do - resource.subscriptions.create(user: user, project: project, subscribed: true) + user_2 = create(:user) + resource.subscriptions.create(user: user_1, subscribed: true) + resource.subscriptions.create(user: user_2, project: project, subscribed: true) resource.subscriptions.create(user: create(:user), project: project, subscribed: false) - expect(resource.subscribers(project)).to eq [user] + expect(resource.subscribers(project)).to contain_exactly(user_1, user_2) end end describe '#toggle_subscription' do - it 'toggles the current subscription state for the given user' do - expect(resource.subscribed?(user, project)).to be_falsey + context 'without project' do + it 'toggles the current subscription state for the given user' do + expect(resource.subscribed?(user_1)).to be_falsey + + resource.toggle_subscription(user_1) + + expect(resource.subscribed?(user_1)).to be_truthy + end + end - resource.toggle_subscription(user, project) + context 'with project' do + it 'toggles the current subscription state for the given user' do + expect(resource.subscribed?(user_1, project)).to be_falsey - expect(resource.subscribed?(user, project)).to be_truthy + resource.toggle_subscription(user_1, project) + + expect(resource.subscribed?(user_1, project)).to be_truthy + end end end describe '#subscribe' do - it 'subscribes the given user' do - expect(resource.subscribed?(user, project)).to be_falsey + context 'without project' do + it 'subscribes the given user' do + expect(resource.subscribed?(user_1)).to be_falsey + + resource.subscribe(user_1) + + expect(resource.subscribed?(user_1)).to be_truthy + end + end + + context 'with project' do + it 'subscribes the given user' do + expect(resource.subscribed?(user_1, project)).to be_falsey - resource.subscribe(user, project) + resource.subscribe(user_1, project) - expect(resource.subscribed?(user, project)).to be_truthy + expect(resource.subscribed?(user_1, project)).to be_truthy + end end end describe '#unsubscribe' do - it 'unsubscribes the given current user' do - resource.subscriptions.create(user: user, project: project, subscribed: true) - expect(resource.subscribed?(user, project)).to be_truthy + context 'without project' do + it 'unsubscribes the given current user' do + resource.subscriptions.create(user: user_1, subscribed: true) + expect(resource.subscribed?(user_1)).to be_truthy + + resource.unsubscribe(user_1) + + expect(resource.subscribed?(user_1)).to be_falsey + end + end + + context 'with project' do + it 'unsubscribes the given current user' do + resource.subscriptions.create(user: user_1, project: project, subscribed: true) + expect(resource.subscribed?(user_1, project)).to be_truthy - resource.unsubscribe(user, project) + resource.unsubscribe(user_1, project) - expect(resource.subscribed?(user, project)).to be_falsey + expect(resource.subscribed?(user_1, project)).to be_falsey + end end end end diff --git a/spec/models/subscription_spec.rb b/spec/models/subscription_spec.rb index ab674958387..9ab112bb2ee 100644 --- a/spec/models/subscription_spec.rb +++ b/spec/models/subscription_spec.rb @@ -8,7 +8,6 @@ describe Subscription, models: true do end describe 'validations' do - it { is_expected.to validate_presence_of(:project) } it { is_expected.to validate_presence_of(:subscribable) } it { is_expected.to validate_presence_of(:user) } diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 64e378d8328..8726c9eaa55 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -382,18 +382,21 @@ describe NotificationService, services: true do user_1 = create(:user) user_2 = create(:user) user_3 = create(:user) + user_4 = create(:user) label = create(:label, project: project, issues: [issue]) group_label = create(:group_label, group: group, issues: [issue]) issue.reload label.toggle_subscription(user_1, project) group_label.toggle_subscription(user_2, project) group_label.toggle_subscription(user_3, another_project) + group_label.toggle_subscription(user_4) notification.new_issue(issue, @u_disabled) should_email(user_1) should_email(user_2) should_not_email(user_3) + should_email(user_4) end context 'confidential issues' do @@ -569,7 +572,8 @@ describe NotificationService, services: true do let(:label_1) { create(:label, project: project, title: 'Label 1', issues: [issue]) } let(:label_2) { create(:label, project: project, title: 'Label 2') } let!(:subscriber_to_group_label_1) { create(:user) { |u| group_label_1.toggle_subscription(u, project) } } - let!(:subscriber_to_group_label_2) { create(:user) { |u| group_label_2.toggle_subscription(u, project) } } + let!(:subscriber_1_to_group_label_2) { create(:user) { |u| group_label_2.toggle_subscription(u, project) } } + let!(:subscriber_2_to_group_label_2) { create(:user) { |u| group_label_2.toggle_subscription(u) } } let!(:subscriber_to_group_label_2_on_another_project) { create(:user) { |u| group_label_2.toggle_subscription(u, another_project) } } let!(:subscriber_to_label_1) { create(:user) { |u| label_1.toggle_subscription(u, project) } } let!(:subscriber_to_label_2) { create(:user) { |u| label_2.toggle_subscription(u, project) } } @@ -580,7 +584,8 @@ describe NotificationService, services: true do should_not_email(subscriber_to_label_1) should_not_email(subscriber_to_group_label_1) should_not_email(subscriber_to_group_label_2_on_another_project) - should_email(subscriber_to_group_label_2) + should_email(subscriber_1_to_group_label_2) + should_email(subscriber_2_to_group_label_2) should_email(subscriber_to_label_2) end @@ -599,7 +604,8 @@ describe NotificationService, services: true do should_not_email(subscriber_to_label_1) should_not_email(subscriber_to_group_label_1) should_not_email(subscriber_to_group_label_2_on_another_project) - should_email(subscriber_to_group_label_2) + should_email(subscriber_1_to_group_label_2) + should_email(subscriber_2_to_group_label_2) should_email(subscriber_to_label_2) end @@ -784,17 +790,20 @@ describe NotificationService, services: true do user_1 = create(:user) user_2 = create(:user) user_3 = create(:user) + user_4 = create(:user) label = create(:label, project: project, merge_requests: [merge_request]) group_label = create(:group_label, group: group, merge_requests: [merge_request]) label.toggle_subscription(user_1, project) group_label.toggle_subscription(user_2, project) group_label.toggle_subscription(user_3, another_project) + group_label.toggle_subscription(user_4) notification.new_merge_request(merge_request, @u_disabled) should_email(user_1) should_email(user_2) should_not_email(user_3) + should_email(user_4) end context 'participating' do @@ -893,7 +902,8 @@ describe NotificationService, services: true do let(:label_1) { create(:label, project: project, title: 'Label 1', merge_requests: [merge_request]) } let(:label_2) { create(:label, project: project, title: 'Label 2') } let!(:subscriber_to_group_label_1) { create(:user) { |u| group_label_1.toggle_subscription(u, project) } } - let!(:subscriber_to_group_label_2) { create(:user) { |u| group_label_2.toggle_subscription(u, project) } } + let!(:subscriber_1_to_group_label_2) { create(:user) { |u| group_label_2.toggle_subscription(u, project) } } + let!(:subscriber_2_to_group_label_2) { create(:user) { |u| group_label_2.toggle_subscription(u) } } let!(:subscriber_to_group_label_2_on_another_project) { create(:user) { |u| group_label_2.toggle_subscription(u, another_project) } } let!(:subscriber_to_label_1) { create(:user) { |u| label_1.toggle_subscription(u, project) } } let!(:subscriber_to_label_2) { create(:user) { |u| label_2.toggle_subscription(u, project) } } @@ -904,7 +914,8 @@ describe NotificationService, services: true do should_not_email(subscriber_to_label_1) should_not_email(subscriber_to_group_label_1) should_not_email(subscriber_to_group_label_2_on_another_project) - should_email(subscriber_to_group_label_2) + should_email(subscriber_1_to_group_label_2) + should_email(subscriber_2_to_group_label_2) should_email(subscriber_to_label_2) end @@ -923,7 +934,8 @@ describe NotificationService, services: true do should_not_email(subscriber_to_label_1) should_not_email(subscriber_to_group_label_1) should_not_email(subscriber_to_group_label_2_on_another_project) - should_email(subscriber_to_group_label_2) + should_email(subscriber_1_to_group_label_2) + should_email(subscriber_2_to_group_label_2) should_email(subscriber_to_label_2) end end -- cgit v1.2.1