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

Re-implemented RhythmOne audit beacon in prebid 1.0 interface #1953

Merged
merged 3 commits into from
Jan 10, 2018

Conversation

jstocker76
Copy link
Contributor

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
  • Other

Description of change

Re-implemented our fraud audit beacon in the 1.0 adapter. This beacon was present in the previous versions of rhythm one's bidder, but was removed for the sake of expediting the 1.0 version changes.

  • test parameters for validating bids
    {
      bidder: 'rhythmone',
      params: 
      { 
        placementId: '411806',
        endpoint: "//tag.1rx.io/rmp/72721/0/mvo?z=1r" // only required for testing.  this api guarantees no 204 responses
      }
    }

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

Other information

@mike-chowla
Copy link
Contributor

Test coverage is under 80%:
screen shot 2017-12-14 at 3 27 07 pm

@mike-chowla
Copy link
Contributor

The code in the pull request uses $$PREBID_GLOBAL$$

Copy link
Contributor

@mike-chowla mike-chowla left a comment

Choose a reason for hiding this comment

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

Code coverage is below the 80% requirement

$$PREBID_GLOBAL$$ should not be used.

@jstocker76
Copy link
Contributor Author

is there another way I can get the version of prebid without using $$PREBID_GLOBAL$$?

@mike-chowla
Copy link
Contributor

For the version, you can have it substituted by webpack at build time using '$prebid.version$'

@mike-chowla
Copy link
Contributor

The earlier issues are fixed but I noticed something new:

When syncOptions.pixelEnabled is off, the code goes and creates its own image to fire off a sync. If the publisher has disabled syncs, no sync should be done.

@jstocker76
Copy link
Contributor Author

jstocker76 commented Jan 5, 2018

The purpose of this beacon is to detect fraudulent use of the adapter. If it can be easily disabled, it has no point.

What is the meaning of "user sync" in this context? This beacon doesn't drop any tracking cookies.

@mike-chowla
Copy link
Contributor

So my understanding is that the adapters are not supposed to trigger beacons except as a user syncs. @mkendall07 , you can take a look to see if this is permitted or not? Line 66 in the adapter

@mkendall07
Copy link
Member

@mike-chowla
correct. @jstocker76
it looks like the code on line 66 is unnecessary and can be dropped.

@jstocker76
Copy link
Contributor Author

dropped

@jstocker76
Copy link
Contributor Author

the details of that check failure seem to be unrelated to my changes...

@mike-chowla
Copy link
Contributor

Yeah, I can not see how they would be related. Tests past on my machine.

Copy link
Contributor

@mike-chowla mike-chowla left a comment

Choose a reason for hiding this comment

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

LGTM

Only note is the tests show as failing. However, the output of the test failures is unrelated to the final change. Tests pass on my machine.

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.

lgtm

@mkendall07 mkendall07 merged commit 3502919 into prebid:master Jan 10, 2018
chanand pushed a commit to chanand/Prebid.js that referenced this pull request Jan 18, 2018
…#1953)

* Re-implemented RhythmOne audit beacon in prebid 1.0 interface

* removed references to prebid_global and added more unit test coverage

* removed block that sends beacons when usersync is disabled
Millerrok pushed a commit to Vertamedia/Prebid.js that referenced this pull request Jan 23, 2018
* 'master' of https://github.com/prebid/Prebid.js: (23 commits)
  Update Atomx adapter for Prebid v1.0 (prebid#2026)
  Add vi bid adapter (prebid#2020)
  Add eplanningBidAdapter (prebid#2003)
  OpenX Adapter: Update to support mediaTypes field, instead of the deprecated mediaType field (prebid#1974)
  Separate bids & won calls (prebid#2015)
  1.0 adapter support for mantis (prebid#1840)
  Media.net adapter added (prebid#2038)
  GumGum Adapter for Prebid.js 1.0 (prebid#1966)
  Serverbid Bid Adapter: updated docs and ad sizes (prebid#2023)
  Adding districtm as an alias (prebid#2018)
  Use sudo to workaround Travis regression (prebid#2041)
  Fix uncached video bids triggering callback early (prebid#2017)
  Re-implemented RhythmOne audit beacon in prebid 1.0 interface (prebid#1953)
  Add NasmediaAdmixer adapter for Perbid.js 1.0 (prebid#1937)
  Update Adform adapter to Prebid v1.0 (prebid#1947)
  Upgrade Admixer adapter for Prebid 1.0 (prebid#1755)
  multiformat size validation checks (prebid#1964)
  Gjirafa Bidder Adapter (prebid#1944)
  pin gulp-connect at non-broken version (prebid#2008)
  Added dynamic ttl property for One Display and One Mobile. (prebid#2004)
  ...
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
…#1953)

* Re-implemented RhythmOne audit beacon in prebid 1.0 interface

* removed references to prebid_global and added more unit test coverage

* removed block that sends beacons when usersync is disabled
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.

4 participants