Plagiarism checks: Add direct link to plagiarism detection case view
#9747
Conversation
github-actions
bot
added
tests
client
AjayvirS
changed the title
AjayvirS
changed the title
AjayvirS
changed the title
Development: Add direct link to plagiarism detection case view
HawKhiem
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
eceeeren
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
eceeeren
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
.../app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.scss
Outdated
Show resolved
Hide resolved
![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 (2)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (1)
24-26: Add JSDoc comment to explain the purpose of the property.The comment explains why the property is needed, but it should be in JSDoc format for better documentation.
- // method called as html template variable, angular only recognises reference variables in html if they are a property - // of the corresponding component class - getExerciseUrlSegment = getExerciseUrlSegment; + /** + * Method called as html template variable. + * Angular only recognizes reference variables in html if they are a property of the corresponding component class. + */ + getExerciseUrlSegment = getExerciseUrlSegment;src/test/javascript/spec/component/plagiarism/plagiarism-cases-instructor-view.component.spec.ts (1)
36-36: Consider extracting test fixtures into a separate fileWhile the test data is well-structured, consider moving the mock data (exercises, submissions, and cases) into a separate test fixtures file to improve maintainability and reusability across test files.
Example structure:
// src/test/javascript/spec/fixtures/plagiarism-cases.fixtures.ts export const mockExercises = { exercise1: { id: 1, title: 'Test Exercise 1', type: ExerciseType.TEXT, } as TextExercise, // ... other exercises }; export const mockPlagiarismSubmissions = { submission1: { id: 1, studentLogin: 'studentA', } as PlagiarismSubmission<TextSubmissionElement>, // ... other submissions };Also applies to: 45-57, 77-77
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.html(3 hunks)src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts(3 hunks)src/main/webapp/i18n/de/plagiarism.json(1 hunks)src/main/webapp/i18n/en/plagiarism.json(1 hunks)src/test/javascript/spec/component/plagiarism/plagiarism-cases-instructor-view.component.spec.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.html
🧰 Additional context used
📓 Path-based instructions (3)
src/main/webapp/i18n/de/plagiarism.json (1)
Pattern src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
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/instructor-view/plagiarism-cases-instructor-view.component.ts (1)
🪛 eslint
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
[error] 194-194: Insert ⏎
(prettier/prettier)
🔇 Additional comments (7)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (2)
21-21: LGTM! Type safety improvement.
The change from any to GroupedPlagiarismCases type improves type safety and code maintainability.
171-193: LGTM! Well-structured grouping logic with proper type safety.
The new method effectively groups plagiarism cases by exercise while maintaining type safety. The implementation:
- Uses proper TypeScript type annotations
- Handles undefined exercise IDs gracefully
- Initializes groups and maintains the exercise list
- Uses reduce for efficient grouping
Let's verify the usage of this grouping method:
✅ Verification successful
Grouping logic is correctly implemented and properly used
The verification confirms:
- The method is used in the component to populate
groupedPlagiarismCases - The
GroupedPlagiarismCasesinterface is properly defined in a dedicated types file with correct typing ([exerciseId: number]: PlagiarismCase[]) - The implementation aligns with the interface definition and its usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of getGroupedPlagiarismCasesByExercise method
# and ensure it's called appropriately
# Search for method calls
rg "getGroupedPlagiarismCasesByExercise" -A 2
# Search for GroupedPlagiarismCases type usage
ast-grep --pattern 'interface GroupedPlagiarismCases {
$$$
}'
Length of output: 1584
src/main/webapp/i18n/en/plagiarism.json (1)
117-118: LGTM! Clear and consistent translations.
The new translations "studentNotified" and "viewComparisons" are clear, concise, and follow the existing translation patterns.
src/main/webapp/i18n/de/plagiarism.json (1)
117-118: LGTM! Translations follow German language guidelines.
The new translations:
- Use informal style (dutzen) as required
- Maintain consistency with English translations
- Follow natural German language patterns
src/test/javascript/spec/component/plagiarism/plagiarism-cases-instructor-view.component.spec.ts (3)
4-4: LGTM: Import statements are well-organized and necessary
The new imports are properly organized and support the added routing functionality testing.
Also applies to: 14-25
106-106: LGTM: Test configuration follows Angular testing best practices
The test module setup properly configures the necessary dependencies, mock components, and services for isolated component testing.
Also applies to: 110-120
232-243: LGTM with suggestions for additional test coverage
The navigation test is well-structured but could be enhanced with additional test cases:
- Navigation with invalid exercise ID
- Navigation when course ID is missing
- Error handling scenarios
Let's verify the test coverage for navigation-related code:
.../app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (1)
24-26: Enhance the comment to be more specific about Angular's template behaviorWhile the implementation is correct, the comment could be more specific about why Angular needs this pattern. Consider updating it to:
-// method called as html template variable, angular only recognises reference variables in html if they are a property -// of the corresponding component class +// Angular's template engine requires methods used in templates to be properties of the component class +// This allows the template to maintain a stable reference to the getExerciseUrlSegment function
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.html(3 hunks)src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.html
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (1)
🔇 Additional comments (4)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (4)
6-6: LGTM: Import and decorator changes follow Angular best practices
The new imports and styleUrls addition are well-structured and follow Angular's style guide.
Also applies to: 9-9, 15-15
21-21: Great improvement: Enhanced type safety with GroupedPlagiarismCases
The change from any to a specific type improves type safety and code maintainability.
48-48: LGTM: Good separation of concerns
Moving the grouping logic to a dedicated method improves code organization and readability.
6-6: Verify the implementation meets the PR requirements
The changes support adding direct links to the plagiarism detection view by providing URL generation capabilities. However, let's verify that all necessary components are in place.
Also applies to: 26-26
✅ Verification successful
Implementation aligns with PR requirements
The changes support the PR objectives by correctly implementing direct links to plagiarism detection case views:
- The
getExerciseUrlSegmentfunction is properly implemented inexercise.model.tsto generate correct URL segments based on exercise type - The template correctly uses the function in
routerLinkto create navigation links to plagiarism views - The component properly imports and exposes the function for template usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the template uses the URL generation and proper routing
# Check if the template uses getExerciseUrlSegment for routing
rg -l "getExerciseUrlSegment" "src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.html"
# Check if there are any tests covering the navigation
rg -l "routerLink.*getExerciseUrlSegment" "src/test/javascript/spec/component/plagiarism/**/*spec.ts"
Length of output: 447
Script:
#!/bin/bash
# Let's verify the implementation more thoroughly
# Check the actual usage of getExerciseUrlSegment in the template
rg -A 5 "getExerciseUrlSegment" "src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.html"
# Check if there are any tests in a broader scope
fd ".*spec.ts$" src/test/javascript/spec
# Check the implementation of getExerciseUrlSegment
rg -A 5 "getExerciseUrlSegment" "src/main/webapp/app/entities/exercise.model.ts"
# Check the component implementation
rg -A 5 "getExerciseUrlSegment" "src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts"
Length of output: 66351
Script:
#!/bin/bash
# Let's check the implementation of getExerciseUrlSegment and its usage
# Check the implementation of getExerciseUrlSegment in exercise.model.ts
ast-grep --pattern 'export function getExerciseUrlSegment($_) { $$$ }'
# Check the component implementation for routing usage
rg -A 10 "getExerciseUrlSegment" "src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts"
# Check if there are any plagiarism-related test files in a different location
fd "plagiarism.*spec.ts$" src/test/javascript
Length of output: 2444
Script:
#!/bin/bash
# Let's check the test implementation and exercise model
# Check the plagiarism test implementation for routing tests
rg -A 10 "getExerciseUrlSegment" "src/test/javascript/spec/component/plagiarism/plagiarism-cases-instructor-view.component.spec.ts"
# Check the exercise model implementation since ast-grep didn't find it
rg -A 5 "export function getExerciseUrlSegment" "src/main/webapp/app/entities/exercise.model.ts"
# Check the template usage in more detail
rg -B 2 -A 2 "getExerciseUrlSegment" "src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.html"
Length of output: 1050
...pp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (1)
🪛 Biome (1.9.4)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
[error] 179-179: Expected an expression but instead found 'case => case.exercise'.
Expected an expression here.
(parse)
🪛 GitHub Check: client-compilation
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
🪛 GitHub Check: client-tests-selected
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
🪛 GitHub Check: client-tests
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
🔇 Additional comments (6)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (6)
6-6: LGTM: Import additions enhance type safety and support new linking functionality
The new imports properly support the PR objectives by providing necessary types and utilities for direct linking to plagiarism detection.
Also applies to: 9-9
15-15: LGTM: Component styling follows Angular best practices
The addition of styleUrls properly separates styling concerns into a dedicated SCSS file.
21-21: LGTM: Type safety improvement
Replacing any with GroupedPlagiarismCases enhances type safety and maintainability.
24-26: Consider enhancing the documentation
While the comment explains why the property is needed, it could be more specific about Angular's template binding mechanism.
Consider this improved documentation:
-// method called as html template variable, angular only recognises reference variables in html if they are a property
-// of the corresponding component class
+/**
+ * Exposes the getExerciseUrlSegment utility for template usage.
+ * Angular requires template-referenced functions to be class properties
+ * to enable proper binding and change detection.
+ */171-197: LGTM: Well-structured grouping implementation
The implementation:
- Pre-collects exercises to avoid side effects
- Includes proper null safety checks
- Uses type predicates for filtering
- Follows functional programming practices
This aligns well with the PR objectives by preparing data for the plagiarism detection view.
🧰 Tools
🪛 Biome (1.9.4)
[error] 179-179: Expected an expression but instead found 'case => case.exercise'.
Expected an expression here.
(parse)
🪛 GitHub Check: client-compilation
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
🪛 GitHub Check: client-tests-selected
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
🪛 GitHub Check: client-tests
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
[failure] 179-179:
Argument expression expected.
26-26: Verify template implementation of plagiarism detection links
Let's verify that the template properly utilizes the getExerciseUrlSegment function for navigation.
✅ Verification successful
Template implementation of plagiarism detection links is correct
The verification shows proper implementation:
- The
getExerciseUrlSegmentfunction is correctly used in both template files for navigation - In the instructor view, it's used to create links to plagiarism detection via
[routerLink]="['/course-management', courseId, getExerciseUrlSegment(exercise.type), exercise.id, 'plagiarism']" - In the detail view, it's used similarly for exercise navigation and plagiarism detection links in the dropdown menu
- The routing structure is consistent with the URL pattern used in the templates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify template usage of getExerciseUrlSegment and proper routing setup
# Check template implementation
rg -l "getExerciseUrlSegment" --type html
# Verify router configuration includes plagiarism detection routes
rg -l "plagiarism.*detection" --type ts | grep -i "routing"
Length of output: 335
Script:
#!/bin/bash
# Let's check the actual template implementation to verify proper usage
cat src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.html
# Also check the detail view template
cat src/main/webapp/app/course/plagiarism-cases/instructor-view/detail-view/plagiarism-case-instructor-detail-view.component.html
# Let's also verify the routing setup more broadly
rg -l "plagiarism" --type ts | grep -i "route"
Length of output: 18347
...pp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (1)
179-179: Fix arrow function parentheses to match style guidelinesThe static analysis tools indicate a style violation.
- .map(plagiarismCase => plagiarismCase.exercise) + .map((plagiarismCase) => plagiarismCase.exercise)🧰 Tools
🪛 eslint
[error] 179-179: Replace
plagiarismCasewith(plagiarismCase)(prettier/prettier)
🪛 GitHub Check: client-style
[failure] 179-179:
ReplaceplagiarismCasewith(plagiarismCase)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (1)
🪛 eslint
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
[error] 179-179: Replace plagiarismCase with (plagiarismCase)
(prettier/prettier)
🪛 GitHub Check: client-style
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
[failure] 179-179:
Replace plagiarismCase with (plagiarismCase)
🔇 Additional comments (6)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (6)
6-6: LGTM: Import changes align with new functionality
The new imports support URL generation for exercises and improve type safety through the GroupedPlagiarismCases type.
Also applies to: 9-9
15-15: LGTM: Component styling follows Angular best practices
The addition of styleUrls properly encapsulates component-specific styles.
21-21: LGTM: Improved type safety for grouped cases
The explicit typing with GroupedPlagiarismCases enhances code reliability and maintainability.
24-26: Consider using a proper getter method instead of property assignment
While the comment explains why the property assignment is needed, a getter method would be a cleaner approach and more aligned with Angular best practices.
- // method called as html template variable, angular only recognises reference variables in html if they are a property
- // of the corresponding component class
- getExerciseUrlSegment = getExerciseUrlSegment;
+ getExerciseUrlSegment(exercise: Exercise): string {
+ return getExerciseUrlSegment(exercise);
+ }48-48: LGTM: Improved code organization
Moving the grouping logic to a dedicated method enhances maintainability and readability.
171-197: LGTM: Well-implemented grouping logic with proper safety checks
The implementation includes:
- Pre-collection of exercises to avoid side effects
- Proper null safety checks
- Clear separation of group initialization and assignment
🧰 Tools
🪛 eslint
[error] 179-179: Replace plagiarismCase with (plagiarismCase)
(prettier/prettier)
🪛 GitHub Check: client-style
[failure] 179-179:
Replace plagiarismCase with (plagiarismCase)
sachmii
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Client
Motivation and Context
Fixes #5974 .
In plagiarism cases' page should be a link to view directly the plagiarism detection for the exercises. This issue adds a link for every plagiarism case to the plagiarism detection view for that exercise.
Description
A routerLink was added to the plagiarism detection view, enabling navigation to the respective course and exercise based on their IDs. A new class was introduced to group plagiarism cases by exercise, improving clarity and organization. Additionally, tests were implemented to verify the functionality of the routerLink.
Steps for Testing
Prerequisites:
Testing:
3. Click on the linked "Appears in x comparison(s)" text*Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Style
Tests
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Tests
Documentation