Skip to content

Feature: more specific mailing #1231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lodewiges merged 8 commits into from
Feb 18, 2026
Merged

Feature: more specific mailing #1231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0119de7
Intial commit
lodewiges Feb 10, 2026
d86d6ea
fix for tests
lodewiges Feb 10, 2026
e78cc97
fix lint
lodewiges Feb 10, 2026
c8071ad
fix lint
lodewiges Feb 13, 2026
23fb8e2
make the code cleaner
lodewiges Feb 18, 2026
343b936
remove unnessary lint exception
lodewiges Feb 18, 2026
9dd7310
revert my smart idea
lodewiges Feb 18, 2026
2f133d2
fix lint
lodewiges Feb 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions app/jobs/credit_insufficient_notification_job.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,9 @@ def users_with_insufficient_credit
end

def send_notification_delivery_reports(success_count, unnotifyable_users)
User.treasurer.each do |treasurer|
UserCreditMailer.credit_delivery_report_mail(
treasurer, success_count, unnotifyable_users
).deliver_later
end
UserCreditMailer.credit_delivery_report_mail(
success_count, unnotifyable_users
).deliver_later

return if Rails.env.local?

Expand Down
8 changes: 5 additions & 3 deletions app/mailers/user_credit_mailer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,17 @@ def insufficient_credit_mail(user)
mail to: user.email, subject: 'Verzoek met betrekking tot uw Zatladder saldo'
end

def credit_delivery_report_mail(treasurer, success_count, unnotifyable_users)
@user = treasurer
def credit_delivery_report_mail(success_count, unnotifyable_users)
@user = Struct.new(:name).new(
Rails.application.config.x.treasurer_name
)
Comment on lines +11 to +13
Copy link

@coderabbitai coderabbitai bot Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Extract the Struct to a constant and guard against a nil treasurer name.

Two concerns here:

  1. Struct.new(:name) anti-pattern — Struct.new(...) inside an instance method creates an unnecessary new anonymous class on every invocation, which also creates new independent entries in the method cache. Since treasurer_name is a single static field, define the Struct as a class-level constant instead.

  2. Silent blank name — config/application.rb sets treasurer_name to ENV.fetch('TREASURER_NAME', nil), so @user.name will be nil when the env var is absent. If the template uses @user.name in a greeting, it renders blank with no warning.

♻️ Proposed fix 
+  TreasurerRecipient = Struct.new(:name)
+
   # rubocop:disable Metrics/AbcSize
   def credit_delivery_report_mail(success_count, unnotifyable_users)
-    `@user` = Struct.new(:name).new(
-      Rails.application.config.x.treasurer_name
-    )
+    `@user` = TreasurerRecipient.new(
+      Rails.application.config.x.treasurer_name || 'Treasurer'
+    )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@user = Struct.new(:name).new(
Rails.application.config.x.treasurer_name
)
TreasurerRecipient = Struct.new(:name)
# rubocop:disable Metrics/AbcSize
def credit_delivery_report_mail(success_count, unnotifyable_users)
`@user` = TreasurerRecipient.new(
Rails.application.config.x.treasurer_name || 'Treasurer'
)
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@app/mailers/user_credit_mailer.rb` around lines 12 - 14, Extract the
anonymous Struct into a class-level constant (e.g., TREASURER =
Struct.new(:name)) and stop creating a new Struct on each call; then fetch the
configured treasurer name into a local (e.g., treasurer_name =
Rails.application.config.x.treasurer_name.presence || 'Treasurer') and
instantiate `@user` via `@user` = TREASURER.new(treasurer_name) so `@user.name` is
never nil and the Struct allocation is constant across invocations.

@unnotifyable_users = unnotifyable_users
@success_count = success_count
@title = 'Notificatie over de saldomail'

subject = "Er is #{@success_count.positive? ? 'een' : 'geen'} saldomail verstuurd"

mail to: treasurer.email, subject:
mail to: Rails.application.config.x.treasurer_email, subject:
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The report email template currently won’t display the success count because it uses a non-output ERB tag (<% @success_count %>) instead of <%= @success_count %> in app/views/user_credit_mailer/credit_delivery_report_mail.html.erb:4. Since this PR changes how the report is delivered, it’s worth fixing so the email contains the intended information.

Suggested change
mail to: Rails.application.config.x.treasurer_email, subject:
mail to: Rails.application.config.x.treasurer_email, subject: subject

Copilot uses AI. Check for mistakes.
end

Comment on lines 14 to 22
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credit_delivery_report_mail no longer sets @user, but the shared mailer layout renders Beste <%= @user.name %>, (see app/views/layouts/mailer.html.erb:364). This will raise a NoMethodError when generating the report email. Set @user to a suitable object (e.g., a lightweight struct using config.x.treasurer_name/treasurer_title) or update the mailer layout to handle the report mail without requiring @user.

Copilot uses AI. Check for mistakes.
def new_credit_mutation_mail(credit_mutation)
Expand Down
8 changes: 1 addition & 7 deletions spec/jobs/credit_insufficient_notification_job_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,6 @@
let(:user_without_email) do
create(:user, name: 'Good Emailless', email: nil, provider: 'some_external_source')
end
let(:treasurer) do
create(:user,
name: 'Sis Treasures', email: 'treasurer@csvalpha.nl',
roles: [create(:role, role_type: :treasurer)])
end
let(:emails) { ActionMailer::Base.deliveries }

subject(:job) { perform_enqueued_jobs { described_class.perform_now } }
Expand All @@ -25,14 +20,13 @@
create(:credit_mutation, user: negative_user, amount: -2)
create(:credit_mutation, user: negative_user_without_email, amount: -2)
user_without_email
treasurer
job
end

it { expect(emails.size).to eq 2 }
it { expect(emails.first.to.first).to eq negative_user.email }
it { expect(emails.first.body.to_s).to include 'http://testhost:1337/payments/add' }
it { expect(emails.second.to.first).to eq treasurer.email }
it { expect(emails.second.to.first).to eq Rails.application.config.x.treasurer_email }
it { expect(emails.second.body.to_s).to include negative_user_without_email.name }
it { expect(emails.second.body.to_s).not_to include user_without_email.name }
end
Expand Down
3 changes: 1 addition & 2 deletions spec/mailers/previews/user_credit_mailer_preview.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,10 @@ def insufficient_credit_mail
end

def credit_delivery_report_mail
treasurer = FactoryBot.create(:user, :treasurer)
unnotifyable_users = FactoryBot.create_list(:user, 2).map(&:name)
success_count = 2

UserCreditMailer.credit_delivery_report_mail(treasurer, success_count, unnotifyable_users)
UserCreditMailer.credit_delivery_report_mail(success_count, unnotifyable_users)
end

def new_credit_mutation_mail
Expand Down