Fix #141 and add custom event trigger binding support #193

SilverStrings024 · 2021-07-12T14:38:33Z

This should work to make Matomo respect consent.
I created two new utility functions, one builds paq commands, the other builds javascript to bind an event listener to elements with a certain class name. That will allow users to make their own custom template tags then use analytical.utils.get_event_bind_js() to bind events to their elements.

Added tracking support

Added paq builder function and function to build javascript for binding events

Added missing new end line

SilverStrings024 · 2021-07-12T17:26:21Z

@bittner would you mind looking over this PR? It's pretty simple but can expose some extra options for the users of the package

analytical/utils.py

Update `utils.build_paq_cmd()` to convert all elements in the args list to their javascript counter-parts. This will recursively convert the keys and values of a dictionary until there are no more nested dictionaries and will also convert all the elements of a list as well. Also made it so things like booleans, lists, and dictionaries are not surrounded in quotation marks so they're treated like a normal javascript object. If that behavior needs changed. Please let me know

Updated to use the tested and better documented version (Is not tested with Matomo or Django. Only with python)

analytical/templatetags/matomo.py

SilverStrings024

This is kind of important considering it can possibly break the code.

analytical/templatetags/matomo.py

Modified utils.build_paq_cmd to remove the trailing comma and white space as well as add a semi colon to the end of the push command. Added context_providers directory with matomo.py file with a context provider that builds the tracking consent code and adds it to the context. Modified matomo.MatomoNode.render. Removed original ugly settings check code with simply rendering the 'consent_script' context variable and adding it to the html variable only if its length is greater than 1. NOTE: The context provider and rendering it (line 110 templatetags/matomo.py) are untested. Both utils.build_paq_cmd and utils.get_event_bind_js are tested but need need more rigorous testing to be sure it won't break.

Renamed context provider to 'consent_provider' to make more sense when adding it to your context_provider

SilverStrings024 · 2021-07-13T04:44:06Z

@bittner This seems ready, would you mind looking it over?

bittner · 2021-07-13T04:44:51Z

AFAICS, you're making unrelated changes to analytical/utils.py. Would you mind moving that to a separate PR?
Tests are missing for your changes. Please supply them with your feature enhancement.
Please squash your changes. If the change set is small enough the PR should have only one or two commits (e.g. one for the tests, one for the feature enhancement).

Removed a random note I forgot to take out

SilverStrings024 · 2021-07-13T05:48:24Z

* [ ]   AFAICS, you're making unrelated changes to `analytical/utils.py`. Would you mind moving that to a separate PR?

* [ ]   Tests are missing for your changes. Please supply them with your feature enhancement.

* [ ]   Please squash your changes. If the change set is small enough the PR should have only one or two commits (e.g. one for the tests, one for the feature enhancement).

I actuallly don't see that; I thought not having hard coded chunks of JS would be best. I figured dynamically creating the javascript for event binding could let people trigger custom events as well as keep the files relatively free of chunks of hard coded javascript. So, it just made sense to add them in and since I already had the code it made no sense to hard code the JS when what I already wrote makes doing so completely obsolete (for event binding and paq commands anyway).

It may take me a minute to figure out how to properly write tests for this. I'll dig in on my next break from my current project.

Squashing the changes means using git rebase right? I've never done this before so, sorry for the inexperience.

When it comes to splitting up the PRs, from what I understand and, forgive me if I'm misunderstanding, I need to rebase all of the changes out, take out the functions I added in analytics/utils.py then just hardcode the javascript into the context provider? Then on a separate PR introduce the two utility functions again?
Wouldn't that be redundant considering it would go from dynamic to harcoded then back and we'd have to change the files back to their original state after the initial push.

bittner

I left a few style comments.

More than that, there are no tests for your code. Please add some!

bittner · 2021-11-27T18:17:44Z

analytical/context_providers/matomo.py

+    # Do we require consent?
+    if getattr(settings, 'MATOMO_REQUIRE_CONSENT', False):


How about using a self-explanatory variable instead of a comment? e.g.

Suggested change

# Do we require consent?

if getattr(settings, 'MATOMO_REQUIRE_CONSENT', False):

consent_required = getattr(settings, 'MATOMO_REQUIRE_CONSENT', False)

if consent_required:

bittner · 2021-11-27T18:22:37Z

analytical/context_providers/matomo.py

+        provide_script = True
+        if request.user.is_authenticated and not getattr(settings, "ALWAYS_TRACK_REGISTERED", True):
+            provide_script = False


This could be make more readable by using another self-explanatory variable, e.g.

Suggested change

provide_script = True

if request.user.is_authenticated and not getattr(settings, "ALWAYS_TRACK_REGISTERED", True):

provide_script = False

always_track = getattr(settings, "ALWAYS_TRACK_REGISTERED", True)

provide_script = not request.user.is_authenticated or always_track

bittner · 2021-11-27T18:26:11Z

analytical/context_providers/matomo.py

+        if provide_script:
+            grant_class_name = getattr(settings, 'GRANT_CONSENT_TAG_CLASSNAME')
+            revoke_class_name = getattr(settings, 'REVOKE_CONSENT_CLASSNAME')
+            return {"consent_script":"""


Suggested change

return {"consent_script":"""

return {"consent_script": """

Flake8 will complain otherwise, I believe.

bittner · 2021-11-27T18:28:54Z

analytical/templatetags/matomo.py

 from analytical.utils import (
    disable_html,
    get_identity,
    get_required_setting,
-    is_internal_ip,
+    is_internal_ip


I would suggest to leave the trailing comma here. That makes subsequent git diffs more obvious.

bittner · 2021-11-27T18:31:37Z

analytical/templatetags/matomo.py

@@ -109,8 +106,14 @@ def render(self, context):
            'variables': '\n  '.join(variables_code),
            'commands': '\n  '.join(commands)
        }
+        # Force the consent script to render so we can inject it into the template
+        consent_script = Template("{{consent_script}}").render(context)
+        if len(consent_script) > 1:


Suggested change

if len(consent_script) > 1:

if consent_script:

... would be more pythonic.

In this case, though, the consent_script really needs to have a zero-length or no value ("" or None). I guess that's the case.

bittner · 2021-11-27T18:48:03Z

analytical/utils.py

+
+        return arg
+
+    paq = "_paq.push(['%s'" % (cmd)


Round braces are not required with a single argument:

Suggested change

paq = "_paq.push(['%s'" % (cmd)

paq = "_paq.push(['%s'" % cmd

bittner · 2021-11-27T18:49:32Z

analytical/utils.py

+    paq = "_paq.push(['%s'" % (cmd)
+    if len(args) > 0:
+        paq += ", "
+        for arg_idx in range(len(args)):


Please make this pythonic, e.g.

Suggested change

for arg_idx in range(len(args)):

for item in args:

bittner · 2021-11-27T18:53:24Z

analytical/utils.py

+                if no_quotes:
+                    segment = "%s]);" % (current_arg)
+                else:
+                    segment = "'%s']);" % (current_arg)
+            else:
+                if no_quotes:
+                    segment = "%s, "% (current_arg)
+                else:
+                    segment = "'%s', " % (current_arg)


Why don't you simply add the quotes around the current_arg value before the entire block? Then none of those if clauses are needed. Example:

current_arg = current_arg if no_quotes else "'%s'" % current_arg

bittner · 2021-11-27T19:01:46Z

analytical/utils.py

+        for arg_idx in range(len(args)):
+            current_arg = __to_js_arg(args[arg_idx])
+            no_quotes = type(current_arg) in [bool, int, dict, list]
+            if arg_idx == len(args)-1:


This is – and the else black below – is not an elegant solution. Why don't you simply always add the ", " and replace it by "]);" after the end of the loop? e.g.

paq = paq[:-3)] + "]);" return paq

bittner · 2021-11-27T19:03:23Z

analytical/utils.py

+def get_event_bind_js(
+    class_name, matomo_event,
+    matomo_args=[], js_event="onclick",
+    ):


Suggested change

def get_event_bind_js(

class_name, matomo_event,

matomo_args=[], js_event="onclick",

):

def get_event_bind_js(

class_name, matomo_event, matomo_args=[], js_event="onclick"

):

bittner · 2021-11-27T19:14:31Z

It may take me a minute to figure out how to properly write tests for this.

Take a look at the tests that are there. The idea is simple:

Your function generates output for some specific input. Hence, in your test code, call your function with some arguments.
After that, in the line afterwards (in the same test function) verify the result using assert.

That's it.

If there are various possible outcomes for different input, add more of those calls with different input, so that all of your if-else clauses are being traveled through. – Our test suite if full of examples. Take a look!

SilverStrings024 added 3 commits July 12, 2021 10:31

Adding tracking support

40493fa

Added tracking support

Added utility functions

0bc55d1

Added paq builder function and function to build javascript for binding events

Add files via upload

3761e73

Added missing new end line

SilverStrings024 mentioned this pull request Jul 12, 2021

Add support for Matomo user commands #191

Closed

SilverStrings024 commented Jul 12, 2021

View reviewed changes

analytical/utils.py Outdated Show resolved Hide resolved

SilverStrings024 added 2 commits July 12, 2021 18:03

Update utils.py

3a51444

Updated to use the tested and better documented version (Is not tested with Matomo or Django. Only with python)

SilverStrings024 commented Jul 12, 2021

View reviewed changes

analytical/templatetags/matomo.py Outdated Show resolved Hide resolved

SilverStrings024 commented Jul 13, 2021

View reviewed changes

analytical/templatetags/matomo.py Outdated Show resolved Hide resolved

SilverStrings024 added 2 commits July 13, 2021 00:35

Added blank like at the end of context_providers/matomo.py for Flake8

2d22868

Renamed context provider to 'consent_provider' to make more sense when adding it to your context_provider

Update matomo.py

b4611da

Removed a random note I forgot to take out

SilverStrings024 mentioned this pull request Jul 13, 2021

Respecting Consent ( Possible fix for #141 ) #192

Closed

bittner requested changes Nov 27, 2021

View reviewed changes

SilverStrings024 closed this by deleting the head repository Oct 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #141 and add custom event trigger binding support #193

Fix #141 and add custom event trigger binding support #193

SilverStrings024 commented Jul 12, 2021

SilverStrings024 commented Jul 12, 2021

SilverStrings024 left a comment •

edited

Loading

SilverStrings024 commented Jul 13, 2021

bittner commented Jul 13, 2021

SilverStrings024 commented Jul 13, 2021 •

edited

Loading

bittner left a comment

bittner Nov 27, 2021

bittner Nov 27, 2021

bittner Nov 27, 2021

bittner Nov 27, 2021

bittner Nov 27, 2021

bittner Nov 27, 2021

bittner Nov 27, 2021

bittner Nov 27, 2021

bittner Nov 27, 2021

bittner Nov 27, 2021

bittner commented Nov 27, 2021

		# Do we require consent?
		if getattr(settings, 'MATOMO_REQUIRE_CONSENT', False):

	paq = "_paq.push(['%s'" % (cmd)
	paq = "_paq.push(['%s'" % cmd

Fix #141 and add custom event trigger binding support #193

Fix #141 and add custom event trigger binding support #193

Conversation

SilverStrings024 commented Jul 12, 2021

SilverStrings024 commented Jul 12, 2021

SilverStrings024 left a comment • edited Loading

Choose a reason for hiding this comment

SilverStrings024 commented Jul 13, 2021

bittner commented Jul 13, 2021

SilverStrings024 commented Jul 13, 2021 • edited Loading

bittner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bittner commented Nov 27, 2021

SilverStrings024 left a comment •

edited

Loading

SilverStrings024 commented Jul 13, 2021 •

edited

Loading