Iris: Add FAQ consistency check for rewritting pipeline
#10437
Conversation
github-actions
bot
added
server
cremertim
changed the title
IRIS: Add FAQ consistency check for rewritting pipelineIris: Add FAQ consistency check for rewritting pipeline
github-actions
bot
added
tests
core
WalkthroughThe changes extend the rewriting pipeline functionality both on the backend and frontend. On the server side, the DTOs for rewriting pipeline execution and status update are updated to include additional parameters such as course ID, inconsistencies, suggestions, and improvement details, and the logging in the resource is modified. On the client side, new interfaces and Angular components are introduced to handle consistency check results, and the action flow is enhanced with a result signal. Test cases and i18n files are also updated accordingly. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🪧 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 (6)
src/main/webapp/app/faq/faq-consistency.component.html (2)
8-11: Consider semantic HTML structure improvements.The current HTML structure uses a div inside a list item which breaks standard list semantics.
- <div> - <li>{{ element.suggestion }}</li> - <p>{{ element.inconsistentFaq }}</p> - </div> + <li> + <div>{{ element.suggestion }}</div> + <p>{{ element.inconsistentFaq }}</p> + </li>
16-16: Remove unnecessary 'this' reference.In Angular templates, the 'this' keyword is optional and typically omitted for cleaner code.
- <div [innerHtml]="this.improvement()"></div> + <div [innerHtml]="improvement()"></div>src/main/webapp/app/faq/faq-update.component.ts (1)
194-201: Added method to dismiss consistency check resultsThe dismissConsistencyCheck method properly resets all fields in the consistency check result signal. However, consider extracting the default empty result object to a constant to avoid duplication between initialization and reset.
+ private readonly EMPTY_CONSISTENCY_CHECK_RESULT: RewriteResult = { + result: '', + inconsistencies: [], + suggestions: [], + improvement: '', + }; - renderedConsistencyCheckResultMarkdown = signal<RewriteResult>({ - result: '', - inconsistencies: [], - suggestions: [], - improvement: '', - }); + renderedConsistencyCheckResultMarkdown = signal<RewriteResult>(this.EMPTY_CONSISTENCY_CHECK_RESULT); dismissConsistencyCheck() { - this.renderedConsistencyCheckResultMarkdown.set({ - result: '', - inconsistencies: [], - suggestions: [], - improvement: '', - }); + this.renderedConsistencyCheckResultMarkdown.set(this.EMPTY_CONSISTENCY_CHECK_RESULT); }src/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingTest.java (1)
53-61: Test for instructor rewriting pipeline access.This test verifies that instructors can access the rewriting pipeline. However, it only checks the HTTP status code without verifying the actual response content or structure.
Consider enhancing this test to verify the actual response structure and content to ensure the rewriting pipeline returns the expected results with inconsistencies and suggestions. You could modify the test to:
@Test @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") void callRewritingPipeline() throws Exception { activateIrisFor(course1); irisRequestMockProvider.mockRewritingPipelineResponse(dto -> { + // Setup mock to return specific inconsistencies/suggestions }); PyrisRewriteTextRequestDTO requestDTO = new PyrisRewriteTextRequestDTO("test", RewritingVariant.FAQ); - request.postWithoutResponseBody("/api/iris/courses/" + course1.getId() + "/rewrite-text", requestDTO, HttpStatus.OK); + var response = request.post("/api/iris/courses/" + course1.getId() + "/rewrite-text", requestDTO, HttpStatus.OK, String.class); + // Assert response contains expected structure with inconsistencies/suggestions }src/test/javascript/spec/service/artemis-intelligence.service.spec.ts (2)
63-71: Consider using more specific spy expectations.While the test is correct, using
toHaveBeenCalledExactlyOnceWithinstead oftoHaveBeenCalledWithwould be more aligned with the coding guidelines.-expect(alertService.success).toHaveBeenCalledWith('artemisApp.markdownEditor.artemisIntelligence.alerts.rewrite.success'); +expect(alertService.success).toHaveBeenCalledExactlyOnceWith('artemisApp.markdownEditor.artemisIntelligence.alerts.rewrite.success');
124-135: Missing HTTP error test for consistencyCheck.While WebSocket errors are tested, there's no test for HTTP errors during consistency check.
Add a test for HTTP errors similar to the one for the rewrite method:
it('should handle HTTP error during consistency check', () => { const exerciseId = 42; service.consistencyCheck(exerciseId).subscribe({ next: () => fail('Expected an error'), error: (err) => expect(err.status).toBe(400), }); const req = httpMock.expectOne(`api/iris/consistency-check/exercises/${exerciseId}`); req.flush({ message: 'Error' }, { status: 400, statusText: 'Bad Request' }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisRewritingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/PyrisRewritingPipelineExecutionDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/PyrisRewritingStatusUpdateDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/web/IrisRewritingResource.java(0 hunks)src/main/webapp/app/exercises/programming/manage/instructions-editor/programming-exercise-editable-instruction.component.ts(2 hunks)src/main/webapp/app/faq/faq-consistency.component.html(1 hunks)src/main/webapp/app/faq/faq-consistency.component.ts(1 hunks)src/main/webapp/app/faq/faq-update.component.html(1 hunks)src/main/webapp/app/faq/faq-update.component.ts(4 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/artemis-intelligence/RewriteResult.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/artemis-intelligence/artemis-intelligence.service.ts(3 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/artemis-intelligence/rewrite.action.ts(3 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts(2 hunks)src/main/webapp/i18n/de/programmingExercise.json(1 hunks)src/main/webapp/i18n/en/programmingExercise.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java(3 hunks)src/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingTest.java(1 hunks)src/test/javascript/spec/service/artemis-intelligence.service.spec.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/de/tum/cit/aet/artemis/iris/web/IrisRewritingResource.java
🧰 Additional context used
📓 Path-based instructions (6)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/shared/monaco-editor/model/actions/artemis-intelligence/RewriteResult.tssrc/main/webapp/app/faq/faq-consistency.component.tssrc/main/webapp/app/shared/monaco-editor/model/actions/artemis-intelligence/rewrite.action.tssrc/main/webapp/app/exercises/programming/manage/instructions-editor/programming-exercise-editable-instruction.component.tssrc/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.tssrc/main/webapp/app/shared/monaco-editor/model/actions/artemis-intelligence/artemis-intelligence.service.tssrc/main/webapp/app/faq/faq-update.component.ts
`src/main/webapp/i18n/de/**/*.json`: German language transla...
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/i18n/de/programmingExercise.json
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
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/IrisRewritingService.javasrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/PyrisRewritingPipelineExecutionDTO.javasrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/PyrisRewritingStatusUpdateDTO.java
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
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/main/webapp/app/faq/faq-consistency.component.htmlsrc/main/webapp/app/faq/faq-update.component.html
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
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/core/connector/IrisRequestMockProvider.javasrc/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingTest.java
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
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/service/artemis-intelligence.service.spec.ts
🧠 Learnings (1)
src/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingTest.java (1)
Learnt from: alexjoham
PR: ls1intum/Artemis#9455
File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401
Timestamp: 2024-11-12T12:51:58.050Z
Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (37)
src/main/webapp/app/shared/monaco-editor/model/actions/artemis-intelligence/RewriteResult.ts (1)
1-6: Well-structured interface design.The RewriteResult interface provides a clear data structure for handling rewrite operations with consistency checking. The properties are well-named and follow the appropriate casing conventions.
src/main/webapp/i18n/en/programmingExercise.json (1)
750-751: Translation keys correctly implemented.The new translation keys for the consistency check feature are properly structured and have clear, descriptive values.
src/main/webapp/app/faq/faq-update.component.html (1)
29-36: Clean integration of the consistency check component.The faq-consistency-check component is properly integrated with appropriate properties and event binding. The usage of modern Angular @if syntax aligns with the coding guidelines.
src/main/webapp/i18n/de/programmingExercise.json (1)
750-753: LGTM: New translations added correctlyThe new German translations for the consistency check alert are concise and appropriate nouns. They correctly follow the guidelines for German translations, using "Verbesserungsempfehlung" and "Inkonsistenzen" as substantive terms.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/PyrisRewritingPipelineExecutionDTO.java (1)
14-14: LGTM: Added courseId parameter to the recordThe addition of the
courseIdparameter to the DTO record follows Java best practices:
- Uses camelCase for the parameter name
- Maintains the record's single responsibility of encapsulating rewriting pipeline execution data
- Provides the necessary context for course-specific FAQ consistency checks
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisRewritingService.java (2)
65-65: Improved enum handlingChanged from
variant.toString()tovariant.name().toLowerCase(), which provides more explicit control over the string representation of the enum value.
68-69: Updated constructor calls for enhanced functionalityThe changes properly expand the DTO and status update objects to support the new FAQ consistency check feature:
- Added
course.getId()to the PyrisRewritingPipelineExecutionDTO to provide course context- Updated PyrisRewritingStatusUpdateDTO with additional parameters for inconsistencies, suggestions, and improvement details
These changes align with the requirements for implementing the FAQ consistency check functionality.
src/main/webapp/app/faq/faq-consistency.component.ts (1)
1-39: Well-structured new component for FAQ consistency checksThe new
FaqConsistencyComponentfollows Angular best practices:
- Uses standalone component architecture
- Follows proper naming conventions with PascalCase for component class and camelCase for properties/methods
- Leverages Angular's input/output API for component communication
- Uses reactive signals and effects for state management
- Implements clean template/component separation
The component correctly formats and displays inconsistencies and suggestions, with a clear dismissal mechanism.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/PyrisRewritingStatusUpdateDTO.java (1)
15-22: Updates to DTO record with consistency check parametersThe DTO has been appropriately extended with the new parameters needed for the FAQ consistency check feature. The record uses proper Java patterns and includes all required fields with clear parameter documentation.
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java (2)
183-183: Method renamed for clarityThe method rename from
mockRunRewritingResponseAndtomockRewritingPipelineResponseimproves naming clarity and better reflects its purpose.
191-191: Updated parameter name for consistencyParameter name change from
executionDTOConsumertoresponseConsumerprovides better semantic clarity while maintaining the same functionality.src/main/webapp/app/exercises/programming/manage/instructions-editor/programming-exercise-editable-instruction.component.ts (2)
52-52: Added import for new RewriteResult interfaceThe import statement is correctly added to support the new consistency check functionality.
94-99: Updated RewriteAction instantiation with result signalThe RewriteAction constructor now properly includes the new signal parameter for handling consistency check results. This aligns with the updated rewrite pipeline that checks FAQ consistency.
src/main/webapp/app/faq/faq-update.component.ts (5)
26-27: Added imports for consistency check componentsThe imports for RewriteResult and FaqConsistencyComponent are correctly added to support the new consistency check functionality.
33-33: Updated component imports with FaqConsistencyComponentThe FaqConsistencyComponent is properly added to the component imports array.
54-59: Added signal for consistency check resultsThe RewriteResult signal is properly initialized with default empty values for all fields. This will store the consistency check results from the rewriting pipeline.
61-61: Added computed property for conditionally showing consistency checksThe computed property correctly determines whether to show consistency check results based on the presence of result content.
64-66: Updated artemisIntelligenceActions to use consistency check result signalThe RewriteAction is now correctly wired with the consistency check result signal, allowing it to store and update consistency check results.
src/main/webapp/app/shared/monaco-editor/model/actions/artemis-intelligence/rewrite.action.ts (3)
5-6: New imports added for the FAQ consistency check feature.The added imports for
WritableSignalandRewriteResultsuggest implementation of a reactive approach to handle the consistency check results. This aligns well with Angular's recommended practices for state management.
20-20: Constructor parameter added to handle rewrite results.The added
resultSignalparameter introduces a reactive way to communicate inconsistencies and suggestions back to the UI, which is essential for the FAQ consistency check feature.
30-30: Updated method call with new result signal parameter.The implementation correctly passes the
resultSignalto therewriteMarkdownmethod, ensuring that any inconsistencies discovered during the FAQ rewriting process are properly communicated.src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts (3)
14-14: New import for handling structured rewrite results.The addition of the
RewriteResultimport supports the structured response handling needed for the FAQ consistency check feature.
308-316: Method signature update with improved JSDoc.The
rewriteMarkdownmethod now accepts aresultSignalparameter with clear documentation about its purpose. This change properly supports the consistency check functionality by providing a channel to communicate inconsistencies back to the UI.
320-322: Enhanced rewrite result handling.The implementation now correctly:
- Uses the structured result's
resultproperty for the editor content- Updates the signal with the complete result object containing inconsistencies and suggestions
This implementation properly supports the new FAQ consistency check feature.
src/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingTest.java (2)
1-52: Well-structured test class setup.The test class follows JUnit 5 best practices with proper annotations, dependency injection, and test data setup. The
@BeforeEachmethod correctly initializes test users and courses needed for the tests.
63-72: Test for student access restriction.This test correctly verifies that students cannot access the rewriting pipeline, following good security testing practices. The empty string in the request DTO also tests boundary conditions.
src/main/webapp/app/shared/monaco-editor/model/actions/artemis-intelligence/artemis-intelligence.service.ts (3)
7-7: Import for structured rewrite result handling.The import of
RewriteResultsupports the enhanced functionality to handle inconsistencies and suggestions in the rewriting pipeline.
32-32: Updated return type for structured response.The method signature now correctly reflects that it returns a complex object containing not just the rewritten text but also inconsistencies and suggestions for the FAQ consistency check.
34-35: Observable creation for structured response.The Observable now properly defines the structure of the response object.
src/test/javascript/spec/service/artemis-intelligence.service.spec.ts (8)
5-5: Good import additions for enhanced testing.The addition of
of,throwErrorfrom rxjs, andAlertServiceproperly supports the new test cases.Also applies to: 10-10
18-29: Great mock implementation with comprehensive test data.The mock now returns additional properties (
inconsistencies,suggestions,improvement) which aligns with the PR objective of implementing FAQ consistency checks.
31-33: Properly mocked AlertService for testing success notifications.The mock service correctly implements the required method for testing.
54-54: Good test hygiene with jest.clearAllMocks().Clearing mocks after each test ensures test isolation and prevents side effects.
83-86: Good use of subscribed error handling.The error handling follows best practices with a proper failure check and specific status code expectation.
92-104: Well-structured WebSocket error test.This test appropriately handles the WebSocket error case and verifies the subscription was called.
107-122: Good implementation of consistency check test.The test properly verifies WebSocket topic subscription/unsubscription and result handling.
137-149: Excellent loading state testing.The test correctly verifies the loading state transitions and properly cleans up the subscription, preventing memory leaks.
.../app/shared/monaco-editor/model/actions/artemis-intelligence/artemis-intelligence.service.ts
Show resolved
Hide resolved
Note
This PR depends on the counterpart in IRIS. For testing, deploy Pyris#209 via
deploy:pyris-testif it is not already deployed (lock:pyris-test). You can only use TS3 for testing because you potentially need turn on the IRIS settings of the server with an admin account.Checklist
General
Server
Client
Motivation and Context
If a user creates a FAQ, the system should ensure that the newly created FAQ is consistent with all existing FAQs.
Description
I extended the rewrite pipeline of iris to show , wheter the provided information are consistent or not
Steps for Testing
Prerequisites:
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
Exam Mode Test
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Tests