fix(reviews): put reviews where they can be read and written - #140
Conversation
|
@MOHITKOURAV01 Please resolve the merge conflicts in The implementation looks good and the checks are passing. Once the conflicts are cleared and the branch is up to date with |
The reviews feature is complete on the server and was all but unreachable in the browser. <CourseReviews> was rendered in exactly one place in the entire frontend — inside the certificate modal on the course player, which only opens once every section is complete. So nobody could read a review. Every catalogue card renders a rating badge showing a star average and a count, and it was an inert div: no link, no button, no expanded form, and no course detail page to navigate to. A prospective student saw "4.6 (23)" with no route to any of the 23. The public GET /api/reviews/:courseId had no caller in the frontend at all. And most enrolled students could not write one. The server asks for enrolment and nothing else; the UI asked for 100% completion. A student nine sections into ten was authorised by the API and had no button, so reviews were collected only from the people who finish courses — which biases every average on the catalogue upward, invisibly, because the count looks like it covers all enrolments. That gate was never a predicate to fix — reviewEligibility.js already states the server's rule, and asks for enrolment and non-authorship, not progress. The completion check was the mount point. So CourseReviews moves out of the certificate modal and onto the player page, below the sections and the video, where an enrolled student reaches it at any progress. The modal keeps its congratulation and its download. CourseRatingBadge takes an optional onOpen and renders as a button when it has one, opening the course's reviews from the catalogue; without it, it is the same div as before, so the single-course usages are unaffected. Its accessible name is the whole sentence, because three separate nodes are not one and a control should say what it does. Nothing is fetched until the badge is activated, so the catalogue's critical path is unchanged. Reading works signed out. CourseReviews already had the message for that case; it had never been rendered anywhere a signed-out visitor could reach it.
708477f to
491993b
Compare
|
@udaycodespace rebased on latest main — conflicts resolved, but this one changed shape, so it is worth a second look. #122 landed The What is left is placement only: Frontend 330 pass. |
Really good PR, @MOHITKOURAV01 ! Unfortunately, it won’t get scored since the program deadline has already passed. Still, really appreciate the effort you put in. ❤️ All the best for your upcoming OSS programs! |
Closes #136.
The defect
The reviews feature added in #18 is complete on the server and was all but
unreachable in the browser.
GET /api/reviews/:courseIdis deliberately public — noauthMiddlewareonthat line — and
createReviewgates on enrolment and nothing else:<CourseReviews>, the component that consumes all of it, was rendered inexactly one place in the entire frontend — inside
<Modal.Body>of thecertificate modal in
CourseContent.jsx. That modal opens from a buttonthat only renders once every section is complete.
Nobody could read a review. Every catalogue card renders a
<CourseRatingBadge>showing a star average and a count. It was an inert<div>: not a link, not a button, with no expanded form and no course detailpage to navigate to. A prospective student saw "4.6 (23)" and had no route to
any of the 23, on the one screen where reviews exist to inform a decision. The
public
listReviewsendpoint had no caller in the frontend at all.Most enrolled students could not write one. The server's rule is enrolment;
the UI's rule was 100% completion. A student nine sections into a ten-section
course was authorised by the API and had no button. Reviews were therefore
collected only from the people who finish courses, which biases every average
on the catalogue upward — invisibly, because the count looks like it covers all
enrolments.
What is here
The rule, stated as a rule.
frontend/src/lib/reviewAccess.jsholds thefour states a viewer can be in, replacing three nested ternaries in the
component.
The important property is a negative one:
describeReviewAccesstakes noprogress argument. Completion is not part of the server's rule and must not
become part of this one — that divergence is the defect, and a function with no
input for it cannot drift back to one.
One case worth flagging for review:
/minereportscanReviewas "may createa new one", which goes false the moment a review exists. Reading it as "may
not submit" would make a review uneditable as soon as it was written, so an
existing review implies the enrolment that allowed it and keeps its edit path.
Reviews on the course player.
<CourseReviews>moves out of thecertificate modal and onto the page, below the sections and the video, where
any enrolled student reaches it at any progress. The certificate modal keeps
its congratulation and its download — it is a reasonable prompt, just not the
only door.
Reviews from the catalogue.
<CourseRatingBadge>takes an optionalonOpen. With it the badge renders as a<button>that opens the course'sreviews; without it it is the same inert div as before, so the single-course
usages are unaffected.
AllCoursesrenders<CourseReviews>in a scrollablemodal beside the payment one.
The accessible name is the whole sentence — "Rated 4.6 out of 5 from 23 reviews
for Intro to Testing. Open reviews" — because the stars, the average and the
count render as three separate nodes, which is not a sentence, and a control
should say what activating it does.
Nothing is fetched until the badge is activated, so the catalogue's critical
path is unchanged: the summaries are still batched by
useRatingSummariesinthe one request that already existed. This does not reintroduce the N+1 that
#86 removed.
Reading works signed out.
CourseReviewsalready had the message for that case— it had simply never been rendered anywhere a signed-out visitor could reach.
Tests
frontend/src/lib/reviewAccess.test.js— 12 tests. The regression is "anenrolled student may write, at any progress". Alongside it: a signed-out
visitor may read but not write; a signed-in non-enrolled student is told to
enrol; an existing review stays editable once
canReviewgoes false; a reviewheld by a signed-out viewer is still not writable; no viewer at all does not
throw; every state names itself and exactly the blocked ones carry a message;
and the badge label reads as a sentence, singularises one review, says "no
reviews yet" rather than "0 reviews", and produces no
NaNfrom a missingsummary.
docs/issue-136-reviews-reachable.mdhas the write-up.Checklist
cd frontend && npm test— 226 pass (214 onmain, 12 added)cd frontend && npm run buildcd backend && npm test— 505 pass, untouched by this changenpm run lint— does not pass onmain(69 problems) and does not here.AllCourses.jsxreports the same 5 before and after,CourseContent.jsxthe same 3,
CourseReviews.jsxthe same 2 — all pre-existingno-unused-varsandreact/prop-types.lib/reviewAccess.jsandCourseRatingBadge.jsxlint clean.Notes
No API change: every endpoint used here already existed and is untouched. There
is still no course detail page — the catalogue modal is the reading surface,
which suits a card grid but is not a shareable URL for a course and its
reviews; a
/course/:courseIdroute is a larger change and separate work. Theupward bias in the averages collected while this was broken is in the data
already, and nothing here rewrites it.