Add revenue analytics endpoints for educators and platform reporting - #80
Add revenue analytics endpoints for educators and platform reporting#80Myart352 wants to merge 1 commit into
Conversation
WalkthroughAdds transaction analytics APIs, course sections, course progress tracking, and learning dashboard endpoints. Protected routes and tests cover analytics access control, decimal totals, progress updates, and learning data retrieval. ChangesAnalytics and Learning Progress
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant analyticsRoutes
participant analyticsController
participant MongoDB
Client->>analyticsRoutes: Request protected analytics or learning endpoint
analyticsRoutes->>analyticsController: Apply authorization and invoke handler
analyticsController->>MongoDB: Aggregate transactions or query progress
MongoDB-->>analyticsController: Return analytics or progress data
analyticsController-->>Client: Return JSON response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 9
🧹 Nitpick comments (4)
src/routes/analytics/analyticsRoutes.js (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCourse progress endpoints are mounted twice, under two different domains, for the exact same handlers.
getCourseProgress/updateCourseProgressare wired at both/api/analytics/courses/:id/progressand/api/courses/:id/progress. This doubles the API surface to document/support for no behavioral benefit and invites drift if one copy is later updated (e.g., different middleware) without the other.
src/routes/analytics/analyticsRoutes.js#L18-19: drop these two routes — course progress belongs under the courses domain.src/routes/courses/courseRoutes.js#L56-66: keep this as the single source of truth for/:id/progress(GET/POST).🤖 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/analytics/analyticsRoutes.js` at line 1, Remove the duplicate course progress route registrations from the analytics routes, including the GET and POST handlers for getCourseProgress and updateCourseProgress. Keep the corresponding /:id/progress routes in courseRoutes.js as the sole source of truth.src/controllers/analytics/analyticsController.js (1)
36-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated aggregation logic across educator/platform reports.
The "totalVolume/transactionCount/uniqueBuyers" summary shape (lines 39-53 vs 99-117) and the "top items" pipeline (lines 57-60 vs 143-147) are copy-pasted with only minor field differences. Extracting a small helper (e.g.,
buildSummaryPipeline(match, extraGroupFields)andbuildTopItemsPipeline(match, limit)) would reduce the risk of the two copies drifting (e.g., if$toDecimalhandling needs to change later, it'd need updating in 4 places today).Also applies to: 95-149
🤖 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 36 - 61, Extract the duplicated aggregation stages in the analytics controller into shared helpers, such as buildSummaryPipeline(match, extraGroupFields) for the totalVolume/transactionCount/uniqueBuyers summary and buildTopItemsPipeline(match, limit) for the top-items results. Update both educator and platform report paths to use these helpers while preserving their existing field-specific grouping and output shapes.test/analytics.test.js (1)
10-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for several new endpoints.
This suite only exercises
/me/earningsand/platform. The following new routes have no automated coverage:/platform/timeseries,/top-educators,/top-items,GET/POST /courses/:id/progress(or/analytics/courses/:id/progress), and the two learning-dashboard routes. Given these are new, security-sensitive (auth/role-gated) endpoints, please add at least smoke tests asserting status codes and access control for each.As per path instructions for
src/**/*.js: "New or changed endpoints need Jest coverage."🤖 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/analytics.test.js` around lines 10 - 166, Extend the “Revenue analytics endpoints” Jest suite with smoke tests for /platform/timeseries, /top-educators, /top-items, both course-progress methods, and both learning-dashboard routes. For each endpoint, assert successful access for the appropriate authenticated role and rejection for unauthorized or disallowed roles, reusing the existing educatorToken, adminToken, studentToken, and test setup.Source: Path instructions
src/routes/courses/courseRoutes.js (1)
12-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWiring itself is correct; see consolidated comment for route duplication with
analyticsRoutes.js.Also applies to: 56-66
🤖 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/courses/courseRoutes.js` around lines 12 - 15, Remove the duplicated course-progress route wiring between courseRoutes.js and analyticsRoutes.js, including the handlers imported from getCourseProgress and updateCourseProgress. Keep these endpoints registered in only one canonical route module and preserve their existing behavior.
🤖 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 `@src/controllers/analytics/analyticsController.js`:
- Around line 1-6: Implement the missing buyer-spending analytics by adding a
getBuyerSpending or getMyPurchases controller that aggregates confirmed
Transaction records by buyer and sums amounts using $toDecimal, then expose it
through the corresponding analytics route. Follow the existing controller and
route patterns for authentication, response shape, and error handling.
- Around line 253-278: Remove the duplicate getUserLearning implementation and
its route usage, reusing userController.js#getLearningDashboard for the learning
dashboard endpoint instead. Ensure the consolidated handler retains the required
status filtering and response shape while using lean queries and selecting only
the course fields needed by the mapping.
- Around line 14-22: Update buildDateFilter to validate each provided from and
to value before assigning it to confirmedAt.$gte or confirmedAt.$lte. Reject
malformed dates through the controller’s established client-error path so
callers receive a clear 400 response, while preserving the existing filter
behavior for valid or omitted values across educator earnings and platform
analytics.
- Around line 172-251: Replace the findOne-then-conditional-create
initialization in both getCourseProgress and updateCourseProgress with an atomic
upsert using the existing { user, course } key. Preserve the current default
progress fields and ensure concurrent requests reuse the existing document
rather than returning a duplicate-key 500; retain the subsequent update and
response behavior.
- Around line 24-33: Update getEducatorEarnings to read currency and network
from req.query and apply both as filters in its match criteria, using the same
filter field names and handling conventions as getPlatformAnalytics. Preserve
the existing educator, confirmed-status, date-range, and limit behavior.
- Around line 88-93: Add compound indexes in the Transaction model to cover
analytics queries filtering by status, confirmedAt, and optionally
currency/network, while also covering the creator/status/confirmedAt pattern
used by the analytics match. Preserve all existing indexes and define the new
indexes alongside them in the model schema.
In `@src/routes/analytics/analyticsRoutes.js`:
- Line 20: The learning dashboard is duplicated across analytics and user
routes. Remove the analytics route using getUserLearning and its redundant
controller implementation, while keeping getLearningDashboard in
userController.js as the canonical implementation and preserving the sole
/me/learning endpoint in src/routes/userRoutes.js at line 129; no direct change
is needed there.
- Around line 13-17: Update the analytics route definitions using
getPlatformAnalytics so each endpoint selects a dedicated report/view for its
advertised data: platform, timeseries, top-educators, and top-items. Modify
getPlatformAnalytics to honor that selector and run/return only the
corresponding aggregation, while preserving protect and admin authorization.
In `@test/analytics.test.js`:
- Around line 42-44: Remove the hardcoded fallback from the token setup around
educatorToken, adminToken, and studentToken. Validate that
process.env.JWT_SECRET is present before signing tokens, failing fast with a
clear configuration error when it is missing, and pass the validated environment
value to jwt.sign for all test tokens.
---
Nitpick comments:
In `@src/controllers/analytics/analyticsController.js`:
- Around line 36-61: Extract the duplicated aggregation stages in the analytics
controller into shared helpers, such as buildSummaryPipeline(match,
extraGroupFields) for the totalVolume/transactionCount/uniqueBuyers summary and
buildTopItemsPipeline(match, limit) for the top-items results. Update both
educator and platform report paths to use these helpers while preserving their
existing field-specific grouping and output shapes.
In `@src/routes/analytics/analyticsRoutes.js`:
- Line 1: Remove the duplicate course progress route registrations from the
analytics routes, including the GET and POST handlers for getCourseProgress and
updateCourseProgress. Keep the corresponding /:id/progress routes in
courseRoutes.js as the sole source of truth.
In `@src/routes/courses/courseRoutes.js`:
- Around line 12-15: Remove the duplicated course-progress route wiring between
courseRoutes.js and analyticsRoutes.js, including the handlers imported from
getCourseProgress and updateCourseProgress. Keep these endpoints registered in
only one canonical route module and preserve their existing behavior.
In `@test/analytics.test.js`:
- Around line 10-166: Extend the “Revenue analytics endpoints” Jest suite with
smoke tests for /platform/timeseries, /top-educators, /top-items, both
course-progress methods, and both learning-dashboard routes. For each endpoint,
assert successful access for the appropriate authenticated role and rejection
for unauthorized or disallowed roles, reusing the existing educatorToken,
adminToken, studentToken, and test setup.
🪄 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: 99bb07bb-5546-4d56-a7b0-7fc2c34b390a
📒 Files selected for processing (10)
app.jssrc/controllers/analytics/analyticsController.jssrc/controllers/courses/courseController.jssrc/controllers/userController.jssrc/models/Course.jssrc/models/CourseProgress.jssrc/routes/analytics/analyticsRoutes.jssrc/routes/courses/courseRoutes.jssrc/routes/userRoutes.jstest/analytics.test.js
| import mongoose from "mongoose"; | ||
| import Transaction from "../../models/Transaction.js"; | ||
| import CourseProgress from "../../models/CourseProgress.js"; | ||
| import Course from "../../models/Course.js"; | ||
| import User from "../../models/User.js"; | ||
| import logger from "../../config/logger.js"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Missing required "buyer spending" analytics endpoint.
Issue #61 explicitly lists platform volume, time series, top educators, top items, per-educator earnings, and buyer spending as required analytics. This file implements all of those except buyer spending — there's no endpoint aggregating spend per buyer.
Would you like me to draft a getBuyerSpending/getMyPurchases aggregation (grouping confirmed Transactions by buyer, summing $toDecimal amounts) and the corresponding route?
🤖 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 1 - 6,
Implement the missing buyer-spending analytics by adding a getBuyerSpending or
getMyPurchases controller that aggregates confirmed Transaction records by buyer
and sums amounts using $toDecimal, then expose it through the corresponding
analytics route. Follow the existing controller and route patterns for
authentication, response shape, and error handling.
| const buildDateFilter = ({ from, to }) => { | ||
| const filter = {}; | ||
| if (from || to) { | ||
| filter.confirmedAt = {}; | ||
| if (from) filter.confirmedAt.$gte = new Date(from); | ||
| if (to) filter.confirmedAt.$lte = new Date(to); | ||
| } | ||
| return filter; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate from/to before building the date filter.
new Date(from)/new Date(to) on a malformed query string silently produces Invalid Date, which then flows into $gte/$lte on confirmedAt and can throw a cast error inside the aggregation (surfacing as an opaque 500) instead of a clear 400. This affects every caller of buildDateFilter (educator earnings and platform analytics).
🛡️ Proposed validation
const buildDateFilter = ({ from, to }) => {
const filter = {};
if (from || to) {
filter.confirmedAt = {};
- if (from) filter.confirmedAt.$gte = new Date(from);
- if (to) filter.confirmedAt.$lte = new Date(to);
+ if (from) {
+ const fromDate = new Date(from);
+ if (Number.isNaN(fromDate.getTime())) throw new Error("Invalid 'from' date");
+ filter.confirmedAt.$gte = fromDate;
+ }
+ if (to) {
+ const toDate = new Date(to);
+ if (Number.isNaN(toDate.getTime())) throw new Error("Invalid 'to' date");
+ filter.confirmedAt.$lte = toDate;
+ }
}
return filter;
};📝 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.
| const buildDateFilter = ({ from, to }) => { | |
| const filter = {}; | |
| if (from || to) { | |
| filter.confirmedAt = {}; | |
| if (from) filter.confirmedAt.$gte = new Date(from); | |
| if (to) filter.confirmedAt.$lte = new Date(to); | |
| } | |
| return filter; | |
| }; | |
| const buildDateFilter = ({ from, to }) => { | |
| const filter = {}; | |
| if (from || to) { | |
| filter.confirmedAt = {}; | |
| if (from) { | |
| const fromDate = new Date(from); | |
| if (Number.isNaN(fromDate.getTime())) throw new Error("Invalid 'from' date"); | |
| filter.confirmedAt.$gte = fromDate; | |
| } | |
| if (to) { | |
| const toDate = new Date(to); | |
| if (Number.isNaN(toDate.getTime())) throw new Error("Invalid 'to' date"); | |
| filter.confirmedAt.$lte = toDate; | |
| } | |
| } | |
| return filter; | |
| }; |
🤖 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 14 - 22,
Update buildDateFilter to validate each provided from and to value before
assigning it to confirmedAt.$gte or confirmedAt.$lte. Reject malformed dates
through the controller’s established client-error path so callers receive a
clear 400 response, while preserving the existing filter behavior for valid or
omitted values across educator earnings and platform analytics.
Source: Path instructions
| export const getEducatorEarnings = async (req, res) => { | ||
| try { | ||
| const educatorId = req.user._id; | ||
| const { from, to, limit = 10 } = req.query; | ||
|
|
||
| const match = { | ||
| creator: educatorId, | ||
| status: "confirmed", | ||
| ...buildDateFilter({ from, to }), | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Educator earnings ignores currency/network filters.
The PR objectives state shared filters across the analytics endpoints include date range, interval, currency, network, and limit — getPlatformAnalytics honors currency/network (lines 92-93) but getEducatorEarnings only supports from/to/limit. An educator can't scope their own earnings by network/currency the same way admins can for platform data.
🔧 Proposed fix
const educatorId = req.user._id;
- const { from, to, limit = 10 } = req.query;
+ const { from, to, limit = 10, currency, network } = req.query;
const match = {
creator: educatorId,
status: "confirmed",
...buildDateFilter({ from, to }),
};
+ if (currency) match.currency = currency;
+ if (network) match.network = network;📝 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 getEducatorEarnings = async (req, res) => { | |
| try { | |
| const educatorId = req.user._id; | |
| const { from, to, limit = 10 } = req.query; | |
| const match = { | |
| creator: educatorId, | |
| status: "confirmed", | |
| ...buildDateFilter({ from, to }), | |
| }; | |
| export const getEducatorEarnings = async (req, res) => { | |
| try { | |
| const educatorId = req.user._id; | |
| const { from, to, limit = 10, currency, network } = req.query; | |
| const match = { | |
| creator: educatorId, | |
| status: "confirmed", | |
| ...buildDateFilter({ from, to }), | |
| }; | |
| if (currency) match.currency = currency; | |
| if (network) match.network = network; |
🤖 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 24 - 33,
Update getEducatorEarnings to read currency and network from req.query and apply
both as filters in its match criteria, using the same filter field names and
handling conventions as getPlatformAnalytics. Preserve the existing educator,
confirmed-status, date-range, and limit behavior.
| const match = { | ||
| status: "confirmed", | ||
| ...buildDateFilter({ from, to }), | ||
| }; | ||
| if (currency) match.currency = currency; | ||
| if (network) match.network = network; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect Transaction model for indexes supporting analytics queries
fd -i transaction.js src/models
cat -n src/models/Transaction.js 2>/dev/null | sed -n '1,200p'
rg -n 'index\(|index:\s*true' -g '*Transaction.js'Repository: Deen-Bridge/dnb-backend
Length of output: 4951
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the analytics controller query patterns around the reported lines
cat -n src/controllers/analytics/analyticsController.js | sed -n '1,220p'
# Look for any additional Transaction indexes or analytics-specific query builders
rg -n "confirmedAt|buildDateFilter|status: \"confirmed\"|creator|currency|network|index\\(" src/controllers src/modelsRepository: Deen-Bridge/dnb-backend
Length of output: 20524
Add a compound index for the analytics match patterns src/models/Transaction.js only has indexes on buyer/status, creator/status, itemType/itemId, and type/status/createdAt; these analytics queries filter on creator/status/confirmedAt and status/confirmedAt (plus optional currency/network), so add an index that includes confirmedAt for these read paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/controllers/analytics/analyticsController.js` around lines 88 - 93, Add
compound indexes in the Transaction model to cover analytics queries filtering
by status, confirmedAt, and optionally currency/network, while also covering the
creator/status/confirmedAt pattern used by the analytics match. Preserve all
existing indexes and define the new indexes alongside them in the model schema.
| export const getCourseProgress = async (req, res) => { | ||
| try { | ||
| const courseId = req.params.id; | ||
| const userId = req.user._id; | ||
| const course = await Course.findById(courseId); | ||
| if (!course) return res.status(404).json({ success: false, message: "Course not found" }); | ||
|
|
||
| const isOwner = course.createdBy && course.createdBy.toString() === userId.toString(); | ||
| const isEnrolled = course.enrolledUsers?.some((entry) => entry.toString() === userId.toString()); | ||
| if (!isOwner && !isEnrolled) { | ||
| return res.status(403).json({ success: false, message: "You do not have access to this course" }); | ||
| } | ||
|
|
||
| let progress = await CourseProgress.findOne({ user: userId, course: courseId }); | ||
| if (!progress) { | ||
| const lessonCount = (course.sections || []).reduce((count, section) => count + (section.lessons || []).length, 0) || 1; | ||
| progress = await CourseProgress.create({ | ||
| user: userId, | ||
| course: courseId, | ||
| lessonsCompleted: [], | ||
| lastLesson: null, | ||
| lastPositionSeconds: 0, | ||
| percentComplete: 0, | ||
| }); | ||
| progress = progress.toObject(); | ||
| } | ||
|
|
||
| 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" }); | ||
| } | ||
| }; | ||
|
|
||
| export const updateCourseProgress = async (req, res) => { | ||
| try { | ||
| const courseId = req.params.id; | ||
| const userId = req.user._id; | ||
| const { lessonId, lastPositionSeconds } = req.body; | ||
| const course = await Course.findById(courseId); | ||
| if (!course) return res.status(404).json({ success: false, message: "Course not found" }); | ||
|
|
||
| const isOwner = course.createdBy && course.createdBy.toString() === userId.toString(); | ||
| const isEnrolled = course.enrolledUsers?.some((entry) => entry.toString() === userId.toString()); | ||
| if (!isOwner && !isEnrolled) { | ||
| return res.status(403).json({ success: false, message: "You do not have access to this course" }); | ||
| } | ||
|
|
||
| const lessonCount = (course.sections || []).reduce((count, section) => count + (section.lessons || []).length, 0) || 1; | ||
|
|
||
| let progress = await CourseProgress.findOne({ user: userId, course: courseId }); | ||
| if (!progress) { | ||
| progress = await CourseProgress.create({ | ||
| user: userId, | ||
| course: courseId, | ||
| lessonsCompleted: [], | ||
| lastLesson: null, | ||
| lastPositionSeconds: 0, | ||
| percentComplete: 0, | ||
| }); | ||
| } | ||
|
|
||
| if (lessonId) { | ||
| const lessonObjectId = new mongoose.Types.ObjectId(lessonId); | ||
| progress.lessonsCompleted = Array.from(new Set([...(progress.lessonsCompleted || []), lessonObjectId])); | ||
| } | ||
|
|
||
| progress.lastLesson = lessonId ? new mongoose.Types.ObjectId(lessonId) : progress.lastLesson; | ||
| progress.lastPositionSeconds = lastPositionSeconds ?? progress.lastPositionSeconds; | ||
| const percent = Math.min(100, Math.round((progress.lessonsCompleted.length / lessonCount) * 100)); | ||
| progress.percentComplete = percent; | ||
| if (percent >= 100) progress.completedAt = progress.completedAt || new Date(); | ||
| await progress.save(); | ||
|
|
||
| res.status(200).json({ success: true, progress }); | ||
| } catch (error) { | ||
| logger.error("Update course progress error:", error); | ||
| res.status(500).json({ success: false, message: "Failed to update course progress" }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Race condition creating CourseProgress under concurrent requests.
Both getCourseProgress and updateCourseProgress do a findOne followed by a conditional create (lines 185-197, 222-232). If two requests for the same (user, course) pair race (e.g., a user opening the same course in two tabs), both can pass the !progress check before either insert completes; the second create will throw a duplicate-key error against the unique {user,course} index and bubble up as a generic 500 instead of succeeding.
Use an atomic upsert to remove the TOCTOU window:
🔒 Proposed fix (apply to both functions)
- let progress = await CourseProgress.findOne({ user: userId, course: courseId });
- if (!progress) {
- progress = await CourseProgress.create({
- user: userId,
- course: courseId,
- lessonsCompleted: [],
- lastLesson: null,
- lastPositionSeconds: 0,
- percentComplete: 0,
- });
- }
+ const progress = await CourseProgress.findOneAndUpdate(
+ { user: userId, course: courseId },
+ { $setOnInsert: { lessonsCompleted: [], lastLesson: null, lastPositionSeconds: 0, percentComplete: 0 } },
+ { upsert: true, new: true }
+ );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/controllers/analytics/analyticsController.js` around lines 172 - 251,
Replace the findOne-then-conditional-create initialization in both
getCourseProgress and updateCourseProgress with an atomic upsert using the
existing { user, course } key. Preserve the current default progress fields and
ensure concurrent requests reuse the existing document rather than returning a
duplicate-key 500; retain the subsequent update and response behavior.
| export const getUserLearning = async (req, res) => { | ||
| try { | ||
| const userId = req.user._id; | ||
| const { status = "in-progress" } = req.query; | ||
| const progressRecords = await CourseProgress.find({ user: userId }).sort({ updatedAt: -1 }).populate("course"); | ||
| const courses = progressRecords | ||
| .map((record) => ({ | ||
| _id: record.course?._id, | ||
| title: record.course?.title || "Course", | ||
| percentComplete: record.percentComplete || 0, | ||
| lastLesson: record.lastLesson, | ||
| lastPositionSeconds: record.lastPositionSeconds || 0, | ||
| completedAt: record.completedAt, | ||
| updatedAt: record.updatedAt, | ||
| })) | ||
| .filter((entry) => { | ||
| if (status === "completed") return entry.percentComplete >= 100; | ||
| return entry.percentComplete < 100; | ||
| }); | ||
|
|
||
| res.status(200).json({ success: true, courses }); | ||
| } catch (error) { | ||
| logger.error("Get user learning error:", error); | ||
| res.status(500).json({ success: false, message: "Failed to fetch learning dashboard" }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Duplicates userController.js#getLearningDashboard, and fetches more data than needed.
This handler and getLearningDashboard in src/controllers/userController.js do essentially the same query/filter/map over CourseProgress, exposed on two different routes (/api/analytics/users/me/learning vs /api/users/me/learning). Unlike the userController.js version, this one doesn't use .lean() or restrict populate("course") to specific fields, so it fetches the entire Course document (description, price, reviews, enrolledUsers, etc.) per progress record just to discard most of it in the .map(). See the consolidated comment for the full cross-file picture.
🤖 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 253 - 278,
Remove the duplicate getUserLearning implementation and its route usage, reusing
userController.js#getLearningDashboard for the learning dashboard endpoint
instead. Ensure the consolidated handler retains the required status filtering
and response shape while using lean queries and selecting only the course fields
needed by the mapping.
| router.get("/me/earnings", protect, getEducatorEarnings); | ||
| router.get("/platform", protect, authorizeRoles("admin"), getPlatformAnalytics); | ||
| router.get("/platform/timeseries", protect, authorizeRoles("admin"), getPlatformAnalytics); | ||
| router.get("/top-educators", protect, authorizeRoles("admin"), getPlatformAnalytics); | ||
| router.get("/top-items", protect, authorizeRoles("admin"), getPlatformAnalytics); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== analyticsRoutes.js outline =="
ast-grep outline src/routes/analytics/analyticsRoutes.js --view expanded || true
echo
echo "== analytics controller outline =="
fd -a 'analytics' src | head -20
fd -a 'analyticsController' src | head -20
fd -a 'analytics*.js' src | head -20Repository: Deen-Bridge/dnb-backend
Length of output: 582
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== analyticsRoutes.js (relevant lines) =="
cat -n src/routes/analytics/analyticsRoutes.js | sed -n '1,120p'
echo
echo "== controller files mentioning getPlatformAnalytics =="
rg -n "getPlatformAnalytics|platform/timeseries|top-educators|top-items|platform" src/routes src/controllers src -g '*.js' -g '*.mjs' || trueRepository: Deen-Bridge/dnb-backend
Length of output: 11047
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the relevant files in small slices.
echo "== src/routes/analytics/analyticsRoutes.js =="
cat -n src/routes/analytics/analyticsRoutes.js | sed -n '1,120p'
echo
echo "== files that define getPlatformAnalytics or related routes =="
rg -n "getPlatformAnalytics|/platform/timeseries|/top-educators|/top-items|/me/earnings" src -g '*.js' || trueRepository: Deen-Bridge/dnb-backend
Length of output: 2158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print only the route definitions around the analytics endpoints.
sed -n '1,80p' src/routes/analytics/analyticsRoutes.js
echo
echo "---"
# Print the controller implementation if present.
fd -a 'analyticsController.js' src || trueRepository: Deen-Bridge/dnb-backend
Length of output: 1177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('src/routes/analytics/analyticsRoutes.js')
print(p.read_text())
PYRepository: Deen-Bridge/dnb-backend
Length of output: 1105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== controller file size =="
wc -l src/controllers/analytics/analyticsController.js
echo
echo "== controller outline =="
ast-grep outline src/controllers/analytics/analyticsController.js --view expanded || true
echo
echo "== getPlatformAnalytics implementation =="
rg -n -A120 -B20 "export const getPlatformAnalytics" src/controllers/analytics/analyticsController.jsRepository: Deen-Bridge/dnb-backend
Length of output: 6668
Split the analytics routes into dedicated handlers
These four routes all invoke getPlatformAnalytics, so every request runs the full set of aggregations and returns the same combined response. Split them into route-specific handlers, or pass a view/report param, so each endpoint computes and returns only what it advertises.
🤖 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/analytics/analyticsRoutes.js` around lines 13 - 17, Update the
analytics route definitions using getPlatformAnalytics so each endpoint selects
a dedicated report/view for its advertised data: platform, timeseries,
top-educators, and top-items. Modify getPlatformAnalytics to honor that selector
and run/return only the corresponding aggregation, while preserving protect and
admin authorization.
| router.get("/top-items", protect, authorizeRoles("admin"), getPlatformAnalytics); | ||
| router.get("/courses/:id/progress", protect, getCourseProgress); | ||
| router.post("/courses/:id/progress", protect, updateCourseProgress); | ||
| router.get("/users/me/learning", protect, getUserLearning); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Two independent "learning dashboard" implementations, exposed on two different routes. getUserLearning and getLearningDashboard both query CourseProgress by user, sort by updatedAt, filter by percentComplete vs status, and map to a similar courses payload — but they aren't the same code, and only one of them (userController.js) uses .lean() with a projected populate(). Having both live on is a maintenance trap: a future bug fix (e.g., status filter edge case) applied to one will silently miss the other, and clients now have two URLs returning near-identical data.
src/controllers/analytics/analyticsController.js#L253-278: removegetUserLearning(or make it a thin re-export/delegate) and drop the redundant/users/me/learningroute in favor of theuserController.jsimplementation.src/controllers/userController.js#L534-566: keep this as the single canonical implementation (already uses.lean()+ field-projectedpopulate()).src/routes/analytics/analyticsRoutes.js#L20: remove this route, or have it simply forward togetLearningDashboard.src/routes/userRoutes.js#L129: keep as the sole/me/learningendpoint.
📍 Affects 2 files
src/routes/analytics/analyticsRoutes.js#L20-L20(this comment)src/routes/userRoutes.js#L129-L129
🤖 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/analytics/analyticsRoutes.js` at line 20, The learning dashboard
is duplicated across analytics and user routes. Remove the analytics route using
getUserLearning and its redundant controller implementation, while keeping
getLearningDashboard in userController.js as the canonical implementation and
preserving the sole /me/learning endpoint in src/routes/userRoutes.js at line
129; no direct change is needed there.
| educatorToken = jwt.sign({ userId: educator._id, sessionId: "s1" }, process.env.JWT_SECRET || "deenbridge-temp-secret-key-2024"); | ||
| adminToken = jwt.sign({ userId: admin._id, sessionId: "s2" }, process.env.JWT_SECRET || "deenbridge-temp-secret-key-2024"); | ||
| studentToken = jwt.sign({ userId: student._id, sessionId: "s3" }, process.env.JWT_SECRET || "deenbridge-temp-secret-key-2024"); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Hardcoded JWT secret fallback in test source.
.env.example documents that JWT_SECRET should be a strong, dedicated env var (min 32 chars) for tests. Hardcoding the literal secret as a fallback here means the test suite can silently sign valid tokens even if JWT_SECRET is missing/misconfigured in the test environment, masking a real config problem and duplicating a secret value in source control.
🔧 Suggested fix
- educatorToken = jwt.sign({ userId: educator._id, sessionId: "s1" }, process.env.JWT_SECRET || "deenbridge-temp-secret-key-2024");
+ educatorToken = jwt.sign({ userId: educator._id, sessionId: "s1" }, process.env.JWT_SECRET);Fail fast (e.g., assert process.env.JWT_SECRET in a setup step) instead of silently falling back.
Also applies to: 73-75
🤖 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/analytics.test.js` around lines 42 - 44, Remove the hardcoded fallback
from the token setup around educatorToken, adminToken, and studentToken.
Validate that process.env.JWT_SECRET is present before signing tokens, failing
fast with a clear configuration error when it is missing, and pass the validated
environment value to jwt.sign for all test tokens.
Source: Path instructions
|
@Myart352 fix conflicts please |
2 participants
closes #61
This PR adds backend analytics support for educator earnings and platform-level reporting so the application can expose revenue insights for courses and creators.
What this change adds:
Files changed:
What we changed:
Summary by CodeRabbit