WIP more tweaks

Author: matthewhilton

classes/local/store/object_file_system.php

@@ -1160,11 +1160,15 @@ private function update_object(array $result): array {
 
     /**
      * Syncs tags post upload for a given hash.
-     * Note this function assumes tagging is enable and supported by the fs.
+     * External client must support tagging.
      *
      * @param string $contenthash file to sync tags for
      */
     public function sync_object_tags(string $contenthash) {
+        if (!$this->get_external_client()->is_tagging_supported()) {
+            throw new coding_exception("Cannot sync tags, external client does not support tagging.");
+        }
+
         // Get a lock before syncing, to ensure other parts of objectfs are not moving/interacting with this object.
         $lock = $this->acquire_object_lock($contenthash, 5);
 
@@ -1177,18 +1181,17 @@ public function sync_object_tags(string $contenthash) {
             $objectexists = $this->is_file_readable_externally_by_hash($contenthash);
             // If object does not exist, cannot sync tags to nothing, abort.
             if (!$objectexists) {
-                // TODO maybe this should be a failed status?
                 tag_manager::mark_object_tag_sync_status($contenthash, tag_manager::SYNC_STATUS_SYNC_NOT_REQUIRED);
                 return;
             }
 
-            // Cannot override and object exists, query existing and store.
-            if (!manager::can_override_existing_objects() && $objectexists) {
+            // Cannot override and object exists, query existing and store locally.
+            if ($objectexists && !manager::can_override_existing_objects()) {
                 // Query existing tags and store them.
                 $existingtags = $this->get_external_client()->get_object_tags($contenthash);
                 tag_manager::store_tags_locally($contenthash, $existingtags);
 
-            // Else can override, upload new and store.
+            // Else can override, upload new and store locally.
             } else {
                 $tags = tag_manager::gather_object_tags_for_upload($contenthash);
                 $this->get_external_client()->set_object_tags($contenthash, $tags);

classes/local/store/s3/client.php

@@ -929,7 +929,7 @@ public function test_set_object_tag(): stdClass {
      * @return array tags in s3 format.
      */
     private function convert_tags_to_s3_format(array $tags): array {
-        foreach($tags as $key => $value) {
+        foreach ($tags as $key => $value) {
             $s3tags[] = [
                 'Key' => $key,
                 'Value' => $value,
@@ -972,7 +972,7 @@ public function get_object_tags(string $contenthash): array {
 
         // Ensure tags are what we expect, and AWS have not changed the format.
         if (empty($result->toArray()['TagSet'])) {
-            throw new coding_exception("Unexpected tag format received"); // TODO different ex type.
+            throw new coding_exception("Unexpected tag format received. Result did not contain a TagSet");
         }
 
         // Convert from S3 format to key=>value format.

classes/local/store/s3/file_system.php

@@ -89,7 +89,7 @@ public function copy_from_local_to_external($contenthash) {
 
         try {
             // If cannot override, and already exists, skip.
-            // We don't want to upload and potentially override.
+            // We don't want to upload and potentially override the object, or associated data e.g. tags.
             if (!manager::can_override_existing_objects() && $this->is_file_readable_externally_by_hash($contenthash)) {
                 return true;
             }

classes/local/tag/environment_source.php

@@ -33,13 +33,21 @@ public static function get_identifier(): string {
         return 'environment';
     }
 
+    /**
+     * Description for source displayed in the admin settings.
+     * @return string
+     */
     public static function get_description(): string {
         return get_string('tagsource:environment', 'tool_objectfs', self::get_env());
     }
 
+    /**
+     * Returns current env value from $CFG
+     * @return string|null string if set, else null
+     */
     private static function get_env(): ?string {
         global $CFG;
-        return $CFG->objectfs_environment_name ?? null;
+        return !empty($CFG->objectfs_environment_name) ? $CFG->objectfs_environment_name : null;
     }
 
     /**

classes/local/tag/mime_type_source.php

@@ -33,6 +33,10 @@ public static function get_identifier(): string {
         return 'mimetype';
     }
 
+    /**
+     * Description for source displayed in the admin settings.
+     * @return string
+     */
     public static function get_description(): string {
         return get_string('tagsource:mimetype', 'tool_objectfs');
     }

classes/local/tag/tag_manager.php

@@ -17,9 +17,7 @@
 namespace tool_objectfs\local\tag;
 
 use coding_exception;
-use html_writer;
 use tool_objectfs\local\manager;
-use tool_objectfs\local\store\object_file_system;
 
 defined('MOODLE_INTERNAL') || die();
 require_once($CFG->dirroot . '/admin/tool/objectfs/lib.php');
@@ -78,11 +76,6 @@ public static function is_tagging_enabled_and_supported(): bool {
         return $enabledinconfig && $supportedbyfs;
     }
 
-    // TODO unit tests for this functionality
-    public static function can_override_existing_tags(): bool {
-        return !empty(get_config('tool_objectfs', 'overridetags'));
-    }
-
     /**
      * Gathers the tag values for a given content hash
      * @param string $contenthash
@@ -141,8 +134,6 @@ public static function get_objects_needing_sync(int $limit) {
         // Find object records where the status is NEEDS_SYNC and is replicated.
         [$insql, $inparams] = $DB->get_in_or_equal([OBJECT_LOCATION_DUPLICATED, OBJECT_LOCATION_EXTERNAL, OBJECT_LOCATION_ORPHANED], SQL_PARAMS_NAMED);
         $inparams['syncstatus'] = self::SYNC_STATUS_NEEDS_SYNC;
-
-        // TODO somehow exclude ones already queued ?
         $records = $DB->get_records_select('tool_objectfs_objects', 'tagsyncstatus = :syncstatus AND location ' . $insql, $inparams, '', 'contenthash', 0, $limit);
         return array_column($records, 'contenthash');
     }

db/tasks.php

@@ -116,6 +116,7 @@
         'dayofweek' => '*',
         'month'     => '*',
         // Default disabled - intended to be manually run.
+        // Also, objectfs tagging support is default off.
         'disabled' => true,
     ),
 );

tests/local/tagging_test.php

@@ -77,7 +77,7 @@ public function test_tag_sources_identifier(tag_source $source) {
      */
     public function test_tag_sources_value(tag_source $source) {
         // Ensure tag value is < 128 chars, AWS & Azure spec allow for 256, but we reserve 128 for future use.
-        $file = $this->create_duplicated_object();
+        $file = $this->create_duplicated_object('tag source value test ' . $source->get_identifier());
         $value = $source->get_value_for_contenthash($file->contenthash);
 
         // Null value - allowed, but means we cannot test.
@@ -90,9 +90,6 @@ public function test_tag_sources_value(tag_source $source) {
         $this->assertGreaterThan(0, $count);
     }
 
-    // TODO ensure all sources have unique keys.
-    // TODO ensure keys and values are all strings.
-
     /**
      * Provides values to test_is_tagging_enabled_and_supported
      * @return array
@@ -141,8 +138,12 @@ public function test_is_tagging_enabled_and_supported(bool $enabledinconfig, boo
         $this->assertEquals($expected, tag_manager::is_tagging_enabled_and_supported());
     }
 
+    /**
+     * Tests gather_object_tags_for_upload
+     * @covers \tool_objectfs\local\tag_manager::gather_object_tags_for_upload
+     */
     public function test_gather_object_tags_for_upload() {
-        $object = $this->create_duplicated_object();
+        $object = $this->create_duplicated_object('gather tags for upload test');
         $tags = tag_manager::gather_object_tags_for_upload($object->contenthash);
 
         $this->assertArrayHasKey('mimetype', $tags);
@@ -151,6 +152,10 @@ public function test_gather_object_tags_for_upload() {
         $this->assertEquals('test', $tags['environment']);
     }
 
+    /**
+     * Tests store_tags_locally
+     * @covers \tool_objectfs\local\tag_manager::store_tags_locally
+     */
     public function test_store_tags_locally() {
         global $DB;
 
@@ -228,13 +233,13 @@ public function test_get_objects_needing_sync(int $location, int $syncstatus, bo
         // Create the test object at the required location.
         switch ($location) {
             case OBJECT_LOCATION_DUPLICATED:
-                $object = $this->create_duplicated_object();
+                $object = $this->create_duplicated_object('tagging test object duplicated');
                 break;
             case OBJECT_LOCATION_LOCAL:
-                $object = $this->create_local_object();
+                $object = $this->create_local_object('tagging test object local');
                 break;
             case OBJECT_LOCATION_EXTERNAL:
-                $object = $this->create_remote_object();
+                $object = $this->create_remote_object('tagging test object remote');
                 break;
             default:
                 throw new coding_exception("Object location not handled in test");
@@ -261,16 +266,20 @@ public function test_get_objects_needing_sync_limit() {
         global $DB;
 
         // Create two duplicated objects needing sync.
-        $object = $this->create_duplicated_object();
+        $object = $this->create_duplicated_object('sync limit test duplicated');
         $DB->set_field('tool_objectfs_objects', 'tagsyncstatus', tag_manager::SYNC_STATUS_NEEDS_SYNC, ['id' => $object->id]);
-        $object = $this->create_remote_object();
+        $object = $this->create_remote_object('sync limit test remote');
         $DB->set_field('tool_objectfs_objects', 'tagsyncstatus', tag_manager::SYNC_STATUS_NEEDS_SYNC, ['id' => $object->id]);
 
         // Ensure a limit of 2 returns 2, and limit of 1 returns 1.
         $this->assertCount(2, tag_manager::get_objects_needing_sync(2));
         $this->assertCount(1, tag_manager::get_objects_needing_sync(1));
     }
 
+    /**
+     * Test get_tag_summary_html
+     * @covers \tool_objectfs\local\tag_manager::get_tag_summary_html
+     */
     public function test_get_tag_summary_html() {
         // Quick test just to ensure it generates and nothing explodes.
         $html = tag_manager::get_tag_summary_html();

tests/task/populate_objects_filesize_test.php

@@ -179,6 +179,7 @@ public function test_that_non_null_values_are_not_updated() {
      */
     public function test_orphaned_objects_are_not_updated() {
         global $DB;
+        $objects = $DB->get_records('tool_objectfs_objects');
         $file1 = $this->create_local_file("Test 1");
         $this->create_local_file("Test 2");
         $this->create_local_file("Test 3");