Commit 89a2c873 authored by Felipe's avatar Felipe

Implement custom notification level options

parent 8bfbafbb
Pipeline #3524897 passed with stages
in 505 minutes and 39 seconds
......@@ -54,6 +54,7 @@ v 8.9.0 (unreleased)
- Remove 'main language' feature
- Pipelines can be canceled only when there are running builds
- Use downcased path to container repository as this is expected path by Docker
- Customized notification settings for projects
- Projects pending deletion will render a 404 page
- Measure queue duration between gitlab-workhorse and Rails
- Make Omniauth providers specs to not modify global configuration
......
......@@ -75,6 +75,7 @@ class Dispatcher
when 'projects:show'
shortcut_handler = new ShortcutsNavigation()
new NotificationsForm()
new TreeView() if $('#tree-slider').length
when 'groups:activity'
new Activities()
......
class @NotificationsForm
constructor: ->
@form = $('.custom-notifications-form')
@removeEventListeners()
@initEventListeners()
removeEventListeners: ->
$(document).off 'change', '.js-custom-notification-event'
initEventListeners: ->
$(document).on 'change', '.js-custom-notification-event', @toggleCheckbox
toggleCheckbox: (e) =>
$checkbox = $(e.currentTarget)
$parent = $checkbox.closest('.checkbox')
@saveEvent($checkbox, $parent)
showCheckboxLoadingSpinner: ($parent) ->
$parent
.addClass 'is-loading'
.find '.custom-notification-event-loading'
.removeClass 'fa-check'
.addClass 'fa-spin fa-spinner'
.removeClass 'is-done'
saveEvent: ($checkbox, $parent) ->
$.ajax(
url: @form.attr('action')
method: 'patch'
dataType: 'json'
data: @form.serialize()
beforeSend: =>
@showCheckboxLoadingSpinner($parent)
).done (data) ->
$checkbox.enable()
if data.saved
$parent
.find '.custom-notification-event-loading'
.toggleClass 'fa-spin fa-spinner fa-check is-done'
setTimeout(->
$parent
.removeClass 'is-loading'
.find '.custom-notification-event-loading'
.toggleClass 'fa-spin fa-spinner fa-check is-done'
, 2000)
......@@ -34,21 +34,26 @@ class @Project
$(@).parents('.no-password-message').remove()
e.preventDefault()
$('.update-notification').on 'click', (e) ->
e.preventDefault()
notification_level = $(@).data 'notification-level'
label = $(@).data 'notification-title'
$('#notification_setting_level').val(notification_level)
$('#notification-form').submit()
$('#notifications-button').empty().append("<i class='fa fa-bell'></i>" + label + "<i class='fa fa-angle-down'></i>")
$(@).parents('ul').find('li.active').removeClass 'active'
$(@).parent().addClass 'active'
$('#notification-form').on 'ajax:success', (e, data) ->
if data.saved
new Flash("Notification settings saved", "notice")
else
new Flash("Failed to save new settings", "alert")
$(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()
......
......@@ -133,11 +133,6 @@
}
}
.btn-group:not(:first-child):not(:last-child) > .btn {
border-top-right-radius: 3px;
border-bottom-right-radius: 3px;
}
form {
margin-left: 10px;
}
......@@ -607,3 +602,20 @@ pre.light-well {
}
}
}
.custom-notifications-form {
.is-loading {
.custom-notification-event-loading {
display: inline-block;
}
}
}
.custom-notification-event-loading {
display: none;
margin-left: 5px;
&.is-done {
color: $gl-text-green;
}
}
......@@ -2,15 +2,20 @@ 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)
@notification_setting = current_user.notification_settings_for(project)
saved = @notification_setting.update_attributes(notification_setting_params)
render json: { saved: saved }
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
params.require(:notification_setting).permit(:level)
allowed_fields = NotificationSetting::EMAIL_EVENTS.dup
allowed_fields << :level
params.require(:notification_setting).permit(allowed_fields)
end
end
class NotificationSetting < ActiveRecord::Base
enum level: { global: 3, watch: 2, mention: 4, participating: 1, disabled: 0 }
enum level: { global: 3, watch: 2, mention: 4, participating: 1, disabled: 0, custom: 5 }
default_value_for :level, NotificationSetting.levels[:global]
......@@ -15,6 +15,24 @@ class NotificationSetting < ActiveRecord::Base
scope :for_groups, -> { where(source_type: 'Namespace') }
scope :for_projects, -> { where(source_type: 'Project') }
EMAIL_EVENTS = [
:new_note,
:new_issue,
:reopen_issue,
:close_issue,
:reassign_issue,
:new_merge_request,
:reopen_merge_request,
:close_merge_request,
:reassign_merge_request,
:merge_merge_request
]
store :events, accessors: EMAIL_EVENTS, coder: JSON
before_create :set_events
before_save :events_to_boolean
def self.find_or_create_for(source)
setting = find_or_initialize_by(source: source)
......@@ -24,4 +42,21 @@ class NotificationSetting < ActiveRecord::Base
setting
end
# Set all event attributes to false when level is not custom or being initialized for UX reasons
def set_events
return if custom?
EMAIL_EVENTS.each do |event|
events[event] = false
end
end
# Validates store accessors values as boolean
# It is a text field so it does not cast correct boolean values in JSON
def events_to_boolean
EMAIL_EVENTS.each do |event|
events[event] = ActiveRecord::ConnectionAdapters::Column::TRUE_VALUES.include?(events[event])
end
end
end
This diff is collapsed.
- if @notification_setting
= 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
.dropdown.hidden-sm
%button.btn.btn-default.notifications-btn#notifications-button{ data: { toggle: "dropdown" }, aria: { haspopup: "true", expanded: "false" } }
= icon('bell')
= notification_title(@notification_setting.level)
= icon('caret-down')
%ul.dropdown-menu.dropdown-menu-no-wrap.dropdown-menu-align-right.dropdown-menu-selectable.dropdown-menu-large{ role: "menu" }
- NotificationSetting.levels.each do |level|
= notification_list_item(level.first, @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"
#custom-notifications-modal.modal.fade{ tabindex: "-1", role: "dialog", 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, url: namespace_project_notification_setting_path(@project.namespace.becomes(Namespace), @project), method: :patch, html: { class: "custom-notifications-form" } do |f|
.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("notification_setting", event, {id: "events_#{event}", class: "js-custom-notification-event"})
%strong
= event.to_s.humanize
= icon("spinner spin", class: "custom-notification-event-loading")
%ul.dropdown-menu.dropdown-menu-no-wrap.dropdown-menu-align-right.dropdown-menu-selectable.dropdown-menu-large{ role: "menu" }
- NotificationSetting.levels.each do |level|
- if level.first != "custom"
= notification_list_item(level.first, @notification_setting)
- unless @notification_setting.custom?
%li.divider
%li
%a.update-notification{ href: "#", role: "button", data: { toggle: "modal", target: "#custom-notifications-modal", notification_level: "custom", notification_title: "Custom" } }
Custom
class AddEventsToNotificationSettings < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
def change
add_column :notification_settings, :events, :text
end
end
......@@ -4,7 +4,7 @@ GitLab has a notification system in place to notify a user of events that are im
## Notification settings
Under user profile page you can find the notification settings.
You can find notification settings under the user profile.
![notification settings](notifications/settings.png)
......@@ -20,6 +20,7 @@ Each of these settings have levels of notification:
* Participating - receive notifications from related resources
* Watch - receive notifications from projects or groups user is a member of
* Global - notifications as set at the global settings
* Custom - Notification is set based on events selected by the user.(Available only in project level)
#### Global Settings
......@@ -55,7 +56,7 @@ Below is the table of events users can be notified of:
| User added to project | User | Sent when user is added to project |
| Project access level changed | User | Sent when user project access level is changed |
| User added to group | User | Sent when user is added to group |
| Group access level changed | User | Sent when user group access level is changed |
| Group access level changed | User | Sent when user group access level is changed |
| Project moved | Project members [1] | [1] not disabled |
### Issue / Merge Request events
......@@ -71,6 +72,7 @@ In all of the below cases, the notification will be sent to:
- Watchers: users with notification level "Watch"
- Subscribers: anyone who manually subscribed to the issue/merge request
- Custom: Users with notification level "custom" who turned on notifications for any of the events present in the table below
| Event | Sent to |
|------------------------|---------|
......
......@@ -126,7 +126,7 @@ class Spinach::Features::Project < Spinach::FeatureSteps
end
step 'I click notifications drop down button' do
find('#notifications-button').click
first('.notifications-btn').click
end
step 'I choose Mention setting' do
......
......@@ -33,6 +33,25 @@ describe Projects::NotificationSettingsController do
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
......
......@@ -12,5 +12,30 @@ RSpec.describe NotificationSetting, type: :model do
it { is_expected.to validate_presence_of(:user) }
it { is_expected.to validate_presence_of(:level) }
it { is_expected.to validate_uniqueness_of(:user_id).scoped_to([:source_id, :source_type]).with_message(/already exists in source/) }
context "events" do
let(:user) { create(:user) }
let(:notification_setting) { NotificationSetting.new(source_id: 1, source_type: 'Project', user_id: user.id) }
before do
notification_setting.level = "custom"
notification_setting.new_note = "true"
notification_setting.new_issue = 1
notification_setting.close_issue = "1"
notification_setting.merge_merge_request = "t"
notification_setting.close_merge_request = "nil"
  • @felipe_artur I'm updating rails and I have some troubles with this spec and this is why I would like to know why we test "nil" here?

  • @vsizov, this is just for testing if string values coming from the API are being parsed to boolean.

    "1".should eq(true)
    "t".should eq(true)
    "nil".should eq(false)

    Edited by Felipe Artur
  • @felipe_artur I don't see any notes in the API documentation that we accept "nil" as a false value. The rails 5 consider this as a true value which is pretty natural to me.

    @smcgivern Mentioning you, as you recently changed something about it too.

    I fixed it by changing the expected effective value in a1674389, any objections?

  • @vsizov no, that makes sense to me 👍

  • @vsizov What is the problem you are having with Rails 5?

    We are converting this values to make the parameters sent by view and API compatible. For example checkboxes does not send true or false as parameter, as far as i remember they were sending 0 or 1. Also those values were being saved on a serialized field.

    We are just converting to boolean ActiveRecord::ConnectionAdapters::Column::TRUE_VALUE as Rails do. Check NotificationSetting#events_to_boolean.

    Edited by Felipe Artur
  • The problem is that Rails 5 parses "true values" differently. Tha main difference is that "nil" is now true.

    Edited by Valery Sizov
  • Cool! I don't see any problem either!!

    Thanks =)

Please register or sign in to reply
notification_setting.reopen_merge_request = "false"
notification_setting.save
end
it "parses boolean before saving" do
expect(notification_setting.new_note).to eq(true)
expect(notification_setting.new_issue).to eq(true)
expect(notification_setting.close_issue).to eq(true)
expect(notification_setting.merge_merge_request).to eq(true)
expect(notification_setting.close_merge_request).to eq(false)
expect(notification_setting.reopen_merge_request).to eq(false)
end
end
end
end
......@@ -46,6 +46,7 @@ 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)
end
describe :new_note do
......@@ -66,6 +67,7 @@ describe NotificationService, services: true do
should_email(@subscriber)
should_email(@watcher_and_subscriber)
should_email(@subscribed_participant)
should_not_email(@u_guest_custom)
should_not_email(@u_guest_watcher)
should_not_email(note.author)
should_not_email(@u_participating)
......@@ -116,6 +118,7 @@ describe NotificationService, services: true do
should_email(note.noteable.author)
should_email(note.noteable.assignee)
should_email(@u_mentioned)
should_not_email(@u_guest_custom)
should_not_email(@u_guest_watcher)
should_not_email(@u_watcher)
should_not_email(note.author)
......@@ -241,13 +244,14 @@ 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)
end
describe '#new_note, #perform_enqueued_jobs' do
it do
notification.new_note(note)
should_email(@u_guest_watcher)
should_email(@u_guest_custom)
should_email(@u_committer)
should_email(@u_watcher)
should_not_email(@u_mentioned)
......@@ -288,6 +292,7 @@ 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)
end
describe '#new_issue' do
......@@ -297,6 +302,7 @@ describe NotificationService, services: true do
should_email(issue.assignee)
should_email(@u_watcher)
should_email(@u_guest_watcher)
should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_not_email(@u_mentioned)
should_not_email(@u_participating)
......@@ -356,12 +362,15 @@ describe NotificationService, services: true do
end
describe '#reassigned_issue' do
before { update_custom_notification(:reassign_issue) }
it 'emails new assignee' do
notification.reassigned_issue(issue, @u_disabled)
should_email(issue.assignee)
should_email(@u_watcher)
should_email(@u_guest_watcher)
should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_email(@subscriber)
should_not_email(@unsubscriber)
......@@ -378,6 +387,7 @@ describe NotificationService, services: true do
should_email(@u_mentioned)
should_email(@u_watcher)
should_email(@u_guest_watcher)
should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_email(@subscriber)
should_not_email(@unsubscriber)
......@@ -394,6 +404,7 @@ describe NotificationService, services: true do
should_email(issue.assignee)
should_email(@u_watcher)
should_email(@u_guest_watcher)
should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_email(@subscriber)
should_not_email(@unsubscriber)
......@@ -410,6 +421,7 @@ describe NotificationService, services: true do
should_email(issue.assignee)
should_email(@u_watcher)
should_email(@u_guest_watcher)
should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_email(@subscriber)
should_not_email(@unsubscriber)
......@@ -425,6 +437,7 @@ describe NotificationService, services: true do
expect(issue.assignee).to be @u_mentioned
should_email(@u_watcher)
should_email(@u_guest_watcher)
should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_email(@subscriber)
should_not_email(issue.assignee)
......@@ -529,6 +542,8 @@ describe NotificationService, services: true do
end
describe '#close_issue' do
before { update_custom_notification(:close_issue) }
it 'should sent email to issue assignee and issue author' do
notification.close_issue(issue, @u_disabled)
......@@ -536,6 +551,7 @@ describe NotificationService, services: true do
should_email(issue.author)
should_email(@u_watcher)
should_email(@u_guest_watcher)
should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_email(@subscriber)
should_email(@watcher_and_subscriber)
......@@ -575,6 +591,8 @@ describe NotificationService, services: true do
end
describe '#reopen_issue' do
before { update_custom_notification(:reopen_issue) }
it 'should send email to issue assignee and issue author' do
notification.reopen_issue(issue, @u_disabled)
......@@ -582,6 +600,7 @@ describe NotificationService, services: true do
should_email(issue.author)
should_email(@u_watcher)
should_email(@u_guest_watcher)
should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_email(@subscriber)
should_email(@watcher_and_subscriber)
......@@ -631,6 +650,8 @@ describe NotificationService, services: true do
end
describe '#new_merge_request' do
before { update_custom_notification(:new_merge_request) }
it do
notification.new_merge_request(merge_request, @u_disabled)
......@@ -639,6 +660,7 @@ describe NotificationService, services: true do
should_email(@watcher_and_subscriber)
should_email(@u_participant_mentioned)
should_email(@u_guest_watcher)
should_email(@u_guest_custom)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
......@@ -685,6 +707,8 @@ describe NotificationService, services: true do
end
describe '#reassigned_merge_request' do
before { update_custom_notification(:reassign_merge_request) }
it do
notification.reassigned_merge_request(merge_request, merge_request.author)
......@@ -694,6 +718,7 @@ describe NotificationService, services: true do
should_email(@subscriber)
should_email(@watcher_and_subscriber)
should_email(@u_guest_watcher)
should_email(@u_guest_custom)
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_disabled)
......@@ -761,12 +786,15 @@ describe NotificationService, services: true do
end
describe '#closed_merge_request' do
before { update_custom_notification(:close_merge_request) }
it do
notification.close_mr(merge_request, @u_disabled)
should_email(merge_request.assignee)
should_email(@u_watcher)
should_email(@u_guest_watcher)
should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_email(@subscriber)
should_email(@watcher_and_subscriber)
......@@ -807,6 +835,8 @@ describe NotificationService, services: true do
end
describe '#merged_merge_request' do
before { update_custom_notification(:merge_merge_request) }
it do
notification.merge_mr(merge_request, @u_disabled)
......@@ -816,6 +846,7 @@ describe NotificationService, services: true do
should_email(@subscriber)
should_email(@watcher_and_subscriber)
should_email(@u_guest_watcher)
should_email(@u_guest_custom)
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_disabled)
......@@ -853,6 +884,8 @@ describe NotificationService, services: true do
end
describe '#reopen_merge_request' do
before { update_custom_notification(:reopen_merge_request) }
it do
notification.reopen_mr(merge_request, @u_disabled)
......@@ -862,6 +895,7 @@ describe NotificationService, services: true do
should_email(@subscriber)
should_email(@watcher_and_subscriber)
should_email(@u_guest_watcher)
should_email(@u_guest_custom)
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_disabled)
......@@ -915,6 +949,7 @@ describe NotificationService, services: true do
should_email(@u_participating)
should_email(@u_lazy_participant)
should_not_email(@u_guest_watcher)
should_not_email(@u_guest_custom)
should_not_email(@u_disabled)
end
end
......@@ -935,7 +970,8 @@ describe NotificationService, services: true do
# It should be treated with a :participating notification_level
@u_lazy_participant = create(:user, username: 'lazy-participant')
create_guest_watcher
@u_guest_watcher = create_user_with_notification(:watch, 'guest_watching')
@u_guest_custom = create_user_with_notification(:custom, 'guest_custom')
project.team << [@u_watcher, :master]
project.team << [@u_participating, :master]
......@@ -955,10 +991,18 @@ describe NotificationService, services: true do
user
end
def create_guest_watcher
@u_guest_watcher = create(:user, username: 'guest_watching')
setting = @u_guest_watcher.notification_settings_for(project)
setting.level = :watch
def create_user_with_notification(level, username)
user = create(:user, username: username)
setting = user.notification_settings_for(project)
setting.level = level
setting.save
user
end
def update_custom_notification(event)
setting = @u_guest_custom.notification_settings_for(project)
setting.events[event] = true
setting.save
end
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment