Make minimum amount env & other small change for euros#1165
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughExternalized the minimum payment amount into config.x.min_payment_amount (from MIN_PAYMENT_AMOUNT), propagated it to frontend dataset and JS, replaced hard-coded 20/21.80 values in validations and mailer, changed mailer header logo, and made the insufficient-credit mail render conditional payment-method options; adjusted OTP JSON handling in login view. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## staging #1165 +/- ##
===========================================
+ Coverage 77.22% 77.24% +0.01%
===========================================
Files 54 54
Lines 1348 1349 +1
===========================================
+ Hits 1041 1042 +1
Misses 307 307 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/views/user_credit_mailer/insufficient_credit_mail.html.erb (1)
4-4: Replace hardcoded "5 euro" with the configurable minimum amount.Line 4 hardcodes "minstens 5 euro positief" but this PR externalizes the minimum amount to
config.x.min_payment_amount. For consistency with the payment model validation, mailer logic, and frontend display, this should dynamically reference the configured value.Apply this diff to use the config value:
- U wordt verzocht dit aan te vullen tot minstens 5 euro positief zoals + U wordt verzocht dit aan te vullen tot minstens <%= number_to_currency(Rails.application.config.x.min_payment_amount, unit: '€') %> positief zoals
🧹 Nitpick comments (1)
app/views/sofia_accounts/login.html.erb (1)
122-132: Previous error handling issue has been addressed.The error message handling is now correctly positioned outside the state conditionals (lines 130-132), ensuring errors are displayed regardless of the response state. This resolves the concern from the previous review.
Consider aligning error message handling with
authenticate_login.Line 131 passes
{ allowHtml: true }toshowAuthenticateFlashMessage, while the equivalent call inauthenticate_login(line 100) does not. Unless OTP error messages specifically require HTML rendering, consider maintaining consistency between both functions for security and maintainability.Apply this diff if HTML rendering isn't required for OTP errors:
if (json.error_message) { - showAuthenticateFlashMessage(json.error_message, { allowHtml: true }); + showAuthenticateFlashMessage(json.error_message); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/images/sofia.pngis excluded by!**/*.png
📒 Files selected for processing (5)
app/models/payment.rb(1 hunks)app/views/layouts/mailer.html.erb(1 hunks)app/views/sofia_accounts/login.html.erb(1 hunks)app/views/user_credit_mailer/insufficient_credit_mail.html.erb(1 hunks)config/application.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/models/payment.rb
- config/application.rb
- app/views/layouts/mailer.html.erb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (4)
app/views/user_credit_mailer/insufficient_credit_mail.html.erb (4)
10-31: LGTM! Clean conditional rendering for dual payment methods.The logic correctly checks for both payment method configurations and presents clear options to users. The IBAN consistency issue noted in past reviews has been resolved (line 28 correctly uses
personal_transaction_iban).
32-39: LGTM! Mollie-only path is correct.Clean fallback when only iDEAL payment is configured.
40-45: LGTM! IBAN-only path is correct.Clean fallback when only bank transfer is configured. The IBAN reference is consistent with the conditional check.
46-50: LGTM! Good defensive programming.The else branch handles the case when no payment method is configured, directing users to contact the treasurer rather than showing broken or missing instructions.
The last PR before Euros use the system on there first borrel
Summary by CodeRabbit
New Features
Bug Fixes
UI/Style
✏️ Tip: You can customize this high-level summary in your review settings.