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

Davidgu/list unclaimed items query #12

Merged
merged 5 commits into from
Feb 4, 2025
Merged

Conversation

shaply
Copy link
Collaborator

@shaply shaply commented Jan 24, 2025

Description

Create a GET route handler at /api/unclaimedItems that returns a list of all UnclaimedItem records.

Success Criteria

  • If the request does not have a valid session, then GET /api/unclaimedItems returns a 401 status code with JSON body {"message": "Session required"}
  • GET /api/unclaimedItems returns a 200 status code and all UnclaimedItem records as JSON objects (include id, name, quantity, and expirationDate as an ISO-8601 string)
  • The route handler is documented with a description of its function, parameters, and responses
  • There is a test file that covers the success criteria

Checklist

  • The ticket is mentioned above
  • The changes fulfill the success criteria of the ticket
  • The changes were self-reviewed
  • A review was requested

Created documentation and GET for api/unclaimedItems/route.ts
Created authMockUtil and dbMockUtils tools for easier verification and db testing
No idea what I did to authMock but it is working properly
Copy link

netlify bot commented Jan 24, 2025

Deploy Preview for hope-for-haiti ready!

Name Link
🔨 Latest commit ee3e32c
🔍 Latest deploy log https://app.netlify.com/sites/hope-for-haiti/deploys/67a26d9ad70e950008af1f4c
😎 Deploy Preview https://deploy-preview-12--hope-for-haiti.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -13,4 +13,4 @@ beforeEach(() => {
mockReset(authMock);
});

export const authMock = auth as unknown as jest.Mock<() => Session | null>;
export const authMock = auth as unknown as jest.Mock<() => Session | null>;
Copy link
Contributor

Choose a reason for hiding this comment

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

iirc prettier should add a newline? if you are using vs code, you can install the prettier extension and configure it to run on save.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it

Comment on lines 7 to 11
* Handles GET requests to retrieve unclaimed items from the unclaimedItem database.
*
* @returns {NextResponse} On success, returns a status of 200 and a JSON object containing an array of unclaimed items.
* @returns {NextResponse} On authentication error, returns a status of 401 and a JSON object containing an error message.
* @returns {NextResponse} On error, returns a status of 500 and a JSON object containing an error message.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you reformat to match the example

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed it and I think it matches the example

Comment on lines 13 to 31
* @example
* // Example response:
* {
* "unclaimedItems": [
* {
* "id": 1,
* "name": "Banana",
* "quantity": 10,
* "expirationDate": "2025-01-22T14:45:43.241Z"
* },
* {
* "id": 2,
* "name": "Apple",
* "quantity": 100,
* "expirationDate": "2025-01-22T14:45:43.243Z"
* }
* ]
* }
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

let's replace this doc example with a typescript interface:

// at the top of the file somewhere
interface Response {
  numberOfPatients: number;
  organizationType: OrganizationType;
  // ...
}

// handler function
export async function GET(...): Promise<NextResponse<Response>> {
  // ...
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created an interface

export async function GET() {
try {
const session = await auth();
if (!session) return authenticationError('Session required');
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be more extensive (also lowkey this is my bad for giving a bad example 💀 it's updated now)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a if (!session?.user) return error

Comment on lines 41 to 48
} catch (err: unknown) {
if (err instanceof Error) {
console.error(err.message);
} else {
console.error('An unknown error occurred');
}
return internalError();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nextjs already returns a 500 and logs uncaught exceptions from route handlers

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I got rid of the try catch statement

* @param expires Optional, default is ""
*/
export async function validateSession(
user: { id: string, type: UserType } = { id: "1234", type: "ADMIN" },
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: i think this session helper could be more useful if it:

  • didn't provide a default user value but instead..
  • accepted a default user type value
  • randomly generated a user ID and returned it (this is to prevent magic numbers from existing in tests)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Need to provide user type now, default expiration is now a day from current day, and randomly generates a user id. Returns the generated session.

@@ -27,3 +27,7 @@ export function internalError() {
export function ok() {
return NextResponse.json({ message: "OK" }, { status: 200 });
}

export function successResponse<T>(data: T) {
return NextResponse.json(data, { status: 200 });
Copy link
Contributor

Choose a reason for hiding this comment

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

i would prefer if you just used NextResponse.json() in the route handler as it defaults to status 200.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Deleted successResponse

Comment on lines 41 to 42
dbMock.unclaimedItem.create({ data: { id: 1, name: "Test Item 1", quantity: 1, expirationDate: new Date() } });
const items: UnclaimedItem[] = await manyUnclaimedItems(3);
Copy link
Contributor

Choose a reason for hiding this comment

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

you might be confused on what the mock does. for every call to db in your route code, there should be the same call to dbMock in the test. e.g. if you only do db.unclaimedItem.findMany in the route, then you should only call dbMock once in your test which will be dbMock.unclaimedItem.findMany

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know if I did it correctly but I changed the test

@kavinphan kavinphan self-requested a review February 1, 2025 21:11
Comment on lines 10 to 17
unclaimedItems:
| {
id: number;
name: string;
quantity: number;
expirationDate: Date | null;
}[]
| [];
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this is kinda wonky. why is this a union type with empty array?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was an error earlier because when I pulled it in the tests it would be null and not an empty array, but I'm not getting the error anymore so I've changed it back to normal.


// Response for GET /api/unclaimedItems
interface UnclaimedItemsResponse {
numberOfItems: number | 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this can just be 0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

!

const unclaimedItems = await db.unclaimedItem.findMany();

return NextResponse.json({
numberOfItems: unclaimedItems?.length, // ?.length for when unclaimedItems is undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to return the number of items

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@shaply shaply merged commit dca05c0 into main Feb 4, 2025
5 checks passed
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