diff options
36 files changed, 439 insertions, 310 deletions
diff --git a/CHANGELOG b/CHANGELOG index fb9500e68a0..e9e7c88d860 100644 --- a/CHANGELOG +++ b/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 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 affc4a842a7..86ab84615ba 100644 --- a/app/models/concerns/issuable.rb +++ b/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: 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 b44086d4205..23b1ed1e51b 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 aff384c2949..be29b6d66ff 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/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 } 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 |