Case Import Payment Validation #35803

zandre-eng · 2025-02-19T12:58:43Z

Product Description

If an Excel file import for the payment case type does not include headers for all the required fields, then the following error will be shown after continuing past step 2 of the import process:

Additionally, if the import contains rows that either does not have values for the required fields, or sets values for the no edit fields, then the following errors will be shown in the import summary:

Technical Summary

Link to ticket here.
Link to tech spec here.

This PR adds some extra validation to the case import workflow when a user is importing cases for the payment case type. This extra validation seeks to ensure that all the necessary information exists on the case for payment verification.

Feature Flag

MOBILE_USER_VERIFICATION

Safety Assurance

Safety story

Local testing done

Automated test coverage

Automated tests do exist.

QA Plan

No QA planned.

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

Charl1996 · 2025-02-20T06:55:08Z

corehq/apps/case_importer/views.py

+            return render_error(
+                request,
+                domain,
+                _('The Excel file is missing one or more required fields for the "{}" case type: {}'.format(


Very nit: The error message as is was a little surprising to me when it listed out the missing case types since the message didn't sound like it will do that.

How about changing the text to something below (in my mind it reads a little better)?

The Excel file is missing the following required fields for the "{}" case type: {}

(it is very nit, so feel free to ignore if you want, but I thought I'd mention in nonetheless)

Your suggestion does make the text feel a little more streamlined and easier to read. Happy to update this.

Addressed in 926eb08.

Additionally, I think the correct way to use the translation would be
_('The Excel file ... "{}" case type: {}').format()

@mkangia You're absolutely right. Here is a code ref that confirms how the translation should be. I'll update this.

Addressed translation formatting in 9da1701.

mkangia · 2025-02-20T17:53:11Z

corehq/toggles/__init__.py

@@ -3010,3 +3010,10 @@ def domain_has_privilege_from_toggle(privilege_slug, domain):
    tag=TAG_SOLUTIONS,
    namespaces=[NAMESPACE_DOMAIN],
 )
+
+MOBILE_WORKER_VERIFICATION = StaticToggle(


nit: if this is only for MTN I'd suggest renaming this to MTN_MOBILE_WORKER_VERIFICATION OR MTN_USER_VERIFICATION.

Renamed in d1d68f2.

mkangia · 2025-02-20T17:55:06Z

corehq/apps/case_importer/views.py

@@ -377,6 +379,16 @@ def excel_fields(request, domain):

    case_field_specs = [field_spec.to_json() for field_spec in field_specs]

+    if MOBILE_WORKER_VERIFICATION.enabled(domain) and case_type == MOMO_PAYMENT_CASE_TYPE:


Do we need to worry about bulk import?

Seeing at the function documentation

case_type: For a bulk import, this will be displayed as the special value for bulk case import/export, and the fetching of the case types will be handled in the next step

We should accommodate bulk imports too, as noted by this Jira comment.

It is true that a bulk import will have a special case type and so this check will fail, however we handle validation for bulk imports separately within the _create_bulk_configs() function.

so this check will fail

and so the columns wont be verified for a bulk import here but they are verified in _create_bulk_configs?

That is correct. The config for a bulk import only gets created in excel_commit() and that is where _create_bulk_configs() is called which will then validate for a bulk import.

does that catch missing columns as well or just checks for values that should not be set?

The check in _create_bulk_configs() will only validate that all the required headers are included. When checking for the values themselves, this will happen in the same place as a single case type import within do_import.py::_validate_payment_fields()

mkangia · 2025-02-20T17:56:20Z

corehq/apps/case_importer/views.py

@@ -377,6 +379,16 @@ def excel_fields(request, domain):

    case_field_specs = [field_spec.to_json() for field_spec in field_specs]

+    if MOBILE_WORKER_VERIFICATION.enabled(domain) and case_type == MOMO_PAYMENT_CASE_TYPE:
+        missing_fields = _check_payment_fields_exist(columns)


nit: since the function is return missing fields, I'd suggest renaming the function to _get_missing_payment_fields

Renamed in 523002f.

mkangia · 2025-02-20T17:59:13Z

corehq/apps/case_importer/views.py

+            return render_error(
+                request,
+                domain,
+                _('The Excel file is missing one or more required fields for the "{}" case type: {}'.format(


Additionally, I think the correct way to use the translation would be
_('The Excel file ... "{}" case type: {}').format()

mkangia · 2025-02-20T18:00:12Z

corehq/apps/case_importer/views.py

+
+
+def _check_payment_fields_exist(columns):
+    return [


would it be better to use sets here to find the missing fields?

Do you mean by doing something as follows?

return set(MOMO_REQUIRED_PAYMENT_FIELDS) - set(columns)

From a performance perspective the difference will be neglible for a list this small, but using sets does make the code footprint a tad less and perhaps a bit easier to read. I'll update this.

Changed in 985e235.

corehq/apps/case_importer/views.py

mkangia · 2025-02-20T18:04:17Z

corehq/apps/case_importer/do_import.py

@@ -169,6 +175,12 @@ def _do_import(self, spreadsheet):
    def import_row(self, row_num, raw_row, import_context):
        search_id = self._parse_search_id(raw_row)
        fields_to_update = self._populate_updated_fields(raw_row)
+        if (


is there a reason for this check to be during import and not earlier when uploading file?
is it because this could take really long so we don't want to do it then?

My main motivation for doing it here is that this is the part in the import where we're looping through all the sheet rows, and so we're able to easily see if there are cell values with empty required fields.

Doing this at an earlier point would mean we would have to do an extra loop through all the rows to validate their values which felt unnecessary. Additionally, failing it here simply results in the row not getting imported. This therefore then doesn't block the entire import and other rows can still get imported if they're valid.

mkangia · 2025-02-21T10:09:01Z

corehq/apps/case_importer/views.py

@@ -297,7 +297,7 @@ def _create_bulk_configs(domain, request, case_upload):
    errors = []
    for index, title in enumerate(worksheet_titles):
        with case_upload.get_spreadsheet(index) as spreadsheet:
-            _, excel_fields, _ = _process_excel_mapping(


double underscores!

Yeah these were changed to double underscores otherwise they overshadow the gettext import which is imported as _.

If you think the double underscores don't look right, I can always change these to have variable names instead 🤔

mkangia · 2025-02-21T10:10:28Z

corehq/apps/case_importer/do_import.py

@@ -227,6 +262,10 @@ def _perform_custom_case_import_operations(self, row_num, raw_row, fields_to_upd
    def user(self):
        return CouchUser.get_by_user_id(self.config.couch_user_id)

+    @cached_property


mkangia

Looking good.

Just wanted to check once about testing on this. We have tested that this does not impact imports outside the feature flag i.e the blast radius is limited to only the feature flagged domains?

zandre-eng · 2025-02-21T10:36:31Z

Just wanted to check once about testing on this. We have tested that this does not impact imports outside the feature flag i.e the blast radius is limited to only the feature flagged domains?

@mkangia With local testing everything looks fine. I tested with the following workflows:

Standard single case type import.
Single case type payment import.
- This includes checking for errors with headers and missing values.
Bulk import including payment case type.
Standard bulk import.

All the code added in this PR sits behind the FF conditional, and so if it were to break anything I expect this would only be when (a) the FF is enabled and (b) a user is attempting a payment import.

zandre-eng added 4 commits February 19, 2025 13:53

create feature flag

16c0438

validate required fields exist

4b7401b

validate required fields exist for bulk import

80fb505

validate field values

24971cd

zandre-eng added the product/feature-flag Change will only affect users who have a specific feature flag enabled label Feb 19, 2025

zandre-eng requested review from kaapstorm, mkangia, Charl1996 and ajeety4 February 19, 2025 12:58

dimagimon added the Risk: High Change affects files that have been flagged as high risk. label Feb 19, 2025

zandre-eng marked this pull request as ready for review February 19, 2025 12:58

Charl1996 reviewed Feb 20, 2025

View reviewed changes

minor text update

926eb08

mkangia requested changes Feb 20, 2025

View reviewed changes

zandre-eng added 3 commits February 21, 2025 11:48

fix translation formatting

9da1701

update ff name

d1d68f2

update function name

523002f

mkangia reviewed Feb 21, 2025

View reviewed changes

zandre-eng added 2 commits February 21, 2025 12:24

use sets to check for missing fields

985e235

remove test code

bfaf1b0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Case Import Payment Validation #35803

Case Import Payment Validation #35803

zandre-eng commented Feb 19, 2025

Charl1996 Feb 20, 2025

zandre-eng Feb 20, 2025

mkangia Feb 20, 2025

zandre-eng Feb 21, 2025

zandre-eng Feb 21, 2025

mkangia Feb 20, 2025

zandre-eng Feb 21, 2025

mkangia Feb 20, 2025

zandre-eng Feb 21, 2025

mkangia Feb 21, 2025

zandre-eng Feb 21, 2025

mkangia Feb 21, 2025

zandre-eng Feb 21, 2025

mkangia Feb 20, 2025

zandre-eng Feb 21, 2025

mkangia Feb 20, 2025

mkangia Feb 20, 2025

zandre-eng Feb 21, 2025

mkangia Feb 21, 2025

zandre-eng Feb 21, 2025

zandre-eng Feb 21, 2025

mkangia Feb 20, 2025

zandre-eng Feb 21, 2025

mkangia Feb 21, 2025

zandre-eng Feb 21, 2025

zandre-eng Feb 21, 2025

mkangia Feb 21, 2025

mkangia left a comment

zandre-eng commented Feb 21, 2025

		@@ -377,6 +379,16 @@ def excel_fields(request, domain):

		case_field_specs = [field_spec.to_json() for field_spec in field_specs]

		if MOBILE_WORKER_VERIFICATION.enabled(domain) and case_type == MOMO_PAYMENT_CASE_TYPE:

Case Import Payment Validation #35803

Are you sure you want to change the base?

Case Import Payment Validation #35803

Conversation

zandre-eng commented Feb 19, 2025

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Rollback instructions

Labels & Review

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

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkangia left a comment

Choose a reason for hiding this comment

zandre-eng commented Feb 21, 2025