Development: Update LTI components to use Angular 18 practices
#9908
Conversation
github-actions
bot
added
the
client
raffifasaro
changed the title
Development: Update LTI components to use Angular 18 practices
WalkthroughThe changes in this pull request involve updating multiple components within the LTI module of the Angular application to be standalone components. This is achieved by adding the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (14)
src/main/webapp/app/lti/lti13-select-course.component.ts (1)
Line range hint
24-39: Implement proper subscription management and consistent async patterns.Several improvements can enhance this component:
- Implement
OnDestroyto clean up subscriptions- Use consistent async patterns
- Add proper error typing
Consider applying these changes:
-export class LtiCoursesComponent implements OnInit { +export class LtiCoursesComponent implements OnInit, OnDestroy { public courses: OnlineCourseDtoModel[]; + private destroy$ = new Subject<void>(); constructor( private courseService: CourseManagementService, private sessionStorageService: SessionStorageService, private alertService: AlertService, ) {} - async ngOnInit() { + ngOnInit() { this.loadAndFilterCourses(); } + ngOnDestroy() { + this.destroy$.next(); + this.destroy$.complete(); + } loadAndFilterCourses() { const clientId = this.sessionStorageService.retrieve('clientRegistrationId'); - this.courseService.findAllOnlineCoursesWithRegistrationId(clientId).subscribe({ + this.courseService.findAllOnlineCoursesWithRegistrationId(clientId) + .pipe( + takeUntil(this.destroy$), + catchError((error: HttpErrorResponse) => { + this.alertService.error('error.unexpectedError', { + error: error.message, + }); + return EMPTY; + }) + ).subscribe({ next: (courseResponse: OnlineCourseDtoModel[]) => { this.courses = courseResponse; - }, - error: (error) => { - this.alertService.error('error.unexpectedError', { - error: error.message, - }); }, }); }This refactor:
- Prevents memory leaks by properly managing subscriptions
- Provides better error typing with
HttpErrorResponse- Uses RxJS operators for error handling
- Removes unnecessary async/await since we're using observables
src/main/webapp/app/lti/lti13-dynamic-registration.component.ts (3)
4-4: Use single quotes for string literalsAccording to the coding guidelines, string literals should use single quotes.
-import { TranslateDirective } from "../shared/language/translate.directive"; +import { TranslateDirective } from '../shared/language/translate.directive';
Line range hint
26-29: Subscribe to route params should be properly managedThe subscription to route params should be properly managed to prevent memory leaks.
+ private destroy$ = new Subject<void>(); ngOnInit(): void { - this.route.params.subscribe((params) => { + this.route.params.pipe( + takeUntil(this.destroy$) + ).subscribe((params) => { this.courseId = Number(params['courseId']); });Also add:
ngOnDestroy(): void { this.destroy$.next(); this.destroy$.complete(); }
Line range hint
44-54: Enhance error handling and user feedbackThe error handling could be improved by:
- Providing specific error details for debugging
- Adding proper loading state management for UI feedback
- Adding type safety for window messaging
.subscribe({ next: () => { this.registeredSuccessfully = true; }, - error: () => { + error: (error: HttpErrorResponse) => { this.registeredSuccessfully = false; + console.error('LTI registration failed:', error); }, }) .add(() => { this.isRegistering = false; - (window.opener || window.parent).postMessage({ subject: 'org.imsglobal.lti.close' }, '*'); + const targetWindow = (window.opener || window.parent) as Window; + targetWindow.postMessage({ subject: 'org.imsglobal.lti.close' }, '*'); });src/main/webapp/app/lti/lti13-select-content.component.ts (1)
1-6: Use single quotes for string literalsAccording to the coding guidelines, string literals should use single quotes.
Apply this diff to fix the quotes:
-import { Component, ElementRef, NgZone, OnInit, SecurityContext, ViewChild, inject } from "@angular/core"; -import { ActivatedRoute } from "@angular/router"; -import { DomSanitizer } from "@angular/platform-browser"; -import { TranslateDirective } from "../shared/language/translate.directive"; -import { FormsModule } from "@angular/forms"; -import { ArtemisSharedPipesModule } from "../shared/pipes/shared-pipes.module"; +import { Component, ElementRef, NgZone, OnInit, SecurityContext, ViewChild, inject } from '@angular/core'; +import { ActivatedRoute } from '@angular/router'; +import { DomSanitizer } from '@angular/platform-browser'; +import { TranslateDirective } from '../shared/language/translate.directive'; +import { FormsModule } from '@angular/forms'; +import { ArtemisSharedPipesModule } from '../shared/pipes/shared-pipes.module';src/main/webapp/app/lti/lti13-exercise-launch.component.ts (5)
9-9: Consider using absolute import pathReplace the relative import path with an absolute path for better maintainability.
-import { TranslateDirective } from '../shared/language/translate.directive'; +import { TranslateDirective } from 'app/shared/language/translate.directive';
Line range hint
41-67: Add CSRF protection and input validationThe
sendRequestmethod handles sensitive authentication data but lacks some security measures:
- No CSRF protection in the POST request
- No validation of state and idToken parameters before use
Consider applying these security enhancements:
sendRequest(): void { const state = this.route.snapshot.queryParamMap.get('state'); const idToken = this.route.snapshot.queryParamMap.get('id_token'); - if (!state || !idToken) { + if (!state?.match(/^[a-zA-Z0-9-_]+$/) || !idToken?.match(/^[a-zA-Z0-9-_]+\.([a-zA-Z0-9-_]+\.)?[a-zA-Z0-9-_]+$/)) { console.error('Required parameter for LTI launch missing'); this.isLaunching = false; return; } const requestBody = new HttpParams().set('state', state).set('id_token', idToken); this.http .post<LtiLaunchResponse>('api/public/lti13/auth-login', requestBody.toString(), { - headers: new HttpHeaders().set('Content-Type', 'application/x-www-form-urlencoded'), + headers: new HttpHeaders() + .set('Content-Type', 'application/x-www-form-urlencoded') + .set('X-XSRF-TOKEN', this.getCsrfToken()), })
Line range hint
127-146: Enhance security of session storage handlingThe
storeLtiSessionDatamethod stores sensitive tokens in session storage without encryption:
- Consider using more secure storage methods
- Implement token expiration
- Add sanitization before storage
Consider implementing a secure storage service:
private secureStore(key: string, value: string): void { // Encrypt before storage const encryptedValue = this.encryptionService.encrypt(value); // Set expiration const data = { value: encryptedValue, expiry: Date.now() + (60 * 60 * 1000) // 1 hour }; this.sessionStorageService.store(key, JSON.stringify(data)); }
Line range hint
148-156: Prevent potential memory leaksThe component subscribes to authentication state but doesn't unsubscribe, which could cause memory leaks.
Implement proper subscription management:
+private destroy$ = new Subject<void>(); replaceWindowLocationWrapper(url: string): void { this.ltiService.setShownViaLti(true); this.themeService.applyThemePreference(Theme.LIGHT); const path = new URL(url).pathname; this.router.navigate([path], { replaceUrl: true }); } +ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); +}
Line range hint
51-67: Replace console.error with proper error loggingError logging to console exposes sensitive information in production.
Use a proper logging service:
-console.error('Required parameter for LTI launch missing'); +this.loggingService.logError('LTI launch failed: missing parameters', { + sensitive: false, + context: 'LTI Launch' +});src/main/webapp/app/lti/lti13-deep-linking.component.ts (4)
13-18: LGTM! Consider organizing imports.The standalone component migration looks good. All necessary modules are imported and properly configured in the component decorator.
Consider organizing imports into these groups, separated by newlines:
- Angular core/common
- Application-specific imports
- Third-party libraries
import { Component, OnInit } from '@angular/core'; import { ActivatedRoute, Router } from '@angular/router'; import { HttpClient, HttpParams } from '@angular/common/http'; import { FormsModule } from '@angular/forms'; import { CourseManagementService } from 'app/course/manage/course-management.service'; import { Exercise } from 'app/entities/exercise.model'; import { SortService } from 'app/shared/service/sort.service'; import { AccountService } from 'app/core/auth/account.service'; import { Course } from 'app/entities/course.model'; import { AlertService } from 'app/core/util/alert.service'; import { ArtemisSharedModule } from 'app/shared/shared.module'; import { TranslateDirective } from '../shared/language/translate.directive'; import { ArtemisSharedComponentModule } from '../shared/components/shared-component.module'; import { ArtemisSharedCommonModule } from '../shared/shared-common.module'; import { faExclamationTriangle, faSort, faWrench } from '@fortawesome/free-solid-svg-icons'; import { FaIconComponent } from '@fortawesome/angular-fontawesome'; import { SessionStorageService } from 'ngx-webstorage';Also applies to: 23-24
Line range hint
26-51: Prevent memory leaks by implementing proper subscription cleanup.The component has multiple subscriptions that need to be cleaned up to prevent memory leaks.
Implement the following changes:
-export class Lti13DeepLinkingComponent implements OnInit { +export class Lti13DeepLinkingComponent implements OnInit, OnDestroy { + private destroy$ = new Subject<void>(); + courseId: number; exercises: Exercise[]; selectedExercises?: Set<number> = new Set(); course: Course; // ... rest of the properties ... ngOnInit() { - this.route.params.subscribe((params) => { + this.route.params.pipe( + takeUntil(this.destroy$) + ).subscribe((params) => { // ... existing code ... }); } + ngOnDestroy() { + this.destroy$.next(); + this.destroy$.complete(); + }Also update the
redirectUserToLoginThenTargetLinkmethod:redirectUserToLoginThenTargetLink(currentLink: string): void { this.router.navigate(['/']).then(() => { - this.accountService.getAuthenticationState().subscribe((user) => { + this.accountService.getAuthenticationState().pipe( + takeUntil(this.destroy$) + ).subscribe((user) => { // ... existing code ... }); }); }
26-26: Add class-level documentation.Missing JSDoc documentation for the component class.
Add comprehensive documentation:
+/** + * Component for handling LTI 1.3 deep linking functionality. + * Allows users to select exercises from a course and create deep links for LTI integration. + * Manages the selection, validation, and linking process while handling authentication + * and error states appropriately. + */ export class Lti13DeepLinkingComponent implements OnInit {
Line range hint
147-167: Enhance error handling with specific error messages.The error handling in
sendDeepLinkRequestcould be more specific to help with debugging.Consider enhancing the error handling:
sendDeepLinkRequest() { if (this.selectedExercises?.size) { // ... existing code ... this.http.post<DeepLinkingResponse>(`api/lti13/deep-linking/${this.courseId}`, null, { observe: 'response', params: httpParams }).subscribe({ next: (response) => { if (response.status === 200) { if (response.body) { const targetLink = response.body.targetLinkUri; window.location.replace(targetLink); } } else { this.isLinking = false; - this.alertService.error('artemisApp.lti13.deepLinking.unknownError'); + this.alertService.error('artemisApp.lti13.deepLinking.responseError', { status: response.status }); } }, error: (error) => { this.isLinking = false; - onError(this.alertService, error); + const errorKey = error.status === 403 ? 'artemisApp.lti13.deepLinking.unauthorized' : + error.status === 404 ? 'artemisApp.lti13.deepLinking.notFound' : + 'artemisApp.lti13.deepLinking.unknownError'; + this.alertService.error(errorKey, { status: error.status }); }, }); } else { this.alertService.error('artemisApp.lti13.deepLinking.selectToLink'); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
src/main/webapp/app/lti/lti-course-card.component.ts(1 hunks)src/main/webapp/app/lti/lti.module.ts(1 hunks)src/main/webapp/app/lti/lti13-deep-linking.component.ts(1 hunks)src/main/webapp/app/lti/lti13-dynamic-registration.component.ts(1 hunks)src/main/webapp/app/lti/lti13-exercise-launch.component.ts(2 hunks)src/main/webapp/app/lti/lti13-select-content.component.ts(1 hunks)src/main/webapp/app/lti/lti13-select-course.component.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/app/lti/lti.module.ts
🧰 Additional context used
📓 Path-based instructions (6)
src/main/webapp/app/lti/lti-course-card.component.ts (1)
src/main/webapp/app/lti/lti13-deep-linking.component.ts (1)
src/main/webapp/app/lti/lti13-dynamic-registration.component.ts (1)
src/main/webapp/app/lti/lti13-exercise-launch.component.ts (1)
src/main/webapp/app/lti/lti13-select-content.component.ts (1)
src/main/webapp/app/lti/lti13-select-course.component.ts (1)
🔇 Additional comments (9)
src/main/webapp/app/lti/lti-course-card.component.ts (1)
5-7: LGTM! Imports follow Angular guidelines.
The new imports are correctly organized and follow the Angular style guide recommendations.
src/main/webapp/app/lti/lti13-select-course.component.ts (3)
6-7: LGTM! Import statements are properly structured.
The new imports for standalone components follow Angular style guidelines and match the component's declared dependencies.
12-13: LGTM! Component decorator properly configured for standalone mode.
The standalone configuration and imports are correctly implemented.
Line range hint 1-39: Verify string literal consistency across the component.
Let's ensure all string literals follow the single quotes convention as per coding guidelines.
✅ Verification successful
String literals in the component follow the single quotes convention
The TypeScript component correctly uses single quotes for all string literals:
- Import paths: '@angular/core', 'ngx-webstorage', etc.
- Component selector: 'jhi-lti-courses-overview'
- Template URL: './lti13-select-course.component.html'
- Storage key: 'clientRegistrationId'
- Error message: 'error.unexpectedError'
The double quotes in the HTML template file are appropriate as they follow HTML conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for double-quoted strings in the component and template
rg -l "\"[^\"]+\"" "src/main/webapp/app/lti/lti13-select-course.component.*"
Length of output: 171
Script:
#!/bin/bash
# First, let's find the exact file paths
fd "lti13-select-course.component" src/main/webapp
# Then check for double-quoted strings in those files
rg -l "\"[^\"]+\"" $(fd "lti13-select-course.component" src/main/webapp)
Length of output: 411
Script:
#!/bin/bash
# Let's see the actual content to verify string usage in both files
echo "=== TypeScript Component ==="
rg -A 1 "'" src/main/webapp/app/lti/lti13-select-course.component.ts
echo -e "\n=== Double quotes in TypeScript Component ==="
rg -A 1 "\"" src/main/webapp/app/lti/lti13-select-course.component.ts
echo -e "\n=== Template File ==="
cat src/main/webapp/app/lti/lti13-select-course.component.html
Length of output: 1946
src/main/webapp/app/lti/lti13-dynamic-registration.component.ts (2)
9-10: LGTM! Good standalone component setup
The component is correctly configured as standalone with the necessary TranslateDirective import, which aligns with modern Angular practices and the localization requirements.
Line range hint 12-54: Add unit tests for component logic
The PR objectives emphasize the importance of integration tests. Please ensure proper test coverage for:
- Successful registration flow
- Error handling scenarios
- Window message posting
- Route parameter handling
src/main/webapp/app/lti/lti13-select-content.component.ts (3)
18-19: LGTM! Good job on making the component standalone
The component is correctly configured as standalone with the necessary imports. This change improves modularity and follows modern Angular practices.
22-24: LGTM! Good use of modern DI pattern
The dependency injection using the inject() function is well implemented. The approach:
- Reduces boilerplate compared to constructor injection
- Maintains proper typing
- Follows the no-prefix guideline for private members
Line range hint 56-58: Verify complete XSS protection for deep linking
While the code correctly uses sanitize() for the URL, let's verify that all potential XSS vectors are covered.
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information about the security measures in place:
Deep linking implementation has proper XSS protection
The code implements comprehensive XSS protection for deep linking through:
- Proper URL sanitization using both
DomSanitizerandSecurityContext.URL - No direct DOM manipulation through innerHTML/outerHTML that could bypass sanitization
- Strict parameter validation before form submission
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other deep linking related security measures
rg -l "deepLink" | xargs rg "sanitize|SecurityContext|DomSanitizer" -A 3
# Check for any direct DOM manipulation that might bypass sanitization
ast-grep --pattern 'innerHTML = $_'
ast-grep --pattern 'outerHTML = $_'
Length of output: 3569
…tion-lti-selection-components
…nal-migration-lti-selection-components' into chore/performance/angular-18-signal-migration-lti-selection-components
…tion-lti-selection-components
…tion-lti-selection-components
…tion-lti-selection-components
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (19)
src/main/webapp/app/lti/lti-course-card.component.ts (1)
18-18: Consider using computed() for courseColorThe current implementation uses effect() to update courseColor, but since it's a derived state from course input, consider using computed() instead:
-courseColor: string; +courseColor = computed(() => { + const courseValue = this.course(); + return courseValue?.color || this.ARTEMIS_DEFAULT_COLOR; +}); -constructor() { - effect(() => { - const courseValue = this.course(); - this.courseColor = courseValue?.color || this.ARTEMIS_DEFAULT_COLOR; - }); -}Also applies to: 22-26
src/main/webapp/app/overview/exercise-details/lti-initializer-modal.component.ts (1)
1-1: Consider using signals for form stateSince we're adopting Angular 18 practices, consider using signals for the form state:
-import { Component, inject } from '@angular/core'; +import { Component, inject, signal } from '@angular/core'; export class LtiInitializerModalComponent { private activeModal = inject(NgbActiveModal); private alertService = inject(AlertService); private router = inject(Router); private activatedRoute = inject(ActivatedRoute); - password: string; - loginName: string; - readAndUnderstood = false; + password = signal(''); + loginName = signal(''); + readAndUnderstood = signal(false);Also applies to: 11-14
src/main/webapp/app/lti/lti-course-card.component.html (2)
5-5: Avoid duplicate routerLinkThe routerLink is duplicated in both the div and anchor elements. Consider consolidating:
<div id="course-{{ course()?.id }}-header" class="card-header text-white" - [routerLink]="['/lti/exercises', course()?.id]" [ngStyle]="{ '--background-color-for-hover': courseColor }" > <a class="stretched-link" [routerLink]="['/lti/exercises', course()?.id]"></a>Also applies to: 8-8
18-18: Consider using computed signal for titleTo improve performance by reducing template expressions:
// In component.ts +fullTitle = computed(() => { + const c = this.course(); + return c ? `${c.title} (${c.shortName})` : ''; +}); // In template -{{ course()?.title + ' (' + course()?.shortName + ')' }} +{{ fullTitle() }}src/main/webapp/app/lti/lti13-select-course.component.ts (2)
Line range hint
23-25: Add error handling for async initializationThe async ngOnInit should include try-catch error handling to prevent unhandled promise rejections.
- async ngOnInit() { - this.loadAndFilterCourses(); + async ngOnInit() { + try { + await this.loadAndFilterCourses(); + } catch (error) { + this.alertService.error('error.unexpectedError', { error: error.message }); + }
21-21: Initialize courses array with proper typingThe courses property should be initialized to prevent potential undefined errors.
- public courses: OnlineCourseDtoModel[]; + public courses: OnlineCourseDtoModel[] = [];src/main/webapp/app/overview/exercise-details/lti-initializer.component.ts (1)
Line range hint
24-43: Address potential memory leaks and security concernsSeveral issues need attention:
- Subscription cleanup missing
- Password exposed in component instance
- Nested subscriptions can lead to memory leaks
+ private destroy$ = new Subject<void>(); ngOnInit() { - this.activatedRoute.queryParams.subscribe((queryParams) => { + this.activatedRoute.queryParams.pipe( + takeUntil(this.destroy$), + filter(params => params['initialize'] !== undefined) + ).subscribe((queryParams) => { - if (queryParams['initialize'] !== undefined) { this.userService.initializeLTIUser().subscribe((res) => { const password = res.body?.password; if (!password) { this.alertService.info('artemisApp.lti.initializationError'); this.router.navigate([], { relativeTo: this.activatedRoute, queryParams: { initialize: null }, queryParamsHandling: 'merge', }); return; } this.modalRef = this.modalService.open(LtiInitializerModalComponent, { size: 'lg', backdrop: 'static', keyboard: false }); this.modalRef.componentInstance.loginName = this.accountService.userIdentity?.login; - this.modalRef.componentInstance.password = password; + // Set password and immediately clear it from memory + this.modalRef.componentInstance.password = password; + setTimeout(() => password = undefined, 0); }); - } }); } + ngOnDestroy() { + this.destroy$.next(); + this.destroy$.complete(); + if (this.modalRef) { + this.modalRef.close(); + } + }src/main/webapp/app/admin/lti-configuration/lti-configuration.service.ts (2)
Line range hint
25-27: Improve type safety and error handlingThe HTTP methods should use more specific types than 'any' for better type safety.
- addLtiPlatformConfiguration(ltiPlatformConfiguration: LtiPlatformConfiguration): Observable<HttpResponse<any>> { + addLtiPlatformConfiguration(ltiPlatformConfiguration: LtiPlatformConfiguration): Observable<HttpResponse<LtiPlatformConfiguration>> { - updateLtiPlatformConfiguration(ltiPlatformConfiguration: LtiPlatformConfiguration): Observable<HttpResponse<any>> { + updateLtiPlatformConfiguration(ltiPlatformConfiguration: LtiPlatformConfiguration): Observable<HttpResponse<LtiPlatformConfiguration>> {Also applies to: 34-36
Line range hint
15-20: Consider using environment configuration for API endpointsAPI endpoints should be configurable through environment files for better maintainability.
+ private readonly API_BASE = environment.apiUrl; query(req?: any): Observable<HttpResponse<LtiPlatformConfiguration[]>> { const params: HttpParams = createRequestOption(req); - return this.http.get<LtiPlatformConfiguration[]>('api/lti-platforms', { + return this.http.get<LtiPlatformConfiguration[]>(`${this.API_BASE}/lti-platforms`, {Also applies to: 25-27, 34-36, 42-44, 46-48
src/main/webapp/app/course/manage/course-lti-configuration/course-lti-configuration.component.ts (1)
15-17: Add type annotations to inject callsFor better type safety and code clarity, explicitly specify the return type of inject calls.
- private route = inject(ActivatedRoute); - private sortService = inject(SortService); - private courseManagementService = inject(CourseManagementService); + private route: ActivatedRoute = inject(ActivatedRoute); + private sortService: SortService = inject(SortService); + private courseManagementService: CourseManagementService = inject(CourseManagementService);src/main/webapp/app/admin/lti-configuration/edit-lti-configuration.component.ts (2)
Line range hint
33-43: Fix duplicate form initialization in ngOnInitThe
initializeForm()is called twice: once unconditionally at the end ofngOnInitand once within the subscription callback. This could lead to unnecessary form resets.ngOnInit() { const platformId = this.route.snapshot.paramMap.get('platformId'); if (platformId) { this.ltiConfigurationService.getLtiPlatformById(Number(platformId)).subscribe({ next: (data) => { this.platform = data; this.initializeForm(); }, error: (error) => { this.alertService.error(error); }, }); + } else { + this.initializeForm(); } - this.initializeForm(); }
15-18: Add type annotations to inject callsFor better type safety and code clarity, explicitly specify the return type of inject calls.
- private route = inject(ActivatedRoute); - private ltiConfigurationService = inject(LtiConfigurationService); - private router = inject(Router); - private alertService = inject(AlertService); + private route: ActivatedRoute = inject(ActivatedRoute); + private ltiConfigurationService: LtiConfigurationService = inject(LtiConfigurationService); + private router: Router = inject(Router); + private alertService: AlertService = inject(AlertService);src/main/webapp/app/course/manage/course-lti-configuration/edit-course-lti-configuration.component.ts (3)
22-25: Add type annotations to inject callsFor better type safety and code clarity, explicitly specify the return type of inject calls.
- private route = inject(ActivatedRoute); - private courseService = inject(CourseManagementService); - private router = inject(Router); - private ltiConfigurationService = inject(LtiConfigurationService); + private route: ActivatedRoute = inject(ActivatedRoute); + private courseService: CourseManagementService = inject(CourseManagementService); + private router: Router = inject(Router); + private ltiConfigurationService: LtiConfigurationService = inject(LtiConfigurationService);
Line range hint
71-79: Add type safety to loadData parametersThe query parameters object would benefit from strong typing to prevent potential runtime errors.
+ interface QueryParams { + page: number; + size: number; + sort: string[]; + } + loadData(): void { this.ltiConfigurationService - .query({ + .query(<QueryParams>{ page: this.page - 1, size: this.itemsPerPage, sort: ['id', 'asc'], }) .subscribe((res: HttpResponse<LtiPlatformConfiguration[]>) => this.onSuccess(res.body, res.headers)); }
Line range hint
17-21: Convert to standalone component as per PR objectivesThe PR objectives mention migrating to standalone components, but this component is still using the traditional module-based approach.
@Component({ + standalone: true, + imports: [ + CommonModule, + RouterModule, + ReactiveFormsModule + ], selector: 'jhi-edit-course-lti-configuration', templateUrl: './edit-course-lti-configuration.component.html', })src/main/webapp/app/admin/lti-configuration/lti-configuration.component.ts (1)
Line range hint
144-154: Consider streamlining error handling.The method currently uses both
dialogErrorSourceandalertServicefor error handling. Consider consolidating to use just thealertServicefor a more consistent approach across the application.deleteLtiPlatform(platformId: number): void { this.ltiConfigurationService.deleteLtiPlatform(platformId).subscribe({ next: () => { - this.dialogErrorSource.next(''); this.router.navigate(['admin', 'lti-configuration']); }, error: (error: HttpErrorResponse) => { - this.dialogErrorSource.next(error.message); this.alertService.error('artemisApp.lti13.deletePlatformError'); }, }); }src/main/webapp/app/lti/lti13-exercise-launch.component.ts (1)
Line range hint
46-71: Enhance security measures for token handling.Consider implementing additional security measures:
- Sanitize and validate state and idToken parameters before use
- Avoid exposing login information in error responses
- Add rate limiting for the auth-login endpoint
sendRequest(): void { const state = this.route.snapshot.queryParamMap.get('state'); const idToken = this.route.snapshot.queryParamMap.get('id_token'); + // Validate token format and length + if (!this.isValidToken(state) || !this.isValidToken(idToken)) { + console.error('Invalid token format'); + this.isLaunching = false; + return; + } if (!state || !idToken) { console.error('Required parameter for LTI launch missing'); this.isLaunching = false; return; }src/main/webapp/app/lti/lti13-deep-linking.component.ts (2)
23-24: Consider optimizing module imports.The component imports several shared modules which might include unused features. Consider breaking down the imports to include only the necessary components and directives for better tree-shaking.
Line range hint
156-185: Enhance security and error handling in deep linking.
- Add token validation before making the request
- Use more specific error messages based on error types
- Consider implementing retry logic for network failures
sendDeepLinkRequest() { if (this.selectedExercises?.size) { const ltiIdToken = this.sessionStorageService.retrieve('ltiIdToken') ?? ''; const clientRegistrationId = this.sessionStorageService.retrieve('clientRegistrationId') ?? ''; + + if (!this.isValidToken(ltiIdToken) || !this.isValidToken(clientRegistrationId)) { + this.alertService.error('artemisApp.lti13.deepLinking.invalidToken'); + return; + } const exerciseIds = Array.from(this.selectedExercises).join(','); // ... rest of the method } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
src/main/webapp/app/admin/lti-configuration/edit-lti-configuration.component.ts(2 hunks)src/main/webapp/app/admin/lti-configuration/lti-configuration.component.ts(2 hunks)src/main/webapp/app/admin/lti-configuration/lti-configuration.service.ts(1 hunks)src/main/webapp/app/course/manage/course-lti-configuration/course-lti-configuration.component.ts(2 hunks)src/main/webapp/app/course/manage/course-lti-configuration/edit-course-lti-configuration.component.ts(2 hunks)src/main/webapp/app/lti/lti-course-card.component.html(1 hunks)src/main/webapp/app/lti/lti-course-card.component.ts(1 hunks)src/main/webapp/app/lti/lti13-deep-linking.component.ts(2 hunks)src/main/webapp/app/lti/lti13-dynamic-registration.component.ts(1 hunks)src/main/webapp/app/lti/lti13-exercise-launch.component.ts(2 hunks)src/main/webapp/app/lti/lti13-select-course.component.ts(1 hunks)src/main/webapp/app/overview/exercise-details/lti-initializer-modal.component.ts(2 hunks)src/main/webapp/app/overview/exercise-details/lti-initializer.component.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/lti/lti13-dynamic-registration.component.ts
🧰 Additional context used
📓 Path-based instructions (12)
src/main/webapp/app/admin/lti-configuration/edit-lti-configuration.component.ts (1)
src/main/webapp/app/admin/lti-configuration/lti-configuration.service.ts (1)
src/main/webapp/app/lti/lti-course-card.component.ts (1)
src/main/webapp/app/admin/lti-configuration/lti-configuration.component.ts (1)
src/main/webapp/app/course/manage/course-lti-configuration/course-lti-configuration.component.ts (1)
src/main/webapp/app/overview/exercise-details/lti-initializer.component.ts (1)
src/main/webapp/app/lti/lti-course-card.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/app/lti/lti13-select-course.component.ts (1)
src/main/webapp/app/course/manage/course-lti-configuration/edit-course-lti-configuration.component.ts (1)
src/main/webapp/app/overview/exercise-details/lti-initializer-modal.component.ts (1)
src/main/webapp/app/lti/lti13-exercise-launch.component.ts (1)
src/main/webapp/app/lti/lti13-deep-linking.component.ts (1)
🔇 Additional comments (8)
src/main/webapp/app/lti/lti-course-card.component.ts (2)
1-1: LGTM: Imports are correctly organized
The imports are properly organized with core Angular imports first, followed by application-specific imports. The standalone component dependencies (RouterLink, NgStyle, ArtemisSharedModule) are correctly imported.
Also applies to: 5-7
13-14: Verify test files update for standalone migration
While the standalone configuration is correct, the test files still need to be updated to match the new standalone setup.
#!/bin/bash
# Description: Check if test files have been updated for standalone components
rg -t ts "TestBed.configureTestingModule.*LtiCourseCardComponent" "src/test"src/main/webapp/app/lti/lti13-select-course.component.ts (2)
12-13: LGTM: Proper standalone component configuration
The component is correctly configured as standalone with the necessary imports.
16-18: LGTM: Modern dependency injection pattern
Correctly uses the new inject() pattern for service dependencies.
src/main/webapp/app/overview/exercise-details/lti-initializer.component.ts (1)
14-19: LGTM: Clean dependency injection implementation
Services are properly injected using the modern inject() pattern.
src/main/webapp/app/admin/lti-configuration/lti-configuration.service.ts (1)
9-9: LGTM: Proper dependency injection
The HttpClient is correctly injected using the modern inject() pattern.
src/main/webapp/app/admin/lti-configuration/lti-configuration.component.ts (1)
20-24: LGTM! Clean dependency injection implementation.
The migration to using the inject() function follows Angular 18's best practices for dependency injection.
src/main/webapp/app/lti/lti13-exercise-launch.component.ts (1)
21-22: LGTM! CommonModule properly imported.
The addition of CommonModule addresses the previous review comment about the [hidden] directive dependency.
| private activeModal = inject(NgbActiveModal); | ||
| private alertService = inject(AlertService); | ||
| private router = inject(Router); | ||
| private activatedRoute = inject(ActivatedRoute); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider converting to standalone component
While the migration to inject() is good, consider completing the Angular 18 migration by converting this to a standalone component:
@Component({
selector: 'jhi-lti-initializer-modal',
templateUrl: './lti-initializer-modal.component.html',
+ standalone: true,
+ imports: [NgbActiveModal, FormsModule]
})Committable suggestion skipped: line range outside the PR's diff.
| <ng-container *ngIf="course()?.courseIcon"> | ||
| <jhi-secured-image [cachingStrategy]="CachingStrategy.LOCAL_STORAGE" [src]="course()?.courseIcon || ''"></jhi-secured-image> | ||
| </ng-container> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
*Use @if instead of ngIf for better performance
As per the coding guidelines, @if should be used instead of *ngIf:
-<ng-container *ngIf="course()?.courseIcon">
+@if (course()?.courseIcon) {
<jhi-secured-image [cachingStrategy]="CachingStrategy.LOCAL_STORAGE" [src]="course()?.courseIcon || ''"></jhi-secured-image>
-</ng-container>
+}📝 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.
| <ng-container *ngIf="course()?.courseIcon"> | |
| <jhi-secured-image [cachingStrategy]="CachingStrategy.LOCAL_STORAGE" [src]="course()?.courseIcon || ''"></jhi-secured-image> | |
| </ng-container> | |
| @if (course()?.courseIcon) { | |
| <jhi-secured-image [cachingStrategy]="CachingStrategy.LOCAL_STORAGE" [src]="course()?.courseIcon || ''"></jhi-secured-image> | |
| } |
| @@ -1,4 +1,4 @@ | |||
| import { Component, OnInit } from '@angular/core'; | |||
| import { Component, OnInit, inject } from '@angular/core'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Convert to standalone component as per PR objectives
The PR objectives mention migrating to standalone components, but this component is still using the traditional module-based approach. Consider converting it to a standalone component.
-@Component({
+@Component({
+ standalone: true,
+ imports: [CommonModule, RouterModule],
selector: 'jhi-course-lti-configuration',
templateUrl: './course-lti-configuration.component.html',
})Also applies to: 10-14
Checklist
General
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
Migrating all LTI components to use Angular 18 practices
Description
Migrated LTI components in regard to standalone, input and signals
Adjusted tests to work with migrated components
Steps for Testing
Prerequisites:
(If Moodle throws an error in the iFrame, just refresh the page)
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
Performance Review
Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores