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

Ad unit parse fix #36

Merged
merged 13 commits into from
Aug 20, 2018
Merged

Conversation

pm-shashank-jain
Copy link
Collaborator

Colon parsing issue. Changed the sequence of spiting and parsing.

return;
slot = splits[0];
if (splits.length == 2) {
bid.params.adUnitIndex = splits[1].split(":")[1];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will break in case the split on colon doesn't produce two elements. I believe this is not a mandatory check.

if (splits.length != 2) {
utils.logWarn('AdSlot Error: adSlot not in required format');
return;
slot = splits[0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we initializing this variable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's already initialized on line no 75 and value has been assigned for ad slot on line no 82

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok

}
splits = splits[1].split(":")[0].split("x");
if (splits.length != 2) {
utils.logWarn('AdSlot Error: adSlot not in required format');
Copy link
Collaborator

@pm-abhinav-sinha pm-abhinav-sinha Aug 16, 2018

Choose a reason for hiding this comment

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

Not specific to this changes but all messages should be of format:
utils.logWarn(BIDDER_CODE : Message)
Also it would be good to print the format here i.e
<adunit>@<width>x<height>

Copy link
Collaborator

@pm-abhinav-sinha pm-abhinav-sinha left a comment

Choose a reason for hiding this comment

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

Please ensure we add Unit test cases for this particular format of adunits as well

return;
slot = splits[0];
if (splits.length == 2) {
bid.params.adUnitIndex = splits[1].split(":")[1];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line sets adUnitIndex to undefined in case of bid = {params:{adSlot: 'abcd@10x80'}};
Default value for adUnitIndex is '0'


slot = splits[0];
if (splits.length == 2) {
bid.params.adUnitIndex = splits[1];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Complete unit test cases before QA drop.

@pm-abhinav-sinha
Copy link
Collaborator

Looks good.

@pm-shashank-jain pm-shashank-jain merged commit 0d11a7c into prebid_1.x_upgrade_gdpr_1.13 Aug 20, 2018
PubMatic-OpenWrap pushed a commit that referenced this pull request Jun 16, 2024
* sends refererInfo to kraken

* minor change

* removes comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants