Conversation
jo-elimu
requested review from
AshishBagdane,
alexander-kuruvilla and
eymaal
and removed request for
a team
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2308 +/- ##
============================================
+ Coverage 16.48% 16.93% +0.44%
- Complexity 453 455 +2
============================================
Files 260 260
Lines 7711 7713 +2
Branches 892 894 +2
============================================
+ Hits 1271 1306 +35
+ Misses 6366 6325 -41
- Partials 74 82 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughNull and blank value checks were added to the extraction logic for NumberLearningEvent fields in the CSV analytics helper, preventing parsing errors when CSV fields are empty. A new unit test was introduced to verify correct handling of missing numberSymbol and numberId fields in CSV input. The project version was incremented. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as CsvAnalyticsExtractionHelperTest
participant Helper as CsvAnalyticsExtractionHelper
participant CSV as CSV File
Test->>Helper: extractNumberLearningEvents(CSV)
Helper->>CSV: Read next record
alt number_symbol or number_id is blank
Helper-->>Helper: Skip setting numberSymbol/numberId
else
Helper-->>Helper: Parse and set numberSymbol/numberId
end
Helper-->>Test: Return extracted NumberLearningEvent(s)
Estimated code review effort2 (~15 minutes) ✨ 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: 0
🧹 Nitpick comments (1)
src/test/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelperTest.java (1)
245-245: Update or remove outdated TODO comment.The TODO comment "// TODO: number learning events" should be updated or removed since the test method
testExtractNumberLearningEvents_v4000028()was just added above, providing coverage for number learning events.Apply this diff to remove the outdated TODO:
- // TODO: number learning events
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/test/resources/ai/elimu/util/csv/5b7c682a12ecbe2e_4000028_number-learning-events_2025-07-04.csvis excluded by!**/*.csv,!**/*.csv
📒 Files selected for processing (3)
pom-dependency-tree.txt(1 hunks)src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java(1 hunks)src/test/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelperTest.java(2 hunks)
🧠 Learnings (4)
📓 Common learnings
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.
Learnt from: jo-elimu
PR: elimu-ai/webapp#2269
File: src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java:80-88
Timestamp: 2025-06-20T05:28:00.962Z
Learning: In CsvAnalyticsExtractionHelper.java, the CSV columns for research_experiment and experiment_group contain integer values (0 or 1) that need to be mapped to enum ordinals by adding 1 to the CSV value (0 maps to ordinal 1, 1 maps to ordinal 2).
pom-dependency-tree.txt (1)
Learnt from: jo-elimu
PR: elimu-ai/webapp#0
File: :0-0
Timestamp: 2025-05-03T08:01:30.217Z
Learning: The elimu-ai/webapp repository has a dependency on ai.elimu:model (version 2.0.97) that provides shared model classes and enums. The Language enum should be imported from ai.elimu.model.v2.enums.Language instead of creating local duplicates.
src/test/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelperTest.java (4)
Learnt from: jo-elimu
PR: #2269
File: src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java:80-88
Timestamp: 2025-06-20T05:28:00.962Z
Learning: In CsvAnalyticsExtractionHelper.java, the CSV columns for research_experiment and experiment_group contain integer values (0 or 1) that need to be mapped to enum ordinals by adding 1 to the CSV value (0 maps to ordinal 1, 1 maps to ordinal 2).
Learnt from: jo-elimu
PR: #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.
Learnt from: venkatesh2k3
PR: #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.
Learnt from: jo-elimu
PR: elimu-ai/webapp#0
File: :0-0
Timestamp: 2025-05-03T08:01:30.217Z
Learning: The elimu-ai/webapp repository has a dependency on ai.elimu:model (version 2.0.97) that provides shared model classes and enums. The Language enum should be imported from ai.elimu.model.v2.enums.Language instead of creating local duplicates.
src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java (3)
Learnt from: jo-elimu
PR: #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.
Learnt from: jo-elimu
PR: #2269
File: src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java:80-88
Timestamp: 2025-06-20T05:28:00.962Z
Learning: In CsvAnalyticsExtractionHelper.java, the CSV columns for research_experiment and experiment_group contain integer values (0 or 1) that need to be mapped to enum ordinals by adding 1 to the CSV value (0 maps to ordinal 1, 1 maps to ordinal 2).
Learnt from: jo-elimu
PR: #2284
File: src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java:242-245
Timestamp: 2025-06-25T07:56:10.189Z
Learning: In Apache Commons CSV, the CSVRecord.isSet(String name) method returns true only if the column exists AND the value is not null. This means it already handles both column existence and non-null value validation, making it safe to use for conditional field processing without additional empty/blank checks.
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
Learnt from: jo-elimu
PR: elimu-ai/webapp#2269
File: src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java:80-88
Timestamp: 2025-06-20T05:28:00.962Z
Learning: In CsvAnalyticsExtractionHelper.java, the CSV columns for research_experiment and experiment_group contain integer values (0 or 1) that need to be mapped to enum ordinals by adding 1 to the CSV value (0 maps to ordinal 1, 1 maps to ordinal 2).
pom-dependency-tree.txt (1)
Learnt from: jo-elimu
PR: elimu-ai/webapp#0
File: :0-0
Timestamp: 2025-05-03T08:01:30.217Z
Learning: The elimu-ai/webapp repository has a dependency on ai.elimu:model (version 2.0.97) that provides shared model classes and enums. The Language enum should be imported from ai.elimu.model.v2.enums.Language instead of creating local duplicates.
src/test/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelperTest.java (4)
Learnt from: jo-elimu
PR: #2269
File: src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java:80-88
Timestamp: 2025-06-20T05:28:00.962Z
Learning: In CsvAnalyticsExtractionHelper.java, the CSV columns for research_experiment and experiment_group contain integer values (0 or 1) that need to be mapped to enum ordinals by adding 1 to the CSV value (0 maps to ordinal 1, 1 maps to ordinal 2).
Learnt from: jo-elimu
PR: #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.
Learnt from: venkatesh2k3
PR: #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.
Learnt from: jo-elimu
PR: elimu-ai/webapp#0
File: :0-0
Timestamp: 2025-05-03T08:01:30.217Z
Learning: The elimu-ai/webapp repository has a dependency on ai.elimu:model (version 2.0.97) that provides shared model classes and enums. The Language enum should be imported from ai.elimu.model.v2.enums.Language instead of creating local duplicates.
src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java (3)
Learnt from: jo-elimu
PR: #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.
Learnt from: jo-elimu
PR: #2269
File: src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java:80-88
Timestamp: 2025-06-20T05:28:00.962Z
Learning: In CsvAnalyticsExtractionHelper.java, the CSV columns for research_experiment and experiment_group contain integer values (0 or 1) that need to be mapped to enum ordinals by adding 1 to the CSV value (0 maps to ordinal 1, 1 maps to ordinal 2).
Learnt from: jo-elimu
PR: #2284
File: src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java:242-245
Timestamp: 2025-06-25T07:56:10.189Z
Learning: In Apache Commons CSV, the CSVRecord.isSet(String name) method returns true only if the column exists AND the value is not null. This means it already handles both column existence and non-null value validation, making it safe to use for conditional field processing without additional empty/blank checks.
⏰ 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). (12)
- GitHub Check: build (macos-latest, 17)
- GitHub Check: build (ubuntu-latest, 21)
- GitHub Check: build (windows-latest, 17)
- GitHub Check: build (ubuntu-latest, 17)
- GitHub Check: build (macos-latest, 21)
- GitHub Check: build (windows-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 (4)
src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java (1)
341-349: LGTM! Proper null/blank validation added for optional fields.The addition of
StringUtils.isNotBlank()checks before parsing and settingnumberSymbolandnumberIdcorrectly preventsNumberFormatExceptionwhen these CSV fields contain empty or null values. This implementation is consistent with the existing pattern used for other optional fields throughout the file.pom-dependency-tree.txt (1)
1-1: Appropriate version increment for bug fix.The version increment from
2.6.103-SNAPSHOTto2.6.105-SNAPSHOTis appropriate for this bug fix addressing the number format exception issue.src/test/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelperTest.java (2)
20-20: LGTM! Import added for the new test.The import for
NumberLearningEventis correctly added to support the new test method.
193-216: Excellent test coverage for the bug fix.This test method comprehensively validates the fix by:
- Testing extraction from a CSV file with blank
numberSymbolandnumberIdfields- Asserting that these fields remain null when blank (preventing the original NumberFormatException)
- Validating that all other fields are correctly extracted
The test correctly demonstrates that the extraction logic now handles missing optional fields gracefully.
jo-elimu
deleted the
2307-numberformatexception-in-numberlearningeventimportscheduler
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.