MDL-70059 core_badges: avoid duplicate key error

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.

diff --git a/badges/tests/badgeslib_test.php b/badges/tests/badgeslib_test.php
index e8a6956..f764c8a 100644 (file)
--- a/badges/tests/badgeslib_test.php
+++ b/badges/tests/badgeslib_test.php
@@ -1012,41 +1012,40 @@ class core_badges_badgeslib_testcase extends advanced_testcase {
         ];
     }
 
-
     /**
-     * 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) {
@@ -1061,19 +1060,66 @@ class core_badges_badgeslib_testcase extends advanced_testcase {
      *
      * @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,
             ],
         ];
     }
@@ -1083,7 +1129,7 @@ class core_badges_badgeslib_testcase extends advanced_testcase {
      *
      * @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;
@@ -1129,7 +1175,7 @@ class core_badges_badgeslib_testcase extends advanced_testcase {
     /**
      * 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],
@@ -1253,7 +1299,7 @@ class core_badges_badgeslib_testcase extends advanced_testcase {
      * 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 = [
@@ -1289,7 +1335,7 @@ class core_badges_badgeslib_testcase extends advanced_testcase {
      *
      * @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],

diff --git a/lib/badgeslib.php b/lib/badgeslib.php
index 81bb7ef..2c6e44b 100644 (file)
--- a/lib/badgeslib.php
+++ b/lib/badgeslib.php
@@ -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);
@@ -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;
     }