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

KYC Config Page #35716

Merged
merged 13 commits into from
Feb 4, 2025
Merged

KYC Config Page #35716

merged 13 commits into from
Feb 4, 2025

Conversation

zandre-eng
Copy link
Contributor

@zandre-eng zandre-eng commented Jan 30, 2025

Product Description

A new "KYC Configuration" page is added to the "Data" sidebar:
Screenshot from 2025-01-30 23-35-53

This new page shows the configuration settings for setting up KYC for a domain:
Screenshot from 2025-02-04 08-44-59

Please note that there is an extra "Provider" input on this page, however this is hidden due to there only currently being one available provider.

Upon clicking "Save", a new alert banner will be shown to the user if the form was successfully saved:
Screenshot from 2025-02-04 08-45-08

Technical Summary

Link to ticket here.
Link to design doc here.
Link to tech spec here.

This PR adds a new configuration page for the KYC feature. Specifically, this page will be used to set up the following:

  • Where to fetch the necessary verification data from
  • How the data should be mapped for the KYC API
  • Which provider API is being used
  • What connection settings to use for the API

Feature Flag

KYC_VERIFICATION

Safety Assurance

Safety story

  • Local testing done
  • Unit tests exist

Automated test coverage

New unit tests have been created to verify that a user cannot access the configuration page if they are not logged in or if the domain does not have access to the feature flag.

QA Plan

No QA planned.

Migrations

  • The migrations in this code can be safely applied first independently of the code

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

@zandre-eng zandre-eng added the product/feature-flag Change will only affect users who have a specific feature flag enabled label Jan 30, 2025
@dimagimon dimagimon added the reindex/migration Reindex or migration will be required during or before deploy label Jan 30, 2025
x_init='provider = $el.value',
x_show='showProvider',
),
crispy.Field(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An open question I have is how to make the JSON field more user-friendly. Currently, it is simply a text box with JSON validation which can be difficult to correclty format JSON in. What makes this challenging is that the crispy form is loaded in as a partial HTML template, and so importing something like the base_ace JSON editor in the parent template is not possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

I took a quick look at QuillJS, which is currently being evaluated.

Would it be possible to load it in corehq/apps/integration/templates/kyc/kyc_config_base.html?

It looks like it has syntax highlighting for code blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay it looks like I managed to successfully load the base_ace JSON editor (8bc0145). The tricky part was with saving the form. When this happens the partial gets loaded again, causing the JSON editor to be removed. I managed to work around this by initializing the editor again after the form gets saved.

@zandre-eng zandre-eng marked this pull request as ready for review January 30, 2025 22:09
api_field_to_user_data_map = JsonField(
label=_('API Field to User Data Map'),
required=True,
expected_type=list,
Copy link
Contributor

Choose a reason for hiding this comment

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

The model field is api_field_to_user_data_map = jsonfield.JSONField(default=dict). Should this be dict? i.e.

Suggested change
expected_type=list,
expected_type=dict,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The model field has been updated to take list as a default type here. The expected format for the API mapping will be something like this:

[
  ...
  {
    "fieldName": "foo",
    "mapsTo": "bar",
    "source": "standard"
  }
  ...
]

x_init='provider = $el.value',
x_show='showProvider',
),
crispy.Field(
Copy link
Contributor

Choose a reason for hiding this comment

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

I took a quick look at QuillJS, which is currently being evaluated.

Would it be possible to load it in corehq/apps/integration/templates/kyc/kyc_config_base.html?

It looks like it has syntax highlighting for code blocks.

Copy link
Contributor

@kaapstorm kaapstorm left a comment

Choose a reason for hiding this comment

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

This is great. I just have the one question about list vs dict.

corehq/toggles/__init__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ajeety4 ajeety4 left a comment

Choose a reason for hiding this comment

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

Looks clean and clear.

form = self.config_form
show_success = False
if form.is_valid():
form.save(commit=True)
Copy link
Contributor

@Charl1996 Charl1996 Feb 3, 2025

Choose a reason for hiding this comment

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

I presume the fact that the form is a ModelForm will persist the model to the database with commit=True?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct. The save() function's help text reads "Save this form's self.instance object if commit=True...". I also verified this in my local testing by checking that the instance is actually getting saved with this function call.

@Charl1996
Copy link
Contributor

Few nits, but otherwise good!

Copy link
Contributor

@Charl1996 Charl1996 left a comment

Choose a reason for hiding this comment

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

Thank you for updating!

@zandre-eng zandre-eng merged commit ea10a9c into master Feb 4, 2025
14 checks passed
@zandre-eng zandre-eng deleted the ze/kyc-config-page branch February 4, 2025 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/feature-flag Change will only affect users who have a specific feature flag enabled reindex/migration Reindex or migration will be required during or before deploy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants