Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces per-treasurer iteration with a single consolidated credit delivery report sent to the configured treasurer email ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors the credit delivery report email system to send notifications to a single configured email address (TREASURER_EMAIL) instead of iterating through all users with the treasurer role. This change reduces email duplication, prevents administrative accounts from receiving unnecessary notifications, and decouples notification recipients from user roles.
Changes:
- Removed the loop over
User.treasurerin favor of a single email to the configured treasurer address - Updated the
credit_delivery_report_mailmethod signature to remove thetreasurerparameter - Updated tests and mailer previews to reflect the new method signature
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| app/jobs/credit_insufficient_notification_job.rb | Simplified notification sending by removing the loop over treasurer users and sending a single email to the configured address |
| app/mailers/user_credit_mailer.rb | Removed the treasurer parameter and updated the email recipient to use Rails.application.config.x.treasurer_email |
| spec/jobs/credit_insufficient_notification_job_spec.rb | Removed the treasurer user fixture and updated the test to verify email is sent to the configured address |
| spec/mailers/previews/user_credit_mailer_preview.rb | Updated the preview to match the new method signature without the treasurer parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## staging #1231 +/- ##
===========================================
- Coverage 77.61% 77.59% -0.02%
===========================================
Files 54 54
Lines 1407 1406 -1
===========================================
- Hits 1092 1091 -1
Misses 315 315 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @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: | ||
| end | ||
|
|
There was a problem hiding this comment.
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.
| subject = "Er is #{@success_count.positive? ? 'een' : 'geen'} saldomail verstuurd" | ||
|
|
||
| mail to: treasurer.email, subject: | ||
| mail to: Rails.application.config.x.treasurer_email, subject: |
There was a problem hiding this comment.
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.
| mail to: Rails.application.config.x.treasurer_email, subject: | |
| mail to: Rails.application.config.x.treasurer_email, subject: subject |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/mailers/user_credit_mailer.rb (1)
18-18:TREASURER_EMAILunconfigured silently falls back totreasurer@example.com.With this change the configured address is now the sole recipient. If
TREASURER_EMAILis not set in a production or staging environment, credit delivery reports will route totreasurer@example.com(the default inconfig/application.rbLine 63) with no error or warning. Consider adding a startup check or at minimum a log warning when the value matches the default placeholder, so misconfiguration is detected early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/mailers/user_credit_mailer.rb` at line 18, The mailer call in UserCreditMailer (the mail line using Rails.application.config.x.treasurer_email) will silently send to the default treasurer@example.com when TREASURER_EMAIL is not configured; add a detection + warning so misconfiguration is visible: implement a small check (either in an application initializer or in UserCreditMailer.before_action/initialize) that compares Rails.application.config.x.treasurer_email against the known placeholder "treasurer@example.com" and logs a WARN/error message (including the current environment) when they match, and optionally raise or notify depending on env (e.g., raise in production/staging or fail fast at startup). Ensure the check references the exact config key (Rails.application.config.x.treasurer_email) and the mailer class name (UserCreditMailer) so it triggers before sending.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/mailers/user_credit_mailer.rb`:
- Line 18: The mailer call in UserCreditMailer (the mail line using
Rails.application.config.x.treasurer_email) will silently send to the default
treasurer@example.com when TREASURER_EMAIL is not configured; add a detection +
warning so misconfiguration is visible: implement a small check (either in an
application initializer or in UserCreditMailer.before_action/initialize) that
compares Rails.application.config.x.treasurer_email against the known
placeholder "treasurer@example.com" and logs a WARN/error message (including the
current environment) when they match, and optionally raise or notify depending
on env (e.g., raise in production/staging or fail fast at startup). Ensure the
check references the exact config key
(Rails.application.config.x.treasurer_email) and the mailer class name
(UserCreditMailer) so it triggers before sending.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/mailers/user_credit_mailer.rb (1)
17-17: Placeholder fallbacktreasurer@example.comwill silently swallow emails ifTREASURER_EMAILis unset.From
config/application.rb:config.x.treasurer_email = ENV.fetch('TREASURER_EMAIL', 'treasurer@example.com')If
TREASURER_EMAILis not configured in a deployment environment, every credit delivery report will be silently delivered to a non-existent placeholder address rather than failing loudly. This PR concentrates all treasurer notifications to this single address, which makes the misconfiguration risk higher than before — previously, at least some real users would still receive the email.Consider replacing the soft default with a hard
KeyErrorat startup, or adding an initializer check:🔧 Suggested change in
config/application.rb- config.x.treasurer_email = ENV.fetch('TREASURER_EMAIL', 'treasurer@example.com') + config.x.treasurer_email = ENV.fetch('TREASURER_EMAIL')Or, if a fallback is required for non-production environments, guard it explicitly:
- config.x.treasurer_email = ENV.fetch('TREASURER_EMAIL', 'treasurer@example.com') + config.x.treasurer_email = ENV.fetch('TREASURER_EMAIL') do + raise "TREASURER_EMAIL must be configured" if Rails.env.production? + 'treasurer@example.com' + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/mailers/user_credit_mailer.rb` at line 17, The mailer currently uses a soft default treasurer@example.com via config.x.treasurer_email which will silently swallow real reports; remove the fallback and make TREASURER_EMAIL required by changing the config to use ENV.fetch('TREASURER_EMAIL') with no default (or add an application initializer that raises if ENV['TREASURER_EMAIL'] is blank in non-development/test), and keep app/mailers/user_credit_mailer.rb referencing Rails.application.config.x.treasurer_email unchanged so any missing config surfaces as a startup error rather than routing emails to the placeholder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/mailers/user_credit_mailer.rb`:
- Line 17: The mailer currently uses a soft default treasurer@example.com via
config.x.treasurer_email which will silently swallow real reports; remove the
fallback and make TREASURER_EMAIL required by changing the config to use
ENV.fetch('TREASURER_EMAIL') with no default (or add an application initializer
that raises if ENV['TREASURER_EMAIL'] is blank in non-development/test), and
keep app/mailers/user_credit_mailer.rb referencing
Rails.application.config.x.treasurer_email unchanged so any missing config
surfaces as a startup error rather than routing emails to the placeholder.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/mailers/user_credit_mailer.rb (2)
10-10:rubocop:disable Metrics/AbcSizeis likely only needed because of theStruct.newchain.The chained
.new(:name).new(...)call adds extra branches that inflate the ABC score. If the Struct is extracted to a constant (as suggested above), this disable directive can probably be removed, keeping the metrics cop active as a natural quality gate.Also applies to: 23-23
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/mailers/user_credit_mailer.rb` at line 10, The Metrics/AbcSize disable in user_credit_mailer.rb was added to suppress the inflated ABC score caused by the chained Struct.new(...).new(...) pattern; extract the temporary Struct into a named constant (e.g., define a USER_CREDIT_STRUCT = Struct.new(:name) or similar at top-level or inside the class) and replace the chained .new(...) usage with calls to that constant, which eliminates the extra branches and lets you remove the `# rubocop:disable Metrics/AbcSize` directive (also update the similar occurrence around line 23) so RuboCop can enforce the ABC metric normally.
21-21: EnsureTREASURER_EMAILis set in all environments to avoid silent misdelivery.
Rails.application.config.x.treasurer_emailfalls back to'treasurer@example.com'when the env var is absent. Since this mailer now sends all credit delivery reports to this single address, a missing or misconfigured env var in staging/production will silently route all reports to a placeholder address with no error raised.Consider adding a startup check (e.g., in an initializer) to assert that
TREASURER_EMAILis present and non-default before the application boots in production.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/mailers/user_credit_mailer.rb` at line 21, Add a startup assertion that the TREASURER_EMAIL env var is set and not the default placeholder to prevent silent misdelivery; create an initializer that reads Rails.application.config.x.treasurer_email (the same config used in user_credit_mailer.rb) and, in production (or any non-development env), raise or abort boot if the value is blank or equals 'treasurer@example.com' so the app fails fast when the treasurer address is misconfigured.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/mailers/user_credit_mailer.rb`:
- Around line 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.
---
Nitpick comments:
In `@app/mailers/user_credit_mailer.rb`:
- Line 10: The Metrics/AbcSize disable in user_credit_mailer.rb was added to
suppress the inflated ABC score caused by the chained Struct.new(...).new(...)
pattern; extract the temporary Struct into a named constant (e.g., define a
USER_CREDIT_STRUCT = Struct.new(:name) or similar at top-level or inside the
class) and replace the chained .new(...) usage with calls to that constant,
which eliminates the extra branches and lets you remove the `# rubocop:disable
Metrics/AbcSize` directive (also update the similar occurrence around line 23)
so RuboCop can enforce the ABC metric normally.
- Line 21: Add a startup assertion that the TREASURER_EMAIL env var is set and
not the default placeholder to prevent silent misdelivery; create an initializer
that reads Rails.application.config.x.treasurer_email (the same config used in
user_credit_mailer.rb) and, in production (or any non-development env), raise or
abort boot if the value is blank or equals 'treasurer@example.com' so the app
fails fast when the treasurer address is misconfigured.
| @user = Struct.new(:name).new( | ||
| Rails.application.config.x.treasurer_name | ||
| ) |
There was a problem hiding this comment.
Extract the Struct to a constant and guard against a nil treasurer name.
Two concerns here:
-
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. Sincetreasurer_nameis a single static field, define the Struct as a class-level constant instead. -
Silent blank name —
config/application.rbsetstreasurer_nametoENV.fetch('TREASURER_NAME', nil), so@user.namewill benilwhen the env var is absent. If the template uses@user.namein 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.
| @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.
2 participants
Summary
Refactors credit delivery report emails to send to a single configured email address (TREASURER_EMAIL) instead of every user with the treasurer role.
Motivation
Previously, all users with the treasurer role received credit notification emails. This caused:
Administrative accounts to receive unnecessary emails
Which annoyed me a lot, so it needed to change :)
Changes
This fixes #442 (kinda)
Summary by CodeRabbit
Refactor
Tests
Documentation / Previews