-
Notifications
You must be signed in to change notification settings - Fork 1
Created admin handlers and corresponding unit tests #19
base: main
Are you sure you want to change the base?
Conversation
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.
Couple changes related to test patterns and use of constants - feel free to merge after these are fixed.
|
||
handler(req, mockedRes) | ||
|
||
expect(mockedJson.mock.calls[1][0]).toHaveLength(838) |
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.
Should use ECS_COUNT
and ELW_COUNT
whenever possible
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.
Rather than 838
, we should use ECS_COUNT + ELW_COUNT
to make it more clear how we get 838.
|
||
handler(req, mockedRes) | ||
|
||
expect(mockedJson).toHaveBeenNthCalledWith(1, [{"building_id": ELW_ID, "num": 100, "reported_at": null, "user_id": 1}]) |
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.
We should use toHaveBeenLastCalledWith to simply this
pages/api/admin/deregisterAll.ts
Outdated
if (req.method !== "GET") { | ||
return res.status(405).json({ message: "Method not allowed" }); | ||
} | ||
const deregistered_list = deregisterAll() |
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.
we should rename deregistered_list to something more descriptive (e.g. deregister_count)
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.
This file should be in .gitignore
.
@@ -0,0 +1,20 @@ | |||
const nodemailer = require("nodemailer"); |
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 we be using import
here?
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 purpose of this file?
|
||
handler(req, mockedRes) | ||
|
||
expect(mockedJson.mock.calls[1][0]).toHaveLength(838) |
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.
Rather than 838
, we should use ECS_COUNT + ELW_COUNT
to make it more clear how we get 838.
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.
We should include a test case for calling with a non-GET method for better coverage.
|
||
handler(req, mockedRes) | ||
|
||
expect(mockedJson.mock.calls[1][0]).toHaveLength(1) |
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.
We should also test for correct format of the returned registration object.
const email = req.query.email as string; | ||
const restrict_email = req.query.restrict_email as unknown as boolean; | ||
|
||
if (isNaN(building) || isNaN(number) || !isValidName(name)) { |
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.
We should also validate the email - the regex that HTML5 uses is /^[a-zA-Z0-9.!#$%&’*+/=?^_`{|}~-]+@[a-zA-Z0-9-]+(?:\.[a-zA-Z0-9-]+)*$/
return res.status(404).json({ message: "Unable to register locker because it doesn't exist or it has already been registered" }); | ||
} | ||
|
||
return res.status(201).json(registration); |
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.
This should be status 200
.
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.
Can you add a TODO or other comment to indicate that this is for testing purposes?
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.
Same thing as in api/hello.ts
.
No description provided.