-
Notifications
You must be signed in to change notification settings - Fork 1
[PIDM-292] feat: add fields to getPositionByIUPD API #295
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
base: main
Are you sure you want to change the base?
Conversation
add class to host the new fields extending the base response; change interface return type to new response; add new fields mapping to controller method; add new response to openapi (excluding v3); change API response schema on openapi specs (excluding v3);
This pull request does not contain a valid label. Please add one of the following labels: |
add missing debtor fields to response;
update openapi to avoid merge conflicts
fix formatter job
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…/pagopa/pagopa-debt-position into PIDM-292-add-api-response-fields
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a new model? Can we add the fields in the PaymentPositionModelBaseResponse
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about at first but then I noticed that that the PaymentPositionModelBaseResponse
is being used as a response model for other APIs. The task was to change one response to one specific API, so if I added the new field to the base model instead of extending it I would have added the new fields to API responses that were not asked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other API is getDebtPositionByIUV
, it can be changed but the DWISP GPD client must be updated.
PaymentPositionModelBaseResponse basePaymentPositionResponse = | ||
ObjectMapperUtils.map(paymentPosition, PaymentPositionModelBaseResponse.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since PaymentPositionModelEnhancedResponse
extends PaymentPositionModelBaseResponse
you could use directly the enhanced version here avoiding using reflection below (the copyProperties) to clone objects and then leave the set additional fields part.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, the idea of a model enhanced is reasonable. I suggest evaluating @pietro-tota's advice on the use of copyProperties
and consider updating the API on pagopa-infra.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other API is getDebtPositionByIUV
, it can be changed but the DWISP GPD client must be updated.
List of Changes
Motivation and Context
This change was requested to allow coherence checks on payment positions when comparing MYPAY with GPD
How Has This Been Tested?
/organizations/{organizationfiscalcode}/debtpositions/{iupd}
Screenshots (if appropriate):
Response:
Types of changes
Checklist: