Extending the Real Time Data Module #5519

omerDotan · 2020-07-21T12:39:11Z

Real time data module according to -
#4610

browsi sub module for real time data, new hook bidsBackCallback, fix for config unsubscribe

update docs

configure submodule on submodules.json

browsi sub module for real time data, new hook bidsBackCallback, fix for config unsubscribe

update docs

configure submodule on submodules.json

# Conflicts: # modules/browsiRtdProvider.js

# Conflicts: # modules/browsiRtdProvider.js # modules/rtdModule/index.js

modules/rtdModule/index.js

karimMourra · 2020-07-21T21:37:43Z

modules/rtdModule/index.js

+/**
+ * @property
+ * @summary delay auction for this sub module
+ * @name ModuleConfig#waitForIt


would renaming waitForIt to isAsync express the intention more clearly ?

all are async, so it might be confusing. please see comment on the second conversation

karimMourra · 2020-07-21T21:38:50Z

modules/rtdModule/index.js

- const callbackExpected = subModules.length;
- let dataReceived = [];
+export function getProviderData(adUnits, callback) {
+ const mustWaitSubModulesLength = subModules.filter(sm => sm.config && sm.config.waitForIt).length;


would asyncSubmodulesCount be a more expressive var name ?

all submodules are async
the wait for is actually a way to say what submodules are less important - meaning we prefer not to delay the auction any more for them

I did change the names a bit, let me know what you think

I understand now thank you so much for the clarification! I think "topPriority" would be a good term; it might be easier for future readers

karimMourra · 2020-07-21T21:40:43Z

modules/rtdModule/index.js

+export function getProviderData(adUnits, callback) {
+ const mustWaitSubModulesLength = subModules.filter(sm => sm.config && sm.config.waitForIt).length;
+ let callbackExpected = mustWaitSubModulesLength || subModules.length;
+ const mustHaveModules = mustWaitSubModulesLength > 0;


how about isAsync or mustWait as var name alternatives ? At first I didn't understand what mustHaveModules meant

changed the names a bit and added comments on the function, hope it makes things more clear

karimMourra · 2020-07-24T03:36:16Z

@omerBrowsi an integration test failed because array.find is not supported. This is fixed in 34b9f9b

jsnellbaker

I think on the whole this looks okay. I did have a questions/concern on one part. Please see in-line comments below.

Thanks for putting this together.

jsnellbaker · 2020-07-29T13:51:19Z

modules/rtdModule/index.js

+ let subModulesByOrder = [];
+ _dataProviders.forEach(provider => {
+ const sm = subModules.find(s => s.name === provider.name);
+ const initResponse = sm && sm.init && sm.init(provider, gdprDataHandler.getConsentData(), uspDataHandler.getConsentData()) !== 'failure';


To clarify on the GDPR and USP data, are we sure that in the sequence of things that this data would be available by the time the submodule's init functions are called?

This initSubModules function is loaded as part of the overall rtdModule's init(), which in turn is loaded as the file is loaded.

But the consent information is obtained once the requestBids call chain starts, which should be after this code here has already executed. So could there be a scenario here where these consent params in the sm.init wouldn't have the real data? Or am I misreading something?

Yes, you are correct
I added a hook so it will run before 'makeBidRequests', if you think of a better solution please let me know

variables naming

lgtm-com · 2020-08-02T05:54:20Z

This pull request introduces 1 alert when merging 735e8e9 into 477fe0c - view on LGTM.com

new alerts:

1 for Expression has no effect

karimMourra · 2020-08-12T03:14:35Z

Hi @omerBrowsi and @jsnellbaker is there an ETA for when this branch will be merged ? It's currently a blocker for #5537

pm-harshad-mane

Can we separate the test cases for RTD Module and BrowsiViewability(RTD Sub-module)?

Asafsham · 2020-08-17T11:36:35Z

Hey @robertrmartinez, would you be able to get to this PR? tools PMC really wants to push it asap.

bretg · 2020-08-19T18:39:43Z

We already have two reviewers, so taking Bobby off as a required reviewer. @Asafsham - we're going to a document at some point soon: a 'how to write an RTD module' guide similar to https://docs.prebid.org/dev-docs/integrate-with-the-prebid-analytics-api.html

But I'm not going to call that a blocker. So if @pm-harshad-mane is willing to push his unit test splitting request to a future PR, this can be merged.

pm-harshad-mane

As discussed with @bretg , unit test cases will be implemented in a separate PR.
@omerBrowsi please add unit-test cases soon :-)

bretg

I'd like for this to work like the User ID sub-modules: when someone builds the 'browsiRtdProvider', the RTD core is automatically added. Am going to assume that's the case as I see this in https://github.com/prebid/Prebid.js/blob/master/modules/.submodules.json

src/targeting.js

Asafsham · 2020-08-20T08:59:59Z

@bretg @pm-harshad-mane We are working on the Docs, should be ready be Monday.

omerDotan · 2020-08-23T09:12:09Z

@bretg I opened pull request for a bug related to this
#5651
Can we have it approved and merged before release?

* real time data module, browsi sub module for real time data, new hook bidsBackCallback, fix for config unsubscribe * change timeout&primary ad server only to auctionDelay update docs * support multiple providers * change promise to callbacks configure submodule on submodules.json * bug fixes * use Prebid ajax * tests fix * browsi real time data provider improvements * real time data module, browsi sub module for real time data, new hook bidsBackCallback, fix for config unsubscribe * change timeout&primary ad server only to auctionDelay update docs * support multiple providers * change promise to callbacks configure submodule on submodules.json * bug fixes * use Prebid ajax * tests fix * browsi real time data provider improvements * RTD module extend prebid#4610 * add hook for submodule init variables naming

This reverts commit e47b087.

omerDotan added 30 commits August 25, 2019 14:59

real time data module,

6121b4e

browsi sub module for real time data, new hook bidsBackCallback, fix for config unsubscribe

change timeout&primary ad server only to auctionDelay

1a80b14

update docs

support multiple providers

3b85815

change promise to callbacks

0cb7b69

configure submodule on submodules.json

bug fixes

0908134

use Prebid ajax

cf4c5a9

tests fix

7beeee3

browsi real time data provider improvements

60aaeaa

real time data module,

0e06e6f

browsi sub module for real time data, new hook bidsBackCallback, fix for config unsubscribe

change timeout&primary ad server only to auctionDelay

e9312c7

update docs

support multiple providers

c0901fe

change promise to callbacks

398f922

configure submodule on submodules.json

bug fixes

b3d0bea

use Prebid ajax

a4f2de6

tests fix

65ed991

browsi real time data provider improvements

15337d2

Merge remote-tracking branch 'origin/master'

76b3208

Merge remote-tracking branch 'upstream/master'

6a8c111

# Conflicts: # modules/browsiRtdProvider.js

Merge remote-tracking branch 'upstream/master'

b9b05f0

Merge remote-tracking branch 'upstream/master'

89013d7

Merge remote-tracking branch 'upstream/master'

342484c

Merge remote-tracking branch 'upstream/master'

faa02bf

Merge remote-tracking branch 'upstream/master'

88430b1

Merge remote-tracking branch 'upstream/master'

b73b6d7

# Conflicts: # modules/browsiRtdProvider.js # modules/rtdModule/index.js

Merge remote-tracking branch 'upstream/master'

00c027c

Merge remote-tracking branch 'upstream/master'

51555b6

Merge remote-tracking branch 'upstream/master'

c959997

Merge remote-tracking branch 'upstream/master'

a277d65

Merge remote-tracking branch 'upstream/master'

ebf1fa8

Merge remote-tracking branch 'upstream/master'

6ec752e

bretg requested review from jsnellbaker, robertrmartinez and pm-harshad-mane July 21, 2020 14:28

bretg assigned pm-harshad-mane and unassigned FilipStamenkovic Jul 21, 2020

karimMourra reviewed Jul 21, 2020

View reviewed changes

karimMourra mentioned this pull request Jul 23, 2020

JW Player Real Time Data Provider #5537

Merged

1 task

bretg mentioned this pull request Jul 24, 2020

Proposed Prebid.js Module Rules #5239

Closed

jsnellbaker requested changes Jul 29, 2020

View reviewed changes

add hook for submodule init

735e8e9

variables naming

pm-harshad-mane requested changes Aug 12, 2020

View reviewed changes

jsnellbaker approved these changes Aug 14, 2020

View reviewed changes

karimMourra approved these changes Aug 17, 2020

View reviewed changes

bretg removed the request for review from robertrmartinez August 19, 2020 18:36

bretg added the needs docs label Aug 19, 2020

pm-harshad-mane approved these changes Aug 19, 2020

View reviewed changes

bretg reviewed Aug 19, 2020

View reviewed changes

src/targeting.js Show resolved Hide resolved

bretg approved these changes Aug 19, 2020

View reviewed changes

bretg merged commit 23af840 into prebid:master Aug 19, 2020

omerDotan mentioned this pull request Aug 23, 2020

Rtd extend bug fix #5651

Closed

1 task

BrightMountainMediaInc added a commit to BrightMountainMediaInc/Prebid.js that referenced this pull request Sep 14, 2020

Revert "Extending the Real Time Data Module (prebid#5519)"

4405b05

This reverts commit e47b087.

GeoEdge-r-and-d mentioned this pull request Oct 19, 2020

Add Geoedge RTD provider submodule #5869

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extending the Real Time Data Module #5519

Extending the Real Time Data Module #5519

omerDotan commented Jul 21, 2020

karimMourra Jul 21, 2020

omerDotan Aug 2, 2020 •

edited

Loading

karimMourra Jul 21, 2020

omerDotan Aug 2, 2020

karimMourra Aug 3, 2020

karimMourra Jul 21, 2020

omerDotan Aug 2, 2020

karimMourra commented Jul 24, 2020

jsnellbaker left a comment

jsnellbaker Jul 29, 2020

omerDotan Aug 2, 2020

lgtm-com bot commented Aug 2, 2020

karimMourra commented Aug 12, 2020

pm-harshad-mane left a comment

Asafsham commented Aug 17, 2020

bretg commented Aug 19, 2020

pm-harshad-mane left a comment

bretg left a comment •

edited

Loading

Asafsham commented Aug 20, 2020

omerDotan commented Aug 23, 2020

Extending the Real Time Data Module #5519

Extending the Real Time Data Module #5519

Conversation

omerDotan commented Jul 21, 2020

Choose a reason for hiding this comment

omerDotan Aug 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karimMourra commented Jul 24, 2020

jsnellbaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Aug 2, 2020

karimMourra commented Aug 12, 2020

pm-harshad-mane left a comment

Choose a reason for hiding this comment

Asafsham commented Aug 17, 2020

bretg commented Aug 19, 2020

pm-harshad-mane left a comment

Choose a reason for hiding this comment

bretg left a comment • edited Loading

Choose a reason for hiding this comment

Asafsham commented Aug 20, 2020

omerDotan commented Aug 23, 2020

omerDotan Aug 2, 2020 •

edited

Loading

bretg left a comment •

edited

Loading