diff --git a/app/assets/javascripts/dispatcher.js.coffee b/app/assets/javascripts/dispatcher.js.coffee index ad0d2617294..404101710b9 100644 --- a/app/assets/javascripts/dispatcher.js.coffee +++ b/app/assets/javascripts/dispatcher.js.coffee @@ -126,6 +126,8 @@ class Dispatcher shortcut_handler = new ShortcutsDashboardNavigation() when 'profiles' new Profile() + new NotificationsForm() + new NotificationsDropdown() when 'projects' new Project() new ProjectAvatar() @@ -139,6 +141,7 @@ class Dispatcher new ProjectNew() when 'show' new ProjectShow() + new NotificationsDropdown() when 'wikis' new Wikis() shortcut_handler = new ShortcutsNavigation() diff --git a/app/assets/javascripts/notifications_dropdown.js.coffee b/app/assets/javascripts/notifications_dropdown.js.coffee new file mode 100644 index 00000000000..15daf027c0a --- /dev/null +++ b/app/assets/javascripts/notifications_dropdown.js.coffee @@ -0,0 +1,21 @@ +class @NotificationsDropdown + $ -> + $(document) + .off 'click', '.update-notification' + .on 'click', '.update-notification', (e) -> + e.preventDefault() + notificationLevel = $(@).data 'notification-level' + label = $(@).data 'notification-title' + form = $(this).parents('form:first') + form.find('.js-notification-loading').toggleClass 'fa-bell fa-spin fa-spinner' + form.find('#notification_setting_level').val(notificationLevel) + form.submit(); + + $(document) + .off 'ajax:success', '#notification-form' + .on 'ajax:success', '#notification-form', (e, data) -> + if data.saved + new Flash('Notification settings saved', 'notice') + $(e.currentTarget).closest('.notification-dropdown').replaceWith(data.html) + else + new Flash('Failed to save new settings', 'alert') diff --git a/app/assets/javascripts/notifications_form.js.coffee b/app/assets/javascripts/notifications_form.js.coffee index cfe8e133b66..3432428702a 100644 --- a/app/assets/javascripts/notifications_form.js.coffee +++ b/app/assets/javascripts/notifications_form.js.coffee @@ -1,7 +1,5 @@ class @NotificationsForm constructor: -> - @form = $('.custom-notifications-form') - @removeEventListeners() @initEventListeners() @@ -14,7 +12,6 @@ class @NotificationsForm toggleCheckbox: (e) => $checkbox = $(e.currentTarget) $parent = $checkbox.closest('.checkbox') - @saveEvent($checkbox, $parent) showCheckboxLoadingSpinner: ($parent) -> @@ -26,11 +23,14 @@ class @NotificationsForm .removeClass 'is-done' saveEvent: ($checkbox, $parent) -> + form = $parent.parents('form:first') + $.ajax( - url: @form.attr('action') - method: 'patch' + url: form.attr('action') + method: form.attr('method') dataType: 'json' - data: @form.serialize() + data: form.serialize() + beforeSend: => @showCheckboxLoadingSpinner($parent) ).done (data) -> diff --git a/app/assets/javascripts/profile.js.coffee b/app/assets/javascripts/profile.js.coffee index 26a12423521..1583d1ba6f9 100644 --- a/app/assets/javascripts/profile.js.coffee +++ b/app/assets/javascripts/profile.js.coffee @@ -8,6 +8,10 @@ class @Profile $('.js-preferences-form').on 'change.preference', 'input[type=radio]', -> $(this).parents('form').submit() + # Automatically submit email form when it changes + $('#user_notification_email').on 'change', -> + $(this).parents('form').submit() + $('.update-username').on 'ajax:before', -> $('.loading-username').show() $(this).find('.update-success').hide() diff --git a/app/assets/javascripts/project.js.coffee b/app/assets/javascripts/project.js.coffee index 236f0899147..d12bad97a05 100644 --- a/app/assets/javascripts/project.js.coffee +++ b/app/assets/javascripts/project.js.coffee @@ -34,27 +34,6 @@ class @Project $(@).parents('.no-password-message').remove() e.preventDefault() - $(document) - .off 'click', '.update-notification' - .on 'click', '.update-notification', (e) -> - e.preventDefault() - notificationLevel = $(@).data 'notification-level' - label = $(@).data 'notification-title' - $('.js-notification-loading').toggleClass 'fa-bell fa-spin fa-spinner' - $('#notification_setting_level').val(notificationLevel) - $('#notification-form').submit() - - $(document) - .off 'ajax:success', '#notification-form' - .on 'ajax:success', '#notification-form', (e, data) -> - if data.saved - new Flash('Notification settings saved', 'notice') - $('.js-notification-toggle-btns') - .closest('.notification-dropdown') - .replaceWith(data.html) - else - new Flash('Failed to save new settings', 'alert') - @projectSelectDropdown() diff --git a/app/controllers/groups/notification_settings_controller.rb b/app/controllers/groups/notification_settings_controller.rb deleted file mode 100644 index de13b16ccf2..00000000000 --- a/app/controllers/groups/notification_settings_controller.rb +++ /dev/null @@ -1,16 +0,0 @@ -class Groups::NotificationSettingsController < Groups::ApplicationController - before_action :authenticate_user! - - def update - notification_setting = current_user.notification_settings_for(group) - saved = notification_setting.update_attributes(notification_setting_params) - - render json: { saved: saved } - end - - private - - def notification_setting_params - params.require(:notification_setting).permit(:level) - end -end diff --git a/app/controllers/notification_settings_controller.rb b/app/controllers/notification_settings_controller.rb new file mode 100644 index 00000000000..5d425ad8420 --- /dev/null +++ b/app/controllers/notification_settings_controller.rb @@ -0,0 +1,34 @@ +class NotificationSettingsController < ApplicationController + before_action :authenticate_user! + + def create + project = current_user.projects.find(params[:project][:id]) + + @notification_setting = current_user.notification_settings_for(project) + @saved = @notification_setting.update_attributes(notification_setting_params) + + render_response + end + + def update + @notification_setting = current_user.notification_settings.find(params[:id]) + @saved = @notification_setting.update_attributes(notification_setting_params) + + render_response + end + + private + + def render_response + render json: { + html: view_to_html_string("notifications/buttons/_notifications", notification_setting: @notification_setting), + saved: @saved + } + end + + def notification_setting_params + allowed_fields = NotificationSetting::EMAIL_EVENTS.dup + allowed_fields << :level + params.require(:notification_setting).permit(allowed_fields) + end +end diff --git a/app/controllers/profiles/notifications_controller.rb b/app/controllers/profiles/notifications_controller.rb index 40d1906a53f..b8b71d295f6 100644 --- a/app/controllers/profiles/notifications_controller.rb +++ b/app/controllers/profiles/notifications_controller.rb @@ -1,13 +1,13 @@ class Profiles::NotificationsController < Profiles::ApplicationController def show @user = current_user - @group_notifications = current_user.notification_settings.for_groups - @project_notifications = current_user.notification_settings.for_projects + @group_notifications = current_user.notification_settings.for_groups.order(:id) + @project_notifications = current_user.notification_settings.for_projects.order(:id) @global_notification_setting = current_user.global_notification_setting end def update - if current_user.update_attributes(user_params) && update_notification_settings + if current_user.update_attributes(user_params) flash[:notice] = "Notification settings saved" else flash[:alert] = "Failed to save new settings" @@ -19,16 +19,4 @@ class Profiles::NotificationsController < Profiles::ApplicationController def user_params params.require(:user).permit(:notification_email) end - - def global_notification_setting_params - params.require(:global_notification_setting).permit(:level) - end - - private - - def update_notification_settings - return true unless global_notification_setting_params - - current_user.global_notification_setting.update_attributes(global_notification_setting_params) - end end diff --git a/app/controllers/projects/notification_settings_controller.rb b/app/controllers/projects/notification_settings_controller.rb deleted file mode 100644 index 05fe5accc65..00000000000 --- a/app/controllers/projects/notification_settings_controller.rb +++ /dev/null @@ -1,21 +0,0 @@ -class Projects::NotificationSettingsController < Projects::ApplicationController - before_action :authenticate_user! - - def update - @notification_setting = current_user.notification_settings_for(project) - saved = @notification_setting.update_attributes(notification_setting_params) - - render json: { - html: view_to_html_string("projects/buttons/_notifications", locals: { project: @project, notification_setting: @notification_setting }), - saved: saved - } - end - - private - - def notification_setting_params - allowed_fields = NotificationSetting::EMAIL_EVENTS.dup - allowed_fields << :level - params.require(:notification_setting).permit(allowed_fields) - end -end diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index 70d56e4ac08..77783cd7640 100644 --- a/app/helpers/notifications_helper.rb +++ b/app/helpers/notifications_helper.rb @@ -64,22 +64,14 @@ module NotificationsHelper end end - def notification_level_radio_buttons - html = "" + # Identifier to trigger individually dropdowns and custom settings modals in the same view + def notifications_menu_identifier(type, notification_setting) + "#{type}-#{notification_setting.user_id}-#{notification_setting.source_id}-#{notification_setting.source_type}" + end - NotificationSetting.levels.each_key do |level| - level = level.to_sym - next if level == :global - - html << content_tag(:div, class: "radio") do - content_tag(:label, { value: level }) do - radio_button_tag(:"global_notification_setting[level]", level, @global_notification_setting.level.to_sym == level) + - content_tag(:div, level.to_s.capitalize, class: "level-title") + - content_tag(:p, notification_description(level)) - end - end - end - - html.html_safe + # Create hidden field to send notification setting source to controller + def hidden_setting_source_input(notification_setting) + return unless notification_setting.source_type + hidden_field_tag "#{notification_setting.source_type.downcase}[id]", notification_setting.source_id end end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 5bdefcc8116..e455101f526 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -159,7 +159,7 @@ class NotificationService recipients = add_project_watchers(recipients, note.project) # Merge project with custom notification - recipients = add_project_custom_notifications(recipients, note.project, :new_note) + recipients = add_custom_notifications(recipients, note.project, :new_note) # Reject users with Mention notification level, except those mentioned in _this_ note. recipients = reject_mention_users(recipients - mentioned_users, note.project) @@ -277,12 +277,20 @@ class NotificationService protected # Get project/group users with CUSTOM notification level - def add_project_custom_notifications(recipients, project, action) + def add_custom_notifications(recipients, project, action) user_ids = [] + # Users with a notification setting on group or project user_ids += notification_settings_for(project, :custom, action) user_ids += notification_settings_for(project.group, :custom, action) + # Users with global level custom + users_with_project_level_global = notification_settings_for(project, :global) + users_with_group_level_global = notification_settings_for(project.group, :global) + + global_users_ids = users_with_project_level_global.concat(users_with_group_level_global) + user_ids += users_with_global_level_custom(global_users_ids, action) + recipients.concat(User.find(user_ids)) end @@ -291,7 +299,8 @@ class NotificationService project_members = notification_settings_for(project) users_with_project_level_global = notification_settings_for(project, :global) - users_with_group_level_global = notification_settings_for(project, :global) + users_with_group_level_global = notification_settings_for(project.group, :global) + users = users_with_global_level_watch([users_with_project_level_global, users_with_group_level_global].flatten.uniq) users_with_project_setting = select_project_member_setting(project, users_with_project_level_global, users) @@ -313,11 +322,21 @@ class NotificationService end def users_with_global_level_watch(ids) + settings_with_global_level_of(:watch, ids).pluck(:user_id) + end + + def users_with_global_level_custom(ids, action) + settings = settings_with_global_level_of(:custom, ids) + settings = settings.select { |setting| setting.events[action] } + settings.map(&:user_id) + end + + def settings_with_global_level_of(level, ids) NotificationSetting.where( user_id: ids, source_type: nil, - level: NotificationSetting.levels[:watch] - ).pluck(:user_id) + level: NotificationSetting.levels[level] + ) end # Build a list of users based on project notifcation settings @@ -497,7 +516,8 @@ class NotificationService recipients = target.participants(current_user) recipients = add_project_watchers(recipients, project) - recipients = add_project_custom_notifications(recipients, project, custom_action) + + recipients = add_custom_notifications(recipients, project, custom_action) recipients = reject_mention_users(recipients, project) recipients = recipients.uniq diff --git a/app/views/notifications/buttons/_notifications.html.haml b/app/views/notifications/buttons/_notifications.html.haml new file mode 100644 index 00000000000..03740f2587b --- /dev/null +++ b/app/views/notifications/buttons/_notifications.html.haml @@ -0,0 +1,24 @@ +- if notification_setting + .dropdown.notification-dropdown.pull-right + = form_for notification_setting, remote: true, html: { class: "inline", id: "notification-form" } do |f| + = hidden_setting_source_input(notification_setting) + = f.hidden_field :level, class: "notification_setting_level" + .js-notification-toggle-btns + .btn-group + - if notification_setting.custom? + %button.dropdown-new.btn.btn-default.notifications-btn#notifications-button{ type: "button", data: { toggle: "modal", target: "#" + notifications_menu_identifier("modal", notification_setting) } } + = icon("bell", class: "js-notification-loading") + = notification_title(notification_setting.level) + %button.btn.dropdown-toggle{ data: { toggle: "dropdown", target: notifications_menu_identifier("dropdown", notification_setting) } } + %span.caret + .sr-only Toggle dropdown + - else + %button.dropdown-new.btn.btn-default.notifications-btn#notifications-button{ type: "button", data: { toggle: "dropdown", target: notifications_menu_identifier("dropdown", notification_setting) } } + = icon("bell", class: "js-notification-loading") + = notification_title(notification_setting.level) + = icon("caret-down") + + = render "shared/notifications/notification_dropdown", notification_setting: notification_setting + + = content_for :scripts_body do + = render "shared/notifications/custom_notifications", notification_setting: notification_setting diff --git a/app/views/profiles/notifications/_group_settings.html.haml b/app/views/profiles/notifications/_group_settings.html.haml index f0cf82afe83..04becd5d860 100644 --- a/app/views/profiles/notifications/_group_settings.html.haml +++ b/app/views/profiles/notifications/_group_settings.html.haml @@ -9,5 +9,4 @@ = link_to group.name, group_path(group) .pull-right - = form_for [group, setting], remote: true, html: { class: 'update-notifications' } do |f| - = f.select :level, NotificationSetting.levels.keys, {}, class: 'form-control trigger-submit' + = render 'notifications/buttons/notifications', notification_setting: setting diff --git a/app/views/profiles/notifications/_project_settings.html.haml b/app/views/profiles/notifications/_project_settings.html.haml index e0fad555c09..664a17418db 100644 --- a/app/views/profiles/notifications/_project_settings.html.haml +++ b/app/views/profiles/notifications/_project_settings.html.haml @@ -9,5 +9,4 @@ = link_to_project(project) .pull-right - = form_for [project.namespace.becomes(Namespace), project, setting], remote: true, html: { class: 'update-notifications' } do |f| - = f.select :level, NotificationSetting.levels.keys, {}, class: 'form-control trigger-submit' + = render 'notifications/buttons/notifications', notification_setting: setting diff --git a/app/views/profiles/notifications/show.html.haml b/app/views/profiles/notifications/show.html.haml index f2659ac14b5..646dd244596 100644 --- a/app/views/profiles/notifications/show.html.haml +++ b/app/views/profiles/notifications/show.html.haml @@ -24,12 +24,15 @@ .form-group = f.label :notification_email, class: "label-light" = f.select :notification_email, @user.all_emails, { include_blank: false }, class: "select2" - .form-group - = f.label :notification_level, class: 'label-light' - = notification_level_radio_buttons - .prepend-top-default - = f.submit 'Update settings', class: "btn btn-create" + = label_tag :global_notification_level, "Global notification level", class: "label-light" + %br + .clearfix + .form-group.pull-left + = render 'notifications/buttons/notifications', notification_setting: @global_notification_setting + + .clearfix + %hr %h5 Groups (#{@group_notifications.count}) diff --git a/app/views/projects/_home_panel.html.haml b/app/views/projects/_home_panel.html.haml index 2b19ee93eea..df725d81a39 100644 --- a/app/views/projects/_home_panel.html.haml +++ b/app/views/projects/_home_panel.html.haml @@ -29,13 +29,10 @@ .project-clone-holder = render "shared/clone_panel" - .project-repo-buttons.project-right-buttons - - if current_user - = render 'shared/members/access_request_buttons', source: @project - .btn-group - = render "projects/buttons/download" - = render 'projects/buttons/dropdown' - = render 'projects/buttons/notifications' + .project-repo-buttons.btn-group.project-right-buttons + = render "projects/buttons/download" + = render 'projects/buttons/dropdown' + = render 'notifications/buttons/notifications', notification_setting: @notification_setting :javascript new Star(); diff --git a/app/views/projects/buttons/_notifications.html.haml b/app/views/projects/buttons/_notifications.html.haml deleted file mode 100644 index 4c2eafbd7df..00000000000 --- a/app/views/projects/buttons/_notifications.html.haml +++ /dev/null @@ -1,21 +0,0 @@ -- if @notification_setting - .dropdown.notification-dropdown.pull-right - = form_for @notification_setting, url: namespace_project_notification_setting_path(@project.namespace.becomes(Namespace), @project), method: :patch, remote: true, html: { class: "inline", id: "notification-form" } do |f| - = f.hidden_field :level - .js-notification-toggle-btns - - if @notification_setting.custom? - .btn-group - %button.dropdown-new.btn.btn-default.notifications-btn#notifications-button{ type: "button", data: { toggle: "modal", target: "#custom-notifications-modal" } } - = icon("bell", class: "js-notification-loading") - = notification_title(@notification_setting.level) - %button.btn.btn-danger.dropdown-toggle{ data: { toggle: "dropdown", target: ".notification-dropdown" } } - %span.caret - .sr-only Toggle dropdown - - else - %button.dropdown-new.btn.btn-default.notifications-btn#notifications-button{ type: "button", data: { toggle: "dropdown", target: ".notification-dropdown" } } - = icon("bell", class: "js-notification-loading") - = notification_title(@notification_setting.level) - = icon("caret-down") - = render "shared/projects/notification_dropdown" - = content_for :scripts_body do - = render "shared/projects/custom_notifications" diff --git a/app/views/shared/notifications/_custom_notifications.html.haml b/app/views/shared/notifications/_custom_notifications.html.haml new file mode 100644 index 00000000000..f0b46c7a4c0 --- /dev/null +++ b/app/views/shared/notifications/_custom_notifications.html.haml @@ -0,0 +1,28 @@ +.modal.fade{ tabindex: "-1", role: "dialog", id: notifications_menu_identifier("modal", notification_setting), aria: { labelledby: "custom-notifications-title" } } + .modal-dialog + .modal-content + .modal-header + %button.close{ type: "button", data: { dismiss: "modal" }, aria: { label: "close" } } + %span{ aria: { hidden: "true" } } × + %h4#custom-notifications-title.modal-title + Custom notification events + + .modal-body + .container-fluid + = form_for notification_setting, html: { class: "custom-notifications-form" } do |f| + = hidden_setting_source_input(notification_setting) + .row + .col-lg-3 + %h4.prepend-top-0 + Notification events + .col-lg-9 + - NotificationSetting::EMAIL_EVENTS.each do |event, index| + = index + .form-group + .checkbox{ class: ("prepend-top-0" if index == 0) } + %label{ for: "events_#{event}" } + = check_box("", event, { name: "notification_setting[#{event}]", class: "js-custom-notification-event", checked: notification_setting.events[event] }) + + %strong + = event.to_s.humanize + = icon("spinner spin", class: "custom-notification-event-loading") diff --git a/app/views/shared/notifications/_notification_dropdown.html.haml b/app/views/shared/notifications/_notification_dropdown.html.haml new file mode 100644 index 00000000000..e6e04b98c82 --- /dev/null +++ b/app/views/shared/notifications/_notification_dropdown.html.haml @@ -0,0 +1,12 @@ +%ul.dropdown-menu.dropdown-menu-no-wrap.dropdown-menu-align-right.dropdown-menu-selectable.dropdown-menu-large{ role: "menu", class: notifications_menu_identifier("dropdown", notification_setting) } + - NotificationSetting.levels.each_key do |level| + - next if level == "custom" + - next if level == "global" && notification_setting.source.nil? + + = notification_list_item(level, notification_setting) + + - unless notification_setting.custom? + %li.divider + %li + %a.update-notification{ href: "#", role: "button", data: { toggle: "modal", target: "#" + notifications_menu_identifier("modal", notification_setting), notification_level: "custom", notification_title: "Custom" } } + Custom diff --git a/config/routes.rb b/config/routes.rb index d52cbb22428..207298d7963 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -123,9 +123,17 @@ Rails.application.routes.draw do end end + # # Spam reports + # resources :abuse_reports, only: [:new, :create] + # + # Notification settings + # + resources :notification_settings, only: [:create, :update] + + # # Import # @@ -421,7 +429,6 @@ Rails.application.routes.draw do resource :avatar, only: [:destroy] resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :update, :new, :create] - resource :notification_setting, only: [:update] end end @@ -653,7 +660,6 @@ Rails.application.routes.draw do resources :forks, only: [:index, :new, :create] resource :import, only: [:new, :create, :show] - resource :notification_setting, only: [:update] resources :refs, only: [] do collection do diff --git a/doc/workflow/notifications/settings.png b/doc/workflow/notifications/settings.png index 1628e2d568c..7c6857aad1a 100644 Binary files a/doc/workflow/notifications/settings.png and b/doc/workflow/notifications/settings.png differ diff --git a/lib/api/entities.rb b/lib/api/entities.rb index cc29c7ef428..7818fc290f8 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -272,7 +272,8 @@ module API expose :access_level expose :notification_level do |member, options| if member.notification_setting - NotificationSetting.levels[member.notification_setting.level] + setting = member.notification_setting + { level: NotificationSetting.levels[setting.level], events: setting.events } end end end diff --git a/spec/controllers/groups/notification_settings_controller_spec.rb b/spec/controllers/groups/notification_settings_controller_spec.rb deleted file mode 100644 index 0786e45515a..00000000000 --- a/spec/controllers/groups/notification_settings_controller_spec.rb +++ /dev/null @@ -1,32 +0,0 @@ -require 'spec_helper' - -describe Groups::NotificationSettingsController do - let(:group) { create(:group) } - let(:user) { create(:user) } - - describe '#update' do - context 'when not authorized' do - it 'redirects to sign in page' do - put :update, - group_id: group.to_param, - notification_setting: { level: :participating } - - expect(response).to redirect_to(new_user_session_path) - end - end - - context 'when authorized' do - before do - sign_in(user) - end - - it 'returns success' do - put :update, - group_id: group.to_param, - notification_setting: { level: :participating } - - expect(response.status).to eq 200 - end - end - end -end diff --git a/spec/controllers/notification_settings_controller_spec.rb b/spec/controllers/notification_settings_controller_spec.rb new file mode 100644 index 00000000000..15d155833b4 --- /dev/null +++ b/spec/controllers/notification_settings_controller_spec.rb @@ -0,0 +1,125 @@ +require 'spec_helper' + +describe NotificationSettingsController do + let(:project) { create(:empty_project) } + let(:user) { create(:user) } + + before do + project.team << [user, :developer] + end + + describe '#create' do + context 'when not authorized' do + it 'redirects to sign in page' do + post :create, + project: { id: project.id }, + notification_setting: { level: :participating } + + expect(response).to redirect_to(new_user_session_path) + end + end + + context 'when authorized' do + before do + sign_in(user) + end + + it 'returns success' do + post :create, + project: { id: project.id }, + notification_setting: { level: :participating } + + expect(response.status).to eq 200 + end + + context 'and setting custom notification setting' do + let(:custom_events) do + events = {} + + NotificationSetting::EMAIL_EVENTS.each do |event| + events[event] = "true" + end + end + + it 'returns success' do + post :create, + project: { id: project.id }, + notification_setting: { level: :participating, events: custom_events } + + expect(response.status).to eq 200 + end + end + end + + context 'not authorized' do + let(:private_project) { create(:project, :private) } + before { sign_in(user) } + + it 'returns 404' do + post :create, + project: { id: private_project.id }, + notification_setting: { level: :participating } + + expect(response.status).to eq(404) + end + end + end + + describe '#update' do + let(:notification_setting) { user.global_notification_setting } + + context 'when not authorized' do + it 'redirects to sign in page' do + put :update, + id: notification_setting, + notification_setting: { level: :participating } + + expect(response).to redirect_to(new_user_session_path) + end + end + + context 'when authorized' do + before{ sign_in(user) } + + it 'returns success' do + put :update, + id: notification_setting, + notification_setting: { level: :participating } + + expect(response.status).to eq 200 + end + + context 'and setting custom notification setting' do + let(:custom_events) do + events = {} + + NotificationSetting::EMAIL_EVENTS.each do |event| + events[event] = "true" + end + end + + it 'returns success' do + put :update, + id: notification_setting, + notification_setting: { level: :participating, events: custom_events } + + expect(response.status).to eq 200 + end + end + end + + context 'not authorized' do + let(:other_user) { create(:user) } + + before { sign_in(other_user) } + + it 'returns 404' do + put :update, + id: notification_setting, + notification_setting: { level: :participating } + + expect(response.status).to eq(404) + end + end + end +end diff --git a/spec/controllers/projects/notification_settings_controller_spec.rb b/spec/controllers/projects/notification_settings_controller_spec.rb deleted file mode 100644 index a726f70a64a..00000000000 --- a/spec/controllers/projects/notification_settings_controller_spec.rb +++ /dev/null @@ -1,71 +0,0 @@ -require 'spec_helper' - -describe Projects::NotificationSettingsController do - let(:project) { create(:empty_project) } - let(:user) { create(:user) } - - before do - project.team << [user, :developer] - end - - describe '#update' do - context 'when not authorized' do - it 'redirects to sign in page' do - put :update, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - notification_setting: { level: :participating } - - expect(response).to redirect_to(new_user_session_path) - end - end - - context 'when authorized' do - before do - sign_in(user) - end - - it 'returns success' do - put :update, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - notification_setting: { level: :participating } - - expect(response.status).to eq 200 - end - - context 'and setting custom notification setting' do - let(:custom_events) do - events = {} - - NotificationSetting::EMAIL_EVENTS.each do |event| - events[event] = "true" - end - end - - it 'returns success' do - put :update, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - notification_setting: { level: :participating, events: custom_events } - - expect(response.status).to eq 200 - end - end - end - - context 'not authorized' do - let(:private_project) { create(:project, :private) } - before { sign_in(user) } - - it 'returns 404' do - put :update, - namespace_id: private_project.namespace.to_param, - project_id: private_project.to_param, - notification_setting: { level: :participating } - - expect(response.status).to eq(404) - end - end - end -end diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index f167813e07d..2a2ac8710ee 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -428,8 +428,9 @@ describe API::API, api: true do describe 'permissions' do context 'all projects' do - it 'Contains permission information' do - project.team << [user, :master] + before { project.team << [user, :master] } + + it 'contains permission information' do get api("/projects", user) expect(response.status).to eq(200) @@ -437,10 +438,18 @@ describe API::API, api: true do to eq(Gitlab::Access::MASTER) expect(json_response.first['permissions']['group_access']).to be_nil end + + it 'contains notification level information' do + get api("/projects", user) + + expect(response.status).to eq(200) + expect(json_response.first['permissions']['project_access']['notification_level']['level']).to eq(NotificationSetting.levels[:global]) + expect(json_response.first['permissions']['project_access']['notification_level'].keys).to include('events') + end end context 'personal project' do - it 'Sets project access and returns 200' do + it 'sets project access and returns 200' do project.team << [user, :master] get api("/projects/#{project.id}", user) @@ -452,9 +461,11 @@ describe API::API, api: true do end context 'group project' do + let(:project2) { create(:project, group: create(:group)) } + + before { project2.group.add_owner(user) } + it 'should set the owner and return 200' do - project2 = create(:project, group: create(:group)) - project2.group.add_owner(user) get api("/projects/#{project2.id}", user) expect(response.status).to eq(200) @@ -462,6 +473,14 @@ describe API::API, api: true do expect(json_response['permissions']['group_access']['access_level']). to eq(Gitlab::Access::OWNER) end + + it 'shows notification level information' do + get api("/projects/#{project2.id}", user) + + expect(response.status).to eq(200) + expect(json_response['permissions']['group_access']['notification_level']['level']).to eq(NotificationSetting.levels[:global]) + expect(json_response['permissions']['group_access']['notification_level'].keys).to include('events') + end end end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 616d0cd00f7..c4edb8c3fd0 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -46,7 +46,8 @@ describe NotificationService, services: true do project.team << [issue.assignee, :master] project.team << [note.author, :master] create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@subscribed_participant cc this guy') - update_custom_notification(:new_note) + update_custom_notification(:new_note, @u_guest_custom, project) + update_custom_notification(:new_note, @u_custom_global) end describe :new_note do @@ -54,7 +55,7 @@ describe NotificationService, services: true do add_users_with_subscription(note.project, issue) # Ensure create SentNotification by noteable = issue 6 times, not noteable = note - expect(SentNotification).to receive(:record).with(issue, any_args).exactly(7).times + expect(SentNotification).to receive(:record).with(issue, any_args).exactly(8).times ActionMailer::Base.deliveries.clear @@ -63,6 +64,7 @@ describe NotificationService, services: true do should_email(@u_watcher) should_email(note.noteable.author) should_email(note.noteable.assignee) + should_email(@u_custom_global) should_email(@u_mentioned) should_email(@subscriber) should_email(@watcher_and_subscriber) @@ -105,10 +107,12 @@ describe NotificationService, services: true do before do note.project.namespace_id = group.id note.project.group.add_user(@u_watcher, GroupMember::MASTER) + note.project.group.add_user(@u_custom_global, GroupMember::MASTER) note.project.save @u_watcher.notification_settings_for(note.project).participating! @u_watcher.notification_settings_for(note.project.group).global! + update_custom_notification(:new_note, @u_custom_global) ActionMailer::Base.deliveries.clear end @@ -118,6 +122,7 @@ describe NotificationService, services: true do should_email(note.noteable.author) should_email(note.noteable.assignee) should_email(@u_mentioned) + should_email(@u_custom_global) should_not_email(@u_guest_custom) should_not_email(@u_guest_watcher) should_not_email(@u_watcher) @@ -139,6 +144,10 @@ describe NotificationService, services: true do let(:admin) { create(:admin) } let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee) } let(:note) { create(:note_on_issue, noteable: confidential_issue, project: project, note: "#{author.to_reference} #{assignee.to_reference} #{non_member.to_reference} #{member.to_reference} #{admin.to_reference}") } + let(:guest_watcher) { create_user_with_notification(:watch, "guest-watcher-confidential") } + + before do + end it 'filters out users that can not read the issue' do project.team << [member, :developer] @@ -152,6 +161,7 @@ describe NotificationService, services: true do should_not_email(non_member) should_not_email(guest) + should_not_email(guest_watcher) should_email(author) should_email(assignee) should_email(member) @@ -226,6 +236,9 @@ describe NotificationService, services: true do should_email(member) end + # it emails custom global users on mention + should_email(@u_custom_global) + should_email(@u_guest_watcher) should_email(note.noteable.author) should_not_email(note.author) @@ -244,13 +257,15 @@ describe NotificationService, services: true do build_team(note.project) ActionMailer::Base.deliveries.clear allow_any_instance_of(Commit).to receive(:author).and_return(@u_committer) - update_custom_notification(:new_note) + update_custom_notification(:new_note, @u_guest_custom, project) + update_custom_notification(:new_note, @u_custom_global) end describe '#new_note, #perform_enqueued_jobs' do it do notification.new_note(note) should_email(@u_guest_watcher) + should_email(@u_custom_global) should_email(@u_guest_custom) should_email(@u_committer) should_email(@u_watcher) @@ -292,7 +307,8 @@ describe NotificationService, services: true do build_team(issue.project) add_users_with_subscription(issue.project, issue) ActionMailer::Base.deliveries.clear - update_custom_notification(:new_issue) + update_custom_notification(:new_issue, @u_guest_custom, project) + update_custom_notification(:new_issue, @u_custom_global) end describe '#new_issue' do @@ -303,6 +319,7 @@ describe NotificationService, services: true do should_email(@u_watcher) should_email(@u_guest_watcher) should_email(@u_guest_custom) + should_email(@u_custom_global) should_email(@u_participant_mentioned) should_not_email(@u_mentioned) should_not_email(@u_participating) @@ -351,6 +368,7 @@ describe NotificationService, services: true do notification.new_issue(confidential_issue, @u_disabled) + should_not_email(@u_guest_watcher) should_not_email(non_member) should_not_email(author) should_not_email(guest) @@ -362,7 +380,11 @@ describe NotificationService, services: true do end describe '#reassigned_issue' do - before { update_custom_notification(:reassign_issue) } + + before do + update_custom_notification(:reassign_issue, @u_guest_custom, project) + update_custom_notification(:reassign_issue, @u_custom_global) + end it 'emails new assignee' do notification.reassigned_issue(issue, @u_disabled) @@ -371,6 +393,7 @@ describe NotificationService, services: true do should_email(@u_watcher) should_email(@u_guest_watcher) should_email(@u_guest_custom) + should_email(@u_custom_global) should_email(@u_participant_mentioned) should_email(@subscriber) should_not_email(@unsubscriber) @@ -390,6 +413,7 @@ describe NotificationService, services: true do should_email(@u_guest_custom) should_email(@u_participant_mentioned) should_email(@subscriber) + should_email(@u_custom_global) should_not_email(@unsubscriber) should_not_email(@u_participating) should_not_email(@u_disabled) @@ -407,6 +431,7 @@ describe NotificationService, services: true do should_email(@u_guest_custom) should_email(@u_participant_mentioned) should_email(@subscriber) + should_email(@u_custom_global) should_not_email(@unsubscriber) should_not_email(@u_participating) should_not_email(@u_disabled) @@ -424,6 +449,7 @@ describe NotificationService, services: true do should_email(@u_guest_custom) should_email(@u_participant_mentioned) should_email(@subscriber) + should_email(@u_custom_global) should_not_email(@unsubscriber) should_not_email(@u_participating) should_not_email(@u_disabled) @@ -440,6 +466,7 @@ describe NotificationService, services: true do should_email(@u_guest_custom) should_email(@u_participant_mentioned) should_email(@subscriber) + should_email(@u_custom_global) should_not_email(issue.assignee) should_not_email(@unsubscriber) should_not_email(@u_participating) @@ -542,7 +569,11 @@ describe NotificationService, services: true do end describe '#close_issue' do - before { update_custom_notification(:close_issue) } + + before do + update_custom_notification(:close_issue, @u_guest_custom, project) + update_custom_notification(:close_issue, @u_custom_global) + end it 'should sent email to issue assignee and issue author' do notification.close_issue(issue, @u_disabled) @@ -552,6 +583,7 @@ describe NotificationService, services: true do should_email(@u_watcher) should_email(@u_guest_watcher) should_email(@u_guest_custom) + should_email(@u_custom_global) should_email(@u_participant_mentioned) should_email(@subscriber) should_email(@watcher_and_subscriber) @@ -591,7 +623,10 @@ describe NotificationService, services: true do end describe '#reopen_issue' do - before { update_custom_notification(:reopen_issue) } + before do + update_custom_notification(:reopen_issue, @u_guest_custom, project) + update_custom_notification(:reopen_issue, @u_custom_global) + end it 'should send email to issue assignee and issue author' do notification.reopen_issue(issue, @u_disabled) @@ -601,6 +636,7 @@ describe NotificationService, services: true do should_email(@u_watcher) should_email(@u_guest_watcher) should_email(@u_guest_custom) + should_email(@u_custom_global) should_email(@u_participant_mentioned) should_email(@subscriber) should_email(@watcher_and_subscriber) @@ -650,7 +686,10 @@ describe NotificationService, services: true do end describe '#new_merge_request' do - before { update_custom_notification(:new_merge_request) } + before do + update_custom_notification(:new_merge_request, @u_guest_custom, project) + update_custom_notification(:new_merge_request, @u_custom_global) + end it do notification.new_merge_request(merge_request, @u_disabled) @@ -661,6 +700,7 @@ describe NotificationService, services: true do should_email(@u_participant_mentioned) should_email(@u_guest_watcher) should_email(@u_guest_custom) + should_email(@u_custom_global) should_not_email(@u_participating) should_not_email(@u_disabled) should_not_email(@u_lazy_participant) @@ -707,7 +747,10 @@ describe NotificationService, services: true do end describe '#reassigned_merge_request' do - before { update_custom_notification(:reassign_merge_request) } + before do + update_custom_notification(:reassign_merge_request, @u_guest_custom, project) + update_custom_notification(:reassign_merge_request, @u_custom_global) + end it do notification.reassigned_merge_request(merge_request, merge_request.author) @@ -719,6 +762,7 @@ describe NotificationService, services: true do should_email(@watcher_and_subscriber) should_email(@u_guest_watcher) should_email(@u_guest_custom) + should_email(@u_custom_global) should_not_email(@unsubscriber) should_not_email(@u_participating) should_not_email(@u_disabled) @@ -786,7 +830,10 @@ describe NotificationService, services: true do end describe '#closed_merge_request' do - before { update_custom_notification(:close_merge_request) } + before do + update_custom_notification(:close_merge_request, @u_guest_custom, project) + update_custom_notification(:close_merge_request, @u_custom_global) + end it do notification.close_mr(merge_request, @u_disabled) @@ -795,6 +842,7 @@ describe NotificationService, services: true do should_email(@u_watcher) should_email(@u_guest_watcher) should_email(@u_guest_custom) + should_email(@u_custom_global) should_email(@u_participant_mentioned) should_email(@subscriber) should_email(@watcher_and_subscriber) @@ -835,7 +883,11 @@ describe NotificationService, services: true do end describe '#merged_merge_request' do - before { update_custom_notification(:merge_merge_request) } + + before do + update_custom_notification(:merge_merge_request, @u_guest_custom, project) + update_custom_notification(:merge_merge_request, @u_custom_global) + end it do notification.merge_mr(merge_request, @u_disabled) @@ -846,6 +898,7 @@ describe NotificationService, services: true do should_email(@subscriber) should_email(@watcher_and_subscriber) should_email(@u_guest_watcher) + should_email(@u_custom_global) should_email(@u_guest_custom) should_not_email(@unsubscriber) should_not_email(@u_participating) @@ -884,7 +937,10 @@ describe NotificationService, services: true do end describe '#reopen_merge_request' do - before { update_custom_notification(:reopen_merge_request) } + before do + update_custom_notification(:reopen_merge_request, @u_guest_custom, project) + update_custom_notification(:reopen_merge_request, @u_custom_global) + end it do notification.reopen_mr(merge_request, @u_disabled) @@ -896,6 +952,7 @@ describe NotificationService, services: true do should_email(@watcher_and_subscriber) should_email(@u_guest_watcher) should_email(@u_guest_custom) + should_email(@u_custom_global) should_not_email(@unsubscriber) should_not_email(@u_participating) should_not_email(@u_disabled) @@ -948,6 +1005,7 @@ describe NotificationService, services: true do should_email(@u_watcher) should_email(@u_participating) should_email(@u_lazy_participant) + should_email(@u_custom_global) should_not_email(@u_guest_watcher) should_not_email(@u_guest_custom) should_not_email(@u_disabled) @@ -964,6 +1022,7 @@ describe NotificationService, services: true do @u_committer = create(:user, username: 'committer') @u_not_mentioned = create_global_setting_for(create(:user, username: 'regular'), :participating) @u_outsider_mentioned = create(:user, username: 'outsider') + @u_custom_global = create_global_setting_for(create(:user, username: 'custom_global'), :custom) # User to be participant by default # This user does not contain any record in notification settings table @@ -981,6 +1040,7 @@ describe NotificationService, services: true do project.team << [@u_committer, :master] project.team << [@u_not_mentioned, :master] project.team << [@u_lazy_participant, :master] + project.team << [@u_custom_global, :master] end def create_global_setting_for(user, level) @@ -1000,8 +1060,10 @@ describe NotificationService, services: true do user end - def update_custom_notification(event) - setting = @u_guest_custom.notification_settings_for(project) + # Create custom notifications + # When resource is nil it means global notification + def update_custom_notification(event, user, resource = nil) + setting = user.notification_settings_for(resource) setting.events[event] = true setting.save end