galovics commented on code in PR #5847:
URL: https://github.com/apache/fineract/pull/5847#discussion_r3252548254
##########
fineract-charge/src/main/java/org/apache/fineract/portfolio/charge/service/ChargeDropdownReadPlatformServiceImpl.java:
##########
@@ -124,4 +124,36 @@ public List<EnumOptionData>
retrieveSharesCollectionTimeTypes() {
return
Arrays.asList(chargeTimeType(ChargeTimeType.SHAREACCOUNT_ACTIVATION),
chargeTimeType(ChargeTimeType.SHARE_PURCHASE),
chargeTimeType(ChargeTimeType.SHARE_REDEEM));
}
+
+ @Override
+ public List<EnumOptionData> retrieveCalculationTypes(ChargeAppliesTo
chargeAppliesTo, ChargeTimeType chargeTimeType) {
+ if (ChargeAppliesTo.WORKING_CAPITAL_LOAN.equals(chargeAppliesTo)) {
+ if (chargeTimeType == null) {
Review Comment:
I'm not convinced returning `null` is safe here. Today it's probably
unreachable since `validWorkingCapitalLoanValues()` only contains
`SPECIFIED_DUE_DATE`, but if more time types are added to working capital loans
later, any caller iterating the result will get an NPE. Wouldn't it make more
sense to return an empty list (or `Collections.emptyList()`) as the fallback?
Thoughts?
##########
fineract-e2e-tests-runner/src/test/resources/features/WorkingCapitalLoanCharge.feature:
##########
@@ -0,0 +1,8 @@
+@WorkingCapitalLoanChargesFeature
+Feature: WorkingCapitalLoanChargesFeature
+
+ @TestRailId:Cxxxx1
+ Scenario: Verify that charge can be created modified and deleted
Review Comment:
Looks like a placeholder - was this intentional or did the TestRail ID not
get filled in?
##########
fineract-e2e-tests-core/src/test/java/org/apache/fineract/test/stepdef/loan/ChargeStepDef.java:
##########
@@ -23,9 +23,11 @@
import io.cucumber.java.en.When;
import org.apache.fineract.client.feign.FineractFeignClient;
import org.apache.fineract.client.models.ChargeRequest;
+import org.apache.fineract.client.models.PostChargesResponse;
import org.apache.fineract.test.data.ChargeCalculationType;
import org.apache.fineract.test.data.ChargeProductResolver;
import org.apache.fineract.test.data.ChargeProductType;
Review Comment:
The rest of this file uses `ChargeCalculationType.valueOf(...)` and
`ChargeProductType.valueOf(...)` to avoid magic numbers, but these new methods
bypass that entirely and hardcode `5`, `2`, `1` directly (with comments
explaining what they mean, which is a smell in itself). Any chance we can
extract these into constants or reuse the existing enum abstractions the way
the other step defs in this class do? Thanks.
--
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]
