FINERACT-1289: Taxes on Loan charges by alberto-art3ch · Pull Request #5780 · apache/fineract

alberto-art3ch · 2026-04-19T18:12:53Z

Description

There was an issue with the Tax attached to the loan charge is not getting affected at the loan/accounting level. Tthe tax bifurcation was not happening

FINERACT-1289

Loan Charges

Tax Component

Charge Definition linked with the Tax Group

Accrual Loan Transaction for paying the Loan Cahrge

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.

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

Aman-Mittal · 2026-04-20T12:16:09Z

@alberto-art3ch It seems GA check is giving error prone issue on fineract-tax:compileTestJava can you please check.

While i tried to look at the ticket. Can you confirm me the tax bifurcation logic. Asking based on usecase perspective while on ticket i tried to cross-check I cannot seem to understand how tax logic should be calculated not much relevant data i can find.

adamsaghy · 2026-04-20T13:16:03Z

/home/runner/work/fineract/fineract/fineract-tax/src/test/java/org/apache/fineract/portfolio/tax/service/ChargeTaxApplicationServiceTest.java:66: error: [JavaTimeDefaultTimeZone] LocalDate.now() is not allowed because it silently uses the system default time-zone. You must pass an explicit time-zone (e.g., ZoneId.of("America/Los_Angeles")) to this method.
> Task :fineract-tax:compileTestJava FAILED
        Map<TaxComponent, BigDecimal> result = service.computeTax(null, new BigDecimal("100.00"), LocalDate.now(), 6);

                                                                                                               ^
> Task :fineract-loan:compileJava
    (see https://errorprone.info/bugpattern/JavaTimeDefaultTimeZone)
  Did you mean 'Map<TaxComponent, BigDecimal> result = service.computeTax(null, new BigDecimal("100.00"), LocalDate.now(ZoneId.systemDefault()), 6);'?
/home/runner/work/fineract/fineract/fineract-tax/src/test/java/org/apache/fineract/portfolio/tax/service/ChargeTaxApplicationServiceTest.java:75: error: [JavaTimeDefaultTimeZone] LocalDate.now() is not allowed because it silently uses the system default time-zone. You must pass an explicit time-zone (e.g., ZoneId.of("America/Los_Angeles")) to this method.
        Map<TaxComponent, BigDecimal> result = service.computeTax(taxGroup, null, LocalDate.now(), 6);
                                                                                               ^
    (see https://errorprone.info/bugpattern/JavaTimeDefaultTimeZone)
  Did you mean 'Map<TaxComponent, BigDecimal> result = service.computeTax(taxGroup, null, LocalDate.now(ZoneId.systemDefault()), 6);'?
/home/runner/work/fineract/fineract/fineract-tax/src/test/java/org/apache/fineract/portfolio/tax/service/ChargeTaxApplicationServiceTest.java:84: error: [JavaTimeDefaultTimeZone] LocalDate.now() is not allowed because it silently uses the system default time-zone. You must pass an explicit time-zone (e.g., ZoneId.of("America/Los_Angeles")) to this method.
        Map<TaxComponent, BigDecimal> result = service.computeTax(taxGroup, BigDecimal.ZERO, LocalDate.now(), 6);
                                                                                                          ^
    (see https://errorprone.info/bugpattern/JavaTimeDefaultTimeZone)
  Did you mean 'Map<TaxComponent, BigDecimal> result = service.computeTax(taxGroup, BigDecimal.ZERO, LocalDate.now(ZoneId.systemDefault()), 6);'?
3 errors

alberto-art3ch · 2026-04-20T16:51:10Z

@alberto-art3ch It seems GA check is giving error prone issue on fineract-tax:compileTestJava can you please check.

While i tried to look at the ticket. Can you confirm me the tax bifurcation logic. Asking based on usecase perspective while on ticket i tried to cross-check I cannot seem to understand how tax logic should be calculated not much relevant data i can find.

I've reviewed the compilation issue in the fineract-tax Test part and solved. Thanks.

About the bifurcation logic about the Taxes in the Loan Charges is: If you have a Loan Charge with 100.00 as amount and you have a Tax of 15% in this case, the client pays the same 100.00 but internally (in the accounting) you will have three Journal Entries:

GL ACCOUNT	DEBIT	CREDIT
Fees Receivables	100.00
Income For Fees		85.00
Tax Liability		15.00

The Tax liability account is taken from Tax Component definition, and obviously it is linked to the Charge using the Tax Group

We are trying to follow a similar approach that we have in the Savings, so we are creating entities to have the detail about which Taxes (if apply) are being paying on the repayment transactions

Please let me know any comment or doubt

Aman-Mittal · 2026-04-20T22:01:30Z

@alberto-art3ch It seems GA check is giving error prone issue on fineract-tax:compileTestJava can you please check.

While i tried to look at the ticket. Can you confirm me the tax bifurcation logic. Asking based on usecase perspective while on ticket i tried to cross-check I cannot seem to understand how tax logic should be calculated not much relevant data i can find.

@alberto-art3ch It seems GA check is giving error prone issue on fineract-tax:compileTestJava can you please check.

While i tried to look at the ticket. Can you confirm me the tax bifurcation logic. Asking based on usecase perspective while on ticket i tried to cross-check I cannot seem to understand how tax logic should be calculated not much relevant data i can find.

I've reviewed the compilation issue in the fineract-tax Test part and solved. Thanks.

About the bifurcation logic about the Taxes in the Loan Charges is: If you have a Loan Charge with 100.00 as amount and you have a Tax of 15% in this case, the client pays the same 100.00 but internally (in the accounting) you will have three Journal Entries:

GL ACCOUNT DEBIT CREDIT

Fees Receivables 100.00

Income For Fees 85.00

Tax Liability 15.00

The Tax liability account is taken from Tax Component definition, and obviously it is linked to the Charge using the Tax Group

We are trying to follow a similar approach that we have in the Savings, so we are creating entities to have the detail about which Taxes (if apply) are being paying on the repayment transactions

Please let me know any comment or doubt

Yes, can you tell me is there any way user can check how much tax he has paid till now. It will be better for compliance. While I also want to make sure that tax bifurcation is also documented on ticket too.

alberto-art3ch · 2026-04-20T22:12:35Z

@alberto-art3ch It seems GA check is giving error prone issue on fineract-tax:compileTestJava can you please check.
While i tried to look at the ticket. Can you confirm me the tax bifurcation logic. Asking based on usecase perspective while on ticket i tried to cross-check I cannot seem to understand how tax logic should be calculated not much relevant data i can find.

@alberto-art3ch It seems GA check is giving error prone issue on fineract-tax:compileTestJava can you please check.
While i tried to look at the ticket. Can you confirm me the tax bifurcation logic. Asking based on usecase perspective while on ticket i tried to cross-check I cannot seem to understand how tax logic should be calculated not much relevant data i can find.

I've reviewed the compilation issue in the fineract-tax Test part and solved. Thanks.
About the bifurcation logic about the Taxes in the Loan Charges is: If you have a Loan Charge with 100.00 as amount and you have a Tax of 15% in this case, the client pays the same 100.00 but internally (in the accounting) you will have three Journal Entries:

GL ACCOUNT
DEBIT
CREDIT

Fees Receivables
100.00

Income For Fees

85.00

Tax Liability

15.00

The Tax liability account is taken from Tax Component definition, and obviously it is linked to the Charge using the Tax Group
We are trying to follow a similar approach that we have in the Savings, so we are creating entities to have the detail about which Taxes (if apply) are being paying on the repayment transactions
Please let me know any comment or doubt

Yes, can you tell me is there any way user can check how much tax he has paid till now. It will be better for compliance. While I also want to make sure that tax bifurcation is also documented on ticket too.

To start, in the Loan Charge entity there is a new field that will store the Tax Amount (sum of all the different Taxes applied) of this Loan Charge.
Plus there is a new entity named LoanChargeTaxDetails that stores (in detail) the relation between Loan Charge and Tax Component and the amount of this.

In resume the data is already there, one way to know (as other ones) is with a report, or maybe we can extend the Loan Charge api details to have that data

Aman-Mittal · 2026-04-20T22:15:34Z

@alberto-art3ch It seems GA check is giving error prone issue on fineract-tax:compileTestJava can you please check.
While i tried to look at the ticket. Can you confirm me the tax bifurcation logic. Asking based on usecase perspective while on ticket i tried to cross-check I cannot seem to understand how tax logic should be calculated not much relevant data i can find.

@alberto-art3ch It seems GA check is giving error prone issue on fineract-tax:compileTestJava can you please check.
While i tried to look at the ticket. Can you confirm me the tax bifurcation logic. Asking based on usecase perspective while on ticket i tried to cross-check I cannot seem to understand how tax logic should be calculated not much relevant data i can find.

I've reviewed the compilation issue in the fineract-tax Test part and solved. Thanks.
About the bifurcation logic about the Taxes in the Loan Charges is: If you have a Loan Charge with 100.00 as amount and you have a Tax of 15% in this case, the client pays the same 100.00 but internally (in the accounting) you will have three Journal Entries:

GL ACCOUNT
DEBIT
CREDIT

Fees Receivables
100.00

Income For Fees

85.00

Tax Liability

15.00

The Tax liability account is taken from Tax Component definition, and obviously it is linked to the Charge using the Tax Group
We are trying to follow a similar approach that we have in the Savings, so we are creating entities to have the detail about which Taxes (if apply) are being paying on the repayment transactions
Please let me know any comment or doubt

Yes, can you tell me is there any way user can check how much tax he has paid till now. It will be better for compliance. While I also want to make sure that tax bifurcation is also documented on ticket too.

To start, in the Loan Charge entity there is a new field that will store the Tax Amount (sum of all the different Taxes applied) of this Loan Charge.
Plus there is a new entity named LoanChargeTaxDetails that stores (in detail) the relation between Loan Charge and Tax Component and the amount of this.

In resume the data is already there, one way to know (as other ones) is with a report, or maybe we can extend the Loan Charge api details to have that data

Need to ask Bharat and other PMC member for this. Thanks for explaining.

I just want to make sure these things are documented properly.

Aman-Mittal

Code Wise seems ok, I have also checked the relevant ticket, Based on conversation it seems to implement what it implements.

adamsaghy

Please make sure the new functionality and the changes are covered with documentation in the fineract-doc module and under the https://issues.apache.org/jira/browse/FINERACT-1289

jonattanvargas-habi · 2026-04-28T21:50:40Z

Hi @alberto-art3ch

In Colombia, VAT (IVA, 19%) on insurance and similar loan charges is passed through to the customer: the customer pays base + VAT as a single line in the repayment schedule. The bank collects the full
amount and remits the VAT portion to the tax authority.

With the current PR, after applyTaxIfConfigured:

m_loan_charge.amount = base
m_loan_charge.tax_amount = computed tax
feeChargesDue on the schedule = base only

This works perfectly for tax models where the bank absorbs the tax (e.g. savings withholding tax). But for pass-through tax, the customer's feeChargesDue is short by the tax amount — the VAT lives only in
journal entries and doesn't surface in what the customer must pay in the schedule.

Proposal

A tax_added_to_amount boolean on m_tax_group (default false to preserve current behavior). When true, applyTaxIfConfigured sums the tax into amount so the outstanding reflects what the customer
actually owes:

// LoanChargeService.applyTaxIfConfigured
loanCharge.setTaxAmount(totalTax);
if (taxGroup.isTaxAddedToAmount()) {
}                                                                                                                                                                                                                   
loanCharge.setAmountOutstanding(loanCharge.calculateOutstanding());
loanCharge.getTaxDetails().clear();                                                                                                                                                                                 
taxSplit.forEach((component, taxAmt) ->                                                                                                                                                                             
    loanCharge.getTaxDetails().add(new LoanChargeTaxDetails(loanCharge, component, taxAmt)));

Schema delta:

<changeSet author="fineract" id="loan-charge-tax-3">
    <addColumn tableName="m_tax_group">             
        <column name="tax_added_to_amount" type="BOOLEAN" defaultValueBoolean="false">                                                                                                                              
            <constraints nullable="false"/>                                                                                                                                                                         
        </column>                                                                                                                                                                                                   
    </addColumn>                                                                                                                                                                                                    
</changeSet>

The flag is queryable via taxGroup.isTaxAddedToAmount() (Lombok-generated getter) and exposed in TaxGroupData for the API. The accounting separation (your LoanCommonAccountingHelper and
ChargeTaxPaymentDTO work) stays unchanged — tax_amount is still persisted separately, journal entries still split fee vs tax. Only the customer-facing outstanding changes.

I'd be happy to open a follow-up PR with the migration, the flag, the API exposure on TaxGroupData, and tests covering both modes — once this PR lands, or earlier if you'd prefer a single combined PR.

Independent issue, much smaller. The call is:

chargeTaxApplicationService.computeTax(taxGroup, loanCharge.getAmount(), effectiveDate, 6);

The 6 is the scale of the underlying DECIMAL(19,6) column, but splitTax uses it to round the value itself. For a currency like COP with digitsAfterDecimal=0 (no cents), tax ends up as 39527.917300
instead of 39528. The customer-visible amount in the API shows 6 decimals that the currency doesn't support.

Proposal

Use the loan currency's scale instead, with a sensible fallback:

int scale = (loanCharge.getLoan() != null && loanCharge.getLoan().getCurrency() != null)                                                                                                                            
    ? loanCharge.getLoan().getCurrency().getDigitsAfterDecimal()                        
    : 2;                                                                                                                                                                                                            
Map<TaxComponent, BigDecimal> taxSplit =                                                                                                                                                                            
    chargeTaxApplicationService.computeTax(taxGroup, loanCharge.getAmount(), effectiveDate, scale);

This change is risk-free and aligns with how Money/MoneyHelper rounds elsewhere in the core. Could land as a small commit on this PR or as a tiny separate fix.

bharathcgowda · 2026-04-29T07:06:20Z

Hi @alberto-art3ch

In Colombia, VAT (IVA, 19%) on insurance and similar loan charges is passed through to the customer: the customer pays base + VAT as a single line in the repayment schedule. The bank collects the full amount and remits the VAT portion to the tax authority.

With the current PR, after applyTaxIfConfigured:

m_loan_charge.amount = base
m_loan_charge.tax_amount = computed tax
feeChargesDue on the schedule = base only

This works perfectly for tax models where the bank absorbs the tax (e.g. savings withholding tax). But for pass-through tax, the customer's feeChargesDue is short by the tax amount — the VAT lives only in journal entries and doesn't surface in what the customer must pay in the schedule.

Proposal

A tax_added_to_amount boolean on m_tax_group (default false to preserve current behavior). When true, applyTaxIfConfigured sums the tax into amount so the outstanding reflects what the customer actually owes:

// LoanChargeService.applyTaxIfConfigured
loanCharge.setTaxAmount(totalTax);
if (taxGroup.isTaxAddedToAmount()) {
}                                                                                                                                                                                                                   
loanCharge.setAmountOutstanding(loanCharge.calculateOutstanding());
loanCharge.getTaxDetails().clear();                                                                                                                                                                                 
taxSplit.forEach((component, taxAmt) ->                                                                                                                                                                             
    loanCharge.getTaxDetails().add(new LoanChargeTaxDetails(loanCharge, component, taxAmt)));

Schema delta:

<changeSet author="fineract" id="loan-charge-tax-3">
    <addColumn tableName="m_tax_group">             
        <column name="tax_added_to_amount" type="BOOLEAN" defaultValueBoolean="false">                                                                                                                              
            <constraints nullable="false"/>                                                                                                                                                                         
        </column>                                                                                                                                                                                                   
    </addColumn>                                                                                                                                                                                                    
</changeSet>

The flag is queryable via taxGroup.isTaxAddedToAmount() (Lombok-generated getter) and exposed in TaxGroupData for the API. The accounting separation (your LoanCommonAccountingHelper and ChargeTaxPaymentDTO work) stays unchanged — tax_amount is still persisted separately, journal entries still split fee vs tax. Only the customer-facing outstanding changes.

I'd be happy to open a follow-up PR with the migration, the flag, the API exposure on TaxGroupData, and tests covering both modes — once this PR lands, or earlier if you'd prefer a single combined PR.

Independent issue, much smaller. The call is:

chargeTaxApplicationService.computeTax(taxGroup, loanCharge.getAmount(), effectiveDate, 6);

The 6 is the scale of the underlying DECIMAL(19,6) column, but splitTax uses it to round the value itself. For a currency like COP with digitsAfterDecimal=0 (no cents), tax ends up as 39527.917300 instead of 39528. The customer-visible amount in the API shows 6 decimals that the currency doesn't support.

Proposal

Use the loan currency's scale instead, with a sensible fallback:

int scale = (loanCharge.getLoan() != null && loanCharge.getLoan().getCurrency() != null)                                                                                                                            
    ? loanCharge.getLoan().getCurrency().getDigitsAfterDecimal()                        
    : 2;                                                                                                                                                                                                            
Map<TaxComponent, BigDecimal> taxSplit =                                                                                                                                                                            
    chargeTaxApplicationService.computeTax(taxGroup, loanCharge.getAmount(), effectiveDate, scale);

This change is risk-free and aligns with how Money/MoneyHelper rounds elsewhere in the core. Could land as a small commit on this PR or as a tiny separate fix.

@jonattanvargas-habi The enhancement you have mentioned is not part of this ticket and I believe we can handle it through different story. Could you please create a new enhancement ticket on fineract JIRA?

bharathcgowda · 2026-04-29T07:11:57Z

@alberto-art3ch It seems GA check is giving error prone issue on fineract-tax:compileTestJava can you please check.
While i tried to look at the ticket. Can you confirm me the tax bifurcation logic. Asking based on usecase perspective while on ticket i tried to cross-check I cannot seem to understand how tax logic should be calculated not much relevant data i can find.

@alberto-art3ch It seems GA check is giving error prone issue on fineract-tax:compileTestJava can you please check.
While i tried to look at the ticket. Can you confirm me the tax bifurcation logic. Asking based on usecase perspective while on ticket i tried to cross-check I cannot seem to understand how tax logic should be calculated not much relevant data i can find.

I've reviewed the compilation issue in the fineract-tax Test part and solved. Thanks.
About the bifurcation logic about the Taxes in the Loan Charges is: If you have a Loan Charge with 100.00 as amount and you have a Tax of 15% in this case, the client pays the same 100.00 but internally (in the accounting) you will have three Journal Entries:
GL ACCOUNT
DEBIT
CREDIT
Fees Receivables
100.00
Income For Fees
85.00
Tax Liability
15.00
The Tax liability account is taken from Tax Component definition, and obviously it is linked to the Charge using the Tax Group
We are trying to follow a similar approach that we have in the Savings, so we are creating entities to have the detail about which Taxes (if apply) are being paying on the repayment transactions
Please let me know any comment or doubt

Yes, can you tell me is there any way user can check how much tax he has paid till now. It will be better for compliance. While I also want to make sure that tax bifurcation is also documented on ticket too.

To start, in the Loan Charge entity there is a new field that will store the Tax Amount (sum of all the different Taxes applied) of this Loan Charge.
Plus there is a new entity named LoanChargeTaxDetails that stores (in detail) the relation between Loan Charge and Tax Component and the amount of this.
In resume the data is already there, one way to know (as other ones) is with a report, or maybe we can extend the Loan Charge api details to have that data

Need to ask Bharat and other PMC member for this. Thanks for explaining.

I just want to make sure these things are documented properly.

@Aman-Mittal Appreciate all your questions and the detailed code review. The functionality explained by @alberto-art3ch looks correct for the inclusive tax configuration.

I also believe we can further enhance the tax bifurcation space over time, and those improvements can be handled through separate stories instead of being part of this ticket.

@alberto-art3ch , as mentioned by @adamsaghy , could you please document the current behavior in the fineract-doc module under FINERACT-1289?

alberto-art3ch · 2026-04-29T22:54:55Z

@alberto-art3ch It seems GA check is giving error prone issue on fineract-tax:compileTestJava can you please check.
While i tried to look at the ticket. Can you confirm me the tax bifurcation logic. Asking based on usecase perspective while on ticket i tried to cross-check I cannot seem to understand how tax logic should be calculated not much relevant data i can find.

@alberto-art3ch It seems GA check is giving error prone issue on fineract-tax:compileTestJava can you please check.
While i tried to look at the ticket. Can you confirm me the tax bifurcation logic. Asking based on usecase perspective while on ticket i tried to cross-check I cannot seem to understand how tax logic should be calculated not much relevant data i can find.

I've reviewed the compilation issue in the fineract-tax Test part and solved. Thanks.
About the bifurcation logic about the Taxes in the Loan Charges is: If you have a Loan Charge with 100.00 as amount and you have a Tax of 15% in this case, the client pays the same 100.00 but internally (in the accounting) you will have three Journal Entries:
GL ACCOUNT
DEBIT
CREDIT
Fees Receivables
100.00
Income For Fees
85.00
Tax Liability
15.00
The Tax liability account is taken from Tax Component definition, and obviously it is linked to the Charge using the Tax Group
We are trying to follow a similar approach that we have in the Savings, so we are creating entities to have the detail about which Taxes (if apply) are being paying on the repayment transactions
Please let me know any comment or doubt

Yes, can you tell me is there any way user can check how much tax he has paid till now. It will be better for compliance. While I also want to make sure that tax bifurcation is also documented on ticket too.

To start, in the Loan Charge entity there is a new field that will store the Tax Amount (sum of all the different Taxes applied) of this Loan Charge.
Plus there is a new entity named LoanChargeTaxDetails that stores (in detail) the relation between Loan Charge and Tax Component and the amount of this.
In resume the data is already there, one way to know (as other ones) is with a report, or maybe we can extend the Loan Charge api details to have that data

Need to ask Bharat and other PMC member for this. Thanks for explaining.
I just want to make sure these things are documented properly.

@Aman-Mittal Appreciate all your questions and the detailed code review. The functionality explained by @alberto-art3ch looks correct for the inclusive tax configuration.

I also believe we can further enhance the tax bifurcation space over time, and those improvements can be handled through separate stories instead of being part of this ticket.

@alberto-art3ch , as mentioned by @adamsaghy , could you please document the current behavior in the fineract-doc module under FINERACT-1289?

@adamsaghy / @bharathcgowda I've added the adoc file fineract-doc/src/docs/en/chapters/features/taxes-on-loan-charges.adoc

adamsaghy · 2026-04-30T19:50:10Z

+                    for (LoanChargeTaxDetails taxDetail : lc.getTaxDetails()) {
+                        if (taxDetail.getTaxComponent().getCreditAccount() != null) {
+                            final BigDecimal proRatedTax = taxDetail.getAmount().multiply(paidAmount).divide(chargeAmount, 6,
+                                    RoundingMode.HALF_EVEN);


why to hardcode the rounding? Use the configured rounding instead!

adamsaghy · 2026-04-30T19:55:46Z

+                final List<ChargeTaxPaymentDTO> feeTaxPayments = loanCommonAccountingHelper.filterTaxPayments(loanTransactionDTO, false);
+                final BigDecimal feeTaxTotal = loanCommonAccountingHelper.sumTaxAmounts(feeTaxPayments);
+                if (feeTaxTotal.compareTo(BigDecimal.ZERO) > 0) {
+                    final BigDecimal netFees = feesAmount.subtract(feeTaxTotal);
+                    this.helper.createCreditJournalEntryForLoanCharges(office, currencyCode,
+                            AccrualAccountsForLoan.INCOME_FROM_FEES.getValue(), loanProductId, loanId, transactionId, transactionDate,
+                            netFees,
+                            loanCommonAccountingHelper.computeNetChargePayments(loanTransactionDTO.getFeePayments(), feeTaxPayments));
+                    loanCommonAccountingHelper.createTaxLiabilityCreditEntries(office, currencyCode, loanId, transactionId, transactionDate,
+                            feeTaxPayments);
+                } else {
+                    this.helper.createCreditJournalEntryForLoanCharges(office, currencyCode,
+                            AccrualAccountsForLoan.INCOME_FROM_FEES.getValue(), loanProductId, loanId, transactionId, transactionDate,
+                            feesAmount, loanTransactionDTO.getFeePayments());
+                }


You are recalculating tax payments over and over again...

You can SUM and fetch the appropriate tax entries for a particular charge in 1 sql query.

Would that be better than fetching all fee payment and all tax payments and group and sum them over and over again? What do you think?

tax amount is calculated at earlier stage, why dont just use that?

adamsaghy · 2026-04-30T20:06:08Z

+ *
+ * Pure domain-object tests — no Spring context, no persistence.
+ */
+class LoanChargeTaxDetailsTest {


Quite useless tests... i would remove...

adamsaghy · 2026-04-30T20:08:46Z

+                    // Reversal: CR Penalties Receivable (full), DR Income from Penalties (net), DR Tax Liability
+                    this.helper.createCreditJournalEntryForLoanCharges(office, currencyCode,
+                            AccrualAccountsForLoan.PENALTIES_RECEIVABLE.getValue(), loanProductId, loanId, transactionId, transactionDate,
+                            penaltiesAmount, loanTransactionDTO.getPenaltyPayments());


what is the difference between penaltyTaxPayments and loanTransactionDTO.getPenaltyPayments()?

adamsaghy · 2026-04-30T20:09:20Z

+                    this.helper.createDebitJournalEntryForLoanCharges(office, currencyCode,
+                            AccrualAccountsForLoan.PENALTIES_RECEIVABLE.getValue(), loanProductId, loanId, transactionId, transactionDate,
+                            penaltiesAmount, loanTransactionDTO.getPenaltyPayments());
+                    this.helper.createCreditJournalEntryForLoanCharges(office, currencyCode,


Duplicates? Common operations can be extracted... the whole logic would be less complex!

adamsaghy

Kindly review my concerns...

alberto-art3ch force-pushed the FINERACT-1289/taxes-on-loan-charges branch 4 times, most recently from a073ab8 to ee29805 Compare April 20, 2026 03:23

alberto-art3ch force-pushed the FINERACT-1289/taxes-on-loan-charges branch from ee29805 to dbe72e1 Compare April 20, 2026 16:06

Aman-Mittal approved these changes Apr 22, 2026

View reviewed changes

adamsaghy reviewed Apr 22, 2026

View reviewed changes

Comment thread fineract-client/build.gradle

adamsaghy requested changes Apr 22, 2026

View reviewed changes

FINERACT-1289: Taxes on Loan charges

f085e18

alberto-art3ch force-pushed the FINERACT-1289/taxes-on-loan-charges branch from dbe72e1 to f085e18 Compare April 29, 2026 22:53

adamsaghy reviewed Apr 30, 2026

View reviewed changes

adamsaghy requested changes Apr 30, 2026

View reviewed changes

Conversation

alberto-art3ch commented Apr 19, 2026

Description

Checklist

Uh oh!

Aman-Mittal commented Apr 20, 2026

Uh oh!

adamsaghy commented Apr 20, 2026

Uh oh!

alberto-art3ch commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Aman-Mittal commented Apr 20, 2026

Uh oh!

alberto-art3ch commented Apr 20, 2026

Uh oh!

Aman-Mittal commented Apr 20, 2026

Uh oh!

Aman-Mittal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

jonattanvargas-habi commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposal

Proposal

Uh oh!

bharathcgowda commented Apr 29, 2026

Proposal

Proposal

Uh oh!

bharathcgowda commented Apr 29, 2026

Uh oh!

alberto-art3ch commented Apr 29, 2026

Uh oh!

adamsaghy Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

adamsaghy Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

adamsaghy Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

adamsaghy Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

adamsaghy Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alberto-art3ch commented Apr 20, 2026 •

edited

Loading

jonattanvargas-habi commented Apr 28, 2026 •

edited

Loading

adamsaghy Apr 30, 2026 •

edited

Loading