PR fixes

app/mailers/user_mailer.rb

@@ -74,25 +74,11 @@ def work_package_updated(user, journal, author = User.current)
     end
   end
 
-  def work_package_watcher_added(work_package, user, watcher_setter)
+  def work_package_watcher_changed(work_package, user, watcher_changer, action)
     User.execute_as user do
       @issue = work_package
-      @watcher_setter = watcher_setter
-
-      set_work_package_headers(work_package)
-      message_id work_package, user
-      references work_package, user
-
-      with_locale_for(user) do
-        mail to: user.mail, subject: subject_for_work_package(work_package)
-      end
-    end
-  end
-
-  def work_package_watcher_removed(work_package, user, watcher_remover)
-    User.execute_as user do
-      @issue = work_package
-      @watcher_remover = watcher_remover
+      @watcher_changer = watcher_changer
+      @action = action
 
       set_work_package_headers(work_package)
       message_id work_package, user

app/views/user_mailer/work_package_watcher_removed.html.erb → app/views/user_mailer/work_package_watcher_changed.html.erb

@@ -26,7 +26,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 See docs/COPYRIGHT.rdoc for more details.
 
 ++#%>
-<%= t(:text_work_package_watcher_removed, id: "##{@issue.id}", watcher_remover: @watcher_remover) %>
+<%= t("text_work_package_watcher_#{@action}", id: "##{@issue.id}", watcher_changer: @watcher_changer) %>
 <hr />
 <%= render partial: 'issue_details', locals: { issue: @issue } %>
 <p>

app/views/user_mailer/work_package_watcher_added.text.erb → app/views/user_mailer/work_package_watcher_changed.text.erb

@@ -27,7 +27,7 @@ See docs/COPYRIGHT.rdoc for more details.
 
 ++#%>
 
-<%= t(:text_work_package_watcher_added, id: "##{@issue.id}", watcher_setter: @watcher_setter) %>
+<%= t("text_work_package_watcher_#{@action}", id: "##{@issue.id}", watcher_changer: @watcher_changer) %>
 
 ----------------------------------------
 <%= render partial: 'issue_details', locals: { issue: @issue } %>

app/workers/deliver_watcher_added_notification_job.rb

@@ -32,6 +32,6 @@ class DeliverWatcherAddedNotificationJob < DeliverWatcherNotificationJob
   def render_mail(recipient:, sender:)
     return unless watcher
 
-    UserMailer.work_package_watcher_added(watcher.watchable, recipient, sender)
+    UserMailer.work_package_watcher_changed(watcher.watchable, recipient, sender, 'added')
   end
 end

app/workers/deliver_watcher_removed_notification_job.rb

@@ -39,6 +39,6 @@ def perform(watcher_attributes, recipient_id, watcher_remover_id)
   def render_mail(recipient:, sender:)
     return unless watcher
 
-    UserMailer.work_package_watcher_removed(watcher.watchable, recipient, sender)
+    UserMailer.work_package_watcher_changed(watcher.watchable, recipient, sender, 'removed')
   end
 end

config/locales/en.yml

@@ -2315,8 +2315,8 @@ en:
   text_work_package_category_destroy_question: "Some work packages (%{count}) are assigned to this category. What do you want to do?"
   text_work_package_category_reassign_to: "Reassign work packages to this category"
   text_work_package_updated: "Work package %{id} has been updated by %{author}."
-  text_work_package_watcher_added: "You have been added as a watcher to Work package %{id} by %{watcher_setter}."
-  text_work_package_watcher_removed: "You have been removed from watchers of Work package %{id} by %{watcher_remover}."
+  text_work_package_watcher_added: "You have been added as a watcher to Work package %{id} by %{watcher_changer}."
+  text_work_package_watcher_removed: "You have been removed from watchers of Work package %{id} by %{watcher_changer}."
   text_work_packages_destroy_confirmation: "Are you sure you want to delete the selected work package(s)?"
   text_work_packages_ref_in_commit_messages: "Referencing and fixing work packages in commit messages"
   text_journal_added: "%{label} %{value} added"

lib/services/remove_watcher.rb

@@ -33,12 +33,13 @@ def initialize(work_package, user)
   end
 
   def run(success: -> {}, failure: -> {})
-    if @work_package.watcher_users.include?(@user)
-      @watcher = @work_package.watchers.find_by_user_id(@user.id)
+    watcher = @work_package.watchers.find_by_user_id(@user.id)
+
+    if watcher.present?
       @work_package.watcher_users.delete(@user)
       success.call
       OpenProject::Notifications.send('watcher_removed',
-                                      watcher: @watcher,
+                                      watcher: watcher,
                                       watcher_remover: User.current)
     else
       failure.call

