Release 0.3 part 2, Telescope

The second task for our Release 0.3 was to fix and issue and merge a pull request into telescope. This involves finding an issue, fixing it, collaborating with the telescope slack channel, and finally getting feedback and eventually merging the pull request.

This sounds like a lot of steps but depending on the issue, they can take very little time and produce very little headache.

This is not the case with mine.

The issue

So my issue was this:

Add query params validation for DELETE /feeds/:id route using Express Validator and unit tests #1308

tonyvugithub posted on Nov 11, 2020

What would you like to be added: Request to add query params validation for DELETE /feeds/:id route using Express-validator and also unit tests to ensure that it works

Why would you like this to be added: It would be better to validate the query before it reach the route handler.

reference: https://express-validator.github.io/docs/check-api.html

All logic for the middleware should be place inside backend/web/validation.js

View on GitHub

Basically, I'm meant to go into the backend for Telescope and add some query parameters with express-validator. Then I have to link them to the DELETE feed route and write tests for it.

First, what I did was add a new validation parameter to the validation.js, but after some review, we decided it was better to use what already existed to not bloat our code.

validation.js excerpt

const feedsIdParamValidationRules = [
  param('id', 'Id Length is invalid').isLength({ min: 10, max: 10 }),
];

module.exports = {
  validateNewFeed,
  validateQuery: () => validate(queryValidationRules),
  validatePostsQuery: () => validate(postsQueryValidationRules),
  validatePostsIdParam: () => validate(postsIdParamValidationRules),
  validateFeedsIdParam: () => validate(feedsIdParamValidationRules),
};

A fairly simple piece of code that establishes a rule that an id must be a minimum 10, and a maximum 10 (aka exactly 10) characters long.

Then, in our feeds.js file, which holds our routes for blog feeds, we have to make sure our delete feeds route checks our new rule.

delete feeds route

feeds.delete('/:id', validateFeedsIdParam(), protect(), async (req, res) => {
  const { user } = req;
  const { id } = req.params;
  try {
    const feed = await Feed.byId(id);
    if (!feed) {
      return res.status(404).json({ message: `Feed for Feed ID ${id} doesn't exist.` });
    }
    if (!user.owns(feed)) {
      return res.status(403).json({ message: `User does not own this feed.` });
    }
    await feed.delete();
    return res.status(204).json({ message: `Feed ${id} was successfully deleted.` });
  } catch (error) {
    logger.error({ error }, 'Unable to delete feed to Redis');
    return res.status(503).json({
      message: 'Unable to delete to Feed',
    });
  }
});

Notice the promise call that includes validateFeedsIdParam(), this is our rule that will be tested against when we submit an id to the delete request.

the test

it('should return 400 when id is less than 10 characters', async () => {
    const id = '123456789';
    const res = await request(app).delete(`/feeds/${id}`);

    expect(res.status).toEqual(400);
  });

  it('should return 400 when id is more than 10 characters', async () => {
    const id = '123456789123456789';
    const res = await request(app).delete(`/feeds/${id}`);

    expect(res.status).toEqual(400);
  });

keep in mind this is after a multitude of critique.

Oops!

so now that we have our code working, our test working properly, all that's left is to merge into the project right?

Not quite. That's pretty easy, let's just rebase on GitHub since it's only one line that seems to be wrong!

Oops. Again.

Ok ok ok this is fixable, all I have to do is go back, run some commands, fix up in an interactive rebase, and commit again.

For those that don't know, here's a nice image that'll help you out if you end up in my situation.

Wait a sec, it looks like something else is wrong...

Huh?

Ok. So here's what happened. I go ahead and run prettier on my files but I ran it through vscode, which means it's slightly different from the Telescope configuration for prettier. Now you would expect a test to check this, find it wrong, and send back an error, right? But no, it actually thinks everything is good!

So what is there to do?

Be more explicit with prettier config #1399

humphd posted on Nov 18, 2020

We ran into a weird issue where Prettier was not seeming to use some of our values which are implicit defaults. This sets all prettier options explicitly.

View on GitHub

So David opened up a pull request to fix the prettier config and make sure this doesn't happen again, but I'm still in a position where my code is all messed up. So I guess the only thing to do is manually fix our formatting errors.

I'll spare you the screenshots.

Finally, we've got our pull request. Formatting? Great! Functionality? Awesome. Tests? Work properly! Now all we have to do is commit, rebase PROPERLY, and push!

Yes!

So to conclude - this has been a very educational experience. Not just in Git and how to work with a project's contributing guidelines (and what happens when you mess it up), but how to interact in a community and get feedback from members. Until now all my PRs have been basically some sequence of being assigned, working, putting up a PR, getting little to no feedback, and instantly having it merged. This experience really helped me figure out how to take criticism and build on my first would-be contribution.