Add missing node to the parsing #444

Rapha0511 · 2023-03-14T16:22:46Z

Description

The mergeWrapperAdData function has been modified to keep track of the <icon> elements contained in a wrapper's creative. If a creative contain <icon>, the whole creative will be included in the final response.
Since the VASTTracker allow users to convert seconds to timecode with the convertToTimecode function, the parseDuration function has been exposed publicly as a VASTParser method to allow users to convert timecode to seconds if needed
xmlEncoded is now considered as an attribute of the <adParameters> node, instead of a node by itself. The value of <adParameters> is now an Object of type {value: String, xmlEncoded: String}.
The type attribute of the <survey> node has been added to the parsing. The value of survey is now an Object of type {value: String, type: String}.
<iconClickFallbackImages> node has been added to the parsing as an array of Object of type {url: String, width: String, height: String}.
Tests have been added for changes and the addition of the missing nodes. Some of them have been moved from Mocha to Jest
For companions, the parameter adSlotID has been modified to adSlotId to follow the VAST specs

VAST 4.3 features

Type

…o fail

ZacharieTFR

Hello, thanks for your work👍 I left some comments

ZacharieTFR · 2023-03-17T15:23:56Z

docs/api/vast-parser.md

+
+### parseDuration(duration)
+
+Parses a duration in the format `HH:MM:SS`, `HH:MM:SS.mmm`  or `SS` and returns a duration in seconds.
+
+```javascript
+
+const a = parseDuration('00:01:30.000');
+console.log(a); // output: 90
+
+const b = parseDuration('30');
+console.log(b); // output: 30
+
+const c = parseDuration(30);
+console.log(c); // output: 30
+
+const d = parseDuration('thirty');
+console.log(d); // output -1
+
+```
+
+#### Parameter
+
+- **`duration: String`** - The duration represented as a string.


You should move this to the public methods section above

ZacharieTFR · 2023-03-17T15:26:12Z

spec/parser_utils.spec.js

+              type: 'image/jpeg',
+              staticResource: 'link',
+              htmlResource:
+                '<!DOCTYPE html>\r\n                                            <html lang="en">\r\n                                            <head>\r\n                                                <meta charset="UTF-8">\r\n                                                <meta http-equiv="X-UA-Compatible" content="IE=edge">\r\n                                                <meta name="viewport" content="width=device-width, initial-scale=1.0">\r\n                                                <title>Document</title>\r\n                                            </head>\r\n                                            <body>\r\n                                                <h1>titre</h1>\r\n                                            </body>\r\n                                            </html>',


Can you replace the htmlResource with a dummy value to make it easier to read ?

ZacharieTFR · 2023-03-17T15:26:21Z

spec/parser_utils.spec.js

+              type: 'image/jpeg',
+              staticResource: 'link',
+              htmlResource:
+                '<!DOCTYPE html>\r\n                                            <html lang="en">\r\n                                            <head>\r\n                                                <meta charset="UTF-8">\r\n                                                <meta http-equiv="X-UA-Compatible" content="IE=edge">\r\n                                                <meta name="viewport" content="width=device-width, initial-scale=1.0">\r\n                                                <title>Document</title>\r\n                                            </head>\r\n                                            <body>\r\n                                                <h1>titre</h1>\r\n                                            </body>\r\n                                            </html>',


ZacharieTFR · 2023-03-17T15:29:55Z

src/parser/creative_linear_parser.js

+        )
+        .forEach((iconClickFallbackImageElement, id) => {
+          icon.iconClickFallbackImages.push({
+            id,


Why adding the id attribute ? As there is no reference to it in the VAST specs

ZacharieTFR · 2023-03-17T15:31:57Z

docs/api/class-reference.md

+## iconClickFallbackImages <a name="iconclickfallbackimages"></a>
+
+- `id: Number`
+- `url: String`


This is missleading as iconClickFallbackImages is an array and the object correspond to the iconClickFallbackImage (singular)
Also, can the url equal to null or will it be an empty string ?

ZacharieTFR · 2023-03-17T15:32:38Z

spec/samples/creatives.js

+            <IconClickFallbackImages>
+              <IconClickFallbackImage width="10px" height="10px">
+                <![CDATA[http://adserver.com/fallback.png]]>
+              </IconClickFallbackImage>
+            </IconClickFallbackImages> 


We may need a use case with multiple to fully cover use cases

…ges element

… parsing

ZacharieTFR · 2023-03-24T11:19:39Z

docs/api/class-reference.md

+## adParameters<a name="adparameters"></a>
+
+- `value: String`
+- `xmlEncoded: String|null`
+
+## survey<a name="survey"></a>
+
+- `value: String`
+- `type: String|null`
+
+## iconClickFallbackImage <a name="iconclickfallbackimage"></a>


To stay coherent with other sections can you put titles in uppercase ? 🙏

src/parser/parser_utils.js

ZacharieTFR · 2023-03-24T11:36:11Z

src/parser/vast_parser.js

@@ -27,6 +27,7 @@ export class VASTParser extends EventEmitter {
  constructor() {
    super();

+    this.parseDuration = parserUtils.parseDuration;


One discutions we had about exposing/exporting util functions is that we can either do like you did but then we need to create a new instance of vastParser otherwise the second solution would be to export utils function as a standalone then we could use it without creating a new instance and in addition if we don't use it we could benefit from tree-shaking

ZacharieTFR

LGTM ! Just a small nitpicking

ZacharieTFR · 2023-04-14T15:17:14Z

docs/api/vast-parser.md

+
+

Too many breaklines here 😅

Suggested change

clementFrancon

LGTM ! Great work !

Raphael Levin added 6 commits March 3, 2023 17:18

[parser] add support for missing nodes and attributes

8c61b2b

[test] add test for new parsed elements

491507b

[docs] add docs for new parsed elements

f259ce9

[parser] expose parseDuration publicly

826e4fe

[test] change values for icons node

258143a

[parser] fix IconClickFallbackImages node parsing

8b33682

Rapha0511 added enhancement documentation breaking-change labels Mar 14, 2023

Rapha0511 self-assigned this Mar 14, 2023

[parser] fix iconClickFallbackImages node parsing causing the tests t…

53abce9

…o fail

ZacharieTFR reviewed Mar 17, 2023

View reviewed changes

Raphael Levin added 3 commits March 21, 2023 11:39

[docs] change misleading documentation

eceb591

[test] change the use case and the tests for the IconClickFallbackIma…

03d46ce

…ges element

[parser] add with and height attributes to the IconClickFallbackImage…

6f9988d

… parsing

ZacharieTFR reviewed Mar 24, 2023

View reviewed changes

Raphael Levin added 5 commits April 12, 2023 11:08

[docs] update docs for parseDuration function adSlotId parameter

0cd63f7

[test] update tests for adSlotId parameter

3042bcb

[parser] modifie parseDuration export method

4680683

[parser] replace adSlotID by adSlotId to follow the VAST specs

7be2d25

[parser] remove parseDuration method from VASTParser class

bce1813

ZacharieTFR approved these changes Apr 14, 2023

View reviewed changes

[docs] remove unnecessary space

784270c

clementFrancon approved these changes Apr 21, 2023

View reviewed changes

ZacharieTFR merged commit cee0051 into master Apr 27, 2023
1 check passed

ZacharieTFR deleted the add-missing-node branch April 27, 2023 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing node to the parsing #444

Add missing node to the parsing #444

Rapha0511 commented Mar 14, 2023 •

edited

ZacharieTFR left a comment

ZacharieTFR Mar 17, 2023

ZacharieTFR Mar 17, 2023

ZacharieTFR Mar 17, 2023

ZacharieTFR Mar 17, 2023

ZacharieTFR Mar 17, 2023

ZacharieTFR Mar 17, 2023

ZacharieTFR Mar 24, 2023

ZacharieTFR Mar 24, 2023

ZacharieTFR left a comment •

edited

ZacharieTFR Apr 14, 2023

clementFrancon left a comment

Add missing node to the parsing #444

Add missing node to the parsing #444

Conversation

Rapha0511 commented Mar 14, 2023 • edited

Description

Type

ZacharieTFR left a 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

Choose a reason for hiding this comment

ZacharieTFR left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clementFrancon left a comment

Choose a reason for hiding this comment

Rapha0511 commented Mar 14, 2023 •

edited

ZacharieTFR left a comment •

edited