docs(api): add OpenAPI 3.1 spec for the full API surface - #84
Conversation
WalkthroughThe PR adds an OpenAPI 3.1 contract covering backend metadata, authentication, domain resources, Stellar transactions, uploads, and administrative jobs. It defines reusable schemas and security schemes, documents inconsistent response envelopes, and links the specification from the README. ChangesAPI Contract
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/stellar/stellarService.js (1)
678-706: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd the same
assetConfigguard here aspreflightPayment/buildSep7Uri.Every sibling function (
resolveAsset,buildSep7Uri,preflightPayment) explicitly checksif (!config)before touchingconfig.issuer/config.code.verifyPaymentOperationsskips that check, so an unsupported/staleassetCode(e.g. a legacyrefund.currency/transaction.currencyvalue that no longer exists in the registry) throws insidematchesAsset, gets swallowed by the outer try/catch, and surfaces only as the generic"Verification failed"(line 733) instead of a specific, debuggable reason — this is the exact scenario refund/payment verification failures would need to be triaged quickly in production.🛡️ Proposed fix
export const verifyPaymentOperations = async (txHash, expectedPayments, assetCode = DEFAULT_ASSET_CODE) => { try { const verification = await verifyTransaction(txHash); const assetConfig = getAssetConfig(assetCode, NETWORK); + if (!assetConfig) { + return { + verified: false, + reason: `Unsupported asset code: ${assetCode}. Supported: ${getSupportedCodes(NETWORK).join(", ")}`, + }; + } if (!verification.exists) {🤖 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/services/stellar/stellarService.js` around lines 678 - 706, Add an explicit missing-config guard in verifyPaymentOperations immediately after getAssetConfig(assetCode, NETWORK), before matchesAsset accesses assetConfig. Return the function’s existing verification-failure shape with a specific reason for unsupported or stale asset codes, while preserving normal verification behavior for valid configurations and the existing outer error handling.
🧹 Nitpick comments (1)
src/config/assets.js (1)
60-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNice, verified issuer addresses. I cross-checked all four issuer keys (testnet/mainnet USDC & EURC) against Circle's published Stellar issuers and they match — good attention to detail here, this is exactly the kind of thing that would be very costly to get wrong.
Two small hardening ideas while we're at it:
getDefaultAssetCode's fallback (entry ? entry.code : "USDC", line 79) returns a hardcoded string without confirming"USDC"actually exists in that network's registry. If a future network config forgets to mark an entryisDefault, this silently returns an asset code that might not resolve.REGISTRYis exported as-is (line 85) withoutObject.freeze, so any importer could mutate issuer/decimals fields at runtime.♻️ Optional hardening
export const getDefaultAssetCode = (network = NETWORK) => { const registry = getRegistry(network); const entry = Object.values(registry).find((a) => a.isDefault); - return entry ? entry.code : "USDC"; + if (entry) return entry.code; + if (registry.USDC) return "USDC"; + throw new Error(`No default asset configured for network: ${network}`); };-export { REGISTRY }; +export { REGISTRY }; +Object.freeze(REGISTRY); +Object.values(REGISTRY).forEach((registry) => + Object.values(registry).forEach((asset) => Object.freeze(asset)) +);🤖 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/assets.js` around lines 60 - 85, Harden getDefaultAssetCode by validating that the fallback "USDC" exists in the selected registry and return it only when present; otherwise raise an explicit error instead of returning an unresolved code. Also freeze REGISTRY, including nested network and asset configuration objects, before exporting it so imported configurations cannot be mutated at runtime.
🤖 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 `@openapi.yaml`:
- Around line 350-357: Update openapi.yaml at lines 350-357 to replace
Transaction.currency’s USDC-only enum with the supported asset registry codes
and keep it consistent with sendAsset; at lines 231-269, add the multi-asset
currency/asset field to the Book and Course schemas; at lines 1618-1638, expose
sendAsset in the /api/stellar/payment/initialize request body when accepted by
the controller. Also update the top-level description to reflect multi-asset
Stellar payments.
- Around line 538-552: Update the requestBody schema for POST
/api/auth/reset-password to advertise the handler’s expected fields email, otp,
and newPassword, replacing token and password. Mark all three properties as
required and preserve the existing string/password formatting where appropriate.
- Around line 781-841: The OpenAPI contract is missing the mounted
course-progress and learning-dashboard routes plus their progress payload
schema. Add documented operations for /api/courses/{id}/progress and
/api/users/me/learning, matching the routes’ methods, parameters, security,
request bodies, and responses; define and reference a CourseProgress schema when
the progress payload contains structured data, reusing existing component
schemas where applicable.
- Around line 88-92: Split the shared Limit parameter into route-specific
definitions, adding maximum: 50 for reels and maximum: 100 for notifications,
search, payment, and payout routes. Update each route to reference its
appropriate limit definition while preserving the existing query name, minimum,
and default.
In `@src/controllers/analytics/analyticsController.js`:
- Around line 36-43: Update the course lookup flow in the analytics progress
handler to verify that the authenticated user identified by userId is enrolled
in the course’s enrolledUsers collection before recording progress. Return the
existing unauthorized/forbidden response pattern when access is missing, while
preserving the current Course.findById and not-found behavior.
- Around line 56-80: Update the progress persistence flow around
CourseProgress.findOne and findOneAndUpdate to use an atomic update pipeline or
transaction that merges each lesson into lessonsCompleted without
read-modify-replace races. Preserve unique lesson entries, recalculate
percentComplete and completedAt from the merged set, and retry duplicate-key
failures from concurrent upserts so competing first writes do not return a 500.
- Around line 23-30: Standardize endpoint response envelopes to { success,
message, data }: update analyticsController progress success and error paths,
userController dashboard responses, and adjust the corresponding courseProgress
assertions. In src/controllers/analytics/analyticsController.js lines 23-30,
wrap null progress in data.progress with a message; at lines 83-86, apply the
same envelope to updated progress and failures; in
src/controllers/userController.js lines 457-460, place courses under data and
normalize failures; update test/courseProgress.test.js lines 70-80 and 111-114
to assert the normalized payloads.
- Line 38: Validate lastPositionSeconds in the analytics request handler before
persistence: require a finite number greater than or equal to zero, return an
appropriate client error for invalid input, and persist the validated numeric
value instead of coercing malformed or negative values through
Number(lastPositionSeconds || 0).
In `@src/controllers/authController.js`:
- Around line 305-310: Update the password-reset success logging around the
logger.info call to avoid recording the raw email address. Log the user's _id
when available, or mask the email before logging, while preserving the existing
response behavior.
- Around line 271-274: Update the new-password validation in the password-reset
handler around the existing required-fields check and bcrypt.hash call: require
newPassword to be a string and meet the minimum password length, returning the
existing 400 validation response for invalid values before hashing. Preserve the
current validation for email and OTP and ensure only validated string passwords
reach bcrypt.hash.
- Around line 219-261: Equalize timing for unknown-account requests in both
password-reset handlers. In src/controllers/authController.js lines 219-261,
update the not-found branch of requestPasswordReset to perform an
equivalent-cost dummy bcrypt operation or fixed delay before returning; in lines
271-294, update the not-found branch of the OTP verification handler to perform
a dummy bcrypt.compare against a fixed placeholder hash. Preserve the existing
anti-enumeration response bodies.
In `@src/controllers/stellar/paymentController.js`:
- Around line 133-139: Normalize all four payment endpoints in
src/controllers/stellar/paymentController.js:133-139,
src/controllers/stellar/paymentController.js:263-269,
src/controllers/stellar/paymentController.js:355-362, and
src/controllers/stellar/paymentController.js:410-415 to the { success, message,
data } envelope. Add data: null to each rejection response, move quote and
preflight payloads under data, and place initialization success payloads under
data while preserving existing success and error behavior.
In `@src/controllers/userController.js`:
- Around line 434-445: Validate the destructured status value in the dashboard
handler before querying CourseProgress, accepting only "completed" and
"in-progress". For any other explicitly supplied value, return HTTP 400
immediately instead of applying the in-progress filter; preserve the existing
default when status is omitted.
In `@src/models/CourseProgress.js`:
- Line 38: Add a compound index for the getLearningDashboard query using user
ascending and updatedAt descending, alongside the existing courseProgressSchema
index, so filtering and sorting use the database index.
In `@src/models/User.js`:
- Around line 54-56: Update the resetTokenHash field definition in the User
model to use select: false, ensuring the OTP hash is excluded from queries and
serialized user documents by default while preserving its existing type and
behavior when explicitly requested.
In `@test/courseProgress.test.js`:
- Around line 42-43: Update the token setup around ownerToken and learnerToken
to use only the required process.env.JWT_SECRET value, removing the hardcoded
fallback secret. Ensure the test fails clearly when JWT_SECRET is missing and
preserve both jwt.sign calls’ existing payloads.
- Around line 51-115: Extend the course progress tests to call GET
/api/courses/:id/progress, asserting the authenticated learner receives the
existing progress record and the documented response when no progress exists.
Reuse the course, learner authentication, and CourseProgress setup from the
existing tests, and cover both scenarios so the newly mounted endpoint has Jest
coverage.
---
Outside diff comments:
In `@src/services/stellar/stellarService.js`:
- Around line 678-706: Add an explicit missing-config guard in
verifyPaymentOperations immediately after getAssetConfig(assetCode, NETWORK),
before matchesAsset accesses assetConfig. Return the function’s existing
verification-failure shape with a specific reason for unsupported or stale asset
codes, while preserving normal verification behavior for valid configurations
and the existing outer error handling.
---
Nitpick comments:
In `@src/config/assets.js`:
- Around line 60-85: Harden getDefaultAssetCode by validating that the fallback
"USDC" exists in the selected registry and return it only when present;
otherwise raise an explicit error instead of returning an unresolved code. Also
freeze REGISTRY, including nested network and asset configuration objects,
before exporting it so imported configurations cannot be mutated at runtime.
🪄 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: 24a8ba0c-12e6-4601-b6ed-3d38e170e9e3
📒 Files selected for processing (26)
README.mdopenapi.yamlsrc/config/assets.jssrc/config/assets.test.jssrc/controllers/analytics/analyticsController.jssrc/controllers/authController.jssrc/controllers/stellar/paymentController.jssrc/controllers/stellar/refundController.jssrc/controllers/stellar/walletController.jssrc/controllers/userController.jssrc/middlewares/security.jssrc/models/Book.jssrc/models/Course.jssrc/models/CourseProgress.jssrc/models/Transaction.jssrc/models/User.jssrc/routes/courses/courseRoutes.jssrc/routes/userRoutes.jssrc/services/stellar/multiAsset.test.jssrc/services/stellar/stellarService.jssrc/utils/otp.jstest/courseProgress.test.jstest/passwordReset.test.jstest/refund.test.jstest/stellarPaymentController.test.jstest/stellarService.test.js
| Limit: | ||
| name: limit | ||
| in: query | ||
| required: false | ||
| schema: { type: integer, minimum: 1, default: 10 } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find limit parsing/clamping in controllers
rg -nP --type=js -C3 '\b(limit)\b\s*=\s*(Number|parseInt|Math\.min)' src/
rg -nP --type=js -C2 'Math\.min\([^)]*limit' src/Repository: Deen-Bridge/dnb-backend
Length of output: 2689
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the OpenAPI parameter block around the shared pagination params.
sed -n '70,140p' openapi.yaml | cat -n
# Show all OpenAPI references to the shared limit parameter.
rg -n 'Limit:|limit:' openapi.yaml
# Show the relevant controller snippets that clamp limit.
sed -n '260,310p' src/controllers/notificationController.js | cat -n
sed -n '70,90p' src/controllers/reelController.js | cat -n
sed -n '290,305p' src/controllers/reelController.js | cat -n
sed -n '58,72p' src/services/search/searchService.js | cat -n
sed -n '122,132p' src/services/search/searchService.js | cat -nRepository: Deen-Bridge/dnb-backend
Length of output: 6939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find every route that reuses the shared Limit parameter.
rg -n '\$ref:\s*"`#/components/parameters/Limit`"|Limit\b' openapi.yaml
# Show the route blocks around each match so the per-route pagination context is visible.
python3 - <<'PY'
from pathlib import Path
text = Path("openapi.yaml").read_text().splitlines()
for i, line in enumerate(text, 1):
if '`#/components/parameters/Limit`' in line or line.strip() == 'Limit:':
start = max(1, i-12)
end = min(len(text), i+18)
print(f"\n--- lines {start}-{end} ---")
for j in range(start, end+1):
print(f"{j:4d}: {text[j-1]}")
PYRepository: Deen-Bridge/dnb-backend
Length of output: 6458
Split Limit per route (openapi.yaml:88-92)
The cap isn’t uniform: reels clamp at 50, while notifications/search/payment/payout routes clamp at 100. A single shared Limit without maximum misstates the API contract and lets clients send values that will be silently truncated.
🤖 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 `@openapi.yaml` around lines 88 - 92, Split the shared Limit parameter into
route-specific definitions, adding maximum: 50 for reels and maximum: 100 for
notifications, search, payment, and payout routes. Update each route to
reference its appropriate limit definition while preserving the existing query
name, minimum, and default.
| amount: { type: string } | ||
| currency: { type: string, enum: [USDC], default: USDC } | ||
| sendAsset: | ||
| type: object | ||
| properties: | ||
| code: { type: string } | ||
| issuer: { type: string } | ||
| sendMax: { type: string } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The spec describes a USDC-only world, but this PR makes payments multi-asset.
All three sites trace back to one cause: the contract was written against the pre-PR model and never absorbed the asset registry (testnet/mainnet USDC, EURC, XLM) that layer 2 introduces. Left as-is, generated clients and validating middleware will reject or silently mis-handle legitimate EURC/XLM flows, and the description at Line 10 ("Stellar USDC payments") is also now understated.
openapi.yaml#L350-L357: widenTransaction.currencybeyondenum: [USDC]to the registry's asset codes, so it is consistent with thesendAssetobject immediately below.openapi.yaml#L231-L269: add the multi-assetcurrency/asset field to theBookandCourseschemas to match the model changes that payment controllers now validate against.openapi.yaml#L1618-L1638: exposesendAsseton the/api/stellar/payment/initializerequest body (if the controller accepts it) so a buyer can build in the asset they quoted.
📍 Affects 1 file
openapi.yaml#L350-L357(this comment)openapi.yaml#L231-L269openapi.yaml#L1618-L1638
🤖 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 `@openapi.yaml` around lines 350 - 357, Update openapi.yaml at lines 350-357 to
replace Transaction.currency’s USDC-only enum with the supported asset registry
codes and keep it consistent with sendAsset; at lines 231-269, add the
multi-asset currency/asset field to the Book and Course schemas; at lines
1618-1638, expose sendAsset in the /api/stellar/payment/initialize request body
when accepted by the controller. Also update the top-level description to
reflect multi-asset Stellar payments.
| /api/auth/reset-password: | ||
| post: | ||
| tags: [Auth] | ||
| summary: Complete a password reset | ||
| security: [] | ||
| requestBody: | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| token: { type: string } | ||
| password: { type: string, format: password } | ||
| required: [token, password] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# What does the reset-password handler actually read from req.body?
ast-grep run --lang javascript --pattern 'const { $$$ } = req.body' src/controllers/authController.js
rg -nP --type=js -C6 'resetPassword|resetPasswordOtp|otp' src/controllers/authController.js
rg -nP --type=js -C3 'reset-password' src/routes/ test/Repository: Deen-Bridge/dnb-backend
Length of output: 5865
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the documented reset-password request body and nearby auth routes.
sed -n '530,565p' openapi.yaml
printf '\n--- auth routes ---\n'
sed -n '1,80p' src/routes/authRoutes.js
printf '\n--- reset-password handler ---\n'
sed -n '260,310p' src/controllers/authController.jsRepository: Deen-Bridge/dnb-backend
Length of output: 4201
Align the reset-password request body with the OTP flow. The handler reads email, otp, and newPassword, but the OpenAPI schema still advertises token and password, so generated clients will send the wrong payload and get a 400.
🤖 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 `@openapi.yaml` around lines 538 - 552, Update the requestBody schema for POST
/api/auth/reset-password to advertise the handler’s expected fields email, otp,
and newPassword, replacing token and password. Mark all three properties as
required and preserve the existing string/password formatting where appropriate.
| /api/courses: | ||
| get: | ||
| tags: [Courses] | ||
| summary: List courses | ||
| description: Cached list. Some list handlers in this codebase return a bare array. | ||
| security: [] | ||
| responses: | ||
| "200": { $ref: "#/components/responses/Ok" } | ||
| x-envelope-unverified: true | ||
| post: | ||
| tags: [Courses] | ||
| summary: Create a course | ||
| requestBody: | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: { $ref: "#/components/schemas/Course" } | ||
| responses: | ||
| "201": { $ref: "#/components/responses/Created" } | ||
| "400": { $ref: "#/components/responses/BadRequest" } | ||
| "401": { $ref: "#/components/responses/Unauthorized" } | ||
| /api/courses/user: | ||
| get: | ||
| tags: [Courses] | ||
| summary: List courses by creator | ||
| security: [] | ||
| parameters: | ||
| - name: createdBy | ||
| in: query | ||
| required: true | ||
| schema: { $ref: "#/components/schemas/ObjectId" } | ||
| responses: | ||
| "200": { $ref: "#/components/responses/Ok" } | ||
| x-envelope-unverified: true | ||
| /api/courses/recommended: | ||
| post: | ||
| tags: [Courses] | ||
| summary: Recommended courses | ||
| description: Uses POST so a preference payload can be sent. Not cached. | ||
| security: [] | ||
| requestBody: | ||
| required: false | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| interests: | ||
| type: array | ||
| items: { type: string } | ||
| userId: { $ref: "#/components/schemas/ObjectId" } | ||
| responses: | ||
| "200": { $ref: "#/components/responses/Ok" } | ||
| x-envelope-unverified: true | ||
| /api/courses/bookmarks: | ||
| get: | ||
| tags: [Courses] | ||
| summary: Bookmarked courses for the caller | ||
| responses: | ||
| "200": { $ref: "#/components/responses/Ok" } | ||
| "401": { $ref: "#/components/responses/Unauthorized" } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List the progress/dashboard routes that need documenting
rg -nP --type=js -C2 '\b(progress|dashboard)\b' src/routes/Repository: Deen-Bridge/dnb-backend
Length of output: 682
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- openapi.yaml: users section around lines 612-780 ---'
sed -n '612,780p' openapi.yaml | cat -n
echo
echo '--- src/routes/userRoutes.js ---'
cat -n src/routes/userRoutes.js
echo
echo '--- src/controllers/userController.js (dashboard-related symbols) ---'
rg -n -C 2 '\b(dashboard|learning|progress)\b' src/controllers/userController.js src/routes/userRoutes.js src/routes/courses/courseRoutes.jsRepository: Deen-Bridge/dnb-backend
Length of output: 13209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- openapi.yaml: any CourseProgress mentions ---'
rg -n -C 2 'CourseProgress|course progress|progress' openapi.yaml
echo
echo '--- src/routes/courses/courseRoutes.js ---'
cat -n src/routes/courses/courseRoutes.js | sed -n '1,120p'
echo
echo '--- src/routes/userRoutes.js ---'
cat -n src/routes/userRoutes.js | sed -n '1,160p'Repository: Deen-Bridge/dnb-backend
Length of output: 211
Add the missing course-progress and learning-dashboard paths to OpenAPI. /api/courses/:id/progress and /api/users/me/learning are mounted in the app but not documented here, and openapi.yaml has no CourseProgress schema either. Add the paths (and schema if the payload includes progress data) so the published contract matches the routes in this PR.
🤖 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 `@openapi.yaml` around lines 781 - 841, The OpenAPI contract is missing the
mounted course-progress and learning-dashboard routes plus their progress
payload schema. Add documented operations for /api/courses/{id}/progress and
/api/users/me/learning, matching the routes’ methods, parameters, security,
request bodies, and responses; define and reference a CourseProgress schema when
the progress payload contains structured data, reusing existing component
schemas where applicable.
| if (!progress) { | ||
| return res.status(200).json({ success: true, progress: null }); | ||
| } | ||
|
|
||
| res.status(200).json({ success: true, progress }); | ||
| } catch (error) { | ||
| logger.error("Get course progress error:", error); | ||
| res.status(500).json({ success: false, message: "Failed to fetch course progress" }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Standardize the new endpoint response envelopes.
These handlers return resource-specific top-level fields (progress or courses) and omit message/data, contrary to the required API shape. Normalize successful and error responses, then update the affected assertions.
src/controllers/analytics/analyticsController.js#L23-L30: return the no-progress result as{ success, message, data: { progress: null } }.src/controllers/analytics/analyticsController.js#L83-L86: return updated progress and failures through the same envelope.src/controllers/userController.js#L457-L460: return dashboard courses underdataand normalize failures.test/courseProgress.test.js#L70-L80: assert the normalized POST-progress payload.test/courseProgress.test.js#L111-L114: assert the normalized dashboard payload.
As per path instructions, src/**/*.js endpoints require consistent response shapes: ({ success, message, data }).
📍 Affects 3 files
src/controllers/analytics/analyticsController.js#L23-L30(this comment)src/controllers/analytics/analyticsController.js#L83-L86src/controllers/userController.js#L457-L460test/courseProgress.test.js#L70-L80test/courseProgress.test.js#L111-L114
🤖 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/analytics/analyticsController.js` around lines 23 - 30,
Standardize endpoint response envelopes to { success, message, data }: update
analyticsController progress success and error paths, userController dashboard
responses, and adjust the corresponding courseProgress assertions. In
src/controllers/analytics/analyticsController.js lines 23-30, wrap null progress
in data.progress with a message; at lines 83-86, apply the same envelope to
updated progress and failures; in src/controllers/userController.js lines
457-460, place courses under data and normalize failures; update
test/courseProgress.test.js lines 70-80 and 111-114 to assert the normalized
payloads.
Source: Path instructions
| const { status = "in-progress" } = req.query; | ||
|
|
||
| const progressRecords = await CourseProgress.find({ user: userId }) | ||
| .sort({ updatedAt: -1 }) | ||
| .populate("course", "title thumbnail createdBy") | ||
| .lean(); | ||
|
|
||
| const courses = progressRecords | ||
| .filter((record) => { | ||
| if (status === "completed") return record.percentComplete >= 100; | ||
| return record.percentComplete < 100; | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject unsupported dashboard statuses.
?status=anything is silently treated as in-progress, hiding client mistakes and returning the wrong subset. Accept only completed and in-progress, and return a 400 for other values.
As per path instructions, src/**/*.js requires unvalidated request input to be flagged.
🤖 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/userController.js` around lines 434 - 445, Validate the
destructured status value in the dashboard handler before querying
CourseProgress, accepting only "completed" and "in-progress". For any other
explicitly supplied value, return HTTP 400 immediately instead of applying the
in-progress filter; preserve the existing default when status is omitted.
Source: Path instructions
| { timestamps: true } | ||
| ); | ||
|
|
||
| courseProgressSchema.index({ user: 1, course: 1 }, { unique: true }); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Add an index for the learning-dashboard query.
getLearningDashboard filters by user and sorts by updatedAt; { user, course } cannot satisfy that ordering, so each user’s records must be sorted in memory. Add { user: 1, updatedAt: -1 }.
Proposed change
courseProgressSchema.index({ user: 1, course: 1 }, { unique: true });
+courseProgressSchema.index({ user: 1, updatedAt: -1 });As per path instructions, src/models/** must flag “missing indexes for new query patterns.”
📝 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.
| courseProgressSchema.index({ user: 1, course: 1 }, { unique: true }); | |
| courseProgressSchema.index({ user: 1, course: 1 }, { unique: true }); | |
| courseProgressSchema.index({ user: 1, updatedAt: -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 `@src/models/CourseProgress.js` at line 38, Add a compound index for the
getLearningDashboard query using user ascending and updatedAt descending,
alongside the existing courseProgressSchema index, so filtering and sorting use
the database index.
Source: Path instructions
| resetTokenHash: { | ||
| type: String, | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Consider select: false on resetTokenHash.
Unlike a password hash, this stores a bcrypt hash of a 6-digit OTP — only 900,000 possible plaintexts. If this field is ever exposed via a serialized user document (profile endpoint, admin view, etc.), it can be brute-forced offline well within the 15-minute expiry window with modest parallel hardware. Excluding it by default limits the blast radius of any future accidental exposure.
🛡️ Suggested change
resetTokenHash: {
type: String,
+ select: false,
},📝 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.
| resetTokenHash: { | |
| type: String, | |
| }, | |
| resetTokenHash: { | |
| type: String, | |
| select: false, | |
| }, |
🤖 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/User.js` around lines 54 - 56, Update the resetTokenHash field
definition in the User model to use select: false, ensuring the OTP hash is
excluded from queries and serialized user documents by default while preserving
its existing type and behavior when explicitly requested.
Source: Path instructions
| ownerToken = jwt.sign({ userId: owner._id, sessionId: "o1" }, process.env.JWT_SECRET || "deenbridge-temp-secret-key-2024"); | ||
| learnerToken = jwt.sign({ userId: learner._id, sessionId: "l1" }, process.env.JWT_SECRET || "deenbridge-temp-secret-key-2024"); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove the hardcoded JWT-secret fallback.
A known fallback secret can mask missing test configuration and violates the repository rule for JavaScript configuration. Require process.env.JWT_SECRET in the test environment instead of embedding a signing key.
As per path instructions, “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/courseProgress.test.js` around lines 42 - 43, Update the token setup
around ownerToken and learnerToken to use only the required
process.env.JWT_SECRET value, removing the hardcoded fallback secret. Ensure the
test fails clearly when JWT_SECRET is missing and preserve both jwt.sign calls’
existing payloads.
Source: Path instructions
| it("creates progress for a learner and computes percent completion idempotently", async () => { | ||
| const course = await Course.create({ | ||
| title: "Course 1", | ||
| description: "Test", | ||
| category: "Tech", | ||
| createdBy: owner._id, | ||
| video: "video-url", | ||
| sections: [ | ||
| { title: "Section 1", order: 1, lessons: [{ _id: new mongoose.Types.ObjectId(), title: "Lesson 1", order: 1, videoUrl: "v1" }] }, | ||
| ], | ||
| }); | ||
|
|
||
| await Course.updateOne({ _id: course._id }, { $addToSet: { enrolledUsers: learner._id } }); | ||
|
|
||
| const createRes = await request(app) | ||
| .post(`/api/courses/${course._id}/progress`) | ||
| .set("Authorization", `Bearer ${learnerToken}`) | ||
| .send({ lessonId: course.sections[0].lessons[0]._id.toString(), lastPositionSeconds: 90 }); | ||
|
|
||
| expect(createRes.status).toBe(200); | ||
| expect(createRes.body.progress.percentComplete).toBe(100); | ||
|
|
||
| const duplicateRes = await request(app) | ||
| .post(`/api/courses/${course._id}/progress`) | ||
| .set("Authorization", `Bearer ${learnerToken}`) | ||
| .send({ lessonId: course.sections[0].lessons[0]._id.toString(), lastPositionSeconds: 120 }); | ||
|
|
||
| expect(duplicateRes.status).toBe(200); | ||
| expect(duplicateRes.body.progress.lessonsCompleted).toHaveLength(1); | ||
| expect(duplicateRes.body.progress.percentComplete).toBe(100); | ||
| }); | ||
|
|
||
| it("returns the learning dashboard for the authenticated user", async () => { | ||
| const course = await Course.create({ | ||
| title: "Dashboard course", | ||
| description: "Test", | ||
| category: "Tech", | ||
| createdBy: owner._id, | ||
| video: "video-url", | ||
| sections: [ | ||
| { title: "Section 1", order: 1, lessons: [{ _id: new mongoose.Types.ObjectId(), title: "Lesson 1", order: 1, videoUrl: "v1" }, { _id: new mongoose.Types.ObjectId(), title: "Lesson 2", order: 2, videoUrl: "v2" }] }, | ||
| ], | ||
| }); | ||
|
|
||
| await Course.updateOne({ _id: course._id }, { $addToSet: { enrolledUsers: learner._id } }); | ||
| await CourseProgress.create({ | ||
| user: learner._id, | ||
| course: course._id, | ||
| lessonsCompleted: [course.sections[0].lessons[0]._id], | ||
| lastLesson: course.sections[0].lessons[0]._id, | ||
| lastPositionSeconds: 50, | ||
| percentComplete: 50, | ||
| completedAt: null, | ||
| updatedAt: new Date(), | ||
| }); | ||
|
|
||
| const res = await request(app) | ||
| .get("/api/users/me/learning") | ||
| .set("Authorization", `Bearer ${learnerToken}`); | ||
|
|
||
| expect(res.status).toBe(200); | ||
| expect(res.body.success).toBe(true); | ||
| expect(res.body.courses).toHaveLength(1); | ||
| expect(res.body.courses[0].percentComplete).toBe(50); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the new GET progress endpoint.
The suite exercises progress creation and the dashboard, but never calls GET /api/courses/:id/progress. Add coverage for both an existing progress record and the documented no-progress response so this newly mounted endpoint is protected in CI.
As per path instructions, src/**/*.js requires Jest coverage for new or changed endpoints.
🤖 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/courseProgress.test.js` around lines 51 - 115, Extend the course
progress tests to call GET /api/courses/:id/progress, asserting the
authenticated learner receives the existing progress record and the documented
response when no progress exists. Reuse the course, learner authentication, and
CourseProgress setup from the existing tests, and cover both scenarios so the
newly mounted endpoint has Jest coverage.
Source: Path instructions
|
@eleven-smg fix conflicts |
9ebb0fa to
8957874
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@openapi.yaml`:
- Around line 144-151: Update the Error schema to match errorHandler: remove the
undocumented data property, add the emitted status and reqId fields, and retain
message, success, plus the development-only error and stack fields with
appropriate types and descriptions.
🪄 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: 6fbbf0a4-54aa-40e7-aad0-60619554ea59
📒 Files selected for processing (2)
README.mdopenapi.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
| Error: | ||
| type: object | ||
| description: Standard error shape produced by the central error handler | ||
| properties: | ||
| success: { type: boolean, examples: [false] } | ||
| message: { type: string } | ||
| data: { description: Null on error responses } | ||
| stack: { type: string, description: Present only outside production } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Align Error with what errorHandler actually emits.
Nice work documenting the envelope inconsistencies up front — that's genuinely useful. One mismatch here though: src/middlewares/errorHandler.js returns { success, status, message, reqId } (plus error and stack in development), so the documented data key is never present, while status and reqId — the field clients use to correlate a failure with server logs — are missing from the contract.
📐 Proposed alignment
Error:
type: object
description: Standard error shape produced by the central error handler
properties:
success: { type: boolean, examples: [false] }
+ status: { type: string, enum: [fail, error], description: fail for 4xx, error for 5xx }
message: { type: string }
- data: { description: Null on error responses }
+ reqId: { type: string, description: Request correlation id, present when the request id middleware ran }
stack: { type: string, description: Present only outside production }📝 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.
| Error: | |
| type: object | |
| description: Standard error shape produced by the central error handler | |
| properties: | |
| success: { type: boolean, examples: [false] } | |
| message: { type: string } | |
| data: { description: Null on error responses } | |
| stack: { type: string, description: Present only outside production } | |
| Error: | |
| type: object | |
| description: Standard error shape produced by the central error handler | |
| properties: | |
| success: { type: boolean, examples: [false] } | |
| status: { type: string, enum: [fail, error], description: fail for 4xx, error for 5xx } | |
| message: { type: string } | |
| reqId: { type: string, description: Request correlation id, present when the request id middleware ran } | |
| stack: { type: string, description: Present only outside production } |
🤖 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 `@openapi.yaml` around lines 144 - 151, Update the Error schema to match
errorHandler: remove the undocumented data property, add the emitted status and
reqId fields, and retain message, success, plus the development-only error and
stack fields with appropriate types and descriptions.
2 participants
What
Adds openapi.yaml, a single self-contained OpenAPI 3.1 document covering every route mounted in app.js, and links it from the README. No runtime dependency, no new package, no network access at runtime, and no behavioural change — the spec file plus one additive README subsection are the entire diff.
I chose the committed-spec option from the acceptance criteria rather than swagger-ui-express. Serving Swagger UI would add a runtime dependency and an unauthenticated HTML route to a production API for documentation that is fully consumable as a file; the spec renders in Swagger Editor, Redoc or Stoplight as-is. Happy to add the /api/docs mount in a follow-up if maintainers prefer it.
New coverage
Corrections to the issue description
Flagging these rather than silently documenting endpoints that do not exist:
Provenance and limitations
Paths, methods, middleware and auth gating come from app.js and src/routes/**, which are authoritative. Component schemas come from the Mongoose schemas. Per-endpoint request bodies are described from route middleware (multer field names, protect, role guards) and the model fields they write — where a controller may accept more, the body is left permissive instead of invented, and I have not asserted field-level detail I could not see. The provenance note is recorded in info.description so future maintainers know exactly how much to trust each part.
Verified locally: LF endings, no tabs, openapi: 3.1.0 header, and path/operation counts. I have not run a full OpenAPI validator or the jest suite against this branch — the change adds no .js, so the CI syntax/boot check and test job are untouched.
Scope
Two files: openapi.yaml (new) and README.md (one additive subsection under API Overview). No source, route, controller, model, config or test file modified or deleted. Targets dev per the README contribution rules.
Closes #21
Summary by CodeRabbit