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

Exclude sailthru links to podcast on iTunes. #16

Merged
merged 1 commit into from
Jun 29, 2017

Conversation

alloy
Copy link
Contributor

@alloy alloy commented Jun 29, 2017

This makes sure URLs like the following are excluded:

$ curl -sI http://link.artsy.net/click/9878256.349300/aHR0cHM6Ly9pdHVuZXMuYXBwbGUuY29tL3VzL3BvZGNhc3QvYXJ0c3kvaWQxMDk2MTk0NTE2P210PTImdXRtX3NvdXJjZT1zYWlsdGhydSZ1dG1fbWVkaXVtPWVtYWlsJnV0bV9jYW1wYWlnbj05ODc4MjU2LTA2LTE5LTE3LUVkaXRvcmlhbCZ1dG1fY29udGVudD1iYW5uZXI/56969dcb487ccd431a8b4568Cd242a50c | grep Location

Location: https://itunes.apple.com/us/podcast/artsy/id1096194516?mt=2&utm_source=sailthru&utm_medium=email&utm_campaign=9878256-06-19-17-Editorial&utm_content=banner&utm_term=Editorial%20Email%20-%20Daily

@@ -33,6 +33,10 @@ function encodePath(path) {
.join("*")
}

function isNotSailthruPattern(pattern) {
return !/^NOT \/click\/\*/.test(pattern)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so we can add manually Sailthru encoded URLs and not have them encoded again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@kanaabe
Copy link
Contributor

kanaabe commented Jun 29, 2017

Wonder if there's a way we can use some other indicator in the link that's easier to apply one-off, like a query param. I guess it'd depend on if we can append query params to links in ST on a one-off basis, but I bet this is possible.

@kanaabe kanaabe merged commit 50e2ef5 into master Jun 29, 2017
@alloy
Copy link
Contributor Author

alloy commented Jun 29, 2017

Ooohh, that’s a great idea 🙌

Will ask @rchlgss.

@rchlgss
Copy link

rchlgss commented Jun 29, 2017

Yes, we can add something to the link, but it would probably need to be structured as utm_term = no-ul or utm_id=no-ul. Would like @sperle to weigh in on this first before we update.

To summarize, we are trying to come up with an easy workaround so that we can exclude links we don't want to open in Eigen (e.g. external links for UBS SmartWealth or the iTunes store links for the Artsy Podcast). The suggestion is to add some kind of link param that will be consistent for exclusion.

@sperle
Copy link

sperle commented Jul 18, 2017

I think that sounds like a great idea. @cfinsness can weigh in on how/where we'd add a param like that under our new utm structures.

@cfinsness
Copy link

I like Rachel's idea of using a consistent perams independent of content type. We can structure sailthru emails to default with eigen_open=1 if we want it to open through the app when available, and eigen_open=0 if we don't want it opened.

@sperle
Copy link

sperle commented Jul 18, 2017

Well, everything opens in the app by default now -- so it'd just be the exclude that's necessary

@alloy
Copy link
Contributor Author

alloy commented Jul 19, 2017

Hmm, I just thought of an issue with this. Seeing as this param would be tacked on to the end of the URL and the way Base64 encodes (groups of characters), it won’t be possible to always use the exact same param and expect to be able to match the encoded URL with the encoded version of just the param.

For example, consider this encoded version of just the param:

Base64.strict_encode64("no-universal-links")
# => "bm8tdW5pdmVyc2FsLWxpbmtz"

If we were to encode a URL that encodes in such a way that the param lies exactly at the boundary of a character group, then the encoded param is found at the end as expected:

Base64.strict_encode64("https://www.artsy.net/artist/banksy?no-universal-links")
# => "aHR0cHM6Ly93d3cuYXJ0c3kubmV0L2FydGlzdC9iYW5rc3k/bm8tdW5pdmVyc2FsLWxpbmtz"

However, if we were to encode a URL where the character grouping does not exactly work out, the encoded param can’t be found:

Base64.strict_encode64("https://www.artsy.net/artwork/mr-brainwash-einstein-painting?no-universal-links")
# => "aHR0cHM6Ly93d3cuYXJ0c3kubmV0L2FydHdvcmsvbXItYnJhaW53YXNoLWVpbnN0ZWluLXBhaW50aW5nP25vLXVuaXZlcnNhbC1saW5rcw=="

We could remedy that by having a tool that generates the right URL for this to work:

def encode(url, param)
  needle = Base64.strict_encode64(param)
  padding = -1
  begin
    padding += 1
    url_with_param = "#{url}?#{"-" * padding}#{param}"
    haystack = Base64.strict_encode64(url_with_param)
  end until haystack.end_with?(needle)
  url_with_param
end

Which generates a URL that uses paddingto ensure the param will get encoded in a way we can match it (note that the padded param is --no-universal-links):

encode("https://www.artsy.net/artwork/mr-brainwash-einstein-painting", "no-universal-links")
# => "https://www.artsy.net/artwork/mr-brainwash-einstein-painting?--no-universal-links"
Base64.strict_encode64("https://www.artsy.net/artwork/mr-brainwash-einstein-painting?--no-universal-links")
# => "aHR0cHM6Ly93d3cuYXJ0c3kubmV0L2FydHdvcmsvbXItYnJhaW53YXNoLWVpbnN0ZWluLXBhaW50aW5nPy0tbm8tdW5pdmVyc2FsLWxpbmtz"

However, having to use such a tool seems like it would be a lot of hassle for the authors of the emails to have to go through.

@sperle these email get authored on Sailthru’s site, I assume? If so, I don’t see a way for this to be done in an automated fashion. I don’t think Sailthru allows post-process scripts before sending off the emails, is that correct?

@alloy
Copy link
Contributor Author

alloy commented Jul 19, 2017

The alternative, assuming that multiple Sailthru link domains is still not a thing, would be to use an alternate artsy.net redirect subdomain, because the start of the URL will always be encoded the same way. E.g.:

Base64.strict_encode64("https://no-universal-links.artsy.net")
# => "aHR0cHM6Ly9uby11bml2ZXJzYWwtbGlua3MuYXJ0c3kubmV0"
Base64.strict_encode64("https://no-universal-links.artsy.net/artwork/mr-brainwash-einstein-painting")
# => "aHR0cHM6Ly9uby11bml2ZXJzYWwtbGlua3MuYXJ0c3kubmV0L2FydHdvcmsvbXItYnJhaW53YXNoLWVpbnN0ZWluLXBhaW50aW5n"
Base64.strict_encode64("https://no-universal-links.artsy.net/artist/banksy")
# => "aHR0cHM6Ly9uby11bml2ZXJzYWwtbGlua3MuYXJ0c3kubmV0L2FydGlzdC9iYW5rc3k="

I assume it’s not that hard to setup a simple app that just redirects from e.g. https://no-universal-links.artsy.net/artist/banksy to https://www.artsy.net/artist/banksy, but it would also need to be able to redirect to external domains, such as an iTunes podcast link.

@kanaabe Thoughts?

@kanaabe
Copy link
Contributor

kanaabe commented Jul 19, 2017

@alloy Great thoughts and points.

There's no way to do a post-process script in ST. @sperle @rchlgss is it out of the question to turn off link rewriting for these links? We can achieve this by doing:

<a href=http://artsy.net/venice-biennale?no-redirect-link>Link</a>
instead of
<a href="http://artsy.net/venice-biennale?no-redirect-link">Link</a>

Of course that means we get no tracking on these links BUT it means we could do the query param solution reliably. Not sure how important that is to our reporting workflow.

Regarding the second point, that's cool! We're getting real hacky now 😄. I don't have as much expertise in the security/external linking side of it, but leveraging the beginning of the URL seems like a sane idea.

@orta orta deleted the exclude-itunes-podcast-links branch July 19, 2017 17:50
@alloy
Copy link
Contributor Author

alloy commented Jul 19, 2017

@sperle @rchlgss is it out of the question to turn off link rewriting for these links? We can achieve this by doing:

<a href=http://artsy.net/venice-biennale?no-redirect-link>Link</a>
instead of
<a href="http://artsy.net/venice-biennale?no-redirect-link">Link</a>

Is that documented behavior?

We're getting real hacky now 😄.

😄 🥇

I don't have as much expertise in the security/external linking side of it, but leveraging the beginning of the URL seems like a sane idea.

Me neither :/

@kanaabe
Copy link
Contributor

kanaabe commented Jul 19, 2017

I can't find it in the documentation but I spoke with an old colleague at ST who reminded me of this trick.

@sperle
Copy link

sperle commented Jul 20, 2017

So, it's not great -- I can think of a couple of cases where we'd definitely need to be able to track the clicks (e.g. the ubs smartwealth promotion) where the link is offsite and the link tracking is the only way we have to verify performance.

I've never done the above in email -- i'd want to do some serious testing to make sure no email programs handle that weirdly.

@kanaabe
Copy link
Contributor

kanaabe commented Jul 20, 2017

@sperle Totally understandable that not having tracking will be a pain with promotions/paid content.

@alloy What if we have a rule to hand-off only if it contains https://www.artsy.net. All of our content on Sailthru uses the https://www.artsy.net URL. This immediately covers the non-artsy links.

Then, for Artsy links, we could ask email peeps to use a non-wwwified version of the URL, like https://artsy.net/venice-biennale which redirects to https://www.artsy.net/venice-biennale/toward-venice later thanks to the www-ify app.

It goes one step further with the hackiness, but it could work in a fraction of the time?

@sperle
Copy link

sperle commented Jul 20, 2017

oh smart, that could totally work for us, right @rchlgss ?

@rchlgss
Copy link

rchlgss commented Jul 20, 2017

Yes, I think that would work! Also, love how simple and smart this solution is @kanaabe :)

@alloy
Copy link
Contributor Author

alloy commented Jul 20, 2017

@kanaabe Nice, good call 👌

@artsyit
Copy link
Contributor

artsyit commented Nov 2, 2021

🚀 PR was released in v1.1.0 🚀

@artsyit artsyit added the released This issue/pull request has been released. label Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants