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

RTD Module: add 'onBidRequest' event handler for RTD submodules #7729

Merged
merged 4 commits into from
Dec 2, 2021
Merged

RTD Module: add 'onBidRequest' event handler for RTD submodules #7729

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5143d2
RTD: Add 'onBidRequest' event handler for RTD submodules
dgirardi Nov 17, 2021
4d64aca
Merge branch 'master' into rtd-onBidRequest
dgirardi Nov 17, 2021
7854cb0
RTD module: update documentation
dgirardi Dec 1, 2021
df0dcd5
Merge branch 'master' into rtd-onBidRequest
dgirardi Dec 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 36 additions & 17 deletions modules/rtdModule/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@

import {config} from '../../src/config.js';
import {module} from '../../src/hook.js';
import { logError, logWarn } from '../../src/utils.js';
import {logError, logWarn} from '../../src/utils.js';
import events from '../../src/events.js';
import CONSTANTS from '../../src/constants.json';
import {gdprDataHandler, uspDataHandler} from '../../src/adapterManager.js';
Expand All @@ -169,8 +169,43 @@ let _userConsent;
*/
export function attachRealTimeDataProvider(submodule) {
registeredSubModules.push(submodule);
return function detach() {
const idx = registeredSubModules.indexOf(submodule)
if (idx >= 0) {
registeredSubModules.splice(idx, 1);
initSubModules();
}
}
Comment on lines +183 to +189
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand why we need to return the detach function and call initSubmodules func again? I understand that it's getting call in the unit tests file, but does it make sense to have it here as a return value of attachRealTimeDataProvider function. What are your thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is the best way to do it, but I'll take suggestions. Do you prefer a separate global "deinit" function, or something else?
(I think this is the way because an undocumented return value that noone is using at the moment is less ugly than a public "resetSubmodules" function that is only called by the tests. In fact, the real way to do it would be to de-globalize the whole module, but I didn't feel like committing to a huge refactor for this issue).

Copy link
Contributor

@Fawke Fawke Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@dgirardi Sure, I somewhat agree with what you're saying. I don't have a strict preference for either of the approaches here, between yours and the global func 'resetSubmodules'. My concern was the func name, does it represent what is being implemented inside. Would it be possible to at least update the description of the function and document the return value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right - updated the docs.

}

/**
* call each sub module event function by config order
*/
const setEventsListeners = (function () {
let registered = false;
return function setEventsListeners() {
if (!registered) {
Object.entries({
[CONSTANTS.EVENTS.AUCTION_INIT]: 'onAuctionInitEvent',
[CONSTANTS.EVENTS.AUCTION_END]: 'onAuctionEndEvent',
[CONSTANTS.EVENTS.BID_RESPONSE]: 'onBidResponseEvent',
[CONSTANTS.EVENTS.BID_REQUESTED]: 'onBidRequestEvent'
}).forEach(([ev, handler]) => {
events.on(ev, (args) => {
subModules.forEach(sm => {
try {
sm[handler] && sm[handler](args, sm.config, _userConsent)
} catch (e) {
logError(`RTD provider '${sm.name}': error in '${handler}':`, e);
}
});
})
});
registered = true;
}
}
})();
Comment on lines +195 to +218
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice code organisation to avoid calling and defining each handler separately.


export function init(config) {
const confListener = config.getConfig(MODULE_NAME, ({realTimeData}) => {
if (!realTimeData.dataProviders) {
Expand Down Expand Up @@ -211,22 +246,6 @@ function initSubModules() {
subModules = subModulesByOrder;
}

/**
* call each sub module event function by config order
*/
function setEventsListeners() {
events.on(CONSTANTS.EVENTS.AUCTION_INIT, (args) => {
subModules.forEach(sm => { sm.onAuctionInitEvent && sm.onAuctionInitEvent(args, sm.config, _userConsent) })
});
events.on(CONSTANTS.EVENTS.AUCTION_END, (args) => {
getAdUnitTargeting(args);
subModules.forEach(sm => { sm.onAuctionEndEvent && sm.onAuctionEndEvent(args, sm.config, _userConsent) })
});
events.on(CONSTANTS.EVENTS.BID_RESPONSE, (args) => {
subModules.forEach(sm => { sm.onBidResponseEvent && sm.onBidResponseEvent(args, sm.config, _userConsent) })
});
}

/**
* loop through configured data providers If the data provider has registered getBidRequestData,
* call it, providing reqBidsConfigObj, consent data and module params
Expand Down
184 changes: 135 additions & 49 deletions test/spec/modules/realTimeDataModule_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import * as rtdModule from 'modules/rtdModule/index.js';
import { config } from 'src/config.js';
import {config} from 'src/config.js';
import * as sinon from 'sinon';
import {default as CONSTANTS} from '../../../src/constants.json';
import {default as events} from '../../../src/events.js';

const getBidRequestDataSpy = sinon.spy();

Expand Down Expand Up @@ -58,33 +60,87 @@ const conf = {
};

describe('Real time module', function () {
before(function () {
rtdModule.attachRealTimeDataProvider(validSM);
rtdModule.attachRealTimeDataProvider(invalidSM);
rtdModule.attachRealTimeDataProvider(failureSM);
rtdModule.attachRealTimeDataProvider(nonConfSM);
rtdModule.attachRealTimeDataProvider(validSMWait);
});
let eventHandlers;
let sandbox;

after(function () {
config.resetConfig();
});
function mockEmitEvent(event, ...args) {
(eventHandlers[event] || []).forEach((h) => h(...args));
}

beforeEach(function () {
config.setConfig(conf);
before(() => {
eventHandlers = {};
sandbox = sinon.sandbox.create();
sandbox.stub(events, 'on').callsFake((event, handler) => {
if (!eventHandlers.hasOwnProperty(event)) {
eventHandlers[event] = [];
}
eventHandlers[event].push(handler);
});
});

it('should use only valid modules', function () {
rtdModule.init(config);
expect(rtdModule.subModules).to.eql([validSMWait, validSM]);
after(() => {
sandbox.restore();
});

it('should be able to modify bid request', function (done) {
rtdModule.setBidRequestsData(() => {
assert(getBidRequestDataSpy.calledTwice);
assert(getBidRequestDataSpy.calledWith({bidRequest: {}}));
describe('', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any name for the describe block? Usually it helps to have a meaningful name especially when the test fails.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried, I couldn't find one that's better than no name. These are the previously existing tests that I grouped together because I don't need the same setup for my new ones. The only name I can think that makes sense is something like "old tests" which would not help at all on failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine. Didn't know you clubbed old tests inside this describe block.

const PROVIDERS = [validSM, invalidSM, failureSM, nonConfSM, validSMWait];
let _detachers;

beforeEach(function () {
_detachers = PROVIDERS.map(rtdModule.attachRealTimeDataProvider);
rtdModule.init(config);
config.setConfig(conf);
});

afterEach(function () {
_detachers.forEach((f) => f());
config.resetConfig();
});

it('should use only valid modules', function () {
expect(rtdModule.subModules).to.eql([validSMWait, validSM]);
});

it('should be able to modify bid request', function (done) {
rtdModule.setBidRequestsData(() => {
assert(getBidRequestDataSpy.calledTwice);
assert(getBidRequestDataSpy.calledWith({bidRequest: {}}));
done();
}, {bidRequest: {}})
});

it('sould place targeting on adUnits', function (done) {
const auction = {
adUnitCodes: ['ad1', 'ad2'],
adUnits: [
{
code: 'ad1'
},
{
code: 'ad2',
adserverTargeting: {preKey: 'preValue'}
}
]
};

const expectedAdUnits = [
{
code: 'ad1',
adserverTargeting: {key: 'validSMWait'}
},
{
code: 'ad2',
adserverTargeting: {
preKey: 'preValue',
key: 'validSM'
}
}
];

const adUnits = rtdModule.getAdUnitTargeting(auction);
assert.deepEqual(expectedAdUnits, adUnits)
done();
}, {bidRequest: {}})
})
});

it('deep merge object', function () {
Expand Down Expand Up @@ -125,36 +181,66 @@ describe('Real time module', function () {
assert.deepEqual(expected, merged);
});

it('sould place targeting on adUnits', function (done) {
const auction = {
adUnitCodes: ['ad1', 'ad2'],
adUnits: [
{
code: 'ad1'
},
{
code: 'ad2',
adserverTargeting: {preKey: 'preValue'}
}
]
describe('event', () => {
const EVENTS = {
[CONSTANTS.EVENTS.AUCTION_INIT]: 'onAuctionInitEvent',
[CONSTANTS.EVENTS.AUCTION_END]: 'onAuctionEndEvent',
[CONSTANTS.EVENTS.BID_RESPONSE]: 'onBidResponseEvent',
[CONSTANTS.EVENTS.BID_REQUESTED]: 'onBidRequestEvent'
}
const conf = {
'realTimeData': {
dataProviders: [
{
'name': 'tp1',
},
{
'name': 'tp2'
}
]
}
};
let providers;
let _detachers;

const expectedAdUnits = [
{
code: 'ad1',
adserverTargeting: {key: 'validSMWait'}
},
{
code: 'ad2',
adserverTargeting: {
preKey: 'preValue',
key: 'validSM'
}
function eventHandlingProvider(name) {
const provider = {
name: name,
init: () => true
}
];

const adUnits = rtdModule.getAdUnitTargeting(auction);
assert.deepEqual(expectedAdUnits, adUnits)
done();
Object.values(EVENTS).forEach((ev) => provider[ev] = sinon.spy());
return provider;
}

beforeEach(() => {
providers = [eventHandlingProvider('tp1'), eventHandlingProvider('tp2')];
_detachers = providers.map(rtdModule.attachRealTimeDataProvider);
rtdModule.init(config);
config.setConfig(conf);
});

afterEach(() => {
_detachers.forEach((d) => d())
config.resetConfig();
})

Object.entries(EVENTS).forEach(([event, hook]) => {
it(`'${event}' should be propagated to providers through '${hook}'`, () => {
const eventArg = {};
mockEmitEvent(event, eventArg);
providers.forEach((provider) => {
const providerConf = conf.realTimeData.dataProviders.find((cfg) => cfg.name === provider.name);
expect(provider[hook].called).to.be.true;
expect(provider[hook].args).to.have.length(1);
expect(provider[hook].args[0]).to.include.members([eventArg, providerConf])
})
});

it(`${event} should not fail to propagate elsewhere if a provider throws in its event handler`, () => {
providers[0][hook] = function () { throw new Error() };
mockEmitEvent(event);
expect(providers[1][hook].called).to.be.true;
});
});
})
});