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

Audigent RTD configurable per-bidder segment mappings #5903

Merged
merged 26 commits into from
Nov 16, 2020
Merged

Audigent RTD configurable per-bidder segment mappings #5903

merged 26 commits into from
Nov 16, 2020

Conversation

antlauzon
Copy link
Contributor

Type of change

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

This PR adds configurable per-bidder segment mapping capabilities for Audigent's RTD provider. It relieves bid adapter maintainers from having to modify their bid adapters in order to make them compatible with our real-time data module's segments.

Relevant previous pull requests:

#4834
#5777

@antlauzon antlauzon mentioned this pull request Oct 27, 2020
Merged
@aleksatr
Copy link
Contributor

@anthonylauzon I don't see the unit test file for this module at all (audigentRtdProvider_spec.js). Make sure unit tests are added and you have at least 80% coverage.

To generate and view the code coverage reports:

gulp test-coverage
gulp view-coverage

@aleksatr
Copy link
Contributor

@anthonylauzon , I don't see docs PR here, you should open a PR against https://github.com/prebid/prebid.github.io, describing additional params you introduced in this PR. From what I see you're missing docs entry for this module altogether, please take a look at this page on how to add an appropriate docs file.

@antlauzon
Copy link
Contributor Author

https://github.com/prebid/prebid.github.io

prebid/prebid.github.io#2369

bretg
bretg requested changes Oct 31, 2020
View reviewed changes
modules/audigentRtdProvider.js Outdated Show resolved Hide resolved
@antlauzon antlauzon requested a review from bretg November 2, 2020 22:59
bretg
bretg requested changes Nov 3, 2020
View reviewed changes
Copy link
Collaborator

@bretg bretg left a comment

Choose a reason for hiding this comment

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

I'm not looking at the code but have a question and comments about the docs.

integrationExamples/gpt/audigentSegments_example.html Outdated Show resolved Hide resolved
modules/audigentRtdProvider.md Outdated Show resolved Hide resolved
modules/audigentRtdProvider.md Outdated Show resolved Hide resolved
modules/audigentRtdProvider.md Outdated Show resolved Hide resolved
modules/audigentRtdProvider.md Outdated Show resolved Hide resolved
@antlauzon antlauzon requested a review from bretg November 3, 2020 18:27
bretg
bretg requested changes Nov 3, 2020
View reviewed changes
Copy link
Collaborator

@bretg bretg left a comment

Choose a reason for hiding this comment

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

Getting close. (!)

modules/audigentRtdProvider.md Outdated Show resolved Hide resolved
modules/audigentRtdProvider.md Outdated Show resolved Hide resolved
modules/audigentRtdProvider.md Outdated Show resolved Hide resolved
@antlauzon antlauzon requested a review from bretg November 3, 2020 19:12
bretg
bretg approved these changes Nov 3, 2020
View reviewed changes
Copy link
Collaborator

@bretg bretg left a comment

Choose a reason for hiding this comment

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

Approving the documentation and examples. Thanks. Will assign someone else for a secondary code review.

@bretg bretg requested a review from msm0504 November 3, 2020 19:29
@antlauzon antlauzon requested a review from ncolletti November 11, 2020 18:17
@ncolletti
Copy link
Contributor

This PR is slightly below 80% code coverage. Also, I am not receiving audience segments on the example page, it would be helpful to use a test id here that returns a mock response 100% of the time, is that possible?

@antlauzon antlauzon closed this Nov 12, 2020
@antlauzon antlauzon reopened this Nov 12, 2020
@antlauzon
Copy link
Contributor Author

This PR is slightly below 80% code coverage. Also, I am not receiving audience segments on the example page, it would be helpful to use a test id here that returns a mock response 100% of the time, is that possible?

i've added a test id to the integration example that retrieves test segments from our backend

@antlauzon antlauzon requested a review from ncolletti November 13, 2020 16:01
@ncolletti
Copy link
Contributor

LGTM

@aleksatr
Copy link
Contributor

LGTM

@aleksatr aleksatr merged commit fb32955 into prebid:master Nov 16, 2020
@antlauzon antlauzon mentioned this pull request Nov 20, 2020
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncolletti ncolletti ncolletti approved these changes

@bretg bretg bretg approved these changes

@aleksatr aleksatr Awaiting requested review from aleksatr

Assignees

@ncolletti ncolletti

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@antlauzon @aleksatr @ncolletti @bretg