Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

LarissaASLeite
Copy link
Collaborator

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 Outdated Show resolved Hide resolved
@EntityGraph(value = "completeDebtPosition")
DebtPosition findOneWithAllDataByDebtPositionId(Long debtPositionId);

@Modifying
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@Modifying
@RestResource(exported = false)
@Transactional
@Modifying


@Modifying
@Query("UPDATE DebtPosition d SET d.status = :status WHERE d.debtPositionId = :debtPositionId")
void updateStatus(@Param("debtPositionId") Long debtPositionId, @Param("status") String status);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
void updateStatus(@Param("debtPositionId") Long debtPositionId, @Param("status") String status);
void updateStatus(@Param("debtPositionId") Long debtPositionId, @Param("status") DebtPositionStatus status);

import org.springframework.data.rest.core.annotation.RepositoryRestResource;

@RepositoryRestResource(path = "installments")
public interface InstallmentNoPIIRepository extends JpaRepository<InstallmentNoPII, Long> {

InstallmentNoPII findByIudAndIupdPagopaAndStatus(String iud, String iupdPagopa, String status);

@Modifying
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@Modifying
@RestResource(exported = false)
@Transactional
@Modifying


@Modifying
@Query("UPDATE InstallmentNoPII i SET i.status = :status WHERE i.installmentId = :installmentId")
void updateStatus(@Param("installmentId") Long installmentId, @Param("status") String status);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
void updateStatus(@Param("installmentId") Long installmentId, @Param("status") String status);
void updateStatus(@Param("installmentId") Long installmentId, @Param("status") InstallmentStatus status);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should also set the iupdPagopa

import org.springframework.data.rest.core.annotation.RepositoryRestResource;

@RepositoryRestResource(path = "payment-options")
public interface PaymentOptionRepository extends JpaRepository<PaymentOption,Long> {

@Modifying
@Query("UPDATE PaymentOption p SET p.status = :status WHERE p.paymentOptionId = :paymentOptionId")
void updateStatus(@Param("paymentOptionId") Long paymentOptionId, @Param("status") String status);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
void updateStatus(@Param("paymentOptionId") Long paymentOptionId, @Param("status") String status);
void updateStatus(@Param("paymentOptionId") Long paymentOptionId, @Param("status") PaymentOptionStatus status);

@@ -53,5 +63,51 @@ public void saveDebtPosition(DebtPositionDTO debtPositionDTO) {
});
});
}

@Override
public void finalizeSyncStatus(Long debtPositionId, Map<String, IudSyncStatusUpdateDTO> syncStatusDTO) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move the method on a separate Service class

paymentOption.getInstallments().stream()
.filter(installment -> TO_SYNC.name().equals(installment.getStatus()))
.filter(installment -> syncStatusDTO.containsKey(installment.getIud()))
.filter(installment -> installment.getIupdPagopa().equals(syncStatusDTO.get(installment.getIud()).getIupdPagopa()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the iupdPagopa on the request represent the value to store on the db! not a field to check if match ;)
(we are storing the nodo synchronization result, whose will provide us the pagopaIds

Comment on lines +85 to +87
debtPosition.getPaymentOptions().forEach(this::updatePaymentOptionStatus);

updateDebtPositionStatus(debtPosition);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment on lines +85 to +87
debtPosition.getPaymentOptions().forEach(this::updatePaymentOptionStatus);

updateDebtPositionStatus(debtPosition);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants