Remove notification level from user model

CHANGELOG

@@ -54,6 +54,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.
   - An indicator is now displayed at the top of the comment field for confidential issues.
   - RepositoryCheck::SingleRepositoryWorker public and private methods are now instrumented

app/controllers/profiles/notifications_controller.rb

@@ -3,10 +3,11 @@ class Profiles::NotificationsController < Profiles::ApplicationController
     @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"
@@ -18,4 +19,19 @@ class Profiles::NotificationsController < Profiles::ApplicationController
   def user_params
     params.require(:user).permit(:notification_email, :notification_level)
   end
+
+  def notification_params
+    params.require(:notification_level)
+  end
+
+  private
+
+  def update_notification_settings
+    return true unless notification_params
+
+    notification_setting = current_user.global_notification_setting
+    notification_setting.level = notification_params
+
+    notification_setting.save
+  end
 end

app/helpers/notifications_helper.rb

@@ -61,4 +61,36 @@ 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(:notification_level, level, @global_notification_setting.level.to_sym == level) +
+          content_tag(:div, level.to_s.capitalize, class: "level-title") +
+          content_tag(:p, notification_level_description(level))
+        end
+      end
+    end
+
+    html.html_safe
+  end
+
+  def notification_level_description(level)
+    case level
+    when :disabled
+      "You will not get any notifications via email"
+    when :mention
+      "You will receive notifications only for comments in which you were @mentioned"
+    when :participating
+      "You will only receive notifications from related resources (e.g. from your commits or assigned issues)"
+    when :watch
+      "You will receive notifications for any activity"
+    end
+  end
 end

app/models/notification_setting.rb

@@ -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",

app/models/user.rb

@@ -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,14 @@ 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
+    setting = notification_settings.find_or_initialize_by(source: nil)
+    setting.level = NotificationSetting.levels[DEFAULT_NOTIFICATION_LEVEL] unless setting.persisted?
+    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

app/services/notification_service.rb

@@ -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,7 @@ class NotificationService
     users = users.reject(&:blocked?)
 
     users.reject do |user|
-      next user.notification_level == level unless project
+      next user.global_notification_setting.level == level unless project
 
       setting = user.notification_settings_for(project)
 
@@ -361,13 +362,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 user.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? && user.global_notification_setting.level == level
     end
   end
 
@@ -456,7 +457,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)
 

app/views/profiles/notifications/_group_settings.html.haml

 %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)
 

app/views/profiles/notifications/_project_settings.html.haml

 %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)
 

app/views/profiles/notifications/show.html.haml

@@ -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"

db/migrate/20160606192159_remove_notification_setting_not_null_constraints.rb

+class RemoveNotificationSettingNotNullConstraints < ActiveRecord::Migration
+  def up
+    change_column :notification_settings, :source_type, :string, null: true
+    change_column :notification_settings, :source_id, :integer, null: true
+    change_column :users, :notification_level, :integer, null: true
+  end
+end

db/migrate/20160607201627_migrate_users_notification_level.rb

+class MigrateUsersNotificationLevel < ActiveRecord::Migration
+  # Migrates only users which changes theier default notification level :participating
+  # creating a new record on notification settins table
+
+  def up
+    changed_users = exec_query(%Q{
+      SELECT id, notification_level
+      FROM users
+      WHERE notification_level != 1
+    })
+
+    changed_users.each do |row|
+      uid = row['id']
+      u_notification_level = row['notification_level']
+
+      execute(%Q{
+        INSERT INTO notification_settings
+          (user_id, level, created_at, updated_at)
+        VALUES
+          (#{uid}, #{u_notification_level}, now(), now())
+      })
+    end
+  end
+end

spec/models/notification_setting_spec.rb

@@ -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

spec/services/notification_service_spec.rb

@@ -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,19 @@ 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
+              note.author = @u_lazy_participant
+              note.save
+              notification.new_note(note)
+            end
+
+
+            it { should_email(@u_lazy_participant) }
+          end
+        end
       end
 
       describe 'new note on issue in project that belongs to a group' do
@@ -106,6 +120,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 +250,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 +264,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 +297,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 +359,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 +375,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 +391,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 +407,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 +423,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 +530,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 +575,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 +629,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 +640,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 +685,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 +742,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 +761,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 +807,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 +853,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 +901,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,15 +909,20 @@ 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_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(:user, username: 'regular', notification_level: :participating)
+    @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
 
     project.team << [@u_watcher, :master]
@@ -665,6 +932,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