Skip to content
Snippets Groups Projects

Allow users to toggle notifications for access level changes at the group/project levels

Closed Allow users to toggle notifications for access level changes at the group/project levels
dandunckelman/gitlab-ce:add_access_level_change_events into master
4 unresolved threads
Closed username-removed-393947 requested to merge dandunckelman/gitlab-ce:add_access_level_change_events into master
4 unresolved threads

What does this MR do?

Fixes #20147 (closed) by allowing users to toggle notifications for when their user's permission level changes in a group and or project.

Are there points in the code the reviewer needs to double check?

  • How I'm getting the notification setting for the user. I'm currently using user.global_notification_setting, but I think that doesn't cover the case where the global setting differs from what has been explicitly set at the project or group level.
  • I initially used user.notification_settings_for(self.group) to get the setting specific to that group
    • But when that user had the group setting pointing to the global setting, and the global setting is custom, the list of custom events were all false (despite the global setting having some set to true)
      • Maybe I'm doing something wrong here or the notification_settings_for method isn't doing what I think it should do (and maybe I'm wrong in that thought)

Why was this MR needed?

See #20147 (closed)

What are the relevant issue numbers?

Screenshots (if relevant)

  • New toggle options:

Does this MR meet the acceptance criteria?

  • CHANGELOG entry added
    • I'll add this after the version/milestone is selected
  • Documentation created/updated
  • API support added
    • This issue only covers the UI support. API support is being tracked here: #19359 (closed)
  • Tests
    • Added for this feature/bug
    • All builds are passing
      • ATM, there's a build running after I squashed commits. Before then, it had passed, so it should do the same. We'll see.
  • Conform by the style guides
  • Branch has no merge conflicts with master (if you do - rebase it please)
  • Squashed related commits together

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
app/models/notification_setting.rb
66 68 events[event] = ActiveRecord::ConnectionAdapters::Column::TRUE_VALUES.include?(events[event])
67 69 end
68 70 end
71
72 def event_disabled?(event)
73 # this declares the notification as enabled on all other levels. Do we need more granular control?
74 return false if level != "custom"
  • username-removed-128633
    username-removed-128633 @rymai started a thread on an old version of the diff
    • app/models/members/group_member.rb
    40 40 end
    41 41
    42 42 def post_update_hook
    43 if access_level_changed?
    43 if access_level_changed? and !user.global_notification_setting.event_disabled?(:change_group_access_level)
  • username-removed-128633
    username-removed-128633 @rymai started a thread on an old version of the diff
    • app/models/members/project_member.rb
    120 120 end
    121 121
    122 122 def post_update_hook
    123 if access_level_changed?
    123 if access_level_changed? and !user.global_notification_setting.event_disabled?(:change_project_access_level)
  • username-removed-128633
    username-removed-128633 @rymai started a thread on an old version of the diff
    • app/models/notification_setting.rb
    66 68 events[event] = ActiveRecord::ConnectionAdapters::Column::TRUE_VALUES.include?(events[event])
    67 69 end
    68 70 end
    71
    72 def event_disabled?(event)
    73 # this declares the notification as enabled on all other levels. Do we need more granular control?
    74 return false if level != "custom"
    75 self.send(event) == false
  • username-removed-393947 Added 56 commits:

    Added 56 commits:

    • 7db44901...a0788fc3 - 55 commits from branch gitlab-org:master
    • b7ded999 - Allow users to toggle notifications for access level changes at the group/project levels
  • username-removed-393947
    username-removed-393947 @dandunckelman ·
    Author Contributor

    Thanks for the notes @rymai. Just updated. Build is running now.

  • username-removed-393947
    username-removed-393947 @dandunckelman ·
    Author Contributor

    @rymai build passed. Please let me know if there's more I can do here.

    Also, could you or someone else review my questions in the "Are there points in the code the reviewer needs to double check?" section above?

    Thanks

  • username-removed-128633
    username-removed-128633 @rymai ·
    Maintainer

    Hey @dandunckelman, thanks!

    I've reviewed the code and thought a bit about it. I see a few changes needed to make this feature work as expected:

    • Currently, NotificationService#update_group_member sends an "access granted" email to the updated member

      • This email is wrong, it should send an "access level updated" email;

      • This email should be sent to several users, using the #add_custom_notifications method, e.g.:

        def update_group_member(group_member)
recipients = add_custom_notifications([group_member.user_id], group_member.source, :change_group_member_access_level)

recipients.each do |recipient|
  mailer.member_access_level_changed_email(
    group_member.real_source_type,
    group_member.id,
    recipient.id
  ).deliver_later
end
end

      • NotificationService#add_custom_notifications will probably have to be modified to handle group in addition to project, e.g.:

      Get project/group users with CUSTOM notification level

    • def add_custom_notifications(recipients, project, action)

    • def add_custom_notifications(recipients, source, action) user_ids = []

      Users with a notification setting on group or project

    • user_ids += notification_settings_for(project, :custom, action)
    • user_ids += notification_settings_for(project.group, :custom, action)

    • user_ids += notification_settings_for(source, :custom, action)

    • user_ids += notification_settings_for(source.group, :custom, action) if source.try(:group)

      Users with global level custom

    • users_with_project_level_global = notification_settings_for(project, :global)
    • users_with_group_level_global = notification_settings_for(project.group, :global)

    • users_with_project_level_global = notification_settings_for(source, :global)

    • users_with_group_level_global = notification_settings_for(source.group, :global) if source.try(:group)

      global_users_ids = users_with_project_level_global.concat(users_with_group_level_global)

    • Same thing for NotificationService#update_project_member

    What do you think?

  • username-removed-393947
    username-removed-393947 @dandunckelman ·
    Author Contributor

    Aw @rymai, you're right. I had noticed that the email sent was "access granted" and not "access level updated".

    Do you think these changes fit within this MR or should a new one be created?

  • username-removed-393947
    username-removed-393947 @dandunckelman ·
    Author Contributor

    @rymai should we notify all group members of one member's access level being changed? Is that just for visibility?

  • username-removed-128633
    username-removed-128633 @rymai ·
    Maintainer

    @dandunckelman I will hold off the review until we decide something in #20147 (closed). Thanks! :)

  • username-removed-128633
    username-removed-128633 @rymai ·
    Maintainer

    @dandunckelman I think we should probably close this MR since we'll probably go the solution proposed in #20789 (closed), what do you think?

  • username-removed-128633 Assignee removed

    Assignee removed

  • username-removed-393947
    username-removed-393947 @dandunckelman ·
    Author Contributor

    @rymai agreed!

  • username-removed-393947 Status changed to closed

    Status changed to closed

    • Please register or sign in to reply