-
Notifications
You must be signed in to change notification settings - Fork 294
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
Development
: Merge exercise details calls into one DTO
#8712
Development
: Merge exercise details calls into one DTO
#8712
Conversation
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.
Some intermediate feedback:
src/main/java/de/tum/in/www1/artemis/web/rest/dto/ExerciseDetailStudentParticipationDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/www1/artemis/web/rest/dto/ExerciseDetailStudentParticipationDTO.java
Outdated
Show resolved
Hide resolved
76af83a
to
2cb7570
Compare
Development
: Convert exercise details entity to DTODevelopment
: Merging exercise details calls into one DTO
Development
: Merging exercise details calls into one DTODevelopment
: Merge exercise details calls into one DTO
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe changes primarily involve enhancing the retrieval of exercise details by introducing a new Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 20
Outside diff range comments (13)
src/main/webapp/app/exercises/shared/feedback/standalone-feedback/standalone-feedback.component.ts (1)
Line range hint
45-45
: Replace the==
operator with===
to avoid potential issues with type coercion, ensuring strict equality checks.- if (relevantResult.id == resultId) { + if (relevantResult.id === resultId) {Tools
Biome
[error] 2-3: All these imports are only used as types. (lint/style/useImportType)
Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.
[error] 4-5: All these imports are only used as types. (lint/style/useImportType)
Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.
[error] 5-6: All these imports are only used as types. (lint/style/useImportType)
Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.
[error] 6-7: All these imports are only used as types. (lint/style/useImportType)
Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.
[error] 7-8: All these imports are only used as types. (lint/style/useImportType)
Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.src/test/playwright/e2e/exam/ExamAssessment.spec.ts (6)
Line range hint
53-53
: Avoid using non-null assertions. Consider using optional chaining for safer access.- studentOneName = (await users.getUserInfo(studentOne.username, page)).name!; + studentOneName = (await users.getUserInfo(studentOne.username, page))?.name;
Line range hint
71-71
: Avoid using non-null assertions. Consider using optional chaining for safer access.- await examManagement.verifySubmitted(course.id!, exam.id!, studentOneName); + await examManagement.verifySubmitted(course.id?, exam.id?, studentOneName);
Line range hint
105-105
: Avoid using non-null assertions. Consider using optional chaining for safer access.- await login(studentOne, `/courses/${course.id}/exams/${exam.id}`); + await login(studentOne, `/courses/${course.id?}/exams/${exam.id?}`);
Line range hint
138-138
: Avoid using non-null assertions. Consider using optional chaining for safer access.- await examManagement.verifySubmitted(course.id!, exam.id!, studentOneName); + await examManagement.verifySubmitted(course.id?, exam.id?, studentOneName);
Line range hint
172-172
: Avoid using non-null assertions. Consider using optional chaining for safer access.- await login(studentOne, `/courses/${course.id}/exams/${exam.id}`); + await login(studentOne, `/courses/${course.id?}/exams/${exam.id?}`);
Line range hint
179-179
: Avoid using non-null assertions. Consider using optional chaining for safer access.- await examManagement.verifySubmitted(course.id!, exam.id!, studentOneName); + await examManagement.verifySubmitted(course.id?, exam.id?, studentOneName);src/main/webapp/app/exercises/shared/exercise/exercise.service.ts (6)
Line range hint
31-31
: Specify a more explicit type instead ofany
forexampleSolutionUML
.- exampleSolutionUML?: any; + exampleSolutionUML?: ExampleSolutionUMLType; // Define ExampleSolutionUMLType based on expected structure
Line range hint
50-50
: Avoid usingany
for type declarations. Specify more explicit types to enhance type safety.- exampleSolutionUML?: any; + exampleSolutionUML?: ExampleSolutionUMLType; // Define ExampleSolutionUMLType based on expected structure - let exampleSolutionUML = undefined; + let exampleSolutionUML: ExampleSolutionUMLType | undefined = undefined;Also applies to: 52-52
Line range hint
113-115
: Consider simplifying the control flow by removing unnecessaryelse
clauses.- } else { - return true; - } + return true;Also applies to: 121-123, 226-232, 229-232
Line range hint
326-326
: Use optional chaining to simplify null checks.- if (exercise?.exerciseGroup && !exercise?.isAtLeastTutor) { + if (exercise?.exerciseGroup?.title && !exercise?.isAtLeastTutor) {
Line range hint
494-494
: Specify an explicit type forexampleSolutionPublicationDate
to avoid implicitany
type.- let exampleSolutionPublicationDate; + let exampleSolutionPublicationDate: dayjs.Dayjs | undefined;
Line range hint
535-535
: Encapsulate the variable declaration within a block to restrict its scope to the switch case.case ExerciseType.TEXT: case ExerciseType.FILE_UPLOAD: + { const textOrFileUploadExercise = exercise as TextExercise & FileUploadExercise; if (textOrFileUploadExercise.exampleSolution) { exampleSolution = artemisMarkdown.safeHtmlForMarkdown(textOrFileUploadExercise.exampleSolution); } + } break;
src/main/webapp/app/overview/exercise-details/course-exercise-details.component.ts
Show resolved
Hide resolved
src/main/webapp/app/overview/exercise-details/course-exercise-details.component.ts
Show resolved
Hide resolved
src/main/webapp/app/overview/exercise-details/course-exercise-details.component.ts
Show resolved
Hide resolved
src/test/java/de/tum/in/www1/artemis/text/TextSubmissionIntegrationTest.java
Show resolved
Hide resolved
src/main/webapp/app/overview/exercise-details/problem-statement/problem-statement.component.ts
Show resolved
Hide resolved
src/main/java/de/tum/in/www1/artemis/service/plagiarism/PlagiarismCaseService.java
Outdated
Show resolved
Hide resolved
...in/webapp/app/exercises/shared/feedback/standalone-feedback/standalone-feedback.component.ts
Show resolved
Hide resolved
src/main/webapp/app/exercises/shared/exercise/exercise.service.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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.
Actionable comments posted: 2
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.
Actionable comments posted: 1
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.
Code LGTM
Checklist
General
Server
Client
Motivation and Context
When loading the exercise details, many different calls get executed aside of the main call. We can simplify this in one call and improve the performance.
Description
Because converting the whole call into a full DTO is more complex and we have to make certain decisions beforehand, we decided to use a Wrapper DTO, which functions similarly to the separate calls but without the separate calls.
Steps for Testing
Prerequisites:
Feel free to test more, but since I have not yet converted the exercise entity to a DTO, it contains the very same information as before (apart from an additional submission policy).
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Summary by CodeRabbit
New Features
ExerciseDetailsDTO
to encapsulate exercise details along with related settings and hints.Enhancements
Bug Fixes
Refactor
ExerciseDetailsDTO
for handling exercise details more comprehensively.