fix(reviews): stop an educator reviewing their own course - #122
Merged
udaycodespace merged 1 commit intoAug 30, 2026
Merged
Conversation
createReview checked one thing — is the reviewer enrolled — and nothing stops an author enrolling in their own course. Two requests put a five-star review on your own listing, badged "Verified enrollment" and averaged into the rating CourseRatingBadge renders on every catalogue card. A course with one self-review outranked a course with three genuine four-star ones. The badge made it worse: serializeReview stamped verifiedEnrollment as the literal true on every row, and the review card rendered it without reading the field. utils/courseAuthorship owns the comparison, because courseModel.userId is a String while courseReview.userId is an ObjectId and comparing them without coercing both sides is false, always, and silently. createReview refuses the author before the enrolment check — the enrolment is not the thing that is wrong, so "you are not enrolled" would be the wrong answer. getMyReview returns a reason so the client stops rendering a form that answers 403. updateReview reads before writing, because a row written before this guard is still owned by its author and findOneAndUpdate would have let them edit it back into existence. verifiedEnrollment is computed now. Deleting stays open in every case — that is the direction that fixes the problem. Guarding the write leaves the existing rows in the average, so scripts/removeSelfReviews.js clears them, following db:dedupe-emails: one $lookup on the indexed courseId with a $toString on the review's id, --dry-run first. On the client, lib/reviewEligibility replaces one sentence for every denial. An author was being told to enrol, which leads nowhere. Three things deliberately left alone, with reasons in docs/: the enrolment itself, since previewing a course requires one; the `enrolled` counter, which has an older drift problem of its own; and excluding legacy self-reviews at read time, which would put a $lookup in the pipeline udaycodespace#86 built to be one indexed pass.
udaycodespace
self-requested a review
August 28, 2026 09:31
Owner
|
Reviewed this, @MOHITKOURAV01 . The self-review case is handled cleanly at both the backend and frontend levels, with the author check happening before enrollment validation and existing self-reviews being addressed through the cleanup script. The All backend and frontend tests are passing, and the build is clean. Approved and merging. |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #117.
The defect
createReviewgated on one thing — is the reviewer enrolled — and nothing stopsan author enrolling in their own course. Two requests:
The badge made it worse.
serializeReviewstampedverifiedEnrollment: trueonevery row, and the review card rendered
✓ Verified enrollmentwithout readingthe field at all.
It flows into everything built on the summary:
CourseRatingBadgeon thecatalogue card,
buildSummaryPipeline's average, and the rating a shopper readsbefore enrolling. A course with one self-review outranked a course with three
genuine four-star reviews.
What is here
utils/courseAuthorship.jsowns the comparison, becausecourseModel.userIdisa
StringwhilecourseReview.userIdis anObjectId, and comparing themwithout coercing both sides is false — always, and silently.
courseDeletionControlleralready does this by hand andcascadeDeletecarriesa comment about the same trap.
createReviewrefuses the author before the enrolment check. Theordering is the point: the enrolment is not the thing that is wrong, so
"you are not enrolled" would be the wrong answer to give them.
getMyReviewreturnsreason,isAuthorandisEnrolled, so the clientstops rendering a form that answers 403.
updateReviewreads before writing. A row written before this guard isstill owned by its author, and
findOneAndUpdate({ _id, userId })would havelet them edit it — reintroducing by the back door what the create path
refuses.
serializeReviewcomputesverifiedEnrollmentinstead of asserting it,and
CourseReviews.jsxreads it.problem.
Guarding the write leaves existing rows in the average, so
scripts/removeSelfReviews.jsclears them, following thedb:dedupe-emailspattern already in the repo:
On the client,
lib/reviewEligibility.jsreplaces one sentence for everydenial. An author was being told "Enroll in this course before submitting a
verified review" — advice that leads nowhere, since they can enrol and the
review is still refused.
Three things deliberately left alone
The enrolment. An author can still enrol in their own course, and should:
sendCourseContentControllerwill not serve the sections without one, sorefusing it would take away the ability to check the videos play. Blocking the
review protects the rating; blocking the enrolment costs something real and
protects nothing extra.
The
enrolledcounter. An author who enrols still counts as one of their ownlearners. That counter has a separate, older drift problem, and fixing one course
of it here would change a number admins have been reading, for an unrelated
reason.
Excluding legacy self-reviews at read time. It would put a
$lookupinbuildSummaryPipeline— the pipeline #86 built specifically so a twelve-cardpage costs one indexed pass. The script does it once instead of on every render.
Verifying
The original repro, re-run:
Checklist
cd backend && npm test— 433 pass (413 onmain, 20 added)cd frontend && npm test— 180 pass (169 onmain, 11 added)cd frontend && npm run buildnpm run lint— does not pass onmainand does not here.CourseReviews.jsxreports the same 3 problems before and after.lib/reviewEligibility.jslints clean.Write-up:
docs/issue-117-self-review.md.