Skip to content

Commit

Permalink
Merge branch 'MDL-76842-400' of https://github.com/snake/moodle into …
Browse files Browse the repository at this point in the history 
…MOODLE_400_STABLE
  • Loading branch information
@junpataleta
junpataleta committed Jun 8, 2023
2 parents 0f4f1c6 + e76dd38 commit 68fa6f7
Show file tree
Hide file tree
Showing 4 changed files with 130 additions and 27 deletions.
7 changes: 5 additions & 2 deletions auth/lti/auth.php
View file
Expand Up @@ -199,7 +199,6 @@ public function find_or_create_user_from_membership(array $member, string $iss,
}
}
$user = $this->create_new_account($member, $iss);
$this->update_user_account($user, $member, $iss);
return \core_user::get_user($user->id);
}
}
Expand Down Expand Up @@ -381,7 +380,11 @@ protected function update_user_account(stdClass $user, array $userdata, string $
'lastname' => $userdata['family_name'] ?? $iss,
'email' => $email
];
user_update_user($update);
$userfieldstocompare = array_intersect_key((array) $user, $update);

if (!empty(array_diff($update, $userfieldstocompare))) {
user_update_user($update); // Only update if there's a change.
}

if (!empty($userdata['picture'])) {
try {
Expand Down
50 changes: 39 additions & 11 deletions auth/lti/tests/auth_test.php
View file
Expand Up @@ -249,12 +249,15 @@ public function test_find_or_create_user_from_launch(?array $legacydata, array $
$mockjwtdata = $this->get_mock_launchdata_for_user($launchdata['user'], $launchdata['migration_claim'] ?? []);

// Authenticate the platform user.
$sink = $this->redirectEvents();
$countusersbefore = $DB->count_records('user');
$user = $auth->find_or_create_user_from_launch($mockjwtdata, true, $legacysecrets);
if (!empty($expected['migration_debugging'])) {
$this->assertDebuggingCalled();
}
$countusersafter = $DB->count_records('user');
$events = $sink->get_events();
$sink->close();

// Verify user count is correct. i.e. no user is created when migration claim is correctly processed or when
// the user has authenticated with the tool before.
Expand Down Expand Up @@ -295,15 +298,18 @@ public function test_find_or_create_user_from_launch(?array $legacydata, array $
$this->verify_user_profile_image_updated($user->id);
}

// If migrated, verify the user account is reusing the legacy user account.
if (!empty($expected['migrated']) && $expected['migrated']) {
// If migrated, verify the user account is reusing the legacy user account.
$legacyuserids = array_column($legacyusers, 'id');
$this->assertContains($user->id, $legacyuserids);
}

// If the user is authenticating a second time, confirm the same account is being returned.
if (isset($firstauthuser)) {
$this->assertInstanceOf(\core\event\user_updated::class, $events[0]);
} else if (isset($firstauthuser)) {
// If the user is authenticating a second time, confirm the same account is being returned.
$this->assertEquals($firstauthuser->id, $user->id);
$this->assertEmpty($events); // The user authenticated with the same data once before, so we don't expect an update.
} else {
// The user wasn't migrated and hasn't launched before, so we expect a user_created event.
$this->assertInstanceOf(\core\event\user_created::class, $events[0]);
}
}

Expand Down Expand Up @@ -819,9 +825,12 @@ public function test_find_or_create_user_from_membership(?array $legacydata, arr
$mockmemberdata = $this->get_mock_member_data_for_user($memberdata['user'], $memberdata['legacy_user_id'] ?? '');

// Authenticate the platform user.
$sink = $this->redirectEvents();
$countusersbefore = $DB->count_records('user');
$user = $auth->find_or_create_user_from_membership($mockmemberdata, $iss, $legacyconsumerkey ?? '');
$countusersafter = $DB->count_records('user');
$events = $sink->get_events();
$sink->close();

// Verify user count is correct. i.e. no user is created when migration claim is correctly processed or when
// the user has authenticated with the tool before.
Expand Down Expand Up @@ -857,15 +866,18 @@ public function test_find_or_create_user_from_membership(?array $legacydata, arr
break;
}

// If migrated, verify the user account is reusing the legacy user account.
if (!empty($expected['migrated']) && $expected['migrated']) {
// If migrated, verify the user account is reusing the legacy user account.
$legacyuserids = array_column($legacyusers, 'id');
$this->assertContains($user->id, $legacyuserids);
}

// If the user is authenticating a second time, confirm the same account is being returned.
if (isset($firstauthuser)) {
$this->assertInstanceOf(\core\event\user_updated::class, $events[0]);
} else if (isset($firstauthuser)) {
// If the user is authenticating a second time, confirm the same account is being returned.
$this->assertEquals($firstauthuser->id, $user->id);
$this->assertEmpty($events); // The user authenticated with the same data once before, so we don't expect an update.
} else {
// The user wasn't migrated and hasn't launched before, so we expect a user_created event.
$this->assertInstanceOf(\core\event\user_created::class, $events[0]);
}
}

Expand Down Expand Up @@ -1090,7 +1102,23 @@ public function membership_data_provider(): array {
'PII' => self::PII_NONE,
'migrated' => false
]
]
],
'Existing (linked) platform learner including PII, no legacy data, no consumer key bound, no legacy id' => [
'legacy_data' => null,
'launch_data' => [
'has_authenticated_before' => true,
'user' => $this->get_mock_users_with_ids(
['1'],
'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner'
)[0],
],
'iss' => $this->issuer,
'legacy_consumer_key' => null,
'expected' => [
'PII' => self::PII_ALL,
'migrated' => false
]
],
];
}

Expand Down
25 changes: 21 additions & 4 deletions enrol/lti/classes/local/ltiadvantage/repository/user_repository.php
View file
Expand Up @@ -162,19 +162,36 @@ public function save(user $user): user {
"to user '{$ltiuser->userid}' and can't be associated with another user '{$userrecord->id}'.");
}

$userrecord->timemodified = $timenow;
$ltiuserrecord->timemodified = $timenow;
\user_update_user($userrecord);
// Only update the Moodle user record if something has changed.
$rawuser = \core_user::get_user($userrecord->id);
$userfieldstocompare = array_intersect_key(
(array) $rawuser,
(array) $userrecord
);
if (!empty(array_diff((array) $userrecord, $userfieldstocompare))) {
\user_update_user($userrecord);
}
unset($userrecord->id);

$ltiuserrecord->timemodified = $timenow;
$DB->update_record($this->ltiuserstable, $ltiuserrecord);
} else {
// Validate uniqueness of the lti user, in the case of a stale object coming in to be saved.
if ($this->user_exists_for_tool($user)) {
throw new \coding_exception("Cannot create duplicate LTI user '{$user->get_localid()}' for resource " .
"'{$user->get_resourceid()}'.");
}

// Only update the Moodle user record if something has changed.
$userid = $userrecord->id;
\user_update_user($userrecord);
$rawuser = \core_user::get_user($userid);
$userfieldstocompare = array_intersect_key(
(array) $rawuser,
(array) $userrecord
);
if (!empty(array_diff((array) $userrecord, $userfieldstocompare))) {
\user_update_user($userrecord);
}
unset($userrecord->id);

// Create the lti_user record, holding details that have a lifespan equal to that of the enrolment instance.
Expand Down
75 changes: 65 additions & 10 deletions enrol/lti/tests/local/ltiadvantage/repository/user_repository_test.php
View file
Expand Up @@ -31,9 +31,10 @@ class user_repository_test extends \advanced_testcase {
* Helper to generate a new user instance.
*
* @param int $mockresourceid used to spoof a published resource, to which this user is associated.
* @param array $userfields user information like city, timezone which would normally come from the tool configuration.
* @return user a user instance
*/
protected function generate_user(int $mockresourceid = 1): user {
protected function generate_user(int $mockresourceid = 1, array $userfields = []): user {
global $CFG;
$registration = application_registration::create(
'Test',
Expand Down Expand Up @@ -63,16 +64,35 @@ protected function generate_user(int $mockresourceid = 1): user {
$savedcontext->get_id());
$savedresourcelink = $resourcelinkrepo->save($resourcelink);

$user = $this->getDataGenerator()->create_user();
// Create a user using the DB defaults to simulate what would have occurred during an auth_lti user auth.
$user = $this->getDataGenerator()->create_user([
'city' => '',
'country' => '',
'institution' => '',
'timezone' => '99',
'maildisplay' => 2,
'lang' => 'en'
]);

$userdefaultvalues = [
'lang' => $CFG->lang,
'city' => '',
'country' => '',
'institution' => '',
'timezone' => '99',
'maildisplay' => 2
];
if (empty($userfields)) {
// If userfields is omitted, assume the tool default configuration values (as if 'User default values' are unchanged).
$userfields = $userdefaultvalues;
} else {
// If they have been provided, merge and override the defaults.
$userfields = array_merge($userdefaultvalues, $userfields);
}
$ltiuser = $savedresourcelink->add_user(
$user->id,
'source-id-123',
$CFG->lang,
'Perth',
'AU',
'An Example Institution',
'99',
2,
...array_values($userfields)
);

$ltiuser->set_lastgrade(67.33333333);
Expand Down Expand Up @@ -141,19 +161,45 @@ protected function assert_user_db_values(user $expected) {
}

/**
* Tests adding a user to the store.
* Tests adding a user to the store, assuming that the user has been created using the default 'user default values'.
*
* @covers ::save
*/
public function test_save_new() {
public function test_save_new_unchanged_user_defaults() {
$this->resetAfterTest();
$user = $this->generate_user();
$userrepo = new user_repository();
$sink = $this->redirectEvents();
$saveduser = $userrepo->save($user);
$events = $sink->get_events();
$sink->close();

$this->assertIsInt($saveduser->get_id());
$this->assert_same_user_values($user, $saveduser, true);
$this->assert_user_db_values($saveduser);
// No change to underlying user: city, etc. take on default values matching those of the existing user record.
$this->assertEmpty($events);
}

/**
* Tests adding a user to the store, assuming that the user has been created using modified 'user default values'.
*
* @covers ::save
*/
public function test_save_new_changed_user_defaults() {
$this->resetAfterTest();
$user = $this->generate_user(1, ['city' => 'Perth']);
$userrepo = new user_repository();
$sink = $this->redirectEvents();
$saveduser = $userrepo->save($user);
$events = $sink->get_events();
$sink->close();

$this->assertIsInt($saveduser->get_id());
$this->assert_same_user_values($user, $saveduser, true);
$this->assert_user_db_values($saveduser);
// The underlying user record will change: city ('Perth') differs from that of the existing user ('').
$this->assertInstanceOf(\core\event\user_updated::class, $events[0]);
}

/**
Expand All @@ -165,16 +211,25 @@ public function test_save_existing() {
$this->resetAfterTest();
$user = $this->generate_user();
$userrepo = new user_repository();
$sink = $this->redirectEvents();
$saveduser = $userrepo->save($user);
$events = $sink->get_events();
$sink->close();
$this->assertEmpty($events); // No event for the first save, since the underlying user record is unchanged.

$saveduser->set_city('New City');
$saveduser->set_country('NZ');
$saveduser->set_lastgrade(99.99999999);
$sink = $this->redirectEvents();
$saveduser2 = $userrepo->save($saveduser);
$events = $sink->get_events();
$sink->close();

$this->assertEquals($saveduser->get_id(), $saveduser2->get_id());
$this->assert_same_user_values($saveduser, $saveduser2, true);
$this->assert_user_db_values($saveduser2);
// The underlying user record will change now, since city and country have changed.
$this->assertInstanceOf(\core\event\user_updated::class, $events[0]);
}

/**
Expand Down

0 comments on commit 68fa6f7

Please sign in to comment.