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

refund_proposal_and_payment_aligment #152

Merged
merged 25 commits into from
Jul 3, 2024

Conversation

PedroDiez
Copy link
Collaborator

@PedroDiez PedroDiez commented Apr 12, 2024

What type of PR is this?

  • enhancement/feature

What this PR does / why we need it:

This PR covers proposal for refund functionality.

  • It provides a design for both total and partial refunds, allowing for asynchronous model in business proceesing
  • Events Model covered
  • Exceptions Model covered
  • Some alignment within commonalities included
  • Considering remainingAmount not refunded in Payment resource (i.e. API)
  • Considering amount (minimum:0.001) and taxAmount (minimum: 0) to be non-negative
  • PhoneNumber aligment in Carrier billing. Exceptions added (draft input to check the whole model after).
  • merchantIdentifier model, 422 exceptions and more description for refund
  • polimorphic model for refund
  • new proposal for remaining payment amount

Which issue(s) this PR fixes:

Initial Proposal - Baseline for:

Fixes #104
Fixes #158

2024-05-15

Fixes #153 (commit 7b224ea)
Rename status to paymentStatus and refundStatus. Also cover amount (minimum:0.001) and taxAmount (minimum: 0) to be non-negative in refunds (commit d12b4cf)

2024-05-24/25

PhoneNumber aligment in Carrier Billing (commit bb99437)
merchantIdentifier model in Carrier Billing Refund, 422 Exceptions, more description (commit 659b2aa)
polymorphic refund model (commit 08837a8)
new proposal for remaining payment amount (commit efe2444)

2024-06-10/11/12

Alignment with Commonalities PR#214 (x-camara-commonalities header) and Notifications CloudEvent Model (commit 4d03b24)
Inclusion of paymentItemId concept ( commit 9c723cd)
fixing some typos and missings in error exceptions (commits e2be576, 8d079e3 and a647dea)

2024-06-19

Description enhancements and description.info alignment with Commonalities ICM output (# Authorization and authentication section) (commit 85a008a)
Semantic enhancements (denied refund in ASYNC context, remainingAmount behaviour, based on Ludovic comments/suggestions) (commit 9377d6c)

2024-06-21
Fixes #154

Commonalities alignment errors (#154) and endpoint versions (commit 8c230ba)

2024-06-27

Fix typo un refund-completed event (paymentDate should read refundDate) (commit 8cfe775)

Special notes for reviewers:

Copy link

github-actions bot commented Apr 12, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ ACTION actionlint 2 0 0.03s
✅ OPENAPI spectral 2 0 3.95s
✅ REPOSITORY git_diff yes no 0.05s
✅ REPOSITORY secretlint yes no 0.95s
✅ YAML yamllint 2 0 0.9s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by OX Security

@PedroDiez PedroDiez changed the title (refund_proposal_and_payment_aligment refund_proposal_and_payment_aligment Apr 12, 2024
@PedroDiez PedroDiez added the enhancement New feature or request label Apr 12, 2024
Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

Hello @PedroDiez

I begun the review and few comments to share with you. Most of this points are not request for change but question I got when I did this review.

  • Are we sure to manage refund_in_progress, totally_refunded, partially_refunded as TransactionOperationStatus ? my point is that a payment could have been Succeeded and then refunded. Both state are valid. Need to be sure we did not need to tackle that in a specific refundStatus.
  • I'm still not sure if refund should be managed in a separate yaml or same yaml that payment. Have you @PedroDiez good rationale to make them separate?
  • Are we sure that refundAmount should be mandatory ? if type is total this could be source of unnecessary error (what about a request with type:total and amount inconsistent btw the payment and the refund?).
  • If requester fill refundDetails do we need to make check vs payment item? I guess yes.
  • In payment we require a referenceCode in the request - do we need one here for the refund?

@bigludo7
Copy link
Collaborator

One quick question.... what is the correlation between refundDetails.refundItem and paymentDetail.paymentItem ? I guess we expect a strong correlation meaning that if I have 3 payment item I could have a refund only for the second item right?
As such in order to have this correlation I'm wondering if we should not add a paymentItem.id in paymentItem and refer this in refundItem.
Perhaps this is 'too much' but got this comment in looking in refundItem and how to use it.

Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

Hi @PedroDiez
We're almost there. We have changed the authorization sentences in ICM for the documentation.
also we have probably to have a rule for the remaining amount regarding processing refund.
nothing big here.

code/API_definitions/carrier_billing.yaml Outdated Show resolved Hide resolved
code/API_definitions/carrier_billing_refund.yaml Outdated Show resolved Hide resolved
code/API_definitions/carrier_billing_refund.yaml Outdated Show resolved Hide resolved
code/API_definitions/carrier_billing_refund.yaml Outdated Show resolved Hide resolved
code/API_definitions/carrier_billing_refund.yaml Outdated Show resolved Hide resolved
code/API_definitions/carrier_billing_refund.yaml Outdated Show resolved Hide resolved
@PedroDiez PedroDiez requested a review from bigludo7 June 19, 2024 15:24
bigludo7
bigludo7 previously approved these changes Jun 19, 2024
Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

LGTM/

Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

LGTM/

@PedroDiez
Copy link
Collaborator Author

Merging it as commenting in Meeting minutes. Initial wip proposal

@PedroDiez PedroDiez merged commit 7d73fd0 into main Jul 3, 2024
1 check passed
@PedroDiez PedroDiez deleted the refund_proposal_and_payment_aligment branch October 10, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants