-
Notifications
You must be signed in to change notification settings - Fork 18
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
3.1.x new #498
3.1.x new #498
Conversation
// if (ipeer) id does exist | ||
if ($person['id']) { | ||
// Queries role_id saved in iPeer | ||
$role = $this->RolesUser->find('first', array('conditions' => array('user_id' => $person['id']), 'fields' => 'role_id')); |
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.
Could you merge this query with the query above to reduce the number of queries to the database. With large number of enrolment, it can be very slow.
I think the rest is good. Maybe squash the commits into one? What do you think about this pull request @ionparticle ? |
Ok, looks fine to me too. Ideally, I'd like all the changes in v1_controller to be one commit and all the new test cases to be another commit, but squashing them all into one is fine too. |
Would be great too if you add bullet point details to the commit messages like in #499 |
Hi @xcompass, what do you think if I rewrite the multiple users import block to:
Thank you, @ionparticle, I will adjust the commits and messages. |
@tkbaylis, or you probably can do it in one query.e.g., extract all usernames in the decode, query database by usernames and returning the ids and roles (using contain). |
This fixes, for example, iPeer superadmins who enter through external LMS where they are instructors and lose superadmin privileges at login. Now, highest level (lowest role_id) is preserved. - v1_controller compares external id to ipeer id (if saved) and chooses lower - new users created in test cases for this functionality: - same external vs ipeer role_id - external role_id higher than saved in ipeer - external role_id lower than saved in ipeer
I've decreased the number of queries, squashed all commits into one, and added comments. |
Add/update users picks lowest role_id
Looks good. Thanks 👍 |
v1_controller accounts for different role permissions in Connect and iPeer during add/update of user in iPeer