summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--CHANGELOG2
-rw-r--r--app/assets/javascripts/subscription.js.coffee32
-rw-r--r--app/assets/stylesheets/pages/labels.scss4
-rw-r--r--app/controllers/projects/labels_controller.rb9
-rw-r--r--app/mailers/emails/issues.rb11
-rw-r--r--app/mailers/emails/merge_requests.rb14
-rw-r--r--app/models/concerns/issuable.rb30
-rw-r--r--app/models/concerns/subscribable.rb43
-rw-r--r--app/models/label.rb2
-rw-r--r--app/services/issuable_base_service.rb2
-rw-r--r--app/services/issues/update_service.rb7
-rw-r--r--app/services/merge_requests/update_service.rb7
-rw-r--r--app/services/notification_service.rb44
-rw-r--r--app/views/notify/_relabeled_issuable_email.html.haml5
-rw-r--r--app/views/notify/_relabeled_issuable_email.text.erb5
-rw-r--r--app/views/notify/relabeled_issue_email.html.haml1
-rw-r--r--app/views/notify/relabeled_issue_email.text.erb1
-rw-r--r--app/views/notify/relabeled_merge_request_email.html.haml1
-rw-r--r--app/views/notify/relabeled_merge_request_email.text.erb1
-rw-r--r--app/views/projects/labels/_label.html.haml12
-rw-r--r--app/views/shared/issuable/_sidebar.html.haml4
-rw-r--r--config/routes.rb4
-rw-r--r--features/project/labels.feature16
-rw-r--r--features/steps/project/labels.rb34
-rw-r--r--spec/factories/labels.rb2
-rw-r--r--spec/models/concerns/subscribable_spec.rb16
-rw-r--r--spec/services/issues/update_service_spec.rb56
-rw-r--r--spec/services/merge_requests/update_service_spec.rb56
-rw-r--r--spec/services/notification_service_spec.rb87
-rw-r--r--spec/spec_helper.rb1
-rw-r--r--spec/support/email_helpers.rb13
31 files changed, 458 insertions, 64 deletions
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