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

byData Analytics Adapter : send data on bidwon, update adunit path, & add some test params #8621

Merged
merged 5 commits into from
Jul 11, 2022

Conversation

Prebid-bydata
Copy link
Contributor

Type of change

  • Bugfix
  • Feature
  • 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

  • analytics adapter code style and send data payload update
  • sending data at one more events bid won
  • add test url parameter to logs

Be sure to test the integration with your adserver using the [Hello World](/integrationExamples/gpt/hello_world.html) sample page.

- contact email of the adapter’s maintainer
- [ ] official adapter submission

For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:

- A link to a PR on the docs repo at https://github.com/prebid/prebid.github.io/

## Other information
<!-- References to related PR or issue #s, @mentions of the person or team responsible for reviewing changes, etc. -->

@lgtm-com
Copy link

lgtm-com bot commented Jun 29, 2022

This pull request introduces 1 alert when merging 0fb445d into 86399d9 - view on LGTM.com

new alerts:

  • 1 for Incomplete string escaping or encoding

@Prebid-bydata Prebid-bydata force-pushed the feature/byDataAnalyticsAdapter branch from 0fb445d to 5a9bec6 Compare June 29, 2022 11:39
@ChrisHuie ChrisHuie changed the title data on bidwon-adunit path-some test paramets added byData Analytics Adapter : data on bidwon-adunit path-some test paramets added Jun 30, 2022
@ChrisHuie ChrisHuie changed the title byData Analytics Adapter : data on bidwon-adunit path-some test paramets added byData Analytics Adapter : send data on bidwon, update adunit path, & add some test params Jun 30, 2022
@ChrisHuie ChrisHuie requested a review from ncolletti June 30, 2022 10:18
@ncolletti
Copy link
Contributor

Changes in the PR look good. However, I noticed your adapter is directly accessing localStorage via the window. Within this PR, could you include the change to import StorageManager and use the setDataInLocalStorage method to handle the setting of userId. Thank you!

Copy link
Contributor

@ncolletti ncolletti left a comment

Choose a reason for hiding this comment

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

looks good!

@ncolletti ncolletti merged commit 5166dc9 into prebid:master Jul 11, 2022
bwhisp pushed a commit to bwhisp/Prebid.js that referenced this pull request Jul 13, 2022
… add some test params (prebid#8621)

* data on bidwon-adunit path-some test paramets added

* update method for query string parameter

* removed unpadded space

* using StorageManager set/get value on local storage

Co-authored-by: Jitendra-quizlet <[email protected]>
ahmadlob referenced this pull request in taboola/Prebid.js Jul 27, 2022
… add some test params (#8621)

* data on bidwon-adunit path-some test paramets added

* update method for query string parameter

* removed unpadded space

* using StorageManager set/get value on local storage

Co-authored-by: Jitendra-quizlet <[email protected]>
RomainLofaso pushed a commit to criteo-forks/Prebid.js that referenced this pull request Aug 8, 2022
… add some test params (prebid#8621)

* data on bidwon-adunit path-some test paramets added

* update method for query string parameter

* removed unpadded space

* using StorageManager set/get value on local storage

Co-authored-by: Jitendra-quizlet <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants