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

Enable tracking of urls with tokens in Flexmailer #19136

Conversation

artfulrobot
Copy link
Contributor

Overview

Migration of civicrm/org.civicrm.flexmailer#46
See https://lab.civicrm.org/dev/mail/-/issues/81

Before

  • Can't track URLs with tokens in CiviMail

After

  • Can track URLs with tokens in CiviMail, if you're careful.

@civibot
Copy link

civibot bot commented Dec 7, 2020

(Standard links)

@civibot civibot bot added the master label Dec 7, 2020
@seamuslee001
Copy link
Contributor

@artfulrobot bunch of style issues mate https://test.civicrm.org/job/CiviCRM-Core-PR/38259/checkstyleResult/new/

@artfulrobot artfulrobot force-pushed the artfulrobot-flexmailer-tracking-urls-with-tokens branch from daf4c67 to 2dc1a48 Compare December 8, 2020 07:07
@artfulrobot artfulrobot force-pushed the artfulrobot-flexmailer-tracking-urls-with-tokens branch from 2dc1a48 to 3edae51 Compare December 9, 2020 10:12
@artfulrobot
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor

@artfulrobot we chatted about this just now & no-one had concerns about this (although r-run not done yet) other than the documentation / pr explanation side of it - it's hard to tell from the review template what is being changed / what the implications - so if you could amp that out a bit and / or create a docs PR we should be able to get someone to review it & there are no obvious blockers to merging (I didn't quite hear @kcristiano offer to review but his murmur was so encouraging I'm gonna pretend Kevin volunteered.....)

@totten
Copy link
Member

totten commented Jan 14, 2021

@artfulrobot So I've done some r-run, and it is working. Cool. :)

From an r-code perspective, the arrangement around BaseClickTracker::$getTrackerURL feels weird:

  • Partially, it sticks out because it's like a parent-method, except written as a static-variable.
  • But thinking about it a bit more deeply... the essence of this change is that getTrackerURL($origURL): $trackedURL needs to behave differently. But it feels like the current revision changes everything else in an attempt to reshape things to avoid changing getTrackerURL().

This probably made sense if one initially set out with a goal: Let's patch flexmailer.git and leave civicrm-core.git untouched. But now that they're in the same repo... perhaps it would cleaner to put the getTrackerURLForUrlWithTokens as a private/internal part of the implementation of getTrackerURL() -- as in #19386.

@artfulrobot
Copy link
Contributor Author

Closing in favour of #19386

(Yes, I had been limiting changes to the extension, not core.)

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

Successfully merging this pull request may close these issues.

4 participants