Conversation
…specs Updated the payments controller to utilize `current_user` instead of `@current_user` in the `make_payment` method. Enhanced request specs to support full and partial payment scenarios, ensuring accurate redirection based on payment amounts. Added helper text in the payment view to clarify partial payment options for users.
…nd specs Refactor the `make_payment` method in the PaymentsController to include a new `validated_payment_amount` method for improved validation of payment amounts. Implement checks for missing, non-numeric, non-positive, and excessive amounts, redirecting users with appropriate alerts. Update request and controller specs to cover these new validation scenarios, ensuring robust error handling for various payment inputs.
Refactor payments controller to use current_user and enhance payment specs
There was a problem hiding this comment.
Pull request overview
This PR refactors the payment flow to use a validated payment amount derived from the signed-in user’s balance due, and improves partial-payment UX by clarifying that users can override the prefilled amount.
Changes:
- Added server-side validation (
validated_payment_amount) to reject invalid, non-integer, non-positive, over-balance, and over-cap payment amounts. - Added a balance computation helper (
current_balance_due) to support server-side validation against what the user currently owes. - Updated UI and specs to support/verify partial payments and validation failures.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| app/controllers/payments_controller.rb | Adds max cap constant, payment amount validation, and balance-due calculation used by make_payment. |
| app/views/payments/payment_show.html.erb | Adds helper text clarifying partial-payment behavior on the payment form. |
| spec/controllers/payments_controller_spec.rb | Updates controller specs to expect validation rejections for decimal/zero/over-balance inputs. |
| spec/requests/payments_spec.rb | Adds request specs for helper text and multiple invalid/valid make_payment scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| balance_due = current_balance_due | ||
| return nil if balance_due <= 0 | ||
|
|
||
| max_amount = [balance_due.floor, MAX_PAYMENT_AMOUNT].min | ||
| return nil if amount > max_amount |
There was a problem hiding this comment.
validated_payment_amount caps payments using balance_due.floor. Since lodging/partner costs are stored as decimals, current_balance_due can include cents; flooring would make it impossible to pay the actual full balance (e.g., 100.50 becomes a max of 100) and could leave an unpayable remainder. Consider normalizing all money math to integer cents (preferred) or using a rounding strategy that guarantees the remaining balance can be paid off without allowing unintended overpayment.
There was a problem hiding this comment.
Balance and costs are always in rounded dollar amounts
| <small class="form-text text-muted mt-2"> | ||
| The amount is prefilled with your full balance due, but you can enter a different amount to make a partial payment. | ||
| </small> |
There was a problem hiding this comment.
The helper text says the amount is prefilled with the user's full balance due, but the form field is actually prefilled with @balance_due.to_i (truncating any cents). If balances can include cents, this message will be inaccurate/confusing. Either change the prefill behavior to match the message or adjust the message to reflect the rounding/truncation policy.
There was a problem hiding this comment.
Balance and costs are always in rounded dollar amounts
| MAX_PAYMENT_AMOUNT = 2000 | ||
|
|
There was a problem hiding this comment.
MAX_PAYMENT_AMOUNT is introduced here, but the view still hard-codes within: 1..2000. This risks the UI and server validations drifting apart if the cap changes. Consider referencing the controller (or shared) constant from the view, or extracting the cap into a shared configuration constant used by both.
| max_amount = [balance_due.floor, MAX_PAYMENT_AMOUNT].min | ||
| return nil if amount > max_amount | ||
|
|
There was a problem hiding this comment.
The new server-side cap (MAX_PAYMENT_AMOUNT / max_amount) is part of the validation behavior but isn't directly exercised in specs (e.g., amount > 2000 when balance_due is higher). Adding a test for the cap will help prevent regressions and ensure the intended limit is enforced independently of the client-side number_field constraint.
Updated the payment view to use a dynamic maximum payment amount based on the balance due, enhancing user experience. Introduced a new method in the PaymentsController to calculate the maximum payment amount, ensuring consistent validation logic across the application.
Implemented a new test case in the payments request specs to ensure that payment amounts exceeding the defined maximum limit are correctly rejected, even when the balance due is higher. This enhances the validation coverage for payment processing scenarios.
This pull request enhances the payment flow by adding robust validation for payment amounts and improving the user experience around partial payments. The main changes include stricter server-side validation to prevent invalid or excessive payment amounts, improved test coverage for these scenarios, and updated UI messaging to clarify partial payment options.
Payment Amount Validation Improvements:
validated_payment_amountmethod inPaymentsControllerto ensure payment amounts are positive integers, do not exceed the user's current balance due, and are capped at a maximum of 2000. Decimal, zero, negative, or excessive values are now rejected, and users are redirected with an alert if validation fails. (app/controllers/payments_controller.rb,def make_payment,def validated_payment_amount,def current_balance_due)User Experience Enhancements:
app/views/payments/payment_show.html.erb)Test Coverage and Behavior:
spec/controllers/payments_controller_spec.rb,spec/requests/payments_spec.rb)