Implement certificate generation and approval workflow#255
Implement certificate generation and approval workflow#255KotapatiSaiMounika wants to merge 2 commits into
Conversation
|
@KotapatiSaiMounika is attempting to deploy a commit to the openlake's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds certificate batch creation and approval workflows, template CRUD, PDF certificate generation, Cloudinary support, and frontend interfaces for batches, requests, templates, and certificates. ChangesCertificate platform
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Coordinator
participant BatchAPI
participant Approver
participant CertificateService
participant Student
Coordinator->>BatchAPI: Create and submit batch
BatchAPI-->>Approver: Expose submitted request
Approver->>BatchAPI: Approve or reject batch
BatchAPI->>CertificateService: Generate certificates after final approval
CertificateService-->>Student: Store certificate record
Student->>BatchAPI: Fetch certificates
BatchAPI-->>Student: Return certificate list
Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (27)
frontend/src/Components/Templates/Template.jsx-327-329 (1)
327-329: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
t.title.includes("Copy")is a fragile and incorrect authorization proxy.Both the grid view (line 329) and list view (line 495) use
t.title.includes("Copy")to decide whether to show Edit/Delete buttons. This grants edit/delete UI to any user for any template whose title happens to contain "Copy", regardless of ownership or role. Conversely, a duplicated template renamed to "Duplicate" would not show the buttons.If the intent is to let creators edit their own templates, check ownership instead:
🔧 Proposed fix
- {(role === "PRESIDENT" || - role.startsWith("GENSEC") || - t.title.includes("Copy")) ? ( + {(role === "PRESIDENT" || + role.startsWith("GENSEC") || + t.createdBy?._id === currentUser?._id) ? (Apply the same change at line 495. This requires passing the current user ID from
useAdminContext.Also applies to: 493-495
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Templates/Template.jsx` around lines 327 - 329, Replace the title-based authorization condition in both the grid and list view action controls with an ownership check using the current user ID from useAdminContext, while preserving PRESIDENT and GENSEC role access. Update the relevant Template.jsx logic near the existing role checks so edit/delete buttons are shown for authorized roles or templates owned by the current user, and apply the same change in both locations.frontend/src/Components/Templates/Template.jsx-93-102 (1)
93-102: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
filtersuseMemo crashes whencreatedByis null.Line 98 accesses
t.createdBy.personal_info.namewithout optional chaining. If any template has a nullcreatedBy(e.g., the creator's account was deleted), this throwsTypeError: Cannot read properties of null (reading 'personal_info')and crashes the entire component. Thefilteredfunction at line 81 guards against this, butfiltersdoes not.🛡️ Proposed fix
const filters = useMemo( () => ({ categories: [...new Set(templates?.map((t) => t.category))], statuses: [...new Set(templates?.map((t) => t.status))], creators: [ - ...new Set(templates?.map((t) => t.createdBy.personal_info.name)), + ...new Set( + templates + ?.filter((t) => t?.createdBy?.personal_info?.name) + .map((t) => t.createdBy.personal_info.name) + ), ], }), [templates], );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Templates/Template.jsx` around lines 93 - 102, Update the creators mapping in the filters useMemo to safely handle templates with null or missing createdBy data, matching the defensive behavior already used by filtered. Preserve valid creator names while preventing the component from throwing when creator information is absent.backend/controllers/templateController.js-60-73 (1)
60-73: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
designdefault andcategoryrequirement mismatch with schema.Two contract inconsistencies between the controller and
templateSchema.js:
The controller defaults
designto"default.html"(line 70), but the schema default is"Default"and the frontend dropdown (TemplateModal.jsx:24-28) only offers["Default", "Classic", "Modern"]. A template created via direct API call withoutdesignwould store a value not present in the frontend dropdown.The controller requires
category(line 60), but the schema does not mark it asrequired. Templates created through other paths (seeds, other controllers) could lackcategoryand would be silently hidden by the frontend filter atTemplate.jsx:81.🔧 Proposed fix
- design: design || "default.html", + design: design || "Default",Additionally, add
required: trueto thecategoryfield intemplateSchema.jsto align the schema with the controller's validation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/templateController.js` around lines 60 - 73, Align template creation and validation contracts: update the design fallback in the template creation flow to use the schema/frontend value “Default” instead of “default.html”, and mark the category field as required in templateSchema so all creation paths enforce the controller’s requirement. Use the existing template creation logic and category schema definition without changing unrelated behavior.frontend/src/services/templates.js-4-91 (1)
4-91: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInconsistent error handling pattern breaks all component error handling.
Four service functions (
fetchTemplates,fetchTemplate,createTemplate,deleteTemplate) catch errors and returnerr.response?.data?.message(a string orundefined) instead of throwing. This makes everycatchblock in consuming components dead code:
Template.jsx:loadTemplates— silently swallows fetch errors; user sees an empty list with no explanation.Template.jsx:del—deleteTemplatenever throws, sotoast.error("Failed to archive template.")never fires;loadTemplates()runs unconditionally, making the failed delete appear as a no-op.TemplateModal.jsx:handleSubmit—createTemplate/updateTemplatenever throw, so thecatchtoast is dead code; the modal closes viahandleClose()even on API failure, losing the user's form data without feedback.Meanwhile
archiveTemplate(line 79-90) uses the correct pattern (re-throws after logging) but duplicatesdeleteTemplate's API call.Align all functions on throw-after-log:
♻️ Proposed fix (apply to all four functions)
export async function fetchTemplates() { try { const res = await api.get("/api/templates"); return { data: res.data.message, status: res.status, }; } catch (err) { console.error("fetchTemplates error:", err); - return err.response?.data?.message; + throw err; } }Apply the same
throw errreplacement tofetchTemplate,createTemplate, anddeleteTemplate. RemovearchiveTemplateas it becomes identical todeleteTemplate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/services/templates.js` around lines 4 - 91, Update fetchTemplates, fetchTemplate, createTemplate, and deleteTemplate to rethrow the caught error after logging instead of returning err.response?.data?.message, so consuming component catch handlers execute. Remove archiveTemplate because deleteTemplate now provides the same throw-after-log behavior, while preserving each function’s existing API call and successful response shape.backend/controllers/certificateController.js-38-38 (1)
38-38: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDate formatted as
en-GBstring breaks frontend parsing.
toLocaleDateString("en-GB")produces"DD/MM/YYYY"(e.g.,"15/01/2024"). The frontend'sformatDatethen callsnew Date(dateString)on this value, which is implementation-dependent and returnsInvalid Datein most browsers. Send the raw ISO date or timestamp and let the frontend handle all formatting.🐛 Proposed fix
- date: new Date(cert.createdAt).toLocaleDateString("en-GB"), + date: cert.createdAt,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/certificateController.js` at line 38, Update the certificate response date field to return the raw cert.createdAt value (or an ISO-compatible timestamp) instead of formatting it with toLocaleDateString("en-GB"), so the frontend formatDate can parse and format it consistently.backend/controllers/certificateBatchController.js-143-212 (1)
143-212: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
editBatchlacks ownership authorization — any coordinator can edit any batch.
deleteBatchandarchiveBatchboth filter byinitiatedBy: id, buteditBatchfetches bybatchIdonly and never checks thatbatch.initiatedBymatches the caller. A coordinator could edit batches they didn't create. Additionally, there's no state guard preventing edits to batches that are alreadySubmittedorActive.🔒 Proposed fix
const batch = await CertificateBatch.findById(batchId); if (!batch) { return res.status(404).json({ message: "Batch not found" }); } + if (batch.initiatedBy.toString() !== id) { + return res.status(403).json({ message: "You are not authorized to edit this batch" }); + } + + if (batch.lifecycleStatus === "Submitted" || batch.lifecycleStatus === "Active") { + return res.status(400).json({ message: "Cannot edit a batch that is already submitted or active" }); + } + Object.assign(batch, validation.data);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/certificateBatchController.js` around lines 143 - 212, Update editBatch to fetch or validate the CertificateBatch using both batchId and initiatedBy: id, returning the existing not-found response when the caller does not own the batch. Add a state guard before Object.assign so batches with lifecycleStatus "Submitted" or "Active" cannot be edited, while preserving edits for allowed states.backend/controllers/certificateBatchController.js-104-107 (1)
104-107: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
missing.map()result is discarded — error details sent to client are raw IDs, not structured objects.Line 105 calls
missing.map(...)but doesn't assign the result. Thedetailsfield at line 106 receives the originalmissingarray of raw string IDs, not the intended[{ uid, ok: false, reason: "User not found" }]objects.🐛 Proposed fix
if(missing.length > 0){ - missing.map((uid) => ({ uid, ok: false, reason: "User not found" })); + const details = missing.map((uid) => ({ uid, ok: false, reason: "User not found" })); return res.status(400).json({ message: "Invalid user data sent", details: missing }); + return res.status(400).json({ message: "Invalid user data sent", details }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/certificateBatchController.js` around lines 104 - 107, Update the missing-user handling in the certificate batch controller so the mapped objects from missing are retained and sent in the response details. Replace the discarded missing.map(...) call with an assignment or directly use its result, preserving each uid with ok: false and reason: "User not found".backend/controllers/certificateBatchController.js-428-456 (1)
428-456: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
approverEditBatchhas no try-catch — unhandled errors will crash the request.Every other controller function wraps its body in try-catch, but this one doesn't. If
CertificateBatch.findByIdorbatch.save()throws, Express will return an unhandled error.Additionally, there's no check that the caller is actually an assigned approver for this specific batch — any GENSEC or President can modify participants on any batch.
🛡️ Proposed fix
async function approverEditBatch(req, res) { + try { const { id } = req.user; const user = await User.findById(id); if (!user) { return res.status(404).json({ message: "User not found" }); } if (user.role !== "PRESIDENT" && !user.role.startsWith("GENSEC")) { return res.status(403).json({ message: "Access denied" }); } let { users } = req.body; const validation = validateBatchUsersIds.safeParse(users); if (!validation.success) { let errors = validation.error.issues.map((issue) => issue.message); return res.status(400).json({ message: errors }); } const { _id } = req.body; const batch = await CertificateBatch.findById(_id); if (!batch) { return res.status(404).json({ message: "Batch not found" }); } + if (!batch.approverIds.some((a) => a.toString() === id.toString())) { + return res.status(403).json({ message: "You are not an assigned approver for this batch" }); + } batch.users = users; await batch.save(); res.status(200).json({ message: "Batch updated successfully" }); + } catch (err) { + if (err instanceof HttpError) { + const payload = { message: err.message }; + if (err.details) payload.details = err.details; + return res.status(err.statusCode).json(payload); + } + return res.status(500).json({ message: err.message || "Internal server error" }); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/certificateBatchController.js` around lines 428 - 456, Update approverEditBatch to wrap its database lookups, authorization, mutation, and response flow in the controller’s established try-catch pattern, forwarding failures through the existing error-handling mechanism. Before assigning batch.users, verify that the authenticated user is an assigned approver for the specific batch, while preserving the existing role and not-found checks; reject unauthorized callers without modifying or saving the batch.backend/routes/certificateBatch.js-75-87 (1)
75-87: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse a state-changing method for approve/reject
/api/batches/:batchId/approveand/api/batches/:batchId/rejectmutate batch state, and the frontend already calls them withGET. Switch both the routes andfrontend/src/services/batch.jstoPOSTorPATCHso they aren’t accidentally triggered by prefetching or caching behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/routes/certificateBatch.js` around lines 75 - 87, Change the approveBatch and rejectBatch routes from GET to POST or PATCH, and update the corresponding frontend service calls in batch.js to use the same state-changing method. Preserve the existing paths, authentication, authorization, and handlers while ensuring both operations are no longer invoked through GET requests.backend/controllers/certificateBatchController.js-542-559 (1)
542-559: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAvoid marking the batch Active before certificate generation finishes.
generateCertificates()can throw afterfindOneAndUpdate()has already persistedlifecycleStatus: "Active", and the service also allows partial certificate creation before bubbling an error. That leaves the batch in a completed state even though generation failed.Keep the batch in the pre-active state until generation succeeds, or add a compensating/retry path for failed generations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/certificateBatchController.js` around lines 542 - 559, Update the level-1 approval flow around findOneAndUpdate and generateCertificates so lifecycleStatus is not persisted as "Active" until certificate generation completes successfully. Ensure generation failures leave the batch retryable or execute a compensating update that restores the pre-active state, while preserving the existing conflict response and exactly-once approval behavior.backend/controllers/certificateBatchController.js-293-324 (1)
293-324: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestrict
deleteBatchto Draft batches.Activebatches already haveCertificaterecords linked bybatchId, so deleting them leaves orphaned certificates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/certificateBatchController.js` around lines 293 - 324, Update deleteBatch to restrict the findOneAndDelete query to batches whose status is Draft, while preserving the existing batchId validation and initiatedBy ownership checks. Active or any non-Draft batch must follow the existing not-found/unauthorized response and remain undeleted.frontend/src/services/batch.js-23-32 (1)
23-32: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
createBatchhas inconsistent return types between success and error paths.On success, it returns
{data, status}(an object). On error, it returnserr.response?.data.message(a string orundefined). Callers cannot reliably distinguish success from error without checking the shape of the return value. Align with the other functions in this module or throw on error.🔧 Proposed fix for consistent return type
export async function createBatch(data) { try { const response = await api.post("/api/batches/create-batch", data); - console.log("Response", response); - return {data: response.data.message, status: response.status}; + return response.data.message; } catch (err) { console.error("createBatch error:", err); return err.response?.data.message; } }Alternatively, if the status is needed by callers, return a consistent object shape on both paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/services/batch.js` around lines 23 - 32, Update createBatch so its success and error paths use a consistent return contract: either align both with the surrounding batch-service functions or return the same object shape containing data and status for both outcomes. Do not return the raw error message string or undefined; preserve error details through the chosen consistent contract.frontend/src/services/batch.js-88-106 (1)
88-106: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse POST/PATCH for batch approval actions.
approveBatchandrejectBatchmutate server state but are exposed as GET endpoints. With cookie-based auth (withCredentials: true) and no CSRF guard, these requests should be non-GET on both the client andbackend/routes/certificateBatch.js.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/services/batch.js` around lines 88 - 106, Update approveBatch and rejectBatch to send POST or PATCH requests instead of GET while preserving their existing endpoints, response handling, and error handling. Update the corresponding approval and rejection routes in certificateBatch.js to use the same non-GET method so client and server remain aligned.frontend/src/Components/Batches/StudentPanel.jsx-154-154 (1)
154-154: 🎯 Functional Correctness | 🟠 Major | ⚖️ Poor tradeoffTailwind v4 important modifier syntax is incorrect
!rounded-lg/!rounded-xlshould berounded-lg!/rounded-xl!infrontend/src/Components/Batches/StudentPanel.jsx; the same fix applies to the matching occurrences at lines 154, 206, and 213.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Batches/StudentPanel.jsx` at line 154, Update the Tailwind important modifier syntax in StudentPanel.jsx by changing each matching !rounded-lg and !rounded-xl utility to rounded-lg! and rounded-xl!, including the occurrences near the student panel controls at the referenced locations.frontend/src/Components/Batches/batchCard.jsx-201-237 (1)
201-237: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInconsistent action buttons between grid and list views.
The available actions for the same
lifecycleStatusdiffer significantly betweenBatchCard(grid) andBatchList(list):
Status Grid (BatchCard) List (BatchList) Draft View, Edit, Duplicate, Delete Edit, Delete Active View, Duplicate, Archive View, Edit, Duplicate, Delete Submitted View, Duplicate, Archive View, Archive Archived View, Duplicate, Delete View, Duplicate, Delete For example, "Active" batches can be edited and deleted in list view but not in grid view, and "Draft" batches lose View and Duplicate actions in list view. Users switching views will experience unexpected changes in available actions.
Also applies to: 387-454
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Batches/batchCard.jsx` around lines 201 - 237, The action sets in BatchCard and BatchList must be consistent for every lifecycleStatus. Update the status-specific action rendering in BatchCard and the corresponding BatchList branches so Draft, Active, Submitted, and Archived expose the same buttons and handlers in both views, preserving the existing status-to-action mapping and callback parameters.frontend/src/Components/Batches/batchCard.jsx-114-122 (1)
114-122: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd null guards on nested property access.
Several deeply nested property chains (
batch.eventId.title,batch.eventId.organizing_unit_id.name,batch.users.length,b.initiatedBy.personal_info.name) have no optional chaining, while other lines in the same component do (e.g., line 186 usesbatch?.initiatedBy?.personal_info?.name). If the backend returns a batch with a missingeventId,users, orinitiatedBy, these will throw and crash the entire list.Also applies to: 153-153, 159-159, 318-318, 330-330, 360-360
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Batches/batchCard.jsx` around lines 114 - 122, Add optional chaining to the nested batch property accesses identified in the review, including eventId.title, eventId.organizing_unit_id.name, users.length, and initiatedBy.personal_info.name, while preserving existing fallback values such as empty strings. Align these accesses with the guarded pattern already used in batchCard.jsx, including all additionally referenced locations.frontend/src/services/events.js-1-13 (1)
1-13: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winError return value is truthy — callers cannot detect failures.
fetchEventsreturnserr.response?.data.message(a string) on error, but callers likebatches.jsxcheckif (!events)to detect failure. A non-empty error string is truthy, so errors are indistinguishable from valid data. Returnnullon error so callers can reliably detect failures.🐛 Proposed fix
} catch (err) { console.error("fetchEvents error:", err); - return err.response?.data.message; + return null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/services/events.js` around lines 1 - 13, Update the catch block in fetchEvents to return null instead of err.response?.data.message, preserving the existing error logging so callers checking for a falsy result can reliably detect request failures.frontend/src/Components/Batches/batches.jsx-512-514 (1)
512-514: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReact key uses
b.idbut batch objects use_id.
key={b.id}is used in the grid view map, but batch objects from MongoDB use_idas their identifier (as seen inBatchCardwhich checksbatch._id || batch.id || batch.batchId). Ifb.idisundefined, React will useundefinedfor all keys, causing incorrect reconciliation. Useb._idinstead.🐛 Proposed fix
- <BatchCard - key={b.id} + <BatchCard + key={b._id}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Batches/batches.jsx` around lines 512 - 514, Update the key in the grid view’s filtered.map rendering to use each batch’s MongoDB identifier, b._id, instead of b.id, while leaving the surrounding BatchCard rendering unchanged.frontend/src/Components/Batches/batches.jsx-322-342 (1)
322-342: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
updated.length !== 0prevents UI update when last batch is removed.In
delBatch,archiveBatch, anddupBatch, the conditionif (updated && updated.length !== 0) setBatches(updated)means that if the API returns an empty array (e.g., after deleting the last batch),setBatchesis never called and the stale batch remains visible. The check should beif (updated)orif (Array.isArray(updated)).🐛 Proposed fix
- if (updated && updated.length !== 0) setBatches(updated); + if (updated) setBatches(updated);Apply this to lines 326, 333, and 341.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Batches/batches.jsx` around lines 322 - 342, Update the post-fetch state updates in delBatch, archiveBatch, and dupBatch to call setBatches when updated is a valid result even if it is an empty array. Replace the updated.length !== 0 guard with an existence or array-validity check so removing the last batch clears the UI.frontend/src/Components/Batches/batches.jsx-308-310 (1)
308-310: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake
editBatchreturn the same{ status, data }shape ascreateBatch
editBatchreturns onlyres.data.message, but the save flow expectsresponse.statusandresponse.data. On edits, that leaves both undefined and the modal/toast path never completes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Batches/batches.jsx` around lines 308 - 310, Update editBatch to return the same { status, data } response shape as createBatch, preserving the HTTP status alongside the response payload instead of returning only res.data.message. This allows the save flow’s response.status check, closeModal(), and fire(response.data) path to work for edits.frontend/src/Components/Batches/batches.jsx-67-94 (1)
67-94: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winError handling cannot distinguish success from failure for
fetchEvents.
fetchEventsreturnserr.response?.data.message(a truthy string) on error. The checkif (!batches || !events || !templates)on line 76 evaluates tofalsefor a non-empty string, so errors pass through as if they were valid data. The events array would then be set to a string, causing downstream.map()calls to fail. Consider returningnullon error instead.🐛 Proposed fix for events.js
} catch (err) { console.error("fetchEvents error:", err); - return err.response?.data.message; + return null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Batches/batches.jsx` around lines 67 - 94, Update fetchEvents so its error path returns null instead of err.response?.data.message, allowing the existing validation in the batches useEffect to distinguish failures from successful event arrays. Preserve the existing error notification behavior while ensuring setEvents only receives valid event data.backend/services/email.service.js-56-63 (1)
56-63: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUser-controlled data is interpolated into HTML emails without escaping.
Values like
batchObj.title,batchObj.event.name,batchObj.event.description, approvername/batchObj.createdByare inserted directly into HTML templates. If any contain HTML characters, they can break email layout or inject misleading content/links. Add an escape helper and apply it to all interpolated values.🔒 Proposed fix: add an HTML-escape helper and use it
+function escapeHtml(str) { + return String(str) + .replace(/&/g, "&") + .replace(/</g, "<") + .replace(/>/g, ">") + .replace(/"/g, """) + .replace(/'/g, "&`#039`;"); +} + async function newBatchSendEmail(toEmail, ccEmails=[], batchLink, batchObj){ - const approverList = (batchObj.approverList || []).map((a, index) => ` + const approverList = (batchObj.approverList || []).map((a, index) => ` <div> <strong>Approver ${index + 1}:</strong><br/> - Name: ${a.name}<br/> - Email: ${a.email} + Name: ${escapeHtml(a.name)}<br/> + Email: ${escapeHtml(a.email)} </div> <br /> `).join("");Apply
escapeHtml()to all${...}interpolations of user-controlled data across all three functions.Also applies to: 82-86, 136-143, 154-155, 161-164, 205-206, 211-215
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/email.service.js` around lines 56 - 63, In the email-building functions, add a reusable escapeHtml helper and apply it to every user-controlled interpolation, including batchObj.title, batchObj.event.name, batchObj.event.description, batchObj.createdBy, and approver name/email values in approverList. Keep static HTML markup and non-user-controlled formatting unchanged while ensuring all displayed data is escaped before insertion.backend/services/email.service.js-134-263 (1)
134-263: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
batchStatusSendEmailhas multiple robustness defects.
- Line 136:
batchObj.pendingApprovers.map()— no null check. UnlikenewBatchSendEmailwhich uses(batchObj.approverList || []), this will throw ifpendingApproversis undefined (e.g., on rejection).- Line 134:
ccEmailshas no default parameter.ccEmails.join(",")on line 250 throws ifundefined.- Lines 251-252:
Emailformat[action].subjectthrows aTypeErrorfor an invalidactionbefore the validation check on line 256 is reached — the validation is dead code.- Line 261:
throw new Error(err)wraps an Error object, losing the stack trace and original message. Usethrow errinstead.🐛 Proposed fix
-async function batchStatusSendEmail(toEmail, ccEmails, batchLink, batchObj, action){ +async function batchStatusSendEmail(toEmail, ccEmails = [], batchLink, batchObj, action){ + + if(!["approve", "reject"].includes(action)){ + throw new Error("Invalid action"); + } - const approverList = batchObj.pendingApprovers.map((a, index) => ` + const approverList = (batchObj.pendingApprovers || []).map((a, index) => ` <div> <strong>Approver ${index + 1}:</strong><br/> Name: ${a.name}<br/> Email: ${a.email} </div> <br /> `).join("");And move the validation before
optionsconstruction, then fix the catch:try{ - if(!["approve", "reject"].includes(action)){ - throw new Error("Invalid action"); - } await transport.sendMail(options); }catch(err){ - throw new Error(err); + throw err; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/email.service.js` around lines 134 - 263, Update batchStatusSendEmail to default ccEmails and pendingApprovers to empty arrays before calling join or map, validate action against the supported values before constructing options or accessing Emailformat[action], and rethrow the caught error directly with throw err to preserve its original message and stack.backend/services/organization.service.js-50-52 (1)
50-52: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEscape regex metacharacters in email-based org lookup.
userEmailis interpolated directly into aRegExpwithout escaping. Email addresses commonly contain.(matches any character) and+(regex quantifier), sojohn.doe@example.comwould also matchjohn_doe@example.comin the database. SincegetCoordinatorOrganizationdetermines which organization a coordinator is authorized for, an incorrect match is an authorization bypass risk.🔒 Proposed fix — escape regex metacharacters
async function getCoordinatorOrganization(user) { const userEmail = String( user.username || user.personal_info?.email || "" ) .trim() .toLowerCase(); + const escapedEmail = userEmail.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); const organization = await OrganizationalUnit.findOne({ - "contact_info.email": new RegExp(`^${userEmail}$`, "i"), + "contact_info.email": new RegExp(`^${escapedEmail}$`, "i"), });Alternatively, use MongoDB collation for case-insensitive exact match without regex:
const organization = await OrganizationalUnit.findOne({ - "contact_info.email": new RegExp(`^${userEmail}$`, "i"), + "contact_info.email": userEmail, + }).collation({ locale: "en", strength: 2 });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/organization.service.js` around lines 50 - 52, Escape all regular-expression metacharacters in userEmail before constructing the RegExp used by getCoordinatorOrganization’s OrganizationalUnit.findOne lookup, while preserving the existing case-insensitive exact-match behavior. Ensure the escaped value cannot broaden the query beyond the literal email address.Source: Linters/SAST tools
frontend/src/Components/Requests/Requests.jsx-108-121 (1)
108-121: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winError responses displayed as success toasts and can crash the component
approveBatchandrejectBatchreturn error message strings on failure (fromerr.response?.data.message).response && toast.success(response)shows these as green success toasts. Additionally,fetchBatchescan return a string on error, andsetRequests(string)will crash the next render whenfilteredRequests()calls.filter()on a non-array.🛡️ Proposed fix: guard setRequests and add error toast
const approve = async function (batch) { const response = await approveBatch(batch); - console.log("Approve Batch ", response); - response && toast.success(response); + if (response) toast.success(response); + else toast.error("Approval failed"); const updated = await fetchBatches(isUserLoggedIn?._id); - updated && setRequests(updated); + if (Array.isArray(updated)) setRequests(updated); + else toast.error(updated || "Failed to refresh requests"); }; const reject = async function (batch) { const response = await rejectBatch(batch); - response && toast.success(response); + if (response) toast.success(response); + else toast.error("Rejection failed"); const updated = await fetchBatches(isUserLoggedIn?._id); - updated && setRequests(updated); + if (Array.isArray(updated)) setRequests(updated); + else toast.error(updated || "Failed to refresh requests"); };Note: the root cause is that the service layer (
batch.js) catches errors and returns strings instead of throwing. A follow-up should refactor the services to throw on error so callers can distinguish success from failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Requests/Requests.jsx` around lines 108 - 121, Update the approve and reject handlers to treat service-returned error strings as failures: show them with toast.error and only show toast.success for successful responses. Guard the fetchBatches result before calling setRequests, updating state only when the result is an array so filteredRequests remains safe.frontend/src/Components/Requests/editModal.jsx-88-102 (1)
88-102: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winError responses shown as success; modal closes unconditionally on failure
approverEditBatchreturns error strings on failure, displayed as success toasts viaresponse && toast.success(response).fetchBatchescan return a string on error, crashingsetRequests.onClose()is called regardless of success or failure, closing the modal even when the save fails — the user assumes the update succeeded.🛡️ Proposed fix: guard setRequests and prevent close on failure
const onSave = async () => { if (selected.size === 0) { toast.error("Please select at least one user"); return; } const response = await approverEditBatch({ ...form, users: Array.from(selected), }); - response && toast.success(response); + if (!response) { + toast.error("Failed to update batch"); + return; + } + toast.success(response); const updated = await fetchBatches(isUserLoggedIn?._id); - updated && setRequests(updated); - onClose(); + if (Array.isArray(updated)) setRequests(updated); + onClose(); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Requests/editModal.jsx` around lines 88 - 102, Update onSave to distinguish successful results from error strings returned by approverEditBatch and fetchBatches: show failures with an error toast, only call setRequests when fetchBatches returns valid batch data, and call onClose only after the edit and refresh both succeed. Preserve the existing validation for an empty selected set.frontend/src/Components/Requests/viewModal.jsx-35-43 (1)
35-43: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReplace the runtime Tailwind color classes with static styling In
frontend/src/Components/Requests/viewModal.jsx:35-43and:87-99,text-[${C.text}]/text-[${C.warmGray}]are interpolated at runtime, so Tailwind won’t generate those utilities. Use inlinestyle={{ color: ... }}or static class names instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Requests/viewModal.jsx` around lines 35 - 43, Replace the runtime Tailwind color classes in the value display and the corresponding section around the second referenced block with static styling. Remove text-[${C.text}] and text-[${C.warmGray}] usage, applying the respective C.text and C.warmGray values through inline color styles or existing static color classes while preserving the current typography classes.
🟡 Minor comments (13)
frontend/src/Components/Certificates/CertificatesList.jsx-127-127 (1)
127-127: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
!rounded-2xluses Tailwind v3 important syntax; v4 requiresrounded-2xl!.Per Tailwind CSS v4 breaking changes, the
!important modifier moved from prefix to suffix. The project uses Tailwind v4.0.3.💚 Proposed fix
- className={`px-4 h-[40px] !rounded-2xl font-semibold transition-all duration-200 ${ + className={`px-4 h-[40px] rounded-2xl! font-semibold transition-all duration-200 ${🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Certificates/CertificatesList.jsx` at line 127, Update the button className in CertificatesList so the Tailwind v4 important modifier uses the suffix form for rounded-2xl, preserving the existing styling and other classes.frontend/src/Components/Certificates/CertificatesList.jsx-29-34 (1)
29-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNon-array response silently shows empty list with no error feedback.
If
fetchCertificatesreturns a non-array (e.g.,undefinedon network error, or an error message string), the code falls through without settingerrororcertificates. The user sees an empty list with no indication something went wrong.💚 Proposed fix
const response = await fetchCertificates(); if(Array.isArray(response)){ toast.success("Certificates fetched successfully"); setCertificates(response); return ; } + setError("Failed to fetch certificates"); } catch (err) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Certificates/CertificatesList.jsx` around lines 29 - 34, Update the fetchCertificates handling in the Certificates list component so every non-array response is treated as a failure: set the component’s error state and ensure certificates is reset to an empty list, while preserving the existing success path for array responses. Use the component’s existing error state and user-facing feedback mechanism rather than silently falling through.backend/controllers/certificateBatchController.js-271-273 (1)
271-273: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDuplicate title logic produces inconsistent and incorrect results.
Splitting on
"(Copy)"and usingarray.length - 1as the count doesn't track actual copy numbers. For a title"My Title", the first duplicate becomes"My Title Copy(0)". Duplicating that gives"My Title Copy(0) Copy(0)"instead of"My Title Copy(1)". The separator"(Copy)"also doesn't match the output format"Copy(n)".💚 Proposed fix
- const array = batch.title.split("(Copy)"); - const count = array.length - 1; - const title = `${array[0]} Copy(${count})`; + const baseTitle = batch.title.replace(/\s*Copy\(\d+\)\s*$/, "").trim(); + const existingCopies = await CertificateBatch.countDocuments({ + title: { $regex: `^${baseTitle.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*Copy\\(\\d+\\)$` }, + initiatedBy: id, + }); + const title = `${baseTitle} Copy(${existingCopies + 1})`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/certificateBatchController.js` around lines 271 - 273, Update the duplicate-title generation near the batch title handling to parse an existing trailing “Copy(n)” suffix and increment its numeric value, defaulting the first duplicate to Copy(1). Preserve the original base title when no suffix exists, and ensure repeated duplication produces a single correctly incremented Copy(n) suffix rather than splitting on “(Copy)”.frontend/src/Components/Batches/studentPanel.jsx-46-46 (1)
46-46: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove debug
console.logstatement.Line 46 has
console.log("studentIds:", studentIds)left from debugging. This should be removed before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Batches/studentPanel.jsx` at line 46, Remove the debug console.log statement that prints studentIds from the student panel component, leaving the surrounding logic unchanged.frontend/src/services/batch.js-26-26 (1)
26-26: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove
console.logdebug statements.Lines 26 and 91 contain
console.logcalls that should be removed before merge.Also applies to: 91-91
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/services/batch.js` at line 26, Remove the debug console.log calls at both locations in the batch service, including the “Response” log and the statement near the second referenced location, while leaving the surrounding request handling unchanged.frontend/src/Components/Batches/modalDialog.jsx-131-138 (1)
131-138: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard against
undefinedsignatoryDetailsto prevent runtime crash.If
form.signatoryDetailsisundefinedwhen the component mounts,form.signatoryDetails.lengthon line 132 will throw a TypeError. Additionally, line 276 (form.signatoryDetails.map) would crash during render before the effect even runs. Use optional chaining for safety.🛡️ Proposed fix
useEffect(() => { - if (form.signatoryDetails.length === 0) { + if (!form.signatoryDetails || form.signatoryDetails.length === 0) { setForm((prev) => ({ ...prev, signatoryDetails: [{ name: "", role: "" }], })); } }, []);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Batches/modalDialog.jsx` around lines 131 - 138, Update the signatoryDetails handling in the component’s useEffect and render mapping to safely support an undefined value: use optional chaining when checking its length and when invoking map. Preserve initialization to the default signatory entry when the value is absent or empty.frontend/src/Components/Batches/ui.jsx-127-128 (1)
127-128: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
CertThumbusesMath.random()on every render, causing color flicker.The thumbnail color changes randomly on every parent re-render. Use a deterministic index based on the batch ID instead.
🔧 Proposed deterministic color selection
export function CertThumb({ batch }) { - const colorIndex = Math.floor(Math.random() * BATCH_COLORS.length); - const c = BATCH_COLORS[colorIndex % BATCH_COLORS.length]; + const idSum = (batch?._id || batch?.title || "").split("").reduce((a, ch) => a + ch.charCodeAt(0), 0); + const c = BATCH_COLORS[idSum % BATCH_COLORS.length];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Batches/ui.jsx` around lines 127 - 128, Update the color selection in CertThumb to derive a stable index deterministically from the batch ID instead of calling Math.random() during rendering. Preserve the existing BATCH_COLORS selection and ensure the same batch ID consistently produces the same thumbnail color across re-renders.backend/services/certificates.service.js-42-49 (1)
42-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winContradictory certificate type and title.
certificateTypeis "Certificate of Participation" whilecertificateTitleis "Certificate of Achievement". These render in different sections of the same PDF — the small label says "Participation" and the large heading says "Achievement". Pick one category or make them configurable per batch/template.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/certificates.service.js` around lines 42 - 49, Align certificateType and certificateTitle in the certificate data constructed by the certificate-generation method so both represent the same category, or derive them from the same batch/template configuration. Preserve the existing recipient, description, and certificate identifier fields.backend/models/certificateSchema.js-43-54 (1)
43-54: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
signaturedefault evaluates toundefinedat schema definition time.
default: this.nameis evaluated when the object literal is created, not when a subdocument is instantiated. At that pointthisis the module scope, sothis.nameisundefined. Use a function so Mongoose bindsthisto the subdocument being created.🛠️ Proposed fix
name: { type: String, required: true }, - signature: { type: String, default: this.name }, + signature: { type: String, default: function() { return this.name; } }, role: { type: String, required: true },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/models/certificateSchema.js` around lines 43 - 54, Update the signature field definition within signatoryDetails so its default is provided by a function evaluated for each subdocument, with this bound to that subdocument and returning its name. Keep the existing required and lifecycleStatus behavior unchanged.backend/utils/cloudinary.js-20-24 (1)
20-24: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLocal PDF is deleted even when upload fails, preventing retry.
The
finallyblock deletes the local file regardless of upload success. If the upload fails, the PDF is lost and must be regenerated. Consider deleting only on success, or moving cleanup to the caller.🛠️ Proposed fix: delete only on success
try { const result = await cloudinary.uploader.upload(pdfPath, { resource_type: "raw", folder: "certificates", timeout: 120000, }); + if (fs.existsSync(pdfPath)) { + fs.unlinkSync(pdfPath); + } return result.secure_url; } catch (err) { console.error("Cloudinary upload failed:"); console.dir(err, { depth: null }); throw err; - } finally { - if (fs.existsSync(pdfPath)) { - fs.unlinkSync(pdfPath); - } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/utils/cloudinary.js` around lines 20 - 24, Update the cleanup flow surrounding the Cloudinary upload so the local PDF is deleted only after a successful upload; preserve the file when the upload throws or otherwise fails, allowing retry. Adjust the existing finally-block logic and any success tracking in the upload function rather than changing unrelated behavior.backend/utils/batchValidate.js-6-18 (1)
6-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix misleading validation message and typos in error strings.
Line 12 enforces
min(3)but the error message says "at least 5 characters" — users entering a 3–4 character name will see a confusing message. Additionally, "atleast" should be "at least" on lines 6, 12, and 18. These messages are surfaced directly to API callers viavalidation.error.issues.✏️ Proposed fix
title: zod.string().min(5, "Title should be at least 5 characters"), eventId: zodObjectId, templateId: zodObjectId, signatoryDetails: zod .array( zod.object({ - name: zod.string().min(3, "Name must be atleast 5 characters"), + name: zod.string().min(3, "Name must be at least 3 characters"), signature: zod.string().optional(), role: zod.string().min(1, "Invalid position"), }), ) .nonempty("At least one signatory is required"), - users: zod.array(zodObjectId).min(1, "Atleast 1 user must be associated."), + users: zod.array(zodObjectId).min(1, "At least 1 user must be associated."),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/utils/batchValidate.js` around lines 6 - 18, Update the validation messages in the visible schema: make the signatory name message match its min(3) constraint, and replace every “atleast” occurrence in the title, name, and users messages with “at least,” preserving the existing validation rules.frontend/src/Components/Requests/Card.jsx-93-93 (1)
93-93: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix invalid CSS value
solidrgbin border-bottom style
"2px solidrgb(198, 194, 150)"is invalid CSS —solidrgbshould besolid rgb. The browser ignores the entire declaration, so non-last rows have no visible bottom border.🐛 Proposed fix
- borderBottom: last ? "none" : "2px solidrgb(198, 194, 150)", + borderBottom: last ? "none" : "2px solid rgb(198, 194, 150)",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Requests/Card.jsx` at line 93, Correct the borderBottom value in the Card component so the non-last-row style uses valid CSS by separating the “solid” keyword from the rgb color function. Preserve the existing “none” value for the last row.frontend/src/Components/Requests/Requests.jsx-61-98 (1)
61-98: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd null guards in filteredRequests and stats calculation
filteredRequestsaccessesreq.eventId.title,req.eventId.organizing_unit_id.name, andreq.initiatedBy.personal_info.namewithout optional chaining. The statsuseEffectaccessesreq.users.lengthwithout a null check. If any nested object is missing from the API response, the component crashes with a TypeError.🛡️ Proposed fix with optional chaining
- const event = req.eventId.title.toLowerCase(); - const organization = req.eventId.organizing_unit_id.name.toLowerCase(); - const requestedBy = req.initiatedBy.personal_info.name.toLowerCase(); + const event = req.eventId?.title?.toLowerCase() ?? ""; + const organization = req.eventId?.organizing_unit_id?.name?.toLowerCase() ?? ""; + const requestedBy = req.initiatedBy?.personal_info?.name?.toLowerCase() ?? "";- total += req.users.length || 0; + total += req.users?.length || 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/Components/Requests/Requests.jsx` around lines 61 - 98, Update filteredRequests to safely access nested request fields through optional chaining or equivalent fallbacks before lowercasing, and guard req.users when calculating approved request totals in the stats useEffect. Preserve the existing search matching and status-count behavior when the API provides complete data.
1 participant
Summary
This PR introduces a complete certificate management workflow for the Student Database COSA project.
Features
Technical Changes
Known Limitation
Cloudinary upload/download integration is temporarily stubbed (
certificateUrl = "#") due to a local networking issue affecting Cloudinary uploads during development. The certificate workflow is implemented, and Cloudinary integration will be completed in a follow-up PR.Summary by CodeRabbit