Added Notification management aligned with CloudEvents format by bigludo7 · Pull Request #75 · camaraproject/DeviceStatus

bigludo7 · 2023-09-27T09:38:52Z

What type of PR is this?

enhancement/feature

What this PR does / why we need it:

Aligned with CloudEvents format as defined in Commonalities here: camaraproject/Commonalities#56

Which issue(s) this PR fixes:

Fixes #74 #57

Special notes for reviewers:

Changelog input

- Provide roaming status subscription & notification aligned with Cloudevents structure.

Additional documentation

This section can be blank.

docs

Aligned with CloudEvents format

akoshunyadi · 2023-09-27T19:38:54Z

@bigludo7 This PR fixes also Issue #57 ?

bigludo7 · 2023-09-28T07:40:58Z

@bigludo7 This PR fixes also Issue #57 ?

@akoshunyadi correct ! - fixed.

Align with DG

bigludo7

Aligned with merged DG for event & subscriptions
@akoshunyadi @sachinvodafone

jlurien · 2023-10-11T09:42:33Z

code/API_definitions/device-status.yaml

+              requestBody:
+                required: true
+                content:
+                  application/json:


We may need to use application/cloudevents+json as stated in
https://github.com/cloudevents/spec/blob/main/cloudevents/formats/json-format.md#3-envelope

@jlurien Yes make sense.
I will apply.

Do you think we need to go on this detail in the design guideline document as well? I tend to think yes.

yes, I think it will be useful to add this and maybe provide a template or artifact as example. I learned about this from some @patrice-conil 's PR indeed

Changed to content: application/cloudevents+json

bigludo7

Changed content: application/json to content: application/cloudevents+json for alignement with CloudEvents specification

maxl2287 · 2023-10-12T07:17:42Z

code/API_definitions/device-status.yaml

      summary: 'Create a device status event subscription for a device'
      description: Create a device status event subscription for a device
-      operationId: createDevicStatusEventSubscription
+      operationId: createDevicStatusSubscription


typo:
createDeviceStatusSubscription

maxl2287 · 2023-10-12T07:22:48Z

code/API_definitions/device-status.yaml

+        - id
+        - source
+        - specversion
+        - type


"time" is missing as required.

See Commonalities:

Note: Attribute time is tagged as optional in CloudEvents specification but from CAMARA perspective we mandate to value this attributes. https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md#event-notification-definition

sachinvodafone · 2023-10-12T09:21:56Z

code/API_definitions/device-status.yaml

-            roaming: true
-            countryCode: 208
-            countryName: FR
+          roaming: true


As per the document, it's mentioned that "The event subscription identifier must have a value when dealing with Resource-based subscriptions." So we should include the "subscriptionId" in the callback response if it's relevant or applicable in that context.

Took into account team review requests

bigludo7

Took @maxl2287 and @sachinvodafone review comments into account;
Thanks for the review.

akoshunyadi · 2023-10-16T14:06:06Z

code/API_definitions/device-status.yaml

+
+    API consumer is able to verify whether a certain user device is in roaming situation (or not). This capability is provided in 2 ways:
+      - via direct request with the roaming situation in the response.
+      - via a subscription  request - in this case the roaming situation is not in the response but event notification is sent back to the event subscriber when roaming situation change.


Typo: change => changed or has changed

akoshunyadi · 2023-10-16T14:09:00Z

code/API_definitions/device-status.yaml

+
+    ## Device status subscription
+
+    Theses endpoints allow to manage event subscription on roaming device status event. 


Typo: theses => these

akoshunyadi · 2023-10-16T14:11:13Z

code/API_definitions/device-status.yaml

+      - ``org.camaraproject.device-status.v0.roaming-change-country``: Event triggered when the device in roaming change country code
+
+    Note: Additionnally to these list, ``org.camaraproject.device-status.v0.subscription-ends`` notification `type` is sent when the subscription ends. 
+    This notification did not requires dedicated subscription. 


Typo: did not requires => does not require

akoshunyadi · 2023-10-16T14:18:55Z

code/API_definitions/device-status.yaml

+                    schema:
+                      $ref: "#/components/schemas/CloudEvent"
+                    examples:
+                      ROAMING_STATUS:


Do we want to keep these old constants, since they are not used for the type?

I've changed the constant name to be closer to the type but I kept only the significant part;

akoshunyadi · 2023-10-16T14:23:13Z

code/API_definitions/device-status.yaml

      type: string
      description: |
-        ROAMING_STATUS - Event triggered when the device switch from roaming ON to roaming OFF and conversely
+        roaming-status - Event triggered when the device switch from roaming ON to roaming OFF and conversely


I think it would be better to use SubscriptionType or SubscriptionEventType instead of RoamingEventType.

to better distinguish between subscription event type and notification event type

as a preparation for other type of events

akoshunyadi · 2023-10-16T14:39:21Z

code/API_definitions/device-status.yaml


    EventRoamingOff:
-      description:  event structure for roaming on and roaming off change
+      description: event structure for roaming on and roaming off change


This is only for roaming off

akoshunyadi · 2023-10-16T14:40:07Z

code/API_definitions/device-status.yaml

    RoamingStatus:
      description: Event detail structure for ROAMING_STATUS event
-      type : object
+      type: object


There is no ROAMING_STATUS event anymore

akoshunyadi · 2023-10-16T14:41:20Z

code/API_definitions/device-status.yaml

    RoamingChange:
      description: Event detail structure for ROAMING_ON & ROAMING_OFF event
-      type : object
+      type: object


There are no ROAMING_ON and ROAMING_OFF events anymore

akoshunyadi · 2023-10-16T14:43:12Z

code/API_definitions/device-status.yaml

    SubscriptionEnds:
      description: Event detail structure for SUBSCRIPTION_ENDS event
-      type : object
+      type: object


There is no SUBSCRIPTION_ENDS event

akoshunyadi · 2023-10-16T14:43:36Z

code/API_definitions/device-status.yaml

    RoamingChangeCountry:
      description: Event detail structure for ROAMING_CHANGE_COUNTRY event
-      type : object
+      type: object


There is no ROAMING_CHANGE_COUNTRY event

@akoshunyadi

Took into account @akoshunyadi review comments.

bigludo7

Took into account @akoshunyadi review comments.

bigludo7 · 2023-10-19T11:19:20Z

code/API_definitions/device-status.yaml

+                    schema:
+                      $ref: "#/components/schemas/CloudEvent"
+                    examples:
+                      ROAMING_STATUS:


I've changed the constant name to be closer to the type but I kept only the significant part;

fernandopradocabrillo

@bigludo7
Sorry I didn't review before. I leave here a comment regarding the scopes. This is a topic that was raised in Sim swap too, regarding in terms of privacy what scopes should we use for each operation. For example, I might be able to read the roaming status but maybe not to create a subscription, but on the other hand, I should not be able to create a subscription if I don't have permission to read the roaming status

fernandopradocabrillo · 2023-10-25T09:23:47Z

code/API_definitions/device-status.yaml

+      - device-status:roaming:read
+      - device-status:roaming-event-subscriptions:create


How are we going to manage the scopes for the subscriptions? I can see that we have an scope for creating subscriptions but I think we are missing an additional scope to delete subscriptions too. And, in order to retrieve the available subscriptions, are we going to use the same scope as we use to read the roaming status?

Suggested change

- device-status:roaming:read

- device-status:roaming-event-subscriptions:create

- device-status:roaming:read

- device-status:roaming-event-subscriptions:read

- device-status:roaming-event-subscriptions:create

- device-status:roaming-event-subscriptions:delete

Whether we decide to use specific scopes or not, we should use the security property for each individual operation because right now, using the global property some operations don't have a correct scope. For example we have a create scope for a delete operation:

sachinvodafone · 2023-10-25T10:37:20Z

code/API_definitions/device-status.yaml

-          $ref: '#/components/responses/Generic503'
-  /event-subscriptions/{eventSubscriptionId}:
+          $ref: "#/components/responses/Generic503"
+  /subscriptions/{subscriptionId}:


It seems, we have missed to define valid security scope for get/delete subscription operation

sachinvodafone · 2023-10-25T10:54:01Z

code/API_definitions/device-status.yaml

        503:
-          $ref: '#/components/responses/Generic503'
+          $ref: "#/components/responses/Generic503"
    get:


No valid security scope to "list" subscriptions (get operation)

sachinvodafone · 2023-10-25T11:16:59Z

code/API_definitions/device-status.yaml

-                $ref: '#/components/schemas/RoamingStatusResponse'
+                $ref: "#/components/schemas/RoamingStatusResponse"
              examples:
                No-Country-Code:


Is it example of "No-Country-Code" or "No-Country-Name"

sachinvodafone · 2023-10-25T11:31:14Z

In some responses, we are having missing "status":

sachinvodafone · 2023-10-25T11:49:52Z

What kind of response can the client anticipate when they receive a subscription ID in a 202 response code (asynchronous process) and attempt to access /subscriptions/{subscriptionId} where process is still not completed ?

202 Accepted (Still Processing):
404 Not Found (Subscription ID Not Found)

bigludo7 · 2023-10-26T16:14:45Z

What kind of response can the client anticipate when they receive a subscription ID in a 202 response code (asynchronous process) and attempt to access /subscriptions/{subscriptionId} where process is still not completed ?

202 Accepted (Still Processing):

404 Not Found (Subscription ID Not Found)

Good question and probably should tackled in a larger extend than this API. I will imagine 202 is a better option.

@sachinvodafone

Updated following @sachinvodafone and @fernandopradocabrillo review

bigludo7

Updated following @sachinvodafone and @fernandopradocabrillo review

sachinvodafone · 2023-10-27T09:56:04Z

What kind of response can the client anticipate when they receive a subscription ID in a 202 response code (asynchronous process) and attempt to access /subscriptions/{subscriptionId} where process is still not completed ?

202 Accepted (Still Processing):

404 Not Found (Subscription ID Not Found)

Good question and probably should tackled in a larger extend than this API. I will imagine 202 is a better option.

So in this scenario, we must have 202 in response code against "/subscriptions/{subscriptionId}"

bigludo7

Fixed typo for error 500 (Thanks @sachinvodafone )

@sachinvodafone

Added 202 for GET /subscriptions/{subscriptionId} as suggested by @sachinvodafone

bigludo7

Added 202 for GET /subscriptions/{subscriptionId} as suggested by @sachinvodafone

bigludo7 · 2023-10-30T09:32:14Z

Thanks @sachinvodafone for the approval;
@akoshunyadi and @fernandopradocabrillo looking also for your approval or comments. Your are not 'formal code owners but as you provided comments I'l also looking for your approval. Thanks

fernandopradocabrillo · 2023-10-30T14:07:55Z

What kind of response can the client anticipate when they receive a subscription ID in a 202 response code (asynchronous process) and attempt to access /subscriptions/{subscriptionId} where process is still not completed ?

202 Accepted (Still Processing):

404 Not Found (Subscription ID Not Found)

Good question and probably should tackled in a larger extend than this API. I will imagine 202 is a better option.

So in this scenario, we must have 202 in response code against "/subscriptions/{subscriptionId}"

I'm not really sure a 202 is the best option to return. If I create a subscription, receive a 202 and request the info before it is created, the subscription hasn't been created yet so I'd say it is better a 404.

I haven't seen a 202 in a GET, it is a bit weird. I think it adds complexity to the implementation because the response may vary if I make the call and receive a 202 and 5mins later I receive a 200 with the info, this does not happen with the POST/PUT operations since you only make the call once. And this will force the backend to have some way of tracking the process of creating the subscription to know what to return depending on the moment.

IMO I think it is more common to return a 404 and when the subscription is created, return the 200.

cc: @bigludo7 @sachinvodafone

akoshunyadi · 2023-11-02T14:11:24Z

code/API_definitions/device-status.yaml

+      operationId: createDeviceStatusSubscription
+      security:
+        - openId:
+            - device-status:roaming-subscriptions:create


It is a generic operation to create a subscription as the endpoint, description, operationId say, so I think also the scope shouldn't contain "roaming".

Similar for other subscription operations too.

@akoshunyadi Yes make sense. Fixed. I've kept device-status as prefix.

@fernandopradocabrillo

Remove 202 for GET following @fernandopradocabrillo comment. Rename scope Following @akoshunyadi comment

bigludo7

Remove 202 for GET following @fernandopradocabrillo comment.
Rename scope for subscription operation following @akoshunyadi comment

bigludo7 · 2023-11-02T17:09:52Z

What kind of response can the client anticipate when they receive a subscription ID in a 202 response code (asynchronous process) and attempt to access /subscriptions/{subscriptionId} where process is still not completed ?

202 Accepted (Still Processing):

404 Not Found (Subscription ID Not Found)

Good question and probably should tackled in a larger extend than this API. I will imagine 202 is a better option.

So in this scenario, we must have 202 in response code against "/subscriptions/{subscriptionId}"

I'm not really sure a 202 is the best option to return. If I create a subscription, receive a 202 and request the info before it is created, the subscription hasn't been created yet so I'd say it is better a 404.

I haven't seen a 202 in a GET, it is a bit weird. I think it adds complexity to the implementation because the response may vary if I make the call and receive a 202 and 5mins later I receive a 200 with the info, this does not happen with the POST/PUT operations since you only make the call once. And this will force the backend to have some way of tracking the process of creating the subscription to know what to return depending on the moment.

IMO I think it is more common to return a 404 and when the subscription is created, return the 200.

cc: @bigludo7 @sachinvodafone

OK!
I guess need extra discussion for adding 202 for GET when we have potentially ASYNC creation. As this topic is larger to this API I have removed the 202 and adding this point here: camaraproject/Commonalities#83
@sachinvodafone hope it's ok for you.

code/API_definitions/device-status.yaml

Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>

Update device-status.yaml

d54dd63

Aligned with CloudEvents format

bigludo7 requested review from eric-murray and sachinvodafone as code owners September 27, 2023 09:38

Update device-status.yaml

97dce71

Align with DG

bigludo7 commented Sep 29, 2023

View reviewed changes

maxl2287 mentioned this pull request Oct 11, 2023

add /connectivity - endpoint to check the network status of a device #69

Merged

bigludo7 requested a review from fernandopradocabrillo October 11, 2023 09:09

jlurien reviewed Oct 11, 2023

View reviewed changes

Update device-status.yaml

3d0f789

Changed to content: application/cloudevents+json

bigludo7 commented Oct 11, 2023

View reviewed changes

jlurien mentioned this pull request Oct 11, 2023

Define new geofencing subscriptions API with event subscription endpoint camaraproject/DeviceLocation#98

Merged

maxl2287 suggested changes Oct 12, 2023

View reviewed changes

sachinvodafone reviewed Oct 12, 2023

View reviewed changes

Update device-status.yaml

c496439

Took into account team review requests

bigludo7 commented Oct 12, 2023

View reviewed changes

akoshunyadi reviewed Oct 16, 2023

View reviewed changes

Update device-status.yaml

815a42d

Took into account @akoshunyadi review comments.

bigludo7 commented Oct 19, 2023

View reviewed changes

fernandopradocabrillo reviewed Oct 25, 2023

View reviewed changes

sachinvodafone reviewed Oct 25, 2023

View reviewed changes

Update device-status.yaml

28bffa6

Updated following @sachinvodafone and @fernandopradocabrillo review

bigludo7 commented Oct 26, 2023

View reviewed changes

bigludo7 commented Oct 27, 2023

View reviewed changes

Update device-status.yaml

e4b7273

Added 202 for GET /subscriptions/{subscriptionId} as suggested by @sachinvodafone

bigludo7 commented Oct 27, 2023

View reviewed changes

sachinvodafone previously approved these changes Oct 30, 2023

View reviewed changes

akoshunyadi reviewed Nov 2, 2023

View reviewed changes

Update device-status.yaml

1b57abb

Remove 202 for GET following @fernandopradocabrillo comment. Rename scope Following @akoshunyadi comment

bigludo7 dismissed sachinvodafone’s stale review via 1b57abb November 2, 2023 17:05

bigludo7 commented Nov 2, 2023

View reviewed changes

rartych reviewed Nov 8, 2023

View reviewed changes

code/API_definitions/device-status.yaml Outdated Show resolved Hide resolved

rartych reviewed Nov 8, 2023

View reviewed changes

code/API_definitions/device-status.yaml Outdated Show resolved Hide resolved

rartych reviewed Nov 8, 2023

View reviewed changes

code/API_definitions/device-status.yaml Outdated Show resolved Hide resolved

rartych reviewed Nov 8, 2023

View reviewed changes

code/API_definitions/device-status.yaml Outdated Show resolved Hide resolved

rartych reviewed Nov 8, 2023

View reviewed changes

code/API_definitions/device-status.yaml Outdated Show resolved Hide resolved

rartych reviewed Nov 8, 2023

View reviewed changes

code/API_definitions/device-status.yaml Outdated Show resolved Hide resolved

rartych reviewed Nov 8, 2023

View reviewed changes

code/API_definitions/device-status.yaml Outdated Show resolved Hide resolved

bigludo7 and others added 7 commits November 8, 2023 11:01

Update code/API_definitions/device-status.yaml

37aae34

Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>

Update code/API_definitions/device-status.yaml

3efba70

Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>

Update code/API_definitions/device-status.yaml

c8d6f06

Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>

Update code/API_definitions/device-status.yaml

1abfd2c

Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>

Update code/API_definitions/device-status.yaml

e199275

Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>

Update code/API_definitions/device-status.yaml

0442aa5

Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>

Update code/API_definitions/device-status.yaml

da54050

Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>

sachinvodafone approved these changes Nov 8, 2023

View reviewed changes

bigludo7 merged commit f378921 into main Nov 8, 2023

bigludo7 deleted the bigludo7-patch-4 branch November 8, 2023 10:08

akoshunyadi mentioned this pull request Nov 10, 2023

Add documentation into API definition #57

Closed

hdamker mentioned this pull request Nov 12, 2023

enhance v0.4.1 to follow latest CAMARA guidelines #81

Closed


		## Device status subscription

		Theses endpoints allow to manage event subscription on roaming device status event.

		- device-status:roaming:read
		- device-status:roaming-event-subscriptions:create

Conversation

bigludo7 commented Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

akoshunyadi commented Sep 27, 2023

Uh oh!

bigludo7 commented Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bigludo7 left a comment

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

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

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!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fernandopradocabrillo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachinvodafone Oct 25, 2023 • 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!

sachinvodafone commented Oct 25, 2023

Uh oh!

bigludo7 commented Sep 27, 2023 •

edited

Loading

bigludo7 commented Sep 28, 2023 •

edited

Loading

sachinvodafone Oct 25, 2023 •

edited

Loading

bigludo7 commented Oct 30, 2023 •

edited

Loading

fernandopradocabrillo commented Oct 30, 2023 •

edited

Loading

bigludo7 Nov 2, 2023 •

edited

Loading

bigludo7 left a comment •

edited

Loading

bigludo7 commented Nov 2, 2023 •

edited

Loading