Merge branch '217095-design-notifications' into 'master'

Bugfix recipients of design comment notifications

See merge request gitlab-org/gitlab!46642

app/models/design_management/design.rb

@@ -11,12 +11,14 @@ module DesignManagement
     include WhereComposite
     include RelativePositioning
     include Todoable
+    include Participable
 
     belongs_to :project, inverse_of: :designs
     belongs_to :issue
 
     has_many :actions
     has_many :versions, through: :actions, class_name: 'DesignManagement::Version', inverse_of: :designs
+    has_many :authors, -> { distinct }, through: :versions, class_name: 'User'
     # This is a polymorphic association, so we can't count on FK's to delete the
     # data
     has_many :notes, as: :noteable, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
@@ -31,6 +33,9 @@ module DesignManagement
 
     alias_attribute :title, :filename
 
+    participant :authors
+    participant :notes_with_associations
+
     # Pre-fetching scope to include the data necessary to construct a
     # reference using `to_reference`.
     scope :for_reference, -> { includes(issue: [{ project: [:route, :namespace] }]) }
@@ -136,6 +141,12 @@ module DesignManagement
       strong_memoize(:most_recent_action) { actions.ordered.last }
     end
 
+    def participants(current_user = nil)
+      return [] unless Feature.enabled?(:design_management_design_notification_participants, project)
+
+      super
+    end
+
     # A reference for a design is the issue reference, indexed by the filename
     # with an optional infix when full.
     #

changelogs/unreleased/217095-design-notifications.yml

+---
+title: Bugfix email notification recipients for comments on Designs
+merge_request: 46642
+author:
+type: fixed

config/feature_flags/development/design_management_design_notification_participants.yml

+---
+name: design_management_design_notification_participants
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46642
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/277319
+milestone: '13.6'
+type: development
+group: group::knowledge
+default_enabled: false

doc/user/profile/notifications.md

@@ -7,13 +7,15 @@ info: To determine the technical writer assigned to the Stage/Group associated w
 
 # GitLab Notification Emails
 
-GitLab Notifications allow you to stay informed about what's happening in GitLab. With notifications enabled, you can receive updates about activity in issues, merge requests, and epics. Notifications are sent via email.
+GitLab Notifications allow you to stay informed about what's happening in GitLab. With notifications
+enabled, you can receive updates about activity in issues, merge requests, epics, and designs.
+Notifications are sent via email.
 
 ## Receiving notifications
 
 You will receive notifications for one of the following reasons:
 
-- You participate in an issue, merge request, or epic. In this context, _participate_ means comment, or edit.
+- You participate in an issue, merge request, epic or design. In this context, _participate_ means comment, or edit.
 - You enable notifications in an issue, merge request, or epic. To enable notifications, click the **Notifications** toggle in the sidebar to _on_.
 
 While notifications are enabled, you will receive notification of actions occurring in that issue, merge request, or epic.
@@ -209,6 +211,44 @@ If an open merge request becomes unmergeable due to conflict, its author will be
 If a user has also set the merge request to automatically merge once pipeline succeeds,
 then that user will also be notified.
 
