do not merge - this is a concept - feat: acquisition checkpoint #216
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 1.x #216 +/- ##
==========================================
+ Coverage 36.90% 46.22% +9.32%
==========================================
Files 1 1
Lines 271 318 +47
==========================================
+ Hits 100 147 +47
Misses 171 171 ☔ View full report in Codecov by Sentry. |
| const toBinary = (s) => Array.from(s, (c) => parseInt(c, 16).toString(2).padStart(4, '0')).join(''); | ||
| const moduli = [239, 241, 251]; // prime numbers smaller than 256 | ||
| const knownVendors = toBinary('fbdef75ff9f4dedbfdeaba8f21e7884aebf67cfde6eefeea3b8ff32c6fb68a40'); // known vendors bloom filter | ||
| const categories = { |
There was a problem hiding this comment.
You could probably simplify even further with a regex approach.
| const moduli = [239, 241, 251]; // prime numbers smaller than 256 | ||
| const knownVendors = toBinary('fbdef75ff9f4dedbfdeaba8f21e7884aebf67cfde6eefeea3b8ff32c6fb68a40'); // known vendors bloom filter | ||
| const categories = { | ||
| affiliate: ['aff', 'affiliate', 'affiliatemarketing'], |
There was a problem hiding this comment.
Since you check for string inclusion… the 1st actually covers the other 2
| affiliate: ['aff', 'affiliate', 'affiliatemarketing'], | ||
| audio: ['spotify'], | ||
| brand: ['brand'], | ||
| display: ['advertorial', 'banner', 'cpa', 'cpc', 'cpm', 'cpv', 'discover', 'display', 'fbads', 'goppc', 'highimpact', 'inred', 'nps', 'paid', 'paiddisplay', 'placement', 'post', 'poster', 'pp', 'ppc'], |
There was a problem hiding this comment.
All the cp* and pp* could be combined as a simple regex, and paid already covers paiddisplay (also covered by display), post covers poster, etc.
| audio: ['spotify'], | ||
| brand: ['brand'], | ||
| display: ['advertorial', 'banner', 'cpa', 'cpc', 'cpm', 'cpv', 'discover', 'display', 'fbads', 'goppc', 'highimpact', 'inred', 'nps', 'paid', 'paiddisplay', 'placement', 'post', 'poster', 'pp', 'ppc'], | ||
| email: ['em', 'email', 'mail', 'newsletter'], |
There was a problem hiding this comment.
email is already covered by em and mail
| email: ['em', 'email', 'mail', 'newsletter'], | ||
| local: ['yext'], | ||
| owned: ['owned'], | ||
| qr: ['qr', 'qrcode'], |
| web: ['webapp'], | ||
| }; | ||
| const sources = { | ||
| paid: ['affiliate', 'audio', 'display', 'local', 'search', 'social', 'video'], |
There was a problem hiding this comment.
Shopping is a source I see often that I miss here
There was a problem hiding this comment.
I don't see enough values to justify this. The most common I see is igshopping with 4 hosts, but that would be instagram.
There was a problem hiding this comment.
@trieloff Shopping is not an actual value you find necessarily in the params, it's a category you can derive from the hostname/vendor, same as social in a sense or search. Shopping is usually used for e-commerce sites. So think big websites like Amazon/Etsy/Alibaba/Rakuten/…, or anything based on Shopify/Magento/Squarespace/…
So I'd kinda expect in categories above, something like:
shopping: ['alibaba', 'amazon', 'bestbuy', 'ebay', 'flipkart', 'otto', 'rakuten', 'target', 'walmart', …],We can probably also throw in a few European ones in there, like fnac, zalando, etc. would need to see what big hits we get in RUM
| }; | ||
| const sources = { | ||
| paid: ['affiliate', 'audio', 'display', 'local', 'search', 'social', 'video'], | ||
| owned: ['brand', 'email', 'owned', 'qr', 'sms', 'web'], |
There was a problem hiding this comment.
push notifications could be added here as well
There was a problem hiding this comment.
Try to run the same tests that adobe/helix-rum-collector#347 has, so that we can identify the differences in approach.
| const categories = { | ||
| affiliate: ['aff', 'affiliate', 'affiliatemarketing'], | ||
| audio: ['spotify'], | ||
| brand: ['brand'], |
There was a problem hiding this comment.
Brand advertising is not an ad format, is is a type of ad. No point listing it here.
| const knownVendors = toBinary('fbdef75ff9f4dedbfdeaba8f21e7884aebf67cfde6eefeea3b8ff32c6fb68a40'); // known vendors bloom filter | ||
| const categories = { | ||
| affiliate: ['aff', 'affiliate', 'affiliatemarketing'], | ||
| audio: ['spotify'], |
There was a problem hiding this comment.
The things that people click on in Spotify are display ads. You can't click an audio clip.
| display: ['advertorial', 'banner', 'cpa', 'cpc', 'cpm', 'cpv', 'discover', 'display', 'fbads', 'goppc', 'highimpact', 'inred', 'nps', 'paid', 'paiddisplay', 'placement', 'post', 'poster', 'pp', 'ppc'], | ||
| email: ['em', 'email', 'mail', 'newsletter'], | ||
| local: ['yext'], | ||
| owned: ['owned'], |
| local: ['yext'], | ||
| owned: ['owned'], | ||
| qr: ['qr', 'qrcode'], | ||
| search: ['direct', 'google', 'googleflights', 'paidsearch', 'paidsearchnb', 'sea', 'sem'], |
There was a problem hiding this comment.
Cue @ramboz comment about sea including search and so on
| web: ['webapp'], | ||
| }; | ||
| const sources = { | ||
| paid: ['affiliate', 'audio', 'display', 'local', 'search', 'social', 'video'], |
There was a problem hiding this comment.
I don't see enough values to justify this. The most common I see is igshopping with 4 hosts, but that would be instagram.
| const utmMedium = sanitize(new URLSearchParams(window.location.search).get('utm_medium')); | ||
| const utmSource = sanitize(new URLSearchParams(window.location.search).get('utm_source')); |
There was a problem hiding this comment.
This assumes that medium and source are used as different values that carry actual information. I don't think this is the case, and I'd just treat them as one.
| ]; | ||
| const utmMedium = sanitize(new URLSearchParams(window.location.search).get('utm_medium')); | ||
| const utmSource = sanitize(new URLSearchParams(window.location.search).get('utm_source')); | ||
| const preVendor = vendorMappings.find(({ regex }) => regex.test(utmSource))?.result || utmSource; |
There was a problem hiding this comment.
The optional chaining limits browser compatibility. I don't think we use it anywhere else in the code base.
|
Also, this should go against |
Please ensure your pull request adheres to the following guidelines:
Related Issues
Thanks for contributing!