tests

app/assets/javascripts/subscription.js.coffee

@@ -1,14 +1,13 @@
 class @Subscription
   constructor: (url) ->
     $(".subscribe-button").click (event)=>
-      self = @
       btn = $(event.currentTarget)
       action = btn.prop("value")
       current_status = $(".sub_status").text().trim()
       $(".fa-spinner.subscription").removeClass("hidden")
       $(".sub_status").empty()
       
-      $.post url, subscription: action, =>
+      $.post url, =>
         $(".fa-spinner.subscription").addClass("hidden")
         status = if current_status == "subscribed" then "unsubscribed" else "subscribed"
         $(".sub_status").text(status)

app/controllers/projects/issues_controller.rb

@@ -1,6 +1,6 @@
 class Projects::IssuesController < Projects::ApplicationController
   before_filter :module_enabled
-  before_filter :issue, only: [:edit, :update, :show, :set_subscription]
+  before_filter :issue, only: [:edit, :update, :show, :toggle_subscription]
 
   # Allow read any issue
   before_filter :authorize_read_issue!
@@ -97,11 +97,8 @@ class Projects::IssuesController < Projects::ApplicationController
     redirect_to :back, notice: "#{result[:count]} issues updated"
   end
 
-  def set_subscription
-    subscribed = params[:subscription] == "Subscribe"
-
-    sub = @issue.subscriptions.find_or_create_by(user_id: current_user.id)
-    sub.update(subscribed: subscribed)
+  def toggle_subscription
+    @issue.toggle_subscription(current_user)
     
     render nothing: true
   end

app/controllers/projects/merge_requests_controller.rb

@@ -2,7 +2,7 @@ require 'gitlab/satellite/satellite'
 
 class Projects::MergeRequestsController < Projects::ApplicationController
   before_filter :module_enabled
-  before_filter :merge_request, only: [:edit, :update, :show, :diffs, :automerge, :automerge_check, :ci_status, :set_subscription]
+  before_filter :merge_request, only: [:edit, :update, :show, :diffs, :automerge, :automerge_check, :ci_status, :toggle_subscription]
   before_filter :closes_issues, only: [:edit, :update, :show, :diffs]
   before_filter :validates_merge_request, only: [:show, :diffs]
   before_filter :define_show_vars, only: [:show, :diffs]
@@ -174,11 +174,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     render json: response
   end
 
-  def set_subscription
-    subscribed = params[:subscription] == "Subscribe"
-
-    sub = @merge_request.subscriptions.find_or_create_by(user_id: current_user.id)
-    sub.update(subscribed: subscribed)
+  def toggle_subscription
+    @merge_request.toggle_subscription(current_user)
     
     render nothing: true
   end

app/models/concerns/issuable.rb

@@ -133,7 +133,7 @@ module Issuable
     users.concat(mentions.reduce([], :|)).uniq
   end
 
-  def subscription_status(user)
+  def subscribed?(user)
     subscription = subscriptions.find_by_user_id(user.id)
 
     if subscription
@@ -143,6 +143,12 @@ module Issuable
     participants.include?(user)
   end
 
