Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

Request: support tracking URLs with tokens in query strings #30

Open
artfulrobot opened this issue Mar 7, 2019 · 9 comments
Open

Request: support tracking URLs with tokens in query strings #30

artfulrobot opened this issue Mar 7, 2019 · 9 comments

Comments

@artfulrobot
Copy link

Example code at https://civicrm.stackexchange.com/a/24420/35

If people do mailings to supporters with a link to a donate form including a checksum to identify the supporter, they lose the click rate metric, which is very important for knowing which campaigns worked better than others.

We need some way to track clicks to those URLs, ideally so that the report can show them grouped together: e.g. 12% people clicked your.org/donate-now

artfulrobot pushed a commit to artfulrobot/org.civicrm.flexmailer that referenced this issue Mar 7, 2019
@artfulrobot
Copy link
Author

artfulrobot commented Mar 7, 2019

My issue/30 branch does this:

  • If there's no token it does nothing different.
  • If there's a token in the domain or path, it does nothing.
  • If there's tokenised parameters in the query, they are extracted, the rest of the URL is translated to a tracked one and the params (and fragment) are appended.
  • If there's tokens in the fragment, the rest of the URL is tracked and the fragment appended.

And more awkwardly,

  • Moved this new code to a base class that Text and Html ones inherit from.
  • Allow dependency injection to assist unit testing.
  • Include a file of unit tests to test the above.
  • Update the test code base from civix. (hmmm. probably breaks other tests!)
  • I have not removed the old url tracking tests, so they fail.
  • There's also an issue with double html entities at the mo, but interested to know your thoughts on the direction anyway.

artfulrobot pushed a commit to artfulrobot/org.civicrm.flexmailer that referenced this issue Sep 17, 2019
fix double encoding
artfulrobot pushed a commit to artfulrobot/org.civicrm.flexmailer that referenced this issue Sep 18, 2019
artfulrobot pushed a commit to artfulrobot/org.civicrm.flexmailer that referenced this issue Sep 18, 2019
artfulrobot pushed a commit to artfulrobot/org.civicrm.flexmailer that referenced this issue Feb 14, 2020
@KarinG
Copy link

KarinG commented Jun 11, 2020

Hey @artfulrobot - one of our clients found this PR! Where is it at - can we help confirm/test?

artfulrobot pushed a commit to artfulrobot/org.civicrm.flexmailer that referenced this issue Jun 11, 2020
@artfulrobot
Copy link
Author

@KarinG I've used it happily for a while. I've just rebased (and added a commit to fix a failing test since the rebase) it onto master. I've got a branch here that includes the changes on top of v1.1.1 in case you don't want to run the master version.

I guess it would help to get some reviews of my PR (phpunit tests are included)?

@KarinG
Copy link

KarinG commented Jun 19, 2020

Thanks @artfulrobot - one of our clients will give it a spin. We will report back!

@artfulrobot
Copy link
Author

@KarinG you still have to be careful but it's fine if you just want to merge in some IDs or something. It's pretty 'stupid' in that it is not aware of things like urlencoding, or vars that the CMS uses (like q for drupal).

@larssandergreen
Copy link

Thanks @artfulrobot! We're using your branch and seems to be working as it should (our use is just personalized links for webforms, contribution pages and events). Will continue testing and report back. Was happy to see it works with anchors in the link.

Really appreciate your work on this as this has been on our wish list for years.

@larssandergreen
Copy link

We've done a number of mailings in the last month and the tracking seems to be working as expected. Thanks again @artfulrobot

@BeccaTregenna
Copy link

Hi @artfulrobot we've come across this only just recently and are gonna switch to your branch. Just wondering if there's anything stopping it getting merged and if so if there's anything we may be able to help with?

@artfulrobot
Copy link
Author

Hi @BeccaTregenna well just see above really for the cautions/limitations. I think there's a few people using it successfully now. I think it just needs enough people to get on side with merging it.

It seems to have me, @KarinG , @larssg-wildsight using it without problems. Perhaps that's enough of a community review to suggest a merge? But feel free to try and then add your name here!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants