Development: Migrate suspicious behavior module to new client coding guidelines
#9887
Conversation
github-actions
bot
added
tests
client
coolchock
changed the title
Chore: Migrate suspicious behavior module to signals, inject and standaloneGeneral: Migrate suspicious behavior module to signals, inject and standalone
coolchock
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThis pull request introduces significant changes to the Angular components related to suspicious behavior management. The modifications primarily involve transitioning from direct property access to function calls for retrieving data and checking conditions in both HTML templates and component classes. Additionally, the dependency injection method has been updated to use Angular's Changes
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Outside diff range and nitpick comments (19)
src/main/webapp/app/exam/manage/suspicious-behavior/suspicious-sessions/suspicious-sessions.component.html (1)
Line range hint
1-14: Review null handling approach.The template shows inconsistent null handling approaches:
- Non-null assertions (
!) are used forsession.studentExam!- Optional chaining (
?.) is used forsession.studentExam?.user?.loginConsider using optional chaining consistently throughout the template to handle potential null values safely.
- <a [routerLink]="getStudentExamLink(session.studentExam!)"> - {{ session.studentExam!.id }} + <a [routerLink]="getStudentExamLink(session.studentExam)"> + {{ session.studentExam?.id }}src/main/webapp/app/exam/manage/suspicious-behavior/plagiarism-cases-overview/plagiarism-cases-overview.component.ts (2)
12-17: Add JSDoc comments to document input properties.While the input properties are well-typed, adding documentation would improve maintainability and make the purpose of each input clear to other developers.
Consider adding JSDoc comments like this:
+ /** + * List of exercises to check for plagiarism + */ exercises = input.required<Exercise[]>(); + /** + * Maps exercises to their number of plagiarism cases + */ plagiarismCasesPerExercise = input.required<Map<Exercise, number>>();
Line range hint
19-36: Consider extracting route paths to constants.While the navigation logic is correct, hardcoded route paths could be difficult to maintain. Consider extracting these paths to route constants.
Example refactor:
+const ROUTES = { + COURSE_MANAGEMENT: '/course-management', + EXAMS: 'exams', + PLAGIARISM: 'plagiarism', + PLAGIARISM_CASES: 'plagiarism-cases', + EXERCISE_GROUPS: 'exercise-groups', +} as const; goToPlagiarismDetection(exercise: Exercise) { const exerciseGroupId = exercise.exerciseGroup?.id; const exerciseType = exercise.type; this.router.navigate([ - '/course-management', + ROUTES.COURSE_MANAGEMENT, this.courseId(), - 'exams', + ROUTES.EXAMS, // ... rest of the path ]); }src/main/webapp/app/exam/manage/suspicious-behavior/suspicious-sessions.service.ts (2)
10-10: Consider adding readonly modifier to http propertyThe migration to inject() function is good, but the http property could be made readonly since it's only assigned once during initialization.
- private http = inject(HttpClient); + private readonly http = inject(HttpClient);
Fix incorrect parameter mapping for browser fingerprint detection
The review comment is correct. The code analysis reveals a critical bug where
sameStudentExamDifferentBrowserFingerprintsparameter is incorrectly mapped tooptions.differentIpAddressesSameStudentExaminstead ofoptions.differentBrowserFingerprintsSameStudentExam. This is confirmed by:
- The model definition in
exam-session.model.tsshows these are separate properties- The test cases in
suspicious-sessions.service.spec.tsexpect both parameters to be included correctly- The component usage in
suspicious-behavior.component.tspasses these as distinct valuesThe suggested fix in the review comment is accurate and should be applied:
- .set('sameStudentExamDifferentBrowserFingerprints', options.differentIpAddressesSameStudentExam.toString()) + .set('sameStudentExamDifferentBrowserFingerprints', Boolean(options.differentBrowserFingerprintsSameStudentExam).toString())Additionally, the explicit Boolean conversion suggestion is also valid for type safety.
🔗 Analysis chain
Line range hint
12-24: Fix incorrect parameter mapping and enhance type safetyThere's a critical issue in the parameter mapping and some potential improvements:
- The 'sameStudentExamDifferentBrowserFingerprints' parameter is using the wrong option value (it's using differentIpAddressesSameStudentExam instead of differentBrowserFingerprintsSameStudentExam)
- Consider using explicit boolean conversions for type safety
Apply this fix:
getSuspiciousSessions(courseId: number, examId: number, options: SuspiciousSessionsAnalysisOptions): Observable<SuspiciousExamSessions[]> { let params = new HttpParams() - .set('differentStudentExamsSameIPAddress', options.sameIpAddressDifferentStudentExams.toString()) - .set('differentStudentExamsSameBrowserFingerprint', options.sameBrowserFingerprintDifferentStudentExams.toString()) - .set('sameStudentExamDifferentIPAddresses', options.differentIpAddressesSameStudentExam.toString()) - .set('sameStudentExamDifferentBrowserFingerprints', options.differentIpAddressesSameStudentExam.toString()) - .set('ipOutsideOfRange', options.ipAddressOutsideOfRange.toString()); + .set('differentStudentExamsSameIPAddress', Boolean(options.sameIpAddressDifferentStudentExams).toString()) + .set('differentStudentExamsSameBrowserFingerprint', Boolean(options.sameBrowserFingerprintDifferentStudentExams).toString()) + .set('sameStudentExamDifferentIPAddresses', Boolean(options.differentIpAddressesSameStudentExam).toString()) + .set('sameStudentExamDifferentBrowserFingerprints', Boolean(options.differentBrowserFingerprintsSameStudentExam).toString()) + .set('ipOutsideOfRange', Boolean(options.ipAddressOutsideOfRange).toString());Let's verify the usage of this service to ensure the parameter mapping is consistent:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to these properties to verify correct usage rg -t typescript "differentBrowserFingerprintsSameStudentExam|differentIpAddressesSameStudentExam" --no-headingLength of output: 148
Script:
#!/bin/bash # Search for the interface definition and usage rg "SuspiciousSessionsAnalysisOptions|differentBrowserFingerprintsSameStudentExam|differentIpAddressesSameStudentExam" -t ts --no-heading -A 5Length of output: 12975
Script:
#!/bin/bash # Search for any suspicious session related files fd -e ts suspiciousLength of output: 844
Script:
#!/bin/bash # Look for the interface definition in the suspicious-sessions file cat src/main/webapp/app/exam/manage/suspicious-behavior/suspicious-sessions.service.tsLength of output: 1509
src/main/webapp/app/exam/manage/suspicious-behavior/plagiarism-cases-overview/plagiarism-cases-overview.component.html (2)
17-18: Consider performance optimization for function callsThe template now uses function calls instead of property access. While this aligns with the new implementation, these functions will be called during every change detection cycle.
Consider implementing trackBy or memoization if these functions perform expensive computations:
// In the component class private memoizedResults = memoize(() => this.plagiarismResultsPerExercise()); private memoizedCases = memoize(() => this.plagiarismCasesPerExercise());Then in the template:
-<td>{{ plagiarismResultsPerExercise().get(exercise) }}</td> -<td>{{ plagiarismCasesPerExercise().get(exercise) }}</td> +<td>{{ memoizedResults().get(exercise) }}</td> +<td>{{ memoizedCases().get(exercise) }}</td>
31-31: LGTM! Correct usage of new Angular @if syntaxThe implementation correctly uses the new @if syntax. Consider memoizing the anyPlagiarismCases() function if it performs complex calculations.
// In the component class private memoizedAnyPlagiarismCases = memoize(() => this.anyPlagiarismCases());src/main/webapp/app/exam/manage/suspicious-behavior/suspicious-sessions/suspicious-sessions.component.ts (2)
Line range hint
13-24: Consider using computed signals for derived stateWhile the migration to input signals is good, the boolean flags could be more reactive using computed signals instead of being set once in
ngOnInit.Consider this refactoring:
- suspiciousFingerprint = false; - suspiciousIpAddress = false; + suspiciousFingerprint = computed(() => + this.isSuspiciousFor(SuspiciousSessionReason.DIFFERENT_STUDENT_EXAMS_SAME_BROWSER_FINGERPRINT) || + this.isSuspiciousFor(SuspiciousSessionReason.SAME_STUDENT_EXAM_DIFFERENT_BROWSER_FINGERPRINTS) + ); + suspiciousIpAddress = computed(() => + this.isSuspiciousFor(SuspiciousSessionReason.DIFFERENT_STUDENT_EXAMS_SAME_IP_ADDRESS) || + this.isSuspiciousFor(SuspiciousSessionReason.SAME_STUDENT_EXAM_DIFFERENT_IP_ADDRESSES) || + this.isSuspiciousFor(SuspiciousSessionReason.IP_ADDRESS_OUTSIDE_OF_RANGE) + ); - ngOnInit(): void { - this.suspiciousFingerprint = - this.isSuspiciousFor(SuspiciousSessionReason.DIFFERENT_STUDENT_EXAMS_SAME_BROWSER_FINGERPRINT) || - this.isSuspiciousFor(SuspiciousSessionReason.SAME_STUDENT_EXAM_DIFFERENT_BROWSER_FINGERPRINTS); - this.suspiciousIpAddress = - this.isSuspiciousFor(SuspiciousSessionReason.DIFFERENT_STUDENT_EXAMS_SAME_IP_ADDRESS) || - this.isSuspiciousFor(SuspiciousSessionReason.SAME_STUDENT_EXAM_DIFFERENT_IP_ADDRESSES) || - this.isSuspiciousFor(SuspiciousSessionReason.IP_ADDRESS_OUTSIDE_OF_RANGE); - }
Line range hint
26-31: Consider adding URL parameter validationThe URL construction could be more robust by validating the presence of required parameters.
Consider this enhancement:
getStudentExamLink(studentExam: StudentExam) { const studentExamId = studentExam.id; const courseId = studentExam.exam?.course?.id; const examId = studentExam.exam?.id; + if (!courseId || !examId || !studentExamId) { + console.warn('Missing required parameters for student exam link'); + return ''; + } return `/course-management/${courseId}/exams/${examId}/student-exams/${studentExamId}`; }src/test/javascript/spec/component/exam/manage/suspicious-behavior/suspicious-sessions.component.spec.ts (2)
Line range hint
31-33: Add type annotation for suspiciousSessions3For consistency and type safety, add the
SuspiciousExamSessionstype annotation.const suspiciousSessions3 = { examSessions: [ { suspiciousReasons: [SuspiciousSessionReason.IP_ADDRESS_OUTSIDE_OF_RANGE], }, ], - }; + } as SuspiciousExamSessions;
Line range hint
53-67: Refactor test for better organization and clarityWhile the test logic is correct, it could be better organized:
- Split into separate test cases for each scenario
- Add more descriptive test names
- Add cleanup between tests
Consider refactoring like this:
- it('should correctly determine suspicious reasons', () => { - fixture.componentRef.setInput('suspiciousSessions', suspiciousSessions1); - component.ngOnInit(); - expect(component.suspiciousFingerprint).toBeTrue(); - expect(component.suspiciousIpAddress).toBeTrue(); - - fixture.componentRef.setInput('suspiciousSessions', suspiciousSessions2); - component.ngOnInit(); - expect(component.suspiciousFingerprint).toBeFalse(); - expect(component.suspiciousIpAddress).toBeFalse(); - fixture.componentRef.setInput('suspiciousSessions', suspiciousSessions3); - component.ngOnInit(); - expect(component.suspiciousFingerprint).toBeFalse(); - expect(component.suspiciousIpAddress).toBeTrue(); - }); + describe('suspicious reasons detection', () => { + beforeEach(() => { + component.suspiciousFingerprint = false; + component.suspiciousIpAddress = false; + }); + + it('should detect both fingerprint and IP address issues', () => { + fixture.componentRef.setInput('suspiciousSessions', suspiciousSessions1); + component.ngOnInit(); + expect(component.suspiciousFingerprint).toBeTrue(); + expect(component.suspiciousIpAddress).toBeTrue(); + }); + + it('should detect no issues when no suspicious reasons exist', () => { + fixture.componentRef.setInput('suspiciousSessions', suspiciousSessions2); + component.ngOnInit(); + expect(component.suspiciousFingerprint).toBeFalse(); + expect(component.suspiciousIpAddress).toBeFalse(); + }); + + it('should detect only IP address issues', () => { + fixture.componentRef.setInput('suspiciousSessions', suspiciousSessions3); + component.ngOnInit(); + expect(component.suspiciousFingerprint).toBeFalse(); + expect(component.suspiciousIpAddress).toBeTrue(); + }); + });src/test/javascript/spec/component/exam/manage/suspicious-behavior/plagiarism-cases-overview.component.spec.ts (4)
57-61: Add type assertions for Map values to improve type safety.The Maps for
plagiarismCasesPerExerciseandplagiarismResultsPerExerciseshould have explicit type assertions to ensure type safety.-new Map([ - [exercise1, 0], - [exercise2, 1], -]), +new Map<Exercise, number>([ + [exercise1, 0], + [exercise2, 1], +]),Also applies to: 63-68
52-69: Consider extracting test data setup to improve maintainability.The test data initialization could be moved to a separate helper or fixture to improve readability and maintainability. This would make the test setup more concise and reusable.
// Suggested helper function: function setupComponentInputs(fixture: ComponentFixture<PlagiarismCasesOverviewComponent>, testData: { courseId: number; examId: number; exercises: Exercise[]; plagiarismCases: Map<Exercise, number>; plagiarismResults: Map<Exercise, number>; }): void { fixture.componentRef.setInput('courseId', testData.courseId); fixture.componentRef.setInput('examId', testData.examId); fixture.componentRef.setInput('exercises', testData.exercises); fixture.componentRef.setInput('plagiarismCasesPerExercise', testData.plagiarismCases); fixture.componentRef.setInput('plagiarismResultsPerExercise', testData.plagiarismResults); }
74-77: Use recommended spy assertion pattern for navigation test.Following the coding guidelines for spy assertions, use
toHaveBeenCalledExactlyOnceWithfor more precise verification.-expect(router.navigate).toHaveBeenCalledWith(['/course-management', 1, 'exams', 2, 'plagiarism-cases']); +expect(router.navigate).toHaveBeenCalledExactlyOnceWith(['/course-management', 1, 'exams', 2, 'plagiarism-cases']);
Line range hint
74-86: Add test coverage for edge cases.Consider adding test cases for:
- Undefined/null input values
- Empty exercise arrays
- Empty or invalid Maps
Example test case:
it('should handle undefined plagiarism cases gracefully', () => { fixture.componentRef.setInput('plagiarismCasesPerExercise', undefined); fixture.detectChanges(); const viewCasesButton = fixture.debugElement.nativeElement.querySelector('#view-plagiarism-cases-btn'); expect(viewCasesButton).toBeNull(); });src/main/webapp/app/exam/manage/suspicious-behavior/suspicious-behavior.component.ts (4)
17-22: Consider grouping related services for better organizationWhile the migration to
inject()is well implemented, consider grouping related services together for better maintainability:- private suspiciousSessionsService = inject(SuspiciousSessionsService); - private activatedRoute = inject(ActivatedRoute); - private plagiarismCasesService = inject(PlagiarismCasesService); - private examService = inject(ExamManagementService); - private plagiarismResultsService = inject(PlagiarismResultsService); - private router = inject(Router); + // Routing services + private router = inject(Router); + private activatedRoute = inject(ActivatedRoute); + + // Exam services + private examService = inject(ExamManagementService); + private suspiciousSessionsService = inject(SuspiciousSessionsService); + + // Plagiarism services + private plagiarismCasesService = inject(PlagiarismCasesService); + private plagiarismResultsService = inject(PlagiarismResultsService);
Line range hint
72-91: Consider implementing proper subscription cleanupThe
retrievePlagiarismCasesmethod creates multiple subscriptions that should be cleaned up to prevent memory leaks.+ private destroy$ = new Subject<void>(); private retrievePlagiarismCases = () => { this.exercises.forEach((exercise) => { this.plagiarismCasesService.getNumberOfPlagiarismCasesForExercise(exercise).subscribe((res) => { this.plagiarismCasesPerExercise.computeIfAbsent(exercise, () => res); if (res > 0) this.anyPlagiarismCases = true; - }); + }).pipe(takeUntil(this.destroy$)); this.plagiarismResultsService.getNumberOfPlagiarismResultsForExercise(exercise.id!).subscribe((res) => { this.plagiarismResultsPerExercise.computeIfAbsent(exercise, () => res); - }); + }).pipe(takeUntil(this.destroy$)); }); }; + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + }
Line range hint
41-47: Consider extracting IP validation regex to a separate constantThe IP subnet regex pattern is complex and would benefit from being extracted to a separate constant or utility file with detailed documentation.
+// ip-validation.constants.ts +/** + * Regex patterns for IP validation + * IPv4: Matches patterns like '192.168.1.0/24' + * IPv6: Matches patterns like '2001:db8::/32' + * @see https://www.regextester.com/93988 + * @see https://www.regextester.com/93987 + */ +export const IP_SUBNET_REGEX = new RegExp( + '(^([0-9]{1,3}.){3}[0-9]{1,3}(/([0-9]|[1-2][0-9]|3[0-2]))?$)|' + + '(^s*((([0-9A-Fa-f]{1,4}:){7}([0-9A-Fa-f]{1,4}|:))|(([0-9A-Fa-f]{1,4}:){6}(:[0-9A-Fa-f]{1,4}|((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3})|:))|(([0-9A-Fa-f]{1,4}:){5}(((:[0-9A-Fa-f]{1,4}){1,2})|:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3})|:))|(([0-9A-Fa-f]{1,4}:){4}(((:[0-9A-Fa-f]{1,4}){1,3})|((:[0-9A-Fa-f]{1,4})?:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){3}(((:[0-9A-Fa-f]{1,4}){1,4})|((:[0-9A-Fa-f]{1,4}){0,2}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){2}(((:[0-9A-Fa-f]{1,4}){1,5})|((:[0-9A-Fa-f]{1,4}){0,3}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){1}(((:[0-9A-Fa-f]{1,4}){1,6})|((:[0-9A-Fa-f]{1,4}){0,4}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(:(((:[0-9A-Fa-f]{1,4}){1,7})|((:[0-9A-Fa-f]{1,4}){0,5}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:)))(%.+)?s*(\\/([0-9]|[1-9][0-9]|1[0-1][0-9]|12[0-8]))?$' +);Then in the component:
- readonly ipSubnetRegexPattern: RegExp = new RegExp(...); + readonly ipSubnetRegexPattern = IP_SUBNET_REGEX;
Line range hint
92-116: Consider implementing error handling for analyzeSessionsThe error handling in
analyzeSessionsonly sets flags but doesn't inform the user about the failure.analyzeSessions() { const options = new SuspiciousSessionsAnalysisOptions( this.checkboxCriterionDifferentStudentExamsSameIPAddressChecked, this.checkboxCriterionDifferentStudentExamsSameBrowserFingerprintChecked, this.checkboxCriterionSameStudentExamDifferentIPAddressesChecked, this.checkboxCriterionSameStudentExamDifferentBrowserFingerprintsChecked, this.checkboxCriterionIPOutsideOfASpecificRangeChecked, this.ipSubnet, ); this.analyzing = true; this.suspiciousSessionsService.getSuspiciousSessions(this.courseId, this.examId, options).subscribe({ next: (suspiciousSessions) => { this.suspiciousSessions = suspiciousSessions; this.analyzing = false; this.analyzed = true; }, - error: () => { + error: (error) => { this.analyzing = false; this.analyzed = true; + console.error('Failed to analyze sessions:', error); + // Assuming you have an AlertService + this.alertService.error('artemis.examSuspiciousBehavior.analysisFailed'); }, }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
src/main/webapp/app/exam/manage/suspicious-behavior/plagiarism-cases-overview/plagiarism-cases-overview.component.html(2 hunks)src/main/webapp/app/exam/manage/suspicious-behavior/plagiarism-cases-overview/plagiarism-cases-overview.component.ts(3 hunks)src/main/webapp/app/exam/manage/suspicious-behavior/suspicious-behavior.component.ts(2 hunks)src/main/webapp/app/exam/manage/suspicious-behavior/suspicious-sessions.service.ts(1 hunks)src/main/webapp/app/exam/manage/suspicious-behavior/suspicious-sessions/suspicious-sessions.component.html(1 hunks)src/main/webapp/app/exam/manage/suspicious-behavior/suspicious-sessions/suspicious-sessions.component.ts(3 hunks)src/test/javascript/spec/component/exam/manage/suspicious-behavior/plagiarism-cases-overview.component.spec.ts(1 hunks)src/test/javascript/spec/component/exam/manage/suspicious-behavior/suspicious-sessions.component.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
src/main/webapp/app/exam/manage/suspicious-behavior/plagiarism-cases-overview/plagiarism-cases-overview.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/exam/manage/suspicious-behavior/plagiarism-cases-overview/plagiarism-cases-overview.component.ts (1)
src/main/webapp/app/exam/manage/suspicious-behavior/suspicious-behavior.component.ts (1)
src/main/webapp/app/exam/manage/suspicious-behavior/suspicious-sessions.service.ts (1)
src/main/webapp/app/exam/manage/suspicious-behavior/suspicious-sessions/suspicious-sessions.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/exam/manage/suspicious-behavior/suspicious-sessions/suspicious-sessions.component.ts (1)
src/test/javascript/spec/component/exam/manage/suspicious-behavior/plagiarism-cases-overview.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/exam/manage/suspicious-behavior/suspicious-sessions.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}}
🔇 Additional comments (11)
src/main/webapp/app/exam/manage/suspicious-behavior/suspicious-sessions/suspicious-sessions.component.html (2)
1-1: LGTM! Correct usage of new Angular syntax.
The migration to @for syntax from *ngFor is correctly implemented, following the latest Angular guidelines.
3-4: Verify suspicious condition variables.
The template uses suspiciousFingerprint and suspiciousIpAddress for conditional styling, but their source is unclear.
✅ Verification successful
The suspicious condition variables are properly defined and initialized in the component.
The variables suspiciousFingerprint and suspiciousIpAddress are class properties in the suspicious-sessions.component.ts file. They are initialized as false and set in the ngOnInit() lifecycle hook based on the evaluation of specific suspicious session reasons using the isSuspiciousFor() method. The template correctly uses these boolean flags for conditional styling.
The implementation shows:
- Both flags are properly declared as class properties
- Values are computed during component initialization
- Unit tests verify both true and false conditions
- The conditions cover multiple suspicious scenarios for both fingerprints and IP addresses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the source and implementation of suspicious condition variables
# Search for the definition and usage of these variables
ast-grep --pattern 'class $_ {
$$$
suspiciousFingerprint
$$$
}'
ast-grep --pattern 'class $_ {
$$$
suspiciousIpAddress
$$$
}'
# Search for any related methods or computations
rg -A 5 "suspicious(Fingerprint|IpAddress)"
Length of output: 5493
src/main/webapp/app/exam/manage/suspicious-behavior/plagiarism-cases-overview/plagiarism-cases-overview.component.ts (3)
Line range hint 1-9: LGTM! Modern Angular patterns applied correctly.
The component follows Angular best practices with proper imports and modern dependency injection patterns.
10-11: LGTM! Modern dependency injection pattern used.
Using inject() follows Angular's latest best practices for dependency injection.
Line range hint 1-36: LGTM! No memory leak concerns.
The component properly manages resources without any subscriptions or manual event listeners that would require cleanup.
src/main/webapp/app/exam/manage/suspicious-behavior/suspicious-sessions.service.ts (1)
1-2: LGTM: Import changes align with modern Angular practices
The addition of the inject import from '@angular/core' follows Angular's recommended dependency injection patterns.
src/main/webapp/app/exam/manage/suspicious-behavior/plagiarism-cases-overview/plagiarism-cases-overview.component.html (1)
13-13: LGTM! Correct usage of new Angular @for syntax
The implementation correctly uses the new @for syntax with proper tracking and index variable declaration.
src/main/webapp/app/exam/manage/suspicious-behavior/suspicious-sessions/suspicious-sessions.component.ts (2)
Line range hint 1-12: LGTM! Proper migration to signal-based inputs
The import changes align with Angular's modern practices for input signals. The attribute selector, while typically discouraged, is appropriately justified with a comment explaining its necessity for table rendering.
33-35: LGTM! Clean signal usage
The isSuspiciousFor method properly adapts to the new signal-based architecture while maintaining a clean functional implementation.
src/test/javascript/spec/component/exam/manage/suspicious-behavior/suspicious-sessions.component.spec.ts (1)
46-46: LGTM! The change to setInput is correct.
The update to use fixture.componentRef.setInput() aligns with Angular's modern input API.
src/main/webapp/app/exam/manage/suspicious-behavior/suspicious-behavior.component.ts (1)
1-1: LGTM: Clean import of inject
The addition of inject to the imports aligns with Angular's modern dependency injection pattern.
HawKhiem
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
sawys777
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
sawys777
added
deploy:artemis-test5
deploy:artemis-test3
and removed
deploy:artemis-test5
labels
sawys777
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
sawys777
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
changed the title
General: Migrate suspicious behavior module to signals, inject and standaloneGeneral: Migrate suspicious behavior module to new client coding guidelines
krusche
changed the title
General: Migrate suspicious behavior module to new client coding guidelinesDevelopment: Migrate suspicious behavior module to new client coding guidelines
Checklist
General
Client
Description
This pull request migrates suspicious behavior module to signals, inject and standalone.
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
Performance Review
Code Review
Manual Tests
Summary by CodeRabbit
Release Notes
New Features
SuspiciousSessionsOverviewComponentas a standalone component, enhancing its modularity.Bug Fixes
Refactor
Chores
Tests