Skip to content
Snippets Groups Projects

Decouple membership and notifications

Merged Dmytro Zaporozhets (DZ) requested to merge decouple-member-notification into master

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.

  • Make it work
  • Migrations
  • CHANGELOG
  • More tests
  • API

For #3359 (closed)

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 (moved))
  • maybe more tests

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

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • 56 55
    57 56 before_validation :generate_invite_token, on: :create, if: -> (member) { member.invite_email.present? }
    58 57 after_create :send_invite, if: :invite?
    58 after_create :create_notification_setting, unless: :invite?
  • 1 require 'rails_helper'
    2
    3 RSpec.describe NotificationSetting, type: :model do
    4 describe "Associations" do
    5 it { is_expected.to belong_to(:user) }
    6 end
    7
    8 describe "Validation" do
    9 subject { NotificationSetting.new }
    10
    11 it { is_expected.to validate_presence_of(:user) }
    12 it { is_expected.to validate_presence_of(:source) }
    13 it { is_expected.to validate_presence_of(:level) }
  • @dzaporozhets It seems like there's now a lot of duplication between the helpers, the new model, and the Notification model (see Notification.notification_levels, Notification.options_with_labels, etc.

  • @rspeicher Notification should be gone in the end. I don't see duplication between new model and helpers so should be ok when I finish.

  • @rspeicher btw thank you for early review

  • Added 1 commit:

    • 53e17165 - Small refactoring and cleanup of notification logic
  • Added 1 commit:

    • 49b667fb - Improve db migrations for notification settings
  • Added 1 commit:

    • 1c46eb26 - Refactor notification helper and fix notification service
  • Added 1 commit:

    • 7bca2854 - Remove useless Notification model
  • Added 1 commit:

    • 7979226f - Add spec for user_id uniq in NotificationSetting model
  • Added 1 commit:

    • af6442b0 - Refactor creating notification setting with defaults
  • mentioned in issue #14709 (moved)

  • Added 1 commit:

    • 7ee5c45a - Fix few bugs related to recent notifications refactoring
  • About migration path:

    1. We run db migration that will create NotificationSetting for each Member row we have. This will ensure users have same notification settings they had before migration
    2. We create NotificationSetting object if you visit project home page and you are a member of this project/group. This will ensure users who are members of group can change notification settings of projects they have access to.
    3. We create NotificationSetting object every time new "membership" object is created. That means if you join group you will receive NotificationSetting only for group.

    This cause certain behaviour we might want to discuss:

    1. when you join group NotificationSetting created for group you joined. However if you want specific notification for single project inside this group you need to visit this project home page (where notification dropdown is) once. Basically this has good side because if you join group with 100+ projects and you never visited most of them - there is no reason to create notification setting per each. Its more efficient from database perspective, performance (creating NotificationSetting for 100 projects) and UI (notification page flooded with all this projects you don't care about). However you might want to disagree.
    2. If we run migration online it might produce duplicate NotificationSetting for same project as one came from migration and another during project page visit. We can add uniq database index on user_id, source_id, source_type combination to prevent this and any possible race condition.

    We can go different way and don't create NotificationSetting when visit project page you are member of. However I see next disadvantages of this way:

    • we need to monitor creating of new projects in group and immediately create NotificationSetting for each group member
    • we need to create NotificationSetting for each existing project inside group you are member of during migration
    • we need to think about notifications page design immediately as it means having every single project from group there

    Summary

    There are 2 ways of doing notifications for projects:

    • easy and lazy - when person visit project home page they can change notification there or at profile notifications page
    • difficult - make sure every project you have access to has own notification setting created even before (if ever) you visited this project to change it

    cc @DouweM @rspeicher @yorickpeterse @dblessing

    Edited by Dmytro Zaporozhets (DZ)
  • @dzaporozhets Ideally no records are created when merely visiting a page. The check of a presence of one of these rows would have to stick around for all eternity which in turn means we're wasting time checking data that might be already present in 95% of the cases.

    Perhaps the easiest way is to start writing a migration that adds all records during the migration, without any lazy creation. We can then measure the impact of this on a database the size of GitLab.com to see how long it would take. If this were to complete fast enough (e.g. a few minutes) I'd consider it good enough, though it would require downtime for the duration. As a guideline we could go with a maximum of 5 minutes for GitLab.com.

  • Ideally no records are created when merely visiting a page.

    @yorickpeterse in theory yes. However if you don't need resource unless you visit page A to change it does not make sense to create resource anyway.

    The check of a presence of one of these rows would have to stick around for all eternity which in turn means we're wasting time checking data that might be already present in 95% of the cases.

    @yorickpeterse I think I explained it wrong. We load data if exists because we need it on this page. If it does not exists we will create it because we need it. This will stay for all time being. And I don't see how we waste or time here. You do @setting = find. If @setting is nil then we create it. Because on this page we will use it anyway.

    Perhaps the easiest way is to start writing a migration that adds all records during the migration, without any lazy creation.

    The migration already exists and it adds all records that are corresponding to existing membership. Lazy creation is about not existing records that we want to exist only when people want it to exist so people start getting benefit from decouple notification and membership. Feel free to ask for more explanation if its unclear to you.

  • The other approach would be to create NotificationSetting on project page only when you try to select different level then global in dropdown. This means that if person wants to change notification level for specific project they should use only projects page (not /profile/notifications) for case when they are member of group without member of project.

  • @dzaporozhets

    I think I explained it wrong. We load data if exists because we need it on this page. If it does not exists we will create it because we need it. This will stay for all time being. And I don't see how we waste or time here. You do @setting = find. If @setting is nil then we create it. Because on this page we will use it anyway.

    Ah, I indeed misunderstood it. This clears things up, and I think this approach is fine.

  • I will try to make it clear with picture.

    This is project notification dropdown:

    Screenshot_2016-03-29_15.54.05

    Default value is Global. NotificationSetting object makes sense only if value is different from Global. Otherwise there is no need for object to exist. We can create object at different stage. It can be one of this:

    1. Long before user want (or does not) to open project page. (complicated and wasteful)
    2. When user open project page. (more efficient but does not appear on /profile/notification unless you visit project first)
    3. When user change value in dropdown (most efficient but does not appear on /profile/notification unless you make change in dropdown first)

    I am strongly against 1 unless someone have a good reason. I like 3 because it creates object only when you change value instead of page visit. However require some help tip on /profile/notification to explain behaviour.

    Edited by Dmytro Zaporozhets (DZ)
  • Ah, I indeed misunderstood it. This clears things up, and I think this approach is fine.

    @yorickpeterse cool. Thanks. What do you think about https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/3421#note_4479217 ?

  • @dzaporozhets I think option 3 makes the most sense, though it would indeed at the very least require some sort of tooltip to explain the behaviour.

  • @yorickpeterse Thanks for opinion.

  • Added 1 commit:

    • 75853ad5 - Create NotificationSettings object only when user change value in dropdown
  • Added 2 commits:

    • 6a10e2ac - Improve project notification settings explanation
    • b6a4a600 - Change how notification settings in profile are rendered and updated
  • 1 class CreateNotificationSettings < ActiveRecord::Migration
    2 def change
    3 create_table :notification_settings do |t|
    4 t.references :user, null: false
    5 t.references :source, polymorphic: true, null: false
    6 t.integer :level, default: 0, null: false
    7
    8 t.timestamps null: false
    • @yorickpeterse @jacobvosmaer if I add add_index :notification_settings, [:user_id, :source_id, :source_type], uniq: true here would it be a problem with next migration performance? And what happens if during next migration (which can take some time) user try to create same record via UI? Will it crash migration?

      The idea is to put uniq index to prevent duplicate rows. However I don't want make migration offline. Either I add index during table creation or I can add index after 20160328115649_migrate_new_notification_setting.rb but in this case I need to remove duplicates which can possibly appear during online user activity and 20160328115649_migrate_new_notification_setting.rb.

      Any advice?

    • Adding the index before the data is created will slow things down though I think it's worth it considering the alternative is duplicate data (and having to remove that).

      What we can do is extract the migration code into a standalone script and run it on GitLab.com. Since we're populating a new table which isn't used yet at that time (assuming we do this before merging and deploying the changes) it shouldn't affect any users. We can then measure the timings and make a final decision.

      Edited by Yorick Peterse
    • @yorickpeterse since you are closer to operations/performance/deploy I leave the decision up to you. I think having duplicates and removing them later is also OK. My goal is to get this MR into the RC1 so we have more chances to find and fix bugs before release.

  • @yorickpeterse btw this is something that we might want for RC1

  • Dmytro Zaporozhets (DZ) Marked the task Make it work as completed

    Marked the task Make it work as completed

  • @jschatz1 please review frontend part.

    @rspeicher please take a look one more time

  • Added 2 commits:

    • 69ca45c2 - Update API to use notification_level from notification_setting
    • dea8a672 - Add changelog item
  • Dmytro Zaporozhets (DZ) Marked the task CHANGELOG as completed

    Marked the task CHANGELOG as completed

  • Dmytro Zaporozhets (DZ) Marked the task API as completed

    Marked the task API as completed

  • Dmytro Zaporozhets (DZ) Marked the task API as completed

    Marked the task API as completed

  • Dmytro Zaporozhets (DZ) Marked the task CHANGELOG as completed

    Marked the task CHANGELOG as completed

  • Added 2 commits:

    • ca63fa2f - Test changing notification settings per project fron notificaitons page
    • 52aaa087 - Update migration comment
  • Dmytro Zaporozhets (DZ) Marked the task More tests as completed

    Marked the task More tests as completed

  • Dmytro Zaporozhets (DZ) Marked the task Migrations as completed

    Marked the task Migrations as completed

  • Dmytro Zaporozhets (DZ) Unmarked this merge request as a Work In Progress

    Unmarked this merge request as a Work In Progress

  • Added 115 commits:

    • 52aaa087...a9346cab - 96 commits from branch master
    • 31b0e530 - Introduce NotificationSetting model
    • 73c5a341 - Migrate notification setting from members table
    • 359157c0 - Introduce NotificationSetting to user interface
    • 7ea1bcab - Create notification setting when membership created
    • b8f38437 - Update NotificationService to use NotificationSettings instead of membership
    • 4ca73f56 - Small refactoring and cleanup of notification logic
    • 855b2820 - Improve db migrations for notification settings
    • 08b3d7f6 - Refactor notification helper and fix notification service
    • 86418c47 - Remove useless Notification model
    • 630c86a7 - Add spec for user_id uniq in NotificationSetting model
    • 71e7b398 - Refactor creating notification setting with defaults
    • f8f68d6b - Fix few bugs related to recent notifications refactoring
    • 5583197e - Create NotificationSettings object only when user change value in dropdown
    • 729fe42b - Improve project notification settings explanation
    • 26631f99 - Change how notification settings in profile are rendered and updated
    • 949431fa - Update API to use notification_level from notification_setting
    • 49f9873c - Add changelog item
    • 065faac3 - Test changing notification settings per project fron notificaitons page
    • 1a293a43 - Update migration comment
  • Added 3 commits:

  • mentioned in merge request !3452 (merged)

  • Reassigned to @DouweM

  • @dzaporozhets According to the design they should be green. Currently gray.

    Design: ss__2016-03-30_at_10.11.11_AM

    Impl: ss__2016-03-30_at_10.10.41_AM

    Edited by Jacob Schatz
  • Also as above those links should be black.

  • 37 37 $('.update-notification').on 'click', (e) ->
    38 38 e.preventDefault()
    39 39 notification_level = $(@).data 'notification-level'
    40 $('#notification_level').val(notification_level)
    40 label = $(@).data 'notification-title'
    41 $('#notification_setting_level').val(notification_level)
  • @dzaporozhets When you change a radio the notification that it saved successfully shows up at the top. Maybe it should show up in this section. When it shows up up top no one will ever see it unless they scroll there themselves.

    ss__2016-03-30_at_10.17.14_AM

  • 31 .level-title
    32 Disabled
    33 %p You will not get any notifications via email
    34 23
    35 .radio
    36 = f.label :notification_level, value: Notification::N_MENTION do
    37 = f.radio_button :notification_level, Notification::N_MENTION
    38 .level-title
    39 On Mention
    40 %p You will receive notifications only for comments in which you were @mentioned
    24 = form_for @user, url: profile_notifications_path, method: :put, html: { class: 'update-notifications prepend-top-default' } do |f|
    25 .form-group
    26 = f.label :notification_email, class: "label-light"
    27 = f.select :notification_email, @user.all_emails, { include_blank: false }, class: "select2"
    28 .form-group
    29 = f.label :notification_level, class: 'label-light'
    • @dzaporozhets This should be a legend, not a label. The whole thing should be a fieldset. Because the label tag defines a label for an input element. You are using it here to group stuff. Same elsewhere. Maybe you did not write this but it should still be fixed.

  • @dzaporozhets I took at look at the JS and HAML. Made some comments. Everything else looks great!

  • Also @dzaporozhets when you select from the selects you get a little shift in the page.

  • when you select from the selects you get a little shift in the page.

    @jschatz1 because of flash message.

    According to the design they should be green. Currently gray.

    Designer just copied what it was before. I added this green a year ago :smiley: Since we use gray on project page its time to bring consistency here too.

  • Maybe it should show up in this section. When it shows up up top no one will ever see it unless they scroll there themselves.

    @jschatz1 Agree. I just used existing Flash message. It was on purpose to keep this MR as small as possible. Maybe highlight row that was changed instead of Flash in future?

    Edited by Dmytro Zaporozhets (DZ)
  • mentioned in issue #3359 (closed)

  • @DouweM I think this one is ready to merge. It would be better to have it earlier into RC so we have extra time to fix things

    Edited by Dmytro Zaporozhets (DZ)
  • @dzaporozhets Awesome DZ. I'll review it tomorrow.

  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 190 191 # Note: When adding an option, it MUST go on the end of the array.
    191 192 enum project_view: [:readme, :activity, :files]
    192 193
    194 # Notification level
    195 # Note: When adding an option, it MUST go on the end of the array.
    196 #
    197 # TODO: Add '_prefix: :notification' to enum when update to Rails 5. https://github.com/rails/rails/pull/19813
    198 # Because user.notification_disabled? is much better than user.disabled?
    199 enum notification_level: [:disabled, :participating, :watch, :global, :mention]
    • Instead of storing the notification_level like this, how about having user have one NotificationSetting with a nil source? When we start adding more notification options, we need to be able to set those for the user's Global level as well.

    • @DouweM I also think about it but have few reasons to not do it here:

      • to keep merge request as small as possible I focused on decouple notification from membership. Global notifications can be done later.
      • performance impact: extra db query for each user during notification.

      So probably smart to do it in next iteration

  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 1 %li.notification-list-item
    2 %span.notification.fa.fa-holder.append-right-5
    3 - if setting.global?
    4 = notification_icon(current_user.notification_level)
    5 - else
    6 = notification_icon(setting.level)
    7
    8 %span.str-truncated
    9 = link_to group.name, group_path(group)
    10
    11 .pull-right
    12 = form_for [group, setting], remote: true, html: { class: 'update-notifications' } do |f|
    13 = f.select :level, NotificationSetting.levels.keys, {}, class: 'form-control trigger-submit'
  • @DouweM maybe its time to assign it to another developer for final touch?

  • @dzaporozhets You're totally right.

    @dbalexandre Can you take a look at my last batch of comments so we can get this MR into an 8.7 RC soon? You can leave those comments alone where I or DZ have pointed out it should be addressed in a next iteration. Thanks!

  • Reassigned to @dbalexandre

  • @DouweM Sure! I'll get right on it.

  • Added 488 commits:

  • Added 1 commit:

    • d11288be - Use query instead of model on migrations
  • Added 1 commit:

    • 127119f2 - Simplify query to retrieve NotificationSetting on controllers
  • Added 1 commit:

    • 069724ce - Use singular resource for NotificationSetting
  • Added 1 commit:

    • ee497599 - Use default_value_for to set default NotificationSetting#level
  • Added 1 commit:

    • b85b4234 - Add method to return the user notification setting for a group, or a project
  • Added 4 commits:

    • b85b4234...5ef170ae - 2 commits from branch master
    • 851b1c86 - Merge branch 'master' into decouple-member-notification
    • 6a88b3fa - Fix CHANGELOG
  • Added 1 commit:

    • abdecea8 - Fix schema.rb
  • Added 4 commits:

    • 635b65d1 - Add method to return the user notification setting for a group, or a project
    • e3d4ebdd - Update gitlab-shell to 2.7.2
    • 47c8b7f3 - Fix CHANGELOG
    • 99067a50 - Fix schema.rb
  • @DouweM Could you take a look?

  • Added 1 commit:

    • 73fdd4b8 - Use Hash instead of Array on NotificationSetting#level enum
  • Added 97 commits:

  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 1 class Groups::NotificationSettingsController < Groups::ApplicationController
    2 def update
  • Reassigned to @dbalexandre

  • Added 1 commit:

    • 787a5d43 - Reuse User#notification_settings_for when it's possible
  • Added 1 commit:

    • 56742e40 - Reuse User#notification_settings_for when it's possible
  • Added 2 commits:

    • 94300f7d - Requires user to be signed in when changing notification settings
    • 2cc30ce0 - Fix partial for update project notifications
  • Added 1 commit:

    • 1df052c6 - Simplify Projects::NotificationSettingsController
  • Added 3 commits:

    • a1318e2e - Requires user to be signed in when changing notification settings
    • bd85689a - Fix partial for update project notifications
    • 78d8fac3 - Simplify Projects::NotificationSettingsController
  • Added 1 commit:

    • eec52233 - Improve specs for group/project notification controller
  • @DouweM MR updated :) Please take a look one more time.

  • Added 5 commits:

    • 93a10f17 - Reuse User#notification_settings_for when it's possible
    • bee28e17 - Requires user to be signed in when changing notification settings
    • fe58c1f1 - Fix partial for update project notifications
    • 07eb1cbf - Simplify Projects::NotificationSettingsController
    • 245d509e - Improve specs for group/project notification controller
  • Added 2 commits:

    • ef22b76b - Simplify Projects::NotificationSettingsController
    • aabb466e - Improve specs for group/project notification controller
  • Added 1 commit:

    • 61a62e00 - Fix specs for Projects::NotificationSettingsController
  • Douwe Maan Enabled an automatic merge when the build for 61a62e00 succeeds

    Enabled an automatic merge when the build for 61a62e00 succeeds

  • Douwe Maan Status changed to merged

    Status changed to merged

  • Douwe Maan mentioned in commit 4516f40d

    mentioned in commit 4516f40d

  • 1 # This migration will create one row of NotificationSetting for each Member row
    2 # It can take long time on big instances.
    3 #
    4 # This migration can be done online but with following effects:
    5 # - during migration some users will receive notifications based on their global settings (project/group settings will be ignored)
    6 # - its possible to get duplicate records for notification settings since we don't create uniq index yet
    7 #
    8 class MigrateNewNotificationSetting < ActiveRecord::Migration
    9 def up
    10 timestamp = Time.now
    11 execute "INSERT INTO notification_settings ( user_id, source_id, source_type, level, created_at, updated_at ) SELECT user_id, source_id, source_type, notification_level, '#{timestamp}', '#{timestamp}' FROM members WHERE user_id IS NOT NULL"
  • Douwe Maan mentioned in merge request !3047 (closed)

    mentioned in merge request !3047 (closed)

  • mentioned in issue #9013 (closed)

  • Please register or sign in to reply
    Loading