diff --git a/app/mailers/emails/members.rb b/app/mailers/emails/members.rb index 227b8f6695c19538cfd3b4428168713a371c86c6..61262af40200b6f42c4e74e6a0a6f817837f6dd7 100644 --- a/app/mailers/emails/members.rb +++ b/app/mailers/emails/members.rb @@ -39,27 +39,6 @@ def member_expiration_date_updated_email(member_source_type, member_id) 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 - - return unless member_exists? - return unless member.expires_at - - @days_to_expire = (member.expires_at - Date.today).to_i - - return if @days_to_expire <= 0 - - email_with_layout( - to: member.user.notification_email_for(notification_group), - subject: subject( - s_("Your membership will expire in %{days_to_expire} days") % { - days_to_expire: @days_to_expire - } - ) - ) - end - # rubocop: disable CodeReuse/ActiveRecord def member @member ||= Member.find_by(id: @member_id) diff --git a/app/mailers/members/about_to_expire_mailer.rb b/app/mailers/members/about_to_expire_mailer.rb new file mode 100644 index 0000000000000000000000000000000000000000..19afe01a2fb449b97abf04973770388ea9667be9 --- /dev/null +++ b/app/mailers/members/about_to_expire_mailer.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module Members + class AboutToExpireMailer < ApplicationMailer + helper EmailsHelper + + helper_method :member, :member_source, :user, :days_to_expire + + layout 'mailer' + + def email + return if member.blank? || member.expires_at.blank? || days_to_expire <= 0 + return unless member.notifiable?(:mention) + + mail_with_locale( + to: user.notification_email_for(member_source.notification_group), + subject: EmailsHelper.subject_with_prefix_and_suffix([email_subject_text]) + ) + end + + private + + delegate :user, to: :member + delegate :source, to: :member, prefix: true + + def member + params[:member] + end + + def days_to_expire + @days_to_expire ||= (member.expires_at - Time.zone.today).to_i + end + + def email_subject_text + subject = "Your membership will expire in #{days_to_expire} days" + s_(subject) + end + end +end diff --git a/app/mailers/previews/members/about_to_expire_mailer_preview.rb b/app/mailers/previews/members/about_to_expire_mailer_preview.rb new file mode 100644 index 0000000000000000000000000000000000000000..2744bde202afc1e425220d1c3adc8b979e0a4d91 --- /dev/null +++ b/app/mailers/previews/members/about_to_expire_mailer_preview.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Members + class AboutToExpireMailerPreview < ActionMailer::Preview + def email + Members::AboutToExpireMailer.with(member: member).email.message # rubocop:disable CodeReuse/ActiveRecord -- false positive + end + + private + + def member + project.add_member(user, Gitlab::Access::GUEST, expires_at: 7.days.from_now.to_date) + end + + def project + Project.first + end + + def user + User.last + end + end +end diff --git a/app/mailers/previews/notify_preview.rb b/app/mailers/previews/notify_preview.rb index 41977092ed5f01df265257c91a63563143882ec3..b073c56061e2a9ae3f5939b7632b81da2e757d0c 100644 --- a/app/mailers/previews/notify_preview.rb +++ b/app/mailers/previews/notify_preview.rb @@ -204,13 +204,6 @@ def member_access_granted_email Notify.member_access_granted_email(member.source_type, member.id).message end - def member_about_to_expire_email - cleanup do - member = project.add_member(user, Gitlab::Access::GUEST, expires_at: 7.days.from_now.to_date) - Notify.member_about_to_expire_email('project', member.id).message - end - end - def pages_domain_enabled_email cleanup do Notify.pages_domain_enabled_email(pages_domain, user).message diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 8001d5f3a989d24c3c7cec94353190433c142966..31f5c301d73259a5133ab13d58efdba8ec128bc5 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -536,12 +536,6 @@ def updated_member_expiration(member) mailer.member_expiration_date_updated_email(member.real_source_type, member.id).deliver_later end - def member_about_to_expire(member) - return true unless member.notifiable?(:mention) - - mailer.member_about_to_expire_email(member.real_source_type, member.id).deliver_later - end - def project_was_moved(project, old_path_with_namespace) recipients = project_moved_recipients(project) recipients = notifiable_users(recipients, :custom, custom_action: :moved_project, project: project) diff --git a/app/views/members/about_to_expire_mailer/email.html.haml b/app/views/members/about_to_expire_mailer/email.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..f3ed21e19442578bdae66bb3ecf19b710b833219 --- /dev/null +++ b/app/views/members/about_to_expire_mailer/email.html.haml @@ -0,0 +1,6 @@ += email_default_heading(say_hi(user)) + +%p + = member_about_to_expire_text(member_source, days_to_expire, format: :html) +%p + = member_about_to_expire_link(member, member_source, format: :html) diff --git a/app/views/members/about_to_expire_mailer/email.text.erb b/app/views/members/about_to_expire_mailer/email.text.erb new file mode 100644 index 0000000000000000000000000000000000000000..3ad75d3e586eceaa5c7d287a2da456ca3b53506e --- /dev/null +++ b/app/views/members/about_to_expire_mailer/email.text.erb @@ -0,0 +1,5 @@ +<%= say_hi(user) %> + +<%= member_about_to_expire_text(member_source, days_to_expire) %> + +<%= member_about_to_expire_link(member, member_source) %> diff --git a/app/views/notify/member_about_to_expire_email.html.haml b/app/views/notify/member_about_to_expire_email.html.haml deleted file mode 100644 index a9f92d90ae639f9a4ac700df3751c6ab06e9164c..0000000000000000000000000000000000000000 --- a/app/views/notify/member_about_to_expire_email.html.haml +++ /dev/null @@ -1,6 +0,0 @@ -= email_default_heading(say_hi(@member.user)) - -%p - = member_about_to_expire_text(@member_source, @days_to_expire, format: :html) -%p - = member_about_to_expire_link(@member, @member_source, format: :html) diff --git a/app/views/notify/member_about_to_expire_email.text.erb b/app/views/notify/member_about_to_expire_email.text.erb deleted file mode 100644 index 0c6e78bf5017240237c60c78fe8b9104c1438c4f..0000000000000000000000000000000000000000 --- a/app/views/notify/member_about_to_expire_email.text.erb +++ /dev/null @@ -1,5 +0,0 @@ -<%= say_hi(@member.user) %> - -<%= member_about_to_expire_text(@member_source, @days_to_expire) %> - -<%= member_about_to_expire_link(@member, @member_source) %> diff --git a/app/workers/members/expiring_email_notification_worker.rb b/app/workers/members/expiring_email_notification_worker.rb index 9e86653032a1ed47cb6735cff0742e75db8e327e..457e1881ac2abe35d3edcea4a900f11c4dd6af31 100644 --- a/app/workers/members/expiring_email_notification_worker.rb +++ b/app/workers/members/expiring_email_notification_worker.rb @@ -18,7 +18,7 @@ def perform(member_id) return unless valid_for_notification?(member) with_context(user: member.user) do - notification_service.member_about_to_expire(member) + Members::AboutToExpireMailer.with(member: member).email.deliver_later # rubocop:disable CodeReuse/ActiveRecord -- false positive Gitlab::AppLogger.info(message: "Notifying user about expiring membership", member_id: member.id) member.update(expiry_notified_at: Time.current) @@ -31,11 +31,5 @@ def valid_for_notification?(member) member.user.active? && member.user.human? end - - private - - def notification_service - @notification_service ||= NotificationService.new - end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index bc490f6030be9b9cf04f544bc7d38bb4ccdc1800..2175a28434c7d463e0dc27c9a83cf7301fff108f 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -77423,9 +77423,6 @@ msgstr "" msgid "Your membership in %{project_or_group} %{project_or_group_name} will expire in %{days_formatted}." msgstr "" -msgid "Your membership will expire in %{days_to_expire} days" -msgstr "" - msgid "Your merge requests" msgstr "" diff --git a/spec/mailers/members/about_to_expire_mailer_spec.rb b/spec/mailers/members/about_to_expire_mailer_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..d74e0a1b318370b8b9e4583f288a5fbc3b52144d --- /dev/null +++ b/spec/mailers/members/about_to_expire_mailer_spec.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Members::AboutToExpireMailer, feature_category: :groups_and_projects do + include EmailSpec::Matchers + using RSpec::Parameterized::TableSyntax + + describe '#email' do + let(:recipient) { build(:recipient) } + let(:group) { build(:group) } + let(:group_member) { build(:group_member, source: group, user: recipient, expires_at: 7.days.from_now) } + let(:project) { build(:project, :public, group: group) } + let(:project_member) { build(:project_member, source: project, user: recipient, expires_at: 7.days.from_now) } + let(:notifiable) { true } + + before do + allow(member).to receive(:notifiable?).with(:mention).and_return(notifiable) + end + + subject(:email) { described_class.with(member: member).email } + + where(:member, :source, :type) do + ref(:group_member) | ref(:group) | 'group' + ref(:project_member) | ref(:project) | 'project' + end + + with_them do + 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' + + it 'contains all the useful information', :aggregate_failures do + is_expected.to deliver_to member.user.email + is_expected.to have_subject "Your membership will expire in 7 days" + is_expected.to have_body_text "#{type} will expire in 7 days." + is_expected.to have_body_text public_send(:"#{type}_url", source) + is_expected.to have_body_text public_send(:"#{type}_#{type}_members_url", source) + end + + context 'with no expiry on membership' do + before do + group_member.expires_at = nil + project_member.expires_at = nil + end + + it_behaves_like 'no email is sent' + end + + context 'with expired membership' do + before do + group_member.expires_at = Date.current + project_member.expires_at = Date.current + end + + it_behaves_like 'no email is sent' + end + + context 'when the recipient is not notifiable' do + let(:notifiable) { false } + + it_behaves_like 'no email is sent' + end + end + end +end diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 280d785a960489d51e86d135ee97e47e6d23990c..abcfb041c1ec92bc0fd32abd6de121e9fbc5f631 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -1752,68 +1752,6 @@ def invite_to_group(group, inviter:, user: nil) 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) } - - subject { described_class.member_about_to_expire_email("Namespace", 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 deliver_to group_member.user.email - is_expected.to have_subject "Your membership will expire in 7 days" - is_expected.to have_body_text "group will expire in 7 days." - is_expected.to have_body_text group_url(group) - is_expected.to have_body_text group_group_members_url(group) - end - end - - context "with project membership" do - let_it_be(:project_member) { create(:project_member, source: project, expires_at: 7.days.from_now) } - - subject { described_class.member_about_to_expire_email('Project', project_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 deliver_to project_member.user.email - is_expected.to have_subject "Your membership will expire in 7 days" - is_expected.to have_body_text "project will expire in 7 days." - is_expected.to have_body_text project_url(project) - is_expected.to have_body_text project_project_members_url(project) - end - end - - context "with expired membership" do - let_it_be(:project_member) { create(:project_member, source: project, expires_at: Date.today) } - - subject { described_class.member_about_to_expire_email('Project', project_member.id) } - - it 'not deliver expiry email' do - should_not_email_anyone - end - end - - context "with expiry notified membership" do - let_it_be(:project_member) { create(:project_member, source: project, expires_at: 7.days.from_now, expiry_notified_at: Date.today) } - - subject { described_class.member_about_to_expire_email('Project', project_member.id) } - - it 'not deliver expiry email' do - should_not_email_anyone - end - end - end - describe 'admin notification' do let(:example_site_path) { root_path } let(:user) { create(:user) } diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index bfa49bb1c982cf9c7848c9d59b09a4760295eac3..b2f26407e89949da22b6798ff11694c4c795cb08 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -3962,27 +3962,6 @@ def commit_to_hash(commit) end end end - - describe '#member_about_to_expire' do - let_it_be(:group_member) { create(:group_member, expires_at: 7.days.from_now.to_date) } - let_it_be(:project_member) { create(:project_member, expires_at: 7.days.from_now.to_date) } - - context "with group member" do - it 'emails the user that their group membership will be expired' do - notification.member_about_to_expire(group_member) - - should_email(group_member.user) - end - end - - context "with project member" do - it 'emails the user that their project membership will be expired' do - notification.member_about_to_expire(project_member) - - should_email(project_member.user) - end - end - end end describe '#new_member', :deliver_mails_inline do diff --git a/spec/workers/members/expiring_email_notification_worker_spec.rb b/spec/workers/members/expiring_email_notification_worker_spec.rb index 547d291473d3fd6ea16ccc0c2bbfe115f90b8b75..40fda8c05bcca4083a2e05c6e576eae4d25c5c6b 100644 --- a/spec/workers/members/expiring_email_notification_worker_spec.rb +++ b/spec/workers/members/expiring_email_notification_worker_spec.rb @@ -26,12 +26,14 @@ create(:project_member, :guest, user: project_bot, expires_at: 7.days.from_now) end + before do + allow(Members::AboutToExpireMailer).to receive(:with).and_call_original + end + describe '#perform' do context "with not notified member" do it "notify member" do - expect_next_instance_of(NotificationService) do |notification_service| - expect(notification_service).to receive(:member_about_to_expire).with(member) - end + expect(Members::AboutToExpireMailer).to receive(:with).with(member: member) worker.perform(member.id) @@ -39,19 +41,19 @@ end it 'does not notify blocked member' do - expect(NotificationService).not_to receive(:new) + expect(Members::AboutToExpireMailer).not_to receive(:with) worker.perform(blocked_member.id) end it 'does not notify invited member' do - expect(NotificationService).not_to receive(:new) + expect(Members::AboutToExpireMailer).not_to receive(:with) worker.perform(invited_member.id) end it 'does not notify non-human members' do - expect(NotificationService).not_to receive(:new) + expect(Members::AboutToExpireMailer).not_to receive(:with) worker.perform(bot_member.id) end @@ -59,7 +61,7 @@ context "with notified member" do it "not notify member" do - expect(NotificationService).not_to receive(:new) + expect(Members::AboutToExpireMailer).not_to receive(:with) worker.perform(notified_member.id) end