-
Notifications
You must be signed in to change notification settings - Fork 730
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
Ensure facility id is properly sent to public signup viewset. #11846
Ensure facility id is properly sent to public signup viewset. #11846
Conversation
I believe this nuance was my reasoning for adding a constant for it, which could be used here? kolibri/kolibri/core/auth/backends.py Line 9 in 5bd813d
|
@@ -51,7 +51,9 @@ def createuseronremote(self, request): | |||
url = "{}{}".format(baseurl, api_url) | |||
|
|||
payload = { | |||
"facility_id": facility_id, | |||
# N.B. facility is keyed by facility not facility_id on the signup |
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.
What's N.B.
mean here?
Why not update the serializer field to be facility_id
instead?
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.
Too many other places to have to change on the frontend I suspect?
In any case, change looks good to me
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.
Nota Bene
- note well.
Changing the public serializer would involve changing the serializer for all signups, so would be a fairly significant change. If I was going to make a more dramatic change here, I would propagate this back through this API endpoint (to be facility
all the way) but this seemed like the least dramatic fix this close to release.
Build Artifacts
|
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.
I confirm that the issue reported in #11832 is fixed now and the learner user is created in the correct facility. Also no issues observed while regression testing.
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.
Manual QA passed, good to go! 💯
Yes @bjester that does seem appropriate! I'll update. |
Summary
facility_id
instead of the expectedfacility
References
Fixes #11832
Reviewer guidance
Use the database from the linked issue for a server device, and then attempt to create a new user account for
jerrym
username on the second facility (imported from Win11).Testing checklist
PR process
Reviewer checklist
yarn
andpip
)