camaraproject / DeviceLocation

Repository to describe, develop, document and test the DeviceLocation API family
Apache License 2.0
21 stars 31 forks source link

add /subscription-endpoint for location-area #27

Closed maxl2287 closed 1 year ago

maxl2287 commented 1 year ago

This PR relates to #18.

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

maxl2287 commented 1 year ago

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

bigludo7 commented 1 year ago

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

jlurien commented 1 year ago

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 commented 1 year ago

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

maxl2287 commented 1 year ago

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

I thought "area" is part of this PR: https://github.com/camaraproject/DeviceLocation/pull/29

jlurien commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

jlurien commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

jlurien commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@maxl2287 @jlurien Perhaps good for CAMARA consistency that you take a look on this discussion : https://github.com/camaraproject/DeviceStatus/issues/17

maxl2287 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

Hello For information and review --> https://github.com/camaraproject/WorkingGroups/pull/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 commented 1 year ago

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.

maxl2287 commented 1 year ago

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

jlurien commented 1 year ago

@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 ?