refactor: add support for epoch seconds#2253
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2253 +/- ##
============================================
- Coverage 16.27% 16.14% -0.13%
+ Complexity 429 426 -3
============================================
Files 251 251
Lines 7140 7140
Branches 811 811
============================================
- Hits 1162 1153 -9
- Misses 5925 5935 +10
+ Partials 53 52 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughThis change updates timestamp handling in CSV analytics extraction and related DAO logging. Timestamp values are now truncated to seconds (multiplied by 1,000) during extraction, and corresponding tests are adjusted. Logging statements in several DAO classes are updated for consistency and accuracy in timestamp and parameter formatting. CSV export controllers now output timestamps in seconds instead of milliseconds. The main artifact version was also incremented. Changes
Sequence Diagram(s)sequenceDiagram
participant CSV as CsvAnalyticsExtractionHelper
participant DAO as EventDaoJpa
participant DB as Database
CSV->>CSV: Parse CSV record
CSV->>CSV: Extract timestamp string (first 10 chars)
CSV->>CSV: Convert to long and multiply by 1,000 (ms)
CSV->>DAO: Call read(...) with processed timestamp
DAO->>DB: Query event with parameters
alt Event found
DB-->>DAO: Return event
else NoResultException
DAO->>DAO: Log with timestamp.getTime() and formatted params
DAO-->>CSV: Return null
end
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)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (11)
✨ 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
🔭 Outside diff range comments (1)
src/main/java/ai/elimu/dao/jpa/NumberLearningEventDaoJpa.java (1)
23-29:⚠️ Potential issueParameter name typo breaks query binding
setParameter("androidIdevent", androidId)(line 24) does not match the named parameter:androidIddeclared in the JPQL. This causes anIllegalArgumentExceptionat runtime when the query executes.- .setParameter("androidIdevent", androidId) + .setParameter("androidId", androidId)
🧹 Nitpick comments (2)
src/main/java/ai/elimu/dao/jpa/LetterSoundLearningEventDaoJpa.java (1)
28-29: Minor: consider logging epoch milliseconds for easier grepping
timestamp.getTime()returns aDatewhose defaulttoString()is locale-dependent. Usingtimestamp.getTimeInMillis()(ortimestamp.getTime().getTime()) yields a single numeric value that is easier to grep across logs.
No change required, just a suggestion.src/main/java/ai/elimu/dao/jpa/LetterSoundAssessmentEventDaoJpa.java (1)
28-29: Same logging note as aboveSee previous comment regarding
timestamp.getTime()vs milliseconds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
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/NumberLearningEventDaoJpa.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/WordAssessmentEventDaoJpa.java(1 hunks)src/main/java/ai/elimu/dao/jpa/WordLearningEventDaoJpa.java(1 hunks)src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java(6 hunks)src/test/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelperTest.java(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java (1)
Learnt from: jo-elimu
PR: elimu-ai/webapp#2237
File: src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java:287-288
Timestamp: 2025-06-09T06:52:22.910Z
Learning: VideoLearningEvents in CsvAnalyticsExtractionHelper never used the "time" column name and always use "timestamp", unlike other event types (LetterSoundLearningEvent, WordAssessmentEvent, WordLearningEvent, StoryBookLearningEvent) which need conditional logic to handle both "time" (for versions < 3.4.0) and "timestamp" (for versions >= 3.4.0) column names. This is because VideoLearningEvents were introduced after the schema change.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: build (ubuntu-latest, 17)
- GitHub Check: build (windows-latest, 21)
- GitHub Check: build (macos-latest, 21)
- GitHub Check: build (windows-latest, 17)
- GitHub Check: build (macos-latest, 17)
- GitHub Check: build (ubuntu-latest, 21)
- GitHub Check: test_rest
- GitHub Check: test_rest
- GitHub Check: test_rest
- GitHub Check: test_rest
- GitHub Check: test_rest
- GitHub Check: test_rest
🔇 Additional comments (5)
pom-dependency-tree.txt (1)
1-1: Version bump looks fine – remember to sync release artefactsThe move to
2.6.58-SNAPSHOTis straightforward. Make sure the CHANGELOG and any deployment scripts reference the same version to avoid mismatched artefacts during CI/CD.src/main/java/ai/elimu/dao/jpa/WordAssessmentEventDaoJpa.java (1)
28-28: Verify timestamp logging format. The switch totimestamp.getTime()logs aDateobject instead of numeric milliseconds. Confirm this meets the epoch-second support goal; if not, revert totimestamp.getTimeInMillis().src/main/java/ai/elimu/dao/jpa/StoryBookLearningEventDaoJpa.java (1)
36-36: Verify and standardize timestamp logging. Loggingtimestamp.getTime()emits aDatestring rather than epoch milliseconds. Ensure this aligns with epoch-second requirements; otherwise usetimestamp.getTimeInMillis().src/main/java/ai/elimu/dao/jpa/WordLearningEventDaoJpa.java (1)
28-28: Verify timestamp logging behavior. Usingtimestamp.getTime()produces aDateobject string, not numeric milliseconds. Confirm this change supports epoch-second logging, or switch back totimestamp.getTimeInMillis().src/test/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelperTest.java (1)
37-37: Confirm updated timestamp assertions. All tests now expect timestamps truncated to second precision (multiples of 1000), matching the revised CSV extraction logic. The asserted values look correct.Also applies to: 57-57, 80-80, 102-102, 124-124, 146-146, 168-168
| long timestampInMillis = Long.valueOf(csvRecord.get(timestampColumnName).substring(0, 10)) * 1_000; | ||
| Calendar timestamp = Calendar.getInstance(TimeZone.getTimeZone("UTC")); | ||
| timestamp.setTimeInMillis(timestampInMillis); | ||
| letterSoundLearningEvent.setTimestamp(timestamp); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against short or malformed timestamp strings & remove duplication
Each extractor now does
long ts = Long.valueOf(csvRecord.get(col).substring(0, 10)) * 1_000;Issues:
- If the field has < 10 characters (e.g. empty or corrupted)
substringthrowsStringIndexOutOfBoundsException. - The same parsing logic is duplicated six times.
Proposed refactor (utility method + bounds check):
+ private static Calendar toUtcCalendarFromEpochSeconds(String raw) {
+ if (raw == null || raw.length() < 10) {
+ throw new IllegalArgumentException("Invalid epoch string: " + raw);
+ }
+ long millis = Long.parseLong(raw.substring(0, 10)) * 1_000L;
+ Calendar cal = Calendar.getInstance(TimeZone.getTimeZone("UTC"));
+ cal.setTimeInMillis(millis);
+ return cal;
+ }Then replace every block with:
- long timestampInMillis = Long.valueOf(csvRecord.get(col).substring(0, 10)) * 1_000;
- Calendar timestamp = Calendar.getInstance(TimeZone.getTimeZone("UTC"));
- timestamp.setTimeInMillis(timestampInMillis);
+ Calendar timestamp = toUtcCalendarFromEpochSeconds(csvRecord.get(col));This removes repetition and makes failures explicit.
Also applies to: 174-177, 240-243, 306-309, 369-372, 430-433
🤖 Prompt for AI Agents
In src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java around
lines 127 to 130, the code extracts a timestamp substring without checking its
length, risking StringIndexOutOfBoundsException if the string is shorter than 10
characters, and this parsing logic is duplicated multiple times. To fix this,
create a utility method that safely checks the length of the timestamp string
before extracting the substring and converting it to milliseconds, throwing a
clear exception if invalid. Replace all occurrences of the direct substring
parsing with calls to this utility method to remove duplication and improve
error handling. Apply the same refactor to lines 174-177, 240-243, 306-309,
369-372, and 430-433.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/main/java/ai/elimu/web/analytics/students/NumberLearningEventsCsvExportController.java (1)
64-67: Replace magic number withTimeUnitfor self-documentationThe
/ 1_000conversion is functionally correct, but the hard-coded literal obscures intent. UsingTimeUnit.MILLISECONDS.toSeconds(...)expresses the time-unit conversion explicitly and avoids magic numbers.-import java.io.StringWriter; +import java.io.StringWriter; +import java.util.concurrent.TimeUnit; ... - numberLearningEvent.getTimestamp().getTimeInMillis() / 1_000, + TimeUnit.MILLISECONDS.toSeconds( + numberLearningEvent.getTimestamp().getTimeInMillis()),src/main/java/ai/elimu/web/analytics/students/WordLearningEventsCsvExportController.java (1)
63-66: PreferTimeUnithelper for clarity and to avoid magic numbersThe intent—convert milliseconds to epoch‐seconds—is clear, but the magic constant
1_000invites accidental misuse later. Consider using the JDK’s built-in helper for self-documentation:- wordLearningEvent.getTimestamp().getTimeInMillis() / 1_000, + TimeUnit.MILLISECONDS.toSeconds(wordLearningEvent.getTimestamp().getTimeInMillis()),This makes the unit conversion explicit and removes the hard-coded divisor.
src/main/java/ai/elimu/web/analytics/students/LetterSoundLearningEventsCsvExportController.java (2)
63-66: PreferTimeUnitfor clearer millis→seconds conversionDividing by a magic constant (
1_000) works but obscures intent and risks accidental integer-division errors if the operand types ever change. UsingTimeUnitmakes the unit transformation explicit and self-documenting:+import java.util.concurrent.TimeUnit; ... - letterSoundLearningEvent.getTimestamp().getTimeInMillis() / 1_000, + TimeUnit.SECONDS.convert( + letterSoundLearningEvent.getTimestamp().getTimeInMillis(), + TimeUnit.MILLISECONDS),
60-62: Downgrade per-event log entries to DEBUG to avoid log flooding
log.infoinside a potentially large loop can spam application logs and impact I/O performance in production. Consider:- log.info("letterSoundLearningEvent.getId(): " + letterSoundLearningEvent.getId()); + log.debug("letterSoundLearningEvent.getId(): {}", letterSoundLearningEvent.getId());Also leverage parameterized logging to avoid unnecessary string concatenation.
src/main/java/ai/elimu/web/analytics/students/StoryBookLearningEventsCsvExportController.java (2)
67-70: PreferTimeUnitfor clearer, self-documenting time conversionDividing by
1_000works but hides intent and can invite copy-paste mistakes. UsingTimeUnit.MILLISECONDS.toSeconds(...)communicates why the division occurs and eliminates the magic number.+import java.util.concurrent.TimeUnit; ... - storyBookLearningEvent.getTimestamp().getTimeInMillis() / 1_000, + TimeUnit.MILLISECONDS.toSeconds(storyBookLearningEvent.getTimestamp().getTimeInMillis()),
38-46: Use parameterized SLF4J instead of string concatenation
log.info("student.getAndroidId(): " + student.getAndroidId());eagerly concatenates even when the log level is disabled and clutters code. Adopt placeholders:-log.info("student.getAndroidId(): " + student.getAndroidId()); +log.info("student.getAndroidId(): {}", student.getAndroidId());Apply similarly to other log statements in this controller.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/ai/elimu/web/analytics/students/LetterSoundAssessmentEventsCsvExportController.java(1 hunks)src/main/java/ai/elimu/web/analytics/students/LetterSoundLearningEventsCsvExportController.java(1 hunks)src/main/java/ai/elimu/web/analytics/students/NumberLearningEventsCsvExportController.java(1 hunks)src/main/java/ai/elimu/web/analytics/students/StoryBookLearningEventsCsvExportController.java(1 hunks)src/main/java/ai/elimu/web/analytics/students/VideoLearningEventsCsvExportController.java(1 hunks)src/main/java/ai/elimu/web/analytics/students/WordAssessmentEventsCsvExportController.java(1 hunks)src/main/java/ai/elimu/web/analytics/students/WordLearningEventsCsvExportController.java(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/main/java/ai/elimu/web/analytics/students/LetterSoundAssessmentEventsCsvExportController.java
- src/main/java/ai/elimu/web/analytics/students/WordAssessmentEventsCsvExportController.java
- src/main/java/ai/elimu/web/analytics/students/VideoLearningEventsCsvExportController.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test_rest
- GitHub Check: build (ubuntu-latest, 17)
🔇 Additional comments (3)
src/main/java/ai/elimu/web/analytics/students/WordLearningEventsCsvExportController.java (2)
45-54: Header label no longer reflects the new unitThe header still reads
"timestamp", which previously implied milliseconds. Now that the value is epoch seconds, downstream consumers might misinterpret it or double-convert. Recommend renaming to something explicit, e.g."timestamp_seconds", and updating any documentation/import code accordingly.
60-68: Potential collision with sub-second eventsSeveral learning events can legitimately share the same second. By truncating to seconds you lose ordering granularity and may generate duplicate timestamps. Verify that all downstream analytics tolerate this reduced precision (e.g. deduplication, sorting). If not, consider keeping milliseconds or appending a sequence column.
src/main/java/ai/elimu/web/analytics/students/StoryBookLearningEventsCsvExportController.java (1)
49-58: CSV header now misrepresents the unitThe field
timestampused to be milliseconds; it’s now seconds. Down-stream parsers or BI dashboards relying on the old unit will silently mis-interpret the value.Options:
- Rename the column (e.g.,
timestamp_seconds) in the header for explicitness.- Keep the name but bump a CSV schema/version and document the change.
Either way, ensure all consumers are updated before merging.
jo-elimu
deleted the
2252-prevent-duplicates-during-batch-processing-of-learning-events
branch
1 participant
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.