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

PBS adapter - fix logic for undefined endpoint URLs #7076

Merged
merged 5 commits into from
Jul 12, 2021
Merged

PBS adapter - fix logic for undefined endpoint URLs #7076

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3c8055e
PBS adapter - fix logic for undefined endpoint URLs
jsnellbaker Jun 22, 2021
793934b
update messages and add unit test
jsnellbaker Jun 22, 2021
e23cf2d
fix lint error
ChrisHuie Jun 22, 2021
86a73c8
new tests and other changes
jsnellbaker Jun 23, 2021
0a0274f
resolve conflict issue
jsnellbaker Jun 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion modules/prebidServerBidAdapter/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,13 @@ function formatUrlParams(option) {
let temp = option[prop];
option[prop] = { p1Consent: temp, noP1Consent: temp };
}
if (utils.isPlainObject(option[prop]) && (!option[prop].p1Consent || !option[prop].noP1Consent)) {
msm0504 marked this conversation as resolved.
Show resolved Hide resolved
['p1Consent', 'noP1Consent'].forEach((conUrl) => {
if (!option[prop][conUrl]) {
utils.logWarn(`s2sConfig.${prop}.${conUrl} was not defined in manually set s2sConfig. This may prevent PBS requests from executing in certain cases. Please define field manually or instead use the defaultVendor settings of host company (if avialable).`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've been encouraging folks to keep messages short because they don't minify.

How about something more terse like "s2sConfig.${prop}.${conUrl} not defined. PBS request will be skipped in some P1 scenarios."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks, makes sense. updated.

}
});
}
});
}

Expand Down Expand Up @@ -1097,7 +1104,7 @@ export function PrebidServer() {
const request = OPEN_RTB_PROTOCOL.buildRequest(s2sBidRequest, bidRequests, validAdUnits, s2sBidRequest.s2sConfig, requestedBidders);
const requestJson = request && JSON.stringify(request);
utils.logInfo('BidRequest: ' + requestJson);
if (request && requestJson) {
if (request && requestJson && getMatchingConsentUrl(s2sBidRequest.s2sConfig.endpoint, gdprConsent)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe put the URL into a variable. Makes refactoring easier as there's only one place to look for

ajax(
getMatchingConsentUrl(s2sBidRequest.s2sConfig.endpoint, gdprConsent),
{
Expand All @@ -1107,6 +1114,8 @@ export function PrebidServer() {
requestJson,
{ contentType: 'text/plain', withCredentials: true }
);
} else {
utils.logError('Prebid Server request did not execute. Please ensure endpoint URL(s) are properly defined in your s2sConfig. Otherwise check for warnings/errors, likely noted earlier in console logs...');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, with the goal of saving bytes in the package, how about something more terse like

"PBS request not made. Check endpoints."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated.

}
}
};
Expand Down