tbuchok / vast-xml

A Node module for creating real-time VAST XML responses.
48 stars 27 forks source link

Implement VAST with Error when there are no Ads #18

Closed lukaszzdanikowski closed 10 years ago

tbuchok commented 10 years ago

thanks for the PR @lukaszzdanikowski!

can you please add a unit test for this new rendering? without ensuring the changes work within the VAST XSD, i can't merge the changes straight away. see below:


at first glance, i'm not certain that this is how the Error element works.

Error can be added to the Ad of the VAST response and that URI is used by the player whenever an error occurs. it could be for "no ad present", but also for anything else.

http://www.iab.net/media/file/VASTv3.0.pdf (pages 52-52)

perhaps consider widening the scope of your changes and allow for an Error attribute when attaching an ad, i.e.:

var vast = new VAST();
var ad = vast.attachAd({ 
      id : 1
    , structure : 'inline'
    , sequence : 99
    , AdTitle : 'Common name of the ad'
    , AdSystem : { name: 'Test Ad Server', version : '1.0' }
    , Error: 'http://example.com/stderr?code=[ERRORCODE]'
  });

let me know thoughts. looking forward to your response.

lukaszzdanikowski commented 10 years ago

Hi. OK. I will add unit test for this.

Error on the Ad level has different purpose, my implementation refers to point 2.4.2.4 of specification (page 56)

2014-02-13 15:01 GMT+01:00 Tom Buchok notifications@github.com:

thanks for the PR @lukaszzdanikowskihttps://github.com/lukaszzdanikowski !

can you please add a unit test for this new rendering? without ensuring the changes work within the VAST XSD, i can't merge the changes straight

away. see below:

at first glance, i'm not certain that this is how the Error element works.

Error can be added to the Ad of the VAST response and that URI is used by the player whenever an error occurs. it could be for "no ad present", but also for anything else.

http://www.iab.net/media/file/VASTv3.0.pdf (pages 52-52)

perhaps consider widening the scope of your changes and allow for an Errorattribute when attaching an ad, i.e.:

var vast = new VAST(); var ad = vast.attachAd({ id : 1 , structure : 'inline' , sequence : 99 , AdTitle : 'Common name of the ad' , AdSystem : { name: 'Test Ad Server', version : '1.0' } , Error: 'http://example.com/stderr?code=[ERRORCODE]' });

let me know thoughts. looking forward to your response.

— Reply to this email directly or view it on GitHubhttps://github.com/tbuchok/vast-xml/pull/18#issuecomment-34980083 .

tbuchok commented 10 years ago

ah got it! thanks for the quick response and clarification.

if you want to add-in the ad-level errors, too, that'd be awesome. i've added to issues -- #19 -- if not, no worries. i can take care of it.

lukaszzdanikowski commented 10 years ago

I will be working with VAST implementation for the next few weeks so I will try to add few elements :-)

lukaszzdanikowski commented 10 years ago

I've added some tests but from what i can see VAST with error element on top level isn't validating with xsd

tbuchok commented 10 years ago

@lukaszzdanikowski the diff on the index file looked really big and i wanted to simplify the delta visually, you can see what i did here:

https://github.com/tbuchok/vast-xml/commit/255da8a3482d08b7c71773d2144fb956fc03143b

i've cherry-picked your tests and added some descriptions to why the xsd isn't passing -- thanks for taking a look.

looking forward to working more on improving the library together and seeing other areas we can make it better. thanks again for great work.