Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allowPartialPeriodInterestCalculation #2217

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rhopman
Copy link
Contributor

@rhopman rhopman commented Dec 14, 2024

Description

Due to a typo, the allowPartialPeriodInterestCalculation field was not processed correctly.

Checklist

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

  • If you have multiple commits please combine them into one commit by squashing them.

  • Read and understood the contribution guidelines at web-app/.github/CONTRIBUTING.md.

@adamsaghy
Copy link
Collaborator

@rhopman Kindly rebase this PR.

@rhopman rhopman force-pushed the fix-allowPartialPeriodInterestCalculation branch 2 times, most recently from ab27409 to 295c474 Compare December 19, 2024 09:25
Copy link
Collaborator

@adamsaghy adamsaghy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix requires changes at the backend side as well, as this field has the typo there as well!

@rhopman
Copy link
Contributor Author

rhopman commented Dec 19, 2024

@adamsaghy good catch. I thought I checked for that but apparently I made a mistake. Let me look into this. The backend change would be breaking, so that would not be an easy fix.

@adamsaghy
Copy link
Collaborator

@adamsaghy good catch. I thought I checked for that but apparently I made a mistake. Let me look into this. The backend change would be breaking, so that would not be an easy fix.

Yes, probably it would be recommended to announce in the Fineract DEV mail list this change!

Due to a typo, the allowPartialPeriodInterestCalculation field was not
processed correctly. Since the typo also occurs in some places in the
Fineract API, the typo is re-introduced where needed.
@rhopman rhopman force-pushed the fix-allowPartialPeriodInterestCalculation branch from 295c474 to 85d5c1b Compare December 19, 2024 19:26
@rhopman rhopman requested a review from adamsaghy December 19, 2024 19:31
@rhopman
Copy link
Contributor Author

rhopman commented Dec 19, 2024

I did a bit more research. Although the typo is indeed also present in the Fineract backend, it doesn't appear everywhere in the API. I checked the following relevant API endpoints:

  • GET /v1/loanproducts: no typo
  • POST /v1/loanproducts: typo
  • PUT /v1/loanproducts/{productId}: typo
  • GET /v1/loanproducts/{productId}: no typo
  • POST /v1/loans: typo
  • PUT /v1/loans/{loanId}: typo
  • GET /v1/loans/{loanId}: no typo

I think the cleanest solution is to fix the typo throughout the frontend, and then make small adjustments (with comments) whenever the typo needs to be re-introduced before sending it to the backend.

@adamsaghy please review again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants