Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

General: Display recently accessed courses on top #7827

Merged
merged 6 commits into from
Dec 29, 2023

Conversation

bassner
Copy link
Member

@bassner bassner commented Dec 27, 2023

Checklist

General

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data.
  • I followed the coding and design guidelines.
  • Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
  • I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I added authorities to all new routes and checked the course groups for displaying navigation elements (links, buttons).
  • I documented the TypeScript code using JSDoc style.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

Motivation and Context

As a student or instructor with a lot of courses, especially users administrating an Artemis instance, you can have a hard time to find the courses that are currently relevant to you. In particular, during any given semester you're likely to access a small set of courses frequently; however, you still have to crawl the entire list of courses to find them, wasting a lot of time. Therefore, we should speed up that process.

Description

Added a service that stores the last access timestamp for a course in the local storage and provides a list of the most recently accessed courses to other components.

In the course management view, Artemis will list the 3 most recently accessed courses in a separate "semester" right at the start.

In the course overview, Artemis will display the boxes of the 3 most recently accessed courses first if there are more than 5 courses for a user.

Steps for Testing

Prerequisites:

  • 1 Instructor with > 10 courses
  1. Log in to Artemis
  2. Navigate to Course Management
  3. Access two courses (just visit their dashboards)
  4. Return to Course Management
  5. Observe that the courses you accessed are listed in a separate "semester" right at the start
  6. Navigate to the student course overview
  7. Observe that the courses you accessed are listed separately right at the start

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked







Review Progress

Performance Review

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Exam Mode Test

  • Test 1
  • Test 2

Test Coverage

Class/File Line Coverage Confirmation (assert/expect)
course-access-storage.service.ts 100%
course-management-tab-bar.component.ts 95% ✅ ❌
course-management.component.ts 77.52% ✅ ❌
course-overview.component.ts 95.48% ✅ ❌
courses.component.ts 93.05% ✅ ❌

Screenshots

CleanShot 2023-12-27 at 19 44 49
CleanShot 2023-12-27 at 19 46 53

Summary by CodeRabbit

  • New Features

    • Introduced a service for managing course access data, enabling tracking and retrieval of recently accessed courses.
    • Enhanced course management components to display a message for recent semesters.
  • Enhancements

    • Updated course overview to differentiate between recently accessed courses and regular courses.
    • Added logic to sort courses into recently accessed and regular categories.
  • Tests

    • Integrated CourseAccessStorageService into existing component tests.
    • Added new test cases for the service and updated components to ensure functionality.

@bassner bassner added this to the 6.7.4 milestone Dec 27, 2023
@bassner bassner self-assigned this Dec 27, 2023
@github-actions github-actions bot added the client Pull requests that update TypeScript code. (Added Automatically!) label Dec 27, 2023
@github-actions github-actions bot added the tests label Dec 29, 2023
@bassner bassner marked this pull request as ready for review December 29, 2023 11:11
@bassner bassner requested a review from a team as a code owner December 29, 2023 11:11
Copy link

coderabbitai bot commented Dec 29, 2023

Walkthrough

The recent updates introduce a service to track and store data on course access within an Angular application. This service is integrated across various components and templates, enabling the display of recently accessed courses and the sorting of courses into 'recent' and 'regular' categories. The changes enhance the user interface by providing users with a quick view of their last accessed courses, improving navigation and course management efficiency.

Changes

File Pattern Change Summary
.../course-access-storage.service.ts Introduced service for managing course access data.
.../course-management-tab-bar.component.ts, .../overview/course-overview.component.ts Added CourseAccessStorageService import and usage.
.../course-management.component.html, .../overview/courses.component.html Added new conditions for displaying 'recent' semester and sorting courses.
.../course-management.component.ts, .../overview/courses.component.ts Integrated CourseAccessStorageService and added sorting logic for recently accessed courses.
.../component/course-management-tab-bar.component.spec.ts, .../component/course-management.component.spec.ts, .../component/course-overview.component.spec.ts, .../component/course.component.spec.ts Updated component tests to include CourseAccessStorageService and added new test cases.
.../service/course-access-storage.service.spec.ts Introduced unit tests for the CourseAccessStorageService.
.../jest.config.js Modified test coverage thresholds in jest.config.js.

Poem

In the land of code, where logic hops around,
A rabbit weaves changes, with no bound.
🐇💾 With each key tap, courses align,
Now recent paths in the UI, finely shine. 🌟✨

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?

Share

Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • You can directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
  • You can tag CodeRabbit on specific lines of code or entire files in the PR by tagging @coderabbitai in a comment. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • You can tag @coderabbitai in a PR comment and ask questions about the PR and the codebase. Examples:
    • @coderabbitai generate interesting stats about this repository from git and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid.
    • @coderabbitai read the files in the src/scheduler package and generate README in the markdown format.

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.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 6

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between ea888b2 and ddc194d.
Files ignored due to filter (4)
  • src/main/webapp/i18n/de/course.json
  • src/main/webapp/i18n/de/student-dashboard.json
  • src/main/webapp/i18n/en/course.json
  • src/main/webapp/i18n/en/student-dashboard.json
Files selected for processing (12)
  • src/main/webapp/app/course/course-access-storage.service.ts (1 hunks)
  • src/main/webapp/app/course/manage/course-management-tab-bar/course-management-tab-bar.component.ts (3 hunks)
  • src/main/webapp/app/course/manage/course-management.component.html (2 hunks)
  • src/main/webapp/app/course/manage/course-management.component.ts (3 hunks)
  • src/main/webapp/app/overview/course-overview.component.ts (3 hunks)
  • src/main/webapp/app/overview/courses.component.html (1 hunks)
  • src/main/webapp/app/overview/courses.component.ts (4 hunks)
  • src/test/javascript/spec/component/course/course-management-tab-bar.component.spec.ts (5 hunks)
  • src/test/javascript/spec/component/course/course-management.component.spec.ts (3 hunks)
  • src/test/javascript/spec/component/course/course-overview.component.spec.ts (7 hunks)
  • src/test/javascript/spec/component/course/course.component.spec.ts (4 hunks)
  • src/test/javascript/spec/service/course-access-storage.service.spec.ts (1 hunks)
Additional comments: 26
src/main/webapp/app/course/course-access-storage.service.ts (2)
  • 10-10: The constructor is correctly injecting the LocalStorageService for use within the service.

  • 25-31: The method getLastAccessedCourses correctly retrieves and sorts the courses by last access time. However, it assumes that the course IDs are stored as object keys, which are always strings. Ensure that the conversion from string to number (Number(entry[0])) does not lead to any unexpected behavior if non-numeric keys are ever introduced.

src/test/javascript/spec/service/course-access-storage.service.spec.ts (1)
  • 25-30: The test 'should store accessed course' correctly verifies that the onCourseAccessed method stores the course access information.
src/main/webapp/app/course/manage/course-management.component.html (1)
  • 41-51: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [33-48]

The new condition to display a message for the 'recent' semester is implemented correctly. However, ensure that the translation keys like 'artemisApp.course.recentlyAccessed' are defined in the translation files to avoid missing translations.

src/main/webapp/app/course/manage/course-management-tab-bar/course-management-tab-bar.component.ts (1)
  • 110-118: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [89-115]

The addition of CourseAccessStorageService to CourseManagementTabBarComponent and its usage in ngOnInit is implemented correctly. However, ensure that the courseId variable is always defined before it's used in ngOnInit, as it relies on the route parameters being available.

src/test/javascript/spec/component/course/course-management-tab-bar.component.spec.ts (1)
  • 88-95: The new test case 'should register changes in course and notify courseAccessStorageService on init' correctly verifies that the service is called during component initialization. Ensure that the onCourseAccessed method is mocked if it has side effects or makes HTTP requests to prevent flaky tests.
src/test/javascript/spec/component/course/course-management.component.spec.ts (4)
  • 27-27: The import of CourseAccessStorageService is correctly added to the test file.

  • 106-110: The CourseAccessStorageService is correctly provided in the TestBed configuration. Ensure that the mock of this service behaves as expected in the test environment.

  • 143-153: The test case for sorting unique semester names appears to be correct and should validate the sorting logic effectively.

  • 156-177: The test case for sorting courses into semesters now includes the logic for recently accessed courses. It's important to verify that the mock for getLastAccessedCourses returns the expected values to ensure the test is valid.

src/main/webapp/app/course/manage/course-management.component.ts (3)
  • 14-14: The CourseAccessStorageService is correctly imported in the component file.

  • 50-50: The CourseAccessStorageService is correctly added to the constructor for dependency injection.

  • 141-174: The logic for sorting courses into semesters now includes handling for recently accessed courses. Ensure that the getLastAccessedCourses method from CourseAccessStorageService is being used correctly and that the sorting logic is sound.

src/main/webapp/app/overview/course-overview.component.ts (3)
  • 37-37: The CourseAccessStorageService is correctly imported in the component file.

  • 114-114: The CourseAccessStorageService is correctly added to the constructor for dependency injection.

  • 125-125: The call to onCourseAccessed is correctly placed within the ngOnInit lifecycle hook to track when a course is accessed. Ensure that this method is called every time the component is initialized with a new course.

src/test/javascript/spec/component/course/course.component.spec.ts (4)
  • 39-39: The import of CourseAccessStorageService is correctly added to support the new functionality.

  • 112-112: The addition of courseAccessStorageService as a member variable in the CoursesComponent class is consistent with the use of dependency injection in Angular.

  • 141-141: The CourseAccessStorageService is correctly provided as a mock provider in the test module configuration, which is necessary for isolating the unit tests.

  • 149-149: The courseAccessStorageService is correctly injected using TestBed.inject, which is the standard way to get a handle on injected services in Angular tests.

src/test/javascript/spec/component/course/course-overview.component.spec.ts (6)
  • 52-52: The import of CourseAccessStorageService is correctly added to support the new functionality.

  • 77-77: The explicit type declaration for exams as Exam[] improves type safety and clarity in the test setup.

  • 124-124: The addition of courseAccessStorageService as a member variable in the CourseOverviewComponent tests is consistent with the use of dependency injection in Angular.

  • 174-174: The CourseAccessStorageService is correctly provided as a mock provider in the test module configuration, which is necessary for isolating the unit tests.

  • 192-192: The courseAccessStorageService is correctly injected using TestBed.inject, which is the standard way to get a handle on injected services in Angular tests.

  • 218-218: The test case correctly simulates a course access event by calling courseAccessStorageService.onCourseAccessed. This is important for verifying that the component notifies the service about course access, which is part of the new feature's functionality.

Comment on lines +225 to +260
it('should sort courses into regular and recently accessed after loading', () => {
const findAllForDashboardSpy = jest.spyOn(courseService, 'findAllForDashboard');
const sortCoursesInRecentlyAccessedAndRegularCoursesSpy = jest.spyOn(component, 'sortCoursesInRecentlyAccessedAndRegularCourses');
findAllForDashboardSpy.mockReturnValue(of(new HttpResponse({ body: coursesDashboard, headers: new HttpHeaders() })));

component.ngOnInit();

expect(findAllForDashboardSpy).toHaveBeenCalledOnce();
expect(sortCoursesInRecentlyAccessedAndRegularCoursesSpy).toHaveBeenCalledOnce();

const lastAccessedCourses = [1, 2];
const recentCoursesSpy = jest.spyOn(courseAccessStorageService, 'getLastAccessedCourses').mockReturnValue(lastAccessedCourses);

// Test for less than 5 courses
const courses = [];
for (let i = 1; i <= 3; i++) {
const course = { id: i };
courses.push(course);
}

component.courses = courses;
component.sortCoursesInRecentlyAccessedAndRegularCourses();
expect(component.regularCourses).toEqual(courses);
expect(component.recentlyAccessedCourses).toEqual([]);
expect(recentCoursesSpy).not.toHaveBeenCalled();

// Test for more than 5 courses
for (let i = 4; i <= 7; i++) {
const course = { id: i };
courses.push(course);
}
component.courses = courses;
component.sortCoursesInRecentlyAccessedAndRegularCourses();
expect(component.regularCourses).toEqual(courses.slice(2));
expect(component.recentlyAccessedCourses).toEqual(courses.slice(0, 2));
expect(recentCoursesSpy).toHaveBeenCalledOnce();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new test case checks the sorting of courses into regular and recently accessed categories. However, there are a few points to consider:

  1. The test case seems to be testing two different scenarios (less than 5 courses and more than 5 courses). It would be better to split this into two separate test cases for clarity and maintainability.
  2. The recentCoursesSpy is set up after the component.courses is already assigned and sortCoursesInRecentlyAccessedAndRegularCourses is called. The spy should be set up before these actions to capture any calls during the method execution.
  3. The test case is directly manipulating the component.courses property. It would be more robust to use the service's public API to simulate user interaction or lifecycle events that lead to the sorting logic being triggered.
-            const recentCoursesSpy = jest.spyOn(courseAccessStorageService, 'getLastAccessedCourses').mockReturnValue(lastAccessedCourses);
+            const recentCoursesSpy = jest.spyOn(courseAccessStorageService, 'getLastAccessedCourses');
+            recentCoursesSpy.mockReturnValue(lastAccessedCourses);

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
it('should sort courses into regular and recently accessed after loading', () => {
const findAllForDashboardSpy = jest.spyOn(courseService, 'findAllForDashboard');
const sortCoursesInRecentlyAccessedAndRegularCoursesSpy = jest.spyOn(component, 'sortCoursesInRecentlyAccessedAndRegularCourses');
findAllForDashboardSpy.mockReturnValue(of(new HttpResponse({ body: coursesDashboard, headers: new HttpHeaders() })));
component.ngOnInit();
expect(findAllForDashboardSpy).toHaveBeenCalledOnce();
expect(sortCoursesInRecentlyAccessedAndRegularCoursesSpy).toHaveBeenCalledOnce();
const lastAccessedCourses = [1, 2];
const recentCoursesSpy = jest.spyOn(courseAccessStorageService, 'getLastAccessedCourses').mockReturnValue(lastAccessedCourses);
// Test for less than 5 courses
const courses = [];
for (let i = 1; i <= 3; i++) {
const course = { id: i };
courses.push(course);
}
component.courses = courses;
component.sortCoursesInRecentlyAccessedAndRegularCourses();
expect(component.regularCourses).toEqual(courses);
expect(component.recentlyAccessedCourses).toEqual([]);
expect(recentCoursesSpy).not.toHaveBeenCalled();
// Test for more than 5 courses
for (let i = 4; i <= 7; i++) {
const course = { id: i };
courses.push(course);
}
component.courses = courses;
component.sortCoursesInRecentlyAccessedAndRegularCourses();
expect(component.regularCourses).toEqual(courses.slice(2));
expect(component.recentlyAccessedCourses).toEqual(courses.slice(0, 2));
expect(recentCoursesSpy).toHaveBeenCalledOnce();
it('should sort courses into regular and recently accessed after loading', () => {
const findAllForDashboardSpy = jest.spyOn(courseService, 'findAllForDashboard');
const sortCoursesInRecentlyAccessedAndRegularCoursesSpy = jest.spyOn(component, 'sortCoursesInRecentlyAccessedAndRegularCourses');
findAllForDashboardSpy.mockReturnValue(of(new HttpResponse({ body: coursesDashboard, headers: new HttpHeaders() })));
component.ngOnInit();
expect(findAllForDashboardSpy).toHaveBeenCalledOnce();
expect(sortCoursesInRecentlyAccessedAndRegularCoursesSpy).toHaveBeenCalledOnce();
const lastAccessedCourses = [1, 2];
const recentCoursesSpy = jest.spyOn(courseAccessStorageService, 'getLastAccessedCourses');
recentCoursesSpy.mockReturnValue(lastAccessedCourses);
// Test for less than 5 courses
const courses = [];
for (let i = 1; i <= 3; i++) {
const course = { id: i };
courses.push(course);
}
component.courses = courses;
component.sortCoursesInRecentlyAccessedAndRegularCourses();
expect(component.regularCourses).toEqual(courses);
expect(component.recentlyAccessedCourses).toEqual([]);
expect(recentCoursesSpy).not.toHaveBeenCalled();
// Test for more than 5 courses
for (let i = 4; i <= 7; i++) {
const course = { id: i };
courses.push(course);
}
component.courses = courses;
component.sortCoursesInRecentlyAccessedAndRegularCourses();
expect(component.regularCourses).toEqual(courses.slice(2));
expect(component.recentlyAccessedCourses).toEqual(courses.slice(0, 2));
expect(recentCoursesSpy).toHaveBeenCalledOnce();

Comment on lines +12 to +23
onCourseAccessed(courseId: number): void {
const courseAccessMap: { [key: number]: number } = this.localStorage.retrieve(CourseAccessStorageService.STORAGE_KEY) || {};

courseAccessMap[courseId] = Date.now();

if (Object.keys(courseAccessMap).length > 3) {
const oldestEntry = Object.entries(courseAccessMap).reduce((prev, curr) => (prev[1] < curr[1] ? prev : curr));
delete courseAccessMap[oldestEntry[0]];
}

this.localStorage.store(CourseAccessStorageService.STORAGE_KEY, courseAccessMap);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method onCourseAccessed correctly updates the timestamp for the accessed course. However, the logic to limit the stored courses to 3 is not scalable if the requirements change to store more or fewer courses. Consider making the limit a configurable property of the service.

- if (Object.keys(courseAccessMap).length > 3) {
+ const MAX_STORED_COURSES = 3; // This can be moved to a configurable part of the service
+ if (Object.keys(courseAccessMap).length > MAX_STORED_COURSES) {

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
onCourseAccessed(courseId: number): void {
const courseAccessMap: { [key: number]: number } = this.localStorage.retrieve(CourseAccessStorageService.STORAGE_KEY) || {};
courseAccessMap[courseId] = Date.now();
if (Object.keys(courseAccessMap).length > 3) {
const oldestEntry = Object.entries(courseAccessMap).reduce((prev, curr) => (prev[1] < curr[1] ? prev : curr));
delete courseAccessMap[oldestEntry[0]];
}
this.localStorage.store(CourseAccessStorageService.STORAGE_KEY, courseAccessMap);
}
onCourseAccessed(courseId: number): void {
const MAX_STORED_COURSES = 3; // This can be moved to a configurable part of the service
const courseAccessMap: { [key: number]: number } = this.localStorage.retrieve(CourseAccessStorageService.STORAGE_KEY) || {};
courseAccessMap[courseId] = Date.now();
if (Object.keys(courseAccessMap).length > MAX_STORED_COURSES) {
const oldestEntry = Object.entries(courseAccessMap).reduce((prev, curr) => (prev[1] < curr[1] ? prev : curr));
delete courseAccessMap[oldestEntry[0]];
}
this.localStorage.store(CourseAccessStorageService.STORAGE_KEY, courseAccessMap);
}

@@ -219,6 +224,7 @@ describe('CourseOverviewComponent', () => {
expect(getCourseStub).toHaveBeenCalled();
expect(subscribeForQuizChangesStub).toHaveBeenCalledOnce();
expect(subscribeToTeamAssignmentUpdatesStub).toHaveBeenCalledOnce();
expect(notifyAboutCourseAccessStub).toHaveBeenCalledExactlyOnceWith(course1.id);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test case should verify that courseAccessStorageService.onCourseAccessed is called with the correct course ID. It's important to ensure that the right course is being marked as accessed in the service.

-        expect(notifyAboutCourseAccessStub).toHaveBeenCalledExactlyOnceWith(course1.id);
+        expect(notifyAboutCourseAccessStub).toHaveBeenCalledWith(course1.id);

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
expect(notifyAboutCourseAccessStub).toHaveBeenCalledExactlyOnceWith(course1.id);
expect(notifyAboutCourseAccessStub).toHaveBeenCalledWith(course1.id);

Comment on lines +32 to +40
it('should retrieve last accessed courses and remove older courses', fakeAsync(() => {
const courseIds = [123, 456, 789, 101112, 7494];
courseIds.forEach((courseId) => {
service.onCourseAccessed(courseId);
tick(10); // Wait 10ms to ensure that the timestamp is different for each course
});
const lastAccessedCourses = service.getLastAccessedCourses();
expect(lastAccessedCourses).toEqual(courseIds.reverse().slice(0, 3));
}));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test 'should retrieve last accessed courses and remove older courses' correctly simulates accessing multiple courses and checks if only the 3 most recent accesses are kept. However, the test relies on a hardcoded delay (tick(10)) to ensure timestamps are different. This could be made more robust by explicitly setting different timestamps in the test.

- tick(10); // Wait 10ms to ensure that the timestamp is different for each course
+ // Set explicit timestamps to avoid reliance on tick
+ courseAccessMap[courseId] = Date.now() + courseId;

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
it('should retrieve last accessed courses and remove older courses', fakeAsync(() => {
const courseIds = [123, 456, 789, 101112, 7494];
courseIds.forEach((courseId) => {
service.onCourseAccessed(courseId);
tick(10); // Wait 10ms to ensure that the timestamp is different for each course
});
const lastAccessedCourses = service.getLastAccessedCourses();
expect(lastAccessedCourses).toEqual(courseIds.reverse().slice(0, 3));
}));
it('should retrieve last accessed courses and remove older courses', fakeAsync(() => {
const courseIds = [123, 456, 789, 101112, 7494];
courseIds.forEach((courseId) => {
service.onCourseAccessed(courseId);
// Set explicit timestamps to avoid reliance on tick
courseAccessMap[courseId] = Date.now() + courseId;
});
const lastAccessedCourses = service.getLastAccessedCourses();
expect(lastAccessedCourses).toEqual(courseIds.reverse().slice(0, 3));
}));

Comment on lines +52 to +67
@if (recentlyAccessedCourses.length > 0) {
<div class="row">
<h4 class="col mb-3 fw-medium" jhiTranslate="artemisApp.studentDashboard.recentlyAccessed">Recently Accessed Courses</h4>
</div>
<div class="row">
@for (course of recentlyAccessedCourses; track course) {
<jhi-overview-course-card class="col-12 col-lg-6 col-xl-4 pe-2 ps-2 mb-3" [course]="course" [hasGuidedTour]="course === courseForGuidedTour">
</jhi-overview-course-card>
}
</div>
@if (regularCourses.length > 0) {
<div class="row">
<h4 class="col mb-3 fw-medium" jhiTranslate="artemisApp.studentDashboard.otherCourses">Other Courses</h4>
</div>
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional rendering for 'Recently Accessed Courses' and 'Other Courses' is implemented correctly using Angular's structural directives. Ensure that the trackBy function is implemented for the ngFor directive to improve performance by minimizing DOM manipulations.

- @for (course of recentlyAccessedCourses; track course) {
+ @for (course of recentlyAccessedCourses; trackBy: trackCourse) {

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
@if (recentlyAccessedCourses.length > 0) {
<div class="row">
<h4 class="col mb-3 fw-medium" jhiTranslate="artemisApp.studentDashboard.recentlyAccessed">Recently Accessed Courses</h4>
</div>
<div class="row">
@for (course of recentlyAccessedCourses; track course) {
<jhi-overview-course-card class="col-12 col-lg-6 col-xl-4 pe-2 ps-2 mb-3" [course]="course" [hasGuidedTour]="course === courseForGuidedTour">
</jhi-overview-course-card>
}
</div>
@if (regularCourses.length > 0) {
<div class="row">
<h4 class="col mb-3 fw-medium" jhiTranslate="artemisApp.studentDashboard.otherCourses">Other Courses</h4>
</div>
}
}
@if (recentlyAccessedCourses.length > 0) {
<div class="row">
<h4 class="col mb-3 fw-medium" jhiTranslate="artemisApp.studentDashboard.recentlyAccessed">Recently Accessed Courses</h4>
</div>
<div class="row">
@for (course of recentlyAccessedCourses; trackBy: trackCourse) {
<jhi-overview-course-card class="col-12 col-lg-6 col-xl-4 pe-2 ps-2 mb-3" [course]="course" [hasGuidedTour]="course === courseForGuidedTour">
</jhi-overview-course-card>
}
</div>
@if (regularCourses.length > 0) {
<div class="row">
<h4 class="col mb-3 fw-medium" jhiTranslate="artemisApp.studentDashboard.otherCourses">Other Courses</h4>
</div>
}
}

Comment on lines +123 to +135
/**
* Sorts the courses into recently accessed and regular courses.
* If there are less than 5 courses, all courses are displayed in the regular courses section.
*/
sortCoursesInRecentlyAccessedAndRegularCourses() {
if (this.courses.length <= 5) {
this.regularCourses = this.courses;
} else {
const lastAccessedCourseIds = this.courseAccessStorageService.getLastAccessedCourses();
this.recentlyAccessedCourses = this.courses.filter((course) => lastAccessedCourseIds.includes(course.id!));
this.regularCourses = this.courses.filter((course) => !lastAccessedCourseIds.includes(course.id!));
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method sortCoursesInRecentlyAccessedAndRegularCourses is implemented correctly. However, consider adding error handling for cases where course.id might be undefined to prevent runtime errors.

- this.recentlyAccessedCourses = this.courses.filter((course) => lastAccessedCourseIds.includes(course.id!));
+ this.recentlyAccessedCourses = this.courses.filter((course) => course.id && lastAccessedCourseIds.includes(course.id));

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
/**
* Sorts the courses into recently accessed and regular courses.
* If there are less than 5 courses, all courses are displayed in the regular courses section.
*/
sortCoursesInRecentlyAccessedAndRegularCourses() {
if (this.courses.length <= 5) {
this.regularCourses = this.courses;
} else {
const lastAccessedCourseIds = this.courseAccessStorageService.getLastAccessedCourses();
this.recentlyAccessedCourses = this.courses.filter((course) => lastAccessedCourseIds.includes(course.id!));
this.regularCourses = this.courses.filter((course) => !lastAccessedCourseIds.includes(course.id!));
}
}
/**
* Sorts the courses into recently accessed and regular courses.
* If there are less than 5 courses, all courses are displayed in the regular courses section.
*/
sortCoursesInRecentlyAccessedAndRegularCourses() {
if (this.courses.length <= 5) {
this.regularCourses = this.courses;
} else {
const lastAccessedCourseIds = this.courseAccessStorageService.getLastAccessedCourses();
this.recentlyAccessedCourses = this.courses.filter((course) => course.id && lastAccessedCourseIds.includes(course.id));
this.regularCourses = this.courses.filter((course) => !lastAccessedCourseIds.includes(course.id!));
}
}

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between ddc194d and 40d30ea.
Files selected for processing (1)
  • jest.config.js (1 hunks)
Files skipped from review due to trivial changes (1)
  • jest.config.js

@bassner bassner merged commit ce8cb00 into develop Dec 29, 2023
21 of 26 checks passed
@bassner bassner deleted the feature/recently-accessed-courses branch December 29, 2023 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Pull requests that update TypeScript code. (Added Automatically!) enhancement ready for review tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant