summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRémy Coutable <remy@rymai.me>2016-03-01 17:33:13 +0100
committerRémy Coutable <remy@rymai.me>2016-03-10 18:05:19 +0100
commit2e2bf4ed4ba76e7b1eb719995c169b0b7b6c0417 (patch)
tree296b156c1f2af89003556be43275ec05e96079a4
parenta9f5aa6490979dfadf28ff593774e89c2bec8f0b (diff)
downloadgitlab-ce-2e2bf4ed4ba76e7b1eb719995c169b0b7b6c0417.tar.gz
Improving the original label-subscribing implementation12743-subscribe-to-label
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
-rw-r--r--CHANGELOG3
-rw-r--r--app/assets/javascripts/subscription.js.coffee28
-rw-r--r--app/assets/stylesheets/pages/labels.scss4
-rw-r--r--app/controllers/projects/issues_controller.rb2
-rw-r--r--app/controllers/projects/labels_controller.rb2
-rw-r--r--app/controllers/projects/merge_requests_controller.rb2
-rw-r--r--app/helpers/labels_helper.rb8
-rw-r--r--app/mailers/emails/issues.rb25
-rw-r--r--app/mailers/emails/merge_requests.rb55
-rw-r--r--app/models/concerns/issuable.rb10
-rw-r--r--app/models/concerns/subscribable.rb25
-rw-r--r--app/models/subscription.rb2
-rw-r--r--app/services/issuable_base_service.rb17
-rw-r--r--app/services/issues/update_service.rb10
-rw-r--r--app/services/merge_requests/update_service.rb14
-rw-r--r--app/services/notification_service.rb72
-rw-r--r--app/views/layouts/notify.html.haml13
-rw-r--r--app/views/notify/_reassigned_issuable_email.text.erb2
-rw-r--r--app/views/notify/_relabeled_issuable_email.html.haml4
-rw-r--r--app/views/notify/_relabeled_issuable_email.text.erb6
-rw-r--r--app/views/notify/relabeled_issue_email.html.haml2
-rw-r--r--app/views/notify/relabeled_issue_email.text.erb2
-rw-r--r--app/views/notify/relabeled_merge_request_email.html.haml2
-rw-r--r--app/views/notify/relabeled_merge_request_email.text.erb2
-rw-r--r--app/views/projects/labels/_label.html.haml16
-rw-r--r--app/views/shared/issuable/_sidebar.html.haml4
-rw-r--r--features/project/labels.feature13
-rw-r--r--features/steps/project/labels.rb8
-rw-r--r--spec/factories/labels.rb2
-rw-r--r--spec/mailers/notify_spec.rb56
-rw-r--r--spec/mailers/shared/notify.rb6
-rw-r--r--spec/models/concerns/issuable_spec.rb42
-rw-r--r--spec/models/concerns/subscribable_spec.rb51
-rw-r--r--spec/services/issues/update_service_spec.rb55
-rw-r--r--spec/services/merge_requests/update_service_spec.rb55
-rw-r--r--spec/services/notification_service_spec.rb129
36 files changed, 439 insertions, 310 deletions
diff --git a/CHANGELOG b/CHANGELOG
index bcb0117c8c3..3ae61f29840 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -29,6 +29,7 @@ v 8.6.0 (unreleased)
v 8.5.5
- Ensure removing a project removes associated Todo entries.
- Prevent a 500 error in Todos when author was removed.
+ - Allow user subscription to a label: get notified for issues/merge requests related to that label (Timothy Andrew)
v 8.5.4
- Do not cache requests for badges (including builds badge)
@@ -157,8 +158,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
diff --git a/app/assets/javascripts/subscription.js.coffee b/app/assets/javascripts/subscription.js.coffee
index afc17338b26..084f0e0dc65 100644
--- a/app/assets/javascripts/subscription.js.coffee
+++ b/app/assets/javascripts/subscription.js.coffee
@@ -1,19 +1,21 @@
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')
diff --git a/app/assets/stylesheets/pages/labels.scss b/app/assets/stylesheets/pages/labels.scss
index 1791ba9fbd8..61ee34b695e 100644
--- a/app/assets/stylesheets/pages/labels.scss
+++ b/app/assets/stylesheets/pages/labels.scss
@@ -43,5 +43,5 @@
}
.label-subscription {
- display: inline-block;
-} \ No newline at end of file
+ display: inline-block;
+}
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 67faa1e4437..24a862814b3 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/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
diff --git a/app/controllers/projects/labels_controller.rb b/app/controllers/projects/labels_controller.rb
index d0334c37b67..e4dea6b065a 100644
--- a/app/controllers/projects/labels_controller.rb
+++ b/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
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 03ba289eb94..954ee55a211 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/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
diff --git a/app/helpers/labels_helper.rb b/app/helpers/labels_helper.rb
index 89a054289e8..4455dcd0e20 100644
--- a/app/helpers/labels_helper.rb
+++ b/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
diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb
index 2838baa1b4e..160b6df0b97 100644
--- a/app/mailers/emails/issues.rb
+++ b/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
diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb
index 680ce975c79..334bad4e2f8 100644
--- a/app/mailers/emails/merge_requests.rb
+++ b/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
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index e3fd81fb13b..cbc0cbb446f 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -132,6 +132,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,
@@ -162,12 +166,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:
diff --git a/app/models/concerns/subscribable.rb b/app/models/concerns/subscribable.rb
index cab9241ac3d..d5a881b2445 100644
--- a/app/models/concerns/subscribable.rb
+++ b/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)
diff --git a/app/models/subscription.rb b/app/models/subscription.rb
index dd75d3ab8ba..dd800ce110f 100644
--- a/app/models/subscription.rb
+++ b/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
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 971c074882e..18f76d3f650 100644
--- a/app/services/issuable_base_service.rb
+++ b/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
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index b2e63a4e1af..3563cbaa997 100644
--- a/app/services/issues/update_service.rb
+++ b/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
diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb
index 6fd569dc302..477c64e7377 100644
--- a/app/services/merge_requests/update_service.rb
+++ b/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
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index e9955cd3e2d..19a6779dea9 100644
--- a/app/services/notification_service.rb
+++ b/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
diff --git a/app/views/layouts/notify.html.haml b/app/views/layouts/notify.html.haml
index 325c68c69dc..37b4d562966 100644
--- a/app/views/layouts/notify.html.haml
+++ b/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
diff --git a/app/views/notify/_reassigned_issuable_email.text.erb b/app/views/notify/_reassigned_issuable_email.text.erb
index 855d37429d9..daf20a226dd 100644
--- a/app/views/notify/_reassigned_issuable_email.text.erb
+++ b/app/views/notify/_reassigned_issuable_email.text.erb
@@ -1,6 +1,6 @@
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'}" %>
diff --git a/app/views/notify/_relabeled_issuable_email.html.haml b/app/views/notify/_relabeled_issuable_email.html.haml
index a41ff07c306..80a0de255be 100644
--- a/app/views/notify/_relabeled_issuable_email.html.haml
+++ b/app/views/notify/_relabeled_issuable_email.html.haml
@@ -1,5 +1,3 @@
%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}.
-
diff --git a/app/views/notify/_relabeled_issuable_email.text.erb b/app/views/notify/_relabeled_issuable_email.text.erb
index 1a28d7fd352..6a83d79fd61 100644
--- a/app/views/notify/_relabeled_issuable_email.text.erb
+++ b/app/views/notify/_relabeled_issuable_email.text.erb
@@ -1,5 +1,3 @@
-<%= 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 }]) %>
diff --git a/app/views/notify/relabeled_issue_email.html.haml b/app/views/notify/relabeled_issue_email.html.haml
index a3e094e86eb..b17b16e1814 100644
--- a/app/views/notify/relabeled_issue_email.html.haml
+++ b/app/views/notify/relabeled_issue_email.html.haml
@@ -1 +1 @@
-= render "relabeled_issuable_email", issuable: @issue
+= render 'relabeled_issuable_email', issuable: @issue
diff --git a/app/views/notify/relabeled_issue_email.text.erb b/app/views/notify/relabeled_issue_email.text.erb
index de7f04a323a..eeced97f601 100644
--- a/app/views/notify/relabeled_issue_email.text.erb
+++ b/app/views/notify/relabeled_issue_email.text.erb
@@ -1 +1 @@
-<%= render "relabeled_issuable_email", issuable: @issue %>
+<%= render 'relabeled_issuable_email', issuable: @issue %>
diff --git a/app/views/notify/relabeled_merge_request_email.html.haml b/app/views/notify/relabeled_merge_request_email.html.haml
index 3937b449a37..9eaa9afa5b1 100644
--- a/app/views/notify/relabeled_merge_request_email.html.haml
+++ b/app/views/notify/relabeled_merge_request_email.html.haml
@@ -1 +1 @@
-= render "relabeled_issuable_email", issuable: @merge_request
+= render 'relabeled_issuable_email', issuable: @merge_request
diff --git a/app/views/notify/relabeled_merge_request_email.text.erb b/app/views/notify/relabeled_merge_request_email.text.erb
index 5c4d86e4dc2..87bc80ead32 100644
--- a/app/views/notify/relabeled_merge_request_email.text.erb
+++ b/app/views/notify/relabeled_merge_request_email.text.erb
@@ -1 +1 @@
-<%= render "relabeled_issuable_email", issuable: @merge_request %>
+<%= render 'relabeled_issuable_email', issuable: @merge_request %>
diff --git a/app/views/projects/labels/_label.html.haml b/app/views/projects/labels/_label.html.haml
index 3b14a925c46..4927d239c1e 100644
--- a/app/views/projects/labels/_label.html.haml
+++ b/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');
diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml
index f66e7eecdc7..07795600ecb 100644
--- a/app/views/shared/issuable/_sidebar.html.haml
+++ b/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();
diff --git a/features/project/labels.feature b/features/project/labels.feature
index cd3f3a789f0..955bc3d8b1b 100644
--- a/features/project/labels.feature
+++ b/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
diff --git a/features/steps/project/labels.rb b/features/steps/project/labels.rb
index 3f800a10594..17944527e3a 100644
--- a/features/steps/project/labels.rb
+++ b/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
diff --git a/spec/factories/labels.rb b/spec/factories/labels.rb
index 91a7400afa1..ea2be8928d5 100644
--- a/spec/factories/labels.rb
+++ b/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
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index 232a11245a6..f910424d85b 100644
--- a/spec/mailers/notify_spec.rb
+++ b/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) }
diff --git a/spec/mailers/shared/notify.rb b/spec/mailers/shared/notify.rb
index 48c851ebbd6..6019af544d3 100644
--- a/spec/mailers/shared/notify.rb
+++ b/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
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 600089802b2..782bd874a3a 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -68,6 +68,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 }
diff --git a/spec/models/concerns/subscribable_spec.rb b/spec/models/concerns/subscribable_spec.rb
index 9ee60426a5d..e31fdb0bffb 100644
--- a/spec/models/concerns/subscribable_spec.rb
+++ b/spec/models/concerns/subscribable_spec.rb
@@ -1,15 +1,56 @@
-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
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index dc9d8329751..4ffe753fef5 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/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
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index 104e63ccfee..cb8cff2fa8c 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/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
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 35afa768057..b5407397c1d 100644
--- a/spec/services/notification_service_spec.rb
+++ b/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