ptuomola commented on a change in pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#discussion_r437839268
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccountCharge.java
##########
@@ -292,7 +296,9 @@ private void populateDerivedFields(final BigDecimal
transactionAmount, final Big
this.amountWaived = null;
this.amountWrittenOff = null;
break;
- default:
+ case PERCENT_OF_DISBURSEMENT_AMOUNT:
Review comment:
As far as I can see, this case should never happen: we should not have a
savings account with a charge type of PERCENT_OF_DISBURSEMENT_AMOUNT, as that
just doesn't make sense (and is not even valid as set in
validValuesForSavings()). So the behaviour for this case in these functions
should be the same as for the other values that are not valid - e.g.
PERCENT_OF_AMOUNT_AND_INTEREST
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/infrastructure/accountnumberformat/data/AccountNumberFormatDataValidator.java
##########
@@ -127,7 +130,9 @@ public void validateForCreate(final String json) {
case GROUP:
validAccountNumberPrefixes =
AccountNumberFormatEnumerations.accountNumberPrefixesForGroups;
break;
- default:
+ case SHARES:
+ LOG.error("TODO Implement determineValidAccountNumberPrefixes for
SHARES");
+ break;
}
Review comment:
If you look in AccountNumberGenerator.generateAccountNumber(), it seems
that we currently are not generating any prefix for SHARES type accounts.
Similarly AccountNumberFormatEnumerations does not provide a prefix for SHARES
accounts. So means there are currently no valid prefixes for SHARES accounts -
and therefore returning an empty list (which was also existing behaviour) would
seem to be the correct behaviour.
Therefore to me this is therefore not a "TODO": returning the empty list is
correct as the functionality stands. Of course we could implement account
prefixes for SHARES accounts but that would not require just fixing this code,
but all the other places as well...
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/organisation/workingdays/domain/WorkingDaysEnumerations.java
##########
@@ -53,7 +53,10 @@ public static EnumOptionData repaymentRescheduleType(final
RepaymentRescheduleTy
optionData = new
EnumOptionData(RepaymentRescheduleType.MOVE_TO_PREVIOUS_WORKING_DAY.getValue().longValue(),RepaymentRescheduleType.MOVE_TO_PREVIOUS_WORKING_DAY.getCode(),
"move to previous working day");
break;
- default:
+ case MOVE_TO_NEXT_MEETING_DAY:
Review comment:
I wonder if this enum value is actually used for anything? I can't find
any reference to it in the Fineract code. The code that processes all the other
repaymentRescheduleTypeOptions (e.g.
WorkingDaysUtil.getOffSetDateIfNonWorkingDay()) does not have a case or any
logic for this value.
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/Loan.java
##########
@@ -4501,7 +4505,9 @@ private LocalDate getMaxDateLimitForNewRepayment(final
PeriodFrequencyType perio
break;
case INVALID:
break;
- default:
Review comment:
Looks like the whole repayment / interest rate calculation etc logic for
WHOLE_TERM is missing in a number of functions.
What we would need to work out is: is this intentional (i.e. the routines,
without any handling for WHOLE_TERM, will still return the right value) or is
this actually broken even now? I'd suggest trying this out e.g. through the
community app to see if loan products with whole term interest actually work
correctly in scenarios where these functions are called.
If they do, then the right thing to do would be to add a case that does
nothing for WHOLE_TERM so we have made this behaviour explicit. If they don't
work, then we should have an error message with a TO DO.
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/portfolio/note/api/NotesApiResource.java
##########
@@ -228,7 +232,12 @@ private CommandWrapper getResourceDetails(final NoteType
type, final Long resour
resourceNameForPermissions = "GROUPNOTE";
resourceDetails.withGroupId(resourceId);
break;
- default:
+ case SHARE_ACCOUNT:
Review comment:
If you look in createNote(), you can see that we only create specific
types of notes - i.e. a subset of the enum types available. So we should never
get a note with one of these types. In my view, therefore I think it would be
OK to have a default that returns "INVALIDTYPE" for any other type.
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/infrastructure/accountnumberformat/service/AccountNumberFormatReadPlatformServiceImpl.java
##########
@@ -145,7 +149,9 @@ public void determinePrefixTypesForAccounts(Map<String,
List<EnumOptionData>> ac
case GROUP :
accountNumberPrefixTypesSet =
AccountNumberFormatEnumerations.accountNumberPrefixesForGroups;
break;
- default:
+ case SHARES:
+ LOG.error("TODO Implement determinePrefixTypesForAccounts for
SHARES");
+ break;
Review comment:
Same here as above - I don't think we need to log an error, I would just
have a comment stating no prefixes for SHARES accounts:
case SHARES:
// SHARES accounts have no prefix
break;
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/portfolio/interestratechart/service/InterestRateChartEnumerations.java
##########
@@ -52,7 +56,9 @@ public static EnumOptionData periodType(final
PeriodFrequencyType type) {
optionData = new
EnumOptionData(PeriodFrequencyType.YEARS.getValue().longValue(),
PeriodFrequencyType.YEARS.getCode(), "Years");
break;
- default:
+ case WHOLE_TERM:
+ LOG.error("TODO Implement repaymentPeriodFrequencyType for
WHOLE_TERM");
+ break;
Review comment:
Is there a reason why we can't just return the optionData for WHOLE_TERM
just like for all the other values?
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/portfolio/shareaccounts/serialization/ShareAccountDataSerializer.java
##########
@@ -927,7 +931,9 @@ private LocalDate deriveLockinPeriodDuration(final Integer
lockinPeriod, final P
case YEARS:
lockinDate = purchaseDate.plusYears(lockinPeriod) ;
break ;
- default:
+ case WHOLE_TERM:
Review comment:
I don't think a lockinPeriod of WHOLE_TERM makes sense functionally - so
shouldn't the behaviour for this to be the same as for period INVALID?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
