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

1plusX RTD submodule: New RTD Module #8614

Merged
merged 18 commits into from
Jul 14, 2022

Conversation

bwhisp
Copy link
Contributor

@bwhisp bwhisp commented Jun 28, 2022

Type of change

  • Bugfix
  • Feature (New RTD submodule)
  • 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 a RTD submodule for 1plusX
This module will fetch first party data from 1plusX's Profile API consisting of :

  • the audiences the user is part of
  • the topics related to the page

Then it will transmit this data to the supported & selected bid adapters through setBidderConfig

Contents of the PR :
- 1plusX RTD module: modules/1plusXRtdProvider.js
- Unit tests for 1plusX RTD module: test/spec/modules/1plusXRtdProvider_spec.js
- Integration example for 1plusX RTD module: integrationExamples/gpt/1plusXRtdProviderExample.html
- Documentation for 1plusX RTD module: modules/1plusXRtdProvider.md

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

Other information

I am not 100% sure about where to write the data in the ORTB2 format when updating the bid adapter configs.

The data I am fetching from 1plusX's Profile API consists of lists of custom-defined Ids that don't follow the standard taxonomy.
While I found info about where to write audience segments & topics in ORTB2; it seemed that it concerned only those following the standard taxonomy.

I reached a solution by combining the ORTB2 specs (pages 31 & 25) AND some examples from other RTD Providers :

  • audience segments will be written in an element of ortb2.user.data array
  • topics will be written in ortb2.site.keywords

I will add comments to the related parts of the code, please review these parts with special care.

