Conversation
Test results 27 files 27 suites 45m 1s ⏱️ Results for commit a0d5e76. ♻️ This comment has been updated with latest results. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3738 +/- ##
==========================================
- Coverage 63.05% 63.04% -0.02%
==========================================
Files 614 614
Lines 45450 45452 +2
Branches 43 43
==========================================
- Hits 28660 28655 -5
- Misses 16780 16787 +7
Partials 10 10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| # Delete cache entry of how many accounts have password issues | ||
| cache.delete(PASSWORD_ISSUES_CACHE_KEY) | ||
| return HttpResponseRedirect(reverse('useradmin-account_list')) | ||
| with transaction.atomic(): |
There was a problem hiding this comment.
What happens if the transaction fails?
There was a problem hiding this comment.
There's now a guarantee that the log entry is only made iff the user is deleted. Before, you could have it half way: deleting the user and not logging it. With the new order of operations, without the atomic you can log the deletion then not delete the user.
The transaction can fail if the user was already deleted by a different process/logged in users/API call while you attempted to delete via the request.
If we had used a generic view we would have gotten a 404 exception on the attempt if the user was gone as we entered the request, but if it happened during the request I guess we'd get an (uncaught) database error.
Now, we fall off the with and re-render the delete widget/page. It might be that a redirect to the same page after the with would be better, triggering an error message that the account is gone if it is and redirecting to the list page.
Simrayz
left a comment
There was a problem hiding this comment.
Nice solution 😄
(Approved with comment, because changing the indent of the line should not require a re-review)
| messages.success(request, 'Account %s has been deleted.' % (account.name)) | ||
| # Delete cache entry of how many accounts have password issues | ||
| cache.delete(PASSWORD_ISSUES_CACHE_KEY) | ||
| return HttpResponseRedirect(reverse('useradmin-account_list')) |
There was a problem hiding this comment.
This return should probably be outside the transaction block?
There was a problem hiding this comment.
It's there on purpose. I'll merge this as is, there are ways to make it even more robust but that can wait.
9700d56 to
a0d5e76
Compare
|
Scope and purpose
Fixes #3709
Related: #3729
Contributor Checklist
Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to NAV can be found in the
Hacker's guide to NAV.
<major>.<minor>.x). For a new feature or other additions, it should be based onmaster.