Skip to content

Conversation

@henry-r18
Copy link
Contributor

@henry-r18 henry-r18 commented Dec 11, 2025

Implements SYS-2108

Acceptance criteria

-❓Authorized users (those in the “super editors” group) can run batch updates by importing Excel files, using the batch_update management command

  • ✅ Summary of batch update is viewable by the user who ran the update

Description

This PR adds self-service batch import functionality, allowing users with appropriate permissions to apply batch updates to SheetImport records using an uploaded Excel spreadsheet.

There are four main areas of changes:

  1. new form;
  2. new view;
  3. new template partial;
  4. changes to batch_update mgmt command.

The new form has a file upload field on it that limits uploads to XLSX files, applying some client-side validation to the workflow. The new view handles the uploaded file in two parts: first, validating the file using the existing validation function from the mgmt command, and then actually applying the batch update. The new template partial loads within a Bootstrap modal triggered by a new button on the search_results page. The changes to the mgmt command are mainly to improve error handling.

Permissions

At the moment, the button used to launch the batch update modal renders conditionally, based on whether the current user has the ftva_lab_data.change_sheetimport permission. This isn't exactly the same as checking whether the user is in the super_editors group, but I don't know of a way to do that within the template using default template filters. I could add a custom template filter to accomplish that--something like this. What do you think regarding how best to check if the user is in the super_editors group, and render the template accordingly?

Testing

I didn't add any new unit tests. For manual testing, I used the spreadsheet attached to the Jira ticket. I made two additional copies of it, one with fudged headers and the other with some fudged data values, in order to confirm the error handlers work as expected.

Would you please confirm that the valid spreadsheet yields all expected updates, and any invalid spreadsheet does not cause any changes? After the updates are applied, if the valid spreadsheet is tried again, it should result in an error.

screencap

* Add view for handling batch update uploads
* Add URL for batch update
* Update `search_results.html` to use new batch update modal
* Add CSS for loading indicators used by `htmx-indicator` attribute
* Handle empty string values in `batch_update` mgmt command
* Rename modal template
* Update error messages raised by `batch_update` mgmt command
* Add conditional for rendering "Batch Update" button
* Raise error on 0 records updates in batch update mgmt command
@henry-r18 henry-r18 requested a review from akohler December 11, 2025 01:34
Copy link
Member

@akohler akohler left a comment

Choose a reason for hiding this comment

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

@henry-r18 I like the new functionality and the UI you've created for it. The "dry run first" approach works well, providing extra confirmations of what will be done.

The permissions do need to be tightened, as change_sheetimport is not the appropriate level: as-is, anyone who can edit records can run this batch functionality. The super-editors group already controls access to one custom permission, assign_user. I think the batch update functionality can be handled similarly:

  • Define a new batch_update permission on the sheetimportmodel: see the Meta definition, starting line 190 in models.py.
  • See custom permissions documentation for more info.
  • Once the new permission is defined and created, add it to super-editors, instead of the change_sheetimport permission you added - see my other comment on that.
  • The view and the form then can check for the batch_update permission, no need for custom filters.

Thanks --Andy

* Add `batch_update` perm to `super_editors` group in fixture
* Use new perm in view and template
@henry-r18
Copy link
Contributor Author

@akohler this is ready for another look. I added a batch_update permission, per your note, which the view and template now use instead of change_sheetimport. Thanks!

@henry-r18 henry-r18 requested a review from akohler December 11, 2025 21:14
Copy link
Member

@akohler akohler left a comment

Choose a reason for hiding this comment

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

@henry-r18 The permission changes all look correct. I logged in as a user just in the editors group and confirmed that (a) the "Batch Update" button does not appear, and (b) calling the view directly at /batch_update/ gives a 403 Forbidden (and logs the error).

I would like a test added for this. I think just 1-2 additional tests in UserAccessTestCase are enough, confirming that users without permission cannot access the batch update functionality.

There may also be edge cases in the file handling. I didn't try mangling the data itself, as you showed that yesterday. I verified that real xlsx files not in the expected format give error messages. One edge case probably worth adding: "Excel" files which aren't real xlsx files. I renamed a random csv file with xlsx extension, which allows the file-picker to see and select it... but when I click "Apply Updates", the file-picker window just flashes and then does (apparently) nothing, so the user is left wondering. The log shows an Internal Server Error, with specific message "ValueError: Excel file format cannot be determined, you must specify an engine manually.". This may be worth catching and telling the user about... I don't feel strongly about this, but it's not unreasonable for some users to conflate "Excel file" (which csv is) and "xlsx file", and wonder why nothing's happening.

Thanks --Andy

…dpoint

* Add error handling for bad XLSX files
@henry-r18
Copy link
Contributor Author

@akohler I reorganized the try...except block in the batch_update view so that it will catch the "Excel file format cannot be determined" error raised when pandas tries loading a bad XLSX file. I overwrote the error message to be a bit friendlier.

Also added are two new tests: one confirming unauthorized users get 403 when trying to GET or POST on the batch_update URL, and the other showing that authorized users get the expected form.

Should be ready for another look--thanks!

@henry-r18 henry-r18 requested a review from akohler December 12, 2025 00:16
Copy link
Member

@akohler akohler left a comment

Choose a reason for hiding this comment

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

@henry-r18 All changes look good now. The new tests pass and are relevant. The new invalid Excel file message is clear. Merging now, thanks --Andy

@akohler akohler merged commit b44be0e into main Dec 12, 2025
1 check passed
@akohler akohler deleted the SYS-2108/self_service_batch_update branch December 12, 2025 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants