-
Notifications
You must be signed in to change notification settings - Fork 15
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
Ad attribution test flow #93
Conversation
6197edf
to
3ab1600
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM module some suggestions.
} | ||
} | ||
] | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could expand these test cases with Ads 9 - 14 in https://app.asana.com/0/72649045549333/1202521973600829/f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's the plan, I just wanted us to merge the current code then I can start on these.
@@ -49,6 +49,76 @@ app.use(json({ | |||
const partitioningRoutes = require('./privacy-protections/storage-partitioning/server/routes'); | |||
app.use('/partitioning', partitioningRoutes); | |||
|
|||
function joinPath () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to move these into the adClickFlow routes.js?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, these rewrite the path to the relevant /adClickFlow path. The /adClickFlow/server/routes.js is applied only to that path with app.use('/adClickFlow', adClickFlowRoutes);
The only other way here is export a function from the server path; I'd like to punt this to a fast follow commit that has a separate approval as we've already been testing this well.
Prefix domains with the relevant subdomains. Co-authored-by: Steven Englehardt <se@senglehardt.com>
- `convert.ad-company.example` - Simulated ad provider conversion ping | ||
- `www.ad-company.example` - Simulated ad provider non-conversion ping | ||
- `www.payment-company.example` - Simulated payment provider | ||
- `www.publisher-company.example`- Simulated publisher website |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that all resolve to
privacy-test-pages.glitch.me
Sorry, just caught that - these were all supposed to be .site
not .example
, right?
Sample test cases for working through testing our platforms click attribution code.