Development: Migrate client code for complaints
#10085
Conversation
github-actions
bot
added
tests
client
maximiliansoelch
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
maximiliansoelch
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThis pull request focuses on modernizing the Angular module and component structure within the Artemis application, particularly in the complaints-related modules. The changes primarily involve converting components to standalone components, removing deprecated modules, and simplifying dependency injection using Angular's Changes
Suggested labels
Suggested reviewers
Possibly related PRs
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: 0
🧹 Nitpick comments (2)
src/main/webapp/app/complaints/complaints-for-students/complaints-student-view.component.ts (1)
32-36: Good switch to service injection.Using Angular's
injectfunction declutters the constructor and clarifies dependencies. As a best practice, remember to handle subscription cleanup, especially for any streams to prevent potential memory leaks.src/main/webapp/app/complaints/complaint-response.service.ts (1)
15-16: Modernize service injection.Eliminating the constructor in favor of
inject()is a neat approach to reduce boilerplate. Confirm consistency across other services—mixing constructor injection andinject()can lead to confusion if used inconsistently.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/main/webapp/app/assessment/assessment-shared.module.ts(1 hunks)src/main/webapp/app/complaints/complaint-response.service.ts(2 hunks)src/main/webapp/app/complaints/complaint.service.ts(2 hunks)src/main/webapp/app/complaints/complaints-for-students/complaints-student-view.component.ts(2 hunks)src/main/webapp/app/complaints/complaints-for-tutor/complaints-for-tutor.component.ts(2 hunks)src/main/webapp/app/complaints/complaints-for-tutor/complaints-for-tutor.module.ts(0 hunks)src/main/webapp/app/complaints/complaints.module.ts(1 hunks)src/main/webapp/app/complaints/form/complaints-form.component.ts(1 hunks)src/main/webapp/app/complaints/list-of-complaints/list-of-complaints.component.ts(2 hunks)src/main/webapp/app/complaints/list-of-complaints/list-of-complaints.module.ts(1 hunks)src/main/webapp/app/complaints/request/complaint-request.component.ts(1 hunks)src/main/webapp/app/complaints/response/complaint-response.component.ts(1 hunks)src/main/webapp/app/course/manage/course-management.module.ts(2 hunks)src/main/webapp/app/exercises/programming/assess/programming-assessment.module.ts(2 hunks)src/main/webapp/app/exercises/programming/participate/programming-repository.module.ts(2 hunks)src/main/webapp/app/exercises/text/assess/text-submission-assessment.module.ts(2 hunks)src/test/javascript/spec/component/assessment-shared/assessment-layout.component.spec.ts(1 hunks)src/test/javascript/spec/component/complaints/complaints-for-tutor.component.spec.ts(2 hunks)src/test/javascript/spec/component/complaints/complaints-form.component.spec.ts(1 hunks)src/test/javascript/spec/component/complaints/list-of-complaints.component.spec.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/main/webapp/app/complaints/complaints-for-tutor/complaints-for-tutor.module.ts
✅ Files skipped from review due to trivial changes (1)
- src/test/javascript/spec/component/complaints/complaints-form.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (18)
src/main/webapp/app/complaints/complaints.module.ts (1)
src/main/webapp/app/complaints/complaint-response.service.ts (1)
src/main/webapp/app/exercises/programming/assess/programming-assessment.module.ts (1)
src/main/webapp/app/complaints/response/complaint-response.component.ts (1)
src/main/webapp/app/complaints/list-of-complaints/list-of-complaints.module.ts (1)
src/main/webapp/app/course/manage/course-management.module.ts (1)
src/main/webapp/app/complaints/complaint.service.ts (1)
src/main/webapp/app/exercises/programming/participate/programming-repository.module.ts (1)
src/main/webapp/app/assessment/assessment-shared.module.ts (1)
src/test/javascript/spec/component/complaints/complaints-for-tutor.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/assessment-shared/assessment-layout.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/webapp/app/complaints/request/complaint-request.component.ts (1)
src/main/webapp/app/complaints/list-of-complaints/list-of-complaints.component.ts (1)
src/test/javascript/spec/component/complaints/list-of-complaints.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/webapp/app/complaints/complaints-for-students/complaints-student-view.component.ts (1)
src/main/webapp/app/exercises/text/assess/text-submission-assessment.module.ts (1)
src/main/webapp/app/complaints/complaints-for-tutor/complaints-for-tutor.component.ts (1)
src/main/webapp/app/complaints/form/complaints-form.component.ts (1)
🔇 Additional comments (40)
src/main/webapp/app/complaints/complaints-for-students/complaints-student-view.component.ts (3)
1-1: Nice use of Angular'sinjectin the imports.This aligns well with modern Angular architecture and improves the clarity of dependencies by removing constructor parameters.
28-29: Correct standalone component declaration.Declaring the component as stand-alone with its own imports is a best practice in modern Angular, allowing more self-contained and modular code.
17-22: Imports properly configured for a standalone component.These newly imported directives and components are consistent with Angular's standalone approach. Make sure each is used or needed to minimize unnecessary imports.
✅ Verification successful
All imported components and directives are properly used in the standalone component
Based on the search results, all the imported components and directives in
complaints-student-view.component.tsare correctly used in its imports array:imports: [TranslateDirective, FaIconComponent, ComplaintsFormComponent, ComplaintRequestComponent, ComplaintResponseComponent, ArtemisTranslatePipe]Each imported component/directive is:
- Properly declared as standalone
- Used within the component's imports array
- Part of the complaints feature module functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Checking usage of newly imported directives & components rg -A 5 "TranslateDirective|FaIconComponent|ComplaintsFormComponent|ComplaintRequestComponent|ComplaintResponseComponent|ArtemisTranslatePipe" src/main/webapp/app/complaintsLength of output: 13918
src/main/webapp/app/complaints/response/complaint-response.component.ts (2)
3-6: Validate necessity of newly imported modules.
While these modules (NgbTooltip, FormsModule, ArtemisSharedCommonModule, ArtemisTranslatePipe) are often used in templates, verify each is truly needed. If any are unused, consider removing them to reduce bundle size and complexity.
11-12: Good use of Angular standalone components.
Definingstandalone: trueand adding necessary imports aligns with modern Angular best practices. Just ensure that other modules referencing this component remove any redundant declarations.src/main/webapp/app/complaints/list-of-complaints/list-of-complaints.component.ts (5)
1-1: Good use of standalone injection.Using
injectdirectly in your imports clarifies dependencies and aligns well with modern Angular best practices, as recommended by the style guide.
11-11: Tooltip import looks good.Your addition of
NgbTooltipis consistent with ng-bootstrap usage. Ensure you're referencing it in the template for clarity and keep track of possible memory leaks only if tooltips persist across routes.
18-22: Expansion of imports for standalone usage is correct.Bringing in
TranslateDirective,FormsModule,ArtemisSharedCommonModule,FaIconComponent, andArtemisTranslatePipeenables the necessary functionality within this component. This change respects the Angular style guide's recommended approach for standalone components.
27-28: Properly declared standalone component.Defining
standalone: trueand including these modules inimportsis fully compliant with the new Angular standalone pattern.
31-40: Appropriate usage ofinject.All service injections look clean and consistent with the recommended Angular approach. This pattern reduces constructor boilerplate.
src/main/webapp/app/complaints/list-of-complaints/list-of-complaints.module.ts (1)
10-10: Correct module configuration for the standalone component.Importing
ListOfComplaintsComponentin theimportsarray instead of thedeclarationsarray is the right move for a standalone component. This setup matches Angular best practices.src/main/webapp/app/complaints/request/complaint-request.component.ts (2)
3-7: Enhanced imports for expanded functionality.The addition of
NgbTooltip,TranslateDirective,FormsModule,ArtemisSharedCommonModule, andArtemisTranslatePipeensures a robust feature set for your component. The single-quote style and usage conform to the guidelines.
12-13: Refactor to a standalone component is successful.Declaring
standalone: trueand specifying necessary dependencies inimportsis consistent with the recommended approach for Angular standalone components.src/main/webapp/app/complaints/complaints.module.ts (1)
12-12: Switch to importing standalone components.Replacing declarations with imports for
ComplaintsFormComponent,ComplaintsStudentViewComponent,ComplaintRequestComponent, andComplaintResponseComponentis in line with your migration strategy toward standalone components.src/main/webapp/app/assessment/assessment-shared.module.ts (2)
20-20: Direct import of ComplaintsForTutorComponent recognized.Adding
ComplaintsForTutorComponentensures the tutor complaint functionality is available. Make sure to re-check routing references if you removed a related module.
27-27: Module imports revised correctly.Importing the standalone
ComplaintsForTutorComponentin place of the previous module is consistent with the new architecture pattern.src/main/webapp/app/exercises/programming/assess/programming-assessment.module.ts (2)
20-20: Confirm import consistency and single quote usage.Importing the
ComplaintsForTutorComponentdirectly is consistent with the single-quote requirement from the styleguide. Ensure that this path is correct and that any reusability or shared logic is factored into a shared location if it’s used widely across multiple modules.
29-29: Validate standalone component import.By adding
ComplaintsForTutorComponentto theimportsarray, you’re presumably treating it as a standalone component. Confirm that it is indeed declared as standalone in its own file. If it requires additional Angular modules (e.g.CommonModule), ensure these are included in the component’s standalone imports.src/main/webapp/app/exercises/text/assess/text-submission-assessment.module.ts (2)
23-23: Check that the new import path is correct.The import path for the
ComplaintsForTutorComponentlooks valid. Verify that the directory structure and naming conventions conform to the Angular styleguide.
[approve]
33-33: Assess standalone usage.Adding
ComplaintsForTutorComponentto theimportsarray suggests it is a standalone component. Ensure that all necessary providers, pipes, and modules are globally accessible if the component depends on them.src/main/webapp/app/exercises/programming/participate/programming-repository.module.ts (2)
22-22: Import path and single quotes are consistent.The import statement for
ComplaintsForTutorComponentis aligned with the single-quote standard. Verify that the referenced filecomplaints-for-tutor.component.tsis indeed the correct location.
31-31: Ensure standalone correctness.Placing the
ComplaintsForTutorComponentin theimportsarray is correct if the component is truly standalone. Double-check nested dependencies and any relevant entry components or dynamic module imports.src/main/webapp/app/complaints/complaint-response.service.ts (1)
1-1: Confirm Angular version compatibility.Using
injectrequires Angular 14+. Ensure that the project’s Angular version is at least 14 before adopting the new injection mechanism.src/test/javascript/spec/component/assessment-shared/assessment-layout.component.spec.ts (1)
25-25: Ensure mock usage in imports is aligned with best practices.
MovingComplaintsForTutorComponentfromdeclarationstoimportsis correct for a standalone component mock. This helps prevent potential conflicts in the test environment. No further changes needed.src/main/webapp/app/complaints/form/complaints-form.component.ts (3)
2-2: Confirm usage of new imports for standalone approach.
The direct import ofTranslateDirective,FormsModule,TextareaModule, andArtemisTranslatePipeis valid for a standalone component. Ensure each imported module or directive is strictly needed to prevent bloat.Also applies to: 10-13
19-20: Standalone component configuration is properly adopted.
Enablingstandalone: trueand specifyingimportsfor the component fosters clearer, localized dependencies. This aligns with the Angular style guideline on standalone components.
23-25: Validate usage of the Angularinjectfunction for services.
Switching toinject()clarifies dependency usage. Ensure that any side effects or initialization steps formerly in the constructor are still properly handled (if applicable).src/main/webapp/app/course/manage/course-management.module.ts (1)
71-71: Standalone complaints component usage.
ImportingComplaintsForTutorComponentin place of the former module is consistent with the new standalone component structure. Double-check that all references to the old module have been removed to avoid dead code.Also applies to: 100-100
src/main/webapp/app/complaints/complaints-for-tutor/complaints-for-tutor.component.ts (3)
1-1: Review added imports for form and translation handling.
UsingTranslateDirective,FormsModule, andTextareaModulein a standalone component is appropriate. Confirm thatArtemisSharedCommonModulehas no conflicting providers with the new approach.Also applies to: 16-20
27-28: Properly declaring the component as standalone.
The use ofstandalone: trueand explicitly listing imports fosters a self-contained component design.
31-36: Check lifecycle dependencies after constructor removal.
Injecting all services viainject()is fine, but verify that any logic previously handled in a constructor is adequately moved tongOnInitor other lifecycle hooks, if needed.src/main/webapp/app/complaints/complaint.service.ts (3)
34-34: Use of standalone inject approach
Replacing the constructor withinject(HttpClient)is a valid approach for newer Angular versions. Ensure consumers of this service align with Angular >= v14 to avoid compatibility issues.
35-35: Direct injection of ComplaintResponseService
This is consistent with the updated Angular injection style. Verify that no legacy usage patterns rely on constructor injection for testing or reflection-based libraries.
37-37: Maintain consistent naming for resourceUrl
Using a well-defined property likeresourceUrlis consistent with existing patterns. No further issues found.src/test/javascript/spec/component/complaints/list-of-complaints.component.spec.ts (3)
27-27: Importing shared module
You addedArtemisSharedCommonModuleimport but then removed it in the override. Confirm whether the module is actually needed in tests or if using direct imports for its components/pipes is more optimal.
67-67: Declarations array clarifications
Here, you only declare mocks and pipe stubs. Verify that the component under test doesn’t require additional declarations or imports after the override removal ofArtemisSharedCommonModule.
[approve]
79-81: Selective override for component
RemovingArtemisSharedCommonModuleimport fromListOfComplaintsComponentin the test environment can help isolate tests. Ensure that none of the shared services/pipes are inadvertently needed.src/test/javascript/spec/component/complaints/complaints-for-tutor.component.spec.ts (3)
21-22: New TranslateService import and mock
IncludingTranslateServicefrom@ngx-translate/coreandMockTranslateServiceensures translation behavior is isolated in tests. Looks good.
35-36: Standalone component test declarations
Declaring only mocks and pipes is consistent with the standalone component approach. Make sure no other dependencies are missing for the test scenario.
37-44: Provider configuration
RegisteringprovideRouter([])and mocking the relevant services is a best practice. Confirm that you do not need to import the realComplaintResponseServicein providers if the tested logic depends on any real method calls.
DominikRemo
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Client
Motivation and Context
For the client migration of the complaint components, the following files need to be updated:
Description
In this PR, we migrate the above-listed components to standalone and perform the inject migration. The signals migration is not part of this PR!
Steps for Testing
Prerequisites:
Tip
You can find more info about the complaint system and how it works here: https://docs.artemis.cit.tum.de/user/assessment.html#complaints
Exam Mode Testing
Prerequisites:
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
Exam Mode Test
Summary by CodeRabbit
Release Notes
New Features
inject()functionRefactoring
ArtemisComplaintsForTutorModuleacross multiple modulesImprovements