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

(dev/mail#81) Flexmailer - Track click-throughs for URLs with tokens #19386

Merged
merged 3 commits into from
Jan 21, 2021

Conversation

totten
Copy link
Member

@totten totten commented Jan 14, 2021

Overview

When delivering a mail-blast, Flexmailer replaces most URLs with trackable URLs. This expands tracking support for URLs that include tokens.

This is a rebased+modified version of @artfulrobot's #19136 (also civicrm/org.civicrm.flexmailer#46) which simplifies the class design. It addresses https://lab.civicrm.org/dev/mail/-/issues/81

Before

Suppose you send a mailing with content like this:

<!-- Email template -->
<a href="http://example.org/">Example Home</a>
<a href="http://example.org/civicrm/profile/edit?gid=100&id={contact.id}&{contact.checksum}">Update Profile</a>

The first link (which is a simple/static URL) will be replaced with tracking details (ie http://example.org/ <=> u=9000). In the second link, the tokens ({contact.id}) are replaced, but the tracking code is not provided.

<!-- Email output -->
<a href="http://example.org/civicrm/mailing/url?u=9000">Example Home</a>
<a href="http://example.org/civicrm/profile/edit?gid=100&id=200&cs=abcd">Update Profile</a>

After

Both links have a tracking code.

<!-- Email output -->
<a href="http://example.org/civicrm/mailing/url?u=9000">Example Home</a>
<a href="http://example.org/civicrm/mailing/url?u=9001&id=200&cs=abcd">Update Profile</a>

For the second link, note that the URL parameters are split in two parts, the static and dynamic parts. The static part is stored in the tracking record (ie http://example.org/civicrm/profile/edit?gid=100 <=> u=9001), and it will appear in the reporting. The dynamic part is appended (and does not appear in the reporting).

This relies on an existing behavior where the civicrm/mailing/url end-point will relay unrecognized parameters (e.g &id and &cs; per CRM_Mailing_Page_Url::extractPassthroughParameters()).

Comments

The older BAO mailer still behaves as before. This patch only impacts Flexmailer, which is (a) used for Mosaico and (b) optionally used for traditional CiviMail (depending on options in "Administer => CiviMail => Flexmailer").

To see that this impacts Flexmailer and not BAO mailer, consider the mailng-system-tests (CRM_Mailing_MailingSystemTest and Civi\FlexMailer\FlexMailerSystemTest which extend BaseMailingSystemTest). The tests include a scenario for URLs with tokens. The scenario remains unchanged for CRM_Mailing_MailingSystemTest; but for FlexMailerSystemTest, the scenario had to be changed.

(Note: I took a quick look at updating BAO mailer+test, but the underlying codepaths in the BAO side are fairly different and it didn't seem worth the effort.)

totten and others added 3 commits January 14, 2021 02:32
In the prior commit, it needed to conceptually change the behavior of
`getTrackerURL($oldURL): $trackedURL`, but it danced around it (adding
more classes and similar-sounding-methods, but distributed elsewhere).

This re-consolidates to keep a cleaner separation of responsibilities
between `{Html,Text}ClickTracker.php` (*parsing an HTML or text message for URLS*)
and `TrackableURL.php` (*translating the URL*).
@civibot
Copy link

civibot bot commented Jan 14, 2021

(Standard links)

@eileenmcnaughton
Copy link
Contributor

@totten @artfulrobot please merge when you are happy with this

@totten totten changed the title Enable tracking of urls with tokens in Flexmailer (dev/mail#81) Flexmailer - Track click-throughs for URLs with tokens Jan 21, 2021
@totten
Copy link
Member Author

totten commented Jan 21, 2021

I've filled in the description.

Since @artfulrobot gave supportive comments in the other issue discussion, and since there's a lot of relevant test-coverage, I'm flagging 'merge ready'.

@totten totten added the merge ready PR will be merged after a few days if there are no objections label Jan 21, 2021
@artfulrobot
Copy link
Contributor

@totten I've not r-runed your changed code, but I guess if the tests all pass then it should be ok. Thanks for cleaning it up.

(@eileenmcnaughton I don't have merge powers)

@mattwire
Copy link
Contributor

Merging based on @artfulrobot @totten comments

@mattwire mattwire merged commit 7049cd0 into civicrm:master Jan 21, 2021
@totten totten deleted the master-track-token branch January 21, 2021 21:18
@artfulrobot
Copy link
Contributor

Just for the record: Documentation PR at https://lab.civicrm.org/documentation/docs/user-en/-/merge_requests/469

@eileenmcnaughton
Copy link
Contributor

See https://lab.civicrm.org/dev/core/-/issues/2511 for possible related regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants