Create Invitation API [Validation] #35448

Jtang-1 · 2024-11-26T21:31:14Z

Product Description

Technical Summary

The main change in this PR is the addition of an API validation file. This will support the upcoming API for creating Invitations and updating Web Users. The remaining changes involve refactoring to consolidate validations used when bulk uploading Web Users or creating Invitations via the AdminInvitesUserForm.

I reused relevant validations already defined for bulk user import (in user_importer.validation) since the same checks are needed. Some validations previously implemented via the clean method in Django forms were consolidated, while built-in form validations were left as-is.

The key distinction between Invitation creation via the API and via bulk user import is how existing Web Users are handled. Bulk user import checks if the Web User exists and updates it if so. In contrast, the API will have a dedicated POST endpoint for creation. This endpoint will require validating whether the Invitation or Web User already exists and returning an error if it does, mirroring the behavior of the AdminInvitesUserForm.

Feature Flag

No feature flag

Safety Assurance

Safety story

The main changes (addition of validation for Web User creation API) will not be used yet. The changes to the existing validation code are solely refactors and I tested locally that those areas still work as expected.

Automated test coverage

There exists extensive testing on bulk user imports and surrounding validation. test_importer checks that web user and mobile workers can be successfully uploaded/updated. test_validators test that Location, Profile, Custom Data, etc.. validation catches invalid or unauthorized changes. I will add missing Tableau Group and Role validation.

QA Plan

no QA.

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

…aEditor Form

…idating Invitation creation via API

…eck_headers

… / web user edit API

…to make it easier to tell what values are needed

…pdate web user API

…assigned locations or that primary location is defined if assigned locations is defined

…d format from spec

… used for invite creation API

… of the spec format in preparation for reuse

…for web user API. However, there wasn't as much overlap with validation used for AdminInvitesUserForm. So move validation for Web User Resource its own file and create a class to organize the validations relevant to AdminInvitesUserForm

…isting webuser

…mn headers"

AddisonDunn

Love what you're doing here with the refactoring. Was only able to do a partial review so far.

corehq/apps/user_importer/importer.py

corehq/apps/api/tests/test_validation.py

corehq/apps/registration/forms.py

AddisonDunn · 2024-12-02T19:17:13Z

corehq/apps/registration/forms.py

-            raise forms.ValidationError(_("A user with this email address is deactivated. "))
+
+        from corehq.apps.registration.validation import AdminInvitesUserFormValidator
+        error = AdminInvitesUserFormValidator.validate_email(self.domain, email, self.request.method == 'POST')


If an email is being cleaned, isn't this always a POST? Or at least an edit method where you'd want to do this sort of email validation?

Also, I see there is no original call to django.core.validators.validate_email here? Agree that that sort of validation should be done here, aghast that it isn't already.

Lastly sidenote, not asking for this here and wasn't part of this PR obviously but WebUserInvitationForm needs a better name cause this Form architecture is confusing everytime I look at it. Maybe WebUserInvitationValidationForm.

If an email is being cleaned, isn't this always a POST?

you're right, it's always a POST. Removed the is_post parameter here. Also, I realized django.core.validators.validate_email is already done as part of EmailField definition. So I also removed that from AdminInvitesUserFormValidator.validate_email

WebUserInvitationForm needs a better name cause this Form architecture is confusing everytime I look at it. Maybe WebUserInvitationValidationForm.

I'm not sure what you mean by this? I don't think it makes sense to rename WebUserInvitationForm to WebUserInvitationValidationForm

I'm not sure what you mean by this? I don't think it makes sense to rename WebUserInvitationForm to WebUserInvitationValidationForm

The name makes me think it is the form for inviting a web user. But that's AdminInvitesUserForm, this form is for just validating an accepted invitation from what I can tell. Maybe AcceptedWebUserInvitationForm.

oh i see, I agree that's confusing 06dd234

corehq/apps/user_importer/importer.py

AddisonDunn · 2024-12-02T23:36:27Z

corehq/apps/users/tests/test_views.py

+        with patch('corehq.apps.users.views.mobile.users.BaseUploadUser.upload_users'):
+            response = self._make_post_request(file)
+            self.assertEqual(response.status_code, 200)
+            self.assertEqual(response.json(), {'success': True})


I am seeing some of the code for uploading via the API now... with the validation in this PR it seems like we're pretty close to having a complete solution. The spec from July obviously doesn't account for a web user API that uses bulk upload.. which sounds perfectly valid to me. I assume you are planning for us to use that same bulk upload/excel sheet functionality? I think that's probably best but gotta say from the D&A side it will be a bit of a curveball to upload using a CSV.

Well I see you wrote all that code for WebUserResource. I would hate to suggest not using it. It would be a bit easier on the D&A side to use normal pagination/user. Perhaps we could continue this conversation in Jira or slack.

Just copying over what we talked about on slack - I don't plan to use the bulk upload functionality. I plan to implement it as a normal API to be consistent with how it's done for mobile workers

AddisonDunn · 2024-12-02T23:54:25Z

corehq/apps/user_importer/validation.py

@@ -150,12 +151,16 @@ class TableauRoleValidator(ImportValidator):

    def __init__(self, domain):
        super().__init__(domain)
-        self.valid_role_options = [e.value for e in TableauUser.Roles]


Do you need to remove this from __init__ to get the classmethod to work? I think having this line here saves on memory for the import.

Good point, that's preferable to keep it in __init__ 11edd76

corehq/apps/user_importer/validation.py

AddisonDunn · 2024-12-03T00:21:41Z

corehq/apps/user_importer/validation.py

@@ -513,30 +547,18 @@ def _validate_uploading_user_access(self, spec):

        # 2. Ensure the user is only adding the user to/removing from *new locations* that they have permission
        # to access.
-        if 'location_code' in spec:
-            locs_being_assigned = self._get_locs_being_assigned(spec)
+        if locs_ids_being_assigned:


With this change the check that a user has access to the locations being removed will be skipped if the user tries to remove all of them. (blocking if true)

I would reccomend a second variable:

user_is_editing_locations = 'location_code' in spec

Thank you for catching that bug, fixed it a5ee207

I think this is equivalent - I do the location_code' in spec within validate_spec and only do the validations if that's true. For the snippet referenced here, I removed the conditional check so that validation will always run.

So the LocationValidator doesn't just validate that the uploading user has ability to change the locations for another user, IIRC it needs to validate that the uploading user has access to the other user at all. I.e. if the uploading user has access to location 1 and 2 they should not be able to update the profile for a user assigned to just location 3.

So the problem here is that checking 'location_code' in spec skips that validation. Sorry this code is such a pain. I think what might be best is to extract #1 from _validate_uploading_user_access and do that before everything else, and then everything else can go under if 'location_code' in spec.

Oh dang, thanks for catching that. 8fee5fe should fix it. Although I added some nasty looking variable and function names that i'm open to renaming.

Looks good. Almost like reading English (almost). I don't think they're nasty, IMO much better than lumping all the logic in one giant method like some code I've seen. Some method names I'd even make longer.

Also note-to-self: if you have "#1" and "#2" blocks a method, consider just breaking it into 2 different methods.

jingcheng16

Only did partial review. Left a few comments. Nothing blocking.

jingcheng16 · 2024-12-03T20:10:09Z

corehq/apps/registration/forms.py

Can you link which part of CustomDataEditor form already did the validation you deleted?

See my reply here. The validation is done here

jingcheng16 · 2024-12-03T20:18:05Z

corehq/apps/registration/validation.py

@@ -40,3 +51,12 @@ def validate_profile(self, new_profile_name):
        profile_validator = ProfileValidator(self.domain, self.upload_user, True, self.profiles_by_name())
        spec = {'user_profile': new_profile_name}
        return profile_validator.validate_spec(spec)
+
+    def validate_email(self, email, is_post):
+        if is_post:


Why is_post flag is required here? What can happen if we don't have this parameter?

The is_post is needed in preparation for it being used for the Create Invitation / Update Web User API. When we're creating a new invitation, we need to check that the email isn’t already associated with an existing user or invitation. But if we’re updating it, we already know the user or invitation exists, so running the same check would always fail.

jingcheng16 · 2024-12-03T20:29:58Z

corehq/apps/users/validation.py

+
+
+def validate_assigned_locations_has_users(domain, assigned_location_ids):
+    error_message_location_not_has_users = _("These locations cannot have users assigned because of their "


Shouldn't we remove error_message_location_not_has_users from LocationValidator class?

yes you're right, thanks
d3b2d3a

…ocations_has_users

…post" parameter is redundant. Also, EmailField already builds in EmailValidator. So the additional validation check done in the bulk import EmailValidator is redundant when cleaning email from the form

AddisonDunn

Most of the way there with this PR. This is some cool refactoring.

corehq/apps/user_importer/tests/test_validators.py

corehq/apps/users/validation.py

AddisonDunn · 2024-12-04T22:46:03Z

corehq/apps/api/validation.py

+            return error
+
+        location_validator = LocationValidator(self.domain, self.requesting_user, self.location_cache, True)
+        location_codes = list(set(assigned_location_codes + [primary_location_code]))


Doesn't really apply to this PR but I'm just sort of curious... @jingcheng16 isn't there some thing with mobile worker location assignment where the primary location needs to come first in an import or in the API? I vaguely remember some discussion a while back about that.

Doesn't look like for web user import the primary needs to come first in the set of codes, and certainly there is no validation for this in bulk import at the moment.

IIRC, for both mobile and web user bulk import, it assumes that the first location listed is the primary location. So there's no validation involves, it just assumes and updates/assigns the primary location as such.

corehq/apps/api/validation.py

corehq/apps/api/tests/test_validation.py

…cations are being removed locs_ids_being_assigned is empty/falsy)

validation needs to be done that uploading user can access the current locations of the editable user or invitation.

…re changed

AddisonDunn · 2024-12-05T20:48:06Z

corehq/apps/user_importer/tests/test_validators.py

@@ -355,6 +355,10 @@ def test_cant_edit_web_user(self):
        validation_result = self.validator.validate_spec(user_spec)
        assert validation_result == self.validator.error_message_user_access

+        user_spec = {'username': self.editable_user.username}
+        validation_result = self.validator.validate_spec(user_spec)
+        assert validation_result == self.validator.error_message_user_access


AddisonDunn · 2024-12-05T20:59:10Z

corehq/apps/user_importer/validation.py

@@ -513,30 +547,18 @@ def _validate_uploading_user_access(self, spec):

        # 2. Ensure the user is only adding the user to/removing from *new locations* that they have permission
        # to access.
-        if 'location_code' in spec:
-            locs_being_assigned = self._get_locs_being_assigned(spec)
+        if locs_ids_being_assigned:


Looks good. Almost like reading English (almost). I don't think they're nasty, IMO much better than lumping all the logic in one giant method like some code I've seen. Some method names I'd even make longer.

Also note-to-self: if you have "#1" and "#2" blocks a method, consider just breaking it into 2 different methods.

AddisonDunn

Nicely done. Some excellent refactoring done here.

The changes to the existing validation code are solely refactors and I tested locally that those areas still work as expected

Nice. Given the size of this PR, I reccomend re-testing areas that have had recent commits if you haven't already, and testing the forms that were changed, too (if you haven't already).

AddisonDunn · 2024-12-05T21:01:22Z

corehq/apps/users/tests/test_user_invitation.py

@@ -110,7 +110,7 @@ def test_redirect_if_invite_email_does_not_match(self):
        print(form.errors)
        self.assertTrue(form.is_valid())

-        form = WebUserInvitationForm(
+        form = AcceptedWebUserInvitationForm(


Jtang-1 · 2024-12-06T07:43:37Z

Thanks for the review @AddisonDunn! I sanity tested admin invite and bulk upload validation surrounding locations and didn't run into any issues.

dimagimon added Risk: High Change affects files that have been flagged as high risk. Risk: Medium Change affects files that have been flagged as medium risk. labels Nov 26, 2024

Jtang-1 force-pushed the jt/create-user-api-validation branch 4 times, most recently from 6568200 to 1a81553 Compare November 28, 2024 05:02

Jtang-1 added 24 commits November 27, 2024 21:11

create class to consolidate validation of web user invitation fields

2de602f

this validation is already done as part of django form from CustomDat…

1f42d95

…aEditor Form

refactor for readability

b4c260c

adds function to validate profile

2873fcb

refactor validation of email to consolidate

06e60e1

do same validation as done for bulk user import in preparation of val…

591245a

…idating Invitation creation via API

refactor: move validating tableau role and tableau group header to ch…

3d99f35

…eck_headers

prefactor - consolidate validation to be used for invitation creation…

9e801be

… / web user edit API

refactor: extract required values from spec in higher level function …

a0119f6

…to make it easier to tell what values are needed

create validation for location in preparetion for create invitation/u…

0fc4ddd

…pdate web user API

refactor: location_ids is the equivalent return

fc6ced8

refactor: consolidate validation for location having users

30a3214

refactor: consolidate validating that primary location is one of the …

2e1de11

…assigned locations or that primary location is defined if assigned locations is defined

refactor to extract out validation of tableau groups from the expecte…

7584aea

…d format from spec

add validation of tableau group to Admin Invites User Validator to be…

21fd7ac

… used for invite creation API

prefactor: extract out the validation of the tableau_role independent…

ff73910

… of the spec format in preparation for reuse

add validation for tableau role

cd79129

add validation for custom data in preparation for web user creation API

062dde5

remove unnecessary caching

f0cd397

refactor: DRY function

ef2b0e6

perform permission and privilege check only if the parameter exists

a043006

remove location fields from backend if location shouldn't be shown

cca4098

update test and adds test for validating if invitation email is an ex…

7f3807e

…isting webuser

test: new tests for validation of Web User Resourcec

d9f55a1

Jtang-1 marked this pull request as ready for review November 30, 2024 05:15

Jtang-1 requested review from esoergel and stephherbers as code owners November 30, 2024 05:15

Jtang-1 requested review from AddisonDunn, Robert-Costello and jingcheng16 November 30, 2024 05:15

Jtang-1 added 2 commits November 29, 2024 22:47

update test so that order doesn't matter for the listed "illegal colu…

427420a

…mn headers"

update test so it doesn't matter the order of the arguments

c2df916

AddisonDunn reviewed Dec 3, 2024

View reviewed changes

jingcheng16 reviewed Dec 3, 2024

View reviewed changes

Jtang-1 added 6 commits December 4, 2024 11:46

delete unused error message. It is now handled in validate_assigned_l…

d3b2d3a

…ocations_has_users

correct patch, path should be where function is used

eff4a17

For form validation, clean only occurs if request is post so the "is_…

6f0ec77

…post" parameter is redundant. Also, EmailField already builds in EmailValidator. So the additional validation check done in the bulk import EmailValidator is redundant when cleaning email from the form

fix argument

ab7e8a7

fix test - property was moved out of the class

d698b22

avoid regenerating list of roles on each call to reduce memory

11edd76

AddisonDunn reviewed Dec 4, 2024

View reviewed changes

Jtang-1 added 6 commits December 4, 2024 15:25

bug: needs to check user has access to existing locations when all lo…

a5ee207

…cations are being removed locs_ids_being_assigned is empty/falsy)

rename function for clarity

33159f5

remove unneccessary model deletion from test database

da73efa

clarify naming to distinguish from the form to invite web users

06dd234

correct bug where regardless if locations are being changed,

8fee5fe

validation needs to be done that uploading user can access the current locations of the editable user or invitation.

test: tests that invitiation access is checked even if no locations a…

3dfa157

…re changed

AddisonDunn reviewed Dec 5, 2024

View reviewed changes

AddisonDunn approved these changes Dec 5, 2024

View reviewed changes

Jtang-1 changed the title ~~Jt/create user api validation~~ Create Invitation API [Validation] Dec 6, 2024

Jtang-1 merged commit cbac4e4 into master Dec 6, 2024
13 checks passed

Jtang-1 deleted the jt/create-user-api-validation branch December 6, 2024 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Invitation API [Validation] #35448

Create Invitation API [Validation] #35448

Jtang-1 commented Nov 26, 2024 •

edited

Loading

AddisonDunn left a comment

AddisonDunn Dec 2, 2024

AddisonDunn Dec 2, 2024

AddisonDunn Dec 2, 2024

Jtang-1 Dec 4, 2024

AddisonDunn Dec 4, 2024

Jtang-1 Dec 5, 2024

AddisonDunn Dec 5, 2024

AddisonDunn Dec 2, 2024

AddisonDunn Dec 3, 2024

Jtang-1 Dec 4, 2024 •

edited

Loading

AddisonDunn Dec 2, 2024

Jtang-1 Dec 4, 2024

AddisonDunn Dec 3, 2024 •

edited

Loading

AddisonDunn Dec 3, 2024

Jtang-1 Dec 4, 2024

AddisonDunn Dec 5, 2024

Jtang-1 Dec 5, 2024

AddisonDunn Dec 5, 2024

jingcheng16 left a comment

jingcheng16 Dec 3, 2024

Jtang-1 Dec 4, 2024

jingcheng16 Dec 3, 2024

Jtang-1 Dec 4, 2024

jingcheng16 Dec 3, 2024

Jtang-1 Dec 4, 2024

AddisonDunn left a comment

AddisonDunn Dec 4, 2024

Jtang-1 Dec 4, 2024

AddisonDunn Dec 5, 2024

AddisonDunn Dec 5, 2024

AddisonDunn left a comment

AddisonDunn Dec 5, 2024

Jtang-1 commented Dec 6, 2024



		def validate_assigned_locations_has_users(domain, assigned_location_ids):
		error_message_location_not_has_users = _("These locations cannot have users assigned because of their "

Create Invitation API [Validation] #35448

Create Invitation API [Validation] #35448

Conversation

Jtang-1 commented Nov 26, 2024 • edited Loading

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Rollback instructions

Labels & Review

AddisonDunn 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jtang-1 Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AddisonDunn Dec 3, 2024 • 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

jingcheng16 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

AddisonDunn 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

AddisonDunn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jtang-1 commented Dec 6, 2024

Jtang-1 commented Nov 26, 2024 •

edited

Loading

Jtang-1 Dec 4, 2024 •

edited

Loading

AddisonDunn Dec 3, 2024 •

edited

Loading