Plagiarism checks: Enhance navigation to plagiarism cases from detection page
#10078
Conversation
github-actions
bot
added
tests
client
WalkthroughThis pull request introduces enhancements to the plagiarism cases management interface in the Artemis application. The changes focus on improving the user experience by adding dynamic scrolling to specific exercise elements with plagiarism cases and modifying the plagiarism header component's button rendering. The modifications include updating the HTML structure, adding a scrolling method in the TypeScript component, updating localization files, and adding corresponding test cases to verify the new functionality. Changes
Sequence DiagramsequenceDiagram
participant User
participant Component as PlagiarismCasesInstructorView
participant Router
User->>Component: Navigate to page
Component->>Router: Get exercise ID from query params
Router-->>Component: Return exercise ID
Component->>Component: Set exerciseId
Component->>Component: Scroll to specific exercise element
Possibly related PRs
Suggested Labels
Suggested Reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 1
🧹 Nitpick comments (3)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (3)
18-18: Use inline documentation to clarify purpose of exerciseId.
It's helpful to describe why we store exerciseId, especially since it’s only optionally defined.
23-24: Ensure consistency in variable naming.
“exerciseWithPlagCasesElements” is descriptive but quite long. If used in multiple places, consider consolidating or making the name more concise.
43-43: Initialize exerciseId conditionally if query parameter is absent.
Consider checking if the query parameter is defined before assigning.- this.exerciseId = Number(this.route.snapshot.queryParamMap?.get('exerciseId')); + const exerciseQueryParam = this.route.snapshot.queryParamMap?.get('exerciseId'); + if (exerciseQueryParam) { + this.exerciseId = Number(exerciseQueryParam); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.html(1 hunks)src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts(3 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.component.html(1 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(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
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/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/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.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/instructor-view/plagiarism-cases-instructor-view.component.ts (1)
🔇 Additional comments (9)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.component.html (2)
10-12: LGTM! Modern Angular syntax used correctly.
The conditional rendering uses the new @if syntax as per coding guidelines, replacing the older *ngIf directive.
13-20: Verify the router navigation parameters.
The implementation looks good, but let's ensure the navigation parameters are properly handled when exercise.course?.id is undefined.
Run this script to check for proper error handling of undefined course IDs:
✅ Verification successful
Based on the search results, I can now generate the final response:
The router navigation with optional course ID is consistently handled across the codebase
The search results show that the pattern exercise.course?.id is used consistently throughout the application in router navigation. In cases where the course ID might be undefined:
- The optional chaining operator (
?.) is properly used - Some components even have fallback paths (e.g., in example-modeling-submission.component.ts:
exercise.course?.id || exercise.exerciseGroup?.exam?.course?.id) - The Angular router handles undefined segments gracefully by design
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar router navigation patterns to verify consistent handling
ast-grep --pattern '[routerLink]="[_, $course, _]"'
# Check for any error handling patterns around course navigation
rg -A 3 'exercise\.course\?\.id'
Length of output: 6971
src/main/webapp/i18n/en/plagiarism.json (1)
6-6: LGTM! Clear and consistent translation.
The English translation "View Case(s)" is clear, properly handles pluralization, and maintains consistent capitalization with other UI elements.
src/main/webapp/i18n/de/plagiarism.json (1)
6-6: LGTM! Translation follows German language guidelines.
The German translation "Fälle ansehen" is appropriate and follows the informal language requirement (dutzen) as per coding guidelines.
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (2)
2-2: Consider verifying the compatibility of 'effect' and 'viewChildren' usage across Angular versions.
These are relatively new features in Angular signals. Ensure the application’s Angular version supports them and there are no browser compatibility issues.
✅ Verification successful
Let me gather information about the Angular version and usage of these features in the codebase.
The usage of 'effect' and 'viewChildren' is compatible with the current Angular version
The codebase is using Angular v18.2.13, which fully supports both the effect and viewChildren signals features. The search results show these features are widely used across multiple components in the application:
effectis used in 45 different filesviewChildrenis used in 3 different files
This consistent usage pattern and the modern Angular version indicate there are no compatibility concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check package.json for Angular version
cat package.json | jq '.dependencies["@angular/core"]'
# Search for other usages of effect and viewChildren
rg "import.*effect.*from '@angular/core'" -g '*.ts'
rg "import.*viewChildren.*from '@angular/core'" -g '*.ts'
# Check if there are any browser compatibility configurations
fd "browserslist" --type f
cat browserslist 2>/dev/null || true
Length of output: 11094
32-38: Validate that the reactive effect is unsubscribed or cleaned up if needed.
Using an effect in a constructor can lead to potential memory leaks if the component is destroyed frequently. Make sure Angular’s built-in signals or the environment handle this automatically.
✅ Verification successful
Let me gather more information about how the effect is being used and if there are any cleanup mechanisms in place.
Let me search for more specific information about the effect usage and Angular's signal implementation.
Effects in Angular 18+ are automatically cleaned up
The code is using Angular 18.2.13, which includes the new signals and effects system. Angular's effect() automatically handles cleanup when the component is destroyed. The effect is being used correctly in the constructor to handle scrolling when exerciseId changes, and there's no need for manual cleanup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for the component class implementation to understand the context
ast-grep --pattern 'export class PlagiarismCasesInstructorViewComponent {
$$$
}'
# Search for any cleanup methods or ngOnDestroy implementation
ast-grep --pattern 'ngOnDestroy() {
$$$
}'
# Look for any signal or effect related imports
rg "import.*effect" src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
Length of output: 468
Script:
#!/bin/bash
# Search for the entire file content to understand the component implementation
cat src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
# Search for any Angular documentation or configuration files that might indicate the version
rg "\"@angular/core\":" package.json
Length of output: 9083
src/test/javascript/spec/component/plagiarism/plagiarism-cases-instructor-view.component.spec.ts (2)
203-222: Great test coverage for scrollToExercise.
Verifies that the correct element is scrolled into view. This ensures robust coverage of the new logic.
19-19: Check if signal usage requires additional test bed configuration.
Signals in Angular might need certain compiler settings. Confirm your test environment is configured properly for the new Angular APIs.
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.html (1)
14-14: Excellent approach to dynamic element referencing.
The addition of the ID and template reference variable ensures that scrollToExercise can target the correct element. This aligns with Angular’s recommended patterns for interactive UIs.
| scrollToExercise() { | ||
| const element = this.exerciseWithPlagCasesElements().find((elem) => elem.nativeElement.id === 'exercise-with-plagiarism-case-' + this.exerciseId); | ||
| if (element) { | ||
| element.nativeElement.scrollIntoView({ | ||
| behavior: 'smooth', | ||
| block: 'start', | ||
| inline: 'nearest', | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now provide a final response as we've found evidence of fixed headers in the application:
Consider adding offset for fixed navbar when scrolling
The application has a fixed navbar with a height of 68px ($header-height: 68px) and uses sticky positioning. To ensure the target element is not hidden behind the navbar, add an offset:
- Add a scroll margin to the target element:
element.nativeElement.style.scrollMarginTop = '70px'; // slightly more than header height
element.nativeElement.scrollIntoView({
behavior: 'smooth',
block: 'start',
inline: 'nearest',
});🔗 Analysis chain
Validate offset handling for scrollIntoView if headers or sticky toolbars exist.
If there is a top navbar or sticky element, consider adjusting the scroll position to ensure the target element is fully visible.
element.nativeElement.scrollIntoView({
behavior: 'smooth',
block: 'start',
+ // potential offset logic if a fixed header obstructs the view
});Let me search for any fixed headers or navigation bars in the codebase that might affect the scrolling behavior.
Let me search specifically for the navbar component and its styling to verify if there are fixed headers that might affect scrolling.
Let me search for any global styles that might affect the navbar or header positioning.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for navigation components and fixed/sticky positioning
rg -l "position:\s*(fixed|sticky)" --type html
rg "navbar" --type html -l
rg "header" --type html -l
# Search for any CSS classes that might define fixed positioning
rg "\.fixed|\.sticky" --type css --type scss -l
# Look for navigation component definitions
ast-grep --pattern 'class $_ implements OnInit {
$$$
// Look for navbar/header related component
$$$
}'
Length of output: 23385
Script:
#!/bin/bash
# Check navbar component implementation and its styling
cat src/main/webapp/app/shared/layouts/navbar/navbar.component.html
rg "position:\s*(fixed|sticky)" src/main/webapp/app/shared/layouts/navbar/navbar.component.html
rg "position:\s*(fixed|sticky)" src/main/webapp/content/scss/global.scss
Length of output: 20889
Script:
#!/bin/bash
# Search for navbar and header related styles in all scss files
fd -e scss -x rg "navbar|header" {} \;
fd -e scss -x rg "position:\s*(fixed|sticky)" {} \;
# Also check for any global styles that might affect scrolling
fd -e scss -x rg "scroll|overflow" {} \;
Length of output: 22543
AjayvirS
changed the title
Plagiarism checks: Enhance nvaigation to plagiarism cases from detection pagePlagiarism checks: Enhance navigation to plagiarism cases from detection page
Checklist
General
Server
Client
Motivation and Context
The ‘Confirm’ button does not directly send a notification to the students. That has to be done manually from the ‘Plagiarism Cases‘ page. However this is not very clear from the UI
Navigating to that one has to be done via
The change replaces the Confirm button with "View Cases" which leads you to the cases of the exercise and scrolls to it if need be.
Description
Added a routerlink to plagiarism detection page. Also added a scrollIntoView to scroll to the exercise table if it is necessary.
Steps for Testing
Prerequisites:
Exam Mode Testing
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
Exam Mode Test
Performance Tests
Test Coverage
Screenshots
After pressing on Confirm:
Summary by CodeRabbit
New Features
Localization
Bug Fixes
Tests