[PIDM-42] fix(payment-status): payment position/option status not coherent #289

dylantangredi-bvt · 2025-01-31T08:35:46Z

List of Changes

These changes are relevant in case the user pays the full amount for a payment position in presence of already paid installments/payment options:

Fixed payment option status not changing from PO_UNPAID to PO_PAID
Added detailed logs for this specific case, instead of throwing an exception that makes the db not coherent with the payment process on the node
Fixed payment position status not changing from PARTIALLY_PAID to PAID
Moved the exception handling from the /pay to the /get (activate) phase to prevent the issue

Motivation and Context

This is a hotfix that was necessary to make the db coherent with what the user experiences during checkout. Before the fix, if the user paid using a NAV corresponding to the full amount due for the payment position but one or more installment/payment options were already paid for the same payment position, then the payment option corresponding to the full amount did not change status from PO_UNPAID to PAID, causing a discrepancy in the process. This happened because the checkout ended correctly up to the receipt generation for the user payment, but in the backend a HttpStatus.CONFLICT, "Existing related payment found" was being thrown.

How Has This Been Tested?

Tested locally with Postman making the following use case:

Create debt position with 4 payment options, option 1 being for the full amount and option 2/3/4 for the 3 installments
Publish debt position
Pay payment option 2 (corresponding to one installment)
Get payment option 2 by NAV and check db status on the option (PO_PAID)
Get payment option 1 by NAV (corresponding to the full amount) -> error 409
Pay payment option 1 (corresponding to the full amount), skipping activate
Retrieve payment option by NAV and check db status on the option -> PO_PAID and debtPositionStatus -> PAID

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.

…position status not changing to PAID when paying the full amount in presence of already paid installments;

src/main/java/it/gov/pagopa/debtposition/util/DebtPositionValidation.java

…fix-payment-status # Conflicts: # src/main/java/it/gov/pagopa/debtposition/util/DebtPositionValidation.java

cap-ang

Great,

How about preventing these cases by blocking the activation phase with a 409 if the activation is not related to an installment (payment option with isPartialPayment true)?

… PIDM-42-fix-payment-status # Conflicts: # src/main/java/it/gov/pagopa/debtposition/util/DebtPositionValidation.java

…on to get/activate phase; adapt existing unit test;

dylantangredi-bvt · 2025-01-31T11:45:45Z

Great,

How about preventing these cases by blocking the activation phase with a 409 if the activation is not related to an installment (payment option with isPartialPayment true)?

I assume you meant isPartialPayment false because that indicates the full amount payment (so not related to an installment). In this case it has been done with the latest commit

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

src/main/java/it/gov/pagopa/debtposition/util/DebtPositionValidation.java

+      log.warn(
+          "Potential payment state inconsistency detected || "
+              + "Organization: {} || "
+              + "IUPD: {} || "
+              + "NAV: {} || "
+              + "Position Status: {} || "
+              + "Payment Option Status: {} || "
+              + "Is Partial Payment: {} || "
+              + "Timestamp: {}",
+          ppToPay.getOrganizationFiscalCode(),
+          ppToPay.getIupd(),
+          nav,
+          ppToPay.getStatus(),
+          poToPay.getStatus(),
+          poToPay.getIsPartialPayment(),
+          LocalDateTime.now());


To fix the log injection issue, we need to sanitize the nav parameter before logging it. The best way to do this is to remove any potentially harmful characters from the nav parameter, such as new-line characters, which can be used to forge log entries. We can use a utility method to sanitize the input by replacing or removing such characters.

Create a utility method to sanitize the nav parameter by removing new-line characters and other potentially harmful characters.

Use this utility method to sanitize the nav parameter before logging it in the DebtPositionValidation class.

jacopocarlini

🚀

jacopocarlini · 2025-02-04T09:24:04Z

@dylantangredi-bvt can you fix the tests please?

dylantangredi-bvt · 2025-02-04T11:04:05Z

@dylantangredi-bvt can you fix the tests please?

Hello @jacopocarlini , are you referring to these two tests? These were the ones failing when I pushed the latest commits last week.

github-actions · 2025-02-05T14:29:56Z

This PR exceeds the recommended size of 400 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

sonarqubecloud · 2025-02-05T14:32:45Z

Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
96.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[PIDM-42] fix(payment-status): fix payment option status and payment …

34b4721

…position status not changing to PAID when paying the full amount in presence of already paid installments;

dylantangredi-bvt added bug Something isn't working gpd patch minor labels Jan 31, 2025

github-advanced-security bot found potential problems Jan 31, 2025

View reviewed changes

src/main/java/it/gov/pagopa/debtposition/util/DebtPositionValidation.java Fixed Show fixed Hide fixed

dylantangredi-bvt added 2 commits January 31, 2025 09:38

Merge remote-tracking branch 'refs/remotes/origin/main' into PIDM-42-…

cec4944

…fix-payment-status # Conflicts: # src/main/java/it/gov/pagopa/debtposition/util/DebtPositionValidation.java

Merge branch 'main' into PIDM-42-fix-payment-status

c2e1437

github-actions bot assigned dylantangredi-bvt Jan 31, 2025

github-actions bot added the size/small label Jan 31, 2025

cap-ang requested changes Jan 31, 2025

View reviewed changes

dylantangredi-bvt added 2 commits January 31, 2025 11:51

Merge remote-tracking branch 'origin/PIDM-42-fix-payment-status' into…

2ee80ab

… PIDM-42-fix-payment-status # Conflicts: # src/main/java/it/gov/pagopa/debtposition/util/DebtPositionValidation.java

[PIDM-42] fix(payment-status): move existing partial payment validati…

7aac4b2

…on to get/activate phase; adapt existing unit test;

dylantangredi-bvt requested a review from cap-ang January 31, 2025 11:45

dylantangredi-bvt marked this pull request as ready for review January 31, 2025 11:50

dylantangredi-bvt requested a review from a team as a code owner January 31, 2025 11:50

Apply suggestions from code review

906496f

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-advanced-security bot found potential problems Jan 31, 2025

View reviewed changes

format

c3fa871

jacopocarlini approved these changes Feb 4, 2025

View reviewed changes

jacopocarlini added 2 commits February 5, 2025 14:40

Merge branch 'main' into PIDM-42-fix-payment-status

84d7a26

chore: update iban

4aef8bf

github-actions bot added size/large and removed size/small labels Feb 5, 2025

jacopocarlini added 2 commits February 5, 2025 15:16

fix: integration test

3c3570e

fix

636dc15

jacopocarlini merged commit df27e00 into main Feb 5, 2025
15 of 16 checks passed

jacopocarlini deleted the PIDM-42-fix-payment-status branch February 5, 2025 14:34

@@ -25,2 +25,3 @@
             import org.apache.logging.log4j.util.Strings;
+            import org.apache.commons.lang3.StringUtils;
@@ -29,2 +30,9 @@
+              private static String sanitize(String input) {
+                if (input == null) {
+                  return null;
+                }
+                return StringUtils.replaceChars(input, "\n\r", "");
+              }
               private static final String LOG_BASE_PARAMS_DETAIL =
@@ -281,2 +289,3 @@
                   // log detailed information about this edge case
+                  String sanitizedNav = sanitize(nav);
                   log.warn(
@@ -292,3 +301,3 @@
                       ppToPay.getIupd(),
-                      nav,
+                      sanitizedNav,
                       ppToPay.getStatus(),

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PIDM-42] fix(payment-status): payment position/option status not coherent #289

[PIDM-42] fix(payment-status): payment position/option status not coherent #289

dylantangredi-bvt commented Jan 31, 2025 •

edited

Loading

cap-ang left a comment •

edited

Loading

dylantangredi-bvt commented Jan 31, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

jacopocarlini left a comment

jacopocarlini commented Feb 4, 2025

dylantangredi-bvt commented Feb 4, 2025

github-actions bot commented Feb 5, 2025

sonarqubecloud bot commented Feb 5, 2025

[PIDM-42] fix(payment-status): payment position/option status not coherent #289

[PIDM-42] fix(payment-status): payment position/option status not coherent #289

Conversation

dylantangredi-bvt commented Jan 31, 2025 • edited Loading

List of Changes

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

cap-ang left a comment • edited Loading

Choose a reason for hiding this comment

dylantangredi-bvt commented Jan 31, 2025

jacopocarlini left a comment

Choose a reason for hiding this comment

jacopocarlini commented Feb 4, 2025

dylantangredi-bvt commented Feb 4, 2025

github-actions bot commented Feb 5, 2025

sonarqubecloud bot commented Feb 5, 2025

Quality Gate passed

dylantangredi-bvt commented Jan 31, 2025 •

edited

Loading

cap-ang left a comment •

edited

Loading