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

Update Feed in parser service to talk to Supabase instead of Redis #3464

Closed
2 tasks
TueeNguyen opened this issue Apr 12, 2022 · 5 comments · Fixed by #3529
Closed
2 tasks

Update Feed in parser service to talk to Supabase instead of Redis #3464

TueeNguyen opened this issue Apr 12, 2022 · 5 comments · Fixed by #3529
Assignees
Labels
type: enhancement New feature or request
Milestone

Comments

@TueeNguyen
Copy link
Contributor

TueeNguyen commented Apr 12, 2022

In #1907 #3355, we talked about removing feed-related code in Posts.

  • Change setInvalidFeed function to use Supabase

    setInvalidFeed: (id, reason) => {
    const key = createInvalidFeedKey(id);
    const sevenDaysInSeconds = 60 * 60 * 24 * 7; // Expire after 7 days
    return redis.set(key, reason, 'EX', sevenDaysInSeconds);
    },

  • setFlaggedFeed: (id) => redis.smove(feedsKey, flaggedFeedsKey, id),
    unsetFlaggedFeed: (id) => redis.smove(flaggedFeedsKey, feedsKey, id),

@TueeNguyen TueeNguyen added the type: enhancement New feature or request label Apr 12, 2022
@TueeNguyen TueeNguyen added this to the 3.0 Release milestone Apr 12, 2022
@TueeNguyen TueeNguyen self-assigned this Apr 12, 2022
@JerryHue
Copy link
Contributor

There's also this:

  • The parser service does not need to updateFeeds anymore. Refer to:
    const currentFeed = await updateFeed(feed);

    const updateFeed = async (feedData) => {
    let currentFeed;
    // If we have an existing feed in the database for this URL, prefer that,
    // since it might have updated cache info (e.g., etag).
    const existingFeed = await Feed.byUrl(feedData.url);
    if (existingFeed) {
    // We have a version of this feed in the database already, prefer that
    currentFeed = existingFeed;
    } else {
    // First time we're seeing this feed, add it to the database
    const id = await Feed.create(feedData);
    currentFeed = await Feed.byId(id);
    }
    return currentFeed;
    };

@TueeNguyen
Copy link
Contributor Author

TueeNguyen commented Apr 15, 2022

I'm working on this issue to make parser save flagged feeds to supabase (simply change flagged of feed to true/false). I encountered this piece of code

if (await redis.sismember(flaggedFeedsKey, feed.id)) return;

I will also need to change this line if I'm removing all the redis flagging feed code, something like this

if (await isFlaggedFeed(feed.id)) // isFlaggedFeed() is function that uses supabase
  return; 

Therefore, the test won't pass and there's no mock cilent for supabase at the moment. I'm not sure what's the next step is

@humphd
Copy link
Contributor

humphd commented Apr 15, 2022

Mock the isFlaggedFeed() function to return Promise.resolve(true) or Promise.resolve(false) as you require in your tests.

@TueeNguyen
Copy link
Contributor Author

I'll try, thanks

@TueeNguyen
Copy link
Contributor Author

After looking at the function

const updateFeed = async (feedData) => {
let currentFeed;
// If we have an existing feed in the database for this URL, prefer that,
// since it might have updated cache info (e.g., etag).
const existingFeed = await Feed.byUrl(feedData.url);
if (existingFeed) {
// We have a version of this feed in the database already, prefer that
currentFeed = existingFeed;
} else {
// First time we're seeing this feed, add it to the database
const id = await Feed.create(feedData);
currentFeed = await Feed.byId(id);
}
return currentFeed;
};

I believe we can keep it as it looks for feed object in Redis not in Supabase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants