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

Spring25-v2 #121

Merged
merged 19 commits into from
Jan 2, 2025
Merged

Spring25-v2 #121

merged 19 commits into from
Jan 2, 2025

Conversation

FabrizioMoggio
Copy link
Collaborator

@FabrizioMoggio FabrizioMoggio commented Dec 17, 2024

V2 PR to fulfill Spring25 scope: #114

  • update of the Authorization and authentication section in Info. description
  • alignment of section "Identifying the phone number from the access token" in info.description with Commonalities 0.5.0
  • partial update of the RCL
  • update the Readme.md file
  • Remove unused Error codes
  • Alignments in Errors
  • Phone Number retrieval
  • Phone Number retrieval and Testing
  • Align API-Testing with Error codes
  • 422 error code
  • 50x error too obvious
  • Test Case: login_hint

What type of PR is this?

  • documentation
  • correction

What this PR does / why we need it:

To be aligned with Commonalities v0.5.0
To update the code according to the outcomes of the implementation of v0.2.0

Which issue(s) this PR fixes:

#114
#120
#122
#126 (not a complete fix)
#127
#129
#119
#130
#131
#123
#132
#133
#134

Authorization and authentication. Info. description update:camaraproject#120
Phone number from access token: camaraproject#122
First step toward the RCL file for the Spring25 release. 

The "Enhanced API test cases & documentation" links will be included in a following PR.
To reference to v1.0.0 and Release tag R2.0
@FabrizioMoggio FabrizioMoggio mentioned this pull request Dec 18, 2024
Enhancements and Alignments in Errors: camaraproject#119
removed example text in error code 422
This was referenced Dec 19, 2024
Phone Number retrieval and Testing: camaraproject#131

This Commit doesn't fix the other error codes. this will be addressed by: camaraproject#123
Align API-Testing with Error codes: camaraproject#123
@FabrizioMoggio
Copy link
Collaborator Author

FabrizioMoggio commented Jan 2, 2025

Dear @bigludo7 and @chinaunicomyangfan we must merge the PR before mid January. when this PR will be merged I will create a new one moving the release number from WIP to V1.0.0-rc.1 (hoping that GSMA certifies the implementation developed by @StefanoFalsetto-CKHIOD). To meet M3.

@bigludo7
Copy link
Collaborator

bigludo7 commented Jan 2, 2025

Hello dear @FabrizioMoggio
Thanks for this !
Comments to your consideration:

  • We should remove 500 & 503 errors from the yaml as stated here: Enhancements and Alignments in Errors Commonalities#321 (comment) - Indeed we do not have specificities from our API requiring them
  • As in the request we use strictly the phoneNumber and not the device structure (allowing use of ip or externalIdentifier) we should never have the case for 422 UNSUPPORTED_IDENTIFIER. We will not be able to provide a test case. WDYT?
  • IDENTIFIER_MISMATCH value in your enum line 531 must be also removed I guess

@bigludo7
Copy link
Collaborator

bigludo7 commented Jan 2, 2025

Another one @FabrizioMoggio where I would like to get your view (not asking for change but to get your perspective).

In the TC you use the formulation "login_hint" is set to a properly formatted phone number - It closes the use of the 3-legged to CIBA. Should we flex this and use instead The header "Authorization" is set to a valid access token identifying a phoneNumber formulation?

Perhaps the use of CIBA is so evident for this API?

@FabrizioMoggio
Copy link
Collaborator Author

FabrizioMoggio commented Jan 2, 2025

Hello dear @FabrizioMoggio Thanks for this ! Comments to your consideration:

  • We should remove 500 & 503 errors from the yaml as stated here: Enhancements and Alignments in Errors Commonalities#321 (comment) - Indeed we do not have specificities from our API requiring them
  • As in the request we use strictly the phoneNumber and not the device structure (allowing use of ip or externalIdentifier) we should never have the case for 422 UNSUPPORTED_IDENTIFIER. We will not be able to provide a test case. WDYT?
  • IDENTIFIER_MISMATCH value in your enum line 531 must be also removed I guess
  • In my understanding the reason for removing 500 and 503 is that they are common and generic errors that should be implemented but not documented in the YAML because too obvious. Is this correct? So I agree.
  • About 422 UNSUPPORTED_IDENTIFIER, I agree.
  • Bout IDENTIFIER_MISMATCH, I agree

what about 504? I think that, for the same reason we have for 500 and 504, we should also remove 504.

@FabrizioMoggio
Copy link
Collaborator Author

Another one @FabrizioMoggio where I would like to get your view (not asking for change but to get your perspective).

In the TC you use the formulation "login_hint" is set to a properly formatted phone number - It closes the use of the 3-legged to CIBA. Should we flex this and use instead The header "Authorization" is set to a valid access token identifying a phoneNumber formulation?

Perhaps the use of CIBA is so evident for this API?

You are totally right, login_hint is too specific, I also prefer a more generic phrasing as you suggested.

This was referenced Jan 2, 2025
@bigludo7
Copy link
Collaborator

bigludo7 commented Jan 2, 2025

Thanks @FabrizioMoggio

I share your understanding for 5xx errors.
--> Yes we should also remove 504

FabrizioMoggio and others added 5 commits January 2, 2025 14:37
50x error too obvious: camaraproject#132
422 error code: camaraproject#133
Rephrased obtaining phone number by token to  "The header "Authorization" is set to a valid access token identifying a phoneNumber"
Copy link
Collaborator Author

@FabrizioMoggio FabrizioMoggio left a comment

Choose a reason for hiding this comment

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

"phoneNumber" is the name of the parameter in the API. The header identifies the "phone number". Isn't it? So I suggest to modify the sentence as:

The header "Authorization" is set to a valid access token identifying a phone number.

@bigludo7
Copy link
Collaborator

bigludo7 commented Jan 2, 2025

@FabrizioMoggio I've update the feature files to replace the login_hint by the The header "Authorization" is set to a valid access token identifying a phoneNumber

@bigludo7
Copy link
Collaborator

bigludo7 commented Jan 2, 2025

"phoneNumber" is the name of the parameter in the API. The header identifies the "phone number". Isn't it? So I suggest to modify the sentence as:

The header "Authorization" is set to a valid access token identifying a phone number.

Yes you're right !
I update the files

change phoneNumber to phone number ;)
change phoneNumber to phone number ;)
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.

Changed phoneNumber to phone number ;)

Copy link
Collaborator Author

@FabrizioMoggio FabrizioMoggio 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

@FabrizioMoggio FabrizioMoggio merged commit 7d0acd4 into camaraproject:main Jan 2, 2025
2 checks passed
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.

2 participants