General: Add subgrouping to sidebar for lectures and exercises
#10608
Conversation
github-actions
bot
added
tests
client
End-to-End (E2E) Test Results Summary
|
|||||||||||||||||||||||||||||||||||||||
WalkthroughThis pull request extends the data model for lectures by introducing a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SidebarAccordionComponent
participant dayjs
User->>SidebarAccordionComponent: Trigger sidebar render
SidebarAccordionComponent->>SidebarAccordionComponent: Call getGroupedByWeek(groupKey)
SidebarAccordionComponent->>dayjs: Compute week key for each item
dayjs-->>SidebarAccordionComponent: Return computed week key
SidebarAccordionComponent->>SidebarAccordionComponent: Group items by week using the week keys
SidebarAccordionComponent-->>User: Return grouped sidebar data for rendering
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
🧹 Nitpick comments (2)
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (2)
108-112: Consider adding locale support for internationalization.The
getWeekKeymethod correctly formats the week range, but it doesn't account for locale-specific date formatting, which could be important for internationalization in Artemis.private getWeekKey(date: dayjs.Dayjs): string { const weekStart = date.startOf('isoWeek'); const weekEnd = date.endOf('isoWeek'); - return `${weekStart.format('DD MMM')} - ${weekEnd.format('DD MMM YYYY')}`; + return `${weekStart.format('DD MMM')} - ${weekEnd.format('DD MMM YYYY')}`.localeData(); }
114-159: Improve date parsing in the sorting function.The implementation correctly groups items by week and handles various edge cases. However, the date parsing for sorting (lines 155-156) doesn't include the year, which could lead to incorrect sorting for weeks spanning different years.
// In the sorting function, parse the complete date string including year - const aDate = dayjs(a.weekRange.split(' - ')[0], 'DD MMM'); - const bDate = dayjs(b.weekRange.split(' - ')[0], 'DD MMM'); + const aDate = dayjs(a.weekRange.split(' - ')[1], 'DD MMM YYYY').startOf('isoWeek'); + const bDate = dayjs(b.weekRange.split(' - ')[1], 'DD MMM YYYY').startOf('isoWeek'); - return aDate.isBefore(bDate) ? -1 : 1; + return aDate.isBefore(bDate) ? -1 : aDate.isAfter(bDate) ? 1 : 0;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/webapp/app/core/course/overview/course-overview.service.ts(1 hunks)src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.html(1 hunks)src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts(2 hunks)src/main/webapp/app/shared/types/sidebar.ts(1 hunks)src/test/javascript/spec/component/shared/sidebar/sidebar-accordion.component.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalC...
src/main/webapp/app/shared/types/sidebar.tssrc/main/webapp/app/core/course/overview/course-overview.service.tssrc/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts
`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_logi...
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/shared/sidebar/sidebar-accordion.component.spec.ts
`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/**/*.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/sidebar/sidebar-accordion/sidebar-accordion.component.html
🧬 Code Definitions (1)
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (1)
src/main/webapp/app/shared/types/sidebar.ts (1)
SidebarCardElement(48-165)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (8)
src/main/webapp/app/shared/types/sidebar.ts (1)
159-162: Good addition of a well-documented property.The
startDateproperty is properly documented with JSDoc comments and follows the correct typing pattern with the optional modifier. This aligns well with the PR objective of enhancing the sidebar to support week-based grouping.src/main/webapp/app/core/course/overview/course-overview.service.ts (1)
311-311: LGTM: Consistent implementation of the startDate property.The property is correctly assigned from the lecture object, matching the interface definition and enabling the week-based grouping functionality.
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.html (1)
27-49: Well-structured implementation of weekly subgrouping.The template changes effectively implement the weekly subgrouping functionality with a clean UI hierarchy. The code:
- Changes from direct iteration over entities to using the new
getGroupedByWeekmethod- Adds conditional display of week ranges when appropriate
- Maintains consistent styling with
bg-bodyfor content andbg-modulefor containersThese changes properly implement the PR objective of organizing items into weekly buckets when there are more than five items present.
src/test/javascript/spec/component/shared/sidebar/sidebar-accordion.component.spec.ts (2)
15-18: Good setup of dayjs with proper plugin extension.Correctly importing and extending dayjs with the isoWeek plugin ensures consistent date handling throughout the tests.
161-259: Comprehensive test coverage for the new weekly grouping functionality.The tests thoroughly verify the behavior of the
getGroupedByWeekmethod under various conditions:
- Exam-type groups are handled correctly (lines 161-178)
- Small groups and filtered groups maintain their structure (lines 181-208)
- Large groups (>5 items) are properly split by week (lines 210-226)
- Items with only startDateWithTime are correctly grouped (lines 228-239)
- Items without dates are properly handled (lines 241-259)
The parameterized test approach reduces code duplication and the assertions verify both structure and content of the returned groups.
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (3)
11-14: Good setup of dayjs with isoWeek plugin.The dayjs setup with isoWeek plugin is correctly implemented, which will provide the necessary date manipulation capabilities for week-based grouping.
16-19: Well-defined interface for week grouping.The WeekGroup interface provides a clear structure for organizing sidebar items by their week ranges, with appropriate property names and types.
21-21: Good use of a named constant.Using a named constant for the threshold value instead of a magic number improves code readability and maintainability.
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (5)
115-142: Extract date access logic to a helper method.The repeated pattern of accessing dates via
item.exercise?.dueDate || item.startDateWithTime || item.startDateappears multiple times in the code. Consider extracting this to a helper method to improve readability and maintainability.+ private getItemDate(item: SidebarCardElement): dayjs.Dayjs | undefined { + return item.exercise?.dueDate || item.startDateWithTime || item.startDate; + } getGroupedByWeek(groupKey: string): WeekGroup[] { const items = this.groupedData[groupKey].entityData; // Apply search filter const filteredItems = this.searchValue ? items.filter((item) => item.title?.toLowerCase().includes(this.searchValue.toLowerCase()) || item.type?.toLowerCase().includes(this.searchValue.toLowerCase())) : items; // For exams, always return as a single group without week ranges if (groupKey === 'real' || groupKey === 'test' || groupKey === 'attempt') { return [{ weekRange: '', items: filteredItems }]; } if (filteredItems.length <= MIN_ITEMS_TO_GROUP_BY_WEEK || this.searchValue) { return [{ weekRange: '', items: filteredItems }]; } const weekGroups = new Map<string, SidebarCardElement[]>(); for (const item of filteredItems) { - const date = item.exercise?.dueDate || item.startDateWithTime || item.startDate; + const date = this.getItemDate(item); if (!date) { const noDateKey = 'No Date'; const noDateGroup = weekGroups.get(noDateKey) || []; noDateGroup.push(item); weekGroups.set(noDateKey, noDateGroup); continue; }
150-160: Use the same helper method for sorting logic.The date access pattern repeats in the sorting logic. Using the suggested helper method would also simplify this section.
// Sort items within each group by date (newest first) for (const [, items] of weekGroups) { items.sort((a, b) => { - const dateA = a.exercise?.dueDate || a.startDateWithTime || a.startDate; - const dateB = b.exercise?.dueDate || b.startDateWithTime || b.startDate; + const dateA = this.getItemDate(a); + const dateB = this.getItemDate(b); if (!dateA && !dateB) return 0; if (!dateA) return 1; if (!dateB) return -1; return dateB.valueOf() - dateA.valueOf(); }); }
162-186: Consider adding comments to complex sorting logic.The sorting logic for week groups is quite complex. Adding comments would enhance code readability and make future maintenance easier.
return Array.from(weekGroups.entries()) .map(([weekRange, items]) => { const displayRange = weekRange === 'No Date' ? weekRange : weekRange.split(' - ').slice(1).join(' - '); return { weekRange: displayRange, items }; }) .sort((a, b) => { + // Always place items with no date at the end if (a.weekRange === 'No Date') return 1; if (b.weekRange === 'No Date') return -1; + // Find the full week key with year information const aFullKey = Array.from(weekGroups.keys()).find((key) => key.includes(a.weekRange)); const bFullKey = Array.from(weekGroups.keys()).find((key) => key.includes(b.weekRange)); if (!aFullKey || !bFullKey) return 0; + // Sort by year first const [aYear] = aFullKey.split(' - '); const [bYear] = bFullKey.split(' - '); if (aYear !== bYear) { return Number(bYear) - Number(aYear); } + // For the same year, sort by date (newest first) const aDate = dayjs(a.weekRange.split(' - ')[0], 'DD MMM'); const bDate = dayjs(b.weekRange.split(' - ')[0], 'DD MMM'); return bDate.valueOf() - aDate.valueOf(); });
115-186: Consider breaking down the complex grouping method.The
getGroupedByWeekmethod is quite long and handles multiple responsibilities. Consider breaking it down into smaller helper methods to improve maintainability and testability.You could extract logic for:
- Filtering items
- Determining if grouping should be applied
- Creating the week groups
- Sorting the groups
This would make the main method more concise and easier to understand.
Example refactoring structure:
private filterItemsBySearch(items: SidebarCardElement[]): SidebarCardElement[] { // Search filtering logic } private shouldApplyGrouping(filteredItems: SidebarCardElement[], groupKey: string): boolean { // Logic to determine if grouping should be applied } private createWeekGroups(filteredItems: SidebarCardElement[]): Map<string, SidebarCardElement[]> { // Logic to create week groups } private sortWeekGroups(weekGroups: Map<string, SidebarCardElement[]>): WeekGroup[] { // Logic to sort and format week groups } getGroupedByWeek(groupKey: string): WeekGroup[] { const items = this.groupedData[groupKey].entityData; const filteredItems = this.filterItemsBySearch(items); if (!this.shouldApplyGrouping(filteredItems, groupKey)) { return [{ weekRange: '', items: filteredItems }]; } const weekGroups = this.createWeekGroups(filteredItems); return this.sortWeekGroups(weekGroups); }
119-121: Consider using the SearchFilterPipe instead of manual filtering.Since you're already importing SearchFilterPipe, consider leveraging it for consistency instead of implementing the search filter logic manually in this method.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalC...
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts
🧬 Code Definitions (1)
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (1)
src/main/webapp/app/shared/types/sidebar.ts (1)
SidebarCardElement(48-165)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Build .war artifact
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Analyse
🔇 Additional comments (5)
src/main/webapp/app/shared/sidebar/sidebar-accordion/sidebar-accordion.component.ts (5)
11-14: Correct usage of dayjs library and plugin.The import of the dayjs library and the isoWeek plugin is appropriate for handling date-based weekly grouping operations. The plugin extension is properly set up before usage.
16-19: Clean interface definition for week grouping.The WeekGroup interface is well-structured with clear property definitions, making the code more maintainable and type-safe.
21-21: Good use of constant for grouping threshold.Setting a clear constant for the minimum number of items required for grouping makes the code more maintainable and aligns with the PR objective of only grouping when necessary.
108-113: Well-implemented date formatting for week ranges.The getWeekKey method correctly calculates the ISO week boundaries and formats them appropriately for display.
167-185:✅ Verification successful
Verify sort order assumptions with test cases.
The sorting logic makes assumptions about date formatting and processing. Ensure comprehensive test cases cover different date scenarios to verify the correct ordering of week groups.
🏁 Script executed:
#!/bin/bash # Check for test cases that verify week sorting logic rg -A 3 "getGroupedByWeek" "src/test/javascript/spec"Length of output: 2765
Action: Sorting logic verified – tests confirm expected behavior
The existing test cases in
src/test/javascript/spec/component/shared/sidebar/sidebar-accordion.component.spec.ts
adequately exercise the sorting logic. They verify that:
- Entries with a week range of "No Date" sort to the end.
- Comparisons based on year extraction and date parsing work as expected.
If there’s any concern about additional edge cases (e.g., unexpected date formats or missing week range values), consider expanding the tests, but current tests address the main assumptions.
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
End-to-End (E2E) Test Results Summary
|
|||||||||||||||||||||||||||||||||||||||
helios-staging-aet
bot
temporarily deployed
to
artemis-test7.artemis.cit.tum.de
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
Checklist
General
Client
Motivation and Context
This PR introduces consistent sub‐grouping for exercises (and lectures). It allows us to display items in sections by weekly buckets when there are more than 5 items.
Description
Steps for Testing
Prerequisites::
Course with at least 6 exercise or lecture in the same section (Current, Past or Future)
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.
Code Review
Manual Tests
Test Coverage
Client
Client
Screenshots
Summary by CodeRabbit
New Features
Tests