Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exam mode: Enable students to participate in the test exam multiple times #8609

Open
wants to merge 120 commits into
base: develop
Choose a base branch
from

Conversation

coolchock
Copy link
Contributor

@coolchock coolchock commented May 16, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
  • I documented the Java code using JavaDoc style.

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data.
  • I strictly followed the client coding and design guidelines.
  • Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
  • I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I added authorities to all new routes and checked the course groups for displaying navigation elements (links, buttons).
  • I documented the TypeScript code using JSDoc style.

Changes affecting Programming Exercises

  • High priority: I tested all changes and their related features with all corresponding user types on a test server configured with the integrated lifecycle setup (LocalVC and LocalCI).
  • I tested all changes and their related features with all corresponding user types on a test server configured with Gitlab and Jenkins.

Motivation and Context

Currently, students are limited to a single attempt per test exam. This pull request introduces a feature that allows students to take the test exam multiple times.

Description

StudentExam now maintains a list of participations for exercises linked to a particular student exam, enabling students to attempt the test exam multiple times. A student can start a new test exam only if the previous one has been submitted or if its working time has expired. The database index in the participation table has been updated to account for the number of attempts. Normal and practice mode exercises have one participation each, while test exam exercises can have up to 256 participations.

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 1 Student
  • 1 Test Exam
  1. Log in to Artemis
  2. Navigate to Course Administration and create a test exam
  3. Go to the course -> exams and try to participate in the test exam multiple times
  4. Verify that each test exam attempt has it's own participations and results.
  5. Important! Please make sure that it's possible to participate in a normal exercise, in a practice mode exercise, in a normal exam and in a test run.

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked






Review Progress

Performance Review

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Exam Mode Test

  • Test 1
  • Test 2

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced new methods for retrieving student exams with enhanced criteria, including support for test exams.
    • Added functionality to determine if an exam is classified as a test exam.
    • Enhanced data retrieval capabilities for programming exercise student participations.
    • Added new localization keys for improved user interface clarity regarding exam attempts and submissions.
  • Enhancements

    • Improved clarity and specificity in method names for retrieving student exams.
    • Streamlined setup process for test exams and their associated participations.
    • Enhanced error handling in the ExamParticipationComponent for improved user feedback when loading student exams.
    • Refined logic for displaying exam results and participation details in the user interface.
  • Bug Fixes

    • Added comprehensive tests for the ExamAccessService to check exam access under different user roles.
    • Added new tests for ExamParticipationService to handle various submission scenarios effectively.
    • Included new tests for ExamStartInformationComponent to verify correct initialization of exam dates.

@github-actions github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) client Pull requests that update TypeScript code. (Added Automatically!) database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. labels May 16, 2024
Copy link

@HawKhiem HawKhiem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS3. I can indeed participate in a test exam multiple times and each time has a different summary and review!

Screenshot 2024-10-25 193707
Screenshot 2024-10-25 193714

Copy link

@sachmii sachmii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS3, worked fine.

Bildschirmfoto 2024-10-26 um 20 53 58

Copy link
Contributor

@SimonEntholzer SimonEntholzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code

Copy link
Contributor

@JohannesStoehr JohannesStoehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reapprove after merge conflict

Copy link
Contributor

@pzdr7 pzdr7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reapprove after merges

Copy link
Contributor

@EneaGore EneaGore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-approve

Copy link
Member

@BBesrour BBesrour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-approve after merge conflicts

Copy link

@Cathy0123456789 Cathy0123456789 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS1. Works as described.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java (1)

Line range hint 1011-1029: Improve method documentation and structure.

  1. Add documentation for the withAssessor parameter to clarify its purpose.
  2. Consider simplifying the nested if-else structure.

Apply this diff to improve the documentation and structure:

     /**
      * Get all participations for the given studentExam and exercises combined with their submissions with a result.
      * Distinguishes between real exams, test exams and test runs and only loads the respective participations
      *
      * @param studentExam  studentExam with exercises loaded
+     * @param withAssessor if true, loads the assessor information with the result (only for non-test runs)
      * @return student's participations with submissions and results
      */
     default List<StudentParticipation> findByStudentExamWithEagerSubmissionsResult(StudentExam studentExam, boolean withAssessor) {
         if (studentExam.isTestRun()) {
             return findTestRunParticipationsByStudentIdAndIndividualExercisesWithEagerSubmissionsResult(studentExam.getUser().getId(), studentExam.getExercises());
         }
-        if (studentExam.isTestExam()) {
-            if (withAssessor) {
-                return findTestExamParticipationsByStudentIdAndIndividualExercisesWithEagerSubmissionsResultAndAssessorIgnoreTestRuns(studentExam.getId());
-            }
-            else {
-                return findTestExamParticipationsByStudentIdAndIndividualExercisesWithEagerSubmissionsResultIgnoreTestRuns(studentExam.getId());
-            }
-        }
-        else {
-            if (withAssessor) {
-                return findByStudentIdAndIndividualExercisesWithEagerSubmissionsResultAndAssessorIgnoreTestRuns(studentExam.getUser().getId(), studentExam.getExercises());
-            }
-            else {
-                return findByStudentIdAndIndividualExercisesWithEagerSubmissionsResultIgnoreTestRuns(studentExam.getUser().getId(), studentExam.getExercises());
-            }
-        }
+        return studentExam.isTestExam()
+            ? withAssessor
+                ? findTestExamParticipationsByStudentIdAndIndividualExercisesWithEagerSubmissionsResultAndAssessorIgnoreTestRuns(studentExam.getId())
+                : findTestExamParticipationsByStudentIdAndIndividualExercisesWithEagerSubmissionsResultIgnoreTestRuns(studentExam.getId())
+            : withAssessor
+                ? findByStudentIdAndIndividualExercisesWithEagerSubmissionsResultAndAssessorIgnoreTestRuns(studentExam.getUser().getId(), studentExam.getExercises())
+                : findByStudentIdAndIndividualExercisesWithEagerSubmissionsResultIgnoreTestRuns(studentExam.getUser().getId(), studentExam.getExercises());
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between e83d11e and 6fd8939.

⛔ Files ignored due to path filters (1)
  • src/main/resources/config/liquibase/master.xml is excluded by !**/*.xml
📒 Files selected for processing (3)
  • src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/exercise/domain/Exercise.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java (6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (1)

Pattern 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/exercise/domain/Exercise.java (1)

Pattern 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/exercise/repository/StudentParticipationRepository.java (1)

Pattern 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

🔇 Additional comments (6)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (1)

353-357: LGTM! The change correctly supports multiple test exam attempts.

The modification to use findByExamIdAndParticipationId instead of user ID is appropriate for supporting multiple test exam attempts, as it ensures results are filtered based on the specific participation rather than the user, allowing for proper handling of multiple attempts.

Let's verify that this change is consistent with other exam-related queries:

✅ Verification successful

Let me analyze the results and generate additional verification scripts to check the usage context of findByExamIdAndUserId in exam-related services.


Let me generate one final verification script to check the complete context of exam result publication logic.


The change to use findByExamIdAndParticipationId is correct and consistent with the codebase

The verification confirms that:

  1. The change aligns with the test exam's result publication logic in StudentExam.areResultsPublishedYet()
  2. The StudentExamRepository properly supports both access patterns:
    • findByExamIdAndParticipationId for specific participation lookups (used in test exams)
    • findByExamIdAndUserId for general exam access (used in normal exams)
  3. The context in ResultService shows this change is part of the proper sensitive feedback filtering logic for test exams
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances where exam results might still be queried by user ID
# instead of participation ID, which could be problematic with multiple attempts.

# Search for methods that might need similar updates
ast-grep --pattern 'findByExamIdAndUserId'

# Search for other exam result publication checks that might need updates
rg -l "resultsPublished|areResultsPublishedYet" --type java

Length of output: 5221


Script:

#!/bin/bash
# Check the context of findByExamIdAndUserId usage in ExamAccessService and ExamUserService
rg -B 5 -A 5 "findByExamIdAndUserId" src/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.java src/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java

# Check if there are any other methods in StudentExamRepository that might need similar updates
ast-grep --pattern 'interface StudentExamRepository {
  $$$
}'

# Check the context of result publication in StudentExam domain
rg -B 5 -A 5 "resultsPublished|areResultsPublishedYet" src/main/java/de/tum/cit/aet/artemis/exam/domain/StudentExam.java

Length of output: 3797


Script:

#!/bin/bash
# Check the complete context of exam result publication in ResultService
rg -B 10 -A 10 "filterSensitiveFeedbacksInExamExercise" src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java

# Check for any other methods in StudentExamRepository that use participationId
rg -B 5 -A 5 "participationId" src/main/java/de/tum/cit/aet/artemis/exam/repository/StudentExamRepository.java

Length of output: 3191

src/main/java/de/tum/cit/aet/artemis/exercise/domain/Exercise.java (1)

378-381: ⚠️ Potential issue

Add null check to prevent NullPointerException.

The method could throw a NullPointerException if getExam() returns null. This is a critical safety issue that needs to be addressed.

Apply this diff to implement a null-safe check:

@JsonIgnore
public boolean isTestExamExercise() {
-    return isExamExercise() && this.getExam().isTestExam();
+    return isExamExercise() && this.getExam() != null && this.getExam().isTestExam();
}
src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java (4)

121-122: LGTM! Efficient method for finding latest participation.

The method follows Spring Data JPA naming conventions and efficiently retrieves the latest participation using database ordering.


133-147: Refer to existing review comment about redundant method.


773-787: Refer to existing review comment about incorrect IN operator usage.


788-800: Refer to existing review comment about incorrect IN operator usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assessment Pull requests that affect the corresponding module client Pull requests that update TypeScript code. (Added Automatically!) database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. exam Pull requests that affect the corresponding module exercise Pull requests that affect the corresponding module programming Pull requests that affect the corresponding module ready to merge server Pull requests that update Java code. (Added Automatically!) tests
Projects
Status: Ready For Review
Development

Successfully merging this pull request may close these issues.