kolibri
kolibri copied to clipboard
Consolidate list remote users api
Summary
Consolidates the logic of listing remote users from the user_profile and setup_wizard plugins, and migrates them to core auth so that they are accessible by multiple plugins.
There were some differences between both endpoints, and the following decisions were made to merge them:
- The setup wizard endpoint returned an object { admin, students }, while the user_profile endpoint returned a list of users. It was decided to keep the response the same as that of user_profile as it was more flexible, and in the setup wizard to make the admin, students filter on the frontend instead of the backend.
- The setup wizard endpoint returns a 403 error if there are authentication errors, but the user_profile endpoint returns a 200 code, but with an error object. The 403 error was maintained, and where it is called to the user_profile endpoint in the frontend the error handling was updated.
- The setup_wizard endpoint gets a "facility_id" param, while the user_profile endpoint gets a "facility" param. The naming of "facility_id" was preferred.
- The user_profile endpoint validated the input data, and the setup_wizard endpoint did not. It was preferred to validate it as the user_profile.
Reviewer guidance
- Check the flow of importing users as administrator of the setup wizard.
- Check the flow of changing facilities for an "on my own" user, and merging users using administrator credentials.
Testing checklist
- [x] Contributor has fully tested the PR manually
- [ ] If there are any front-end changes, before/after screenshots are included
- [ ] Critical user journeys are covered by Gherkin stories
- [ ] Critical and brittle code paths are covered by unit tests
PR process
- [x] PR has the correct target branch and milestone
- [x] PR has 'needs review' or 'work-in-progress' label
- [ ] If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
- [ ] If this is an important user-facing change, PR or related issue has a 'changelog' label
- [ ] If this includes an internal dependency change, a link to the diff is provided
Reviewer checklist
- Automated test coverage is satisfactory
- PR is fully functional
- PR has been tested for accessibility regressions
- External dependency files were updated if necessary (
yarnandpip) - Documentation is updated
- Contributor is in AUTHORS.md
Build Artifacts
| Asset type | Download link |
|---|---|
| PEX file | kolibri-0.17.0a0.dev0_git.88.g5cf101b9.pex |
| Windows Installer (EXE) | kolibri-0.17.0a0.dev0+git.88.g5cf101b9-windows-setup-unsigned.exe |
| Debian Package | kolibri_0.17.0a0.dev0+git.88.g5cf101b9-0ubuntu1_all.deb |
| Mac Installer (DMG) | kolibri-0.17.0a0.dev0+git.88.g5cf101b9.dmg |
| Android Package (APK) | kolibri-0.17.0a0.dev0+git.88.g5cf101b9-0.1.4-debug.apk |
| TAR file | kolibri-0.17.0a0.dev0+git.88.g5cf101b9.tar.gz |
| WHL file | kolibri-0.17.0a0.dev0+git.88.g5cf101b9-py2.py3-none-any.whl |
Hi @AlexVelezLl - I confirm that there are no issues when importing learners as an administrator in the setup wizard or when changing facilities for an "On my own" learner. However when I attempt to merge a learner that was created on the actual "On my own" device then when I try to migrate the learner either by using his password or by using an admin account I am getting permission denied 403 errors in the console and the learner cannot be migrated:
https://github.com/user-attachments/assets/ca56ec9f-968c-417b-809c-1cc73f6fa8c5
Hi @pcenov! I have solved the mentioned issues both when mergin the self user and with an admin:
@rtibbles I have updated the RemoteFacilityUserAuthenticatedViewset so if the queried user is not an admin we just return the user: https://github.com/learningequality/kolibri/pull/12321/commits/98f2f550c7f2f931fab16f636dde88b810a8d217.
Also, since this commit the IsSelf permission was not working ok because local_user_id was not in kwargs. I tried to solve this here, would love to hear if you have any comment about this :open_hands:.
I think rather than mess with the signature of the task function, it might be simpler just to update how the IsSelf references the args/kwargs?
class IsSelf(BasePermission):
def user_can_run_job(self, user, job):
return user.id == job.args[1] or user.id == job.kwargs.get("remote_user_pk", None)
def user_can_read_job(self, user, job):
return user.id == job.args[1] or user.id == job.kwargs.get("remote_user_pk", None)
Thanks @AlexVelezLl - I confirm that everything is working correctly now!
Thank you @pcenov! @rtibbles would you give it one last look, please? :open_hands:.