camaraproject / KnowYourCustomer

Repository to describe, develop, document and test the KnowYourCustomer API family
Apache License 2.0
9 stars 9 forks source link

Consolidated KYC Match API definition #27

Closed ToshiWakayama-KDDI closed 9 months ago

ToshiWakayama-KDDI commented 9 months ago

What type of PR is this?

What this PR does / why we need it:

EfthymisIsaakidis-DTCS commented 9 months ago

Hello Colleagues, Regarding parameter naming convention, we (DT group) believe it would be better to use the same notation with GSMA Mobile Connect KYC Match, in other words "snake notation" (thus use underscore "_") instead of "camel notation". This will lead to a uniform approach more compatible with that of GSMA. For example: |attribute|Match Request|Match Response| |e.g. given name | given_name | given_name_match |

EfthymisIsaakidis-DTCS commented 9 months ago

Hello Colleagues, Regarding the match result response specifications, we (DT group) believe it would be better to define and use the same values with GSMA Mobile Connect KYC Match: “Y”– match is successful, “N-NA” - match failed, data is not available, “N-AV” – match failed; data is available, “N-AD” – match failed, data is available, but access is denied. This will lead to a uniform approach and will also cover the "access denied" scenario.

GillesInnov35 commented 9 months ago

@EfthymisIsaakidis-DTCS, I'm sorry but I don't agree with your proposition because as it is for all CAMARA API design, we must be compliant with Camara commonalities guidelines https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md

at §4.2 Input/Output Resource Definition it is mentionned that _API input and output business data will follow the camelCase notation. and underscore is never used in the other API field notation. thanks BR Gilles

fernandopradocabrillo commented 9 months ago

Agree with @GillesInnov35

We have to follow CAMARA guidelines as it is done in the rest of the API families.

ToshiWakayama-KDDI commented 9 months ago

Hi @EfthymisIsaakidis-DTCS, Thanks for your comment about parameter naming, but we have already agreed to follow the camelCase notation. As others mentioned, we have to follow Camara commonalities guidelines.

Thank you for your understanding.

Best regards, Toshi

ToshiWakayama-KDDI commented 9 months ago

Hi @EfthymisIsaakidis-DTCS , Thank you for your comments about match result response, but I have to advise you that we have been discussing this long, and we agreed at the meeting on Tuesday 12th December, as below, which is also described in Issue #21.

I forgot to update Issue #21, which may confuse you, so I will update it shortly as it was agreed on 12th December.

So, @EfthymisIsaakidis-DTCS , sorry, but if you propose the modification, please do that for our future releases.

Thank you for your cooperation.

Best regards, Toshi

EfthymisIsaakidis-DTCS commented 9 months ago

Hello Colleagues, Thank you for your feedback on parameter naming convention. Considering your comments and the Section 4.2 in Commonalities we have no objections to follow your suggestion on this issue.

Best Regards, Efthymis

EfthymisIsaakidis-DTCS commented 9 months ago

Hello @ToshiWakayama-KDDI ,

Thank you for your feedback about match result response. I acknowledge that my comment comes somehow late, but I do think that here we are taking a decision that we may need to change in the future . The restriction of access in some of the user's info attributes may be needed in some use cases depending on different laws and regulations in different countries as well. GSMA specs have considered this. Of course I agree that we proceed as already agreed and discuss this modification in a future release, if and when it is needed, but it may be more difficult and "costly" to do the change later.

BR, Efthymis

ToshiWakayama-KDDI commented 9 months ago

Hi @EfthymisIsaakidis-DTCS ,

Thank you, but we have agreed the match result response for our initial version, and we have to complete our initial YAML by Tuesday 19th. If we restart match result response discussion, it will take more time and never end. Especially you mentioned the user's info attributes, and we decided to delay user's info attributes discussion till future releases. I understand your point, but why don't we complete our first version once as a basis, and then I think we can modify and refine it in an agile fashion to make it better. As you know, if we look at some other APIs in CAMARA, they have had several update versions. This is my current view.

Best regards, Toshi

ToshiWakayama-KDDI commented 9 months ago

Hi @EfthymisIsaakidis-DTCS , Thank you for your comments about match result response, but I have to advise you that we have been discussing this long, and we agreed at the meeting on Tuesday 12th December, as below, which is also described in Issue #21.

  • true: successful match
  • false: failed match
  • not_available: data is not available

I forgot to update Issue #21, which may confuse you, so I will update it shortly as it was agreed on 12th December.

So, @EfthymisIsaakidis-DTCS , sorry, but if you propose the modification, please do that for our future releases.

Thank you for your cooperation.

Best regards, Toshi

Hi @EfthymisIsaakidis-DTCS, Regarding your previous question on Match Response, I have an additional comment. I think our current KYC Match can cover your scenarios (MC scenarios?) as below:

Thanks. Toshi

GillesInnov35 commented 9 months ago

@ToshiWakayama-KDDI , thanks a lot for your work. For information it seems that at line 296 there's a little mistake. A ' is missing after 'not_available Do you confirm ? BR Gilles

ToshiWakayama-KDDI commented 9 months ago

Hi @fernandopradocabrillo ,

Thank you for your comments. Too many... I have some questions in order for me to accept your change proposals.

(1) Change 'application/json;charset=utf-8' to 'application/json' I think that there is a recent commonalities discussion (Issue #112) related this point. It is stated that, although utf-8 is the default character set, adding 'charset=utf-8' is optionally ok. But as it is still ongoing and not concluded, I would like to add 'charset=utf-8' just to make sure.

Are you uncomfortable with this (adding charset=utf-8)?

(2) x-correlator in header I have checked the CAMARA API Desing Guidelines, but I am not quite sure if 'x-correlator' is mandatory and if it is required for KYC Match and KYC Fill-in APIs. So, I prefer to not include 'x-correlator'. Do you think we need to include it?

Many thanks, Toshi

ToshiWakayama-KDDI commented 9 months ago

HIi @fernandopradocabrillo ,

description: Address of the customer. This field can be the combination of multiple partial address parameters as streetName and streetNumber. It should not include street type.

For my clarification, some MNOs including KDDI use the Address field as one address string, but you prefer to use this as the combination of partial address parameters, so, you want to add that your case is possible. Is this correct understanding?

As we do not have the 'street type' attribute, there is no need for "It should not include street type", I think.

Many thanks, Toshi

fernandopradocabrillo commented 9 months ago

Hi @ToshiWakayama-KDDI

(1) Change 'application/json;charset=utf-8' to 'application/json' I think that there is a recent commonalities discussion (Issue #112) related this point. It is stated that, although utf-8 is the default character set, adding 'charset=utf-8' is optionally ok. But as it is still ongoing and not concluded, I would like to add 'charset=utf-8' just to make sure.

Are you uncomfortable with this (adding charset=utf-8)?

I wasn't aware of that issue in commonalities! It seems that the comments goes in the line of not including the charset unless it's something different from UTF-8. I think it is still a bit early to include it, it doesn't seem matured enough (in my opinion).

(2) x-correlator in header I have checked the CAMARA API Desing Guidelines, but I am not quite sure if 'x-correlator' is mandatory and if it is required for KYC Match and KYC Fill-in APIs. So, I prefer to not include 'x-correlator'. Do you think we need to include it?

There has been multiple issues in commonalities regarding the x-correlator topic, this is the last one: https://github.com/camaraproject/Commonalities/issues/101

Eric from Vodafone was the one that raised the doubt about including it or not and, as per his last comment, he is finally inclined to include it. If you take a look, it is already in most of the CAMARA APIs, I think only Device Status and Sim swap still don't have it, but at least in the latter a PR is already created. So the only thing left is to adapt the guidelines, which I think will be done shortly.

For my clarification, some MNOs including KDDI use the Address field as one address string, but you prefer to use this as the combination of partial address parameters, so, you want to add that your case is possible. Is this correct understanding?

No, I didn't explain myself correctly. I think that this field is similar to the given name parameter. In essence it is the combination of other partial parameters. What is an address composed of? A street name, a street number, a locality, a region, a country, a postal code, etc I think we should define at least a very basic rules of normalization, for example not including the street_type (street, avenue, park, square, etc)

ToshiWakayama-KDDI commented 9 months ago

Hi @fernandopradocabrillo,

Thank you for your reply.

I wasn't aware of that issue in commonalities! It seems that the comments goes in the line of not including the charset unless it's something different from UTF-8. I think it is still a bit early to include it, it doesn't seem matured enough (in my opinion).

So, are you saying that you are not comfortable with adding 'charset=utf-8', even though we should use UTF-8 as the default characterset? If so, it is OK to remove 'charset=utf-8'.

There has been multiple issues in commonalities regarding the x-correlator topic, this is the last one: https://github.com/camaraproject/Commonalities/issues/101

Eric from Vodafone was the one that raised the doubt about including it or not and, as per his last comment, he is finally inclined to include it. If you take a look, it is already in most of the CAMARA APIs, I think only Device Status and Sim swap still don't have it, but at least in the latter a PR is already created. So the only thing left is to adapt the guidelines, which I think will be done shortly.

Thanks. I will check Commonalities Issue #101. In the meantime, it is OK for me to include the x-correlator. Do in Roma, as the Romans do. But if I find something different, please let us revisit this issue.

No, I didn't explain myself correctly. I think that this field is similar to the given name parameter. In essence it is the combination of other partial parameters. What is an address composed of? A street name, a street number, a locality, a region, a country, a postal code, etc I think we should define at least a very basic rules of normalization, for example not including the street_type (street, avenue, park, square, etc)

We have not discussed normalization yet, so I do not think I can agree with it.
Maybe we could add like this: description: Address of the customer. This field can be used for the whole address but also can be the combination of multiple partial address parameters as streetName and streetNumber.

What do you think?

Thanks.

ToshiWakayama-KDDI commented 9 months ago

Hi @fernandopradocabrillo , Hi @GillesInnov35

I have committed all the change proposals created by Fernando except description of Address of the customer. I think the proposal for desrciption of Address of the customer is related to normalisation, so it should be delayed to future discussion.

And I have corrected some errors detected by the Swagger Editor.

So, I think the Match YAML proposal can be merged now.

Many thanks, Toshi

fernandopradocabrillo commented 9 months ago

Hi @ToshiWakayama-KDDI

So, are you saying that you are not comfortable with adding 'charset=utf-8', even though we should use UTF-8 as the default characterset? If so, it is OK to remove 'charset=utf-8'.

Yes, I prefer to not explicitly include it yet BUT I'm of course open to add it later if it is decided in commonalities. For now we can just asume it is utf-8 like in the rest of the APIs. I'm checking internally if our system supports including the charset explicitly at the moment becasue I'm not sure.

We have not discussed normalization yet, so I do not think I can agree with it. Maybe we could add like this: description: Address of the customer. This field can be used for the whole address but also can be the combination of multiple partial address parameters as streetName and streetNumber.

What do you think?

Yes, I'm okey with this description. The street_type thing was just to make the usecase a little bit more specific so the client knows what to send. As "4 Privet Drive" is not the same as "4 Privet Drive Street". But I'm okey discussing it later.

Thanks!

ToshiWakayama-KDDI commented 9 months ago

Hi @fernandopradocabrillo , HI @GillesInnov35

I have committed additonal change requests and have just done Swagger check. Please approve this PR, so that I can merge it.

Many thanks, Toshi

ToshiWakayama-KDDI commented 9 months ago

HI @fernandopradocabrillo , Hi @GillesInnov35 ,

Could you please approve this.

Thanks, Toshi

ToshiWakayama-KDDI commented 9 months ago

HI @fernandopradocabrillo , Hi @GillesInnov35 ,

Could you please approve this.

Thanks, Toshi

fernandopradocabrillo commented 9 months ago

Thanks @ToshiWakayama-KDDI for the effort

ToshiWakayama-KDDI commented 9 months ago

Hi @fernandopradocabrillo , Hi @GillesInnov35 .

As my last comment above was at 02.20 am, I could not wait for Fernando's approval yesterday. I have just merged the PR. Thank you so much for your cooperation. I will inform GSMA of this.

If you could approve the Fill-in one, PR #28, without delay, it would be much appreciated.

Many thanks, Toshi