feat: export csv - storybook learning events, video learning events #2205
Conversation
jo-elimu
requested review from
AshishBagdane,
nya-elimu and
tomaszsmy
and removed request for
a team
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2205 +/- ##
============================================
+ Coverage 14.41% 14.56% +0.14%
+ Complexity 405 402 -3
============================================
Files 245 242 -3
Lines 6640 6552 -88
Branches 758 744 -14
============================================
- Hits 957 954 -3
+ Misses 5632 5547 -85
Partials 51 51 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Caution Review failedThe pull request is closed. WalkthroughThis change updates the ordering of learning event queries in multiple DAO classes from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AnalyticsUI
participant StudentDao
participant StoryBookLearningEventDao
participant CsvExportController
User->>AnalyticsUI: Clicks "Export StoryBook Learning Events CSV"
AnalyticsUI->>CsvExportController: HTTP GET /analytics/students/{studentId}/storybook-learning-events/csv
CsvExportController->>StudentDao: findById(studentId)
StudentDao-->>CsvExportController: Student (with androidId)
CsvExportController->>StoryBookLearningEventDao: readAll(androidId)
StoryBookLearningEventDao-->>CsvExportController: List of events
CsvExportController-->>AnalyticsUI: CSV file (filtered by studentId)
AnalyticsUI-->>User: CSV download
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
✨ 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: 6
♻️ Duplicate comments (2)
src/main/java/ai/elimu/dao/jpa/LetterSoundLearningEventDaoJpa.java (1)
18-18: Consistent ordering change - same semantic considerations apply.This change follows the same pattern as other DAO classes in this PR, changing from timestamp-based to id-based ordering. Please ensure this ordering change meets the requirements for learning event analytics and CSV exports.
src/main/java/ai/elimu/dao/jpa/StoryBookLearningEventDaoJpa.java (1)
51-51: Consistent ordering change with preserved timestamp ordering elsewhere.This change follows the same pattern as other DAO classes, changing the readAll(androidId) method from timestamp to id ordering. The presence of the
readAllOrderedByTime()method (line 16-22) that still orders by timestamp confirms that this change is intentional and context-specific.
🧹 Nitpick comments (2)
src/main/java/ai/elimu/web/analytics/students/StoryBookLearningEventsCsvExportController.java (2)
41-41: Consider removing sensitive logging information.Logging the Android ID might expose sensitive user information in log files, which could be a privacy concern.
Consider removing or redacting the Android ID from logs:
-log.info("student.getAndroidId(): " + student.getAndroidId()); +log.info("Exporting storybook learning events for student: " + studentId);
32-37: Consider renaming method to be more descriptive.The method name
handleRequestis generic and doesn't clearly indicate its purpose.Consider renaming to be more descriptive:
-public void handleRequest( +public void exportStoryBookLearningEventsCsv(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
pom-dependency-tree.txt(1 hunks)src/main/java/ai/elimu/dao/jpa/LetterSoundAssessmentEventDaoJpa.java(1 hunks)src/main/java/ai/elimu/dao/jpa/LetterSoundLearningEventDaoJpa.java(1 hunks)src/main/java/ai/elimu/dao/jpa/StoryBookLearningEventDaoJpa.java(1 hunks)src/main/java/ai/elimu/dao/jpa/VideoLearningEventDaoJpa.java(1 hunks)src/main/java/ai/elimu/dao/jpa/WordLearningEventDaoJpa.java(1 hunks)src/main/java/ai/elimu/web/analytics/StoryBookLearningEventListController.java(0 hunks)src/main/java/ai/elimu/web/analytics/students/StoryBookLearningEventsCsvExportController.java(4 hunks)src/main/java/ai/elimu/web/analytics/students/WordLearningEventsCsvExportController.java(1 hunks)src/main/java/ai/elimu/web/servlet/CustomDispatcherServlet.java(2 hunks)src/main/webapp/WEB-INF/jsp/analytics/students/id.jsp(3 hunks)src/test/java/selenium/analytics/MainAnalyticsPage.java(0 hunks)src/test/java/selenium/analytics/StoryBookLearningEventsPage.java(0 hunks)src/test/java/selenium/analytics/StoryBookLearningEventsPageTest.java(0 hunks)
💤 Files with no reviewable changes (4)
- src/test/java/selenium/analytics/MainAnalyticsPage.java
- src/test/java/selenium/analytics/StoryBookLearningEventsPage.java
- src/test/java/selenium/analytics/StoryBookLearningEventsPageTest.java
- src/main/java/ai/elimu/web/analytics/StoryBookLearningEventListController.java
🧰 Additional context used
🧠 Learnings (2)
src/main/java/ai/elimu/dao/jpa/LetterSoundAssessmentEventDaoJpa.java (1)
Learnt from: venkatesh2k3
PR: elimu-ai/webapp#1682
File: src/main/java/ai/elimu/dao/LetterSoundContributionEventDao.java:13-13
Timestamp: 2024-07-06T17:37:44.413Z
Learning: In the `LetterSoundContributionEventDao.java` file, the method signatures should use `LetterSoundContributionEvent` instead of `LetterSoundCorrespondenceContributionEvent` to maintain consistency with the renaming of entities.
src/main/java/ai/elimu/dao/jpa/LetterSoundLearningEventDaoJpa.java (1)
Learnt from: venkatesh2k3
PR: elimu-ai/webapp#1682
File: src/main/java/ai/elimu/dao/LetterSoundContributionEventDao.java:13-13
Timestamp: 2024-07-06T17:37:44.413Z
Learning: In the `LetterSoundContributionEventDao.java` file, the method signatures should use `LetterSoundContributionEvent` instead of `LetterSoundCorrespondenceContributionEvent` to maintain consistency with the renaming of entities.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: test_ui
- GitHub Check: test_ui
- GitHub Check: build (windows-latest, 21)
- GitHub Check: build (windows-latest, 17)
- GitHub Check: build (ubuntu-latest, 17)
- GitHub Check: build (macos-latest, 21)
- GitHub Check: test_rest
- GitHub Check: build (macos-latest, 17)
- GitHub Check: build (ubuntu-latest, 21)
- GitHub Check: test_rest
- GitHub Check: test_rest
🔇 Additional comments (9)
pom-dependency-tree.txt (1)
1-1: Version bump forwebapp:warartifact
The root artifact has been correctly updated to2.6.23-SNAPSHOTto match this feature development cycle. Please ensure that your release tagging and downstream CI/CD pipelines reference this new version.src/main/java/ai/elimu/dao/jpa/LetterSoundAssessmentEventDaoJpa.java (1)
18-18:❓ Verification inconclusive
Verify the semantic impact of changing ordering from timestamp to id.
This change modifies the ordering criterion from chronological order (timestamp) to insertion order (id). While ID-based ordering typically provides consistent results, it may not preserve the actual temporal sequence of events if there are delays in event processing or out-of-order insertions.
Please verify that this ordering change aligns with the intended analytics and CSV export requirements, and ensure that downstream consumers of this data are compatible with the new ordering.
🏁 Script executed:
#!/bin/bash # Description: Check for other usages of readAll method to understand impact # Expected: Find usages of readAll method in controllers and services rg -A 3 -B 3 "readAll.*androidId" --type javaLength of output: 6143
🏁 Script executed:
#!/bin/bash # Verify ordering criteria in all JPA DAO implementations rg -n "ORDER BY" --type java src/main/java/ai/elimu/dao/jpaLength of output: 9948
Confirm default id-based ordering is intentional
The change in LetterSoundAssessmentEventDaoJpa.readAll—from ordering by
event.timestamptoevent.id—now matches the pattern used by other event DAOs:
- VideoLearningEventDaoJpa.readAll → ORDER BY event.id
- WordLearningEventDaoJpa.readAll → ORDER BY event.id
- StoryBookLearningEventDaoJpa.readAll → ORDER BY event.id
- LetterSoundLearningEventDaoJpa.readAll → ORDER BY event.id
Chronological queries in some DAOs are still available via explicit
readAllOrderedByTime(*)methods (e.g. StoryBookLearningEventDao). LetterSoundAssessmentEventDao does not expose a time‐ordered variant.Please verify:
- That downstream consumers do not depend on chronological ordering of assessment events.
- If they do, consider adding a
readAllOrderedByTime(..)method to the interface and its JPA implementation.src/main/java/ai/elimu/web/servlet/CustomDispatcherServlet.java (1)
332-332: LGTM! Increased test data generation for better coverage.The change from
Math.random() * 8toMath.random() * 10increases the maximum number of learning events generated per student per week from 0-7 to 0-9. This provides better test data coverage for analytics and CSV export functionality.Also applies to: 344-344
src/main/webapp/WEB-INF/jsp/analytics/students/id.jsp (1)
226-237: LGTM! Well-implemented CSV export functionality.The CSV export button follows the established pattern with proper click handling and user feedback via toast notifications.
src/main/java/ai/elimu/web/analytics/students/StoryBookLearningEventsCsvExportController.java (5)
1-6: Package structure and imports look good.The move to the
studentsanalytics package provides better organization, and the new imports are appropriate for the student-specific functionality.
23-23: Request mapping path follows RESTful conventions.The new endpoint structure
/analytics/students/{studentId}/storybook-learning-events.csvclearly indicates it's a student-specific CSV export, which is good design.
55-58: CSV header structure is well-organized.The new header structure with
storybook_id,learning_event_type, andadditional_dataprovides comprehensive event information while removing the redundantandroid_idcolumn.
72-75: CSV record output matches header structure.The record output correctly corresponds to the updated header structure, ensuring data consistency.
77-77: Good optimization: flush moved outside the loop.Moving the CSV printer flush outside the loop improves performance by reducing I/O operations.
| wordLearningEvent.getPackageName(), | ||
| wordLearningEvent.getWordText(), | ||
| (wordLearningEvent.getWord() != null) ? wordLearningEvent.getWord().getId() : 0, | ||
| (wordLearningEvent.getWord() == null) ? null : wordLearningEvent.getWord().getId(), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Improved data integrity for null word references.
This change correctly represents missing word references as null instead of 0 in the CSV export, which improves data integrity and semantic accuracy.
Please verify that downstream systems consuming this CSV data can handle null values in the word_id column, as this changes the data format from the previous implementation.
🏁 Script executed:
#!/bin/bash
# Description: Check for other CSV export controllers to ensure consistent null handling
# Expected: Find similar patterns in other CSV export controllers
rg -A 5 -B 5 "== null.*\?.*:" --type java src/main/java/ai/elimu/web/analytics/Length of output: 1578
Ensure downstream compatibility and consistent null handling
The change to output null instead of 0 for missing word_id improves data integrity and semantic correctness. A quick search across other CSV export controllers in src/main/java/ai/elimu/web/analytics/ found no similar null-handling logic:
• Only WordLearningEventsCsvExportController.java uses
(wordLearningEvent.getWord() == null) ? null : wordLearningEvent.getWord().getId()in its CSV export.
Recommendations:
- Verify that all downstream systems consuming the CSV can handle
nullvalues in the word_id column without errors. - If other controllers or consumers expect
0for missing IDs, consider updating them to acceptnullor standardize on a single representation across all exports.
🤖 Prompt for AI Agents
In
src/main/java/ai/elimu/web/analytics/students/WordLearningEventsCsvExportController.java
at line 68, the CSV export now outputs null instead of 0 for missing word IDs to
improve data integrity. Verify that all downstream systems consuming this CSV
can handle null values in the word_id column without errors. If other CSV export
controllers or consumers expect 0 for missing IDs, update them to accept null or
standardize the representation across all exports for consistency.
| "FROM VideoLearningEvent event " + | ||
| "WHERE event.androidId = :androidId " + | ||
| "ORDER BY event.timestamp") | ||
| "ORDER BY event.id") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider the implications of ordering by ID instead of timestamp.
Changing from ORDER BY event.timestamp to ORDER BY event.id may not preserve chronological ordering. While IDs are typically sequential, they don't guarantee chronological order if:
- Events are inserted out of sequence
- There are concurrent insertions
- Database transactions complete in different orders
Since this affects how events are displayed in analytics and exported in CSV files, consider whether chronological ordering is important for your use case.
If chronological ordering is required, consider reverting to timestamp ordering:
- "ORDER BY event.id")
+ "ORDER BY event.timestamp ASC")Or use a compound ordering strategy:
- "ORDER BY event.id")
+ "ORDER BY event.timestamp ASC, event.id ASC")🤖 Prompt for AI Agents
In src/main/java/ai/elimu/dao/jpa/VideoLearningEventDaoJpa.java at line 51, the
query orders events by event.id which may not reflect the true chronological
order due to possible out-of-sequence inserts or concurrent transactions. To
ensure events are displayed and exported in the correct chronological order,
change the ORDER BY clause to use event.timestamp instead of event.id, or use a
compound ordering such as ORDER BY event.timestamp, event.id to maintain
consistent and accurate event sequencing.
| "FROM WordLearningEvent event " + | ||
| "WHERE event.androidId = :androidId " + | ||
| "ORDER BY event.timestamp") | ||
| "ORDER BY event.id") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ordering by ID instead of timestamp may affect chronological sequence.
This change mirrors the same pattern in other DAO classes, but ordering by event.id instead of event.timestamp could break chronological ordering of learning events. This is particularly important for analytics and learning progression tracking.
Consider maintaining chronological ordering:
- "ORDER BY event.id")
+ "ORDER BY event.timestamp ASC")📝 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.
| "ORDER BY event.id") | |
| - "ORDER BY event.id") | |
| + "ORDER BY event.timestamp ASC") |
🤖 Prompt for AI Agents
In src/main/java/ai/elimu/dao/jpa/WordLearningEventDaoJpa.java at line 43, the
query orders results by event.id, which may disrupt chronological order. To fix
this, change the ORDER BY clause to use event.timestamp instead of event.id to
ensure learning events are returned in the correct chronological sequence.
| <c:forEach var="i" begin="0" end="4"> | ||
| <c:set var="videoLearningEvent" value="${videoLearningEvents[fn:length(videoLearningEvents) - 1 - i]}" /> | ||
| <tr> | ||
| <td> | ||
| ${videoLearningEvent.id} | ||
| </td> | ||
| <td> | ||
| ${videoLearningEvent.timestamp.time} | ||
| </td> | ||
| <td> | ||
| <code>${videoLearningEvent.packageName}</code> | ||
| </td> | ||
| <td> | ||
| "${videoLearningEvent.videoTitle}" | ||
| </td> | ||
| </tr> | ||
| </c:forEach> |
There was a problem hiding this comment.
Fix potential index out of bounds in video events table.
Same issue as the storybook events table - this could fail if there are fewer than 5 video learning events.
Apply the same fix pattern:
<c:forEach var="i" begin="0" end="4">
- <c:set var="videoLearningEvent" value="${videoLearningEvents[fn:length(videoLearningEvents) - 1 - i]}" />
- <tr>
+ <c:if test="${(fn:length(videoLearningEvents) - 1 - i) >= 0}">
+ <c:set var="videoLearningEvent" value="${videoLearningEvents[fn:length(videoLearningEvents) - 1 - i]}" />
+ <tr>
<td>
${videoLearningEvent.id}
</td>
...
- </tr>
+ </tr>
+ </c:if>
</c:forEach>🤖 Prompt for AI Agents
In src/main/webapp/WEB-INF/jsp/analytics/students/id.jsp around lines 331 to
347, the forEach loop assumes there are always at least 5 videoLearningEvents,
which can cause an index out of bounds error if there are fewer. Fix this by
adjusting the loop to iterate only up to the smaller of 4 or the last index of
videoLearningEvents, ensuring the index used to access videoLearningEvents is
always valid.
| <c:forEach var="i" begin="0" end="4"> | ||
| <c:set var="storyBookLearningEvent" value="${storyBookLearningEvents[fn:length(storyBookLearningEvents) - 1 - i]}" /> | ||
| <tr> | ||
| <td> | ||
| ${storyBookLearningEvent.id} | ||
| </td> | ||
| <td> | ||
| ${storyBookLearningEvent.timestamp.time} | ||
| </td> | ||
| <td> | ||
| <code>${storyBookLearningEvent.packageName}</code> | ||
| </td> | ||
| <td> | ||
| "${storyBookLearningEvent.storyBookTitle}" | ||
| </td> | ||
| </tr> | ||
| </c:forEach> |
There was a problem hiding this comment.
Fix potential index out of bounds in table display.
The table logic could fail if there are fewer than 5 storybook learning events. When i=4 and there are only 2 events, the expression storyBookLearningEvents[fn:length(storyBookLearningEvents) - 1 - i] would try to access index -3.
Add a condition to check if the event exists:
<c:forEach var="i" begin="0" end="4">
- <c:set var="storyBookLearningEvent" value="${storyBookLearningEvents[fn:length(storyBookLearningEvents) - 1 - i]}" />
- <tr>
+ <c:if test="${(fn:length(storyBookLearningEvents) - 1 - i) >= 0}">
+ <c:set var="storyBookLearningEvent" value="${storyBookLearningEvents[fn:length(storyBookLearningEvents) - 1 - i]}" />
+ <tr>
<td>
${storyBookLearningEvent.id}
</td>
...
- </tr>
+ </tr>
+ </c:if>
</c:forEach>📝 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.
| <c:forEach var="i" begin="0" end="4"> | |
| <c:set var="storyBookLearningEvent" value="${storyBookLearningEvents[fn:length(storyBookLearningEvents) - 1 - i]}" /> | |
| <tr> | |
| <td> | |
| ${storyBookLearningEvent.id} | |
| </td> | |
| <td> | |
| ${storyBookLearningEvent.timestamp.time} | |
| </td> | |
| <td> | |
| <code>${storyBookLearningEvent.packageName}</code> | |
| </td> | |
| <td> | |
| "${storyBookLearningEvent.storyBookTitle}" | |
| </td> | |
| </tr> | |
| </c:forEach> | |
| <c:forEach var="i" begin="0" end="4"> | |
| <c:if test="${(fn:length(storyBookLearningEvents) - 1 - i) >= 0}"> | |
| <c:set var="storyBookLearningEvent" | |
| value="${storyBookLearningEvents[fn:length(storyBookLearningEvents) - 1 - i]}" /> | |
| <tr> | |
| <td> | |
| ${storyBookLearningEvent.id} | |
| </td> | |
| <td> | |
| ${storyBookLearningEvent.timestamp.time} | |
| </td> | |
| <td> | |
| <code>${storyBookLearningEvent.packageName}</code> | |
| </td> | |
| <td> | |
| "${storyBookLearningEvent.storyBookTitle}" | |
| </td> | |
| </tr> | |
| </c:if> | |
| </c:forEach> |
🤖 Prompt for AI Agents
In src/main/webapp/WEB-INF/jsp/analytics/students/id.jsp around lines 273 to
289, the loop assumes there are always at least 5 storyBookLearningEvents, which
can cause an index out of bounds error when there are fewer events. Modify the
<c:forEach> loop to iterate only up to the smaller of 4 or the last index of
storyBookLearningEvents, and add a condition inside the loop to check if the
event at the calculated index exists before rendering the table row.
| Student student = studentDao.read(studentId); | ||
| log.info("student.getAndroidId(): " + student.getAndroidId()); | ||
|
|
||
| List<StoryBookLearningEvent> storyBookLearningEvents = storyBookLearningEventDao.readAll(student.getAndroidId()); | ||
| log.info("storyBookLearningEvents.size(): " + storyBookLearningEvents.size()); |
There was a problem hiding this comment.
Add null checking for student lookup.
The code doesn't handle the case where a student with the given ID doesn't exist, which could result in a NullPointerException.
Add null checking to handle non-existent students:
Student student = studentDao.read(studentId);
+if (student == null) {
+ response.setStatus(HttpServletResponse.SC_NOT_FOUND);
+ return;
+}
log.info("student.getAndroidId(): " + student.getAndroidId());📝 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.
| Student student = studentDao.read(studentId); | |
| log.info("student.getAndroidId(): " + student.getAndroidId()); | |
| List<StoryBookLearningEvent> storyBookLearningEvents = storyBookLearningEventDao.readAll(student.getAndroidId()); | |
| log.info("storyBookLearningEvents.size(): " + storyBookLearningEvents.size()); | |
| Student student = studentDao.read(studentId); | |
| if (student == null) { | |
| response.setStatus(HttpServletResponse.SC_NOT_FOUND); | |
| return; | |
| } | |
| log.info("student.getAndroidId(): " + student.getAndroidId()); | |
| List<StoryBookLearningEvent> storyBookLearningEvents = storyBookLearningEventDao.readAll(student.getAndroidId()); | |
| log.info("storyBookLearningEvents.size(): " + storyBookLearningEvents.size()); |
🤖 Prompt for AI Agents
In
src/main/java/ai/elimu/web/analytics/students/StoryBookLearningEventsCsvExportController.java
around lines 40 to 44, add a null check after retrieving the student with
studentDao.read(studentId) to handle the case where the student is not found. If
the student is null, handle this scenario appropriately (e.g., log a warning and
return or throw an exception) before accessing student.getAndroidId() to prevent
a NullPointerException.
Issue Number
android_idwithstudent_id#2103Purpose
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.