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

Fixes #22: Inactive Blog Filter #143

Merged
merged 1 commit into from
Nov 14, 2019
Merged

Conversation

jerryshueh
Copy link
Contributor

@jerryshueh jerryshueh commented Nov 11, 2019

Addresses #22. Had to perform some workarounds on this one as we do not yet have an official feed-worker implemented. That issue has been assigned to @robertbegna in issue #108, and I didn't want to take away from any work he might want to do. In order to test my implementation, I had to create a new, altered feed-worker in /test.

A feeds-redlist.json was created in root, which contains filtered feeds. Since they are not permanently banned or anything, I didn't want to call it a "blacklist". Each item contains an url attribute for the feed, and a lastUpdate attribute (ISO 8601 datetime string) for the blog's last known activity, according to the feed data. A 0 value means the blog has a dead feed.

New module created in /src/inactive-blog-filter.js, which performs most of the logic:

  • First is option check(), which takes a feed URL and a callback function. It will check a provided feed url against the redlist file and return the result (true/false) to the callback, which can then perform whatever task.
  • Second is update(), which performs a complete sweep of all feeds on the feedlist to check if they are inactive or not. Then it re-writes the redlist file to reflect the latest changes. This may become an issue if the feedlist grows, so for now, we'll say we can run this periodically and not live with each feed update. The reason I chose to do this is because, again, we do not have a proper feed-worker to accumulate data, nor a proper database to search. Once we move forward, this module can be updated to perform specific searches on stored blogs.

ODAVING
ODAVING previously approved these changes Nov 11, 2019
Copy link
Contributor

@ODAVING ODAVING left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, looks good to me!

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Did a first look over this, left some comments.

We really need to get the Redis database integration started so that people don't have to read/write files for everything. cc @mskuybeda

src/inactive-blog-filter.js Outdated Show resolved Hide resolved
src/inactive-blog-filter.js Outdated Show resolved Hide resolved
eekbatani
eekbatani previously approved these changes Nov 11, 2019
@jerryshueh
Copy link
Contributor Author

jerryshueh commented Nov 12, 2019

Made the changes to env.example so the filter module now calls process.env to retrieve variables. Non-global requires go against airbnb and fails eslint, so I've reverted back to traditional readFile for now.

humphd
humphd previously requested changes Nov 12, 2019
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

OK, did another pass.

env.example Outdated Show resolved Hide resolved
env.example Outdated Show resolved Hide resolved
feeds-redlist.json Outdated Show resolved Hide resolved
src/inactive-blog-filter.js Outdated Show resolved Hide resolved
src/inactive-blog-filter.js Outdated Show resolved Hide resolved
src/inactive-blog-filter.js Outdated Show resolved Hide resolved
src/inactive-blog-filter.js Outdated Show resolved Hide resolved
src/inactive-blog-filter.js Outdated Show resolved Hide resolved
src/inactive-blog-filter.js Show resolved Hide resolved
test/feed-worker-filtertest.js Outdated Show resolved Hide resolved
@jerryshueh
Copy link
Contributor Author

jerryshueh commented Nov 13, 2019

Pushed new changes to PR:

  • The blog filter module now correctly requires ./config.js to load env variables.
  • check() now uses error-first callback.
  • Redlist uses null to denote "dead' feeds.
  • env.INACTIVETIME changed to env.BLOG_INACTIVE_TIME and represents time in days instead of milliseconds
  • Removed the inappropriate, defunct test code

Potential features to address as separate issues:

  • Make update() properly async so it can be more efficiently run as the feeds update vs. periodic batch run
  • Write some proper tests!

@jerryshueh
Copy link
Contributor Author

Looks like some changes have been made since I rebased, so now there's a conflict with env.example. It will be fixed with the next rebase before the merge, if everything else checks out.

ODAVING
ODAVING previously approved these changes Nov 14, 2019
Copy link
Contributor

@ODAVING ODAVING left a comment

Choose a reason for hiding this comment

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

Passed all the checks, and I find nothing through first glance, other than conflicts which you said you’ll rebase! I think this is good to merge after rebase!! Amazing job Jerry!

UltimaBGD
UltimaBGD previously approved these changes Nov 14, 2019
Copy link
Contributor

@UltimaBGD UltimaBGD left a comment

Choose a reason for hiding this comment

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

Looks good. We might need to get @humphd to review or dismiss his old reviews as stale for the merge.

@jerryshueh jerryshueh dismissed stale reviews from UltimaBGD and ODAVING via 3d69332 November 14, 2019 23:32
@jerryshueh jerryshueh force-pushed the Issue-22 branch 2 times, most recently from 3d69332 to 040c676 Compare November 14, 2019 23:35
@jerryshueh jerryshueh dismissed humphd’s stale review November 14, 2019 23:41

Changes will be addressed in independant issue

@jerryshueh
Copy link
Contributor Author

Sorry, I had to rebase this PR again as there were several merges during the time I addressed the last fix. If I could get another set of reviews that would be great.

Copy link
Contributor

@dbeigi dbeigi left a comment

Choose a reason for hiding this comment

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

I took a glance at the code and it looks good to me. Good documentation, also I can create another issue for the testing of your code. Good job @jerryshueh

@ODAVING ODAVING merged commit cf216c9 into Seneca-CDOT:master Nov 14, 2019
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.

7 participants