From 50f49c1c9e37b5798353ddb1b28cb4b5f8a4fc90 Mon Sep 17 00:00:00 2001 From: Anas Shahid Date: Tue, 21 Oct 2025 02:23:19 -0400 Subject: [PATCH 1/2] Extract member expiration email into dedicated mailer The NotificationService and Notify mailer have become bloated, making them difficult to modify and maintain. This change extracts the member expiration date update email into its own dedicated mailer class. --- app/mailers/emails/members.rb | 17 ---- .../members/expiration_date_updated_mailer.rb | 44 +++++++++ app/services/notification_service.rb | 2 +- .../email.html.haml | 6 ++ .../email.text.erb | 5 + ...er_expiration_date_updated_email.html.haml | 6 -- ...ber_expiration_date_updated_email.text.erb | 5 - .../expiration_date_updated_mailer_spec.rb | 98 +++++++++++++++++++ spec/mailers/notify_spec.rb | 82 ---------------- spec/services/notification_service_spec.rb | 7 +- 10 files changed, 157 insertions(+), 115 deletions(-) create mode 100644 app/mailers/members/expiration_date_updated_mailer.rb create mode 100644 app/views/members/expiration_date_updated_mailer/email.html.haml create mode 100644 app/views/members/expiration_date_updated_mailer/email.text.erb delete mode 100644 app/views/notify/member_expiration_date_updated_email.html.haml delete mode 100644 app/views/notify/member_expiration_date_updated_email.text.erb create mode 100644 spec/mailers/members/expiration_date_updated_mailer_spec.rb diff --git a/app/mailers/emails/members.rb b/app/mailers/emails/members.rb index 227b8f6695c195..4eb3589e9b47b2 100644 --- a/app/mailers/emails/members.rb +++ b/app/mailers/emails/members.rb @@ -22,23 +22,6 @@ def member_access_granted_email(member_source_type, member_id) subject: subject("Access to the #{member_source.human_name} #{member_source.model_name.singular} was granted")) end - def member_expiration_date_updated_email(member_source_type, member_id) - @member_source_type = member_source_type - @member_id = member_id - - return unless member_exists? - - subject = if member.expires? - _('Group membership expiration date changed') - else - _('Group membership expiration date removed') - end - - email_with_layout( - to: member.user.notification_email_for(notification_group), - subject: subject(subject)) - end - def member_about_to_expire_email(member_source_type, member_id) @member_source_type = member_source_type @member_id = member_id diff --git a/app/mailers/members/expiration_date_updated_mailer.rb b/app/mailers/members/expiration_date_updated_mailer.rb new file mode 100644 index 00000000000000..ae1cb6c41a21bf --- /dev/null +++ b/app/mailers/members/expiration_date_updated_mailer.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module Members + class ExpirationDateUpdatedMailer < ApplicationMailer + helper EmailsHelper + + helper_method :member_source, :member + + layout 'mailer' + + def email + return unless member.present? + + mail_with_locale( + to: member.user.notification_email_for(notification_group), + subject: EmailsHelper.subject_with_prefix_and_suffix([email_subject_text]) + ) + end + + private + + delegate :source, to: :member, prefix: true + + def member + params[:member] + end + + def notification_group + member_source_type.casecmp?('project') ? member_source.group : member_source + end + + def member_source_type + params[:member_source_type] + end + + def email_subject_text + if member.expires? + _('Group membership expiration date changed') + else + _('Group membership expiration date removed') + end + end + end +end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 8001d5f3a989d2..0a21e9ed1ef19f 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -533,7 +533,7 @@ def updated_member_expiration(member) return true unless member.source.is_a?(Group) return true unless member.notifiable?(:mention) - mailer.member_expiration_date_updated_email(member.real_source_type, member.id).deliver_later + Members::ExpirationDateUpdatedMailer.with(member: member, member_source_type: member.real_source_type).email.deliver_later # rubocop:disable CodeReuse/ActiveRecord -- false positive end def member_about_to_expire(member) diff --git a/app/views/members/expiration_date_updated_mailer/email.html.haml b/app/views/members/expiration_date_updated_mailer/email.html.haml new file mode 100644 index 00000000000000..ceb7a6747812f6 --- /dev/null +++ b/app/views/members/expiration_date_updated_mailer/email.html.haml @@ -0,0 +1,6 @@ += email_default_heading(say_hi(member.user)) + +%p + = group_membership_expiration_changed_text(member, member_source) +%p + = group_membership_expiration_changed_link(member, member_source, format: :html) diff --git a/app/views/members/expiration_date_updated_mailer/email.text.erb b/app/views/members/expiration_date_updated_mailer/email.text.erb new file mode 100644 index 00000000000000..adab23500c7c58 --- /dev/null +++ b/app/views/members/expiration_date_updated_mailer/email.text.erb @@ -0,0 +1,5 @@ +<%= say_hi(member.user) %> + +<%= group_membership_expiration_changed_text(member, member_source) %> + +<%= group_membership_expiration_changed_link(member, member_source) %> diff --git a/app/views/notify/member_expiration_date_updated_email.html.haml b/app/views/notify/member_expiration_date_updated_email.html.haml deleted file mode 100644 index 6c4db22eeaac4e..00000000000000 --- a/app/views/notify/member_expiration_date_updated_email.html.haml +++ /dev/null @@ -1,6 +0,0 @@ -= email_default_heading(say_hi(@member.user)) - -%p - = group_membership_expiration_changed_text(@member, @member_source) -%p - = group_membership_expiration_changed_link(@member, @member_source, format: :html) diff --git a/app/views/notify/member_expiration_date_updated_email.text.erb b/app/views/notify/member_expiration_date_updated_email.text.erb deleted file mode 100644 index 8b3a5a55e77e81..00000000000000 --- a/app/views/notify/member_expiration_date_updated_email.text.erb +++ /dev/null @@ -1,5 +0,0 @@ -<%= say_hi(@member.user) %> - -<%= group_membership_expiration_changed_text(@member, @member_source) %> - -<%= group_membership_expiration_changed_link(@member, @member_source) %> diff --git a/spec/mailers/members/expiration_date_updated_mailer_spec.rb b/spec/mailers/members/expiration_date_updated_mailer_spec.rb new file mode 100644 index 00000000000000..4efcd9f1e4406b --- /dev/null +++ b/spec/mailers/members/expiration_date_updated_mailer_spec.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Members::ExpirationDateUpdatedMailer, feature_category: :groups_and_projects do + include EmailSpec::Matchers + using RSpec::Parameterized::TableSyntax + + describe '#email' do + let(:recipient) { create(:user) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- shared examples require persisted records + let(:group) { create(:group) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- shared examples require persisted records + let(:group_member) { create(:group_member, source: group, user: recipient, expires_at: 1.day.from_now) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- needs database persistence for updates + let(:member_source_type) { 'Group' } + + subject(:email) { described_class.with(member: group_member, member_source_type: member_source_type).email } + + it_behaves_like 'an email sent from GitLab' do + let(:gitlab_sender_display_name) { Gitlab.config.gitlab.email_display_name } + let(:gitlab_sender) { Gitlab.config.gitlab.email_from } + let(:gitlab_sender_reply_to) { Gitlab.config.gitlab.email_reply_to } + end + + it_behaves_like 'an email sent to a user' + it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like 'a user cannot unsubscribe through footer link' + it_behaves_like 'appearance header and footer enabled' + it_behaves_like 'appearance header and footer not enabled' + + context 'when expiration date is changed' do + context 'when expiration date is one day away' do + it 'contains all the useful information' do + is_expected.to have_subject 'Group membership expiration date changed' + is_expected.to have_body_text group_member.user.name + is_expected.to have_body_text group.name + is_expected.to have_body_text group.web_url + is_expected.to have_body_text group_group_members_url(group, search: group_member.user.username) + is_expected.to have_body_text 'day.' + is_expected.not_to have_body_text 'days.' + end + end + + context 'when expiration date is more than one day away' do + before do + group_member.update!(expires_at: 20.days.from_now) + end + + it 'contains all the useful information' do + is_expected.to have_subject 'Group membership expiration date changed' + is_expected.to have_body_text group_member.user.name + is_expected.to have_body_text group.name + is_expected.to have_body_text group.web_url + is_expected.to have_body_text group_group_members_url(group, search: group_member.user.username) + is_expected.to have_body_text 'days.' + is_expected.not_to have_body_text 'day.' + end + end + + context 'when a group member is newly given an expiration date' do + let(:new_recipient) { create(:user) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- needs database persistence for updates + let(:new_group_member) { create(:group_member, source: group, user: new_recipient) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- needs database persistence for updates + + subject(:email) { described_class.with(member: new_group_member, member_source_type: member_source_type).email } + + before do + new_group_member.update!(expires_at: 5.days.from_now) + end + + it 'contains all the useful information' do + is_expected.to have_subject 'Group membership expiration date changed' + is_expected.to have_body_text new_group_member.user.name + is_expected.to have_body_text group.name + is_expected.to have_body_text group.web_url + is_expected.to have_body_text group_group_members_url(group, search: new_group_member.user.username) + is_expected.to have_body_text 'days.' + is_expected.not_to have_body_text 'day.' + end + end + end + + context 'when expiration date is removed' do + before do + group_member.update!(expires_at: nil) + end + + it 'contains all the useful information' do + is_expected.to have_subject 'Group membership expiration date removed' + is_expected.to have_body_text group_member.user.name + is_expected.to have_body_text group.name + end + end + + context 'when member is nil' do + let(:group_member) { nil } + + it_behaves_like 'no email is sent' + end + end +end diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 280d785a960489..26298be5bb0d3e 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -1670,88 +1670,6 @@ def invite_to_group(group, inviter:, user: nil) ) end - describe 'group expiration date updated' do - let_it_be(:group_member) { create(:group_member, group: group, expires_at: 1.day.from_now) } - - context 'when expiration date is changed' do - subject { described_class.member_expiration_date_updated_email('group', group_member.id) } - - it_behaves_like 'an email sent from GitLab' - it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like 'a user cannot unsubscribe through footer link' - it_behaves_like 'appearance header and footer enabled' - it_behaves_like 'appearance header and footer not enabled' - - context 'when expiration date is one day away' do - it 'contains all the useful information' do - is_expected.to have_subject 'Group membership expiration date changed' - is_expected.to have_body_text group_member.user.name - is_expected.to have_body_text group.name - is_expected.to have_body_text group.web_url - is_expected.to have_body_text group_group_members_url(group, search: group_member.user.username) - is_expected.to have_body_text 'day.' - is_expected.not_to have_body_text 'days.' - end - end - - context 'when expiration date is more than one day away' do - before do - group_member.update!(expires_at: 20.days.from_now) - end - - it 'contains all the useful information' do - is_expected.to have_subject 'Group membership expiration date changed' - is_expected.to have_body_text group_member.user.name - is_expected.to have_body_text group.name - is_expected.to have_body_text group.web_url - is_expected.to have_body_text group_group_members_url(group, search: group_member.user.username) - is_expected.to have_body_text 'days.' - is_expected.not_to have_body_text 'day.' - end - end - - context 'when a group member is newly given an expiration date' do - let_it_be(:group_member) { create(:group_member, group: group) } - - before do - group_member.update!(expires_at: 5.days.from_now) - end - - subject { described_class.member_expiration_date_updated_email('group', group_member.id) } - - it 'contains all the useful information' do - is_expected.to have_subject 'Group membership expiration date changed' - is_expected.to have_body_text group_member.user.name - is_expected.to have_body_text group.name - is_expected.to have_body_text group.web_url - is_expected.to have_body_text group_group_members_url(group, search: group_member.user.username) - is_expected.to have_body_text 'days.' - is_expected.not_to have_body_text 'day.' - end - end - end - - context 'when expiration date is removed' do - before do - group_member.update!(expires_at: nil) - end - - subject { described_class.member_expiration_date_updated_email('group', group_member.id) } - - it_behaves_like 'an email sent from GitLab' - it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like 'a user cannot unsubscribe through footer link' - it_behaves_like 'appearance header and footer enabled' - it_behaves_like 'appearance header and footer not enabled' - - it 'contains all the useful information' do - is_expected.to have_subject 'Group membership expiration date removed' - is_expected.to have_body_text group_member.user.name - is_expected.to have_body_text group.name - end - end - end - describe 'membership about to expire' do context "with group membership" do let_it_be(:group_member) { create(:group_member, source: group, expires_at: 7.days.from_now) } diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index bfa49bb1c982cf..80ab30ff2cd3ee 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -4043,10 +4043,9 @@ def commit_to_hash(commit) let_it_be(:member) { create(:group_member) } it 'triggers a notification about the expiration change' do - updated_member_expiration - - expect_delivery_jobs_count(1) - expect_enqueud_email('Group', member.id, mail: 'member_expiration_date_updated_email') + expect { updated_member_expiration } + .to have_enqueued_mail(Members::ExpirationDateUpdatedMailer, :email) + .with(params: { member: member, member_source_type: member.real_source_type }, args: []) end end -- GitLab From 59d10befbaab9affa380e26060a1df6749c308be Mon Sep 17 00:00:00 2001 From: Anas Shahid Date: Thu, 23 Oct 2025 02:16:25 -0400 Subject: [PATCH 2/2] Refactor member source type assignment in expiration date updated mailer spec --- spec/mailers/members/expiration_date_updated_mailer_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/mailers/members/expiration_date_updated_mailer_spec.rb b/spec/mailers/members/expiration_date_updated_mailer_spec.rb index 4efcd9f1e4406b..e674ad10dce2dc 100644 --- a/spec/mailers/members/expiration_date_updated_mailer_spec.rb +++ b/spec/mailers/members/expiration_date_updated_mailer_spec.rb @@ -10,7 +10,7 @@ let(:recipient) { create(:user) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- shared examples require persisted records let(:group) { create(:group) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- shared examples require persisted records let(:group_member) { create(:group_member, source: group, user: recipient, expires_at: 1.day.from_now) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- needs database persistence for updates - let(:member_source_type) { 'Group' } + let(:member_source_type) { group_member.real_source_type } subject(:email) { described_class.with(member: group_member, member_source_type: member_source_type).email } -- GitLab