Rvr 2369 Refactor events handling (#9) #3683
Conversation
* RVR-2177 - Refactor events handling * RVR-2087 - Inject pbjsGlobalVariable into rivraddon * RVR-2087 - update adapterManager dependency * RVR-2087 - Add ADD_AD_UNITS to Prebid.JS trackable events * RVR-2369 - Update package-lock.json
|
On circle-ci tests pass but the job times out. From circle-ci: |
|
@AlessandroDG It looks like the CircleCI job is running in a different instance than the This sometimes happens when the submitter has their own setup with circleCI, To my knowledge, we haven't found a good fix for this situation. I suppose in this case the reviewer will just have to run the tests locally to verify everything's working fine. |
|
@jsnellbaker thanks for getting back so quickly. I am of course available in case you need me to take any actions. |
There was a problem hiding this comment.
@AlessandroDG I ran the browserstack tests locally and everything passed successfully.
However while looking over the changes, I saw a few things that needed to be reviewed. Please take a look at the items below when you have the chance.
Additionally, do you happen to have any test credentials/information available that I could use to test out the updates to your Analytics Adapter?
Thanks!
modules/rivrAnalyticsAdapter.js
Outdated
| import adapterManager from '../src/adapterManager'; | ||
| import * as utils from '../src/utils'; | ||
| import * as utils from 'src/utils'; |
There was a problem hiding this comment.
Please reinstate the relative import paths; this style is needed for #3435
src/AnalyticsAdapter.js
Outdated
| @@ -16,7 +16,8 @@ const { | |||
| BID_ADJUSTMENT, | |||
| BIDDER_DONE, | |||
| SET_TARGETING, | |||
| AD_RENDER_FAILED | |||
| AD_RENDER_FAILED, | |||
| ADD_AD_UNITS | |||
There was a problem hiding this comment.
Can you clarify on the changes in this file?
If a new analytics event is needed, that should be part of a separate PR (since it's a core change to the module, rather than just your adapter).
There was a problem hiding this comment.
Sure,
The event addAdUnits helps us detect any change on the adUnits object between the moment a new value is assigned to it and any other moment in the flow.
We are currently handling it at a lower level with:
pbjsGlobal.onEvent('addAdUnits', (data) => { myHandler(data); });
but I understood that the events we get from it are not considered a stable interface.
I will create a separate PR fo it.
There was a problem hiding this comment.
@jsnellbaker I updated this PR. Now it is only about rivrAnalyticsAdapter.
There was a problem hiding this comment.
@jsnellbaker PR for changes in core have been moved to #3691
## Type of change - [x] Refactoring (no functional changes, no api changes) ## Description of change Refactor rivrAnalyticsAdapter.js events handling. ## History * RVR-2087 - update adapterManager dependency * RVR-2369 - Update package-lock.json * RVR-2369 - Revert changes in main Analytics adapter It will be handled in a separate PR * RVR-2369 - Use relative import paths Needed for prebid#3435
Handled in separate PR prebid#3683
|
@AlessandroDG This PR looks good. Do you have a preference though when this merged in lieu of #3691? Can this go separate, or do you want the other item merged first before updating your adapter? |
|
@jsnellbaker thanks for asking. |
* RVR-2177 - Refactor events handling * RVR-2087 - Inject pbjsGlobalVariable into rivraddon * RVR-2087 - update adapterManager dependency * RVR-2087 - Add ADD_AD_UNITS to Prebid.JS trackable events * RVR-2369 - Update package-lock.json * RVR-2369 - Revert rivrAnalyticsAdapter changed Handled in separate PR #3683 * RVR-2369 - Add REQUEST_BIDS to trackable events
Type of change
Description of change
Refactor rivrAnalyticsAdapter.js events handling.
Add ADD_AD_UNITS to trackable events.