-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Browsi RTD Module: add pageview billable event #9207
Merged
robertrmartinez
merged 72 commits into
prebid:master
from
omerDotan:pageview-billable-event
Nov 21, 2022
Merged
Changes from all commits
Commits
Show all changes
72 commits
Select commit
Hold shift + click to select a range
6121b4e
real time data module,
omerDotan 1a80b14
change timeout&primary ad server only to auctionDelay
omerDotan 3b85815
support multiple providers
omerDotan 0cb7b69
change promise to callbacks
omerDotan 0908134
bug fixes
omerDotan cf4c5a9
use Prebid ajax
omerDotan 7beeee3
tests fix
omerDotan 60aaeaa
browsi real time data provider improvements
omerDotan 0e06e6f
real time data module,
omerDotan e9312c7
change timeout&primary ad server only to auctionDelay
omerDotan c0901fe
support multiple providers
omerDotan 398f922
change promise to callbacks
omerDotan b3d0bea
bug fixes
omerDotan a4f2de6
use Prebid ajax
omerDotan 65ed991
tests fix
omerDotan 15337d2
browsi real time data provider improvements
omerDotan 76b3208
Merge remote-tracking branch 'origin/master'
omerDotan 6a8c111
Merge remote-tracking branch 'upstream/master'
omerDotan b9b05f0
Merge remote-tracking branch 'upstream/master'
omerDotan 89013d7
Merge remote-tracking branch 'upstream/master'
omerDotan 342484c
Merge remote-tracking branch 'upstream/master'
omerDotan faa02bf
Merge remote-tracking branch 'upstream/master'
omerDotan 88430b1
Merge remote-tracking branch 'upstream/master'
omerDotan b73b6d7
Merge remote-tracking branch 'upstream/master'
omerDotan 00c027c
Merge remote-tracking branch 'upstream/master'
omerDotan 51555b6
Merge remote-tracking branch 'upstream/master'
omerDotan c959997
Merge remote-tracking branch 'upstream/master'
omerDotan a277d65
Merge remote-tracking branch 'upstream/master'
omerDotan ebf1fa8
Merge remote-tracking branch 'upstream/master'
omerDotan 6ec752e
Merge remote-tracking branch 'upstream/master'
omerDotan 6af3494
Merge remote-tracking branch 'upstream/master'
omerDotan f0d9bb7
Merge remote-tracking branch 'upstream/master'
omerDotan 608eabf
Merge remote-tracking branch 'upstream/master'
omerDotan 6bdd935
Merge remote-tracking branch 'upstream/master'
omerDotan ca43a02
Merge remote-tracking branch 'upstream/master'
omerDotan 8346da4
Merge remote-tracking branch 'upstream/master'
omerDotan 1f5a82e
Merge remote-tracking branch 'upstream/master'
omerDotan 1c0d024
Merge remote-tracking branch 'upstream/master'
omerDotan 6c7cac7
Merge remote-tracking branch 'upstream/master'
omerDotan 1543516
Merge remote-tracking branch 'upstream/master'
omerDotan efe5ddf
Merge remote-tracking branch 'upstream/master'
omerDotan b404b69
Merge remote-tracking branch 'upstream/master'
omerDotan 107f00c
Merge remote-tracking branch 'upstream/master'
omerDotan cea0004
Merge remote-tracking branch 'upstream/master'
omerDotan 2dcd7af
Merge remote-tracking branch 'upstream/master'
omerDotan 86d4cf7
Merge remote-tracking branch 'upstream/master'
omerDotan 191056f
Merge remote-tracking branch 'upstream/master'
omerDotan 97c94b2
Merge remote-tracking branch 'upstream/master'
omerDotan ede3d7f
Merge remote-tracking branch 'upstream/master'
omerDotan e435be5
Merge remote-tracking branch 'upstream/master'
omerDotan 6a2293e
Merge remote-tracking branch 'upstream/master'
omerDotan f918fb8
Merge remote-tracking branch 'upstream/master'
omerDotan 60d8c5c
Merge remote-tracking branch 'upstream/master'
omerDotan a1f37fd
Merge remote-tracking branch 'upstream/master'
omerDotan 6dd4ea3
Merge remote-tracking branch 'upstream/master'
omerDotan 66e782e
Merge remote-tracking branch 'upstream/master'
omerDotan 2f16e37
Merge remote-tracking branch 'upstream/master'
omerDotan 1c577d8
Merge remote-tracking branch 'upstream/master'
omerDotan a52ac36
Merge remote-tracking branch 'upstream/master'
omerDotan 3505c14
Merge remote-tracking branch 'upstream/master'
omerDotan 1a6da04
Merge remote-tracking branch 'upstream/master'
omerDotan d5f83b5
Merge remote-tracking branch 'upstream/master'
omerDotan 02c5eae
Merge remote-tracking branch 'upstream/master'
omerDotan 3fca8ef
Merge remote-tracking branch 'upstream/master'
omerDotan 5ccd7db
Merge remote-tracking branch 'upstream/master'
omerDotan 9684d53
Merge remote-tracking branch 'upstream/master'
omerDotan 0f7ff71
Merge remote-tracking branch 'upstream/master'
omerDotan 42eae36
Merge remote-tracking branch 'upstream/master'
omerDotan 074a392
Merge remote-tracking branch 'upstream/master'
omerDotan ee3a43c
Merge remote-tracking branch 'upstream/master'
omerDotan f58a3f6
Merge remote-tracking branch 'upstream/master'
omerDotan ffed007
fire billable event according to event listener
omerDotan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so every time a call to browsi endpoint is called
getPredictionsFromServer
it will add this event listener again.So possible this means it can add several listeners in a given page load and then emit a billing event for each time it added.
I think a flag here to ensure this event listener is only set once is the right thing to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add a flag if you think we need extra validation. but
getPredictionsFromServer
is only called once per instanceThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok did not know that.
What if multiple prebid instances with browsi are added to a page?
Such as an iframe setup where each wrapper is in its own iframe. Do we want to have each instance add an event listener?
Probably fine I just wanted to double check.
Sounds like this will add an event listener any time a wrapper is loaded. Which probably is the right idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very rare case (not only do we need to have a few instances on the page, we need them to use the Browsi RTD provider), but if it happens there should be a pageview event per wrapper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robertrmartinez is there anything you need from me to approve the PR?