Improving the original label-subscribing implementation

1. Make the "subscribed" text in Issuable sidebar reflect the labels
   subscription status

2. Current user mut be logged-in to toggle issue/MR/label subscription

CHANGELOG

@@ -28,6 +28,7 @@ v 8.6.0 (unreleased)
   - Update documentation to reflect Guest role not being enforced on internal projects
   - Allow search for logged out users
   - Allow to define on which builds the current one depends on
+  - Allow user subscription to a label: get notified for issues/merge requests related to that label (Timothy Andrew)
   - Fix bug where Bitbucket `closed` issues were imported as `opened` (Iuri de Silvio)
   - Don't show Issues/MRs from archived projects in Groups view
   - Fix wrong "iid of max iid" in Issuable sidebar for some merged MRs
@@ -174,8 +175,6 @@ v 8.5.0
 
 v 8.4.5
   - No CE-specific changes
-  - Allow user subscription to a label; get notified for issues/merge requests related to that label.
-  - Allow user subscription to a label; get notified for issues/merge requests related to that label. (Timothy Andrew)
 
 v 8.4.4
   - Update omniauth-saml gem to 1.4.2

app/assets/javascripts/subscription.js.coffee

 class @Subscription
-  constructor: (@url, container) ->
-    @subscribe_button = $(container).find(".subscribe-button")
-    @subscription_status = $(container).find(".subscription-status")
-    @subscribe_button.unbind("click").click(@toggleSubscription)
+  constructor: (container) ->
+    $container = $(container)
+    @url = $container.attr('data-url')
+    @subscribe_button = $container.find('.subscribe-button')
+    @subscription_status = $container.find('.subscription-status')
+    @subscribe_button.unbind('click').click(@toggleSubscription)
 
   toggleSubscription: (event) =>
     btn = $(event.currentTarget)
-    action = btn.find("span").text()
-    current_status = @subscription_status.attr("data-status")
-    btn.prop("disabled", true)
+    action = btn.find('span').text()
+    current_status = @subscription_status.attr('data-status')
+    btn.prop('disabled', true)
 
     $.post @url, =>
-      btn.prop("disabled", false)
-      status = if current_status == "subscribed" then "unsubscribed" else "subscribed"
-      @subscription_status.attr("data-status", status)
-      action = if status == "subscribed" then "Unsubscribe" else "Subscribe"
-      btn.find("span").text(action)
-      @subscription_status.find(">div").toggleClass("hidden")
+      btn.prop('disabled', false)
+      status = if current_status == 'subscribed' then 'unsubscribed' else 'subscribed'
+      @subscription_status.attr('data-status', status)
+      action = if status == 'subscribed' then 'Unsubscribe' else 'Subscribe'
+      btn.find('span').text(action)
+      @subscription_status.find('>div').toggleClass('hidden')

app/assets/stylesheets/pages/labels.scss

@@ -43,5 +43,5 @@
 }
 
 .label-subscription {
-    display: inline-block;
-}
+  display: inline-block;
+}
\ No newline at end of file

app/controllers/projects/issues_controller.rb

@@ -111,6 +111,8 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def toggle_subscription
+    return unless current_user
+
     @issue.toggle_subscription(current_user)
 
     render nothing: true

app/controllers/projects/labels_controller.rb

@@ -61,6 +61,8 @@ class Projects::LabelsController < Projects::ApplicationController
   end
 
   def toggle_subscription
+    return unless current_user
+
     @label.toggle_subscription(current_user)
     render nothing: true
   end

app/controllers/projects/merge_requests_controller.rb

@@ -234,6 +234,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def toggle_subscription
+    return unless current_user
+
     @merge_request.toggle_subscription(current_user)
 
     render nothing: true

app/helpers/labels_helper.rb

@@ -124,6 +124,14 @@ module LabelsHelper
     options_from_collection_for_select(grouped_labels, 'name', 'title', params[:label_name])
   end
 
+  def label_subscription_status(label)
+    label.subscribed?(current_user) ? 'subscribed' : 'unsubscribed'
+  end
+
+  def label_subscription_toggle_button_text(label)
+    label.subscribed?(current_user) ? 'Unsubscribe' : 'Subscribe'
+  end
+
   # Required for Banzai::Filter::LabelReferenceFilter
   module_function :render_colored_label, :render_colored_cross_project_label,
                   :text_color_for_bg, :escape_once

app/mailers/emails/issues.rb

@@ -20,10 +20,11 @@ module Emails
       mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id))
     end
 
-    def relabeled_issue_email(recipient_id, issue_id, updated_by_user_id, label_names)
-      setup_issue_mail(issue_id, recipient_id)
+    def relabeled_issue_email(recipient_id, issue_id, label_names, updated_by_user_id)
+      setup_issue_mail(issue_id, recipient_id, sent_notification: false)
+
       @label_names = label_names
-      @updated_by = User.find(updated_by_user_id)
+      @labels_url = namespace_project_labels_url(@project.namespace, @project)
       mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id))
     end
 
@@ -37,6 +38,16 @@ module Emails
 
     private
 
+    def setup_issue_mail(issue_id, recipient_id, sent_notification: true)
+      @issue = Issue.find(issue_id)
+      @project = @issue.project
+      @target_url = namespace_project_issue_url(@project.namespace, @project, @issue)
+
+      if sent_notification
+        @sent_notification = SentNotification.record(@issue, recipient_id, reply_key)
+      end
+    end
+
     def issue_thread_options(sender_id, recipient_id)
       {
         from: sender(sender_id),
@@ -44,13 +55,5 @@ module Emails
         subject: subject("#{@issue.title} (##{@issue.iid})")
       }
     end
-
-    def setup_issue_mail(issue_id, recipient_id)
-      @issue = Issue.find(issue_id)
-      @project = @issue.project
-      @target_url = namespace_project_issue_url(@project.namespace, @project, @issue)
-
-      @sent_notification = SentNotification.record(@issue, recipient_id, reply_key)
-    end
   end
 end

app/mailers/emails/merge_requests.rb

