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.
-
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
Activity
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? @rspeicher how
UserCreateService
is related to creating group/project membership object. Btw I can put intoMember.add_user
as alternative to callback@rspeicher creating
MemberCreateService
will force me to make many changes unrelated to this merge request (which is already growing). This is something we want to avoid in review process. Since now we useMember.add_user
method for creating member I can use it for creating notification setting object. Or leave as is. In any case I will create separate issue to refactor member creation process into service as next step@rspeicher issue for refactoring - #14709 (moved)
- spec/models/notification_setting_spec.rb 0 → 100644
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 (seeNotification.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
mentioned in issue #14709 (moved)
About migration path:
- 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
- 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.
- 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:
- 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.
- 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
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 thenglobal
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.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:
Default value is
Global
.NotificationSetting
object makes sense only if value is different fromGlobal
. Otherwise there is no need for object to exist. We can create object at different stage. It can be one of this:- Long before user want (or does not) to open project page. (complicated and wasteful)
- When user open project page. (more efficient but does not appear on
/profile/notification
unless you visit project first) - 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.
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 and20160328115649_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
@jschatz1 please review frontend part.
@rspeicher please take a look one more time
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
Toggle commit list- 52aaa087...a9346cab - 96 commits from branch
Added 3 commits:
-
1a293a43...7b2af874 - 2 commits from branch
master
- 877f56c9 - Merge branch 'master' into decouple-member-notification
-
1a293a43...7b2af874 - 2 commits from branch
mentioned in merge request !3452 (merged)
Reassigned to @DouweM
- Edited by Jacob Schatz
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) @jschatz1 thanks
@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.
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 alabel
. The whole thing should be afieldset
. Because thelabel
tag defines a label for aninput
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
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.
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] @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
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 100% agree. However its not a regression so we can leave it for next iteration.
@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:
-
877f56c9...0d216c19 - 487 commits from branch
master
- 7afeace3 - Merge branch 'master' into decouple-member-notification
-
877f56c9...0d216c19 - 487 commits from branch
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 4 commits:
- b85b4234...5ef170ae - 2 commits from branch
master
- 851b1c86 - Merge branch 'master' into decouple-member-notification
- 6a88b3fa - Fix CHANGELOG
- b85b4234...5ef170ae - 2 commits from branch
Reassigned to @DouweM
@DouweM Could you take a look?
Added 1 commit:
- 73fdd4b8 - Use Hash instead of Array on NotificationSetting#level enum
Added 97 commits:
-
73fdd4b8...15cbbd09 - 96 commits from branch
master
- 9a44d697 - Merge branch 'master' into decouple-member-notification
-
73fdd4b8...15cbbd09 - 96 commits from branch
1 class Groups::NotificationSettingsController < Groups::ApplicationController 2 def update Can we verify that we are signed in here?
Edited by Douwe Maan
Reassigned to @dbalexandre
Reassigned to @DouweM
@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
Toggle commit list-
93a10f17 - Reuse
Added 1 commit:
- 61a62e00 - Fix specs for Projects::NotificationSettingsController
Enabled an automatic merge when the build for 61a62e00 succeeds
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" I believe this is causing the following issue :
ActiveRecord::StatementInvalid: Mysql2::Error: Incorrect datetime value: '2016-04-12 21:43:41 +0200' for column 'created_at' at row 1: INSERT INTO notification_settings ( user_id, source_id, source_type, level, created_at, updated_at ) SELECT user_id, source_id, source_type, notification_level, '2016-04-12 21:43:41 +0200', '2016-04-12 21:43:41 +0200' FROM members WHERE user_id IS NOT NULL
@dbalexandre Can you check this out?
@DouweM Sure
mentioned in issue #15264 (closed)
mentioned in merge request !3047 (closed)
mentioned in issue #9013 (closed)
Mentioned in commit dblessing/gitlab-ce@4516f40d