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.

Sign up for GitHub

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

Jump to bottom

OpenX: Analytics Adapter update #5449

Merged
merged 1 commit into from
Aug 31, 2020
Merged

OpenX: Analytics Adapter update #5449

merged 1 commit into from
Aug 31, 2020

Conversation

jimee02
Copy link
Contributor

@jimee02 jimee02 commented Jul 2, 2020

  • Feature

Description of change

Rebuild of OpenX's Analytics adapter

  • Changed payload schema and initialization options
  • Added schema versioning for backwards compatibility
  • Updated unit tests to reflect changes
  • Updated documentation to reflect changes (openxAnalyticsAdapter.md)

@lgtm-com
Copy link

lgtm-com bot commented Jul 2, 2020

This pull request introduces 1 alert when merging ed9e6b6 into 8c87a9e - view on LGTM.com

new alerts:

  • 1 for Syntax error

@jimee02 jimee02 force-pushed the analytics-adapter-update branch from ed9e6b6 to b5b5b03 Compare July 2, 2020 16:45
jimee02
jimee02 commented Jul 2, 2020
View reviewed changes
modules/openxAnalyticsAdapter.js
}

/**
* @typedef {Object} PbBidRequest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started defining the PB objects for consistency. Let me know if I should rename PBBidRequest and PbBidderRequest` to avoid conflicts.

@stale
Copy link

stale bot commented Jul 18, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 18, 2020
@stale stale bot closed this Jul 25, 2020
@jimee02
Copy link
Contributor Author

jimee02 commented Jul 28, 2020

@jaiminpanchal27, is it possible to reopen this PR?

@jimee02
Copy link
Contributor Author

jimee02 commented Jul 30, 2020

@mike-chowla, can you reopen this ticket?

@bretg bretg reopened this Aug 7, 2020
@stale stale bot removed the stale label Aug 7, 2020
@jsnellbaker jsnellbaker requested review from jsnellbaker and removed request for jaiminpanchal27 August 10, 2020 14:19
@jimee02
Copy link
Contributor Author

jimee02 commented Aug 10, 2020

Hi @jsnellbaker, I know this is a huge change. How do I make this easier for you?

@jsnellbaker
Copy link
Collaborator

Hi @jimee02 I just pulled this PR to me and I plan to start taking a look at it today. So I'm not sure if there's anything particular as of yet. But I will try to follow-up soon (with either feedback, questions, or both).

jsnellbaker
jsnellbaker requested changes Aug 10, 2020
View reviewed changes
Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

Hi @jimee02
I reviewed the changes and I think on the whole everything looks fine. There were some minor suggestions though (the pubads() more recommended of the two), so please take a look when you have the chance.

In terms of testing the analytics adatper, are the sample values in the .md file still accurate to use? Or are there some new values I should use instead?

Thanks.

jsnellbaker
jsnellbaker requested changes Aug 10, 2020
View reviewed changes
Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

I missed this part at first, but on further review, there is another set of items that need to be addressed.

There are a series of references to the find function in the unit tests that need to be updated to use the polyfill version of find so that the tests can run successfully in all browsers.

I highlighted one example below but there are a handful of other places in the tests. Please take a look through the test file and update all the areas needed.

Thanks again.

@jimee02 jimee02 force-pushed the analytics-adapter-update branch from b5b5b03 to ac03d5c Compare August 25, 2020 04:02
@jimee02 jimee02 force-pushed the analytics-adapter-update branch from ac03d5c to 15dea30 Compare August 25, 2020 04:10
@jimee02
Copy link
Contributor Author

jimee02 commented Aug 25, 2020

Hi @jsnellbaker,

I just realized that the analytics documentation is outdated. The configuration options are accurate so I'm going to make the fixes to the readme in another PR because this one is already big enough. Thank you for taking the time to review this!

Much appreciated.

jsnellbaker
jsnellbaker approved these changes Aug 31, 2020
View reviewed changes
Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

@jimee02
Thanks for making the updates. LGTM

@jsnellbaker jsnellbaker merged commit 6c41243 into prebid:master Aug 31, 2020
BrightMountainMediaInc pushed a commit to BrightMountainMediaInc/Prebid.js that referenced this pull request Sep 14, 2020
@jimee02
OpenX: Analytics Adapter update (prebid#5449)
1f43d7f
BrightMountainMediaInc pushed a commit to BrightMountainMediaInc/Prebid.js that referenced this pull request Sep 14, 2020
Revert "OpenX: Analytics Adapter update (prebid#5449)"
b3d63e0 
This reverts commit 1f43d7f.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsnellbaker jsnellbaker jsnellbaker approved these changes

Assignees

@jsnellbaker jsnellbaker

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jimee02 @jsnellbaker @bretg @jaiminpanchal27