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

add /subscription-endpoint for location-area #27

Conversation

maxl2287
Copy link
Contributor

@maxl2287 maxl2287 commented Mar 1, 2023

This PR relates to #18.

After a discussion of PR #24, this PR just includes the endpoint to create subscriptions for multiple devices.

@maxl2287 maxl2287 changed the title Add /subscription-endpoint for location-area add /subscription-endpoint for location-area Mar 1, 2023
Slightly change date attribute description for your consideration :)
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.

Slightly change date attribute description for your consideration :)

@maxl2287
Copy link
Contributor Author

maxl2287 commented Mar 3, 2023

@bigludo7 @jlurien: Anything else which needs to be adapted? Otherwise I guess it can be merged, right?

@bigludo7
Copy link
Collaborator

bigludo7 commented Mar 3, 2023

Hello @maxl2287 - for me it could be merged !
Thanks for this !
Let's wait @jlurien approval before to push the button :)

@jlurien
Copy link
Collaborator

jlurien commented Mar 3, 2023

The basics are OK, but I would like to wait until next week to meet with our Product guys and get their feedback for this type of functionality. With MWC in place, it has not been possible this week.

I think we can keep the working on this iteration until next WG meeting.

@jlurien
Copy link
Collaborator

jlurien commented Mar 6, 2023

@maxl2287 Did you added the area to the CreateSubscription model?

@maxl2287
Copy link
Contributor Author

maxl2287 commented Mar 7, 2023

@maxl2287 Did you added the area to the CreateSubscription model?

I thought "area" is part of this PR: #29

@jlurien
Copy link
Collaborator

jlurien commented Mar 7, 2023

@maxl2287 Did you added the area to the CreateSubscription model?

I thought "area" is part of this PR: #29

There we define the Area scheme in detail, but it has to be referenced in the subscription creation. With the current proposal clients are subscribing to a set of devices, but they have to provide the area to monitor as well, as events are for AREA_ENTERED and AREA_LEFT, but which area?

@maxl2287
Copy link
Contributor Author

maxl2287 commented Mar 7, 2023

@jlurien ahhhh you're right.
Somehow I forgot to use the latitude, longitude and accuracy to describe the area.
I have added now the field "area" which is described (for now) by just these three properties.

So it acts currently like a circle-area.

@maxl2287
Copy link
Contributor Author

maxl2287 commented Mar 8, 2023

@jlurien any other points or still waiting for internal WG meeting? 😸

@jlurien
Copy link
Collaborator

jlurien commented Mar 8, 2023

@maxl2287 Related to the new issue #32, I'm thinking also about the impact of having a list of devices to subscribe per subscription, specially related to the consent management. Unless all devices belong to the same user it is likely that we will require an specific consent per device. How are we going to deal with the error scenarios when a consent is missing just of a subset of devices.

Until the track about consent management is more advanced in Commonalities I think it would be safer to have an initial proposal just with one device per subscription. In the future we may extend it to allow multiple devices.

This is also something to take into account for the transversal discussion about subscriptions and event notification @bigludo7

@bigludo7
Copy link
Collaborator

bigludo7 commented Mar 8, 2023

Hello
This is a fair point.
I agree on the requirement provided in issue #32 of allowing an array of devices in the subscription in logistic UC for example (a container fleet). Here this IoT fleet and associated SIM are owned by a company and not sure about consent management in his case as not 'real' people involved. While for UC involving real people we have to be cautious with the consent.

But as we have now this issue #32 separably, I tend to agree with @jlurien --> Keep to one in #27 and defer to #32 for multiples.

@maxl2287
Copy link
Contributor Author

maxl2287 commented Mar 8, 2023

I reverted to use just one device for subscription, but I let the schema for a list inside.
I hope this is okay for later usage.

UeIdList:
      description: List of User equipment identifiers
      type: array
      items:
        $ref: '#/components/schemas/UeId'

@bigludo7
Copy link
Collaborator

bigludo7 commented Mar 8, 2023

Look good for me !
Thanks @maxl2287
If OK for @jlurien we can merge.

@jlurien
Copy link
Collaborator

jlurien commented Mar 9, 2023

Don't we need an endpoint to delete a subscription by subscriptioId as well? Otherwise there is no way to cancel an active subscription. Another one to get the susbcription by id is common but is less important.

When dealing with consents another scenarios may rise, for example if a user revoke their consent, any active subscription should be disabled and we have to decide whether the client should be notified and how.

@bigludo7
Copy link
Collaborator

bigludo7 commented Mar 9, 2023

These are great comment @jlurien and good material for guideline evolution for event.

In the current proposal we have a subscriptionExpireTime to terminate a subscription. This field is mandatory but I got your point: API client should be able to terminate the subscription before the expiration (and we cannot change it).

--> We should probably add a DELETE operation and in order to retrieve it we need a GET operation (I mean 2 GET, one per id and another to retrieve list with input parameters).

For the later UC one idea: we could probably add a third eventType = "SUBSCRIPTION_ENDED" ?

@bigludo7
Copy link
Collaborator

bigludo7 commented Mar 9, 2023

@maxl2287 @jlurien Perhaps good for CAMARA consistency that you take a look on this discussion : camaraproject/DeviceStatus#17

@maxl2287
Copy link
Contributor Author

@bigludo7 Thanks for the hint to the discussion in DeviceStatus.
Shell we wait until this is clarified, so that this PR is on hold?

@bigludo7
Copy link
Collaborator

Hi @maxl2287
Yes I think it is probably better to wait for a common agreement on model (in commonalities) and then proceed in each api project.

@bigludo7
Copy link
Collaborator

Hello
For information and review --> camaraproject/WorkingGroups#173

@maxl2287 @jlurien I would like to add you as reviewer in the commonalities to get your view but I was not able to add you. It did not prevent you to comment there.

@jlurien
Copy link
Collaborator

jlurien commented Mar 20, 2023

Hello For information and review --> camaraproject/WorkingGroups#173

@maxl2287 @jlurien I would like to add you as reviewer in the commonalities to get your view but I was not able to add you. It did not prevent you to comment there.

Thanks, I'll take a look, but my colleague Pedro Diez is already in charge of working on subscription/notifications in Commonalities, on behalf of Telefonica. I will coordinate with him.

@jlurien jlurien deleted the branch camaraproject:dev-0.2.0 April 11, 2023 15:27
@jlurien jlurien closed this Apr 11, 2023
@maxl2287
Copy link
Contributor Author

@jlurien @bigludo7 what's the reason of closing this PR?

@jlurien
Copy link
Collaborator

jlurien commented Apr 12, 2023

@maxl2287 It was not my intention to close this PR, but I see is as consequence of deleting the branch, while cleaning up old branches. Anyway, we agreed to propose PRs directly to main branch, as suggested by guidelines. So we'd have to move this PR there. As we are also waiting to align subscription/notifications model to the one to be agreed in Commonalities, once we have that, we can consolidate this PR there. wdyt @bigludo7 ?

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.

3 participants