-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add MongoDB test application #7
Conversation
I've included cli_demo.mp4 |
@AlexLLKong I think it's best to only open the connection once, and close it at the end of the app's lifetime – seems like that's the best practice. I've made that change in the latest commit. |
I've added some
|
Don't have proper tests yet, but everything works circa the latest commit: TBD:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff! Are the Insomnia tests only on your local machine? Can you make them available to the rest of the team through the repo somehow?
@@ -82,6 +84,15 @@ try { | |||
|
|||
|
|||
app.use(express.json()); | |||
app.use(session( | |||
{ | |||
secret: "shhhh...secret", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, still need to figure out how to keep secrets properly. Hng.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to write a proper secret-grabbing thing after #9 is merged.
mongoDbTest.js
Outdated
res.status(401).send("Invalid email"); | ||
} else if (results.length > 1) { | ||
// Many users with same email | ||
// TODO: This is bad and should let someone know |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth keeping a log file for this kind of thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what error code this should be, not sure when the error case would be hit… not sure of much of anything, but this shouldn't be a 400
. Minor quibble, really – don't worry.
mongoDbTest.js
Outdated
const date = new Date().toISOString(); | ||
const achievements = req.body.achievements; | ||
const admin = false; | ||
const kit = req.body.kit; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the kit be created without any involvement from the code calling into the API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a user able to start building a kit before signing up? If not, then I guess that can be changed to a default skeleton object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the expected value for the kit
property in a user's document? I think this might just be me being confused, I should take a look at the schema.
public/js/mongoTestClient.js
Outdated
headers: { | ||
'Content-Type' : 'application/json' | ||
}, | ||
body: JSON.stringify({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess whether we send the request data in the body or the query parameters doesn't really matter, but I thought we were going to use query parameters. 😅
If you don't want to change it, just say so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, actually, don't change this! It makes sense for this to be in a body parameter, at least from reading this blog post.
General rule seems to be:
- Path parameters for getting a specific thing out of the API
- For example: GET
/people/katy_p
- For example: GET
- Query parameters for filtering, or otherwise limiting the output of a query for lots of things
- For example: GET
/people/katy_p/friends?limit=10&nameLike=Alex
- For example: GET
- Body parameters for supplying data needed to fulfill a request to the endpoint
- For example: POST
/create-cat
, body:{name:"Fluffy"}
- For example: POST
Once #9 is merged, we should rebase this branch on top of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly good, take a look at my suggestions though.
9b3ce93
to
adca4bd
Compare
AT LAST! Latest commit should super-duper fix #8. @AlexLLKong if you could make up a random session secret on the Here: app.use(session(
{
secret: "shhhh...secret", // load it in from the secrets object instead!
name: "ShakeGuardSessionID",
resave: false,
// create a unique identifier for that client
saveUninitialized: true
})
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that seems to fix everything I brought up, excellent work. Maybe do a nice thing for our front-end folks and document what all the messages mean somewhere – API.md
or something? Maybe there's a better way, or maybe it's a bit early to put effort into that kind of thing, but would be nice. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops.
Co-authored-by: Katy Petrova <[email protected]>
Co-authored-by: Katy Petrova <[email protected]>
Test application to demonstrate rudimentary CRUD operations on locally hosted node application and node instance.
There isn't much error handling.