Development: Fix exercise deletion with existing Iris messages
#9875
Conversation
github-actions
bot
added
tests
database
github-actions
bot
added
the
iris
kaancayli
changed the title
Development: Fix exercise deletion with existing Iris messages
WalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (3)
src/test/java/de/tum/cit/aet/artemis/iris/IrisExerciseChatSessionIntegrationTest.java (3)
81-91: Consider enhancing JSON content variety.While the implementation is correct, consider making the JSON content more varied to better test different scenarios.
private IrisJsonMessageContent createMockJsonContent() { - var jsonMap = Map.of("key1", "value1", "key2", "value2", "key3", "value3"); + var rdm = ThreadLocalRandom.current(); + var jsonMap = Map.of( + "messageId", String.valueOf(rdm.nextInt(1000)), + "timestamp", String.valueOf(System.currentTimeMillis()), + "content", "Test message " + rdm.nextInt(100) + ); JsonNode jsonNode = new ObjectMapper().valueToTree(jsonMap); return new IrisJsonMessageContent(jsonNode); }
157-174: LGTM! Comprehensive test for text message cleanup.The test properly verifies the cascade deletion of both the session and its messages.
Consider enhancing the verification by checking the message count before deletion:
irisMessageService.saveMessage(createDefaultMockTextMessage(irisSession), irisSession, IrisMessageSender.LLM); +// Verify initial state +assertThat(irisMessageRepository.findAllBySessionId(irisSession.getId())).hasSize(4); assertThat(irisExerciseChatSessionRepository.findByIdElseThrow(irisSession.getId())).isNotNull();
176-193: LGTM! Comprehensive test for JSON message cleanup.The test effectively verifies the cascade deletion behavior with JSON message content.
Consider enhancing the verification similar to the text message test:
irisMessageService.saveMessage(createDefaultMockJsonMessage(irisSession), irisSession, IrisMessageSender.LLM); +// Verify initial state +assertThat(irisMessageRepository.findAllBySessionId(irisSession.getId())).hasSize(4); assertThat(irisExerciseChatSessionRepository.findByIdElseThrow(irisSession.getId())).isNotNull();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20241125000900_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (1)
src/test/java/de/tum/cit/aet/artemis/iris/IrisExerciseChatSessionIntegrationTest.java(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/java/de/tum/cit/aet/artemis/iris/IrisExerciseChatSessionIntegrationTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
📓 Learnings (1)
src/test/java/de/tum/cit/aet/artemis/iris/IrisExerciseChatSessionIntegrationTest.java (1)
Learnt from: alexjoham
PR: ls1intum/Artemis#9455
File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401
Timestamp: 2024-11-12T12:51:58.050Z
Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
🔇 Additional comments (3)
src/test/java/de/tum/cit/aet/artemis/iris/IrisExerciseChatSessionIntegrationTest.java (3)
6-7: LGTM! Dependencies and field injections are properly organized.
The new imports and autowired fields are well-structured and follow the existing patterns in the test class.
Also applies to: 15-23, 28-29, 39-43
58-61: LGTM! Well-structured helper method for session creation.
The method follows the class's existing patterns and properly uses the repository for persistence.
63-79: LGTM! Text message creation is well implemented.
The random text generation provides good test data variety while maintaining readability.
WalkthroughThe changes in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmd (7.7.0)src/test/java/de/tum/cit/aet/artemis/iris/IrisExerciseChatSessionIntegrationTest.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. 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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/test/java/de/tum/cit/aet/artemis/iris/IrisExerciseChatSessionIntegrationTest.java (2)
69-79: Consider extracting test data arrays as constantsThe arrays of adjectives and nouns could be moved to class-level constants to improve maintainability and reusability.
class IrisExerciseChatSessionIntegrationTest extends AbstractIrisIntegrationTest { + private static final String[] TEST_ADJECTIVES = { "happy", "sad", "angry", "funny", "silly", "crazy", "beautiful", "smart" }; + private static final String[] TEST_NOUNS = { "dog", "cat", "house", "car", "book", "computer", "phone", "shoe" }; private IrisMessageContent createMockTextContent() { - String[] adjectives = { "happy", "sad", "angry", "funny", "silly", "crazy", "beautiful", "smart" }; - String[] nouns = { "dog", "cat", "house", "car", "book", "computer", "phone", "shoe" }; var rdm = ThreadLocalRandom.current(); - String randomAdjective = adjectives[rdm.nextInt(adjectives.length)]; - String randomNoun = nouns[rdm.nextInt(nouns.length)]; + String randomAdjective = TEST_ADJECTIVES[rdm.nextInt(TEST_ADJECTIVES.length)]; + String randomNoun = TEST_NOUNS[rdm.nextInt(TEST_NOUNS.length)];
176-193: Consider combining duplicate test logicThis test is nearly identical to the text message test. Consider parameterizing the test to reduce duplication.
+ @ParameterizedTest + @EnumSource(MessageType.class) @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") - void testDeleteExerciseWithIrisMessagesWithJsonMessageContent() throws Exception { + void testDeleteExerciseWithIrisMessages(MessageType messageType) throws Exception { var irisSession = createSessionForUser("instructor1"); // Create and some messages to the session - irisMessageService.saveMessage(createDefaultMockJsonMessage(irisSession), irisSession, IrisMessageSender.USER); - irisMessageService.saveMessage(createDefaultMockJsonMessage(irisSession), irisSession, IrisMessageSender.LLM); - irisMessageService.saveMessage(createDefaultMockJsonMessage(irisSession), irisSession, IrisMessageSender.USER); - irisMessageService.saveMessage(createDefaultMockJsonMessage(irisSession), irisSession, IrisMessageSender.LLM); + var createMessage = messageType == MessageType.TEXT ? + this::createDefaultMockTextMessage : + this::createDefaultMockJsonMessage; + + irisMessageService.saveMessage(createMessage.apply(irisSession), irisSession, IrisMessageSender.USER); + irisMessageService.saveMessage(createMessage.apply(irisSession), irisSession, IrisMessageSender.LLM); + irisMessageService.saveMessage(createMessage.apply(irisSession), irisSession, IrisMessageSender.USER); + irisMessageService.saveMessage(createMessage.apply(irisSession), irisSession, IrisMessageSender.LLM);Add this enum at class level:
private enum MessageType { TEXT, JSON }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20241125000900_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (1)
src/test/java/de/tum/cit/aet/artemis/iris/IrisExerciseChatSessionIntegrationTest.java(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/java/de/tum/cit/aet/artemis/iris/IrisExerciseChatSessionIntegrationTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
📓 Learnings (1)
src/test/java/de/tum/cit/aet/artemis/iris/IrisExerciseChatSessionIntegrationTest.java (1)
Learnt from: alexjoham
PR: ls1intum/Artemis#9455
File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401
Timestamp: 2024-11-12T12:51:58.050Z
Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
🔇 Additional comments (4)
src/test/java/de/tum/cit/aet/artemis/iris/IrisExerciseChatSessionIntegrationTest.java (4)
6-7: LGTM: Dependencies and autowired fields are properly structured
The new dependencies and autowired fields are well-organized and align with the PR's objective of handling Iris messages in tests.
Also applies to: 15-16, 19-23, 28-29, 39-43
58-61: LGTM: Session creation helper is well-structured
The helper method follows good practices by using the repository layer and maintaining test data isolation with the TEST_PREFIX.
157-174: LGTM: Test case properly validates text message deletion
The test follows good practices:
- Creates multiple messages to verify bulk deletion
- Verifies both session and message deletion
- Uses appropriate assertions
157-193: Verify database migration impact
The tests effectively verify the cascade deletion behavior, but we should ensure the database migration has been properly applied before running these tests.
|
I thought this was fixed with #9567? |
|
@MichaelOwenDyer The other PR only added a casecase from exercise -> session. We need to cascade all the way from exercise -> session -> message -> content, though, which we didn't so far |
@MichaelOwenDyer I have migrated my local setup to a new computer and it was not possible to deleting the exercises with an Iris session that includes messages. Other people in the Iris working group reported the same issue. The previous PR solves the issue regarding session, but I can at least confirm from my experience that I was receiving foreign key violations when trying to delete exercises |
bassner
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
dismissed stale reviews from HawKhiem, bassner, sebastianloose, yassinsws, and coderabbitai[bot]
via
a26e77e
Checklist
General
Server
Motivation and Context
Currently, we are unable to delete exercises in some cases which have any associated iris sessions in the database with messages and message_contents, due to a foreign key integrity violation. This PR adds a database migration to introduce a cascading deletes on that foreign keys, so when an exercise is deleted, all associated sessions are automatically deleted as well. This also triggers cascading deletes of all of the session's messages, and all of those messages' contents.
Description
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests
Chores