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

remove superfluous parameter from openx adapter #1237

Merged
merged 1 commit into from
Jun 5, 2017

Conversation

kkharma
Copy link
Contributor

@kkharma kkharma commented May 25, 2017

Type of change

  • [X ] Bugfix

Description of change

removed unused ee parameter from openx adapter

@jaiminpanchal27 jaiminpanchal27 self-assigned this May 30, 2017
@jaiminpanchal27 jaiminpanchal27 self-requested a review June 1, 2017 20:33
Copy link
Collaborator

@jaiminpanchal27 jaiminpanchal27 left a comment

Choose a reason for hiding this comment

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

LGTM

@dbemiller
Copy link
Contributor

@prebid/openx Is this guy from your team? Do you trust these changes?

No offense meant, but... without a link to a public API doc, someone we know to be connected to OpenX vetting you, or bidder params which we can test with, I'm not super comfortable merging this.

The Prebid core team tries to leave adapter maintenance up to the adapter's owners... but if we blindly approve changes like these, I can imagine malicious contributors sabotaging their competitor's adapter without their knowledge.

Would like feedback on how active others think we should be in this process.

@kkharma
Copy link
Contributor Author

kkharma commented Jun 5, 2017

no worries... i work for openx. is there someone from openx you've worked with in the past on adapter changes? i can reach out to them. i'm not sure why this parameter was ever set in the adapter in the first place.

@dbemiller
Copy link
Contributor

I'm new to prebid too... but it looks like the prebid/openx group includes @Heray, who I don't recognize... so he'd be my best guess.

@kkharma
Copy link
Contributor Author

kkharma commented Jun 5, 2017

unfortunately, they are not an openx employee, and i can't see the prebid/openx group. is there anyone else i could try? otherwise, i'll have to ask around internally.

@dbemiller
Copy link
Contributor

dbemiller commented Jun 5, 2017

huh... the git logs show that this file was in the initial commit by @mkendall, who's on vacation for the next week... so I'm not really sure where to go from here.

Are there OpenX API docs that this adapter is calling? I googled and found this page mentioned the URL http://delivery_server_domain/w/1.0/jstag, which looks like it's what the adapter hits.

But if that's the one, then the API params barely line up with the adapter at all. ju, jr, etc... many of them aren't listed there. So I get the feeling I'm missing something.

@dbemiller
Copy link
Contributor

Actually, although he's not a part of that group, @lntho shows up in the commit logs for #896 , official OpenX support

@kkharma
Copy link
Contributor Author

kkharma commented Jun 5, 2017

ah cool... that's someone i recognize. will follow up...

@lntho
Copy link
Contributor

lntho commented Jun 5, 2017

Hello @dbemiller I can confirm this pull request is fine. Is there supposed to be a prebid/openx group? I was the original submitter for the OpenX adapter

@dbemiller
Copy link
Contributor

Ok, thanks @lntho. I'll merge this then.

I do see a bunch of prebid/Adapter teams in this project... but only about 25 of them--so far fewer than the total number of adapters. A prebid/openx team does exist... but my guess is it's not really maintained. It only includes some original members of the Prebid project.

This is the first I've seen of these, and I don't have permissions to edit them... so I've no idea if they're maintained. I'll ping the people who do, though, and make sure you guys get added.

@dbemiller dbemiller merged commit ece853a into prebid:master Jun 5, 2017
jbAdyoulike pushed a commit to jbAdyoulike/Prebid.js that referenced this pull request Sep 21, 2017
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
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