Refactor user import, support user data in import and fix profile data bug #33890

sravfeyn · 2023-12-18T12:19:05Z

Product Description

This is a Refactor https://dimagi-dev.atlassian.net/browse/USH-3613 and a bug fix https://dimagi-dev.atlassian.net/browse/USH-2762

Technical Summary

https://dimagi-dev.atlassian.net/browse/USH-3613

Broken up the long function methods into classes to improve reusability and (hopefully) better readability.

Feature Flag

NA

Safety Assurance

Safety story

I have tested the import locally and there is a good amount of test cases for the code that's refactored.

Automated test coverage

corehq.apps.user_importer.tests

QA Plan

Perhaps not required since the basic import works and there is enough test coverage

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

sravfeyn · 2023-12-21T06:29:50Z

@esoergel

esoergel

Things are looking much better overall - I had difficulty figuring out what changed since it's so much code all at once, but I had some hopefully-useful comments. Any idea how good test coverage is on this?

esoergel · 2023-12-22T15:31:45Z

corehq/apps/user_importer/importer.py

+        current = 0
+        for row in self.user_specs:


Nit: you could use enumerate here instead of incrementing it manually

Suggested change

current = 0

for row in self.user_specs:

for i, row in enumerate(self.user_specs):

esoergel · 2023-12-22T15:42:54Z

corehq/apps/user_importer/importer.py

+        self.domain = domain
+        self.is_web_upload = is_web_upload
+
+    @property


I think all of the @propertys in this class should probably be @cached_propertys, right? Otherwise they'll be called for every row in the import.

That's right, updated it.

esoergel · 2023-12-22T16:00:35Z

corehq/apps/user_importer/importer.py

+    ).run()
+
+
+class UserRowMixin:


Nit: this isn't really a mixin, but a shared base class - there's not multiple inheritance.

esoergel · 2023-12-22T16:10:11Z

corehq/apps/user_importer/importer.py

+        except UserUploadError as e:
+            self.status_row['flag'] = str(e)


Should this go in WebImporter.run instead? Looks like this only catches UserUploadErrors for web users, but I see in the old version except UserUploadError as e: is also in create_or_update_commcare_users_and_groups

(It's still here in commcare_user version as well).

Yes, we can move it, though we still have to have a similar line for CouchUser.Inconsistent for CommCareUser, so it's not a super-worthy change

esoergel · 2023-12-22T16:28:28Z

corehq/apps/user_importer/importer.py

+        self._parse_password()
+        return True
+
+    def _set_user_importer(self):


This method sets properties outside of __init__ which introduces unneeded temporal coupling (this method must be called before you can use those vars). Instead, would you consider making user and commcare_user_importer into @cached_propertys? Then they'd be lazily initialized

That's a good point, updated.

esoergel · 2023-12-22T17:05:36Z

corehq/apps/user_importer/importer.py

-        deactivate_after = row.get('deactivate_after', None) if update_deactivate_after_date else None
-        if isinstance(deactivate_after, datetime):
-            deactivate_after = deactivate_after.strftime("%m-%Y")
-            row['deactivate_after'] = deactivate_after


I like how you moved this logic to where it's actually needed

esoergel · 2023-12-22T17:41:14Z

corehq/apps/user_importer/importer.py

+        self.status_row['row']['password'] = password
+        if self.column_values['user_id'] and is_password(password):
+            self.status_row['row']['password'] = 'REDACTED'


Why does this check if user_id? Shouldn't it always redact passwords, also for new users? It looks like the old version of the code doesn't set password at all if the user ID isn't present.

Why does this check if user_id? Shouldn't it always redact passwords, also for new users?

I was using the older logic. I guess, we could redact. I refactored without changing the logic, I will update.

It looks like the old version of the code doesn't set password at all if the user ID isn't present.

Right, we are still doing the same, no?

There is this test that's failing corehq.apps.user_importer.tests.test_importer:TestMobileUserBulkUpload.test_password_is_not_string when I skip the user_id check. I am not sure why that's the way it is. I will rather leave the logic as is.

It looks like the old version of the code doesn't set password at all if the user ID isn't present.

Right, we are still doing the same, no?

I meant line 505 here, where password is added to the status row - just want to be sure there's no situation where we might store the password in plaintext.

self.status_row['row']['password'] = password

Would it be possible to just delete that line or initialize it to "" or something?

It looks like the status_row data is used only for communicating back to the uploading user when the password is not valid. If it is a valid password and it's set then it gets REDACTED. That's my understanding of the old logic. But of course there should be no problem to set that to empty regardless of whether the password is valid or not. I have updated it as such.

esoergel

apologies for the delay in getting back to the review - looks like this is about done

esoergel · 2024-01-09T19:26:33Z

corehq/apps/user_importer/importer.py

+        self.status_row['row']['password'] = password
+        if self.column_values['user_id'] and is_password(password):
+            self.status_row['row']['password'] = 'REDACTED'


It looks like the old version of the code doesn't set password at all if the user ID isn't present.

Right, we are still doing the same, no?

I meant line 505 here, where password is added to the status row - just want to be sure there's no situation where we might store the password in plaintext.

self.status_row['row']['password'] = password

Would it be possible to just delete that line or initialize it to "" or something?

esoergel · 2024-01-09T19:46:22Z

corehq/apps/user_importer/tests/test_importer.py

@@ -695,6 +695,28 @@ def test_user_data_profile_blank(self):
            PROFILE_SLUG: self.profile.id,
        })

