feat: improve application update logic and validation #2561
Conversation
…ror handling - Added a new error message for editing applications too soon. - Implemented a function to build the update payload for applications. - Updated the application update logic to include user authorization and time-based restrictions. - Refactored the application validator to include comprehensive validation for update data. - Adjusted routes to use the new validation function for application updates.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThis PR implements a 24-hour edit cooldown mechanism for application updates. It introduces an error constant, refactors the update controller with a payload builder, expands validation to distinguish between update and feedback scenarios, adds authorization and cooldown checks in the model layer, and introduces a new PATCH route for applications while updating tests to match the revised signatures. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Validator
participant Model
participant Firestore
Client->>Controller: PATCH /applications/:id<br/>(update data, auth)
Controller->>Validator: validateApplicationUpdateData()
Validator-->>Controller: ✓ valid | ✗ 400
Controller->>Controller: buildApplicationUpdatePayload()
Controller->>Model: updateApplication(payload, id, userId)
rect rgba(100, 150, 200, 0.5)
Note over Model: Transaction Start
Model->>Firestore: Get application by id
alt Application not found
Model-->>Controller: {status: notFound}
else userId mismatch
Model-->>Controller: {status: unauthorized}
else lastEditAt < 24 hours ago
Model-->>Controller: {status: tooSoon}
else Valid
Model->>Firestore: Update data + lastEditAt
Model-->>Controller: {status: success}
end
Note over Model: Transaction End
end
Controller->>Firestore: Log update (on success only)
Controller-->>Client: 200 success | 404/401/409 error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ); | ||
| router.get("/:applicationId", authenticate, authorizeRoles([SUPERUSER]), applications.getApplicationById); | ||
| router.post("/", authenticate, applicationValidator.validateApplicationData, applications.addApplication); | ||
| router.patch("/:applicationId", authenticate, applicationValidator.validateApplicationUpdateData, applications.updateApplication); |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 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 express-rate-limit and applying a limiter either to all routes in the router or specifically to sensitive routes (e.g., write operations).
For this file, the least intrusive and most effective fix is to:
- Import
express-rate-limit. - Define a limiter instance tailored for these routes (e.g., a reasonable per-IP cap over a time window).
- Insert that limiter as middleware in the relevant routes’ middleware chains.
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 (POST /, PATCH /:applicationId, PATCH /:applicationId/feedback, and PATCH /:applicationId/nudge). This preserves existing authentication/authorization/validation and only adds an extra guard against abuse. Concretely in routes/applications.ts, we will:
- Add
const rateLimit = require("express-rate-limit");near the top. - Define a limiter, e.g.
const applicationWriteLimiter = rateLimit({ windowMs: 15 * 60 * 1000, max: 100 });after the router is created. - Update the relevant route definitions to include
applicationWriteLimiterbeforeauthenticate(or immediately after, either is acceptable as long as it runs early) in the middleware arrays.
| @@ -5,9 +5,15 @@ | ||
| const applications = require("../controllers/applications"); | ||
| const { authorizeOwnOrSuperUser } = require("../middlewares/authorizeOwnOrSuperUser"); | ||
| const applicationValidator = require("../middlewares/validators/application"); | ||
| const rateLimit = require("express-rate-limit"); | ||
|
|
||
| const router = express.Router(); | ||
|
|
||
| const applicationWriteLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // limit each IP to 100 write requests per windowMs | ||
| }); | ||
|
|
||
| router.get( | ||
| "/", | ||
| authenticate, | ||
| @@ -16,15 +19,16 @@ | ||
| applications.getAllOrUserApplication | ||
| ); | ||
| router.get("/:applicationId", authenticate, authorizeRoles([SUPERUSER]), applications.getApplicationById); | ||
| router.post("/", authenticate, applicationValidator.validateApplicationData, applications.addApplication); | ||
| router.patch("/:applicationId", authenticate, applicationValidator.validateApplicationUpdateData, applications.updateApplication); | ||
| router.post("/", applicationWriteLimiter, authenticate, applicationValidator.validateApplicationData, applications.addApplication); | ||
| router.patch("/:applicationId", applicationWriteLimiter, authenticate, applicationValidator.validateApplicationUpdateData, applications.updateApplication); | ||
| router.patch( | ||
| "/:applicationId/feedback", | ||
| applicationWriteLimiter, | ||
| authenticate, | ||
| authorizeRoles([SUPERUSER]), | ||
| applicationValidator.validateApplicationFeedbackData, | ||
| applications.submitApplicationFeedback | ||
| ); | ||
| router.patch("/:applicationId/nudge", authenticate, applications.nudgeApplication); | ||
| router.patch("/:applicationId/nudge", applicationWriteLimiter, authenticate, applications.nudgeApplication); | ||
|
|
||
| module.exports = router; |
| @@ -42,7 +42,8 @@ | ||
| "passport-github2": "0.1.12", | ||
| "passport-google-oauth20": "^2.0.0", | ||
| "rate-limiter-flexible": "5.0.3", | ||
| "winston": "3.13.0" | ||
| "winston": "3.13.0", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/chai": "4.3.16", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
| }); | ||
| }); | ||
|
|
||
| describe("updateApplication", function () { |
There was a problem hiding this comment.
will refactor and add this test in the test pr of the edit applications
| feedback: "some feedback", | ||
| }; | ||
| await applicationValidator.validateApplicationUpdateData(req, res, nextSpy); | ||
| await applicationValidator.validateApplicationFeedbackData(req, res, nextSpy); |
There was a problem hiding this comment.
need to update these as changes the validator name
| }; | ||
|
|
||
| const validateApplicationUpdateData = async (req: CustomRequest, res: CustomResponse, next: NextFunction) => { | ||
| const validateApplicationFeedbackData = async (req: CustomRequest, res: CustomResponse, next: NextFunction) => { |
There was a problem hiding this comment.
update this validator name as need separate validator that valid user update flow and this one will validate the feedback api flow
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middlewares/validators/application.ts (1)
107-112: Fix misleading log message.The error log says “recruiter data” but this validator handles application feedback. This will confuse debugging and alerting.
🔧 Suggested fix
- logger.error(`Error in validating recruiter data: ${error}`); + logger.error(`Error in validating application feedback data: ${error}`);
🤖 Fix all issues with AI agents
In `@controllers/applications.ts`:
- Around line 125-133: The updateApplication handler currently calls
ApplicationModel.updateApplication even when
buildApplicationUpdatePayload(rawBody) returns an empty object, causing only
lastEditAt to change and producing misleading success logs; modify the
updateApplication function to check the returned dataToUpdate (from
buildApplicationUpdatePayload) and if it has no own enumerable properties (i.e.,
no updatable fields), respond with a 400/422 error and short-circuit without
calling ApplicationModel.updateApplication or logging success—otherwise proceed
as before using userId and applicationId.
In `@middlewares/validators/application.ts`:
- Around line 141-163: The update schema currently allows an empty object
because no keys are required; modify the schema (the joi.object() assigned to
schema in middlewares/validators/application.ts) to require at least one field
by adding a constraint such as .min(1) (or alternatively
.or('imageUrl','foundFrom','introduction','forFun','funFact','whyRds','numberOfHours','professional','socialLink'))
to the object chain so that empty payloads are rejected while keeping existing
validators like customWordCountValidator, professionalSchema and
socialLinkSchema intact.
In `@routes/applications.ts`:
- Around line 20-26: The two PATCH routes (router.patch("/:applicationId", ...
applications.updateApplication) and router.patch("/:applicationId/feedback", ...
applications.submitApplicationFeedback)) lack rate limiting; import and apply
the project's standard write-rate-limit middleware (symbol name
writeRateLimiter) to both routes and place it after authenticate and before the
validators/handlers so the order becomes: authenticate, writeRateLimiter,
applicationValidator.*, then the controller (applications.updateApplication /
applications.submitApplicationFeedback).
In `@test/integration/application.test.ts`:
- Around line 691-693: The test is mutating lastNudgeAt via
applicationModel.updateApplication which now enforces edit cooldown and touches
lastEditAt, coupling the test to edit logic; change the setup to directly update
the document (bypassing updateApplication) or use a dedicated test helper to
backdate lastNudgeAt so you only modify lastNudgeAt without triggering cooldown
logic—locate the call using applicationModel.updateApplication,
nudgeApplicationId and userId and replace it with a direct DB/document update or
helper that sets lastNudgeAt to twentyFiveHoursAgo while leaving lastEditAt
untouched.
| 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 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Decouple nudge test setup from edit‑cooldown logic.
updateApplication now enforces edit cooldown and updates lastEditAt, so this setup mutation is coupled to edit logic and may become brittle if lastEditAt is present. Prefer a direct document update or a dedicated test helper for backdating lastNudgeAt.
🤖 Prompt for AI Agents
In `@test/integration/application.test.ts` around lines 691 - 693, The test is
mutating lastNudgeAt via applicationModel.updateApplication which now enforces
edit cooldown and touches lastEditAt, coupling the test to edit logic; change
the setup to directly update the document (bypassing updateApplication) or use a
dedicated test helper to backdate lastNudgeAt so you only modify lastNudgeAt
without triggering cooldown logic—locate the call using
applicationModel.updateApplication, nudgeApplicationId and userId and replace it
with a direct DB/document update or helper that sets lastNudgeAt to
twentyFiveHoursAgo while leaving lastEditAt untouched.
Date: 30 Jan 2026
Developer Name: @AnujChhikara
Issue Ticket Number
Tech Doc Link
Business Doc Link
Description
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Test Coverage
Screenshot 1
Additional Notes