@@ -3,49 +3,35 @@ module Emails
     def new_merge_request_email(recipient_id, merge_request_id)
       setup_merge_request_mail(merge_request_id, recipient_id)
 
-      mail_new_thread(@merge_request,
-                      from: sender(@merge_request.author_id),
-                      to: recipient(recipient_id),
-                      subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+      mail_new_thread(@merge_request, merge_request_thread_options(@merge_request.author_id, recipient_id))
     end
 
     def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id, updated_by_user_id)
       setup_merge_request_mail(merge_request_id, recipient_id)
 
       @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
-      mail_answer_thread(@merge_request,
-                         from: sender(updated_by_user_id),
-                         to: recipient(recipient_id),
-                         subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+      mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
     end
 
-    def relabeled_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, label_names)
-      setup_merge_request_mail(merge_request_id, recipient_id)
+    def relabeled_merge_request_email(recipient_id, merge_request_id, label_names, updated_by_user_id)
+      setup_merge_request_mail(merge_request_id, recipient_id, sent_notification: false)
+
       @label_names = label_names
-      @updated_by = User.find(updated_by_user_id)
-      mail_answer_thread(@merge_request,
-                      from: sender(@merge_request.author_id),
-                      to: recipient(recipient_id),
-                      subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+      @labels_url = namespace_project_labels_url(@project.namespace, @project)
+      mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
     end
 
     def closed_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
       setup_merge_request_mail(merge_request_id, recipient_id)
 
       @updated_by = User.find(updated_by_user_id)
-      mail_answer_thread(@merge_request,
-                         from: sender(updated_by_user_id),
-                         to: recipient(recipient_id),
-                         subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+      mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
     end
 
     def merged_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
       setup_merge_request_mail(merge_request_id, recipient_id)
 
-      mail_answer_thread(@merge_request,
-                         from: sender(updated_by_user_id),
-                         to: recipient(recipient_id),
-                         subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+      mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
     end
 
     def merge_request_status_email(recipient_id, merge_request_id, status, updated_by_user_id)
@@ -53,22 +39,27 @@ module Emails
 
       @mr_status = status
       @updated_by = User.find(updated_by_user_id)
-      mail_answer_thread(@merge_request,
-                         from: sender(updated_by_user_id),
-                         to: recipient(recipient_id),
-                         subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+      mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
     end
 
     private
 
-    def setup_merge_request_mail(merge_request_id, recipient_id)
+    def setup_merge_request_mail(merge_request_id, recipient_id, sent_notification: true)
       @merge_request = MergeRequest.find(merge_request_id)
       @project = @merge_request.project
-      @target_url = namespace_project_merge_request_url(@project.namespace,
-                                                        @project,
-                                                        @merge_request)
+      @target_url = namespace_project_merge_request_url(@project.namespace, @project, @merge_request)
+
+      if sent_notification
+        @sent_notification = SentNotification.record(@merge_request, recipient_id, reply_key)
+      end
+    end
 
-      @sent_notification = SentNotification.record(@merge_request, recipient_id, reply_key)
+    def merge_request_thread_options(sender_id, recipient_id)
+      {
+        from: sender(sender_id),
+        to: recipient(recipient_id),
+        subject: subject("#{@merge_request.title} (##{@merge_request.iid})")
+      }
     end
   end
 end

app/models/concerns/issuable.rb

@@ -149,6 +149,10 @@ module Issuable
     notes.awards.where(note: "thumbsup").count
   end
 
+  def subscribed_without_subscriptions?(user)
+    participants(user).include?(user)
+  end
+
   def to_hook_data(user)
     hook_data = {
       object_kind: self.class.name.underscore,
@@ -179,12 +183,6 @@ module Issuable
     end
   end
 
-  # Labels that are currently applied to this object
-  # that are not present in `old_labels`
-  def added_labels(old_labels)
-    self.labels - old_labels
-  end
-
   # Convert this Issuable class name to a format usable by Ability definitions
   #
   # Examples:

app/models/concerns/subscribable.rb

@@ -13,20 +13,21 @@ module Subscribable
   end
 
   def subscribed?(user)
-    subscription = subscriptions.find_by_user_id(user.id)
-
-    if subscription
-      return subscription.subscribed
+    if subscription = subscriptions.find_by_user_id(user.id)
+      subscription.subscribed
+    else
+      subscribed_without_subscriptions?(user)
     end
+  end
 
-    # FIXME
-    # Issue/MergeRequest has participants, but Label doesn't.
-    # Ideally, subscriptions should be separate from participations,
-    # but that seems like a larger change with farther-reaching
-    # consequences, so this is a compromise for the time being.
-    if respond_to?(:participants)
-      participants(user).include?(user)
-    end
+  # Override this method to define custom logic to consider a subscribable as
+  # subscribed without an explicit subscription record.
+  def subscribed_without_subscriptions?(user)
+    false
+  end
+
+  def subscribers
+    subscriptions.where(subscribed: true).map(&:user)
   end
 
   def toggle_subscription(user)

app/models/subscription.rb

@@ -15,7 +15,7 @@ class Subscription < ActiveRecord::Base
   belongs_to :user
   belongs_to :subscribable, polymorphic: true
 
-  validates :user_id, 
+  validates :user_id,
             uniqueness: { scope: [:subscribable_id, :subscribable_type] },
             presence: true
 end

app/services/issuable_base_service.rb

@@ -11,7 +11,10 @@ class IssuableBaseService < BaseService
       issuable, issuable.project, current_user, issuable.milestone)
   end
 
-  def create_labels_note(issuable, added_labels, removed_labels)
+  def create_labels_note(issuable, old_labels)
+    added_labels = issuable.labels - old_labels
+    removed_labels = old_labels - issuable.labels
+
     SystemNoteService.change_label(
       issuable, issuable.project, current_user, added_labels, removed_labels)
   end
@@ -71,20 +74,19 @@ class IssuableBaseService < BaseService
     end
   end
 
-  def has_changes?(issuable, options = {})
+  def has_changes?(issuable, old_labels: [])
     valid_attrs = [:title, :description, :assignee_id, :milestone_id, :target_branch]
 
     attrs_changed = valid_attrs.any? do |attr|
       issuable.previous_changes.include?(attr.to_s)
     end
 
-    old_labels = options[:old_labels]
-    labels_changed = old_labels && issuable.labels != old_labels
+    labels_changed = issuable.labels != old_labels
 
     attrs_changed || labels_changed
   end
 
-  def handle_common_system_notes(issuable, options = {})
+  def handle_common_system_notes(issuable, old_labels: [])
     if issuable.previous_changes.include?('title')
       create_title_change_note(issuable, issuable.previous_changes['title'].first)
     end
@@ -93,9 +95,6 @@ class IssuableBaseService < BaseService
       create_task_status_note(issuable)
     end
 
-    old_labels = options[:old_labels]
-    if old_labels && (issuable.labels != old_labels)
-      create_labels_note(issuable, issuable.added_labels(old_labels), old_labels - issuable.labels)
-    end
+    create_labels_note(issuable, old_labels) if issuable.labels != old_labels
   end
 end

app/services/issues/update_service.rb

@@ -4,8 +4,8 @@ module Issues
       update(issue)
     end
 
-    def handle_changes(issue, old_labels: [], new_labels: [])
-      if has_changes?(issue, options)
+    def handle_changes(issue, old_labels: [])
+      if has_changes?(issue, old_labels: old_labels)
         todo_service.mark_pending_todos_as_done(issue, current_user)
       end
 
@@ -24,9 +24,9 @@ module Issues
         todo_service.reassigned_issue(issue, current_user)
       end
 
-      new_labels = issue.added_labels(old_labels)
-      if new_labels.present?
-        notification_service.relabeled_issue(issue, new_labels, current_user)
+      added_labels = issue.labels - old_labels
+      if added_labels.present?
+        notification_service.relabeled_issue(issue, added_labels, current_user)
       end
     end
 

app/services/merge_requests/update_service.rb

@@ -14,8 +14,8 @@ module MergeRequests
       update(merge_request)
     end
 
-    def handle_changes(issue, old_labels: [], new_labels: [])
-      if has_changes?(merge_request, options)
+    def handle_changes(merge_request, old_labels: [])
+      if has_changes?(merge_request, old_labels: old_labels)
         todo_service.mark_pending_todos_as_done(merge_request, current_user)
       end
 
@@ -45,9 +45,13 @@ module MergeRequests
         merge_request.mark_as_unchecked
       end
 
-      new_labels = merge_request.added_labels(old_labels)
-      if new_labels.present?
-        notification_service.relabeled_merge_request(merge_request, new_labels, current_user)
+      added_labels = merge_request.labels - old_labels
+      if added_labels.present?
+        notification_service.relabeled_merge_request(
+          merge_request,
+          added_labels,
+          current_user
+        )
       end
     end
 

app/services/notification_service.rb

@@ -24,16 +24,17 @@ class NotificationService
     end
   end
 
-  # When create an issue we should send next emails:
+  # When create an issue we should send an email to:
   #
   #  * issue assignee if their notification level is not Disabled
   #  * project team members with notification level higher then Participating
+  #  * watchers of the issue's labels
   #
   def new_issue(issue, current_user)
     new_resource_email(issue, issue.project, 'new_issue_email')
   end
 
-  # When we close an issue we should send next emails:
+  # When we close an issue we should send an email to:
   #
   #  * issue author if their notification level is not Disabled
   #  * issue assignee if their notification level is not Disabled
@@ -43,7 +44,7 @@ class NotificationService
     close_resource_email(issue, issue.project, current_user, 'closed_issue_email')
   end
 
-  # When we reassign an issue we should send next emails:
+  # When we reassign an issue we should send an email to:
   #
   #  * issue old assignee if their notification level is not Disabled
   #  * issue new assignee if their notification level is not Disabled
@@ -52,24 +53,25 @@ class NotificationService
     reassign_resource_email(issue, issue.project, current_user, 'reassigned_issue_email')
   end
 
-  # When we change labels on an issue we should send emails.
+  # When we add labels to an issue we should send an email to:
   #
-  # We pass in the labels, here, because we only want the labels that
-  # have been *added* during this relabel, not all of them.
-  def relabeled_issue(issue, labels, current_user)
-    relabel_resource_email(issue, issue.project, labels, current_user, 'relabeled_issue_email')
+  #  * watchers of the issue's labels
+  #
+  def relabeled_issue(issue, added_labels, current_user)
+    relabeled_resource_email(issue, added_labels, current_user, 'relabeled_issue_email')
   end
 
-
-  # When create a merge request we should send next emails:
+  # When create a merge request we should send an email to:
   #
   #  * mr assignee if their notification level is not Disabled
+  #  * project team members with notification level higher then Participating
+  #  * watchers of the mr's labels
   #
   def new_merge_request(merge_request, current_user)
     new_resource_email(merge_request, merge_request.target_project, 'new_merge_request_email')
   end
 
-  # When we reassign a merge_request we should send next emails:
+  # When we reassign a merge_request we should send an email to:
   #
   #  * merge_request old assignee if their notification level is not Disabled
   #  * merge_request assignee if their notification level is not Disabled
@@ -78,12 +80,12 @@ class NotificationService
     reassign_resource_email(merge_request, merge_request.target_project, current_user, 'reassigned_merge_request_email')
   end
 
-  # When we change labels on a merge request we should send emails.
+  # When we add labels to a merge request we should send an email to:
   #
-  # We pass in the labels, here, because we only want the labels that
-  # have been *added* during this relabel, not all of them.
-  def relabeled_merge_request(merge_request, labels, current_user)
-    relabel_resource_email(merge_request, merge_request.project, labels, current_user, 'relabeled_merge_request_email')
+  #  * watchers of the mr's labels
+  #
+  def relabeled_merge_request(merge_request, added_labels, current_user)
+    relabeled_resource_email(merge_request, added_labels, current_user, 'relabeled_merge_request_email')
   end
 
   def close_mr(merge_request, current_user)
@@ -107,7 +109,8 @@ class NotificationService
     reopen_resource_email(
       merge_request,
       merge_request.target_project,
-      current_user, 'merge_request_status_email',
+      current_user,
+      'merge_request_status_email',
       'reopened'
     )
   end
@@ -158,7 +161,6 @@ class NotificationService
     recipients = reject_muted_users(recipients, note.project)
 
     recipients = add_subscribed_users(recipients, note.noteable)
-    recipients = add_label_subscriptions(recipients, note.noteable)
     recipients = reject_unsubscribed_users(recipients, note.noteable)
 
     recipients.delete(note.author)
@@ -365,29 +367,23 @@ class NotificationService
   end
 
   def add_subscribed_users(recipients, target)
-    return recipients unless target.respond_to? :subscriptions
+    return recipients unless target.respond_to? :subscribers
 
-    subscriptions = target.subscriptions
-
-    if subscriptions.any?
-      recipients + subscriptions.where(subscribed: true).map(&:user)
-    else
-      recipients
-    end
+    recipients + target.subscribers
   end
 
-  def add_label_subscriptions(recipients, target)
+  def add_labels_subscribers(recipients, target, labels: nil)
     return recipients unless target.respond_to? :labels
 
-    target.labels.each do |label|
-      recipients += label.subscriptions.where(subscribed: true).map(&:user)
+    (labels || target.labels).each do |label|
+      recipients += label.subscribers
     end
 
     recipients
   end
 
   def new_resource_email(target, project, method)
-    recipients = build_recipients(target, project, target.author)
+    recipients = build_recipients(target, project, target.author, action: :new)
 
     recipients.each do |recipient|
       mailer.send(method, recipient.id, target.id).deliver_later
@@ -419,12 +415,12 @@ class NotificationService
     end
   end
 
-  def relabel_resource_email(target, project, labels, current_user, method)
-    recipients = build_relabel_recipients(target, project, labels, current_user)
+  def relabeled_resource_email(target, labels, current_user, method)
+    recipients = build_relabeled_recipients(target, current_user, labels: labels)
     label_names = labels.map(&:name)
 
     recipients.each do |recipient|
-      mailer.send(method, recipient.id, target.id, current_user.id, label_names).deliver_later
+      mailer.send(method, recipient.id, target.id, label_names, current_user.id).deliver_later
     end
   end
 
@@ -452,7 +448,11 @@ class NotificationService
 
     recipients = reject_muted_users(recipients, project)
     recipients = add_subscribed_users(recipients, target)
-    recipients = add_label_subscriptions(recipients, target)
+
+    if action == :new
+      recipients = add_labels_subscribers(recipients, target)
+    end
+
     recipients = reject_unsubscribed_users(recipients, target)
 
     recipients.delete(current_user)
@@ -460,8 +460,8 @@ class NotificationService
     recipients.uniq
   end
 
-  def build_relabel_recipients(target, project, labels, current_user)
-    recipients = add_label_subscriptions([], target)
+  def build_relabeled_recipients(target, current_user, labels:)
+    recipients = add_labels_subscribers([], target, labels: labels)
     recipients = reject_unsubscribed_users(recipients, target)
     recipients.delete(current_user)
     recipients.uniq

app/views/layouts/notify.html.haml

@@ -42,12 +42,15 @@
           - else
             #{link_to "View it on GitLab", @target_url}.
           %br
-          -# Don't link the host is the line below, one link in the email is easier to quickly click than two.
+          -# Don't link the host in the line below, one link in the email is easier to quickly click than two.
           You're receiving this email because of your account on #{Gitlab.config.gitlab.host}.
           If you'd like to receive fewer emails, you can
-          - if @sent_notification && @sent_notification.unsubscribable?
-            = link_to "unsubscribe", unsubscribe_sent_notification_url(@sent_notification)
-            from this thread or
-          adjust your notification settings.
+          - if @labels_url
+            adjust your #{link_to 'label subscriptions', @labels_url}.
+          - else
+            - if @sent_notification && @sent_notification.unsubscribable?
+              = link_to "unsubscribe", unsubscribe_sent_notification_url(@sent_notification)
+              from this thread or
+            adjust your notification settings.
 
           = email_action @target_url

app/views/notify/_reassigned_issuable_email.text.erb

 Reassigned <%= issuable.class.model_name.human.titleize %> <%= issuable.iid %>
 
-<%= url_for([issuable.project.namespace.becomes(Namespace), issuable.project, issuable, {only_path: false}]) %>
+<%= url_for([issuable.project.namespace.becomes(Namespace), issuable.project, issuable, { only_path: false }]) %>
 
 Assignee changed <%= "from #{@previous_assignee.name}" if @previous_assignee -%>
  to <%= "#{issuable.assignee_id ? issuable.assignee_name : 'Unassigned'}" %>

app/views/notify/_relabeled_issuable_email.html.haml

 %p
-  #{@updated_by.name} added the
+  #{'Label'.pluralize(@label_names.size)} added:
   %em= @label_names.to_sentence
-  #{"label".pluralize(@label_names.count)} to #{issuable.class.model_name.human} #{issuable.iid}.
-

app/views/notify/_relabeled_issuable_email.text.erb

-<%= issuable.class.model_name.human.titleize %> <%= issuable.iid %> was relabeled.
+<%= 'Label'.pluralize(@label_names.size) %> added: <%= @label_names.to_sentence %>
 
-Issue <%= issuable.iid %>: <%= url_for(namespace_project_issue_url(issuable.project.namespace, issuable.project, issuable)) %>
-Author: <%= issuable.author_name %>
-New Labels: <%= @label_names.to_sentence %>
+<%= url_for([issuable.project.namespace.becomes(Namespace), issuable.project, issuable, { only_path: false }]) %>

app/views/notify/relabeled_issue_email.html.haml

-= render "relabeled_issuable_email", issuable: @issue
+= render 'relabeled_issuable_email', issuable: @issue

app/views/notify/relabeled_issue_email.text.erb

-<%= render "relabeled_issuable_email", issuable: @issue %>
+<%= render 'relabeled_issuable_email', issuable: @issue %>

app/views/notify/relabeled_merge_request_email.html.haml

-= render "relabeled_issuable_email", issuable: @merge_request
+= render 'relabeled_issuable_email', issuable: @merge_request

app/views/notify/relabeled_merge_request_email.text.erb

-<%= render "relabeled_issuable_email", issuable: @merge_request %>
+<%= render 'relabeled_issuable_email', issuable: @merge_request %>

app/views/projects/labels/_label.html.haml

@@ -11,17 +11,15 @@
         = pluralize label.open_issues_count, 'open issue'
 
     - if current_user
-      %div{class: "label-subscription", data: {id: label.id}}
-        - subscribed = label.subscribed?(current_user)
-        - subscription_status = subscribed ? 'subscribed' : 'unsubscribed'
-        .subscription-status{data: {status: subscription_status}}
-        %button.btn.btn-sm.btn-info.subscribe-button{:type => 'button'}
-          %span= subscribed ? 'Unsubscribe' : 'Subscribe'
+      .label-subscription{data: {url: toggle_subscription_namespace_project_label_path(@project.namespace, @project, label)}}
+        .subscription-status{data: {status: label_subscription_status(label)}}
+        %button.btn.btn-sm.btn-info.subscribe-button
+          %span= label_subscription_toggle_button_text(label)
 
     - if can? current_user, :admin_label, @project
       = link_to 'Edit', edit_namespace_project_label_path(@project.namespace, @project, label), class: 'btn btn-sm'
       = link_to 'Delete', namespace_project_label_path(@project.namespace, @project, label), class: 'btn btn-sm btn-remove remove-row', method: :delete, remote: true, data: {confirm: "Remove this label? Are you sure?"}
 
-:javascript
-  new Subscription("#{toggle_subscription_namespace_project_label_path(@project.namespace, @project, label)}",
-                   ".label-subscription[data-id='#{label.id}']");
+- if current_user
+  :javascript
+    new Subscription('##{dom_id(label)} .label-subscription');

app/views/shared/issuable/_sidebar.html.haml

@@ -98,7 +98,7 @@
       %hr
       - if current_user
         - subscribed = issuable.subscribed?(current_user)
-        .block.light.subscription
+        .block.light.subscription{data: {url: toggle_subscription_path(issuable)}}
           .sidebar-collapsed-icon
             = icon('rss')
           .title.hide-collapsed
@@ -124,5 +124,5 @@
           = clipboard_button(clipboard_text: project_ref)
 
     :javascript
-      new Subscription("#{toggle_subscription_path(issuable)}", ".subscription");
+      new Subscription('.subscription');
       new IssuableContext();

features/project/labels.feature

@@ -3,14 +3,13 @@ Feature: Labels
   Background:
     Given I sign in as a user
     And I own project "Shop"
-    And I visit project "Shop" issues page
     And project "Shop" has labels: "bug", "feature", "enhancement"
+    When I visit project "Shop" labels page
 
   @javascript
   Scenario: I can subscribe to a label
-    When I visit project "Shop" labels page
-    Then I should see that I am unsubscribed
-    When I click button "Subscribe"
-    Then I should see that I am subscribed
-    When I click button "Unsubscribe"
-    Then I should see that I am unsubscribed
+    Then I should see that I am not subscribed to the "bug" label
+    When I click button "Subscribe" for the "bug" label
+    Then I should see that I am subscribed to the "bug" label
+    When I click button "Unsubscribe" for the "bug" label
+    Then I should see that I am not subscribed to the "bug" label

features/steps/project/labels.rb

@@ -10,19 +10,19 @@ class Spinach::Features::Labels < Spinach::FeatureSteps
     visit namespace_project_labels_path(project.namespace, project)
   end
 
-  step 'I should see that I am subscribed' do
+  step 'I should see that I am subscribed to the "bug" label' do
     expect(subscribe_button).to have_content 'Unsubscribe'
   end
 
-  step 'I should see that I am unsubscribed' do
+  step 'I should see that I am not subscribed to the "bug" label' do
     expect(subscribe_button).to have_content 'Subscribe'
   end
 
-  step 'I click button "Unsubscribe"' do
+  step 'I click button "Unsubscribe" for the "bug" label' do
     subscribe_button.click
   end
 
-  step 'I click button "Subscribe"' do
+  step 'I click button "Subscribe" for the "bug" label' do
     subscribe_button.click
   end
 

spec/factories/labels.rb

@@ -13,7 +13,7 @@
 
 FactoryGirl.define do
   factory :label do
-    title { FFaker::Color.name }
+    sequence(:title) { |n| "label#{n}" }
     color "#990000"
     project
   end

spec/mailers/notify_spec.rb

@@ -100,6 +100,34 @@ describe Notify do
           end
         end
 
+        describe 'that have been relabeled' do
+          subject { Notify.relabeled_issue_email(recipient.id, issue.id, %w[foo bar baz], current_user.id) }
+
+          it_behaves_like 'a multiple recipients email'
+          it_behaves_like 'an answer to an existing thread', 'issue'
+          it_behaves_like 'it should show Gmail Actions View Issue link'
+          it_behaves_like 'a user cannot unsubscribe through footer link'
+          it_behaves_like 'an email with a labels subscriptions link in its footer'
+
+          it 'is sent as the author' do
+            sender = subject.header[:from].addrs[0]
+            expect(sender.display_name).to eq(current_user.name)
+            expect(sender.address).to eq(gitlab_sender)
+          end
+
+          it 'has the correct subject' do
+            is_expected.to have_subject /#{issue.title} \(##{issue.iid}\)/
+          end
+
+          it 'contains the names of the added labels' do
+            is_expected.to have_body_text /foo, bar, and baz/
+          end
+
+          it 'contains a link to the issue' do
+            is_expected.to have_body_text /#{namespace_project_issue_path project.namespace, project, issue}/
+          end
+        end
+
         describe 'status changed' do
           let(:status) { 'closed' }
           subject { Notify.issue_status_changed_email(recipient.id, issue.id, status, current_user.id) }
@@ -219,6 +247,34 @@ describe Notify do
           end
         end
 
+        describe 'that have been relabeled' do
+          subject { Notify.relabeled_merge_request_email(recipient.id, merge_request.id, %w[foo bar baz], current_user.id) }
+
+          it_behaves_like 'a multiple recipients email'
+          it_behaves_like 'an answer to an existing thread', 'merge_request'
+          it_behaves_like 'it should show Gmail Actions View Merge request link'
+          it_behaves_like 'a user cannot unsubscribe through footer link'
+          it_behaves_like 'an email with a labels subscriptions link in its footer'
+
+          it 'is sent as the author' do
+            sender = subject.header[:from].addrs[0]
+            expect(sender.display_name).to eq(current_user.name)
+            expect(sender.address).to eq(gitlab_sender)
+          end
+
+          it 'has the correct subject' do
+            is_expected.to have_subject /#{merge_request.title} \(##{merge_request.iid}\)/
+          end
+
+          it 'contains the names of the added labels' do
+            is_expected.to have_body_text /foo, bar, and baz/
+          end
+
+          it 'contains a link to the merge request' do
+            is_expected.to have_body_text /#{namespace_project_merge_request_path project.namespace, project, merge_request}/
+          end
+        end
+
         describe 'status changed' do
           let(:status) { 'reopened' }
           subject { Notify.merge_request_status_email(recipient.id, merge_request.id, status, current_user.id) }

spec/mailers/shared/notify.rb

@@ -112,6 +112,10 @@ shared_examples 'an unsubscribeable thread' do
   it { is_expected.to have_body_text /unsubscribe/ }
 end
 
-shared_examples "a user cannot unsubscribe through footer link" do
+shared_examples 'a user cannot unsubscribe through footer link' do
   it { is_expected.not_to have_body_text /unsubscribe/ }
 end
+
+shared_examples 'an email with a labels subscriptions link in its footer' do
+  it { is_expected.to have_body_text /label subscriptions/ }
+end

spec/models/concerns/issuable_spec.rb

@@ -113,6 +113,48 @@ describe Issue, "Issuable" do
     end
   end
 
+  describe '#subscribed?' do
+    context 'user is not a participant in the issue' do
+      before { allow(issue).to receive(:participants).with(user).and_return([]) }
+
+      it 'returns false when no subcription exists' do
+        expect(issue.subscribed?(user)).to be_falsey
+      end
+
+      it 'returns true when a subcription exists and subscribed is true' do
+        issue.subscriptions.create(user: user, subscribed: true)
+
+        expect(issue.subscribed?(user)).to be_truthy
+      end
+
+      it 'returns false when a subcription exists and subscribed is false' do
+        issue.subscriptions.create(user: user, subscribed: false)
+
+        expect(issue.subscribed?(user)).to be_falsey
+      end
+    end
+
+    context 'user is a participant in the issue' do
+      before { allow(issue).to receive(:participants).with(user).and_return([user]) }
+
+      it 'returns false when no subcription exists' do
+        expect(issue.subscribed?(user)).to be_truthy
+      end
+
+      it 'returns true when a subcription exists and subscribed is true' do
+        issue.subscriptions.create(user: user, subscribed: true)
+
+        expect(issue.subscribed?(user)).to be_truthy
+      end
+
+      it 'returns false when a subcription exists and subscribed is false' do
+        issue.subscriptions.create(user: user, subscribed: false)
+
+        expect(issue.subscribed?(user)).to be_falsey
+      end
+    end
+  end
+
   describe "#to_hook_data" do
     let(:data) { issue.to_hook_data(user) }
     let(:project) { issue.project }

spec/models/concerns/subscribable_spec.rb

-require "spec_helper"
+require 'spec_helper'
 
-describe Subscribable, "Subscribable" do
+describe Subscribable, 'Subscribable' do
   let(:resource) { create(:issue) }
   let(:user) { create(:user) }
 
-  describe "#subscribed?" do
-    it do
+  describe '#subscribed?' do
+    it 'returns false when no subcription exists' do
       expect(resource.subscribed?(user)).to be_falsey
-      resource.toggle_subscription(user)
+    end
+
+    it 'returns true when a subcription exists and subscribed is true' do
+      resource.subscriptions.create(user: user, subscribed: true)
+
       expect(resource.subscribed?(user)).to be_truthy
+    end
+
+    it 'returns false when a subcription exists and subscribed is false' do
+      resource.subscriptions.create(user: user, subscribed: false)
+
+      expect(resource.subscribed?(user)).to be_falsey
+    end
+  end
+  describe '#subscribers' do
+    it 'returns [] when no subcribers exists' do
+      expect(resource.subscribers).to be_empty
+    end
+
+    it 'returns the subscribed users' do
+      resource.subscriptions.create(user: user, subscribed: true)
+      resource.subscriptions.create(user: create(:user), subscribed: false)
+
+      expect(resource.subscribers).to eq [user]
+    end
+  end
+
+  describe '#toggle_subscription' do
+    it 'toggles the current subscription state for the given user' do
+      expect(resource.subscribed?(user)).to be_falsey
+
       resource.toggle_subscription(user)
+
+      expect(resource.subscribed?(user)).to be_truthy
+    end
+  end
+
+  describe '#unsubscribe' do
+    it 'unsubscribes the given current user' do
+      resource.subscriptions.create(user: user, subscribed: true)
+      expect(resource.subscribed?(user)).to be_truthy
+
+      resource.unsubscribe(user)
+
       expect(resource.subscribed?(user)).to be_falsey
     end
   end

spec/services/issues/update_service_spec.rb

@@ -6,6 +6,7 @@ describe Issues::UpdateService, services: true do
   let(:user3) { create(:user) }
   let(:issue) { create(:issue, title: 'Old title', assignee_id: user3.id) }
   let(:label) { create(:label) }
+  let(:label2) { create(:label) }
   let(:project) { issue.project }
 
   before do
@@ -148,57 +149,45 @@ describe Issues::UpdateService, services: true do
       end
     end
 
-    context "when the issue is relabeled" do
-      it "sends notifications for subscribers of newly added labels" do
-        subscriber, non_subscriber = create_list(:user, 2)
-        label.toggle_subscription(subscriber)
-        2.times { label.toggle_subscription(non_subscriber) }
+    context 'when the issue is relabeled' do
+      let!(:non_subscriber) { create(:user) }
+      let!(:subscriber) { create(:user).tap { |u| label.toggle_subscription(u) } }
 
+      it 'sends notifications for subscribers of newly added labels' do
         opts = { label_ids: [label.id] }
 
         perform_enqueued_jobs do
           @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
         end
 
-        @issue.reload
         should_email(subscriber)
         should_not_email(non_subscriber)
       end
 
-      it "does send notifications for existing labels" do
-        second_label = create(:label)
-        issue.labels << label
-        subscriber, non_subscriber = create_list(:user, 2)
-        label.toggle_subscription(subscriber)
-        2.times { label.toggle_subscription(non_subscriber) }
+      context 'when issue has the `label` label' do
+        before { issue.labels << label }
 
-        opts = { label_ids: [label.id, second_label.id] }
+        it 'does not send notifications for existing labels' do
+          opts = { label_ids: [label.id, label2.id] }
 
-        perform_enqueued_jobs do
-          @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
-        end
+          perform_enqueued_jobs do
+            @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
+          end
 
-        @issue.reload
-        should_email(subscriber)
-        should_not_email(non_subscriber)
-      end
+          should_not_email(subscriber)
+          should_not_email(non_subscriber)
+        end
 
-      it "does not send notifications for removed labels" do
-        second_label = create(:label)
-        issue.labels << label
-        subscriber, non_subscriber = create_list(:user, 2)
-        label.toggle_subscription(subscriber)
-        2.times { label.toggle_subscription(non_subscriber) }
+        it 'does not send notifications for removed labels' do
+          opts = { label_ids: [label2.id] }
 
-        opts = { label_ids: [second_label.id] }
+          perform_enqueued_jobs do
+            @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
+          end
 
-        perform_enqueued_jobs do
-          @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
+          should_not_email(subscriber)
+          should_not_email(non_subscriber)
         end
-
-        @issue.reload
-        should_not_email(subscriber)
-        should_not_email(non_subscriber)
       end
     end
 

spec/services/merge_requests/update_service_spec.rb

@@ -7,6 +7,7 @@ describe MergeRequests::UpdateService, services: true do
   let(:merge_request) { create(:merge_request, :simple, title: 'Old title', assignee_id: user3.id) }
   let(:project) { merge_request.project }
   let(:label) { create(:label) }
+  let(:label2) { create(:label) }
 
   before do
     project.team << [user, :master]
@@ -176,57 +177,45 @@ describe MergeRequests::UpdateService, services: true do
       end
     end
 
-    context "when the merge request is relabeled" do
-      it "sends notifications for subscribers of newly added labels" do
-        subscriber, non_subscriber = create_list(:user, 2)
-        label.toggle_subscription(subscriber)
-        2.times { label.toggle_subscription(non_subscriber) }
+    context 'when the issue is relabeled' do
+      let!(:non_subscriber) { create(:user) }
+      let!(:subscriber) { create(:user).tap { |u| label.toggle_subscription(u) } }
 
+      it 'sends notifications for subscribers of newly added labels' do
         opts = { label_ids: [label.id] }
 
         perform_enqueued_jobs do
           @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request)
         end
 
-        @merge_request.reload
         should_email(subscriber)
         should_not_email(non_subscriber)
       end
 
-      it "does send notifications for existing labels" do
-        second_label = create(:label)
-        merge_request.labels << label
-        subscriber, non_subscriber = create_list(:user, 2)
-        label.toggle_subscription(subscriber)
-        2.times { label.toggle_subscription(non_subscriber) }
+      context 'when issue has the `label` label' do
+        before { merge_request.labels << label }
 
-        opts = { label_ids: [label.id, second_label.id] }
+        it 'does not send notifications for existing labels' do
+          opts = { label_ids: [label.id, label2.id] }
 
-        perform_enqueued_jobs do
-          @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request)
-        end
+          perform_enqueued_jobs do
+            @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request)
+          end
 
-        @merge_request.reload
-        should_email(subscriber)
-        should_not_email(non_subscriber)
-      end
+          should_not_email(subscriber)
+          should_not_email(non_subscriber)
+        end
 
-      it "does not send notifications for removed labels" do
-        second_label = create(:label)
-        merge_request.labels << label
-        subscriber, non_subscriber = create_list(:user, 2)
-        label.toggle_subscription(subscriber)
-        2.times { label.toggle_subscription(non_subscriber) }
+        it 'does not send notifications for removed labels' do
+          opts = { label_ids: [label2.id] }
 
-        opts = { label_ids: [second_label.id] }
+          perform_enqueued_jobs do
+            @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request)
+          end
 
-        perform_enqueued_jobs do
-          @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request)
+          should_not_email(subscriber)
+          should_not_email(non_subscriber)
         end
-
-        @merge_request.reload
-        should_not_email(subscriber)
-        should_not_email(non_subscriber)
       end
     end
 

spec/services/notification_service_spec.rb

@@ -225,14 +225,13 @@ describe NotificationService, services: true do
         should_not_email(issue.assignee)
       end
 
-      it "should email subscribers of the issue's labels" do
-        subscriber, non_subscriber = create_list(:user, 2)
+      it "emails subscribers of the issue's labels" do
+        subscriber = create(:user)
         label = create(:label, issues: [issue])
         label.toggle_subscription(subscriber)
-        2.times { label.toggle_subscription(non_subscriber) }
         notification.new_issue(issue, @u_disabled)
+
         should_email(subscriber)
-        should_not_email(non_subscriber)
       end
     end
 
@@ -306,6 +305,35 @@ describe NotificationService, services: true do
       end
     end
 
+    describe '#relabeled_issue' do
+      let(:label) { create(:label, issues: [issue]) }
+      let(:label2) { create(:label) }
+      let!(:subscriber_to_label) { create(:user).tap { |u| label.toggle_subscription(u) } }
+      let!(:subscriber_to_label2) { create(:user).tap { |u| label2.toggle_subscription(u) } }
+
+      it "emails subscribers of the issue's added labels only" do
+        notification.relabeled_issue(issue, [label2], @u_disabled)
+
+        should_not_email(subscriber_to_label)
+        should_email(subscriber_to_label2)
+      end
+
+      it "doesn't send email to anyone but subscribers of the given labels" do
+        notification.relabeled_issue(issue, [label2], @u_disabled)
+
+        should_not_email(issue.assignee)
+        should_not_email(issue.author)
+        should_not_email(@u_watcher)
+        should_not_email(@u_participant_mentioned)
+        should_not_email(@subscriber)
+        should_not_email(@watcher_and_subscriber)
+        should_not_email(@unsubscriber)
+        should_not_email(@u_participating)
+        should_not_email(subscriber_to_label)
+        should_email(subscriber_to_label2)
+      end
+    end
+
     describe :close_issue do
       it 'should sent email to issue assignee and issue author' do
         notification.close_issue(issue, @u_disabled)
@@ -336,32 +364,6 @@ describe NotificationService, services: true do
         should_not_email(@u_participating)
       end
     end
-
-    describe :relabel_issue do
-      it "sends email to subscribers of the given labels" do
-        subscriber, non_subscriber = create_list(:user, 2)
-        label = create(:label, issues: [issue])
-        label.toggle_subscription(subscriber)
-        2.times { label.toggle_subscription(non_subscriber) }
-        notification.relabeled_issue(issue, [label], @u_disabled)
-        should_email(subscriber)
-        should_not_email(non_subscriber)
-      end
-
-      it "doesn't send email to anyone but subscribers of the given labels" do
-        label = create(:label, issues: [issue])
-        notification.relabeled_issue(issue, [label], @u_disabled)
-
-        should_not_email(issue.assignee)
-        should_not_email(issue.author)
-        should_not_email(@u_watcher)
-        should_not_email(@u_participant_mentioned)
-        should_not_email(@subscriber)
-        should_not_email(@watcher_and_subscriber)
-        should_not_email(@unsubscriber)
-        should_not_email(@u_participating)
-      end
-    end
   end
 
   describe 'Merge Requests' do
@@ -386,15 +388,13 @@ describe NotificationService, services: true do
         should_not_email(@u_disabled)
       end
 
-      it "should email subscribers of the MR's labels" do
-        subscriber, non_subscriber = create_list(:user, 2)
-        label = create(:label)
-        merge_request.labels << label
+      it "emails subscribers of the merge request's labels" do
+        subscriber = create(:user)
+        label = create(:label, merge_requests: [merge_request])
         label.toggle_subscription(subscriber)
-        2.times { label.toggle_subscription(non_subscriber) }
         notification.new_merge_request(merge_request, @u_disabled)
+
         should_email(subscriber)
-        should_not_email(non_subscriber)
       end
     end
 
@@ -413,6 +413,35 @@ describe NotificationService, services: true do
       end
     end
 
+    describe :relabel_merge_request do
+      let(:label) { create(:label, merge_requests: [merge_request]) }
+      let(:label2) { create(:label) }
+      let!(:subscriber_to_label) { create(:user).tap { |u| label.toggle_subscription(u) } }
+      let!(:subscriber_to_label2) { create(:user).tap { |u| label2.toggle_subscription(u) } }
+
+      it "emails subscribers of the merge request's added labels only" do
+        notification.relabeled_merge_request(merge_request, [label2], @u_disabled)
+
+        should_not_email(subscriber_to_label)
+        should_email(subscriber_to_label2)
+      end
+
+      it "doesn't send email to anyone but subscribers of the given labels" do
+        notification.relabeled_merge_request(merge_request, [label2], @u_disabled)
+
+        should_not_email(merge_request.assignee)
+        should_not_email(merge_request.author)
+        should_not_email(@u_watcher)
+        should_not_email(@u_participant_mentioned)
+        should_not_email(@subscriber)
+        should_not_email(@watcher_and_subscriber)
+        should_not_email(@unsubscriber)
+        should_not_email(@u_participating)
+        should_not_email(subscriber_to_label)
+        should_email(subscriber_to_label2)
+      end
+    end
+
     describe :closed_merge_request do
       it do
         notification.close_mr(merge_request, @u_disabled)
@@ -457,34 +486,6 @@ describe NotificationService, services: true do
         should_not_email(@u_disabled)
       end
     end
-
-    describe :relabel_merge_request do
-      it "sends email to subscribers of the given labels" do
-        subscriber, non_subscriber = create_list(:user, 2)
-        label = create(:label)
-        merge_request.labels << label
-        label.toggle_subscription(subscriber)
-        2.times { label.toggle_subscription(non_subscriber) }
-        notification.relabeled_merge_request(merge_request, [label], @u_disabled)
-        should_email(subscriber)
-        should_not_email(non_subscriber)
-      end
-
-      it "doesn't send email to anyone but subscribers of the given labels" do
-        label = create(:label)
-        merge_request.labels << label
-        notification.relabeled_merge_request(merge_request, [label], @u_disabled)
-
-        should_not_email(merge_request.assignee)
-        should_not_email(merge_request.author)
-        should_not_email(@u_watcher)
-        should_not_email(@u_participant_mentioned)
-        should_not_email(@subscriber)
-        should_not_email(@watcher_and_subscriber)
-        should_not_email(@unsubscriber)
-        should_not_email(@u_participating)
-      end
-    end
   end
 
   describe 'Projects' do