FINERACT-1943 Add SubmittedOnDate to Savings Transaction #3317
Conversation
|
@Mk9894 Some tests are failing, kindly check them |
| @@ -345,6 +348,7 @@ private SavingsAccountTransaction(final SavingsAccount savingsAccount, final Off | |||
| this.reversed = isReversed; | |||
| this.paymentDetail = paymentDetail; | |||
| this.createdDate = createdDate; | |||
| this.submittedOnDate = DateUtils.getBusinessLocalDate(); | |||
There was a problem hiding this comment.
There are multiple constructor of the savings transaction. It might worth to cover all places where a transaction can be created then the submitted on date shall be set as well!
There was a problem hiding this comment.
Agreed. It is a wrong pattern having multiple constructors in the middle of the class, because it leads to these mistakes. Better to have only one at the beginning of the class (or somewhere agreed on generally)
Mk9894
force-pushed
the
FINERACT-1943
branch
2 times, most recently
from
2b47561
to
ffe8652
Compare
| @@ -206,7 +206,8 @@ private void batchUpdate(final List<SavingsAccountData> savingsAccountDataList) | |||
| savingsAccountTransactionData.getBalanceNumberOfDays(), savingsAccountTransactionData.getRunningBalance(), | |||
| savingsAccountTransactionData.getCumulativeBalance(), currentDate, Integer.valueOf(1), | |||
| savingsAccountTransactionData.isManualTransaction(), savingsAccountTransactionData.getRefNo(), | |||
| savingsAccountTransactionData.isReversalTransaction(), savingsAccountTransactionData.getOverdraftAmount(), }); | |||
| savingsAccountTransactionData.isReversalTransaction(), savingsAccountTransactionData.getOverdraftAmount(), | |||
| savingsAccountTransactionData.getTransactionDate() }); | |||
There was a problem hiding this comment.
this looks wrong to me... submitted on date != transaction date
There was a problem hiding this comment.
@adamsaghy I think this should be DateUtils.getBusinessLocalDate() instead of the transaction date. Please correct me if i am wrong
| @@ -345,6 +348,7 @@ private SavingsAccountTransaction(final SavingsAccount savingsAccount, final Off | |||
| this.reversed = isReversed; | |||
| this.paymentDetail = paymentDetail; | |||
| this.createdDate = createdDate; | |||
| this.submittedOnDate = DateUtils.getBusinessLocalDate(); | |||
There was a problem hiding this comment.
Agreed. It is a wrong pattern having multiple constructors in the middle of the class, because it leads to these mistakes. Better to have only one at the beginning of the class (or somewhere agreed on generally)
| @@ -206,7 +206,8 @@ private void batchUpdate(final List<SavingsAccountData> savingsAccountDataList) | |||
| savingsAccountTransactionData.getBalanceNumberOfDays(), savingsAccountTransactionData.getRunningBalance(), | |||
There was a problem hiding this comment.
This change is ok, but checked a bit the usages of SavingsAccountTransactionData and found several issues. Started to collect but there are more.
Please rename SavingsAccountTransactionData constructor parameter createdDate
SavingsAccountTransactionData line 522: this.submittedOnDate = transactionDate;
does not seem to be correct
SavingsAccountReadPlatformServiceImpl.extractData line 603:
final LocalDate transSubmittedOnDate = JdbcSupport.getLocalDate(rs, "createdDate");
not correct
RecurringAccountDepositTransactionTemplateMapper sends m_mandatory_savings_schedule.duedate as submittedOnDate - is this correct?
SavingsAccountTransactionsMapper in DepositAccountReadPlatformServiceImpl sends transaction_date - not correct
and this init is used from 8 other places
Please check all the usages of SavingsAccountTransactionData static initialisers (all of them), even if it is it is hard to follow.
Other place to check for submittedOnDate:
SavingsAccountReadPlatformServiceImpl.SavingsAccountTransactionsMapper
sqlBuilder.append("tr.created_date as submittedOnDate,");
There was a problem hiding this comment.
@marta-jankovics I planned to do this part in the next PR. This PR just introduces submittedOnDate and initializes it. This PR might become huge if we make changes to all places where submittedOnDate is consumed.
What do you think?
adamsaghy
dismissed
their stale review
Marta found new problems that need to be addressed
Description
Describe the changes made and why they were made.
Ignore if these details are present on the associated Apache Fineract JIRA ticket.
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Write the commit message as per https://github.com/apache/fineract/#pull-requests
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 coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/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
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.