Separate endpoint yamls proposal for direct API by bigludo7 · Pull Request #152 · camaraproject/DeviceStatus

bigludo7 · 2024-05-15T08:16:32Z

What type of PR is this?

Add one of the following kinds:

enhancement/feature

What this PR does / why we need it:

Separate endpoint yaml for reachability & roaming.

Which issue(s) this PR fixes:

Fixes #125
issue for Device error identifier to be added.

Special notes for reviewers:

Yaml for both roaming & reachability subscription provided once new subscription model merged in Commonalities
API name & endpoint to be validated by the team

Changelog input

 release-note
- Split yaml device status in 4 separate yaml for reachability-retrieval, roaming-retrieval, reachability-subscriptions, roaming-subscriptions.
- Align device error messages following commonalities device identifier error definition.

Additional documentation

This section can be blank.

docs

Separate endpoint yaml for reachability & roaming.

Fix mega-linter

Fixed megalinter

bigludo7

Fixed mega-Linter

fernandopradocabrillo · 2024-06-10T15:03:39Z

code/API_definitions/device-reachability-status.yaml

+  - name: Device reachability status
+    description: Operations to get the current reachability status of a device
+paths:
+  /retrieve-reachability-status:


@bigludo7 I see that for example in location-retrieval the WG opted for "/retrieve", in location-verification for "/verify", etc
If we have reachability-retrieval maybe we can go with "/retrieve" too? wdyt

Hello @fernandopradocabrillo
Yes make sense !
For the API name, today is reachability-retrieval - do you think we need to shift to reachability-status and then have an endpoint /retrieve - This is perhaps more straighforward?

Same will happen for roaming.

sounds good to me, it looks clearer

/retrieve would be good.
API name: I would prefer device-reachability-status, which contains the context 'device' for the case other reachability status API will come later.

maxl2287 · 2024-06-10T18:15:40Z

@bigludo7 @akoshunyadi
Do we want to do here in this PR the split into 4 Yamls as well?

akoshunyadi · 2024-06-10T20:14:19Z

@maxl2287 we can do the 2 subscription APIs in a 2nd PR to the splitting issue

bigludo7 · 2024-06-11T06:22:43Z

@maxl2287 we can do the 2 subscription APIs in a 2nd PR to the splitting issue

Both work for me
The most convenient for you Max.

maxl2287 · 2024-06-11T07:08:26Z

2nd PR is #161 for splitting /subscriptions

akoshunyadi · 2024-06-11T13:53:55Z

code/API_definitions/device-reachability-status.yaml

+  - name: Device reachability status
+    description: Operations to get the current reachability status of a device
+paths:
+  /retrieve-reachability-status:


/retrieve would be good.
API name: I would prefer device-reachability-status, which contains the context 'device' for the case other reachability status API will come later.

akoshunyadi · 2024-06-11T13:57:06Z

code/API_definitions/device-reachability-status.yaml

+  title: Device Reachability Status Retrieval
+  description: |
+    This API provides the customer with the ability to query device:
+      - Reachability status


This list suggests that other infos might come later, but this is a specialized API.

fernandopradocabrillo · 2024-06-12T16:50:45Z

code/API_definitions/device-reachability-status.yaml

+        "403":
+          $ref: "#/components/responses/ReachabilityPermissionDenied403"
+        "404":
+          $ref: "#/components/responses/ReachabilityNotFound404"


I can see that we have the generic NOT_FOUND in addition to the DeviceIdentifierNotFound.
In which scenario does the API return an standard NOT_FOUND?

I agree with @fernandopradocabrillo

Maybe later for such a case we should more go the way of having a reachability Status UNKNOWN with HTTP - 200 than a HTTP-404 for Reachability not found.

see #148

@bigludo7 wdyt?

@maxl2287 @fernandopradocabrillo I can remove the NOT_FOUND. No problem.

BTW we're still waiting outcomes for #148 which is a fair point. I will comment there.

maxl2287 · 2024-06-13T08:20:44Z

code/API_definitions/device-reachability-status.yaml

+    ReachabilityPermissionDenied403:
+      description: |
+        Client does not have sufficient permission.
+        In addition to regular scenario of `PERMISSION_DENIED`, other scenarios may exist:
+          - Phone number cannot be deducted from access token context.(`{"code": "NUMBER_VERIFICATION.INVALID_TOKEN_CONTEXT","message": "Phone number cannot be deducted from access token context"}`)


@bigludo7, is this a valid 403 error?
Currently, we are not extracting or verifying a phone number from the access token. This situation seems more related to Number Verification rather than Device Status.

Do you agree?

