feat: display reading speed on analytics dashboard #2220
Conversation
jo-elimu
requested review from
AshishBagdane,
shiv810 and
vuriaval
and removed request for
a team
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2220 +/- ##
============================================
- Coverage 16.53% 16.47% -0.07%
Complexity 419 419
============================================
Files 245 245
Lines 6754 6781 +27
Branches 780 785 +5
============================================
Hits 1117 1117
- Misses 5586 5613 +27
Partials 51 51 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughThis change introduces analytics for student reading speed by calculating and displaying weekly averages of correct words per minute based on assessment events. The backend prepares and logs the data, while the frontend renders a new line chart in the student analytics view. Additionally, the project artifact version is incremented. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StudentController
participant Model
participant JSP (View)
User->>StudentController: Request student analytics page
StudentController->>StudentController: Fetch WordAssessmentEvents
StudentController->>StudentController: Compute weekly reading speed averages
StudentController->>Model: Add readingSpeedAvgList, weekList
StudentController->>JSP (View): Render page with model data
JSP (View)->>User: Display analytics page with reading speed chart
Possibly related PRs
Suggested reviewers
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
🧹 Nitpick comments (1)
src/main/java/ai/elimu/web/analytics/students/StudentController.java (1)
163-172: Consider removing debug logging statements.The logging statements on lines 163, 165, 167, and 171 appear to be for debugging purposes. Consider removing them before production deployment to avoid log noise.
Apply this diff to remove debug logging:
- log.info("wordsReadCount: " + wordsReadCount); Long timeSpentMsSum = timeSpentMsSumByWeekMap.getOrDefault(weekAsString, 0L); - log.info("timeSpentMsSum: " + timeSpentMsSum); Double timeSpentInMinutes = (double) timeSpentMsSum / 60_000.0; - log.info("timeSpentInMinutes: " + timeSpentInMinutes); Double wordsPerMinute = 0.00; if (timeSpentInMinutes > 0) { wordsPerMinute = wordsReadCount / timeSpentInMinutes; - log.info("wordsPerMinute: " + wordsPerMinute); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pom-dependency-tree.txt(1 hunks)src/main/java/ai/elimu/web/analytics/students/StudentController.java(1 hunks)src/main/webapp/WEB-INF/jsp/analytics/students/id.jsp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: test_ui
- GitHub Check: build (macos-latest, 21)
- GitHub Check: build (macos-latest, 17)
- GitHub Check: test_rest
- GitHub Check: build (windows-latest, 17)
- GitHub Check: build (windows-latest, 21)
- GitHub Check: build (ubuntu-latest, 21)
- GitHub Check: build (ubuntu-latest, 17)
- GitHub Check: test_rest
- GitHub Check: test_rest
🔇 Additional comments (2)
pom-dependency-tree.txt (1)
1-1: LGTM: Standard version increment for new feature.The version update from
2.6.32-SNAPSHOTto2.6.34-SNAPSHOTappropriately reflects the addition of the reading speed analytics feature.src/main/webapp/WEB-INF/jsp/analytics/students/id.jsp (1)
205-232: LGTM: Well-structured chart implementation.The reading speed chart implementation follows the established patterns in the file with proper Chart.js configuration, consistent styling, and clear structure. The integration with the backend
readingSpeedAvgListdata is correct.
| String eventWeek = simpleDateFormat.format(event.getTimestamp().getTime()); | ||
| if (event.getMasteryScore() >= 0.5) { | ||
| eventCountByWeekMap.put(eventWeek, eventCountByWeekMap.getOrDefault(eventWeek, 0) + 1); | ||
| timeSpentMsSumByWeekMap.put(eventWeek, eventCountByWeekMap.getOrDefault(eventWeek, 0) + event.getTimeSpentMs()); |
There was a problem hiding this comment.
Fix the time sum calculation bug.
Line 156 incorrectly uses eventCountByWeekMap.getOrDefault(eventWeek, 0) instead of the time sum map.
Apply this diff to fix the bug:
- timeSpentMsSumByWeekMap.put(eventWeek, eventCountByWeekMap.getOrDefault(eventWeek, 0) + event.getTimeSpentMs());
+ timeSpentMsSumByWeekMap.put(eventWeek, timeSpentMsSumByWeekMap.getOrDefault(eventWeek, 0L) + event.getTimeSpentMs());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| timeSpentMsSumByWeekMap.put(eventWeek, eventCountByWeekMap.getOrDefault(eventWeek, 0) + event.getTimeSpentMs()); | |
| timeSpentMsSumByWeekMap.put( | |
| eventWeek, | |
| - eventCountByWeekMap.getOrDefault(eventWeek, 0) + event.getTimeSpentMs() | |
| + timeSpentMsSumByWeekMap.getOrDefault(eventWeek, 0L) + event.getTimeSpentMs() | |
| ); |
🤖 Prompt for AI Agents
In src/main/java/ai/elimu/web/analytics/students/StudentController.java at line
156, the code incorrectly sums time spent by using eventCountByWeekMap instead
of timeSpentMsSumByWeekMap. Fix this by replacing
eventCountByWeekMap.getOrDefault(eventWeek, 0) with
timeSpentMsSumByWeekMap.getOrDefault(eventWeek, 0) to correctly accumulate the
total time spent per week.
| log.info("wordsReadCount: " + wordsReadCount); | ||
| Long timeSpentMsSum = timeSpentMsSumByWeekMap.getOrDefault(weekAsString, 0L); | ||
| log.info("timeSpentMsSum: " + timeSpentMsSum); | ||
| Double timeSpentInMinutes = (double) (timeSpentMsSum / 1_000); |
There was a problem hiding this comment.
Correct the milliseconds to minutes conversion.
Line 166 incorrectly converts milliseconds to minutes by dividing by 1,000. The correct conversion should divide by 60,000 (1000ms × 60s = 60,000ms per minute).
Apply this diff to fix the conversion:
- Double timeSpentInMinutes = (double) (timeSpentMsSum / 1_000);
+ Double timeSpentInMinutes = (double) timeSpentMsSum / 60_000.0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Double timeSpentInMinutes = (double) (timeSpentMsSum / 1_000); | |
| - Double timeSpentInMinutes = (double) (timeSpentMsSum / 1_000); | |
| + Double timeSpentInMinutes = (double) timeSpentMsSum / 60_000.0; |
🤖 Prompt for AI Agents
In src/main/java/ai/elimu/web/analytics/students/StudentController.java at line
166, the conversion from milliseconds to minutes is incorrect because it divides
by 1,000 instead of 60,000. Fix this by changing the divisor from 1,000 to
60,000 to correctly convert milliseconds to minutes.
Issue Number
Purpose
Technical Details
Testing Instructions
Screenshots
Format Checks
Note
Files in PRs are automatically checked for format violations with
mvn spotless:check.If this PR contains files with format violations, run
mvn spotless:applyto fix them.