-
Notifications
You must be signed in to change notification settings - Fork 280
feat: improve application update logic and validation #2561
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,7 +69,7 @@ const validateApplicationData = async (req: CustomRequest, res: CustomResponse, | |
| } | ||
| }; | ||
|
|
||
| const validateApplicationUpdateData = async (req: CustomRequest, res: CustomResponse, next: NextFunction) => { | ||
| const validateApplicationFeedbackData = async (req: CustomRequest, res: CustomResponse, next: NextFunction) => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. update this validator name as need separate validator that valid user update flow and this one will validate the feedback api flow |
||
| const schema = joi | ||
| .object({ | ||
| status: joi | ||
|
|
@@ -113,6 +113,68 @@ const validateApplicationUpdateData = async (req: CustomRequest, res: CustomResp | |
| } | ||
| }; | ||
|
|
||
| const validateApplicationUpdateData = async (req: CustomRequest, res: CustomResponse, next: NextFunction) => { | ||
| if (req.body.socialLink?.phoneNo) { | ||
| req.body.socialLink.phoneNo = req.body.socialLink.phoneNo.trim(); | ||
| } | ||
|
|
||
| const socialLinkSchema = joi | ||
| .object({ | ||
| phoneNo: joi.string().optional().regex(phoneNumberRegex).message('"phoneNo" must be in a valid format'), | ||
| github: joi.string().min(1).optional(), | ||
| instagram: joi.string().min(1).optional(), | ||
| linkedin: joi.string().min(1).optional(), | ||
| twitter: joi.string().min(1).optional(), | ||
| peerlist: joi.string().min(1).optional(), | ||
| behance: joi.string().min(1).optional(), | ||
| dribbble: joi.string().min(1).optional(), | ||
| }) | ||
| .optional(); | ||
|
|
||
| const professionalSchema = joi | ||
| .object({ | ||
| institution: joi.string().min(1).optional(), | ||
| skills: joi.string().min(5).optional(), | ||
| }) | ||
| .optional(); | ||
|
|
||
| const schema = joi | ||
| .object() | ||
| .strict() | ||
| .min(1) | ||
| .keys({ | ||
| imageUrl: joi.string().uri().optional(), | ||
| foundFrom: joi.string().min(1).optional(), | ||
| introduction: joi.string().min(1).optional(), | ||
| forFun: joi | ||
| .string() | ||
| .custom((value, helpers) => customWordCountValidator(value, helpers, 100)) | ||
| .optional(), | ||
| funFact: joi | ||
| .string() | ||
| .custom((value, helpers) => customWordCountValidator(value, helpers, 100)) | ||
| .optional(), | ||
| whyRds: joi | ||
| .string() | ||
| .custom((value, helpers) => customWordCountValidator(value, helpers, 100)) | ||
| .optional(), | ||
| numberOfHours: joi.number().min(1).max(100).optional(), | ||
| professional: professionalSchema, | ||
| socialLink: socialLinkSchema, | ||
| }) | ||
| .messages({ | ||
| "object.min": "Update payload must contain at least one allowed field.", | ||
| }); | ||
|
|
||
| try { | ||
| await schema.validateAsync(req.body); | ||
| next(); | ||
| } catch (error) { | ||
| logger.error(`Error in validating application update data: ${error}`); | ||
| res.boom.badRequest(error.details[0].message); | ||
| } | ||
| }; | ||
|
|
||
| const validateApplicationQueryParam = async (req: CustomRequest, res: CustomResponse, next: NextFunction) => { | ||
| const schema = joi.object().strict().keys({ | ||
| userId: joi.string().optional(), | ||
|
|
@@ -133,6 +195,7 @@ const validateApplicationQueryParam = async (req: CustomRequest, res: CustomResp | |
|
|
||
| module.exports = { | ||
| validateApplicationData, | ||
| validateApplicationFeedbackData, | ||
| validateApplicationUpdateData, | ||
| validateApplicationQueryParam, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,11 +17,12 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.get("/:applicationId", authenticate, authorizeRoles([SUPERUSER]), applications.getApplicationById); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.post("/", authenticate, applicationValidator.validateApplicationData, applications.addApplication); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.patch("/:applicationId", authenticate, applicationValidator.validateApplicationUpdateData, applications.updateApplication); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Check failureCode scanning / CodeQL Missing rate limiting High
This route handler performs
authorization Error loading related location Loading This route handler performs authorization Error loading related location Loading This route handler performs authorization Error loading related location Loading
Copilot AutofixAI 1 day ago In general, the problem is fixed by adding a rate-limiting middleware to routes that perform authenticated, potentially expensive operations (like updating an application). In an Express app, this is typically done by using a well-known library such as For this file, the least intrusive and most effective fix is to:
To cover the flagged route and the other similar, authenticated write routes, we can define a single limiter and apply it to all modifying endpoints (
Suggested changeset
2
routes/applications.ts
package.json
Outside changed files
This fix introduces these dependencies
Copilot is powered by AI and may make mistakes. Always verify output.
Refresh and try again.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.patch( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "/:applicationId/feedback", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| authenticate, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| authorizeRoles([SUPERUSER]), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| applicationValidator.validateApplicationUpdateData, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| applicationValidator.validateApplicationFeedbackData, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| applications.submitApplicationFeedback | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
AnujChhikara marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.patch("/:applicationId/nudge", authenticate, applications.nudgeApplication); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -689,7 +689,7 @@ describe("Application", function () { | |
| expect(res.body.nudgeCount).to.be.equal(1); | ||
|
|
||
| const twentyFiveHoursAgo = new Date(Date.now() - 25 * 60 * 60 * 1000).toISOString(); | ||
| applicationModel.updateApplication({ lastNudgeAt: twentyFiveHoursAgo }, nudgeApplicationId).then(() => { | ||
| applicationModel.updateApplication({ lastNudgeAt: twentyFiveHoursAgo }, nudgeApplicationId, userId).then(() => { | ||
| chai | ||
|
Comment on lines
691
to
693
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Decouple nudge test setup from edit‑cooldown logic.
🤖 Prompt for AI Agents |
||
| .request(app) | ||
| .patch(`/applications/${nudgeApplicationId}/nudge`) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,23 +111,23 @@ describe("application validator test", function () { | |
| status: "accepted", | ||
| feedback: "some feedback", | ||
| }; | ||
| await applicationValidator.validateApplicationUpdateData(req, res, nextSpy); | ||
| await applicationValidator.validateApplicationFeedbackData(req, res, nextSpy); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need to update these as changes the validator name |
||
| expect(nextSpy.callCount).to.equal(1); | ||
| }); | ||
|
|
||
| it("should not call next function if any value other than status and feedback is passed", async function () { | ||
| req.body = { | ||
| batman: true, | ||
| }; | ||
| await applicationValidator.validateApplicationUpdateData(req, res, nextSpy); | ||
| await applicationValidator.validateApplicationFeedbackData(req, res, nextSpy); | ||
| expect(nextSpy.callCount).to.equal(0); | ||
| }); | ||
|
|
||
| it("should not call the next function if any value which is not allowed is sent in status", async function () { | ||
| req.body = { | ||
| status: "something", | ||
| }; | ||
| await applicationValidator.validateApplicationUpdateData(req, res, nextSpy); | ||
| await applicationValidator.validateApplicationFeedbackData(req, res, nextSpy); | ||
| expect(nextSpy.callCount).to.equal(0); | ||
| }); | ||
|
|
||
|
|
@@ -136,7 +136,7 @@ describe("application validator test", function () { | |
| status: "accepted", | ||
| feedback: "Great work!", | ||
| }; | ||
| await applicationValidator.validateApplicationUpdateData(req, res, nextSpy); | ||
| await applicationValidator.validateApplicationFeedbackData(req, res, nextSpy); | ||
| expect(nextSpy.callCount).to.equal(1); | ||
| }); | ||
|
|
||
|
|
@@ -145,7 +145,7 @@ describe("application validator test", function () { | |
| status: "rejected", | ||
| feedback: "Not a good fit", | ||
| }; | ||
| await applicationValidator.validateApplicationUpdateData(req, res, nextSpy); | ||
| await applicationValidator.validateApplicationFeedbackData(req, res, nextSpy); | ||
| expect(nextSpy.callCount).to.equal(1); | ||
| }); | ||
|
|
||
|
|
@@ -154,15 +154,15 @@ describe("application validator test", function () { | |
| status: "changes_requested", | ||
| feedback: "Please update your skills section", | ||
| }; | ||
| await applicationValidator.validateApplicationUpdateData(req, res, nextSpy); | ||
| await applicationValidator.validateApplicationFeedbackData(req, res, nextSpy); | ||
| expect(nextSpy.callCount).to.equal(1); | ||
| }); | ||
|
|
||
| it("should not call next function when status is changes_requested without feedback", async function () { | ||
| req.body = { | ||
| status: "changes_requested", | ||
| }; | ||
| await applicationValidator.validateApplicationUpdateData(req, res, nextSpy); | ||
| await applicationValidator.validateApplicationFeedbackData(req, res, nextSpy); | ||
| expect(nextSpy.callCount).to.equal(0); | ||
| }); | ||
|
|
||
|
|
@@ -171,7 +171,7 @@ describe("application validator test", function () { | |
| status: "changes_requested", | ||
| feedback: "", | ||
| }; | ||
| await applicationValidator.validateApplicationUpdateData(req, res, nextSpy); | ||
| await applicationValidator.validateApplicationFeedbackData(req, res, nextSpy); | ||
| expect(nextSpy.callCount).to.equal(0); | ||
| }); | ||
|
|
||
|
|
@@ -180,7 +180,7 @@ describe("application validator test", function () { | |
| status: "accepted", | ||
| feedback: "", | ||
| }; | ||
| await applicationValidator.validateApplicationUpdateData(req, res, nextSpy); | ||
| await applicationValidator.validateApplicationFeedbackData(req, res, nextSpy); | ||
| expect(nextSpy.callCount).to.equal(1); | ||
| }); | ||
|
|
||
|
|
@@ -189,23 +189,23 @@ describe("application validator test", function () { | |
| status: "rejected", | ||
| feedback: "", | ||
| }; | ||
| await applicationValidator.validateApplicationUpdateData(req, res, nextSpy); | ||
| await applicationValidator.validateApplicationFeedbackData(req, res, nextSpy); | ||
| expect(nextSpy.callCount).to.equal(1); | ||
| }); | ||
|
|
||
| it("should not call next function when status is missing", async function () { | ||
| req.body = { | ||
| feedback: "Some feedback", | ||
| }; | ||
| await applicationValidator.validateApplicationUpdateData(req, res, nextSpy); | ||
| await applicationValidator.validateApplicationFeedbackData(req, res, nextSpy); | ||
| expect(nextSpy.callCount).to.equal(0); | ||
| }); | ||
|
|
||
| it("should not call next function when status is null", async function () { | ||
| req.body = { | ||
| status: null, | ||
| }; | ||
| await applicationValidator.validateApplicationUpdateData(req, res, nextSpy); | ||
| await applicationValidator.validateApplicationFeedbackData(req, res, nextSpy); | ||
| expect(nextSpy.callCount).to.equal(0); | ||
| }); | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,16 +109,6 @@ describe("applications", function () { | |
| }); | ||
| }); | ||
|
|
||
| describe("updateApplication", function () { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will refactor and add this test in the test pr of the edit applications |
||
| it("should update a particular application", async function () { | ||
| const dataToUpdate = { status: "accepted" }; | ||
| await ApplicationModel.updateApplication(dataToUpdate, applicationId1); | ||
| const application = await ApplicationModel.getApplicationById(applicationId1); | ||
|
|
||
| expect(application.status).to.be.equal("accepted"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("addApplicationFeedback", function () { | ||
| let testApplicationId: string; | ||
| const reviewerName = "test-reviewer"; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.