(feat): Payments should be associated with a cashier#155
(feat): Payments should be associated with a cashier#155NethmiRodrigo wants to merge 10 commits intomainfrom
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…REST representation
…ate auto-flush Swapped order so validateNewPaymentsHaveCashier runs before validateLineItemsNotModified. The latter triggers a DB query that causes Hibernate to auto-flush transient payments, assigning them IDs and making the id==null check ineffective. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #155 +/- ##
============================================
+ Coverage 26.73% 27.00% +0.27%
- Complexity 487 497 +10
============================================
Files 190 190
Lines 4309 4336 +27
Branches 486 492 +6
============================================
+ Hits 1152 1171 +19
- Misses 3056 3063 +7
- Partials 101 102 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
One thing we need to add is the ability for the client to submit the cashier in case it’s not the current user. It’s fine to use this as a fallback / default though. |
Done! |
| <comment>Add cashier (provider) for each payment to track which cashier processed the payment</comment> | ||
| <addColumn tableName="cashier_bill_payment"> | ||
| <column name="provider_id" type="int"/> | ||
| <constraints nullable="true"/> |
There was a problem hiding this comment.
Constraint should be inside the column tag
<column name="provider_id" type="int">
<constraints nullable="true"/>
</column>
There was a problem hiding this comment.
@claude[agent] do this change in this PR
There was a problem hiding this comment.
Nvm my session credits are over sigh
| BaseRestDataResource.syncCollection(instance.getPayments(), payments); | ||
| for (Payment payment : instance.getPayments()) { | ||
| if (payment.getId() == null && payment.getCashier() == null) { | ||
| Provider cashier = getCurrentCashier(); |
There was a problem hiding this comment.
Can't we use ProviderUtil.getCurrentProvider() here?
There was a problem hiding this comment.
getCurrentCashier() method is the same logic:
- both use Context.getAuthenticatedUser().getPerson()
- both call ProviderService.getProvidersByPerson(...)
- both return first provider or null
There was a problem hiding this comment.
Yeah, why are we duplicating existing functionalities? :)
| // attempting to modify line items | ||
| assertFalse(errors.hasErrors()); | ||
| } | ||
|
|
There was a problem hiding this comment.
It would be great to have some actual tests here :)
There was a problem hiding this comment.
Don’t the tests in BillServiceImplTest.java make sure that the validator works too?
There was a problem hiding this comment.
Yeah but having some seperate tests for the validator is convenient.
There was a problem hiding this comment.
Convenient, how? That would be redundant. Since the validator is already getting tested, having this just means adding another test that tests the same thing. It already takes a while to run tests, why add more if not needed
There was a problem hiding this comment.
By that logic, we shouldn't write tests for the DAO layer since the service layer already covers it. The point of unit tests is to test a unit in isolation, so failures are obvious and edge cases are cheap to cover.
There was a problem hiding this comment.
@wikumChamith I moved the tests to the validator :)
…t edge case Relocates payment-cashier and line-item validation tests from BillServiceImplTest to BillValidatorTest so each test class covers its own layer. Adds validate_shouldTolerateVoidedNewPaymentWithNoCashier to cover the voided-payment branch in BillValidator. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…equire cashier in payment test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4 participants
This PR adds back the functionality that was introduced in this PR, that we had to revert because it was merged in during the release code freeze. This basically does the same except for getting the cashier from the authenticated user, while previously it needed to be sent from the client.
(sub-resource endpoint) and BillResource (embedded payments on bill create/update)
(legacy data) are allowed