Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #8609Exam mode
: Enable students to participate in the test exam multiple times #8609Changes from 74 commits
7392a09
89ef7fb
61cba39
8da735c
07c541e
c6ced67
fff4bfa
6dc3c42
4b90546
b882350
ffb9eab
6912659
261c3be
43b0ef1
c055ef1
9e2e08c
dd4e8a7
e552578
1ab81f1
bf79e1b
7f3da78
f632053
b9cd7db
3a022b5
0023448
0fb3cfd
c58dd87
7d4a49c
0fbb9c3
1a8c1a5
7fc8758
aaaa4b2
87b9321
38e08bf
cb24abf
b1c1b9c
dc95707
564bde9
ca54780
da998a0
044fadc
7e7b868
ae91e10
5f34573
49f5d11
1e1c3b5
ed10000
b604bf0
52f1e5a
74599b9
1c0c7af
e4ddb76
cd7dc52
631dd2f
8476dcd
d00108f
71171c0
6c085c3
8c9f604
6500a9e
3b9a656
8fad453
ac396fd
2e4ad8c
0a2519c
8ed669d
6b4856e
c6d851e
ea1f359
b186bae
afeed60
7b8ec72
ee30325
bfd9323
bac9202
b40c3ab
c85c599
7644eb6
54853d5
edc9c18
f913762
ebcdd54
2025853
8870714
1b6c16e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
The
StudentParticipation
class does not contain a corresponding@ManyToMany
annotation, which is necessary for a bidirectional relationship. This could lead to issues with theManyToMany
relationship in theStudentExam
class.src/main/java/de/tum/in/www1/artemis/domain/participation/StudentParticipation.java
Analysis chain
Ensure that the
ManyToMany
relationship is correctly configured to handle multiple participations per student exam without redundancy.Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 121
Script:
Length of output: 619
Script:
Length of output: 831
Script:
Length of output: 130
Script:
Length of output: 406
Script:
Length of output: 1114
Script:
Length of output: 371
Script:
Length of output: 100
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NumberOfAttempts sounds to me like the maximum number of attempts, but the JavaDoc reads more like this is an indicator on which attempt this is. So just
attempt
would be a more fitting name imo.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify the comment and adjust the field name for clarity.
The comment and field name
numberOfAttempts
might be confusing as it suggests the maximum number of attempts, but it actually tracks the current attempt number. Consider renaming the field and updating the comment for clarity.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LIMIT
is not a generally supported keyword and will break Artemis on Postgres.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did not answer to this comment yet. We can only merge the PR if the tests are successful with MySQL and Postgres
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using SQL-specific keywords in JPA queries.
The use of
LIMIT
in the JPA query might lead to compatibility issues with databases other than MySQL. It's recommended to use Spring Data JPA'sPageable
interface to handle limiting results in a more portable way.And when calling the method:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure proper documentation for the new method
findWithExercisesAndStudentParticipationsById
.Adding a Javadoc comment will help other developers understand the purpose of this method quickly, especially since it involves loading multiple entities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure proper documentation for the new method
findWithExercisesAndStudentParticipationsById
.Adding a Javadoc comment will help other developers understand the purpose of this method quickly, especially since it involves loading multiple entities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding Javadoc for
findWithExercisesAndSessionsById
.This method efficiently fetches
StudentExam
along with exercises, sessions, and participations. However, consider adding a brief Javadoc to explain the purpose and usage of this method, especially since it involves multiple joins which could impact performance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify documentation for
findOneByExamIdAndUserId
.The method documentation is clear about its purpose and the conditions under which it operates, including handling test exams. However, ensure that the documentation explicitly mentions what happens if multiple student exams exist for the same
examId
anduserId
combination.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure unit tests for new methods handling test exams.
The methods
findStudentExamsForTestExamsByUserIdAndCourseId
andfindStudentExamsForTestExamsByUserIdAndExamId
are crucial for the new functionality introduced. It's essential to ensure these methods are covered by unit tests to handle edge cases, especially considering the complex conditions involved.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add exception details in the Javadoc.
While the method
findByIdWithExercisesAndStudentParticipationsElseThrow
throws anEntityNotFoundException
, the Javadoc does not mention what triggers this exception. Clarifying this in the documentation can help developers understand the conditions under which this exception will be thrown.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document exception conditions for
findByIdWithExercisesAndSessionsElseThrow
.Similar to the previous method, it's important to document the conditions under which the
EntityNotFoundException
is thrown for better clarity and maintainability of the code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure the JPQL query is complete.
The
existsByCourseIdAndStudentId
method's JPQL query ends abruptly without a closing parenthesis or a complete WHERE clause. This might lead to a syntax error or unexpected behavior at runtime.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace LIMIT clause in JPQL query.
JPQL does not support the
LIMIT
clause. UsePageable
or set the maximum result size directly in the query method to ensure compatibility and prevent runtime errors.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace LIMIT clause in JPQL query.
Similar to the previous comment, the
LIMIT
clause is not supported in JPQL. Adjusting this to usePageable
or another method to limit results is necessary to avoid syntax errors.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor to consolidate redundant queries.
The two methods
findTestExamParticipationsByStudentIdAndIndividualExercisesWithEagerSubmissionsResultAndAssessorIgnoreTestRuns
andfindTestExamParticipationsByStudentIdAndIndividualExercisesWithEagerSubmissionsResultIgnoreTestRuns
are very similar and differ only in the fetching of the assessor. Consider refactoring to a single method with a parameter to include or exclude the assessor, reducing redundancy and improving maintainability.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor to consolidate redundant queries.
The two methods
findTestExamParticipationsByStudentIdAndIndividualExercisesWithEagerSubmissionsResultAndAssessorIgnoreTestRuns
andfindTestExamParticipationsByStudentIdAndIndividualExercisesWithEagerSubmissionsResultIgnoreTestRuns
are very similar and differ only in the fetching of the assessor. Consider refactoring to a single method with a parameter to include or exclude the assessor, reducing redundancy and improving maintainability.Committable suggestion