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

feat(unruly-bid-adapter): use bidResponse siteId when configuring the renderer #3865

Merged
merged 3 commits into from
Jun 12, 2019
Merged

feat(unruly-bid-adapter): use bidResponse siteId when configuring the renderer #3865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc74ca9
feat(unruly-bid-adapter): use bidResponse siteId when configuring the…
May 29, 2019
35ebd6c
feat(unruly-bid-adapter): bail if siteId is missing
Jun 6, 2019
d97238f
feat(unruly-bid-adapter): log (but don't throw) when siteId is no pre…
Jun 7, 2019
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
16 changes: 14 additions & 2 deletions modules/unrulyBidAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@ import { registerBidder } from '../src/adapters/bidderFactory'
import { VIDEO } from '../src/mediaTypes'

function configureUniversalTag (exchangeRenderer) {
if (!exchangeRenderer.config) throw new Error('UnrulyBidAdapter: Missing renderer config.')
if (!exchangeRenderer.config.siteId) throw new Error('UnrulyBidAdapter: Missing renderer siteId.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

@paprikka

Are these checks now irrelevant?

Due to the filter, it seems a bid would never come into this function unless it already has the config and siteID.

Obviously this is not a show-stopper, but if it is irrelevant, please remove it.

If you do not get to it before the release scheduled for tomorrow, I will make sure to still merge it and we can update in a later PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes—these are not required any more.
I'm afraid I don't time to deal with this today—will submit another PR later this week if that's ok?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, please do not forget to update it when you have a chance!


parent.window.unruly = parent.window.unruly || {};
parent.window.unruly['native'] = parent.window.unruly['native'] || {};
parent.window.unruly['native'].siteId = parent.window.unruly['native'].siteId || exchangeRenderer.siteId;
parent.window.unruly['native'].siteId = parent.window.unruly['native'].siteId || exchangeRenderer.config.siteId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you please add a check to verify exchangeRenderer.config is defined before trying to access siteId?

I notice this exchangeRenderer object comes from your serverBid, and you may guarantee that this is always defined, but we would prefer to be defensive here please !

Copy link
Collaborator

Choose a reason for hiding this comment

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

Want to void Error: Cannot read property 'siteId' of undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Roger that—throwing meaningful exceptions unless siteId is available.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@paprikka Thanks,

quick question, where exactly does this exchangeRenderer.config come from?

The Unruly server request?

Is it set by the publisher somewhere?

Copy link
Contributor Author

@paprikka paprikka Jun 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

exchangeRenderer.config comes from the bidResponse (passed as an ext).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, okay.

So the suggestion here is to not throw an error, but instead use the log utils to log the error and return the expected value back to prebid core.

for example, Prebid Core expects the bids back as an array. So instead of throwing the error,

Can we update to log the error, and simply return back an empty array to Prebid core?

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done. Thanks!

parent.window.unruly['native'].supplyMode = 'prebid';
}

Expand Down Expand Up @@ -35,9 +38,18 @@ const serverResponseToBid = (bid, rendererInstance) => ({

const buildPrebidResponseAndInstallRenderer = bids =>
bids
.filter(serverBid => !!utils.deepAccess(serverBid, 'ext.renderer'))
.filter(serverBid => {
const hasConfig = !!utils.deepAccess(serverBid, 'ext.renderer.config');
const hasSiteId = !!utils.deepAccess(serverBid, 'ext.renderer.config.siteId');

if (!hasConfig) utils.logError(new Error('UnrulyBidAdapter: Missing renderer config.'));
if (!hasSiteId) utils.logError(new Error('UnrulyBidAdapter: Missing renderer siteId.'));

return hasSiteId
})
.map(serverBid => {
const exchangeRenderer = utils.deepAccess(serverBid, 'ext.renderer');

configureUniversalTag(exchangeRenderer);
configureRendererQueue();

Expand Down
81 changes: 71 additions & 10 deletions test/spec/modules/unrulyBidAdapter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ describe('UnrulyAdapter', function () {
'statusCode': statusCode,
'renderer': {
'id': 'unruly_inarticle',
'config': {},
'config': {
'siteId': 123456,
'targetingUUID': 'xxx-yyy-zzz'
},
'url': 'https://video.unrulymedia.com/native/prebid-loader.js'
},
'adUnitCode': adUnitCode
Expand Down Expand Up @@ -125,10 +128,10 @@ describe('UnrulyAdapter', function () {
it('should be a function', function () {
expect(typeof adapter.interpretResponse).to.equal('function');
});
it('should return empty array when serverResponse is undefined', function () {
it('should return [] when serverResponse is undefined', function () {
expect(adapter.interpretResponse()).to.deep.equal([]);
});
it('should return empty array when serverResponse has no bids', function () {
it('should return [] when serverResponse has no bids', function () {
const mockServerResponse = { body: { bids: [] } };
expect(adapter.interpretResponse(mockServerResponse)).to.deep.equal([])
});
Expand Down Expand Up @@ -157,7 +160,13 @@ describe('UnrulyAdapter', function () {
expect(fakeRenderer.setRender.called).to.be.false;

const mockReturnedBid = createOutStreamExchangeBid({adUnitCode: 'video1', bidId: 'mockBidId'});
const mockRenderer = { url: 'value: mockRendererURL' };
const mockRenderer = {
url: 'value: mockRendererURL',
config: {
siteId: 123456,
targetingUUID: 'xxx-yyy-zzz'
}
};
mockReturnedBid.ext.renderer = mockRenderer;
const mockServerResponse = createExchangeResponse(mockReturnedBid);

Expand All @@ -173,6 +182,58 @@ describe('UnrulyAdapter', function () {
sinon.assert.calledWithExactly(fakeRenderer.setRender, sinon.match.func)
});

it('should return [] and log if bidResponse renderer config is not available', function () {
sinon.assert.notCalled(utils.logError)

expect(Renderer.install.called).to.be.false;
expect(fakeRenderer.setRender.called).to.be.false;

const mockReturnedBid = createOutStreamExchangeBid({adUnitCode: 'video1', bidId: 'mockBidId'});
const mockRenderer = {
url: 'value: mockRendererURL'
};
mockReturnedBid.ext.renderer = mockRenderer;
const mockServerResponse = createExchangeResponse(mockReturnedBid);

expect(adapter.interpretResponse(mockServerResponse)).to.deep.equal([]);

const logErrorCalls = utils.logError.getCalls();
expect(logErrorCalls.length).to.equal(2);

const [ configErrorCall, siteIdErrorCall ] = logErrorCalls;

expect(configErrorCall.args.length).to.equal(1);
expect(configErrorCall.args[0].message).to.equal('UnrulyBidAdapter: Missing renderer config.');

expect(siteIdErrorCall.args.length).to.equal(1);
expect(siteIdErrorCall.args[0].message).to.equal('UnrulyBidAdapter: Missing renderer siteId.');
});

it('should return [] and log if siteId is not available', function () {
sinon.assert.notCalled(utils.logError)

expect(Renderer.install.called).to.be.false;
expect(fakeRenderer.setRender.called).to.be.false;

const mockReturnedBid = createOutStreamExchangeBid({adUnitCode: 'video1', bidId: 'mockBidId'});
const mockRenderer = {
url: 'value: mockRendererURL',
config: {}
};
mockReturnedBid.ext.renderer = mockRenderer;
const mockServerResponse = createExchangeResponse(mockReturnedBid);

expect(adapter.interpretResponse(mockServerResponse)).to.deep.equal([]);

const logErrorCalls = utils.logError.getCalls();
expect(logErrorCalls.length).to.equal(1);

const [ siteIdErrorCall ] = logErrorCalls;

expect(siteIdErrorCall.args.length).to.equal(1);
expect(siteIdErrorCall.args[0].message).to.equal('UnrulyBidAdapter: Missing renderer siteId.');
});

it('bid is placed on the bid queue when render is called', function () {
const exchangeBid = createOutStreamExchangeBid({ adUnitCode: 'video', vastUrl: 'value: vastUrl' });
const exchangeResponse = createExchangeResponse(exchangeBid);
Expand All @@ -191,7 +252,7 @@ describe('UnrulyAdapter', function () {
expect(sentRendererConfig.vastUrl).to.equal('value: vastUrl');
expect(sentRendererConfig.renderer).to.equal(fakeRenderer);
expect(sentRendererConfig.adUnitCode).to.equal('video')
})
});

it('should ensure that renderer is placed in Prebid supply mode', function () {
const mockExchangeBid = createOutStreamExchangeBid({adUnitCode: 'video1', bidId: 'mockBidId'});
Expand Down Expand Up @@ -237,28 +298,28 @@ describe('UnrulyAdapter', function () {
type: 'iframe',
url: 'https://video.unrulymedia.com/iframes/third-party-iframes.html'
})
})
});

it('should append consent params if gdpr does apply and consent is given', () => {
const mockConsent = {
gdprApplies: true,
consentString: 'hello'
}
};
const response = {}
const syncOptions = { iframeEnabled: true }
const syncs = adapter.getUserSyncs(syncOptions, response, mockConsent)
expect(syncs[0]).to.deep.equal({
type: 'iframe',
url: 'https://video.unrulymedia.com/iframes/third-party-iframes.html?gdpr=1&gdpr_consent=hello'
})
})
});

it('should append consent param if gdpr applies and no consent is given', () => {
const mockConsent = {
gdprApplies: true,
consentString: {}
}
const response = {}
};
const response = {};
const syncOptions = { iframeEnabled: true }
const syncs = adapter.getUserSyncs(syncOptions, response, mockConsent)
expect(syncs[0]).to.deep.equal({
Expand Down