+## Design email notifications
+
+> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/217095) in GitLab 13.6.
+> - It's [deployed behind a feature flag](../../user/feature_flags.md), disabled by default.
+> - It's disabled on GitLab.com.
+> - It's not recommended for production use.
+> - To use it in GitLab self-managed instances, ask a GitLab administrator to [enable it](#enable-or-disable-design-email-notifications). **(CORE ONLY)**
+
+CAUTION: **Warning:**
+This feature might not be available to you. Check the **version history** note above for details.
+
+Email notifications are sent to the participants when comments are made on a design.
+
+The participants are:
+
+- Authors of the design (can be multiple people if different authors have uploaded different versions of the design).
+- Authors of comments on the design.
+- Anyone that is `@mentioned` in a comment on the design.
+
+### Enable or disable design email notifications **(CORE ONLY)**
+
+The design email notifications feature is under development and not ready for production use. It is
+deployed behind a feature flag that is **disabled by default**.
+[GitLab administrators with access to the GitLab Rails console](../../administration/feature_flags.md)
+can enable it.
+
+To enable it:
+
+```ruby
+Feature.enable(:design_management_design_notification_participants)
+```
+
+To disable it:
+
+```ruby
+Feature.disable(:design_management_design_notification_participants)
+```
+
 ## Filtering email
 
 Notification email messages include GitLab-specific headers. You can filter the notification emails based on the content of these headers to better manage your notifications. For example, you could filter all emails for a specific project where you are being assigned either a merge request or issue.

spec/lib/gitlab/import_export/all_models.yml

@@ -652,6 +652,7 @@ milestone_releases:
 evidences:
 - release
 design: &design
+- authors
 - issue
 - actions
 - versions

spec/models/design_management/design_spec.rb

@@ -23,8 +23,20 @@ RSpec.describe DesignManagement::Design do
     it { is_expected.to belong_to(:issue) }
     it { is_expected.to have_many(:actions) }
     it { is_expected.to have_many(:versions) }
+    it { is_expected.to have_many(:authors) }
     it { is_expected.to have_many(:notes).dependent(:delete_all) }
     it { is_expected.to have_many(:user_mentions) }
+
+    describe '#authors' do
+      it 'returns unique version authors', :aggregate_failures do
+        author = create(:user)
+        create_list(:design_version, 2, designs: [design1], author: author)
+        version_authors = design1.versions.map(&:author)
+
+        expect(version_authors).to contain_exactly(issue.author, author, author)
+        expect(design1.authors).to contain_exactly(issue.author, author)
+      end
+    end
   end
 
   describe 'validations' do
@@ -326,6 +338,46 @@ RSpec.describe DesignManagement::Design do
     end
   end
 
+  describe '#participants' do
+    let_it_be_with_refind(:design) { create(:design, issue: issue) }
+    let_it_be(:current_user) { create(:user) }
+    let_it_be(:version_author) { create(:user) }
+    let_it_be(:note_author) { create(:user) }
+    let_it_be(:mentioned_user) { create(:user) }
+    let_it_be(:design_version) { create(:design_version, :committed, designs: [design], author: version_author) }
+    let_it_be(:note) do
+      create(:diff_note_on_design,
+        noteable: design,
+        issue: issue,
+        project: issue.project,
+        author: note_author,
+        note: mentioned_user.to_reference
+      )
+    end
+
+    subject { design.participants(current_user) }
+
+    it { is_expected.to be_empty }
+
+    context 'when participants can read the project' do
+      before do
+        design.project.add_guest(version_author)
+        design.project.add_guest(note_author)
+        design.project.add_guest(mentioned_user)
+      end
+
+      it { is_expected.to contain_exactly(version_author, note_author, mentioned_user) }
+
+      context 'when the feature flag is disabled' do
+        before do
+          stub_feature_flags(design_management_design_notification_participants: false)
+        end
+
+        it { is_expected.to be_empty }
+      end
+    end
+  end
+
   describe "#new_design?" do
     let(:design) { design1 }
 

spec/services/notification_service_spec.rb

@@ -834,53 +834,63 @@ RSpec.describe NotificationService, :mailer do
       end
     end
 
-    context 'when notified of a new design diff note' do
+    context 'design diff note', :deliver_mails_inline do
       include DesignManagementTestHelpers
 
       let_it_be(:design) { create(:design, :with_file) }
       let_it_be(:project) { design.project }
-      let_it_be(:dev) { create(:user) }
-      let_it_be(:stranger) { create(:user) }
+      let_it_be(:member_and_mentioned) { create(:user, developer_projects: [project]) }
+      let_it_be(:member_and_author_of_second_note) { create(:user, developer_projects: [project]) }
+      let_it_be(:member_and_not_mentioned) { create(:user, developer_projects: [project]) }
+      let_it_be(:non_member_and_mentioned) { create(:user) }
       let_it_be(:note) do
         create(:diff_note_on_design,
            noteable: design,
-           note: "Hello #{dev.to_reference}, G'day #{stranger.to_reference}")
+           note: "Hello #{member_and_mentioned.to_reference}, G'day #{non_member_and_mentioned.to_reference}")
+      end
+      let_it_be(:note_2) do
+        create(:diff_note_on_design, noteable: design, author: member_and_author_of_second_note)
       end
-      let(:mailer) { double(deliver_later: true) }
 
       context 'design management is enabled' do
         before do
           enable_design_management
-          project.add_developer(dev)
-          allow(Notify).to receive(:note_design_email) { mailer }
         end
 
-        it 'sends new note notifications' do
-          expect(subject).to receive(:send_new_note_notifications).with(note)
+        it 'sends new note notifications', :aggregate_failures do
+          notification.new_note(note)
 
-          subject.new_note(note)
+          should_email(design.authors.first)
+          should_email(member_and_mentioned)
+          should_email(member_and_author_of_second_note)
+          should_not_email(member_and_not_mentioned)
+          should_not_email(non_member_and_mentioned)
+          should_not_email(note.author)
         end
 
-        it 'sends a mail to the developer' do
-          expect(Notify)
-            .to receive(:note_design_email).with(dev.id, note.id, 'mentioned')
-
-          subject.new_note(note)
-        end
+        context 'when the feature flag is disabled' do
+          before do
+            stub_feature_flags(design_management_design_notification_participants: false)
+          end
 
-        it 'does not notify non-developers' do
-          expect(Notify)
-            .not_to receive(:note_design_email).with(stranger.id, note.id)
+          it 'sends a new note notification only to the mentioned member', :aggregate_failures do
+            notification.new_note(note)
 
-          subject.new_note(note)
+            should_email(member_and_mentioned)
+            should_not_email(design.authors.first)
+            should_not_email(member_and_author_of_second_note)
+            should_not_email(member_and_not_mentioned)
+            should_not_email(non_member_and_mentioned)
+            should_not_email(note.author)
+          end
         end
       end
 
       context 'design management is disabled' do
-        it 'does not notify the user' do
-          expect(Notify).not_to receive(:note_design_email)
+        it 'does not notify anyone' do
+          notification.new_note(note)
 
-          subject.new_note(note)
+          should_not_email_anyone
         end
       end
     end