-
Notifications
You must be signed in to change notification settings - Fork 22
Enable tracking of urls with tokens (Issue #30) #46
base: master
Are you sure you want to change the base?
Conversation
@artfulrobot Can you update the copyright header to the current civi standard one? Also does this relate to #51 at all? |
@mattwire sure. Interesting question about #fragments. On the one hand it's common to include a link twice and use different fragments, on the other hand there's more and more javascript apps running pages, e.g. civicrm's angularjs urls which do have fragments which could need tracking. Anyway, I believe that this PR does NOT include fragments in the tracked part of the link, and is therefore an alternative to #51 |
27aa8a6
to
3be0772
Compare
Shortened license, rebased on master |
Rebased this to master, added a commit to fix a test that was failing. |
I haven't tried this, but the concept is great, and I love that there are tests. Somewhat related: this relies on the But this is a good use-case for pass-through. I do wonder how far we are from making a safer form of pass-through behavior. The main risk there was ... naming-conflicts between CMS-params and external/target URL params, e.g.
I do wonder how hard it would be to nest the pass-through params, e.g.
I suppose it's not a hard block for this PR -- since the existing But, again, overall, this is a valid use-case for a pass-through mechanism. |
@totten thanks, and good points. There are limitations in this for sure. However I don't think url-encoded passthrough will work since we don't have the token values. e.g. I think we'd need that to be a separate PRs:
Maybe relevant: my use of this patch has been to include the mailing ID as a source param to a form that uses a page on the Civi site. Our site generated donation pages (bespoke, non-civi pages, but served by the same CMS), and we want to know which CiviMail mailing generates donations, so we inject tokens to identify the mailing, but we may also inject contact checksums so users don't have to enter their name and address. So the limitations of this are:
I think these limitations are still worth it. |
This PR does the following:
Notes on the code changes:
Civi\FlexMailer\FlexMailerSystemTest::testUrlTracking
inherits from. This is beacuase that case tests for the problem that this PR solves!