Always email owners of any changes to their roles

rubygems · Sep 13, 2024 · 30fe85f · 30fe85f
1 parent a149d0d
commit 30fe85f
Show file tree

Hide file tree

Showing 8 changed files with 19 additions and 8 deletions.
diff --git a/app/controllers/api/v1/owners_controller.rb b/app/controllers/api/v1/owners_controller.rb
@@ -35,7 +35,6 @@ def update
     ownership = @rubygem.ownerships.find_by!(user: owner)
 
     if ownership.update(ownership_update_params)
-      OwnersMailer.with(ownership: ownership, authorizer: @api_key.user).owner_updated.deliver_later
       render plain: response_with_mfa_warning("Owner updated successfully.")
     else
       render plain: response_with_mfa_warning(ownership.errors.full_messages.to_sentence), status: :unprocessable_entity

diff --git a/app/mailers/owners_mailer.rb b/app/mailers/owners_mailer.rb
@@ -15,13 +15,12 @@ def ownership_confirmation(ownership)
 
   def owner_updated
     @ownership = params[:ownership]
-    @authorizer = params[:authorizer]
     @user = @ownership.user
     @rubygem = @ownership.rubygem
 
     mail(
       to: @user.email,
-      subject: t("mailer.owner_updated.subject", gem: @rubygem.name, host: Gemcutter::HOST_DISPLAY, owner_handle: @authorizer.display_handle)
+      subject: t("mailer.owner_updated.subject", gem: @rubygem.name, host: Gemcutter::HOST_DISPLAY)
     )
   end
 

diff --git a/app/models/ownership.rb b/app/models/ownership.rb
@@ -14,6 +14,7 @@ class Ownership < ApplicationRecord
   after_create :record_create_event
   after_update :record_confirmation_event, if: :saved_change_to_confirmed_at?
   after_update :record_role_updated_event, if: :saved_change_to_role?
+  after_update :notify_user_role_of_role_change, if: :saved_change_to_role?
   after_destroy :record_destroy_event
 
   scope :confirmed, -> { where.not(confirmed_at: nil) }
@@ -127,4 +128,8 @@ def record_destroy_event
       owner_gid: user.to_gid,
       actor_gid: Current.user&.to_gid)
   end
+
+  def notify_user_role_of_role_change
+    OwnersMailer.with(ownership: self).owner_updated.deliver_later
+  end
 end
diff --git a/app/views/owners_mailer/owner_updated.html.erb b/app/views/owners_mailer/owner_updated.html.erb
@@ -9,7 +9,7 @@
       <div class="h3-1-center" style="color:#1e1e1e; font-family:Georgia, serif; min-width:auto !important; font-size:20px; line-height:26px;">
         <br>
         <p>
-          <%= t("mailer.owner_updated.body_html", gem: @rubygem.name, authorizer: @authorizer.display_handle, host: Gemcutter::HOST_DISPLAY, role: Ownership.human_attribute_name("role.#{@ownership.role}")) %>
+          <%= t("mailer.owner_updated.body_html", gem: @rubygem.name, host: Gemcutter::HOST_DISPLAY, role: Ownership.human_attribute_name("role.#{@ownership.role}")) %>
         </p>
         <table width="100%" border="0" cellspacing="0" cellpadding="0" class="spacer" style="font-size:0pt; line-height:0pt; text-align:center; width:100%; min-width:100%"><tr><td height="35" class="spacer" style="font-size:0pt; line-height:0pt; text-align:center; width:100%; min-width:100%">&nbsp;</td></tr></table>
 

diff --git a/app/views/owners_mailer/owner_updated.text.erb b/app/views/owners_mailer/owner_updated.text.erb
@@ -1,5 +1,5 @@
 <%= t("mailer.owner_updated.subtitle", user_handle: @user.handle) %>
 
-<%= t("mailer.owner_updated.body_text", gem: @rubygem.name, authorizer: @authorizer.display_handle, host: Gemcutter::HOST_DISPLAY, role: Ownership.human_attribute_name("role.#{@ownership.role}")) %>
+<%= t("mailer.owner_updated.body_text", gem: @rubygem.name, host: Gemcutter::HOST_DISPLAY, role: Ownership.human_attribute_name("role.#{@ownership.role}")) %>
 
 <%= rubygem_owners_url(@rubygem.slug) %>
diff --git a/config/locales/en.yml b/config/locales/en.yml
@@ -343,8 +343,8 @@ en:
       subject: Your role was updated for %{gem} gem
       title: OWNER ROLE UPDATED
       subtitle: Hi %{user_handle}!
-      body_html: Your role was updated to %{role} for <strong><a href="https://rubygems.org/gems/%{gem}">%{gem}</a></strong> gem by <b>%{authorizer}</b>.
-      body_text: Your role was updated to %{role} for %{gem} gem by %{authorizer}.
+      body_html: Your role was updated to %{role} for <strong><a href="https://rubygems.org/gems/%{gem}">%{gem}</a></strong> gem.
+      body_text: Your role was updated to %{role} for %{gem} gem.
     ownerhip_request_closed:
       title: OWNERSHIP REQUEST
       subtitle: Hi %{handle}!

diff --git a/test/mailers/owners_test.rb b/test/mailers/owners_test.rb
@@ -11,7 +11,7 @@ class OwnersMailerTest < ActionMailer::TestCase
 
   context "#owner_updated" do
     should "include host in subject" do
-      email = OwnersMailer.with(ownership: @maintainer_ownership, authorizer: @owner).owner_updated
+      email = OwnersMailer.with(ownership: @maintainer_ownership).owner_updated
 
       assert_emails(1) { email.deliver_now }
       assert_equal email.subject, "Your role was updated for #{@rubygem.name} gem"

diff --git a/test/models/ownership_test.rb b/test/models/ownership_test.rb
@@ -1,6 +1,8 @@
 require "test_helper"
 
 class OwnershipTest < ActiveSupport::TestCase
+  include ActionMailer::TestHelper
+
   should "be valid with factory" do
     assert_predicate build(:ownership), :valid?
   end
@@ -147,6 +149,12 @@ class OwnershipTest < ActiveSupport::TestCase
         assert_equal "rubygem:owner:updated", event.tag
         assert_equal attributes, event.additional.attributes
       end
+
+      should "enqueue the owner updated email" do
+        assert_enqueued_emails 1 do
+          @ownership.update!(role: :maintainer)
+        end
+      end
     end
   end