django-analytical icon indicating copy to clipboard operation
django-analytical copied to clipboard

Fix #141 and add custom event trigger binding support

Open SilverStrings024 opened this issue 4 years ago • 5 comments
trafficstars

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.

SilverStrings024 avatar Jul 12 '21 14:07 SilverStrings024

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

SilverStrings024 avatar Jul 12 '21 17:07 SilverStrings024

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

SilverStrings024 avatar Jul 13 '21 04:07 SilverStrings024

  • [ ] 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).

bittner avatar Jul 13 '21 04:07 bittner

* [ ]   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.

SilverStrings024 avatar Jul 13 '21 05:07 SilverStrings024

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:

  1. Your function generates output for some specific input. Hence, in your test code, call your function with some arguments.
  2. 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!

bittner avatar Nov 27 '21 19:11 bittner