Good question :)
For me this API could be sent from the device itself in frontend flow and in this case we got the phoneNumber from the network (see ICM proposal here). I need to update the yaml to not have device mandatory anymore.

So I think we should keep this error to manage this case but of course shifting prefix from NUMBER_VERIFICATION to DEVICE_REACHEABILTY_STATUS.

WDYT?

If we make the device not mandatory then, for me, it makes sense to keep the invalid token context error.

Yes @fernandopradocabrillo - This is my preferred option.

If we offer this possibility over the access token, then we should extend the documentation. Otherwise the user has no idea when he needs to add the device.
btw. the example still contains number-verification

@akoshunyadi Thanks - Fixed!
I've applied what we have in commonalities here

PR233 will change this but I will prefer we look for merge this one and craft new PR for additional alignement because then it is complicated to maintain.

Applied same fix for device-roaming-status api

Changed endpoint to /retrieve Took suggestion on documentation Device is not anymore mandatory Add 422 for Network issue - Unable to provide reachability status I think we're not on same page on the api name.

bigludo7

Applied updates on device-reachability-status yaml:

Changed endpoint name to /retrieve
Took suggestion on documentation from @akoshunyadi
Device is not anymore mandatory
Add 422 for Network issue - Unable to provide reachability status

About the api name.
@akoshunyadi propose device-reachability-status (I proposed reachability-status) but ok for me to move forward with Akos proposal - any issue from the team for the name.

Note: I will apply same on device-roaming-status API

Update scope name

Aligned with device-reachability-status update.

bigludo7 · 2024-06-13T16:37:35Z

Aligned device-roaming-status with the work done with device-reacheability-status.
Need review by the team (@akoshunyadi @fernandopradocabrillo @maxl2287 @sachinvodafone) :)

fernandopradocabrillo · 2024-06-19T07:35:31Z

code/API_definitions/device-reachability-status.yaml

+openapi: 3.0.3
+info:
+  title: Device Reachability Status Retrieval
+  description: |


we need to add the Auth section: https://github.com/camaraproject/IdentityAndConsentManagement/blob/main/documentation/CAMARA-API-access-and-user-consent.md#mandatory-template-for-infodescription-in-camara-api-specs

Add ## Authorization and authentication doc

bigludo7 · 2024-06-19T08:11:38Z

Hi team
Seems that I've mixed myself for Device Reachability Status yaml (commit 7e002c9 overrides b1b96c5 corrections).

I've fixed that (I hope so) + added Authorization and authentication § in the doc as suggested by @fernandopradocabrillo for both yaml.

Sorry for the confusion.

akoshunyadi · 2024-06-19T08:26:11Z

Where do we want to delete the old yaml? We could do it in this PR.

maxl2287 · 2024-06-19T08:30:52Z

I aggree with @akoshunyadi.

bigludo7 · 2024-06-19T08:57:10Z

@akoshunyadi @maxl2287 added delete the old file in this PR

akoshunyadi · 2024-06-20T18:53:44Z

code/API_definitions/device-roaming-status.yaml

+        Client does not have sufficient permission.
+        In addition to regular scenario of `PERMISSION_DENIED`, other scenarios may exist:
+          - Phone number cannot be deducted from access token context.(`{"code": "NUMBER_VERIFICATION.INVALID_TOKEN_CONTEXT","message": "Phone number cannot be deducted from access token context"}`)
+      headers:


Here it's a remaining of NUMBER_VERIFICATION, should be replaced

@bigludo7 could you please do this update, so we can approve and merge this PR? Thanks!

@akoshunyadi Done :)

Fixed line 343 - wrong description.

akoshunyadi

LGTM

bigludo7 · 2024-07-01T10:16:29Z

@sachinvodafone @fernandopradocabrillo may I ask you to review this one for blocking point as we target to merge this one soon. Thanks

fernandopradocabrillo

LGTM

bigludo7 · 2024-07-03T10:16:26Z

@akoshunyadi Due to git hub conflict issue I reinserted the 'old' device-status.yaml file and will remove it an a separate PR.
Let's hope it will work.

sachinvodafone · 2024-07-03T10:40:14Z

code/API_definitions/device-reachability-status.yaml

+                    reachabilityStatus: CONNECTED_DATA
+                Not-Connected:
+                  value:
+                    lastStatusTime: "2024-02-20T10:41:38.657Z"


Do we have an example where network is connected to both SMS and Data and we need to send a response accordingly like: {
"lastStatusTime": "2024-02-20T10:41:38.657Z",
"reachabilityStatus": "CONNECTED_SMS_DATA"
}

For me no as my understanding DATA means that SMS is also available.
@sachinvodafone I will merge this PR so probably we could continue the conversation in Discussions tab as it does not impact this merging.

Separate endpoint yaml

9e265ac

Separate endpoint yaml for reachability & roaming.

bigludo7 added v0.6.0 Planned for release v0.6.0 waiting for commonalities labels May 15, 2024

bigludo7 requested review from akoshunyadi, eric-murray and sachinvodafone as code owners May 15, 2024 08:16

bigludo7 added the NotToBeMergedForNow label May 15, 2024

This comment was marked as outdated.

Sign in to view

bigludo7 added 2 commits May 15, 2024 10:32

Update device-reachability-status.yaml

5af73e6

Fix mega-linter

Update device-roaming-status.yaml

c6677cd

Fixed megalinter

bigludo7 commented May 15, 2024

View reviewed changes

fernandopradocabrillo reviewed Jun 10, 2024

View reviewed changes

bigludo7 removed waiting for commonalities NotToBeMergedForNow labels Jun 11, 2024

bigludo7 mentioned this pull request Jun 11, 2024

Separate endpoints into different APIs #125

Closed

akoshunyadi reviewed Jun 11, 2024

View reviewed changes

fernandopradocabrillo reviewed Jun 12, 2024

View reviewed changes

maxl2287 reviewed Jun 13, 2024

View reviewed changes

Update device-reachability-status.yaml

6656937

Changed endpoint to /retrieve Took suggestion on documentation Device is not anymore mandatory Add 422 for Network issue - Unable to provide reachability status I think we're not on same page on the api name.

bigludo7 commented Jun 13, 2024

View reviewed changes

bigludo7 added 2 commits June 13, 2024 16:55

Update device-reachability-status.yaml

b1b96c5

Update scope name

Update device-roaming-status.yaml

b537f60

Aligned with device-reachability-status update.

bigludo7 changed the title ~~Separate endpoint yamls proposal~~ Separate endpoint yamls proposal for direct API Jun 13, 2024

maxl2287 mentioned this pull request Jun 17, 2024

Consolidate Common Data Types into a single YAML-File #169

Closed

Update device-reachability-status.yaml

7e002c9

Update device-roaming-status.yaml

3a7cb16

akoshunyadi added Fall24 and removed v0.6.0 Planned for release v0.6.0 labels Jun 18, 2024

fernandopradocabrillo reviewed Jun 19, 2024

View reviewed changes

bigludo7 added 2 commits June 19, 2024 10:03

Update device-reachability-status.yaml

d7abdf6

Update device-roaming-status.yaml

7b30641

Add ## Authorization and authentication doc

Delete code/API_definitions/device-status.yaml

9746f3f

akoshunyadi reviewed Jun 20, 2024

View reviewed changes

Update device-roaming-status.yaml

86bc949

Fixed line 343 - wrong description.

akoshunyadi previously approved these changes Jul 1, 2024

View reviewed changes

fernandopradocabrillo self-requested a review July 1, 2024 11:48

sachinvodafone previously approved these changes Jul 3, 2024

View reviewed changes

fernandopradocabrillo approved these changes Jul 3, 2024

View reviewed changes

Add files via upload

0924159

bigludo7 dismissed stale reviews from sachinvodafone and akoshunyadi via 0924159 July 3, 2024 10:12

sachinvodafone reviewed Jul 3, 2024

View reviewed changes

akoshunyadi approved these changes Jul 3, 2024

View reviewed changes

bigludo7 merged commit fb334e9 into main Jul 3, 2024

bigludo7 deleted the SeparatenEndpoints branch July 3, 2024 11:57

Conversation

bigludo7 commented May 15, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for reviewers:

Changelog input

Additional documentation

Uh oh!

This comment was marked as outdated.

bigludo7 left a comment

Choose a reason for hiding this comment

Uh oh!

fernandopradocabrillo Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxl2287 commented Jun 10, 2024

Uh oh!

akoshunyadi commented Jun 10, 2024

Uh oh!

bigludo7 commented Jun 11, 2024

Uh oh!

maxl2287 commented Jun 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxl2287 Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bigludo7 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bigludo7 commented Jun 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bigludo7 commented Jun 19, 2024

Uh oh!

akoshunyadi commented Jun 19, 2024

Uh oh!

maxl2287 commented Jun 19, 2024

Uh oh!

bigludo7 commented Jun 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

fernandopradocabrillo Jun 10, 2024 •

edited

Loading

maxl2287 Jun 13, 2024 •

edited

Loading

bigludo7 left a comment •

edited

Loading