Iris: Enhance student support with proactive assistance
#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
krusche
changed the title
Iris: Proactive Event SystemIris: Enhance student support with proactive assistance
kaancayli
dismissed stale reviews from yassinsws, isabellagessl, coderabbitai[bot], sebastianloose, SimonEntholzer, alexjoham, and coolchock
via
6a36edb
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/test/java/de/tum/cit/aet/artemis/iris/settings/IrisSettingsIntegrationTest.java (1)
434-464: Consider extracting helper methods for better readability.The test method effectively verifies the combination of disabled events but could benefit from helper methods to improve readability and reusability.
Consider extracting helper methods:
private IrisCourseSettings createCourseSettingsWithDisabledEvent(Course course, IrisEventType eventType) { var settings = new IrisCourseSettings(); settings.setCourse(course); settings.setIrisChatSettings(new IrisChatSubSettings()); settings.getIrisChatSettings().setEnabled(true); settings.getIrisChatSettings().setSelectedVariant(null); settings.getIrisChatSettings().setDisabledProactiveEvents(new TreeSet<>(Set.of(eventType.name().toLowerCase()))); return settings; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
src/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (12)
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/settings/IrisSettingsService.java(6 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/settings/IrisSubSettingsService.java(5 hunks)src/main/webapp/app/entities/iris/settings/iris-sub-settings.model.ts(1 hunks)src/main/webapp/app/iris/settings/iris-settings-update/iris-common-sub-settings-update/iris-common-sub-settings-update.component.ts(7 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/iris/AbstractIrisIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/iris/settings/IrisSettingsIntegrationTest.java(5 hunks)src/test/javascript/spec/component/iris/settings/iris-course-settings-update.component.spec.ts(3 hunks)src/test/javascript/spec/component/iris/settings/iris-global-settings-update.component.spec.ts(1 hunks)src/test/javascript/spec/component/iris/settings/mock-settings.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/test/java/de/tum/cit/aet/artemis/iris/AbstractIrisIntegrationTest.java
- src/main/webapp/app/entities/iris/settings/iris-sub-settings.model.ts
- src/test/javascript/spec/component/iris/settings/mock-settings.ts
- src/test/javascript/spec/component/iris/settings/iris-global-settings-update.component.spec.ts
- src/test/javascript/spec/component/iris/settings/iris-course-settings-update.component.spec.ts
- src/main/webapp/i18n/en/iris.json
- src/main/webapp/i18n/de/iris.json
- src/main/java/de/tum/cit/aet/artemis/iris/service/settings/IrisSubSettingsService.java
- src/main/webapp/app/iris/settings/iris-settings-update/iris-common-sub-settings-update/iris-common-sub-settings-update.component.ts
🧰 Additional context used
📓 Path-based instructions (3)
src/test/java/de/tum/cit/aet/artemis/iris/settings/IrisSettingsIntegrationTest.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/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.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/iris/service/settings/IrisSettingsService.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 (9)
src/test/java/de/tum/cit/aet/artemis/iris/settings/IrisSettingsIntegrationTest.java (3)
149-152: LGTM! Correctly handles collection order independence.
The modification properly accounts for the order-independent nature of the disabledProactiveEvents collection in the assertion.
445-445: Verify case sensitivity handling in the system.
The use of toLowerCase() on enum names suggests potential case sensitivity issues in the system. This could lead to inconsistencies if the case handling is not standardized across all components.
Also applies to: 457-457
✅ Verification successful
Based on the search results, I can now generate the final response:
Case sensitivity is consistently handled throughout the codebase
The codebase shows a standardized approach to case handling for event types:
- The enum
IrisEventTypedefines the events in uppercase (BUILD_FAILED, PROGRESS_STALLED) - All comparisons consistently use
toLowerCase()when storing/checking disabled events - The frontend (TypeScript) and backend (Java) maintain consistent casing in their interfaces
- API endpoints and mock tests demonstrate consistent lowercase usage for event parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for inconsistent case handling of event types
# Test: Search for different case variations of event type handling
# Look for event type case handling
rg -A 2 "IrisEventType\."
# Look for direct string usage of event types
rg -i "progress_stalled|build_failed"
Length of output: 13573
252-270: 🛠️ Refactor suggestion
Use IrisEventType enum instead of string literals.
The test correctly verifies the functionality but uses string literals for event types. Using enum constants would make the code more maintainable and type-safe.
Apply this diff to use enum constants:
- loadedSettings1.getIrisChatSettings().setDisabledProactiveEvents(new TreeSet<>(Set.of("PROGRESS_STALLED")));
+ loadedSettings1.getIrisChatSettings().setDisabledProactiveEvents(new TreeSet<>(Set.of(IrisEventType.PROGRESS_STALLED.name())));
- assertThat(updatedSettings.getIrisChatSettings().getDisabledProactiveEvents()).containsExactly("PROGRESS_STALLED");
+ assertThat(updatedSettings.getIrisChatSettings().getDisabledProactiveEvents()).containsExactly(IrisEventType.PROGRESS_STALLED.name());Likely invalid or redundant comment.
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (1)
118-120: 🛠️ Refactor suggestion
Consider using a marker interface instead of Object for type safety
Using Object as a parameter type loses type safety and could lead to runtime errors. This could be problematic when handling different event types (build_failed, progress_stalled, jol).
The previous suggestion to create a marker interface is still valid:
public interface IrisEventPayload {}
private void requestAndHandleResponse(IrisCourseChatSession session, String variant, IrisEventPayload payload) {
var chatSession = (IrisCourseChatSession) irisSessionRepository.findByIdWithMessagesAndContents(session.getId());
pyrisPipelineService.executeCourseChatPipeline(variant, chatSession, payload);
}src/main/java/de/tum/cit/aet/artemis/iris/service/settings/IrisSettingsService.java (5)
44-44: Import Statement Added
The import of IrisEventType is necessary for handling the new event types and appears correctly added.
88-92: Parameter Renaming Enhances Readability
Renaming the parameter from event to ignoredEvent in the execute method clarifies that the parameter is not used within the method, improving code readability.
487-493:
Incorrect Feature Check: Should Verify PROACTIVITY Instead of CHAT
In the method isActivatedForElseThrow(IrisEventType type, Course course), the code checks if the CHAT feature is enabled. Since this method pertains to events related to proactivity, it would be more appropriate to check if the PROACTIVITY feature is enabled.
Apply this diff to correct the feature check:
public void isActivatedForElseThrow(IrisEventType type, Course course) {
- isEnabledForElseThrow(IrisSubSettingsType.CHAT, course);
+ isEnabledForElseThrow(IrisSubSettingsType.PROACTIVITY, course);
if (!isActivatedFor(type, course)) {
throw new AccessForbiddenAlertException("The Iris " + type.name() + " event is disabled for this course.", "Iris",
"iris." + type.name().toLowerCase() + "Disabled");
}
}503-509:
Incorrect Feature Check in Exercise Method: Should Verify PROACTIVITY Instead of CHAT
Similarly, in the method isActivatedForElseThrow(IrisEventType type, Exercise exercise), the code checks if the CHAT feature is enabled. It should check the PROACTIVITY feature to align with the event type.
Apply this diff to correct the feature check:
public void isActivatedForElseThrow(IrisEventType type, Exercise exercise) {
- isEnabledForElseThrow(IrisSubSettingsType.CHAT, exercise);
+ isEnabledForElseThrow(IrisSubSettingsType.PROACTIVITY, exercise);
if (!isActivatedFor(type, exercise)) {
throw new AccessForbiddenAlertException("The Iris " + type.name() + " event is disabled for this exercise.", "Iris",
"iris." + type.name().toLowerCase() + "Disabled");
}
}759-779: 🛠️ Refactor suggestion
Simplify Event Validation Logic to Reduce Code Duplication
The method isEventEnabledInSettings contains duplicate logic within the switch cases for PROGRESS_STALLED and BUILD_FAILED. To improve readability and maintainability, consider refactoring the code to eliminate duplication.
Apply this diff to refactor the method:
private boolean isEventEnabledInSettings(IrisCombinedSettingsDTO settings, IrisEventType type) {
+ if (type == IrisEventType.PROGRESS_STALLED || type == IrisEventType.BUILD_FAILED) {
+ var disabledEvents = settings.irisChatSettings().disabledProactiveEvents();
+ return disabledEvents == null || !disabledEvents.contains(type.name().toLowerCase());
+ }
+ throw new IllegalStateException("Unexpected value: " + type); // TODO: Add JOL event, once Course Chat Settings are implemented
- return switch (type) {
- case PROGRESS_STALLED -> {
- if (settings.irisChatSettings().disabledProactiveEvents() != null) {
- yield !settings.irisChatSettings().disabledProactiveEvents().contains(IrisEventType.PROGRESS_STALLED.name().toLowerCase());
- }
- else {
- yield true;
- }
- }
- case BUILD_FAILED -> {
- if (settings.irisChatSettings().disabledProactiveEvents() != null) {
- yield !settings.irisChatSettings().disabledProactiveEvents().contains(IrisEventType.BUILD_FAILED.name().toLowerCase());
- }
- else {
- yield true;
- }
- }
- default -> throw new IllegalStateException("Unexpected value: " + type); // TODO: Add JOL event, once Course Chat Settings are implemented
- };
}Also, there's a TODO comment in the default case regarding adding the JOL event. Would you like me to create a GitHub issue to track the implementation of the JOL event?
Likely invalid or redundant comment.
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java
Show resolved
Hide resolved
github-actions
bot
added
the
communication
kaancayli
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
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
BUILD_FAILEDandPROGRESS_STALLED, enhancing the system's responsiveness to user interactions.PyrisEventDTOfor encapsulating event data along with its type.Bug Fixes
Documentation
Tests