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

33Across Analytics Adapter: initial release #10635

Merged
merged 84 commits into from
Nov 13, 2023

Conversation

macinjosh32
Copy link
Contributor

@macinjosh32 macinjosh32 commented Oct 20, 2023

Type of change

  • Bugfix

  • Feature

  • New Analytics adapter

  • Code style update (formatting, local variables)

  • Refactoring (no functional changes, no api changes)

  • Build related changes

  • CI related changes

  • Does this change affect user-facing APIs or examples documented on http://prebid.org?

  • Other

Description of change

The debut of a new Prebid Analytics Adapter from 33Across.

Other information

Documentation PR: prebid/prebid.github.io#4944

mscottnelson and others added 30 commits May 16, 2023 19:19
- Single responsibility for transaction manager
- Send one report per auction
- Clear pending events that were causing conflicts in specs
…auctionEnd (in the case that auction ends AND not all bids have resolved), and general refactoring
@macinjosh32
Copy link
Contributor Author

I've sync'ed from the latest upstream main branch. Let me know if anything else is needed.

@macinjosh32
Copy link
Contributor Author

@jlquaccia - Can you please review this pull request when you have a chance?

Copy link
Collaborator

@jlquaccia jlquaccia left a comment

Choose a reason for hiding this comment

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

overall lgtm

@macinjosh32
Copy link
Contributor Author

Thanks, @jlquaccia! Feel free to merge at your earliest convenience.

@jlquaccia
Copy link
Collaborator

Hey @macinjosh32, apologies for overlooking this the other day, but looks like there are a few tests that are failing (flagged from the circleci build above). Could you address these? Then this PR should be good to go!

@jlquaccia jlquaccia self-requested a review November 10, 2023 18:38
Copy link
Collaborator

@jlquaccia jlquaccia left a comment

Choose a reason for hiding this comment

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

Need to address some tests that are failing.

@macinjosh32
Copy link
Contributor Author

macinjosh32 commented Nov 10, 2023

@jlquaccia - I found the issue and fixed it, and all tests are passing when I run "npm run test" locally.

However, when I push the fix and Prebid.js's CI runs, the BrowserStack testing fails on older Safari browsers due to a connection issue. I think the connection issue is unrelated to this pull request because CI is also failing on prebid:master with a similar error. I wanted to try re-running the CI, but it looks like write access is required for that. Can you please give it a try?

@mscottnelson
Copy link
Contributor

@jlquaccia any updates on this? Let me know if there's anything I can do to help

Copy link
Collaborator

@jlquaccia jlquaccia left a comment

Choose a reason for hiding this comment

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

LGTM

@jlquaccia
Copy link
Collaborator

@macinjosh32 @mscottnelson sorry for the delay, wanted to confirm with another Prebid member that the failed test was something unrelated as suspected above and turns out it is unrelated. Re-ran the tests just now and everything passed. All good to go, will merge!

@jlquaccia jlquaccia merged commit af4936a into prebid:master Nov 13, 2023
4 checks passed
@mscottnelson
Copy link
Contributor

Not a problem, and thank you!

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

Successfully merging this pull request may close these issues.

6 participants