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.

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

MEM-566 ETL - Remove duplicate External Links (MMTI Project Pages) #135

Merged
merged 1 commit into from
Oct 25, 2017

Conversation

asanchezr
Copy link
Contributor

No description provided.

@asanchezr asanchezr requested a review from marklise October 24, 2017 22:45
Copy link
Contributor

@marklise marklise left a comment

Choose a reason for hiding this comment

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

In general logic is correct, however I think we'd be better suited to not reinvent the wheel and leverage _.uniqBy

var mongodb = require('mongodb');
var Promise = require('promise');
var _ = require('lodash');
var uniqueBy = require('./utils').uniqueBy;
Copy link
Contributor

Choose a reason for hiding this comment

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

While correct, is not necessary due to the require of lodash. Lodash has a method _.uniqBy that achieves the same result as the code in the utils file.

console.log(': found ' + originalLinkCount + ' external links');

// filter out duplicate links
var links = uniqueBy(mmtiProject.externalLinks, 'link');
Copy link
Contributor

Choose a reason for hiding this comment

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

As noted above, you can replace this with lodash prototype _.uniqBy

* Cleanup `externalLinks` in Project model
* Require Lodash v4.17.4 (for _.uniqBy)
@asanchezr asanchezr force-pushed the MEM-566-duplicate-links-etl branch from 063840f to 187f0da Compare October 25, 2017 17:39
@asanchezr
Copy link
Contributor Author

@marklise I've updated the PR. Please review

@asanchezr asanchezr requested a review from marklise October 25, 2017 17:41
Copy link
Contributor

@marklise marklise left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@marklise marklise merged commit 355fa9f into bcgov:master Oct 25, 2017
@asanchezr asanchezr deleted the MEM-566-duplicate-links-etl branch October 25, 2017 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants