Player integration for Freewheel video ad server #177

spra85 · 2016-10-11T01:23:05Z

Updating the query in revealed.js for the new Freewheel ad server.

Test Links

http://freewheel.test.avclub.com
http://freewheel.test.theonion.com
http://freewheel.test.clickhole.com

to test open javascript console in your browser, inside of the network tab, you can filter by XHR there should be a request to this url http://111976.v.fwmrm.net after you click play on a video

spra85 · 2016-10-11T01:25:03Z

elements/bulbs-video/components/revealed.js


+  vastUrl (videoMeta) {
+    let baseUrl = `http://${SETTINGS.FREEWHEEL_NETWORK_ID}v.fwmrm.net/ad/g/1?`;


@MelizzaP missing a . between the network id and the v. Example (http://1b656.v.fwmrm.net/ad/g/1?...)

spra85 · 2016-10-11T01:25:57Z

lib/bulbs-elements/settings.js

+  base: {
+    BULBS_ELEMENTS_ONIONSTUDIOS_GA_ID: 'UA-223393-14',
+    BULBS_ELEMENTS_COMSCORE_ID: '6036328',
+    RESP: 'vmap1',


I think we can just hardcode this instead of making it a setting, it'll never change.

spra85 · 2016-10-11T01:29:58Z

lib/bulbs-elements/settings.js

+    RESP: 'vmap1',
+  },
+  development: {
+    FREEWHEEL_NETWORK_ID: '111976',


I think these network IDs should actually be stored in each property's python settings and then passed into the the bulbs-element somehow, maybe even as a namespaced variable on window. I don't believe the NODE_ENV gives you what you need to integrate into each of the bulbs properties, but I'd want to @collin to weigh in.

my logic behind it was that they were the same for each site, so we wouldn't have to repeat them. And I just felt weird about putting them on the window.

So far we've been kicking the can down the road on some of these bulbs-elements settings.

I think this is a good step to have a module that exports settings.

Maybe we could add a settings object to the properties header (before bulbs-elements is loaded).

<script> window.BULBS_ELEMENTS_SETTINGS = {}; </script>

And in settings.js

if (window.BULBS_ELEMENTS_SETTINGS) { Object.assign(settings, window.BULBS_ELEMENTS_SETTINGS); }

Then we can decide where a setting should live on a case-by-case basis.

In the long-term I'd like to get bulbs-elements into omni and find some way to access the python settings at build time. Then we could have one settings file for everything.

But 1 settings.py location and 1 settings.js location beats having these settings strewn about the individual js files.

if the end goal is to have all the settings in one settings.py file and we are pushing these settings to window anyway. I'd argue, it might make sense to just put them in each sites' settings.py file. Having a settings.js and settings.py file just seems to add an unnecessary and possibly confusing extra layer of complication.

collin · 2016-10-11T15:18:26Z

elements/bulbs-video/components/revealed.js

-global.BULBS_ELEMENTS_ONIONSTUDIOS_GA_ID = 'UA-223393-14';
-global.BULBS_ELEMENTS_COMSCORE_ID = '6036328';
+global.BULBS_ELEMENTS_ONIONSTUDIOS_GA_ID = SETTINGS.BULBS_ELEMENTS_ONIONSTUDIOS_GA_ID;
+global.BULBS_ELEMENTS_COMSCORE_ID = SETTINGS.BULBS_ELEMENTS_COMSCORE_ID;


Rather than dump this on settings, I think we should just read it from SETTINGS every time we use these values.

spra85 · 2016-10-11T19:44:49Z

elements/bulbs-video/components/revealed.js

+  }
+
+  getCsidValue (videoMeta) {
+    let deviceAcronym = this.setDeviceAcronym();


Can we make this getDeviceAcronym to stay consistent with those other accessor functions?

I was trying to differentiate from values we are pulling in from other places the vs values we are setting.

but I suppose its really 6 of one half a dozen of the other

spra85 · 2016-10-11T19:46:21Z

elements/bulbs-video/components/revealed.js

+    }
+  }
+
+  getCsidValue (videoMeta) {


We should try to make the method name for this much more descriptive than getCsidValue because csid is sort of cryptic. Maybe getSiteSection?

spra85 · 2016-10-11T19:49:15Z

elements/bulbs-video/components/revealed.js

+  }
+
+  getSiteName () {
+    return window.location.host.split('.')[1];


We actually want this to be the target_channel set as dimension1 further up in Revealed.js. It's meant to be the channel code of the video you're playing, not necessarily the site it is playing on. For instance, if we're playing an labs video on The Onion, we wouldn't want the theonion passed up as the site name.

I'm not sure we actually need this method at all, since we should be able to pull this value direct from the video props

some videos have their target_channel set to onion-labs, do we want that value or should I just make it default to theonion?

…wheel-query

MelizzaP · 2016-10-12T18:43:03Z

Add required query string parameters to the VAST ad request call

spra85 · 2016-10-13T16:53:48Z

elements/bulbs-video/components/revealed.js

+    return dfpSection;
+  }
+
+  buildCsid (hostChannel) {


buildCustomSiteSectionId

spra85 · 2016-10-13T16:53:58Z

elements/bulbs-video/components/revealed.js

+    return `${deviceAcronym}.${siteName}_${siteSection}_${hostChannel}`;
+  }
+
+  buildCaid (videohubReferenceId) {


buildCustomContentVideoAssetId

spra85 · 2016-10-13T16:55:52Z

elements/bulbs-video/components/revealed.js

+    let campaignId = this.props.targetCampaignId;
+    let specialCoverage = this.props.targetSpecialCoverage;
+
+    let baseUrl = `http://${window.FREEWHEEL_NETWORK_ID}.v.fwmrm.net/ad/g/1?`;


This should be the freewheel network hash

spra85 · 2016-10-13T16:56:08Z

elements/bulbs-video/components/revealed.js

+
+    let baseUrl = `http://${window.FREEWHEEL_NETWORK_ID}.v.fwmrm.net/ad/g/1?`;
+
+    // required global params


We are missing the nw param, which should be window.FREEWHEEL_NETWORK_ID

spra85 · 2016-10-13T16:59:07Z

elements/bulbs-video/components/revealed.js

+    baseUrl += '&vprn=' + randomVideoPlayerNumber;
+
+    // optional global param
+    if (vastTestId) { baseUrl += '&cana=' + vastTestId; }


Let's modify this line in vastTest:

let vastId = this.parseParam('xgid', searchString);

So we make the parameter (which is ad placement id), let's call it apid

spra85 · 2016-10-13T17:01:43Z

elements/bulbs-video/components/revealed.js

+    if (vastTestId) { baseUrl += '&cana=' + vastTestId; }
+
+    // Key Values
+    baseUrl += ';&video_id=' + videohubReferenceId;


We shouldn't need the ampersand after the semi-colon, so ;&video_id becomes ;video_id

spra85 · 2016-10-13T17:02:03Z

elements/bulbs-video/components/revealed.js

+    if (specialCoverage) { baseUrl += '&special_coverage=' + specialCoverage; }
+
+    // Slot Params *Required Fields*
+    baseUrl += ';&slid=' + 'Preroll';


We shouldn't need the ampersand after the semi-colon, so ;&slid becomes ;slid

…id" and add nw parameter to vastUrl

… contain FREEWHEEL_NETWORK_ID and FREEWHEEL_NETWORK_HASH properties

spra85 · 2016-10-14T16:57:43Z

elements/bulbs-video/components/revealed.js

+  }
+
+  buildCustomSiteSectionId (hostChannel) {
+    // format: <device acronym>.<site name>_<dfp section>_<host channel>


I think we can remove this comment, because that format is clear in the interpolation below

spra85 · 2016-10-14T16:57:47Z

elements/bulbs-video/components/revealed.js

+  }
+
+  buildCustomContentVideoAssetId (videohubReferenceId) {
+    // format: onion_<videohub reference id>


I think we can remove this comment, because that format is clear in the interpolation below

MelizzaP added 3 commits October 10, 2016 14:55

wip

cc9115f

update specs

fa3aeeb

add getProfValue function

57f4d6c

spra85 commented Oct 11, 2016

View reviewed changes

remove resp from settings update freewheel netid

01e1e8f

collin reviewed Oct 11, 2016

View reviewed changes

add csid logic

67940ab

MelizzaP changed the title ~~(IN PROGRESS) Player integration for Freewheel video ad server~~ Player integration for Freewheel video ad server Oct 11, 2016

MelizzaP added the Work in Progress label Oct 11, 2016

spra85 commented Oct 11, 2016

View reviewed changes

MelizzaP added 3 commits October 11, 2016 15:13

pull freewheel network id from sites settings.py file

80ff21e

refactor, rename variables

b5e393b

add caid

1889522

MelizzaP force-pushed the freewheel-query branch from 6f04999 to 8bfd453 Compare October 11, 2016 21:13

cleanup variable names and delete uneeded tests and code

74f7633

MelizzaP force-pushed the freewheel-query branch from 8bfd453 to 74f7633 Compare October 11, 2016 21:26

MelizzaP added 7 commits October 11, 2016 16:34

fix broken specs

12d2e85

add vprn

54018d8

add cana value

c7c7150

add key values

2d9ea9a

add slot params

16c0d31

linter fixes

de5e7a2

get site name from targeting channel

197df85

MelizzaP force-pushed the freewheel-query branch from 67f02b0 to 197df85 Compare October 12, 2016 17:07

MelizzaP added 2 commits October 12, 2016 12:31

Merge branch 'master' of github.com:theonion/bulbs-elements into free…

b6f5c0c

…wheel-query

make tests safari friendly

b716fdd

MelizzaP added Ready for Review and removed Work in Progress labels Oct 12, 2016

spra85 commented Oct 13, 2016

View reviewed changes

elements/bulbs-video/components/revealed.js

return dfpSection;

}

buildCsid (hostChannel) {

Copy link

Contributor Author

spra85 Oct 13, 2016

Choose a reason for hiding this comment

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

buildCustomSiteSectionId

spra85 commented Oct 13, 2016

View reviewed changes

MelizzaP added 3 commits October 13, 2016 15:02

update variable names, replace xgid property with apid "ad placement …

e8acadb

…id" and add nw parameter to vastUrl

fix linting errors

aeca9b9

python and js objects werent playing well together, changed window to…

4f97e9b

… contain FREEWHEEL_NETWORK_ID and FREEWHEEL_NETWORK_HASH properties

spra85 commented Oct 14, 2016

View reviewed changes

rm comments

fe7e831

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Player integration for Freewheel video ad server #177

Player integration for Freewheel video ad server #177

spra85 commented Oct 11, 2016 •

edited by MelizzaP

Loading

spra85 Oct 11, 2016

spra85 Oct 11, 2016

spra85 Oct 11, 2016

MelizzaP Oct 11, 2016

collin Oct 11, 2016

collin Oct 11, 2016

MelizzaP Oct 11, 2016 •

edited

Loading

collin Oct 11, 2016

collin Oct 11, 2016

spra85 Oct 11, 2016

MelizzaP Oct 11, 2016

MelizzaP Oct 11, 2016

spra85 Oct 11, 2016

spra85 Oct 11, 2016 •

edited

Loading

MelizzaP Oct 12, 2016 •

edited

Loading

MelizzaP commented Oct 12, 2016

spra85 Oct 13, 2016

spra85 Oct 13, 2016

spra85 Oct 13, 2016

spra85 Oct 13, 2016

spra85 Oct 13, 2016

spra85 Oct 13, 2016

spra85 Oct 13, 2016

spra85 Oct 14, 2016

spra85 Oct 14, 2016


		vastUrl (videoMeta) {
		let baseUrl = `http://${SETTINGS.FREEWHEEL_NETWORK_ID}v.fwmrm.net/ad/g/1?`;


		let baseUrl = `http://${window.FREEWHEEL_NETWORK_ID}.v.fwmrm.net/ad/g/1?`;

		// required global params

Player integration for Freewheel video ad server #177

Are you sure you want to change the base?

Player integration for Freewheel video ad server #177

Conversation

spra85 commented Oct 11, 2016 • edited by MelizzaP Loading

Test Links

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MelizzaP Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spra85 Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

MelizzaP Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

MelizzaP commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spra85 commented Oct 11, 2016 •

edited by MelizzaP

Loading

MelizzaP Oct 11, 2016 •

edited

Loading

spra85 Oct 11, 2016 •

edited

Loading

MelizzaP Oct 12, 2016 •

edited

Loading