Skip to content

Commit

Permalink
MDL-69251 enrol_lti: fix sync_members task inconsistent internal state
Browse files Browse the repository at this point in the history
  • Loading branch information
@denisjuergen @snake
denisjuergen authored and snake committed Jul 15, 2022
1 parent 8984ffe commit f9e7aa5
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 37 deletions.
39 changes: 18 additions & 21 deletions enrol/lti/classes/task/sync_members.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,6 @@ class sync_members extends scheduled_task {
/** @var array Array of user photos. */
protected $userphotos = [];

/** @var array Array of current LTI users. */
protected $currentusers = [];

/** @var data_connector $dataconnector A data_connector instance. */
protected $dataconnector;

Expand Down Expand Up @@ -111,15 +108,15 @@ public function execute() {

// Fetched members count.
$membercount = count($members);
$usercount += $membercount;
mtrace("$membercount members received.\n");

// Process member information.
list($usercount, $enrolcount) = $this->sync_member_information($tool, $consumer, $members);
}
list($users, $enrolledcount) = $this->sync_member_information($tool, $consumer, $members);
$enrolcount += $enrolledcount;

// Now we check if we have to unenrol users who were not listed.
if ($this->should_sync_unenrol($tool->membersyncmode)) {
$unenrolcount = $this->sync_unenrol($tool);
// Now sync unenrolments for the consumer.
$unenrolcount += $this->sync_unenrol($tool, $consumer->getKey(), $users);
}

mtrace("Completed - Synced members for tool '$tool->id' in the course '$tool->courseid'. " .
Expand Down Expand Up @@ -216,17 +213,15 @@ protected function should_sync_enrol($syncmode) {
* @param stdClass $tool
* @param ToolConsumer $consumer
* @param User[] $members
* @return array An array containing the number of members that were processed and the number of members that were enrolled.
* @return array An array of users from processed members and the number that were enrolled.
*/
protected function sync_member_information(stdClass $tool, ToolConsumer $consumer, $members) {
global $DB;
$usercount = 0;
$users = [];
$enrolcount = 0;

// Process member information.
foreach ($members as $member) {
$usercount++;

// Set the user data.
$user = new stdClass();
$user->username = helper::create_username($consumer->getKey(), $member->ltiUserId);
Expand All @@ -248,9 +243,7 @@ protected function sync_member_information(stdClass $tool, ToolConsumer $consume
user_update_user($user);

// Add the information to the necessary arrays.
if (!in_array($user->id, $this->currentusers)) {
$this->currentusers[] = $user->id;
}
$users[$user->id] = $user;
$this->userphotos[$user->id] = $member->image;
} else {
if ($this->should_sync_enrol($tool->membersyncmode)) {
Expand All @@ -264,7 +257,7 @@ protected function sync_member_information(stdClass $tool, ToolConsumer $consume
$user->id = user_create_user($user);

// Add the information to the necessary arrays.
$this->currentusers[] = $user->id;
$users[$user->id] = $user;
$this->userphotos[$user->id] = $member->image;
}
}
Expand All @@ -290,16 +283,18 @@ protected function sync_member_information(stdClass $tool, ToolConsumer $consume
}
}

return [$usercount, $enrolcount];
return [$users, $enrolcount];
}

/**
* Performs unenrolment of users that are no longer enrolled in the consumer side.
*
* @param stdClass $tool The tool record object.
* @param string $consumerkey ensure we only unenrol users from this tool consumer.
* @param array $currentusers The list of current users.
* @return int The number of users that have been unenrolled.
*/
protected function sync_unenrol(stdClass $tool) {
protected function sync_unenrol(stdClass $tool, string $consumerkey, array $currentusers) {
global $DB;

$ltiplugin = enrol_get_plugin('lti');
Expand All @@ -308,16 +303,18 @@ protected function sync_unenrol(stdClass $tool) {
return 0;
}

if (empty($this->currentusers)) {
if (empty($currentusers)) {
return 0;
}

$unenrolcount = 0;

$ltiusersrs = $DB->get_recordset('enrol_lti_users', array('toolid' => $tool->id), 'lastaccess DESC', 'userid');
$select = "toolid = :toolid AND " . $DB->sql_compare_text('consumerkey', 255) . " = :consumerkey";
$ltiusersrs = $DB->get_recordset_select('enrol_lti_users', $select, ['toolid' => $tool->id, 'consumerkey' => $consumerkey],
'lastaccess DESC', 'userid');
// Go through the users and check if any were never listed, if so, remove them.
foreach ($ltiusersrs as $ltiuser) {
if (!in_array($ltiuser->userid, $this->currentusers)) {
if (!array_key_exists($ltiuser->userid, $currentusers)) {
$instance = new stdClass();
$instance->id = $tool->enrolid;
$instance->courseid = $tool->courseid;
Expand Down
27 changes: 11 additions & 16 deletions enrol/lti/tests/sync_members_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,10 +213,10 @@ public function test_should_sync_unenrol() {
* Test for sync_members::sync_member_information().
*/
public function test_sync_member_information() {
list($totalcount, $enrolledcount) = $this->task->sync_member_information($this->tool, $this->consumer, $this->members);
list($users, $enrolledcount) = $this->task->sync_member_information($this->tool, $this->consumer, $this->members);
$membercount = count($this->members);
$this->assertCount(10, $this->members);
$this->assertEquals($membercount, $totalcount);
$this->assertCount($membercount, $users);
$this->assertEquals($membercount, $enrolledcount);
}

Expand All @@ -225,10 +225,10 @@ public function test_sync_member_information() {
*/
public function test_sync_profile_images() {
$task = $this->task;
list($totalcount, $enrolledcount) = $task->sync_member_information($this->tool, $this->consumer, $this->members);
list($users, $enrolledcount) = $task->sync_member_information($this->tool, $this->consumer, $this->members);
$membercount = count($this->members);
$this->assertCount(10, $this->members);
$this->assertEquals($membercount, $totalcount);
$this->assertCount($membercount, $users);
$this->assertEquals($membercount, $enrolledcount);

// Suppress output.
Expand All @@ -244,14 +244,14 @@ public function test_sync_unenrol() {
$tool = $this->tool;
$task = $this->task;

$task->sync_member_information($tool, $this->consumer, $this->members);
list($users) = $task->sync_member_information($tool, $this->consumer, $this->members);

// Simulate that the fetched list of current users has been reduced by 3.
$unenrolcount = 3;
for ($i = 0; $i < $unenrolcount; $i++) {
$task->pop_current_users();
array_pop($users);
}
$this->assertEquals($unenrolcount, $task->sync_unenrol($tool));
$this->assertEquals($unenrolcount, $task->sync_unenrol($tool, 'Consumer1Key', $users));
}

/**
Expand Down Expand Up @@ -297,13 +297,6 @@ public function get_dataconnector() {
return $this->dataconnector;
}

/**
* Helper method that removes an element in the array of current users.
*/
public function pop_current_users() {
array_pop($this->currentusers);
}

/**
* Exposes sync_members::do_context_membership_request()
*
Expand Down Expand Up @@ -390,10 +383,12 @@ public function sync_profile_images() {
* Exposes sync_members::sync_unenrol()
*
* @param \stdClass $tool
* @param string $consumerkey
* @param array $currentusers
* @return int
*/
public function sync_unenrol(\stdClass $tool) {
$count = parent::sync_unenrol($tool);
public function sync_unenrol(\stdClass $tool, string $consumerkey, array $currentusers) {
$count = parent::sync_unenrol($tool, $consumerkey, $currentusers);
return $count;
}
}

0 comments on commit f9e7aa5

Please sign in to comment.