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

Fix Issue 446 #465

Merged
merged 9 commits into from Dec 6, 2019
Merged

Fix Issue 446 #465

merged 9 commits into from Dec 6, 2019

Conversation

@MusaBajwa
Copy link
Contributor

MusaBajwa commented Dec 5, 2019

I added a route for '/count'. Please let me know if I have to make changes or if I did something wrong.

MusaBajwa added 2 commits Dec 5, 2019
@MusaBajwa MusaBajwa requested review from manekenpix and humphd Dec 5, 2019
@@ -1,6 +1,7 @@
const express = require('express');
const { getPosts } = require('../../utils/storage');
const { logger } = require('../../utils/logger');
const { count } = require('../../utils/storage.js');

This comment has been minimized.

Copy link
@humphd

humphd Dec 5, 2019

Contributor

const { getPostsCount } = require('../../utils/storage.js');, there's nothing called count to get here.

@@ -31,4 +32,8 @@ posts.get('/', async (req, res) => {
);
});

posts.get('/count', async (req, res) => {
res.json(count.getPostsCount());

This comment has been minimized.

Copy link
@humphd

humphd Dec 5, 2019

Contributor

The call to getPostsCount needs to be awaited, since it's async, and you'll need to wrap in a try/catch and give back a 500 if the redis call fails.

This comment has been minimized.

Copy link
@MusaBajwa

MusaBajwa Dec 6, 2019

Author Contributor

Ok thanks for the feedback. I tried to fix this in the following commit.

@MusaBajwa

This comment has been minimized.

Copy link
Contributor Author

MusaBajwa commented Dec 6, 2019

@manekenpix Could I get some feedback on my recent changes, if possible.

@@ -33,7 +33,14 @@ posts.get('/', async (req, res) => {
});

posts.get('/count', async (req, res) => {
res.json(count.getPostsCount());
try {
await res.json(getPostsCount());

This comment has been minimized.

Copy link
@humphd

humphd Dec 6, 2019

Contributor

Are you testing your code locally? This won't work. You need to await the portion of this that is asynchronous (i.e., returns a Promise). The call to res.json(...) is synchronous, and will block while it runs. However, the call to getPostsCount() is promise-based, and needs to go and talk to the database without blocking the main thread. As such, you need to do something like:

try {
  res.json(await getPostsCount());

You could also break it up a bit if you want:

try {
  const count = await getPostsCount();
  res.json(count);

This comment has been minimized.

Copy link
@MusaBajwa

MusaBajwa Dec 6, 2019

Author Contributor

Ok thanks will make the changes right now, yes I am running the code locally (testing using npm test) but it does not show any errors with posts.js.

This comment has been minimized.

Copy link
@humphd

humphd Dec 6, 2019

Contributor

Nothing in our tests is calling this route. You should add a test for this too, which will make it a lot easier to understand if it's working or not.

In our posts test file, you want something like:

  it('returns correct number of posts', async () => {
    const count = await request(app).get('/posts/count');

    expect(res.status).toEqual(200);
    expect(res.get('Content-type')).toContain('application/json');
    expect(res.body).toBe(count);
  });

(untested code, fix as makes sense...)

This comment has been minimized.

Copy link
@MusaBajwa

MusaBajwa Dec 6, 2019

Author Contributor

Ok I am going to add that now thanks David.

@MusaBajwa

This comment has been minimized.

Copy link
Contributor Author

MusaBajwa commented Dec 6, 2019

So I tested using the tester that you gave me and I made the changes necessary. It gave me no errors.
image

@MusaBajwa MusaBajwa requested a review from humphd Dec 6, 2019
@humphd
humphd approved these changes Dec 6, 2019
Copy link
Contributor

humphd left a comment

Nice!

Copy link
Contributor

wajeehsheikh left a comment

Great way to get the number of posts. Great for analysis. Good job!

Copy link
Contributor

wajeehsheikh left a comment

Great way to get the number of posts. Great for analysis. Good job!

@MusaBajwa MusaBajwa merged commit f568286 into Seneca-CDOT:master Dec 6, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MusaBajwa MusaBajwa deleted the MusaBajwa:issue-446 branch Dec 6, 2019
Copy link
Contributor

jerryshueh left a comment

Very nice, thanks for responding to all the fixes. This will be a good tool going forward!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.