KYC Service Layer #35774

kaapstorm · 2025-02-13T23:18:54Z

Technical Summary

Jira: SC-4128, SC-4139
Base branch: #35765

This change adds a service layer to the KYC integration that makes API calls and parses the responses.

Feature Flag

KYC_VERIFICATION

Safety Assurance

Safety story

We are unable to test API calls yet. We are waiting for a service provider to grant us access to the API.

Automated test coverage

Tests included. More tests to follow once we have settled on return values.

Rollback instructions

This PR can be reverted after deploy with no further considerations

Labels & Review

Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change

Form validation still works because it is a ModelForm.

kaapstorm · 2025-02-17T14:19:38Z

To avoid confusion, please don't merge until #35765 has been merged.

ajeety4 · 2025-02-18T07:14:00Z

settings.py

@@ -1172,6 +1172,14 @@ def _pkce_required(client_id):
 # used by periodic tasks that delete soft deleted data older than PERMANENT_DELETION_WINDOW days
 PERMANENT_DELETION_WINDOW = 30  # days

+# Used by `corehq.apps.integration.kyc`. Override in localsettings.py
+MTN_KYC_CONNECTION_SETTINGS = {


(I am probably behind on recent updates with MTN )
What is the rationale for having a common connection settings ? With this not be project specific ?

It turns out that the API service provider will have a contract with the platform provider (i.e. Dimagi), and will grant access to them, and not to each project ...

... which has just made me think that saving the ConnectionSettings instance is not a good idea. Projects/users can't read the secrets of ConnectionSettings instances, but they can change them. So either we need to have a way to write-protect ConnectioinSettings, or we need to create them on the fly without persisting them.

corehq/apps/integration/kyc/tests/test_models.py

ajeety4 · 2025-02-18T07:21:53Z

corehq/apps/integration/kyc/models.py

@@ -52,6 +53,21 @@ class Meta:
            models.UniqueConstraint(fields=['domain', 'provider'], name='unique_domain_provider'),
        ]

+    def clean(self):


corehq/apps/integration/kyc/models.py

ajeety4 · 2025-02-18T07:32:55Z

corehq/apps/integration/kyc/services.py

+def verify_users(user_objs, config):
+    # TODO: An endpoint to verify a group of users does not seem to be
+    #       available using Chenosis. If we have to do this with
+    #       multiple calls, consider using Celery gevent workers.


ajeety4 · 2025-02-18T07:36:20Z

corehq/apps/integration/kyc/services.py

+    for user_obj in user_objs:
+        is_verified = verify_user(user_obj, config)
+        save_result(user_obj, config, is_verified)
+        results.append(is_verified)


I feel like having just verification results is not very helpful to the caller. Consider adding an identifier (e.g. user_id) along with the verification result.

results.append( { 'user_id': 'abc', 'is_verified': True, } )

ajeety4 · 2025-02-18T07:40:28Z

corehq/apps/integration/kyc/services.py

+            config.domain,
+            case.case_id,
+            case_properties=update,
+            device_id='corehq.apps.integration.kyc.services.save_result',


nit: Consider using device_id=__name__ + '.save_result'

ajeety4 · 2025-02-18T07:59:35Z

corehq/apps/integration/kyc/models.py

@@ -18,6 +20,9 @@ class UserDataStore(object):


 class KycProviders(models.TextChoices):
+    # When adding a new provider:


It would be good to have a short documentation for steps to add a new provider.
Could be done in the follow up work along with TODOs.

ajeety4

Thanks Norman for working on this.
This is looking quite good. Left some minor queries and suggestions.

Charl1996 · 2025-02-18T09:37:17Z

corehq/apps/integration/kyc/services.py

+
+    # TODO: Determine what thresholds we want
+    required_thresholds = {
+        'firstName': 100,


@kaapstorm How do these scores function?

It looks like a percentage of how close the value that was submitted is to the value that MTN has on record. We haven't been able to test this yet, because we don't (yet) have credentials we can use for testing.

Charl1996 · 2025-02-18T09:56:09Z

Just a note:

I see a lot of places where we check the user_data_store on the KycProviders config. This makes me think that something like the adapter pattern might come in handy here where the service makes use of the adapter to interact with the various underlying data store models. That way we could always just query the adapter for data-store-specific actions/data instead of having if-else blocks. The adapter pattern will also make is very easy to add new data stores, since we would only have to implement the new adapter with the various ways in which data can be read from/written to this potentially new data store without having to update if-else blocks.

That said, I think the approach in this PR is OK as is since I don't think this particular feature will see new data stores being added frequently (if ever).

If a field is in a user's usercase, or custom user data, then use that, otherwise fall back to a CommCareUser property. Only allow safe properties.

Safe mapping

ajeety4 · 2025-02-20T08:31:16Z

corehq/apps/integration/kyc/services.py

+
+def save_result(user_obj, config, is_verified):
+    update = {
+        'kyc_provider': config.provider.value,  # TODO: Or config.provider.label?


config.provider.value does not work as provider is the actual string value.

Suggested change

'kyc_provider': config.provider.value, # TODO: Or config.provider.label?

'kyc_provider': config.provider, # TODO: Or config.provider.get_provider_display()?

I am bit inclined towards storing the value here in case we want to make a conditional check on this later.

kaapstorm · 2025-02-20T14:42:54Z

the adapter pattern might come in handy here

I think this is a good idea. It might be out of scope for this PR, but worth looking at in a follow-up PR.

kaapstorm added the product/invisible Change has no end-user visible impact label Feb 13, 2025

kaapstorm requested review from zandre-eng and ajeety4 February 13, 2025 23:18

kaapstorm force-pushed the nh/kyc/service branch from da063e7 to 171bf21 Compare February 16, 2025 19:00

dimagimon added the reindex/migration Reindex or migration will be required during or before deploy label Feb 16, 2025

Add KycConfig.get_connection_settings()

fc323ea

kaapstorm force-pushed the nh/kyc/service branch from 1dd08f4 to 6e02dc4 Compare February 16, 2025 22:55

kaapstorm added 2 commits February 16, 2025 17:59

_row_data() -> KycConfig.get_user_objects()

929185b

Move clean() from form to model

4ea342f

Form validation still works because it is a ModelForm.

kaapstorm force-pushed the nh/kyc/service branch from 6e02dc4 to 65f119b Compare February 17, 2025 00:03

API call to verify user

143f16c

kaapstorm force-pushed the nh/kyc/service branch from 65f119b to 143f16c Compare February 17, 2025 13:52

kaapstorm added 2 commits February 17, 2025 08:54

Move tests

4ae1201

KycConfig.get_user_objects_by_ids()

1d2c951

kaapstorm changed the base branch from master to ze/kyc-verify-report February 17, 2025 14:16

kaapstorm marked this pull request as ready for review February 17, 2025 14:18

ajeety4 reviewed Feb 18, 2025

View reviewed changes

corehq/apps/integration/kyc/tests/test_models.py Show resolved Hide resolved

ajeety4 reviewed Feb 18, 2025

View reviewed changes

corehq/apps/integration/kyc/models.py Show resolved Hide resolved

ajeety4 reviewed Feb 18, 2025

View reviewed changes

Charl1996 reviewed Feb 18, 2025

View reviewed changes

Base automatically changed from ze/kyc-verify-report to master February 18, 2025 10:45

Default to "custom", fall back to "standard"

7666d11

If a field is in a user's usercase, or custom user data, then use that, otherwise fall back to a CommCareUser property. Only allow safe properties.

kaapstorm and others added 2 commits February 18, 2025 18:01

Drop "source"

b06292d

Merge pull request #35804 from dimagi/nh/mapping

945da40

Safe mapping

ajeety4 reviewed Feb 20, 2025

View reviewed changes

kaapstorm added 2 commits February 20, 2025 09:45

config.provider is the string value

d709e5d

Don't save system ConnectionSettings

7697748

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KYC Service Layer #35774

KYC Service Layer #35774

kaapstorm commented Feb 13, 2025 •

edited by jira bot

Loading

kaapstorm commented Feb 17, 2025

ajeety4 Feb 18, 2025

kaapstorm Feb 20, 2025

ajeety4 Feb 18, 2025

ajeety4 Feb 18, 2025

ajeety4 Feb 18, 2025 •

edited

Loading

ajeety4 Feb 18, 2025

ajeety4 Feb 18, 2025

ajeety4 left a comment

Charl1996 Feb 18, 2025 •

edited

Loading

kaapstorm Feb 20, 2025

Charl1996 commented Feb 18, 2025

ajeety4 Feb 20, 2025

ajeety4 Feb 20, 2025

kaapstorm commented Feb 20, 2025

		@@ -18,6 +20,9 @@ class UserDataStore(object):


		class KycProviders(models.TextChoices):
		# When adding a new provider:

	'kyc_provider': config.provider.value, # TODO: Or config.provider.label?
	'kyc_provider': config.provider, # TODO: Or config.provider.get_provider_display()?

KYC Service Layer #35774

Are you sure you want to change the base?

KYC Service Layer #35774

Conversation

kaapstorm commented Feb 13, 2025 • edited by jira bot Loading

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

Rollback instructions

Labels & Review

kaapstorm commented Feb 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajeety4 Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajeety4 left a comment

Choose a reason for hiding this comment

Charl1996 Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Charl1996 commented Feb 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaapstorm commented Feb 20, 2025

kaapstorm commented Feb 13, 2025 •

edited by jira bot

Loading

ajeety4 Feb 18, 2025 •

edited

Loading

Charl1996 Feb 18, 2025 •

edited

Loading