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

cleaning up repo #48

Merged
merged 5 commits into from
Sep 3, 2023
Merged

Conversation

DT-DawidWroblewski
Copy link
Collaborator

@DT-DawidWroblewski DT-DawidWroblewski commented Jul 26, 2023

What type of PR is this?

  • cleanup

What this PR does / why we need it:

clean up repo

Which issue(s) this PR fixes:

Fixes #27
Fixes #26
Fixes #25
Fixes #10

Special notes for reviewers:

cleans up repo

Changelog input

Removed MobileConnect definition
Created reference to Mobile Connect in CAMARA API

Additional documentation

Copy link

@patrice-conil patrice-conil left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Not sure if the reference to Mobile Connect clarifies anything about usage or will distract the developer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mobile Connect APIs exists in API universe and this CAMARA API derives from it (Mobile Connect Account Takeover Protection were used in discussions around this product). CAMARA aims to deliver a solution for broad community of developers, so let's stick to it when crafting this API

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reference makes sense from my perspective.

Text proposed by @diegogonmar and @bigludo7 in camaraproject/NumberVerification#51 (comment) now used for that.

@@ -1,6 +1,7 @@
# Overview

The SIM Swap API provides a programmable interface for developers and other users (capabilities consumers) to request the last date of a SIM swap performed on the mobile line, or, to check whether a SIM swap has been performed during a past period.
This API derives from the GSMA Mobile Connect Account Takeover Protection specification [Mobile Connect Account Takeover Protection](https://www.gsma.com/identity/wp-content/uploads/2022/12/IDY.24-Mobile-Connect-Account-Takeover-Protection-Definition-and-Technical-Requirements-v2.0.pdf). For more about Mobile Connect, please see [about Mobile Connect](https://mobileconnect.io/).

Choose a reason for hiding this comment

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

Same remark

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same as previous comment

Copy link
Collaborator

Choose a reason for hiding this comment

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

Plese review @diegogonmar comments on this topic raised in Number Verification too: camaraproject/NumberVerification#51 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

commented there as well...
comment

Copy link
Collaborator

Choose a reason for hiding this comment

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

Text as proposed in camaraproject/NumberVerification#51 (comment) is now used as reference within YAML and MD. Consolidating the .md based documentation into the .yaml to be done in a separate issue/PR.

# Overview

The SIM Swap API provides a programmable interface for developers and other users (capabilities consumers) to request the last date of a SIM swap performed on the mobile line, or, to check whether a SIM swap has been performed during a past period.
This API derives from the GSMA Mobile Connect Account Takeover Protection specification [Mobile Connect Account Takeover Protection](https://www.gsma.com/identity/wp-content/uploads/2022/12/IDY.24-Mobile-Connect-Account-Takeover-Protection-Definition-and-Technical-Requirements-v2.0.pdf). For more about Mobile Connect, please see [about Mobile Connect](https://mobileconnect.io/).

Choose a reason for hiding this comment

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

Idem


Depending on the network provider implementation, legal enforcement, etc... either one or both resources could be implemented.

__Note__: SIM swap is covered as well in the Mobile Connect (MC) ATP (Account-Takeover-Protection) process. The MC pattern is exposed in another Camara API from the SIM Swap API family. Network providers are free to implement the MC version, this version, or both.

Choose a reason for hiding this comment

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

Idem

documentation/API_documentation/README.MD Outdated Show resolved Hide resolved

Please note, the credentials for API authentication purposes need to be adjusted based on target security system configuration.

| Snippet 1. Request last simswap date |

Choose a reason for hiding this comment

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

Nice work of documentation, I like the snippets.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, great idea!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is good!

Copy link

@diegogonmar diegogonmar left a comment

Choose a reason for hiding this comment

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

Similar comments as in this PR: camaraproject/NumberVerification#51

@@ -1,6 +1,7 @@
# Overview

The SIM Swap API provides a programmable interface for developers and other users (capabilities consumers) to request the last date of a SIM swap performed on the mobile line, or, to check whether a SIM swap has been performed during a past period.
This API derives from the GSMA Mobile Connect Account Takeover Protection specification [Mobile Connect Account Takeover Protection](https://www.gsma.com/identity/wp-content/uploads/2022/12/IDY.24-Mobile-Connect-Account-Takeover-Protection-Definition-and-Technical-Requirements-v2.0.pdf). For more about Mobile Connect, please see [about Mobile Connect](https://mobileconnect.io/).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Plese review @diegogonmar comments on this topic raised in Number Verification too: camaraproject/NumberVerification#51 (comment)


SIM Swap API provides the customer the ability to obtain information on any recent SIM pairing change related to the User's mobile account.

This API derives from the GSMA Mobile Connect Account Takeover Protection specification [Mobile Connect Account Takeover Protection](https://www.gsma.com/identity/wp-content/uploads/2022/12/IDY.24-Mobile-Connect-Account-Takeover-Protection-Definition-and-Technical-Requirements-v2.0.pdf). For more about Mobile Connect, please see [about Mobile Connect](https://mobileconnect.io/).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @DT-DawidWroblewski! I have been reviewing the discussions that have been made concerning this topic and in this issue camaraproject/WorkingGroups#200 it was concluded that the MC proposal should be removed since it's only causing confusion and noise in the repo. This is also the solution agreed in the GSMA workshop.

The relation with MC should be in the documentation .md file and not in the yaml. Yaml is API spec and has to stick to API Definition.

@diegogonmar left comments regarding this topic that should be applied here too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

According to what has been approved in the working groups discussion please, remove this part from the yaml since it is already compiled in the documentation md file documentation/API_documentation/Check_sim_swap_API.md

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi!

if we're aiming to have only yaml file that describes API, then it should contain a proper description, that includes reference to Mobile Connect.

Afterwards, we can delete markdown, although I think that we should keep it as MD files works perfectly to provide additional content (e.g. pictures, diagrams, etc.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tend to be agreed with @fernandopradocabrillo. If we keep there mention of Mobile Connect while the API did not fulfill MC pattern, don't you think @DT-DawidWroblewski that it can trigger confusion for application developer, WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

As above: Text as proposed in camaraproject/NumberVerification#51 (comment) is now used as reference within YAML and MD. Consolidating the .md based documentation into the .yaml to be done in a separate issue/PR.

@@ -1,6 +1,7 @@
# Overview

The SIM Swap API provides a programmable interface for developers and other users (capabilities consumers) to request the last date of a SIM swap performed on the mobile line, or, to check whether a SIM swap has been performed during a past period.
This API derives from the GSMA Mobile Connect Account Takeover Protection specification [Mobile Connect Account Takeover Protection](https://www.gsma.com/identity/wp-content/uploads/2022/12/IDY.24-Mobile-Connect-Account-Takeover-Protection-Definition-and-Technical-Requirements-v2.0.pdf). For more about Mobile Connect, please see [about Mobile Connect](https://mobileconnect.io/).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This API derives from the GSMA Mobile Connect Account Takeover Protection specification [Mobile Connect Account Takeover Protection](https://www.gsma.com/identity/wp-content/uploads/2022/12/IDY.24-Mobile-Connect-Account-Takeover-Protection-Definition-and-Technical-Requirements-v2.0.pdf). For more about Mobile Connect, please see [about Mobile Connect](https://mobileconnect.io/).
This API considers as a source input the GSMA Mobile Connect Account Takeover Protection specification [Mobile Connect Account Takeover Protection](https://www.gsma.com/identity/wp-content/uploads/2022/12/IDY.24-Mobile-Connect-Account-Takeover-Protection-Definition-and-Technical-Requirements-v2.0.pdf). For more about Mobile Connect, please see [about Mobile Connect](https://mobileconnect.io/).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Text as proposed in camaraproject/NumberVerification#51 (comment) is now used as reference within YAML and MD. Consolidating the .md based documentation into the .yaml to be done in a separate issue/PR.

Choose a reason for hiding this comment

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

Great thanks. Ok to consolidating .md in different PR


SIM Swap API provides the customer the ability to obtain information on any recent SIM pairing change related to the User's mobile account.

This API derives from the GSMA Mobile Connect Account Takeover Protection specification [Mobile Connect Account Takeover Protection](https://www.gsma.com/identity/wp-content/uploads/2022/12/IDY.24-Mobile-Connect-Account-Takeover-Protection-Definition-and-Technical-Requirements-v2.0.pdf). For more about Mobile Connect, please see [about Mobile Connect](https://mobileconnect.io/).
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to what has been approved in the working groups discussion please, remove this part from the yaml since it is already compiled in the documentation md file documentation/API_documentation/Check_sim_swap_API.md

@@ -75,10 +37,8 @@ Described inside yaml files.
|No|Version|Changelog|
|:---:|---:|:---|
|1|0.5.0|Camara & Mobile Connect flavours available|
|2|0.6.0|Camara only version<br>(Mobile Connect archived)|
Copy link
Collaborator

Choose a reason for hiding this comment

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

@DT-DawidWroblewski We fully believe that, according what has been agreed, the reference to MC should remain just as a source of input and with these changes we state perfectly that relation. We think it is better to remove all files, if you consider that, for history purposes, it should remain in the changelog, please consider this wording.

Suggested change
|2|0.6.0|Camara only version<br>(Mobile Connect archived)|
|2|0.6.0|Camara only version<br>(Mobile Connect proposal removed. Moved as supporting documentation to documentation/SupportingDocuments/Removed/MobileConnect)|

Copy link
Collaborator

Choose a reason for hiding this comment

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

One thing no note here, current version of the API is 0.4.0, is the changelog correct? I'm probably missing something but, as it is right now, I think it's not clear enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd suggest to use following statement here:

|2|0.6.0|Camara only version
(Mobile Connect moved as supporting documentation to documentation/SupportingDocuments/MobileConnect)|

as well as delete the yaml file and leave MC specifications only as a referenced/supporting docs

Choose a reason for hiding this comment

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

Include the reference is fine, but there is no point in maintaining MC specifications in this repo. A reference is a link to a place where documentation is, replicating such documentation in this repo has no utility, will cause confusion and will become obsolete. For example for dates we use RFC3339 and we reference it, but we don't have a copy of such RFC in the commonalities repo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

MC spec removed now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

File path should be: documentation/SupportingDocuments/Removed/MobileConnect/MC_ATP.yaml

Copy link

@diegogonmar diegogonmar Aug 31, 2023

Choose a reason for hiding this comment

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

Files should be removed actually

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

Choose a reason for hiding this comment

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

File path should be: documentation/SupportingDocuments/Removed/MobileConnect/MobileConnectATP.md

Choose a reason for hiding this comment

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

Files should be removed actually

Copy link
Collaborator

@fernandopradocabrillo fernandopradocabrillo Aug 11, 2023

Choose a reason for hiding this comment

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

CAMARA folder no longer needed, the path for the Check_sim_swap_API.md should be: documentation/API_documentation/Check_sim_swap_API.md

I was thinking that we can even explore the option of removing the documentation/API_documentation/README.md file completely and incorporate the relevant information into Check_sim_swap_API.md. What do you think @patrice-conil @DT-DawidWroblewski?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not the biggest fan of having only yaml as a documentation. If we can provide a sufficient documentation artifacts, like pictures, diagrams, code snippets inside yaml, then I'm fine with keeping only yaml.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@DT-DawidWroblewski this is a commonalities decision. Looks on QoD API - seems to me that we can provide nice documentation with picture, etc...

Copy link

@diegogonmar diegogonmar Aug 31, 2023

Choose a reason for hiding this comment

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

Yes, I also didn't realize that when I raised some comments about .md. We have to follow commonalities, so please lets move forward with removing these to have the repo clean, while in parallel we agree with the reference wording

Copy link
Collaborator

Choose a reason for hiding this comment

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

Consolidating the .md based documentation into the .yaml to be done in a separate issue/PR. It would overload the purpose of this PR.

Choose a reason for hiding this comment

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

Agreed


Depending on the network provider implementation, legal enforcement, etc... either one or both resources could be implemented.

__Note__: SIM swap is covered as well in the Mobile Connect (MC) ATP (Account-Takeover-Protection) process. The MC pattern is exposed in another Camara API from the SIM Swap API family. Network providers are free to implement the MC version, this version, or both.
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to what has been agreed, this information is no longer valid and should be removed too

documentation/API_documentation/README.MD Outdated Show resolved Hide resolved
documentation/API_documentation/README.MD Outdated Show resolved Hide resolved
@engst03
Copy link
Collaborator

engst03 commented Aug 14, 2023 via email

@fernandopradocabrillo
Copy link
Collaborator

Hi @DT-DawidWroblewski! Please take a look at the proposed changes to continue with the implementation and complete de clean up.

Thanks!!

DT-DawidWroblewski and others added 2 commits August 30, 2023 17:04
Co-authored-by: Fernando Prado Cabrillo <pradocabrillo.fernando@gmail.com>
ok

Co-authored-by: Fernando Prado Cabrillo <pradocabrillo.fernando@gmail.com>
@diegogonmar
Copy link

diegogonmar commented Aug 31, 2023

Dear all, We are just integrating the SimSwap API from https://github.com/camaraproject/SimSwap/blob/main/documentation/API_documentation/CAMARA/Check_sim_swap_API.md and have one question regarding the response: response will be: 200 -d '{ "swapped": "false" }' In your sample the boolean value “swapped” is actually of type string. Is that a typo or intentionally? Best Regards

Hi @engst03 . This looks like a typo in the .md. This .md file will be removed, the normative file is the .yaml and there it's fine, its a boolean

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.

Let's go with this 'massive clean up' and then we will see in detail in we are well aligned between SimSwap, numberVerification & OTP.

@@ -18,10 +18,10 @@ info:
license:
name: Apache 2.0
url: https://www.apache.org/licenses/LICENSE-2.0.html
version: 0.4.0
version: 0.6.1

Choose a reason for hiding this comment

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

Why version 0.6.1 ? I would suggest maintainging 0.4.0 as no changes in interface but only documentation. 0.4.1 could be another option, no strong opinion

Copy link

@diegogonmar diegogonmar left a comment

Choose a reason for hiding this comment

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

Fine with the approach discussed and agreed, thanks for the effort @DT-DawidWroblewski and @hdamker.
Just one last comment, PR moves version from 0.4.0 to 0.6.1, which looks strange. Please fix that, thanks

@DT-DawidWroblewski DT-DawidWroblewski merged commit a544bf6 into camaraproject:main Sep 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants