diff --git a/classes/local/store/object_file_system.php b/classes/local/store/object_file_system.php index 202ad43e..3cfe18b0 100644 --- 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 index 8691315e..3306614a 100644 --- 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 index 00052480..23d660f9 100644 --- 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 index eb3bfb1e..bedb05fd 100644 --- 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.