Merge branch 'decouple-member-notification' into 'master'

Decouple membership and notifications

This allow you to have notification setting per project even if you are member of group. 
It also creates background for having notification settings in project you are not member of. 


- [x] Make it work
- [x] Migrations
- [x] CHANGELOG
- [x] More tests
- [x] API

For #3359 

After this merge request there is still some work to be done: 

* create migration that remove duplicates in notification settings table and create uniq index (8.8 probably)
* remove notification_level field from Member model in 9.0
* make proper API for notification settings
* use `MemberCreateService` instead of Member#after_create callback for creating notification settings (after #14709) 
* maybe more tests 


Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

See merge request !3421

CHANGELOG

@@ -31,6 +31,7 @@ v 8.7.0 (unreleased)
   - Hide `Create a group` help block when creating a new project in a group
   - Implement 'TODOs View' as an option for dashboard preferences !3379 (Elias W.)
   - Gracefully handle notes on deleted commits in merge requests (Stan Hu)
+  - Decouple membership and notifications
   - Fix creation of merge requests for orphaned branches (Stan Hu)
   - API: Ability to retrieve a single tag (Robert Schilling)
   - Fall back to `In-Reply-To` and `References` headers when sub-addressing is not available (David Padilla)

app/assets/javascripts/profile.js.coffee

@@ -18,8 +18,11 @@ class @Profile
       $(this).find('.btn-save').enable()
       $(this).find('.loading-gif').hide()
 
-    $('.update-notifications').on 'ajax:complete', ->
-      $(this).find('.btn-save').enable()
+    $('.update-notifications').on 'ajax:success', (e, data) ->
+      if data.saved
+        new Flash("Notification settings saved", "notice")
+      else
+        new Flash("Failed to save new settings", "alert")
 
     @bindEvents()
 

app/assets/javascripts/project.js.coffee

@@ -37,19 +37,20 @@ class @Project
     $('.update-notification').on 'click', (e) ->
       e.preventDefault()
       notification_level = $(@).data 'notification-level'
-      $('#notification_level').val(notification_level)
+      label = $(@).data 'notification-title'
+      $('#notification_setting_level').val(notification_level)
       $('#notification-form').submit()
-      label = null
-      switch notification_level
-        when 0 then label = ' Disabled '
-        when 1 then label = ' Participating '
-        when 2 then label = ' Watching '
-        when 3 then label = ' Global '
-        when 4 then label = ' On Mention '
       $('#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")
+
+
     @projectSelectDropdown()
 
   projectSelectDropdown: ->

app/controllers/groups/notification_settings_controller.rb

+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

app/controllers/profiles/notifications_controller.rb

 class Profiles::NotificationsController < Profiles::ApplicationController
   def show
     @user = current_user
-    @notification = current_user.notification
-    @project_members = current_user.project_members
-    @group_members = current_user.group_members
+    @group_notifications = current_user.notification_settings.for_groups
+    @project_notifications = current_user.notification_settings.for_projects
   end
 
   def update
-    type = params[:notification_type]
-
-    @saved = if type == 'global'
-               current_user.update_attributes(user_params)
-             elsif type == 'group'
-               group_member = current_user.group_members.find(params[:notification_id])
-               group_member.notification_level = params[:notification_level]
-               group_member.save
-             else
-               project_member = current_user.project_members.find(params[:notification_id])
-               project_member.notification_level = params[:notification_level]
-               project_member.save
-             end
-
-    respond_to do |format|
-      format.html do
-        if @saved
-          flash[:notice] = "Notification settings saved"
-        else
-          flash[:alert] = "Failed to save new settings"
-        end
-
-        redirect_back_or_default(default: profile_notifications_path)
-      end
-
-      format.js
+    if current_user.update_attributes(user_params)
+      flash[:notice] = "Notification settings saved"
+    else
+      flash[:alert] = "Failed to save new settings"
     end
+
+    redirect_back_or_default(default: profile_notifications_path)
   end
 
   def user_params

app/controllers/projects/notification_settings_controller.rb

+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: { saved: saved }
+  end
+
+  private
+
+  def notification_setting_params
+    params.require(:notification_setting).permit(:level)
+  end
+end

app/controllers/projects_controller.rb

@@ -101,14 +101,18 @@ class ProjectsController < Projects::ApplicationController
 
     respond_to do |format|
       format.html do
+        if current_user
+          @membership = @project.team.find_member(current_user.id)
+
+          if @membership
+            @notification_setting = current_user.notification_settings_for(@project)
+          end
+        end
+
         if @project.repository_exists?
           if @project.empty_repo?
             render 'projects/empty'
           else
-            if current_user
-              @membership = @project.team.find_member(current_user.id)
-            end
-
             render :show
           end
         else

app/helpers/notifications_helper.rb

 module NotificationsHelper
   include IconsHelper
 
-  def notification_icon(notification)
-    if notification.disabled?
-      icon('volume-off', class: 'ns-mute')
-    elsif notification.participating?
-      icon('volume-down', class: 'ns-part')
-    elsif notification.watch?
-      icon('volume-up', class: 'ns-watch')
-    else
-      icon('circle-o', class: 'ns-default')
+  def notification_icon_class(level)
+    case level.to_sym
+    when :disabled
+      'microphone-slash'
+    when :participating
+      'volume-up'
+    when :watch
+      'eye'
+    when :mention
+      'at'
+    when :global
+      'globe'
     end
   end
 
-  def notification_list_item(notification_level, user_membership)
-    case notification_level
-    when Notification::N_DISABLED
-      update_notification_link(Notification::N_DISABLED, user_membership, 'Disabled', 'microphone-slash')
-    when Notification::N_PARTICIPATING
-      update_notification_link(Notification::N_PARTICIPATING, user_membership, 'Participate', 'volume-up')
-    when Notification::N_WATCH
-      update_notification_link(Notification::N_WATCH, user_membership, 'Watch', 'eye')
-    when Notification::N_MENTION
-      update_notification_link(Notification::N_MENTION, user_membership, 'On mention', 'at')
-    when Notification::N_GLOBAL
-      update_notification_link(Notification::N_GLOBAL, user_membership, 'Global', 'globe')
-    else
-      # do nothing
-    end
+  def notification_icon(level, text = nil)
+    icon("#{notification_icon_class(level)} fw", text: text)
   end
 
-  def update_notification_link(notification_level, user_membership, title, icon)
-    content_tag(:li, class: active_level_for(user_membership, notification_level)) do
-      link_to '#', class: 'update-notification', data: { notification_level: notification_level } do
-        icon("#{icon} fw", text: title)
-      end
+  def notification_title(level)
+    case level.to_sym
+    when :participating
+      'Participate'
+    when :mention
+      'On mention'
+    else
+      level.to_s.titlecase
     end
   end
 
-  def notification_label(user_membership)
-    Notification.new(user_membership).to_s
-  end
+  def notification_list_item(level, setting)
+    title = notification_title(level)
+
+    data = {
+      notification_level: level,
+      notification_title: title
+    }
 
-  def active_level_for(user_membership, level)
-    'active' if user_membership.notification_level == level
+    content_tag(:li, class: ('active' if setting.level == level)) do
+      link_to '#', class: 'update-notification', data: data do
+        notification_icon(level, title)
+      end
+    end
   end
 end

app/models/concerns/notifiable.rb

-# == Notifiable concern
-#
-# Contains notification functionality
-#
-module Notifiable
-  extend ActiveSupport::Concern
-
-  included do
-    validates :notification_level, inclusion: { in: Notification.project_notification_levels }, presence: true
-  end
-
-  def notification
-    @notification ||= Notification.new(self)
-  end
-end

app/models/group.rb

@@ -27,6 +27,7 @@ class Group < Namespace
   has_many :users, through: :group_members
   has_many :project_group_links, dependent: :destroy
   has_many :shared_projects, through: :project_group_links, source: :project
+  has_many :notification_settings, dependent: :destroy, as: :source
 
   validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
   validate :visibility_level_allowed_by_projects

app/models/member.rb

@@ -19,7 +19,6 @@
 
 class Member < ActiveRecord::Base
   include Sortable
-  include Notifiable
   include Gitlab::Access
 
   attr_accessor :raw_invite_token
@@ -56,12 +55,15 @@ class Member < ActiveRecord::Base
 
   before_validation :generate_invite_token, on: :create, if: -> (member) { member.invite_email.present? }
   after_create :send_invite, if: :invite?
+  after_create :create_notification_setting, unless: :invite?
   after_create :post_create_hook, unless: :invite?
   after_update :post_update_hook, unless: :invite?
   after_destroy :post_destroy_hook, unless: :invite?
 
   delegate :name, :username, :email, to: :user, prefix: true
 
+  default_value_for :notification_level, NotificationSetting.levels[:global]
+
   class << self
     def find_by_invite_token(invite_token)
       invite_token = Devise.token_generator.digest(self, :invite_token, invite_token)
@@ -160,6 +162,14 @@ class Member < ActiveRecord::Base
     send_invite
   end
 
+  def create_notification_setting
+    user.notification_settings.find_or_create_for(source)
+  end
+
+  def notification_setting
+    @notification_setting ||= user.notification_settings_for(source)
+  end
+
   private
 
   def send_invite

app/models/members/group_member.rb

@@ -24,7 +24,6 @@ class GroupMember < Member
 
   # Make sure group member points only to group as it source
   default_value_for :source_type, SOURCE_TYPE
-  default_value_for :notification_level, Notification::N_GLOBAL
   validates_format_of :source_type, with: /\ANamespace\z/
   default_scope { where(source_type: SOURCE_TYPE) }
 

app/models/members/project_member.rb

@@ -27,7 +27,6 @@ class ProjectMember < Member
 
   # Make sure project member points only to project as it source
   default_value_for :source_type, SOURCE_TYPE
-  default_value_for :notification_level, Notification::N_GLOBAL
   validates_format_of :source_type, with: /\AProject\z/
   default_scope { where(source_type: SOURCE_TYPE) }
 

app/models/notification.rb

-class Notification
-  #
-  # Notification levels
-  #
-  N_DISABLED = 0
-  N_PARTICIPATING = 1
-  N_WATCH = 2
-  N_GLOBAL = 3
-  N_MENTION = 4
-
-  attr_accessor :target
-
-  class << self
-    def notification_levels
-      [N_DISABLED, N_MENTION, N_PARTICIPATING, N_WATCH]
-    end
-
-    def options_with_labels
-      {
-        disabled: N_DISABLED,
-        participating: N_PARTICIPATING,
-        watch: N_WATCH,
-        mention: N_MENTION,
-        global: N_GLOBAL
-      }
-    end
-
-    def project_notification_levels
-      [N_DISABLED, N_MENTION, N_PARTICIPATING, N_WATCH, N_GLOBAL]
-    end
-  end
-
-  def initialize(target)
-    @target = target
-  end
-
-  def disabled?
-    target.notification_level == N_DISABLED
-  end
-
-  def participating?
-    target.notification_level == N_PARTICIPATING
-  end
-
-  def watch?
-    target.notification_level == N_WATCH
-  end
-
-  def global?
-    target.notification_level == N_GLOBAL
-  end
-
-  def mention?
-    target.notification_level == N_MENTION
-  end
-
-  def level
-    target.notification_level
-  end
-  
-  def to_s
-    case level
-    when N_DISABLED
-      'Disabled'
-    when N_PARTICIPATING
-      'Participating'
-    when N_WATCH
-      'Watching'
-    when N_MENTION
-      'On mention'
-    when N_GLOBAL
-      'Global'
-    else
-      # do nothing      
-    end
-  end
-end

app/models/notification_setting.rb

+class NotificationSetting < ActiveRecord::Base
+  enum level: { disabled: 0,  participating: 1,  watch: 2,  global: 3, mention: 4 }
+
+  default_value_for :level, NotificationSetting.levels[:global]
+
+  belongs_to :user
+  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",
+                                    allow_nil: true }
+
+  scope :for_groups, -> { where(source_type: 'Namespace') }
+  scope :for_projects, -> { where(source_type: 'Project') }
+
+  def self.find_or_create_for(source)
+    setting = find_or_initialize_by(source: source)
+
+    unless setting.persisted?
+      setting.save
+    end
+
+    setting
+  end
+end

