You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@fineract.apache.org by "ruchiD (via GitHub)" <gi...@apache.org> on 2023/11/21 12:22:36 UTC

[PR] FINERACT-1992: Multi delinquency implication on events [fineract]

ruchiD opened a new pull request, #3595:
URL: https://github.com/apache/fineract/pull/3595

## Description
Event payload changes to include installment level delinquency data for LoanDelinquencyRangeChangeEvent.

Describe the changes made and why they were made.

Ignore if these details are present on the associated [Apache Fineract JIRA ticket](https://github.com/apache/fineract/pull/1284).

## Checklist

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

- [ ] Write the commit message as per https://github.com/apache/fineract/#pull-requests

- [ ] 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 coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/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

- [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)

FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.

--
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: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-1992: Multi delinquency implication on events [fineract]

Posted by "reluxa (via GitHub)" <gi...@apache.org>.

reluxa commented on code in PR #3595:
URL: https://github.com/apache/fineract/pull/3595#discussion_r1403075538


##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/event/external/service/serialization/serializer/loan/LoanDelinquencyRangeChangeBusinessEventSerializer.java:
##########
@@ -104,7 +116,79 @@ public <T> ByteBufferSerializable toAvroDTO(BusinessEvent<T> rawEvent) {
                 .setAmount(amount)//
                 .setCurrency(currencyMapper.map(data.getCurrency()))//
                 .setDelinquentDate(delinquentDate)//
-                .build();
+                .setInstallmentDelinquencyBuckets(installmentsDelinquencyData).build();

Review Comment:
   This should be calculated and set only when the installment level delinquency is enabled on the loan.
   
   https://github.com/apache/fineract/blob/f089f011168e09abeedc53313bde2b24c4239da2/fineract-loan/src/main/java/org/apache/fineract/portfolio/loanproduct/domain/LoanProduct.java#L213



-- 
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: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-1992: Multi delinquency implication on events [fineract]

Posted by "ruchiD (via GitHub)" <gi...@apache.org>.

ruchiD commented on code in PR #3595:
URL: https://github.com/apache/fineract/pull/3595#discussion_r1402193532


##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/event/external/service/serialization/serializer/loan/LoanDelinquencyRangeChangeBusinessEventSerializer.java:
##########
@@ -104,7 +116,80 @@ public <T> ByteBufferSerializable toAvroDTO(BusinessEvent<T> rawEvent) {
                 .setAmount(amount)//
                 .setCurrency(currencyMapper.map(data.getCurrency()))//
                 .setDelinquentDate(delinquentDate)//
-                .build();
+                .setInstallmentDelinquencyBuckets(installmentsDelinquencyData)//

Review Comment:
   done



-- 
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: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-1992: Multi delinquency implication on events [fineract]

Posted by "ruchiD (via GitHub)" <gi...@apache.org>.

ruchiD commented on code in PR #3595:
URL: https://github.com/apache/fineract/pull/3595#discussion_r1402194130


##########
fineract-avro-schemas/src/main/avro/loan/v1/LoanInstallmentDelinquencyBucketDataV1.avsc:
##########
@@ -0,0 +1,31 @@
+{
+    "name": "LoanInstallmentDelinquencyBucketDataV1",
+    "namespace": "org.apache.fineract.avro.loan.v1",
+    "type": "record",
+    "fields": [
+        {
+            "default": null,
+            "name": "delinquencyRange",
+            "type": [
+                "null",
+                "org.apache.fineract.avro.loan.v1.DelinquencyRangeDataV1"

Review Comment:
   done



-- 
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: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-1992: Multi delinquency implication on events [fineract]

Posted by "reluxa (via GitHub)" <gi...@apache.org>.

reluxa commented on PR #3595:
URL: https://github.com/apache/fineract/pull/3595#issuecomment-1824051736

   LGTM


-- 
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: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-1992: Multi delinquency implication on events [fineract]

Posted by "reluxa (via GitHub)" <gi...@apache.org>.

reluxa commented on PR #3595:
URL: https://github.com/apache/fineract/pull/3595#issuecomment-1822350257

   Have you covered the below scenarios as well? 
   
   - When loan goes into pause delinquency started (consider the design for other programs like re-age, payment holiday starts)
   - When loan comes out of pause delinquency ended (consider the design for other programs like re-age, payment holiday ends)
   
   According to the delinquency event shall be emitted in this case too.


-- 
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: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-1992: Multi delinquency implication on events [fineract]

Posted by "ruchiD (via GitHub)" <gi...@apache.org>.

ruchiD commented on code in PR #3595:
URL: https://github.com/apache/fineract/pull/3595#discussion_r1403077893


##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/event/external/service/serialization/serializer/loan/LoanDelinquencyRangeChangeBusinessEventSerializer.java:
##########
@@ -104,7 +116,79 @@ public <T> ByteBufferSerializable toAvroDTO(BusinessEvent<T> rawEvent) {
                 .setAmount(amount)//
                 .setCurrency(currencyMapper.map(data.getCurrency()))//
                 .setDelinquentDate(delinquentDate)//
-                .build();
+                .setInstallmentDelinquencyBuckets(installmentsDelinquencyData).build();

Review Comment:
   @reluxa , This check is there in calculateInstallmentLevelDelinquencyData method.
   
    private List<LoanInstallmentDelinquencyBucketDataV1> calculateInstallmentLevelDelinquencyData(Loan loan, LoanAccountData data) {
           List<LoanInstallmentDelinquencyBucketDataV1> loanInstallmentDelinquencyData = new ArrayList<>();
           if (loan.isEnableInstallmentLevelDelinquency()) {



-- 
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: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-1992: Multi delinquency implication on events [fineract]

Posted by "ruchiD (via GitHub)" <gi...@apache.org>.

ruchiD commented on code in PR #3595:
URL: https://github.com/apache/fineract/pull/3595#discussion_r1403078694


##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/event/external/service/serialization/serializer/loan/LoanDelinquencyRangeChangeBusinessEventSerializer.java:
##########
@@ -104,7 +116,79 @@ public <T> ByteBufferSerializable toAvroDTO(BusinessEvent<T> rawEvent) {
                 .setAmount(amount)//
                 .setCurrency(currencyMapper.map(data.getCurrency()))//
                 .setDelinquentDate(delinquentDate)//
-                .build();
+                .setInstallmentDelinquencyBuckets(installmentsDelinquencyData).build();

Review Comment:
   okay



-- 
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: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-1992: Multi delinquency implication on events [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.

adamsaghy merged PR #3595:
URL: https://github.com/apache/fineract/pull/3595


-- 
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: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-1992: Multi delinquency implication on events [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.

adamsaghy commented on PR #3595:
URL: https://github.com/apache/fineract/pull/3595#issuecomment-1821006341

   @reluxa FYI


-- 
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: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-1992: Multi delinquency implication on events [fineract]

Posted by "reluxa (via GitHub)" <gi...@apache.org>.

reluxa commented on code in PR #3595:
URL: https://github.com/apache/fineract/pull/3595#discussion_r1401702511


##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/event/external/service/serialization/serializer/loan/LoanDelinquencyRangeChangeBusinessEventSerializer.java:
##########
@@ -104,7 +116,80 @@ public <T> ByteBufferSerializable toAvroDTO(BusinessEvent<T> rawEvent) {
                 .setAmount(amount)//
                 .setCurrency(currencyMapper.map(data.getCurrency()))//
                 .setDelinquentDate(delinquentDate)//
-                .build();
+                .setInstallmentDelinquencyBuckets(installmentsDelinquencyData)//

Review Comment:
   I think it might make sense to cover the logic which assembles installmentsDelinquencyData with a few unit tests.



-- 
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: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-1992: Multi delinquency implication on events [fineract]

Posted by "reluxa (via GitHub)" <gi...@apache.org>.

reluxa commented on code in PR #3595:
URL: https://github.com/apache/fineract/pull/3595#discussion_r1403076392


##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/event/external/service/serialization/serializer/loan/LoanDelinquencyRangeChangeBusinessEventSerializer.java:
##########
@@ -104,7 +116,79 @@ public <T> ByteBufferSerializable toAvroDTO(BusinessEvent<T> rawEvent) {
                 .setAmount(amount)//
                 .setCurrency(currencyMapper.map(data.getCurrency()))//
                 .setDelinquentDate(delinquentDate)//
-                .build();
+                .setInstallmentDelinquencyBuckets(installmentsDelinquencyData).build();

Review Comment:
   Ah, please ignore, I can see that you are checking it in line 124



-- 
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: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-1992: Multi delinquency implication on events [fineract]

Posted by "reluxa (via GitHub)" <gi...@apache.org>.

reluxa commented on code in PR #3595:
URL: https://github.com/apache/fineract/pull/3595#discussion_r1401712809


##########
fineract-avro-schemas/src/main/avro/loan/v1/LoanInstallmentDelinquencyBucketDataV1.avsc:
##########
@@ -0,0 +1,31 @@
+{
+    "name": "LoanInstallmentDelinquencyBucketDataV1",
+    "namespace": "org.apache.fineract.avro.loan.v1",
+    "type": "record",
+    "fields": [
+        {
+            "default": null,
+            "name": "delinquencyRange",
+            "type": [
+                "null",
+                "org.apache.fineract.avro.loan.v1.DelinquencyRangeDataV1"

Review Comment:
   According to the design the fields here should have been moved up, like `installmentDelinquencyBuckets[n].minimumAgeDays` and yours is `installmentDelinquencyBuckets[n].delinquencyRange.minimumAgeDays` I think it's perfectly fine, but please update the documentation to make it inline with the implementation.



-- 
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: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org