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

Dev uc event level report #534

Open
wants to merge 18 commits into
base: dev
Choose a base branch
from

Conversation

sinnew
Copy link

@sinnew sinnew commented Jan 23, 2025

Prior to creating a pull request, please follow all the steps in the contributing guide.

Description

add new page dedicated for event level report demo
add trigger registration for add to cart button
add DSP endpoint to report event level only without aggregated report
add document

Related Issue

NA

Affected services

  • Home
  • News
  • Shop
  • Travel
  • DSP
  • SSP
  • ALL

Other:

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

Hi @sinnew, reviewers, and future readers!

I'm Gemini, and I'm currently reviewing this pull request. In the meantime, I'm providing this summary to help everyone quickly understand the changes and intent.

This pull request, "Dev uc event level report," aims to add an event-level reporting demo. Specifically, it introduces:

  • A new page dedicated to demonstrating event-level reporting.
  • Trigger registration for the "Add to Cart" button on the shop demo.
  • A new DSP endpoint to report events individually, without aggregated reports.
  • Updated documentation.

Here's a breakdown of the changes:

File Changes
services/ad-tech/src/lib/arapi.ts Added getTriggerData function to assign trigger data and priority based on conversion type (lines 234-246).
services/ad-tech/src/lib/attribution-reporting-helper.ts Added getEventLevelAttributionTriggerHeaders function to return ARA trigger registration headers for event-level reporting (lines 89-104).
services/ad-tech/src/routes/common/attribution-reporting-router.ts Added a new route /register-event-level-trigger to register an attribution trigger (event) without an aggregated report (lines 98-110).
services/home/docs/demos/index.md Added a link to the new event-level report demo.
services/home/docs/demos/single-touch-event-level-report.md Added a comprehensive documentation page for the event-level report demo.
services/home/docs/intro.md Added a link to the new event-level report demo.
services/news/src/views/components/aside.ejs Added a link to the new event-level report demo in the aside.
services/news/src/views/components/static-img-ad.ejs Added a button to navigate to the shop without an ad click.
services/news/src/views/iframe-static-img-ad.ejs Added a new page for the event level report demo.
services/shop/src/index.ts Added getEventTriggerUrl function to generate the URL for registering event-level triggers.
services/shop/src/views/item.ejs Added addToCart function to register an event-level trigger when adding an item to the cart.

I'll provide a more detailed review shortly. In the meantime, please let me know if you have any questions.

And now, a little haiku to brighten your day:

Code flows like a stream,
Tests pass, a joyful green light,
Bugs flee in the night.

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

This pull request adds a new page for an event-level report demo, trigger registration for an add-to-cart button, a DSP endpoint to report event-level data, and documentation. The implementation looks good overall, but there are a few minor suggestions.

I've summarized the style guide elements I referenced below:

Google JavaScript Style Guide:

  • Function and method naming conventions
  • Type annotations
  • Use of console.log
  • String literals

TypeScript Style Guide:

  • Type casing

services/ad-tech/src/lib/arapi.ts Outdated Show resolved Hide resolved
services/ad-tech/src/lib/arapi.ts Outdated Show resolved Hide resolved
services/ad-tech/src/lib/arapi.ts Outdated Show resolved Hide resolved
sinnew and others added 5 commits January 24, 2025 12:22
Co-authored-by: code-review-assist[bot] <182814678+code-review-assist[bot]@users.noreply.github.com>
Co-authored-by: code-review-assist[bot] <182814678+code-review-assist[bot]@users.noreply.github.com>
Co-authored-by: code-review-assist[bot] <182814678+code-review-assist[bot]@users.noreply.github.com>
@chasinandrew chasinandrew self-requested a review January 24, 2025 20:20
Copy link

@chasinandrew chasinandrew left a comment

Choose a reason for hiding this comment

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

LGTM after all comments are addressed on variable names and parameters

services/ad-tech/src/lib/arapi.ts Outdated Show resolved Hide resolved
services/ad-tech/src/lib/arapi.ts Show resolved Hide resolved
services/home/docs/demos/index.md Show resolved Hide resolved
services/news/src/views/iframe-static-img-ad.ejs Outdated Show resolved Hide resolved
Copy link

@chasinandrew chasinandrew left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants