Athena: Add popup for accepting external LLM usage when requesting feedback
#10187
Conversation
github-actions
bot
added
tests
server
ahmetsenturk
requested review from
maximiliansoelch,
LeonWehrhahn and
EneaGore
WalkthroughThe pull request introduces a comprehensive refactoring of user acceptance tracking, replacing references to "Iris" with "external LLM" across multiple files in the Artemis application. This change involves modifying user-related models, services, repositories, and components to shift from a specific Iris-related acceptance mechanism to a more generalized external Large Language Model (LLM) acceptance system. The modifications span both backend Java and frontend TypeScript files, ensuring consistent changes throughout the application's architecture. Changes
Suggested Labels
Suggested Reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
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
🧹 Nitpick comments (8)
src/test/java/de/tum/cit/aet/artemis/core/user/util/UserFactory.java (1)
93-93: Consider using a fixed date for deterministic testing.
UsingZonedDateTime.now()can cause subtle test issues in scenarios that require consistent timestamps. Adopting a fixed time (e.g.,ZonedDateTime.of(...)) can ensure truly repeatable tests.src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.ts (2)
80-82: Method to set user acceptance state is concise.
setUserAcceptedExternalLLMs()correctly retrievesexternalLLMAcceptedfromuserIdentity. Consider handling the scenario whereuserIdentitymight be undefined if the service has not yet loaded user data.setUserAcceptedExternalLLMs(): void { - this.userAccepted = !!this.accountService.userIdentity?.externalLLMAccepted; + const identity = this.accountService.userIdentity; + this.userAccepted = !!(identity && identity.externalLLMAccepted); }
84-94: Chained acceptance and feedback request could cause unexpected flow.In
acceptExternalLLMs(modal: any), you close the modal and immediately check conditions for feedback request. If the request to accept external LLM is still in transit, a race condition might arise. Consider deferring or chaining the feedback request only after the subscription completes to ensure safe ordering.src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (1)
79-79: Mandatory external LLM acceptance checks
The repeated calls touser.hasAcceptedExternalLLMElseThrow()enforce that the user has granted consent before using LLM services. Consider extracting a helper to reduce code repetition, if desired.Also applies to: 140-140, 155-155, 185-185
src/main/webapp/app/iris/base-chatbot/iris-base-chatbot.component.ts (1)
237-238: Consider unit testing acceptance logic.The method
checkIfUserAcceptedExternalLLM()correctly setsuserAccepted. Consider adding or enhancing tests that verify the component reacts to changes inexternalLLMAccepted.src/main/java/de/tum/cit/aet/artemis/core/domain/User.java (2)
536-537: Expose null-safe API.Returning a nullable
ZonedDateTimemight lead to defensive null-checking in consumers. Consider returning an Optional for improved clarity.
540-541: Setter is straightforward.The setter properly updates the acceptance date. If needed, add validation or logging to track acceptance changes.
src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.html (1)
43-53: Clear and user-friendly confirmation modal
The new modal structure is straightforward. Consider confirming it meets accessibility guidelines (focus management, ARIA labels) and brand requirements.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20250122114457_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (25)
src/main/java/de/tum/cit/aet/artemis/core/domain/User.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/dto/UserDTO.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/core/repository/UserRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/UserResource.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/IrisSessionService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/iris/web/IrisCourseChatSessionResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/iris/web/IrisTextExerciseChatSessionResource.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java(1 hunks)src/main/webapp/app/core/user/user.model.ts(3 hunks)src/main/webapp/app/core/user/user.service.ts(1 hunks)src/main/webapp/app/iris/base-chatbot/iris-base-chatbot.component.ts(2 hunks)src/main/webapp/app/iris/iris-chat.service.ts(3 hunks)src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.html(2 hunks)src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.ts(7 hunks)src/main/webapp/i18n/de/exercise-actions.json(1 hunks)src/main/webapp/i18n/en/exercise-actions.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/core/user/util/UserFactory.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java(1 hunks)src/test/javascript/spec/component/iris/iris-chat.service.spec.ts(1 hunks)src/test/javascript/spec/component/iris/ui/exercise-chatbot-button.component.spec.ts(1 hunks)src/test/javascript/spec/component/iris/ui/iris-base-chatbot.component.spec.ts(3 hunks)src/test/javascript/spec/component/overview/exercise-details/request-feedback-button/request-feedback-button.component.spec.ts(11 hunks)src/test/javascript/spec/service/user.service.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (24)
src/main/java/de/tum/cit/aet/artemis/iris/web/IrisCourseChatSessionResource.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/IrisSessionService.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/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/web/IrisTextExerciseChatSessionResource.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/webapp/i18n/de/exercise-actions.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/main/webapp/app/core/user/user.service.ts (1)
src/test/javascript/spec/component/iris/ui/exercise-chatbot-button.component.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/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.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/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/test/javascript/spec/service/user.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/main/webapp/app/iris/iris-chat.service.ts (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
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.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/javascript/spec/component/iris/iris-chat.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/main/webapp/app/core/user/user.model.ts (1)
src/main/webapp/app/iris/base-chatbot/iris-base-chatbot.component.ts (1)
src/test/java/de/tum/cit/aet/artemis/core/user/util/UserFactory.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/core/repository/UserRepository.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/core/dto/UserDTO.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/javascript/spec/component/overview/exercise-details/request-feedback-button/request-feedback-button.component.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/ui/iris-base-chatbot.component.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/main/java/de/tum/cit/aet/artemis/core/web/UserResource.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/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.ts (1)
src/main/java/de/tum/cit/aet/artemis/core/domain/User.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
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: server-tests
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Analyse
- GitHub Check: Mend Security Check
🔇 Additional comments (75)
src/main/webapp/app/core/user/user.service.ts (1)
45-48: Looks great!
This new method for accepting an external LLM policy aligns well with the Angular style guidelines. The template literal usage is valid since it interpolatesthis.resourceUrl.src/main/webapp/app/core/user/user.model.ts (1)
18-18: New acceptance property is consistent and well-named.
ReplacingirisAcceptedwithexternalLLMAcceptedfollows the camelCase convention, and usingdayjs.Dayjsfor date handling is in line with existing patterns. These changes are cohesive with the broader refactor, ensuring consistency.Also applies to: 39-39, 52-52
src/test/javascript/spec/service/user.service.spec.ts (1)
59-62: Test correctly verifies the new acceptance endpoint.
The test name is descriptive, and the URL check matches the updated service method. This ensures coverage of the external LLM acceptance path.src/main/java/de/tum/cit/aet/artemis/iris/web/IrisCourseChatSessionResource.java (3)
77-77: Potential mismatch between HTTP method annotation and Javadoc description.The Javadoc refers to "GET course-chat/{courseId}/sessions/current," yet the method is annotated with
@PostMapping. This can cause confusion for API clients and maintainers. Consider aligning the HTTP verb and documentation for clarity.
96-96: Consistent user acceptance check.Good job replacing
hasAcceptedIrisElseThrow()withhasAcceptedExternalLLMElseThrow(). The logic is straightforward, and the throw-based mechanism cleanly handles rejection scenarios.
116-116: Appropriate check for external LLM acceptance.The shift to
hasAcceptedExternalLLMElseThrow()aligns with the new external LLM acceptance policy. The code remains concise and appears correct.src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.ts (5)
17-19: New imports expand dependency scope.The introduction of
AccountService,UserService, andNgbModalis appropriate given the new external LLM acceptance flow and modal-based user interaction. Ensure these imports remain in sync with the Angular module declarations if they evolve in the future.
32-32: Boolean property naming.
userAcceptedis clearly communicated and follows the camelCase guideline. No issues detected.
48-50: Injected services follow Angular's recommended pattern.The usage of
inject(...)and separate private fields for each service is consistent. This maintains readability and aligns with the Angular style guide.
64-64: Initialization of user acceptance state.Calling
setUserAcceptedExternalLLMs()inngOnInit()is straightforward and ensuresuserAcceptedis always in sync with the user's profile at component load.
96-105: Modal-based confirmation logic is user-friendly.By opening a modal and returning if the user is not accepted, you ensure clarity in the user experience. The subsequent
if (!this.assureConditionsSatisfied()) { ... }approach is straightforward and meets the single-responsibility principle.src/main/java/de/tum/cit/aet/artemis/iris/service/IrisSessionService.java (1)
71-71: User acceptance check consistency.Replacing the Iris-specific acceptance check with
user.hasAcceptedExternalLLMElseThrow();aligns with the new policy. The method name remains clear, and the throw-based approach ensures immediate termination if the user has not yet consented.src/main/java/de/tum/cit/aet/artemis/iris/web/IrisTextExerciseChatSessionResource.java (2)
101-101: Aligned with external LLM acceptance.
user.hasAcceptedExternalLLMElseThrow();properly replaces the Iris-specific acceptance check. No logical issues are apparent in this context.
123-123: Ensures external LLM acceptance before retrieving sessions.This call is consistent with the updated acceptance policy and blocks access unless the user has agreed to external LLM usage.
src/test/javascript/spec/component/iris/ui/exercise-chatbot-button.component.spec.ts (2)
42-42: Renaming mock function to align with new acceptance logic
Switching fromacceptIristoacceptExternalLLMaccurately reflects the broader context of external LLM usage and ensures test consistency.
45-45: Refactor userIdentity property for external LLM acceptance
ReplacingirisAcceptedwithexternalLLMAcceptedis consistent with the revised acceptance model. Usingdayjs()is appropriate for mocking the date-based acceptance.src/main/java/de/tum/cit/aet/artemis/core/dto/UserDTO.java (6)
79-79: Introducing the externalLLMAccepted field
Renaming the acceptance field clarifies its purpose, aligning with the system-wide shift away from "irisAccepted."
88-89: Updating constructor argument to use externalLLMAccepted
Incorporating the external LLM acceptance timestamp into the constructor ensures the new field is properly initialized.
118-118: Synchronizing field assignment
Explicitly settingthis.externalLLMAcceptedensures the acceptance time is accurately propagated.
278-279: Getter for externalLLMAccepted
This method correctly exposes the acceptance timestamp for external LLM usage.
282-283: Setter for externalLLMAccepted
Allowing updates to the acceptance timestamp further aligns with the overall acceptance model.
94-94: Adding externalLLMAccepted to the constructor signature
Including this parameter aligns the DTO with the updated user domain model.To confirm no calls to the old constructor remain, run:
✅ Verification successful
Constructor change verification successful
The codebase consistently uses the User object constructor overload or empty constructor with specific setters. No instances of the full parameter constructor were found, indicating the change is safe.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify references to the updated constructor rg -A5 "new\s+UserDTO"Length of output: 6924
src/main/java/de/tum/cit/aet/artemis/core/web/UserResource.java (1)
103-103: Clearing externalLLMAccepted in search results
NullifyingexternalLLMAcceptedhere prevents exposing acceptance data unnecessarily in the client response.src/test/javascript/spec/component/iris/iris-chat.service.spec.ts (2)
41-41: Renamed mock method aligns well with new acceptance logic
The rename ofacceptIristoacceptExternalLLMkeeps the test suite consistent with the PR’s updated naming convention.
44-44: Updated property name reflects external LLM acceptance
ReplacingirisAcceptedwithexternalLLMAcceptedensures consistent language across the codebase.src/test/javascript/spec/component/overview/exercise-details/request-feedback-button/request-feedback-button.component.spec.ts (28)
2-2: Imported TemplateRef for modal usage
IntroducingTemplateRefhelps in passing template-based popups to the component.
18-18: Importing NgbModal and NgbTooltipModule
Using these modules facilitates modal interactions and tooltips within the test environment.
31-32: Extended testbed imports & providers
IncludingNgbModalandNgbTooltipModulein the test configuration is consistent with the new UI popup approach.
54-58: Helper function for consistent test setup
initAndTick()centralizes fixture initialization and change detection logic, reducing boilerplate.
60-68: Utility function for constructing a base Exercise
createBaseExercise()clarifies the test setup and improves maintainability.
70-76: Encapsulation of student participation creation
createParticipation()is a neat way to generate mock participations for test scenarios.
78-88: Centralized method for setting component inputs
setupComponentInputs()refactors repeated code and improves readability of test cases.
89-90: Well-labeled test for requestFeedback error scenario
Renaming or clarifying the test purpose helps ensure clarity regarding expected error-handling outcomes.
Line range hint
91-103: Thorough error-handling verification
The mock error response and alert checks confirm the component gracefully handles failed feedback requests.
111-112: Base exercise creation for non-exam scenario
Creating a standard text exercise to validate the Athena button presence is a solid approach.
114-114: Calling initAndTick for stable test state
Ensures the component has fully updated before assertions.
123-124: Exercise creation for exam scenario
Correctly checks that the request feedback button does not display in exam contexts.
126-126: Synchronous test initialization
initAndTick()used again to handle lifecycle updates before verifications.
136-137: Validates button state with missing participation
The test ensures the button is disabled if the user cannot request feedback.
139-139: Another call to initAndTick
Consistent usage helps unify test patterns.
148-150: Comprehensive scenario building
CombiningcreateParticipation()andcreateBaseExercise()fosters clarity in test data setup.
153-153: initAndTick invocation
Maintains consistent test flow to settle asynchronous changes.
164-167: Coverage for user acceptance set to true
Ensures the code path that bypasses the modal is tested.
169-169: initAndTick for finalizing changes
Allows the test to capture the component’s updated state properly.
172-172: Spying on requestFeedback
Covers the service’s call from the component and ensures it is triggered upon button click.
184-186: Mock scenario for userAccepted = true
Consistently tests skipping the consent popup logic.
191-191: Alert mechanism for invalid feedback request
Verifies the user sees a warning message as expected.
198-200: Base exercise without final submission
Validates disabling the request feedback button under incomplete conditions.
202-202: Ensuring stable state with initAndTick
Reduces timing-related flaky tests.
211-213: Button enabled for valid submission
Tests the scenario where the user can request feedback if the submission is complete.
215-215: Another initAndTick usage
Consistent approach to ensuring the DOM is updated.
222-241: Testing modal open for userAccepted = false
Confirms correct behavior when the user must first consent to external LLM usage.
243-262: Verifying absence of modal for userAccepted = true
Ensures direct feedback request proceeds without prompting.src/main/webapp/app/iris/iris-chat.service.ts (3)
48-48: Explicit naming for acceptance flag
hasJustAcceptedExternalLLMclarifies its purpose, matching the new external LLM naming convention.
59-59: Conditional check for externalLLMAccepted
Ensures new session creation only if the user is recognized as having given consent.
155-156: Refined acceptance logic
ReplacingacceptIriswithacceptExternalLLMsolidifies the new acceptance API usage throughout the code.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java (1)
217-217: Condition updated to reflect external LLM acceptance
Switching fromhasAcceptedIris()tohasAcceptedExternalLLM()aligns MessagingService with the rebranded acceptance flow.src/main/webapp/app/iris/base-chatbot/iris-base-chatbot.component.ts (1)
204-204: Validate acceptance check invocation.Invoking
checkIfUserAcceptedExternalLLM()here ensures that the user acceptance state is fresh. No issues detected.src/main/java/de/tum/cit/aet/artemis/core/domain/User.java (3)
213-214: Field naming and usage look good.Storing acceptance timestamps in
externalLLMAcceptedmatches the updated naming. Ensure that timezone handling is consistent across the application.
544-545: Boolean check is concise.The
hasAcceptedExternalLLM()method is self-explanatory and succinct. No further issues found.
549-554: Ensures enforcement of acceptance requirement.Throwing an
AccessForbiddenExceptionhere cleanly enforces user acceptance of the policy.src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (2)
92-92: Align test data with acceptance refactor.Setting
externalLLMAcceptedTimestampis consistent with the new field. Consider adding a test case for a user without acceptance.
95-95: Add negative test case if applicable.Similarly, using the new acceptance timestamp for
student2looks fine. Test coverage of unaccepted students might be beneficial.src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (2)
323-323: Guard condition is properly updated.Calling
user.hasAcceptedExternalLLMElseThrow()ensures sessions cannot be created without acceptance.
344-344: Check acceptance for new sessions.This call again judiciously prevents non-accepted users from creating chat sessions. Logic is consistent with the updated approach.
src/test/javascript/spec/component/iris/ui/iris-base-chatbot.component.spec.ts (5)
53-53: Consistent mock method naming
Renaming fromacceptIristoacceptExternalLLMin the mock service aligns well with the new acceptance policy.
56-56: Good default acceptance date usage
Usingdayjs()forexternalLLMAcceptedensures the user is recognized as having accepted the policy.
61-61: Duplicate acceptance mock
This snippet is the same as line 56 and doesn't introduce new logic.
117-117: Appropriate test scenario
VerifyingexternalLLMAccepted = undefinedsetscomponent.userAcceptedto false is a solid test case.
128-128: Spy usage for policy acceptance
Spying onmockUserService.acceptExternalLLMneatly confirms the call when accepting the policy.src/main/java/de/tum/cit/aet/artemis/core/repository/UserRepository.java (1)
745-748: Successful adaptation to external LLM acceptance
UpdatingexternalLLMAcceptedand renaming the method toupdateExternalLLMAcceptedToDatealigns with the new acceptance logic.src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.html (1)
6-6: Proper integration of popup parameter
PassingpopuptorequestFeedbackis a clear approach. Verify the method handles fallback scenarios if the popup is not needed, and confirm the disabled state reflects acceptance requirements.Also applies to: 16-16, 18-18, 20-20, 26-26, 29-29, 31-31, 33-33, 39-39
src/main/webapp/i18n/en/exercise-actions.json (2)
73-73: Minor punctuation tweak
No functional change; the punctuation adjustment is acceptable.
74-78: Localized external LLM consent prompts
The newly added keys accurately capture the consent message. Ensure translations are synchronized across other supported locales.src/main/webapp/i18n/de/exercise-actions.json (1)
74-78: LGTM! German translations follow the informal language guidelines.The translations consistently use the informal "du" form as required, and the message is clear and properly structured.
src/main/java/de/tum/cit/aet/artemis/core/web/UserResource.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/repository/UserRepository.java
Outdated
Show resolved
Hide resolved
...app/overview/exercise-details/request-feedback-button/request-feedback-button.component.html
Outdated
Show resolved
Hide resolved
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
src/test/javascript/spec/component/iris/ui/iris-base-chatbot.component.spec.ts
Outdated
Show resolved
Hide resolved
ahmetsenturk
dismissed stale reviews from HawKhiem and coderabbitai[bot]
via
b5a9f86
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
krusche
changed the title
Athena: Popup for accepting external LLM usage when requesting feedbackAthena: Add popup for accepting external LLM usage when requesting feedback
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
There was a problem hiding this comment.
Tested on TS1, works as expected.
Code looks good 👍
However, I am not sure if the translation adaption/generalization for Iris is ideal (see comment)
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
Checklist
General
Server
Client
Motivation and Context
Since the feedback generated by Athena currently relies on external LLM services (e.g., Azure OpenAI), students' data is processed outside of university premises. Therefore, it is necessary to ask students before clicking on the "Request AI Feedback" button if they agree with this situation.
Description
Iris already asks about this to students to get their permission, and there is already a column in the user model
iris_acceptedthat holds the date students accepted this term. This attribute has been renamedexternal_llm_acceptedand related methods have been updated.Also, a popup asks - if the user has not accepted this term before - about this term to the user before generating AI feedback.
Steps for Testing
Prerequisites:
external_llm_acceptedattribute equals toNULL, 1 with set to a timestamp)1. Testcase: Request Feedback from Student who has not accepted external LLM usage
2. Testcase: Request Feedback from Student who has already accepted external LLM usage
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
Test Coverage
Client
Server
Coverage artifact not found, tests probably failed.
Screenshots
Summary by CodeRabbit
Summary by CodeRabbit
Based on the comprehensive changes across multiple files, here are the updated release notes:
New Features
User Experience
Privacy and Compliance
Technical Updates