Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix3.0/issue 324 #326

Merged
merged 9 commits into from
Oct 7, 2019
Merged

Fix3.0/issue 324 #326

merged 9 commits into from
Oct 7, 2019

Conversation

rgulrajani
Copy link
Contributor

Description

Changes to add the id attribute to:

  • impressionURLTemplates ids
  • advertiser id
  • companionClickTrackingURLTemplates ids
  • iconClickTrackingURLTemplates ids
  • nonlinearClickTrackingURLTemplates ids
  • videoClickThroughURLTemplate id
  • videoClickTrackingURLTemplates ids
  • videoCustomClickURLTemplates ids.

Issue

#324

Type

  • Breaking change
  • Enhancement
  • Fix
  • Documentation
  • Tooling

@rgulrajani
[parser][tracker][util]test] Changes to add the id attribute to the I…
728afcc 
…mpressionURLTemplates

dailymotion#324
@rgulrajani
[parser][test] Changes to add the id attribute to the URLTemplates:
02c4c79 
companionClickTrackingURLTemplates, iconClickTrackingURLTemplates, nonlinearClickTrackingURLTemplates, videoClickThroughURLTemplate, videoClickTrackingURLTemplates, videoCustomClickURLTemplates
@rgulrajani
[parser][test] Changes to add the id attribute for Advertiser
7f2a369 
dailymotion#324
@kobawan kobawan added 3.0 3.0 roadmap breaking-change hacktoberfest Participate in 2019's hacktoberfest! labels Oct 3, 2019
kobawan
kobawan suggested changes Oct 3, 2019
View reviewed changes
Copy link
Contributor

@kobawan kobawan left a comment

Choose a reason for hiding this comment

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

Pretty great! Many thanks for the contribution, I just have a few comments. Also be sure to update documentation .md files that are affected by these changes :)

src/parser/creative_companion_parser.js Outdated Show resolved Hide resolved
src/parser/creative_linear_parser.js Show resolved Hide resolved
src/util/util.js Outdated Show resolved Hide resolved
src/util/util.js Outdated Show resolved Hide resolved
src/util/util.js Outdated Show resolved Hide resolved
@kobawan kobawan added this to the 3.0 milestone Oct 3, 2019
@rgulrajani
[parser][util][test] Changes as per the PR review
3f3440b 
Updated to use map, instead of forEach
Added function to check for equality of objects using lodash.isequal
Added function to create an array of only unique objects when two arrays are merged
@rgulrajani
Updated the package-lock.json for lodash.isequal
127143a
rumesh
rumesh reviewed Oct 4, 2019
View reviewed changes
package.json Outdated
@@ -53,6 +53,7 @@
"sinon": "^2.3.6"
},
"dependencies": {
"lodash.isequal": "^4.5.0",
Copy link
Contributor

@rumesh rumesh Oct 4, 2019 •
edited

Choose a reason for hiding this comment

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

Do we really need to add this dependency ? we try to keep vast-client with fewer decencies as possible. And when I checked the code we don't need to have lodash.isequal which do deeper check inside an object. What changed here is just instead of having a url now we have plain object with id and url.
IMO we should write isTrackingObjectEqual which is really simple like this:

Suggested change
"lodash.isequal": "^4.5.0",
function isTrackingObjectEqual(obj1, obj2) {
return obj1.id === obj2.id && obj1.url === obj2.url;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

yes I also agree, lodash.isequal is rather heavy. It is best to do our own custom function for our needs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the dependency and added a custom method for checking equality

kobawan
kobawan reviewed Oct 4, 2019
View reviewed changes
package.json Outdated
@@ -53,6 +53,7 @@
"sinon": "^2.3.6"
},
"dependencies": {
"lodash.isequal": "^4.5.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

yes I also agree, lodash.isequal is rather heavy. It is best to do our own custom function for our needs

src/util/util.js Outdated Show resolved Hide resolved
src/vast_tracker.js Outdated Show resolved Hide resolved
src/util/util.js Outdated Show resolved Hide resolved
@rgulrajani
[tracker][util][test] Updates as per PR review
e4e899e 
Got rid of lodash.isequal, replaced it with a custom equal check
Moved extractURLsFromTemplates from the tracker to the util class
kobawan
kobawan reviewed Oct 7, 2019
View reviewed changes
Copy link
Contributor

@kobawan kobawan left a comment •
edited

Choose a reason for hiding this comment

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

Almost done! Please also remove the package-lock.json changes :)

src/util/util.js Outdated
* @param {Object} obj - The object who existence is to be checked.
* @param {Array} list - List of objects.
*/
function containsObject(obj, list) {
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry just another thing to add, the names of these functions shouldn't be so generic because in fact this function is only usable for url templates. same for joinArrayOfUniqueObjs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to reflect functionality

@rgulrajani
Copy link
Contributor Author

Almost done! Please also remove the package-lock.json changes :)
Done. Used npm install --save lodash to remove package

kobawan
kobawan approved these changes Oct 7, 2019
View reviewed changes
Copy link
Contributor

@kobawan kobawan left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@kobawan kobawan merged commit 571371e into dailymotion:3.0-version Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rumesh rumesh rumesh approved these changes

@kobawan kobawan kobawan approved these changes

Assignees
No one assigned
Labels
3.0 3.0 roadmap breaking-change hacktoberfest Participate in 2019's hacktoberfest!
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rgulrajani @rumesh @kobawan