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

Clarification for initialEvent use in documentation #237

Merged
merged 9 commits into from
Jan 23, 2025
Merged

Conversation

bigludo7
Copy link
Collaborator

What type of PR is this?

Add one of the following kinds:

  • documentation

What this PR does / why we need it:

Clarify behavior of subscription/event server regarding use of initialEvent
Clarify roaming country change behavior

Which issue(s) this PR fixes:

Fixes #222

Special notes for reviewers:

Adding @gmuratk as reviewer.

Changelog input

 release-note
device-reachability-status-subscriptions:
- update yaml documentation part for initialEvent
device-roaming-status-subscriptions:
- update yaml documentation part for initialEvent
- update yaml documentation part for initialEvent roaming country change behavior

Additional documentation

This section can be blank.

docs

Copy link

github-actions bot commented Dec 22, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ ACTION actionlint 2 0 0.02s
✅ OPENAPI spectral 6 0 10.13s
✅ REPOSITORY git_diff yes no 0.01s
✅ REPOSITORY secretlint yes no 0.8s
✅ YAML yamllint 6 0 1.28s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by OX Security


Following table illustrate behaviour regarding event triggering depending on **initial** reachability state of the device:

| subscrived event-type | device reachability status at subscription time | event send if ``initialEvent`` set to true |
Copy link
Contributor

Choose a reason for hiding this comment

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

minor typos in table header:
subscrived --> subscribed
event send --> event sent


Following table illustrate behaviour regarding event triggering depending on **initial** roaming state of the device:

| subscrived event-type | device roaming status at subscription time | event send if ``initialEvent`` set to true |
Copy link
Contributor

Choose a reason for hiding this comment

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

minor typos in table header:
subscrived --> subscribed
event send --> event sent


**Clarification on ``roaming-change-country`` event-type:**

``roaming-change-country`` event is send only when the device stays in roaming situation and change country.
Copy link
Contributor

Choose a reason for hiding this comment

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

event is 'sent'

Fix typos in table header
Fix typos
| reachability-sms | disconnected | No |
| reachability-disconnected | OK for Data or SMS usage | No |
| reachability-disconnected | disconnected | Yes |

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am afraid that table is not coming nicely on the swagger editor

Copy link
Collaborator

Choose a reason for hiding this comment

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

I took some time to interpret the table. Can we add some more information like (not necessary if others are fine with existing information):
Screenshot 2025-01-14 at 12 25 56

@akoshunyadi
Copy link
Collaborator

@bigludo7 only the new connected-network-type-subscriptions file is missing here, then we could merge it... 🙂

@bigludo7
Copy link
Collaborator Author

@akoshunyadi This is because I do not have in my branch and I'm not able te retreive it :(
Do you think we can merge this one and then I trigger a specific PR for connected-network-type-subscriptions ?

@akoshunyadi
Copy link
Collaborator

@bigludo7 sure we can do it.... but..is there a specific error when you want to update the branch?

@bigludo7
Copy link
Collaborator Author

@bigludo7 sure we can do it.... but..is there a specific error when you want to update the branch?

I would like to populate my branch with latest code but it trigger a PR to the project :(

@bigludo7
Copy link
Collaborator Author

@bigludo7 sure we can do it.... but..is there a specific error when you want to update the branch?

I would like to populate my branch with latest code but it triggers a PR to the project :(

@akoshunyadi
Copy link
Collaborator

that's weird, anyway then I review this one and the 3rd api will be done in a new PR

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.

LGTM.
Just a comment: checking the reachability status subscriptions file... it doesn't really fit now the reachability retrieve API, which supports both data and sms at the same time. Do we need a change for that? Not for Spring25, for sure.

@bigludo7
Copy link
Collaborator Author

Thanks @akoshunyadi
Will merge it tomorrow and craft a new one for the last api.

Fair question. I tend to think this is less relevant for the subscription than for the 'direct' request but in same time have same behavior btw both API is more consistent.

Let's open an issue to gather team feedback.

@bigludo7 bigludo7 merged commit 0bc9796 into main Jan 23, 2025
2 checks passed
@bigludo7 bigludo7 deleted the intialEventDoc branch January 23, 2025 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance API documentation
4 participants