Add auto-provisioning for matomo by SimonasAdams · Pull Request #52 · catalyst/moodle-tool_webanalytics

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.

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

    }

+    /**
+     * 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.

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


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


Are those leftovers?

Correct and now removed.

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

+- 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

@@ -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

+        <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

+        $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

+        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

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


Indenting changed to being incorrect

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

+     * 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

+    $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

+    $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

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


curl and moodle_url aren't used

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

+use tool_webanalytics\tool\tool_interface;
+


Is this needed?

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

+    public function form_build_settings(stdClass $data): array {
        $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

+        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

+        $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

+        $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

+                    $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

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


Properties for these haven't been declared.