Plagiarism checks: Inform instructors when student replies on a plagiarism case
#10287
Conversation
github-actions
bot
added
tests
server
…-over-student-response
WalkthroughThis pull request introduces a new notification type, PLAGIARISM_CASE_REPLY, across both the backend and frontend. It removes the obsolete constant Changes
Suggested labels
Suggested reviewers
✨ Finishing Touches
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 (1)
src/main/webapp/app/course/plagiarism-cases/shared/plagiarism-cases.service.ts (1)
137-146: Avoid non-null assertions and handle potential null values safely.The method uses multiple non-null assertions (
!) which could lead to runtime errors if the values are actually null.Apply this diff to handle null values safely:
- if (exercise.exerciseGroup) { - courseId = exercise.exerciseGroup.exam!.course!.id!; + if (exercise.exerciseGroup?.exam?.course?.id) { + courseId = exercise.exerciseGroup.exam.course.id; } else { - courseId = exercise.course!.id!; + if (!exercise.course?.id) { + throw new Error('Exercise must have either a course or an exercise group with course'); + } + courseId = exercise.course.id; } - const exerciseId = exercise!.id; + if (!exercise.id) { + throw new Error('Exercise must have an id'); + } + const exerciseId = exercise.id;
🧹 Nitpick comments (6)
src/test/javascript/spec/service/plagiarism-cases.service.spec.ts (1)
178-184: Enhance test coverage with additional test cases.While the basic test is good, consider adding test cases for:
- Error responses (4xx, 5xx)
- Network failures
- Invalid post IDs
Example test cases to add:
it('should handle error response when informing instructor', fakeAsync(() => { service.informInstructorAboutPostReply(1).subscribe({ error: (error) => expect(error.status).toBe(404) }); const req = httpMock.expectOne('api/posts/1/informInstructor'); req.flush('Not found', { status: 404, statusText: 'Not Found' }); tick(); })); it('should handle network error when informing instructor', fakeAsync(() => { service.informInstructorAboutPostReply(1).subscribe({ error: (error) => expect(error.name).toBe('HttpErrorResponse') }); const req = httpMock.expectOne('api/posts/1/informInstructor'); req.error(new ErrorEvent('Network error')); tick(); }));src/test/java/de/tum/cit/aet/artemis/plagiarism/PlagiarismPostIntegrationTest.java (1)
332-336: Enhance test coverage with additional assertions.While the basic HTTP status check is good, consider adding assertions for:
- Verify that the instructor was actually notified
- Test error cases (e.g., non-existent post ID)
- Verify security (e.g., test access denied for unauthorized users)
@Test @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") void testInformInstructorAboutPostReply_asStudent() throws Exception { - request.get("/api/posts/" + existingPosts.getFirst().getId() + "/informInstructor", HttpStatus.OK, Void.class); + // Test successful notification + request.get("/api/posts/" + existingPosts.getFirst().getId() + "/informInstructor", HttpStatus.OK, Void.class); + verify(singleUserNotificationService).notifyInstructorAboutPlagiarismCaseReply(any()); + + // Test non-existent post + request.get("/api/posts/99999/informInstructor", HttpStatus.NOT_FOUND, Void.class); + + // Test unauthorized access + mockUser(TEST_PREFIX + "unauthorized", Set.of()); + request.get("/api/posts/" + existingPosts.getFirst().getId() + "/informInstructor", HttpStatus.FORBIDDEN, Void.class); }src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java (1)
270-272: Fix typo in method name.The method name has a typo: "Instruction" should be "Instructor".
Apply this diff to fix the typo:
- public void notifyInstructionAboutPlagiarismCaseReply(PlagiarismCase plagiarismCase, User instructor) { + public void notifyInstructorAboutPlagiarismCaseReply(PlagiarismCase plagiarismCase, User instructor) {src/main/webapp/i18n/en/notification.json (1)
110-110: Make title consistent with other plagiarism case titles.For consistency with other plagiarism case titles, consider using a more concise format.
Apply this diff to make the title more consistent:
- "plagiarismCaseReply": "Reply from Student on Plagiarism Case", + "plagiarismCaseReply": "Plagiarism Case Reply",src/main/webapp/i18n/de/notification.json (2)
110-110: Review of New Notification Title EntryThe new key "plagiarismCaseReply" with the title
"Antwort auf Plagiatsfall von einem Studierenden"
has been correctly added. It clearly indicates that a reply from a student is involved.Consider verifying that the tone aligns with the informal style guidelines for German translations. Since other instructor-facing notifications (e.g. those for Tutors) sometimes use possessive pronouns (like "deiner"), ensure that using this more neutral construction is intentional for the context (i.e. notifying instructors). If you prefer a more direct informal address and the plagiarism case is relevant for the instructor’s context, you might want to adjust the phrasing accordingly.
138-138: Review of New Notification Text EntryThe new text for "plagiarismCaseReply" reads:
"Der Plagiatsfall für die {{ placeholderValues.1 }} Aufgabe "{{ placeholderValues.2 }}" hat eine Antwort bekommen."While this message is clear, consider a couple of improvements:
- For better clarity and a slightly more active tone, you might rephrase it to emphasize that a student’s reply has been submitted. For example:
"Der Plagiatsfall für die {{ placeholderValues.1 }} Aufgabe "{{ placeholderValues.2 }}" wurde von einem Studierenden beantwortet."- Verify that the tone remains consistent with other notifications. Since the guidelines mandate an informal style (using du/dein), check if there’s any need to personalize or adjust the phrasing based on whether the notification is directed to instructors (who might expect a less personal address) or students.
Below is a diff suggestion if you choose to adopt the rewording:
- "plagiarismCaseReply": "Der Plagiatsfall für die {{ placeholderValues.1 }} Aufgabe \"{{ placeholderValues.2 }}\" hat eine Antwort bekommen.", + "plagiarismCaseReply": "Der Plagiatsfall für die {{ placeholderValues.1 }} Aufgabe \"{{ placeholderValues.2 }}\" wurde von einem Studierenden beantwortet.",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/main/java/de/tum/cit/aet/artemis/communication/domain/NotificationType.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/NotificationConstants.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/SingleUserNotificationFactory.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/service/PlagiarismCaseService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/service/PlagiarismPostService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/web/PlagiarismCaseResource.java(3 hunks)src/main/webapp/app/course/plagiarism-cases/shared/plagiarism-cases.service.ts(3 hunks)src/main/webapp/app/course/plagiarism-cases/student-view/detail-view/plagiarism-case-student-detail-view.component.html(1 hunks)src/main/webapp/app/course/plagiarism-cases/student-view/detail-view/plagiarism-case-student-detail-view.component.ts(1 hunks)src/main/webapp/app/entities/notification.model.ts(1 hunks)src/main/webapp/app/shared/notification/notification.service.ts(2 hunks)src/main/webapp/i18n/de/notification.json(2 hunks)src/main/webapp/i18n/en/notification.json(2 hunks)src/test/java/de/tum/cit/aet/artemis/plagiarism/PlagiarismPostIntegrationTest.java(1 hunks)src/test/javascript/spec/component/plagiarism/plagiarism-case-student-detail-view.component.spec.ts(4 hunks)src/test/javascript/spec/service/plagiarism-cases.service.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/communication/domain/NotificationType.javasrc/main/java/de/tum/cit/aet/artemis/plagiarism/service/PlagiarismPostService.javasrc/main/java/de/tum/cit/aet/artemis/plagiarism/service/PlagiarismCaseService.javasrc/main/java/de/tum/cit/aet/artemis/communication/domain/notification/SingleUserNotificationFactory.javasrc/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.javasrc/main/java/de/tum/cit/aet/artemis/communication/domain/notification/NotificationConstants.javasrc/main/java/de/tum/cit/aet/artemis/plagiarism/web/PlagiarismCaseResource.java
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/entities/notification.model.tssrc/main/webapp/app/shared/notification/notification.service.tssrc/main/webapp/app/course/plagiarism-cases/student-view/detail-view/plagiarism-case-student-detail-view.component.tssrc/main/webapp/app/course/plagiarism-cases/shared/plagiarism-cases.service.ts
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/plagiarism/PlagiarismPostIntegrationTest.java
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
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/detail-view/plagiarism-case-student-detail-view.component.html
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
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/plagiarism-case-student-detail-view.component.spec.tssrc/test/javascript/spec/service/plagiarism-cases.service.spec.ts
`src/main/webapp/i18n/de/**/*.json`: German language transla...
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/i18n/de/notification.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: server-tests
- GitHub Check: client-tests-selected
- GitHub Check: Analyse
🔇 Additional comments (17)
src/main/java/de/tum/cit/aet/artemis/communication/domain/NotificationType.java (1)
7-8: LGTM!The changes maintain a clean enum structure with proper ordering of constants.
src/main/webapp/app/entities/notification.model.ts (1)
88-88: LGTM!The new constant follows the established naming pattern and maintains proper ordering.
src/main/webapp/app/course/plagiarism-cases/shared/plagiarism-cases.service.ts (1)
148-150: LGTM!The method correctly uses the inherited
getmethod fromBaseApiHttpServiceto make the API call.src/test/javascript/spec/component/plagiarism/plagiarism-case-student-detail-view.component.spec.ts (2)
165-170: LGTM!The test case correctly verifies that
informInstructorcalls the service method with the expected post ID.
172-178: LGTM!The test case properly verifies both the instructor notification and modal opening functionality.
src/main/java/de/tum/cit/aet/artemis/plagiarism/service/PlagiarismPostService.java (1)
93-96: LGTM!The method correctly retrieves the post and delegates to the appropriate service for instructor notification. The repository's
findPostByIdElseThrowmethod handles the error case when the post is not found.src/main/java/de/tum/cit/aet/artemis/plagiarism/web/PlagiarismCaseResource.java (2)
66-74: LGTM! Constructor follows dependency injection best practices.The constructor properly uses constructor injection for the new
PlagiarismPostServicedependency.
279-288: LGTM! The endpoint is well-implemented with proper security and logging.The implementation:
- Uses proper security annotation
@EnforceAtLeastStudent- Includes performance logging
- Returns appropriate HTTP status
src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/NotificationConstants.java (1)
41-41: LGTM! Constants are well-organized and follow conventions.The new constants for plagiarism case reply notifications:
- Follow established naming patterns
- Use proper translation keys
- Are correctly mapped in the notification type mapping
Also applies to: 133-134, 224-225, 302-302
src/main/webapp/app/shared/notification/notification.service.ts (1)
321-324: LGTM! Navigation logic is well-implemented.The implementation:
- Follows Angular routing patterns
- Constructs route components consistently
- Handles navigation similarly to other notification types
src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/SingleUserNotificationFactory.java (2)
50-51: LGTM!The imports for the new notification constants follow the existing pattern and are correctly ordered.
199-204: LGTM!The new case for
PLAGIARISM_CASE_REPLYfollows the same pattern as other plagiarism-related cases and correctly reuses the existing placeholder creation method.src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java (2)
15-15: LGTM!The import for the new notification type follows the existing pattern and is correctly ordered.
131-132: LGTM!The addition of
PLAGIARISM_CASE_REPLYto the switch case follows the existing pattern and is correctly grouped with other plagiarism-related cases.src/main/webapp/app/course/plagiarism-cases/student-view/detail-view/plagiarism-case-student-detail-view.component.html (2)
87-87: LGTM!The button click handler update follows Angular's syntax and correctly implements the new reply functionality.
95-102: LGTM!The new section follows Angular's new
@ifsyntax and correctly shows comparisons only after the due date.src/main/webapp/i18n/en/notification.json (1)
138-138: LGTM!The notification text follows the existing pattern and correctly uses placeholders for dynamic content.
...e/plagiarism-cases/student-view/detail-view/plagiarism-case-student-detail-view.component.ts
Outdated
Show resolved
Hide resolved
...e/plagiarism-cases/student-view/detail-view/plagiarism-case-student-detail-view.component.ts
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/plagiarism/service/PlagiarismCaseService.java
Show resolved
Hide resolved
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
|
Hey @az108! I tried to test the PR on https://artemis-test3.artemis.cit.tum.de/, but the Plagiarism Cases page always looked like this: I'm logged in as artemis_test_user_16 (instructor) and created a programming exercise with two identical submissions and manual assessment. What am I missing? |
|
@simonbohnen Thanks for pointing this out. I was not aware of this as its visible there after a plagiarism was detected. I updated the testing steps accordingly and now you should be able to test it :) |
There was a problem hiding this comment.
Was able to test this on https://artemis-test3.artemis.cit.tum.de/ 👌🏼
…hecks/notify-instructor-over-student-response
…-over-student-response
…nstructor-over-student-response' into chore/plagiarism-checks/notify-instructor-over-student-response
az108
dismissed stale reviews from coderabbitai[bot] and simonbohnen
via
8e66800
…-over-student-response
…nstructor-over-student-response' into chore/plagiarism-checks/notify-instructor-over-student-response
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
…-over-student-response
…-over-student-response
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
…nstructor-over-student-response' into chore/plagiarism-checks/notify-instructor-over-student-response
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java (1)
337-343: LGTM with a suggestion for error handling.The implementation correctly sets up the context and subject for plagiarism case replies. However, consider adding null checks for the exercise and course retrieval to prevent potential NPEs.
Consider adding null checks:
if (notificationType == NotificationType.PLAGIARISM_CASE_REPLY) { notification.getTargetTransient().setMainPage("course-management"); Exercise exercise = plagiarismCase.getExercise(); + if (exercise == null) { + throw new IllegalStateException("Exercise not found for plagiarism case"); + } Course course = exercise.getCourseViaExerciseGroupOrCourseMember(); + if (course == null) { + throw new IllegalStateException("Course not found for exercise"); + } return messageSource.getMessage("artemisApp.singleUserNotification.title.plagiarismCaseReply", new Object[] { exercise.getTitle(), course.getTitle() }, context.getLocale()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java(2 hunks)src/main/resources/i18n/messages.properties(1 hunks)src/main/resources/i18n/messages_de.properties(1 hunks)src/main/resources/i18n/messages_en.properties(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/resources/i18n/messages.properties
- src/main/resources/i18n/messages_en.properties
- src/main/resources/i18n/messages_de.properties
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: client-compilation
- GitHub Check: server-style
- GitHub Check: client-tests-selected
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: client-tests
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Analyse
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java (1)
396-396: LGTM!The switch case correctly maps the
PLAGIARISM_CASE_REPLYnotification type to its corresponding email template.
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java
Show resolved
Hide resolved
…-over-student-response
az108
requested review from
AjayvirS,
simonbohnen,
HawKhiem,
sachmii and
HanyangXu0508
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
This PR resolves the issue described in #6169. Instructors will now be notified when students defend themselves against accusations of plagiarism.
Description
I implemented a client-side function that is triggered when a student responds to a plagiarism accusation. This function sends a REST call to the server, which then notifies the corresponding instructor.
Steps for Testing
Prerequisites:
1 Instructor
2 Students
1 Programming Exercise with Complaints enabled
Additional Email Test (1 Manual Test for this is sufficient):
Use a instructor account with an email set to which you have access and verify if you receive an email.
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Performance Review
Code Review
Manual Tests
Performance Tests
Test Coverage
Screenshots
Notification:
Email:
Summary by CodeRabbit