FINERACT-2455: WC - Loan Details API - Expose Payment date / Payment amount

[somasorosdpc]

Description

Loan Details API - Expose

• Payment date / Payment amount
• Repayment date / Repayment amount

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Write the commit message as per our guidelines
• Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
• Create/update unit or integration tests for verifying the changes made.
• Follow our coding conventions.
• Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
• This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.
• If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.

Your assigned reviewer(s) will follow our guidelines for code reviews.

[galovics]

The API change itself is purely additive so nothing breaking there. Two things worth fixing on the substance though.

GOODWILL_CREDIT is missing from the "payment" transaction type list, even though the WC module treats it as payment-like everywhere else - WorkingCapitalLoanWritePlatformServiceImpl handles it alongside REPAYMENT/PAYOUT_REFUND/CHARGE_ADJUSTMENT for undo and balance purposes, and WorkingCapitalLoanBusinessEventSerializer already includes it in its own PAYMENT_TRANSACTION_TYPES constant. For a loan whose most recent balance-reducing transaction is a goodwill credit, lastPaymentDate/lastPaymentAmount will silently report the older repayment instead - which contradicts the Swagger text ("last paid by any repayment like transaction").

That points at the real issue: this computation is a second, independent implementation of something that already exists.WorkingCapitalLoanBusinessEventSerializer.populateLastTransactions already computes the same four values for the external event payload, using LoanTransactionType.getRepaymentLikeTransactionTypes() (8 types, including goodwill credit) rather than this PR's hardcoded 3-type list. Once this ships, the API and the business event will report different lastPaymentDate for the same loan - a real data-consistency problem for anyone reconciling the two. Worth making this new service the single source of truth (or at least sharing the type list/constant) and having the serializer call into it instead of maintaining a parallel computation.

Smaller things: LastPaymentDetail (new) duplicates the shape of the existing TransactionDateAndAmountHolder record in the same package - worth reusing that instead. And LastPaymentDetail vs LastPaymentDetails differing by one character in the same package is a readability trap that already shows up at the call site (lastRepaymentDetail.getLastPaymentDate() reads like a bug even though it isn't one).

I know it's a draft, but since the value of this PR is entirely in "which transaction types count and which wins on a tie", I'd want a unit test on retrieveLastPaymentDetails covering the goodwill-credit case, reversed transactions being excluded, and the same-date tie-break before this comes out of draft.

Recommendation: CHANGES_REQUESTED

[adamsaghy]

LGTM

[adamsaghy]

LGTM

All concerns were addressed.

[somasorosdpc]

Description

Loan Details API - Expose

• Payment date / Payment amount
• Repayment date / Repayment amount

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Write the commit message as per our guidelines
• Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
• Create/update unit or integration tests for verifying the changes made.
• Follow our coding conventions.
• Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
• This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.
• If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.

Your assigned reviewer(s) will follow our guidelines for code reviews.

[galovics]

The API change itself is purely additive so nothing breaking there. Two things worth fixing on the substance though.

GOODWILL_CREDIT is missing from the "payment" transaction type list, even though the WC module treats it as payment-like everywhere else - WorkingCapitalLoanWritePlatformServiceImpl handles it alongside REPAYMENT/PAYOUT_REFUND/CHARGE_ADJUSTMENT for undo and balance purposes, and WorkingCapitalLoanBusinessEventSerializer already includes it in its own PAYMENT_TRANSACTION_TYPES constant. For a loan whose most recent balance-reducing transaction is a goodwill credit, lastPaymentDate/lastPaymentAmount will silently report the older repayment instead - which contradicts the Swagger text ("last paid by any repayment like transaction").

That points at the real issue: this computation is a second, independent implementation of something that already exists.WorkingCapitalLoanBusinessEventSerializer.populateLastTransactions already computes the same four values for the external event payload, using LoanTransactionType.getRepaymentLikeTransactionTypes() (8 types, including goodwill credit) rather than this PR's hardcoded 3-type list. Once this ships, the API and the business event will report different lastPaymentDate for the same loan - a real data-consistency problem for anyone reconciling the two. Worth making this new service the single source of truth (or at least sharing the type list/constant) and having the serializer call into it instead of maintaining a parallel computation.

Smaller things: LastPaymentDetail (new) duplicates the shape of the existing TransactionDateAndAmountHolder record in the same package - worth reusing that instead. And LastPaymentDetail vs LastPaymentDetails differing by one character in the same package is a readability trap that already shows up at the call site (lastRepaymentDetail.getLastPaymentDate() reads like a bug even though it isn't one).

I know it's a draft, but since the value of this PR is entirely in "which transaction types count and which wins on a tie", I'd want a unit test on retrieveLastPaymentDetails covering the goodwill-credit case, reversed transactions being excluded, and the same-date tie-break before this comes out of draft.

Recommendation: CHANGES_REQUESTED

[adamsaghy]

LGTM

[adamsaghy]

LGTM

All concerns were addressed.

[somasorosdpc]

Description

Loan Details API - Expose

• Payment date / Payment amount
• Repayment date / Repayment amount

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Write the commit message as per our guidelines
• Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
• Create/update unit or integration tests for verifying the changes made.
• Follow our coding conventions.
• Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
• This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.
• If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.

Your assigned reviewer(s) will follow our guidelines for code reviews.

[galovics]

The API change itself is purely additive so nothing breaking there. Two things worth fixing on the substance though.

GOODWILL_CREDIT is missing from the "payment" transaction type list, even though the WC module treats it as payment-like everywhere else - WorkingCapitalLoanWritePlatformServiceImpl handles it alongside REPAYMENT/PAYOUT_REFUND/CHARGE_ADJUSTMENT for undo and balance purposes, and WorkingCapitalLoanBusinessEventSerializer already includes it in its own PAYMENT_TRANSACTION_TYPES constant. For a loan whose most recent balance-reducing transaction is a goodwill credit, lastPaymentDate/lastPaymentAmount will silently report the older repayment instead - which contradicts the Swagger text ("last paid by any repayment like transaction").

That points at the real issue: this computation is a second, independent implementation of something that already exists.WorkingCapitalLoanBusinessEventSerializer.populateLastTransactions already computes the same four values for the external event payload, using LoanTransactionType.getRepaymentLikeTransactionTypes() (8 types, including goodwill credit) rather than this PR's hardcoded 3-type list. Once this ships, the API and the business event will report different lastPaymentDate for the same loan - a real data-consistency problem for anyone reconciling the two. Worth making this new service the single source of truth (or at least sharing the type list/constant) and having the serializer call into it instead of maintaining a parallel computation.

Smaller things: LastPaymentDetail (new) duplicates the shape of the existing TransactionDateAndAmountHolder record in the same package - worth reusing that instead. And LastPaymentDetail vs LastPaymentDetails differing by one character in the same package is a readability trap that already shows up at the call site (lastRepaymentDetail.getLastPaymentDate() reads like a bug even though it isn't one).

I know it's a draft, but since the value of this PR is entirely in "which transaction types count and which wins on a tie", I'd want a unit test on retrieveLastPaymentDetails covering the goodwill-credit case, reversed transactions being excluded, and the same-date tie-break before this comes out of draft.

Recommendation: CHANGES_REQUESTED

[adamsaghy]

LGTM

[adamsaghy]

LGTM

All concerns were addressed.

[somasorosdpc]

Description

Loan Details API - Expose

• Payment date / Payment amount
• Repayment date / Repayment amount

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Write the commit message as per our guidelines
• Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
• Create/update unit or integration tests for verifying the changes made.
• Follow our coding conventions.
• Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
• This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.
• If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.

Your assigned reviewer(s) will follow our guidelines for code reviews.

[galovics]

The API change itself is purely additive so nothing breaking there. Two things worth fixing on the substance though.

GOODWILL_CREDIT is missing from the "payment" transaction type list, even though the WC module treats it as payment-like everywhere else - WorkingCapitalLoanWritePlatformServiceImpl handles it alongside REPAYMENT/PAYOUT_REFUND/CHARGE_ADJUSTMENT for undo and balance purposes, and WorkingCapitalLoanBusinessEventSerializer already includes it in its own PAYMENT_TRANSACTION_TYPES constant. For a loan whose most recent balance-reducing transaction is a goodwill credit, lastPaymentDate/lastPaymentAmount will silently report the older repayment instead - which contradicts the Swagger text ("last paid by any repayment like transaction").

That points at the real issue: this computation is a second, independent implementation of something that already exists.WorkingCapitalLoanBusinessEventSerializer.populateLastTransactions already computes the same four values for the external event payload, using LoanTransactionType.getRepaymentLikeTransactionTypes() (8 types, including goodwill credit) rather than this PR's hardcoded 3-type list. Once this ships, the API and the business event will report different lastPaymentDate for the same loan - a real data-consistency problem for anyone reconciling the two. Worth making this new service the single source of truth (or at least sharing the type list/constant) and having the serializer call into it instead of maintaining a parallel computation.

Smaller things: LastPaymentDetail (new) duplicates the shape of the existing TransactionDateAndAmountHolder record in the same package - worth reusing that instead. And LastPaymentDetail vs LastPaymentDetails differing by one character in the same package is a readability trap that already shows up at the call site (lastRepaymentDetail.getLastPaymentDate() reads like a bug even though it isn't one).

I know it's a draft, but since the value of this PR is entirely in "which transaction types count and which wins on a tie", I'd want a unit test on retrieveLastPaymentDetails covering the goodwill-credit case, reversed transactions being excluded, and the same-date tie-break before this comes out of draft.

Recommendation: CHANGES_REQUESTED

[adamsaghy]

LGTM

[adamsaghy]

LGTM

All concerns were addressed.

[somasorosdpc]

Description

Loan Details API - Expose

• Payment date / Payment amount
• Repayment date / Repayment amount

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Write the commit message as per our guidelines
• Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
• Create/update unit or integration tests for verifying the changes made.
• Follow our coding conventions.
• Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
• This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.
• If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.

Your assigned reviewer(s) will follow our guidelines for code reviews.

[galovics]

The API change itself is purely additive so nothing breaking there. Two things worth fixing on the substance though.

GOODWILL_CREDIT is missing from the "payment" transaction type list, even though the WC module treats it as payment-like everywhere else - WorkingCapitalLoanWritePlatformServiceImpl handles it alongside REPAYMENT/PAYOUT_REFUND/CHARGE_ADJUSTMENT for undo and balance purposes, and WorkingCapitalLoanBusinessEventSerializer already includes it in its own PAYMENT_TRANSACTION_TYPES constant. For a loan whose most recent balance-reducing transaction is a goodwill credit, lastPaymentDate/lastPaymentAmount will silently report the older repayment instead - which contradicts the Swagger text ("last paid by any repayment like transaction").

That points at the real issue: this computation is a second, independent implementation of something that already exists.WorkingCapitalLoanBusinessEventSerializer.populateLastTransactions already computes the same four values for the external event payload, using LoanTransactionType.getRepaymentLikeTransactionTypes() (8 types, including goodwill credit) rather than this PR's hardcoded 3-type list. Once this ships, the API and the business event will report different lastPaymentDate for the same loan - a real data-consistency problem for anyone reconciling the two. Worth making this new service the single source of truth (or at least sharing the type list/constant) and having the serializer call into it instead of maintaining a parallel computation.

Smaller things: LastPaymentDetail (new) duplicates the shape of the existing TransactionDateAndAmountHolder record in the same package - worth reusing that instead. And LastPaymentDetail vs LastPaymentDetails differing by one character in the same package is a readability trap that already shows up at the call site (lastRepaymentDetail.getLastPaymentDate() reads like a bug even though it isn't one).

I know it's a draft, but since the value of this PR is entirely in "which transaction types count and which wins on a tie", I'd want a unit test on retrieveLastPaymentDetails covering the goodwill-credit case, reversed transactions being excluded, and the same-date tie-break before this comes out of draft.

Recommendation: CHANGES_REQUESTED

[adamsaghy]

LGTM

[adamsaghy]

LGTM

All concerns were addressed.

[somasorosdpc]

Description

Loan Details API - Expose

• Payment date / Payment amount
• Repayment date / Repayment amount

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Write the commit message as per our guidelines
• Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
• Create/update unit or integration tests for verifying the changes made.
• Follow our coding conventions.
• Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
• This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.
• If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.

Your assigned reviewer(s) will follow our guidelines for code reviews.

[galovics]

The API change itself is purely additive so nothing breaking there. Two things worth fixing on the substance though.

GOODWILL_CREDIT is missing from the "payment" transaction type list, even though the WC module treats it as payment-like everywhere else - WorkingCapitalLoanWritePlatformServiceImpl handles it alongside REPAYMENT/PAYOUT_REFUND/CHARGE_ADJUSTMENT for undo and balance purposes, and WorkingCapitalLoanBusinessEventSerializer already includes it in its own PAYMENT_TRANSACTION_TYPES constant. For a loan whose most recent balance-reducing transaction is a goodwill credit, lastPaymentDate/lastPaymentAmount will silently report the older repayment instead - which contradicts the Swagger text ("last paid by any repayment like transaction").

That points at the real issue: this computation is a second, independent implementation of something that already exists.WorkingCapitalLoanBusinessEventSerializer.populateLastTransactions already computes the same four values for the external event payload, using LoanTransactionType.getRepaymentLikeTransactionTypes() (8 types, including goodwill credit) rather than this PR's hardcoded 3-type list. Once this ships, the API and the business event will report different lastPaymentDate for the same loan - a real data-consistency problem for anyone reconciling the two. Worth making this new service the single source of truth (or at least sharing the type list/constant) and having the serializer call into it instead of maintaining a parallel computation.

Smaller things: LastPaymentDetail (new) duplicates the shape of the existing TransactionDateAndAmountHolder record in the same package - worth reusing that instead. And LastPaymentDetail vs LastPaymentDetails differing by one character in the same package is a readability trap that already shows up at the call site (lastRepaymentDetail.getLastPaymentDate() reads like a bug even though it isn't one).

I know it's a draft, but since the value of this PR is entirely in "which transaction types count and which wins on a tie", I'd want a unit test on retrieveLastPaymentDetails covering the goodwill-credit case, reversed transactions being excluded, and the same-date tie-break before this comes out of draft.

Recommendation: CHANGES_REQUESTED

[adamsaghy]

LGTM

[adamsaghy]

LGTM

All concerns were addressed.

[somasorosdpc]

Description

Loan Details API - Expose

• Payment date / Payment amount
• Repayment date / Repayment amount

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Write the commit message as per our guidelines
• Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
• Create/update unit or integration tests for verifying the changes made.
• Follow our coding conventions.
• Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
• This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.
• If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.

Your assigned reviewer(s) will follow our guidelines for code reviews.

[galovics]

The API change itself is purely additive so nothing breaking there. Two things worth fixing on the substance though.

GOODWILL_CREDIT is missing from the "payment" transaction type list, even though the WC module treats it as payment-like everywhere else - WorkingCapitalLoanWritePlatformServiceImpl handles it alongside REPAYMENT/PAYOUT_REFUND/CHARGE_ADJUSTMENT for undo and balance purposes, and WorkingCapitalLoanBusinessEventSerializer already includes it in its own PAYMENT_TRANSACTION_TYPES constant. For a loan whose most recent balance-reducing transaction is a goodwill credit, lastPaymentDate/lastPaymentAmount will silently report the older repayment instead - which contradicts the Swagger text ("last paid by any repayment like transaction").

That points at the real issue: this computation is a second, independent implementation of something that already exists.WorkingCapitalLoanBusinessEventSerializer.populateLastTransactions already computes the same four values for the external event payload, using LoanTransactionType.getRepaymentLikeTransactionTypes() (8 types, including goodwill credit) rather than this PR's hardcoded 3-type list. Once this ships, the API and the business event will report different lastPaymentDate for the same loan - a real data-consistency problem for anyone reconciling the two. Worth making this new service the single source of truth (or at least sharing the type list/constant) and having the serializer call into it instead of maintaining a parallel computation.

Smaller things: LastPaymentDetail (new) duplicates the shape of the existing TransactionDateAndAmountHolder record in the same package - worth reusing that instead. And LastPaymentDetail vs LastPaymentDetails differing by one character in the same package is a readability trap that already shows up at the call site (lastRepaymentDetail.getLastPaymentDate() reads like a bug even though it isn't one).

I know it's a draft, but since the value of this PR is entirely in "which transaction types count and which wins on a tie", I'd want a unit test on retrieveLastPaymentDetails covering the goodwill-credit case, reversed transactions being excluded, and the same-date tie-break before this comes out of draft.

Recommendation: CHANGES_REQUESTED

[adamsaghy]

LGTM

[adamsaghy]

LGTM

All concerns were addressed.

[somasorosdpc]

Description

Loan Details API - Expose

• Payment date / Payment amount
• Repayment date / Repayment amount

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Write the commit message as per our guidelines
• Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
• Create/update unit or integration tests for verifying the changes made.
• Follow our coding conventions.
• Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
• This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.
• If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.

Your assigned reviewer(s) will follow our guidelines for code reviews.

[galovics]

The API change itself is purely additive so nothing breaking there. Two things worth fixing on the substance though.

GOODWILL_CREDIT is missing from the "payment" transaction type list, even though the WC module treats it as payment-like everywhere else - WorkingCapitalLoanWritePlatformServiceImpl handles it alongside REPAYMENT/PAYOUT_REFUND/CHARGE_ADJUSTMENT for undo and balance purposes, and WorkingCapitalLoanBusinessEventSerializer already includes it in its own PAYMENT_TRANSACTION_TYPES constant. For a loan whose most recent balance-reducing transaction is a goodwill credit, lastPaymentDate/lastPaymentAmount will silently report the older repayment instead - which contradicts the Swagger text ("last paid by any repayment like transaction").

That points at the real issue: this computation is a second, independent implementation of something that already exists.WorkingCapitalLoanBusinessEventSerializer.populateLastTransactions already computes the same four values for the external event payload, using LoanTransactionType.getRepaymentLikeTransactionTypes() (8 types, including goodwill credit) rather than this PR's hardcoded 3-type list. Once this ships, the API and the business event will report different lastPaymentDate for the same loan - a real data-consistency problem for anyone reconciling the two. Worth making this new service the single source of truth (or at least sharing the type list/constant) and having the serializer call into it instead of maintaining a parallel computation.

Smaller things: LastPaymentDetail (new) duplicates the shape of the existing TransactionDateAndAmountHolder record in the same package - worth reusing that instead. And LastPaymentDetail vs LastPaymentDetails differing by one character in the same package is a readability trap that already shows up at the call site (lastRepaymentDetail.getLastPaymentDate() reads like a bug even though it isn't one).

I know it's a draft, but since the value of this PR is entirely in "which transaction types count and which wins on a tie", I'd want a unit test on retrieveLastPaymentDetails covering the goodwill-credit case, reversed transactions being excluded, and the same-date tie-break before this comes out of draft.

Recommendation: CHANGES_REQUESTED

[adamsaghy]

LGTM

[adamsaghy]

LGTM

All concerns were addressed.