Plagiarism checks: Change separation operator for plagiarism csv file
#9847
Conversation
github-actions
bot
added
tests
client
AjayvirS
changed the title
Plagiarism checks: fix operator for plagiarism csv file
AjayvirS
changed the title
Plagiarism checks: fix operator for plagiarism csv filePlagiarism checks: change separation operator for plagiarism csv file
AjayvirS
changed the title
Plagiarism checks: change separation operator for plagiarism csv filePlagiarism checks: Change separation operator for plagiarism csv file
AjayvirS
added
component:Plagiarism Detection
small
bugfix
plagiarism
WalkthroughThe changes in this pull request enhance the CSV export functionality within the Changes
Possibly related PRs
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: 2
🧹 Outside diff range and nitpick comments (2)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (1)
Line range hint
136-149: Consider architectural improvements for CSV handling.While the current implementation works, consider these improvements:
- Use a CSV library (e.g.,
papaparse) for more robust CSV handling- Add proper typing for the CSV data structure
- Add error handling for large datasets
- Consider streaming for large exports
Would you like me to provide an example implementation using a CSV library?
src/test/javascript/spec/component/plagiarism/plagiarism-cases-instructor-view.component.spec.ts (1)
193-195: LGTM! Consider adding more comprehensive test cases.The test correctly verifies the new CSV format with semicolon separators and the matriculation number column. However, consider enhancing the test coverage:
- Add test cases with actual matriculation numbers to verify proper handling:
const plagiarismCase1 = { id: 1, exercise: exercise1, - student: { id: 1, login: 'Student 1' }, + student: { id: 1, login: 'Student 1', matriculationNumber: '12345678' }, verdict: PlagiarismVerdict.PLAGIARISM, // ... } as PlagiarismCase; // In the test: const expectedBlob = [ 'Student Login; Matr. Nr.; Exercise;Verdict; Verdict Date\n', - `Student 1; -; Test Exercise 1; PLAGIARISM; ${date}; Test Instructor 1\n`, + `Student 1; 12345678; Test Exercise 1; PLAGIARISM; ${date}; Test Instructor 1\n`, 'Student 2; -; Test Exercise 2; No verdict yet; -; -\n', ];
- Add test cases to verify proper escaping of semicolons in field values:
const plagiarismCaseWithSemicolon = { student: { login: 'Student; with; semicolons' }, exercise: { title: 'Exercise; with; semicolons' }, // ... } as PlagiarismCase;
📜 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.ts(1 hunks)src/test/javascript/spec/component/plagiarism/plagiarism-cases-instructor-view.component.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-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}}
🔇 Additional comments (1)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (1)
Line range hint 136-149: Verify the CSV export functionality in the UI.
Please ensure that:
- The exported CSV file opens correctly in spreadsheet applications
- The matriculation number is displayed in a separate column
- Special characters are handled correctly
Run the following script to check for potential UI tests:
✅ Verification successful
Let me check the test file content to verify if there are tests covering the CSV export functionality.
CSV export functionality has been verified through unit tests
The test file includes comprehensive verification of the CSV export functionality:
- The test "should export plagiarism cases as CSV" confirms:
- Correct CSV header format with separate columns for Student Login, Matriculation Number, Exercise, Verdict, and Verdict Date
- Proper handling of cases with and without verdicts
- Special characters in exercise titles are handled correctly
- The file is downloaded with correct MIME type ('text/csv')
- The file is named 'plagiarism-cases.csv'
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for UI tests related to CSV export functionality
# Search for test files related to this component
fd -e spec.ts plagiarism-cases-instructor-view
# Search for CSV export related tests
rg -l "exportPlagiarismCases" --type ts
Length of output: 388
Script:
#!/bin/bash
# Check the content of the test file
cat src/test/javascript/spec/component/plagiarism/plagiarism-cases-instructor-view.component.spec.ts
Length of output: 9334
...pp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
Outdated
Show resolved
Hide resolved
...pp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
Outdated
Show resolved
Hide resolved
HawKhiem
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
…agiarism-cases-instructor-view.component.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
github-actions
bot
removed
the
plagiarism
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (1)
137-146: Consider using a CSV library for robust handlingInstead of manual CSV generation and sanitization, consider using a robust CSV library like
papaparseor Angular's built-in CSV handling utilities. This would provide:
- Proper escaping and sanitization
- Consistent delimiter handling
- Protection against CSV injection
- Better maintainability
Would you like assistance in implementing this using a CSV library?
🧰 Tools
🪛 eslint (1.23.1)
[error] 137-137: Replace
header·=>·header.replace(/;/g,·''));·with(header)·=>·header.replace(/;/g,·''));(prettier/prettier)
🪛 GitHub Check: client-style
[failure] 137-137:
Replaceheader·=>·header.replace(/;/g,·''));·with(header)·=>·header.replace(/;/g,·''));
📜 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(1 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 (1.23.1)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
[error] 137-137: Replace header·=>·header.replace(/;/g,·''));· with (header)·=>·header.replace(/;/g,·''));
(prettier/prettier)
🪛 GitHub Check: client-style
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
[failure] 137-137:
Replace header·=>·header.replace(/;/g,·''));· with (header)·=>·header.replace(/;/g,·''));
🔇 Additional comments (1)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (1)
143-143: Standardize data sanitization and null handling
The current implementation has inconsistent sanitization and spacing:
- Exercise title sanitizes commas but other fields aren't sanitized
- Inconsistent null handling with
!operator
The previous review comment about creating a helper function for CSV field sanitization is still valid and should be implemented.
...pp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
Outdated
Show resolved
Hide resolved
...pp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (2)
40-64: LGTM! Consider using a more specific type for groupedPlagiarismCases.The grouping logic is well-implemented with proper null checks and type safety. However, the component property
groupedPlagiarismCasescould benefit from a more specific type annotation matching the reducer's accumulator type.- groupedPlagiarismCases: any; // maybe? { [key: number]: PlagiarismCase[] } + groupedPlagiarismCases: { [exerciseId: number]: PlagiarismCase[] };
157-158: Add localization for CSV headers.The header strings should be localized to support multiple languages.
- const headers = ['Student Login', 'Matr. Nr.', 'Exercise', 'Verdict', 'Verdict Date', 'Verdict By'].map((header) => this.sanitizeCSVField(header)); + const headers = [ + this.translateService.instant('plagiarismCases.csv.studentLogin'), + this.translateService.instant('plagiarismCases.csv.matriculationNumber'), + this.translateService.instant('plagiarismCases.csv.exercise'), + this.translateService.instant('plagiarismCases.csv.verdict'), + this.translateService.instant('plagiarismCases.csv.verdictDate'), + this.translateService.instant('plagiarismCases.csv.verdictBy'), + ].map((header) => this.sanitizeCSVField(header));
📜 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(2 hunks)
🧰 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 (1)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (1)
142-151: Skipping comment as issues were already raised in past reviews.
The CSV sanitization concerns were thoroughly covered in previous review comments.
...pp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
Outdated
Show resolved
Hide resolved
…agiarism-cases-instructor-view.component.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (1)
40-64: Consider using Object.groupBy for more concise grouping logicThe grouping implementation could be simplified using newer JavaScript features while maintaining type safety.
Consider this alternative implementation:
- this.groupedPlagiarismCases = this.plagiarismCases.reduce( - ( - acc: { - [exerciseId: number]: PlagiarismCase[]; - }, - plagiarismCase, - ) => { - const caseExerciseId = plagiarismCase.exercise?.id; - if (caseExerciseId === undefined) { - return acc; - } - - // Group initialization - if (!acc[caseExerciseId]) { - acc[caseExerciseId] = []; - this.exercisesWithPlagiarismCases.push(plagiarismCase.exercise!); - } - - // Grouping - acc[caseExerciseId].push(plagiarismCase); - - return acc; - }, - {}, - ); + const validCases = this.plagiarismCases.filter(case => case.exercise?.id !== undefined); + this.exercisesWithPlagiarismCases = [...new Set(validCases.map(case => case.exercise!))]; + this.groupedPlagiarismCases = Object.groupBy(validCases, case => case.exercise!.id);
📜 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(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts (1)
🪛 GitHub Check: client-compilation
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
[failure] 180-180:
Property 'jhiAlertService' does not exist on type 'PlagiarismCasesInstructorViewComponent'.
🪛 GitHub Check: client-tests-selected
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
[failure] 180-180:
Property 'jhiAlertService' does not exist on type 'PlagiarismCasesInstructorViewComponent'.
🪛 GitHub Check: client-tests
src/main/webapp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
[failure] 180-180:
Property 'jhiAlertService' does not exist on type 'PlagiarismCasesInstructorViewComponent'.
...pp/app/course/plagiarism-cases/instructor-view/plagiarism-cases-instructor-view.component.ts
Outdated
Show resolved
Hide resolved
...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
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/app/exam/manage/student-exams/student-exam-timeline/student-exam-timeline.component.ts (1)
Line range hint
1-424: Consider architectural improvements for better maintainability.The component handles multiple responsibilities and could benefit from:
- Extracting submission processing logic into a dedicated service
- Using TypeScript discriminated unions for better type safety
- Adding error handling for failed submissions
- Creating separate interfaces for different submission types
Consider creating:
- A
SubmissionProcessingServiceto handle submission type checking and processing- A discriminated union type for submissions:
type Submission = | { type: 'version'; data: SubmissionVersion } | { type: 'programming'; data: ProgrammingSubmission } | { type: 'fileUpload'; data: FileUploadSubmission };
- Error boundaries for failed submission processing
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/webapp/app/exam/manage/student-exams/student-exam-timeline/student-exam-timeline.component.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/exam/manage/student-exams/student-exam-timeline/student-exam-timeline.component.ts (1)
🔇 Additional comments (2)
src/main/webapp/app/exam/manage/student-exams/student-exam-timeline/student-exam-timeline.component.ts (2)
1-1: LGTM: Import statements are properly organized.
The addition of ChangeDetectorRef and tap operator imports is appropriate for the implemented changes.
Also applies to: 11-11
60-60: Verify the necessity of manual change detection.
The addition of ChangeDetectorRef suggests potential view update issues. Manual change detection should typically be avoided unless absolutely necessary. Please verify:
- Are there specific scenarios where automatic change detection fails?
- Have you considered using
ChangeDetectionStrategy.OnPushinstead?
HawKhiem
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
MarkusPaulsen
added
the
maintainer-approved
AjayvirS
requested review from
krusche
and removed request for
SimonEntholzer
krusche
deleted the
bugfix/plagiarism-checks/5599-fix-csv-spreadsheet-format
branch
Checklist
General
Client
Motivation and Context
CSV files can use either a comma (,) or a semicolon (;) as the separator. The issue mentions CSV files not opening correctly with the .xlsx extension, which is a Microsoft Excel file format. In some regions, Excel expects semicolon-separated values for CSV files instead of commas.
This change is required because the existing occurrences of CSV parsing in the issue use comma (,) as the separator, which does not align with the regional default for Excel. Ensuring consistency by introducing (;) across the project will prevent errors when opening CSV files in Excel.
Additionally, the plagiarism check CSV file must include the matriculation numbers and split each parameter into separate cells in the spreadsheet. This ensures the data is structured correctly and can be processed reliably.
Fixes #5599
Description
The code was updated to enhance quality and include improved error handling. Additionally, a Matr. Nr. field was added, and data sanitation was made more robust. Commas were replaced with semicolons before creating the CSV Blob, and the associated tests were adjusted to reflect these changes.
Steps for Testing
Steps 1-6 can be skipped on TS1 and TS3.
TS1: https://artemis-test1.artemis.cit.tum.de/course-management/88/plagiarism-cases
TS3: https://artemis-test3.artemis.cit.tum.de/course-management/48/plagiarism-cases
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
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Tests