spec/mailers/user_mailer_spec.rb

@@ -53,8 +53,10 @@
   end
 
   shared_examples_for 'mail is sent' do
+    let(:letters_sent_count) { 1 }
+
     it 'actually sends a mail' do
-      expect(ActionMailer::Base.deliveries.size).to eql(1)
+      expect(ActionMailer::Base.deliveries.size).to eql(letters_sent_count)
     end
 
     it 'is sent to the recipient' do
@@ -66,6 +68,12 @@
     end
   end
 
+  shared_examples_for 'multiple mails are sent' do |set_letters_sent_count|
+    it_behaves_like 'mail is sent' do
+      let(:letters_sent_count) { set_letters_sent_count }
+    end
+  end
+
   shared_examples_for 'mail is not sent' do
     it 'sends no mail' do
       expect(ActionMailer::Base.deliveries.size).to eql(0)
@@ -130,26 +138,14 @@
     it_behaves_like 'does only send mails to author if permitted'
   end
 
-  describe '#work_package_watcher_added' do
-    let(:watcher_setter) { user }
+  describe '#work_package_watcher_changed' do
+    let(:watcher_changer) { user }
     before do
-      UserMailer.work_package_watcher_added(work_package, recipient, watcher_setter).deliver_now
+      UserMailer.work_package_watcher_changed(work_package, recipient, watcher_changer, 'added').deliver_now
+      UserMailer.work_package_watcher_changed(work_package, recipient, watcher_changer, 'removed').deliver_now
     end
 
-    it_behaves_like 'mail is sent'
-
-    it 'contains the WP subject in the mail subject' do
-      expect(ActionMailer::Base.deliveries.first.subject).to include(work_package.subject)
-    end
-  end
-
-  describe '#work_package_watcher_removed' do
-    let(:watcher_remover) { user }
-    before do
-      UserMailer.work_package_watcher_removed(work_package, recipient, watcher_remover).deliver_now
-    end
-
-    it_behaves_like 'mail is sent'
+    include_examples 'multiple mails are sent', 2
 
     it 'contains the WP subject in the mail subject' do
       expect(ActionMailer::Base.deliveries.first.subject).to include(work_package.subject)

spec/workers/mail_notification_jobs/deliver_watcher_notification_job_spec.rb

@@ -29,8 +29,8 @@
 
 require 'spec_helper'
 
-shared_examples "DeliverWatcherNotificationJob" do |mailer_method_name|
-  let(:mailer_method_name) { mailer_method_name }
+shared_examples "DeliverWatcherNotificationJob" do |action|
+  let(:action) { action }
   let(:project) { FactoryBot.create(:project) }
   let(:role) { FactoryBot.create(:role, permissions: [:view_work_packages]) }
   let(:watcher_changer) { FactoryBot.create(:user) }
@@ -44,14 +44,15 @@
 
   before do
     # make sure no actual calls make it into the UserMailer
-    allow(UserMailer).to receive(mailer_method_name)
+    allow(UserMailer).to receive(:work_package_watcher_changed)
       .and_return(double('mail', deliver_now: nil))
   end
 
   it 'sends a mail' do
-    expect(UserMailer).to receive(mailer_method_name).with(work_package,
-                                                           watcher_user,
-                                                           watcher_changer)
+    expect(UserMailer).to receive(:work_package_watcher_changed).with(work_package,
+                                                                      watcher_user,
+                                                                      watcher_changer,
+                                                                      action)
     subject
   end
 
@@ -60,7 +61,7 @@
       before do
         mail = double('mail')
         allow(mail).to receive(:deliver_now).and_raise(SocketError)
-        expect(UserMailer).to receive(mailer_method_name).and_return(mail)
+        expect(UserMailer).to receive(:work_package_watcher_changed).and_return(mail)
       end
 
       it 'raises the error' do
@@ -71,13 +72,13 @@
 end
 
 describe DeliverWatcherRemovedNotificationJob, type: :model do
-  include_examples "DeliverWatcherNotificationJob", :work_package_watcher_removed do
+  include_examples "DeliverWatcherNotificationJob", 'removed' do
     let(:watcher_parameter) { watcher.attributes }
   end
 end
 
 describe DeliverWatcherAddedNotificationJob, type: :model do
-  include_examples "DeliverWatcherNotificationJob", :work_package_watcher_added do
+  include_examples "DeliverWatcherNotificationJob", 'added' do
     let(:watcher_parameter) { watcher }
   end
 end