mariiaKraievska commented on PR #6673:
URL: https://github.com/apache/fineract/pull/6673#issuecomment-6263827885
> ## Review
> Small one, looks fine. Only a naming question on the new field.
>
>
`fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/data/WorkingCapitalLoanData.java`:
>
> > Minor thing: everything else client-related here is prefixed
(`clientId`, `clientName`, `clientOfficeId`), and this is the client's office
too (`client.office.name`). Can we call it `clientOfficeName`? Once it's in the
API it's hard to rename. Unless the ticket asks for `officeName` specifically,
then ignore me.
>
>
`fineract-working-capital-loan/src/test/java/.../serialization/mapper/WorkingCapitalLoanAccountDataMapperTest.java`:
>
> > The `.officeName("Head Office")` line in `fullLoanData()` doesn't verify
anything. That fixture feeds the Avro `WorkingCapitalLoanAccountDataV1` mapper,
which has `clientOfficeId` but no office name, so the value is unused test
data. Drop it, or add a test on `WorkingCapitalLoanMapper` mapping
`client.office.name` (right now only the e2e row covers it). Also, should the
business event payload carry the office name too, for parity with REST?
>
> Pls fill in the PR description before taking it out of draft.
>
> Recommendation: COMMENT
Thank you!
Renamed to clientOfficeName. Dropped the unused value from the Avro fixture
— e2e covers the REST mapping. Not adding the office name to the business-event
payload: term loan also only exposes clientOfficeId. PR description updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
