Add auto-provisioning for matomo #52

SimonasAdams · 2023-05-16T13:42:19Z

This commit contains the ability to auto provision sites based on either a value in the config.php file or via the instance form on the front end.

To auto provision on install or upgrade add a setting watool_matomo into config.php as an array, e.g:

$CFG->watool_matomo = [
        'instance1' => [        
                'apitoken' => 'abcdef123456',
                'siteurl' => 'https://example1.com'
        ],
        'instance2' => [
                'apitoken' => 'abcdef123456',
                'siteurl' => 'https://example2.com'
        ],
];

Where the apitoken is the auth_token of the user that is allowed to do API requests on the Matomo server, and the 'siteurl' is the URL of the Matomo server instance.

During install if the above setting is present it will loop through the array and attempt to create a site on the remote Matomo instance per array item. If successful, it create new records locally and stores the siteid, the apitoken, and the siteurl per instance created.

When manually adding a new instance via the front end, an auto provision attempt will be made if the form submission contains both the siteurl and the apitoken. If successful, the siteid is retrieved and stored against the local record. If the form is submitted with a siteid, there will be not attempt to register with the remote server.

Additionally, at the point of submisision the current CFG->wwwroot is stored against that instance locally. On page loads that check if the tag should be output, a check is made to see if the current CFG->wwwroot matches that stored against the local record. If it does not match, an attempt is made to update the remotely registered instance with the new url. If successful, this new url is then stored locally. This is to allow the remote to track if and when the url of the server changes.

classes/injector.php

brendanheywood · 2023-06-07T14:47:19Z

classes/tool/tool_base.php

@@ -144,4 +146,13 @@ public function trackurl($urlencode = false, $leadingslash = false): string {
        return $trackurl;
    }

+    /**
+     * Override in child to implement.


Can you please flesh this out more with how this should work generically, eg if someone wanted to come and add support for GA later on

That should be more descriptive now.

tool/matomo/classes/client.php

tool/matomo/lib.php

dmitriim · 2023-06-07T22:33:05Z

classes/form/edit.php

@@ -27,6 +27,9 @@

 use moodleform;
 use tool_webanalytics\plugin_manager;
+use tool_webanalytics\record;


Are those leftovers?

Correct and now removed.

classes/injector.php

tool/matomo/README.md

brendanheywood · 2023-07-08T04:35:02Z

tool/matomo/README.md

+- Set the global config settings 'apitoken' and 'apiurl' to enable auto provisioning. These can also be set in config.php e.g: 
+   - `$CFG->forced_plugin_settings['watool_matomo']['apiurl'] = 'https://matomo.org';`
+   - `$CFG->forced_plugin_settings['watool_matomo']['apitoken'] = 'xxxx';`
+- Auto provisioning attempts are made if the current site url has changed since any of the instances were stored.


Can you please flesh this out a bit more, this might only need comments but might be some missing code too. I want to understand what is happened if we roll this auto provisioning out toa pre production side, eg p-client.catalyst-au.net and they get a matomo instance, and then a few months later they go live and the dns changes to moodle.client.com

In this scenario I can see a method called update_site but it isn't actually called from anywhere?

I would expect this to continue using the same matomo id, ie the one instance will seamlessly show stats from before and after the dns change. This should be the same as if the client later changes their domain for marketing reasons, the stats should continue to be seamless.

It was previously just creating a new instance on the matomo server when the DNS changed but is now updating the url over the API if the DNS changes, essentially keeping the site id the same. Have fleshed this out in the docs.

Note that when manually adding an instance config on the front end it will also try to create/update over the API if the API config is in the form.

I have also documented these methods in unit tests for the watool_matomo plugin too.

brendanheywood · 2023-08-22T01:25:15Z

tool/matomo/README.md

@@ -0,0 +1,22 @@
+# Matomo (formerly Piwik)


I've also realized this is a new readme file. Even though this is specific to the tool_matomo I think this should be a section under the main readme purely to make it easier to find when casually browsing github so you don't have to go dig. We could just cross link the two readmes but this chunk of help is small enough that we can just embed it directly.

In theory if we added this same support to GA and other tools they could all be inline in the main readme

Have moved this to the parent plugin README

bwalkerl

Mentioning the issues I've noticed. We'll work on getting these sorted.

bwalkerl · 2023-11-27T23:35:39Z

tool/matomo/db/install.xml

+        <TABLE NAME="watool_matomo_api_error_log" COMMENT="Matomo Track log errors">
+            <FIELDS>
+                <FIELD NAME="id" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="true"/>
+                <FIELD NAME="endpoint" TYPE="text" LENGTH="255" NOTNULL="false" SEQUENCE="false" COMMENT="The name of the endpoint function called during the sync attempt"/>
+                <FIELD NAME="error" TYPE="text" LENGTH="255" NOTNULL="false" SEQUENCE="false" COMMENT="The string given as the error"/>
+                <FIELD NAME="timecreated" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false"/>
+            </FIELDS>
+            <KEYS>
+                <KEY NAME="primary" TYPE="primary" FIELDS="id"/>
+            </KEYS>
+        </TABLE>


This table is missing an upgrade step

Table name is needlessly long and could cause issues

agree we probably just don't need the table

bwalkerl · 2023-11-27T23:38:45Z

tool/matomo/classes/client.php

+        $request['method'] = 'SitesManager.addSite';
+        $request['siteName'] = !empty($sitename) ? $sitename : $SITE->fullname;
+        $request['timezone'] = !empty($timezone) ? $timezone : core_date::get_server_timezone();
+        $request['currency'] = !empty($currency) ? $currency : 'GBP';


Fallback currency should be more global

bwalkerl · 2023-11-27T23:41:41Z

tool/matomo/classes/client.php

+        if (!empty($url['scheme'])) {
+            return $this->config->siteurl;
+        }
+        return "https://{$url['path']}";


This doesn't look like it would form a proper url

bwalkerl · 2023-11-27T23:43:38Z

tool/matomo/classes/tool/tool.php

-            'id' => 'id',
-            'username' => 'username',
+                'id' => 'id',
+                'username' => 'username',


Indenting changed to being incorrect

bwalkerl · 2023-11-27T23:48:30Z

tool/matomo/classes/tool/tool.php

+     * Otherwise, create a new instance over the API.
+     *
+     * @param $client
+     * @return int siteid if successful, 0 if not.


Would recommend clarifying what is returned for already registered sites. This appears to be siteid, so would be "if successful or already registered"

bwalkerl · 2023-11-27T23:51:48Z

tool/matomo/settings.php

+    $settings->add(
+            new admin_setting_heading(
+                    'watool_matomo_autoprovision',
+                    get_string('autoprovision_heading', 'watool_matomo'),
+                    ''
+            )
+    );
+    $settings->add(
+            new admin_setting_configtext(
+                    'watool_matomo_siteurl',
+                    get_string('apiurl', 'watool_matomo'),
+                    get_string('apiurl_help', 'watool_matomo'),
+                    $CFG->forced_plugin_settings['watool_matomo']['url'] ?? '',
+                    PARAM_URL
+            )
+    );
+    $settings->add(
+            new admin_setting_configpasswordunmask(
+                    'watool_matomo_token',
+                    get_string('apitoken', 'watool_matomo'),
+                    get_string('apitoken_desc', 'watool_matomo'),
+                    $CFG->forced_plugin_settings['watool_matomo']['token'] ?? '',
+            )


bwalkerl · 2023-11-27T23:59:10Z

settings.php

+    $category = new admin_category(
+            'tool_webanalytics',
+            new lang_string('pluginname', 'tool_webanalytics'),
+            false
+    );
+    $ADMIN->add('tools', $category);
+
    $externalpage = new admin_externalpage(
-        'tool_webanalytics_manage',
-        get_string('pluginname', 'tool_webanalytics'),
-        new moodle_url('/admin/tool/webanalytics/manage.php')
+            'tool_webanalytics_manage',
+            get_string('pluginname', 'tool_webanalytics'),
+            new moodle_url('/admin/tool/webanalytics/manage.php')


bwalkerl · 2023-11-27T23:59:36Z

tool/matomo/classes/client.php

+use curl;
+use Exception;
+use moodle_url;


curl and moodle_url aren't used

bwalkerl · 2023-11-28T00:06:42Z

classes/injector.php

+use tool_webanalytics\tool\tool_interface;
+


Is this needed?

brendanheywood · 2023-11-28T03:26:47Z

tool/matomo/classes/tool/tool.php

        $settings = [];
-        $settings['siteid']  = isset($data->siteid) ? $data->siteid : '';
+        $settings['siteid'] = isset($data->siteid) ? $data->siteid : '';


this is a bit repetitive to me, can this be a $defaults array and then cast the stdClass to an array and merge them and skip all the isset bits?

bwalkerl

I found a couple of extra issues after more testing

bwalkerl · 2023-11-29T00:24:15Z

tool/matomo/classes/tool/tool.php

+        foreach ($records as $record) {
+            $settings = $record->get_property('settings');
+            $name = $record->get_property('name');
+            if ((!empty($settings['wwwroot']) && $settings['wwwroot'] === $CFG->wwwroot) || $name === 'auto-provisioned:FAILED') {


If $settings['wwwroot'] doesn't exist (which will happen for existing records since this is a new setting) auto provision is allowed for sites that may already have the proper URL set. Right now this will lead to creating a duplicate record due to other mentioned issues.

bwalkerl · 2023-11-29T00:35:17Z

tool/matomo/classes/client.php

+        $request['siteName'] = !empty($sitename) ? $sitename : $SITE->fullname;
+        $request['timezone'] = !empty($timezone) ? $timezone : core_date::get_server_timezone();
+        $request['currency'] = !empty($currency) ? $currency : 'GBP';
+        $request = array_merge($request, $this->build_urls_for_request($urls));


If the build_urls_for_request function isn't provided with the existing urls (none are passed here), it will override all existing sites with just the updated site. This is problematic for registrations that are connected to multiple site urls. This is used in both registrations and auto provisioning.

bwalkerl · 2023-11-29T00:57:57Z

tool/matomo/classes/tool/tool.php

+        $allrecords = $rm->get_all();
+        $autoprovisioned = array_filter($allrecords, function($record) {
+            return preg_match("/^auto-provisioned:/", $record->get_property('name'));
+        });
+
+        if ($autoprovisioned = reset($autoprovisioned)) {
+            $apsettings = $autoprovisioned->get_property('settings');
+            $data = $autoprovisioned->export();
+        } else {
+            $data = new stdClass();
+            $data->name = 'auto-provisioned:' . uniqid();
+        }
+
+        $hasdnschanged = !empty($apsettings['wwwroot']) && $apsettings['wwwroot'] !== $CFG->wwwroot;
+
+        try {
+            // If the DNS has changed, let's try to update an existing site.
+            if ($hasdnschanged && $siteid = $client->get_siteid_from_url($apsettings['wwwroot'])) {
+                $client->update_site($siteid, '', [$CFG->wwwroot]);
+            } else {
+                // No site provisioned yet, create a new one.
+                $siteid = $client->add_site();
+            }


This is hardcoded to 'auto-provisioned', but if there's no match it goes straight to adding a site, which will create duplicates for sites that have a manual link or have changed the name. Needs some extra checks.

bwalkerl · 2023-11-29T01:09:34Z

tool/matomo/settings.php

+                    $CFG->forced_plugin_settings['watool_matomo']['url'] ?? '',
+                    PARAM_URL
+            )
+    );
+    $settings->add(
+            new admin_setting_configpasswordunmask(
+                    'watool_matomo_token',
+                    get_string('apitoken', 'watool_matomo'),
+                    get_string('apitoken_desc', 'watool_matomo'),
+                    $CFG->forced_plugin_settings['watool_matomo']['token'] ?? '',


The defaults don't match the names of the settings that are used elsewhere or in the readme, and the token setting name doesn't match either. It may also be better to not use these as the default so the token can be kept hidden?

bwalkerl · 2023-11-29T01:20:38Z

classes/client_base.php

+        $this->config = $config;
+        $this->settings = $settings;


Properties for these haven't been declared.