[GitHub] [fineract] adamsaghy commented on a diff in pull request #2852: FINERACT-1847: Removing Edit function from Goodwill Credit or Payout Refund or Merch…

2023-01-18 Thread GitBox


adamsaghy commented on code in PR #2852:
URL: https://github.com/apache/fineract/pull/2852#discussion_r1073524648


##
fineract-provider/src/main/java/org/apache/fineract/portfolio/client/service/ClientWritePlatformServiceJpaRepositoryImpl.java:
##
@@ -430,7 +430,8 @@ public CommandProcessingResult updateClient(final Long 
clientId, final JsonComma
 }
 
 final ExternalId externalId = 
externalIdFactory.createFromCommand(command, 
ClientApiConstants.externalIdParamName);
-if 
(command.isChangeInStringParameterNamed(ClientApiConstants.externalIdParamName, 
externalId.getValue())) {
+if 
(command.isChangeInStringParameterNamed(ClientApiConstants.externalIdParamName,

Review Comment:
   Please raise a separate PR for fixing the client external id updating bug!



-- 
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: commits-unsubscr...@fineract.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



[GitHub] [fineract] adamsaghy commented on a diff in pull request #2852: FINERACT-1847: Removing Edit function from Goodwill Credit or Payout Refund or Merch…

2023-01-10 Thread GitBox


adamsaghy commented on code in PR #2852:
URL: https://github.com/apache/fineract/pull/2852#discussion_r1065583905


##
integration-tests/src/test/java/org/apache/fineract/integrationtests/ClientLoanCreditBalanceRefundandRepaymentTypeIntegrationTest.java:
##
@@ -438,4 +439,73 @@ public void 
newGoodwillCreditCreatesCorrectJournalEntriesForCashAccountingTest()
 
 }
 
+@Test

Review Comment:
   Please add test cases to cover the adjusting of the goodwill credit, payout 
refund and merchant issued refund and chargeback is not possible and they throw 
the proper error.



-- 
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: commits-unsubscr...@fineract.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



[GitHub] [fineract] adamsaghy commented on a diff in pull request #2852: FINERACT-1847: Removing Edit function from Goodwill Credit or Payout Refund or Merch…

2023-01-09 Thread GitBox


adamsaghy commented on code in PR #2852:
URL: https://github.com/apache/fineract/pull/2852#discussion_r1064756703


##
integration-tests/src/test/java/org/apache/fineract/integrationtests/LoanTransactionChargebackTest.java:
##
@@ -114,7 +116,7 @@ public void applyLoanTransactionChargeback() {
 
 // Try to reverse a Loan Transaction charge back
 PostLoansLoanIdTransactionsResponse reverseTransactionResponse = 
loanTransactionHelper.reverseLoanTransaction(loanId,
-chargebackTransactionId, operationDate, responseSpecErr503);
+chargebackTransactionId, operationDate, responseSpecErr403);

Review Comment:
   So reversing or adjusting a chargeback is not possible, right?



-- 
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: commits-unsubscr...@fineract.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



[GitHub] [fineract] adamsaghy commented on a diff in pull request #2852: FINERACT-1847: Removing Edit function from Goodwill Credit or Payout Refund or Merch…

2023-01-09 Thread GitBox


adamsaghy commented on code in PR #2852:
URL: https://github.com/apache/fineract/pull/2852#discussion_r1064749265


##
fineract-provider/src/main/java/org/apache/fineract/commands/service/CommandWrapperBuilder.java:
##
@@ -913,6 +913,15 @@ public CommandWrapperBuilder adjustTransaction(final Long 
loanId, final Long tra
 return this;
 }
 
+public CommandWrapperBuilder undoTransaction(final Long loanId, final Long 
transactionId) {

Review Comment:
   Dead code?



-- 
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: commits-unsubscr...@fineract.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



[GitHub] [fineract] adamsaghy commented on a diff in pull request #2852: FINERACT-1847: Removing Edit function from Goodwill Credit or Payout Refund or Merch…

2023-01-06 Thread GitBox


adamsaghy commented on code in PR #2852:
URL: https://github.com/apache/fineract/pull/2852#discussion_r1063438155


##
integration-tests/src/test/java/org/apache/fineract/integrationtests/ClientLoanCreditBalanceRefundandRepaymentTypeIntegrationTest.java:
##
@@ -438,4 +439,76 @@ public void 
newGoodwillCreditCreatesCorrectJournalEntriesForCashAccountingTest()
 
 }
 
+@Test
+public void tryToUndoGoodWillCreditTransactionTest() {

Review Comment:
   Please amend this test cases to tests for testing whether:
   - these transactions can be undone
   - these transactions cannot be adjusted 



-- 
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: commits-unsubscr...@fineract.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



[GitHub] [fineract] adamsaghy commented on a diff in pull request #2852: FINERACT-1847: Removing Edit function from Goodwill Credit or Payout Refund or Merch…

2023-01-06 Thread GitBox


adamsaghy commented on code in PR #2852:
URL: https://github.com/apache/fineract/pull/2852#discussion_r1063436939


##
integration-tests/src/test/java/org/apache/fineract/integrationtests/BatchApiTest.java:
##
@@ -1830,7 +1825,7 @@ public void 
shouldReturnOkStatusForBatchGoodwillCreditReversal() {
  * @see AdjustTransactionCommandStrategy
  */
 @Test
-public void 
shouldReturnOkStatusOnSuccessfulTransactionMerchantIssuedAndPayoutRefundReversal()
 {
+public void 
shouldReturnNOkStatusOnUnSuccessfulTransactionMerchantIssuedAndPayoutRefundReversal()
 {

Review Comment:
   Undo merchant issues refund is supported. Please revert this test case.



-- 
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: commits-unsubscr...@fineract.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



[GitHub] [fineract] adamsaghy commented on a diff in pull request #2852: FINERACT-1847: Removing Edit function from Goodwill Credit or Payout Refund or Merch…

2023-01-06 Thread GitBox


adamsaghy commented on code in PR #2852:
URL: https://github.com/apache/fineract/pull/2852#discussion_r1063436619


##
integration-tests/src/test/java/org/apache/fineract/integrationtests/BatchApiTest.java:
##
@@ -1808,19 +1806,16 @@ public void 
shouldReturnOkStatusForBatchGoodwillCreditReversal() {
 
 final List responses = 
BatchHelper.postBatchRequestsWithoutEnclosingTransaction(this.requestSpec, 
this.responseSpec,
 BatchHelper.toJsonString(batchRequests));
+Assertions.assertEquals(HttpStatus.SC_INTERNAL_SERVER_ERROR, (long) 
responses.get(4).getStatusCode(),

Review Comment:
   Undo goodwill credit is supported. Please revert this test case.



-- 
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: commits-unsubscr...@fineract.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



[GitHub] [fineract] adamsaghy commented on a diff in pull request #2852: FINERACT-1847: Removing Edit function from Goodwill Credit or Payout Refund or Merch…

2023-01-06 Thread GitBox


adamsaghy commented on code in PR #2852:
URL: https://github.com/apache/fineract/pull/2852#discussion_r1063435445


##
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanWritePlatformServiceJpaRepositoryImpl.java:
##
@@ -1063,9 +1063,10 @@ public CommandProcessingResult 
adjustLoanTransaction(final Long loanId, final Lo
 transactionId);
 }
 
-if (transactionToAdjust.isChargeback()) {
-throw new 
PlatformServiceUnavailableException("error.msg.loan.transaction.update.not.allowed",
 "Loan transaction:"
-+ transactionId + " update not allowed as loan transaction 
is charge back and is linked to other transaction",
+if (transactionToAdjust.isChargeback() || 
transactionToAdjust.isGoodwillCredit() || transactionToAdjust.isPayoutRefund()

Review Comment:
   This is not correct. We should let the undo, but the adjusting.
   Please amend this logic whether it is just undo (transactionAmount=0) or 
undo and create new (transactionAmount>0)



-- 
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: commits-unsubscr...@fineract.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org