-
Notifications
You must be signed in to change notification settings - Fork 0
SYS-2108: Allow authorized users to run batch updates via Excel imports #92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
* 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
* Remove TODO
* Raise error on 0 records updates in batch update mgmt command
There was a problem hiding this 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_updatepermission on thesheetimportmodel: see theMetadefinition, starting line 190 inmodels.py. - See custom permissions documentation for more info.
- Once the new permission is defined and created, add it to
super-editors, instead of thechange_sheetimportpermission you added - see my other comment on that. - The view and the form then can check for the
batch_updatepermission, no need for custom filters.
Thanks --Andy
* Add `batch_update` perm to `super_editors` group in fixture * Use new perm in view and template
* Add release note
|
@akohler this is ready for another look. I added a |
akohler
left a comment
There was a problem hiding this 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
|
@akohler I reorganized the Also added are two new tests: one confirming unauthorized users get Should be ready for another look--thanks! |
akohler
left a comment
There was a problem hiding this 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
Implements SYS-2108
Acceptance criteria
-❓Authorized users (those in the “super editors” group) can run batch updates by importing Excel files, using the
batch_updatemanagement commandDescription
This PR adds self-service batch import functionality, allowing users with appropriate permissions to apply batch updates to
SheetImportrecords using an uploaded Excel spreadsheet.There are four main areas of changes:
batch_updatemgmt command.The new form has a file upload field on it that limits uploads to
XLSXfiles, 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 thesearch_resultspage. 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_sheetimportpermission. This isn't exactly the same as checking whether the user is in thesuper_editorsgroup, 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 thesuper_editorsgroup, 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.