diff options
author | Grzegorz Bizon <grzesiek.bizon@gmail.com> | 2016-11-17 20:19:16 +0100 |
---|---|---|
committer | Grzegorz Bizon <grzesiek.bizon@gmail.com> | 2016-11-17 20:19:16 +0100 |
commit | d9f9be3c9548e8e99e071d764163c47c073c3c28 (patch) | |
tree | 879d15c8d794a2f0265debbb3dd76a73e1eb4432 | |
parent | 8a7860fd7fe91a9d2642dab9e66be5b22012901d (diff) | |
parent | 3344e1e8dbcb3e53e388511a661f6769b375b748 (diff) | |
download | gitlab-ce-d9f9be3c9548e8e99e071d764163c47c073c3c28.tar.gz |
Merge branch 'master' into 22539-display-folders
* master: (26 commits)
Remove extra subscribable_type filter on migration
Add feature spec for labels subscription
Avoid code duplication for label subscription status on label partial
Rename LabelSubscription javascript to ProjectLabelSubscription
Fix label subscription menu on small screens resolution
Allow users to subscribe to a group label at group or project level
Use button instead of an icon to subscribe/unsubscribe to labels
Add CHANGELOG entry
Add toggle_subscription action to Groups::LabelsController
Allow subscriptions to be created without a project
Use subqueries instead of joins to migrate subscriptions
Add helper method to toggle label subscription on labels controller spec
Remove default value for `project` argument on subscribable concern
Use @project as default on ToggleSubscriptionAction concern
Allow users to subscribe to group labels at project-level
Pass project to Entities::Label to check if user is subscribed
Fix specs to pass a project when creating subscriptions
Refactoring label subscription toggle button text to accept a project
Refactoring label subscription status to accept a project
Refactoring notification service to find subscriptions per project
...
46 files changed, 754 insertions, 227 deletions
diff --git a/app/assets/javascripts/group_label_subscription.js.es6 b/app/assets/javascripts/group_label_subscription.js.es6 new file mode 100644 index 00000000000..eea6cd40859 --- /dev/null +++ b/app/assets/javascripts/group_label_subscription.js.es6 @@ -0,0 +1,53 @@ +/* eslint-disable */ +(function(global) { + class GroupLabelSubscription { + constructor(container) { + const $container = $(container); + this.$dropdown = $container.find('.dropdown'); + this.$subscribeButtons = $container.find('.js-subscribe-button'); + this.$unsubscribeButtons = $container.find('.js-unsubscribe-button'); + + this.$subscribeButtons.on('click', this.subscribe.bind(this)); + this.$unsubscribeButtons.on('click', this.unsubscribe.bind(this)); + } + + unsubscribe(event) { + event.preventDefault(); + + const url = this.$unsubscribeButtons.attr('data-url'); + + $.ajax({ + type: 'POST', + url: url + }).done(() => { + this.toggleSubscriptionButtons(); + this.$unsubscribeButtons.removeAttr('data-url'); + }); + } + + subscribe(event) { + event.preventDefault(); + + const $btn = $(event.currentTarget); + const url = $btn.attr('data-url'); + + this.$unsubscribeButtons.attr('data-url', url); + + $.ajax({ + type: 'POST', + url: url + }).done(() => { + this.toggleSubscriptionButtons(); + }); + } + + toggleSubscriptionButtons() { + this.$dropdown.toggleClass('hidden'); + this.$subscribeButtons.toggleClass('hidden'); + this.$unsubscribeButtons.toggleClass('hidden'); + } + } + + global.GroupLabelSubscription = GroupLabelSubscription; + +})(window.gl || (window.gl = {})); diff --git a/app/assets/javascripts/project_label_subscription.js.es6 b/app/assets/javascripts/project_label_subscription.js.es6 new file mode 100644 index 00000000000..03a115cb35b --- /dev/null +++ b/app/assets/javascripts/project_label_subscription.js.es6 @@ -0,0 +1,53 @@ +/* eslint-disable */ +(function(global) { + class ProjectLabelSubscription { + constructor(container) { + this.$container = $(container); + this.$buttons = this.$container.find('.js-subscribe-button'); + + this.$buttons.on('click', this.toggleSubscription.bind(this)); + } + + toggleSubscription(event) { + event.preventDefault(); + + const $btn = $(event.currentTarget); + const $span = $btn.find('span'); + const url = $btn.attr('data-url'); + const oldStatus = $btn.attr('data-status'); + + $btn.addClass('disabled'); + $span.toggleClass('hidden'); + + $.ajax({ + type: 'POST', + url: url + }).done(() => { + let newStatus, newAction; + + if (oldStatus === 'unsubscribed') { + [newStatus, newAction] = ['subscribed', 'Unsubscribe']; + } else { + [newStatus, newAction] = ['unsubscribed', 'Subscribe']; + } + + $span.toggleClass('hidden'); + $btn.removeClass('disabled'); + + this.$buttons.attr('data-status', newStatus); + this.$buttons.find('> span').text(newAction); + + for (let button of this.$buttons) { + let $button = $(button); + + if ($button.attr('data-original-title')) { + $button.tooltip('hide').attr('data-original-title', newAction).tooltip('fixTitle'); + } + } + }); + } + } + + global.ProjectLabelSubscription = ProjectLabelSubscription; + +})(window.gl || (window.gl = {})); diff --git a/app/assets/stylesheets/pages/labels.scss b/app/assets/stylesheets/pages/labels.scss index 397f89f501a..e39ce19f846 100644 --- a/app/assets/stylesheets/pages/labels.scss +++ b/app/assets/stylesheets/pages/labels.scss @@ -90,7 +90,7 @@ @media (min-width: $screen-sm-min) { display: inline-block; - width: 40%; + width: 30%; margin-left: 10px; margin-bottom: 0; vertical-align: middle; @@ -222,6 +222,14 @@ width: 100%; } +.label-subscription { + vertical-align: middle; + + .dropdown-group-label a { + cursor: pointer; + } +} + .label-subscribe-button { .label-subscribe-button-icon { &[disabled] { diff --git a/app/controllers/concerns/toggle_subscription_action.rb b/app/controllers/concerns/toggle_subscription_action.rb index 9e3b9be2ff4..92cb534343e 100644 --- a/app/controllers/concerns/toggle_subscription_action.rb +++ b/app/controllers/concerns/toggle_subscription_action.rb @@ -4,13 +4,17 @@ module ToggleSubscriptionAction def toggle_subscription return unless current_user - subscribable_resource.toggle_subscription(current_user) + subscribable_resource.toggle_subscription(current_user, subscribable_project) head :ok end private + def subscribable_project + @project || raise(NotImplementedError) + end + def subscribable_resource raise NotImplementedError end diff --git a/app/controllers/groups/labels_controller.rb b/app/controllers/groups/labels_controller.rb index 29528b2cfaa..587898a8634 100644 --- a/app/controllers/groups/labels_controller.rb +++ b/app/controllers/groups/labels_controller.rb @@ -1,4 +1,6 @@ class Groups::LabelsController < Groups::ApplicationController + include ToggleSubscriptionAction + before_action :label, only: [:edit, :update, :destroy] before_action :authorize_admin_labels!, only: [:new, :create, :edit, :update, :destroy] before_action :save_previous_label_path, only: [:edit] @@ -69,6 +71,11 @@ class Groups::LabelsController < Groups::ApplicationController def label @label ||= @group.labels.find(params[:id]) end + alias_method :subscribable_resource, :label + + def subscribable_project + nil + end def label_params params.require(:label).permit(:title, :description, :color) diff --git a/app/controllers/projects/labels_controller.rb b/app/controllers/projects/labels_controller.rb index 42fd09e9b7e..824ed7be73e 100644 --- a/app/controllers/projects/labels_controller.rb +++ b/app/controllers/projects/labels_controller.rb @@ -3,7 +3,7 @@ class Projects::LabelsController < Projects::ApplicationController before_action :module_enabled before_action :label, only: [:edit, :update, :destroy] - before_action :find_labels, only: [:index, :set_priorities, :remove_priority] + before_action :find_labels, only: [:index, :set_priorities, :remove_priority, :toggle_subscription] before_action :authorize_read_label! before_action :authorize_admin_labels!, only: [:new, :create, :edit, :update, :generate, :destroy, :remove_priority, @@ -123,7 +123,10 @@ class Projects::LabelsController < Projects::ApplicationController def label @label ||= @project.labels.find(params[:id]) end - alias_method :subscribable_resource, :label + + def subscribable_resource + @available_labels.find(params[:id]) + end def find_labels @available_labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute diff --git a/app/controllers/sent_notifications_controller.rb b/app/controllers/sent_notifications_controller.rb index 3085ff33aba..04c36b3ebfe 100644 --- a/app/controllers/sent_notifications_controller.rb +++ b/app/controllers/sent_notifications_controller.rb @@ -12,7 +12,7 @@ class SentNotificationsController < ApplicationController def unsubscribe_and_redirect noteable = @sent_notification.noteable - noteable.unsubscribe(@sent_notification.recipient) + noteable.unsubscribe(@sent_notification.recipient, @sent_notification.project) flash[:notice] = "You have been unsubscribed from this thread." diff --git a/app/helpers/labels_helper.rb b/app/helpers/labels_helper.rb index 221a84b042f..4f180456b16 100644 --- a/app/helpers/labels_helper.rb +++ b/app/helpers/labels_helper.rb @@ -68,14 +68,6 @@ module LabelsHelper end end - def toggle_subscription_data(label) - return unless label.is_a?(ProjectLabel) - - { - url: toggle_subscription_namespace_project_label_path(label.project.namespace, label.project, label) - } - end - def render_colored_label(label, label_suffix = '', tooltip: true) label_color = label.color || Label::DEFAULT_COLOR text_color = text_color_for_bg(label_color) @@ -148,20 +140,24 @@ module LabelsHelper end end - def label_subscription_status(label) - case label - when GroupLabel then 'Subscribing to group labels is currently not supported.' - when ProjectLabel then label.subscribed?(current_user) ? 'subscribed' : 'unsubscribed' - end + def label_subscription_status(label, project) + return 'project-level' if label.subscribed?(current_user, project) + return 'group-level' if label.subscribed?(current_user) + + 'unsubscribed' end - def label_subscription_toggle_button_text(label) - case label - when GroupLabel then 'Subscribing to group labels is currently not supported.' - when ProjectLabel then label.subscribed?(current_user) ? 'Unsubscribe' : 'Subscribe' + def group_label_unsubscribe_path(label, project) + case label_subscription_status(label, project) + when 'project-level' then toggle_subscription_namespace_project_label_path(@project.namespace, @project, label) + when 'group-level' then toggle_subscription_group_label_path(label.group, label) end end + def label_subscription_toggle_button_text(label, project) + label.subscribed?(current_user, project) ? 'Unsubscribe' : 'Subscribe' + end + def label_deletion_confirm_text(label) case label when GroupLabel then 'Remove this label? This will affect all projects within the group. Are you sure?' diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 664bb594aa9..ec9e7e5ae2b 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -215,7 +215,7 @@ module Issuable end end - def subscribed_without_subscriptions?(user) + def subscribed_without_subscriptions?(user, project) participants(user).include?(user) end diff --git a/app/models/concerns/subscribable.rb b/app/models/concerns/subscribable.rb index 083257f1005..83daa9b1a64 100644 --- a/app/models/concerns/subscribable.rb +++ b/app/models/concerns/subscribable.rb @@ -12,39 +12,71 @@ module Subscribable has_many :subscriptions, dependent: :destroy, as: :subscribable end - def subscribed?(user) - if subscription = subscriptions.find_by_user_id(user.id) + def subscribed?(user, project = nil) + if subscription = subscriptions.find_by(user: user, project: project) subscription.subscribed else - subscribed_without_subscriptions?(user) + subscribed_without_subscriptions?(user, project) end end # Override this method to define custom logic to consider a subscribable as # subscribed without an explicit subscription record. - def subscribed_without_subscriptions?(user) + def subscribed_without_subscriptions?(user, project) false end - def subscribers - subscriptions.where(subscribed: true).map(&:user) + def subscribers(project) + subscriptions_available(project). + where(subscribed: true). + map(&:user) end - def toggle_subscription(user) - subscriptions. - find_or_initialize_by(user_id: user.id). - update(subscribed: !subscribed?(user)) + def toggle_subscription(user, project = nil) + unsubscribe_from_other_levels(user, project) + + find_or_initialize_subscription(user, project). + update(subscribed: !subscribed?(user, project)) + end + + def subscribe(user, project = nil) + unsubscribe_from_other_levels(user, project) + + find_or_initialize_subscription(user, project) + .update(subscribed: true) + end + + def unsubscribe(user, project = nil) + unsubscribe_from_other_levels(user, project) + + find_or_initialize_subscription(user, project) + .update(subscribed: false) end - def subscribe(user) + private + + def unsubscribe_from_other_levels(user, project) + other_subscriptions = subscriptions.where(user: user) + + other_subscriptions = + if project.blank? + other_subscriptions.where.not(project: nil) + else + other_subscriptions.where(project: nil) + end + + other_subscriptions.update_all(subscribed: false) + end + + def find_or_initialize_subscription(user, project) subscriptions. - find_or_initialize_by(user_id: user.id). - update(subscribed: true) + find_or_initialize_by(user_id: user.id, project_id: project.try(:id)) end - def unsubscribe(user) + def subscriptions_available(project) + t = Subscription.arel_table + subscriptions. - find_or_initialize_by(user_id: user.id). - update(subscribed: false) + where(t[:project_id].eq(nil).or(t[:project_id].eq(project.try(:id)))) end end diff --git a/app/models/issue.rb b/app/models/issue.rb index 4a4017003d8..6e8f5d3c422 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -266,7 +266,7 @@ class Issue < ActiveRecord::Base def as_json(options = {}) super(options).tap do |json| - json[:subscribed] = subscribed?(options[:user]) if options.has_key?(:user) && options[:user] + json[:subscribed] = subscribed?(options[:user], project) if options.has_key?(:user) && options[:user] if options.has_key?(:labels) json[:labels] = labels.as_json( diff --git a/app/models/subscription.rb b/app/models/subscription.rb index 3b8aa1eb866..17869c8bac2 100644 --- a/app/models/subscription.rb +++ b/app/models/subscription.rb @@ -1,8 +1,9 @@ class Subscription < ActiveRecord::Base belongs_to :user + belongs_to :project belongs_to :subscribable, polymorphic: true - validates :user_id, - uniqueness: { scope: [:subscribable_id, :subscribable_type] }, - presence: true + validates :user, :subscribable, presence: true + + validates :project_id, uniqueness: { scope: [:subscribable_id, :subscribable_type, :user_id] } end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index bb92cd80cc9..575795788de 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -212,9 +212,9 @@ class IssuableBaseService < BaseService def change_subscription(issuable) case params.delete(:subscription_event) when 'subscribe' - issuable.subscribe(current_user) + issuable.subscribe(current_user, project) when 'unsubscribe' - issuable.unsubscribe(current_user) + issuable.unsubscribe(current_user, project) end end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 6697840cc26..ecdcbf08ee1 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -75,7 +75,7 @@ class NotificationService # * 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) + relabeled_resource_email(issue, issue.project, added_labels, current_user, :relabeled_issue_email) end # When create a merge request we should send an email to: @@ -118,7 +118,7 @@ class NotificationService # * 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) + relabeled_resource_email(merge_request, merge_request.target_project, added_labels, current_user, :relabeled_merge_request_email) end def close_mr(merge_request, current_user) @@ -205,7 +205,7 @@ class NotificationService recipients = reject_muted_users(recipients, note.project) - recipients = add_subscribed_users(recipients, note.noteable) + recipients = add_subscribed_users(recipients, note.project, note.noteable) recipients = reject_unsubscribed_users(recipients, note.noteable) recipients = reject_users_without_access(recipients, note.noteable) @@ -393,7 +393,7 @@ class NotificationService ) end - # Build a list of users based on project notifcation settings + # Build a list of users based on project notification settings def select_project_member_setting(project, global_setting, users_global_level_watch) users = notification_settings_for(project, :watch) @@ -505,17 +505,17 @@ class NotificationService end end - def add_subscribed_users(recipients, target) + def add_subscribed_users(recipients, project, target) return recipients unless target.respond_to? :subscribers - recipients + target.subscribers + recipients + target.subscribers(project) end - def add_labels_subscribers(recipients, target, labels: nil) + def add_labels_subscribers(recipients, project, target, labels: nil) return recipients unless target.respond_to? :labels (labels || target.labels).each do |label| - recipients += label.subscribers + recipients += label.subscribers(project) end recipients @@ -571,8 +571,8 @@ class NotificationService end end - def relabeled_resource_email(target, labels, current_user, method) - recipients = build_relabeled_recipients(target, current_user, labels: labels) + def relabeled_resource_email(target, project, labels, current_user, method) + recipients = build_relabeled_recipients(target, project, current_user, labels: labels) label_names = labels.map(&:name) recipients.each do |recipient| @@ -608,10 +608,10 @@ class NotificationService end recipients = reject_muted_users(recipients, project) - recipients = add_subscribed_users(recipients, target) + recipients = add_subscribed_users(recipients, project, target) if [:new_issue, :new_merge_request].include?(custom_action) - recipients = add_labels_subscribers(recipients, target) + recipients = add_labels_subscribers(recipients, project, target) end recipients = reject_unsubscribed_users(recipients, target) @@ -622,8 +622,8 @@ class NotificationService recipients.uniq end - def build_relabeled_recipients(target, current_user, labels:) - recipients = add_labels_subscribers([], target, labels: labels) + def build_relabeled_recipients(target, project, current_user, labels:) + recipients = add_labels_subscribers([], project, target, labels: labels) recipients = reject_unsubscribed_users(recipients, target) recipients = reject_users_without_access(recipients, target) recipients.delete(current_user) diff --git a/app/services/slash_commands/interpret_service.rb b/app/services/slash_commands/interpret_service.rb index 5a81194a5f4..d75c5b1800e 100644 --- a/app/services/slash_commands/interpret_service.rb +++ b/app/services/slash_commands/interpret_service.rb @@ -193,7 +193,7 @@ module SlashCommands desc 'Subscribe' condition do issuable.persisted? && - !issuable.subscribed?(current_user) + !issuable.subscribed?(current_user, project) end command :subscribe do @updates[:subscription_event] = 'subscribe' @@ -202,7 +202,7 @@ module SlashCommands desc 'Unsubscribe' condition do issuable.persisted? && - issuable.subscribed?(current_user) + issuable.subscribed?(current_user, project) end command :unsubscribe do @updates[:subscription_event] = 'unsubscribe' diff --git a/app/views/shared/_label.html.haml b/app/views/shared/_label.html.haml index 6ccdef0df46..db324d8868e 100644 --- a/app/views/shared/_label.html.haml +++ b/app/views/shared/_label.html.haml @@ -1,6 +1,7 @@ - label_css_id = dom_id(label) - open_issues_count = label.open_issues_count(current_user) - open_merge_requests_count = label.open_merge_requests_count(current_user) +- status = label_subscription_status(label, @project).inquiry if current_user - subject = local_assigns[:subject] %li{id: label_css_id, data: { id: label.id } } @@ -18,10 +19,19 @@ %li = link_to_label(label, subject: subject) do = pluralize open_issues_count, 'open issue' - - if current_user - %li.label-subscription{ data: toggle_subscription_data(label) } - %a.js-subscribe-button.label-subscribe-button.subscription-status{ role: "button", href: "#", data: { toggle: "tooltip", status: label_subscription_status(label) } } - %span= label_subscription_toggle_button_text(label) + - if current_user && defined?(@project) + %li.label-subscription + - if label.is_a?(ProjectLabel) + %a.js-subscribe-button.label-subscribe-button{ role: 'button', href: '#', data: { status: status, url: toggle_subscription_namespace_project_label_path(@project.namespace, @project, label) } } + %span= label_subscription_toggle_button_text(label, @project) + - else + %a.js-unsubscribe-button.label-subscribe-button{ role: 'button', href: '#', class: ('hidden' if status.unsubscribed?), data: { url: group_label_unsubscribe_path(label, @project) } } + %span Unsubscribe + %a.js-subscribe-button.label-subscribe-button{ role: 'button', href: '#', class: ('hidden' unless status.unsubscribed?), data: { url: toggle_subscription_namespace_project_label_path(@project.namespace, @project, label) } } + %span Subscribe at project level + %a.js-subscribe-button.label-subscribe-button{ role: 'button', href: '#', class: ('hidden' unless status.unsubscribed?), data: { url: toggle_subscription_group_label_path(label.group, label) } } + %span Subscribe at group level + - if can?(current_user, :admin_label, label) %li = link_to 'Edit', edit_label_path(label) @@ -34,12 +44,27 @@ = link_to_label(label, subject: subject, css_class: 'btn btn-transparent btn-action') do = pluralize open_issues_count, 'open issue' - - if current_user - .label-subscription.inline{ data: toggle_subscription_data(label) } - %button.js-subscribe-button.label-subscribe-button.btn.btn-transparent.btn-action.subscription-status{ type: "button", title: label_subscription_toggle_button_text(label), data: { toggle: "tooltip", status: label_subscription_status(label) } } - %span.sr-only= label_subscription_toggle_button_text(label) - = icon('eye', class: 'label-subscribe-button-icon', disabled: label.is_a?(GroupLabel)) - = icon('spinner spin', class: 'label-subscribe-button-loading') + - if current_user && defined?(@project) + .label-subscription.inline + - if label.is_a?(ProjectLabel) + %button.js-subscribe-button.label-subscribe-button.btn.btn-default.btn-action{ type: 'button', title: label_subscription_toggle_button_text(label, @project), data: { toggle: 'tooltip', status: status, url: toggle_subscription_namespace_project_label_path(@project.namespace, @project, label) } } + %span= label_subscription_toggle_button_text(label, @project) + = icon('spinner spin', class: 'label-subscribe-button-loading') + - else + %button.js-unsubscribe-button.label-subscribe-button.btn.btn-default.btn-action{ type: 'button', class: ('hidden' if status.unsubscribed?), title: 'Unsubscribe', data: { toggle: 'tooltip', url: group_label_unsubscribe_path(label, @project) } } + %span Unsubscribe + = icon('spinner spin', class: 'label-subscribe-button-loading') + + .dropdown.dropdown-group-label{ class: ('hidden' unless status.unsubscribed?) } + %button.dropdown-menu-toggle{type: 'button', 'data-toggle' => 'dropdown'} + %span Subscribe + = icon('chevron-down') + %ul.dropdown-menu + %li + %a.js-subscribe-button{ data: { url: toggle_subscription_namespace_project_label_path(@project.namespace, @project, label) } } + Project level + %a.js-subscribe-button{ data: { url: toggle_subscription_group_label_path(label.group, label) } } + Group level - if can?(current_user, :admin_label, label) = link_to edit_label_path(label), title: "Edit", class: 'btn btn-transparent btn-action', data: {toggle: "tooltip"} do @@ -49,6 +74,10 @@ %span.sr-only Delete = icon('trash-o') - - if current_user && label.is_a?(ProjectLabel) - :javascript - new Subscription('##{dom_id(label)} .label-subscription'); + - if current_user && defined?(@project) + - if label.is_a?(ProjectLabel) + :javascript + new gl.ProjectLabelSubscription('##{dom_id(label)} .label-subscription'); + - else + :javascript + new gl.GroupLabelSubscription('##{dom_id(label)} .label-subscription'); diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml index 7363ead09ff..f166fac105d 100644 --- a/app/views/shared/issuable/_sidebar.html.haml +++ b/app/views/shared/issuable/_sidebar.html.haml @@ -140,7 +140,7 @@ = render "shared/issuable/participants", participants: issuable.participants(current_user) - if current_user - - subscribed = issuable.subscribed?(current_user) + - subscribed = issuable.subscribed?(current_user, @project) .block.light.subscription{data: {url: toggle_subscription_path(issuable)}} .sidebar-collapsed-icon = icon('rss') diff --git a/changelogs/unreleased/feature-subscribe-to-group-level-labels.yml b/changelogs/unreleased/feature-subscribe-to-group-level-labels.yml new file mode 100644 index 00000000000..ea336716dce --- /dev/null +++ b/changelogs/unreleased/feature-subscribe-to-group-level-labels.yml @@ -0,0 +1,4 @@ +--- +title: Allow users to subscribe to group labels +merge_request: 7215 +author: diff --git a/config/routes/group.rb b/config/routes/group.rb index 3c392f77ef6..068e0b6e843 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -30,7 +30,10 @@ scope(path: 'groups/:group_id', module: :groups, as: :group) do resource :avatar, only: [:destroy] resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :update, :new, :create] - resources :labels, except: [:show], constraints: { id: /\d+/ } + + resources :labels, except: [:show], constraints: { id: /\d+/ } do + post :toggle_subscription, on: :member + end end # Must be last route in this file diff --git a/db/migrate/20161031171301_add_project_id_to_subscriptions.rb b/db/migrate/20161031171301_add_project_id_to_subscriptions.rb new file mode 100644 index 00000000000..97534679b59 --- /dev/null +++ b/db/migrate/20161031171301_add_project_id_to_subscriptions.rb @@ -0,0 +1,14 @@ +class AddProjectIdToSubscriptions < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + add_column :subscriptions, :project_id, :integer + add_foreign_key :subscriptions, :projects, column: :project_id, on_delete: :cascade + end + + def down + remove_column :subscriptions, :project_id + end +end diff --git a/db/migrate/20161031174110_migrate_subscriptions_project_id.rb b/db/migrate/20161031174110_migrate_subscriptions_project_id.rb new file mode 100644 index 00000000000..549145a0a65 --- /dev/null +++ b/db/migrate/20161031174110_migrate_subscriptions_project_id.rb @@ -0,0 +1,44 @@ +class MigrateSubscriptionsProjectId < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = true + DOWNTIME_REASON = 'Subscriptions will not work as expected until this migration is complete.' + + def up + execute <<-EOF.strip_heredoc + UPDATE subscriptions + SET project_id = ( + SELECT issues.project_id + FROM issues + WHERE issues.id = subscriptions.subscribable_id + ) + WHERE subscriptions.subscribable_type = 'Issue'; + EOF + + execute <<-EOF.strip_heredoc + UPDATE subscriptions + SET project_id = ( + SELECT merge_requests.target_project_id + FROM merge_requests + WHERE merge_requests.id = subscriptions.subscribable_id + ) + WHERE subscriptions.subscribable_type = 'MergeRequest'; + EOF + + execute <<-EOF.strip_heredoc + UPDATE subscriptions + SET project_id = ( + SELECT projects.id + FROM labels INNER JOIN projects ON projects.id = labels.project_id + WHERE labels.id = subscriptions.subscribable_id + ) + WHERE subscriptions.subscribable_type = 'Label'; + EOF + end + + def down + execute <<-EOF.strip_heredoc + UPDATE subscriptions SET project_id = NULL; + EOF + end +end diff --git a/db/migrate/20161031181638_add_unique_index_to_subscriptions.rb b/db/migrate/20161031181638_add_unique_index_to_subscriptions.rb new file mode 100644 index 00000000000..4b1b29e1265 --- /dev/null +++ b/db/migrate/20161031181638_add_unique_index_to_subscriptions.rb @@ -0,0 +1,18 @@ +class AddUniqueIndexToSubscriptions < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = true + DOWNTIME_REASON = 'This migration requires downtime because it changes a column to not accept null values.' + + disable_ddl_transaction! + + def up + add_concurrent_index :subscriptions, [:subscribable_id, :subscribable_type, :user_id, :project_id], { unique: true, name: 'index_subscriptions_on_subscribable_and_user_id_and_project_id' } + remove_index :subscriptions, name: 'subscriptions_user_id_and_ref_fields' if index_name_exists?(:subscriptions, 'subscriptions_user_id_and_ref_fields', false) + end + + def down + add_concurrent_index :subscriptions, [:subscribable_id, :subscribable_type, :user_id], { unique: true, name: 'subscriptions_user_id_and_ref_fields' } + remove_index :subscriptions, name: 'index_subscriptions_on_subscribable_and_user_id_and_project_id' if index_name_exists?(:subscriptions, 'index_subscriptions_on_subscribable_and_user_id_and_project_id', false) + end +end diff --git a/db/schema.rb b/db/schema.rb index 7b04ade3400..cdd80d1f092 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1070,9 +1070,10 @@ ActiveRecord::Schema.define(version: 20161113184239) do t.boolean "subscribed" t.datetime "created_at" t.datetime "updated_at" + t.integer "project_id" end - add_index "subscriptions", ["subscribable_id", "subscribable_type", "user_id"], name: "subscriptions_user_id_and_ref_fields", unique: true, using: :btree + add_index "subscriptions", ["subscribable_id", "subscribable_type", "user_id", "project_id"], name: "index_subscriptions_on_subscribable_and_user_id_and_project_id", unique: true, using: :btree create_table "taggings", force: :cascade do |t| t.integer "tag_id" @@ -1268,6 +1269,7 @@ ActiveRecord::Schema.define(version: 20161113184239) do add_foreign_key "personal_access_tokens", "users" add_foreign_key "protected_branch_merge_access_levels", "protected_branches" add_foreign_key "protected_branch_push_access_levels", "protected_branches" + add_foreign_key "subscriptions", "projects", on_delete: :cascade add_foreign_key "trending_projects", "projects", on_delete: :cascade add_foreign_key "u2f_registrations", "users" end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 6e370e961c4..33cb6fd3704 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -218,7 +218,7 @@ module API expose :assignee, :author, using: Entities::UserBasic expose :subscribed do |issue, options| - issue.subscribed?(options[:current_user]) + issue.subscribed?(options[:current_user], options[:project] || issue.project) end expose :user_notes_count expose :upvotes, :downvotes @@ -248,7 +248,7 @@ module API expose :diff_head_sha, as: :sha expose :merge_commit_sha expose :subscribed do |merge_request, options| - merge_request.subscribed?(options[:current_user]) + merge_request.subscribed?(options[:current_user], options[:project]) end expose :user_notes_count expose :should_remove_source_branch?, as: :should_remove_source_branch @@ -454,7 +454,7 @@ module API end expose :subscribed do |label, options| - label.subscribed?(options[:current_user]) + label.subscribed?(options[:current_user], options[:project]) end end diff --git a/lib/api/issues.rb b/lib/api/issues.rb index c9689e6f8ef..eea5b91d4f9 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -120,7 +120,7 @@ module API issues = issues.reorder(issuable_order_by => issuable_sort) - present paginate(issues), with: Entities::Issue, current_user: current_user + present paginate(issues), with: Entities::Issue, current_user: current_user, project: user_project end # Get a single project issue @@ -132,7 +132,7 @@ module API # GET /projects/:id/issues/:issue_id get ":id/issues/:issue_id" do @issue = find_project_issue(params[:issue_id]) - present @issue, with: Entities::Issue, current_user: current_user + present @issue, with: Entities::Issue, current_user: current_user, project: user_project end # Create a new project issue @@ -174,7 +174,7 @@ module API end if issue.valid? - present issue, with: Entities::Issue, current_user: current_user + present issue, with: Entities::Issue, current_user: current_user, project: user_project else render_validation_error!(issue) end @@ -217,7 +217,7 @@ module API issue = ::Issues::UpdateService.new(user_project, current_user, attrs).execute(issue) if issue.valid? - present issue, with: Entities::Issue, current_user: current_user + present issue, with: Entities::Issue, current_user: current_user, project: user_project else render_validation_error!(issue) end @@ -239,7 +239,7 @@ module API begin issue = ::Issues::MoveService.new(user_project, current_user).execute(issue, new_project) - present issue, with: Entities::Issue, current_user: current_user + present issue, with: Entities::Issue, current_user: current_user, project: user_project rescue ::Issues::MoveService::MoveError => error render_api_error!(error.message, 400) end diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index f9720786e63..4176c7eec06 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -60,7 +60,7 @@ module API end merge_requests = merge_requests.reorder(issuable_order_by => issuable_sort) - present paginate(merge_requests), with: Entities::MergeRequest, current_user: current_user + present paginate(merge_requests), with: Entities::MergeRequest, current_user: current_user, project: user_project end desc 'Create a merge request' do @@ -87,7 +87,7 @@ module API merge_request = ::MergeRequests::CreateService.new(user_project, current_user, mr_params).execute if merge_request.valid? - present merge_request, with: Entities::MergeRequest, current_user: current_user + present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project else handle_merge_request_errors! merge_request.errors end @@ -120,7 +120,7 @@ module API get path do merge_request = user_project.merge_requests.find(params[:merge_request_id]) authorize! :read_merge_request, merge_request - present merge_request, with: Entities::MergeRequest, current_user: current_user + present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project end desc 'Get the commits of a merge request' do @@ -167,7 +167,7 @@ module API merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, mr_params).execute(merge_request) if merge_request.valid? - present merge_request, with: Entities::MergeRequest, current_user: current_user + present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project else handle_merge_request_errors! merge_request.errors end @@ -212,7 +212,7 @@ module API execute(merge_request) end - present merge_request, with: Entities::MergeRequest, current_user: current_user + present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project end desc 'Cancel merge if "Merge when build succeeds" is enabled' do diff --git a/lib/api/milestones.rb b/lib/api/milestones.rb index ba4a84275bc..937c118779d 100644 --- a/lib/api/milestones.rb +++ b/lib/api/milestones.rb @@ -114,7 +114,7 @@ module API } issues = IssuesFinder.new(current_user, finder_params).execute - present paginate(issues), with: Entities::Issue, current_user: current_user + present paginate(issues), with: Entities::Issue, current_user: current_user, project: user_project end end end diff --git a/lib/api/subscriptions.rb b/lib/api/subscriptions.rb index 00a79c24f96..10749b34004 100644 --- a/lib/api/subscriptions.rb +++ b/lib/api/subscriptions.rb @@ -24,11 +24,11 @@ module API post ":id/#{type}/:subscribable_id/subscription" do resource = instance_exec(params[:subscribable_id], &finder) - if resource.subscribed?(current_user) + if resource.subscribed?(current_user, user_project) not_modified! else - resource.subscribe(current_user) - present resource, with: entity_class, current_user: current_user + resource.subscribe(current_user, user_project) + present resource, with: entity_class, current_user: current_user, project: user_project end end @@ -38,11 +38,11 @@ module API delete ":id/#{type}/:subscribable_id/subscription" do resource = instance_exec(params[:subscribable_id], &finder) - if !resource.subscribed?(current_user) + if !resource.subscribed?(current_user, user_project) not_modified! else - resource.unsubscribe(current_user) - present resource, with: entity_class, current_user: current_user + resource.unsubscribe(current_user, user_project) + present resource, with: entity_class, current_user: current_user, project: user_project end end end diff --git a/spec/controllers/groups/labels_controller_spec.rb b/spec/controllers/groups/labels_controller_spec.rb new file mode 100644 index 00000000000..899d8ebd12b --- /dev/null +++ b/spec/controllers/groups/labels_controller_spec.rb @@ -0,0 +1,22 @@ +require 'spec_helper' + +describe Groups::LabelsController do + let(:group) { create(:group) } + let(:user) { create(:user) } + + before do + group.add_owner(user) + + sign_in(user) + end + + describe 'POST #toggle_subscription' do + it 'allows user to toggle subscription on group labels' do + label = create(:group_label, group: group) + + post :toggle_subscription, group_id: group.to_param, id: label.to_param + + expect(response).to have_http_status(200) + end + end +end diff --git a/spec/controllers/projects/boards/issues_controller_spec.rb b/spec/controllers/projects/boards/issues_controller_spec.rb index cbe0417a4a7..299d2c981d3 100644 --- a/spec/controllers/projects/boards/issues_controller_spec.rb +++ b/spec/controllers/projects/boards/issues_controller_spec.rb @@ -25,7 +25,7 @@ describe Projects::Boards::IssuesController do create(:labeled_issue, project: project, labels: [planning]) create(:labeled_issue, project: project, labels: [development], due_date: Date.tomorrow) create(:labeled_issue, project: project, labels: [development], assignee: johndoe) - issue.subscribe(johndoe) + issue.subscribe(johndoe, project) list_issues user: user, board: board, list: list2 diff --git a/spec/controllers/projects/labels_controller_spec.rb b/spec/controllers/projects/labels_controller_spec.rb index 8faecec0063..ec6cea5c0f4 100644 --- a/spec/controllers/projects/labels_controller_spec.rb +++ b/spec/controllers/projects/labels_controller_spec.rb @@ -72,14 +72,8 @@ describe Projects::LabelsController do end describe 'POST #generate' do - let(:admin) { create(:admin) } - - before do - sign_in(admin) - end - context 'personal project' do - let(:personal_project) { create(:empty_project) } + let(:personal_project) { create(:empty_project, namespace: user.namespace) } it 'creates labels' do post :generate, namespace_id: personal_project.namespace.to_param, project_id: personal_project.to_param @@ -96,4 +90,26 @@ describe Projects::LabelsController do end end end + + describe 'POST #toggle_subscription' do + it 'allows user to toggle subscription on project labels' do + label = create(:label, project: project) + + toggle_subscription(label) + + expect(response).to have_http_status(200) + end + + it 'allows user to toggle subscription on group labels' do + group_label = create(:group_label, group: group) + + toggle_subscription(group_label) + + expect(response).to have_http_status(200) + end + + def toggle_subscription(label) + post :toggle_subscription, namespace_id: project.namespace.to_param, project_id: project.to_param, id: label.to_param + end + end end diff --git a/spec/controllers/sent_notifications_controller_spec.rb b/spec/controllers/sent_notifications_controller_spec.rb index 191e290a118..954fc2eaf21 100644 --- a/spec/controllers/sent_notifications_controller_spec.rb +++ b/spec/controllers/sent_notifications_controller_spec.rb @@ -3,11 +3,11 @@ require 'rails_helper' describe SentNotificationsController, type: :controller do let(:user) { create(:user) } let(:project) { create(:empty_project) } - let(:sent_notification) { create(:sent_notification, noteable: issue, recipient: user) } + let(:sent_notification) { create(:sent_notification, project: project, noteable: issue, recipient: user) } let(:issue) do create(:issue, project: project, author: user) do |issue| - issue.subscriptions.create(user: user, subscribed: true) + issue.subscriptions.create(user: user, project: project, subscribed: true) end end @@ -17,7 +17,7 @@ describe SentNotificationsController, type: :controller do before { get(:unsubscribe, id: sent_notification.reply_key, force: true) } it 'unsubscribes the user' do - expect(issue.subscribed?(user)).to be_falsey + expect(issue.subscribed?(user, project)).to be_falsey end it 'sets the flash message' do @@ -33,7 +33,7 @@ describe SentNotificationsController, type: :controller do before { get(:unsubscribe, id: sent_notification.reply_key) } it 'does not unsubscribe the user' do - expect(issue.subscribed?(user)).to be_truthy + expect(issue.subscribed?(user, project)).to be_truthy end it 'does not set the flash message' do @@ -53,7 +53,7 @@ describe SentNotificationsController, type: :controller do before { get(:unsubscribe, id: sent_notification.reply_key.reverse) } it 'does not unsubscribe the user' do - expect(issue.subscribed?(user)).to be_truthy + expect(issue.subscribed?(user, project)).to be_truthy end it 'does not set the flash message' do @@ -69,7 +69,7 @@ describe SentNotificationsController, type: :controller do before { get(:unsubscribe, id: sent_notification.reply_key, force: true) } it 'unsubscribes the user' do - expect(issue.subscribed?(user)).to be_falsey + expect(issue.subscribed?(user, project)).to be_falsey end it 'sets the flash message' do @@ -85,14 +85,14 @@ describe SentNotificationsController, type: :controller do context 'when the force param is not passed' do let(:merge_request) do create(:merge_request, source_project: project, author: user) do |merge_request| - merge_request.subscriptions.create(user: user, subscribed: true) + merge_request.subscriptions.create(user: user, project: project, subscribed: true) end end - let(:sent_notification) { create(:sent_notification, noteable: merge_request, recipient: user) } + let(:sent_notification) { create(:sent_notification, project: project, noteable: merge_request, recipient: user) } before { get(:unsubscribe, id: sent_notification.reply_key) } it 'unsubscribes the user' do - expect(merge_request.subscribed?(user)).to be_falsey + expect(merge_request.subscribed?(user, project)).to be_falsey end it 'sets the flash message' do diff --git a/spec/factories/subscriptions.rb b/spec/factories/subscriptions.rb new file mode 100644 index 00000000000..b11b0a0a17b --- /dev/null +++ b/spec/factories/subscriptions.rb @@ -0,0 +1,7 @@ +FactoryGirl.define do + factory :subscription do + user + project factory: :empty_project + subscribable factory: :issue + end +end diff --git a/spec/features/projects/labels/subscription_spec.rb b/spec/features/projects/labels/subscription_spec.rb new file mode 100644 index 00000000000..3130d87fba5 --- /dev/null +++ b/spec/features/projects/labels/subscription_spec.rb @@ -0,0 +1,74 @@ +require 'spec_helper' + +feature 'Labels subscription', feature: true do + let(:user) { create(:user) } + let(:group) { create(:group) } + let(:project) { create(:empty_project, :public, namespace: group) } + let!(:bug) { create(:label, project: project, title: 'bug') } + let!(:feature) { create(:group_label, group: group, title: 'feature') } + + context 'when signed in' do + before do + project.team << [user, :developer] + login_as user + end + + scenario 'users can subscribe/unsubscribe to labels', js: true do + visit namespace_project_labels_path(project.namespace, project) + + expect(page).to have_content('bug') + expect(page).to have_content('feature') + + within "#project_label_#{bug.id}" do + expect(page).not_to have_button 'Unsubscribe' + + click_button 'Subscribe' + + expect(page).not_to have_button 'Subscribe' + expect(page).to have_button 'Unsubscribe' + + click_button 'Unsubscribe' + + expect(page).to have_button 'Subscribe' + expect(page).not_to have_button 'Unsubscribe' + end + + within "#group_label_#{feature.id}" do + expect(page).not_to have_button 'Unsubscribe' + + click_link_on_dropdown('Group level') + + expect(page).not_to have_selector('.dropdown-group-label') + expect(page).to have_button 'Unsubscribe' + + click_button 'Unsubscribe' + + expect(page).to have_selector('.dropdown-group-label') + + click_link_on_dropdown('Project level') + + expect(page).not_to have_selector('.dropdown-group-label') + expect(page).to have_button 'Unsubscribe' + end + end + end + + context 'when not signed in' do + it 'users can not subscribe/unsubscribe to labels' do + visit namespace_project_labels_path(project.namespace, project) + + expect(page).to have_content 'bug' + expect(page).to have_content 'feature' + expect(page).not_to have_button('Subscribe') + expect(page).not_to have_selector('.dropdown-group-label') + end + end + + def click_link_on_dropdown(text) + find('.dropdown-group-label').click + + page.within('.dropdown-group-label') do + find('a.js-subscribe-button', text: text).click + end + end +end diff --git a/spec/features/unsubscribe_links_spec.rb b/spec/features/unsubscribe_links_spec.rb index 33b52d1547e..e2d9cfdd0b0 100644 --- a/spec/features/unsubscribe_links_spec.rb +++ b/spec/features/unsubscribe_links_spec.rb @@ -26,11 +26,11 @@ describe 'Unsubscribe links', feature: true do expect(current_path).to eq unsubscribe_sent_notification_path(SentNotification.last) expect(page).to have_text(%(Unsubscribe from issue #{issue.title} (#{issue.to_reference}))) expect(page).to have_text(%(Are you sure you want to unsubscribe from issue #{issue.title} (#{issue.to_reference})?)) - expect(issue.subscribed?(recipient)).to be_truthy + expect(issue.subscribed?(recipient, project)).to be_truthy click_link 'Unsubscribe' - expect(issue.subscribed?(recipient)).to be_falsey + expect(issue.subscribed?(recipient, project)).to be_falsey expect(current_path).to eq new_user_session_path end @@ -38,11 +38,11 @@ describe 'Unsubscribe links', feature: true do visit body_link expect(current_path).to eq unsubscribe_sent_notification_path(SentNotification.last) - expect(issue.subscribed?(recipient)).to be_truthy + expect(issue.subscribed?(recipient, project)).to be_truthy click_link 'Cancel' - expect(issue.subscribed?(recipient)).to be_truthy + expect(issue.subscribed?(recipient, project)).to be_truthy expect(current_path).to eq new_user_session_path end end @@ -51,7 +51,7 @@ describe 'Unsubscribe links', feature: true do visit header_link expect(page).to have_text('unsubscribed') - expect(issue.subscribed?(recipient)).to be_falsey + expect(issue.subscribed?(recipient, project)).to be_falsey end end @@ -62,14 +62,14 @@ describe 'Unsubscribe links', feature: true do visit body_link expect(page).to have_text('unsubscribed') - expect(issue.subscribed?(recipient)).to be_falsey + expect(issue.subscribed?(recipient, project)).to be_falsey end it 'unsubscribes from the issue when visiting the link from the header' do visit header_link expect(page).to have_text('unsubscribed') - expect(issue.subscribed?(recipient)).to be_falsey + expect(issue.subscribed?(recipient, project)).to be_falsey end end end diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 6e987967ca5..6f84bffe046 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -176,23 +176,25 @@ describe Issue, "Issuable" do end describe '#subscribed?' do + let(:project) { issue.project } + 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 + expect(issue.subscribed?(user, project)).to be_falsey end it 'returns true when a subcription exists and subscribed is true' do - issue.subscriptions.create(user: user, subscribed: true) + issue.subscriptions.create(user: user, project: project, subscribed: true) - expect(issue.subscribed?(user)).to be_truthy + expect(issue.subscribed?(user, project)).to be_truthy end it 'returns false when a subcription exists and subscribed is false' do - issue.subscriptions.create(user: user, subscribed: false) + issue.subscriptions.create(user: user, project: project, subscribed: false) - expect(issue.subscribed?(user)).to be_falsey + expect(issue.subscribed?(user, project)).to be_falsey end end @@ -200,19 +202,19 @@ describe Issue, "Issuable" 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 + expect(issue.subscribed?(user, project)).to be_truthy end it 'returns true when a subcription exists and subscribed is true' do - issue.subscriptions.create(user: user, subscribed: true) + issue.subscriptions.create(user: user, project: project, subscribed: true) - expect(issue.subscribed?(user)).to be_truthy + expect(issue.subscribed?(user, project)).to be_truthy end it 'returns false when a subcription exists and subscribed is false' do - issue.subscriptions.create(user: user, subscribed: false) + issue.subscriptions.create(user: user, project: project, subscribed: false) - expect(issue.subscribed?(user)).to be_falsey + expect(issue.subscribed?(user, project)).to be_falsey end end end diff --git a/spec/models/concerns/subscribable_spec.rb b/spec/models/concerns/subscribable_spec.rb index b7fc5a92497..58f5c164116 100644 --- a/spec/models/concerns/subscribable_spec.rb +++ b/spec/models/concerns/subscribable_spec.rb @@ -1,67 +1,128 @@ require 'spec_helper' describe Subscribable, 'Subscribable' do - let(:resource) { create(:issue) } - let(:user) { create(:user) } + let(:project) { create(:empty_project) } + let(:resource) { create(:issue, project: project) } + let(:user_1) { create(:user) } describe '#subscribed?' do - it 'returns false when no subcription exists' do - expect(resource.subscribed?(user)).to be_falsey - end + context 'without project' do + it 'returns false when no subscription exists' do + expect(resource.subscribed?(user_1)).to be_falsey + end + + it 'returns true when a subcription exists and subscribed is true' do + resource.subscriptions.create(user: user_1, subscribed: true) + + expect(resource.subscribed?(user_1)).to be_truthy + end - it 'returns true when a subcription exists and subscribed is true' do - resource.subscriptions.create(user: user, subscribed: true) + it 'returns false when a subcription exists and subscribed is false' do + resource.subscriptions.create(user: user_1, subscribed: false) - expect(resource.subscribed?(user)).to be_truthy + expect(resource.subscribed?(user_1)).to be_falsey + end end - it 'returns false when a subcription exists and subscribed is false' do - resource.subscriptions.create(user: user, subscribed: false) + context 'with project' do + it 'returns false when no subscription exists' do + expect(resource.subscribed?(user_1, project)).to be_falsey + end + + it 'returns true when a subcription exists and subscribed is true' do + resource.subscriptions.create(user: user_1, project: project, subscribed: true) + + expect(resource.subscribed?(user_1, project)).to be_truthy + end - expect(resource.subscribed?(user)).to be_falsey + it 'returns false when a subcription exists and subscribed is false' do + resource.subscriptions.create(user: user_1, project: project, subscribed: false) + + expect(resource.subscribed?(user_1, project)).to be_falsey + end end end + describe '#subscribers' do it 'returns [] when no subcribers exists' do - expect(resource.subscribers).to be_empty + expect(resource.subscribers(project)).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) + user_2 = create(:user) + resource.subscriptions.create(user: user_1, subscribed: true) + resource.subscriptions.create(user: user_2, project: project, subscribed: true) + resource.subscriptions.create(user: create(:user), project: project, subscribed: false) - expect(resource.subscribers).to eq [user] + expect(resource.subscribers(project)).to contain_exactly(user_1, user_2) end end describe '#toggle_subscription' do - it 'toggles the current subscription state for the given user' do - expect(resource.subscribed?(user)).to be_falsey + context 'without project' do + it 'toggles the current subscription state for the given user' do + expect(resource.subscribed?(user_1)).to be_falsey - resource.toggle_subscription(user) + resource.toggle_subscription(user_1) - expect(resource.subscribed?(user)).to be_truthy + expect(resource.subscribed?(user_1)).to be_truthy + end + end + + context 'with project' do + it 'toggles the current subscription state for the given user' do + expect(resource.subscribed?(user_1, project)).to be_falsey + + resource.toggle_subscription(user_1, project) + + expect(resource.subscribed?(user_1, project)).to be_truthy + end end end describe '#subscribe' do - it 'subscribes the given user' do - expect(resource.subscribed?(user)).to be_falsey + context 'without project' do + it 'subscribes the given user' do + expect(resource.subscribed?(user_1)).to be_falsey + + resource.subscribe(user_1) + + expect(resource.subscribed?(user_1)).to be_truthy + end + end + + context 'with project' do + it 'subscribes the given user' do + expect(resource.subscribed?(user_1, project)).to be_falsey - resource.subscribe(user) + resource.subscribe(user_1, project) - expect(resource.subscribed?(user)).to be_truthy + expect(resource.subscribed?(user_1, project)).to be_truthy + end 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 + context 'without project' do + it 'unsubscribes the given current user' do + resource.subscriptions.create(user: user_1, subscribed: true) + expect(resource.subscribed?(user_1)).to be_truthy + + resource.unsubscribe(user_1) + + expect(resource.subscribed?(user_1)).to be_falsey + end + end + + context 'with project' do + it 'unsubscribes the given current user' do + resource.subscriptions.create(user: user_1, project: project, subscribed: true) + expect(resource.subscribed?(user_1, project)).to be_truthy - resource.unsubscribe(user) + resource.unsubscribe(user_1, project) - expect(resource.subscribed?(user)).to be_falsey + expect(resource.subscribed?(user_1, project)).to be_falsey + end end end end diff --git a/spec/models/subscription_spec.rb b/spec/models/subscription_spec.rb new file mode 100644 index 00000000000..9ab112bb2ee --- /dev/null +++ b/spec/models/subscription_spec.rb @@ -0,0 +1,20 @@ +require 'spec_helper' + +describe Subscription, models: true do + describe 'relationships' do + it { is_expected.to belong_to(:project) } + it { is_expected.to belong_to(:subscribable) } + it { is_expected.to belong_to(:user) } + end + + describe 'validations' do + it { is_expected.to validate_presence_of(:subscribable) } + it { is_expected.to validate_presence_of(:user) } + + it 'validates uniqueness of project_id scoped to subscribable_id, subscribable_type, and user_id' do + create(:subscription) + + expect(subject).to validate_uniqueness_of(:project_id).scoped_to([:subscribable_id, :subscribable_type, :user_id]) + end + end +end diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index beed53d1e5c..7bae055b241 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -637,7 +637,7 @@ describe API::API, api: true do it "sends notifications for subscribers of newly added labels" do label = project.labels.first - label.toggle_subscription(user2) + label.toggle_subscription(user2, project) perform_enqueued_jobs do post api("/projects/#{project.id}/issues", user), @@ -828,7 +828,7 @@ describe API::API, api: true do it "sends notifications for subscribers of newly added labels when issue is updated" do label = create(:label, title: 'foo', color: '#FFAABB', project: project) - label.toggle_subscription(user2) + label.toggle_subscription(user2, project) perform_enqueued_jobs do put api("/projects/#{project.id}/issues/#{issue.id}", user), diff --git a/spec/requests/api/labels_spec.rb b/spec/requests/api/labels_spec.rb index 77dfebf1a98..aaf41639277 100644 --- a/spec/requests/api/labels_spec.rb +++ b/spec/requests/api/labels_spec.rb @@ -339,7 +339,7 @@ describe API::API, api: true do end context "when user is already subscribed to label" do - before { label1.subscribe(user) } + before { label1.subscribe(user, project) } it "returns 304" do post api("/projects/#{project.id}/labels/#{label1.id}/subscription", user) @@ -358,7 +358,7 @@ describe API::API, api: true do end describe "DELETE /projects/:id/labels/:label_id/subscription" do - before { label1.subscribe(user) } + before { label1.subscribe(user, project) } context "when label_id is a label title" do it "unsubscribes from the label" do @@ -381,7 +381,7 @@ describe API::API, api: true do end context "when user is already unsubscribed from label" do - before { label1.unsubscribe(user) } + before { label1.unsubscribe(user, project) } it "returns 304" do delete api("/projects/#{project.id}/labels/#{label1.id}/subscription", user) diff --git a/spec/services/issuable/bulk_update_service_spec.rb b/spec/services/issuable/bulk_update_service_spec.rb index 6f7ce8ca992..5f3020b6525 100644 --- a/spec/services/issuable/bulk_update_service_spec.rb +++ b/spec/services/issuable/bulk_update_service_spec.rb @@ -260,14 +260,14 @@ describe Issuable::BulkUpdateService, services: true do it 'subscribes the given user' do bulk_update(issues, subscription_event: 'subscribe') - expect(issues).to all(be_subscribed(user)) + expect(issues).to all(be_subscribed(user, project)) end end describe 'unsubscribe from issues' do let(:issues) do create_list(:closed_issue, 2, project: project) do |issue| - issue.subscriptions.create(user: user, subscribed: true) + issue.subscriptions.create(user: user, project: project, subscribed: true) end end @@ -275,7 +275,7 @@ describe Issuable::BulkUpdateService, services: true do bulk_update(issues, subscription_event: 'unsubscribe') issues.each do |issue| - expect(issue).not_to be_subscribed(user) + expect(issue).not_to be_subscribed(user, project) end end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 1638a46ed51..4777a90639e 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -215,7 +215,7 @@ describe Issues::UpdateService, services: true do let!(:subscriber) do create(:user).tap do |u| - label.toggle_subscription(u) + label.toggle_subscription(u, project) project.team << [u, :developer] end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 2433a7dad06..cb5d7cdb467 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -199,7 +199,7 @@ describe MergeRequests::UpdateService, services: true do context 'when the issue is relabeled' do let!(:non_subscriber) { create(:user) } - let!(:subscriber) { create(:user).tap { |u| label.toggle_subscription(u) } } + let!(:subscriber) { create(:user) { |u| label.toggle_subscription(u, project) } } before do project.team << [non_subscriber, :developer] diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 8ce35354c22..8726c9eaa55 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -342,7 +342,9 @@ describe NotificationService, services: true do end describe 'Issues' do - let(:project) { create(:empty_project, :public) } + let(:group) { create(:group) } + let(:project) { create(:empty_project, :public, namespace: group) } + let(:another_project) { create(:empty_project, :public, namespace: group) } let(:issue) { create :issue, project: project, assignee: create(:user), description: 'cc @participant' } before do @@ -377,13 +379,24 @@ describe NotificationService, services: true do end it "emails subscribers of the issue's labels" do - subscriber = create(:user) - label = create(:label, issues: [issue]) + user_1 = create(:user) + user_2 = create(:user) + user_3 = create(:user) + user_4 = create(:user) + label = create(:label, project: project, issues: [issue]) + group_label = create(:group_label, group: group, issues: [issue]) issue.reload - label.toggle_subscription(subscriber) + label.toggle_subscription(user_1, project) + group_label.toggle_subscription(user_2, project) + group_label.toggle_subscription(user_3, another_project) + group_label.toggle_subscription(user_4) + notification.new_issue(issue, @u_disabled) - should_email(subscriber) + should_email(user_1) + should_email(user_2) + should_not_email(user_3) + should_email(user_4) end context 'confidential issues' do @@ -399,14 +412,14 @@ describe NotificationService, services: true do project.team << [member, :developer] project.team << [guest, :guest] - label = create(:label, issues: [confidential_issue]) + label = create(:label, project: project, issues: [confidential_issue]) confidential_issue.reload - label.toggle_subscription(non_member) - label.toggle_subscription(author) - label.toggle_subscription(assignee) - label.toggle_subscription(member) - label.toggle_subscription(guest) - label.toggle_subscription(admin) + label.toggle_subscription(non_member, project) + label.toggle_subscription(author, project) + label.toggle_subscription(assignee, project) + label.toggle_subscription(member, project) + label.toggle_subscription(guest, project) + label.toggle_subscription(admin, project) reset_delivered_emails! @@ -554,20 +567,30 @@ describe NotificationService, services: true do 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) } } + let(:group_label_1) { create(:group_label, group: group, title: 'Group Label 1', issues: [issue]) } + let(:group_label_2) { create(:group_label, group: group, title: 'Group Label 2') } + let(:label_1) { create(:label, project: project, title: 'Label 1', issues: [issue]) } + let(:label_2) { create(:label, project: project, title: 'Label 2') } + let!(:subscriber_to_group_label_1) { create(:user) { |u| group_label_1.toggle_subscription(u, project) } } + let!(:subscriber_1_to_group_label_2) { create(:user) { |u| group_label_2.toggle_subscription(u, project) } } + let!(:subscriber_2_to_group_label_2) { create(:user) { |u| group_label_2.toggle_subscription(u) } } + let!(:subscriber_to_group_label_2_on_another_project) { create(:user) { |u| group_label_2.toggle_subscription(u, another_project) } } + let!(:subscriber_to_label_1) { create(:user) { |u| label_1.toggle_subscription(u, project) } } + let!(:subscriber_to_label_2) { create(:user) { |u| label_2.toggle_subscription(u, project) } } 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) + notification.relabeled_issue(issue, [group_label_2, label_2], @u_disabled) + + should_not_email(subscriber_to_label_1) + should_not_email(subscriber_to_group_label_1) + should_not_email(subscriber_to_group_label_2_on_another_project) + should_email(subscriber_1_to_group_label_2) + should_email(subscriber_2_to_group_label_2) + should_email(subscriber_to_label_2) end it "doesn't send email to anyone but subscribers of the given labels" do - notification.relabeled_issue(issue, [label2], @u_disabled) + notification.relabeled_issue(issue, [group_label_2, label_2], @u_disabled) should_not_email(issue.assignee) should_not_email(issue.author) @@ -578,8 +601,12 @@ describe NotificationService, services: true do 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) + should_not_email(subscriber_to_label_1) + should_not_email(subscriber_to_group_label_1) + should_not_email(subscriber_to_group_label_2_on_another_project) + should_email(subscriber_1_to_group_label_2) + should_email(subscriber_2_to_group_label_2) + should_email(subscriber_to_label_2) end context 'confidential issues' do @@ -590,19 +617,19 @@ describe NotificationService, services: true do let(:guest) { create(:user) } let(:admin) { create(:admin) } let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignee: assignee) } - let!(:label_1) { create(:label, issues: [confidential_issue]) } - let!(:label_2) { create(:label) } + let!(:label_1) { create(:label, project: project, issues: [confidential_issue]) } + let!(:label_2) { create(:label, project: project) } it "emails subscribers of the issue's labels that can read the issue" do project.team << [member, :developer] project.team << [guest, :guest] - label_2.toggle_subscription(non_member) - label_2.toggle_subscription(author) - label_2.toggle_subscription(assignee) - label_2.toggle_subscription(member) - label_2.toggle_subscription(guest) - label_2.toggle_subscription(admin) + label_2.toggle_subscription(non_member, project) + label_2.toggle_subscription(author, project) + label_2.toggle_subscription(assignee, project) + label_2.toggle_subscription(member, project) + label_2.toggle_subscription(guest, project) + label_2.toggle_subscription(admin, project) reset_delivered_emails! @@ -725,7 +752,9 @@ describe NotificationService, services: true do end describe 'Merge Requests' do - let(:project) { create(:project, :public) } + let(:group) { create(:group) } + let(:project) { create(:project, :public, namespace: group) } + let(:another_project) { create(:empty_project, :public, namespace: group) } let(:merge_request) { create :merge_request, source_project: project, assignee: create(:user), description: 'cc @participant' } before do @@ -758,12 +787,23 @@ describe NotificationService, services: true do end it "emails subscribers of the merge request's labels" do - subscriber = create(:user) - label = create(:label, merge_requests: [merge_request]) - label.toggle_subscription(subscriber) + user_1 = create(:user) + user_2 = create(:user) + user_3 = create(:user) + user_4 = create(:user) + label = create(:label, project: project, merge_requests: [merge_request]) + group_label = create(:group_label, group: group, merge_requests: [merge_request]) + label.toggle_subscription(user_1, project) + group_label.toggle_subscription(user_2, project) + group_label.toggle_subscription(user_3, another_project) + group_label.toggle_subscription(user_4) + notification.new_merge_request(merge_request, @u_disabled) - should_email(subscriber) + should_email(user_1) + should_email(user_2) + should_not_email(user_3) + should_email(user_4) end context 'participating' do @@ -857,20 +897,30 @@ describe NotificationService, services: true do 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) } } + let(:group_label_1) { create(:group_label, group: group, title: 'Group Label 1', merge_requests: [merge_request]) } + let(:group_label_2) { create(:group_label, group: group, title: 'Group Label 2') } + let(:label_1) { create(:label, project: project, title: 'Label 1', merge_requests: [merge_request]) } + let(:label_2) { create(:label, project: project, title: 'Label 2') } + let!(:subscriber_to_group_label_1) { create(:user) { |u| group_label_1.toggle_subscription(u, project) } } + let!(:subscriber_1_to_group_label_2) { create(:user) { |u| group_label_2.toggle_subscription(u, project) } } + let!(:subscriber_2_to_group_label_2) { create(:user) { |u| group_label_2.toggle_subscription(u) } } + let!(:subscriber_to_group_label_2_on_another_project) { create(:user) { |u| group_label_2.toggle_subscription(u, another_project) } } + let!(:subscriber_to_label_1) { create(:user) { |u| label_1.toggle_subscription(u, project) } } + let!(:subscriber_to_label_2) { create(:user) { |u| label_2.toggle_subscription(u, project) } } 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) + notification.relabeled_merge_request(merge_request, [group_label_2, label_2], @u_disabled) + + should_not_email(subscriber_to_label_1) + should_not_email(subscriber_to_group_label_1) + should_not_email(subscriber_to_group_label_2_on_another_project) + should_email(subscriber_1_to_group_label_2) + should_email(subscriber_2_to_group_label_2) + should_email(subscriber_to_label_2) end it "doesn't send email to anyone but subscribers of the given labels" do - notification.relabeled_merge_request(merge_request, [label2], @u_disabled) + notification.relabeled_merge_request(merge_request, [group_label_2, label_2], @u_disabled) should_not_email(merge_request.assignee) should_not_email(merge_request.author) @@ -881,8 +931,12 @@ describe NotificationService, services: true do should_not_email(@unsubscriber) should_not_email(@u_participating) should_not_email(@u_lazy_participant) - should_not_email(subscriber_to_label) - should_email(subscriber_to_label2) + should_not_email(subscriber_to_label_1) + should_not_email(subscriber_to_group_label_1) + should_not_email(subscriber_to_group_label_2_on_another_project) + should_email(subscriber_1_to_group_label_2) + should_email(subscriber_2_to_group_label_2) + should_email(subscriber_to_label_2) end end @@ -1290,10 +1344,10 @@ describe NotificationService, services: true do project.team << [@unsubscriber, :master] project.team << [@watcher_and_subscriber, :master] - issuable.subscriptions.create(user: @subscriber, subscribed: true) - issuable.subscriptions.create(user: @subscribed_participant, subscribed: true) - issuable.subscriptions.create(user: @unsubscriber, subscribed: false) + issuable.subscriptions.create(user: @subscriber, project: project, subscribed: true) + issuable.subscriptions.create(user: @subscribed_participant, project: project, subscribed: true) + issuable.subscriptions.create(user: @unsubscriber, project: project, subscribed: false) # Make the watcher a subscriber to detect dupes - issuable.subscriptions.create(user: @watcher_and_subscriber, subscribed: true) + issuable.subscriptions.create(user: @watcher_and_subscriber, project: project, subscribed: true) end end diff --git a/spec/services/slash_commands/interpret_service_spec.rb b/spec/services/slash_commands/interpret_service_spec.rb index b57e338b782..becf627a4f5 100644 --- a/spec/services/slash_commands/interpret_service_spec.rb +++ b/spec/services/slash_commands/interpret_service_spec.rb @@ -169,7 +169,7 @@ describe SlashCommands::InterpretService, services: true do shared_examples 'unsubscribe command' do it 'populates subscription_event: "unsubscribe" if content contains /unsubscribe' do - issuable.subscribe(developer) + issuable.subscribe(developer, project) _, updates = service.execute(content, issuable) expect(updates).to eq(subscription_event: 'unsubscribe') @@ -321,7 +321,7 @@ describe SlashCommands::InterpretService, services: true do it_behaves_like 'multiple label with same argument' do let(:content) { %(/label ~"#{inprogress.title}" \n/label ~#{inprogress.title}) } let(:issuable) { issue } - end + end it_behaves_like 'unlabel command' do let(:content) { %(/unlabel ~"#{inprogress.title}") } diff --git a/spec/support/features/issuable_slash_commands_shared_examples.rb b/spec/support/features/issuable_slash_commands_shared_examples.rb index 5e3b8f2b23e..194620d0a68 100644 --- a/spec/support/features/issuable_slash_commands_shared_examples.rb +++ b/spec/support/features/issuable_slash_commands_shared_examples.rb @@ -230,31 +230,31 @@ shared_examples 'issuable record that supports slash commands in its description context "with a note subscribing to the #{issuable_type}" do it "creates a new todo for the #{issuable_type}" do - expect(issuable.subscribed?(master)).to be_falsy + expect(issuable.subscribed?(master, project)).to be_falsy write_note("/subscribe") expect(page).not_to have_content '/subscribe' expect(page).to have_content 'Your commands have been executed!' - expect(issuable.subscribed?(master)).to be_truthy + expect(issuable.subscribed?(master, project)).to be_truthy end end context "with a note unsubscribing to the #{issuable_type} as done" do before do - issuable.subscribe(master) + issuable.subscribe(master, project) end it "creates a new todo for the #{issuable_type}" do - expect(issuable.subscribed?(master)).to be_truthy + expect(issuable.subscribed?(master, project)).to be_truthy write_note("/unsubscribe") expect(page).not_to have_content '/unsubscribe' expect(page).to have_content 'Your commands have been executed!' - expect(issuable.subscribed?(master)).to be_falsy + expect(issuable.subscribed?(master, project)).to be_falsy end end end |