Development: Refactor course sidebar into its own component
#10455
Conversation
github-actions
bot
added
tests
client
WalkthroughThe pull request introduces significant modifications to the course overview and sidebar components. It adds new input properties to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Overview
participant Sidebar
User->>Overview: Load course overview
Overview->>Sidebar: Provide course data and state flags
User->>Sidebar: Toggle collapse / select course action
Sidebar-->>Overview: Emit event (switchCourse, courseActionItemClick, toggleCollapseState)
Note over Sidebar: Internal state updated on window resize
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 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
🔭 Outside diff range comments (1)
src/main/webapp/app/overview/course-overview.component.scss (1)
1-262:⚠️ Potential issueFix code duplication in SCSS file
The file contains significant duplication with almost identical code appearing twice (first between lines 1-262 and then again from lines 263-524). This duplication should be removed to improve maintainability.
Remove the duplicated code by keeping only one copy of each CSS rule.
Also applies to: 263-524
🧹 Nitpick comments (5)
src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts (1)
77-101: Consider potential memory leak with window resize listener.The window resize event listener is properly implemented with HostListener. However, to ensure there are no memory leaks, consider adding cleanup in the OnDestroy lifecycle hook.
-export class CourseSidebarComponent implements OnInit, OnChanges { +export class CourseSidebarComponent implements OnInit, OnChanges, OnDestroy { // ... existing code ... + ngOnDestroy() { + // Clean up logic if needed + } }src/main/webapp/app/overview/course-sidebar/course-sidebar.component.html (2)
101-111: Great keyboard accessibility for the collapse control!The collapse chevron includes a keyboard shortcut (Ctrl + M) in the tooltip, which enhances accessibility. Consider adding ARIA attributes for screen readers.
<div class="double-arrow mb-2" [ngClass]="{ 'menu-closed': isNavbarCollapsed }" [ngbTooltip]="(isNavbarCollapsed ? 'Expand' : 'Collapse') + ' Menu (Ctrl + M)'" (click)="toggleCollapseState.emit()" + role="button" + tabindex="0" + aria-label="Toggle sidebar menu" >
169-178: Consider simplifying conditional CSS classesThe
ngClassobject could become complex to maintain. Consider extracting this logic to a method in the component class for better readability and testability.[ngClass]="{ 'guided-tour': sidebarItem.guidedTour, - newMessage: !communicationRouteLoaded && hasUnreadMessages && sidebarItem.title === 'Communication', + newMessage: hasNewCommunicationMessage(sidebarItem), collapsed: isNavbarCollapsed, }"Then in your component:
hasNewCommunicationMessage(item: SidebarItem): boolean { return !this.communicationRouteLoaded && this.hasUnreadMessages && item.title === 'Communication'; }src/main/webapp/app/overview/course-overview.component.html (1)
36-37: Minor spacing inconsistencyThere's an extra blank line after the sidebar component that could be removed for more consistent spacing with the rest of the file.
<mat-sidenav disableClose [ngClass]="{ 'sidenav-height-dev': !isProduction || isTestServer }" class="module-bg rounded-3" opened="true" mode="side" [hidden]="isExamStarted" > <jhi-course-sidebar [course]="course" [courses]="courses" [sidebarItems]="sidebarItems" [courseActionItems]="courseActionItems" [isNavbarCollapsed]="isNavbarCollapsed" [isExamStarted]="isExamStarted" [isProduction]="isProduction" [isTestServer]="isTestServer" [hasUnreadMessages]="hasUnreadMessages" [communicationRouteLoaded]="communicationRouteLoaded" (switchCourse)="switchCourse($event)" (courseActionItemClick)="courseActionItemClick($event)" (toggleCollapseState)="toggleCollapseState()" /> - </mat-sidenav>src/main/webapp/app/overview/course-overview.component.ts (1)
219-220: Type annotation improvement opportunityThe subscription handler lacks proper type annotations for the params object. Consider adding a stronger type to improve type safety.
- this.subscription = this.route.params.subscribe((params: { courseId: string }) => { + this.subscription = this.route.params.subscribe((params: { courseId: string; [key: string]: string }) => { this.courseId = Number(params.courseId); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/webapp/app/overview/course-overview.component.html(1 hunks)src/main/webapp/app/overview/course-overview.component.scss(1 hunks)src/main/webapp/app/overview/course-overview.component.ts(7 hunks)src/main/webapp/app/overview/course-sidebar/course-sidebar.component.html(1 hunks)src/main/webapp/app/overview/course-sidebar/course-sidebar.component.scss(1 hunks)src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts(1 hunks)src/test/javascript/spec/component/course/course-overview.component.spec.ts(2 hunks)src/test/javascript/spec/component/course/course-sidebar.component.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`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/component/course/course-overview.component.spec.tssrc/test/javascript/spec/component/course/course-sidebar.component.spec.ts
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/overview/course-sidebar/course-sidebar.component.tssrc/main/webapp/app/overview/course-overview.component.ts
`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/overview/course-overview.component.htmlsrc/main/webapp/app/overview/course-sidebar/course-sidebar.component.html
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (33)
src/test/javascript/spec/component/course/course-overview.component.spec.ts (2)
62-62: LGTM! Import for the new sidebar component.The import for
CourseSidebarComponentcorrectly follows the project's import structure.
187-187: LGTM! Proper mocking of the new sidebar component.The course sidebar component is properly mocked in the test setup using NgMocks, which follows the project's testing guidelines.
src/test/javascript/spec/component/course/course-sidebar.component.spec.ts (6)
1-23: LGTM! Well-structured test imports and dependencies.The imports are properly organized and include all necessary components, directives, and services for testing. The mock services and test utilities are appropriately imported.
24-92: LGTM! Comprehensive test setup with well-defined mock data.The test data setup is thorough and includes mock courses, sidebar items, and action items with all necessary properties. The various sidebar item configurations test different use cases including feature toggles, Orion window behavior, and guidedTour support.
93-143: LGTM! Well-configured TestBed setup.The TestBed configuration properly includes all necessary imports, components, and providers. The component instance is initialized with appropriate test data. The setup includes all required Angular testing modules and proper mocking of external dependencies.
145-217: LGTM! Thorough tests for visibility and threshold calculations.The tests effectively verify the component's core functionality for calculating visibility thresholds and updating visible navbar items. Tests cover initialization, changes to inputs, window resize events, and dropdown behavior.
219-254: LGTM! Good coverage of display logic tests.The tests properly verify the display logic for course titles, icons, and dropdown visibility based on component state.
256-283: LGTM! Comprehensive event emission tests.The tests efficiently verify that the component correctly emits events for user interactions, including toggling collapse state, switching courses, and clicking action items.
src/main/webapp/app/overview/course-sidebar/course-sidebar.component.scss (5)
1-21: LGTM! Well-structured SCSS with proper variable definitions.The import structure follows best practices by including Bootstrap functions, variables, and mixins. The custom variables for menu widths and transition durations are well-defined and enhance maintainability.
23-46: LGTM! Clean implementation of message notification styles.Using a placeholder selector (
%message-block) for the notification styles is a good practice for code reuse. The styles are appropriately applied to both normal and collapsed menu states.
48-77: LGTM! Smooth transition effects for menu controls.The transition effects for the double arrow and icon rotation provide good visual feedback for users. The transform and transition properties are well-utilized for creating the animation effects.
79-131: LGTM! Good responsive design implementation.The styles properly handle different screen sizes with appropriate media queries. The course circle and title styles maintain consistent visual appearance across different states.
133-167: LGTM! Well-structured dropdown styles with print considerations.The dropdown menu styles are comprehensive and maintain consistent appearance. The print media query appropriately hides the sidebar when printing, which is a good user experience consideration.
src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts (5)
1-13: LGTM! Proper import structure following Angular guidelines.The imports are organized correctly, with Angular core imports followed by application-specific imports. All necessary components, directives, and services are properly imported.
14-32: LGTM! Well-defined interfaces for component types.The
CourseActionItemandSidebarIteminterfaces are well-structured and provide clear type definitions for the component's data models. Using interfaces enhances code readability and type safety.
34-56: LGTM! Proper component configuration using standalone components.The component is properly configured as a standalone component with all necessary imports. The selector, template, and stylesheet paths follow Angular naming conventions.
57-76: LGTM! Well-structured component with proper input/output properties.The component has clearly defined input and output properties. The state management for hidden items is properly encapsulated within the component.
102-134: LGTM! Well-implemented sidebar visibility logic.The methods for calculating thresholds and determining visible items are well-structured and properly commented. The reverse order approach for hiding items from the bottom is a good implementation detail.
src/main/webapp/app/overview/course-sidebar/course-sidebar.component.html (4)
1-113: Good implementation of the sidebar container structure!The sidebar layout is well-organized with a clean flex structure that separates the component into logical sections (course header, navigation items, and action items). The collapsed state is properly managed through the
isNavbarCollapsedproperty and consistently applied with ngClass.
7-15: Effective use of the new Angular control flow syntax!Great job using the modern
@ifand@forsyntax instead of the deprecated*ngIfand*ngFordirectives, which aligns with the provided coding guidelines.
43-77: Well-implemented overflow handling for navigation items!The implementation of hidden items with a dropdown menu ensures good UX when there are too many navigation items. The three-dots menu provides a clean way to access overflow items.
115-182: Excellent use of templates for code reusability!Breaking out the common UI patterns into ng-templates promotes reusability and maintainability. The templates for course images and navigation items are well-structured and consistently applied throughout the component.
src/main/webapp/app/overview/course-overview.component.scss (4)
240-252: Good implementation of print styles!The print-specific media query that hides the sidebar when printing is a thoughtful addition that improves the user experience for printed content.
156-171: Well-structured button hover and focus statesThe hover and focus states for the sidebar collapse button provide good visual feedback to users. The transitions are smooth and enhance the user experience.
9-14: Good use of CSS variables for transitionsDefining transition timing variables at the top of the file promotes consistency and makes it easier to fine-tune animations throughout the component.
254-262: Responsive handling for small screensThe media query for small screens appropriately adjusts the chevron rotation direction for the communication module, ensuring consistent behavior across device sizes.
src/main/webapp/app/overview/course-overview.component.html (2)
21-35: Excellent component refactoring!The replacement of complex sidebar markup with a single
<jhi-course-sidebar>component significantly improves code readability and maintainability. The input and output bindings are well-organized and follow Angular best practices.
1-131: Good use of modern Angular control flow syntaxThe template uses the new Angular control flow syntax with
@ifand@forinstead of the older*ngIfand*ngFordirectives, which aligns with the provided coding guidelines.src/main/webapp/app/overview/course-overview.component.ts (5)
16-24: Well-organized importsThe imports have been properly reorganized and grouped by type (Angular core, HTTP, RxJS, etc.), making the code more maintainable.
81-81: Good component modularity with interface importsImporting the interfaces from the new sidebar component (instead of duplicating them) follows the DRY principle and establishes a single source of truth for these types.
83-103: Proper use of standalone component featureAdding
standalone: trueand explicitly importing all the required dependencies helps with tree-shaking and follows Angular's modern architecture practices.
715-719: Good documentation in ngOnDestroy methodThe added comments in the ngOnDestroy method improve code readability by clarifying the purpose of cleanup operations.
732-738: Comprehensive unsubscribe handlingThe ngOnDestroy method properly unsubscribes from all subscriptions, including the new sidebar-related ones, which effectively prevents memory leaks.
…component' into chore/course-sidebar-standalone-component
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/webapp/app/overview/course-sidebar/course-sidebar.component.html (3)
44-51: Consider usability for many hidden items.
WhenhiddenItems().lengthgrows large, the layout applies a fixed content size. Consider adding auto-scroll or pagination for a better user experience.
115-125: Add alt text for accessibility.
The<jhi-secured-image>displays a course icon but lacks analtattribute. Consider dynamically setting a descriptivealt(e.g., the course title) to improve screen reader support.- <jhi-secured-image [src]="courseImage"/> + <jhi-secured-image [src]="courseImage" alt="{{ courseTitle }}"/>
137-142: Check for code duplication in nav templates.
navIconAndTextandnavIconAndTextHiddenare nearly identical except for the collapsed text handling. Consider consolidating these to reduce repetition.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/webapp/app/overview/course-overview.component.html(1 hunks)src/main/webapp/app/overview/course-overview.component.scss(1 hunks)src/main/webapp/app/overview/course-overview.component.ts(5 hunks)src/main/webapp/app/overview/course-sidebar/course-sidebar.component.html(1 hunks)src/main/webapp/app/overview/course-sidebar/course-sidebar.component.scss(1 hunks)src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts(1 hunks)src/test/javascript/spec/component/course/course-overview.component.spec.ts(2 hunks)src/test/javascript/spec/component/course/course-sidebar.component.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/overview/course-sidebar/course-sidebar.component.scss
🧰 Additional context used
📓 Path-based instructions (3)
`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/component/course/course-overview.component.spec.tssrc/test/javascript/spec/component/course/course-sidebar.component.spec.ts
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/overview/course-overview.component.tssrc/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts
`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/overview/course-overview.component.htmlsrc/main/webapp/app/overview/course-sidebar/course-sidebar.component.html
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (53)
src/test/javascript/spec/component/course/course-overview.component.spec.ts (3)
60-60: Import of the new component added correctly.The new
CourseSidebarComponentis now properly imported for mocking in the test suite.
183-183: Good integration of the new component into test declarations.The
CourseSidebarComponentis properly mocked usingMockComponent(), ensuring it's available for testing without bringing in its actual implementation dependencies.
583-593: The sidebar toggling tests are well-implemented.The test correctly verifies that the component responds to sidebar events from the
courseSidebarService, ensuring that the collapsed state is properly updated.src/test/javascript/spec/component/course/course-sidebar.component.spec.ts (24)
1-25: Well-structured imports for the new component test.The imports are well-organized, including all necessary Angular testing utilities, components, and services needed for testing the sidebar component.
26-60: Clear test structure with comprehensive setup.The test suite uses a proper setup with component fixture creation and provides mock data for courses with various configurations for testing different scenarios.
108-148: Thorough TestBed configuration and component initialization.The test setup uses proper Angular testing patterns:
- Comprehensive set of imports and providers
- MockComponent/MockDirective for dependencies
- Setting all required component inputs before detection
150-158: Good verification of component initialization methods.The test correctly verifies that the component properly initializes visible/hidden items during component initialization by spying on the relevant methods.
160-175: Properly testing component's reaction to input changes.The test verifies that the component recalculates hidden items when the
sidebarItemsinput changes by properly mocking theSimpleChangesobject.
177-183: Testing window resize event handling.The test ensures the component responds to window resize events by dispatching a mock event and verifying the appropriate method is called.
185-200: Good testing of dropdown visibility logic.The test verifies that the dropdown is properly closed when updating visible navbar items, using mocks for the NgbDropdown component.
202-210: Threshold calculation verification is accurate.The test verifies the threshold calculation logic, ensuring it accounts for window offset and item height based on the number of sidebar items.
212-225: Comprehensive testing of item visibility threshold application.The test verifies both scenarios:
- When items should be hidden (threshold > height)
- When all items should be visible (threshold < height)
227-234: Proper testing of course title display.The test ensures the course title is correctly displayed when the navbar is not collapsed.
236-244: Good testing of the "more" icon visibility logic.The test verifies that the "three dots" icon for hidden items is correctly shown or hidden based on the
anyItemHiddenstate.
245-262: Complete testing of course icon display logic.The tests verify both scenarios:
- When a course has an icon, it's properly displayed
- When a course has no icon, a fallback circle is displayed instead
264-271: Proper event emission testing for collapse toggle.The test correctly verifies that clicking the collapse chevron emits the
toggleCollapseStateevent.
273-281: Good verification of course switching event emission.The test confirms that selecting a course from the dropdown correctly emits the
switchCourseevent.
283-291: Proper testing of action item click event emission.The test verifies that clicking an action item correctly emits the
courseActionItemClickevent with the correct item.
293-302: Good testing of sidebar items creation with analytics dashboard.The test verifies that when the
studentCourseAnalyticsDashboardEnabledflag is true, the dashboard item is included in the sidebar items.
304-311: Proper testing of default sidebar items.The test ensures that the default sidebar items (Exercises, Lectures) are always included regardless of other settings.
313-321: Testing exams tab visibility logic.The test verifies that the exams tab is included and positioned correctly in the sidebar when visible exams are present.
323-335: Comprehensive testing of learning path and competencies tabs.The test ensures that learning path and competencies tabs are included when the appropriate flags are set on the course.
337-346: Testing FAQ tab inclusion logic.The test verifies that the FAQ tab is included when the
faqEnabledflag is set on the course.
348-359: Good verification of visible exams logic.The test correctly mocks the server date service and verifies that exams are considered visible when their visible date is before the current date.
361-372: Proper testing of exams visibility when not visible.The test verifies that exams with a future visible date are correctly identified as not visible.
374-382: Good testing of competencies and tutorial groups detection.The test verifies that the methods for detecting competencies and tutorial groups return the correct values based on the course properties.
384-388: Testing unenroll action item creation.The test verifies that the unenroll action item is correctly included when the course has unenrollment enabled, a future end date, and the user is not a tutor.
src/main/webapp/app/overview/course-overview.component.scss (6)
156-171: Well-structured CSS for the sidebar collapse button.The styling for the new
.btn-sidebar-collapseclass is well-organized with clear:
- Positioning and layout properties
- Transition effects for smooth animations
- Proper hover and focus state styling
173-196: Good use of pseudo-elements for button styling.The use of
::beforeand::afterpseudo-elements provides a clean way to implement the layered background effects for the collapse button, with appropriate z-index values and transitions.
198-212: Well-defined hover and active states.The hover and active states for the collapse button are properly defined with clear background color changes, providing good visual feedback to users.
214-220: Proper styling for collapsed state.The styling for the collapsed state of the button is well-implemented, with appropriate border color changes and opacity adjustments for the pseudo-elements.
222-238: Clean implementation of chevron animation.The styling for the collapse button chevron is well-done with:
- Proper rotation transform for the icon
- Smooth transition effects
- Appropriate spacing
241-243: Updated print styles for sidebar hiding.The print styles have been updated to target the new
.mat-drawerclass instead of the previously used element, ensuring the sidebar is correctly hidden when printing.src/main/webapp/app/overview/course-overview.component.html (2)
21-34: Clean integration of the new course-sidebar component.The implementation of the
<jhi-course-sidebar>component is well-structured with:
- All necessary inputs properly bound
- Event handlers for user interactions
- Clean syntax with proper indentation
This refactoring significantly improves readability by encapsulating complex sidebar logic into a dedicated component.
36-36: Clean line break after component.The empty line after the sidebar component closing tag helps maintain readability in the template.
src/main/webapp/app/overview/course-overview.component.ts (12)
16-24: No issues with revised imports.
These additional imports look appropriate for the new sidebar functionality and align well with Angular best practices.
41-41: Icon import acknowledged.
ImportingfacSidebaris fine and doesn't pose any conflicts.
46-46: Service import is valid.
ReferencingCourseManagementServicefrom../course/manage/course-management.serviceis correct and consistent.
57-62: New service imports appear correct.
These additions forAlertService,LtiService,CourseSidebarService,PROFILE_ATLAS, andTranslateDirectiveintegrate cleanly with the existing architecture.
69-69: Exam participation import is valid.
No issues with referencingExamParticipationServicefor exam state management.
71-71: Side menu component imports.
Bringing inCourseActionItemandCourseSidebarComponentis aligned with the new sidebar architecture.
81-81: Sidenav content import is acknowledged.
DeclaringMatSidenavContentin the component imports is correct for the new layout usage.
87-90: Template and directive imports look good.
AddingNgTemplateOutlet,FaIconComponent,TranslateDirective, andCourseSidebarComponentensures the relevant functionalities are declared properly.
132-132: Dependency injection note.
inject(CourseSidebarService)is a valid modern approach in standalone components. No concerns here.
187-201: Sidebar open/close subscriptions.
The new subscriptions for open, close, and toggle events on the sidebar are straightforward and unsubscribed inngOnDestroy. Good job preventing memory leaks.
202-214: Additional subscriptions for route/profile/exam.
Using separate subscriptions for route parameters, profile info, and exam start state is clear. They are also properly unsubscribed.
238-238: LTI subscription integration.
Subscribing toisShownViaLti$is handled properly. Ensuring you unsubscribe inngOnDestroyis equally good.src/main/webapp/app/overview/course-sidebar/course-sidebar.component.html (1)
1-114: Well-structured layout and use of new Angular syntax.
The use of@ifand@foris consistent with the guidelines. The collapsible design withngbDropdownhandles multiple course items logically.src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts (5)
1-45: Imports and new interfaces are clearly defined.
The introducedCourseActionItemandSidebarIteminterfaces are concise and typed properly. This is a clean approach for item definitions.
60-82: New standalone component structure looks correct.
Declaring necessary imports in the@Componentmetadata ensures this standalone component is self-sufficient and modular.
83-93: Signal-based inputs are a modern Angular approach.
Using signals is advanced yet valid. Ensure you keep an eye on potential bridging complexities if mixing with older Angular patterns, though no issues are apparent here.
147-174: Threshold-based hiding logic.
Your approach to hiding items based on window height is a neat solution. Just be mindful of edge cases (e.g., extremely small viewport).
395-404: Unenrollment logic.
Providing a dedicated action item for student unenrollment is a clean solution. The modal integration is straightforward and fosters good UX.
helios-aet
bot
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
tobias-lippert
dismissed stale reviews from SimonEntholzer, chrisknedl, coolchock, ole-ve, and coderabbitai[bot]
via
9f8d4b6
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/webapp/app/shared/util/markdown.conversion.util.ts (1)
101-150: Consider renamingMarkdownitTagClassto maintain consistent naming.
The functionMarkdownitTagClassuses a mixed-casing style and could be renamed tomarkdownItTagClassormarkdownItTagClassPluginfor consistency with typical camelCase naming in TypeScript.-export function MarkdownitTagClass(markdown: MarkdownIt, mapping: TagClassMapping = {}): void { +export function markdownItTagClass(markdown: MarkdownIt, mapping: TagClassMapping = {}): void {src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts (2)
92-92: NewatlasEnabledinput introduced.
If this is not explicitly used in the sidebar template, confirm whether it’s planned for future features or if it should be removed to reduce confusion.
114-125: Wide icon set included.
All new icons appear consistent with the needs of the sidebar. If any are no longer required, consider removing them to keep the codebase clean and reduce bundle size.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (46)
src/main/webapp/app/overview/course-overview.component.html(1 hunks)src/main/webapp/app/overview/course-overview.component.ts(7 hunks)src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts(1 hunks)src/main/webapp/app/overview/course-overview.component.html(0 hunks)src/main/webapp/app/overview/course-overview.component.ts(2 hunks)src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts(3 hunks)src/main/webapp/app/overview/course-overview.component.ts(0 hunks)src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts(2 hunks)src/test/javascript/spec/component/course/course-overview.component.spec.ts(2 hunks)src/test/javascript/spec/component/course/course-sidebar.component.spec.ts(1 hunks)src/main/webapp/app/overview/course-overview.component.ts(0 hunks)src/main/webapp/app/overview/course-overview.component.ts(0 hunks)src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts(4 hunks)src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts(1 hunks)src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts(0 hunks)src/main/webapp/app/overview/course-overview.component.html(1 hunks)src/main/webapp/app/overview/course-overview.component.ts(2 hunks)src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts(5 hunks)src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts(4 hunks)src/test/javascript/spec/component/course/course-overview.component.spec.ts(0 hunks)src/test/javascript/spec/component/course/course-sidebar.component.spec.ts(11 hunks)build.gradle(8 hunks)gradle.properties(1 hunks)gradle/wrapper/gradle-wrapper.properties(1 hunks)jest.config.js(1 hunks)package.json(8 hunks)src/main/webapp/app/exam/manage/students/exam-students.component.html(1 hunks)src/main/webapp/app/exam/manage/students/upload-images/students-upload-images-button.component.ts(2 hunks)src/main/webapp/app/index.d.ts(0 hunks)src/main/webapp/app/shared/circular-progress-bar/circular-progress-bar.component.html(0 hunks)src/main/webapp/app/shared/circular-progress-bar/circular-progress-bar.component.scss(0 hunks)src/main/webapp/app/shared/circular-progress-bar/circular-progress-bar.component.ts(0 hunks)src/main/webapp/app/shared/course-group/course-group.component.html(1 hunks)src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.html(2 hunks)src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts(1 hunks)src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.html(1 hunks)src/main/webapp/app/shared/user-import/users-import-button.component.ts(2 hunks)src/main/webapp/app/shared/util/markdown.conversion.util.ts(3 hunks)src/test/javascript/spec/component/shared/circular-progress-bar.component.spec.ts(0 hunks)src/main/webapp/app/overview/course-overview.component.html(1 hunks)src/main/webapp/app/overview/course-overview.component.ts(10 hunks)src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts(3 hunks)src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts(3 hunks)src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts(2 hunks)src/test/javascript/spec/component/course/course-overview.component.spec.ts(7 hunks)src/test/javascript/spec/component/course/course-sidebar.component.spec.ts(4 hunks)
💤 Files with no reviewable changes (5)
- src/main/webapp/app/index.d.ts
- src/main/webapp/app/shared/circular-progress-bar/circular-progress-bar.component.scss
- src/test/javascript/spec/component/shared/circular-progress-bar.component.spec.ts
- src/main/webapp/app/shared/circular-progress-bar/circular-progress-bar.component.ts
- src/main/webapp/app/shared/circular-progress-bar/circular-progress-bar.component.html
✅ Files skipped from review due to trivial changes (3)
- src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts
- src/main/webapp/app/overview/course-overview.component.ts
- jest.config.js
🚧 Files skipped from review as they are similar to previous changes (11)
- src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts
- src/main/webapp/app/overview/course-overview.component.html
- src/main/webapp/app/overview/course-overview.component.ts
- src/main/webapp/app/overview/course-overview.component.html
- src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts
- src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts
- src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts
- src/main/webapp/app/overview/course-overview.component.html
- src/main/webapp/app/overview/course-overview.component.ts
- src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts
- src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts
🧰 Additional context used
📓 Path-based instructions (3)
`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/shared/course-group/course-group.component.htmlsrc/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.htmlsrc/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.htmlsrc/main/webapp/app/exam/manage/students/exam-students.component.htmlsrc/main/webapp/app/overview/course-overview.component.html
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.tssrc/main/webapp/app/exam/manage/students/upload-images/students-upload-images-button.component.tssrc/main/webapp/app/shared/user-import/users-import-button.component.tssrc/main/webapp/app/overview/course-overview.component.tssrc/main/webapp/app/overview/course-sidebar/course-sidebar.component.tssrc/main/webapp/app/shared/util/markdown.conversion.util.ts
`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/component/course/course-overview.component.spec.tssrc/test/javascript/spec/component/course/course-sidebar.component.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (56)
src/main/webapp/app/shared/course-group/course-group.component.html (1)
9-9: Event name is more descriptive. Good change!Updating from
(finish)to(importDone)creates a more self-documenting event name that better describes the action completion.src/main/webapp/app/shared/user-import/users-import-button.component.ts (2)
33-33: Good renaming for better semantic clarityChanging the output event from
finishtoimportDoneimproves code readability by clearly indicating what action has completed.
52-52: Updated event emission to match renamed outputCorrectly updated the event emission to use the renamed property.
src/main/webapp/app/exam/manage/students/upload-images/students-upload-images-button.component.ts (2)
32-32: Good renaming for better semantic clarityChanging the output event from
finishtouploadDoneimproves code readability by clearly indicating what action has completed.
48-48: Updated event emission to match renamed outputCorrectly updated the event emission to use the renamed property.
src/main/webapp/app/exam/manage/students/exam-students.component.html (2)
19-19: Updated event binding to match component's output nameCorrectly updated the event binding from
(finish)to(importDone)to match the renamed output in the UserImportButtonComponent.
22-22: Updated event binding to match component's output nameCorrectly updated the event binding from
(finish)to(uploadDone)to match the renamed output in the StudentsUploadImagesButtonComponent.src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts (1)
247-247: Appropriate addition of the isInCommunication property.The new input property follows Angular's modern input API pattern and is correctly implemented with a default value of false. This properly follows the camelCase naming convention per Angular style guidelines.
src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.html (1)
27-27: LGTM: Property binding correctly implemented.The
[isInCommunication]="true"binding is properly implemented and placed alongside other input properties. This ensures the markdown editor component knows it's being used in a communication context.src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.html (2)
48-50: Correctly updated conditional display logic with modern syntax.The condition properly uses the new Angular
@ifsyntax as specified in the coding guidelines, and correctly adds theisInCommunication()check to ensure the send button only appears in appropriate contexts.
77-79: Consistent implementation of updated conditional logic.This change maintains consistency with the earlier condition modification, ensuring the send button visibility logic is the same across different parts of the template.
gradle/wrapper/gradle-wrapper.properties (1)
3-3: Good update to stable Gradle versionUpdating from the release candidate (rc-2) to the stable release of Gradle 8.13 is a good practice for production code.
src/test/javascript/spec/component/course/course-overview.component.spec.ts (4)
62-62: Properly importing the new component for testingThe import of CourseSidebarComponent aligns with the PR objective of refactoring the sidebar into its own component.
187-187: Good use of MockComponent for the refactored sidebarUsing MockComponent from ng-mocks is the correct approach for isolating the CourseOverviewComponent in unit tests.
266-277: Good test coverage for new sidebar methodsThe test properly verifies that the new sidebar-related methods are called during initialization, which helps ensure the refactored code works correctly. The use of jest's toHaveBeenCalledOnce() also follows the test guidelines specified in the coding standards.
290-305: Comprehensive test cases for sidebar item generationThese new tests provide good coverage for the sidebar item generation logic, testing both when the analytics dashboard is enabled and the default case. This ensures the sidebar properly displays the correct navigation items in different scenarios.
gradle.properties (1)
21-51:❓ Verification inconclusive
Good dependency version updates
Updating to the latest library versions is good practice for incorporating bug fixes and security patches. The JUnit update to 5.12.0 is more significant than the other minor version updates.
Since the JUnit version update from 5.11.4 to 5.12.0 is more significant than the other minor version bumps, it's worth verifying that there aren't any breaking changes that might affect the test suite:
🏁 Script executed:
#!/bin/bash # Check for failing tests that might be related to the JUnit update ./gradlew test --tests "*Test" --info | grep -E "(FAILED|junit)"Length of output: 256
Action Required: Re-run tests with a valid JAVA_HOME
The dependency updates look good overall. However, the verification script for the JUnit update couldn’t provide any meaningful output due to an environment error (JAVA_HOME is set to an invalid directory), so we can’t confirm that no tests broke with JUnit 5.12.0. Please manually re-run the tests after setting JAVA_HOME to a valid Java installation. For example:
export JAVA_HOME=/path/to/valid/java-installation ./gradlew test --tests "*Test" --info | grep -E "(FAILED|junit)"Once you verify that the tests pass without issues, please update the status accordingly.
src/test/javascript/spec/component/course/course-sidebar.component.spec.ts (7)
10-24: Good use of modern testing imports and utilitiesThe test file correctly imports the necessary testing utilities and uses MockProvider from ng-mocks, which aligns with the specified coding guidelines. Adding the ArtemisServerDateService and dayjs imports ensures proper date handling capabilities in the tests.
37-49: Well-structured test data with comprehensive propertiesThe test courses have been enhanced with all the necessary properties to thoroughly test sidebar functionality, including competencies, tutorial groups, and prerequisites counts that would affect sidebar display.
131-131: Properly mocking the server date serviceUsing MockProvider for ArtemisServerDateService ensures the tests run deterministically without actual server dependencies.
139-147: Modern component input initializationUsing
fixture.componentRef.setInput()instead of direct property assignment follows best practices for Angular component testing, making the test more resilient to Angular's change detection mechanisms.
150-158: Comprehensive initialization testingGood practice to verify that all necessary methods are called during component initialization, with proper parameter validation.
161-225: Good use of Angular signals for state managementThe test uses Angular's signal-based reactivity system (using
.set()methods and function-style accessors likeanyItemHidden()) which represents a more modern and robust approach compared to direct property access.
228-256: Thorough testing of UI state transitionsThe tests properly verify how the component responds to changes in inputs and state, testing both the behavior and the visual display aspects of the sidebar.
src/main/webapp/app/overview/course-overview.component.html (1)
21-37: Consider passing the newatlasEnabledproperty for consistency.
The<jhi-course-sidebar>component now supports anatlasEnabledinput, but it's not used here. If this functionality is relevant in the overview, you may want to pass it as an input to keep the component configuration consistent with its declarations.src/main/webapp/app/shared/util/markdown.conversion.util.ts (1)
53-78: Validate plugin ordering to avoid unintentional token modifications.
You're chaining multiple MarkdownIt extensions, which can potentially introduce ordering constraints (e.g., if one plugin transforms tokens that another plugin depends upon). Verify that the final rendering aligns with your expectations and that none of the plugin transformations conflict.build.gradle (9)
22-22: Confirm plugin compatibility.
Updatingcom.gorylenko.gradle-git-propertiesplugin to 2.5.0 generally works well, but ensure no breaking changes occurred in how Git properties are generated and consumed.
40-40: Check Gradle 8.13 stability.
Upgrading to Gradle 8.13 may impact certain plugins or scripts that rely on prior behavior. Verify that the build and all tasks, including Docker tasks, run successfully with this version.
108-108: Excluding legacy JUnit from the classpath.
Removing the old JUnit dependency is recommended when migrating fully to JUnit 5. No concerns here.
162-162: Verify new GitLab4J API version.
Upgrading from rc.8 to rc.9 is typically minor, but ensure that any feature usage in the GitLab4J library remains compatible.
227-227: Check for newer jsoup versions.
jsoup:1.19.1is relatively recent, but it's good to confirm if a patch or minor update like1.19.2exists to address potential bugs or security issues.
326-326: Ensure no security advisories for Nimbus.
com.nimbusds:nimbus-jose-jwt:10.0.2is a recent version, but double-check for any new security advisories or patches.
415-415: Validate Gradle Tooling API updates.
Includingorg.gradle:gradle-tooling-api:8.13should match your Gradle wrapper. Confirm compatibility to avoid version mismatch.
429-433: Good transition to JUnit 5.
Adopting the JUnit 5 libraries (org.junit.jupiter) is a best practice and aligns with modern testing frameworks. No issues found.
435-436: JUnit Platform additions.
Introducing platform dependencies (junit-platform-commonsandjunit-platform-engine) further solidifies the JUnit 5 setup. Looks good.src/main/webapp/app/overview/course-sidebar/course-sidebar.component.ts (5)
83-85: Enforcing non-nullable course inputs.
Switching fromCourse | undefinedtoCourseensures the course is always provided. Verify that upstream callers satisfy this new requirement to avoid runtime errors.
96-97: Migrating to signals for sidebar items.
This approach can simplify reactivity. Ensure change detection is triggered properly and that the rest of your code listens for these signal updates.
99-99:canUnenrollsignal addition.
Make sure to handle this signal in the template or other logic if the unenrollment feature is fully implemented. Otherwise, it could be misleading to have an unused state variable.
127-128: Using Angular's DI withinject().
This is a modern approach in Angular. Just be mindful that this pattern can complicate testing if providers aren't configured properly in your test setup.
130-134: Initializing sidebar items and action items onngOnInit.
Good practice—ensures the data is ready before change detection runs. Validate any external dependencies or asynchronous fetches if they’re integrated within these initialization functions.src/main/webapp/app/overview/course-overview.component.ts (12)
23-23: Appropriate imports added for the new sidebar functionality.The imports are well organized and include all necessary modules for the new sidebar component implementation.
Also applies to: 63-64, 77-77, 81-81, 110-111, 117-118
144-147: Good addition of strongly typed properties for sidebar functionality.The new properties
sidebarItems,courseActionItems, andcanUnenrollare properly typed using appropriate interfaces, improving code robustness and maintainability.
253-254: Excellent initialization of sidebar items in ngOnInit.The initialization of sidebar items and course action items is appropriately placed in the ngOnInit lifecycle hook, ensuring that these items are created when the component is initialized.
289-297: Well-structured method for generating course action items.The
getCourseActionItemsmethod cleanly implements the logic for determining available actions for the current course, with proper conditional logic for the unenroll functionality.
299-334: Excellent implementation of sidebar items generation.The
getSidebarItemsmethod is well organized and follows a clear pattern to build the sidebar dynamically based on course properties. The code makes good use of the helper methods to create specific items and appropriately positions them in the sidebar.
336-344: Clean implementation of unenroll item creation.The
getUnenrollItemmethod creates a properly structured action item with all required properties, including the translation key and action callback.
346-480: Great modularization of sidebar item creation methods.Breaking down the sidebar item creation into individual methods for each type of item (lectures, exams, communications, etc.) improves code readability and maintenance. Each method returns a properly structured item with all necessary properties.
510-512: Good implementation of student unenrollment check.The
canStudentUnenrollmethod performs a proper validation by checking both the course's unenrollment flag and the current date against the unenrollment end date.
520-523: Properly implemented modal opening for student unenrollment.The
openUnenrollStudentModalmethod correctly uses the NgbModal service to open the unenrollment modal dialog and passes the current course to the modal component.
756-765: Well-implemented visibility check for exams.The
hasVisibleExamsmethod correctly iterates through the course's exams to determine if any should be visible based on their visibility date compared to the current server date.
770-772: Concise competencies check method.The
hasCompetenciesmethod efficiently checks for the presence of competencies or prerequisites using a nullish coalescing operator.
777-779: Effective tutorial groups check.The
hasTutorialGroupsmethod uses a concise approach to determine if the course has any tutorial groups.package.json (4)
16-27: Angular dependencies updated to consistent versions.The Angular packages have been updated to matching versions (19.2.1/19.2.2), ensuring compatibility between the core framework and related libraries.
39-39: Third-party library versions updated.Dependencies like Sentry, core-js, and simple-statistics have been updated to newer versions, which likely include bug fixes and security improvements.
Also applies to: 47-47, 75-75
104-104: Build tool dependencies properly updated.Build-related packages like ESLint and Vite have been updated to their latest compatible versions.
Also applies to: 112-113, 127-127
137-147: Development dependencies consistently updated.The Angular ESLint packages, TypeScript-related libraries, and testing tools have been updated to ensure a consistent development environment.
Also applies to: 155-164, 166-168, 184-191
helios-aet
bot
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
Checklist
General
Client
Motivation and Context
We want to introduce the sidebar layout in the course management view with the next major release.
As first step make the sidebar in the student course view its own component to be able to reuse it in the management view
Description
Created a new component
course-sidebarand adjustedcourse-overviewto use the newcourse-sidebar.Steps for Testing
Prerequisites:
Exam Mode 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
Code Review
Manual Tests
Exam Mode Test
Test Coverage
Summary by CodeRabbit