Fixes Issue #462 Integrate inactive blog url and include logic to filter inactive feeds #520

c3ho · 2019-12-16T16:34:32Z

Issue This PR Addresses

Type of Change

Bugfix: Change which fixes an issue
New Feature: Change which adds functionality
Documentation Update: Change which improves documentation
UI: Change which improves UI

Description

There is no logic for handling inactive feeds in storage.js. I added a few functions that allows for active feeds to be moved to inactive feeds, get all inactive feeds and get count of all inactive feeds. There is one function that should be debated which is getInactiveFeed.

Still a WIP but would like some feedback on current changes before continuing

Checklist

Quality: This PR builds and passes our npm test and works locally
Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

…ter inactive feeds added functions for inactive feedsd

c3ho · 2019-12-16T16:46:20Z

I opted to use SMOVE to move members from one set to another instead of doing an SINT(set intersection).

manekenpix

Looks good, just one comment

manekenpix · 2019-12-17T02:39:24Z

src/backend/utils/storage.js

+   * Gets an array of guids from redis
+   * @param from lower index
+   * @param to higher index, it needs -1 because redis includes the element at this index in the returned array
+   * @return Array of guids
+   */


Do you need this comment here? You're not passing anything to 'getInactiveFeeds` so I don't know what that comment is for.

Hahaha, you're right. I had fixed this in a new commit but didn't push it to origin. I just wanted some thought on it.

humphd

This feels like it's duplicating code. Could we not refactor things so that we pass FEEDS vs. INACTIVE_FEEDS to some private functions that actually do the shuffling of keys between sets?

This PR makes me want to think harder about our data and how we're structuring it, but I should probably leave that for later. I suspect we need to make some changes soon.

c3ho · 2019-12-17T03:15:03Z

I had to double the code unfortunately, because we can't SMOVE if there is no destination.

c3ho · 2019-12-17T03:19:50Z

This feels like it's duplicating code. Could we not refactor things so that we pass FEEDS vs. INACTIVE_FEEDS to some private functions that actually do the shuffling of keys between sets?

This PR makes me want to think harder about our data and how we're structuring it, but I should probably leave that for later. I suspect we need to make some changes soon.

Oh wait I see what you mean. I can probably do that

humphd · 2019-12-17T03:20:17Z

So I wonder if what we really need to do, is create a class for Feed like we have now for Post, and let it manage some of this state.

const feed = new Feed(url, name, ...);
await feed.save(); // now in Redis, and part of FEEDS set
...
const feed = await Feed.byUrl('https://.....'); // get feed from Redis 
feed.inactive = true; // indicate that we want this to go into INACTIVE_FEEDS when we save
feed.save(); // saved, and the "inactive" flag is used to move it into the right set

I'm just thinking out loud, but your code at this level could support that one level above.

c3ho · 2019-12-17T15:48:50Z

@humphd do you want get these changes through first and then file a separate issue to refactor, although it is a WIP there is still another issue that depends on this one. I'll be happy to refactor it and move all of it to its own class after.

I'll work on this now actually as part of this fix.

humphd · 2019-12-17T22:20:38Z

src/backend/feed.js

+  constructor(url, name) {
+    this.url = url;
+    this.name = name;
+    this.inactive = false;


I don't think we should include this in the actual data. Let's have it be something that gets exposed based on which set it's in. When we read from the db, we'll figure that out.

so in this case we just want the this.inactive as the only property?

It's going to have to be an async getter/setter, since it requires a database operation vs. storing something on this. We don't need an actual property, just a way to get at the database.

Got it, don't we have a problem here then? If we're trying to retrieve from the database, should active/inactive be kept track of in the DB as well?

My thought here is that the state we're going to store won't be on the Object, but rather, in which of the two sets this hash object lives in Redis. A feed is either in the feeds or inactive-feeds set, but not both. So we can use that as a state we read from the database, and expose as a boolean via the JS API.

What about:

const isActive = await feed.isActive(); // checks if this feed is in feeds set await feed.inactive(); // set this feed inactive (move to inactive feeds) await feed.active(); // set this feed active (move to feeds)

I guess what I'm confused about is I have code like this now

async inactive() { const isActive = await this.isActive(); await this.save(); }

I won't be passing isActive to save(). So now my addFeed(feed) function which is called when save() is called can't differentiate between whether it should add it to FEEDS or INACTIVE_FEEDS. The only workaround I can think about this is to separate the addFeed function into a addActiveFeed(feed) and addInactiveFeed(feed).

src/backend/feed.js

humphd · 2019-12-17T22:21:45Z

src/backend/feed.js

+   * Marks the feed as inactive
+   */
+  inactive() {
+    this.inactive = true;


Instead of storing state for this, put it in the appropriate set.

How we we re-activate a feed?

on it, added an active() function that will do the opposite and adjusted the storage.js addFeed to handle it as well

humphd · 2019-12-18T04:03:21Z

src/backend/utils/storage.js

+   * @return feedId
+   */
+  addFeed: async feed => {
+    if (feed.feedId && (await redis.exists(INACTIVE_FEEDS)) === 1) {


These add* functions are doing two things, and should be split. First, you're dealing with creating a hash to store the feed data. That makes sense, and should stay here. Then, you're putting the id into a set, and that can get moved out to separate functions.

Also, you probably want to use sismember vs. exists.

You're right, I think I was trying to do too much with this function.

Would addFeed even be an appropriate name for this function, since we're not technically adding the feed anywhere?

humphd · 2019-12-18T04:04:37Z

src/backend/utils/storage.js

+   * @param feed Feed Object
+   */
+  addInactiveFeed: async feed => {
+    if ((await redis.exists(INACTIVE_FEEDS)) === 1) {


This code is identical to what you do above. The only difference is that one uses FEEDS where the other uses INACTIVE_FEEDS. As such, you can refactor this to use 2 arguments passed to a function, and then only do this code once, but call it from two helper functions that deal with the order of these args.

I slowly realized this... and have something working in storage.js as with feedSet as a param for the set to send it to

addFeed: async (feed, feedSet){ }

whoops please disregard this as I replied to this before the previous one.

humphd · 2019-12-18T04:07:00Z

src/backend/utils/storage.js

+
+  getInactiveFeedsCount: () => redis.scard(INACTIVE_FEEDS),
+
+  getFeedStatus: feedId => redis.sismember(FEEDS, feedId),


This isn't well named. isFeedActive maybe?

humphd

Did another look over this. I'm having trouble figuring out what API to recommend, and with names. I wonder if part of the problem is that we have no tests, no callers for Feed. If we added that, it might become more clear as we try and use it.

Also, I'm going to vanish into holidays for a while, so I may not get back to this til after the break.

humphd · 2019-12-18T18:09:26Z

src/backend/feed.js

   */
  async save() {
    this.feedId = await storage.addFeed(this);
+    await storage.storeFeed(this.feedId);
  }

  /**
   * Checks if feed is part active feeds list. If it is, will move feed from active FEEDS set to INACTIVE_FEEDS set


part of active feeds list

humphd · 2019-12-18T18:09:36Z

src/backend/feed.js

  }

  /**
   * Checks if feed is part active feeds list. If it is, will move feed from active FEEDS set to INACTIVE_FEEDS set
+   * else will move ffeed from INACTIVE_FEEDS to FEEDS


s/ffeed/feed/

humphd · 2019-12-18T18:10:51Z

src/backend/feed.js

   */
-  async inactive() {
-    const isActive = await this.isActive(this);
+  async move() {


So this is really toggleStatus vs. move right?

I think you'd be better off having 2 functions for this vs. one. Let the caller mange deciding which one to use.

humphd · 2019-12-18T18:12:56Z

src/backend/utils/storage.js

+   * @param feedId unique ID of feed to be stored
+   * @param destination set feed is being added into
+   */
+  storeFeed: async (feedId, destination) => {


Not sure about this name, but also don't know what to suggest. Need to think on it.

c3ho · 2019-12-18T18:47:41Z

Thank you for the review, I'll write some tests and make the requested changes. Happy holidays!

c3ho · 2020-01-13T15:25:23Z

Will move this PR to 0.6

humphd · 2020-01-23T20:18:50Z

My work in #592 will help with this, I think. It will also require rebasing.

c3ho · 2020-01-23T20:21:40Z

@humphd thanks! Will review that asap

c3ho · 2020-01-27T15:18:59Z

Code has changed a lot since this PR was submitted, will close and submit a new PR when ready.

c3ho added 2 commits December 16, 2019 11:26

fixes issue-462, Integrate inactive blog url and include logic to fil…

7667239

…ter inactive feeds added functions for inactive feedsd

added functions for inactive feeds

2e2828a

c3ho changed the title ~~Issue 462~~ Fixes Issue #462 Integrate inactive blog url and include logic to filter inactive feeds Dec 16, 2019

still updating

6854049

manekenpix requested changes Dec 17, 2019

View reviewed changes

manekenpix added area: back-end area: redis Redis Database related type: enhancement New feature or request labels Dec 17, 2019

humphd requested changes Dec 17, 2019

View reviewed changes

added new file for feed class, refactored some functions

1b2b5ad

humphd requested changes Dec 17, 2019

View reviewed changes

This was referenced Dec 17, 2019

Standardize on base64 encoded, SHA-256 hashed, normalized URIs as Redis keys #522

Closed

Add /feeds route to match what /posts does #523

Closed

split addFeed functions into addFeed and addInactiveFeed in storage.js

439f9ea

c3ho force-pushed the issue-462 branch from c01f4db to 439f9ea Compare December 18, 2019 03:55

humphd requested changes Dec 18, 2019

View reviewed changes

split functions so hset, sadd and smove are in separate functions

8d81925

humphd requested changes Dec 18, 2019

View reviewed changes

c3ho closed this Jan 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes Issue #462 Integrate inactive blog url and include logic to filter inactive feeds #520

Fixes Issue #462 Integrate inactive blog url and include logic to filter inactive feeds #520

c3ho commented Dec 16, 2019

c3ho commented Dec 16, 2019

manekenpix left a comment

manekenpix Dec 17, 2019

c3ho Dec 17, 2019

humphd left a comment

c3ho commented Dec 17, 2019

c3ho commented Dec 17, 2019

humphd commented Dec 17, 2019

c3ho commented Dec 17, 2019 •

edited

Loading

humphd Dec 17, 2019

c3ho Dec 17, 2019

humphd Dec 17, 2019

c3ho Dec 17, 2019

humphd Dec 17, 2019

c3ho Dec 18, 2019

humphd Dec 17, 2019

c3ho Dec 17, 2019

humphd Dec 18, 2019

c3ho Dec 18, 2019 •

edited

Loading

humphd Dec 18, 2019

c3ho Dec 18, 2019

c3ho Dec 18, 2019

humphd Dec 18, 2019

humphd left a comment

humphd Dec 18, 2019

humphd Dec 18, 2019

humphd Dec 18, 2019

humphd Dec 18, 2019

humphd Dec 18, 2019

c3ho commented Dec 18, 2019 •

edited

Loading

c3ho commented Jan 13, 2020

humphd commented Jan 23, 2020

c3ho commented Jan 23, 2020

c3ho commented Jan 27, 2020


		getInactiveFeedsCount: () => redis.scard(INACTIVE_FEEDS),

		getFeedStatus: feedId => redis.sismember(FEEDS, feedId),

Fixes Issue #462 Integrate inactive blog url and include logic to filter inactive feeds #520

Fixes Issue #462 Integrate inactive blog url and include logic to filter inactive feeds #520

Conversation

c3ho commented Dec 16, 2019

Issue This PR Addresses

Type of Change

Description

Checklist

c3ho commented Dec 16, 2019

manekenpix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

c3ho commented Dec 17, 2019

c3ho commented Dec 17, 2019

humphd commented Dec 17, 2019

c3ho commented Dec 17, 2019 • edited Loading

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

c3ho Dec 18, 2019 • edited Loading

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

humphd 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

c3ho commented Dec 18, 2019 • edited Loading

c3ho commented Jan 13, 2020

humphd commented Jan 23, 2020

c3ho commented Jan 23, 2020

c3ho commented Jan 27, 2020

c3ho commented Dec 17, 2019 •

edited

Loading

c3ho Dec 18, 2019 •

edited

Loading

c3ho commented Dec 18, 2019 •

edited

Loading