GitLab wurde erfolgreich aktualisiert. Durch regelmäßige Updates bleibt das THM GitLab sicher. Danke für Ihre Geduld.

Commit 0444fa56 authored by Timothy Andrew's avatar Timothy Andrew Committed by Rémy Coutable

Original implementation to allow users to subscribe to labels

1. Allow subscribing (the current user) to a label

- Refactor the `Subscription` coffeescript class
  - The main change is that it accepts a container, and conducts all
    DOM queries within its scope. We need this because the labels
    page has multiple instances of `Subscription` on the same page.

2. Creating an issue or MR with labels notifies users subscribed to those labels

- Label `has_many` subscribers through subscriptions.

3. Adding a label to an issue or MR notifies users subscribed to those labels

- This only applies to subscribers of the label that has just been
  added, not all labels for the issue.
parent 178c80a5
......@@ -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
......
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")
......@@ -41,3 +41,7 @@
.color-label {
padding: 3px 4px;
}
.label-subscription {
display: inline-block;
}
\ No newline at end of file
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 @@ def destroy
end
end
def toggle_subscription
@label.toggle_subscription(current_user)
render nothing: true
end
protected
def module_enabled
......
......@@ -16,7 +16,14 @@ def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id, updated
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 @@ def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_i
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
......
......@@ -19,10 +19,20 @@ def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assi
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 @@ def merge_request_status_email(recipient_id, merge_request_id, status, updated_b
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),
......
......@@ -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 @@ def upvotes
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 @@ def add_labels_by_names(label_names)
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:
......
# == 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
......@@ -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)
......
......@@ -95,7 +95,7 @@ def handle_common_system_notes(issuable, options = {})
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
......@@ -4,7 +4,7 @@ def execute(issue)
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 @@ def handle_changes(issue, options = {})
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
......
......@@ -14,7 +14,7 @@ def execute(merge_request)
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 @@ def handle_changes(merge_request, options = {})
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
......
......@@ -52,6 +52,14 @@ def reassigned_issue(issue, current_user)
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 @@ def reassigned_merge_request(merge_request, current_user)
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 @@ def new_note(note)
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 @@ def add_subscribed_users(recipients, target)
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 @@ def reassign_resource_email(target, project, current_user, method)
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 @@ def build_recipients(target, project, current_user, action: nil, previous_assign
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 @@ def build_recipients(target, project, current_user, action: nil, previous_assign
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
......
%p
#{@updated_by.name} added the
%em= @label_names.to_sentence
#{"label".pluralize(@label_names.count)} to #{issuable.class.model_name.human} #{issuable.iid}.
<%= 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 %>
= render "relabeled_issuable_email", issuable: @issue
<%= render "relabeled_issuable_email", issuable: @issue %>
= render "relabeled_issuable_email", issuable: @merge_request
<%= render "relabeled_issuable_email", issuable: @merge_request %>
......@@ -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}']");
......@@ -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();
......@@ -675,6 +675,10 @@
collection do
post :generate
end
member do
post :toggle_subscription
end
end
resources :issues, constraints: { id: /\d+/ }, except: [:destroy] do
......
@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
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
......@@ -13,7 +13,7 @@
FactoryGirl.define do
factory :label do
title "Bug"
title { FFaker::Color.name }
color "#990000"
project
end
......
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
......@@ -48,7 +48,7 @@ def update_issue(opts)
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 @@ def update_issue(opts)
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" }) }
......
......@@ -53,7 +53,7 @@ def update_merge_request(opts)
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 @@ def update_merge_request(opts)
end
end