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

Error Model aligment with Commonalities Meta-Release Fall24 #154

Closed
PedroDiez opened this issue May 7, 2024 · 7 comments · Fixed by #152
Closed

Error Model aligment with Commonalities Meta-Release Fall24 #154

PedroDiez opened this issue May 7, 2024 · 7 comments · Fixed by #152
Labels
enhancement New feature or request

Comments

@PedroDiez
Copy link
Collaborator

Problem description
In the Context of Commonalities/Issue 180 and Commonalities/Issue 127. An error model alingment is required.

@bigludo7 also commented in 17th April meeting that could make sense to have some errors being addressed with 422 exception instead of 403 one.

Based on that aligment, it would be required to make proper adaptations in both carrier billing payment and carrier billing refund functionality.

Possible evolution
Pending to be proposed

Alternative solution
N/A

Additional context
N/A

@PedroDiez
Copy link
Collaborator Author

Please find attached error model aligment porposal to discuss today (based on Commonalities Aligment PR#213)

CB_Error_Model_Alignment_Proposal.xlsx

cc @bigludo7, @rartych, @eragaji

@PedroDiez
Copy link
Collaborator Author

Hi,

@bigludo7, @rartych please can you provide feedback? Based on that we can try to reach a conclusion about updates in error excepctions

@bigludo7
Copy link
Collaborator

Hello @PedroDiez
Sorry to be late on this.
Took a look.

Only one comment :
I'm bit skeptical of the propose use of 429 for CARRIER_BILLING.USER_AMOUNT_THRESHOLD_OVERPASSED. For me

  • 429 is used if we got too many 'technical' request from a consumer. I'm not thinking of a business error.
  • Here we have a business error (for my understanding) - as a carrier billing operator we do not allow consumer to spend more than x euros per month on Carrier billing payment. In this case I will probably more expect a 422 (and the code CARRIER_BILLING.USER_AMOUNT_THRESHOLD_OVERPASSED is perfect).

@PedroDiez
Copy link
Collaborator Author

Hello @PedroDiez Sorry to be late on this. Took a look.

Only one comment : I'm bit skeptical of the propose use of 429 for CARRIER_BILLING.USER_AMOUNT_THRESHOLD_OVERPASSED. For me

  • 429 is used if we got too many 'technical' request from a consumer. I'm not thinking of a business error.
  • Here we have a business error (for my understanding) - as a carrier billing operator we do not allow consumer to spend more than x euros per month on Carrier billing payment. In this case I will probably more expect a 422 (and the code CARRIER_BILLING.USER_AMOUNT_THRESHOLD_OVERPASSED is perfect).

Understood @bigludo7, makes sense to use 422, also being compliant with the guidelines. For the rest of the proposal, I guess you seems right, doesn't?

@bigludo7
Copy link
Collaborator

@PedroDiez

For the rest of the proposal, I guess you seems right, doesn't?

Yes the rest of the proposal is fine for me :)

@PedroDiez
Copy link
Collaborator Author

Will wait EOD this Thursday to check other comments and moving forward with the updates

@PedroDiez
Copy link
Collaborator Author

Already covered in #152

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
Development

Successfully merging a pull request may close this issue.

2 participants