-
-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(dao) : LetterSound #1677 #1725
Conversation
WalkthroughThe overall change involves renaming the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LetterSoundUsageCountScheduler
participant WordDao
participant LetterSoundDao
User->>LetterSoundUsageCountScheduler: Trigger usage count update
LetterSoundUsageCountScheduler->>WordDao: Retrieve all Words
WordDao-->>LetterSoundUsageCountScheduler: Return Word list
LetterSoundUsageCountScheduler->>LetterSoundDao: Retrieve all LetterSounds
LetterSoundDao-->>LetterSoundUsageCountScheduler: Return LetterSound list
LetterSoundUsageCountScheduler->>LetterSoundDao: Update usage counts
LetterSoundDao-->>LetterSoundUsageCountScheduler: Confirm update
LetterSoundUsageCountScheduler-->>User: Usage counts updated
Assessment against linked issues
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional context usedLearnings (1)
Additional comments not posted (6)
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Outside diff range, codebase verification and nitpick comments (2)
src/main/java/ai/elimu/tasks/LetterSoundUsageCountScheduler.java (1)
19-22
: Use a more specific logger name.Using a more specific logger name can help in identifying logs from this class.
- private Logger logger = LogManager.getLogger(); + private Logger logger = LogManager.getLogger(LetterSoundUsageCountScheduler.class);src/main/java/ai/elimu/web/content/letter_sound/LetterSoundCsvExportController.java (1)
Line range hint
55-79
: Optimize JSON array creation.Consider using Java streams to simplify the creation of JSON arrays.
JSONArray letterIdsJsonArray = new JSONArray(); letterSound.getLetters().forEach(letter -> letterIdsJsonArray.put(letter.getId())); JSONArray letterTextsJsonArray = new JSONArray(); letterSound.getLetters().forEach(letter -> letterTextsJsonArray.put(letter.getText())); JSONArray soundIdsJsonArray = new JSONArray(); letterSound.getSounds().forEach(sound -> soundIdsJsonArray.put(sound.getId())); JSONArray soundValuesIpaJsonArray = new JSONArray(); letterSound.getSounds().forEach(sound -> soundValuesIpaJsonArray.put(sound.getValueIpa()));
src/main/java/ai/elimu/tasks/LetterSoundUsageCountScheduler.java
Outdated
Show resolved
Hide resolved
src/main/java/ai/elimu/tasks/LetterSoundUsageCountScheduler.java
Outdated
Show resolved
Hide resolved
src/main/java/ai/elimu/web/content/letter_sound/LetterSoundCsvExportController.java
Show resolved
Hide resolved
src/main/java/ai/elimu/web/content/peer_review/LetterSoundPeerReviewEventCreateController.java
Show resolved
Hide resolved
src/main/java/ai/elimu/web/content/peer_review/LetterSoundPeerReviewEventCreateController.java
Show resolved
Hide resolved
|
src/main/java/ai/elimu/web/content/peer_review/LetterSoundPeerReviewEventCreateController.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
@jo-elimu , can you please review and approve my PR |
src/main/java/ai/elimu/tasks/LetterSoundUsageCountScheduler.java
Outdated
Show resolved
Hide resolved
src/main/java/ai/elimu/tasks/LetterSoundUsageCountScheduler.java
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@SnehaHS65 I applied some suggestions: 40cf0e1 So please fetch the latest changes before you continue working on the branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
@jo-elimu , Can you please check now? |
@jo-elimu , now all the files should be in sync with the Main branch, can you please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
src/main/java/ai/elimu/tasks/LetterSoundUsageCountScheduler.java (1)
41-46
: Optimize logging within loops.Logging within loops can be expensive. Consider reducing the log level or aggregating logs.
- logger.debug("word.getText(): " + word.getText()); + logger.trace("word.getText(): " + word.getText());
Thank you, @SnehaHS65. I will test your code changes later today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested webapp on localhost, and this seems to be running well 👍
The jpa-schema-export.sql
has not been updated to reflect the changes. Needs to be re-generated by running mvn jetty:run
.
src/main/java/ai/elimu/tasks/LetterSoundUsageCountScheduler.java
Outdated
Show resolved
Hide resolved
src/main/java/ai/elimu/tasks/LetterSoundUsageCountScheduler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SnehaHS65 The DB migration script also needs to include renaming of the LetterSoundCorrespondence_id
column, because I don't think that's handled automatically by Hibernate.
Other than that, all your changes look very good to me 🙂
…into 1677-LetterSound
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @SnehaHS65. I think these changes look good. Please merge, so that we can create a release and try out the migration script on the databases in production (Hindi + Tagalog).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
src/main/resources/db/migration/2004022.sql (1)
1-2
: Clarify the version number.The version number in the SQL file should be updated to
2.4.22-SNAPSHOT
to align with the versioning strategy in thepom.xml
file.
- File:
src/main/resources/db/migration/2004022.sql
- Line: 1
Update:
# 2.4.22-SNAPSHOT
Analysis chain
Clarify the version number.
The comment indicates the version as
2.4.22
. Ensure that this version number aligns with the versioning strategy and thepom.xml
file.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the version number in the `pom.xml` file. # Test: Search for the version number. Expect: The version number should be `2.4.22`. rg --type xml -A 5 $'<version>2.4.22</version>'Length of output: 48
Script:
#!/bin/bash # Description: List all version tags in the `pom.xml` file to understand the versioning strategy. # Test: Search for all version tags in the `pom.xml` file. rg --type xml '<version>' -A 1Length of output: 4922
@jo-elimu , Merged!! |
closes #1677
The version of webapp in pom.xml is still 2.4.11 and the db migration script in this name already exists. If you could change the version, I can create db script.