Route Refactoring #342
Route Refactoring #342
Conversation
|
Throughout the development of this revision, I have changed the structure of the routes to accommodate a routes file and a functions file for each subcategory of routes. Simply put, the routes have the actual HTTP requests, and call the associated functions for those routes. Those functions then return a promise to provide data. |
| db_router.use("/", filesRoutes(db)); | ||
| db_router.use("/", semesterRoutes(db)); | ||
| db_router.use("/", devOnlyRoutes(db)); | ||
| db_router.use("/", dashboardRoutes(db)); | ||
| const CONSTANTS = require("../consts"); | ||
| const { ROLES } = require("../consts"); | ||
| const { off } = require("process"); |
There was a problem hiding this comment.
The import "const {off} = require ("process")" appears unused, consider removing it to avoid confusion.
| // Attachment Handling | ||
| if (files && files.files) { | ||
| // If there is only one attachment, then it does not come as a list | ||
| if (files.files.length === undefined) { |
There was a problem hiding this comment.
Before checking files.files.length, consider guarding against files or files.files being undefined to avoid runtime errors when no files are sent.
| if (err) { | ||
| reject(err); | ||
| return; | ||
| } |
There was a problem hiding this comment.
for getFiles, this is good but one suggestion, maybe collect and return a list of files that failed statSync so callers can optionally log or surface that.
Rtyujklop
left a comment
Rtyujklop
left a comment
There was a problem hiding this comment.
This is an excellent refactoring that significantly improves code organization and maintainability. Breaking down the monolithic db_routes.js file into domain-specific modules makes the codebase much easier to navigate and extend. Just go over these issues and it should be good.
| "/createAction", | ||
| [UserAuth.isAdmin, UserAuth.canWrite, body("page_html").unescape()], | ||
| db_router.get( | ||
| "/getActiveTimelines", |
There was a problem hiding this comment.
Could move to dashboard_routes.js or a new timelines_routes.js
| db_router.get("/getHtml", (req, res, next) => { | ||
| let getHtmlQuery = `SELECT * FROM page_html`; | ||
| let queryParams = []; | ||
| //If there is a query parameter, then select html from specified table. | ||
| if (typeof req.query.name !== "undefined" && req.query.name) { | ||
| getHtmlQuery = `SELECT html FROM page_html WHERE name = ?`; | ||
| queryParams = [req.query.name]; | ||
| } | ||
| db.query(getHtmlQuery, queryParams) |
There was a problem hiding this comment.
Could move to a new content_routes.js or pages_routes.js For getHTML
| .withMessage("Cannot be empty") | ||
| .isLength({ max: 50 }), | ||
| ], | ||
| "/editPage", |
There was a problem hiding this comment.
Could move to a new content_routes.js or pages_routes.js for /editPage route similar to /getHtml
| let params = []; | ||
|
|
||
| switch (user.type) { | ||
| case ROLES.STUDENT: |
There was a problem hiding this comment.
There is a SQL injection risk, While user probably comes from authenticated session data, it's safer to use parameterized queries consistently. you could refactor this function with: Build the filter with placeholders,
return both the filter string and the parameters array, and pass parameters to db.query()
| } | ||
|
|
||
| const darkModeRaw = result[0].dark_mode; | ||
| const dark_mode = Boolean(darkModeRaw); |
There was a problem hiding this comment.
This works, but explicitly checking values like you do in other view preference functions, is more consistent and handles edge cases better.
| const getPeerEvalLogsQuery = `SELECT action_log.*, users.fname, users.lname, users.type | ||
| FROM action_log | ||
| LEFT JOIN users ON action_log.system_id = users.system_id | ||
| WHERE action_template IN (${actionIds.join(",")}) |
There was a problem hiding this comment.
This is vulnerable to SQL injection if action_id values are user-controlled. Maybe use parameterized queries instead.
#309
DB Routes is an unnecessarily long file at the moment. These changes should move a lot of the bulk in it over to separate subfiles.