Iris: Allow team repository access for Iris
#9975
Conversation
github-actions
bot
added
tests
server
bassner
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
bassner
changed the title
Iris: Allow team repo access for IrisIris: Allow team repository access for Iris
|
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/core/connector/IrisRequestMockProvider.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. WalkthroughThe changes introduced in this pull request enhance the functionality of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 2
🧹 Outside diff range and nitpick comments (4)
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java (2)
103-133: Add JavaDoc documentation for the new method.As per PR objectives mentioning JavaDoc style documentation, please add comprehensive JavaDoc for this method describing its purpose, parameters, and expected behavior.
+ /** + * Mocks a programming exercise chat response while validating the submission ID in the request. + * + * @param responseConsumer Consumer that processes the chat pipeline execution DTO + * @param submissionId Expected submission ID that should be present in the request + * @throws AssertionError if the submission ID is missing or doesn't match the expected value + */ public void mockProgrammingExerciseChatResponseExpectingSubmissionId(...)
103-133: Consider extracting common response handling logic.The response handling logic is duplicated between this method and
mockProgrammingExerciseChatResponse. Consider extracting the common parts into a private helper method.+ private void handleChatResponse(MockClientHttpRequest request, Consumer<PyrisExerciseChatPipelineExecutionDTO> responseConsumer) throws IOException { + var dto = mapper.readValue(request.getBodyAsString(), PyrisExerciseChatPipelineExecutionDTO.class); + responseConsumer.accept(dto); + return MockRestResponseCreators.withRawStatus(HttpStatus.ACCEPTED.value()).createResponse(request); + } public void mockProgrammingExerciseChatResponseExpectingSubmissionId(...) { mockServer .expect(...) .andExpect(...) - .andRespond(request -> { - var mockRequest = (MockClientHttpRequest) request; - var dto = mapper.readValue(mockRequest.getBodyAsString(), PyrisExerciseChatPipelineExecutionDTO.class); - responseConsumer.accept(dto); - return MockRestResponseCreators.withRawStatus(HttpStatus.ACCEPTED.value()).createResponse(request); - }); + .andRespond(request -> handleChatResponse((MockClientHttpRequest) request, responseConsumer)); }src/test/java/de/tum/cit/aet/artemis/iris/IrisChatMessageIntegrationTest.java (1)
77-79: Consider using constructor injection instead of field injection for 'teamRepository'While field injection is common in tests, using constructor injection for
teamRepositorycan enhance test readability and promote better dependency management.src/test/java/de/tum/cit/aet/artemis/exercise/util/ExerciseUtilService.java (1)
170-172: Consider clarifying the filter condition in 'getFirstExerciseWithType'The added filter
!ex.isTeamMode()excludes team exercises. Ensure that this method is intended to only retrieve solo exercises and that the documentation reflects this behavior.Consider updating the method documentation:
- * Accesses the first found non-team exercise of a course with the passed type. + * Retrieves the first non-team exercise of a given type from the course.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/util/ExerciseUtilService.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/iris/IrisChatMessageIntegrationTest.java(13 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.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
src/test/java/de/tum/cit/aet/artemis/exercise/util/ExerciseUtilService.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
src/test/java/de/tum/cit/aet/artemis/iris/IrisChatMessageIntegrationTest.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 (2)
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (3)
Learnt from: Hialus
PR: ls1intum/Artemis#8607
File: src/main/java/de/tum/in/www1/artemis/web/rest/programming/ProgrammingExerciseResource.java:64-64
Timestamp: 2024-11-12T12:51:51.201Z
Learning: For the Artemis project, import statements are automatically managed by formatters and should not be commented on.
Learnt from: julian-christl
PR: ls1intum/Artemis#9322
File: src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java:0-0
Timestamp: 2024-11-12T12:51:51.201Z
Learning: In Artemis, an `ExerciseGroup` always has an associated `Exam`, so `exerciseGroup.exam` is never null.
Learnt from: julian-christl
PR: ls1intum/Artemis#9322
File: src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java:170-172
Timestamp: 2024-11-12T12:51:46.554Z
Learning: In Artemis, `exercise.exerciseGroup` may be null, as not all exercises belong to an `ExerciseGroup`.
src/test/java/de/tum/cit/aet/artemis/iris/IrisChatMessageIntegrationTest.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 (7)
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java (1)
103-133: Implementation looks good with suggested improvements.
The new method effectively validates the submission ID in mock responses, which aligns well with the PR objective of enabling team repository access. The implementation is consistent with the existing codebase and follows similar patterns.
src/test/java/de/tum/cit/aet/artemis/iris/IrisChatMessageIntegrationTest.java (3)
83-89: Fields for solo and team exercises are appropriately added
The addition of soloExercise, teamExercise, soloParticipation, and teamParticipation fields effectively supports testing for both solo and team exercises.
180-189: Refactored test methods enhance clarity and coverage
Splitting sendOneMessage into sendOneMessageNormalExercise and sendOneMessageTeamExercise improves test clarity and ensures that both exercise modes are thoroughly tested.
380-382: Proper role assignment in 'sendMessageRateLimitReached' test
Changing the mock user to student3 with the ADMIN role accurately reflects the required permissions for this test case, enhancing its validity.
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (1)
149-156: Ensure proper retrieval of team participations
The added conditional correctly differentiates between team and solo exercises when retrieving submissions. Make sure that this logic aligns with all possible exercise modes and that edge cases are handled.
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java (1)
198-209: Method 'findAllWithSubmissionByExerciseIdAndStudentLoginInTeam' follows repository patterns
The new method adheres to the repository's query conventions and efficiently retrieves participations for team exercises.
src/test/java/de/tum/cit/aet/artemis/exercise/util/ExerciseUtilService.java (1)
174-183: Addition of 'getFirstTeamExerciseWithType' method enhances utility service
The new method getFirstTeamExerciseWithType effectively retrieves team exercises and complements the existing utility functions.
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
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/core/connector/IrisRequestMockProvider.java (2)
104-134: Add JavaDoc documentation for the new methodThe implementation looks good and properly validates the submission ID. However, as per PR objectives, please add JavaDoc documentation to describe the method's purpose, parameters, and behavior.
Add documentation like this:
+ /** + * Mocks a programming exercise chat response while validating the submission ID in the request. + * + * @param responseConsumer Consumer that processes the chat pipeline execution DTO + * @param submissionId Expected submission ID that should be present in the request + */ public void mockProgrammingExerciseChatResponseExpectingSubmissionId(...)
104-134: Consider extracting validation logic for reusabilityThe JSON validation logic could be extracted into a separate method to improve readability and enable reuse by other mock methods.
Consider refactoring like this:
+ /** + * Validates that the request JSON contains a submission with the expected ID. + * + * @param jsonNode Request body as JsonNode + * @param expectedSubmissionId Expected submission ID + */ + private void validateSubmissionId(JsonNode jsonNode, long expectedSubmissionId) { + assertThat(jsonNode.has("submission")) + .withFailMessage("Request body must contain a 'submission' field") + .isTrue(); + assertThat(jsonNode.get("submission").isObject()) + .withFailMessage("The 'submission' field must be an object") + .isTrue(); + assertThat(jsonNode.get("submission").has("id")) + .withFailMessage("The 'submission' object must contain an 'id' field") + .isTrue(); + assertThat(jsonNode.get("submission").get("id").asLong()) + .withFailMessage("Submission ID in request (%d) does not match expected ID (%d)", + jsonNode.get("submission").get("id").asLong(), expectedSubmissionId) + .isEqualTo(expectedSubmissionId); + } public void mockProgrammingExerciseChatResponseExpectingSubmissionId( Consumer<PyrisExerciseChatPipelineExecutionDTO> responseConsumer, long submissionId ) { mockServer .expect(ExpectedCount.once(), requestTo(pipelinesApiURL + "/tutor-chat/default/run")) .andExpect(method(HttpMethod.POST)) .andExpect(request -> { var mockRequest = (MockClientHttpRequest) request; var jsonNode = mapper.readTree(mockRequest.getBodyAsString()); validateSubmissionId(jsonNode, submissionId); }) .andRespond(request -> { var mockRequest = (MockClientHttpRequest) request; var dto = mapper.readValue(mockRequest.getBodyAsString(), PyrisExerciseChatPipelineExecutionDTO.class); responseConsumer.accept(dto); return MockRestResponseCreators.withRawStatus(HttpStatus.ACCEPTED.value()).createResponse(request); }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.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
🔇 Additional comments (1)
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java (1)
4-4: LGTM: Import aligns with testing guidelines
The static import of assertThat from AssertJ follows the coding guidelines for test assertions.
There was a problem hiding this comment.
I tested on TS3. I created this exercise:
https://artemis-test3.artemis.cit.tum.de/courses/29/exercises/825
I created a team of Test User 1 and Test User 2.
I used Test User 1 to submit the code and talking to Iris works fine for Test User 1.
However when I switch to Test User 2 and tried to talk to Iris, it said that Iris is disabled for this exercise
bassner
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
|
I can not reproduce the issue above for test user 2. |
Checklist
General
Server
Motivation and Context
Iris should be able to help on all programming exericses; however, it currently can't see the code of submissions in team exercises.
Description
Added compatibility with team exercises by checking wether the exercise is a team exercise, then altering the retrieval logic accordingly.
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
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Summary by CodeRabbit
New Features
Bug Fixes
Tests