Development: Migrate client code for the assessment dashboard
#10086
Conversation
github-actions
bot
added
tests
client
maximiliansoelch
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe pull request focuses on modernizing several components in the assessment dashboard module of an Angular application. The changes primarily involve converting components to standalone architecture, utilizing 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 (1)
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.component.ts (1)
25-35: Ensure only necessary modules are imported.These lines add multiple modules and components (e.g.
ArtemisSharedComponentModule,FormsModule,NgbTooltip). Consider confirming each import is required to keep the bundle lean and maintain clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard-information.component.ts(2 hunks)src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.component.ts(2 hunks)src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.module.ts(1 hunks)src/main/webapp/app/course/dashboards/assessment-dashboard/exam-assessment-buttons/exam-assessment-buttons.component.ts(2 hunks)src/test/javascript/spec/component/assessment-dashboard/assessment-dashboard-information.component.spec.ts(1 hunks)src/test/javascript/spec/component/assessment-dashboard/assessment-dashboard.component.spec.ts(0 hunks)src/test/javascript/spec/component/assessment-dashboard/exam-assessment-buttons.component.spec.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/javascript/spec/component/assessment-dashboard/assessment-dashboard.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (6)
src/test/javascript/spec/component/assessment-dashboard/assessment-dashboard-information.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/course/dashboards/assessment-dashboard/assessment-dashboard-information.component.ts (1)
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.component.ts (1)
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.module.ts (1)
src/main/webapp/app/course/dashboards/assessment-dashboard/exam-assessment-buttons/exam-assessment-buttons.component.ts (1)
src/test/javascript/spec/component/assessment-dashboard/exam-assessment-buttons.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}}
🔇 Additional comments (16)
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.module.ts (1)
29-31: Validate usage of standalone components in imports array.
Placing these standalone components (AssessmentDashboardComponent, AssessmentDashboardInformationComponent, ExamAssessmentButtonsComponent) in the imports array is valid for modern Angular. Verify that they are no longer declared elsewhere, as standalone components typically should not appear in a declarations array.
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.component.ts (4)
1-2: Use of inject and simplified imports is consistent with Angular best practices.
Adopting the inject function promotes constructor-free dependency injection, keeping the class definition succinct.
20-20: Importing the AssessmentDashboardInformationComponent.
Confirm that all references to the imported component have been updated to reflect its standalone architecture.
42-57: Transition to standalone.
Declaring standalone: true with an imports array is a sound approach in Angular 15+ for better modularity.
60-68: Injecting services without a constructor.
Using the inject function for each service clarifies your dependencies at the property level. Well done on leveraging the latest Angular style.
src/test/javascript/spec/component/assessment-dashboard/assessment-dashboard-information.component.spec.ts (1)
23-23: Removed AssessmentDashboardInformationComponent from declarations.
Since it’s now standalone, it is no longer declared in the testing module. This setup aligns with Angular’s standalone component testing approach.
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard-information.component.ts (5)
1-1: Switching to property injection with inject.
This import explicitly pulls in inject from @angular/core, aligning with newer Angular features for dependency injection.
3-3: Inclusion of PieChartModule.
Brings in chart functionality, useful for visual representations in the dashboard. Confirm that the chart usage is tested thoroughly.
8-11: Importing additional shared modules, directives, and pipes.
These imports (TranslateDirective, ArtemisSidePanelModule, etc.) indicate a self-sufficient component design. Ensure each is used or remove unused imports.
40-41: Enabling standalone: true with an internal imports array.
Declaring a standalone component fosters a clearer, modular architecture.
44-45: Injecting TranslateService.
Using inject(TranslateService) avoids constructor boilerplate and keeps this component succinct. Make sure to handle any potential subscription cleanups to avert memory leaks.
src/main/webapp/app/course/dashboards/assessment-dashboard/exam-assessment-buttons/exam-assessment-buttons.component.ts (4)
1-2: Constructor-free dependency imports.
The shift to inject for ActivatedRoute and other services matches Angular’s modern injection patterns.
18-19: Importing FaIconComponent & TranslateDirective.
Adding these dependencies at the component level ensures minimal overhead and isolates usage to this component.
24-25: Standalone component declaration and local imports.
Marking the component as standalone simplifies your module dependencies, reducing the module-level declarations footprint.
28-35: Migrating to inject for multiple services.
Direct property injection improves clarity around the component’s service usage; just confirm proper error handling in the calling or parent context if any of these fail.
src/test/javascript/spec/component/assessment-dashboard/exam-assessment-buttons.component.spec.ts (1)
119-119: Ensure the standalone component is consistently imported for testing.
By removing ExamAssessmentButtonsComponent from the declarations, the tests may rely on it being a standalone component. Verify that this change is intentional and that the component is properly imported if you intend to test its template and lifecycle. Otherwise, the fixture-based tests might still pass but skip certain template checks or behaviors.
WalkthroughThe pull request focuses on modernizing the Angular components in the assessment dashboard by converting them to standalone components. This involves updating 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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/javascript/spec/component/assessment-dashboard/exam-assessment-buttons.component.spec.ts (1)
119-119: Consider avoiding full module imports in test.Currently, this test imports
ArtemisTestModule, which might be broad. According to the project guidelines, try replacing it with mock providers or minimal imports for performance gains and clearer test isolation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard-information.component.ts(2 hunks)src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.component.ts(2 hunks)src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.module.ts(1 hunks)src/main/webapp/app/course/dashboards/assessment-dashboard/exam-assessment-buttons/exam-assessment-buttons.component.ts(2 hunks)src/test/javascript/spec/component/assessment-dashboard/assessment-dashboard-information.component.spec.ts(1 hunks)src/test/javascript/spec/component/assessment-dashboard/assessment-dashboard.component.spec.ts(0 hunks)src/test/javascript/spec/component/assessment-dashboard/exam-assessment-buttons.component.spec.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/javascript/spec/component/assessment-dashboard/assessment-dashboard.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (6)
src/test/javascript/spec/component/assessment-dashboard/assessment-dashboard-information.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-dashboard/exam-assessment-buttons.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/course/dashboards/assessment-dashboard/assessment-dashboard.module.ts (1)
src/main/webapp/app/course/dashboards/assessment-dashboard/exam-assessment-buttons/exam-assessment-buttons.component.ts (1)
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard-information.component.ts (1)
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.component.ts (1)
🔇 Additional comments (14)
src/main/webapp/app/course/dashboards/assessment-dashboard/exam-assessment-buttons/exam-assessment-buttons.component.ts (4)
1-2: Good use of the new inject API in imports.
These imports align with Angular's standalone component pattern, reducing boilerplate while maintaining clarity.
18-19: Appropriate addition of translation and icon imports.
Bringing FaIconComponent and TranslateDirective into the component context is consistent with Angular's recommended standalone approach.
24-25: Integration with standalone property and imports array recognized.
Marking the component as standalone and explicitly listing dependencies ensures isolation and clarity.
28-35: Injecting services with the new approach is valid and concise.
This pattern improves maintainability and aligns with Angular guidelines.
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.component.ts (4)
1-2: Clean adoption of the inject() method in imports.
This approach eliminates constructor boilerplate and is consistent with modern Angular style.
20-35: Expanded imports for required shared modules and components.
Importing AssessmentDashboardInformationComponent, FaIconComponent, and other modules at the component level fosters modular design.
42-57: Use of standalone property and explicit imports is aligned with Angular guidelines.
Defining all dependencies in the imports array clarifies the component's self-contained structure.
60-68: inject-based property assignments are well-structured.
Injecting services without a constructor is clean and enhances readability.
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.module.ts (1)
29-31: Declaring standalone components in the imports array.
Moving AssessmentDashboardComponent, AssessmentDashboardInformationComponent, and ExamAssessmentButtonsComponent into the imports array is correct for standalone usage.
src/test/javascript/spec/component/assessment-dashboard/assessment-dashboard-information.component.spec.ts (1)
23-23: Removal of the tested component from declarations is correct for standalone.
Since the component is standalone, it no longer needs to be declared explicitly in the test module.
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard-information.component.ts (3)
1-1: Updated imports match standalone component best practices.
These imports ensure all required modules and directives are available within the component’s context.
Also applies to: 3-3, 8-11
40-41: Marking the component as standalone.
This follows Angular’s recommendation to reduce module dependencies and promote reusability.
44-45: inject-based translation service usage is concise.
Using the inject function rather than a constructor aligns with the new Angular DI pattern.
src/test/javascript/spec/component/assessment-dashboard/exam-assessment-buttons.component.spec.ts (1)
119-119: Adoption of standalone component setup confirmed.
Removing ExamAssessmentButtonsComponent from the declarations array aligns with Angular’s standalone component approach. Ensure that if you need features from this component within the template, you include it in the imports array (e.g., imports: [ExamAssessmentButtonsComponent]) or the overall test module if necessary.
Would you like to run tests to confirm that the component remains properly instantiated and tested after this removal?
WalkthroughThe pull request focuses on refactoring several Angular components in the assessment dashboard module to adopt standalone component architecture. The changes primarily involve updating 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
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.component.ts (3)
Line range hint
236-311: Consider breaking down the loadAll method for better maintainability.The
loadAllmethod is quite complex with multiple responsibilities. Consider extracting the exam mode and course mode logic into separate methods.-loadAll() { +loadAll() { + this.isExamMode ? this.loadExamMode() : this.loadCourseMode(); +} + +private loadExamMode() { if (this.isExamMode) { this.hideFinishedExercises = false; this.examManagementService.getExamWithInterestingExercisesForAssessmentDashboard(/*...*/) this.examManagementService.getStatsForExamAssessmentDashboard(/*...*/) } +} + +private loadCourseMode() { else { this.courseService.getCourseWithInterestingExercisesForTutors(/*...*/) this.courseService.getStatsForTutors(/*...*/) } -}
Line range hint
313-386: Consider extracting performance metrics calculation logic.The
computeIssuesWithTutorPerformancemethod handles multiple calculations. Consider extracting the metrics calculations into separate methods for better readability and maintainability.+private calculateCourseInformation(entries: TutorLeaderboardElement[]) { + return entries.reduce( + (accumulator, entry) => ({ + summedAverageRatings: accumulator.summedAverageRatings + entry.averageRating, + summedAverageScore: accumulator.summedAverageScore + entry.averageScore, + summedComplaintRatio: accumulator.summedComplaintRatio + this.calculateComplaintRatio(entry), + }), + { summedAverageRatings: 0, summedAverageScore: 0, summedComplaintRatio: 0 }, + ); +} + +private calculateComplaintRatio(entry: TutorLeaderboardElement): number { + return entry.numberOfAssessments === 0 ? 0 : (100 * entry.numberOfTutorComplaints) / entry.numberOfAssessments; +}
Line range hint
388-434: Consider using early returns to reduce nesting.The
extractExercisesmethod has nested conditions. Consider using early returns to improve readability.-private extractExercises(exercises?: Exercise[]) { - if (exercises && exercises.length > 0) { - this.allExercises = exercises; - // ... rest of the code - } -} +private extractExercises(exercises?: Exercise[]) { + if (!exercises?.length) { + return; + } + + this.allExercises = exercises; + // ... rest of the code +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard-information.component.ts(2 hunks)src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.component.ts(2 hunks)src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.module.ts(1 hunks)src/main/webapp/app/course/dashboards/assessment-dashboard/exam-assessment-buttons/exam-assessment-buttons.component.ts(2 hunks)src/test/javascript/spec/component/assessment-dashboard/assessment-dashboard-information.component.spec.ts(1 hunks)src/test/javascript/spec/component/assessment-dashboard/assessment-dashboard.component.spec.ts(0 hunks)src/test/javascript/spec/component/assessment-dashboard/exam-assessment-buttons.component.spec.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/javascript/spec/component/assessment-dashboard/assessment-dashboard.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (6)
src/test/javascript/spec/component/assessment-dashboard/assessment-dashboard-information.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/course/dashboards/assessment-dashboard/exam-assessment-buttons/exam-assessment-buttons.component.ts (1)
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.module.ts (1)
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard-information.component.ts (1)
src/test/javascript/spec/component/assessment-dashboard/exam-assessment-buttons.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/course/dashboards/assessment-dashboard/assessment-dashboard.component.ts (1)
🔇 Additional comments (9)
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.module.ts (1)
29-31: Good use of standalone components in module imports.
Moving these components from declarations to imports is correct for standalone components. Ensure that you have removed all redundant declarations of these components from other modules to avoid conflicts.
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard-information.component.ts (4)
1-3: Imports comply with Angular style guidelines.
Using single quotes, properly grouping modules, and referencing them once is consistent with Angular’s recommended style.
8-11: Relevant modules and directives for this standalone component.
Including TranslateDirective, ArtemisSidePanelModule, RouterLink, and ArtemisTranslatePipe directly in the standalone imports ensures self-contained reuse. This approach promotes clearer dependencies and aligns with best practices.
40-41: Standalone component configuration looks correct.
Defining standalone: true and moving imports into the component decorator is an effective way to enhance modularity and clarity in Angular.
44-45: Using inject() for dependency injection.
Switching from a constructor-based injection to the inject function is a clean approach that removes boilerplate constructors and simplifies test setups. This aligns with modern Angular patterns.
src/main/webapp/app/course/dashboards/assessment-dashboard/exam-assessment-buttons/exam-assessment-buttons.component.ts (1)
1-2: Successfully migrated to a standalone component and inject().
• Marking the component as standalone and adding required modules in imports eliminates the need for module-specific declarations.
• Replacing the constructor with inject() fosters cleaner code and a more direct service usage pattern, following Angular’s recommended approach.
Also applies to: 18-19, 24-25, 28-35
src/test/javascript/spec/component/assessment-dashboard/exam-assessment-buttons.component.spec.ts (1)
119-119: Declaration removal for standalone component.
Removing ExamAssessmentButtonsComponent from declarations aligns with its new standalone status. Ensure the test suite runs successfully and that the component is properly detected by the test environment (e.g., direct .createComponent() usage) without further declarations.
src/main/webapp/app/course/dashboards/assessment-dashboard/assessment-dashboard.component.ts (2)
42-57: LGTM! Proper standalone component configuration.
The component is correctly configured as standalone with all necessary dependencies imported.
60-67: LGTM! Clean dependency injection using inject().
Dependencies are properly injected using the new inject() function, following Angular's latest best practices.
...cript/spec/component/assessment-dashboard/assessment-dashboard-information.component.spec.ts
Show resolved
Hide resolved
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
...app/app/course/dashboards/assessment-dashboard/assessment-dashboard-information.component.ts
Show resolved
Hide resolved
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/assessment.html#assessment-dashboard
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
injectfunctionRefactor
Tests