Upload data loading indicator #211
Conversation
|
@pollardld @rhodges add you as reviewers for visibility on the work, but your review isn't blocking! Always appreciate feedback but don't feel pressure to review. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements improved user feedback for database export/import operations by replacing browser alerts with a Bootstrap modal interface and adding cookie-based status tracking for the export process.
- Replaces
window.alertdialogs with a reusable Bootstrap modal for export/import confirmations and feedback - Adds cookie-based status tracking (
export_status) to provide real-time feedback during database export operations - Refactors export/import UI to use buttons instead of links and improves styling consistency
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| TEKDB/TEKDB/views.py | Added cookie status indicators (done/error) in the export database function to communicate operation results |
| TEKDB/TEKDB/tests/test_views.py | Split the export test into multiple focused tests and added assertions for cookie status validation |
| TEKDB/TEKDB/templates/admin/index.html | Replaced export link with button, added error message display, and integrated Bootstrap modal for info dialogs |
| TEKDB/TEKDB/static/admin/js/admin_index.js | Replaced alert-based flows with modal-based UI, added export polling logic, and improved error handling |
| TEKDB/TEKDB/static/admin/css/itk_base.css | Removed redundant CSS rules for export/import buttons |
| .gitignore | Added VS Code settings directory to ignore list |
Comments suppressed due to low confidence (1)
TEKDB/TEKDB/static/admin/js/admin_index.js:152
- This expression has no effect.
$;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| if not test: | ||
| os.remove(tmp_zip.name) | ||
|
|
||
| response.set_cookie("export_status", "done") | ||
| except (PermissionError, NotADirectoryError): | ||
| response.set_cookie("export_status", "error") | ||
| pass |
There was a problem hiding this comment.
The response variable is not defined when an exception occurs in the try block (lines 74-90) before line 89. If an exception is raised during zip creation, the code will reach the finally block without response being assigned, causing a NameError when trying to set cookies. The cookie setting logic should be conditional on whether response exists.
|
Looks great. Thank you! |
rhodges
left a comment
rhodges
left a comment
There was a problem hiding this comment.
I'm late to the party to prevent the merge, and my only issue with the code doesn't hurt anything (it either doesn't do anything [harmless] OR it's tied to something I don't understand). I honestly would like a walkthrough of the response work in views.py if they are functional.
| a safe and secure practice for regular backups, which may or may | ||
| not involve this tool.`; | ||
|
|
||
| $(function () { |
| <div class="module" id="export-module"> | ||
| <h2>{% translate 'Export database' %}<button class='btn btn-info' id='export-info'>Info</button></h2> | ||
| <a href="/export_database/" id='export-button' class="btn btn-primary"> | ||
| <h2>{% translate 'Export database' %}<button class='btn btn-info' id='export-info' data-bs-toggle="modal" data-bs-target="#exportImportModal">Info</button></h2> |
There was a problem hiding this comment.
I love Bootstrap modals, and we already installed Bootstrap to the admin view, so this solution is perfect.
I expect my love of Bootstrap is dated (Twitter built it long before their name change, which is already old news -- I assume newer frameworks are en vogue ATM). But replacing Bootstrap with a modern framework here should be informed by and performed in conjunction with an often-suggested rebuild of the front-end.
| os.remove(tmp_zip.name) | ||
|
|
||
| if response: | ||
| response.set_cookie("export_status", "done") |
There was a problem hiding this comment.
response is set to None and not handed to HttpResponse() in the return statement. Does this do anything? If so, how?
There was a problem hiding this comment.
On line 90 we were returning response. I was trying to not change too much of the logic here, but I think that you are identifying a code smell. I've tried to clean up the logic in this PR #219.
|
I should also add that the screenshots are such an improvement over the old UI! One test I recommend (having been bitten by it many times in the past) is testing modal visability on a very short screen: Sometimes scrolling works fine and the whole modal is available; other times it's fixed and pushes all of the buttons off the page bottom making it unusable. |
task: Add spinner when importing/exporting database for restore/backup purposes
task: Data Upload Spinner
Improves the experience for the import, export and info modals.
Features Include:
window.alert. I moved to a modal because with window.alert, a user can click the “dont show again” checkbox and if they do, the window alerts do not open at all. This could create a lot of confusion for users when they try to click buttons and nothing happens. The modal is static so that users can not click outside to close the modal while requests are happening. This prevents users from clicking random things while the import request is processing.Screenshots:
Info modal:
Import modal:
Import modal loading:
Import success:
Import error:
Export loading:
Export error:
