diff --git a/app/mailers/emails/members.rb b/app/mailers/emails/members.rb index 227b8f6695c19538cfd3b4428168713a371c86c6..4eb3589e9b47b2917b990284326d4b9eb3983969 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 0000000000000000000000000000000000000000..ae1cb6c41a21bf9fa537f6b4d11691caf581fefb --- /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 8001d5f3a989d24c3c7cec94353190433c142966..0a21e9ed1ef19f62baea73ccda4072732175fe40 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 0000000000000000000000000000000000000000..ceb7a6747812f66b57bdfb3fbe7f341d9737d3a4 --- /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 0000000000000000000000000000000000000000..adab23500c7c58e7fd497a3cf7a1e3b16c2af5c8 --- /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 6c4db22eeaac4e82f13ea7b16528d9d97f4f054c..0000000000000000000000000000000000000000 --- 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 8b3a5a55e77e81545ed8e17ece36a3f81fe5daa8..0000000000000000000000000000000000000000 --- 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 0000000000000000000000000000000000000000..e674ad10dce2dcbc4b6f124b89da8bcd4747bc0a --- /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_member.real_source_type } + + 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 280d785a960489d51e86d135ee97e47e6d23990c..26298be5bb0d3e17425487206f09fac1d76daff2 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 bfa49bb1c982cf9c7848c9d59b09a4760295eac3..80ab30ff2cd3ee444c2abaa513571eab9b95ad98 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