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

Development: Update LTI components to use Angular 18 practices #9908

Conversation

raffifasaro
Copy link
Contributor

@raffifasaro raffifasaro commented Nov 29, 2024

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 (e.g. using paging).
  • I strictly followed the principle of data economy for all client-server REST calls.
  • I strictly followed the client 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

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:

  • Access to Moodle
  • Testserver 1
  1. Make sure you are logged in to Artemis with a test user (artemis_test_user_{1-5})
  2. Navigate to Moodle and login with the same test user (artemis_test_user_{1-5}, same PW as for the Artemis Test Server)
  3. Go to My Courses and open the course TS1 - Artemis Feature Demo Course
  4. Click on one of the exercise
  5. Artemis should open in an iFrame and display the exercise
    (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

  • 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 even for very large courses with more than 2000 students.
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance even for very large courses with more than 2000 students.

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Screenshots

Summary by CodeRabbit

Release Notes

  • New Features

    • Several components have been updated to be standalone, enhancing modularity and reusability.
    • New imports added to components allow for improved functionality, including translation support.
  • Bug Fixes

    • No bug fixes were included in this release.
  • Documentation

    • No new documentation was provided in this release.
  • Chores

    • Improved the structure and readability of module imports and declarations across multiple components.
    • Simplified dependency injection across components, enhancing clarity and reducing boilerplate code.

@raffifasaro raffifasaro requested a review from a team as a code owner November 29, 2024 16:24
@github-actions github-actions bot added the client Pull requests that update TypeScript code. (Added Automatically!) label Nov 29, 2024
@raffifasaro raffifasaro marked this pull request as draft November 29, 2024 16:24
@raffifasaro raffifasaro changed the title DeLTI Client Migration Development: Update LTI components to use Angular 18 practices Nov 29, 2024
Copy link

coderabbitai bot commented Nov 29, 2024

Walkthrough

The 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 standalone: true property to the @Component decorator for each component. Additionally, various components now include updated imports arrays to incorporate new dependencies, enhancing their modularity and reusability. The overall logic and functionality of the components remain unchanged, focusing primarily on structural improvements.

Changes

File Path Change Summary
src/main/webapp/app/lti/lti-course-card.component.ts Updated @Component to include standalone: true and added imports: [RouterLink, NgStyle, ArtemisSharedModule]. Removed OnChanges lifecycle hook and changed @Input() to input() for course.
src/main/webapp/app/lti/lti.module.ts Reformatted imports and declarations arrays in @NgModule for improved readability; no functional changes.
src/main/webapp/app/lti/lti13-deep-linking.component.ts Updated @Component to include standalone: true and added imports: [ArtemisSharedModule, TranslateDirective, ArtemisSharedComponentModule, ArtemisSharedCommonModule, FaIconComponent, FormsModule]. Removed constructor in favor of property injection.
src/main/webapp/app/lti/lti13-dynamic-registration.component.ts Updated @Component to include standalone: true and added imports: [TranslateDirective]. Removed constructor in favor of property injection.
src/main/webapp/app/lti/lti13-exercise-launch.component.ts Updated @Component to include standalone: true and added imports: [TranslateDirective, CommonModule]. Removed constructor in favor of property injection.
src/main/webapp/app/lti/lti13-select-content.component.ts Updated @Component to include standalone: true and added imports: [TranslateDirective, FormsModule, ArtemisSharedPipesModule]. Removed constructor in favor of property injection.
src/main/webapp/app/lti/lti13-select-course.component.ts Updated @Component to include standalone: true and added imports: [LtiCourseCardComponent, TranslateDirective]. Removed constructor in favor of property injection.
src/main/webapp/app/admin/lti-configuration/edit-lti-configuration.component.ts Removed constructor and transitioned to property injection for services.
src/main/webapp/app/admin/lti-configuration/lti-configuration.component.ts Removed constructor and transitioned to property injection for services.
src/main/webapp/app/admin/lti-configuration/lti-configuration.service.ts Removed constructor and transitioned to property injection for HttpClient.
src/main/webapp/app/course/manage/course-lti-configuration/course-lti-configuration.component.ts Removed constructor and transitioned to property injection for services.
src/main/webapp/app/course/manage/course-lti-configuration/edit-course-lti-configuration.component.ts Removed constructor and transitioned to property injection for services.
src/main/webapp/app/lti/lti-course-card.component.html Updated template to access course object via function call instead of direct property access.
src/main/webapp/app/overview/exercise-details/lti-initializer-modal.component.ts Removed constructor and transitioned to property injection for services.
src/main/webapp/app/overview/exercise-details/lti-initializer.component.ts Removed constructor and transitioned to property injection for services.
src/main/webapp/app/course/manage/course-management.module.ts Modified ArtemisCourseManagementModule to add CourseLtiConfigurationComponent and EditCourseLtiConfigurationComponent to imports and removed them from declarations.

Possibly related PRs

Suggested labels

refactoring, small, ready to merge, documentation

Suggested reviewers

  • JohannesStoehr
  • florian-glombik
  • edkaya
  • pzdr7
  • HawKhiem

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

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

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.

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:

  1. Implement OnDestroy to clean up subscriptions
  2. Use consistent async patterns
  3. 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 literals

According 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 managed

The 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 feedback

The error handling could be improved by:

  1. Providing specific error details for debugging
  2. Adding proper loading state management for UI feedback
  3. 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 literals

According 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 path

Replace 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 validation

The sendRequest method handles sensitive authentication data but lacks some security measures:

  1. No CSRF protection in the POST request
  2. 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 handling

The storeLtiSessionData method stores sensitive tokens in session storage without encryption:

  1. Consider using more secure storage methods
  2. Implement token expiration
  3. 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 leaks

The 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 logging

Error 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:

  1. Angular core/common
  2. Application-specific imports
  3. 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 redirectUserToLoginThenTargetLink method:

     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 sendDeepLinkRequest could 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

📥 Commits

Reviewing files that changed from the base of the PR and between b365494 and 56dae8c.

📒 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)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/lti/lti13-deep-linking.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/lti/lti13-dynamic-registration.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/lti/lti13-exercise-launch.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/lti/lti13-select-content.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/lti/lti13-select-course.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

