DeviceVisitLocation initial version yaml [don't merge, see comment]#22
DeviceVisitLocation initial version yaml [don't merge, see comment]#22chinaunicomyangfan wants to merge 10 commits intocamaraproject:mainfrom
Conversation
|
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 tokenThis API requires the API consumer to identify a device | phone number as the subject of the API as follows:
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:
|
|
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. |
|
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). |
Change errorcode format add errorcode
|
@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? |
|
@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 :-). |
|
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 |
137:49
|
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. |
There was a problem hiding this comment.
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
@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. |
FabrizioMoggio
left a comment
There was a problem hiding this comment.
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. |
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 |
The description of the 422 error code in this file seems to be consistent with the YAML I uploaded. Can you help confirm it? |
FabrizioMoggio
left a comment
There was a problem hiding this comment.
The errors, and specifically 422, are aligned with Commonalities 5.0.
|
Hi @FabrizioMoggio, @chinaunicomyangfan 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. |
Hi @fernandopradocabrillo ,please check if I can merge the PR now?I want to make a meta release on Fall25. |
|
Hi @chinaunicomyangfan I'm sorry I didn't see this message earlier. I'm preparing the PR for the release candidate of Location Insights. Here is the changes needed to include for Fall25: https://lf-camaraproject.atlassian.net/wiki/spaces/CAM/pages/132218881/Analysis+of+Commonalities+0.6.0-rc.1+changes You can take most of them from other APIs that have already applied them. I suggest the DeviceLocation, DeviceRoaming or even the most frequent location changes that I'm going to upload. I'm happy to approve this PR once everything is aligned and include it in the release candidate PR cc: @FabrizioMoggio |
There was a problem hiding this comment.
@chinaunicomyangfan I'm aware that you are planning to get this API into the Fall25 release. But as far as I can see there are several points open for this API, which are required to be part of a release of the release-candidate version, especially are the mandatory test definitions are missing.
These missing prerequisites for the new API device-visit-location and the necessary changes which will be raised during the release review are creating a risk for the M3 release of most-frequent-location which is only an update of an existing initial API.
My proposal is therefore to split the repository LocationInsights NOW, even short-term before the M3 release. These are the steps (subject to approval by APIBacklog or TSC):
-
Create issue in API Backlog for short-term approval to split the repository LocationInsights in the following way:
- Create a new Sandbox API Repository for "DeviceVisitLocation" asap
- Renaming the existing repository "LocationInsights" into "MostFrequentLocation"
- Both repositories will keep same Codeowners and mailing list, meeting as before
- The name "Location Insights" will be reserved to a future Sub Project including the two repositories and potential others (as soon as one of the Sandbox repositories get Incubating status)
-
As soon as the repository is created, re-create the pull request #22 within the new repository DeviceVisitLocation. There are several options:
- Work will catch-up and deliver the prerequisites for Fall25 very soon -> we can consider it for Fall25
- The Sandbox repository will have an initial release independent of the meta-release (that can happen at any time, also before Fall25)
- The team decides to target Spring25 (potentially with the second initial release)
With the approach above the repository can progress to a proper M3 release for most-frequent-location, as long there is nothing merged for device-visit-location.
BTW: until the above proposal is decided you can continue to work on https://github.com/chinaunicomyangfan/LocationInsights/tree/devicevisitlocationyaml and adding the requested artifacts for the API as listed in the checklist.
|
@chinaunicomyangfan Repository https://github.com/camaraproject/DeviceVisitLocation is available, you can recreate the PR there and close it here. |

What type of PR is this?
What this PR does / why we need it:
First version upload of DeviceVisitLocation YAML file
Which issue(s) this PR fixes:
Fixes #21