Development: Migrate client code for plagiarism
#9901
Conversation
….ts to standalone
…endency injection from constructor to inject
….ts dependency injection from constructor to inject
…on from constructor to inject
…from constructor to inject
…ism-case-review.components.ts to standalone components
… to standalone component
… dependency injection from constructor to inject
github-actions
bot
added
tests
client
xHadie
changed the title
Development: Migrate plagiarism submodule of courses module to signals,inject and standalone
xHadie
changed the title
Development: Migrate plagiarism submodule of courses module to signals,inject and standaloneDevelopment: Migrate plagiarism submodule of courses module to signals, inject and standalone
github-actions
bot
added
server
AjayvirS
changed the title
Development: Migrate plagiarism submodule of courses module to signals, inject and standaloneDevelopment: Migrate plagiarism submodule of courses module to signals inject and standalone
AjayvirS
changed the title
Development: Migrate plagiarism submodule of courses module to signals inject and standaloneDevelopment: Migrate plagiarism submodule of courses module to signals inject and standalone
…igration' into chore/plagiarism-checks/client-migration
AjayvirS
changed the title
Development: Migrate plagiarism submodule of courses module to signals inject and standaloneDevelopment: Migrate client code for the plagiarism component
AjayvirS
changed the title
Development: Migrate client code for the plagiarism componentDevelopment: Migrate client code for plagiarism
WalkthroughThis pull request introduces a comprehensive refactoring of the plagiarism detection components in the Angular application. The changes primarily focus on modernizing the component architecture by adopting standalone components, updating dependency injection methods using the Changes
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: 3
🔭 Outside diff range comments (2)
src/main/webapp/app/course/plagiarism-cases/shared/verdict/plagiarism-case-verdict.component.html (1)
Line range hint
22-34: Remove duplicate verdict display logicThe verdict display logic is duplicated between the main template and the ng-template.
Consider extracting the shared verdict display logic into a separate component or template reference:
-<ng-template #verdict> - <div - class="badge py-2" - [class]="plagiarismCase().verdict === plagiarismVerdict.NO_PLAGIARISM ? 'bg-success' : 'bg-danger'" - ngbTooltip="{{ - 'artemisApp.plagiarism.plagiarismCases.verdict.description' - | artemisTranslate: { date: plagiarismCase().verdictDate | artemisDate, user: plagiarismCase().verdictBy?.name } - }}" - > - <h6 class="mb-0">{{ 'artemisApp.plagiarism.plagiarismCases.verdict.verdict' | artemisTranslate }} : {{ verdictTranslationString | artemisTranslate }}</h6> - </div> -</ng-template> +<ng-template #verdict> + <ng-container *ngTemplateOutlet="verdictDisplay"></ng-container> +</ng-template> + +<ng-template #verdictDisplay> + <div + class="badge py-2" + [class]="getVerdictClass()" + [ngbTooltip]="getVerdictDescription() | artemisTranslate" + > + <h6 class="mb-0"> + {{ 'artemisApp.plagiarism.plagiarismCases.verdict.verdict' | artemisTranslate }} : + {{ verdictTranslationString | artemisTranslate }} + </h6> + </div> +</ng-template>src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/modeling-submission-viewer/modeling-submission-viewer.component.ts (1)
Line range hint
31-44: Prevent potential memory leaks in subscriptionThe subscription in
ngOnChangesshould be cleaned up to prevent memory leaks.Consider this implementation:
+ private destroy$ = new Subject<void>(); ngOnChanges(changes: SimpleChanges): void { if (changes.plagiarismSubmission) { const currentPlagiarismSubmission: PlagiarismSubmission<ModelingSubmissionElement> = changes.plagiarismSubmission.currentValue; if (!this.hideContent) { this.loading = true; - this.modelingSubmissionService.getSubmissionWithoutLock(currentPlagiarismSubmission.submissionId).subscribe({ + this.modelingSubmissionService.getSubmissionWithoutLock(currentPlagiarismSubmission.submissionId) + .pipe(takeUntil(this.destroy$)) + .subscribe({ next: (submission: ModelingSubmission) => { this.loading = false; this.submissionModel = JSON.parse(submission.model!) as UMLModel; }, error: () => { this.loading = false; }, }); } } } + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + }Don't forget to import:
import { Subject } from 'rxjs'; import { takeUntil } from 'rxjs/operators';
🧹 Nitpick comments (17)
src/main/webapp/app/course/plagiarism-cases/shared/review/plagiarism-case-review.component.ts (1)
15-16: Use of theinputfunction is consistent.Replacing
@Input()withinput()is a valid Angular approach in newer versions. Ensure that downstream references are updated to callplagiarismCase()andforStudent()rather than accessing them as normal properties.src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.component.ts (1)
43-43: Conditional check uses function call.
this.comparison()might beundefined; verifying before using properties is safer. Watch out for the double question mark operator if you need a fallback assignment.src/test/javascript/spec/component/plagiarism/plagiarism-header.component.spec.ts (5)
43-44: InitializesplitControlSubjectonce if possible.It may be beneficial to keep a single subject initialization in the
beforeEachfor consistency across tests, rather than re-initializing it in multiple places. This ensures a more uniform test setup and potentially reduces boilerplate.
45-45: CalldetectChanges()after setting all inputs for clarity.Although calling
detectChanges()here is valid, consider groupingsetInputcalls and callingdetectChanges()once afterward. This pattern can help make the test flow clearer and reduce the risk of missing a re-render in more complex scenarios.
82-84: Avoid mutating the samecomparisonreference across tests.Similar to line 61–63, ensure that test data is consistently reset to a fresh object in each test. This approach prevents leftover states from affecting subsequent tests, making them more robust.
117-117: Add negative test for failure scenario.While we see a test verifying the correct status update path, consider also testing a failure scenario from the service (e.g., an HTTP 400 or 500), to ensure robust error handling in the UI.
122-123: ResetsplitControlSubjectconsistently.Re-initializing
splitControlSubjecthere is fine; however, doing it once per test or in thebeforeEachblock might reduce repetitive code. Also, confirm that unsubscription or teardown logic is in place ifsplitControlSubjectcan emit memory leaks in real usage.Also applies to: 131-131
src/test/javascript/spec/component/plagiarism/plagiarism-cases-instructor-view.component.spec.ts (3)
109-109: Use a typed or specialized router mock if available.Using a
MockRouteris an excellent approach, but ensure that it stays in sync with the methods used by your real routing logic if they evolve (e.g.,router.navigate,router.navigateByUrl, etc.). This ensures the mock consistently reflects real usage.
152-156: Add a negative test forgetExamPlagiarismCasesForInstructor.You test success cases for fetching exam plagiarism data. Consider adding a negative test for an error response to ensure the component handles fetch failures gracefully.
269-269: Add a newline for style compliance.A static analysis tool flagged this line for missing a newline. To keep a clean style, add a newline at the end or as appropriate.
} }) +🧰 Tools
🪛 eslint
[error] 269-269: Insert
⏎(prettier/prettier)
🪛 GitHub Check: client-style
[failure] 269-269:
Insert⏎src/main/webapp/app/course/plagiarism-cases/shared/verdict/plagiarism-case-verdict.component.html (1)
Line range hint
13-20: Consider extracting complex template expressionsWhile the code is functional, the template interpolation could be simplified for better maintainability.
Consider moving the complex date and user formatting logic to the component:
-[class]="plagiarismCase().verdict === plagiarismVerdict.NO_PLAGIARISM ? 'bg-success' : 'bg-danger'" +[class]="getVerdictClass()" -| artemisTranslate: { date: plagiarismCase().verdictDate | artemisDate, user: plagiarismCase().verdictBy?.name } +| artemisTranslate: getVerdictDescription()Add these methods to your component:
getVerdictClass(): string { return this.plagiarismCase().verdict === this.plagiarismVerdict.NO_PLAGIARISM ? 'bg-success' : 'bg-danger'; } getVerdictDescription() { return { date: this.plagiarismCase().verdictDate | artemisDate, user: this.plagiarismCase().verdictBy?.name }; }src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.component.html (1)
10-22: Simplify button state managementThe disabled state logic is repeated and could be simplified for better maintainability.
Consider moving the disabled state logic to the component:
-[disabled]="comparison()?.status === plagiarismStatus.CONFIRMED || isLoading || exercise().teamMode" +[disabled]="isConfirmButtonDisabled()" -[disabled]="comparison()?.status === plagiarismStatus.DENIED || isLoading || exercise().teamMode" +[disabled]="isDenyButtonDisabled()"Add these methods to your component:
isConfirmButtonDisabled(): boolean { return this.comparison()?.status === this.plagiarismStatus.CONFIRMED || this.isLoading || this.exercise().teamMode; } isDenyButtonDisabled(): boolean { return this.comparison()?.status === this.plagiarismStatus.DENIED || this.isLoading || this.exercise().teamMode; }src/main/webapp/app/exercises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.component.ts (3)
67-67: Clarify the Boolean expression.
The conditional logic is correct, but since optional chaining can yieldundefined, consider handling that case explicitly for readability.
97-108: Use consistent naming for the threshold parameter.
"threshHold" is spelled with an uppercase "H." Consider standardizing tothresholdfor consistency.- updateSimilarityThreshold(threshHold: number) { + updateSimilarityThreshold(threshold: number) {
136-147: Optional validation for plagiarism case response period.
If there's an upper limit or a recommended range, consider enforcing it within the update method.src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/text-submission-viewer/text-submission-viewer.component.ts (1)
29-30: Standalone declaration and imports array.Adopting the standalone approach in Angular 17 is strongly encouraged. Check any subscriptions introduced here to ensure correct cleanup in
ngOnDestroy()(or that you rely on Angular’s async pipe or a completed subject).src/main/webapp/app/exercises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.component.html (1)
Line range hint
41-57: Enhance form accessibilityThe numeric inputs lack proper aria labels for screen readers.
Add aria-labels to the numeric inputs:
<input type="number" required class="form-control" min="0" max="100" step="5" id="plagiarism-similarity-threshold" name="plagiarismChecksSimilarityThresholdPercentage" + [attr.aria-label]="'artemisApp.plagiarism.similarityThreshold' | artemisTranslate" [ngModel]="exercise()?.plagiarismDetectionConfig?.similarityThreshold" (ngModelChange)="updateSimilarityThreshold($event)" />Apply similar changes to other numeric inputs.
Also applies to: 61-77, 81-97, 101-119
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
src/main/webapp/app/course/plagiarism-cases/instructor-view/detail-view/plagiarism-case-instructor-detail-view.component.ts(2 hunks)src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts(1 hunks)src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.module.ts(1 hunks)src/main/webapp/app/course/plagiarism-cases/shared/plagiarism-cases-shared.module.ts(1 hunks)src/main/webapp/app/course/plagiarism-cases/shared/plagiarism-cases.service.ts(2 hunks)src/main/webapp/app/course/plagiarism-cases/shared/plagiarism-results.service.ts(1 hunks)src/main/webapp/app/course/plagiarism-cases/shared/review/plagiarism-case-review.component.html(2 hunks)src/main/webapp/app/course/plagiarism-cases/shared/review/plagiarism-case-review.component.ts(1 hunks)src/main/webapp/app/course/plagiarism-cases/shared/verdict/plagiarism-case-verdict.component.html(3 hunks)src/main/webapp/app/course/plagiarism-cases/shared/verdict/plagiarism-case-verdict.component.ts(1 hunks)src/main/webapp/app/course/plagiarism-cases/student-view/detail-view/plagiarism-case-student-detail-view.component.ts(2 hunks)src/main/webapp/app/course/plagiarism-cases/student-view/plagiarism-cases-student-view.module.ts(1 hunks)src/main/webapp/app/exercises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.component.html(6 hunks)src/main/webapp/app/exercises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.component.ts(5 hunks)src/main/webapp/app/exercises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.module.ts(1 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-details/plagiarism-details.component.html(1 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-details/plagiarism-details.component.ts(1 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.component.html(1 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.component.ts(4 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-run-details/plagiarism-run-details.component.ts(2 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-sidebar/plagiarism-sidebar.component.ts(1 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/modeling-submission-viewer/modeling-submission-viewer.component.ts(1 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/plagiarism-split-view.component.ts(2 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts(2 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/text-submission-viewer/text-submission-viewer.component.ts(2 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism.module.ts(2 hunks)src/test/javascript/spec/component/plagiarism/exercise-update-plagiarism.component.spec.ts(3 hunks)src/test/javascript/spec/component/plagiarism/modeling-submission-viewer.component.spec.ts(1 hunks)src/test/javascript/spec/component/plagiarism/plagiarism-case-instructor-detail-view.component.spec.ts(1 hunks)src/test/javascript/spec/component/plagiarism/plagiarism-case-student-detail-view.component.spec.ts(1 hunks)src/test/javascript/spec/component/plagiarism/plagiarism-case-verdict.component.spec.ts(3 hunks)src/test/javascript/spec/component/plagiarism/plagiarism-cases-instructor-view.component.spec.ts(3 hunks)src/test/javascript/spec/component/plagiarism/plagiarism-header.component.spec.ts(5 hunks)src/test/javascript/spec/component/plagiarism/plagiarism-run-details.component.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (34)
src/main/webapp/app/course/plagiarism-cases/shared/review/plagiarism-case-review.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/test/javascript/spec/component/plagiarism/modeling-submission-viewer.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/webapp/app/course/plagiarism-cases/shared/plagiarism-results.service.ts (1)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.module.ts (1)
src/test/javascript/spec/component/plagiarism/plagiarism-run-details.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-sidebar/plagiarism-sidebar.component.ts (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.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/course/plagiarism-cases/shared/plagiarism-cases.service.ts (1)
src/test/javascript/spec/component/plagiarism/plagiarism-case-student-detail-view.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/modeling-submission-viewer/modeling-submission-viewer.component.ts (1)
src/test/javascript/spec/component/plagiarism/plagiarism-case-verdict.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/plagiarism/exercise-update-plagiarism.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/webapp/app/course/plagiarism-cases/shared/verdict/plagiarism-case-verdict.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/course/plagiarism-cases/student-view/plagiarism-cases-student-view.module.ts (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-run-details/plagiarism-run-details.component.ts (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-details/plagiarism-details.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/course/plagiarism-cases/shared/review/plagiarism-case-review.component.ts (1)
src/test/javascript/spec/component/plagiarism/plagiarism-case-instructor-detail-view.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/webapp/app/course/plagiarism-cases/shared/plagiarism-cases-shared.module.ts (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-details/plagiarism-details.component.ts (1)
src/test/javascript/spec/component/plagiarism/plagiarism-header.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/webapp/app/exercises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.module.ts (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/plagiarism-split-view.component.ts (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/text-submission-viewer/text-submission-viewer.component.ts (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism.module.ts (1)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (1)
src/main/webapp/app/course/plagiarism-cases/student-view/detail-view/plagiarism-case-student-detail-view.component.ts (1)
src/test/javascript/spec/component/plagiarism/plagiarism-cases-instructor-view.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/webapp/app/course/plagiarism-cases/shared/verdict/plagiarism-case-verdict.component.ts (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.component.ts (1)
src/main/webapp/app/course/plagiarism-cases/instructor-view/detail-view/plagiarism-case-instructor-detail-view.component.ts (1)
src/main/webapp/app/exercises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.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/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts (1)
src/main/webapp/app/exercises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.component.ts (1)
🪛 eslint
src/test/javascript/spec/component/plagiarism/plagiarism-cases-instructor-view.component.spec.ts
[error] 269-269: Insert ⏎
(prettier/prettier)
🪛 GitHub Check: client-style
src/test/javascript/spec/component/plagiarism/plagiarism-cases-instructor-view.component.spec.ts
[failure] 269-269:
Insert ⏎
src/main/webapp/app/exercises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.component.ts
[warning] 74-74:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
🔇 Additional comments (114)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-details/plagiarism-details.component.html (2)
1-1: LGTM! Proper usage of new Angular control flow syntax.
The @if syntax correctly replaces *ngIf as per Angular 17+ guidelines.
3-4: Verify signal implementation in the component class.
The template correctly uses function calls for signals (comparison() and exercise()). However, let's verify the component class implementation:
The component structure looks good, with proper usage of signals in the template. The verification will help ensure the component class is properly implemented.
✅ Verification successful
Let me verify the signal implementation in the component class with a more specific search.
Signals are correctly implemented in the component class
The component class correctly uses Angular's input signals:
comparison = input<PlagiarismComparison<TextSubmissionElement | ModelingSubmissionElement>>();
exercise = input.required<Exercise>();The template's usage of comparison() and exercise() matches the signal implementation in the component. The splitControlSubject is correctly kept as a regular Subject since it's used for control flow rather than reactive state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify signal implementation in the component class
# Check if the component properly implements the signals
ast-grep --pattern 'class PlagiarismDetailsComponent {
$$$
comparison = signal($$$);
exercise = signal($$$);
$$$
}'
# Verify there are no leftover references to the old style
rg -A 5 "comparison: \w+.*;"
rg -A 5 "exercise: \w+.*;"
Length of output: 66001
Script:
#!/bin/bash
# Search for the PlagiarismDetailsComponent implementation
rg -A 10 "class PlagiarismDetailsComponent"
# Search for signal imports
rg "import.*signal.*from.*@angular/core"
Length of output: 13020
src/main/webapp/app/course/plagiarism-cases/shared/plagiarism-results.service.ts (2)
1-1: Switched to the new inject import successfully
Good to see inject from @angular/core imported. This aligns with modern Angular guidelines for simplified dependency injection, removing the need for a constructor.
7-7: Verified the http property injection
Replacing constructor injection with private http = inject(HttpClient) is consistent with Angular's latest DI approach. The usage in getNumberOfPlagiarismResultsForExercise() remains clear and maintainable.
src/main/webapp/app/course/plagiarism-cases/shared/plagiarism-cases.service.ts (2)
1-1: Updated import to include inject
This import change reflects the shift from constructor injection to the inject function. Make sure to remove any unused constructor references, if any, within the file.
18-18: Use of private http = inject(HttpClient) looks good
This is a clean, modern approach to Angular DI. Confirm that no logic was moved or lost from the old constructor. Otherwise, the code effectively maintains previous functionality.
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-run-details/plagiarism-run-details.component.ts (2)
11-15: Ensure all newly added imports are used or remove any unnecessary ones.
It looks like you’ve added several imports (TranslateDirective, ArtemisSharedComponentModule, BarChartModule, DatePipe, ArtemisTranslatePipe). Please confirm each is actually used in the template or component logic to avoid unnecessary bundle size increases. If some are unused, consider removing them for better performance.
27-28: Great move to standalone components—confirm removal from any parent NgModule.
Marking this component as standalone follows Angular 17 best practices. Ensure that any module declarations referencing this component are removed to avoid duplication or confusion.
src/main/webapp/app/course/plagiarism-cases/shared/verdict/plagiarism-case-verdict.component.ts (5)
4-5: Usage of new imports is consistent
You properly reference NgbTooltip and ArtemisSharedCommonModule in the component’s imports. This aligns with the standalone component approach.
10-11: Standalone component structure looks good
Defining standalone: true and listing all required imports within the component metadata is a solid approach.
14-15: New input-based property initialization
The plagiarismCase = input.required<PlagiarismCase>(); and hideDetails = input(false); declarations are consistent with the new signal-based input approach in Angular. Both variables use correct camelCase naming, adhering to your coding guidelines.
20-20: Signal-like property access
Calling this.plagiarismCase() within the switch statement is correct for function-based inputs. Double-check that the component never processes a null or undefined input, although required mitigates that risk.
1-1: Check for Angular version compatibility
Be sure your project is on Angular 16+ so the new input API is supported. Otherwise, consider reverting to the classic @Input().
Run this script to confirm Angular’s version:
✅ Verification successful
Angular version is compatible with input API
The project uses Angular 18.2.13, which fully supports the new input API introduced in Angular 16+. The usage of input from @angular/core is correct and follows the latest Angular practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verifies that @angular/core version is at least 16 or 17 in package.json
rg '"@angular/core":' package.json
Length of output: 72
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (5)
2-3: Adopting the inject function
Replacing constructor dependency injection with inject() is valid in Angular 16+. This is concise and modern.
9-15: Expanded imports
All newly imported directives and modules (e.g., TranslateDirective, ArtemisSharedComponentModule, etc.) are properly addressed in the standalone component style. Verify each is used or needed to keep dependencies lean.
23-23: Standalone usage
Declaring standalone: true ensures this component can be used without an NgModule. This is aligned with best practices in newer Angular versions.
24-33: Component-level imports array
Listing these modules (TranslateDirective, ArtemisSharedCommonModule, etc.) directly in the imports array is the correct standalone approach for reusability.
36-38: Inline injection for services
inject(PlagiarismCasesService), inject(ActivatedRoute), and inject(AlertService) streamline setup. This aligns with your preference of having no underscore or “priv” prefix.
src/main/webapp/app/course/plagiarism-cases/instructor-view/detail-view/plagiarism-case-instructor-detail-view.component.ts (6)
1-1: Inline injection from @angular/core
Using inject for OnDestroy or OnInit is valid if your Angular version supports it. Confirm the library version in your environment.
4-4: Router imports
RouterLink is properly imported for navigation. Ensure it’s consistently used in the template to avoid dead code.
20-40: Comprehensive new imports block
You have introduced a variety of modules (TranslateDirective, NgbDropdown, etc.). This is a correct shift to on-demand, per-component imports in a standalone environment.
47-47: Marking component as standalone
standalone: true is correct. It helps keep the module’s declarations small and centralizes dependencies in the component.
48-69: Component-level imports
This array enumerates all modules used by the HTML template (MetisModule, FaIconComponent, PlagiarismCaseVerdictComponent, etc.). It’s consistent with the new Angular style guidelines.
72-79: Dependency injection with the inject function
All services (MetisService, AlertService, TranslateService, etc.) are injected inline, eliminating the constructor. This is valid and keeps your component’s constructor minimal.
src/main/webapp/app/course/plagiarism-cases/shared/plagiarism-cases-shared.module.ts (1)
10-10: Removal of declared/ exported components
Removing PlagiarismCaseVerdictComponent and others from exports is consistent with these components now being standalone. Ensure no stale imports remain in older modules.
src/main/webapp/app/course/plagiarism-cases/student-view/plagiarism-cases-student-view.module.ts (1)
21-21: Imports array update
Including PlagiarismCaseStudentDetailViewComponent as a standalone component in imports avoids duplication in declarations. This is aligned with Angular’s recommended approach.
src/main/webapp/app/course/plagiarism-cases/shared/review/plagiarism-case-review.component.ts (3)
1-1: Check usage of the new input import.
Great to see the transition to input from @angular/core. Make sure that any references to the old @Input decorator have been removed so there are no collisions or confusion for maintainers.
4-6: Modular imports look fine.
Using @ng-bootstrap/ng-bootstrap and the Artemis modules is consistent with Angular best practices for modularizing functionality.
11-12: Good move to a standalone component.
Declaring this component as standalone and specifying required imports helps with faster compilation and better tree shaking. Keep verifying that all needed imports are referenced for template functionality.
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.module.ts (1)
31-31: Use of imported standalone components.
Removing declarations and moving to a purely imported approach aligns with Angular's standalone component style. Confirm that any references within templates still function properly as expected.
src/test/javascript/spec/component/plagiarism/plagiarism-case-verdict.component.spec.ts (3)
17-17: Stand-alone test setup improvements.
Importing PlagiarismCaseVerdictComponent directly in the imports array simplifies the module configuration. This approach helps ensure that the component, along with its required dependencies, is properly initialized.
32-33: Better clarity in input assignment.
By calling fixture.componentRef.setInput('plagiarismCase', mockPlagiarismCase), you explicitly set the input value. This is a clean approach for verifying input-dependent logic in tests.
46-47: Consistent mocking pattern.
Same pattern for setting up the mock plagiarismCase here. It increases test readability and reduces ad-hoc assignments. Good job maintaining consistency.
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.component.ts (7)
1-1: Switched to inject and input.
Using inject replaces a constructor-based DI, making the component simpler. The input function is well-aligned with the rest of the code changes in this PR.
11-12: Selective imports for translation features.
Importing TranslateDirective and ArtemisTranslatePipe directly in the standalone component prevents redundant module overhead while enabling translation. Great approach.
18-19: Standalone component usage is good.
Making this component standalone streamlines dependencies and fosters reusability. Keep an eye on any subcomponents that might still rely on a parent module import.
22-23: Replacing constructor with inject.
inject() usage can help avoid typical constructor boilerplate. Just make sure that the order of assignment is correct if you inject multiple services with potential dependencies among them.
25-27: Properly declared inputs with required.
Using the required form ensures that these inputs must be provided, preventing potential runtime errors when needed data is missing. Good approach.
66-73: Prevent concurrency issues with local variable usage.
Storing this.comparison() into a local variable before the async request is beneficial. It avoids referencing stale data if the input changes mid-request. Nicely done.
77-81: Forwarding commands to the split pane from inputs.
Using splitControlSubject() references in the methods ensures correct data flow. This chaining is consistent with the rest of the PR's approach of using function-based input references.
src/test/javascript/spec/component/plagiarism/modeling-submission-viewer.component.spec.ts (1)
30-30: **Adopted standalone component testing approach **
Good job transitioning to using the component directly in the imports. This aligns with Angular's testing patterns for standalone components. Ensure that any mock dependencies not included in these imports are provided through the providers array or appropriate mocking utilities.
src/test/javascript/spec/component/plagiarism/plagiarism-run-details.component.spec.ts (2)
6-6: **Use of ng-mocks for module and providers **
Importing MockModule, MockPipe, and MockProvider from ng-mocks is a clean way to simplify module mocking. This helps maintain lightweight and focused tests.
26-27: **Standalone component import in test **
The shift of PlagiarismRunDetailsComponent from declarations to imports aligns with Angular's standalone component convention. Keeping just the MockPipe in declarations is correct, since it's not a standalone pipe.
src/main/webapp/app/course/plagiarism-cases/student-view/detail-view/plagiarism-case-student-detail-view.component.ts (6)
1-1: **Usage of inject from '@angular/core' **
Adopting inject() over constructor-based injection is a valid approach for standalone components in Angular. It can simplify constructor signatures and is consistent with the new Angular style.
4-4: **RouterLink import for in-template routing **
Including RouterLink ensures the component can leverage standard Angular navigation features in its template. This is necessary for direct usage of [routerLink] in the HTML.
16-22: **Expanded utility imports for shared modules **
Importing these shared modules (ArtemisSharedComponentModule, ArtemisSharedCommonModule, etc.) plus the TranslateDirective and FaIconComponent is a good practice, ensuring the component can directly use commonly needed directives and components without duplicating code.
29-29: **Marking the component as standalone **
Setting standalone: true is beneficial for modularizing the component and easing future reuse or lazy-loading configurations.
30-39: **Specifying imports array for the standalone component **
Listing all dependencies in the imports array is crucial to enable each referenced directive or component. This approach reduces reliance on large feature modules and is consistent with updated Angular best practices.
42-45: **Dependency injection via inject() for services **
Injecting services with the new inject() mechanism is a clean replacement for constructor injections. It also helps keep the class signature lean and ties well into Angular’s new DI patterns.
src/test/javascript/spec/component/plagiarism/plagiarism-case-student-detail-view.component.spec.ts (1)
53-53: **Standalone component in imports **
Directly importing PlagiarismCaseStudentDetailViewComponent in the test setup is consistent with standalone component testing in Angular. Affirm that the other test utilities/mocks correctly handle the component’s dependencies.
src/test/javascript/spec/component/plagiarism/plagiarism-header.component.spec.ts (5)
38-42: Ensure correct usage of fixture.componentRef.setInput() for comparison.
Using fixture.componentRef.setInput() is generally best practice in Angular 17+ to properly propagate changes through the component lifecycle. This change looks good and complies with the standard approach to setting inputs in tests.
61-63: Check for potential concurrency issues when reassigning comparison.
While this is a test scenario, reassigning the same object reference to comparison and calling setInput is correct. If concurrency or asynchronous updates were introduced in future tests, consider creating new objects or using an immutable pattern to avoid accidental side effects on shared references.
97-99: Confirm transitions between statuses.
The logic checks if the current status is CONFIRMED before opening a confirmation modal to deny plagiarism. This is appropriate. Just ensure that future statuses or expanded logic (e.g., partial plagiarism) also follows a similar confirm/deny pattern.
135-136: Validate user-triggered splits thoroughly.
These lines handle user actions: expanding left/right or resetting splits. Testing all user flows is good. If not already covered, consider adding negative or boundary tests to confirm the correct handling of edge cases (e.g., multiple consecutive clicks).
Also applies to: 144-144, 148-149, 157-157
161-163: Test the disabled state thoroughly in team mode.
Confirm that if other components or conditions keep the button disabled, those edge cases are tested. This approach will ensure that all states around team exercises are well covered.
src/test/javascript/spec/component/plagiarism/plagiarism-cases-instructor-view.component.spec.ts (3)
20-28: Mocking additional imports effectively.
Including these mock modules and directives helps isolate the component under test from unrelated logic. Great job adhering to best practices by mocking the icons, directives, etc.
112-112: Limit direct reliance on RouterModule in tests.
While RouterModule is mocked here, consider verifying that no extraneous route references creep into the test environment. This helps maintain a test scope focused on plagiarism cases alone.
119-144: Override vs. add components in overrideComponent carefully.
Removing real modules and adding mock modules is an effective approach. Keep an eye on the overridden elements so as not to remove or inadvertently exclude dependencies needed in other test scenarios. This is especially relevant if the real module’s configuration changes in the future.
src/test/javascript/spec/component/plagiarism/plagiarism-case-instructor-detail-view.component.spec.ts (1)
51-52: Adopt the new standalone component usage.
You are directly including PlagiarismCaseInstructorDetailViewComponent in imports instead of declarations, reflecting the new Angular standalone approach. This is correct and ensures the test harness better matches real usage.
src/main/webapp/app/course/plagiarism-cases/shared/review/plagiarism-case-review.component.html (2)
2-2: Modern usage of @for for iteration.
Switching to @for improves readability and aligns with newer Angular template syntax. This looks good.
12-13: Guard or default for potentially undefined properties.
Accessing plagiarismCase()?.exercise, (plagiarismCase()?.student)!.login, and calling forStudent() is valid, but ensure that null/undefined checks or fallback logic handle unexpected states. In large apps, data may arrive asynchronously, so consider marking these fields optional or safely handling their absence.
Also applies to: 15-15
src/main/webapp/app/course/plagiarism-cases/shared/verdict/plagiarism-case-verdict.component.html (1)
Line range hint 1-12: LGTM! Proper usage of new Angular syntax
The code correctly implements the new @if syntax and properly handles the plagiarismCase() as a function call, aligning with Angular 17+ patterns.
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.component.html (1)
1-7: LGTM! Good error handling
The code properly implements optional chaining with signal function calls and includes appropriate fallback text for unknown students.
src/main/webapp/app/exercises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.component.ts (9)
1-11: Imports look consistent with Angular style guidelines.
All added imports align well with the standalone component strategy. Confirm that model is indeed part of your intended custom library or extension and is properly documented in the project.
16-17: Standalone component usage and imports array are correctly specified.
This configuration follows Angular 17+ best practices for modularity.
20-20: Verify the required exercise approach.
exercise = model.required<Exercise>(); is innovative but ensure robust error handling if the exercise is not passed in or is undefined at runtime. Consider adding a fallback or fallback error message.
42-45: Ensure exercise is defined before assigning a default config.
If this.exercise() is undefined at this point, the assignment will fail. Validate the lifecycle flow or provide a fallback to prevent potential runtime errors.
84-84: Switch statement for exercise type is appropriate.
Implementation is clear and conformant with the style guidelines.
110-121: Method for updating the minimum score is clear.
The approach is straightforward. It would be good to confirm that boundary scenarios (e.g. negative values) are covered in tests.
123-134: Consider edge case tests for minimum size updates.
Validate zero, negative, and extremely large values to avoid unforeseen side effects.
149-160: Toggle method for continuous plagiarism control appears correct.
Ensure that toggling is covered by unit tests to confirm correct state updates.
162-173: Post-due-date checks toggling is straightforward.
This follows the same pattern as the other toggles, no issues found.
src/main/webapp/app/exercises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.module.ts (1)
6-6: Importing the standalone component is correct.
This pattern is recommended for Angular 17+ standalone components.
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-details/plagiarism-details.component.ts (4)
1-1: Shift from @input() to the new “input” function.
This modern pattern can improve code clarity, but ensure team familiarity and confirm that your build/test environment fully supports it.
7-8: Imports for PlagiarismHeaderComponent and PlagiarismSplitViewComponent.
Integrates well with the new standalone approach.
14-15: Standalone declaration and imports.
Aligns with evolving Angular best practices for slimmer, more modular components.
18-19: Functional input properties for comparison and exercise.
Ensure that these inputs can’t produce undefined states. Adding defensive checks or fallback logic might be beneficial.
src/main/webapp/app/exercises/shared/plagiarism/plagiarism.module.ts (3)
17-18: New imports for PlagiarismDetailsComponent and PlagiarismInspectorComponent.
Confirms consistent usage of standalone components.
21-29: Moving standalone components into the imports array.
This is correct for components that declare themselves as standalone.
40-41: Updated declarations and exports.
If the PlagiarismInspectorComponent needs to be used elsewhere, ensure it’s declared or exported in the proper place.
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-sidebar/plagiarism-sidebar.component.ts (2)
7-10: Added imports for icons, translations, and pipes.
These extensions align with a self-contained Angular 17+ approach.
16-17: Standalone component configuration.
Declaring everything in imports fosters modularity. Good job keeping the component self-contained.
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts (6)
5-5: Imports look good.
These class-based imports from @ng-bootstrap/ng-bootstrap are consistent with Angular best practices and align with the single-quote style requirement.
6-6: Import of NgClass is straightforward.
No issues found; it follows the Angular style guide.
7-7: FontAwesome import appears correct.
No concerns; the single-quote style is properly followed.
8-8: Translation directive import.
This directive import aligns with the localization best practice (localize:true).
9-9: Translation pipe import.
Inclusion of ArtemisTranslatePipe fosters DRY by centralizing translation logic.
23-24: Standalone mode and imports are aligned with Angular 17.
Marking the component as standalone: true and supplying its dependencies in imports is consistent with the guidelines for standalone components. This should enhance modularity and reusability.
src/test/javascript/spec/component/plagiarism/exercise-update-plagiarism.component.spec.ts (21)
1-13: New and updated imports look consistent.
All newly added or updated imports comply with jest testing patterns and Angular style guidelines. Single quotes are properly used.
17-17: Introduction of ComponentFixture.
Using ComponentFixture is the recommended way to test Angular components with TestBed.
20-23: Configuration with TestBed.configureTestingModule is best practice.
You are mocking dependencies via ng-mocks and providing a mock translation service, which aligns well with the guidelines to avoid full module imports.
24-24: Empty line.
No code changes required here.
25-27: Component creation is correct.
Properly instantiating the component with TestBed.createComponent and setting up the exercise property is aligned with Angular test patterns.
38-41: Reactive update approach.
Using comp.exercise.update(...) to modify the exercise object is a clear and structured way to handle state changes in the tests. Ensure that all essential properties are set within the update callback.
45-45: Assertion with toEqual is appropriate.
You're verifying the entire config object, which supports the guideline for expectation specificity.
49-52: Testing undefined plagiarism config.
Setting plagiarismDetectionConfig to undefined ensures the component falls back to defaults. Good coverage of edge cases.
56-56: Expectation for fallback is correct.
Confirming that the default config has been applied properly verifies robust fallback handling.
60-63: Assigning exercise type to PROGRAMMING for test clarity.
This directly verifies the logic handling different exercise types.
69-72: Maintaining consistent usage of update(...).
No issues; code remains consistent and test is clear.
74-74: Reliable check of default config.
Again, verifying the fallback logic thoroughly.
78-81: Toggling CPC enabled.
The approach of using comp.exercise.update(...) for state changes is consistent and keeps the code readable.
83-84: Boolean assertions.
Using .toBeTrue() matches the recommended style for boolean checks.
88-91: Toggling CPC disabled.
Consistent approach with the preceding test case. Good coverage.
93-94: Boolean checks with .toBeFalse().
Fulfills the test guidelines for specificity.
98-101: Testing exercise type TEXT.
Ensures the correct tooltip is selected, validating logic for text exercises.
106-109: Testing exercise type TEXT again.
Double-checking correct translation keys for text exercises. Nicely done.
114-117: Testing exercise type MODELING.
Ensures the correct tooltip is used for modeling exercises—high coverage of exercise types.
125-128: Aggregating input changes.
Verifies that updates to multiple fields get aggregated correctly, which is crucial for form synchronization.
145-150: Final update callback.
Ensures that toggling fields recomputes the form validity. Good demonstration of reactive state handling.
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/plagiarism-split-view.component.ts (3)
15-17: New viewer component imports.
Introducing the ModelingSubmissionViewerComponent and TextSubmissionViewerComponent helps keep the logic compartmentalized. This aligns with the guideline for code reuse.
19-22: SplitPaneDirective as standalone.
This makes the directive self-contained and easy to reuse. Consider verifying that downstream modules are prepared to handle a standalone directive if needed.
35-35: Optional comparison property.
Marking @Input() comparison? is fine; ensure that any user of comparison handles undefined values gracefully to avoid runtime errors.
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/text-submission-viewer/text-submission-viewer.component.ts (1)
17-20: Expanded imports for a more feature-rich component.
Importing SplitPaneHeaderComponent, FaIconComponent, TranslateDirective, and NgClass is consistent with single-quote usage, and fosters better code reuse and localization.
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/modeling-submission-viewer/modeling-submission-viewer.component.ts (1)
15-16: LGTM: Proper standalone component configuration
The component is correctly configured as standalone with the necessary imports.
...ercises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.component.ts
Show resolved
Hide resolved
...app/app/exercises/shared/plagiarism/plagiarism-split-view/plagiarism-split-view.component.ts
Show resolved
Hide resolved
...cises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.component.html
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/main/webapp/app/exercises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.component.ts (1)
Line range hint
20-26: Consider safer ViewChild queriesThe ViewChild queries use non-null assertions which could lead to runtime errors. Consider using optional chaining or providing default values.
-@ViewChild('exercise.plagiarismDetectionConfig!.similarityThreshol') fieldThreshhold?: NgModel; +@ViewChild('exercise.plagiarismDetectionConfig?.similarityThreshol') fieldThreshhold?: NgModel; -@ViewChild('exercise.plagiarismDetectionConfig!.continuousPlagiarismControlPlagiarismCaseStudentResponsePeriod') fieldResponsePeriod?: NgModel; +@ViewChild('exercise.plagiarismDetectionConfig?.continuousPlagiarismControlPlagiarismCaseStudentResponsePeriod') fieldResponsePeriod?: NgModel;
🧹 Nitpick comments (2)
src/main/webapp/app/exercises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.component.ts (2)
67-71: Enhance form validation robustnessThe form validation could be more defensive against null values and provide better type safety.
calculateFormValid(): void { this.formValid = Boolean( - !this.exercise()?.plagiarismDetectionConfig?.continuousPlagiarismControlEnabled || - (this.fieldThreshhold?.valid && this.fieldMinScore?.valid && this.fieldMinSize?.valid && this.fieldResponsePeriod?.valid), + !this.exercise()?.plagiarismDetectionConfig?.continuousPlagiarismControlEnabled || ( + Boolean(this.fieldThreshhold?.valid) && + Boolean(this.fieldMinScore?.valid) && + Boolean(this.fieldMinSize?.valid) && + Boolean(this.fieldResponsePeriod?.valid) + ) ); this.formValidChanges.next(this.formValid); }
100-176: Consider consolidating update methodsThe update methods follow a repetitive pattern. Consider creating a generic update method to reduce code duplication.
private updateConfig<K extends keyof Exercise['plagiarismDetectionConfig']>( key: K, value: Exercise['plagiarismDetectionConfig'][K] ) { this.exercise.update((exercise) => { if (exercise.plagiarismDetectionConfig) { exercise.plagiarismDetectionConfig[key] = value; } return exercise; }); } // Usage example: updateSimilarityThreshold(threshold: number) { this.updateConfig('similarityThreshold', threshold); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/webapp/app/exercises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.component.ts(4 hunks)src/test/javascript/spec/component/plagiarism/plagiarism-cases-instructor-view.component.spec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/javascript/spec/component/plagiarism/plagiarism-cases-instructor-view.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/exercises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.component.ts (1)
🔇 Additional comments (2)
src/main/webapp/app/exercises/shared/plagiarism/exercise-update-plagiarism/exercise-update-plagiarism.component.ts (2)
1-17: LGTM: Clean migration to standalone component
The imports are well-organized and the standalone component configuration is properly set up with the necessary imports.
74-80: Avoid non-null assertions with optional chaining
This is a duplicate of a previous review comment about safely handling null checks.
AjayvirS
force-pushed
the
chore/plagiarism-checks/client-migration
branch
from
9f5b0d8 to
6a5c5de
Compare
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
Checklist
General
Client
Motivation and Context
The following files are to be migrated in this PR:
The latest version can be found here: https://confluence.ase.in.tum.de/spaces/ArTEMiS/pages/236914348/Plagiarism+Checks+Client+Migration+%E2%86%92+Team+Plagarism
Description
In this PR we migrate the above-listed components to Angular 17+ and also adapt the tests s.t. they adhere to the newer constructs.
Note: Files 9/10 and 10/11 signal migration is not part of this PR due to failing tests. Further inspection is needed to migrate the signals
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Summary by CodeRabbit
Here are the release notes for the pull request:
Refactoring
inject()function.Performance
Technical Improvements