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

Admedia usersync #1923

Closed
wants to merge 4 commits into from
Closed

Conversation

devmusings
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

Added usersync to Admedia's bidder.

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

Other information

Suggested updates per #1864

@devmusings devmusings mentioned this pull request Dec 6, 2017
9 tasks
@harpere
Copy link
Collaborator

harpere commented Dec 6, 2017

unit tests should be added for the change.

@harpere harpere self-requested a review December 6, 2017 19:01
@@ -67,6 +69,16 @@ var AdmediaAdapter = function AdmediaAdapter() {

adloader.loadScript(endpoint);
}

if (userSync.iframeEnabled) {
userSync.registerSync('iframe', 'admedia', '//b.admedia.com/sync/iframe/');
Copy link
Collaborator

Choose a reason for hiding this comment

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

userSync.iframeEnabled does not exist. You'd have to look at the config directly, like config.getConfig('userSync');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use config.

@harpere harpere self-assigned this Dec 6, 2017
@devmusings
Copy link
Contributor Author

Using config object and added unit tests.

@harpere
Copy link
Collaborator

harpere commented Dec 8, 2017

@devmusings looks like there's a conflict in admediaBidAdapter.js that needs to be resolved.

});

it('should call usersync after default 3000 seconds', () => {
window.setTimeout(function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

A couple of problems with the setTimeout here.

  1. you're not using an async callback (usually called done) to notify the framework the test is finished.
  2. if this works, its adding a 3 second delay every time the tests are run.

I think a better approach would be to test that your usersync.registerSync() calls are happening as expected and let the usersync module tests validate that they are triggered properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Updated the tests.

@devmusings
Copy link
Contributor Author

@harpere Looks like the file modules/admediaBidAdapter.js has been removed in #1936 which is causing the conflict.

@mkendall07
Copy link
Member

@devmusings
The adapter was removed because it doesn't comply with our new prebid 1.0 adapter requirements. You should review http://prebid.org/dev-docs/bidder-adaptor.html and submit a new new adapter that meets the requirements. I'm going to close this as it's unmergeable now.

@mkendall07 mkendall07 closed this Jan 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants