Skip to content

fix(courses): let every section be completed and date the certificate properly - #98

Open
MOHITKOURAV01 wants to merge 4 commits into
udaycodespace:mainfrom
MOHITKOURAV01:fix/93-course-completion
Open

fix(courses): let every section be completed and date the certificate properly#98
MOHITKOURAV01 wants to merge 4 commits into
udaycodespace:mainfrom
MOHITKOURAV01:fix/93-course-completion

Conversation

@MOHITKOURAV01

@MOHITKOURAV01 MOHITKOURAV01 commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Summary

The course player hid the Completed button inside {section.S_content && ...}, decided completion in the browser from an array that is known to contain duplicates and stale ids, and dated the certificate from the enrolment's last-write timestamp. This makes the server the authority on all three.

Related Issue

Closes #93

What changed?

backend/utils/courseProgress.js (new) — the progress rule in one place. countCompletedSections and buildProgressSummary moved here out of enrolledCoursesController, which imports and re-exports them, so the player and My Courses cannot report different numbers for the same enrolment. Adds completedSectionIds, isEnrollmentComplete and describeSections.

describeSections reads sections through normalizeSections, so an object-shaped field is described in order, and resolves completed against both addressing schemes — an index and a section _id — because normalizeSectionId accepts either and older rows hold whichever the client sent. hasVideo and completed are deliberately independent fields; nothing downstream may derive the second from the first.

backend/controllers/courseContentController.js (new) — replaces sendCourseContentController in the userControllers aggregator:

  • the enrolment is resolved from req.user, not req.body.userId — the same change [Security]: A teacher can publish a course under someone else's account — /addcourse takes userId from the upload form #83 made to /addcourse;
  • :courseid is validated before it reaches Mongoose, so a malformed id is a 400 rather than a CastError surfacing as a 500;
  • a caller who is not enrolled gets 403 "You are not enrolled in this course" instead of 404 "User not found";
  • the response carries progress, isComplete, certificateDate, courseTitle and courseEducator;
  • certficateData — the entire enrolment document, sent for the sake of one field — is gone;
  • courseContent and completeModule keep their original names, so anything still reading them keeps working.

backend/controllers/progressController.jscompletemodule returns the recomputed progress, isComplete and certificateDate, so the common case needs no follow-up request, and stamps the certificate when the last section lands:

await EnrolledCourseModel.updateOne(
  { _id: enrollment._id,
    $or: [{ certificateDate: { $exists: false } }, { certificateDate: null }] },
  { $set: { certificateDate } },
);

Guarded on the field still being unset, so two requests completing the last section at once cannot overwrite each other's date. The summary is projected from the array that was read plus the id $addToSet just wrote, so there is no second read.

frontend/src/lib/courseProgress.js (new) — normalises the response. Recomputes nothing the server sent: readIsComplete trusts isComplete and falls back to comparing the summary, never to a bare array length. readVideoPath handles /uploads/x, uploads/x, x, \x and absolute URLs, which the component used to do inline inside a JSX onClick.

frontend/src/components/user/student/CourseContent.jsxMark complete is outside the hasVideo guard; a progress bar and n of m sections complete at the top; the certificate is gated on the server's isComplete and dated from certificateDate, with the date line omitted rather than rendering Invalid Date; loading and error states with a retry; alert() replaced with the Toast component from #36, reflecting the alreadyCompleted flag.

Type

  • Bug fix
  • New feature
  • Refactor
  • Docs only
  • Tests
  • Config / workflow
  • Security
  • Breaking change

Areas touched

  • Frontend
  • Backend
  • Database
  • Docs
  • Workflow / GitHub Actions
  • Config / environment

certificateDate was already declared on enrolledCourseModel. This is the first code that writes it; no migration is needed, and enrolments completed before this ship keep certificateDate: null and simply show no date line.

Testing

  • Tested locally
  • Build passes
  • Lint passes
  • Tests added or updated
  • Docs only, no runtime testing needed

npm test in backend/254 passing, up from 234 on main.
npm test in frontend/89 passing, up from 73.
npm run build in frontend/ — clean.

npm run lint has never passed on main: it reports 69 problems there, almost all pre-existing no-unused-vars on React imports and missing react/prop-types. npx eslint is clean on every file this PR adds or rewrites.

Test steps

  1. Seed and enrol a student in Introduction to HTML and CSS.
  2. db.courses.updateOne({C_title: "Introduction to HTML and CSS"}, {$unset: {"sections.1.S_content": ""}}).
  3. Open the course. Section 2 reads This section has no video and still offers Mark complete — on main it renders no controls at all and the certificate is unreachable from here.
  4. Complete both sections. The bar reaches 100% and the certificate button appears.
  5. db.enrolledcourses.findOne(...)certificateDate is set. Complete a section again: the toast says it was already complete and the date does not move.
  6. Open a course you are not enrolled in → 403 "You are not enrolled in this course", where main says 404 "User not found".

Screenshots

  • Not needed
  • Added below

Edge cases checked

  • Empty or missing data
  • Loading / slow response
  • API failure / server error
  • Rate limit / throttling
  • Invalid or unexpected input
  • Permission / access denied
  • Partial or inconsistent data
  • Mobile / small screen behavior
  • Other

Other edge case details

  • A course with no sections is never complete — isEnrollmentComplete requires total > 0, so an enrolment created against an empty course does not read as finished on first render.
  • Progress rows for sections the course no longer has are counted as distinct ids but capped at total, so they cannot unlock a certificate.
  • Two concurrent completions of the last section: the second updateOne matches nothing and the first stamp stands.
  • sections stored as an object map is described in order rather than dropped.
  • A section completed by _id before the index scheme is still shown as complete.

Checklist

  • Read CONTRIBUTING.md
  • Linked the issue
  • Assigned before starting or approved by maintainer
  • Changes are focused on one issue
  • No debug logs or unused code
  • Documentation updated if needed
  • No new warnings or console errors
  • Changes are meaningful, not trivial

Notes

docs/issue-93-course-completion.md has the full write-up, including why the old completedModule.length === courseContent.length was wrong in both directions.

The clock in progressController is injectable (setClock) purely so a test can assert the stamped date without racing it; production behaviour is unchanged.


Update: the stylesheet moved out of App.css

I originally appended this feature's rules to the end of frontend/src/App.css. Four of the branches I have in flight were each doing that, which conflicts on every pair of them — nine conflicting pairs, none of them about anything real.

They now live in frontend/src/styles/course-player.css, imported by the component that needs them. The shared classes this feature reuses (catalog-search, catalog-filter, the design tokens) stay in App.css; only what is new to #93 moved.

That was the right call independently of git — a rule for one screen belongs next to that screen, not at the bottom of a growing global sheet — but the concrete effect is that all five of my open feature PRs now merge into main and into each other with no conflicts at all. I verified it by merging all five together and running both suites on the combined tree: 317 backend tests and 127 frontend tests pass, and the frontend build is clean.

… properly

The course player hid the Completed button inside `{section.S_content && ...}`,
so a section with no video had no control to complete it. course_Length counts
every section, so one such section made the total unreachable and the
certificate never appeared.

The certificate gate was `completedModule.length === courseContent.length`,
computed in the browser over the enrolment's raw progress array. That array can
hold the same sectionId twice — rows written before udaycodespace#39 had no uniqueness
guard, which is why countCompletedSections exists — and ids for sections the
course no longer has. Both inflate the length, so the comparison could unlock a
certificate early as well as withhold one that was earned.

And the certificate was dated `certficateData.updatedAt`, Mongoose's last-write
timestamp, which moves on every progress save. `certificateDate` has been on
enrolledCourseModel since it was written and nothing ever set it.

utils/courseProgress.js holds the rule now: countCompletedSections and
buildProgressSummary move there out of enrolledCoursesController, which imports
them back, so the player and My Courses cannot disagree about one enrolment.
describeSections keeps hasVideo and completed independent, and resolves
completion against both an index and a section _id.

courseContentController replaces sendCourseContentController: the enrolment is
resolved from req.user rather than req.body.userId, an unenrolled caller gets
403 instead of 404 "User not found", a malformed id is a 400 rather than a
CastError, and the whole enrolment document is no longer shipped as
certficateData. completemodule returns the recomputed summary and stamps
certificateDate once, under a filter guarded on the field still being unset.

The player renders a progress bar, offers Mark complete on every section,
gates the certificate on the server's isComplete, omits the date line when
there is none, handles a failed load, and uses the Toast from udaycodespace#36 instead of
alert().

Closes udaycodespace#93
App.css was gaining an appended block per feature branch, which conflicts on
every pair of them and says nothing about where a rule belongs. The player's
rules live next to the player instead, imported by the component that needs
them.
The aggregator's module.exports block is a single region that several branches
touch at once, and renaming a key inside it collides with any neighbouring
line. Binding the old name to the new controller next to the require says the
same thing and leaves the shared block alone.
@MOHITKOURAV01

Copy link
Copy Markdown
Contributor Author

Overlap with #81, worth knowing about

Both merge cleanly into main, but they conflict with each other on backend/controllers/userControllers.js, backend/routers/userRoutes.js and frontend/src/components/user/student/CourseContent.jsx — and the last one is substantive:

  • fix(security): stop serving course videos to anyone who asks #81 stops /uploads being world-readable and moves section videos behind an authenticated, enrolment-checked route, changing how CourseContent.jsx resolves a video URL.
  • this PR rewrites CourseContent.jsx so a section without a video can still be completed, and replaces sendCourseContentController with courseContentController.

They pull in the same direction and the resolution is mechanical, but it is a real merge rather than a union of unrelated lines. The readVideoPath helper this PR adds in frontend/src/lib/courseProgress.js is the natural place for #81's change to land — it is the single point where a stored S_content becomes a URL, which is exactly what #81 needs to redirect.

Whichever you take second, tell me and I will push the resolution.

@udaycodespace
udaycodespace self-requested a review August 24, 2026 15:50
@udaycodespace udaycodespace added ECSoC26 Required label for a PR to be eligible for Sentinel scoring in review PR is up and waiting on maintainer review and removed frontend documentation backend fullstack tests labels Aug 24, 2026
Both conflicts are the same shape: an import block where this branch and
main each added a different neighbour.

userRoutes and userControllers each gained getCourseContentController
here and the resend-OTP wiring on main (udaycodespace#73). Nothing competes — both
sides are kept.

No behavioural change from the merge. Backend 433 pass, frontend 184,
build clean.
@udaycodespace udaycodespace added redo Reviewed — needs changes before it can be merged and removed in review PR is up and waiting on maintainer review labels Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ECSoC26 Required label for a PR to be eligible for Sentinel scoring redo Reviewed — needs changes before it can be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: A section without a video can never be completed, so the certificate is unreachable and its date is the last write to the enrolment

2 participants