app/models/project.rb

@@ -154,6 +154,7 @@ class Project < ActiveRecord::Base
   has_many :project_group_links, dependent: :destroy
   has_many :invited_groups, through: :project_group_links, source: :group
   has_many :todos, dependent: :destroy
+  has_many :notification_settings, dependent: :destroy, as: :source
 
   has_one :import_data, dependent: :destroy, class_name: "ProjectImportData"
 

app/models/user.rb

@@ -143,6 +143,7 @@ class User < ActiveRecord::Base
   has_many :spam_logs,                dependent: :destroy
   has_many :builds,                   dependent: :nullify, class_name: 'Ci::Build'
   has_many :todos,                    dependent: :destroy
+  has_many :notification_settings,    dependent: :destroy
 
   #
   # Validations
@@ -157,7 +158,7 @@ class User < ActiveRecord::Base
     presence: true,
     uniqueness: { case_sensitive: false }
 
-  validates :notification_level, inclusion: { in: Notification.notification_levels }, presence: true
+  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? }
@@ -190,6 +191,13 @@ 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
@@ -349,10 +357,6 @@ class User < ActiveRecord::Base
     "#{self.class.reference_prefix}#{username}"
   end
 
-  def notification
-    @notification ||= Notification.new(self)
-  end
-
   def generate_password
     if self.force_random_password
       self.password = self.password_confirmation = Devise.friendly_token.first(8)
@@ -827,6 +831,10 @@ class User < ActiveRecord::Base
     end
   end
 
+  def notification_settings_for(source)
+    notification_settings.find_or_initialize_by(source: source)
+  end
+
   private
 
   def projects_union

app/services/notification_service.rb

@@ -253,8 +253,8 @@ class NotificationService
   def project_watchers(project)
     project_members = project_member_notification(project)
 
-    users_with_project_level_global = project_member_notification(project, Notification::N_GLOBAL)
-    users_with_group_level_global = group_member_notification(project, Notification::N_GLOBAL)
+    users_with_project_level_global = project_member_notification(project, :global)
+    users_with_group_level_global = group_member_notification(project, :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)
@@ -264,18 +264,16 @@ class NotificationService
   end
 
   def project_member_notification(project, notification_level=nil)
-    project_members = project.project_members
-
     if notification_level
-      project_members.where(notification_level: notification_level).pluck(:user_id)
+      project.notification_settings.where(level: NotificationSetting.levels[notification_level]).pluck(:user_id)
     else
-      project_members.pluck(:user_id)
+      project.notification_settings.pluck(:user_id)
     end
   end
 
   def group_member_notification(project, notification_level)
     if project.group
-      project.group.group_members.where(notification_level: notification_level).pluck(:user_id)
+      project.group.notification_settings.where(level: NotificationSetting.levels[notification_level]).pluck(:user_id)
     else
       []
     end
@@ -284,13 +282,13 @@ class NotificationService
   def users_with_global_level_watch(ids)
     User.where(
       id: ids,
-      notification_level: Notification::N_WATCH
+      notification_level: NotificationSetting.levels[:watch]
     ).pluck(:id)
   end
 
   # Build a list of users based on project notifcation settings
   def select_project_member_setting(project, global_setting, users_global_level_watch)
-    users = project_member_notification(project, Notification::N_WATCH)
+    users = project_member_notification(project, :watch)
 
     # If project setting is global, add to watch list if global setting is watch
     global_setting.each do |user_id|
@@ -304,7 +302,7 @@ class NotificationService
 
   # Build a list of users based on group notification settings
   def select_group_member_setting(project, project_members, global_setting, users_global_level_watch)
-    uids = group_member_notification(project, Notification::N_WATCH)
+    uids = group_member_notification(project, :watch)
 
     # Group setting is watch, add to users list if user is not project member
     users = []
@@ -331,40 +329,46 @@ class NotificationService
   # Remove users with disabled notifications from array
   # Also remove duplications and nil recipients
   def reject_muted_users(users, project = nil)
-    reject_users(users, :disabled?, project)
+    reject_users(users, :disabled, project)
   end
 
   # Remove users with notification level 'Mentioned'
   def reject_mention_users(users, project = nil)
-    reject_users(users, :mention?, project)
+    reject_users(users, :mention, project)
   end
 
-  # Reject users which method_name from notification object returns true.
+  # Reject users which has certain notification level
   #
   # Example:
-  #   reject_users(users, :watch?, project)
+  #   reject_users(users, :watch, project)
   #
-  def reject_users(users, method_name, project = nil)
+  def reject_users(users, level, project = nil)
+    level = level.to_s
+
+    unless NotificationSetting.levels.keys.include?(level)
+      raise 'Invalid notification level'
+    end
+
     users = users.to_a.compact.uniq
     users = users.reject(&:blocked?)
 
     users.reject do |user|
-      next user.notification.send(method_name) unless project
+      next user.notification_level == level unless project
 
-      member = project.project_members.find_by(user_id: user.id)
+      setting = user.notification_settings_for(project)
 
-      if !member && project.group
-        member = project.group.group_members.find_by(user_id: user.id)
+      if !setting && project.group
+        setting = user.notification_settings_for(project.group)
       end
 
-      # reject users who globally set mention notification and has no membership
-      next user.notification.send(method_name) unless member
+      # reject users who globally set mention notification and has no setting per project/group
+      next user.notification_level == level unless setting
 
       # reject users who set mention notification in project
-      next true if member.notification.send(method_name)
+      next true if setting.level == level
 
-      # reject users who have N_MENTION in project and disabled in global settings
-      member.notification.global? && user.notification.send(method_name)
+      # reject users who have mention level in project and disabled in global settings
+      setting.global? && user.notification_level == level
     end
   end
 

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)
+    - else
+      = notification_icon(setting.level)
+
+  %span.str-truncated
+    = 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'

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)
+    - else
+      = notification_icon(setting.level)
+
+  %span.str-truncated
+    = 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'