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

Rubicon adapter: get referrer from bidderRequest.refererInfo.referer; #3087

Merged
merged 14 commits into from
Sep 28, 2018

Conversation

AntoineJac
Copy link
Contributor

Appnexus new module is passing the encoded url and sometimes Publisher are passing encoded url in the referrer parameter.
This fix is a protection in case a module is update and start passing encode url. Our AE is not decoding and from some test I did, monetisation could be highly impacted.

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

  • test parameters for validating bids
{
  bidder: '<bidder name>',
  params: {
    // ...
  }
}

Be sure to test the integration with your adserver using the Hello World sample page.

  • contact email of the adapter’s maintainer
  • official adapter submission

For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:

Other information

Antoine Jacquemin (Rubicon) added 2 commits September 13, 2018 16:25
Appnexus new module is passing the encoded url and sometimes Publisher are passing encoded url in the referrer parameter. 
This fix is a protection in case a module is update and start passing encode url. Our AE is not decoding and from some test I did, monetisation could be highly impacted.
@bretg bretg changed the title DecodeUrl if url is encoded Rubicon adapter: DecodeUrl if url is encoded Sep 13, 2018
@mkendall07
Copy link
Member

@AntoineJac
you have some test failure.

@AntoineJac
Copy link
Contributor Author

@mkendall07 , thanks I indeed forget to push the new specs.
@bretg , can you please review?

}
return bidRequest.params.secure ? pageUrl.replace(/^http:/i, 'https:') : pageUrl;
let _pageUrl = bidRequest.params.secure ? pageUrl.replace(/^http:/i, 'https:') : pageUrl;
return decodeURIComponent(_pageUrl.replace(/\+/g, ' '))
Copy link
Collaborator

Choose a reason for hiding this comment

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

the title of the PR says, "DecodeUrl if url is encoded". Do we need to check first if it's encoded?

Copy link
Member

Choose a reason for hiding this comment

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

question - should we update the referer module to return unencoded referer? I'm sort of on the fence on this but curious to get other's input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@harpere , I guess that if the url is decoded, decoding should not provoke any error.

@mkendall07 , I would indeed recommend returning decoding url as other partner may face some issues.

Copy link
Collaborator

@harpere harpere Sep 18, 2018

Choose a reason for hiding this comment

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

@AntoineJac - I think decoding again might only make a difference if there was a % in the referrer. But if we know the module is encoding, we're fine.

@mkendall07 - my impulse is that the module should NOT encoded referer and let the adapter encode it if they need to. Let us know if you decide to change it and we'll change our implementation. not a bid deal either way.

@bretg
Copy link
Collaborator

bretg commented Sep 19, 2018

Which 'Appnexus module' is generating this URL? Is this the new 'referer detection' PR #3067 ?

Assigning to @mkendall07 to make a call here - we'll abandon this PR if referer detection flips the change. But since that's a breaking change, am guessing a change at this point would be hard.

@mkendall07
Copy link
Member

mkendall07 commented Sep 19, 2018

@bretg thanks. yes we should not encode the referral in the referer detection module. @jaiminpanchal27 will make the change and you can update this PR as required or just close it.

CC:
@harpere @AntoineJac

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

Need to update the refer detection API first.

@bretg
Copy link
Collaborator

bretg commented Sep 26, 2018

Closing this -- resolved by #3116

@bretg bretg closed this Sep 26, 2018
@bretg
Copy link
Collaborator

bretg commented Sep 27, 2018

re-opening so @AntoineJac can update it to remove the decode part (and update title)

@bretg bretg reopened this Sep 27, 2018
@AntoineJac
Copy link
Contributor Author

@bretg , PR is ready for reviewing.
Thanks,

@harpere harpere changed the title Rubicon adapter: DecodeUrl if url is encoded Rubicon adapter: get referrer from bidderRequest.refererInfo.referer; Sep 28, 2018
@harpere harpere merged commit ee1cd7e into prebid:master Sep 28, 2018
ArmandChoy pushed a commit to RockYou-Ads/Prebid.js that referenced this pull request Sep 28, 2018
* 'master' of https://github.com/prebid/Prebid.js: (367 commits)
  Rubicon adapter: get referrer from bidderRequest.refererInfo.referer; (prebid#3087)
  Minor freewheel-ssp update (prebid#3119)
  fixes prebid#3128 YieldlabBidAdapter is not using bidRequest.params.adSize (prebid#3129)
  Support Video Renderer (prebid#3104)
  Fix for Issue 3130: passing new copy of adUnits object to every adapter (prebid#3131)
  Add video params to Beachfront adapter (prebid#3121)
  Sonobi - Fix ref encoding (prebid#3125)
  update circleci link to just Prebid.js builds (prebid#3132)
  Bugfix: Issue 3111 (prebid#3122)
  increment prebid version
  Prebid 1.25.0 Release
  adding account to s2s bidder-sync request (prebid#3123)
  Revert "Trafficroots Bid Adapter Submission (prebid#2993)" (prebid#3124)
  Trafficroots Bid Adapter Submission (prebid#2993)
  add versioning and deprecation policy doc (prebid#3103)
  improving kargo unit tests for currency handling (prebid#3106)
  AdOcean adapter improvment (prebid#3011)
  Serverbid Bid Adapter: Add pubnx alias (prebid#3064)
  Adds an id parameter (prebid#3107)
  added sizes for rubicon (prebid#3094)
  ...
pedrolopezmrf pushed a commit to Marfeel/Prebid.js that referenced this pull request Mar 18, 2019
…prebid#3087)

* DecodeUrl if url is encoded

Appnexus new module is passing the encoded url and sometimes Publisher are passing encoded url in the referrer parameter. 
This fix is a protection in case a module is update and start passing encode url. Our AE is not decoding and from some test I did, monetisation could be highly impacted.

* Update rubiconBidAdapter.js

* Update rubiconBidAdapter_spec.js

* Update rubiconBidAdapter_spec.js

* Update rubiconBidAdapter_spec.js

* Update rubiconBidAdapter_spec.js

* Update rubiconBidAdapter_spec.js

* Update rubiconBidAdapter_spec.js

* Update rubiconBidAdapter.js

* Update rubiconBidAdapter.js

* Update rubiconBidAdapter_spec.js

* Update rubiconBidAdapter.js

* Update rubiconBidAdapter_spec.js

* Update rubiconBidAdapter_spec.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants