Update sim-swap-notification-subscription.yaml by bigludo7 · Pull Request #111 · camaraproject/SimSwap

bigludo7 · 2024-06-05T14:29:53Z

What type of PR is this?

Add one of the following kinds:

correction

What this PR does / why we need it:

Modify sim-swap-notification-subscription.yaml accordingly to the new format defined in Commonalities.
This format is aligned with CloudEvents.

Which issue(s) this PR fixes:

Fixes #108
#113

Special notes for reviewers:

Changelog input

 release-note
- align subscription model accordingly to the new format defined in Commonalities.
This format is aligned with CloudEvents.
- Fixes examples for phoneNumber

Additional documentation

This section can be blank.

docs

Align with CloudEvents model for subscription

github-actions · 2024-06-05T14:31:15Z

🦙 MegaLinter status: ✅ SUCCESS

Descriptor	Linter	Files	Errors	Elapsed time
✅ ACTION	actionlint	2	0	0.03s
✅ OPENAPI	spectral	2	0	3.22s
✅ REPOSITORY	git_diff	yes	no	0.01s
✅ REPOSITORY	secretlint	yes	no	0.72s
✅ YAML	yamllint	2	0	0.51s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by

Fix MegaLinter

Fixes MegaLinter

Fix MegaLinter issue

Fix MegaLinter

maxl2287

@bigludo7 I have added some points.
Wdyt?

code/API_definitions/sim-swap-notification-subscription.yaml

maxl2287 · 2024-06-07T09:29:00Z

Maybe you also need to create an example for the request, as the example request for POST /subscriptions is not showing the required "protocol" - property.

Or do you have an idea how to tell the example to use "type": "HTTP" ?

code/API_definitions/sim-swap-notification-subscription.yaml

bigludo7 · 2024-06-07T11:57:48Z

💯 thanks @maxl2287 for your review !
I will consider all these.

maxl2287 · 2024-06-07T12:01:12Z

Maybe you also need to create an example for the request, as the example request for POST /subscriptions is not showing the required "protocol" - property.

Or do you have an idea how to tell the example to use "type": "HTTP" ?

I added an issue for this behaviour / bug:
camaraproject/Commonalities#226

Updated after https://github.com/maxl2287 review.

fix megaLinter issues

bigludo7 · 2024-06-07T16:42:58Z

Fixed all points raised by @maxl2287 (I hope so)

code/API_definitions/sim-swap-notification-subscription.yaml

@gregory1g

Updated following @gregory1g proposal

Added quote for phoneNumber in the examples.

maxl2287 · 2024-06-12T20:08:19Z

LGTM 🎉

code/API_definitions/sim-swap-notification-subscription.yaml

fixed DEACTIVE to INACTIVE

maxl2287

Updating the name of the file to sim-swap-subscriptions.yaml ?
At least the plural -subscriptions has to be at the end.

Removing -notifications- would also include updating the whole file to remove Notifications also for, e.g., tags.

maxl2287

@bigludo7 I added some comments.
Please take a look 🐦

maxl2287 · 2024-06-21T09:26:54Z

code/API_definitions/sim-swap-notification-subscription.yaml

      security:
        - openId:
-            - sim-swap:subscriptions:create
+            - sim-swap-subscriptions:org.camaraproject.sim-swap-subscriptions.v0.swapped:read


For creating a subscription we should use here create as an action.

maxl2287 · 2024-06-21T09:27:59Z

code/API_definitions/sim-swap-notification-subscription.yaml

      security:
        - openId:
-            - sim-swap:subscriptions:read
+            - sim-swap-subscriptions:read


sim-swap-subscriptions:org.camaraproject.sim-swap-subscriptions.v0.swapped:read

Hummm - I dont think so regarding the template yaml for the GET operation which is on the subscription itself and not the event.

maxl2287 · 2024-06-21T09:28:05Z

code/API_definitions/sim-swap-notification-subscription.yaml

      security:
        - openId:
-            - sim-swap:subscriptions:read
+            - sim-swap-subscriptions:read


sim-swap-subscriptions:org.camaraproject.sim-swap-subscriptions.v0.swapped:read

Hummm - I dont think so regarding the template yaml for the GET operation which is on the subscription itself and not the event.

maxl2287 · 2024-06-21T09:28:17Z

code/API_definitions/sim-swap-notification-subscription.yaml

      security:
        - openId:
-            - sim-swap:subscriptions:delete
+            - sim-swap-subscriptions:delete


sim-swap-subscriptions:org.camaraproject.sim-swap-subscriptions.v0.swapped:delete

Hummm - I dont think so regarding the template yaml for the DELETE operation which is on the subscription itself and not the event.

maxl2287 · 2024-06-21T09:28:44Z

code/API_definitions/sim-swap-notification-subscription.yaml

+        - Sim Swap Notification Subscription
      summary: 'Delete a sim swap event subscription'
      operationId: deleteSubscription
      description: delete a  given event subscription.


two whitespaces between a given

updated the tag

maxl2287 · 2024-06-21T09:32:19Z

code/API_definitions/sim-swap-notification-subscription.yaml

+        types:
+          description: |
+            Camara Event types eligible for subscription:
+            - org.camaraproject.sim-swap-subscriptions.v0.swapped: receive a notification when a sim swap is performed on the line.
+          type: array
+          example:
+            - "org.camaraproject.sim-swap-subscriptions.v0.swapped"
+          items:
+            type: string


I recommend adding here like for DeviceLocation and DeviceStatus the minItems and maxItems and set them to 1

types: description: | Camara Event types eligible to be delivered by this subscription. Note: As of now we enforce to have only event type per subscription. type: array minItems: 1 maxItems: 1 items: $ref: "#/components/schemas/SubscriptionEventType"

Also think about using here the enum, even for the single one.
It enforces then to avoid requesting with unsupported event-types here.

maxl2287 · 2024-06-21T09:34:51Z

code/API_definitions/sim-swap-notification-subscription.yaml

+        types:
+          description: |
+            Camara Event types eligible for subscription:
+            - org.camaraproject.sim-swap-subscriptions.v0.swapped: receive a notification when a sim swap is performed on the line.
+            Note: for the Commonalities meta-release v0.4 we enforce to have only event type per subscription then for following meta-release use of array MUST be decided
+             at API project level.
+          type: array
+          items:
+            type: string


The same like for the request.
Extracting the types into an own component and setting the min and maxItems

…-subscriptions.yaml Updated following Max review

bigludo7 · 2024-07-02T10:27:26Z

Updating the name of the file to sim-swap-subscriptions.yaml ? At least the plural -subscriptions has to be at the end.

Removing -notifications- would also include updating the whole file to remove Notifications also for, e.g., tags.

You're right - done !

bigludo7 · 2024-07-02T13:20:42Z

@maxl2287 I've considered all your comments.

bigludo7 · 2024-07-05T08:07:09Z

@maxl2287 @gregory1g @fernandopradocabrillo Gentle reminder for this one. As it is opened & discussed for long time are they still pending issue on your side preventing merging?

maxl2287 · 2024-07-10T16:51:45Z

LGTM thanks @bigludo7 👌🏻

code/API_definitions/sim-swap-subscriptions.yaml

Co-authored-by: Fernando Prado Cabrillo <fernando.pradocabrillo@telefonica.com>

fernandopradocabrillo

LGTM

fernandopradocabrillo · 2024-07-15T13:01:16Z

Thanks for the work on the subscriptios @bigludo7 !
Approved from my side, I don't see anything else. If something comes up we can fix it in the future

Update sim-swap-notification-subscription.yaml

e6be496

Align with CloudEvents model for subscription

bigludo7 requested a review from fernandopradocabrillo as a code owner June 5, 2024 14:29

bigludo7 added 5 commits June 5, 2024 16:45

Update sim-swap-notification-subscription.yaml

b558b3c

Fix MegaLinter

Update sim-swap-notification-subscription.yaml

00d6396

Fixes MegaLinter

Update sim-swap-notification-subscription.yaml

4e431a3

Fix MegaLinter issue

Update sim-swap-notification-subscription.yaml

8f274bf

Fix MegaLinter

Update sim-swap-notification-subscription.yaml

f38ef5e

Fix MegaLinter

bigludo7 mentioned this pull request Jun 7, 2024

Geofencing - Align with new model camaraproject/DeviceLocation#200

Closed

maxl2287 reviewed Jun 7, 2024

View reviewed changes

code/API_definitions/sim-swap-notification-subscription.yaml Show resolved Hide resolved

maxl2287 reviewed Jun 7, 2024

View reviewed changes

code/API_definitions/sim-swap-notification-subscription.yaml Outdated Show resolved Hide resolved

bigludo7 added 2 commits June 7, 2024 18:33

Update sim-swap-notification-subscription.yaml

88cd5ea

Updated after https://github.com/maxl2287 review.

Update sim-swap-notification-subscription.yaml

6ececbe

fix megaLinter issues

gregory1g reviewed Jun 10, 2024

View reviewed changes

code/API_definitions/sim-swap-notification-subscription.yaml Outdated Show resolved Hide resolved

gregory1g reviewed Jun 10, 2024

View reviewed changes

code/API_definitions/sim-swap-notification-subscription.yaml Outdated Show resolved Hide resolved

bigludo7 added 2 commits June 10, 2024 09:52

Update sim-swap-notification-subscription.yaml

d8f8fe3

Updated following @gregory1g proposal

Update sim-swap-notification-subscription.yaml

8cd1350

Added quote for phoneNumber in the examples.

maxl2287 reviewed Jun 17, 2024

View reviewed changes

code/API_definitions/sim-swap-notification-subscription.yaml Outdated Show resolved Hide resolved

Update sim-swap-notification-subscription.yaml

7117fd1

fixed DEACTIVE to INACTIVE

maxl2287 reviewed Jun 21, 2024

View reviewed changes

Update and rename sim-swap-notification-subscription.yaml to sim-swap…

6efb081

…-subscriptions.yaml Updated following Max review

maxl2287 approved these changes Jul 10, 2024

View reviewed changes

fernandopradocabrillo reviewed Jul 15, 2024

View reviewed changes

code/API_definitions/sim-swap-subscriptions.yaml Outdated Show resolved Hide resolved

fernandopradocabrillo self-requested a review July 15, 2024 08:11

Update code/API_definitions/sim-swap-subscriptions.yaml

65b51c8

Co-authored-by: Fernando Prado Cabrillo <fernando.pradocabrillo@telefonica.com>

fernandopradocabrillo approved these changes Jul 15, 2024

View reviewed changes

bigludo7 merged commit 5b43bf7 into main Jul 15, 2024

bigludo7 deleted the sim-swap-subscription branch July 15, 2024 13:22

bigludo7 mentioned this pull request Jul 24, 2024

Swap Notification example list phoneNumber value without quotes #113

Closed

Conversation

bigludo7 commented Jun 5, 2024 • 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!

github-actions bot commented Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦙 MegaLinter status: ✅ SUCCESS

Uh oh!

maxl2287 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

bigludo7 commented Jun 7, 2024

Uh oh!

maxl2287 commented Jun 7, 2024

Uh oh!

bigludo7 commented Jun 7, 2024

Uh oh!

Uh oh!

Uh oh!

maxl2287 commented Jun 12, 2024

Uh oh!

Uh oh!

maxl2287 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!

maxl2287 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bigludo7 commented Jul 2, 2024

Uh oh!

bigludo7 commented Jul 2, 2024

Uh oh!

bigludo7 commented Jul 5, 2024

Uh oh!

maxl2287 commented Jul 10, 2024

Uh oh!

Uh oh!

fernandopradocabrillo left a comment

bigludo7 commented Jun 5, 2024 •

edited

Loading

github-actions bot commented Jun 5, 2024 •

edited

Loading

maxl2287 commented Jun 7, 2024 •

edited

Loading

maxl2287 left a comment •

edited

Loading