Make popular courses only show courses in the user's current language#1289
Make popular courses only show courses in the user's current language#1289
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1289 +/- ##
==========================================
+ Coverage 62.83% 65.80% +2.97%
==========================================
Files 151 154 +3
Lines 6388 6604 +216
Branches 1580 1631 +51
==========================================
+ Hits 4014 4346 +332
+ Misses 2204 2108 -96
+ Partials 170 150 -20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nygrenh
force-pushed
the
popular-courses-fixes
branch
from
aca7613 to
c7bf4f6
Compare
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant Client as Frontend Client
participant GraphQL as GraphQL API
participant Resolver as popularCourses Resolver
participant DB as Database (Prisma)
Client->>GraphQL: PopularCourses Query (language: "fi_FI")
GraphQL->>Resolver: Execute popularCourses Field
Resolver->>DB: Fetch courses where<br/>- hidden = false<br/>- status != CourseStatus.Ended<br/>- completions_handled_by_id = null
DB-->>Resolver: Course records
Resolver->>DB: Fetch translations for courses filtered by language
DB-->>Resolver: Translation records
rect rgba(100,150,200,0.5)
Note over Resolver: Map translations into course fields
Resolver->>Resolver: Fill name/description/link from translation (or fallback)
Resolver->>Resolver: Shuffle results
Resolver->>Resolver: Limit to 4 items
end
Resolver-->>GraphQL: Filtered & shuffled course list (max 4)
GraphQL-->>Client: popularCourses response (id, slug, name, description, link, status)
Client->>Client: Render SelectedCourses with popular courses
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
🧹 Nitpick comments (2)
backend/schema/Course/__test__/Course.queries.test.ts (1)
383-393: Consider improving test isolation.The test "excludes hidden courses" checks that the "handled" course is excluded, but this course is filtered out for two reasons:
hidden: true(from fixtures)completions_handled_by_idis set (from fixtures)The backend filters on both conditions (lines 329-334 and 349 in queries.ts), so while the test passes correctly, it's ambiguous which filter is actually being verified. Consider adding a test case with a course that is hidden but does NOT have
completions_handled_by_idset to explicitly verify the hidden filter in isolation.backend/schema/Course/queries.ts (1)
311-389: Consider extracting the shared translation mapping logic.The implementation is functionally correct and properly filters courses by language, visibility, status, and completion handling. However, the translation mapping logic (lines 358-377) is nearly identical to the same logic in the
coursesquery (lines 278-299).Consider extracting this into a shared helper function to improve maintainability and reduce duplication:
const mapCourseWithTranslation = ( course: Course & { course_translations?: Array<CourseTranslation> }, language?: string ) => { const translationForLanguage = language ? course?.course_translations?.find((t) => t.language === language) : undefined const translation = translationForLanguage ?? course?.course_translations?.[0] return { ...omit(course, ["course_translations", "tags", "handles_completions_for"]), description: translation?.description ?? "", link: translation?.link ?? "", name: translation?.name ?? course?.name ?? "", } }This would make both resolvers more maintainable and reduce the risk of inconsistencies when updating translation logic in the future.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/schema/Course/__test__/Course.queries.test.tsbackend/schema/Course/queries.tsfrontend/components/NewLayout/Frontpage/SelectedCourses.tsxfrontend/graphql/queries/course.queries.graphql
🧰 Additional context used
🧬 Code graph analysis (2)
backend/schema/Course/__test__/Course.queries.test.ts (1)
backend/tests/data/seed.ts (1)
seed(36-146)
backend/schema/Course/queries.ts (2)
backend/tests/data/fixtures.ts (1)
courses(169-324)backend/schema/Course/model.ts (1)
Course(9-189)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build-deploy
🔇 Additional comments (5)
frontend/graphql/queries/course.queries.graphql (1)
21-25: LGTM!The new
PopularCoursesquery is well-structured and correctly uses theNewCourseFieldsfragment for consistency with other queries.backend/schema/Course/__test__/Course.queries.test.ts (2)
326-369: LGTM!The test suite setup and initial tests provide good coverage of the language filtering and exclusion logic for the new
popularCoursesfield.
651-663: LGTM!The test query correctly mirrors the fields needed to verify the
popularCoursesresolver behavior.backend/schema/Course/queries.ts (1)
1-1: LGTM!The new imports for
shuffleandCourseStatusare correctly added to support thepopularCoursesimplementation.Also applies to: 12-12
frontend/components/NewLayout/Frontpage/SelectedCourses.tsx (1)
2-2: LGTM!The component correctly migrates from the
NewCoursesDocumentquery with client-side filtering to the newPopularCoursesDocumentquery with server-side filtering. The changes properly update:
- The GraphQL query and variable naming
- The data binding to use
data.popularCourses- Removal of the unused
sampleimportThe loading behavior remains consistent with 4 skeleton cards.
Also applies to: 29-29, 107-112
2 participants
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.