jocke/bilhej
1
0
Fork 0
Code Issues 6 Pull requests 6 Projects Releases 1 Packages Wiki Activity Actions

fix(payment): persist amountPaid on payment confirmation (#20) #27

Open
hermes wants to merge 1 commit from fix/persist-amount-paid into master
pull from: fix/persist-amount-paid
merge into: jocke:master
Conversation 0 Commits 1 Files changed 2 +10
hermes commented 2026-07-18 11:38:22 +00:00
Collaborator
Copy link

Why

Issue #20: confirmGuestPayment and confirmPayment set order status to PROCESSING but never called order.setAmountPaid(...). amountPaid always read null even after payment, blocking finance reconciliation.

Changes

  • OrderService: inject app.payment.letter-price (default 49) via @Value and call order.setAmountPaid(BigDecimal.valueOf(letterPrice)) before save in both confirmGuestPayment and confirmPayment
  • OrderServiceTest: assert amountPaid == 49 after both confirm paths

Test plan

  • ./gradlew :backend:test — BUILD SUCCESSFUL

Closes #20

## Why Issue #20: `confirmGuestPayment` and `confirmPayment` set order status to PROCESSING but never called `order.setAmountPaid(...)`. `amountPaid` always read null even after payment, blocking finance reconciliation. ## Changes - OrderService: inject `app.payment.letter-price` (default 49) via `@Value` and call `order.setAmountPaid(BigDecimal.valueOf(letterPrice))` before save in both `confirmGuestPayment` and `confirmPayment` - OrderServiceTest: assert `amountPaid == 49` after both confirm paths ## Test plan - [x] `./gradlew :backend:test` — BUILD SUCCESSFUL Closes #20
hermes added 1 commit 2026-07-18 11:38:23 +00:00
fix(payment): persist amountPaid on payment confirmation
Some checks failed
CI / Lint, type check, unit tests, coverage (pull_request) Successful in 3m26s
Details
CI / E2E browser tests (pull_request) Failing after 1m53s
Details
7d6d541354 
confirmGuestPayment and confirmPayment set the order status to
PROCESSING but never called order.setAmountPaid(...). As a result
amountPaid always read null even after payment, blocking finance
reconciliation against the Swish payout report.

The frontend test mock (GuestPaymentRedirect.spec.ts) expected
amountPaid: 49 but the real backend returned null.

Changes:
  - OrderService: inject app.payment.letter-price (default 49) via
    @Value and set order.setAmountPaid(BigDecimal.valueOf(letterPrice))
    before save in both confirmGuestPayment and confirmPayment
  - OrderServiceTest: assert amountPaid == 49 after both confirm paths

Verified: ./gradlew :backend:test — BUILD SUCCESSFUL

Closes #20
hermes reviewed 2026-07-18 11:41:18 +00:00
hermes left a comment
Author
Collaborator
Copy link

Verdict: Approve-worthy fix — amountPaid is now persisted on both payment-confirmation paths with state-guarded idempotency and matching tests. A few advisory nits; none block merge.

Critical

None.

Warnings

  • amountPaid records the configured letter price, not the amount actually charged. Both confirm methods set amountPaid = BigDecimal.valueOf(letterPrice) from app.payment.letter-price. Fine for the Phase 0 honor-system flow, but the field name (amount_paid) and the issue's "finance reconciliation" goal imply "what the customer paid." Once Stripe/Swish webhook integration lands, prefer populating this from the payment intent's captured amount (handles partial payments, refunds, price drift mid-flight). At minimum, add a Javadoc note that this currently equals the expected price, not a verified received amount.

Suggestions

  • The @Value config wiring is effectively untested. OrderServiceTest is a pure Mockito test (@InjectMocks does not resolve @Value), and the field initializer = 49 is identical to the resolved default — so the test passes even if app.payment.letter-price is misnamed, missing from application.yml, or @Value is deleted entirely. To actually exercise the config path: load the value via a @SpringBootTest slice and assert a non-sentinel price, or ReflectionTestUtils.setField(orderService, "letterPrice", 99) and assert 99, or at minimum drop the = 49 initializer so a broken @Value surfaces as 0.
  • Redundant default + int money type. @Value("${app.payment.letter-price:49}") private int letterPrice = 49; declares the 49 default twice and the same value also sits in application.yml. Consider private BigDecimal letterPrice; (Spring converts the string to BigDecimal) and order.setAmountPaid(letterPrice) directly — removes the BigDecimal.valueOf(int) cast, the redundant default, and future-proofs for öre (e.g. 49.50) since the column is already precision=10, scale=2.
  • DRY: shared transition block. confirmGuestPayment (lines ~78-82) and confirmPayment (lines ~100-104) run an identical setStatus(PROCESSING) -> setAmountPaid -> save -> notify -> return sequence; extract private Order markPaidAndProcessing(Order order) (duplication pre-existed, deepened here).
  • BigDecimal equality is scale-sensitive. assertEquals(new BigDecimal("49"), result.getAmountPaid()) passes here only because the mocked repo returns the same in-memory instance (scale 0). An integration test reloading from the scale=2 column would yield 49.00, and new BigDecimal("49").equals(49.00) is false. Prefer assertEquals(0, new BigDecimal("49").compareTo(result.getAmountPaid())), or assert new BigDecimal("49.00").

Looks Good

  • Idempotency preserved: both confirm methods require PENDING_PAYMENT (via requirePendingOwnedBy / getOrderByGuestToken + status check), so a duplicate confirmation throws InvalidOrderStateException instead of double-recording.
  • Security intact: price is config-driven (no request input reaches it, so no injection/XSS surface), and the userId/guestToken authorization checks are untouched.
  • Symmetric coverage: both confirm paths get the new amountPaid assertion; BigDecimal import added in both source and test.
  • Tight scope matching Issue #20.
**Verdict:** Approve-worthy fix — `amountPaid` is now persisted on both payment-confirmation paths with state-guarded idempotency and matching tests. A few advisory nits; none block merge. ### Critical None. ### Warnings - **`amountPaid` records the configured letter price, not the amount actually charged.** Both confirm methods set `amountPaid = BigDecimal.valueOf(letterPrice)` from `app.payment.letter-price`. Fine for the Phase 0 honor-system flow, but the field name (`amount_paid`) and the issue's "finance reconciliation" goal imply "what the customer paid." Once Stripe/Swish webhook integration lands, prefer populating this from the payment intent's captured amount (handles partial payments, refunds, price drift mid-flight). At minimum, add a Javadoc note that this currently equals the expected price, not a verified received amount. ### Suggestions - **The `@Value` config wiring is effectively untested.** `OrderServiceTest` is a pure Mockito test (`@InjectMocks` does not resolve `@Value`), and the field initializer `= 49` is identical to the resolved default — so the test passes even if `app.payment.letter-price` is misnamed, missing from `application.yml`, or `@Value` is deleted entirely. To actually exercise the config path: load the value via a `@SpringBootTest` slice and assert a non-sentinel price, or `ReflectionTestUtils.setField(orderService, "letterPrice", 99)` and assert `99`, or at minimum drop the `= 49` initializer so a broken `@Value` surfaces as `0`. - **Redundant default + `int` money type.** `@Value("${app.payment.letter-price:49}") private int letterPrice = 49;` declares the `49` default twice and the same value also sits in `application.yml`. Consider `private BigDecimal letterPrice;` (Spring converts the string to BigDecimal) and `order.setAmountPaid(letterPrice)` directly — removes the `BigDecimal.valueOf(int)` cast, the redundant default, and future-proofs for öre (e.g. 49.50) since the column is already `precision=10, scale=2`. - **DRY: shared transition block.** `confirmGuestPayment` (lines ~78-82) and `confirmPayment` (lines ~100-104) run an identical `setStatus(PROCESSING) -> setAmountPaid -> save -> notify -> return` sequence; extract `private Order markPaidAndProcessing(Order order)` (duplication pre-existed, deepened here). - **BigDecimal equality is scale-sensitive.** `assertEquals(new BigDecimal("49"), result.getAmountPaid())` passes here only because the mocked repo returns the same in-memory instance (scale 0). An integration test reloading from the `scale=2` column would yield `49.00`, and `new BigDecimal("49").equals(49.00)` is `false`. Prefer `assertEquals(0, new BigDecimal("49").compareTo(result.getAmountPaid()))`, or assert `new BigDecimal("49.00")`. ### Looks Good - **Idempotency preserved:** both confirm methods require `PENDING_PAYMENT` (via `requirePendingOwnedBy` / `getOrderByGuestToken` + status check), so a duplicate confirmation throws `InvalidOrderStateException` instead of double-recording. - **Security intact:** price is config-driven (no request input reaches it, so no injection/XSS surface), and the `userId`/`guestToken` authorization checks are untouched. - **Symmetric coverage:** both confirm paths get the new `amountPaid` assertion; `BigDecimal` import added in both source and test. - **Tight scope** matching Issue #20.
backend/src/main/java/se/bilhalsning/service/OrderService.java
hermes commented 2026-07-18 11:41:18 +00:00
Author
Collaborator
Copy link

💡 Redundant = 49 initializer + int money type. The 49 default lives in three places (the :49, the field init, and application.yml), and the = 49 masks @Value failures in the unit test (Mockito's @InjectMocks never resolves @Value, so the test rides on the initializer). Consider private BigDecimal letterPrice; and order.setAmountPaid(letterPrice) directly — Spring converts the string to BigDecimal and you're future-proofed for öre (the column is already scale=2). See review body for the full test-coverage note.

💡 Redundant `= 49` initializer + `int` money type. The `49` default lives in three places (the `:49`, the field init, and `application.yml`), and the `= 49` masks `@Value` failures in the unit test (Mockito's `@InjectMocks` never resolves `@Value`, so the test rides on the initializer). Consider `private BigDecimal letterPrice;` and `order.setAmountPaid(letterPrice)` directly — Spring converts the string to BigDecimal and you're future-proofed for öre (the column is already `scale=2`). See review body for the full test-coverage note.
hermes commented 2026-07-18 11:41:18 +00:00
Author
Collaborator
Copy link

♻️ The setStatus -> setAmountPaid -> save -> notify -> return block is duplicated in confirmPayment (line 102). Extract private Order markPaidAndProcessing(Order order) to collapse it (the duplication pre-existed but is deepened here).

♻️ The `setStatus -> setAmountPaid -> save -> notify -> return` block is duplicated in `confirmPayment` (line 102). Extract `private Order markPaidAndProcessing(Order order)` to collapse it (the duplication pre-existed but is deepened here).
hermes commented 2026-07-18 11:41:18 +00:00
Author
Collaborator
Copy link

⚠️ amountPaid here is the configured letterPrice, not an amount verified as received from a payment provider. Fine for Phase 0 honor-system, but once Stripe/Swish webhooks land this should come from the captured payment-intent amount (handles partial payments / refunds / price drift). Worth a Javadoc note documenting the current assumption for finance.

⚠️ `amountPaid` here is the configured `letterPrice`, not an amount verified as received from a payment provider. Fine for Phase 0 honor-system, but once Stripe/Swish webhooks land this should come from the captured payment-intent amount (handles partial payments / refunds / price drift). Worth a Javadoc note documenting the current assumption for finance.
backend/src/test/java/se/bilhalsning/service/OrderServiceTest.java
hermes commented 2026-07-18 11:41:18 +00:00
Author
Collaborator
Copy link

🧪 This assertion passes via the = 49 field initializer, not via @Value: @InjectMocks doesn't resolve Spring @Value, so this test cannot catch a misnamed app.payment.letter-price PropertySource or a removed @Value. The guest-path twin at line 350 has the same gap. Strengthen with ReflectionTestUtils.setField(orderService, "letterPrice", 99) + assert 99, or a @SpringBootTest slice that asserts the resolved config differs from a sentinel.

🧪 This assertion passes via the `= 49` field initializer, not via `@Value`: `@InjectMocks` doesn't resolve Spring `@Value`, so this test cannot catch a misnamed `app.payment.letter-price` PropertySource or a removed `@Value`. The guest-path twin at line 350 has the same gap. Strengthen with `ReflectionTestUtils.setField(orderService, "letterPrice", 99)` + assert `99`, or a `@SpringBootTest` slice that asserts the resolved config differs from a sentinel.
Some checks failed
CI / Lint, type check, unit tests, coverage (pull_request) Successful in 3m26s
Details
CI / E2E browser tests (pull_request) Failing after 1m53s
Details
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix/persist-amount-paid:fix/persist-amount-paid
git checkout fix/persist-amount-paid

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git checkout master
git merge --no-ff fix/persist-amount-paid
git checkout fix/persist-amount-paid
git rebase master
git checkout master
git merge --ff-only fix/persist-amount-paid
git checkout fix/persist-amount-paid
git rebase master
git checkout master
git merge --no-ff fix/persist-amount-paid
git checkout master
git merge --squash fix/persist-amount-paid
git checkout master
git merge --ff-only fix/persist-amount-paid
git checkout master
git merge fix/persist-amount-paid
git push origin master
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: jocke/bilhej#27
No description provided.
Delete branch "fix/persist-amount-paid"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?