diff --git a/CHANGELOG b/CHANGELOG index 015efa05c6a..fb9500e68a0 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -174,6 +174,8 @@ 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 7f41616d4e7..afc17338b26 100644 --- a/app/assets/javascripts/subscription.js.coffee +++ b/app/assets/javascripts/subscription.js.coffee @@ -1,17 +1,19 @@ class @Subscription - constructor: (url) -> - $(".subscribe-button").unbind("click").click (event)=> - btn = $(event.currentTarget) - 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>div").toggleClass("hidden") + constructor: (@url, container) -> + @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) + + $.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") diff --git a/app/assets/stylesheets/pages/labels.scss b/app/assets/stylesheets/pages/labels.scss index 5ec0966194c..1791ba9fbd8 100644 --- a/app/assets/stylesheets/pages/labels.scss +++ b/app/assets/stylesheets/pages/labels.scss @@ -41,3 +41,7 @@ .color-label { padding: 3px 4px; } + +.label-subscription { + display: inline-block; +} \ No newline at end of file diff --git a/app/controllers/projects/labels_controller.rb b/app/controllers/projects/labels_controller.rb index ecac3c395ec..d0334c37b67 100644 --- a/app/controllers/projects/labels_controller.rb +++ b/app/controllers/projects/labels_controller.rb @@ -1,8 +1,8 @@ class Projects::LabelsController < Projects::ApplicationController before_action :module_enabled - before_action :label, only: [:edit, :update, :destroy] + before_action :label, only: [:edit, :update, :destroy, :toggle_subscription] before_action :authorize_read_label! - before_action :authorize_admin_labels!, except: [:index] + before_action :authorize_admin_labels!, except: [:index, :toggle_subscription] respond_to :js, :html @@ -60,6 +60,11 @@ class Projects::LabelsController < Projects::ApplicationController end end + def toggle_subscription + @label.toggle_subscription(current_user) + render nothing: true + end + protected def module_enabled diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb index 4a88cb61132..2838baa1b4e 100644 --- a/app/mailers/emails/issues.rb +++ b/app/mailers/emails/issues.rb @@ -16,7 +16,14 @@ module Emails def closed_issue_email(recipient_id, issue_id, updated_by_user_id) setup_issue_mail(issue_id, recipient_id) - @updated_by = User.find updated_by_user_id + @updated_by = User.find(updated_by_user_id) + 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) + @label_names = label_names + @updated_by = User.find(updated_by_user_id) mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id)) end @@ -24,7 +31,7 @@ module Emails setup_issue_mail(issue_id, recipient_id) @issue_status = status - @updated_by = User.find updated_by_user_id + @updated_by = User.find(updated_by_user_id) mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id)) end diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index 325996e2e16..680ce975c79 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/app/mailers/emails/merge_requests.rb @@ -19,10 +19,20 @@ module Emails subject: subject("#{@merge_request.title} (##{@merge_request.iid})")) 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) + @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})")) + 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 + @updated_by = User.find(updated_by_user_id) mail_answer_thread(@merge_request, from: sender(updated_by_user_id), to: recipient(recipient_id), @@ -42,7 +52,7 @@ module Emails setup_merge_request_mail(merge_request_id, recipient_id) @mr_status = status - @updated_by = User.find updated_by_user_id + @updated_by = User.find(updated_by_user_id) mail_answer_thread(@merge_request, from: sender(updated_by_user_id), to: recipient(recipient_id), diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 3c42f582937..affc4a842a7 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -8,6 +8,7 @@ module Issuable extend ActiveSupport::Concern include Participable include Mentionable + include Subscribable include StripAttribute included do @@ -18,7 +19,6 @@ module Issuable has_many :notes, as: :noteable, dependent: :destroy has_many :label_links, as: :target, dependent: :destroy has_many :labels, through: :label_links - has_many :subscriptions, dependent: :destroy, as: :subscribable validates :author, presence: true validates :title, presence: true, length: { within: 0..255 } @@ -149,28 +149,6 @@ module Issuable notes.awards.where(note: "thumbsup").count end - def subscribed?(user) - subscription = subscriptions.find_by_user_id(user.id) - - if subscription - return subscription.subscribed - end - - participants(user).include?(user) - end - - def toggle_subscription(user) - subscriptions. - find_or_initialize_by(user_id: user.id). - update(subscribed: !subscribed?(user)) - end - - def unsubscribe(user) - subscriptions. - find_or_initialize_by(user_id: user.id). - update(subscribed: false) - end - def to_hook_data(user) hook_data = { object_kind: self.class.name.underscore, @@ -201,6 +179,12 @@ 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 new file mode 100644 index 00000000000..cab9241ac3d --- /dev/null +++ b/app/models/concerns/subscribable.rb @@ -0,0 +1,43 @@ +# == Subscribable concern +# +# Users can subscribe to these models. +# +# Used by Issue, MergeRequest, Label +# + +module Subscribable + extend ActiveSupport::Concern + + included do + has_many :subscriptions, dependent: :destroy, as: :subscribable + end + + def subscribed?(user) + subscription = subscriptions.find_by_user_id(user.id) + + if subscription + return subscription.subscribed + 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 + end + + def toggle_subscription(user) + subscriptions. + find_or_initialize_by(user_id: user.id). + update(subscribed: !subscribed?(user)) + end + + def unsubscribe(user) + subscriptions. + find_or_initialize_by(user_id: user.id). + update(subscribed: false) + end +end diff --git a/app/models/label.rb b/app/models/label.rb index 5ff644b8426..f7ffc0b7f36 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -14,6 +14,8 @@ class Label < ActiveRecord::Base include Referable + include Subscribable + # Represents a "No Label" state used for filtering Issues and Merge # Requests that have no label assigned. LabelStruct = Struct.new(:title, :name) diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index ca87dca4a70..971c074882e 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -95,7 +95,7 @@ class IssuableBaseService < BaseService old_labels = options[:old_labels] if old_labels && (issuable.labels != old_labels) - create_labels_note(issuable, issuable.labels - old_labels, old_labels - issuable.labels) + create_labels_note(issuable, issuable.added_labels(old_labels), old_labels - issuable.labels) end end end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index 51ef9dfe610..b2e63a4e1af 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -4,7 +4,7 @@ module Issues update(issue) end - def handle_changes(issue, options = {}) + def handle_changes(issue, old_labels: [], new_labels: []) if has_changes?(issue, options) todo_service.mark_pending_todos_as_done(issue, current_user) end @@ -23,6 +23,11 @@ module Issues notification_service.reassigned_issue(issue, current_user) 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) + end end def reopen_service diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 6319ad805b6..6fd569dc302 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -14,7 +14,7 @@ module MergeRequests update(merge_request) end - def handle_changes(merge_request, options = {}) + def handle_changes(issue, old_labels: [], new_labels: []) if has_changes?(merge_request, options) todo_service.mark_pending_todos_as_done(merge_request, current_user) end @@ -44,6 +44,11 @@ module MergeRequests merge_request.previous_changes.include?('source_branch') 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) + end end def reopen_service diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index ca8a41d93b8..e9955cd3e2d 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -52,6 +52,14 @@ 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. + # + # 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') + end + # When create a merge request we should send next emails: # @@ -70,6 +78,14 @@ 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. + # + # 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') + end + def close_mr(merge_request, current_user) close_resource_email(merge_request, merge_request.target_project, current_user, 'closed_merge_request_email') end @@ -142,6 +158,7 @@ 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) @@ -359,6 +376,16 @@ class NotificationService end end + def add_label_subscriptions(recipients, target) + return recipients unless target.respond_to? :labels + + target.labels.each do |label| + recipients += label.subscriptions.where(subscribed: true).map(&:user) + end + + recipients + end + def new_resource_email(target, project, method) recipients = build_recipients(target, project, target.author) @@ -392,6 +419,15 @@ class NotificationService end end + def relabel_resource_email(target, project, labels, current_user, method) + recipients = build_relabel_recipients(target, project, labels, current_user) + label_names = labels.map(&:name) + + recipients.each do |recipient| + mailer.send(method, recipient.id, target.id, current_user.id, label_names).deliver_later + end + end + def reopen_resource_email(target, project, current_user, method, status) recipients = build_recipients(target, project, current_user) @@ -416,6 +452,7 @@ class NotificationService recipients = reject_muted_users(recipients, project) recipients = add_subscribed_users(recipients, target) + recipients = add_label_subscriptions(recipients, target) recipients = reject_unsubscribed_users(recipients, target) recipients.delete(current_user) @@ -423,6 +460,13 @@ class NotificationService recipients.uniq end + def build_relabel_recipients(target, project, labels, current_user) + recipients = add_label_subscriptions([], target) + recipients = reject_unsubscribed_users(recipients, target) + recipients.delete(current_user) + recipients.uniq + end + def mailer Notify end diff --git a/app/views/notify/_relabeled_issuable_email.html.haml b/app/views/notify/_relabeled_issuable_email.html.haml new file mode 100644 index 00000000000..a41ff07c306 --- /dev/null +++ b/app/views/notify/_relabeled_issuable_email.html.haml @@ -0,0 +1,5 @@ +%p + #{@updated_by.name} added the + %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 new file mode 100644 index 00000000000..1a28d7fd352 --- /dev/null +++ b/app/views/notify/_relabeled_issuable_email.text.erb @@ -0,0 +1,5 @@ +<%= issuable.class.model_name.human.titleize %> <%= issuable.iid %> was relabeled. + +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 %> diff --git a/app/views/notify/relabeled_issue_email.html.haml b/app/views/notify/relabeled_issue_email.html.haml new file mode 100644 index 00000000000..a3e094e86eb --- /dev/null +++ b/app/views/notify/relabeled_issue_email.html.haml @@ -0,0 +1 @@ += 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 new file mode 100644 index 00000000000..de7f04a323a --- /dev/null +++ b/app/views/notify/relabeled_issue_email.text.erb @@ -0,0 +1 @@ +<%= 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 new file mode 100644 index 00000000000..3937b449a37 --- /dev/null +++ b/app/views/notify/relabeled_merge_request_email.html.haml @@ -0,0 +1 @@ += 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 new file mode 100644 index 00000000000..5c4d86e4dc2 --- /dev/null +++ b/app/views/notify/relabeled_merge_request_email.text.erb @@ -0,0 +1 @@ +<%= 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 f7ddd30c5a9..3b14a925c46 100644 --- a/app/views/projects/labels/_label.html.haml +++ b/app/views/projects/labels/_label.html.haml @@ -10,6 +10,18 @@ = link_to_label(label) do = 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' + - 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}']"); diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml index 9020a1330a3..b44086d4205 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 + .block.light.subscription .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)}"); + new Subscription("#{toggle_subscription_path(issuable)}", ".subscription"); new IssuableContext(); diff --git a/config/routes.rb b/config/routes.rb index 780ad757c5b..2ae282f48a6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -675,6 +675,10 @@ Rails.application.routes.draw do collection do post :generate end + + member do + post :toggle_subscription + end end resources :issues, constraints: { id: /\d+/ }, except: [:destroy] do diff --git a/features/project/labels.feature b/features/project/labels.feature new file mode 100644 index 00000000000..cd3f3a789f0 --- /dev/null +++ b/features/project/labels.feature @@ -0,0 +1,16 @@ +@labels +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" + + @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 diff --git a/features/steps/project/labels.rb b/features/steps/project/labels.rb new file mode 100644 index 00000000000..3f800a10594 --- /dev/null +++ b/features/steps/project/labels.rb @@ -0,0 +1,34 @@ +class Spinach::Features::Labels < Spinach::FeatureSteps + include SharedAuthentication + include SharedIssuable + include SharedProject + include SharedNote + include SharedPaths + include SharedMarkdown + + step 'And I visit project "Shop" labels page' do + visit namespace_project_labels_path(project.namespace, project) + end + + step 'I should see that I am subscribed' do + expect(subscribe_button).to have_content 'Unsubscribe' + end + + step 'I should see that I am unsubscribed' do + expect(subscribe_button).to have_content 'Subscribe' + end + + step 'I click button "Unsubscribe"' do + subscribe_button.click + end + + step 'I click button "Subscribe"' do + subscribe_button.click + end + + private + + def subscribe_button + first('.subscribe-button span') + end +end diff --git a/spec/factories/labels.rb b/spec/factories/labels.rb index 6e70af10af3..91a7400afa1 100644 --- a/spec/factories/labels.rb +++ b/spec/factories/labels.rb @@ -13,7 +13,7 @@ FactoryGirl.define do factory :label do - title "Bug" + title { FFaker::Color.name } color "#990000" project end diff --git a/spec/models/concerns/subscribable_spec.rb b/spec/models/concerns/subscribable_spec.rb new file mode 100644 index 00000000000..9ee60426a5d --- /dev/null +++ b/spec/models/concerns/subscribable_spec.rb @@ -0,0 +1,16 @@ +require "spec_helper" + +describe Subscribable, "Subscribable" do + let(:resource) { create(:issue) } + let(:user) { create(:user) } + + describe "#subscribed?" do + it do + expect(resource.subscribed?(user)).to be_falsey + resource.toggle_subscription(user) + expect(resource.subscribed?(user)).to be_truthy + resource.toggle_subscription(user) + expect(resource.subscribed?(user)).to be_falsey + end + end +end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index e579e49dfa7..dc9d8329751 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -48,7 +48,7 @@ describe Issues::UpdateService, services: true do it { expect(@issue.assignee).to eq(user2) } it { expect(@issue).to be_closed } it { expect(@issue.labels.count).to eq(1) } - it { expect(@issue.labels.first.title).to eq('Bug') } + it { expect(@issue.labels.first.title).to eq(label.name) } it 'should send email to user2 about assign of new issue and email to user3 about issue unassignment' do deliveries = ActionMailer::Base.deliveries @@ -148,6 +148,60 @@ 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) } + + 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) } + + opts = { label_ids: [label.id, second_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 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) } + + opts = { label_ids: [second_label.id] } + + perform_enqueued_jobs do + @issue = Issues::UpdateService.new(project, user, opts).execute(issue) + end + + @issue.reload + should_not_email(subscriber) + should_not_email(non_subscriber) + end + end + context 'when Issue has tasks' do before { update_issue({ description: "- [ ] Task 1\n- [ ] Task 2" }) } diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 99703c7a8ec..104e63ccfee 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -53,7 +53,7 @@ describe MergeRequests::UpdateService, services: true do it { expect(@merge_request.assignee).to eq(user2) } it { expect(@merge_request).to be_closed } it { expect(@merge_request.labels.count).to eq(1) } - it { expect(@merge_request.labels.first.title).to eq('Bug') } + it { expect(@merge_request.labels.first.title).to eq(label.name) } it { expect(@merge_request.target_branch).to eq('target') } it 'should execute hooks with update action' do @@ -176,6 +176,60 @@ 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) } + + 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) } + + opts = { label_ids: [label.id, second_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 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) } + + opts = { label_ids: [second_label.id] } + + perform_enqueued_jobs do + @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + end + + @merge_request.reload + should_not_email(subscriber) + should_not_email(non_subscriber) + end + end + context 'when MergeRequest has tasks' do before { update_merge_request({ description: "- [ ] Task 1\n- [ ] Task 2" }) } diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 2d0b5df4224..35afa768057 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -224,6 +224,16 @@ 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) + 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 describe :reassigned_issue do @@ -326,6 +336,32 @@ 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 @@ -349,6 +385,17 @@ describe NotificationService, services: true do should_not_email(@u_participating) 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 + 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 describe :reassigned_merge_request do @@ -410,6 +457,34 @@ 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 @@ -467,16 +542,4 @@ describe NotificationService, services: true do # Make the watcher a subscriber to detect dupes issuable.subscriptions.create(user: @watcher_and_subscriber, subscribed: true) end - - def sent_to_user?(user) - ActionMailer::Base.deliveries.map(&:to).flatten.count(user.email) == 1 - end - - def should_email(user) - expect(sent_to_user?(user)).to be_truthy - end - - def should_not_email(user) - expect(sent_to_user?(user)).to be_falsey - end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 7d939ca7509..596d607f2a1 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -32,6 +32,7 @@ RSpec.configure do |config| config.include LoginHelpers, type: :feature config.include LoginHelpers, type: :request config.include StubConfiguration + config.include EmailHelpers config.include RelativeUrl, type: feature config.include TestEnv config.include ActiveJob::TestHelper diff --git a/spec/support/email_helpers.rb b/spec/support/email_helpers.rb new file mode 100644 index 00000000000..a85ab22ce36 --- /dev/null +++ b/spec/support/email_helpers.rb @@ -0,0 +1,13 @@ +module EmailHelpers + def sent_to_user?(user) + ActionMailer::Base.deliveries.map(&:to).flatten.count(user.email) == 1 + end + + def should_email(user) + expect(sent_to_user?(user)).to be_truthy + end + + def should_not_email(user) + expect(sent_to_user?(user)).to be_falsey + end +end