MDL-70059 core_badges: avoid duplicate key error
authorSara Arjona <sara@moodle.com>
Thu, 29 Oct 2020 17:25:16 +0000 (18:25 +0100)
committerSara Arjona <sara@moodle.com>
Mon, 2 Nov 2020 09:48:06 +0000 (10:48 +0100)
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.

badges/tests/badgeslib_test.php
lib/badgeslib.php

index e8a6956..f764c8a 100644 (file)
@@ -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;
         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);
         }
 
         $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_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);
             $this->assertEmpty($record);
+            $total = $DB->count_records('badge_backpack');
+            $this->assertEquals(0, $total);
         } else {
             $this->assertNotEmpty($record);
         } else {
             $this->assertNotEmpty($record);
+            $this->assertEquals($record->userid, $userid);
         }
 
         if ($duplicates) {
         }
 
         if ($duplicates) {
@@ -1061,19 +1060,66 @@ class core_badges_badgeslib_testcase extends advanced_testcase {
      *
      * @return array
      */
      *
      * @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 [
         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
      *
      * @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;
      */
     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
      */
     /**
      * 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],
         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.
      * 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 = [
      */
     public function test_badges_get_site_primary_backpack($withauth) {
         $data = [
@@ -1289,7 +1335,7 @@ class core_badges_badgeslib_testcase extends advanced_testcase {
      *
      * @return array
      */
      *
      * @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],
         return [
             "Test with auth details" => [true],
             "Test without auth details" => [false],
index 81bb7ef..2c6e44b 100644 (file)
@@ -856,13 +856,13 @@ function badges_save_external_backpack(stdClass $data) {
         $backpack->sortorder = $data->sortorder;
     }
 
         $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;
         $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);
 
     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;
 
         $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;
     }
 
         return $backpack->externalbackpackid;
     }