Development: Migrate client code for the grading system
#10055
Conversation
github-actions
bot
added
tests
client
maximiliansoelch
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThis pull request introduces a significant refactoring of various Angular components and modules in the grading system and shared modules. The primary changes include transitioning components to standalone mode, updating dependency injection to use the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (4)
src/main/webapp/app/exercises/programming/shared/instructions-render/programming-exercise-instructions-render.module.ts (1)
12-12: Ensure consistent ordering and grouping of imports.While adding
SafeHtmlPipeis correct, consider placing standalone imports at the end or in a standalone-specific section of theimportsarray to keep the module’s structure tidy.src/main/webapp/app/grading-system/interval-grading-system/interval-grading-system.component.ts (1)
6-14: Check potential overlap in shared modules
While migrating to a standalone component, you are importing bothArtemisSharedModuleandArtemisSharedComponentModule. Consider verifying if both are truly needed. Removing any duplicate modules can simplify the dependency graph and improve maintainability.Also applies to: 20-31
src/main/webapp/app/exam/manage/exam-management.module.ts (1)
78-78: Defined pipes in providers array.Registering
ArtemisDurationFromSecondsPipe,SafeHtmlPipe, andGradeStepBoundsPipein theprovidersarray is valid. However, if some or all are purely standalone pipes, you could import them in components on demand for a more modular approach.src/main/webapp/app/grading-system/bonus/bonus.service.ts (1)
17-17: Maintain consistent property ordering.The
resourceUrlis now placed below the newly injected services. Although valid, consider ordering properties for clarity, such as placing constants and configuration properties near the class start, followed by service dependencies.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
src/main/webapp/app/exam/manage/exam-management.module.ts(2 hunks)src/main/webapp/app/exercises/programming/shared/instructions-render/programming-exercise-instructions-render.module.ts(1 hunks)src/main/webapp/app/grading-system/base-grading-system/base-grading-system.component.ts(2 hunks)src/main/webapp/app/grading-system/bonus/bonus.component.ts(3 hunks)src/main/webapp/app/grading-system/bonus/bonus.service.ts(2 hunks)src/main/webapp/app/grading-system/detailed-grading-system/detailed-grading-system.component.ts(1 hunks)src/main/webapp/app/grading-system/grading-key-overview/grading-key-overview.component.ts(1 hunks)src/main/webapp/app/grading-system/grading-key-overview/grading-key-overview.module.ts(1 hunks)src/main/webapp/app/grading-system/grading-key-overview/grading-key/grading-key-table.component.ts(2 hunks)src/main/webapp/app/grading-system/grading-system-info-modal/grading-system-info-modal.component.ts(1 hunks)src/main/webapp/app/grading-system/grading-system-presentations/grading-system-presentations.component.ts(2 hunks)src/main/webapp/app/grading-system/grading-system.component.ts(1 hunks)src/main/webapp/app/grading-system/grading-system.module.ts(1 hunks)src/main/webapp/app/grading-system/grading-system.service.ts(2 hunks)src/main/webapp/app/grading-system/interval-grading-system/interval-grading-system.component.ts(1 hunks)src/main/webapp/app/shared/pipes/grade-step-bounds.pipe.ts(1 hunks)src/main/webapp/app/shared/pipes/safe-html.pipe.ts(1 hunks)src/main/webapp/app/shared/pipes/shared-pipes.module.ts(0 hunks)src/test/javascript/spec/component/bonus/bonus.component.spec.ts(1 hunks)src/test/javascript/spec/component/exam/participate/exam-navigation-sidebar.component.spec.ts(1 hunks)src/test/javascript/spec/component/grading-system/detailed-grading-system.component.spec.ts(0 hunks)src/test/javascript/spec/component/grading-system/grading-key-overview.component.spec.ts(1 hunks)src/test/javascript/spec/component/grading-system/grading-key-table.component.spec.ts(2 hunks)src/test/javascript/spec/component/grading-system/grading-system-presentations.component.spec.ts(1 hunks)src/test/javascript/spec/component/grading-system/grading-system.component.spec.ts(1 hunks)src/test/javascript/spec/component/grading-system/interval-grading-system.component.spec.ts(0 hunks)
💤 Files with no reviewable changes (3)
- src/main/webapp/app/shared/pipes/shared-pipes.module.ts
- src/test/javascript/spec/component/grading-system/interval-grading-system.component.spec.ts
- src/test/javascript/spec/component/grading-system/detailed-grading-system.component.spec.ts
✅ Files skipped from review due to trivial changes (1)
- src/test/javascript/spec/component/grading-system/grading-system.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (22)
src/test/javascript/spec/component/bonus/bonus.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/exam/manage/exam-management.module.ts (1)
src/test/javascript/spec/component/exam/participate/exam-navigation-sidebar.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/grading-system/grading-system-presentations.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/grading-system/grading-key-table.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/shared/pipes/safe-html.pipe.ts (1)
src/main/webapp/app/exercises/programming/shared/instructions-render/programming-exercise-instructions-render.module.ts (1)
src/main/webapp/app/grading-system/grading-system.service.ts (1)
src/test/javascript/spec/component/grading-system/grading-key-overview.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/grading-system/grading-system.module.ts (1)
src/main/webapp/app/grading-system/detailed-grading-system/detailed-grading-system.component.ts (1)
src/main/webapp/app/grading-system/grading-system-info-modal/grading-system-info-modal.component.ts (1)
src/main/webapp/app/shared/pipes/grade-step-bounds.pipe.ts (1)
src/main/webapp/app/grading-system/bonus/bonus.service.ts (1)
src/main/webapp/app/grading-system/grading-key-overview/grading-key-overview.module.ts (1)
src/main/webapp/app/grading-system/interval-grading-system/interval-grading-system.component.ts (1)
src/main/webapp/app/grading-system/grading-key-overview/grading-key-overview.component.ts (1)
src/main/webapp/app/grading-system/bonus/bonus.component.ts (1)
src/main/webapp/app/grading-system/grading-system-presentations/grading-system-presentations.component.ts (1)
src/main/webapp/app/grading-system/grading-key-overview/grading-key/grading-key-table.component.ts (1)
src/main/webapp/app/grading-system/grading-system.component.ts (1)
src/main/webapp/app/grading-system/base-grading-system/base-grading-system.component.ts (1)
🔇 Additional comments (38)
src/main/webapp/app/grading-system/detailed-grading-system/detailed-grading-system.component.ts (2)
4-9: Verify the necessity of newly added imports.
Great job adding the required imports for standalone usage (such as TranslateDirective, ArtemisSharedComponentModule, FormsModule, etc.). However, please confirm that each import is actually used in the template and that redundant imports aren't introduced. If ArtemisSharedModule re-exports everything you need from ArtemisSharedComponentModule, consider removing duplicates to keep imports minimal and consistent.
15-16: Validate standalone component configuration.
Marking the component as standalone is a welcome modernization. Double-check that all required common Angular features (e.g., directives, pipes from CommonModule) are provided—either by the included shared modules or directly—especially if you use built-in directives (*ngIf, *ngFor, etc.).
src/main/webapp/app/grading-system/grading-system.module.ts (1)
14-24: Confirm that all imported components are standalone.
Switching these components from declarations to imports is valid if they're declared as standalone components with standalone: true. Otherwise, it may cause unexpected runtime errors where Angular cannot locate the component definitions. Ensure each component in this list is indeed declared as a standalone component.
src/main/webapp/app/grading-system/grading-key-overview/grading-key/grading-key-table.component.ts (3)
14-18: Additional imports for standalone usage
Providing the necessary directives, pipes, and modules in the imports array ensures seamless usage within a standalone component. Good job keeping them consistent with single quotes.
24-25: Marking the component as standalone
Enabling standalone: true and specifying all required imports here is correct for Angular 14+ standalone components. This modernizes your component configuration.
28-32: Refactor from constructor injection to inject()
Using the inject() API is a clean approach if you're aiming for reduced boilerplate compared to traditional constructor injection. Make sure your tests are updated accordingly.
src/main/webapp/app/shared/pipes/safe-html.pipe.ts (1)
4-4: Marking the pipe as standalone
Declaring standalone: true allows you to import and use this pipe anywhere without needing to declare it in a module. This looks good.
src/main/webapp/app/grading-system/grading-key-overview/grading-key-overview.module.ts (1)
8-8: Importing standalone components directly
Listing GradingKeyOverviewComponent and GradingKeyTableComponent under imports is valid only if they each have standalone: true. Ensure this aligns with your project’s Angular version and architecture plans.
src/main/webapp/app/grading-system/grading-system-info-modal/grading-system-info-modal.component.ts (3)
1-5: Imports for standalone component
These imports allow your standalone component to leverage translation and icon functionalities without a traditional module declaration.
10-11: Defining a standalone component
Using standalone: true with an imports array follows Angular’s recommended configuration for standalone components and keeps the setup more self-contained.
14-14: Using the inject() API
Replacing constructor-based injection with inject(NgbModal) is consistent with the new Angular practices. Make sure related tests handle this pattern correctly.
src/main/webapp/app/exercises/programming/shared/instructions-render/programming-exercise-instructions-render.module.ts (1)
9-9: Good addition of the SafeHtmlPipe import.
Bringing in standalone pipes via direct imports is consistent with Angular’s recommended approach. This step effectively ensures the pipe is available within this module.
src/main/webapp/app/shared/pipes/grade-step-bounds.pipe.ts (1)
9-9: Standalone pipe usage is aligned with Angular’s best practices.
Declaring this pipe as standalone provides a clean way to use it without module scoping.
src/main/webapp/app/grading-system/grading-key-overview/grading-key-overview.component.ts (4)
1-1: Switched from constructor injection to the inject() function.
This aligns well with the Angular 14+ recommended dependency injection approach.
8-11: Import statements match the coding guidelines.
Using single quotes and grouping external imports together is consistent with the Angular style guide.
17-18: Transition to a standalone component is appropriate.
Marking the component as standalone and providing dependent imports here simplifies the module structure and eliminates the need for the traditional NgModule-based declarations.
21-24: Utilizing inject() for route, navigation, and theme services.
This approach is concise and future-proof, aligning the code with modern Angular patterns.
src/main/webapp/app/grading-system/grading-system.component.ts (3)
1-14: Imports array efficiently organizes dependencies.
These additions adhere to the Angular style guide, facilitate code reuse, and support lazy loading.
20-33: Standalone declaration streamlines component configuration.
Specifying standalone: true and using the imports array in the component decorator reduces overhead and clarifies the component’s dependencies.
36-37: Switched to inject() for inserting ActivatedRoute.
This new pattern eliminates boilerplate constructors while preserving readability and maintainability.
src/test/javascript/spec/component/grading-system/grading-key-overview.component.spec.ts (1)
41-41: Smooth migration to standalone testing approach
Using the imports array to directly import the standalone component and its mocked dependencies is aligned with Angular's recommended practices. This setup looks good.
src/test/javascript/spec/component/grading-system/grading-system-presentations.component.spec.ts (1)
46-46: Confirm removal of ModePickerComponent mock
Since the ModePickerComponent mock is removed, verify whether it's genuinely no longer needed in these tests or if coverage is affected.
src/main/webapp/app/grading-system/grading-system-presentations/grading-system-presentations.component.ts (1)
4-7: Standalone component adoption
Migrating this component to standalone mode and updating its imports array is consistent with Angular style guidelines. No issues found in this implementation.
Also applies to: 29-30
src/test/javascript/spec/component/grading-system/grading-key-table.component.spec.ts (2)
4-4: Good use of ng-mocks for mocking dependencies.
These imports simplify testing by mocking directives, pipes, and components.
73-73: Proper usage of component-based test imports.
By importing the standalone GradingKeyTableComponent, you align with Angular's recommended approach for testing standalone components. This reduces the need for extensive module declarations.
src/main/webapp/app/exam/manage/exam-management.module.ts (2)
71-72: Clear import statements for standalone pipes.
Importing both SafeHtmlPipe and GradeStepBoundsPipe prior to providing them ensures their availability throughout the module without adding them to declarations. This is a clean approach consistent with Angular’s standalone pipes.
114-116: Importing standalone pipes and a component into the module.
Lines 114-116 add SafeHtmlPipe, GradeStepBoundsPipe, and BonusComponent to the module’s imports array. This is aligned with the standalone approach, ensuring these features can be used throughout the module without listing them in declarations.
src/test/javascript/spec/component/exam/participate/exam-navigation-sidebar.component.spec.ts (1)
39-39: Removed ExamNavigationSidebarComponent from declarations.
Adopting child component mocks (ExamTimerComponent, ExamLiveEventsButtonComponent, and SearchFilterComponent) simplifies testing and focuses on verifying the specialized functionalities. However, ensure integration tests exist elsewhere to confirm that the ExamNavigationSidebarComponent itself is correctly instantiated and functions end-to-end.
src/main/webapp/app/grading-system/bonus/bonus.service.ts (2)
1-1: Usage of the inject function.
Importing inject from @angular/core indicates a transition to the newer injection mechanism. Ensure that the rest of the application remains consistent, preventing hybrid usage of constructor-based injection and inject.
14-15: Injected services as class fields.
Declaring private http and private gradingSystemService with inject(...) is a concise alternative to a constructor. Verify that testing frameworks and tests remain functional, as some older test setups may rely on constructor-based injection.
src/main/webapp/app/grading-system/bonus/bonus.component.ts (4)
1-1: Use of inject aligns with modern Angular standards.
Ensures constructor-free, streamlined dependency injection.
15-19: Imports for standalone component modules and pipes look correct.
These imports appear consistent with Angular style guidelines for standalone components.
35-36: Declaration as a standalone component with an imports array is appropriate.
This setup correctly decouples the component from module-based declarations.
39-44: Migration to inject() for service dependencies is successful.
This approach removes the need for a constructor and keeps the code concise, aligning with Angular best practices.
src/main/webapp/app/grading-system/grading-system.service.ts (1)
1-1: Refactor to using inject() in the service.
Adopting inject(HttpClient) is consistent with the new DI pattern. The public resourceUrl remains accessible as needed.
Also applies to: 16-18
src/test/javascript/spec/component/bonus/bonus.component.spec.ts (1)
19-19: Import of FormsModule and ReactiveFormsModule is valid.
This ensures that the necessary form features are available for the BonusComponent tests.
src/main/webapp/app/grading-system/base-grading-system/base-grading-system.component.ts (2)
1-1: Use of inject from @angular/core supports constructor-free DI.
The import matches Angular style guidelines.
42-47: Switch to inject() for service injection is clean and consistent.
Replaces constructor injection, simplifying the component and maintaining best practices for maintainability.
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 grading system and how it works here: https://docs.artemis.cit.tum.de/user/grading.html
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
New Features
SafeHtmlPipeandGradeStepBoundsPipefor enhanced data handling.Bug Fixes
importsarray and placing them in thedeclarationsarray where appropriate.Documentation
Tests