Iris: Add chat in lectures
#9740
Conversation
…is into feature/iris/lecture-chat
isabellagessl
added
ready for review
server
github-actions
bot
added
tests
core
|
![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 (4)
src/test/javascript/spec/component/iris/settings/mock-settings.ts (1)
Line range hint
40-44: Consider updating mockEmptySettings()The
mockEmptySettings()function might need to explicitly setirisLectureChatSettingsto ensure proper testing of edge cases where lecture chat settings are not configured.export function mockEmptySettings() { const irisSettings = new IrisGlobalSettings(); irisSettings.id = 1; + irisSettings.irisLectureChatSettings = undefined; return irisSettings; }src/test/javascript/spec/component/iris/settings/iris-settings.service.spec.ts (3)
87-96: Enhance test specificity for better coverage.While the test structure is good, consider these improvements for better specificity:
it('should get uncombined lecture settings', fakeAsync(() => { const mockedSettings = mockSettings(); service .getUncombinedLectureSettings(1) .pipe(take(1)) - .subscribe((resp) => expect(resp).toEqual(mockedSettings)); + .subscribe((resp) => expect(resp).toContainEntries(Object.entries(mockedSettings))); - const req = httpMock.expectOne({ method: 'GET' }); + const req = httpMock.expectOne({ + method: 'GET', + url: '/api/iris/lectures/1/settings' + }); req.flush(mockedSettings, { status: 200, statusText: 'Ok' }); tick(); }));
120-129: Enhance test specificity and request verification.While the test structure is good, consider these improvements for better coverage:
it('should set lecture settings', fakeAsync(() => { const mockedSettings = mockSettings(); service .setLectureSettings(1, mockedSettings) .pipe(take(1)) - .subscribe((resp) => expect(resp.body).toEqual(mockedSettings)); + .subscribe((resp) => expect(resp.body).toContainEntries(Object.entries(mockedSettings))); - const req = httpMock.expectOne({ method: 'PUT' }); + const req = httpMock.expectOne({ + method: 'PUT', + url: '/api/iris/lectures/1/settings' + }); + expect(req.request.body).toEqual(mockedSettings); req.flush(mockedSettings, { status: 200, statusText: 'Ok' }); tick(); }));
Line range hint
87-129: Consider adding error handling test cases.The new lecture settings tests cover the happy path, but we should also test error scenarios. Consider adding test cases for:
- Network errors
- Invalid course IDs
- Server errors (4xx/5xx responses)
Example structure:
it('should handle errors when getting lecture settings', fakeAsync(() => { service .getUncombinedLectureSettings(1) .pipe(take(1)) .subscribe({ error: (error) => expect(error.status).toBe(404) }); const req = httpMock.expectOne({ method: 'GET', url: '/api/iris/lectures/1/settings' }); req.flush('Not found', { status: 404, statusText: 'Not Found' }); tick(); }));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/main/webapp/app/overview/course-lectures/course-lecture-details.component.html(1 hunks)src/test/javascript/spec/component/iris/settings/iris-settings.service.spec.ts(2 hunks)src/test/javascript/spec/component/iris/settings/mock-settings.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/overview/course-lectures/course-lecture-details.component.html
🧰 Additional context used
📓 Path-based instructions (2)
src/test/javascript/spec/component/iris/settings/iris-settings.service.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/iris/settings/mock-settings.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
🔇 Additional comments (3)
src/test/javascript/spec/component/iris/settings/mock-settings.ts (3)
5-5: LGTM! Import follows established patterns
The new import follows the existing pattern and is correctly grouped with related settings imports.
36-36: LGTM! Settings assignment is consistent
The assignment of lecture chat settings follows the established pattern and naming conventions.
26-28: Verify completeness of mock lecture chat settings
While the basic setup follows the established pattern, please verify if IrisLectureChatSubSettings has additional properties that should be mocked for comprehensive testing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/main/webapp/app/iris/settings/iris-settings-update/iris-settings-update.component.ts (2)
111-113: Consider refactoring sub-settings initialization for better maintainabilityWhile the initialization logic is correct, the
fillEmptyIrisSubSettingsmethod is growing with similar initialization blocks. Consider refactoring to reduce duplication and improve type safety.Here's a suggested refactor using a type-safe mapping approach:
- if (!this.irisSettings.irisLectureChatSettings) { - this.irisSettings.irisLectureChatSettings = new IrisLectureChatSubSettings(); - } + const settingsMap = { + irisChatSettings: IrisChatSubSettings, + irisTextExerciseChatSettings: IrisTextExerciseChatSubSettings, + irisLectureChatSettings: IrisLectureChatSubSettings, + irisLectureIngestionSettings: IrisLectureIngestionSubSettings, + irisCompetencyGenerationSettings: IrisCompetencyGenerationSubSettings, + } as const; + + if (this.irisSettings) { + Object.entries(settingsMap).forEach(([key, SettingsClass]) => { + if (!this.irisSettings![key as keyof typeof settingsMap]) { + this.irisSettings![key as keyof typeof settingsMap] = new SettingsClass(); + } + }); + }This refactor:
- Reduces code duplication
- Improves maintainability
- Provides better type safety
- Makes it easier to add new settings types in the future
Line range hint
156-182: Critical: Add support for LECTURE settings typeThe component doesn't handle the LECTURE settings type in the following methods:
loadParentIrisSettingsObservableloadIrisSettingsObservablesaveIrisSettingsObservableThis will prevent the lecture chat functionality from working correctly.
Add the LECTURE case to each method:
loadParentIrisSettingsObservable(): Observable<IrisSettings | undefined> { switch (this.settingsType) { case IrisSettingsType.GLOBAL: return new Observable<IrisSettings | undefined>(); case IrisSettingsType.COURSE: return this.irisSettingsService.getGlobalSettings(); case IrisSettingsType.EXERCISE: return this.irisSettingsService.getCombinedCourseSettings(this.courseId!); + case IrisSettingsType.LECTURE: + return this.irisSettingsService.getCombinedCourseSettings(this.courseId!); } } loadIrisSettingsObservable(): Observable<IrisSettings | undefined> { switch (this.settingsType) { case IrisSettingsType.GLOBAL: return this.irisSettingsService.getGlobalSettings(); case IrisSettingsType.COURSE: return this.irisSettingsService.getUncombinedCourseSettings(this.courseId!); case IrisSettingsType.EXERCISE: return this.irisSettingsService.getUncombinedExerciseSettings(this.exerciseId!); + case IrisSettingsType.LECTURE: + return this.irisSettingsService.getUncombinedLectureSettings(this.courseId!); } } saveIrisSettingsObservable(): Observable<HttpResponse<IrisSettings | undefined>> { switch (this.settingsType) { case IrisSettingsType.GLOBAL: return this.irisSettingsService.setGlobalSettings(this.irisSettings!); case IrisSettingsType.COURSE: return this.irisSettingsService.setCourseSettings(this.courseId!, this.irisSettings!); case IrisSettingsType.EXERCISE: return this.irisSettingsService.setExerciseSettings(this.exerciseId!, this.irisSettings!); + case IrisSettingsType.LECTURE: + return this.irisSettingsService.setLectureSettings(this.courseId!, this.irisSettings!); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/main/webapp/app/entities/iris/settings/iris-settings.model.ts(4 hunks)src/main/webapp/app/iris/settings/iris-settings-update/iris-settings-update.component.ts(2 hunks)src/main/webapp/app/iris/settings/shared/iris-enabled.component.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/webapp/app/entities/iris/settings/iris-settings.model.ts
- src/main/webapp/app/iris/settings/shared/iris-enabled.component.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/iris/settings/iris-settings-update/iris-settings-update.component.ts (1)
🔇 Additional comments (2)
src/main/webapp/app/iris/settings/iris-settings-update/iris-settings-update.component.ts (2)
14-14: LGTM: Import follows Angular style guide
The new import for IrisLectureChatSubSettings is correctly placed and follows naming conventions.
Line range hint 156-182: Verify the implementation of lecture-related service methods
Let's verify that the required service methods and types exist in the codebase.
src/main/java/de/tum/cit/aet/artemis/iris/repository/IrisLectureChatSessionRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/web/open/PublicPyrisStatusUpdateResource.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/main/java/de/tum/cit/aet/artemis/iris/repository/IrisLectureChatSessionRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/web/IrisLectureChatSessionResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/web/open/PublicPyrisStatusUpdateResource.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/de/tum/cit/aet/artemis/iris/web/IrisLectureChatSessionResource.java
- src/main/java/de/tum/cit/aet/artemis/iris/web/open/PublicPyrisStatusUpdateResource.java
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/iris/repository/IrisLectureChatSessionRepository.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 (3)
src/main/java/de/tum/cit/aet/artemis/iris/repository/IrisLectureChatSessionRepository.java (3)
1-17: LGTM! Clean and well-organized imports.
The imports follow Java best practices with appropriate use of static imports for constants and no wildcard imports.
18-20: LGTM! Properly configured repository interface.
The repository is correctly annotated and extends the appropriate base repository with proper type parameters.
22-27: LGTM! Well-structured query methods with appropriate fetch strategies.
The methods follow Spring Data JPA naming conventions and use @entitygraph appropriately for optimizing message loading.
src/main/java/de/tum/cit/aet/artemis/iris/repository/IrisLectureChatSessionRepository.java
Show resolved
Hide resolved
|
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. |
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisLectureChatSessionService.java
Show resolved
Hide resolved
| @Override | ||
| public IrisLectureChatSubSettings getIrisLectureChatSettings() { | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public void setIrisLectureChatSettings(IrisLectureChatSubSettings irisLectureChatSettings) { | ||
| } | ||
|
|
There was a problem hiding this comment.
Is there a reason that the setter sets nothing and the getter returns null?
| @Override | ||
| public IrisLectureIngestionSubSettings getIrisLectureIngestionSettings() { | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public void setIrisLectureIngestionSettings(IrisLectureIngestionSubSettings irisLectureIngestionSettings) { | ||
| } | ||
|
|
||
| @Override | ||
| public IrisChatSubSettings getIrisChatSettings() { | ||
| return irisChatSettings; | ||
| } | ||
|
|
||
| @Override | ||
| public void setIrisChatSettings(IrisChatSubSettings irisChatSettings) { | ||
| this.irisChatSettings = irisChatSettings; | ||
| } | ||
|
|
||
| @Override | ||
| public IrisTextExerciseChatSubSettings getIrisTextExerciseChatSettings() { | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public void setIrisTextExerciseChatSettings(IrisTextExerciseChatSubSettings irisTextExerciseChatSettings) { | ||
| } | ||
|
|
||
| @Override | ||
| public IrisLectureChatSubSettings getIrisLectureChatSettings() { | ||
| return irisLectureChatSettings; | ||
| } | ||
|
|
||
| @Override | ||
| public void setIrisLectureChatSettings(IrisLectureChatSubSettings irisLectureChatSettings) { | ||
| this.irisLectureChatSettings = irisLectureChatSettings; | ||
| } | ||
|
|
||
| @Override | ||
| public IrisCompetencyGenerationSubSettings getIrisCompetencyGenerationSettings() { | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public void setIrisCompetencyGenerationSettings(IrisCompetencyGenerationSubSettings irisCompetencyGenerationSubSettings) { | ||
| } |
There was a problem hiding this comment.
Same question here: is there a reason it sometimes returns null and the setter is doing nothing?
| @@ -72,6 +72,7 @@ protected void updateLatestSuggestions(S session, List<String> latestSuggestions | |||
| public TrackedSessionBasedPyrisJob handleStatusUpdate(TrackedSessionBasedPyrisJob job, PyrisChatStatusUpdateDTO statusUpdate) { | |||
| var session = (S) irisSessionRepository.findByIdWithMessagesAndContents(job.sessionId()); | |||
| IrisMessage savedMessage; | |||
|
|
|||
There was a problem hiding this comment.
Same here: this new line is not useful
| @@ -7,4 +7,5 @@ | |||
|
|
|||
| @JsonInclude(JsonInclude.Include.NON_EMPTY) | |||
| public record PyrisLectureDTO(long id, String title, String description, ZonedDateTime startDate, ZonedDateTime endDate, List<PyrisLectureUnitDTO> units) { | |||
|
|
|||
There was a problem hiding this comment.
This new line seems unintentional, its also the only change in that file
| } | ||
|
|
||
| @Override | ||
| public void setIrisLectureChatSettings(IrisLectureChatSubSettings irisLectureChatSettings) { |
There was a problem hiding this comment.
| public void setIrisLectureChatSettings(IrisLectureChatSubSettings irisLectureChatSettings) { | |
| @Override | |
| public void setIrisLectureChatSettings(IrisLectureChatSubSettings irisLectureChatSettings) { | |
| // Empty because exercises don't have lecture chat settings | |
| } | |
I think we should write a comment in the body as to why we've left the function body blank
| var allowedVariants = !minimal ? getCombinedAllowedVariants(settingsList, IrisSettings::getIrisChatSettings) : null; | ||
| var selectedVariant = !minimal ? getCombinedSelectedVariant(settingsList, IrisSettings::getIrisChatSettings) : null; | ||
| var enabledForCategories = !minimal ? getCombinedEnabledForCategories(settingsList, IrisSettings::getIrisChatSettings) : null; |
There was a problem hiding this comment.
Why do we use IrisSettings::getIrisChatSettings here?
|
|
||
| @Service | ||
| @Profile("iris") | ||
| public class IrisLectureChatSessionService implements IrisChatBasedFeatureInterface<IrisLectureChatSession>, IrisRateLimitedFeatureInterface { |
There was a problem hiding this comment.
| public class IrisLectureChatSessionService implements IrisChatBasedFeatureInterface<IrisLectureChatSession>, IrisRateLimitedFeatureInterface { | |
| public class IrisLectureChatSessionService AbstractIrisChatSessionService<IrisLectureChatSession> { |
Why not extend this class from AbstractIrisChatSessionService?
| } | ||
|
|
||
| public LectureChatJob handleStatusUpdate(LectureChatJob job, PyrisLectureChatStatusUpdateDTO statusUpdate) { | ||
| // TODO: LLM Token Tracking - or better, make this class a subclass of AbstractIrisChatSessionService |
There was a problem hiding this comment.
Is there a reason why the token tracking is not in the scope of this PR? I think we should be able to track the token usages for each session type.
| @Override | ||
| public void setIrisTextExerciseChatSettings(IrisTextExerciseChatSubSettings irisTextExerciseChatSettings) { | ||
| } |
There was a problem hiding this comment.
| @Override | |
| public void setIrisTextExerciseChatSettings(IrisTextExerciseChatSubSettings irisTextExerciseChatSettings) { | |
| } | |
| @Override | |
| public void setIrisTextExerciseChatSettings(IrisTextExerciseChatSubSettings irisTextExerciseChatSettings) { | |
| // Empty because lectures don't have text exercise chat settings | |
| } |
We should explain the in the body of the function, why we've left this function empty
| @Override | ||
| public void setIrisCompetencyGenerationSettings(IrisCompetencyGenerationSubSettings irisCompetencyGenerationSubSettings) { | ||
| } |
There was a problem hiding this comment.
| @Override | |
| public void setIrisCompetencyGenerationSettings(IrisCompetencyGenerationSubSettings irisCompetencyGenerationSubSettings) { | |
| } | |
| @Override | |
| public void setIrisCompetencyGenerationSettings(IrisCompetencyGenerationSubSettings irisCompetencyGenerationSubSettings) { | |
| // Empty because lectures don't have competency settings | |
| } |
| @Override | ||
| public void setIrisLectureIngestionSettings(IrisLectureIngestionSubSettings irisLectureIngestionSettings) { | ||
| } |
There was a problem hiding this comment.
| @Override | |
| public void setIrisLectureIngestionSettings(IrisLectureIngestionSubSettings irisLectureIngestionSettings) { | |
| } | |
| @Override | |
| public void setIrisLectureIngestionSettings(IrisLectureIngestionSubSettings irisLectureIngestionSettings) { | |
| } |
Here also, we should justify why it's empty.
There was a problem hiding this comment.
Why do we need a separate settings class for Lecture settings? I think we can place the IrisLectureChatSubSettings under the IrisCourseSettings and IrisGlobalSettings.
Checklist
General
Server
Client
Motivation and Context
For now Iris was only available in exercises and the course but now it is also available for lectures.
Description
Iris has been added to lectures.
Steps for Testing
Prerequisites:
Pipeline: Add lecture chat pipeline connection Pyris#173) to the pyris test serverTestserver 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
Release Notes
New Features
Bug Fixes
Documentation
Tests