🔇 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:

  1. Successful registration flow
  2. Error handling scenarios
  3. Window message posting
  4. 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 DomSanitizer and SecurityContext.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

@github-actions github-actions bot added the tests label Dec 3, 2024
@raffifasaro raffifasaro marked this pull request as ready for review December 4, 2024 13:57
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.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)
src/main/webapp/app/lti/lti13-exercise-launch.component.ts (3)

Line range hint 63-86: Add unsubscribe handling for HTTP subscription

The HTTP subscription in sendRequest() should be cleaned up to prevent potential memory leaks.

Consider using the async pipe in the template or implementing proper cleanup:

+ private destroy$ = new Subject<void>();

  ngOnInit(): void {
    this.sendRequest();
  }

+ ngOnDestroy(): void {
+   this.destroy$.next();
+   this.destroy$.complete();
+ }

  sendRequest(): void {
    // ... existing code ...
    this.http
      .post<LtiLaunchResponse>(...)
+     .pipe(takeUntil(this.destroy$))
      .subscribe({
        // ... existing code ...
      });
  }

Line range hint 146-159: Enhance URL validation in replaceWindowLocationWrapper

The URL handling could be vulnerable to open redirect attacks.

Add URL validation before navigation:

  replaceWindowLocationWrapper(url: string): void {
    this.ltiService.setShownViaLti(true);
    this.themeService.applyThemePreference(Theme.LIGHT);
    let path;
+   // Whitelist of allowed paths
+   const allowedPaths = ['/lti/select-course'];
    if (url === '/lti/select-course') {
      path = url;
    } else {
+     try {
        const parsedUrl = new URL(url);
+       // Ensure URL is from the same origin or in allowedPaths
+       if (!allowedPaths.includes(parsedUrl.pathname) && parsedUrl.origin !== window.location.origin) {
+         console.error('Invalid URL detected');
+         return;
+       }
        path = parsedUrl.pathname;
+     } catch (e) {
+       console.error('Invalid URL format');
+       return;
+     }
    }
    this.router.navigate([path], { replaceUrl: true });
  }

Line range hint 122-137: Enhance error handling in storeLtiSessionData

The error handling in storeLtiSessionData could be more robust.

Consider adding specific error types and user feedback:

  storeLtiSessionData(ltiIdToken: string, clientRegistrationId: string): void {
    if (!ltiIdToken) {
-     captureException(new Error('LTI ID token required to store session data.'));
+     const error = new Error('LTI ID token required to store session data.');
+     captureException(error);
+     this.handleLtiLaunchError();
      return;
    }

    if (!clientRegistrationId) {
-     captureException(new Error('LTI client registration ID required to store session data.'));
+     const error = new Error('LTI client registration ID required to store session data.');
+     captureException(error);
+     this.handleLtiLaunchError();
      return;
    }

    try {
      this.sessionStorageService.store('ltiIdToken', ltiIdToken);
      this.sessionStorageService.store('clientRegistrationId', clientRegistrationId);
    } catch (error) {
-     console.error('Failed to store session data:', error);
+     captureException(error);
+     this.handleLtiLaunchError();
    }
  }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 3c3d5c2 and e803712.

📒 Files selected for processing (1)
  • src/main/webapp/app/lti/lti13-exercise-launch.component.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/lti/lti13-exercise-launch.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

🔇 Additional comments (2)
src/main/webapp/app/lti/lti13-exercise-launch.component.ts (2)

Line range hint 1-17: LGTM: Imports and type definitions are well-organized

The imports section includes all necessary modules and the type definition is properly structured.


18-32: Verify template usage of TranslateDirective

While the component structure and dependency injection look good, let's verify that the TranslateDirective is being used in the template.

Copy link
Contributor

@SimonEntholzer SimonEntholzer left a comment

Choose a reason for hiding this comment

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

Works on Ts1+Moodle with test user 1

Copy link

@HawKhiem HawKhiem left a comment

Choose a reason for hiding this comment

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

Reapprove

@MaximilianAnzinger MaximilianAnzinger added ready to merge maintainer-approved The feature maintainer has approved the PR and removed ready for review labels Dec 13, 2024
@krusche krusche added this to the 7.8.0 milestone Dec 14, 2024
@krusche krusche merged commit 7a6ba30 into develop Dec 14, 2024
54 of 58 checks passed
@krusche krusche deleted the chore/performance/angular-18-signal-migration-lti-selection-components branch December 14, 2024 11:37
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!) maintainer-approved The feature maintainer has approved the PR ready to merge tests
Projects
Status: Merged
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants