-
Notifications
You must be signed in to change notification settings - Fork 322
Communication
: Add Iris Tutor Suggestions
#10666
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
Communication
: Add Iris Tutor Suggestions
#10666
Conversation
End-to-End (E2E) Test Results Summary
|
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
♻️ Duplicate comments (4)
src/test/java/de/tum/cit/aet/artemis/iris/IrisTutorSuggestionIntegrationTest.java (4)
51-80
: Use service-based testing approach rather than direct repository access.According to your coding guidelines (
avoid_db_access: true
), tests should use service methods instead of direct repository access.The test injects multiple repositories directly, which violates the guidelines. Consider using service methods for repository operations.
90-102
: Include database query counting in test setup for performance tracking.Your coding guidelines specify
db_query_count_tests: track_performance
, but the test setup doesn't include query count tracking.Add query count tracking setup in your
@BeforeEach
method as mentioned in past review comments.
104-112
: Use descriptive test method name and test service methods instead of repositories.The test method name lacks descriptiveness and uses direct repository access.
Follow the descriptive test naming pattern (
testX_shouldY
) used in other methods and use service methods instead of repository access.
133-146
: 🛠️ Refactor suggestionInconsistent usage of service vs repository methods for message handling.
This test uses a service method for saving but direct repository access for retrieval.
Make the approach consistent by using service methods for both operations:
var irisSession = request.postWithResponseBody(tutorSuggestionUrl(post.getId()), null, IrisSession.class, HttpStatus.CREATED); var message = new IrisMessage(); message.addContent(new IrisTextMessageContent("Test tutor suggestion request")); message.setSender(IrisMessageSender.LLM); message.setSession(irisSession); irisMessageService.saveMessage(message, irisSession, IrisMessageSender.LLM); -var messages = irisMessageRepository.findAllBySessionId(irisSession.getId()); +var messages = irisMessageService.findAllBySessionId(irisSession.getId()); assertThat(messages).hasSize(1);
🧹 Nitpick comments (3)
src/test/java/de/tum/cit/aet/artemis/iris/IrisTutorSuggestionIntegrationTest.java (3)
1-47
: Add class-level documentation to describe the purpose of the integration test.This integration test class lacks JavaDoc documentation explaining its purpose and relationship to the Iris Tutor Suggestion feature.
Add a JavaDoc comment at the class level:
@ActiveProfiles(PROFILE_IRIS) +/** + * Integration tests for the Iris Tutor Suggestion feature. + * Tests the creation of tutor suggestion sessions, messaging, pipeline execution, + * and access control for different user roles. + */ class IrisTutorSuggestionIntegrationTest extends AbstractIrisIntegrationTest {
230-271
: Refactor complex test method for better readability.This test method is quite long and complex, containing token generation, request authentication, and status verification.
Extract helper methods to improve readability:
@Test @WithMockUser(username = TEST_PREFIX + "tutor1", roles = "TA") void testTutorSuggestionStatusUpdateShouldBeHandled() throws Exception { var post = createPostInExerciseChat(exercise, TEST_PREFIX); var conversation = post.getConversation(); conversation.setCourse(course); post = postRepository.save(post); var irisSession = request.postWithResponseBody(tutorSuggestionUrl(post.getId()), null, IrisTutorSuggestionSession.class, HttpStatus.CREATED); irisSession.setPostId(post.getId()); - String token = pyrisJobService.addTutorSuggestionJob(post.getId(), course.getId(), irisSession.getId()); - - HttpHeaders headers = new HttpHeaders(); - headers.add("Authorization", "Bearer " + token); - // Manually authenticate the job to verify token registration - var mockRequest = new org.springframework.mock.web.MockHttpServletRequest(); - mockRequest.addHeader("Authorization", "Bearer " + token); - var job = pyrisJobService.getAndAuthenticateJobFromHeaderElseThrow(mockRequest, de.tum.cit.aet.artemis.iris.service.pyris.job.TutorSuggestionJob.class); - assertThat(job).isNotNull(); - assertThat(job.jobId()).isEqualTo(token); + String token = createAndVerifyTutorSuggestionJob(post.getId(), course.getId(), irisSession.getId()); List<PyrisStageDTO> stages = List.of(new PyrisStageDTO("Test stage", 0, PyrisStageState.DONE, "Done")); var statusUpdate = new TutorSuggestionStatusUpdateDTO("Test suggestion", "Test result", stages, null); var mockRequestForStatusUpdate = new org.springframework.mock.web.MockHttpServletRequest(); mockRequestForStatusUpdate.addHeader("Authorization", "Bearer " + token); publicPyrisStatusUpdateResource.setTutorSuggestionJobStatus(token, statusUpdate, mockRequestForStatusUpdate); - // Remove the job and assert that accessing it throws an exception - var requestAfterRemoval = new org.springframework.mock.web.MockHttpServletRequest(); - requestAfterRemoval.addHeader("Authorization", "Bearer " + token); - assertThatThrownBy( - () -> pyrisJobService.getAndAuthenticateJobFromHeaderElseThrow(requestAfterRemoval, de.tum.cit.aet.artemis.iris.service.pyris.job.TutorSuggestionJob.class)) - .isInstanceOf(de.tum.cit.aet.artemis.core.exception.AccessForbiddenException.class).hasMessageContaining("No valid token provided"); + verifyJobRemovalThrowsException(token); // Check if the messages were saved var messages = irisMessageRepository.findAllBySessionId(irisSession.getId()); assertThat(messages).hasSize(2); assertThat(messages.getFirst().getContent().getFirst().toString()).contains("Test suggestion"); assertThat(messages.getFirst().getSender()).isEqualTo(IrisMessageSender.ARTIFACT); assertThat(messages.get(1).getContent().getFirst().toString()).contains("Test result"); assertThat(messages.get(1).getSender()).isEqualTo(IrisMessageSender.LLM); } +/** + * Creates a tutor suggestion job and verifies it was registered correctly + */ +private String createAndVerifyTutorSuggestionJob(Long postId, Long courseId, Long sessionId) { + String token = pyrisJobService.addTutorSuggestionJob(postId, courseId, sessionId); + + HttpHeaders headers = new HttpHeaders(); + headers.add("Authorization", "Bearer " + token); + // Manually authenticate the job to verify token registration + var mockRequest = new org.springframework.mock.web.MockHttpServletRequest(); + mockRequest.addHeader("Authorization", "Bearer " + token); + var job = pyrisJobService.getAndAuthenticateJobFromHeaderElseThrow(mockRequest, de.tum.cit.aet.artemis.iris.service.pyris.job.TutorSuggestionJob.class); + assertThat(job).isNotNull(); + assertThat(job.jobId()).isEqualTo(token); + return token; +} + +/** + * Verifies that attempting to access a removed job throws the expected exception + */ +private void verifyJobRemovalThrowsException(String token) { + var requestAfterRemoval = new org.springframework.mock.web.MockHttpServletRequest(); + requestAfterRemoval.addHeader("Authorization", "Bearer " + token); + assertThatThrownBy( + () -> pyrisJobService.getAndAuthenticateJobFromHeaderElseThrow(requestAfterRemoval, de.tum.cit.aet.artemis.iris.service.pyris.job.TutorSuggestionJob.class)) + .isInstanceOf(de.tum.cit.aet.artemis.core.exception.AccessForbiddenException.class).hasMessageContaining("No valid token provided"); +}Also, fix the comment typo: "where" should be "were".
230-264
: Use Spring's MockHttpServletRequestBuilder instead of manual MockHttpServletRequest creation.The test creates and configures mock HTTP request objects manually.
Use Spring's MockMvc request builders for creating test requests:
-var mockRequest = new org.springframework.mock.web.MockHttpServletRequest(); -mockRequest.addHeader("Authorization", "Bearer " + token); +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; + +var mockRequest = post("/api/mock-endpoint") + .header("Authorization", "Bearer " + token) + .buildRequest(null);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/test/java/de/tum/cit/aet/artemis/iris/IrisTutorSuggestionIntegrationTest.java
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/test/java/**/*.java`: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_...
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/IrisTutorSuggestionIntegrationTest.java
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: server-style
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: client-tests-selected
- GitHub Check: Analyse
src/test/java/de/tum/cit/aet/artemis/iris/IrisTutorSuggestionIntegrationTest.java
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
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.
Code looks good to me!
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 locally works as expected. re-approve
End-to-End (E2E) Test Results Summary
|
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.
Reapprove! Code looks still good
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.
Communication
: Add first Iris Tutor Suggestion InterfaceCommunication
: Add Iris Tutor Suggestions
Checklist
General
Server
Client
authorities
to all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
The communication section gets a big upgrade: The tutor suggestion. This adds a new tool for all tutors to use when answering student questions. When opening a thread, Iris creates a suggestion for tutors to use when answering questions from students. This means the communication section needs to add this feature. For that, a first version is needed that already includes: Settings to toggle this feature, Authentification to only allow tutors and higher to see the suggestions, and a database migration that saves sessions and history. This basic version lays the ground for further improvements to be added in follow-up PRs.
Description
!! Important: This is work in progress! With this PR a first version of the tutor suggestion feature is introduced. More changes are added with upcoming PRs !!
This PR introduces the Iris Tutor Suggestion functionality, enabling the generation of Iris-assisted tutor suggestions in response to student posts within conversation threads.
Domain & Session Layer:
IrisTutorSuggestionSession
, a new session type that links a tutor’s Iris chat session to a specific Post.IrisMessageSender
to includeARTIFACT
as a new message source.IrisSessionService
to handle the new session type and associated access control.Settings Infrastructure:
IrisTutorSuggestionSubSettings
with support across global, course, and exercise-level configurations.IrisCombinedSettingsDTO
.IrisSettingsService
andIrisSubSettingsService
to manage these new settings and enable cascading logic.Pipeline Execution
TutorSuggestionJob
andPyrisTutorSuggestionPipelineExecutionDTO
to define the tutor suggestion pipeline in Pyris.PyrisPipelineService
and status handling inPyrisStatusUpdateService
.REST API:
Added
IrisTutorSuggestionSessionResource
with endpoints to:IrisMessageResource
to allowARTIFACT
messages to be saved and processed appropriately.Frontend:
New Angular component
TutorSuggestionComponent
:ConversationThreadSidebar
to render suggestions in line.Persistence:
Liquibase changes to:
post_id
toiris_session
.Steps for Testing
Prerequisites:
Activate the feature:
Test for students:
Test for tutors:
Check settings:
To roll back the migration, execute this SQL statement:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Client
Server
Screenshots
Running pipeline
Finished pipeline with result
Failed pipeline
Summary by CodeRabbit
New Features
Bug Fixes
Tests