chore: #1677 Rename LetterSoundCorrespondence to LetterSound #1682
Conversation
WalkthroughThe changes involve renaming the Changes
Possibly related issues
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (22)
Files not summarized due to errors (1)
Files skipped from review due to trivial changes (2)
Additional context usedLearnings (17)
Additional comments not posted (39)
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 Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
src/main/java/ai/elimu/web/content/peer_review/LetterSoundPeerReviewEventCreateController.java
Show resolved
Hide resolved
src/main/java/ai/elimu/dao/jpa/LetterSoundContributionEventDaoJpa.java
Outdated
Show resolved
Hide resolved
src/main/java/ai/elimu/dao/jpa/LetterSoundPeerReviewEventDaoJpa.java
Outdated
Show resolved
Hide resolved
src/main/java/ai/elimu/logic/converters/StringToLetterSoundCorrespondenceConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/ai/elimu/model/contributor/LetterSoundCorrespondenceContributionEvent.java
Show resolved
Hide resolved
src/main/java/ai/elimu/tasks/LetterSoundCorrespondenceUsageCountScheduler.java
Outdated
Show resolved
Hide resolved
|
@venkatesh2k3 , I have changed in all the java files as you mentioned, should I change in webapp folder(in jsp files) as well? Let me know and I can commit |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/main/java/ai/elimu/tasks/WordUsageCountScheduler.java (1)
80-82: Clarify the warning message.The warning message should reflect the new terminology "LetterSound" instead of "Letter-sound correspondences."
- logger.warn("Letter-sound correspondences not yet added. Skipping usage count update for word..."); + logger.warn("Letter sounds not yet added. Skipping usage count update for word...");
There was a problem hiding this comment.
@SnehaHS65 Thank you for opening this pull request 🙂
I will have to ask you to revert the second commit in your pull request: 35093ef
This is because:
- The scope for the changes should only be for
LetterSoundCorrespondence.java, and not for additional entities. (The number of code changes is already high in the initial commit, so we shouldn't add additional complexity.) - The changes are touching the database layer, which means that we will also have to create a database migration script for your changes. See DB_MIGRATION.md.
- A database schema export file is missing. You can generate it by running
mvn jetty:runto launch the webapp locally. You will then see code changes in thejpa-schema-export.sqlfile, and those changes should also be included in your PR.
Note
The reason why we need to create a DB migration script is because we already have a MySQL database deployed in production. So anytime we make changes to the database structure, we have to write SQL scripts for updating the existing database. See DB_MIGRATION.md.
Tip
When you create the database migration script, you can look at the changes in jpa-schema-export.sql to know which database tables/columns to modify.
As this PR is already quite large, you might want to split it up into smaller parts to make sure that it doesn't break the database structure. For example one PR for only renaming the LetterSoundCorrespondence database table, and another PR for renaming the LetterSoundCorrespondence_id references another database table, etc.
As you can see in the jpa-schema-export.sql diff, it gets quite complicated with all the changes to the database column names, primary keys, and foreign key constraints. And that is why I recommend we reduce the size of your PR.
|
@jo-elimu , Can you review the latest commit now, I refactored |
There was a problem hiding this comment.
Actionable comments posted: 24
Outside diff range and nitpick comments (8)
src/main/java/ai/elimu/dao/LetterSoundPeerReviewEventDao.java (1)
Line range hint
18-18:
Update method signature to reflect new naming convention.The method signature should use
LetterSoundPeerReviewEventinstead ofLetterSoundCorrespondencePeerReviewEvent.- List<LetterSoundCorrespondencePeerReviewEvent> readAll(LetterSoundCorrespondenceContributionEvent letterSoundCorrespondenceContributionEvent) throws DataAccessException; + List<LetterSoundPeerReviewEvent> readAll(LetterSoundContributionEvent letterSoundContributionEvent) throws DataAccessException;src/main/java/ai/elimu/dao/jpa/LetterSoundDaoJpa.java (1)
Line range hint
15-27: Update variable names for consistency.The method
readcorrectly usesLetterSound, but the variable names still referenceletterSoundCorrespondence. Update variable names for consistency.- String letterSoundCorrespondenceLetters = letters.stream().map(Letter::getText).collect(Collectors.joining()); - String letterSoundCorrespondenceSounds = sounds.stream().map(Sound::getValueIpa).collect(Collectors.joining()); - for (LetterSound letterSoundCorrespondence : readAllOrderedByUsage()) { - String lettersAsString = letterSoundCorrespondence.getLetters().stream().map(Letter::getText).collect(Collectors.joining()); - String soundsAsString = letterSoundCorrespondence.getSounds().stream().map(Sound::getValueIpa).collect(Collectors.joining()); - if (lettersAsString.equals(letterSoundCorrespondenceLetters) && soundsAsString.equals(letterSoundCorrespondenceSounds)) { - return letterSoundCorrespondence; + String letterSoundLetters = letters.stream().map(Letter::getText).collect(Collectors.joining()); + String letterSoundSounds = sounds.stream().map(Sound::getValueIpa).collect(Collectors.joining()); + for (LetterSound letterSound : readAllOrderedByUsage()) { + String lettersAsString = letterSound.getLetters().stream().map(Letter::getText).collect(Collectors.joining()); + String soundsAsString = letterSound.getSounds().stream().map(Sound::getValueIpa).collect(Collectors.joining()); + if (lettersAsString.equals(letterSoundLetters) && soundsAsString.equals(letterSoundSounds)) { + return letterSound;src/main/java/ai/elimu/tasks/SoundUsageCountScheduler.java (1)
Line range hint
45-54: Update variable names for consistency.The method logic correctly uses
LetterSound, but the variable names still referenceletterSoundCorrespondence. Update variable names for consistency.- for (LetterSound letterSoundCorrespondence : word.getLetterSoundCorrespondences()) { - for (Sound sound : letterSoundCorrespondence.getSounds()) { - soundFrequencyMap.put(sound.getId(), soundFrequencyMap.getOrDefault(sound.getId(), 0) + letterSoundCorrespondence.getUsageCount()); + for (LetterSound letterSound : word.getLetterSounds()) { + for (Sound sound : letterSound.getSounds()) { + soundFrequencyMap.put(sound.getId(), soundFrequencyMap.getOrDefault(sound.getId(), 0) + letterSound.getUsageCount());src/main/java/ai/elimu/dao/jpa/LetterSoundContributionEventDaoJpa.java (1)
Line range hint
22-29: Update variable names for consistency.The method
readAllcorrectly usesLetterSound, but the variable names still referenceletterSoundCorrespondence. Update variable names for consistency.- public List<LetterSoundCorrespondenceContributionEvent> readAll(LetterSound letterSound) throws DataAccessException { + public List<LetterSoundCorrespondenceContributionEvent> readAll(LetterSound letterSound) throws DataAccessException { return em.createQuery( "SELECT e " + "FROM LetterSoundCorrespondenceContributionEvent e " + - "WHERE e.letterSoundCorrespondence = :letterSoundCorrespondence " + + "WHERE e.letterSound = :letterSound " + "ORDER BY e.time DESC") - .setParameter("letterSoundCorrespondence", letterSound) + .setParameter("letterSound", letterSound) .getResultList(); }src/main/java/ai/elimu/tasks/LetterSoundCorrespondenceUsageCountScheduler.java (1)
44-51: Rename variable for consistency.Consider renaming
letterSoundCorrespondenceFrequencyMaptoletterSoundFrequencyMapto reflect the updated entity name.- Map<Long, Integer> letterSoundCorrespondenceFrequencyMap = new HashMap<>(); + Map<Long, Integer> letterSoundFrequencyMap = new HashMap<>();src/main/java/ai/elimu/dao/jpa/LetterSoundPeerReviewEventDaoJpa.java (1)
Line range hint
27-34: Update SQL query and parameter names for consistency.The SQL query and parameter names should be updated to reflect the new entity name.
- "WHERE event.letterSoundCorrespondenceContributionEvent.letterSoundCorrespondence = :letterSoundCorrespondence " + + "WHERE event.letterSoundCorrespondenceContributionEvent.letterSound = :letterSound " + - .setParameter("letterSoundCorrespondence", letterSound) + .setParameter("letterSound", letterSound)src/main/java/ai/elimu/web/content/letter_sound/LetterSoundPeerReviewsController.java (1)
62-62: Rename variable for consistency.Consider renaming
letterSoundPeerReviewEventstoletterSoundReviewEventsto reflect the updated entity name.- List<LetterSoundCorrespondencePeerReviewEvent> letterSoundPeerReviewEvents = letterSoundPeerReviewEventDao.readAll(mostRecentLetterSoundContributionEvent, contributor); + List<LetterSoundCorrespondencePeerReviewEvent> letterSoundReviewEvents = letterSoundPeerReviewEventDao.readAll(mostRecentLetterSoundContributionEvent, contributor);src/main/java/ai/elimu/web/content/word/WordCsvExportController.java (1)
62-62: Rename variable for consistency.Consider renaming
letterSoundCorrespondenceJsonObjecttoletterSoundJsonObjectto reflect the updated entity name.- JSONObject letterSoundCorrespondenceJsonObject = new JSONObject(); - letterSoundCorrespondenceJsonObject.put("id", letterSoundCorrespondence.getId()); - letterSoundCorrespondenceJsonObject.put("letters", lettersArray); - letterSoundCorrespondenceJsonObject.put("sounds", soundsArray); - letterSoundCorrespondenceJsonObject.put("usageCount", letterSoundCorrespondence.getUsageCount()); - letterSoundCorrespondencesJsonArray.put(index, letterSoundCorrespondenceJsonObject); + JSONObject letterSoundJsonObject = new JSONObject(); + letterSoundJsonObject.put("id", letterSoundCorrespondence.getId()); + letterSoundJsonObject.put("letters", lettersArray); + letterSoundJsonObject.put("sounds", soundsArray); + letterSoundJsonObject.put("usageCount", letterSoundCorrespondence.getUsageCount()); + letterSoundCorrespondencesJsonArray.put(index, letterSoundJsonObject);
| @@ -10,13 +11,13 @@ public class LetterSoundCorrespondenceContributionEvent extends ContributionEven | |||
|
|
|||
| @NotNull | |||
| @ManyToOne | |||
| private LetterSoundCorrespondence letterSoundCorrespondence; | |||
| private LetterSound letterSoundCorrespondence; | |||
There was a problem hiding this comment.
Update property name to reflect new naming convention.
The property name should be updated to letterSound.
- private LetterSound letterSoundCorrespondence;
+ private LetterSound letterSound;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.
| private LetterSound letterSoundCorrespondence; | |
| private LetterSound letterSound; |
| public LetterSound getLetterSoundCorrespondence() { | ||
| return letterSoundCorrespondence; | ||
| } |
There was a problem hiding this comment.
Update getter method to reflect new naming convention.
The getter method should be updated to getLetterSound.
- public LetterSound getLetterSoundCorrespondence() {
- return letterSoundCorrespondence;
+ public LetterSound getLetterSound() {
+ return letterSound;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.
| public LetterSound getLetterSoundCorrespondence() { | |
| return letterSoundCorrespondence; | |
| } | |
| public LetterSound getLetterSound() { | |
| return letterSound; |
| @@ -1,6 +1,7 @@ | |||
| package ai.elimu.model.contributor; | |||
|
|
|||
| import ai.elimu.model.content.LetterSoundCorrespondence; | |||
| import ai.elimu.model.content.LetterSound; | |||
There was a problem hiding this comment.
Update import statement to reflect new naming convention.
The import statement should use LetterSound instead of LetterSoundCorrespondence.
- import ai.elimu.model.content.LetterSoundCorrespondence;
+ import ai.elimu.model.content.LetterSound;Committable suggestion was skipped due to low confidence.
| public void setLetterSoundCorrespondence(LetterSound letterSoundCorrespondence) { | ||
| this.letterSoundCorrespondence = letterSoundCorrespondence; | ||
| } |
There was a problem hiding this comment.
Update setter method to reflect new naming convention.
The setter method should be updated to setLetterSound.
- public void setLetterSoundCorrespondence(LetterSound letterSoundCorrespondence) {
- this.letterSoundCorrespondence = letterSoundCorrespondence;
+ public void setLetterSound(LetterSound letterSound) {
+ this.letterSound = letterSound;
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.
| public void setLetterSoundCorrespondence(LetterSound letterSoundCorrespondence) { | |
| this.letterSoundCorrespondence = letterSoundCorrespondence; | |
| } | |
| public void setLetterSound(LetterSound letterSound) { | |
| this.letterSound = letterSound; | |
| } |
| @@ -1,25 +1,25 @@ | |||
| package ai.elimu.logic.converters; | |||
|
|
|||
| import org.apache.commons.lang.StringUtils; | |||
| import ai.elimu.model.content.LetterSoundCorrespondence; | |||
| import ai.elimu.model.content.LetterSound; | |||
There was a problem hiding this comment.
Update import statement to reflect new naming convention.
The import statement should use LetterSound instead of LetterSoundCorrespondence.
- import ai.elimu.model.content.LetterSoundCorrespondence;
+ import ai.elimu.model.content.LetterSound;
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.
| import ai.elimu.model.content.LetterSound; | |
| import ai.elimu.model.content.LetterSound; |
| @@ -79,7 +76,7 @@ public static List<LetterSoundCorrespondence> getLetterSoundCorrespondencesFromC | |||
| for (CSVRecord csvRecord : csvParser) { | |||
| logger.info("csvRecord: " + csvRecord); | |||
|
|
|||
| LetterSoundCorrespondence letterSoundCorrespondence = new LetterSoundCorrespondence(); | |||
| LetterSound letterSoundCorrespondence = new LetterSound(); | |||
There was a problem hiding this comment.
Variable name should be updated for consistency.
Update the variable name letterSoundCorrespondence to letterSound for consistency with the new class name.
- LetterSound letterSoundCorrespondence = new LetterSound();
+ LetterSound letterSound = new LetterSound();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.
| LetterSound letterSoundCorrespondence = new LetterSound(); | |
| LetterSound letterSound = new LetterSound(); |
| logger.info("getLetterSoundCorrespondencesFromCsvBackup"); | ||
|
|
||
| List<LetterSoundCorrespondence> letterSoundCorrespondences = new ArrayList<>(); | ||
| List<LetterSound> letterSoundCorrespondences = new ArrayList<>(); |
There was a problem hiding this comment.
Variable name should be updated for consistency.
Update the variable name letterSoundCorrespondences to letterSounds for consistency with the new class name.
- List<LetterSound> letterSoundCorrespondences = new ArrayList<>();
+ List<LetterSound> letterSounds = new ArrayList<>();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.
| List<LetterSound> letterSoundCorrespondences = new ArrayList<>(); | |
| List<LetterSound> letterSounds = new ArrayList<>(); |
| @@ -174,7 +171,7 @@ public static List<Word> getWordsFromCsvBackup(File csvFile, LetterDao letterDao | |||
| Sound sound = soundDao.readByValueIpa(soundsJsonArray.getString(j)); | |||
| sounds.add(sound); | |||
| } | |||
| LetterSoundCorrespondence letterSoundCorrespondence = letterSoundDao.read(letters, sounds); | |||
| LetterSound letterSoundCorrespondence = letterSoundDao.read(letters, sounds); | |||
There was a problem hiding this comment.
Variable name should be updated for consistency.
Update the variable name letterSoundCorrespondence to letterSound for consistency with the new class name.
- LetterSound letterSoundCorrespondence = letterSoundDao.read(letters, sounds);
- logger.info("letterSoundCorrespondence.getId(): " + letterSoundCorrespondence.getId());
- letterSoundCorrespondences.add(letterSoundCorrespondence);
+ LetterSound letterSound = letterSoundDao.read(letters, sounds);
+ logger.info("letterSound.getId(): " + letterSound.getId());
+ letterSounds.add(letterSound);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.
| LetterSound letterSoundCorrespondence = letterSoundDao.read(letters, sounds); | |
| LetterSound letterSound = letterSoundDao.read(letters, sounds); | |
| logger.info("letterSound.getId(): " + letterSound.getId()); | |
| letterSounds.add(letterSound); |
| @@ -56,10 +53,10 @@ public class CsvContentExtractionHelper { | |||
| /** | |||
| * For information on how the CSV files were generated, see {@link LetterSoundCsvExportController#handleRequest}. | |||
| */ | |||
| public static List<LetterSoundCorrespondence> getLetterSoundCorrespondencesFromCsvBackup(File csvFile, LetterDao letterDao, SoundDao soundDao, LetterSoundDao letterSoundDao) { | |||
| public static List<LetterSound> getLetterSoundCorrespondencesFromCsvBackup(File csvFile, LetterDao letterDao, SoundDao soundDao, LetterSoundDao letterSoundDao) { | |||
There was a problem hiding this comment.
Method name should be updated for consistency.
Update the method name getLetterSoundCorrespondencesFromCsvBackup to getLetterSoundsFromCsvBackup for consistency with the new class name.
- public static List<LetterSound> getLetterSoundCorrespondencesFromCsvBackup(File csvFile, LetterDao letterDao, SoundDao soundDao, LetterSoundDao letterSoundDao) {
+ public static List<LetterSound> getLetterSoundsFromCsvBackup(File csvFile, LetterDao letterDao, SoundDao soundDao, LetterSoundDao letterSoundDao) {
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.
| public static List<LetterSound> getLetterSoundCorrespondencesFromCsvBackup(File csvFile, LetterDao letterDao, SoundDao soundDao, LetterSoundDao letterSoundDao) { | |
| public static List<LetterSound> getLetterSoundsFromCsvBackup(File csvFile, LetterDao letterDao, SoundDao soundDao, LetterSoundDao letterSoundDao) { |
| @@ -158,7 +155,7 @@ public static List<Word> getWordsFromCsvBackup(File csvFile, LetterDao letterDao | |||
|
|
|||
| JSONArray letterSoundCorrespondencesJsonArray = new JSONArray(csvRecord.get("letter_sound_correspondences")); | |||
| logger.info("letterSoundCorrespondencesJsonArray: " + letterSoundCorrespondencesJsonArray); | |||
| List<LetterSoundCorrespondence> letterSoundCorrespondences = new ArrayList<>(); | |||
| List<LetterSound> letterSoundCorrespondences = new ArrayList<>(); | |||
There was a problem hiding this comment.
Variable name should be updated for consistency.
Update the variable name letterSoundCorrespondences to letterSounds for consistency with the new class name.
- List<LetterSound> letterSoundCorrespondences = new ArrayList<>();
+ List<LetterSound> letterSounds = new ArrayList<>();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.
| List<LetterSound> letterSoundCorrespondences = new ArrayList<>(); | |
| List<LetterSound> letterSounds = new ArrayList<>(); |
|
Hi @jo-elimu , @nya-elimu , @venkatesh2k3 , can you please look at the latest changes that I did to rename LetterSoundCorrespondence to LetterSound. This is the first PR to just rename the LetterSoundCorrespondence database table as suggested by @jo-elimu. If you approve this, I can raise one more PR for renaming the LetterSoundCorrespondence_id references. |
@SnehaHS65 If you look at your changes in This is why I suggested above that we split #1677 into smaller pull requests to make each database migration less complicated. I've created an example here: #1713. This pull request only makes one small change (renaming from |
@jo-elimu ,So there are ContributionEvent and PeerReviewEvent which uses this letterSoundCorrespondence, I will now only edit for ContributionEvent first and commit? |
@SnehaHS65 Yes, that's right 🙂 I suggest you close this pull request, and open a new pull request only for renaming the So in total we'll end up with 6 smaller PRs for closing #1677. |
SnehaHS65
deleted the
chore-#1677-rename-LetterSoundCorrespondence-to-LetterSound
branch
This pull request addresses issue #1677 by renaming the class LetterSoundCorrespondence to LetterSound and updating all references throughout the codebase.