NOISS: Moodle roster integrations#909
Conversation
| } | ||
|
|
||
| private function update_moodle_user(User|Visitor $user): null|int { | ||
| $cid = (class_basename($user) == 'User') ? $user->id : $user->cid; |
There was a problem hiding this comment.
For code clarity, can we break these out into a separate variable (perhaps isHomeController or similar?)
There was a problem hiding this comment.
Sure, no problem. Done.
| ['id' => (count($moodle_user) > 0) ? $moodle_user[0] : null, 'username' => $cid], | ||
| $query_parameters | ||
| ); | ||
| $moodle_user = DB::connection('moodle')->table('user')->where('username', $cid)->first(); |
There was a problem hiding this comment.
Not necessarily in need of immediate change, but this routine seems like it's going to absolutely slam the database (both of them, but mostly the Moodle one) with like 4-5 queries per user.. can we optimize this maybe with like RETURNING id for example? Or, not querying for the entire object to get one field off it... stuff like that.
There was a problem hiding this comment.
Point taken and yes, I see it too. Unfortunately, query builder is just very inefficient.
| } | ||
| } | ||
|
|
||
| private function add_visit_request_cohort(int $cid): void { |
There was a problem hiding this comment.
I feel like this function is duplicating the effort of sync_visit_requests. I understand why it's separate (this one only assigns it to one user), but I don't think we need two functions that do the same thing. Is it possible to merge these into one function and just pass an optional parameter?
There was a problem hiding this comment.
True. I made a decision to break this out to keep the Moodle stuff separate. Don't know if you recall, but years ago we had lots of Moodle integrations throughout the website. We got rid of Moodle and I asked everyone on staff if I should save the integrations. They swore up and down that we'd never have another Moodle again because of the VATUSA academy. Now we're back to Moodle. I'm trying to make this easier on the next WM who confronts this situation by keeping the Moodle code together.
| ]); | ||
| } | ||
| } | ||
| $this->cohorts = DB::connection('moodle')->table('cohort')->select(['id', 'name'])->pluck('name', 'id')->toArray(); |
There was a problem hiding this comment.
See below about database optimizations; we make this query twice, when upserts (or just INSERT IGNORE) would do the trick
There was a problem hiding this comment.
I added a check to only run second select statement in the rare case where we do perform insert operations. upsert won't work - it is only for models (this is a query builder) and insert ignore would cause us to hit the database 3 more times per operation (one for each of the 3 cohorts).
| $moodle_user_id = $this->update_moodle_user($user); | ||
| $cohort_id = null; | ||
| if ($user->status == 1) { | ||
| $cohort_id = array_search('Home', $this->cohorts); |
There was a problem hiding this comment.
We search this array a lot.. a map might be better?
There was a problem hiding this comment.
Removed all array_search() operations by re-indexing the array by cohort name
2 participants
This PR will: