Add note_design_email method for Notes mailer

Includes suggestion from @.luke
Co-authored-by: Alex Kalderimis <akalderimis@gitlab.com>
Co-authored-by: Luke Duncalfe <lduncalfe@gitlab.com>

app/mailers/emails/notes.rb

@@ -45,7 +45,11 @@ module Emails
     private
 
     def note_target_url_options
-      [@project || @group, @note.noteable, anchor: "note_#{@note.id}"]
+      [@project || @group, @note.noteable, note_target_url_query_params]
+    end
+
+    def note_target_url_query_params
+      { anchor: "note_#{@note.id}" }
     end
 
     def note_thread_options(recipient_id, reason)

ee/app/mailers/ee/emails/notes.rb

@@ -10,6 +10,18 @@ module EE
         @target_url = group_epic_url(*note_target_url_options)
         mail_answer_note_thread(@epic, @note, note_thread_options(recipient_id, reason))
       end
+
+      def note_design_email(recipient_id, note_id, reason = nil)
+        setup_note_mail(note_id, recipient_id)
+
+        design = @note.noteable
+        @target_url = ::Gitlab::Routing.url_helpers.designs_project_issue_url(
+          @note.parent,
+          design.issue,
+          note_target_url_query_params.merge(vueroute: design.filename)
+        )
+        mail_answer_note_thread(design, @note, note_thread_options(recipient_id, reason))
+      end
     end
   end
 end

ee/app/models/design_management/design.rb

@@ -5,6 +5,7 @@ module DesignManagement
     include Noteable
     include Gitlab::FileTypeDetection
     include Gitlab::Utils::StrongMemoize
+    include Referable
 
     belongs_to :project, inverse_of: :designs
     belongs_to :issue
@@ -67,8 +68,17 @@ module DesignManagement
       strong_memoize(:most_recent_design_version) { design_versions.ordered.last }
     end
 
+    # A reference for a design is the issue reference, indexed by the filename
+    # with an optional infix when full.
+    #
+    # e.g.
+    #   #123[homescreen.png]
+    #   other-project#72[sidebar.jpg]
+    #   #38/designs[transition.gif]
     def to_reference(from = nil, full: false)
-      filename
+      infix = full ? '/designs' : ''
+
+      "%s%s[%s]" % [issue.to_reference(from, full: full), infix, filename]
     end
 
     def to_ability_name

ee/app/models/ee/note.rb

@@ -52,7 +52,7 @@ module EE
     end
 
     def for_design?
-      noteable.is_a?(DesignManagement::Design)
+      noteable_type == DesignManagement::Design.name
     end
 
     override :parent

ee/app/views/notify/note_design_email.html.haml

+= render 'note_email'

ee/app/views/notify/note_design_email.text.erb

+<%= render 'note_email' %>

ee/changelogs/unreleased/ajk-design-todos-send-notifications.yml

+---
+title: Ensure design notifications are sent
+merge_request: 15250
+author:
+type: changed

ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb

@@ -87,12 +87,16 @@ module Gitlab
           approvals_before_merge || target_project.approvals_before_merge
         end
 
+        def distinct(column)
+          Arel.sql("distinct #{column}")
+        end
+
         def approver_ids
-          @approver_ids ||= Approver.where(target_type: 'MergeRequest', target_id: id).joins(:user).pluck('distinct user_id')
+          @approver_ids ||= Approver.where(target_type: 'MergeRequest', target_id: id).joins(:user).pluck(distinct(:user_id))
         end
 
         def approver_group_ids
-          @approver_group_ids ||= ApproverGroup.where(target_type: 'MergeRequest', target_id: id).joins(:group).pluck('distinct group_id')
+          @approver_group_ids ||= ApproverGroup.where(target_type: 'MergeRequest', target_id: id).joins(:group).pluck(distinct(:group_id))
         end
 
         def sync_code_owners_with_approvers

