ptuomola commented on a change in pull request #2002:
URL: https://github.com/apache/fineract/pull/2002#discussion_r775186243



##########
File path: 
fineract-provider/src/main/java/org/apache/fineract/portfolio/client/domain/AccountNumberGenerator.java
##########
@@ -143,8 +166,13 @@ private String generateAccountNumber(Map<String, String> 
propertyMap, AccountNum
             }
             if 
(accountNumberPrefixType.getValue().equals(AccountNumberPrefixType.PREFIX_SHORT_NAME.getValue()))
 {
                 Integer prefixLength = prefix.length();
-                Integer numberLength = accountMaxLength - prefixLength;
-                accountNumber = StringUtils.leftPad(propertyMap.get(ID), 
numberLength, '0');
+
+                if (randomAccountNumber.isEnabled()) {
+                    accountNumber = accountNumber.substring(prefixLength);

Review comment:
       Not sure I understand this code... why do we make the random account 
number the same length as the prefix? And since we are truncating the random 
number here, how do we ensure it is still unique after this truncation? 
   
   If the problem is that we don't know the length of the random number we are 
creating, maybe one solution would be to store that as a configuration 
parameter as part of the AccountNumberFormat?

##########
File path: 
fineract-provider/src/main/java/org/apache/fineract/portfolio/client/domain/AccountNumberGenerator.java
##########
@@ -108,7 +124,14 @@ private String generateAccountNumber(Map<String, String> 
propertyMap, AccountNum
             }
         }
 
-        String accountNumber = StringUtils.leftPad(propertyMap.get(ID), 
accountMaxLength, '0');
+        final GlobalConfigurationPropertyData randomAccountNumber = 
this.configurationReadPlatformService
+                .retrieveGlobalConfiguration("random-account-number");

Review comment:
       Does it make sense to have this as a single global configuration 
property so that then all numbers are random (client, loan, savings account)? 
Or would it make more sense to store this as part of the AccountNumberFormat as 
a parameter so that people can enable this for different entities if they'd 
like to? Then we could also store the length of the random number to be created 
as part of the AccountNumberFormat. 




-- 
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]


Reply via email to