Remove notification level fild from users, improve migrations and specsissue_3359_2

9 files changed, 55 insertions, 57 deletions

diff --git a/app/controllers/profiles/notifications_controller.rb b/app/controllers/profiles/notifications_controller.rb
index 1e9ceb87857..40d1906a53f 100644
--- a/app/controllers/profiles/notifications_controller.rb
+++ b/app/controllers/profiles/notifications_controller.rb
@@ -17,21 +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 notification_params
-    params.require(:notification_level)
+  def global_notification_setting_params
+    params.require(:global_notification_setting).permit(:level)
   end
 
   private
 
   def update_notification_settings
-    return true unless notification_params
+    return true unless global_notification_setting_params
 
-    notification_setting = current_user.global_notification_setting
-    notification_setting.level = notification_params
-
-    notification_setting.save
+    current_user.global_notification_setting.update_attributes(global_notification_setting_params)
   end
 end
diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb
index 9769458f79e..50c21fc0d49 100644
--- a/app/helpers/notifications_helper.rb
+++ b/app/helpers/notifications_helper.rb
@@ -71,26 +71,13 @@ module NotificationsHelper
 
       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) +
+          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_level_description(level))
+          content_tag(:p, notification_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
diff --git a/app/models/user.rb b/app/models/user.rb
index d2da83ab4b3..7afbfbf112a 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -797,9 +797,12 @@ class User < ActiveRecord::Base
   # 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
+    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)
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index af7bbe37439..875a3f4fab6 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -353,7 +353,9 @@ class NotificationService
     users = users.reject(&:blocked?)
 
     users.reject do |user|
-      next user.global_notification_setting.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)
 
@@ -362,13 +364,13 @@ class NotificationService
       end
 
       # reject users who globally set mention notification and has no setting per project/group
-      next user.global_notification_setting.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.global_notification_setting.level == level
+      setting.global? && global_notification_setting.level == level
     end
   end
 
diff --git a/db/migrate/20160607201627_migrate_users_notification_level.rb b/db/migrate/20160607201627_migrate_users_notification_level.rb
deleted file mode 100644
index 7417d66fef7..00000000000
--- a/db/migrate/20160607201627_migrate_users_notification_level.rb
+++ /dev/null
@@ -1,24 +0,0 @@
-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
diff --git a/db/migrate/20160606192159_remove_notification_setting_not_null_constraints.rb b/db/migrate/20160610140403_remove_notification_setting_not_null_constraints.rb
index c20ac9acdc2..259abb08e47 100644
--- a/db/migrate/20160606192159_remove_notification_setting_not_null_constraints.rb
+++ b/db/migrate/20160610140403_remove_notification_setting_not_null_constraints.rb
@@ -2,6 +2,10 @@ 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
+
+  def down
+    change_column :notification_settings, :source_type, :string, null: false
+    change_column :notification_settings, :source_id, :integer, null: false
   end
 end
diff --git a/db/migrate/20160610201627_migrate_users_notification_level.rb b/db/migrate/20160610201627_migrate_users_notification_level.rb
new file mode 100644
index 00000000000..760b766828e
--- /dev/null
+++ b/db/migrate/20160610201627_migrate_users_notification_level.rb
@@ -0,0 +1,21 @@
+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
diff --git a/db/migrate/20160610301627_remove_notification_level_from_users.rb b/db/migrate/20160610301627_remove_notification_level_from_users.rb
new file mode 100644
index 00000000000..8afb14df2cf
--- /dev/null
+++ b/db/migrate/20160610301627_remove_notification_level_from_users.rb
@@ -0,0 +1,7 @@
+class RemoveNotificationLevelFromUsers < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  def change
+    remove_column :users, :notification_level, :integer
+  end
+end
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 5a9a9d62a15..b99e02ba678 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -85,13 +85,14 @@ describe NotificationService, services: true do
         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_email(@u_lazy_participant) }
+            it { should_not_email(@u_lazy_participant) }
           end
         end
       end
@@ -953,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]