General: Implement Course Archive to see old courses from previous semesters
#9343
Conversation
Pull latest changes
Pull latest changes
Pull latest changes
pull latest changes
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (1)
- src/test/javascript/spec/component/course/course-archive.component.spec.ts (1 hunks)
Additional context used
Path-based instructions (1)
src/test/javascript/spec/component/course/course-archive.component.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/course/course-archive.component.spec.ts
Show resolved
Hide resolved
src/test/javascript/spec/component/course/course-archive.component.spec.ts
Show resolved
Hide resolved
src/test/javascript/spec/component/course/course-archive.component.spec.ts
Show resolved
Hide resolved
src/test/javascript/spec/component/course/course-archive.component.spec.ts
Show resolved
Hide resolved
edkaya
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
I just tested it on TS3 and noticed that the course images reload quite often. For example, when collapsing a semester in the archive and then opening it again, all course images appear to be loaded again from scratch. Same thing with search, if I remove parts of the search string, any courses that appear will reload their image.
Is it possible to cache these images?
There was a problem hiding this comment.
looks good in general, i would just move the server side query more into the db.
And maybe one UI change: currently the current semester is also displayed in the archive. does this make sense?
because those courses are already visible on the main course overview.
but I have no strong opinion on that so ill leave that open for discussion
src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java
Outdated
Show resolved
Hide resolved
edkaya
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
src/main/webapp/app/overview/course-archive/course-archive.component.ts
Outdated
Show resolved
Hide resolved
edkaya
dismissed stale reviews from BBesrour, coolchock, SimonEntholzer, and coderabbitai
via
1884775
We actually use caching strategy for course card icons to load them and cache them in the local storage. For the current local storage, the maxCacheCount is 30, which means that 30 images (or entries) can be stored in localStorage. So in the test servers, there is way more than 30 icons on that page, leading to not to cache all icons. However, I will have a detailed look in a follow up, because this will be the case for course overview as well. |
There was a problem hiding this comment.
Actionable comments posted: 8
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (6)
- src/main/java/de/tum/cit/aet/artemis/core/repository/CourseRepository.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java (1 hunks)
- src/main/webapp/app/overview/course-archive/course-archive.component.ts (1 hunks)
- src/main/webapp/app/overview/courses.component.html (1 hunks)
- src/main/webapp/i18n/de/student-dashboard.json (1 hunks)
- src/main/webapp/i18n/en/student-dashboard.json (1 hunks)
Additional context used
Path-based instructions (5)
src/main/java/de/tum/cit/aet/artemis/core/repository/CourseRepository.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/webapp/app/overview/course-archive/course-archive.component.ts (1)
src/main/webapp/app/overview/courses.component.html (1)
Pattern
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/i18n/de/student-dashboard.json (1)
Pattern
src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
Additional comments not posted (1)
src/main/webapp/i18n/en/student-dashboard.json (1)
15-18: Consider refining the archive text structure for better internationalizationThe addition of the "archive" object aligns well with the PR objective of implementing a course archive feature. However, there are two points to consider:
Splitting the sentence into two parts ("oldCourses" and "here") might create challenges for translators in other languages where word order or sentence structure differs significantly from English.
The "here" string is very generic and could potentially be reused in other contexts throughout the application.
Consider the following suggestions:
Combine the two strings into a single entry with a placeholder:
"archive": { "oldCoursesLink": "Looking for old courses? Click {0}" }This approach allows for more flexibility in translation while maintaining the clickable nature of the text.
Move the "here" string to a more shared location in the i18n structure, as it could be reused in various contexts. This aligns with the previous reviewer's comment about moving shared words to a common location.
To check if "here" is used in other contexts, you can run:
This will help determine if creating a shared location for the "here" string would be beneficial.
| @if (coursesLoaded) { | ||
| <div class="d-flex justify-content-center mt-3"> | ||
| <div jhiTranslate="artemisApp.studentDashboard.archive.oldCourses"></div> | ||
| <div> </div> | ||
| <a [routerLink]="['/courses/archive']" jhiTranslate="artemisApp.studentDashboard.archive.here"></a> | ||
| </div> | ||
| } |
There was a problem hiding this comment.
LGTM! Minor suggestion for consistency.
The new code block successfully implements the course archive feature as described in the PR objectives. It correctly uses the @if directive and jhiTranslate for internationalization.
For consistency with the rest of the file, consider using a more semantic HTML structure:
@if (coursesLoaded) {
- <div class="d-flex justify-content-center mt-3">
- <div jhiTranslate="artemisApp.studentDashboard.archive.oldCourses"></div>
- <div> </div>
- <a [routerLink]="['/courses/archive']" jhiTranslate="artemisApp.studentDashboard.archive.here"></a>
+ <div class="text-center mt-3">
+ <span jhiTranslate="artemisApp.studentDashboard.archive.oldCourses"></span>
+ <a [routerLink]="['/courses/archive']" class="ms-1" jhiTranslate="artemisApp.studentDashboard.archive.here"></a>
</div>
}This change:
- Uses
text-centerinstead ofd-flex justify-content-centerfor centering. - Replaces the non-semantic
<div>elements with a<span>and removes the empty space<div>. - Adds a small margin (
ms-1) to the link for proper spacing.
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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @if (coursesLoaded) { | |
| <div class="d-flex justify-content-center mt-3"> | |
| <div jhiTranslate="artemisApp.studentDashboard.archive.oldCourses"></div> | |
| <div> </div> | |
| <a [routerLink]="['/courses/archive']" jhiTranslate="artemisApp.studentDashboard.archive.here"></a> | |
| </div> | |
| } | |
| @if (coursesLoaded) { | |
| <div class="text-center mt-3"> | |
| <span jhiTranslate="artemisApp.studentDashboard.archive.oldCourses"></span> | |
| <a [routerLink]="['/courses/archive']" class="ms-1" jhiTranslate="artemisApp.studentDashboard.archive.here"></a> | |
| </div> | |
| } |
| "archive": { | ||
| "oldCourses": "Suchst du nach alten Kursen? Klicke", | ||
| "here": "hier" | ||
| }, |
There was a problem hiding this comment.
Combine "oldCourses" and "here" entries for a complete sentence.
The current implementation splits the sentence into two parts, which may cause issues in some UI contexts. Consider combining them into a single, complete sentence.
Apply this change to create a complete sentence in a single entry:
"archive": {
- "oldCourses": "Suchst du nach alten Kursen? Klicke",
- "here": "hier"
+ "oldCourses": "Suchst du nach alten Kursen? Klicke hier"
},This change ensures that the translation is complete and can be used flexibly in different UI contexts.
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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "archive": { | |
| "oldCourses": "Suchst du nach alten Kursen? Klicke", | |
| "here": "hier" | |
| }, | |
| "archive": { | |
| "oldCourses": "Suchst du nach alten Kursen? Klicke hier" | |
| }, |
| @Query(""" | ||
| SELECT DISTINCT c | ||
| FROM Course c | ||
| JOIN UserGroup ug ON c.studentGroupName = ug.group | ||
| OR c.teachingAssistantGroupName = ug.group | ||
| OR c.editorGroupName = ug.group | ||
| OR c.instructorGroupName = ug.group | ||
| WHERE (ug.userId = :userId OR :isAdmin = TRUE) | ||
| AND c.semester IS NOT NULL | ||
| """) | ||
| List<Course> findCoursesForUserRolesWithNonNullSemester(@Param("userId") Long userId, @Param("isAdmin") boolean isAdmin); | ||
|
|
There was a problem hiding this comment.
LGTM! Consider adding a brief Javadoc comment.
The new method findCoursesForUserRolesWithNonNullSemester is well-implemented and follows good practices:
- Clear method name and appropriate return type
- Proper use of @query and @param annotations
- Efficient JOIN operation to check user roles
- Correct filtering for non-null semesters
Consider adding a brief Javadoc comment to explain the method's purpose, parameters, and return value. This would enhance code documentation and make it easier for other developers to understand and use the method. For example:
/**
* Finds courses for a user based on their roles and admin status, excluding courses without a semester.
*
* @param userId The ID of the user
* @param isAdmin Whether the user is an admin
* @return A list of courses the user has access to with non-null semesters
*/
@Query(...)
List<Course> findCoursesForUserRolesWithNonNullSemester(@Param("userId") Long userId, @Param("isAdmin") boolean isAdmin);| /** | ||
| * Fetches all courses, filters out courses that the user does not have | ||
| * access to based on their role, and excludes any courses that do not | ||
| * belong to a specific semester. | ||
| * | ||
| * @return A list of courses for the course archive | ||
| */ | ||
| public List<Course> getAllCoursesForCourseArchive() { | ||
| var user = userRepository.getUserWithGroupsAndAuthorities(); | ||
| boolean isAdmin = authCheckService.isAdmin(user); | ||
| return courseRepository.findCoursesForUserRolesWithNonNullSemester(user.getId(), isAdmin); | ||
| } | ||
|
|
There was a problem hiding this comment.
LGTM! Consider adding a brief Javadoc comment.
The implementation of getAllCoursesForCourseArchive() looks good and aligns well with the PR objectives. It correctly filters courses based on user roles and ensures only courses with non-null semesters are included.
Consider adding a brief Javadoc comment to explain the purpose of this method and its return value. This would improve code documentation and make it easier for other developers to understand the method's functionality.
+ /**
+ * Retrieves all courses for the course archive feature.
+ * Filters courses based on user roles and includes only courses with non-null semesters.
+ *
+ * @return A list of courses suitable for the course archive
+ */
public List<Course> getAllCoursesForCourseArchive() {
var user = userRepository.getUserWithGroupsAndAuthorities();
boolean isAdmin = authCheckService.isAdmin(user);
return courseRepository.findCoursesForUserRolesWithNonNullSemester(user.getId(), isAdmin);
}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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Fetches all courses, filters out courses that the user does not have | |
| * access to based on their role, and excludes any courses that do not | |
| * belong to a specific semester. | |
| * | |
| * @return A list of courses for the course archive | |
| */ | |
| public List<Course> getAllCoursesForCourseArchive() { | |
| var user = userRepository.getUserWithGroupsAndAuthorities(); | |
| boolean isAdmin = authCheckService.isAdmin(user); | |
| return courseRepository.findCoursesForUserRolesWithNonNullSemester(user.getId(), isAdmin); | |
| } | |
| /** | |
| * Retrieves all courses for the course archive feature. | |
| * Filters courses based on user roles and includes only courses with non-null semesters. | |
| * | |
| * @return A list of courses suitable for the course archive | |
| */ | |
| public List<Course> getAllCoursesForCourseArchive() { | |
| var user = userRepository.getUserWithGroupsAndAuthorities(); | |
| boolean isAdmin = authCheckService.isAdmin(user); | |
| return courseRepository.findCoursesForUserRolesWithNonNullSemester(user.getId(), isAdmin); | |
| } |
| private alertService: AlertService, | ||
| ) {} |
There was a problem hiding this comment.
Adjust constructor formatting to adhere to coding guidelines
Remove the trailing comma after the last parameter and place the opening brace { on the same line as the closing parenthesis ) in the constructor to comply with the open_braces_same_line coding guideline.
Apply this diff to fix the formatting:
constructor(
private courseService: CourseManagementService,
- private alertService: AlertService,
-) {}
+ private alertService: AlertService) {}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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private alertService: AlertService, | |
| ) {} | |
| private courseService: CourseManagementService, | |
| private alertService: AlertService) {} |
| for (const semester of this.semesters) { | ||
| this.semesterCollapsed[semester] = isCollapsed; | ||
| this.coursesBySemester[semester] = this.courses.filter((course) => course.semester === semester); | ||
| this.fullFormOfSemesterStrings[semester] = semester.startsWith('WS') ? 'artemisApp.course.archive.winterSemester' : 'artemisApp.course.archive.summerSemester'; | ||
| isCollapsed = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Clarify the logic for collapsing semesters
The current logic in mapCoursesIntoSemesters() sets isCollapsed to false for the first semester and true for subsequent semesters. Consider adding a comment to explain this behavior for better code readability.
Add a comment to clarify the logic:
let isCollapsed = false;
+ // The first semester is expanded by default; subsequent semesters are collapsed
for (const semester of this.semesters) {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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const semester of this.semesters) { | |
| this.semesterCollapsed[semester] = isCollapsed; | |
| this.coursesBySemester[semester] = this.courses.filter((course) => course.semester === semester); | |
| this.fullFormOfSemesterStrings[semester] = semester.startsWith('WS') ? 'artemisApp.course.archive.winterSemester' : 'artemisApp.course.archive.summerSemester'; | |
| isCollapsed = true; | |
| } | |
| } | |
| let isCollapsed = false; | |
| // The first semester is expanded by default; subsequent semesters are collapsed | |
| for (const semester of this.semesters) { | |
| this.semesterCollapsed[semester] = isCollapsed; | |
| this.coursesBySemester[semester] = this.courses.filter((course) => course.semester === semester); | |
| this.fullFormOfSemesterStrings[semester] = semester.startsWith('WS') ? 'artemisApp.course.archive.winterSemester' : 'artemisApp.course.archive.summerSemester'; | |
| isCollapsed = true; | |
| } | |
| } |
| constructor( | ||
| private courseService: CourseManagementService, | ||
| private alertService: AlertService, | ||
| ) {} |
There was a problem hiding this comment.
Remove unused import and align imports with Angular style guide
Ensure that all imported modules and components are used within the component. Also, follow the Angular style guide for organizing imports.
Review and remove any unused imports, and organize them according to the Angular style guide:
import { Component, OnDestroy, OnInit } from '@angular/core';
import { Course } from 'app/entities/course.model';
import { CourseManagementService } from '../../course/manage/course-management.service';
import { HttpErrorResponse, HttpResponse } from '@angular/common/http';
import { AlertService } from 'app/core/util/alert.service';
import { onError } from 'app/shared/util/global.utils';
import { Subscription } from 'rxjs';
import { faAngleDown, faAngleUp, faArrowDownAZ, faArrowUpAZ, faQuestionCircle } from '@fortawesome/free-solid-svg-icons';
import { sortCourses } from 'app/shared/util/course.util';
import { SizeProp } from '@fortawesome/fontawesome-svg-core';
-import { ArtemisSharedModule } from 'app/shared/shared.module';
-import { ArtemisSharedComponentModule } from 'app/shared/components/shared-component.module';
-import { CourseCardHeaderComponent } from '../course-card-header/course-card-header.component';If ArtemisSharedModule, ArtemisSharedComponentModule, and CourseCardHeaderComponent are not used in the component's class or template, consider removing them from both the import statements and the imports array in the @Component decorator.
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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor( | |
| private courseService: CourseManagementService, | |
| private alertService: AlertService, | |
| ) {} | |
| import { Component, OnDestroy, OnInit } from '@angular/core'; | |
| import { Course } from 'app/entities/course.model'; | |
| import { CourseManagementService } from '../../course/manage/course-management.service'; | |
| import { HttpErrorResponse, HttpResponse } from '@angular/common/http'; | |
| import { AlertService } from 'app/core/util/alert.service'; | |
| import { onError } from 'app/shared/util/global.utils'; | |
| import { Subscription } from 'rxjs'; | |
| import { faAngleDown, faAngleUp, faArrowDownAZ, faArrowUpAZ, faQuestionCircle } from '@fortawesome/free-solid-svg-icons'; | |
| import { sortCourses } from 'app/shared/util/course.util'; | |
| import { SizeProp } from '@fortawesome/fontawesome-svg-core'; | |
| constructor( | |
| private courseService: CourseManagementService, | |
| private alertService: AlertService, | |
| ) {} |
| this.archiveCourseSubscription.unsubscribe(); | ||
| this.courseService.disableCourseOverviewBackground(); | ||
| } |
There was a problem hiding this comment.
Add null check before unsubscribing to prevent potential errors
In the ngOnDestroy() method, it's good practice to check if archiveCourseSubscription exists before calling unsubscribe() to prevent potential runtime errors if the subscription was not established.
Apply this diff to add a null check:
ngOnDestroy(): void {
- this.archiveCourseSubscription.unsubscribe();
+ if (this.archiveCourseSubscription) {
+ this.archiveCourseSubscription.unsubscribe();
+ }
this.courseService.disableCourseOverviewBackground();
}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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.archiveCourseSubscription.unsubscribe(); | |
| this.courseService.disableCourseOverviewBackground(); | |
| } | |
| ngOnDestroy(): void { | |
| if (this.archiveCourseSubscription) { | |
| this.archiveCourseSubscription.unsubscribe(); | |
| } | |
| this.courseService.disableCourseOverviewBackground(); | |
| } |
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
Students often want access to old or completed courses on Artemis from previous semesters in which they were enrolled. While they can technically access these courses if they remember the course link, relying on memory and attempting to access the course via the link is not practical. This PR closes #9129.
Description
Steps for Testing
Prerequisites:
(I already created some old courses on TS3, and test users 1-5 are enrolled in these courses. Other test users are not enrolled. You can test that only users 1-5 can see these additional tests courses, where other test users can not. )
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Screenshots
Summary by CodeRabbit
Release Notes
New Features
User Interface Improvements
Localization
Bug Fixes
Tests