[GitHub] [fineract] ptuomola commented on a change in pull request #1006: FINERACT-822 add MissingCasesInEnumSwitch

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org