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

DeviceVisitLocation initial version yaml #22

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

chinaunicomyangfan
Copy link

What type of PR is this?

  • enhancement/feature

What this PR does / why we need it:

First version upload of DeviceVisitLocation YAML file

Which issue(s) this PR fixes:

Fixes #21

@FabrizioMoggio
Copy link

FabrizioMoggio commented Dec 20, 2024

The API does not follow the Guidelines for the usage of the optional Device parameter with 3-Legs: https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md#appendix-a-normative-infodescription-template-for-when-user-identification-can-be-from-either-an-access-token-or-explicit-identifier

The parameter Device is optional: it must be used with 2-Legs and should not be used with 3-Legs.

This is the required text from commonalities:

Identifying the device | phone number from the access token

This API requires the API consumer to identify a device | phone number as the subject of the API as follows:

  • When the API is invoked using a two-legged access token, the subject will be identified from the optional device object | phoneNumber field, which therefore MUST be provided.
  • When a three-legged access token is used however, this optional identifier MUST NOT be provided, as the subject will be uniquely identified from the access token.

This approach simplifies API usage for API consumers using a three-legged access token to invoke the API by relying on the information that is associated with the access token and was identified during the authentication process.

Error handling:

  • If the subject cannot be identified from the access token and the optional device object | phoneNumber field is not included in the request, then the server will return an error with the 422 MISSING_IDENTIFIER error code.

  • If the subject can be identified from the access token and the optional device object | phoneNumber field is also included in the request, then the server will return an error with the 422 UNNECESSARY_IDENTIFIER error code. This will be the case even if the same device | phone number is identified by these two methods, as the server is unable to make this comparison.

@FabrizioMoggio
Copy link

Th Introduction chapter is misleading in my opinion. It implies that the API returns the visited location of a device at a certain time, and the visited location is returned in the form of a postal code or a zip code. Instead, in my understanding the API gets as an input a postal code or a zip code returning a score. Is my understanding wrong?

@chinaunicomyangfan
Copy link
Author

Th Introduction chapter is misleading in my opinion. It implies that the API returns the visited location of a device at a certain time, and the visited location is returned in the form of a postal code or a zip code. Instead, in my understanding the API gets as an input a postal code or a zip code returning a score. Is my understanding wrong?

@FabrizioMoggio Sorry, I uploaded the wrong version. I have re uploaded the correct one. Please review it again. Thank you.

@FabrizioMoggio
Copy link

Dear @chinaunicomyangfan my comment here: #22 (comment) is not yet addressed :-)

@chinaunicomyangfan
Copy link
Author

chinaunicomyangfan commented Dec 23, 2024

Dear @chinaunicomyangfan my comment here: #22 (comment) is not yet addressed :-)

@FabrizioMoggio Thank you for your suggestion. I'm not sure if my understanding is correct. Are you saying that I should add two error code explanations(422 MISSING_IDENTIFIER and 422 UNNECESSARY_IDENTIFIER ) to the API according to Camara's API design specifications? My implementation referred to the definition of most frequent location API. I think our two APIs should use the same authentication method, but I didn't see any explanation about these two error codes in it. What's the reason for this?

@FabrizioMoggio
Copy link

Dear @chinaunicomyangfan my comment here: #22 (comment) is not yet addressed :-)

@FabrizioMoggio Thank you for your suggestion. I'm not sure if my understanding is correct. Are you saying that I should add two error code explanations(422 MISSING_IDENTIFIER and 422 UNNECESSARY_IDENTIFIER ) to the API according to Camara's API design specifications? My implementation referred to the definition of most frequent location API. I think our two APIs should use the same authentication method, but I didn't see any explanation about these two error codes in it. What's the reason for this?

Dear @chinaunicomyangfan I'm just referring to the last CAMARA Guidelines for the Spring25 release :-) Before (Fall24) the text and the API behavior was different. Our API is aligned with Fall24 and not Spring25. My understanding is that, for this new release, we need to add the new text and those error codes (422) to handle two possible implementation of the API (with 2Legs or 3Legs).

@chinaunicomyangfan chinaunicomyangfan changed the title Add files via upload DeviceVisitLocation initial version yaml Jan 22, 2025
@chinaunicomyangfan
Copy link
Author

chinaunicomyangfan commented Jan 22, 2025

@FabrizioMoggio I added a 422 error code and related description based on Commonality v0.5, but encountered a megalint error when submitting, but there is no detailed information about the error. Do you know how to solve this?
image

@FabrizioMoggio
Copy link

FabrizioMoggio commented Jan 22, 2025

@chinaunicomyangfan the errors are listed below. I tried to fix the first one (line 135:12) as a test. It is my first time fixing a code directly upon a PR created by someone else :-).

@FabrizioMoggio
Copy link

FabrizioMoggio commented Jan 22, 2025

It worked, that specific error was fixed. I can try to also have a look on the other errors. Anyway there is an overall action to do, the lines length can not exceed 80 characters.

....... or maybe this is not a rule anymore, I noticed that MegaLinter doesn't provide any error for the lines length

@FabrizioMoggio
Copy link

Anyway, I'm not sure to have got your question right, you are referring to error 422 but I don't see any error related to 422. Am I missing something?

value:
status: 422
code: UNNECESSARY_IDENTIFIER
message: The device is already identified by the access token.
Copy link
Author

Choose a reason for hiding this comment

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

Anyway, I'm not sure to have got your question right, you are referring to error 422 but I don't see any error related to 422. Am I missing something?
@FabrizioMoggio This is the definition of error code 422. Please review it

@chinaunicomyangfan
Copy link
Author

It worked, that specific error was fixed. I can try to also have a look on the other errors. Anyway there is an overall action to do, the lines length can not exceed 80 characters.

....... or maybe this is not a rule anymore, I noticed that MegaLinter doesn't provide any error for the lines length

@FabrizioMoggio Thank you for your help. I have reviewed the MegaLinter report again and submitted a new commit, and now I should have fixed all the errors.

Copy link

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

@chinaunicomyangfan
Copy link
Author

The Generic errors are not aligned with COMMON_YAML: https://github.com/eric-murray/Commonalities/blob/d0eb0652080e35be54c13ac8f9c92775b7570ec2/artifacts/CAMARA_common.yaml

The one in your link should be commonalities v0.5.
I used this commonalities yaml: https://github.com/camaraproject/Commonalities/blob/main/artifacts/CAMARA_common.yaml , Which version is more suitable for me? I understand that you link is a stable version, right?

@FabrizioMoggio
Copy link

FabrizioMoggio commented Jan 23, 2025

The Generic errors are not aligned with COMMON_YAML: https://github.com/eric-murray/Commonalities/blob/d0eb0652080e35be54c13ac8f9c92775b7570ec2/artifacts/CAMARA_common.yaml

The one in your link should be commonalities v0.5. I used this commonalities yaml: https://github.com/camaraproject/Commonalities/blob/main/artifacts/CAMARA_common.yaml , Which version is more suitable for me? I understand that you link is a stable version, right?

Yes I was referring to Spring25, Sorry I referenced the file from the PR. The file to consider (Commonalities 5.0), in my understanding, should be: https://github.com/camaraproject/Commonalities/blob/r2.1/artifacts/CAMARA_common.yaml

The source of this information is: https://lf-camaraproject.atlassian.net/wiki/spaces/CAM/pages/14560849/Meta-release+Spring25#Commonalities-%26-ICM

@chinaunicomyangfan
Copy link
Author

https://github.com/camaraproject/Commonalities/blob/r2.1/artifacts/CAMARA_common.yaml

The description of the 422 error code in this file seems to be consistent with the YAML I uploaded. Can you help confirm it?

Copy link

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

The errors, and specifically 422, are aligned with Commonalities 5.0.

@fernandopradocabrillo
Copy link
Contributor

fernandopradocabrillo commented Jan 23, 2025

Hi @FabrizioMoggio, @chinaunicomyangfan
This PR shouldn't be merge until PR #23 is approved by Release Management and merged.

Additionally, if you want this API to be part of Spring25 meta, you'll need to do all related requirements and preparations for it by tomorrow since it is the last day of M3 milestone.

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.

First version upload of DeviceVisitLocation YAML file
3 participants