* @param {string[]} topics Represents the topics of the page
* @returns {Object} Object describing the updates to make on bidder configs
*/
export const buildOrtb2Updates = ({ segments = [], topics = [] }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function will create the updates to the ORTB2 object to be set on bid adapters configs

Comment on lines 102 to 119
const userData = {
name: '1plusX.com',
segment: segments.map((segmentId) => ({ id: segmentId }))
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the element I will add to ortb2.user.data containing the audience segments

Comment on lines 106 to 108
const site = {
keywords: topics.join(',')
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here are the topics I will write to ortb2.site.keywords

Copy link
Collaborator

Choose a reason for hiding this comment

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

site.content.data.segment.id

Copy link
Collaborator

Choose a reason for hiding this comment

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

you may notice that appnexus doesnt support site.content.data.segment.id. In that case, we would prefer a change to the appnexus adapter to put the data in the keywords vector than a change to your module to write it to where it no longer belongs or special appnexus handling in your adapter. We have an open issue to remove similar special handling for appnexus in the airgrid and permutive adapters (among possible others)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @patmmccann,

I will make the change by handling appnexus separately
May I get the issue number you were referring to ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment on lines 126 to 127
const currentSite = deepAccess(bidderConfigCopy, 'ortb2.site')
const updatedSite = mergeDeep(currentSite, site);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Writing topics to ortb2.site.keywords

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is the wrong place for these data

Comment on lines 129 to 133
const currentUserData = deepAccess(bidderConfigCopy, 'ortb2.user.data') || [];
const updatedUserData = [
...currentUserData.filter(({ name }) => name != userData.name),
userData
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In ortb2.user.data; replacing the element bearing 1plusX.com name with the element defined at buildOrtb2Updates::const userData

@ChrisHuie ChrisHuie changed the title 1plusX RTD submodule 1plusX RTD submodule: New RTD Module Jun 28, 2022
@ChrisHuie ChrisHuie requested a review from Fawke June 28, 2022 17:43
Copy link
Contributor

@Fawke Fawke left a comment

Choose a reason for hiding this comment

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

Hi @bwhisp,

Thanks for the PR. While I'm still reviewing your changes, here are a few quick things that I noticed.

  • Your test coverage is at ~67%. We need to have more than 80% code coverage.
  • You need to add your module in .submodules.json file.

**to check code-coverage, you can run the command, gulp test-coverage and see the report. Here's a screenshot of what I got when I ran it.

Screenshot 2022-07-05 at 4 29 38 PM

| params.timeout | Integer | timeout (ms) | 1000ms |

## Supported Bidders
At the moment only Appnexus (`appnexus`) and Magnite (`rubicon`) are supported
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a rules violation

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 just removed the SUPPORTED_BIDDERS limitation

gads.type = 'text/javascript';
var useSSL = 'https:' == document.location.protocol;
gads.src = (useSSL ? 'https:' : 'http:') +
'//www.googletagservices.com/tag/js/gpt.js';
Copy link
Collaborator

Choose a reason for hiding this comment

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

please load from correct location

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @patmmccann
I took this piece of code from another module implementation example.
These same lines are used in several module examples.

Shall I take /workspaces/Prebid.js/integrationExamples/gpt/hello_world.html as a model instead ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have an open docs issue to update all the locations to gpt.js new official host (doubleclick.net), while that is still outstanding, we'd prefer to not add things to fix later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

}
}

it("doesn't set config for unsupported bidders", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

demonstrates the rules violation

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 just removed the SUPPORTED_BIDDERS limitation

@bwhisp
Copy link
Contributor Author

bwhisp commented Jul 5, 2022

Hi @bwhisp,

Thanks for the PR. While I'm still reviewing your changes, here are a few quick things that I noticed.

  • Your test coverage is at ~67%. We need to have more than 80% code coverage.
  • You need to add your module in .submodules.json file.

**to check code-coverage, you can run the command, gulp test-coverage and see the report. Here's a screenshot of what I got when I ran it.

Screenshot 2022-07-05 at 4 29 38 PM

Hi @Fawke

Thank you for the heads up!
I updated my tests for better coverage, adding tests for the extractConfig function
Now the coverage should be better

Tomorrow I'll work on @patmmccann 's comments which will have the effect of removing the SUPPORTED_BIDDERS limitation

@bwhisp bwhisp requested a review from patmmccann July 6, 2022 10:22
@bwhisp
Copy link
Contributor Author

bwhisp commented Jul 6, 2022

Hi @patmmccann

thank you for the review

I've worked on the requested changes:

  • removed the supported bidders limitation
  • changed location to write topics to site.content.data.segment.id
  • introduced legacy processing for appnexus (writing data to site.keywords for this bidder)
  • updated tests

There's still 1 pending question about your comment on 1plusXRtdProviderExample.html

@patmmccann
Copy link
Collaborator

@bwhisp thank you for promptly addressing the rules concerns.

@Fawke remains the code reviewer; i just wanted to assist with rules feedback

} else {
const siteContentData = {
name: ORTB2_NAME,
segment: topics.map((topicId) => ({ id: topicId }))
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should also write to site.content.data.ext.segtax, if the segtax is custom to 1+x, you can reserve an integer with a pr to iab segtax.md file in the openrtb repo

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 will take care of it tomorrow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shall I also write a segtax for what I put in user.data.segment ?

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 opened InteractiveAdvertisingBureau/openrtb#108 for that matter

@bwhisp bwhisp requested review from Fawke and patmmccann July 8, 2022 08:33
Copy link
Contributor

@Fawke Fawke left a comment

Choose a reason for hiding this comment

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

Hi @bwhisp,

I went through your changes, they mostly look good. I just wanted to say that the Appnexus bid adapter doesn't yet read values from the ortb2 object. So, the special handling that you are doing for Appnexus currently, it won't work.

You will need to modify the bidRequest object directly and set bid.params.keywords. See an example here for airgridRtdProvider.

I also see a merge conflict in the .submodules.json file.

@bwhisp
Copy link
Contributor Author

bwhisp commented Jul 12, 2022

Hi @Fawke

thank you for the review.
How shall I proceed with the merge conflict? On the PR it states that Only those with write access to this repository can merge pull requests.. If required I can pull master and perform the merge locally but I want to make sure to follow any process if there's one.

I will also look at airgridRtdProvider and update the handling for Appnexus

@Fawke
Copy link
Contributor

Fawke commented Jul 13, 2022

@bwhisp Yeah, you can pull in master and perform the merge locally.

@bwhisp bwhisp force-pushed the DC-2488-1plusX-RTD-module branch from 98f4e94 to 04ae259 Compare July 13, 2022 08:36
@bwhisp bwhisp force-pushed the DC-2488-1plusX-RTD-module branch from 04ae259 to 7d91251 Compare July 13, 2022 08:59
@bwhisp
Copy link
Contributor Author

bwhisp commented Jul 13, 2022

Hi @Fawke

following your feedback I updated the handling of Appnexus & I resolved the merge conflict

@bwhisp bwhisp requested a review from Fawke July 13, 2022 13:55
Copy link
Contributor

@Fawke Fawke left a comment

Choose a reason for hiding this comment

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

LGTM!
Will wait for @patmmccann approval before merging.

@bwhisp
Copy link
Contributor Author

bwhisp commented Jul 14, 2022

Hello @patmmccann @Fawke

thank you for the review and the approval :)

I created a PR at InteractiveAdvertisingBureau/openrtb#108 to introduce the 1plusX audience & content taxonomies that will be used in this RTD module
Do you know if it's a prerequisite that InteractiveAdvertisingBureau/openrtb#108 is merged before #8614?
If yes, do you know who can perform a review? I've seen @patmmccann doing a few of them in the past but not 100% sure it's still the case

@patmmccann patmmccann merged commit b613039 into prebid:master Jul 14, 2022
@patmmccann
Copy link
Collaborator

IAB reviews those

ahmadlob referenced this pull request in taboola/Prebid.js Jul 27, 2022
* Empty shell for 1plusX RTD submodule (#1)

* Submodule initialization & functions (init; getBidRequestData) skeletons (#2)

* Testing for init function (#3)

* Requesting Profile API for Data (#4)

* Extract PAPI response & implementation example

* Transmitting targeting data to bidder adapters

* Markdown file documentation

* Code cleaned & jsDoc completed

* Change contact email + beautify parameters table + fix type in param name

* Change customerId param type to string in doc

* Add 1plusXRtdProvider as submodule of rtdModule

* Add more tests on extractConfig amongst others

* Remove SUPPORTED_BIDDERS limitation

* Remove supported bidders from docs

* Write to site.content.data.segment.id & keep legacy support for appnexus

* Change location of googleTagServices

* Add segtax for site.content.data

* Handle audiences for appNexus by putting them in config.appnexusAuctionKeywords
jorgeluisrocha pushed a commit to jwplayer/Prebid.js that referenced this pull request May 23, 2023
* Empty shell for 1plusX RTD submodule (#1)

* Submodule initialization & functions (init; getBidRequestData) skeletons (#2)

* Testing for init function (#3)

* Requesting Profile API for Data (#4)

* Extract PAPI response & implementation example

* Transmitting targeting data to bidder adapters

* Markdown file documentation

* Code cleaned & jsDoc completed

* Change contact email + beautify parameters table + fix type in param name

* Change customerId param type to string in doc

* Add 1plusXRtdProvider as submodule of rtdModule

* Add more tests on extractConfig amongst others

* Remove SUPPORTED_BIDDERS limitation

* Remove supported bidders from docs

* Write to site.content.data.segment.id & keep legacy support for appnexus

* Change location of googleTagServices

* Add segtax for site.content.data

* Handle audiences for appNexus by putting them in config.appnexusAuctionKeywords
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