Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions analytical/context_providers/matomo.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import utils
from django.conf import settings

def consent_provider(request):
"""
Add Mamoto consent script to the requests context.
"""
# Do we require consent?
if getattr(settings, 'MATOMO_REQUIRE_CONSENT', False):
Comment on lines +8 to +9
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

provide_script = True
if request.user.is_authenticated and not getattr(settings, "ALWAYS_TRACK_REGISTERED", True):
provide_script = False
Comment on lines +10 to +12
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return {"consent_script":"""
return {"consent_script": """

Flake8 will complain otherwise, I believe.

%s;
%s
%s
""" % (
utils.build_paq_cmd('requireConsent'),
utils.get_event_bind_js(
class_name=grant_class_name,
matomo_event="rememberConsentGiven",
),
utils.get_event_bind_js(
class_name=revoke_class_name,
matomo_event="forgetConsentGiven",
)
)}
return {'consent_script': ""}
13 changes: 8 additions & 5 deletions analytical/templatetags/matomo.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,12 @@
from itertools import chain

from django.conf import settings
from django.template import Library, Node, TemplateSyntaxError

from django.template import Library, Node, TemplateSyntaxError, Template
from analytical.utils import (
disable_html,
get_identity,
get_required_setting,
is_internal_ip,
is_internal_ip
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

# domain name (characters separated by a dot), optional port, optional URI path, no slash
Expand Down Expand Up @@ -48,10 +47,8 @@

MatomoVar = namedtuple('MatomoVar', ('index', 'name', 'value', 'scope'))


register = Library()


@register.tag
def matomo(parser, token):
"""
Expand Down Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

html += consent_script

if is_internal_ip(context, 'MATOMO'):
html = disable_html(html, 'Matomo')

return html


Expand Down
89 changes: 88 additions & 1 deletion analytical/utils.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"""
Utility function for django-analytical.
"""

from copy import deepcopy
from django.conf import settings
from django.core.exceptions import ImproperlyConfigured

Expand Down Expand Up @@ -163,3 +163,90 @@ class AnalyticalException(Exception):
be silenced in templates.
"""
silent_variable_failure = True

def build_paq_cmd(cmd, args=[]):
"""
:Args:
- cmd: The command to be pushed to paq (i.e enableHeartbeatTimer or contentInteraction)
- args: Arguments to be added to the paq command. This is mainly
used when building commands to be used on manual event trigger.

:Returns:
A complete '_paq.push([])' command in string form
"""
def __to_js_arg(arg):
"""
Turn 'arg' into its javascript counter-part
:Args:
- arg: the argument that's to be passed to the array in _paq.push()
:Return:
The javascript counter-part to the argument that was passed
"""
if isinstance(arg, dict):
arg_cpy = deepcopy(arg)
for k, v in arg_cpy.items():
arg.pop(k)
arg[__to_js_arg(k)] = __to_js_arg(v)
return arg
elif isinstance(arg, bool):
if arg:
arg = "true"
else:
arg = "false"
Comment on lines +192 to +195
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the type on an object label is something that is usually avoided. 🤔

But if you do it, at least, make it concise. 🤓

Suggested change
if arg:
arg = "true"
else:
arg = "false"
arg = "true" if arg else "false"

elif isinstance(arg, list):
for elem_idx in range(len(arg)):
arg[elem_idx] = __to_js_arg(arg[elem_idx])
Comment on lines +197 to +198
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A list comprehension would be the pythonic way to code this, e.g.

Suggested change
for elem_idx in range(len(arg)):
arg[elem_idx] = __to_js_arg(arg[elem_idx])
arg = [__to_js_arg(item) for item in arg]


return arg

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round braces are not required with a single argument:

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

if len(args) > 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if len(args) > 0:
if args:

... is pythonic.

paq += ", "
for arg_idx in range(len(args)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this pythonic, e.g.

Suggested change
for arg_idx in range(len(args)):
for item in 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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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)
Comment on lines +209 to +217
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

paq += segment
else:
paq += "]);"
return paq

def get_event_bind_js(
class_name, matomo_event,
matomo_args=[], js_event="onclick",
):
Comment on lines +223 to +226
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
):

"""
Build a javascript command to bind an onClick event to some
element whose handler pushes something to _paq
:Args:
- class_name: Value of the 'class' attribute of the tag
the event is to be bound to.
- matomo_event: The matomo event to be pushed to _paq
such as enableHeartbeatTimer or contentInteraction
- matomo_args: The arguments to be passed with the matomo event
meaning
:Return:
A string of javascript that loops the elements found by
document.getElementByClassName and binds the motomo event
to each element that was found
"""
script = f"""
var elems = document.getElementByClassName('%s');
for (var i=0; i++; i < elems.length){{
elems[i].addEventListener('%s',
function(){{
%s;
}}
);
}}
""" % (class_name, js_event, build_paq_cmd(matomo_event, matomo_args))
return script