Sort by ID during CSV export #1999
Conversation
Fixes elimu-ai#1997 Add methods to sort by ID in various DAO interfaces and implementations. * **DAO Interfaces:** - Add `readAllOrderedById` method to `EmojiDao`, `ImageDao`, `LetterDao`, `LetterSoundDao`, `NumberDao`, `SoundDao`, `StoryBookDao`, and `WordDao` interfaces. * **DAO Implementations:** - Implement `readAllOrderedById` method in `EmojiDaoJpa`, `ImageDaoJpa`, `LetterDaoJpa`, `LetterSoundDaoJpa`, `NumberDaoJpa`, `SoundDaoJpa`, `StoryBookDaoJpa`, and `WordDaoJpa` classes. * **CSV Export Controllers:** - Update `EmojiCsvExportController` to use `emojiDao.readAllOrderedById`. - Update `LetterSoundCsvExportController` to use `letterSoundDao.readAllOrderedById`. - Update `LetterCsvExportController` to use `letterDao.readAllOrderedById`. - Update `NumberCsvExportController` to use `numberDao.readAllOrderedById`. - Update `SoundCsvExportController` to use `soundDao.readAllOrderedById`. - Update `StoryBookCsvExportController` to use `storyBookDao.readAllOrderedById`. - Update `WordCsvExportController` to use `wordDao.readAllOrderedById`.
AbaSheger
requested review from
nya-elimu,
tomaszsmy and
alexander-kuruvilla
WalkthroughThis change introduces a new method, Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as CSV Export Controller
participant Dao as DAO Interface
participant DaoJpa as DAO JPA Implementation
participant DB as Database
Controller->>Dao: Call readAllOrderedById()
Dao->>DaoJpa: Delegate call to implementation
DaoJpa->>DB: Execute JPQL query ordering by id
DB-->>DaoJpa: Return ordered entity list
DaoJpa-->>Dao: Return ordered entity list
Dao-->>Controller: Return ordered entity list
Controller->>Controller: Generate CSV output from list
Suggested reviewers
✨ Finishing Touches
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 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 (8)
src/main/java/ai/elimu/dao/NumberDao.java (1)
14-15: Add Javadoc documentation for the new method.Please add Javadoc documentation to clarify the difference between
readAllOrderedById()andreadAllOrdered(). This will help implementers and consumers understand when to use each method.+ /** + * Retrieves all Number entities ordered by their ID. + * + * @return List of all Number entities sorted by ID in ascending order + * @throws DataAccessException if a data access error occurs + */ List<Number> readAllOrderedById() throws DataAccessException;src/main/java/ai/elimu/dao/LetterDao.java (1)
15-16: Add comprehensive documentation for ordering methods.With three different ordering methods (
readAllOrdered(),readAllOrderedByUsage(), andreadAllOrderedById()), clear documentation is essential. Please add Javadoc to explain the specific use case and ordering criteria for each method.+ /** + * Retrieves all Letter entities ordered by their ID. + * + * @return List of all Letter entities sorted by ID in ascending order + * @throws DataAccessException if a data access error occurs + */ List<Letter> readAllOrderedById() throws DataAccessException;Consider also updating documentation for the existing ordering methods to clearly differentiate their purposes.
src/main/java/ai/elimu/dao/EmojiDao.java (1)
18-19: Add Javadoc documentation for consistency.The interface follows good documentation practices (e.g., for
readAllLabeled()). Please maintain this consistency by adding Javadoc for the new method.+ /** + * Retrieves all Emoji entities ordered by their ID. + * + * @return List of all Emoji entities sorted by ID in ascending order + * @throws DataAccessException if a data access error occurs + */ List<Emoji> readAllOrderedById() throws DataAccessException;src/main/java/ai/elimu/dao/ImageDao.java (1)
20-21: Add Javadoc documentation for consistency.The interface follows good documentation practices (e.g., for
readAllLabeled()). Please maintain this consistency by adding Javadoc for the new method.+ /** + * Retrieves all Image entities ordered by their ID. + * + * @return List of all Image entities sorted by ID in ascending order + * @throws DataAccessException if a data access error occurs + */ List<Image> readAllOrderedById() throws DataAccessException;src/main/java/ai/elimu/dao/StoryBookDao.java (1)
20-20: Consider removing or documenting the "Pd3b6" comment.The comment "Pd3b6" at the end of the line appears to be a reference or identifier, but its purpose is unclear. Consider either removing it or documenting its significance.
src/main/java/ai/elimu/dao/jpa/LetterDaoJpa.java (1)
45-52: Consider adding Javadoc to clarify ordering strategy.The implementation is correct, but since this class has multiple ordering methods (by text, usage, and now ID), it would be helpful to document the specific use case for ID-based ordering.
+ /** + * Retrieves all letters ordered by their database ID. + * This ordering is primarily used for consistent CSV exports. + * + * @return List of letters ordered by ID + * @throws DataAccessException if a database access error occurs + */ @Override public List<Letter> readAllOrderedById() throws DataAccessException {src/main/java/ai/elimu/dao/jpa/ImageDaoJpa.java (1)
49-56: LGTM! Consider index optimization for large datasets.The implementation is consistent with other DAOs. Since this handles multimedia content which could grow to a large dataset, ensure there's an index on the
idcolumn for optimal query performance.src/main/java/ai/elimu/web/content/storybook/StoryBookCsvExportController.java (1)
90-91: Address TODO comment about code relocation.There's an unrelated TODO comment about moving the image content removal code block to
JpaToGsonConverter. This would improve code organization and maintainability.Would you like me to help implement this refactoring in a separate PR?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/main/java/ai/elimu/dao/EmojiDao.java(1 hunks)src/main/java/ai/elimu/dao/ImageDao.java(1 hunks)src/main/java/ai/elimu/dao/LetterDao.java(1 hunks)src/main/java/ai/elimu/dao/LetterSoundDao.java(1 hunks)src/main/java/ai/elimu/dao/NumberDao.java(1 hunks)src/main/java/ai/elimu/dao/SoundDao.java(1 hunks)src/main/java/ai/elimu/dao/StoryBookDao.java(1 hunks)src/main/java/ai/elimu/dao/WordDao.java(1 hunks)src/main/java/ai/elimu/dao/jpa/EmojiDaoJpa.java(1 hunks)src/main/java/ai/elimu/dao/jpa/ImageDaoJpa.java(1 hunks)src/main/java/ai/elimu/dao/jpa/LetterDaoJpa.java(1 hunks)src/main/java/ai/elimu/dao/jpa/LetterSoundDaoJpa.java(1 hunks)src/main/java/ai/elimu/dao/jpa/NumberDaoJpa.java(1 hunks)src/main/java/ai/elimu/dao/jpa/SoundDaoJpa.java(1 hunks)src/main/java/ai/elimu/dao/jpa/StoryBookDaoJpa.java(1 hunks)src/main/java/ai/elimu/dao/jpa/WordDaoJpa.java(1 hunks)src/main/java/ai/elimu/web/content/emoji/EmojiCsvExportController.java(1 hunks)src/main/java/ai/elimu/web/content/letter/LetterCsvExportController.java(1 hunks)src/main/java/ai/elimu/web/content/letter_sound/LetterSoundCsvExportController.java(1 hunks)src/main/java/ai/elimu/web/content/number/NumberCsvExportController.java(1 hunks)src/main/java/ai/elimu/web/content/sound/SoundCsvExportController.java(1 hunks)src/main/java/ai/elimu/web/content/storybook/StoryBookCsvExportController.java(1 hunks)src/main/java/ai/elimu/web/content/word/WordCsvExportController.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (windows-latest, 21)
- GitHub Check: build (windows-latest, 17)
- GitHub Check: build (ubuntu-latest, 21)
- GitHub Check: test_ui_ENG
- GitHub Check: build (ubuntu-latest, 17)
- GitHub Check: test_rest_ENG
🔇 Additional comments (18)
src/main/java/ai/elimu/dao/ImageDao.java (1)
20-21: LGTM! Consistent implementation across all DAOs.The addition of
readAllOrderedById()is consistently implemented across all DAO interfaces, which is great for maintaining a uniform API. The method signature follows the established pattern and integrates well with the existing interface design.src/main/java/ai/elimu/dao/LetterSoundDao.java (1)
17-18: LGTM! Method signature follows existing patterns.The new method follows the same pattern as existing ordering methods and correctly throws
DataAccessException.src/main/java/ai/elimu/dao/SoundDao.java (1)
19-20: LGTM! Method signature follows existing patterns.The new method follows the same pattern as existing ordering methods and correctly throws
DataAccessException.src/main/java/ai/elimu/dao/StoryBookDao.java (1)
19-20: LGTM! Method signature follows existing patterns.The new method follows the same pattern as existing ordering methods and correctly throws
DataAccessException.src/main/java/ai/elimu/dao/WordDao.java (1)
23-24: LGTM! Method signature follows existing patterns.The new method follows the same pattern as existing ordering methods and correctly throws
DataAccessException.src/main/java/ai/elimu/dao/jpa/NumberDaoJpa.java (1)
36-43: LGTM! Clean implementation of ID-based ordering.The implementation follows JPA best practices and maintains consistency with existing query patterns.
src/main/java/ai/elimu/dao/jpa/EmojiDaoJpa.java (1)
48-55: LGTM! Consistent implementation across DAOs.The implementation maintains consistency with other DAO implementations while preserving the specialized emoji-specific functionality.
src/main/java/ai/elimu/dao/jpa/StoryBookDaoJpa.java (1)
58-65: LGTM!The implementation of
readAllOrderedById()is consistent with the class's existing query patterns and error handling. The method correctly ordersStoryBookentities by their ID.src/main/java/ai/elimu/dao/jpa/LetterSoundDaoJpa.java (1)
48-55: LGTM!The implementation of
readAllOrderedById()is consistent with the class's existing query patterns and error handling. The method correctly ordersLetterSoundentities by their ID.src/main/java/ai/elimu/dao/jpa/SoundDaoJpa.java (1)
67-74: LGTM!The implementation of
readAllOrderedById()is consistent with the class's existing query patterns and error handling. The method correctly ordersSoundentities by their ID.src/main/java/ai/elimu/web/content/letter/LetterCsvExportController.java (1)
39-39: LGTM!The update to use
readAllOrderedById()aligns with the PR objective to sort data by ID during CSV export. The change is consistent with similar updates across other CSV export controllers.src/main/java/ai/elimu/web/content/sound/SoundCsvExportController.java (1)
37-37: Verify impact of changing sort order from usage-based to ID-based.While the implementation is correct, changing from
readAllOrderedByUsage()toreadAllOrderedById()might affect users who rely on usage-based ordering in their CSV exports.Please confirm if users have been notified about this change in sorting behavior, as it might affect their workflows that depend on usage-based ordering.
src/main/java/ai/elimu/dao/jpa/WordDaoJpa.java (1)
92-99: LGTM! Clean and efficient implementation.The implementation follows JPA best practices and maintains consistency with other ordering methods in the class.
src/main/java/ai/elimu/web/content/number/NumberCsvExportController.java (1)
39-39: LGTM! Consistent with the ID-based ordering initiative.The change aligns with the PR objectives while maintaining the existing CSV export functionality.
src/main/java/ai/elimu/web/content/emoji/EmojiCsvExportController.java (1)
39-39: LGTM! Consistent implementation across controllers.The change maintains consistency with other CSV export controllers while preserving the existing functionality.
src/main/java/ai/elimu/web/content/letter_sound/LetterSoundCsvExportController.java (1)
40-40: LGTM! Verify impact on CSV consumers.The change aligns with the PR objective to sort by ID during CSV export. However, since this changes the ordering from usage-based to ID-based, we should verify that no consumers depend on the previous usage-based ordering.
Let's check for potential consumers of this CSV endpoint:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for references to the CSV endpoint rg -l "letter-sounds\.csv"Length of output: 228
CSV Consumers Unaffected by Ordering Change
The investigation of the CSV endpoint references in:
src/main/webapp/WEB-INF/jsp/content/letter-sound/list.jspsrc/main/java/ai/elimu/util/db/DbContentImportHelper.javareveals no evidence that these consumers depend on the previous usage-based ordering. Both the import helper and the JSP file reference the CSV endpoint without imposing any order-specific constraints. Changing to ID-based ordering for CSV export appears to be benign in terms of consumer impact.
src/main/java/ai/elimu/web/content/word/WordCsvExportController.java (1)
42-42: LGTM! Consider educational impact.The change aligns with the PR objective to sort by ID during CSV export. However, since words were previously ordered by usage, which could be pedagogically relevant, we should verify that this change doesn't impact any educational features.
Let's check for potential educational features that might depend on word ordering:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for references to the CSV endpoint and word ordering rg -l "words\.csv|readAllOrderedByUsage.*Word"Length of output: 225
I'll run a script to search for all occurrences of
readAllOrderedByUsagein the repository to understand its usage and potential impact on educational features.
🏁 Script executed:
#!/bin/bash rg "readAllOrderedByUsage"Length of output: 3442
The CSV export change in the WordCsvExportController now uses ordering by ID instead of usage, but a quick scan of the codebase shows that other general educational features—such as those in the interactive word list (see WordListController and related JSPs)—continue to use ordering by usage. There isn’t a clear indication that any educational functionality relies on the CSV export's ordering.
Action Items:
- The export’s sorting by ID is isolated to CSV export.
- Other word-related features (e.g. WordListController, WordEditController) still use the pedagogically relevant “readAllOrderedByUsage” method.
Conclusion:
The change appears to be isolated with no adverse impact on educational features.src/main/java/ai/elimu/web/content/storybook/StoryBookCsvExportController.java (1)
55-55: LGTM!The change aligns with the PR objective to sort by ID during CSV export.
|
@AbaSheger Thank you for opening this pull request 🙏 Note that another pull request (with similar changes) was opened and merged just a short while ago: #1998 So closing this one. |
Fixes #1999
Add methods to sort by ID in various DAO interfaces and implementations.
DAO Interfaces:
readAllOrderedByIdmethod toEmojiDao,ImageDao,LetterDao,LetterSoundDao,NumberDao,SoundDao,StoryBookDao, andWordDaointerfaces.DAO Implementations:
readAllOrderedByIdmethod inEmojiDaoJpa,ImageDaoJpa,LetterDaoJpa,LetterSoundDaoJpa,NumberDaoJpa,SoundDaoJpa,StoryBookDaoJpa, andWordDaoJpaclasses.CSV Export Controllers:
EmojiCsvExportControllerto useemojiDao.readAllOrderedById.LetterSoundCsvExportControllerto useletterSoundDao.readAllOrderedById.LetterCsvExportControllerto useletterDao.readAllOrderedById.NumberCsvExportControllerto usenumberDao.readAllOrderedById.SoundCsvExportControllerto usesoundDao.readAllOrderedById.StoryBookCsvExportControllerto usestoryBookDao.readAllOrderedById.WordCsvExportControllerto usewordDao.readAllOrderedById.