Iris: Proactive Event System
#9558
Conversation
github-actions
bot
added
tests
server
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
kaancayli
force-pushed
the
feature/iris/event-service
branch
from
1052ebc
to
41dc8dd
Compare
kaancayli
force-pushed
the
feature/iris/event-service
branch
from
41dc8dd
to
8f0912f
Compare
sebastianloose
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
|
github-actions
bot
added
the
deployment-error
…on with Submission::getParticipation()
kaancayli
dismissed stale reviews from yassinsws, coderabbitai[bot], sebastianloose, and alexjoham
via
f8952bb
![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 (1)
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (1)
214-252: Simplify complex conditional logic in onNewResultThe method contains nested conditions and multiple responsibility branches. Consider extracting the score analysis logic.
Extract the score analysis logic into a separate method:
+ private void handleProgressStallCheck(ProgrammingExerciseStudentParticipation participation, List<Submission> submissions) { + var successfulSubmission = submissions.stream() + .anyMatch(submission -> submission.getLatestResult() != null && + submission.getLatestResult().getScore() == getSuccessThreshold()); + + if (!successfulSubmission && submissions.size() >= 3) { + processNonSuccessfulSubmissions(participation, submissions); + } + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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
🔇 Additional comments (5)
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (5)
264-296: LGTM! Well-structured helper methods
The implementation is clean, well-documented, and includes proper input validation. The methods follow single responsibility principle and have clear, focused logic.
328-328: Add JavaDoc for createSession method
The method is missing JavaDoc documentation.
Add JavaDoc similar to other methods in the class:
/**
* Creates a new Iris session for the given exercise and user.
* Validates user acceptance and authorization before creating the session.
*
* @param exercise The exercise the session belongs to
* @param user The user the session belongs to
* @param sendInitialMessage Whether to send an initial message from Iris
* @return The created session
* @throws AccessForbiddenException if the user doesn't have required permissions
*/307-354: LGTM! Well-implemented session management
The session management methods are well-structured with proper validation, error handling, and async operations where needed.
79-82: 🛠️ Refactor suggestion
Externalize configuration constants
The SUCCESS_THRESHOLD and INTERVAL_SIZE are hardcoded with TODO comments. These should be moved to the Iris settings for better configurability.
Consider moving these values to the Iris settings configuration:
- private final double SUCCESS_THRESHOLD = 100.0; // TODO: Retrieve configuration from Iris settings
- private final int INTERVAL_SIZE = 3; // TODO: Retrieve configuration from Iris settings
+ private double getSuccessThreshold() {
+ return irisSettingsService.getCombinedIrisSettingsFor(exercise, false).getSuccessThreshold();
+ }
+ private int getIntervalSize() {
+ return irisSettingsService.getCombinedIrisSettingsFor(exercise, false).getIntervalSize();
+ }186-207: 🛠️ Refactor suggestion
Enhance error handling in onBuildFailure
The async operation should handle potential exceptions to prevent silent failures.
Apply this improvement:
- CompletableFuture.runAsync(() -> requestAndHandleResponse(session, Optional.of("build_failed")));
+ CompletableFuture.runAsync(() -> {
+ try {
+ requestAndHandleResponse(session, Optional.of("build_failed"));
+ } catch (Exception e) {
+ log.error("Failed to process build failure event for user {}: {}", user.getName(), e.getMessage(), e);
+ }
+ });
kaancayli
removed
the
deployment-error
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/main/webapp/i18n/en/iris.json (1)
98-100: Consider a more descriptive message for chat bubble expansion.While the message is clear, it could be more descriptive to help users understand what "full message" means.
Consider updating the message to be more specific:
"chatBubble": { - "seeFull": "See full message" + "seeFull": "Click to expand full message" },src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java (2)
150-177: Add JavaDoc documentation for the new methodsAccording to the coding guidelines, all methods should have JavaDoc documentation explaining their purpose, parameters, and expected behavior.
Add JavaDoc for each new method:
/** * Mocks a response for the build failed event in the tutor chat pipeline. * Allows for a maximum of two expectations. * * @param responseConsumer Consumer to process the request body */ public void mockBuildFailedRunResponse(Consumer<PyrisExerciseChatPipelineExecutionDTO> responseConsumer) { // existing implementation } /** * Mocks a response for the progress stalled event in the tutor chat pipeline. * Allows for a maximum of two expectations. * * @param responseConsumer Consumer to process the request body */ public void mockProgressStalledEventRunResponse(Consumer<PyrisCourseChatPipelineExecutionDTO> responseConsumer) { // existing implementation } /** * Mocks a response for the JOL event in the course chat pipeline. * * @param responseConsumer Consumer to process the request body */ public void mockJolEventRunResponse(Consumer<PyrisCourseChatPipelineExecutionDTO> responseConsumer) { // existing implementation }
150-177: Consider refactoring duplicated response handling logicThe response handling logic is duplicated across all mock methods. Consider extracting it into a reusable helper method.
private <T> ResponseCreator createAcceptedResponse(MockClientHttpRequest request, Class<T> dtoClass, Consumer<T> responseConsumer) throws IOException { var dto = mapper.readValue(request.getBodyAsString(), dtoClass); responseConsumer.accept(dto); return MockRestResponseCreators.withRawStatus(HttpStatus.ACCEPTED.value()).createResponse(request); }Then use it in the mock methods:
public void mockBuildFailedRunResponse(Consumer<PyrisExerciseChatPipelineExecutionDTO> responseConsumer) { mockServer.expect(ExpectedCount.max(2), requestTo(pipelinesApiURL + "/tutor-chat/default/run?event=build_failed")) .andExpect(method(HttpMethod.POST)) - .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 -> createAcceptedResponse((MockClientHttpRequest) request, + PyrisExerciseChatPipelineExecutionDTO.class, responseConsumer)); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisConnectorService.java(3 hunks)src/main/webapp/i18n/de/iris.json(2 hunks)src/main/webapp/i18n/en/iris.json(2 hunks)src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/iris/PyrisConnectorServiceTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisConnectorService.java
- src/test/java/de/tum/cit/aet/artemis/iris/PyrisConnectorServiceTest.java
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/i18n/de/iris.json (1)
Pattern src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
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 (6)
src/main/webapp/i18n/en/iris.json (2)
35-42: LGTM! But JOL event settings are still missing.
The proactivity event settings for build failures and progress stalling are well-defined with clear labels and tooltips.
As noted in previous reviews, the JOL (Judgment of Learning) event settings are still missing from this configuration.
43-47: LGTM! Clear and informative proactivity settings.
The proactivity settings section provides clear context and explanations for the feature's behavior, including inheritance rules.
src/main/webapp/i18n/de/iris.json (3)
35-38: Make translations more informal and personal (dutzen)
The translations should use informal language (dutzen) as per coding guidelines.
"proactivityBuildFailedEventEnabled": {
- "label": "Build-Fehler überwachen",
- "tooltip": "Wenn aktiviert, wird Iris proaktiv Hilfe-Nachrichten senden, wenn der Build einer Abbgabe fehlschlägt."
+ "label": "Deine Build-Fehler überwachen",
+ "tooltip": "Wenn du dies aktivierst, sendet dir Iris proaktiv Hilfe-Nachrichten, wenn dein Build fehlschlägt."
},39-42: Make translations more informal and personal (dutzen)
The translations should use informal language (dutzen) as per coding guidelines.
"proactivityProgressStalledEventEnabled": {
- "label": "Übungsleistung überwachen",
- "tooltip": "Wenn aktiviert, wird Iris proaktiv Hilfe-Nachrichten senden, wenn sich die Übungsleistung des Studenten nicht verbessert."
+ "label": "Deinen Lernfortschritt überwachen",
+ "tooltip": "Wenn du dies aktivierst, sendet dir Iris proaktiv Hilfe-Nachrichten, wenn sich deine Übungsleistung nicht verbessert."
},98-100: Chat bubble translation is acceptable
The current translation uses an imperative form which is neither strictly formal nor informal. While it could be made more informal as suggested in past reviews, the current form is acceptable for this context.
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java (1)
150-168: Verify the ExpectedCount.max(2) usage
Both mockBuildFailedRunResponse and mockProgressStalledEventRunResponse use ExpectedCount.max(2) while other methods use ExpectedCount.once(). Please verify if this is intentional.
✅ Verification successful
Let me check the test file that uses these mock methods to understand the usage pattern.
ExpectedCount.max(2) is appropriate for these event handlers
The usage of ExpectedCount.max(2) in both mockBuildFailedRunResponse and mockProgressStalledEventRunResponse is correct. The test cases show that these mocks are used in scenarios where:
- The main test case verifies the event firing (
testShouldFireProgressStalledEvent,testShouldFireBuildFailedEvent) - Additional test cases verify when these events should not fire (
testShouldShouldNotFireProgressStalledEventWithExistingSuccessfulSubmission)
Setting max(2) allows the mock to be used across multiple test methods while ensuring the mock expectations aren't too strict, which is appropriate for event-based testing scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for test cases that use these mock methods to understand the expected count
# Search for test files using these methods
rg -l "mockBuildFailedRunResponse|mockProgressStalledEventRunResponse" --type java
# Search for ExpectedCount usage patterns in test files
rg "ExpectedCount\." --type java
Length of output: 7320
Script:
#!/bin/bash
# Get the content of the test file that uses these mock methods
rg -A 10 -B 10 "mockBuildFailedRunResponse|mockProgressStalledEventRunResponse" src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemTest.java
Length of output: 3586
Checklist
General
Server
Client
Motivation and Context
Implements the feature proposal described in #8800
Description
Steps for Testing
Prerequisites:
Monitor student submission build failuresunder Iris Chat Settings EnabledMonitor exercise performance progressunder Iris Chat Settings EnabledExercise Chat: Implement native function calling agent Pyris#154 merged or deployed on pyris-testLog in to Artemis
Navigate to the programming exercise with Iris enabled
Make sure the proactive event options under Iris Chat Settings are enabled in all levels:
Testing events:
Build failed event:
Progress stalled event:
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
Client
Server
Screenshots
Proactivity Settings
Proactive responses
Summary by CodeRabbit
Release Notes
New Features
IrisEventTypefor proactive event management.IrisExerciseChatSessionServiceandIrisCourseChatSessionServiceto handle new event types and improve chat session management.ProgrammingExerciseRepositoryfor retrieving comprehensive data related to programming exercises.PyrisEventServicefor managing events within the Iris application.IrisChatSubSettingsto managedisabledProactiveEventsand integrated these settings into the UI.Bug Fixes
Documentation
Style
Tests