+  def toggle_subscription(user)
+    subscriptions.
+      find_or_initialize_by(user_id: user.id).
+      update(subscribed: !subscribed?(user))
+  end
+
   def to_hook_data(user)
     {
       object_kind: self.class.name.underscore,

app/models/subscription.rb

@@ -1,3 +1,16 @@
+# == Schema Information
+#
+# Table name: subscriptions
+#
+#  id                :integer          not null, primary key
+#  user_id           :integer
+#  subscribable_id   :integer
+#  subscribable_type :string(255)
+#  subscribed        :boolean
+#  created_at        :datetime
+#  updated_at        :datetime
+#
+
 class Subscription < ActiveRecord::Base
   belongs_to :user
   belongs_to :subscribable, polymorphic: true

app/services/notification_service.rb

@@ -92,6 +92,8 @@ class NotificationService
   #
   def merge_mr(merge_request, current_user)
     recipients = reject_muted_users([merge_request.author, merge_request.assignee], merge_request.target_project)
+    recipients = add_subscribed_users(recipients, merge_request)
+    recipients = reject_unsubscribed_users(recipients, merge_request)
     recipients = recipients.concat(project_watchers(merge_request.target_project)).uniq
     recipients.delete(current_user)
 
@@ -333,7 +335,7 @@ class NotificationService
     subscriptions = target.subscriptions
 
     if subscriptions.any?
-      recipients + subscriptions.where("subscribed is true").map(&:user)
+      recipients + subscriptions.where(subscribed: true).map(&:user)
     else
       recipients
     end

app/views/projects/issues/_issue_context.html.haml

@@ -33,12 +33,12 @@
         Subscription:
         %i.fa.fa-spinner.fa-spin.hidden.subscription
         %span.sub_status
-          = @issue.subscription_status(current_user) ? "subscribed" : "unsubscribed"
-    - subscribe_action = @issue.subscription_status(current_user) ? "Unsubscribe" : "Subscribe"
+          = @issue.subscribed?(current_user) ? "subscribed" : "unsubscribed"
+    - subscribe_action = @issue.subscribed?(current_user) ? "Unsubscribe" : "Subscribe"
     %input.btn.subscribe-button{:type => "button", :value => subscribe_action}
 
 :coffeescript
   $ ->
-    new Subscription("#{set_subscription_namespace_project_issue_path(@issue.project.namespace, @project, @issue)}")
+    new Subscription("#{toggle_subscription_namespace_project_issue_path(@issue.project.namespace, @project, @issue)}")
     
       

app/views/projects/merge_requests/show/_context.html.haml

@@ -35,12 +35,12 @@
         Subscription:
         %i.fa.fa-spinner.fa-spin.hidden.subscription
         %span.sub_status
-          = @merge_request.subscription_status(current_user) ? "subscribed" : "unsubscribed"
-    - subscribe_action = @merge_request.subscription_status(current_user) ? "Unsubscribe" : "Subscribe"
+          = @merge_request.subscribed?(current_user) ? "subscribed" : "unsubscribed"
+    - subscribe_action = @merge_request.subscribed?(current_user) ? "Unsubscribe" : "Subscribe"
     %input.btn.subscribe-button{:type => "button", :value => subscribe_action}
 
 :coffeescript
   $ ->
-    new Subscription("#{set_subscription_namespace_project_issue_path(@merge_request.project.namespace, @project, @merge_request)}")
+    new Subscription("#{toggle_subscription_namespace_project_merge_request_path(@merge_request.project.namespace, @project, @merge_request)}")
     
       

config/routes.rb

@@ -406,7 +406,7 @@ Gitlab::Application.routes.draw do
             post :automerge
             get :automerge_check
             get :ci_status
-            post :set_subscription
+            post :toggle_subscription
           end
 
           collection do
@@ -442,7 +442,7 @@ Gitlab::Application.routes.draw do
 
         resources :issues, constraints: { id: /\d+/ }, except: [:destroy] do
           member do
-            post :set_subscription
+            post :toggle_subscription
           end
           collection do
             post  :bulk_update

db/migrate/20150313012111_create_subscriptions_table.rb

@@ -8,6 +8,9 @@ class CreateSubscriptionsTable < ActiveRecord::Migration
       t.timestamps
     end
 
-    add_index :subscriptions, [:subscribable_id, :subscribable_type, :user_id], unique: true, name: 'subscriptions_user_id_and_ref_fields'
+    add_index :subscriptions, 
+              [:subscribable_id, :subscribable_type, :user_id],
+              unique: true,
+              name: 'subscriptions_user_id_and_ref_fields'
   end
 end

features/project/issues/issues.feature

@@ -202,3 +202,11 @@ Feature: Project Issues
     And I click link "Edit" for the issue
     And I preview a description text like "Bug fixed :smile:"
     Then I should see the Markdown write tab
+
+  @javascript
+  Scenario: I can unsubscribe from issue
+    Given project "Shop" has "Tasks-open" open issue with task markdown
+    When I visit issue page "Tasks-open"
+    Then I should see that I am subscribed
+    When I click button "Unsubscribe"
+    Then I should see that I am unsubscribed

features/project/merge_requests.feature

@@ -225,3 +225,10 @@ Feature: Project Merge Requests
     When I fill in merge request search with "Fe"
     Then I should see "Feature NS-03" in merge requests
     And I should not see "Bug NS-04" in merge requests
+
+  @javascript
+  Scenario: I can unsubscribe from merge request
+    Given I visit merge request page "Bug NS-04"
+    Then I should see that I am subscribed
+    When I click button "Unsubscribe"
+    Then I should see that I am unsubscribed

features/steps/project/issues/issues.rb

@@ -18,10 +18,23 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps
     page.should_not have_content "Tweet control"
   end
 
+  step 'I should see that I am subscribed' do
+    find(".sub_status").text.should == "subscribed"
+  end
+
+  step 'I should see that I am unsubscribed' do
+    sleep 0.2
+    find(".sub_status").text.should == "unsubscribed"
+  end
+
   step 'I click link "Closed"' do
     click_link "Closed"
   end
 
+  step 'I click button "Unsubscribe"' do
+    click_on "Unsubscribe"
+  end
+
   step 'I should see "Release 0.3" in issues' do
     page.should have_content "Release 0.3"
   end

features/steps/project/merge_requests.rb

@@ -56,6 +56,19 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     page.should_not have_content "Bug NS-04"
   end
 
+  step 'I should see that I am subscribed' do
+    find(".sub_status").text.should == "subscribed"
+  end
+
+  step 'I should see that I am unsubscribed' do
+    sleep 0.2
+    find(".sub_status").text.should == "unsubscribed"
+  end
+
+  step 'I click button "Unsubscribe"' do
+    click_on "Unsubscribe"
+  end
+
   step 'I click link "Close"' do
     first(:css, '.close-mr-link').click
   end

spec/services/notification_service_spec.rb

@@ -41,13 +41,18 @@ describe NotificationService do
 
       describe :new_note do
         it do
+          add_users_with_subscription(note.project, issue)
+
           should_email(@u_watcher.id)
           should_email(note.noteable.author_id)
           should_email(note.noteable.assignee_id)
           should_email(@u_mentioned.id)
+          should_email(@subscriber.id)
           should_not_email(note.author_id)
           should_not_email(@u_participating.id)
           should_not_email(@u_disabled.id)
+          should_not_email(@unsubscriber.id)
+
           notification.new_note(note)
         end
 
@@ -191,6 +196,7 @@ describe NotificationService do
 
     before do
       build_team(issue.project)
+      add_users_with_subscription(issue.project, issue)
     end
 
     describe :new_issue do
@@ -224,6 +230,8 @@ describe NotificationService do
         should_email(issue.assignee_id)
         should_email(@u_watcher.id)
         should_email(@u_participant_mentioned.id)
+        should_email(@subscriber.id)
+        should_not_email(@unsubscriber.id)
         should_not_email(@u_participating.id)
         should_not_email(@u_disabled.id)
 
@@ -245,6 +253,8 @@ describe NotificationService do
         should_email(issue.author_id)
         should_email(@u_watcher.id)
         should_email(@u_participant_mentioned.id)
+        should_email(@subscriber.id)
+        should_not_email(@unsubscriber.id)
         should_not_email(@u_participating.id)
         should_not_email(@u_disabled.id)
 
@@ -266,6 +276,8 @@ describe NotificationService do
         should_email(issue.author_id)
         should_email(@u_watcher.id)
         should_email(@u_participant_mentioned.id)
+        should_email(@subscriber.id)
+        should_not_email(@unsubscriber.id)
         should_not_email(@u_participating.id)
         should_not_email(@u_disabled.id)
 
@@ -287,6 +299,7 @@ describe NotificationService do
 
     before do
       build_team(merge_request.target_project)
+      add_users_with_subscription(merge_request.target_project, merge_request)
     end
 
     describe :new_merge_request do
@@ -311,6 +324,8 @@ describe NotificationService do
       it do
         should_email(merge_request.assignee_id)
         should_email(@u_watcher.id)
+        should_email(@subscriber.id)
+        should_not_email(@unsubscriber.id)
         should_not_email(@u_participating.id)
         should_not_email(@u_disabled.id)
         notification.reassigned_merge_request(merge_request, merge_request.author)
@@ -329,6 +344,8 @@ describe NotificationService do
       it do
         should_email(merge_request.assignee_id)
         should_email(@u_watcher.id)
+        should_email(@subscriber.id)
+        should_not_email(@unsubscriber.id)
         should_not_email(@u_participating.id)
         should_not_email(@u_disabled.id)
         notification.close_mr(merge_request, @u_disabled)
@@ -347,6 +364,8 @@ describe NotificationService do
       it do
         should_email(merge_request.assignee_id)
         should_email(@u_watcher.id)
+        should_email(@subscriber.id)
+        should_not_email(@unsubscriber.id)
         should_not_email(@u_participating.id)
         should_not_email(@u_disabled.id)
         notification.merge_mr(merge_request, @u_disabled)
@@ -365,6 +384,8 @@ describe NotificationService do
       it do
         should_email(merge_request.assignee_id)
         should_email(@u_watcher.id)
+        should_email(@subscriber.id)
+        should_not_email(@unsubscriber.id)
         should_not_email(@u_participating.id)
         should_not_email(@u_disabled.id)
         notification.reopen_mr(merge_request, @u_disabled)
@@ -420,4 +441,15 @@ describe NotificationService do
     project.team << [@u_mentioned, :master]
     project.team << [@u_committer, :master]
   end
+
+  def add_users_with_subscription(project, issuable)
+    @subscriber = create :user
+    @unsubscriber = create :user
+
+    project.team << [@subscriber, :master]
+    project.team << [@unsubscriber, :master]
+
+    issuable.subscriptions.create(user: @subscriber, subscribed: true)
+    issuable.subscriptions.create(user: @unsubscriber, subscribed: false)
+  end
 end