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

Ads team ppa testing (fix #14839) #14850

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

wen-2018
Copy link
Collaborator

@wen-2018 wen-2018 commented Jul 18, 2024

One-line summary

Significant changes and points to review

Issue / Bugzilla link

#14839

Testing

@wen-2018 wen-2018 added the WIP 🚧 Pull request is still work in progress label Jul 18, 2024
@wen-2018 wen-2018 changed the title Ads team ppa testing Ads team ppa testing (fix #14839) Jul 18, 2024
Copy link

codecov bot commented Jul 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.38%. Comparing base (0de8389) to head (dcf1ba1).
Report is 24 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #14850   +/-   ##
=======================================
  Coverage   77.37%   77.38%           
=======================================
  Files         161      161           
  Lines        8302     8305    +3     
=======================================
+ Hits         6424     6427    +3     
  Misses       1878     1878           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@craigcook craigcook left a comment

Choose a reason for hiding this comment

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

This does seem to do what it should, in that the event gets added on click, but I'm not sure how to verify the data is appearing on the other end. We could put this on a demo server for the ads team to test, but I'm not sure who to talk to. Emma would know.


link.addEventListener('click', () => {
navigator.privateAttribution.measureConversion({
task: 'task_id', // Unique task ID. Supplied by Mozilla.
Copy link
Member

Choose a reason for hiding this comment

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

Has this been provided yet? I assume our unique task ID is not actually task_id.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Both task and histogramSize have not been provided yet. I will update the PR once they are available

'use strict';

link.addEventListener('click', () => {
navigator.privateAttribution.measureConversion({
Copy link
Member

Choose a reason for hiding this comment

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

Will this throw an error in older Firefox versions? It might be worth wrapping in a try/catch if so?

@alexgibson alexgibson added the Frontend HTML, CSS, JS... client side stuff label Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Frontend HTML, CSS, JS... client side stuff WIP 🚧 Pull request is still work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants