Development: Use directive for programming test status details and diff report

[florian-glombik]

Checklist

General

• I have tested the changes on a test server
• I chose a title conforming to the naming conventions for pull requests.

Client

• I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
• I documented the TypeScript code using JSDoc style.
• I added multiple screenshots/screencasts of my UI changes.

Motivation and Context

Follow up on Development: Add directive to fix types in exercise detail overview list #8644

Description

• Added a component for ProgrammingDiffReportDetails
• Added a component for ProgrammingTestStatusDetails
• Using ExerciseDetails directive for ProgrammingDiffReportDetails
• Using ExerciseDetails directive for ProgrammingTestStatusDetails
• Added unit tests
• Added folder structure to detail components
• Made the ArtemisTranslate Pipe a standalone component

Steps for Testing

Prerequisites:

• 1 Instructor

1. Navigate to the details page of a programming exercise
2. Verify that the template / solution result is displayed properly and triggering the build works as expected
3. Verify the template solution comparison is displayed properly

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

• Code Review 1
• Code Review 2

Manual Tests

• Test 1
• Test 2

Screenshots

The UI should not have changed

Summary by CodeRabbit

• New Features

  • Introduced two new components: ProgrammingDiffReportDetailComponent and ProgrammingTestStatusDetailComponent for enhanced display of programming exercise details.
  • Added translation support for short descriptions in the diff report component.
  • Implemented a button to view Git diff reports, conditionally displayed based on changes.

• Bug Fixes

  • Removed unused imports and functionalities related to Git diff reporting to streamline the interface.

• Documentation

  • Updated visibility of interfaces for better accessibility across modules.

• Chores

  • Restructured import paths for better organization and maintainability.
  • Enhanced test coverage for the ProgrammingDiffReportDetailComponent and ProgrammingTestStatusDetailComponent with unit tests.

[coderabbitai]

Walkthrough

The changes introduce two new Angular components, ProgrammingDiffReportDetailComponent and ProgrammingTestStatusDetailComponent, to enhance the display of programming exercise details. The DetailOverviewListComponent has been simplified by removing outdated rendering logic for these details. The TranslateDirective has been updated to be standalone, and various import paths have been reorganized for improved modularity. Additionally, certain interfaces have been marked as exported to enhance accessibility.

Changes

Files	Change Summary
src/main/webapp/app/detail-overview-list/components/programming-diff-report-detail/...	Introduced ProgrammingDiffReportDetailComponent for displaying programming diff reports with translation and interactive features.
src/main/webapp/app/detail-overview-list/components/programming-test-status-detail/...	Introduced ProgrammingTestStatusDetailComponent for displaying programming test statuses with conditional rendering based on participation data.
src/main/webapp/app/detail-overview-list/detail-overview-list.component.html	Removed rendering logic for ProgrammingTestStatus and ProgrammingDiffReport, simplifying the template.
src/main/webapp/app/detail-overview-list/detail-overview-list.component.ts	Removed unused imports and methods related to Git diff reporting.
src/main/webapp/app/detail-overview-list/detail.model.ts	Exported ProgrammingTestStatusDetail and ProgrammingDiffReportDetail interfaces for better accessibility.
src/main/webapp/app/detail-overview-list/exercise-detail.directive.ts	Updated import paths for components and added mappings for new detail types: ProgrammingTestStatus and ProgrammingDiffReport.
src/main/webapp/app/shared/language/translate.directive.ts	Updated TranslateDirective to be standalone, allowing independent usage.
src/main/webapp/app/shared/shared-common.module.ts	Moved TranslateDirective from declarations to imports, promoting modularity.
src/test/javascript/spec/component/exercise-detail.directive.spec.ts	Updated tests to include new components and improved import organization, enhancing test coverage for new functionalities.
src/test/javascript/spec/component/detail-overview-list.component.spec.ts	Removed tests related to modal functionality and adjusted import statements for better organization.
src/test/javascript/spec/component/programming-diff-report-detail.component.spec.ts	Added unit tests for ProgrammingDiffReportDetailComponent, verifying modal functionality and handling of valid and undefined inputs.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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>.
  • 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 generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @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.

[coderabbitai]

Actionable comments posted: 10

Outside diff range comments (7)

src/main/webapp/app/shared/language/translate.directive.ts (1)

Line range hint 14-55: Consider these minor improvements to enhance the directive.

The overall implementation is solid, but here are a few suggestions to further improve the code:

1. Enhance error handling in the getTranslation method.
2. Consider using Angular's DomSanitizer when setting innerHTML to mitigate potential XSS risks.
3. Add more JSDoc comments to improve code documentation.

Here's how you could implement these improvements:

1. Improve error handling:

private getTranslation(): void {
    this.translateService
        .get(this.jhiTranslate, this.translateValues)
        .pipe(takeUntil(this.directiveDestroyed))
        .subscribe({
            next: (value) => {
                this.el.nativeElement.innerHTML = value;
            },
            error: (error) => {
                console.error(`Translation error for key [${this.jhiTranslate}]:`, error);
                this.el.nativeElement.innerHTML = `${translationNotFoundMessage}[${this.jhiTranslate}]`;
            },
        });
}

2. Use DomSanitizer (add this import: import { DomSanitizer } from '@angular/platform-browser';):

constructor(
    private el: ElementRef,
    private translateService: TranslateService,
    private sanitizer: DomSanitizer
) {}

// In getTranslation method:
this.el.nativeElement.innerHTML = this.sanitizer.sanitize(SecurityContext.HTML, value) || '';

3. Add JSDoc comments:

/**
 * A directive that wraps the translate pipe from ngx-translate.
 * It provides a more concise way to translate content in templates.
 */
@Directive({
    selector: '[jhiTranslate]',
    standalone: true,
})
export class TranslateDirective implements OnChanges, OnInit, OnDestroy {
    /** The translation key */
    @Input() jhiTranslate!: string;

    /** Optional values to be used in the translation */
    @Input() translateValues?: { [key: string]: unknown };

    // ... rest of the code ...

    /**
     * Fetches and sets the translated content.
     * If an error occurs during translation, it displays an error message.
     */
    private getTranslation(): void {
        // ... implementation ...
    }
}

These changes will improve error handling, security, and code documentation.

src/main/webapp/app/shared/shared-common.module.ts (1)

Line range hint 18-45: Consider converting other directives to standalone

While TranslateDirective has been converted to standalone, there are other directives in this module that could potentially be converted as well. This would further improve modularity and tree-shaking capabilities.

Consider converting the following directives to standalone:

• SortByDirective
• SortDirective

Here's an example of how you might refactor SortByDirective:

import { Directive, Input, ... } from '@angular/core';

@Directive({
  selector: '[jhiSortBy]',
  standalone: true
})
export class SortByDirective {
  // ... existing implementation
}

After converting these directives to standalone, update the module definition accordingly:

@NgModule({
    imports: [
        ArtemisSharedLibsModule,
        TranslateDirective,
        SortByDirective,
        SortDirective
    ],
    declarations: [
        ArtemisDatePipe,
        ArtemisDateRangePipe,
        FindLanguageFromKeyPipe,
        AlertOverlayComponent,
        ArtemisTranslatePipe,
        ArtemisTimeAgoPipe,
        ArtemisDurationFromSecondsPipe,
        DurationPipe,
        CloseCircleComponent,
    ],
    // ... rest of the module definition
})
export class ArtemisSharedCommonModule {}

This refactoring would make your module more aligned with Angular's latest best practices.

src/main/webapp/app/detail-overview-list/exercise-detail.directive.ts (1)

Line range hint 1-78: Consider improving future extensibility.

The changes are well-implemented and maintain the existing structure and patterns. However, as the number of detail types grows, the current approach might become harder to maintain.

Consider refactoring the detailTypeToComponent mapping to use a more dynamic approach. This could involve creating a registry of detail components that can be easily extended without modifying this directive. For example:

// In a separate file, e.g., detail-component-registry.ts
export const DetailComponentRegistry = new Map<DetailType, Type<any>>([
  [DetailType.Text, TextDetailComponent],
  [DetailType.Date, DateDetailComponent],
  // ... other mappings
]);

// In the directive
import { DetailComponentRegistry } from './detail-component-registry';

// ...

ngOnInit() {
  if (!this.isShownDetail()) {
    return;
  }
  this.detail = this.detail as ShownDetail;

  const detailComponent = DetailComponentRegistry.get(this.detail.type);
  if (detailComponent) {
    this.componentRef = this.viewContainerRef.createComponent(detailComponent);
    this.assignAttributes();
  }
}

This approach would make it easier to add new detail types in the future without modifying the directive itself.

src/main/webapp/app/detail-overview-list/detail-overview-list.component.ts (1)

Line range hint 1-101: Suggestions for code improvements

While the remaining code is well-structured, consider the following improvements:

1. Type safety: Add explicit return types to methods, including lifecycle hooks.
2. Code organization: Group related properties and methods together for better readability.
3. Constant enums: Consider using const enums for DetailType to improve performance.
4. Readonly properties: Mark input properties and those that don't change after initialization as readonly.

Here's an example of how you could apply these suggestions:

import { Component, Input, OnDestroy, OnInit, ViewEncapsulation } from '@angular/core';
// ... other imports ...

const enum DetailType {
    Link = 'detail-link',
    Text = 'detail-text',
    // ... other enum values ...
}

@Component({
    selector: 'jhi-detail-overview-list',
    templateUrl: './detail-overview-list.component.html',
    styleUrls: ['./detail-overview-list.component.scss'],
    encapsulation: ViewEncapsulation.None,
})
export class DetailOverviewListComponent implements OnInit, OnDestroy {
    // Constants and readonly properties
    protected readonly isEmpty = isEmpty;
    protected readonly DetailType = DetailType;
    protected readonly FeatureToggle = FeatureToggle;
    protected readonly ButtonSize = ButtonSize;
    protected readonly ProgrammingExerciseParticipationType = ProgrammingExerciseParticipationType;
    protected readonly TooltipPlacement = TooltipPlacement;
    readonly CHAT = IrisSubSettingsType.CHAT;

    // Inputs
    @Input() readonly sections!: DetailOverviewSection[];

    // Component properties
    headlines: { id: string; translationKey: string }[] = [];
    headlinesRecord: Record<string, string> = {};
    isLocalVC = false;

    private profileSubscription?: Subscription;

    constructor(
        private readonly modelingExerciseService: ModelingExerciseService,
        private readonly alertService: AlertService,
        private readonly profileService: ProfileService,
    ) {}

    ngOnInit(): void {
        this.initializeHeadlines();
        this.subscribeToProfileInfo();
    }

    ngOnDestroy(): void {
        this.profileSubscription?.unsubscribe();
    }

    downloadApollonDiagramAsPDf(umlModel?: UMLModel, title?: string): void {
        // ... method implementation ...
    }

    private initializeHeadlines(): void {
        this.headlines = this.sections.map((section) => ({
            id: section.headline.replaceAll('.', '-'),
            translationKey: section.headline,
        }));
        this.headlinesRecord = this.headlines.reduce((acc, curr) => ({
            ...acc,
            [curr.translationKey]: curr.id,
        }), {});
    }

    private subscribeToProfileInfo(): void {
        this.profileSubscription = this.profileService.getProfileInfo().subscribe((profileInfo) => {
            this.isLocalVC = profileInfo.activeProfiles.includes(PROFILE_LOCALVC);
        });
    }
}

This refactored version improves type safety, organizes the code better, and uses const enums and readonly properties where appropriate.

src/test/javascript/spec/component/exercise-detail.directive.spec.ts (2)

Line range hint 56-107: Consider enhancing test coverage.

The test suite is well-structured and covers various detail types and components. To further improve it:

1. Add test cases for error scenarios (e.g., invalid detail types).
2. Consider testing the directive with different input combinations.
3. Verify that the correct properties are passed to the created components.

These additions would enhance the robustness and completeness of your test suite.

---

Line range hint 110-127: Enhance jest expectations for more specific assertions.

While the current use of jest is good, consider the following improvements:

1. Replace expect(createComponentSpy).not.toHaveBeenCalled() with expect(createComponentSpy).not.toHaveBeenCalled() for more explicit assertions.
2. For positive cases, use toHaveBeenCalledExactlyOnceWith(expectedComponent) instead of toHaveBeenCalledWith(expectedComponent) to ensure the component is created exactly once with the correct parameters.

These changes will make your tests more precise and easier to debug if they fail.

src/main/webapp/app/detail-overview-list/detail-overview-list.component.html (1)

Line range hint 1-114: Consider enhancing consistency and clarity in the template.

1. Translation approach: The template uses both jhiTranslate directive and artemisTranslate pipe. Consider standardizing the translation approach for better maintainability.

2. Component documentation: For components like jhi-programming-exercise-instructions and jhi-programming-exercise-lifecycle, consider adding comments to explain their purpose and expected inputs.

3. isEmpty function: The usage of isEmpty(detail.data.umlModel?.elements) could be replaced with a more explicit check, e.g., detail.data.umlModel?.elements?.length > 0, for better readability.

[az108]

reapprove after merge

[JohannesWt]

Code reapprove

[pzdr7]

Code

[SimonEntholzer]

Works on TS3 👍