Feature: Implement course rating system#87
Feature: Implement course rating system#87charlottenguyen05 wants to merge 4 commits intotarinagarwal:mainfrom
Conversation
|
@charlottenguyen05 ill review it asap |
|
@tarinagarwal Can you check my code please. |
There was a problem hiding this comment.
Changes Requested 🐈
This PR introduces a course rating system with new UI components and backend API endpoints. While the implementation is functional and well-structured, there are significant security concerns regarding authorization checks, missing documentation on key functions, and opportunities for performance and validation improvements.
There are a few things I'd like to see addressed before we merge this:
Before merging
- Implement authorization checks on the server to ensure only permitted users can rate courses.
- Add comprehensive JSDoc comments to all new exported functions and components for maintainability.
- Improve input validation and optimize rating data fetching with caching or aggregation to enhance security and performance.
Findings breakdown (38 total)
5 high / 10 medium / 12 low / 11 info
Confidence: 90%
🔗 View Full Review Report — detailed findings, severity breakdown, and agent analysis
Reviewed by Looks Good To Meow — AI-powered code review
| } | ||
| }); | ||
|
|
||
| router.post("/:id/rate", authenticateToken, async (req, res) => { |
There was a problem hiding this comment.
Add authorization checks to ensure the user is allowed to rate the course, such as verifying enrollment or course visibility before allowing rating submission.
security
| navigate(`/courses/${course.id}/test/${testId}/results`); | ||
| }; | ||
|
|
||
| const handleRatingSubmit = async (rating: number) => { |
There was a problem hiding this comment.
Add JSDoc comment for 'handleRatingSubmit' describing its purpose, parameters (rating: number), return type (Promise), and possible exceptions.
documentation
| } | ||
| }; | ||
|
|
||
| const fetchUserRating = async () => { |
There was a problem hiding this comment.
Add JSDoc comment for 'fetchUserRating' describing its purpose, parameters (none), return type (Promise), and possible exceptions.
documentation
| return response.data; | ||
| }; | ||
|
|
||
| export const rateCourse = async ( |
There was a problem hiding this comment.
Add JSDoc comment for 'rateCourse' describing its purpose, parameters (courseId: string, rating: number), return type (Promise with message and rating), and possible errors.
documentation
| return response.data; | ||
| }; | ||
|
|
||
| export const getCourseRatings = async ( |
There was a problem hiding this comment.
Add JSDoc comment for 'getCourseRatings' describing its purpose, parameter (courseId: string), return type (Promise with ratings array and metadata), and possible errors.
documentation
| </span> | ||
| </div> | ||
| </div> | ||
| <div className="mt-2 sm:mt-4 flex items-center space-x-2 text-sm text-gray-400"> |
There was a problem hiding this comment.
💡 Suggestion — The averageRating state is rounded with Math.round when passed to StarRating component, but displayed with toFixed(1) in text, causing slight inconsistency in displayed rating vs stars.
Use consistent rounding for both star display and numeric display to avoid confusion.
best-practices
| <span className="text-gray-300">Rate this course:</span> | ||
| <StarRating readonly={false} rating={rating} onRatingChange={setRating} /> | ||
| <div className="ml-4"> | ||
| <button onClick={() => handleRatingSubmit(rating)} className="bg-alien-green text-royal-black px-3 py-2 rounded-lg font-semibold hover:bg-alien-green/90 transition-colors duration-300"> |
There was a problem hiding this comment.
💡 Suggestion — The Submit Rating button triggers handleRatingSubmit with current rating state, but the rating state is updated only on StarRating onRatingChange. There is potential for stale rating if user clicks submit without changing stars.
Consider disabling submit button when rating is zero or add immediate validation feedback to prevent submitting invalid rating.
best-practices
| navigate(`/courses/${course.id}/test/${testId}/results`); | ||
| }; | ||
|
|
||
| const handleRatingSubmit = async (rating: number) => { |
There was a problem hiding this comment.
💡 Suggestion — The function 'handleRatingSubmit' uses the parameter name 'rating' which shadows the state variable 'rating'. This can cause confusion when reading the code.
Rename the parameter to something like 'selectedRating' to clearly distinguish it from the state variable.
readability
| } | ||
| }; | ||
|
|
||
| const fetchUserRating = async () => { |
There was a problem hiding this comment.
💡 Suggestion — The function 'fetchUserRating' fetches ratings and updates multiple state variables. The logic to update 'rating' state is conditional on 'response.hasRated' and 'response.userRating !== null'. This logic is clear but could benefit from a comment explaining the purpose.
Add a comment explaining that the rating state is set only if the user has rated the course, otherwise it remains 0.
readability
| </span> | ||
| </div> | ||
| </div> | ||
| <div className="mt-2 sm:mt-4 flex items-center space-x-2 text-sm text-gray-400"> |
There was a problem hiding this comment.
💡 Suggestion — The JSX for the course completion certificate and rating UI is nested more than 3 levels deep, which can reduce readability.
Consider using early returns or extracting nested JSX into smaller components to reduce nesting depth.
readability
📝 Description
Frontend Changes
client/src/components/ui/Star.tsx (New File): Created reusable Star component
client/src/components/ui/StarRating.tsx (New File)
client/src/components/courses/CoursesPage.tsx
client/src/components/courses/CourseDetailPage.tsx
rating,averageRating,ratingCount)handleRatingSubmit()function with validationfetchUserRating()to load existing user ratingsclient/src/types/index.ts
average_rating: numberandrating_count: numbertoCourseinterfaceclient/src/utils/api.ts
rateCourse()API function to submit/update ratingsgetCourseRatings()API function to fetch ratings dataBackend Changes
server/prisma/schema.prisma
CourseRatingmodel with fields:id: Unique identifiercourseId: Reference to courseuserId: Reference to userrating: Integer value (1-5)server/routes/courses.js
average_ratingandrating_countfor each course (conform to the new interface of Course in index.ts)average_ratingandrating_countfor the course with that id (conform to the new interface of Course in index.ts)Documentation Changes
docs/API.md: Added documentation for
GET /api/courses/:id/rateendpoint andPOST /api/courses/:id/rateendpoint🔗 Related Issue
Closes #5
🏷️ Type of Change
📸 Screenshots (if applicable)
Video demo (this video is done before I swapped the position of sharing button and rating in header in the last commit):
cinnamon-2026-01-05T185512+0100.webm
cinnamon-2026-01-05T185402+0100.webm
✅ Checklist
🧪 Testing
📋 Additional Notes
I'm SWOC 2026 participant. Can you add the label to this PR for me. Thank you so much.
SWOC 2026 Participant? Add
swoc2026label to your PR! 🎉