refactor: integrate tagpushedtime into single update query

catalyst · Aug 19, 2024 · e3dd9d0 · e3dd9d0
1 parent 165fa0a
commit e3dd9d0
Show file tree

Hide file tree

Showing 4 changed files with 27 additions and 29 deletions.
diff --git a/classes/local/store/object_file_system.php b/classes/local/store/object_file_system.php
@@ -1192,16 +1192,20 @@ public function push_object_tags(string $contenthash) {
             // Avoid unnecessarily checking tags, since this is an extra API call.
             $canset = $objectexists && (tag_manager::can_overwrite_object_tags() ||
                 empty($this->get_external_client()->get_object_tags($contenthash)));
+            $timepushed = 0;
 
             if ($canset) {
                 $tags = tag_manager::gather_object_tags_for_upload($contenthash);
                 $this->get_external_client()->set_object_tags($contenthash, $tags);
                 tag_manager::store_tags_locally($contenthash, $tags);
-                tag_manager::record_tag_pushed_time($contenthash, time());
+
+                // Record the time it was actually pushed to the external store
+                // (i.e. not when it existed already and we pulled the tags down).
+                $timepushed = time();
             }
 
             // Regardless, it has synced.
-            tag_manager::mark_object_tag_sync_status($contenthash, tag_manager::SYNC_STATUS_COMPLETE);
+            tag_manager::mark_object_tag_sync_status($contenthash, tag_manager::SYNC_STATUS_COMPLETE, $timepushed);
         } catch (Throwable $e) {
             $lock->release();
 

diff --git a/classes/local/tag/tag_manager.php b/classes/local/tag/tag_manager.php
@@ -149,23 +149,25 @@ public static function get_objects_needing_sync(int $limit) {
      * @param string $contenthash
      * @param int $status one of SYNC_STATUS_* constants
      */
-    public static function mark_object_tag_sync_status(string $contenthash, int $status) {
+    public static function mark_object_tag_sync_status(string $contenthash, int $status, int $tagpushedtime = 0) {
         global $DB;
         if (!in_array($status, self::SYNC_STATUSES)) {
             throw new coding_exception("Invalid object tag sync status " . $status);
         }
-        $DB->set_field('tool_objectfs_objects', 'tagsyncstatus', $status, ['contenthash' => $contenthash]);
-    }
 
-    /**
-     * Updates the tagslastpushed time for a given object.
-     * This should only be done once successfully pushed to the external store.
-     * @param string $contenthash
-     * @param int $time time to set
-     */
-    public static function record_tag_pushed_time(string $contenthash, int $time) {
-        global $DB;
-        $DB->set_field('tool_objectfs_objects', 'tagslastpushed', $time, ['contenthash' => $contenthash]);
+        $timeupdate = !empty($tagpushedtime) ? ',tagslastpushed = :time' : '';
+        $params = [
+            'status' => $status,
+            'contenthash' => $contenthash,
+            'time' => $tagpushedtime,
+        ];
+
+        // Need raw execute since update_records requires an id column, but we use contenthash instead.
+        $DB->execute("UPDATE {tool_objectfs_objects}
+                         SET tagsyncstatus = :status
+                         {$timeupdate}
+                       WHERE contenthash = :contenthash",
+                       $params);
     }
 
     /**

diff --git a/tests/local/tagging_test.php b/tests/local/tagging_test.php
@@ -330,21 +330,6 @@ public function test_object_tag_sync_error() {
         $this->assertEquals(tag_manager::SYNC_STATUS_ERROR, $status);
     }
 
-    /**
-     * Tests tag_manager::record_tag_pushed_time
-     * @covers \tool_objectfs\local\tag_manager::record_tag_pushed_time
-     */
-    public function test_record_tag_pushed_time() {
-        global $DB;
-        $object = $this->create_remote_object();
-        $initialtime = $DB->get_field('tool_objectfs_objects', 'tagslastpushed', ['contenthash' => $object->contenthash]);
-        $this->assertEquals(0, $initialtime);
-
-        tag_manager::record_tag_pushed_time($object->contenthash, 100);
-        $newtime = $DB->get_field('tool_objectfs_objects', 'tagslastpushed', ['contenthash' => $object->contenthash]);
-        $this->assertEquals(100, $newtime);
-    }
-
     /**
      * Tests tag_manger::get_tag_sync_status_summary
      * @covers \tool_objectfs\local\tag_manager::get_tag_sync_status_summary

diff --git a/tests/object_file_system_test.php b/tests/object_file_system_test.php
@@ -1102,6 +1102,7 @@ public function test_push_object_tags_replicated(bool $canoverride) {
         // Tags should now be replicated locally.
         $localtags = $DB->get_records('tool_objectfs_object_tags', ['contenthash' => $object->contenthash]);
         $externaltags = $this->filesystem->get_external_client()->get_object_tags($object->contenthash);
+        $time = $DB->get_field('tool_objectfs_objects', 'tagslastpushed', ['id' => $object->id]);
 
         if ($canoverride) {
             // If can override, we expect it to be overwritten by the tags defined in the sources.
@@ -1110,12 +1111,18 @@ public function test_push_object_tags_replicated(bool $canoverride) {
 
             // Also expect the external store to be updated.
             $this->assertCount($expectednum, $externaltags);
+
+            // Tag push time should be set, since it actually pushed the tags.
+            $this->assertNotEquals(0, $time);
         } else {
             // If cannot overwrite, no tags should be synced.
             $this->assertCount(0, $localtags);
 
             // External store should not be changed.
             $this->assertCount(count($testtags), $externaltags);
+
+            // The tag last push time should remain unchanged, since it didn't actually push any tags.
+            $this->assertEquals(0, $time);
         }
 
         // Ensure status changed to not needing sync.