Skip to content

Commit

Permalink
MDL-70059 core_badges: avoid duplicate key error
Browse files Browse the repository at this point in the history 
When 2 or more backpack were created without credentials,
a "Duplicate key value violates unique constraint" error
was raised because externalbackpackid was not taking the
correct value.
Other improvements have been done to the code too in order
to make it more readable.
  • Loading branch information
@sarjona
sarjona committed Nov 2, 2020
1 parent 6bb7478 commit 01c8c88
Show file tree
Hide file tree
Showing 2 changed files with 92 additions and 47 deletions.
112 changes: 79 additions & 33 deletions badges/tests/badgeslib_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1012,41 +1012,40 @@ public function save_backpack_credentials_provider(): array {
];
}


/**
* Test badges_save_external_backpack without any auth details and also tests duplicate entries.
* Test badges_save_external_backpack.
*
* @param boolean $withauth Test with authentication details provided
* @param boolean $duplicates Test for duplicates
* @dataProvider test_badges_save_external_backpack_provider
* @throws dml_exception
* @dataProvider badges_save_external_backpack_provider
* @param array $data Backpack data to save.
* @param bool $adduser True if a real user has to be used for creating the backpack; false otherwise.
* @param bool $duplicates True if duplicates has to be tested too; false otherwise.
*/
public function test_badges_save_external_backpack($withauth, $duplicates) {
public function test_badges_save_external_backpack(array $data, bool $adduser, bool $duplicates) {
global $DB;
$this->resetAfterTest();
$user = $this->getDataGenerator()->create_user();

$data = [
'userid' => $user->id,
'apiversion' => 2,
'backpackapiurl' => 'https://api.ca.badgr.io/v2',
'backpackweburl' => 'https://ca.badgr.io',
];
$this->resetAfterTest();

if ($withauth) {
$data['backpackemail'] = 'test@test.com';
$data['password'] = 'test';
$userid = 0;
if ($adduser) {
$user = $this->getDataGenerator()->create_user();
$userid = $user->id;
$data['userid'] = $user->id;
}

$result = badges_save_external_backpack((object) $data);
$this->assertNotEquals(0, $result);
$record = $DB->get_record('badge_external_backpack', ['id' => $result]);
$this->assertEquals($record->backpackweburl, $data['backpackweburl']);
$this->assertEquals($record->backpackapiurl, $data['backpackapiurl']);
$record = $DB->get_record('badge_backpack', ['userid' => $user->id]);
if (!$withauth) {

$record = $DB->get_record('badge_backpack', ['externalbackpackid' => $result]);
if (!array_key_exists('backpackemail', $data) && !array_key_exists('password', $data)) {
$this->assertEmpty($record);
$total = $DB->count_records('badge_backpack');
$this->assertEquals(0, $total);
} else {
$this->assertNotEmpty($record);
$this->assertEquals($record->userid, $userid);
}

if ($duplicates) {
Expand All @@ -1061,19 +1060,66 @@ public function test_badges_save_external_backpack($withauth, $duplicates) {
*
* @return array
*/
public function test_badges_save_external_backpack_provider() {
public function badges_save_external_backpack_provider() {
$data = [
'apiversion' => 2,
'backpackapiurl' => 'https://api.ca.badgr.io/v2',
'backpackweburl' => 'https://ca.badgr.io',
];
return [
"Test without any auth details and duplicates" => [
false, true
'Test without user and auth details. Check duplicates too' => [
'data' => $data,
'adduser' => false,
'duplicates' => true,
],
'Test without user and auth details. No duplicates' => [
'data' => $data,
'adduser' => false,
'duplicates' => false,
],
'Test with user and without auth details' => [
'data' => $data,
'adduser' => true,
'duplicates' => false,
],
'Test with user and without auth details. Check duplicates too' => [
'data' => $data,
'adduser' => true,
'duplicates' => true,
],
'Test with empty backpackemail, password and id' => [
'data' => array_merge($data, [
'backpackemail' => '',
'password' => '',
'id' => 0,
]),
'adduser' => false,
'duplicates' => false,
],
"Test without any auth details and without duplicates" => [
false, false
'Test with empty backpackemail, password and id but with user' => [
'data' => array_merge($data, [
'backpackemail' => '',
'password' => '',
'id' => 0,
]),
'adduser' => true,
'duplicates' => false,
],
"Test with auth details and duplicates" => [
true, true
'Test with auth details but without user' => [
'data' => array_merge($data, [
'backpackemail' => 'test@test.com',
'password' => 'test',
]),
'adduser' => false,
'duplicates' => false,
],
"Test with any auth details and duplicates" => [
true, false
'Test with auth details and user' => [
'data' => array_merge($data, [
'backpackemail' => 'test@test.com',
'password' => 'test',
]),
'adduser' => true,
'duplicates' => false,
],
];
}
Expand All @@ -1083,7 +1129,7 @@ public function test_badges_save_external_backpack_provider() {
*
* @param boolean $isadmin
* @param boolean $updatetest
* @dataProvider test_badges_create_site_backpack_provider
* @dataProvider badges_create_site_backpack_provider
*/
public function test_badges_create_site_backpack($isadmin, $updatetest) {
global $DB;
Expand Down Expand Up @@ -1129,7 +1175,7 @@ public function test_badges_create_site_backpack($isadmin, $updatetest) {
/**
* Provider for test_badges_(create/update)_site_backpack
*/
public function test_badges_create_site_backpack_provider() {
public function badges_create_site_backpack_provider() {
return [
"Test as admin user - creation test" => [true, true],
"Test as admin user - update test" => [true, false],
Expand Down Expand Up @@ -1253,7 +1299,7 @@ public function test_badges_get_user_backpack() {
* Test the badges_get_site_primary_backpack function
*
* @param boolean $withauth Testing with authentication or not.
* @dataProvider test_badges_get_site_primary_backpack_provider
* @dataProvider badges_get_site_primary_backpack_provider
*/
public function test_badges_get_site_primary_backpack($withauth) {
$data = [
Expand Down Expand Up @@ -1289,7 +1335,7 @@ public function test_badges_get_site_primary_backpack($withauth) {
*
* @return array
*/
public function test_badges_get_site_primary_backpack_provider() {
public function badges_get_site_primary_backpack_provider() {
return [
"Test with auth details" => [true],
"Test without auth details" => [false],
Expand Down
27 changes: 13 additions & 14 deletions lib/badgeslib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -856,13 +856,13 @@ function badges_save_external_backpack(stdClass $data) {
$backpack->sortorder = $data->sortorder;
}

$method = 'insert_record';
if (isset($data->id) && $data->id) {
if (empty($data->id)) {
$backpack->id = $DB->insert_record('badge_external_backpack', $backpack);
} else {
$backpack->id = $data->id;
$method = 'update_record';
$DB->update_record('badge_external_backpack', $backpack);
}
$record = $DB->$method('badge_external_backpack', $backpack, true);
$data->externalbackpackid = $data->id ?? $record;
$data->externalbackpackid = $backpack->id;

unset($data->id);
badges_save_backpack_credentials($data);
Expand All @@ -888,19 +888,18 @@ function badges_save_backpack_credentials(stdClass $data) {
$backpack->backpackuid = $data->backpackuid ?? 0;
$backpack->autosync = $data->autosync ?? 0;

$id = null;
if (isset($data->badgebackpack) && $data->badgebackpack) {
$id = $data->badgebackpack;
} else if (isset($data->id) && $data->id) {
$id = $data->id;
if (!empty($data->badgebackpack)) {
$backpack->id = $data->badgebackpack;
} else if (!empty($data->id)) {
$backpack->id = $data->id;
}

$method = $id ? 'update_record' : 'insert_record';
if ($id) {
$backpack->id = $id;
if (empty($backpack->id)) {
$backpack->id = $DB->insert_record('badge_backpack', $backpack);
} else {
$DB->update_record('badge_backpack', $backpack);
}

$DB->$method('badge_backpack', $backpack);
return $backpack->externalbackpackid;
}

Expand Down

0 comments on commit 01c8c88

Please sign in to comment.