feat: P4ADEV-1744-add-api-finalize-sync-status #15

LarissaASLeite · 2025-01-15T15:36:03Z

Description

List of Changes

Motivation and Context

How Has This Been Tested?

Pre-Deploy Test
- Unit
- Integration (Narrow)
Post-Deploy Test
- Isolated Microservice
- Broader Integration
- Acceptance
- Performance & Load

Types of changes

PATCH - Bug fix (backwards compatible bug fixes)
MINOR - New feature (add functionality in a backwards compatible manner)
MAJOR - Breaking change (fix or feature that would cause existing functionality to not work as expected)
CHORE - Minor Change (fix or feature that don't impact the functionality e.g. Documentation or lint configuration)

Checklist:

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

…ync-status # Conflicts: # openapi/generated.openapi.json # src/main/java/it/gov/pagopa/pu/debtpositions/mapper/DebtPositionMapper.java # src/main/java/it/gov/pagopa/pu/debtpositions/mapper/InstallmentMapper.java # src/main/java/it/gov/pagopa/pu/debtpositions/mapper/PaymentOptionMapper.java # src/main/java/it/gov/pagopa/pu/debtpositions/mapper/PersonMapper.java # src/main/java/it/gov/pagopa/pu/debtpositions/mapper/TransferMapper.java # src/main/java/it/gov/pagopa/pu/debtpositions/model/DebtPosition.java # src/main/java/it/gov/pagopa/pu/debtpositions/model/PaymentOption.java # src/main/java/it/gov/pagopa/pu/debtpositions/model/Transfer.java # src/main/java/it/gov/pagopa/pu/debtpositions/service/DebtPositionService.java # src/main/java/it/gov/pagopa/pu/debtpositions/service/DebtPositionServiceImpl.java # src/test/java/it/gov/pagopa/pu/debtpositions/util/faker/DebtPositionFaker.java # src/test/java/it/gov/pagopa/pu/debtpositions/util/faker/InstallmentFaker.java # src/test/java/it/gov/pagopa/pu/debtpositions/util/faker/PaymentOptionFaker.java # src/test/java/it/gov/pagopa/pu/debtpositions/util/faker/PersonFaker.java # src/test/java/it/gov/pagopa/pu/debtpositions/util/faker/TransferFaker.java

openapi/p4pa-debt-position.openapi.yaml

src/main/java/it/gov/pagopa/pu/debtpositions/repository/DebtPositionRepository.java

src/main/java/it/gov/pagopa/pu/debtpositions/repository/InstallmentNoPIIRepository.java

src/main/java/it/gov/pagopa/pu/debtpositions/repository/PaymentOptionRepository.java

src/main/java/it/gov/pagopa/pu/debtpositions/service/DebtPositionServiceImpl.java

antonioT90 · 2025-01-16T12:36:35Z

src/main/java/it/gov/pagopa/pu/debtpositions/service/DebtPositionServiceImpl.java

+    debtPosition.getPaymentOptions().forEach(this::updatePaymentOptionStatus);
+
+    updateDebtPositionStatus(debtPosition);


move these on a separate Service (DebtPositionHierarchyStatusAlignerService) which will set PaymentOptions and DebtPosition status based on their child.
The right logic is described on Confluence https://pagopa.atlassian.net/wiki/spaces/SPAC/pages/1420755133/Macchina+Stati+Posizione+Debitoria

PaymentOption status, prioritized order:

TO_SYNC -> if at least one Installment is TO_SYNC

PARTIALLY_PAID -> if at least one Installment is PAID and at least one is UNPAID or EXPIRED

UNPAID -> if all installments are UNPAID or CANCELLED

PAID -> if all installments are PAID or CANCELLED

REPORTED -> if all installments are REPORTED or CANCELLED

INVALID -> if all installments are INVALID or CANCELLED

CANCELLED -> if all installments are CANCELLED

EXPIRED -> if all installments are EXPIRED

DebtPosition status, prioritized order:

TO_SYNC -> if at least one PaymentOptions is TO_SYNC

PARTIALLY_PAID -> if at least one PaymentOptions is PAID and at least one is UNPAID or EXPIRED

UNPAID -> if all PaymentOptions are UNPAID or CANCELLED

PAID -> if all PaymentOptions are PAID or CANCELLED

REPORTED -> if all PaymentOptions are REPORTED or CANCELLED

INVALID -> if all PaymentOptions are INVALID or CANCELLED

CANCELLED -> if all PaymentOptions are CANCELLED

EXPIRED -> if all PaymentOptions are EXPIRED

Both update logic could be implemented on separate Services DebtPositionInnerStatusAlignerService and PaymentOptionInnerStatusAlignerService
group all these services on a common package

let's apply Chain Of Responsibility wiriting a separate class for each status, see https://github.com/pagopa/p4pa-payhub-activities/blob/main/src/main/java/it/gov/pagopa/payhub/activities/service/classifications/TransferClassificationService.java#L45 for an example of its application

(group these classes on nested packages, separated between DebtPosition and PaymentOptions statuses)

we could have the following structure:

service/statusalign/
** DebtPositionHierarchyStatusAlignerService
** DebtPositionInnerStatusAlignerService (no interface for this, because it should not be call outside)
** PaymentOptionInnerStatusAlignerService (no interface for this, because it should not be call outside)
*** debtposition/
**** DebtPositionStatusChecker (common interface implemented by all classes on this package, eg: DebtPositionToSyncStatusChecker)
*** paymentoption/
**** PaymentOptionStatusChecker (common interface implemented by all classes on this package)

Each DebtPositionStatusChecker and PaymentOptionStatusChecker should have the @ordered annotation configured.

DebtPositionInnerStatusAlignerService and PaymentOptionInnerStatusAlignerService should accept the firstNull returned object

We should write 2 SpringBoot Test in order to check if the configuration of @order is rigth:

DebtPositionStatusCheckerTest

PaymentOptionStatusChecker
These SpringBootTest will just autowire the List<.*StatusChecker> and the @test will verify the order
(Possibly, let's configure the test to scan just the service/statusalign/ package)

antonioT90 · 2025-01-16T13:01:16Z

src/main/java/it/gov/pagopa/pu/debtpositions/service/DebtPositionServiceImpl.java

+    debtPosition.getPaymentOptions().forEach(this::updatePaymentOptionStatus);
+
+    updateDebtPositionStatus(debtPosition);


Each DebtPositionStatusChecker and PaymentOptionStatusChecker should have the @ordered annotation configured.

DebtPositionInnerStatusAlignerService and PaymentOptionInnerStatusAlignerService should accept the firstNull returned object

We should write 2 SpringBoot Test in order to check if the configuration of @order is rigth:

DebtPositionStatusCheckerTest

PaymentOptionStatusChecker
These SpringBootTest will just autowire the List<.*StatusChecker> and the @test will verify the order
(Possibly, let's configure the test to scan just the service/statusalign/ package)

…ync-status # Conflicts: # openapi/generated.openapi.json

sonarqubecloud · 2025-01-20T12:12:11Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

antonioT90 · 2025-01-20T13:53:43Z

openapi/p4pa-debt-position.openapi.yaml

+        - INVALID
+        - EXPIRED
+        - UNPAID
+    SyncStatusDTO:


Suggested change

SyncStatusDTO:

SyncStatusUpdateRequestDTO:

antonioT90 · 2025-01-20T13:53:55Z

openapi/p4pa-debt-position.openapi.yaml

+        content:
+          application/json:
+            schema:
+              $ref: "#/components/schemas/SyncStatusDTO"


Suggested change

$ref: "#/components/schemas/SyncStatusDTO"

$ref: "#/components/schemas/SyncStatusUpdateRequestDTO"

antonioT90 · 2025-01-20T14:00:00Z

...gopa/pu/debtpositions/service/statusalign/DebtPositionHierarchyStatusAlignerServiceImpl.java

+        .filter(installment -> TO_SYNC.equals(installment.getStatus()))
+        .filter(installment -> syncStatusDTO.containsKey(installment.getIud()))


can you put just 1 filter operation in order to:

log as ERROR a TO_SYNC installment not present in the map

log as ERROR an installment in the map, but without TO_SYNC status
The filter will return the AND between the actually configured conditions

antonioT90 · 2025-01-20T14:00:41Z

...gopa/pu/debtpositions/service/statusalign/DebtPositionHierarchyStatusAlignerServiceImpl.java

+          IudSyncStatusUpdateDTO updateDTO = syncStatusDTO.get(installment.getIud());
+
+          try {
+            InstallmentStatus newStatus = InstallmentStatus.valueOf(updateDTO.getNewStatus().toUpperCase());


Is this conversion still necessary? aren't they the same enum type?

No, in the map it's still a string, but i can change it

antonioT90 · 2025-01-20T14:01:20Z

...gopa/pu/debtpositions/service/statusalign/DebtPositionHierarchyStatusAlignerServiceImpl.java

+          } catch (IllegalArgumentException e) {
+            throw new InvalidStatusException("Invalid status: " + updateDTO.getNewStatus());
+          }


if same enum, this catch should not be necessary

antonioT90 · 2025-01-20T14:16:21Z

src/main/java/it/gov/pagopa/pu/debtpositions/exception/custom/InvalidStatusException.java

if the enums are the same, this exception could not more be used

i still use it in the class PaymentOptionStatusChecker at the else condition:

else { throw new InvalidStatusException("Unable to determine status for PaymentOption"); }

src/test/java/it/gov/pagopa/pu/debtpositions/exception/DebtPositionExceptionHandlerTest.java

antonioT90 · 2025-01-20T14:19:36Z

src/main/java/it/gov/pagopa/pu/debtpositions/exception/DebtPositionExceptionHandler.java

if InvalidStatusException could be removed, let's not delete this, but let's handle some other exception here, (eg MethodArgumentNotValidException)

antonioT90 · 2025-01-20T14:25:33Z

src/main/java/it/gov/pagopa/pu/debtpositions/service/statusalign/StatusRulesHandler.java

+    repositoryUpdater.accept(entity, newStatus);
+  }
+
+  public boolean isToSync(List<E> statusList) {


can you rename all the parameters statusList into childrenStatuses or childrenStatusList

antonioT90 · 2025-01-20T14:37:40Z

src/main/java/it/gov/pagopa/pu/debtpositions/service/statusalign/StatusRulesHandler.java

+    return allMatchOrEmpty(statusList, status -> status.equals(unpaidStatus)) ||
+      (allMatchOrEmpty(statusList, status -> status.equals(cancelledStatus) || status.equals(unpaidStatus)) &&
+        statusList.contains(unpaidStatus));


This is not efficient, you are iterating statusList for each status!

what you should implement is:
statusList.contains(unpaidStatus) && statusList.stream().allMatch(status -> status.equals(cancelledStatus) || status.equals(unpaidStatus));

you could generalize it the following way:

private allMatch(List<E> statusList, E requiredState, Set<E> allowedStatuses){ statusList.contains(requiredState) && statusList.stream().allMatch(status -> requiredState.equals(status) || allowedStatuses.contains(status)); }

LarissaASLeite added 4 commits January 14, 2025 17:58

P4ADEV-1744-add-api-finalize-sync-status

38ce7ff

P4ADEV-1744-add-api-finalize-sync-status

40e3008

P4ADEV-1744-add-api-finalize-sync-status

0d13047

LarissaASLeite requested review from antocalo, Benedetta-fabbri, RiccardoGiuliani, oleksiybozhykntt, ElisKina-dev and a team as code owners January 15, 2025 15:36

LarissaASLeite and others added 7 commits January 15, 2025 16:44

P4ADEV-1744-add-api-finalize-sync-status

06e1503

P4ADEV-1744-add-api-finalize-sync-status

6679f7c

P4ADEV-1744-add-api-finalize-sync-status

b9a0275

fix tests

925fe19

fix tests

14da8f9

P4ADEV-1744-add-api-finalize-sync-status

cacad37

P4ADEV-1744-add-api-finalize-sync-status

7ac2824

antonioT90 requested changes Jan 16, 2025

View reviewed changes

LarissaASLeite added 6 commits January 16, 2025 16:44

P4ADEV-1744-add-api-finalize-sync-status

a4c0aea

P4ADEV-1744-add-api-finalize-sync-status

847690d

Merge branch 'refs/heads/develop' into P4ADEV-1744-add-api-finalize-s…

0de05e3

…ync-status # Conflicts: # openapi/generated.openapi.json

P4ADEV-1744-add-api-finalize-sync-status

534ec97

P4ADEV-1744-add-api-finalize-sync-status

c35340a

P4ADEV-1744-add-api-finalize-sync-status

7c7118b

antonioT90 requested changes Jan 20, 2025

View reviewed changes

add-openapi for finalizeSyncStatus

0967ccd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: P4ADEV-1744-add-api-finalize-sync-status #15

feat: P4ADEV-1744-add-api-finalize-sync-status #15

LarissaASLeite commented Jan 15, 2025

antonioT90 Jan 16, 2025

antonioT90 Jan 16, 2025

antonioT90 Jan 16, 2025

antonioT90 Jan 16, 2025

antonioT90 Jan 16, 2025

sonarqubecloud bot commented Jan 20, 2025

antonioT90 Jan 20, 2025

antonioT90 Jan 20, 2025

antonioT90 Jan 20, 2025

antonioT90 Jan 20, 2025

LarissaASLeite Jan 20, 2025

antonioT90 Jan 20, 2025

antonioT90 Jan 20, 2025

LarissaASLeite Jan 20, 2025

antonioT90 Jan 20, 2025

antonioT90 Jan 20, 2025

antonioT90 Jan 20, 2025

		debtPosition.getPaymentOptions().forEach(this::updatePaymentOptionStatus);

		updateDebtPositionStatus(debtPosition);

	$ref: "#/components/schemas/SyncStatusDTO"
	$ref: "#/components/schemas/SyncStatusUpdateRequestDTO"

		.filter(installment -> TO_SYNC.equals(installment.getStatus()))
		.filter(installment -> syncStatusDTO.containsKey(installment.getIud()))

feat: P4ADEV-1744-add-api-finalize-sync-status #15

Are you sure you want to change the base?

feat: P4ADEV-1744-add-api-finalize-sync-status #15

Conversation

LarissaASLeite commented Jan 15, 2025

Description

List of Changes

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 20, 2025

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment