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

Widespace bid adapter: support for the floors module #7006

Closed
wants to merge 8 commits into from

Conversation

talha08
Copy link
Contributor

@talha08 talha08 commented Jun 10, 2021

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

Widespace adapter: add support for adomain and floors module for Prebid 5.
#6465
#6650

@patmmccann patmmccann changed the title WIdespace adapter update for prebid 5 Widespace bid adapter: support for the floors module Jun 10, 2021
@patmmccann
Copy link
Collaborator

tyvm!

@talha08
Copy link
Contributor Author

talha08 commented Jun 10, 2021

I can see some test cases failed in other adapters. But here it shows, Your tests failed on CircleCI

I'm new here, What should I need to do? Please let me know!
@patmmccann

Copy link
Contributor

@Fawke Fawke left a comment

Choose a reason for hiding this comment

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

Hi @talha08,

Thanks for this PR. Change looks good but I had a few things which can be looked at.

  1. I don't see any unit test case covering the new getBidFloor function that you wrote.
  2. You need to submit a docs PR to change this field, Floors Module Support to yes.

Do not worry about the test cases for other adapters failing on cirle ci, I see that they're passing now. Some tests are flaky which can cause the circle ci build to fail.

Comment on lines +260 to +264
let bidFloor = bid.getFloor({
mediaType: '*',
size: '*',
currency: 'EUR'
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be wrapped in a try/catch block incase getFloor throws an error.

@Fawke
Copy link
Contributor

Fawke commented Jul 7, 2021

@talha08 Hey, just checking! I see no updates on this PR for quite a few days, do you need any help with this?

@Fawke
Copy link
Contributor

Fawke commented Sep 15, 2021

@ChrisHuie Do we wanna close this PR, I haven't seen any activity on this since two months.

@ChrisHuie ChrisHuie assigned ChrisHuie and unassigned Fawke Sep 29, 2021
@ChrisHuie
Copy link
Collaborator

@Fawke I will just change the assignee to me so you don't have to worry about it :) I will try and reach out.

@ChrisHuie
Copy link
Collaborator

@talha08 is this pr still being worked on? Haven't heard anything in a while and we are cleaning out stale prs. If I don't hear back by Thanksgiving I will just go ahead and close this out and you can reopen again later if needed.

@ChrisHuie ChrisHuie closed this Nov 23, 2021
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.

5 participants