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

Update the subscription models to align on CAMARA commonalities #170

Merged

Conversation

maxl2287
Copy link
Contributor

What type of PR is this?

Add one of the following kinds:

  • correction
  • enhancement/feature

What this PR does / why we need it:

Update of the subscription-models based on:

Which issue(s) this PR fixes:

Fixes #162

@maxl2287 maxl2287 self-assigned this Jun 18, 2024
@maxl2287 maxl2287 changed the title Feature/update subscription models Update the subscription models to align on CAMARA commonalities Jun 18, 2024
@maxl2287 maxl2287 added enhancement New feature or request v0.6.0 Planned for release v0.6.0 labels Jun 18, 2024
@akoshunyadi akoshunyadi added Fall24 and removed v0.6.0 Planned for release v0.6.0 labels Jun 18, 2024
@akoshunyadi
Copy link
Collaborator

The base url should be with apiName, so 'device-' is missing.. (I can't comment there directly)

@maxl2287 maxl2287 requested a review from akoshunyadi June 21, 2024 08:37
sachinvodafone
sachinvodafone previously approved these changes Jun 21, 2024
bigludo7
bigludo7 previously approved these changes Jul 3, 2024
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
Nice job Max ! thanks

@sachinvodafone
Copy link
Collaborator

As per today discussion over subscription response, would apprecaite if we can include examples in specification that pointed in discussion

@bigludo7
Copy link
Collaborator

bigludo7 commented Jul 3, 2024

@maxl2287 I have a second thought about making the device identifier optional in the subscription.
My point is the following:
For the POST that's fine to not have this device identifier as we have it thru the access token.

But, when the developer check the subscription 10 days later, in the GET /subscriptions response better to have this device identifier present because without it impossible to know what device is targeted....

So... either 1/ we keep the device mandatory or 2/we make it optionnal for the POST request but mandatory in the GET response

I prefer 1/ but perhaps we can have other solution.

cc: @fernandopradocabrillo

@maxl2287
Copy link
Contributor Author

maxl2287 commented Jul 3, 2024

@maxl2287 I have a second thought about making the device identifier optional in the subscription. My point is the following: For the POST that's fine to not have this device identifier as we have it thru the access token.

But, when the developer check the subscription 10 days later, in the GET /subscriptions response better to have this device identifier present because without it impossible to know what device is targeted....

So... either 1/ we keep the device mandatory or 2/we make it optionnal for the POST request but mandatory in the GET response

I prefer 1/ but perhaps we can have other solution.

cc: @fernandopradocabrillo

@bigludo7 I aggree also on letting it mandatory here.

@maxl2287
Copy link
Contributor Author

maxl2287 commented Jul 4, 2024

Thanks @maxl2287 , Can we get an example for "GET /subscriptions" where we provide only the subscriptionID for subscriptions with incomplete asynchronous calls and show the full response for the others (synchronous)?

@sachinvodafone
However, this will not align with the specified response for GET /subscriptions.
The response for this endpoint includes a list of only having Subscription components.

There should not be a mix of Subscription objects and subscriptionId values within the same list.

@bigludo7
Copy link
Collaborator

bigludo7 commented Jul 4, 2024

@maxl2287 I have a second thought about making the device identifier optional in the subscription. My point is the following: For the POST that's fine to not have this device identifier as we have it thru the access token.
But, when the developer check the subscription 10 days later, in the GET /subscriptions response better to have this device identifier present because without it impossible to know what device is targeted....
So... either 1/ we keep the device mandatory or 2/we make it optionnal for the POST request but mandatory in the GET response
I prefer 1/ but perhaps we can have other solution.
cc: @fernandopradocabrillo

@bigludo7 I aggree also on letting it mandatory here.

I suggest we do this way - let keep it mandatory. If in future we shift to optional this is not a breaking change.

@sachinvodafone
Copy link
Collaborator

Thanks @maxl2287 , Can we get an example for "GET /subscriptions" where we provide only the subscriptionID for subscriptions with incomplete asynchronous calls and show the full response for the others (synchronous)?

@sachinvodafone However, this will not align with the specified response for GET /subscriptions. The response for this endpoint includes a list of only having Subscription components.

There should not be a mix of Subscription objects and subscriptionId values within the same list.

@maxl2287 I understand your point that it will only include those subscriptions that have already been created. Thank you.

@maxl2287
Copy link
Contributor Author

maxl2287 commented Jul 4, 2024

Thanks @maxl2287 , Can we get an example for "GET /subscriptions" where we provide only the subscriptionID for subscriptions with incomplete asynchronous calls and show the full response for the others (synchronous)?

@sachinvodafone However, this will not align with the specified response for GET /subscriptions. The response for this endpoint includes a list of only having Subscription components.
There should not be a mix of Subscription objects and subscriptionId values within the same list.

@maxl2287 I understand your point that it will only include those subscriptions that have already been created. Thank you.

@sachinvodafone yes, but to be more precise:
The list includes every subscription, which was created by the user, even those which are still in the creation process (see status ACTIVATION_REQUESTED).

@sachinvodafone
Copy link
Collaborator

Thanks @maxl2287 , Can we get an example for "GET /subscriptions" where we provide only the subscriptionID for subscriptions with incomplete asynchronous calls and show the full response for the others (synchronous)?

@sachinvodafone However, this will not align with the specified response for GET /subscriptions. The response for this endpoint includes a list of only having Subscription components.
There should not be a mix of Subscription objects and subscriptionId values within the same list.

@maxl2287 I understand your point that it will only include those subscriptions that have already been created. Thank you.

@sachinvodafone yes, but to be more precise: The list includes every subscription, which was created by the user, even those which are still in the creation process (see status ACTIVATION_REQUESTED).

Suppose if we have only one subscription and that is asynchronous which still not completed, in this case ,we will get "ACTIVATION_REQUESTED" if user try for "GET /Subscriptions" . If Yes, then I am having another question.

@maxl2287
Copy link
Contributor Author

maxl2287 commented Jul 4, 2024

Suppose if we have only one subscription and that is asynchronous which still not completed, in this case ,we will get "ACTIVATION_REQUESTED" if user try for "GET /Subscriptions" . If Yes, then I am having another question.

You will receive something like:

[
  {
    "sink": "https://endpoint.example.com/sink",
    "sinkCredential": {},
    "types": [
      "string"
    ],
    "config": {
      "subscriptionDetail": {
        "device": {
          "phoneNumber": "+123456789",
          "networkAccessIdentifier": "[email protected]",
          "ipv4Address": {
            "publicAddress": "84.125.93.10",
            "publicPort": 59765
          },
          "ipv6Address": "2001:db8:85a3:8d3:1319:8a2e:370:7344"
        }
      },
      "subscriptionExpireTime": "2023-01-17T13:18:23.682Z",
      "subscriptionMaxEvents": 5,
      "initialEvent": true
    },
    "id": "550e8400-e29b-41d4-a716-446655440000",
    "startsAt": "2024-07-04T11:44:36.664Z",
    "expiresAt": "2024-07-04T11:44:36.664Z",
    "status": "ACTIVATION_REQUESTED"
  }
]

@sachinvodafone
Copy link
Collaborator

Suppose if we have only one subscription and that is asynchronous which still not completed, in this case ,we will get "ACTIVATION_REQUESTED" if user try for "GET /Subscriptions" . If Yes, then I am having another question.

You will receive something like:

[
  {
    "sink": "https://endpoint.example.com/sink",
    "sinkCredential": {},
    "types": [
      "string"
    ],
    "config": {
      "subscriptionDetail": {
        "device": {
          "phoneNumber": "+123456789",
          "networkAccessIdentifier": "[email protected]",
          "ipv4Address": {
            "publicAddress": "84.125.93.10",
            "publicPort": 59765
          },
          "ipv6Address": "2001:db8:85a3:8d3:1319:8a2e:370:7344"
        }
      },
      "subscriptionExpireTime": "2023-01-17T13:18:23.682Z",
      "subscriptionMaxEvents": 5,
      "initialEvent": true
    },
    "id": "550e8400-e29b-41d4-a716-446655440000",
    "startsAt": "2024-07-04T11:44:36.664Z",
    "expiresAt": "2024-07-04T11:44:36.664Z",
    "status": "ACTIVATION_REQUESTED"
  }
]

I have updated the scenario in discussion board here

@maxl2287
Copy link
Contributor Author

maxl2287 commented Jul 8, 2024

@bigludo7 can we do here a final review?
I just have added some examples based on @sachinvodafone's feedback.

bigludo7
bigludo7 previously approved these changes Jul 9, 2024
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.
Thanks

@bigludo7
Copy link
Collaborator

bigludo7 commented Jul 9, 2024

@bigludo7 can we do here a final review? I just have added some examples based on @sachinvodafone's feedback.

Thanks @maxl2287 - Look good for me. Let's get final review from @sachinvodafone and @akoshunyadi and we're good to go.

Copy link
Collaborator

@akoshunyadi akoshunyadi left a comment

Choose a reason for hiding this comment

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

The server url seems to miss the device- prefix for roaming

akoshunyadi
akoshunyadi previously approved these changes Jul 9, 2024
Copy link
Collaborator

@fernandopradocabrillo fernandopradocabrillo left a comment

Choose a reason for hiding this comment

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

LGTM

bigludo7
bigludo7 previously approved these changes Jul 9, 2024
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

@bigludo7
Copy link
Collaborator

@sachinvodafone it is ok for you now? Looking for you thumb up to merge :)
Thanks

maxl2287 added 2 commits July 11, 2024 13:09
…e/update-subscription-models

# Conflicts:
#	code/API_definitions/device-reachability-status-subscriptions.yaml
#	code/API_definitions/device-roaming-status-subscriptions.yaml
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

@akoshunyadi akoshunyadi merged commit 9cdf97b into camaraproject:main Jul 11, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Fall24
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align subscription APIs with new template
5 participants