Add Swagger documentation setup and update route parameters in controllers#289
Add Swagger documentation setup and update route parameters in controllers#289Akshat-Kalra wants to merge 5 commits intoubco-db:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR integrates Swagger UI for API documentation in development mode and refactors several controller routes to replace unsupported optional path parameters (:param?) with query parameters.
- Add SwaggerModule setup guarded by
NODE_ENV === 'development' - Update route decorators in organization and course controllers to use
@Queryinstead of optional:param? - Add
@nestjs/swaggerdependency inpackage.json
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/server/src/organization/organization.controller.ts | Converted :page? path params to @Query('page') |
| packages/server/src/course/course.controller.ts | Made role a query param instead of optional path segment |
| packages/server/src/bootstrap.ts | Imported SwaggerModule, built DocumentBuilder, and set up UI |
| packages/server/package.json | Added @nestjs/swagger dependency |
Comments suppressed due to low confidence (1)
packages/server/src/bootstrap.ts:66
- [nitpick] The
catstag may be confusing—consider renaming it to match actual API domains (e.g.,organizations,courses, etc.) for clarity in the Swagger UI.
.addTag('cats')
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…eter from GET /organization/:oid/get_users and GET /organization/:oid/get_courses routes
| .addTag('cats') | ||
| .build(); | ||
| const document = SwaggerModule.createDocument(app, config); | ||
| if (process.env.NODE_ENV === 'development') { |
There was a problem hiding this comment.
I would use the !isProd() helper for this since iirc something weird is set up with production's setup but isProd() is known to work
There was a problem hiding this comment.
also I guess I would wrap all of these new lines of code in the same if statement block, just so that no parts of swagger load in proud just an extra precaution, even though it probably doesn't matter
| @Get(':oid/get_users') | ||
| @UseGuards(JwtAuthGuard, OrganizationRolesGuard, EmailVerifiedGuard) | ||
| @Roles(OrganizationRole.ADMIN) | ||
| async getUsers( | ||
| @Param('oid', ParseIntPipe) oid: number, | ||
| @Param('page', ParseIntPipe) page: number, | ||
| @Query('page', new DefaultValuePipe(1), ParseIntPipe) page: number, | ||
| @Query('search') search: string, | ||
| ): Promise<OrgUser[]> { | ||
| const pageSize = 50; |
There was a problem hiding this comment.
this new query syntax for some reason seems to be breaking the tests, I am not sure why but I would maybe try manually testing the endpoint first by opening localhost:3000/organization/users and seeing if the users load up
There was a problem hiding this comment.
Yeah, I even tried the way listed on the path-to-regexp github, using this {/:param} for optionals
AdamFipke
left a comment
AdamFipke
left a comment
There was a problem hiding this comment.
see comments, sorry for taking a bit longer to get around to reviewing it
Description
Added Swagger UI integration only in development mode.
Will be available at port 3002 only in development mode.
Also updated the optional route parameters in some of the endpoints,
/:param?is not supported anymore by path-to-regexp, changed all such instances to{/:param}. Referring to the documentation here: https://github.com/pillarjs/path-to-regexpCloses # (issue number): none
Type of change
yarn installHow Has This Been Tested?
Ran yarn test to check if the endpoints are still working correctly after the parameter change.
Checklist: