-
Notifications
You must be signed in to change notification settings - Fork 827
Check unusable device on user_auth plugin #13759
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
Check unusable device on user_auth plugin #13759
Conversation
Build Artifacts
|
92888d6 to
b207005
Compare
b207005 to
3be12eb
Compare
28ad678 to
bd35f6a
Compare
nucleogenesis
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.
Just one question - I'll tag in @pcenov to confirm the fix.
| clearTasks(); | ||
| redirectBrowser(); |
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.
When the browser redirects here is there a chance we'd end up cancelling/clobbering the async function call through page navigation? This initially stuck out to me because you call an async function synchronously which seems less problematic above, but here I wonder if we might miss out on clearing the tasks if these run synchronously
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.
It shouldn't; we are not setting an abort controller on page navigation, therefore, the request won't be aborted. What we are not doing here is not caring about the API response and whether it was successful or not. But in theory, in this case, if the clear tasks fail, it is not enough reason to stop the user workflow. So, awaiting the clear tasks to finish would make sense here if we want to set up a retry logic until the tasks are actually cleared, but we haven't done this in any other place.
pcenov
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.
LGTM, implemented as specified above but in order to fully test it I will need #13757 implemented.
At this point I can confirm that if the superuser is removed on the server, then once this gets synced to the device, if I sign out and attempt to sign in I am seeing the message informing me that the device is unusable.
If I restore the user and restart the app I can sign in again as the same superuser and see my previous interactions and progress made restored.
If I remove a normal learner account and sign out once this has been synced to the device, then the account is no longer listed at the sign in page. It can also be restored, and the progress made by the learner is preserved.
|
Seems like all comments have been addressed and what can be manually QAed is checking out. I am going to merge this, so I can rebase #13757 on top for fuller QA testing (and finalization of that PR). |
Summary
deprovisioncommand logic to be a utils module so it can be reused in the command and in an async task.FacilityUsers.objects.getto support soft-deleted and hard-deleted syncs.Grabacion.de.pantalla.2025-10-01.a.la.s.1.19.22.p.m.mov
References
Closes #13397
Notes