Feature/course categories - #55
Conversation
feat(stellar): add path payment support with quote endpoint and slipp…
…d-jobs feat(jobs): add durable background processing
WalkthroughThis PR adds category taxonomy APIs and migration tooling, a Mongo-backed background job system with OTP and receipt handling, and Stellar path-payment quoting and construction. It also updates payment verification, caching, authorization, environment configuration, scripts, and integration coverage. ChangesCategory taxonomy
Background job processing
Stellar path payments
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 19
🧹 Nitpick comments (8)
src/routes/stellar/paymentRoutes.js (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew
/quoteendpoint has no direct Jest coverage.
test/pathPayments.test.jsonly unit-testsapplySlippage/calculateFeeSplit/toStroops/fromStroopsand manually rebuilds XDR ops — it never exercises this route/controller through an HTTP request (e.g. viasupertest), so regressions ingetQuote's validation, error paths, or response payload wouldn't be caught by CI.As per path instructions, "New or changed endpoints need Jest coverage (the runner uses --experimental-vm-modules) and consistent response shapes."
Also applies to: 19-19
🤖 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 `@src/routes/stellar/paymentRoutes.js` at line 7, Add direct Jest HTTP coverage for the `/quote` endpoint handled by `getQuote`, using the project’s existing request-testing approach (such as supertest). Cover successful responses, validation failures, and controller error paths, and assert the expected response payload shape without relying on manually rebuilt XDR operations.Source: Path instructions
test/pathPayments.test.js (1)
96-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the fee-splitting builder directly
test/pathPayments.test.jsstill builds the twopathPaymentStrictReceiveops by hand and re-derives the split math, so it only checks XDR serialization. Add a test forbuildPathPaymentTransaction(...)(or switch this case to it) so bugs in the production split logic, destination selection, or asset ordering can’t slip through.🤖 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 `@test/pathPayments.test.js` around lines 96 - 157, Update the fee-split test to invoke the production buildPathPaymentTransaction function instead of manually constructing both pathPaymentStrictReceive operations and recomputing split amounts. Keep assertions for two operations, destination PLATFORM_WALLET, correct native/USDC asset ordering, and summed destination/send-max amounts so the production split and transaction-building logic are exercised directly.src/controllers/courses/courseController.js (1)
63-69: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
getCoursesfilter is inconsistent with the create/update/detail paths.Two small mismatches versus the rest of the category logic:
- It resolves
{ slug: category }only, whereascreateCourse/updateCourseaccept an ObjectId or slug.- It omits
isActive: true, so a soft-deleted category's slug still returns its courses — whilegetCategoryBySlugcorrectly hides inactive categories.Not a blocker, but aligning the filter avoids callers getting different results from the two entry points.
♻️ Suggested alignment
if (category) { - const cat = await Category.findOne({ slug: category }); + const cat = await Category.findOne({ slug: category, isActive: true }); if (!cat) { return res.status(200).json({ success: true, courses: [] }); } query.categoryRef = cat._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 `@src/controllers/courses/courseController.js` around lines 63 - 69, Update the category lookup in getCourses to accept the category filter as either an ObjectId or slug, matching createCourse and updateCourse, and require isActive: true. Preserve the existing empty-course response when no active category matches, then apply the resolved category reference to the course query.src/controllers/categoryController.js (1)
85-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResponse shape deviates from the documented
{ success, message, data }contract.The category endpoints return ad-hoc shapes (
{ success, categories },{ success, category, courses, pagination }, etc.). Per the repo convention these new endpoints should use a consistent{ success, message, data }envelope so clients can parse responses uniformly. Not blocking, but worth aligning while the surface is new — feel free to fold the extra fields underdata.As per path instructions: "consistent response shapes ({ success, message, 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 `@src/controllers/categoryController.js` at line 85, Update the category controller endpoint responses, including the response at the categories success path, to consistently use the documented { success, message, data } envelope. Move category, courses, pagination, and other endpoint-specific fields under data, and provide an appropriate message while preserving the existing success payload values.Source: Path instructions
src/controllers/stellar/donationController.js (1)
196-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new transient-retry (202) path.
This new branch is on the payment-integrity critical path (deciding whether a donation ends up
retryingvsfailed), but no test in this batch exercises it. Given the emphasis on payment correctness, consider adding an integration test asserting the 202 response,status: "retrying"persistence, and thatverifyPaymentOnChaingets enqueued with the expected idempotency key.🤖 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 `@src/controllers/stellar/donationController.js` around lines 196 - 220, Add integration coverage for the transient branch in the donation controller’s verification flow, asserting the 202 response and persisted donation status are "retrying". Also verify that verifyPaymentOnChain is enqueued with the donation transaction ID and idempotency key `verify:${result.hash}`.Source: Path instructions
test/jobs.test.js (1)
1-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffGood dedup/retry/auth coverage; mongo-driver paths remain untested.
These tests only exercise the inline driver (dedup + retry) and dashboard auth rejection. The mongo-backed path (
processNext, dead-lettering aftermaxAttempts, and the startup active→retrying requeue instartJobs) has no coverage in this batch — worth adding once mongo-memory-server tooling is available, given this is the production driver.🤖 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 `@test/jobs.test.js` around lines 1 - 66, Add tests covering the Mongo-backed queue path, using mongo-memory-server when available: exercise processNext, verify failed jobs are dead-lettered after maxAttempts, and verify startJobs requeues startup jobs from active to retrying. Keep the existing inline deduplication, retry, and dashboard-auth tests unchanged.src/models/Job.js (1)
1-28: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNo retention/cleanup path for completed or dead jobs.
Schema/indexes look correct for the query patterns in
queue.js(compound{status, runAt}index coversprocessNext, unique index covers idempotency lookups). One gap: there's no TTL or archival strategy forcompleted/deaddocuments, so this collection will grow unbounded in production. Consider a TTL index oncompletedAt(e.g., purge completed jobs after N days) or a periodic archival job — but keepdeadjobs retained longer/indefinitely for debugging.🤖 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 `@src/models/Job.js` around lines 1 - 28, Implement a retention strategy for completed jobs in the Job schema by adding a TTL index on completedAt with the configured retention duration, while excluding dead jobs from automatic deletion so they remain available for debugging. Use an appropriate partial filter or equivalent index configuration to limit expiration to completed documents.Source: Path instructions
src/routes/jobsRoutes.js (1)
13-20: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider a constant-time comparison for the dashboard token.
Nice touch gating the whole router and returning
404when the token is unset. One hardening suggestion: comparing the bearer token with!==is not constant-time, so it can leak timing information about the secret. For an admin surface exposing job internals/errors, a constant-time compare is cheap insurance.The hand-rolled
escapeHtmlat Lines 5-11 is actually complete and correct for this server-rendered table, so the static-analysis XSS hint there is a false positive — no change needed.🔒 Constant-time token check
+import crypto from "crypto"; router.use((req, res, next) => { const token = process.env.JOBS_DASHBOARD_TOKEN; if (!token) return res.status(404).json({ success: false, message: "Not found" }); - if (req.headers.authorization !== `Bearer ${token}`) { + const provided = req.headers.authorization || ""; + const expected = `Bearer ${token}`; + const providedBuf = Buffer.from(provided); + const expectedBuf = Buffer.from(expected); + if ( + providedBuf.length !== expectedBuf.length || + !crypto.timingSafeEqual(providedBuf, expectedBuf) + ) { return res.status(401).json({ success: false, message: "Unauthorized" }); } next(); });🤖 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 `@src/routes/jobsRoutes.js` around lines 13 - 20, Update the authorization check in the router middleware to compare the supplied bearer token and JOBS_DASHBOARD_TOKEN using a constant-time comparison, handling differing lengths safely before comparison. Preserve the existing 404 response for an unset token, 401 response for invalid credentials, and next() behavior for valid credentials.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@package.json`:
- Line 9: The package scripts no longer provide the documented npm run seed
command. Update the package.json script configuration around seed:categories to
restore the seed alias, or consistently rename the command in README.md and
migration instructions while preserving the documented seeding workflow.
In `@services/emails/sendMail.js`:
- Around line 33-43: Update sendOtpEmail and sendReceiptEmail to stop logging
plaintext email addresses; log a non-identifying reference such as an available
userId or masked/hashed email instead, while preserving the existing email
delivery behavior and transaction-hash context.
- Line 6: Remove the fallback from EMAILJS_RECEIPT_TEMPLATE_ID so it cannot
reuse EMAILJS_TEMPLATE_ID; validate that the receipt template ID is configured
and fail loudly when it is missing. Also add the required
EMAILJS_RECEIPT_TEMPLATE_ID configuration entry to .env.example.
In `@src/controllers/authController.js`:
- Around line 126-131: Update registerUser to generate a fresh OTP for each
registration instead of using the module-level generatedOtp imported from
emailRoutes.js. Remove that dependency in this flow, create the OTP within
registerUser, and pass the per-registration value to the sendOtpEmail enqueue
payload and idempotencyKey.
In `@src/controllers/categoryController.js`:
- Around line 189-214: Update deleteCategory to count Book documents with the
same categoryRef in addition to Course documents, and soft-delete the category
whenever either count is greater than zero. Preserve the existing hard-delete
behavior only when neither courses nor books reference the category.
In `@src/jobs/handlers.js`:
- Around line 56-73: Move the transaction status update and initial save out of
the beginning of the purchase-confirmation flow in the relevant job handler, so
recordSaleEarnings and the purchasedBooks/purchasedCourses/enrolledUsers updates
execute first. Only after all purchase side effects succeed should the handler
set status to confirmed, clear failureReason, assign confirmedAt, and save;
preserve queueReceipt behavior and the existing idempotent retry flow.
In `@src/jobs/queue.js`:
- Around line 41-59: The inline path in enqueue silently ignores
options.session, so session-scoped jobs do not preserve transactional behavior.
Update enqueue to handle session-scoped calls explicitly: either defer
executeInline until the session transaction resolves, or reject them in inline
mode with a clear error; document the chosen limitation or behavior. Keep
non-session inline deduplication and execution unchanged.
- Around line 123-133: Update the startup recovery query in startJobs so it only
changes active jobs whose lockedAt timestamp is older than the configured or
established lock-timeout threshold. Preserve the existing retrying status, runAt
reset, and lockedAt removal for those stale jobs, while leaving fresh active
jobs owned by other workers untouched.
- Around line 61-77: Update the enqueue flow around Job.findOneAndUpdate to
enable includeResultMetadata while preserving new: true, then use the returned
metadata to set duplicate based on lastErrorObject.updatedExisting or the
absence of upserted rather than job.attemptsMade. Continue returning the
document’s _id as id and preserve the existing queued response.
In `@src/middlewares/authMiddleware.js`:
- Around line 46-50: Update the 403 response in the role check within
authMiddleware to include data: null alongside success and message, preserving
the required { success, message, data } response shape and existing
authorization behavior.
In `@src/models/Category.js`:
- Around line 69-84: Resolve the dead `isModified('name')` condition in the
`categorySchema` save hook by explicitly choosing the intended slug behavior:
either remove the name-change check and keep slugs immutable, or regenerate the
slug through `generateUniqueSlug` when `name` is modified while preserving the
current-category exclusion.
In `@src/routes/categoryRoutes.js`:
- Around line 22-24: Extend the existing category integration Jest suite to
cover PATCH /api/categories/:id and DELETE /api/categories/:id. Add authorized
and unauthorized cases, verify category update and soft-delete behavior, and
assert cache-visible responses after each operation while preserving the
existing POST coverage.
In `@src/routes/courses/courseRoutes.js`:
- Around line 2-3: Remove the duplicate ESM import declarations for
invalidateCacheMiddleware and CACHE_KEYS in courseRoutes.js, retaining a single
import for each binding so the module parses successfully.
In `@src/routes/stellar/paymentRoutes.js`:
- Line 19: Update the controller used by the /quote route, getQuote, to return
the standard { success, message, data } envelope, moving the quote payload from
the bespoke quote property into data and providing the required message while
preserving the existing quote contents.
In `@start-mongo.js`:
- Line 9: Update the start-mongo.js bootstrap to avoid overwriting the
developer’s existing .env by writing only to a dedicated test environment file
or refusing when .env exists. Remove the hardcoded JWT_SECRET value from the
script, read the test secret from an environment variable, and document that
variable in .env.example.
In `@test-phase5.js`:
- Around line 53-60: Update the course-update request in the test flow around
courseId and updateRes to use PUT instead of PATCH, matching the registered
/api/courses/:id route so the category update is actually exercised.
- Line 19: Remove the hardcoded JWT fallback secrets and require JWT_SECRET from
the environment, failing test setup when it is absent. Update test-phase5.js:19
and both fallback uses in test/category.integration.test.js:27-28 to use the
validated environment value; document the test-only variable in .env.example if
it is not already documented.
In `@test-phase6.js`:
- Around line 19-22: Remove the hardcoded JWT secret fallback from both
token-signing calls in test-phase6.js. Read JWT_SECRET from the environment
once, fail fast with a clear error when it is missing, and reuse that validated
value for adminToken and studentToken; do not modify the script’s logging or
test structure.
In `@test/category.integration.test.js`:
- Around line 18-22: Require MONGO_URI_TEST in test/category.integration.test.js
lines 18-22, test/category.test.js lines 5-15, and test-phase5.js lines 10-16;
validate that each URI targets an explicitly test-named database, connect using
it exclusively, and perform fixture deletion only after that validation.
---
Nitpick comments:
In `@src/controllers/categoryController.js`:
- Line 85: Update the category controller endpoint responses, including the
response at the categories success path, to consistently use the documented {
success, message, data } envelope. Move category, courses, pagination, and other
endpoint-specific fields under data, and provide an appropriate message while
preserving the existing success payload values.
In `@src/controllers/courses/courseController.js`:
- Around line 63-69: Update the category lookup in getCourses to accept the
category filter as either an ObjectId or slug, matching createCourse and
updateCourse, and require isActive: true. Preserve the existing empty-course
response when no active category matches, then apply the resolved category
reference to the course query.
In `@src/controllers/stellar/donationController.js`:
- Around line 196-220: Add integration coverage for the transient branch in the
donation controller’s verification flow, asserting the 202 response and
persisted donation status are "retrying". Also verify that verifyPaymentOnChain
is enqueued with the donation transaction ID and idempotency key
`verify:${result.hash}`.
In `@src/models/Job.js`:
- Around line 1-28: Implement a retention strategy for completed jobs in the Job
schema by adding a TTL index on completedAt with the configured retention
duration, while excluding dead jobs from automatic deletion so they remain
available for debugging. Use an appropriate partial filter or equivalent index
configuration to limit expiration to completed documents.
In `@src/routes/jobsRoutes.js`:
- Around line 13-20: Update the authorization check in the router middleware to
compare the supplied bearer token and JOBS_DASHBOARD_TOKEN using a constant-time
comparison, handling differing lengths safely before comparison. Preserve the
existing 404 response for an unset token, 401 response for invalid credentials,
and next() behavior for valid credentials.
In `@src/routes/stellar/paymentRoutes.js`:
- Line 7: Add direct Jest HTTP coverage for the `/quote` endpoint handled by
`getQuote`, using the project’s existing request-testing approach (such as
supertest). Cover successful responses, validation failures, and controller
error paths, and assert the expected response payload shape without relying on
manually rebuilt XDR operations.
In `@test/jobs.test.js`:
- Around line 1-66: Add tests covering the Mongo-backed queue path, using
mongo-memory-server when available: exercise processNext, verify failed jobs are
dead-lettered after maxAttempts, and verify startJobs requeues startup jobs from
active to retrying. Keep the existing inline deduplication, retry, and
dashboard-auth tests unchanged.
In `@test/pathPayments.test.js`:
- Around line 96-157: Update the fee-split test to invoke the production
buildPathPaymentTransaction function instead of manually constructing both
pathPaymentStrictReceive operations and recomputing split amounts. Keep
assertions for two operations, destination PLATFORM_WALLET, correct native/USDC
asset ordering, and summed destination/send-max amounts so the production split
and transaction-building logic are exercised directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f1d67c2d-a545-4243-ab1f-69c3bd095ef1
📒 Files selected for processing (38)
.env.exampleREADME.mdapp.jspackage.jsonserver.jsservices/emails/sendMail.jssrc/config/validateEnv.jssrc/controllers/authController.jssrc/controllers/categoryController.jssrc/controllers/courses/courseController.jssrc/controllers/stellar/donationController.jssrc/controllers/stellar/paymentController.jssrc/jobs/handlers.jssrc/jobs/queue.jssrc/middlewares/authMiddleware.jssrc/models/Book.jssrc/models/Category.jssrc/models/Course.jssrc/models/Job.jssrc/models/Transaction.jssrc/models/User.jssrc/routes/categoryRoutes.jssrc/routes/courses/courseRoutes.jssrc/routes/jobsRoutes.jssrc/routes/stellar/paymentRoutes.jssrc/scripts/migrateCategories.jssrc/scripts/seedCategories.jssrc/services/stellar/stellarService.jssrc/utils/cache.jsstart-mongo.jstest-phase5.jstest-phase6.jstest/category.integration.test.jstest/category.test.jstest/jobHandlers.test.jstest/jobs.test.jstest/migrateCategories.test.jstest/pathPayments.test.js
| "start": "node server.js", | ||
| "dev": "nodemon server.js", | ||
| "seed": "node src/scripts/seedDatabase.js", | ||
| "seed:categories": "node src/scripts/seedCategories.js", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve or update the documented seed command.
Replacing seed breaks the npm run seed command still documented in README.md. Restore the old alias or update the documentation and migration instructions together.
🤖 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 `@package.json` at line 9, The package scripts no longer provide the documented
npm run seed command. Update the package.json script configuration around
seed:categories to restore the seed alias, or consistently rename the command in
README.md and migration instructions while preserving the documented seeding
workflow.
| const EMAILJS_PUBLIC_KEY = process.env.EMAILJS_PUBLIC_KEY || "p_hFoYPb6o0w7206-"; | ||
| const EMAILJS_SERVICE_ID = process.env.EMAILJS_SERVICE_ID; | ||
| const EMAILJS_TEMPLATE_ID = process.env.EMAILJS_TEMPLATE_ID; | ||
| const EMAILJS_RECEIPT_TEMPLATE_ID = process.env.EMAILJS_RECEIPT_TEMPLATE_ID || EMAILJS_TEMPLATE_ID; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Silent fallback to the OTP template for receipts risks sending the wrong email content.
If EMAILJS_RECEIPT_TEMPLATE_ID is unset, EMAILJS_RECEIPT_TEMPLATE_ID becomes EMAILJS_TEMPLATE_ID (the OTP template). sendTemplate's guard only checks that a templateId is truthy, so this passes validation and silently sends receipt data through the OTP template — likely rendering garbage or the wrong message to the donor. Prefer failing loudly (throw if EMAILJS_RECEIPT_TEMPLATE_ID is missing) over reusing an unrelated template. As per path instructions, this configuration belongs in .env.example.
🤖 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 `@services/emails/sendMail.js` at line 6, Remove the fallback from
EMAILJS_RECEIPT_TEMPLATE_ID so it cannot reuse EMAILJS_TEMPLATE_ID; validate
that the receipt template ID is configured and fail loudly when it is missing.
Also add the required EMAILJS_RECEIPT_TEMPLATE_ID configuration entry to
.env.example.
Source: Path instructions
| export const sendOtpEmail = async (otp, email) => { | ||
| if (!email || !otp) throw new Error("Email and OTP are required to send the email."); | ||
| logger.info(`Sending OTP email to: ${email}`); | ||
| return sendTemplate(EMAILJS_TEMPLATE_ID, email, { otp }); | ||
| }; | ||
|
|
||
| export const sendReceiptEmail = async (receipt) => { | ||
| if (!receipt.email || !receipt.txHash) throw new Error("Receipt email and transaction hash are required"); | ||
| logger.info(`Sending receipt for ${receipt.txHash} to: ${receipt.email}`); | ||
| return sendTemplate(EMAILJS_RECEIPT_TEMPLATE_ID, receipt.email, receipt); | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid logging raw email addresses.
Sending OTP email to: ${email} and Sending receipt for ${receipt.txHash} to: ${receipt.email} write plaintext user emails to logs. Static analysis flags this too. Prefer logging a non-identifying reference (e.g., userId, or a masked/hashed email) to avoid PII retention in log stores.
🛡 Proposed fix
- logger.info(`Sending OTP email to: ${email}`);
+ logger.info("Sending OTP email");
...
- logger.info(`Sending receipt for ${receipt.txHash} to: ${receipt.email}`);
+ logger.info(`Sending receipt for ${receipt.txHash}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const sendOtpEmail = async (otp, email) => { | |
| if (!email || !otp) throw new Error("Email and OTP are required to send the email."); | |
| logger.info(`Sending OTP email to: ${email}`); | |
| return sendTemplate(EMAILJS_TEMPLATE_ID, email, { otp }); | |
| }; | |
| export const sendReceiptEmail = async (receipt) => { | |
| if (!receipt.email || !receipt.txHash) throw new Error("Receipt email and transaction hash are required"); | |
| logger.info(`Sending receipt for ${receipt.txHash} to: ${receipt.email}`); | |
| return sendTemplate(EMAILJS_RECEIPT_TEMPLATE_ID, receipt.email, receipt); | |
| }; | |
| export const sendOtpEmail = async (otp, email) => { | |
| if (!email || !otp) throw new Error("Email and OTP are required to send the email."); | |
| logger.info("Sending OTP email"); | |
| return sendTemplate(EMAILJS_TEMPLATE_ID, email, { otp }); | |
| }; | |
| export const sendReceiptEmail = async (receipt) => { | |
| if (!receipt.email || !receipt.txHash) throw new Error("Receipt email and transaction hash are required"); | |
| logger.info(`Sending receipt for ${receipt.txHash}`); | |
| return sendTemplate(EMAILJS_RECEIPT_TEMPLATE_ID, receipt.email, receipt); | |
| }; |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 34-34: Avoid logging sensitive data
Context: logger.info(Sending OTP email to: ${email})
Note: [CWE-532] Insertion of Sensitive Information into Log File.
(log-sensitive-data)
[warning] 40-40: Avoid logging sensitive data
Context: logger.info(Sending receipt for ${receipt.txHash} to: ${receipt.email})
Note: [CWE-532] Insertion of Sensitive Information into Log File.
(log-sensitive-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 `@services/emails/sendMail.js` around lines 33 - 43, Update sendOtpEmail and
sendReceiptEmail to stop logging plaintext email addresses; log a
non-identifying reference such as an available userId or masked/hashed email
instead, while preserving the existing email delivery behavior and
transaction-hash context.
Source: Linters/SAST tools
| await enqueue( | ||
| "sendOtpEmail", | ||
| { userId: user._id.toString(), otp: generatedOtp }, | ||
| { attempts: 5, backoffMs: 1000, idempotencyKey: `otp:${user._id}:${generatedOtp}` } | ||
| ); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether generatedOtp is defined anywhere in authController.js or delegated to a User model method.
rg -n 'generatedOtp' src/controllers/authController.js
rg -n -A5 -B5 'generateOtp|randomInt|otp' src/models/User.jsRepository: Deen-Bridge/dnb-backend
Length of output: 372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- authController.js outline ---'
ast-grep outline src/controllers/authController.js --view expanded || true
echo '--- emailRoutes.js outline ---'
ast-grep outline src/routes/emailRoutes.js --view expanded || true
echo '--- authController.js relevant lines ---'
sed -n '1,180p' src/controllers/authController.js | cat -n
echo '--- emailRoutes.js relevant lines ---'
sed -n '1,220p' src/routes/emailRoutes.js | cat -nRepository: Deen-Bridge/dnb-backend
Length of output: 9386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in ["src/controllers/authController.js","src/routes/emailRoutes.js"]:
p=Path(path)
print(f"\n## {path} ({sum(1 for _ in p.open())} lines)")
PYRepository: Deen-Bridge/dnb-backend
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## authController.js\n'
sed -n '1,170p' src/controllers/authController.js
printf '\n## emailRoutes.js\n'
sed -n '1,220p' src/routes/emailRoutes.jsRepository: Deen-Bridge/dnb-backend
Length of output: 6517
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- search OTP usage ---'
rg -n "generatedOtp|otp" src | sed -n '1,200p'
echo
echo '--- emailRoutes.js with line numbers ---'
cat -n src/routes/emailRoutes.js
echo
echo '--- authController registerUser region ---'
sed -n '103,135p' src/controllers/authController.js | cat -nRepository: Deen-Bridge/dnb-backend
Length of output: 3870
Generate a per-registration OTP here. generatedOtp is imported as a module-level constant from src/routes/emailRoutes.js, so every signup reuses the same code until the process restarts. Move OTP creation into registerUser and pass that fresh value to enqueue().
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 131-131: Avoid logging sensitive data
Context: logger.info(✅ User registered successfully: ${email} (ID: ${user._id}))
Note: [CWE-532] Insertion of Sensitive Information into Log File.
(log-sensitive-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 `@src/controllers/authController.js` around lines 126 - 131, Update
registerUser to generate a fresh OTP for each registration instead of using the
module-level generatedOtp imported from emailRoutes.js. Remove that dependency
in this flow, create the OTP within registerUser, and pass the per-registration
value to the sendOtpEmail enqueue payload and idempotencyKey.
| export const deleteCategory = catchAsync(async (req, res) => { | ||
| const categoryId = req.params.id; | ||
| const category = await Category.findById(categoryId); | ||
|
|
||
| if (!category) { | ||
| return res.status(404).json({ success: false, message: 'Category not found' }); | ||
| } | ||
|
|
||
| const coursesCount = await Course.countDocuments({ categoryRef: categoryId }); | ||
|
|
||
| if (coursesCount > 0) { | ||
| category.isActive = false; | ||
| await category.save(); | ||
| return res.status(200).json({ | ||
| success: true, | ||
| message: 'Category soft-deleted because it has associated courses', | ||
| category | ||
| }); | ||
| } else { | ||
| await Category.deleteOne({ _id: categoryId }); | ||
| return res.status(200).json({ | ||
| success: true, | ||
| message: 'Category hard-deleted because it has no associated courses' | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Soft-delete guard ignores Books, so book-only categories get hard-deleted and orphan their categoryRef.
The intent (per issue #42) is soft deletion whenever a category is referenced. Right now deleteCategory only counts Course documents. Since Book also carries a categoryRef (see src/models/Book.js lines 14-17), a category referenced solely by books falls into the else branch and is hard-deleted — leaving those books pointing at a categoryRef that no longer exists.
Why it matters: those dangling references break any populate/lookup on books and quietly corrupt referential integrity that the migration just worked to establish.
🛡️ Proposed fix: count books too
+import Book from '../models/Book.js';
+
export const deleteCategory = catchAsync(async (req, res) => {
const categoryId = req.params.id;
const category = await Category.findById(categoryId);
if (!category) {
return res.status(404).json({ success: false, message: 'Category not found' });
}
- const coursesCount = await Course.countDocuments({ categoryRef: categoryId });
+ const [coursesCount, booksCount] = await Promise.all([
+ Course.countDocuments({ categoryRef: categoryId }),
+ Book.countDocuments({ categoryRef: categoryId }),
+ ]);
- if (coursesCount > 0) {
+ if (coursesCount + booksCount > 0) {
category.isActive = false;
await category.save();
return res.status(200).json({
success: true,
- message: 'Category soft-deleted because it has associated courses',
+ message: 'Category soft-deleted because it has associated courses or books',
category
});
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const deleteCategory = catchAsync(async (req, res) => { | |
| const categoryId = req.params.id; | |
| const category = await Category.findById(categoryId); | |
| if (!category) { | |
| return res.status(404).json({ success: false, message: 'Category not found' }); | |
| } | |
| const coursesCount = await Course.countDocuments({ categoryRef: categoryId }); | |
| if (coursesCount > 0) { | |
| category.isActive = false; | |
| await category.save(); | |
| return res.status(200).json({ | |
| success: true, | |
| message: 'Category soft-deleted because it has associated courses', | |
| category | |
| }); | |
| } else { | |
| await Category.deleteOne({ _id: categoryId }); | |
| return res.status(200).json({ | |
| success: true, | |
| message: 'Category hard-deleted because it has no associated courses' | |
| }); | |
| } | |
| }); | |
| import Book from '../models/Book.js'; | |
| export const deleteCategory = catchAsync(async (req, res) => { | |
| const categoryId = req.params.id; | |
| const category = await Category.findById(categoryId); | |
| if (!category) { | |
| return res.status(404).json({ success: false, message: 'Category not found' }); | |
| } | |
| const [coursesCount, booksCount] = await Promise.all([ | |
| Course.countDocuments({ categoryRef: categoryId }), | |
| Book.countDocuments({ categoryRef: categoryId }), | |
| ]); | |
| if (coursesCount + booksCount > 0) { | |
| category.isActive = false; | |
| await category.save(); | |
| return res.status(200).json({ | |
| success: true, | |
| message: 'Category soft-deleted because it has associated courses or books', | |
| category | |
| }); | |
| } else { | |
| await Category.deleteOne({ _id: categoryId }); | |
| return res.status(200).json({ | |
| success: true, | |
| message: 'Category hard-deleted because it has no associated courses' | |
| }); | |
| } | |
| }); |
🤖 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 `@src/controllers/categoryController.js` around lines 189 - 214, Update
deleteCategory to count Book documents with the same categoryRef in addition to
Course documents, and soft-delete the category whenever either count is greater
than zero. Preserve the existing hard-delete behavior only when neither courses
nor books reference the category.
| const uri = mongod.getUri(); | ||
|
|
||
| // Write to .env | ||
| fs.writeFileSync('.env', `MONGO_URI=${uri}\nJWT_SECRET=supersecret\nNODE_ENV=test\nPORT=5000\n`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Unconditionally overwriting .env will clobber a developer's real config, and the hardcoded JWT secret should be documented in .env.example.
Thanks for adding an easy in-memory Mongo bootstrap — really handy! Two things to fix before this lands:
- Destructive write:
fs.writeFileSync('.env', ...)replaces the entire.envfile. Anyone who runs this with an existing.env(real Mongo URI, live JWT secret, API keys) will silently lose it. Please write to a dedicated file (e.g..env.test) or refuse to run if.envalready exists. - Hardcoded secret:
JWT_SECRET=supersecretis a literal secret in source. Even for local/test use, configuration (including test secrets) should come from environment variables documented in.env.examplerather than being baked into a committed script.
As per path instructions: "Flag hardcoded secrets, connection strings, JWT secrets, or wallet keys; all configuration belongs in environment variables documented in .env.example."
🛡️ Safer approach
- // Write to .env
- fs.writeFileSync('.env', `MONGO_URI=${uri}\nJWT_SECRET=supersecret\nNODE_ENV=test\nPORT=5000\n`);
+ // Write to a dedicated test env file so we never clobber a real .env
+ const jwtSecret = process.env.JWT_SECRET || 'test-only-secret';
+ fs.writeFileSync('.env.test', `MONGO_URI=${uri}\nJWT_SECRET=${jwtSecret}\nNODE_ENV=test\nPORT=5000\n`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fs.writeFileSync('.env', `MONGO_URI=${uri}\nJWT_SECRET=supersecret\nNODE_ENV=test\nPORT=5000\n`); | |
| // Write to a dedicated test env file so we never clobber a real .env | |
| const jwtSecret = process.env.JWT_SECRET || 'test-only-secret'; | |
| fs.writeFileSync('.env.test', `MONGO_URI=${uri}\nJWT_SECRET=${jwtSecret}\nNODE_ENV=test\nPORT=5000\n`); |
🤖 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 `@start-mongo.js` at line 9, Update the start-mongo.js bootstrap to avoid
overwriting the developer’s existing .env by writing only to a dedicated test
environment file or refusing when .env exists. Remove the hardcoded JWT_SECRET
value from the script, read the test secret from an environment variable, and
document that variable in .env.example.
Source: Path instructions
| await User.deleteMany({}); | ||
|
|
||
| const user = await User.create({ name: 'Admin User', email: 'admin@phase5.com', password: 'password123', role: 'tutor' }); | ||
| const token = jwt.sign({ userId: user._id, sessionId: '123' }, process.env.JWT_SECRET || "deenbridge-temp-secret-key-2024", { expiresIn: '1h' }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove hardcoded JWT fallback secrets.
These literals violate the configuration boundary and make tests silently use a known signing key when configuration is missing. Load a test-only JWT_SECRET from the environment and fail setup if it is absent.
test-phase5.js#L19-L19: remove thedeenbridge-temp-secret-key-2024fallback.test/category.integration.test.js#L27-L28: remove both fallback uses.
As per path instructions, “all configuration belongs in environment variables documented in .env.example.”
📍 Affects 2 files
test-phase5.js#L19-L19(this comment)test/category.integration.test.js#L27-L28
🤖 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 `@test-phase5.js` at line 19, Remove the hardcoded JWT fallback secrets and
require JWT_SECRET from the environment, failing test setup when it is absent.
Update test-phase5.js:19 and both fallback uses in
test/category.integration.test.js:27-28 to use the validated environment value;
document the test-only variable in .env.example if it is not already documented.
Source: Path instructions
| // 4. Update course | ||
| const courseId = createRes.body.course._id; | ||
| const updateRes = await request(app) | ||
| .patch(`/api/courses/${courseId}`) | ||
| .set('Authorization', `Bearer ${token}`) | ||
| .send({ | ||
| category: 'aqeedah' // Change category | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the route’s PUT method for the update check.
Line 56 calls PATCH /api/courses/:id, but the registered route is PUT /api/courses/:id. This request currently returns 404 and the script only logs it, so the category-update flow is never exercised.
🤖 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 `@test-phase5.js` around lines 53 - 60, Update the course-update request in the
test flow around courseId and updateRes to use PUT instead of PATCH, matching
the registered /api/courses/:id route so the category update is actually
exercised.
| const adminToken = jwt.sign({ userId: admin._id, sessionId: '123' }, process.env.JWT_SECRET || "deenbridge-temp-secret-key-2024", { expiresIn: '1h' }); | ||
|
|
||
| const student = await User.create({ name: 'Student', email: 'student@test.com', password: 'password123', role: 'student' }); | ||
| const studentToken = jwt.sign({ userId: student._id, sessionId: '456' }, process.env.JWT_SECRET || "deenbridge-temp-secret-key-2024", { expiresIn: '1h' }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Avoid hardcoding the JWT secret fallback in the test script.
The literal "deenbridge-temp-secret-key-2024" fallback appears here (lines 19 and 22). If JWT_SECRET isn't set, these tokens are signed with a secret baked into source — the same weak default that middleware falls back to. Please rely on process.env.JWT_SECRET (documented in .env.example) and fail fast if it's missing, rather than shipping the fallback in a committed file.
Also worth noting for CI health: this file uses console.log rather than assertions, so it won't fail the Jest runner on regressions — the assertion-based test/*.test.js files are the ones that actually protect these paths.
As per path instructions: "Flag hardcoded secrets, connection strings, JWT secrets, or wallet keys; all configuration belongs in environment variables documented in .env.example."
🤖 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 `@test-phase6.js` around lines 19 - 22, Remove the hardcoded JWT secret
fallback from both token-signing calls in test-phase6.js. Read JWT_SECRET from
the environment once, fail fast with a clear error when it is missing, and reuse
that validated value for adminToken and studentToken; do not modify the script’s
logging or test structure.
Source: Path instructions
| beforeAll(async () => { | ||
| await mongoose.connect(process.env.MONGO_URI); | ||
| await Category.deleteMany({}); | ||
| await Course.deleteMany({}); | ||
| await User.deleteMany({}); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Isolate destructive tests from the application database.
These test paths connect to the normal MONGO_URI and then delete durable collections. Running Jest or the phase script against an incorrectly configured environment can erase real users, courses, and categories. Require a dedicated MONGO_URI_TEST, reject non-test database names, and use it exclusively in test setup.
test/category.integration.test.js#L18-L22: connect to and clear only the dedicated test database.test/category.test.js#L5-L15: connect to and clear only the dedicated test database.test-phase5.js#L10-L16: require the dedicated test database before deleting fixtures.
📍 Affects 3 files
test/category.integration.test.js#L18-L22(this comment)test/category.test.js#L5-L15test-phase5.js#L10-L16
🤖 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 `@test/category.integration.test.js` around lines 18 - 22, Require
MONGO_URI_TEST in test/category.integration.test.js lines 18-22,
test/category.test.js lines 5-15, and test-phase5.js lines 10-16; validate that
each URI targets an explicitly test-named database, connect using it
exclusively, and perform fixture deletion only after that validation.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/config/validateEnv.js (2)
27-30: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire
JOBS_DASHBOARD_TOKENfor production.Listing this variable as optional allows production to start without dashboard authentication configured;
src/routes/jobsRoutes.jsthen returns404, silently disabling the dashboard. Move it to production-required validation (while retaining test/dev flexibility) and validate that the configured token is sufficiently strong.🤖 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 `@src/config/validateEnv.js` around lines 27 - 30, Update the environment validation in validateEnv to treat JOBS_DASHBOARD_TOKEN as required in production while preserving its optional behavior for test and development environments. Add production-only validation that the configured token meets the project’s required strength criteria, and remove it from the optional-variable list.Source: Path instructions
38-45: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject unknown
STELLAR_NETWORKvalues instead of defaulting them to testnet.A typo such as
mainentsilently selectshttps://horizon-testnet.stellar.org, which can make production payment verification and path-payment flows use the wrong network. Accept onlymainnetortestnet, and fail validation otherwise.Proposed validation
const network = process.env.STELLAR_NETWORK || "testnet"; +if (!["mainnet", "testnet"].includes(network)) { + throw new Error("STELLAR_NETWORK must be either mainnet or testnet"); +}🤖 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 `@src/config/validateEnv.js` around lines 38 - 45, Update the network validation in validateEnv so STELLAR_NETWORK accepts only "mainnet" or "testnet"; reject any other explicitly provided value instead of treating it as testnet. Preserve the existing default to testnet when the variable is unset, and only assign the corresponding HORIZON_URLS value after validation succeeds.
🤖 Prompt for all review comments with 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.
Outside diff comments:
In `@src/config/validateEnv.js`:
- Around line 27-30: Update the environment validation in validateEnv to treat
JOBS_DASHBOARD_TOKEN as required in production while preserving its optional
behavior for test and development environments. Add production-only validation
that the configured token meets the project’s required strength criteria, and
remove it from the optional-variable list.
- Around line 38-45: Update the network validation in validateEnv so
STELLAR_NETWORK accepts only "mainnet" or "testnet"; reject any other explicitly
provided value instead of treating it as testnet. Preserve the existing default
to testnet when the variable is unset, and only assign the corresponding
HORIZON_URLS value after validation succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2de1e654-a1ad-4e1b-9e8b-410cc3334aef
📒 Files selected for processing (3)
.env.examplesrc/config/validateEnv.jssrc/services/stellar/stellarService.js
🚧 Files skipped from review as they are similar to previous changes (2)
- .env.example
- src/services/stellar/stellarService.js
4 participants
Description
Resolves #42
This PR overhauls the category system by migrating from hardcoded string-based categories to a dedicated
Categorymodel. It establishes a robust taxonomy with automated caching, dynamic aggregation, and role-based access control, setting the foundation for dynamic content discovery.Changes Made
1. Data Models & Taxonomies
Categorymodel (src/models/Category.js) withname,slug,description,icon, andisActivefields.CourseandBookmodels to include acategoryRefreference to theCategorymodel, while keeping the stringcategoryfield synced (viapre('save')hooks) for backward compatibility.slugifylogic inpre('save')hooks on the Category model to automatically format the category names into URL-friendly slugs.2. Controllers & Routes
categoryController.jswith CRUD endpoints:GET /api/categories: Fetch all active categories with dynamic stats (course count and book count) using MongoDB aggregation ($lookupand$group).GET /api/categories/:slug: Fetch a single category by slug.POST /api/categories: Admin-only route to create a new category.PUT /api/categories/:slug: Admin-only route to update an existing category.DELETE /api/categories/:slug: Admin-only route to soft-delete or disable a category.categoryRoutes.jsand wired it intoapp.jsunder/api/categories.3. Automated Caching & Invalidation
cacheMiddleware) to the read routes (GET /api/categories,GET /api/categories/:slug) with a 1-hour TTL.invalidateCacheMiddlewareon related mutating endpoints (Course/Book creation/updates) to automatically bust the Category caches and keep the dynamic course/book counters accurate.4. Migration & Seeding Scripts
src/scripts/seedCategories.js: Seed script to initialize the core disciplines (Qur'an, Hadith, Fiqh, Aqeedah, etc.) into the database. (Idempotent and safe to run multiple times).src/scripts/migrateCategories.js: Data migration script to scan all existing Courses/Books, canonicalize their legacy string categories, create correspondingCategorydocuments, and backfill the newcategoryRefObjectIds.5. Documentation
README.mdto include instructions for runningnpm run seed:categories.Testing
Categorymodel creation and slugification via unit tests.adminandstudentroles to ensure RBAC is enforced.GETand correctly busted onPOST/PUT/DELETEmutations.migrateCategories.jsbackfills existing data smoothly and is completely idempotent.Deployment Notes
Before launching this feature, you must run the following script in the production environment to backfill legacy data: