Allow subscriptions to be created without a project

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