enhance: add lecturer course leaderboards containing collected points rolling over last 7 or 14 days #4521
Conversation
… rolling over last 7 or 14 days
dosubot
bot
added
the
size:L
📝 WalkthroughWalkthroughThis pull request extends the leaderboard functionality by introducing two new leaderboard types— Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant F as Frontend
participant G as GraphQL API
participant S as Course Service
U->>F: Select leaderboard type ('7rolling'/'14rolling')
F->>G: Request getCourseLeaderboard (with rollingSelection flag and days)
G->>S: Forward request with parameters
alt Rolling leaderboard selected
S->>S: Execute computeRollingLeaderboardEntries
else Other leaderboard type
S->>S: Execute existing leaderboard logic
end
S-->>G: Return leaderboard data
G-->>F: Respond with data
F-->>U: Display updated leaderboard
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![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)
packages/graphql/src/services/courses.ts (1)
628-629: Enhance code consistency for 'biweekly' mode.The
mode === 'biweekly'branch now correctly leveragescomputeRollingLeaderboardEntrieswith a fixed 14-day range. Consider unifying or consolidating modes like 'biweekly' and'14rolling'to avoid logic duplication if they serve the same purpose.apps/frontend-manage/src/components/courses/SuspendedCourseLeaderboard.tsx (1)
150-156: Consider recompute option for rolling leaderboards.Right now, the “Recompute” button only triggers timeline updates for
'weekly'and'custom'. You might want to clarify or extend this feature for'7rolling'and'14rolling'as well, in case a manual refresh of data is desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
packages/graphql/src/graphql/ops/QGetCourseLeaderboard.graphqlis excluded by!**/**/graphql/ops/**packages/graphql/src/ops.schema.jsonis excluded by!**/**/ops.schema.jsonpackages/graphql/src/ops.tsis excluded by!**/**/ops.tspackages/graphql/src/public/client.jsonis excluded by!**/**/public/**packages/graphql/src/public/schema.graphqlis excluded by!**/**/public/**packages/graphql/src/public/server.jsonis excluded by!**/**/public/**
📒 Files selected for processing (6)
apps/frontend-manage/src/components/courses/IndividualLeaderboard.tsx(2 hunks)apps/frontend-manage/src/components/courses/SuspendedCourseLeaderboard.tsx(5 hunks)packages/graphql/src/schema/query.ts(1 hunks)packages/graphql/src/services/courses.ts(4 hunks)packages/i18n/messages/de.ts(1 hunks)packages/i18n/messages/en.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: cypress-run
- GitHub Check: build
- GitHub Check: Analyze (javascript)
- GitHub Check: check
🔇 Additional comments (9)
packages/graphql/src/services/courses.ts (2)
351-526: Validate date range logic and random ID generation.
- The function uses
(days - 1)when filtering out certain quizzes while usingdaysfor timeline entries. This leads to slightly different boundary intervals between live quizzes and timeline entries. Please confirm if this staggering of dates (one day difference) is intentional or if you want both intervals to match.- The random ID (
Math.floor(random(1000000000))) could potentially collide when the dataset grows large. Consider using a more robust unique identifier or simply omit the ID if it's only used for display purposes.- While the current approach may suffice for smaller datasets, you may want to move part of the aggregation logic to database queries for efficiency if the leaderboard grows over time.
[verify, refactor_suggestion_good_to_have]
1107-1112: Rolling leaderboard parameters introduced.The added
rollingSelectionanddaysparameters cleanly extend the leaderboard functionality. However, note that the codeif (!days) return nullsilently fails instead of throwing a more descriptive error for missing days. Consider warning or throwing an explicit message for better debugging.apps/frontend-manage/src/components/courses/SuspendedCourseLeaderboard.tsx (2)
42-49: Check default fallback for non-rolling leaderboard.Currently,
daysdefaults to 14 unless explicitly set to 7 for'7rolling'. For other leaderboard types, this extra value does no harm becauserollingSelectionis false. Ensure that logic is clear to future maintainers, or conditionally passdaysonly whenrollingSelectionis true for extra clarity.
71-81: Align displayed rolling date range with backend logic.When the leaderboard type is
'7rolling'or'14rolling', the displayed date range in the UI is computed withdayjs(data.getCourseLeaderboard.computedAt).subtract(...). Meanwhile, the backend uses(days - 1)for certain queries anddaysfor timeline entries. Consider verifying these offsets to ensure the displayed range fully matches the server’s data range.Also applies to: 123-147
apps/frontend-manage/src/components/courses/IndividualLeaderboard.tsx (2)
41-42: Expanded leaderboard type state.The new
'7rolling'and'14rolling'options correctly extend the existing types. No immediate issues noted.
81-85: Additional rolling options and trigger width.
- Adding
'7rolling'and'14rolling'nicely integrates rolling leaderboards into the UI.- Widening the select trigger (
w-52) could affect layout; ensure that the rest of the page still aligns as intended.packages/graphql/src/schema/query.ts (1)
504-508: LGTM! The new arguments for rolling leaderboard are well-defined.The addition of
rollingSelectionanddaysarguments to thegetCourseLeaderboardquery is well-structured:
rollingSelectionis correctly marked as required to explicitly enable rolling leaderboard calculationdaysis correctly marked as optional since it's only needed whenrollingSelectionis truepackages/i18n/messages/en.ts (1)
1895-1896: LGTM! The localization entries for rolling leaderboard options are well-defined.The new entries for 7-day and 14-day rolling leaderboards are:
- Added in the correct location within the
coursesection- Follow consistent naming and labeling patterns
- Match the functionality described in the AI summary
packages/i18n/messages/de.ts (1)
1927-1928: LGTM! The new leaderboard type translations are well-implemented.The added German translations for rolling 7-day and 14-day leaderboard types are accurate, consistent with the existing translations, and properly convey the rolling nature of the new leaderboard views.
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
rolling-leaderboards-lecturer
|
| Run status |
|
| Run duration | 26m 44s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
349
|
| View all changes introduced in this branch ↗︎ | |
Summary by CodeRabbit