ee/spec/features/dashboards/todos_spec.rb

@@ -34,8 +34,8 @@ describe 'Dashboard todos' do
       expect(page).to have_selector('.todos-list .todo', count: 1)
     end
 
-    it 'has a link that will take me to the design page', :js do
-      click_link "design #{target.filename}"
+    it 'has a link that will take me to the design page' do
+      click_link "design #{target.to_reference}"
 
       expectation = Gitlab::Routing.url_helpers.designs_project_issue_path(
         target.project, target.issue, target.filename

ee/spec/helpers/ee/todos_helper_spec.rb

@@ -29,7 +29,7 @@ describe ::TodosHelper do
       it 'produces a good link' do
         path = helper.todo_target_path(todo)
         link = helper.todo_target_link(todo)
-        expected = "<a href=\"#{path}\">design #{design.filename}</a>"
+        expected = "<a href=\"#{path}\">design #{design.to_reference}</a>"
 
         expect(link).to eq(expected)
       end

ee/spec/mailers/notify_spec.rb

@@ -36,6 +36,30 @@ describe Notify do
                            description: 'Awesome description')
   end
 
+  describe '.note_design_email' do
+    set(:design) { create(:design, :with_file) }
+    set(:recipient) { create(:user) }
+    set(:note) do
+      create(:diff_note_on_design,
+         noteable: design,
+         project: design.project,
+         note: "Hello #{recipient.to_reference}")
+    end
+
+    let(:header_name) { 'X-Gitlab-DesignManagement-Design-ID' }
+    let(:refer_to_design) do
+      have_attributes(subject: a_string_including(design.filename))
+    end
+
+    subject { described_class.note_design_email(recipient.id, note.id) }
+
+    it { is_expected.to have_header(header_name, design.id.to_s) }
+
+    it { is_expected.to have_body_text(design.filename) }
+
+    it { is_expected.to refer_to_design }
+  end
+
   context 'for a project' do
     context 'for service desk issues' do
       before do
@@ -90,7 +114,7 @@ describe Notify do
         end
 
         it "contains the approvers list" do
-          is_expected.to have_body_text /#{merge_request.approvers.first.user.name}/
+          is_expected.to have_body_text %r[#{merge_request.approvers.first.user.name}]
         end
       end
 
@@ -117,26 +141,32 @@ describe Notify do
         end
 
         it 'has the correct subject' do
-          is_expected.to have_subject /#{merge_request.title} \(#{merge_request.to_reference}\)/
+          is_expected.to have_attributes(
+            subject: a_string_including("#{merge_request.title} (#{merge_request.to_reference})")
+          )
         end
 
         it 'contains the new status' do
-          is_expected.to have_body_text /approved/i
+          is_expected.to have_body_text('approved')
         end
 
         it 'contains a link to the merge request' do
-          is_expected.to have_body_text /#{project_merge_request_path project, merge_request}/
+          is_expected.to have_body_text("#{project_merge_request_path project, merge_request}")
         end
 
         it 'contains the names of all of the approvers' do
-          merge_request.approvals.each do |approval|
-            is_expected.to have_body_text /#{approval.user.name}/
+          names = merge_request.approvals.map { |a| a.user.name }
+
+          aggregate_failures do
+            names.each { |name| is_expected.to have_body_text(name) }
           end
         end
 
         it 'contains the names of all assignees' do
-          merge_request.assignees.each do |assignee|
-            is_expected.to have_body_text /#{assignee.name}/
+          names = merge_request.assignees.map(&:name)
+
+          aggregate_failures do
+            names.each { |name| is_expected.to have_body_text(name) }
           end
         end
 
@@ -173,26 +203,32 @@ describe Notify do
         end
 
         it 'has the correct subject' do
-          is_expected.to have_subject /#{merge_request.title} \(#{merge_request.to_reference}\)/
+          is_expected.to have_attributes(
+            subject: a_string_including("#{merge_request.title} (#{merge_request.to_reference})")
+          )
         end
 
         it 'contains the new status' do
-          is_expected.to have_body_text /unapproved/i
+          is_expected.to have_body_text('unapproved')
         end
 
         it 'contains a link to the merge request' do
-          is_expected.to have_body_text /#{project_merge_request_path project, merge_request}/
+          is_expected.to have_body_text("#{project_merge_request_path project, merge_request}")
         end
 
         it 'contains the names of all of the approvers' do
-          merge_request.approvals.each do |approval|
-            is_expected.to have_body_text /#{approval.user.name}/
+          names = merge_request.approvals.map { |a| a.user.name }
+
+          aggregate_failures do
+            names.each { |name| is_expected.to have_body_text(name) }
           end
         end
 
         it 'contains the names of all assignees' do
-          merge_request.assignees.each do |assignee|
-            is_expected.to have_body_text /#{assignee.name}/
+          names = merge_request.assignees.map(&:name)
+
+          aggregate_failures do
+            names.each { |name| is_expected.to have_body_text(name) }
           end
         end
       end
@@ -208,7 +244,7 @@ describe Notify do
         end
 
         it 'contains the new status' do
-          is_expected.to have_body_text /unapproved/i
+          is_expected.to have_body_text('unapproved')
         end
       end
     end

ee/spec/services/ee/notes/create_service_spec.rb

@@ -39,6 +39,17 @@ describe Notes::CreateService do
         expect(note.noteable).to eq(design)
       end
 
+      it 'sends a notification about this note' do
+        notifier = double
+        allow(::NotificationService).to receive(:new).and_return(notifier)
+
+        expect(notifier)
+          .to receive(:new_note)
+          .with have_attributes(noteable: design)
+
+        service.execute
+      end
+
       it 'correctly builds the position of the note' do
         note = service.execute
 

ee/spec/services/ee/notification_service_spec.rb

@@ -3,13 +3,65 @@ require 'spec_helper'
 describe EE::NotificationService, :mailer do
   include EmailSpec::Matchers
   include NotificationHelpers
+  include DesignManagementTestHelpers
 
   let(:subject) { NotificationService.new }
 
+  let(:mailer) { double(deliver_later: true) }
+
+  context 'when notified of a new design diff note' do
+    set(:design) { create(:design, :with_file) }
+    set(:project) { design.project }
+    set(:dev) { create(:user) }
+    set(:stranger) { create(:user) }
+    set(:note) do
+      create(:diff_note_on_design,
+         noteable: design,
+         project: project,
+         note: "Hello #{dev.to_reference}, G'day #{stranger.to_reference}")
+    end
+
+    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)
+
+        subject.new_note(note)
+      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
+
+      it 'does not notify non-developers' do
+        expect(Notify)
+          .not_to receive(:note_design_email).with(stranger.id, note.id)
+
+        subject.new_note(note)
+      end
+    end
+
+    context 'design management is disabled' do
+      it 'does not notify the user' do
+        expect(Notify).not_to receive(:note_design_email)
+
+        subject.new_note(note)
+      end
+    end
+  end
+
   context 'service desk issues' do
     before do
       allow(Notify).to receive(:service_desk_new_note_email)
-                         .with(kind_of(Integer), kind_of(Integer)).and_return(double(deliver_later: true))
+                         .with(Integer, Integer).and_return(mailer)
 
       allow(::EE::Gitlab::ServiceDesk).to receive(:enabled?).and_return(true)
       allow(::Gitlab::IncomingEmail).to receive(:enabled?) { true }
@@ -17,7 +69,8 @@ describe EE::NotificationService, :mailer do
     end
 
     def should_email!
-      expect(Notify).to receive(:service_desk_new_note_email).with(issue.id, kind_of(Integer))
+      expect(Notify).to receive(:service_desk_new_note_email)
+        .with(issue.id, note.id)
     end
 
     def should_not_email!