+    def test_required_field_optional_if_profile_set(self):


esoergel · 2024-01-09T19:55:13Z

Oh whoops, one note - this line in the description is no longer accurate:

This is a Refactor without any product changes https://dimagi-dev.atlassian.net/browse/USH-3613

This PR now fixes that user data profile bug

Support web user data update via bulk upload

sravfeyn · 2024-01-22T08:42:22Z

@esoergel I have updated the description and also merged in the other PR into this #33930

gherceg · 2024-02-02T23:20:43Z

@sravfeyn is it fair to say you can now remove the HELPME comment in the create_or_update_commcare_users_and_groups method? I imagine it no longer triggers an 'E' or 'F' classification in static analysis.

sravfeyn requested a review from esoergel December 18, 2023 12:19

dimagimon added the Risk: Medium Change affects files that have been flagged as medium risk. label Dec 18, 2023

sravfeyn force-pushed the sr/refusers branch 4 times, most recently from be2ad1f to 5fbb769 Compare December 19, 2023 12:51

Refactor user import code

21957e1

sravfeyn force-pushed the sr/refusers branch from 5fbb769 to 21957e1 Compare December 21, 2023 06:24

sravfeyn changed the title ~~[WIP] Refactor user import code~~ Refactor user import code Dec 21, 2023

sravfeyn marked this pull request as ready for review December 21, 2023 06:29

sravfeyn requested a review from stephherbers as a code owner December 21, 2023 06:29

esoergel reviewed Dec 22, 2023

View reviewed changes

PR Feedback

b19e19a

sravfeyn force-pushed the sr/refusers branch from a816022 to b19e19a Compare December 28, 2023 05:33

sravfeyn added 5 commits December 28, 2023 13:04

Fix required-fields validation bug for profile fields

9a190c4

dump userdata in webuser bulk download

2f2198b

refactor webuser importer code

e3e5788

Merge branch 'sr/refusers' into sr/webuserdata

1e418e4

Support web user data update via bulk upload

de266b4

sravfeyn mentioned this pull request Dec 30, 2023

Support web user data update via bulk upload #33930

Merged

3 tasks

esoergel approved these changes Jan 9, 2024

View reviewed changes

don't show password in response

2473d8f

sravfeyn changed the title ~~Refactor user import code~~ Refactor user import code and fix profile data bug Jan 22, 2024

sravfeyn added 2 commits January 22, 2024 14:06

refactor setup_users

e95ba67

Merge pull request #33930 from dimagi/sr/webuserdata

12d4687

Support web user data update via bulk upload

sravfeyn changed the title ~~Refactor user import code and fix profile data bug~~ Refactor user import, support user data in import and fix profile data bug Jan 22, 2024

esoergel approved these changes Jan 23, 2024

View reviewed changes

sravfeyn merged commit bbe7d35 into master Jan 24, 2024
11 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor user import, support user data in import and fix profile data bug #33890

Refactor user import, support user data in import and fix profile data bug #33890

sravfeyn commented Dec 18, 2023 •

edited

Loading

sravfeyn commented Dec 21, 2023

esoergel left a comment

esoergel Dec 22, 2023

sravfeyn Dec 26, 2023

esoergel Dec 22, 2023

sravfeyn Dec 26, 2023

esoergel Dec 22, 2023

esoergel Dec 22, 2023

sravfeyn Dec 26, 2023 •

edited

Loading

esoergel Dec 22, 2023

sravfeyn Dec 26, 2023

esoergel Dec 22, 2023

esoergel Dec 22, 2023

sravfeyn Dec 26, 2023

sravfeyn Dec 28, 2023

esoergel Jan 9, 2024

sravfeyn Jan 22, 2024

esoergel left a comment

esoergel Jan 9, 2024

esoergel Jan 9, 2024

esoergel commented Jan 9, 2024

sravfeyn commented Jan 22, 2024 •

edited

Loading

gherceg commented Feb 2, 2024

	current = 0
	for row in self.user_specs:
	for i, row in enumerate(self.user_specs):

		except UserUploadError as e:
		self.status_row['flag'] = str(e)

Refactor user import, support user data in import and fix profile data bug #33890

Refactor user import, support user data in import and fix profile data bug #33890

Conversation

sravfeyn commented Dec 18, 2023 • edited Loading

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Rollback instructions

Labels & Review

sravfeyn commented Dec 21, 2023

esoergel left a 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

sravfeyn Dec 26, 2023 • edited Loading

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

esoergel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esoergel commented Jan 9, 2024

sravfeyn commented Jan 22, 2024 • edited Loading

gherceg commented Feb 2, 2024

sravfeyn commented Dec 18, 2023 •

edited

Loading

sravfeyn Dec 26, 2023 •

edited

Loading

sravfeyn commented Jan 22, 2024 •

edited

Loading