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

Rhythmone Adapter - Remove usersync, devicetype, and bad banners #3927

Merged
merged 1 commit into from Jul 19, 2019
Merged

Rhythmone Adapter - Remove usersync, devicetype, and bad banners #3927

merged 1 commit into from Jul 19, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jun 18, 2019

Type of change

  • Bugfix
  • Other: Feature removal

Description of change

  • Removes the usersync pixel. We don't need it.
  • Removes the devicetype property. We don't use it.
  • Does not send incorrectly specified banner imps.

@ghost
Copy link
Author

ghost commented Jun 18, 2019

Hmm, rubicon's got failing tests? That failure is not related to this PR.

Copy link
Contributor

@sumit116 sumit116 left a comment

Choose a reason for hiding this comment

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

Hi,
I am getting 204 response while hitting http://tag.1rx.io/rmp/80184/0/mvo?z=1r&hbv=2.21.0-pre,2.0.1 url. I am making a request from India. Can you check if you need to provide me any access to the content so that I can test your changes.

@@ -92,15 +36,21 @@ function RhythmOneBidAdapter() {
impObj.secure = win.location.protocol === 'https:' ? 1 : 0;

if (utils.deepAccess(BRs[i], 'mediaTypes.banner') || utils.deepAccess(BRs[i], 'mediaType') === 'banner') {
impObj.banner = frameBanner(BRs[i]);
let banner = frameBanner(BRs[i]);
if (banner !== false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we simplify it by if(banner)

Copy link
Author

Choose a reason for hiding this comment

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

Done

@ghost
Copy link
Author

ghost commented Jul 8, 2019

OK, the comment's been addressed. We've configured our system to consistently return ads. @sumit116 could you try again? The ci error above is unrelated to this PR.

@sumit116 sumit116 self-requested a review July 15, 2019 11:01
Copy link
Contributor

@sumit116 sumit116 left a comment

Choose a reason for hiding this comment

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

I could not get the response yet. Getting 204 No content in response. Approving the PR based on the request object passed.

@sumit116 sumit116 requested a review from jsnellbaker July 15, 2019 11:04
@sumit116 sumit116 added the needs 2nd review Core module updates require two approvals from the core team label Jul 15, 2019
@jsnellbaker jsnellbaker merged commit 1b331f3 into prebid:master Jul 19, 2019
leonardlabat pushed a commit to criteo-forks/Prebid.js that referenced this pull request Jul 30, 2019
VideoReach pushed a commit to VideoReach/Prebid.js that referenced this pull request Aug 1, 2019
sa1omon pushed a commit to gamoshi/Prebid.js that referenced this pull request Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs update needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants