Commit c03f1259 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'issue_3359_2' into 'master'

Remove notification level from user model

part of #3359 

See merge request !4494
parents 65df6bcb 39ead205
......@@ -57,6 +57,7 @@ v 8.9.0 (unreleased)
- Improve error handling importing projects
- Remove duplicated notification settings
- Put project Files and Commits tabs under Code tab
- Decouple global notification level from user model
- Replace Colorize with Rainbow for coloring console output in Rake tasks.
- Add workhorse controller and API helpers
- An indicator is now displayed at the top of the comment field for confidential issues.
......
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
@user = current_user
@group_notifications = current_user.notification_settings.for_groups
@project_notifications = current_user.notification_settings.for_projects
@global_notification_setting = current_user.global_notification_setting
end
def update
if current_user.update_attributes(user_params)
if current_user.update_attributes(user_params) && update_notification_settings
flash[:notice] = "Notification settings saved"
else
flash[:alert] = "Failed to save new settings"
......@@ -16,6 +17,18 @@ class Profiles::NotificationsController < Profiles::ApplicationController
end
def user_params
params.require(:user).permit(:notification_email, :notification_level)
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
......@@ -61,4 +61,23 @@ module NotificationsHelper
end
end
end
def notification_level_radio_buttons
html = ""
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
end
end
......@@ -7,7 +7,6 @@ class NotificationSetting < ActiveRecord::Base
belongs_to :source, polymorphic: true
validates :user, presence: true
validates :source, presence: true
validates :level, presence: true
validates :user_id, uniqueness: { scope: [:source_type, :source_id],
message: "already exists in source",
......
......@@ -10,6 +10,8 @@ class User < ActiveRecord::Base
include CaseSensitivity
include TokenAuthenticatable
DEFAULT_NOTIFICATION_LEVEL = :participating
add_authentication_token_field :authentication_token
default_value_for :admin, false
......@@ -99,7 +101,6 @@ class User < ActiveRecord::Base
presence: true,
uniqueness: { case_sensitive: false }
validates :notification_level, presence: true
validate :namespace_uniq, if: ->(user) { user.username_changed? }
validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
validate :unique_email, if: ->(user) { user.email_changed? }
......@@ -133,13 +134,6 @@ class User < ActiveRecord::Base
# Note: When adding an option, it MUST go on the end of the array.
enum project_view: [:readme, :activity, :files]
# Notification level
# Note: When adding an option, it MUST go on the end of the array.
#
# TODO: Add '_prefix: :notification' to enum when update to Rails 5. https://github.com/rails/rails/pull/19813
# Because user.notification_disabled? is much better than user.disabled?
enum notification_level: [:disabled, :participating, :watch, :global, :mention]
alias_attribute :private_token, :authentication_token
delegate :path, to: :namespace, allow_nil: true, prefix: true
......@@ -800,6 +794,17 @@ class User < ActiveRecord::Base
notification_settings.find_or_initialize_by(source: source)
end
# Lazy load global notification setting
# Initializes User setting with Participating level if setting not persisted
def global_notification_setting
return @global_notification_setting if defined?(@global_notification_setting)
@global_notification_setting = notification_settings.find_or_initialize_by(source: nil)
@global_notification_setting.update_attributes(level: NotificationSetting.levels[DEFAULT_NOTIFICATION_LEVEL]) unless @global_notification_setting.persisted?
@global_notification_setting
end
def assigned_open_merge_request_count(force: false)
Rails.cache.fetch(['users', id, 'assigned_open_merge_request_count'], force: force) do
assigned_merge_requests.opened.count
......
......@@ -279,10 +279,11 @@ class NotificationService
end
def users_with_global_level_watch(ids)
User.where(
id: ids,
notification_level: NotificationSetting.levels[:watch]
).pluck(:id)
NotificationSetting.where(
user_id: ids,
source_type: nil,
level: NotificationSetting.levels[:watch]
).pluck(:user_id)
end
# Build a list of users based on project notifcation settings
......@@ -352,7 +353,9 @@ class NotificationService
users = users.reject(&:blocked?)
users.reject do |user|
next user.notification_level == level unless project
global_notification_setting = user.global_notification_setting
next global_notification_setting.level == level unless project
setting = user.notification_settings_for(project)
......@@ -361,13 +364,13 @@ class NotificationService
end
# reject users who globally set mention notification and has no setting per project/group
next user.notification_level == level unless setting
next global_notification_setting.level == level unless setting
# reject users who set mention notification in project
next true if setting.level == level
# reject users who have mention level in project and disabled in global settings
setting.global? && user.notification_level == level
setting.global? && global_notification_setting.level == level
end
end
......@@ -456,7 +459,6 @@ class NotificationService
def build_recipients(target, project, current_user, action: nil, previous_assignee: nil)
recipients = target.participants(current_user)
recipients = add_project_watchers(recipients, project)
recipients = reject_mention_users(recipients, project)
......
%li.notification-list-item
%span.notification.fa.fa-holder.append-right-5
- if setting.global?
= notification_icon(current_user.notification_level)
= notification_icon(current_user.global_notification_setting.level)
- else
= notification_icon(setting.level)
......
%li.notification-list-item
%span.notification.fa.fa-holder.append-right-5
- if setting.global?
= notification_icon(current_user.notification_level)
= notification_icon(current_user.global_notification_setting.level)
- else
= notification_icon(setting.level)
......
......@@ -26,33 +26,7 @@
= f.select :notification_email, @user.all_emails, { include_blank: false }, class: "select2"
.form-group
= f.label :notification_level, class: 'label-light'
.radio
= f.label :notification_level, value: :disabled do
= f.radio_button :notification_level, :disabled
.level-title
Disabled
%p You will not get any notifications via email
.radio
= f.label :notification_level, value: :mention do
= f.radio_button :notification_level, :mention
.level-title
On Mention
%p You will receive notifications only for comments in which you were @mentioned
.radio
= f.label :notification_level, value: :participating do
= f.radio_button :notification_level, :participating
.level-title
Participating
%p You will only receive notifications from related resources (e.g. from your commits or assigned issues)
.radio
= f.label :notification_level, value: :watch do
= f.radio_button :notification_level, :watch
.level-title
Watch
%p You will receive notifications for any activity
= notification_level_radio_buttons
.prepend-top-default
= f.submit 'Update settings', class: "btn btn-create"
......
class RemoveNotificationSettingNotNullConstraints < ActiveRecord::Migration
def up
change_column :notification_settings, :source_type, :string, null: true
change_column :notification_settings, :source_id, :integer, null: true
end
def down
change_column :notification_settings, :source_type, :string, null: false
change_column :notification_settings, :source_id, :integer, null: false
end
end
class MigrateUsersNotificationLevel < ActiveRecord::Migration
# Migrates only users who changed their default notification level :participating
# creating a new record on notification settings table
def up
execute(%Q{
INSERT INTO notification_settings
(user_id, level, created_at, updated_at)
(SELECT id, notification_level, created_at, updated_at FROM users WHERE notification_level != 1)
})
end
# Migrates from notification settings back to user notification_level
# If no value is found the default level of 1 will be used
def down
execute(%Q{
UPDATE users u SET
notification_level = COALESCE((SELECT level FROM notification_settings WHERE user_id = u.id AND source_type IS NULL), 1)
})
end
end
class RemoveNotificationLevelFromUsers < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
def change
remove_column :users, :notification_level, :integer
end
end
......@@ -10,7 +10,6 @@ RSpec.describe NotificationSetting, type: :model do
subject { NotificationSetting.new(source_id: 1, source_type: 'Project') }
it { is_expected.to validate_presence_of(:user) }
it { is_expected.to validate_presence_of(:source) }
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/) }
end
......
......@@ -72,6 +72,7 @@ describe NotificationService, services: true do
should_not_email(@u_disabled)
should_not_email(@unsubscriber)
should_not_email(@u_outsider_mentioned)
should_not_email(@u_lazy_participant)
end
it 'filters out "mentioned in" notes' do
......@@ -80,6 +81,20 @@ describe NotificationService, services: true do
expect(Notify).not_to receive(:note_issue_email)
notification.new_note(mentioned_note)
end
context 'participating' do
context 'by note' do
before do
ActionMailer::Base.deliveries.clear
note.author = @u_lazy_participant
note.save
notification.new_note(note)
end
it { should_not_email(@u_lazy_participant) }
end
end
end
describe 'new note on issue in project that belongs to a group' do
......@@ -106,6 +121,7 @@ describe NotificationService, services: true do
should_not_email(note.author)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
end
end
end
......@@ -235,6 +251,7 @@ describe NotificationService, services: true do
should_not_email(note.author)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
end
it do
......@@ -248,10 +265,11 @@ describe NotificationService, services: true do
should_not_email(note.author)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
end
it do
@u_committer.update_attributes(notification_level: :mention)
@u_committer = create_global_setting_for(@u_committer, :mention)
notification.new_note(note)
should_not_email(@u_committer)
end
......@@ -280,10 +298,11 @@ describe NotificationService, services: true do
should_not_email(@u_mentioned)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
end
it do
issue.assignee.update_attributes(notification_level: :mention)
create_global_setting_for(issue.assignee, :mention)
notification.new_issue(issue, @u_disabled)
should_not_email(issue.assignee)
......@@ -341,6 +360,7 @@ describe NotificationService, services: true do
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
end
it 'emails previous assignee even if he has the "on mention" notif level' do
......@@ -356,6 +376,7 @@ describe NotificationService, services: true do
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
end
it 'emails new assignee even if he has the "on mention" notif level' do
......@@ -371,6 +392,7 @@ describe NotificationService, services: true do
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
end
it 'emails new assignee' do
......@@ -386,6 +408,7 @@ describe NotificationService, services: true do
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
end
it 'does not email new assignee if they are the current user' do
......@@ -401,6 +424,35 @@ describe NotificationService, services: true do
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
end
context 'participating' do
context 'by assignee' do
before do
issue.update_attribute(:assignee, @u_lazy_participant)
notification.reassigned_issue(issue, @u_disabled)
end
it { should_email(@u_lazy_participant) }
end
context 'by note' do
let!(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: 'anything', author: @u_lazy_participant) }
before { notification.reassigned_issue(issue, @u_disabled) }
it { should_email(@u_lazy_participant) }
end
context 'by author' do
before do
issue.author = @u_lazy_participant
notification.reassigned_issue(issue, @u_disabled)
end
it { should_email(@u_lazy_participant) }
end
end
end
......@@ -479,6 +531,35 @@ describe NotificationService, services: true do
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
end
context 'participating' do
context 'by assignee' do
before do
issue.update_attribute(:assignee, @u_lazy_participant)
notification.close_issue(issue, @u_disabled)
end
it { should_email(@u_lazy_participant) }
end
context 'by note' do
let!(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: 'anything', author: @u_lazy_participant) }
before { notification.close_issue(issue, @u_disabled) }
it { should_email(@u_lazy_participant) }
end
context 'by author' do
before do
issue.author = @u_lazy_participant
notification.close_issue(issue, @u_disabled)
end
it { should_email(@u_lazy_participant) }
end
end
end
......@@ -495,6 +576,35 @@ describe NotificationService, services: true do
should_email(@watcher_and_subscriber)
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_lazy_participant)
end
context 'participating' do
context 'by assignee' do
before do
issue.update_attribute(:assignee, @u_lazy_participant)
notification.reopen_issue(issue, @u_disabled)
end
it { should_email(@u_lazy_participant) }
end
context 'by note' do
let!(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: 'anything', author: @u_lazy_participant) }
before { notification.reopen_issue(issue, @u_disabled) }
it { should_email(@u_lazy_participant) }
end
context 'by author' do
before do
issue.author = @u_lazy_participant
notification.reopen_issue(issue, @u_disabled)
end
it { should_email(@u_lazy_participant) }
end
end
end
end
......@@ -520,6 +630,7 @@ describe NotificationService, services: true do
should_email(@u_guest_watcher)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
end
it "emails subscribers of the merge request's labels" do
......@@ -530,6 +641,36 @@ describe NotificationService, services: true do
should_email(subscriber)
end
context 'participating' do
context 'by assignee' do
before do
merge_request.update_attribute(:assignee, @u_lazy_participant)
notification.new_merge_request(merge_request, @u_disabled)
end
it { should_email(@u_lazy_participant) }
end
context 'by note' do
let!(:note) { create(:note_on_issue, noteable: merge_request, project_id: project.id, note: 'anything', author: @u_lazy_participant) }
before { notification.new_merge_request(merge_request, @u_disabled) }
it { should_email(@u_lazy_participant) }
end
context 'by author' do
before do
merge_request.author = @u_lazy_participant
merge_request.save
notification.new_merge_request(merge_request, @u_disabled)
end
it { should_not_email(@u_lazy_participant) }
end
end
end
describe '#reassigned_merge_request' do
......@@ -545,6 +686,36 @@ describe NotificationService, services: true do
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
end
context 'participating' do
context 'by assignee' do
before do
merge_request.update_attribute(:assignee, @u_lazy_participant)
notification.reassigned_merge_request(merge_request, @u_disabled)
end
it { should_email(@u_lazy_participant) }
end
context 'by note' do
let!(:note) { create(:note_on_issue, noteable: merge_request, project_id: project.id, note: 'anything', author: @u_lazy_participant) }
before { notification.reassigned_merge_request(merge_request, @u_disabled) }
it { should_email(@u_lazy_participant) }
end
context 'by author' do
before do
merge_request.author = @u_lazy_participant
merge_request.save
notification.reassigned_merge_request(merge_request, @u_disabled)
end
it { should_email(@u_lazy_participant) }
end
end
end
......@@ -572,6 +743,7 @@ describe NotificationService, services: true do
should_not_email(@watcher_and_subscriber)
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)
end
......@@ -590,6 +762,36 @@ describe NotificationService, services: true do
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
end
context 'participating' do
context 'by assignee' do
before do
merge_request.update_attribute(:assignee, @u_lazy_participant)
notification.close_mr(merge_request, @u_disabled)
end
it { should_email(@u_lazy_participant) }
end
context 'by note' do
let!(:note) { create(:note_on_issue, noteable: merge_request, project_id: project.id, note: 'anything', author: @u_lazy_participant) }
before { notification.close_mr(merge_request, @u_disabled) }
it { should_email(@u_lazy_participant) }
end
context 'by author' do
before do
merge_request.author = @u_lazy_participant
merge_request.save
notification.close_mr(merge_request, @u_disabled)
end
it { should_email(@u_lazy_participant) }
end
end
end
......@@ -606,6 +808,36 @@ describe NotificationService, services: true do
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
end
context 'participating' do
context 'by assignee' do
before do
merge_request.update_attribute(:assignee, @u_lazy_participant)
notification.merge_mr(merge_request, @u_disabled)
end
it { should_email(@u_lazy_participant) }
end
context 'by note' do
let!(:note) { create(:note_on_issue, noteable: merge_request, project_id: project.id, note: 'anything', author: @u_lazy_participant) }
before { notification.merge_mr(merge_request, @u_disabled) }
it { should_email(@u_lazy_participant) }
end
context 'by author' do
before do
merge_request.author = @u_lazy_participant
merge_request.save
notification.merge_mr(merge_request, @u_disabled)
end
it { should_email(@u_lazy_participant) }
end
end
end
......@@ -622,6 +854,36 @@ describe NotificationService, services: true do
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
end
context 'participating' do
context 'by assignee' do
before do
merge_request.update_attribute(:assignee, @u_lazy_participant)
notification.reopen_mr(merge_request, @u_disabled)
end
it { should_email(@u_lazy_participant) }
end
context 'by note' do
let!(:note) { create(:note_on_issue, noteable: merge_request, project_id: project.id, note: 'anything', author: @u_lazy_participant) }
before { notification.reopen_mr(merge_request, @u_disabled) }
it { should_email(@u_lazy_participant) }
end
context 'by author' do
before do
merge_request.author = @u_lazy_participant
merge_request.save
notification.reopen_mr(merge_request, @u_disabled)
end
it { should_email(@u_lazy_participant) }
end
end
end
end
......@@ -640,6 +902,7 @@ describe NotificationService, services: true do
should_email(@u_watcher)
should_email(@u_participating)
should_email(@u_lazy_participant)
should_not_email(@u_guest_watcher)
should_not_email(@u_disabled)
end
......@@ -647,14 +910,19 @@ describe NotificationService, services: true do
end
def build_team(project)
@u_watcher = create(:user, notification_level: :watch)
@u_participating = create(:user, notification_level: :participating)
@u_participant_mentioned = create(:user, username: 'participant', notification_level: :participating)
@u_disabled = create(:user, notification_level: :disabled)
@u_mentioned = create(:user, username: 'mention', notification_level: :mention)
@u_committer = create(:user, username: 'committer')
@u_not_mentioned = create(:user, username: 'regular', notification_level: :participating)
@u_outsider_mentioned = create(:user, username: 'outsider')
@u_watcher = create_global_setting_for(create(:user), :watch)
@u_participating = create_global_setting_for(create(:user), :participating)
@u_participant_mentioned = create_global_setting_for(create(:user, username: 'participant'), :participating)
@u_disabled = create_global_setting_for(create(:user), :disabled)
@u_mentioned = create_global_setting_for(create(:user, username: 'mention'), :mention)
@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')
# User to be participant by default
# This user does not contain any record in notification settings table
# It should be treated with a :participating notification_level
@u_lazy_participant = create(:user, username: 'lazy-participant')
create_guest_watcher
......@@ -665,6 +933,15 @@ describe NotificationService, services: true do
project.team << [@u_mentioned, :master]
project.team << [@u_committer, :master]
project.team << [@u_not_mentioned, :master]
project.team << [@u_lazy_participant, :master]
end
def create_global_setting_for(user, level)
setting = user.global_notification_setting
setting.level = level
setting.save
user
end
def create_guest_watcher
......@@ -677,8 +954,8 @@ describe NotificationService, services: true do
def add_users_with_subscription(project, issuable)
@subscriber = create :user
@unsubscriber = create :user
@subscribed_participant = create(:user, username: 'subscribed_participant', notification_level: :participating)
@watcher_and_subscriber = create(:user, notification_level: :watch)
@subscribed_participant = create_global_setting_for(create(:user, username: 'subscribed_participant'), :participating)
@watcher_and_subscriber = create_global_setting_for(create(:user), :watch)
project.team << [@subscribed_participant, :master]
project.team << [@subscriber, :master]
......
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