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

#160445750 Implement API endpoints #12

Merged
merged 5 commits into from
Sep 19, 2018
Merged

Conversation

sudhons
Copy link
Owner

@sudhons sudhons commented Sep 16, 2018

What does this PR do?
Implement endpoints required for fast-food-fast

Description of Task to be completed?
The following endpoints:

  • user should get all orders on route GET /api/v1/orders
  • user should get a single order with the order Id on route GET /api/v1/orders/<orderId>
  • user should post an order on route POST /api/v1/orders
  • Admin should update the status of an order on route UPDATE /api/v1/orders/<orderId>

What are the relevant pivotal tracker stories?
#160445750 #160508060 #160505911 #160477398 #160448545

@sudhons sudhons temporarily deployed to food-fast September 17, 2018 11:01 Inactive
.eslintrc.json Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
API/src/app.js Outdated
return next(error);
});

// eslint-disable-next-line no-unused-vars
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's necessary to disable the rules on this line. You should be able to remove the arguments that are not used.

Copy link
Owner Author

Choose a reason for hiding this comment

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

If I remove the next argument, the middleware will not work. I wanted to create a middleware that catches and resolves all errors.

Copy link
Collaborator

@dogoyaro dogoyaro Sep 17, 2018

Choose a reason for hiding this comment

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

You aren't using next though. I don't understand what you mean by it wouldn't work. I'll try this locally and get back to you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It works without the next as I expected @sudhons. Please check and get back to me on this.

Copy link
Owner Author

@sudhons sudhons Sep 19, 2018

Choose a reason for hiding this comment

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

It works but when I pass an error object into next in any of the middlewares, I wanted this error handler alone to catch it. That syntax is the convention for error handlers. Without next, the default error handler will throw an error on the console and postman to the client.
Please check this: https://developer.mozilla.org/en-US/docs/Learn/Server-side/Express_Nodejs/Introduction#Handling_errors
and this: expressjs/express#2896

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not return the response with the error within the middle-ware that the error is found?

API/src/app.js Outdated

const PORT = parseInt(process.env.PORT, 10) || 5000;

if (require.main === module) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am curious about this check. Please explain.

Copy link
Owner Author

Choose a reason for hiding this comment

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

The purpose is to check whether app.js is running as a script or it is being used as an imported file (which is the case when I run tests). When it runs as a script the check equals true and app.js runs its server. Since chai-http creates a different server for its tests, It wouldn't need the server on app.js. Without the check the server would be started when tests run.

I will remove the check and use another method to stop the server

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sudhons. You might not need to do that. I just wanted to understand what you'd done. If you find a better way to handle this though.
I am concerned that in the test environment, what you are checking for might not really matter.

API/src/controllers/ordersController.js Outdated Show resolved Hide resolved
API/src/queries/orderQueries.js Outdated Show resolved Hide resolved
@@ -0,0 +1,143 @@
import { param, body, validationResult } from 'express-validator/check';
import { sanitizeBody } from 'express-validator/filter';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you allowed to use a third-party validator (package) to do this. Please do whatever validation is needed with your own custom code.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ok. I will remove that but I won't be able to sanitize user's inputs. Isn't that necessary?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You should be able to sanitize with your own custom code.

Procfile Show resolved Hide resolved
@sudhons sudhons force-pushed the feature-api-endpoints-160445750 branch from 739f096 to 440ef83 Compare September 19, 2018 13:25
@dogoyaro
Copy link
Collaborator

You can go on to merge this after squashing unnecessary commits.

[Chore #160445843] Set up eslint and airbnb sytle guide

[Chore #160446860] set up babel transpiler

[Chore #160446860] set up babel transpiler

[Chore #160445853] set up mocha testing framework

write queries to get all orders

write test to get orders when there are no orders

add controller to get all questions

add controller to get all questions

create queries for querying data

write more tests

write tests for queries

add build folder to .gitignore

[chore #160463621]set up travis and coveralls

[bug #160464176] reconfigure travis

[bug #160464176] reconfigure travis

write test to confirm customer value exist

[chore #160478899] install express validator

[chore #160479790] write vaidation for post order route
write tests for get an order route

write tests for get an order route

write route to get a single order
write tests for update route

write validator for update route
[Chore #160528618] configures API for heroku deployment
@sudhons sudhons force-pushed the feature-api-endpoints-160445750 branch from 440ef83 to bfa500e Compare September 19, 2018 18:35
@sudhons sudhons merged commit 2e3ee10 into develop Sep 19, 2018
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.

2 participants