WIP more cleanups

Author: matthewhilton

classes/check/tagging_status.php

@@ -29,7 +29,15 @@
  * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 class tagging_status extends check {
-    // TODO action link.
+    /**
+     * Link to ObjectFS settings page.
+     *
+     * @return \action_link|null
+     */
+    public function get_action_link(): ?\action_link {
+        $url = new \moodle_url('/admin/category.php', ['category' => 'tool_objectfs']);
+        return new \action_link($url, get_string('pluginname', 'tool_objectfs'));
+    }
 
     /**
      * Get result
@@ -46,11 +54,9 @@ public function get_result(): result {
         $result = $client->test_set_object_tag();
 
         if ($result->success) {
-            // TODO lang.
-            return new result(result::OK, 'tagging success', $result->details);
+            return new result(result::OK, get_string('check:tagging:ok', 'tool_objectfs'), $result->details);
         } else {
-            // TODO lang.
-            return new result(result::ERROR, 'tagging failed', $result->details);
+            return new result(result::ERROR, get_string('check:tagging:error', 'tool_objectfs'), $result->details);
         }
     }
 }

classes/local/store/s3/client.php

@@ -916,9 +916,8 @@ public function test_set_object_tag(): stdClass {
      * @param array $tags array of key=>value pairs to set as tags.
      */
     public function set_object_tags(string $contenthash, array $tags) {
-        // First convert PHP array tags to XML string.
+        // Convert tags into format S3 client expects.
         $s3tags = [];
-
         foreach($tags as $key => $value) {
             $s3tags[] = [
                 'Key' => $key,

classes/local/tag/tag_manager.php

@@ -45,6 +45,9 @@ class tag_manager {
      */
     public const SYNC_STATUS_SYNC_NOT_REQUIRED = 1;
 
+    /**
+     * @var array All possible tag sync statuses.
+     */
     public const SYNC_STATUSES = [
         self::SYNC_STATUS_NEEDS_SYNC,
         self::SYNC_STATUS_SYNC_NOT_REQUIRED
@@ -131,7 +134,7 @@ public static function update_tags_if_necessary(string $contenthash, bool $force
     }
 
     /**
-     * Updates the tags for a given file. Updates both local db and external.
+     * Updates the local db tag records, and marks object as needing sync to sync local tags to external.
      * @param int $objectid
      * @param array $tags
      */
@@ -202,7 +205,6 @@ public static function replicate_local_to_external_tags_for_object(string $conte
         }
 
         try {
-            
             // TODO refactor so all func use contenthash, no objectid.
             // Then also probably remove from DB.
             $tags = self::query_local_tags($contenthash);
@@ -249,12 +251,15 @@ private static function mark_object_tag_sync_status(string $contenthash, int $st
     }
 
     /**
-     * Returns true if the tags given are different
-     * @param array $a
-     * @param array $b
+     * Returns true if the tags given are different. They are different when their keys or values do not match, order does not matter.
+     * @param array $a array of key=>value where keys and values are both strings
+     * @param array $b array of key=>value where keys and values are both strings
      * @return bool
      */
     private static function are_tags_different(array $a, array $b): bool {
+        // Order the keys in both arrays, then do a simple equality check.
+        ksort($a);
+        ksort($b);
         return $a !== $b;
     }
 }

classes/task/queue_objects_needing_tags.php

@@ -46,8 +46,8 @@ public function execute() {
             return;
         }
 
-        // TODO configurable limit per run.	
-        $contenthashes = tag_manager::get_objects_needing_sync(1000);
+        $limit = get_config('tool_objectfs', 'maxtaggingperrun');
+        $contenthashes = tag_manager::get_objects_needing_sync($limit);
         mtrace("Found " . count($contenthashes) . " objects needing tag sync");
 
         foreach ($contenthashes as $contenthash) {

classes/task/update_object_tags.php

@@ -33,20 +33,22 @@ class update_object_tags extends adhoc_task {
      */
     public function execute() {
         if (!tag_manager::is_tagging_enabled_and_supported()) {
-            mtrace("Tagging feature not enabled or supported by filesystem, exiting early.");
+            mtrace("Tagging feature not enabled or supported by filesystem, exiting.");
             return;
         }
 
         // Find the object from the customdata.
         $contenthash = $this->get_custom_data()->contenthash ?? null;
 
         if (empty($contenthash)) {
-            mtrace("No content hash given, invalid customdata, exiting");
+            mtrace("No contenthash given, invalid customdata, exiting.");
             return;
         }
 
+        // Update local tags in db.
         tag_manager::update_tags_if_necessary($contenthash);
-        // TODO if this fails because of 404, just ignore since file is clearly not replicated yet
+
+        // Sync these to external store.
         tag_manager::replicate_local_to_external_tags_for_object($contenthash);
     }
 }

db/upgrade.php

@@ -171,7 +171,41 @@ function xmldb_tool_objectfs_upgrade($oldversion) {
         upgrade_plugin_savepoint(true, 2023013100, 'tool', 'objectfs');
     }
 
-    // TODO db update step for db.
+    if ($oldversion < 2023051704) {
+
+        // Define table tool_objectfs_object_tags to be created.
+        $table = new xmldb_table('tool_objectfs_object_tags');
+
+        // Adding fields to table tool_objectfs_object_tags.
+        $table->add_field('id', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, XMLDB_SEQUENCE, null);
+        $table->add_field('contenthash', XMLDB_TYPE_CHAR, '40', null, XMLDB_NOTNULL, null, null);
+        $table->add_field('tagkey', XMLDB_TYPE_CHAR, '15', null, XMLDB_NOTNULL, null, null);
+        $table->add_field('tagvalue', XMLDB_TYPE_CHAR, '20', null, XMLDB_NOTNULL, null, null);
+        $table->add_field('timemodified', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, null);
+
+        // Adding keys to table tool_objectfs_object_tags.
+        $table->add_key('primary', XMLDB_KEY_PRIMARY, ['id']);
+
+        // Adding indexes to table tool_objectfs_object_tags.
+        $table->add_index('objecttagkey_idx', XMLDB_INDEX_UNIQUE, ['contenthash', 'tagkey']);
+
+        // Conditionally launch create table for tool_objectfs_object_tags.
+        if (!$dbman->table_exists($table)) {
+            $dbman->create_table($table);
+        }
+
+        // Define field tagsyncstatus to be added to tool_objectfs_objects.
+        $table = new xmldb_table('tool_objectfs_objects');
+        $field = new xmldb_field('tagsyncstatus', XMLDB_TYPE_INTEGER, '2', null, XMLDB_NOTNULL, null, '0', 'filesize');
+
+        // Conditionally launch add field tagsyncstatus.
+        if (!$dbman->field_exists($table, $field)) {
+            $dbman->add_field($table, $field);
+        } 
+
+        // Objectfs savepoint reached.
+        upgrade_plugin_savepoint(true, 2023051704, 'tool', 'objectfs');
+    }
 
     return true;
 }

lang/en/tool_objectfs.php

@@ -274,3 +274,8 @@
 $string['checkproxy_range_request'] = 'Pre-signed URL range request proxy';
 
 $string['task:queueobjectsneedingtags'] = 'Queue objects needing tagging';
+$string['checktagging_status'] = 'Object tagging';
+$string['check:tagging:ok'] = 'Object tagging ok';
+$string['check:tagging:error'] = 'Error trying to tag object';
+$string['settings:maxtaggingperrun'] = 'Max objects tagged per run';
+$string['settings:maxtaggingperrun:desc'] = 'The maximum number of objects to be queued for tag updates per run of the queue_objects_needing_tags scheduled task.';

settings.php

@@ -250,11 +250,18 @@
     $settings->add(new admin_setting_configcheckbox('tool_objectfs/preferexternal',
         new lang_string('settings:preferexternal', 'tool_objectfs'), '', ''));
 
+    // Tagging settings.
     $settings->add(new admin_setting_heading('tool_objectfs/taggingsettings',
         new lang_string('settings:taggingheader', 'tool_objectfs'), ''));
 
     // TODO some info here about tagging, maybe a link to doc in git.
 
     $settings->add(new admin_setting_configcheckbox('tool_objectfs/taggingenabled',
         new lang_string('settings:taggingenabled', 'tool_objectfs'), '', ''));
+
+    $settings->add(new admin_setting_configtext('tool_objectfs/maxtaggingperrun',
+        new lang_string('settings:maxtaggingperrun', 'tool_objectfs'),
+        get_string('settings:maxtaggingperrun:desc', 'tool_objectfs'),
+        10000,
+        PARAM_INT));
 }

tests/local/tagging_test.php

@@ -84,6 +84,9 @@ 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