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

Improvements for browsi RTD provider #4598

Merged
merged 30 commits into from
Feb 12, 2020
Merged

Improvements for browsi RTD provider #4598

merged 30 commits into from
Feb 12, 2020

Conversation

omerDotan
Copy link
Contributor

Type of change

  • Bugfix
  • Other

Description of change

  1. Improvements for browsi RTD provider (support for placement id macros, timeout for ajax request)
  2. Change bidsBackCallback hook data (adUnits instead of adUnits codes)
  3. Add timeout param (used when we don't need auction dealy)

@omerDotan omerDotan changed the title Rtd 1.1 Improvements for browsi RTD provider Dec 15, 2019
@omerDotan
Copy link
Contributor Author

updated docs pull request -
prebid/prebid.github.io#1690

@jsnellbaker jsnellbaker self-requested a review December 16, 2019 14:18
@jsnellbaker jsnellbaker self-assigned this Dec 16, 2019
Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

Hi @omerBrowsi
When you have the chance, can you take a look at the comment below?

modules/browsiRtdProvider.js Outdated Show resolved Hide resolved
modules/browsiRtdProvider.js Show resolved Hide resolved
@stale
Copy link

stale bot commented Jan 1, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 1, 2020
@jsnellbaker jsnellbaker removed the stale label Jan 2, 2020
src/auction.js Show resolved Hide resolved
@bretg
Copy link
Collaborator

bretg commented Jan 2, 2020

Admittedly I didn't do as much digging as I should have, but seems like this item would have affected more places. Can you please provide a bit of background for why this change is proposed?

Change bidsBackCallback hook data (adUnits instead of adUnits codes)

@jsnellbaker
Copy link
Collaborator

@omerBrowsi When you have the chance, can you resolve the conflicts here?

# Conflicts:
#	modules/browsiRtdProvider.js
@omerDotan
Copy link
Contributor Author

@jsnellbaker
we prefer not using the loadExternalScriptfunction
we need to set attributes and objects on our script and the function doesn't support it

@snapwich - the code was changed by you, is it critical that we use loadExternalScript?
if so, we will need to adjust this function
(code was changed on this pull request - #4687)

@snapwich
Copy link
Collaborator

@omerBrowsi yes it is required; your module was one of many that were making live requests during runs of the test suite in CI; that's not ideal for obvious reasons. Rather than verify everyone is properly stubbing it's much easier for us to just mandate use of the loadExternalScript when you load an external script (something that should happen rarely to begin with) and have loadExternalScript stub.

How do you need to adjust the function? I did make changes in my pull-request to accommodate your use-case: loadExternalScript now returns the script tag which can then be modified however you would like.

@omerDotan
Copy link
Contributor Author

omerDotan commented Jan 29, 2020

@snapwich @jsnellbaker @bretg
changed to use loadExternalScript
I didn't make any changes to loadExternalScript function, we will handle the difference on our side.
Update the macro to support new configs on our side

@omerDotan
Copy link
Contributor Author

@bretg @jsnellbaker any update on this? do you need anything from me?

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

Hi @omerBrowsi

I took another look again at the changes, and I think overall things look okay. There were two items I found that would need some feedback/update.

Can you please take a look at these items when you have the chance?

Thanks.

modules/browsiRtdProvider.js Outdated Show resolved Hide resolved
modules/browsiRtdProvider.js Show resolved Hide resolved
@jsnellbaker jsnellbaker assigned bretg and unassigned jsnellbaker Feb 12, 2020
@bretg bretg merged commit 720038e into prebid:master Feb 12, 2020
@omerDotan
Copy link
Contributor Author

@bretg @jsnellbaker Thanks!
@bretg the real-time data (and browsi) module does not appear on the download page, is there anything we need to do on our side?

hellsingblack pushed a commit to SublimeSkinz/Prebid.js that referenced this pull request Mar 5, 2020
* real time data module,
browsi sub module for real time data,
new hook bidsBackCallback,
fix for config unsubscribe

* change timeout&primary ad server only to auctionDelay
update docs

* support multiple providers

* change promise to callbacks
configure submodule on submodules.json

* bug fixes

* use Prebid ajax

* tests fix

* browsi real time data provider improvements

* real time data module,
browsi sub module for real time data,
new hook bidsBackCallback,
fix for config unsubscribe

* change timeout&primary ad server only to auctionDelay
update docs

* support multiple providers

* change promise to callbacks
configure submodule on submodules.json

* bug fixes

* use Prebid ajax

* tests fix

* browsi real time data provider improvements

* remove eval

* use external script, update macro function

* lint errors

* removed log
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants