Bulk Upload Org Structure and Mobile Workers via API

[szymonbalasz]

Product Description

This change introduces new API endpoints for bulk location and mobile worker (user) uploads, which were previously only available via the Bulk Upload feature in the front end.

Technical Summary

The following changes were made:

1. Added new API endpoints for bulk location upload (bulk_location_upload_api) and bulk user upload (bulk_user_upload_api).
2. Refactored the existing BaseUploadUser class to keep the code DRY and support the new bulk user upload endpoint.
3. Implemented extensive test coverage for both new endpoints, covering authentication, file validation, error handling, and success scenarios.
4. The implementation is based on the existing bulk_case_upload_api endpoint and follows a similar approach.

Feature Flag

N/A

Safety Assurance

Safety story

The changes introduced in this PR were extensively tested manually via the API and frontend to ensure that the refactoring did not introduce any regressions. The new tests cover the endpoints themselves, including authentication, exceptions, and various scenarios. However, the tests do not cover the underlying import process.

The code changes are isolated to the new endpoints and the refactored BaseUploadUser class. The existing functionality remains unchanged.

Automated test coverage

Two new test classes, BulkLocationUploadAPITest and BulkUserUploadAPITest, have been added to cover the new endpoints.
The tests cover scenarios such as:

1. Successful file upload and processing
2. Authentication (no authentication, invalid authentication)
3. Error handling (no file uploaded, invalid file format)
4. Concurrent update handling (for locations)

QA Plan

QA Ticket: https://dimagi.atlassian.net/browse/QA-6726

This plan covers testing the new bulk location and user upload API endpoints, as well as ensuring no regressions in the existing frontend functionality.

Locations:

A testing environment with an org structure seeded with sufficient location data (a few hundred entities) is recommended.

1. Test the API endpoint at /a/[domain]/settings/locations/import/bulk_location_upload_api/

• Ensure APIKey authorization works and other authorization methods fail
• Verify file caching by sending a bulk_upload_file key with the file value (form-data)
• Confirm the file is processed in the same way as the frontend bulk upload feature
• When a file is processing, attempting to upload another file should result in an error about the file already processing
• Uploading via frontend and then API (and vice versa) while the file is still processing should result in an error
• API should return appropriate errors when no file is attached or the file is not a valid Excel file with expected workbooks

2. Frontend Testing:

• Upload a bulk location file and ensure it processes correctly
• While a file is processing, the upload page should display processing status

Users:

Populate and seed some mobile worker/user data in a project and download the bulk file.

1. Frontend Testing:

• Bulk upload users should work as before, with no changes
• Header validation and file validation (e.g., file has no workbooks) should be preserved
• The PARALLEL_USER_IMPORTS setting for the domain should apply

2. Test the API endpoint at /a/[domain]/settings/users/commcare/upload/bulk_user_upload_api/

• Ensure only API authorization is permitted, not any other authorization method
• Confirm only POST requests are allowed
• Verify a bulk_upload_file key is required
• If the file has no workbooks or invalid headers, an error should be returned
• A "users" worksheet and a "groups" worksheet must be present
• Verify that users were updates as expected in the frontend

Please note that the code responsible for updating locations and users wasn't affected by this PR. If the process is initialized by either endpoint, previous business rules and logic will continue to apply.

Migrations

N/A

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

Additional Notes

1. The user upload endpoint doesn't have a concurrent update checker like the location endpoint via the @lock_locations decorator
2. There is some repetition in the tests, but for a first draft, it seems acceptable given the differentiation between the two endpoints. Abstracting into a base test class may add unnecessary complexity at this stage.

Comments and feedback are welcome!

[szymonbalasz]

Hi @snopoke , just a friendly ping about this PR. More than happy to make any further changes or add any kind of test coverage you feel is required.

[snopoke]

LGTM. I'm going to see if I can one more set of eyes on the PR before approval.

[jingcheng16]

Hi @szymonbalasz , thank you for making this PR! Our product team is going to discuss this and will keep you updated.

[jingcheng16]

Hi @szymonbalasz, I noticed that many of your commit messages are simply labeled as “wip”, to make the review process easier and more efficient, and to keep our git history clean, could you please update your commit messages to be more descriptive? Also, we plan to trigger QA for your feature. A QA plan helps us ensure that all aspects of the changes have been tested and validated. Can you include some test cases ( including edge cases ) and the expected outcomes of those scenarios under QA plan section, so our QA team knows how to test it. Let me know if you need any assistance!

[szymonbalasz]

Hi @jingcheng16

As requested, I have added some steps to test the changes that this PR has introduced. Please let me know if this is sufficient.

Regarding updating the commit messages, I wanted to let you know that rebasing is not a viable option in this case. Since master has already been merged into the branch, attempting to rebase would result in duplicate commits as I resolve conflicts.
As an alternative, may I suggest using the --squash merge strategy, similar to what was done in my previous PR (#34557)? This approach would squash all the commit messages and preserve only the PR title in dimagi's commcare repo, effectively keeping the commit history clean without the need for rebasing.

[jingcheng16]

@szymonbalasz Thank you for the QA plan, I'll submit a QA ticket for you!

For the commit history, it's ok this time. If you contribute in the future, please try to have a cleaner commit history before opening a PR. If helpful, you can refer to our docs on clean git histories. Thanks again!

[jingcheng16]

@szymonbalasz Hi, I put your branch on staging and our delivery team run into issues when access /a/{domain}/settings/users/web/upload/. The error seemed to apply to any successful import. Can you look into this error that might related to your changes:

AttributeError: 'UploadWebUsers' object has no attribute 'get_success_response'
  File "django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
  File "django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "ddtrace/contrib/trace_utils.py", line 339, in wrapper
    return func(mod, pin, wrapped, instance, args, kwargs)
  File "ddtrace/contrib/django/patch.py", line 284, in wrapped
    return func(*args, **kwargs)
  File "django/views/generic/base.py", line 104, in view
    return self.dispatch(request, *args, **kwargs)
  File "ddtrace/contrib/trace_utils.py", line 339, in wrapper
    return func(mod, pin, wrapped, instance, args, kwargs)
  File "ddtrace/contrib/django/patch.py", line 284, in wrapped
    return func(*args, **kwargs)
  File "django/utils/decorators.py", line 46, in _wrapper
    return bound_method(*args, **kwargs)
  File "corehq/apps/accounting/decorators.py", line 150, in shim
    return view_func(request, *args, **kwargs)
  File "django/utils/decorators.py", line 46, in _wrapper
    return bound_method(*args, **kwargs)
  File "corehq/apps/domain/decorators.py", line 119, in _inner
    return call_view()
  File "corehq/apps/domain/decorators.py", line 91, in call_view
    return view_func(req, domain_name, *args, **kwargs)
  File "corehq/apps/users/decorators.py", line 47, in _inner
    return view_func(request, domain, *args, **kwargs)
  File "django/utils/decorators.py", line 46, in _wrapper
    return bound_method(*args, **kwargs)
  File "corehq/apps/accounting/decorators.py", line 50, in wrapped
    return requires_privilege(slug, **assignment)(fn)(
  File "django_prbac/decorators.py", line 21, in wrapped
    return fn(request, *args, **kwargs)
  File "corehq/apps/users/views/__init__.py", line 1345, in dispatch
    return super(UploadWebUsers, self).dispatch(request, *args, **kwargs)
  File "ddtrace/contrib/trace_utils.py", line 339, in wrapper
    return func(mod, pin, wrapped, instance, args, kwargs)
  File "ddtrace/contrib/django/patch.py", line 284, in wrapped
    return func(*args, **kwargs)
  File "django/utils/decorators.py", line 46, in _wrapper
    return bound_method(*args, **kwargs)
  File "corehq/apps/domain/decorators.py", line 119, in _inner
    return call_view()
  File "corehq/apps/domain/decorators.py", line 91, in call_view
    return view_func(req, domain_name, *args, **kwargs)
  File "corehq/apps/domain/decorators.py", line 200, in dispatch
    return super(LoginAndDomainMixin, self).dispatch(*args, **kwargs)
  File "ddtrace/contrib/trace_utils.py", line 339, in wrapper
    return func(mod, pin, wrapped, instance, args, kwargs)
  File "ddtrace/contrib/django/patch.py", line 284, in wrapped
    return func(*args, **kwargs)
  File "django/views/generic/base.py", line 143, in dispatch
    return handler(request, *args, **kwargs)
  File "ddtrace/contrib/trace_utils.py", line 339, in wrapper
    return func(mod, pin, wrapped, instance, args, kwargs)
  File "ddtrace/contrib/django/patch.py", line 284, in wrapped
    return func(*args, **kwargs)
  File "corehq/apps/users/views/__init__.py", line 1355, in post
    return super(UploadWebUsers, self).post(request, *args, **kwargs)
  File "ddtrace/contrib/trace_utils.py", line 339, in wrapper
    return func(mod, pin, wrapped, instance, args, kwargs)
  File "ddtrace/contrib/django/patch.py", line 284, in wrapped
    return func(*args, **kwargs)
  File "corehq/apps/users/views/__init__.py", line 1252, in post
    return self.get_success_response(request, task_ref)

[szymonbalasz]

@jingcheng16 Update pushed and additional test coverage added. Thank you for notifying me and apologies for the inconvenience. I have run another set of manual tests myself and asked a team member to help as well. Should be good now. 🙈

[jingcheng16]

@szymonbalasz Thank you. I'll let the team know and add your branch back.

[szymonbalasz]

@jingcheng16 Hi! Just a friendly bump on this issue. Any notes from the testing team?

[snopoke]

@szymonbalasz we're going through the QA feedback and will update back here soon. Apologies for the delay.

[snopoke]

@szymonbalasz please make this one update (#34634 (comment)) and also merge in the latest code from master. Once that's done this is ready to go.

[szymonbalasz]

@snopoke

Thanks so much for the feedback, I have added the check against multiple files and covered with tests: 181f1bc

Master merged. Thanks again :)