-
Notifications
You must be signed in to change notification settings - Fork 276
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
: Convert classes into records
#8571
base: develop
Are you sure you want to change the base?
Conversation
Development
: Convert classes into records
# Conflicts: # src/main/java/de/tum/in/www1/artemis/service/connectors/apollon/ApollonConversionService.java
# Conflicts: # src/main/java/de/tum/in/www1/artemis/service/connectors/athena/AthenaFeedbackSuggestionsService.java
# Conflicts: # src/main/java/de/tum/in/www1/artemis/service/exam/ExamService.java # src/main/java/de/tum/in/www1/artemis/web/rest/dto/ExamChecklistDTO.java # src/test/java/de/tum/in/www1/artemis/exam/ExamIntegrationTest.java # src/test/java/de/tum/in/www1/artemis/service/exam/ExamServiceTest.java
Fix AthenaSubmissionSendingServiceTest Fix AthenaFeedbackSendingServiceTest Improve TextAssessmentUpdateDTO
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 overall. I have some suggestions for improvements, but those can also be added in a follow-up.
Key Points (for the future):
- IMO DTOs should support null values for collections, so we should remove the compact constructors setting empty collections
- IMO we should use
of(...)
methods in the DTO rather thanasDTO(...)
methods in the entity
.../tum/in/www1/artemis/service/connectors/gitlab/dto/GitLabPersonalAccessTokenResponseDTO.java
Outdated
Show resolved
Hide resolved
...e/tum/in/www1/artemis/service/connectors/gitlab/dto/GitLabPersonalAccessTokenRequestDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/www1/artemis/service/connectors/gitlab/dto/GitLabCommitDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/www1/artemis/service/exam/ExamService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/www1/artemis/service/metis/ConversationMessagingService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/www1/artemis/web/rest/dto/AssessmentUpdateDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/www1/artemis/web/rest/dto/PostContextFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/www1/artemis/web/rest/dto/examevent/ExamLiveEventDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/www1/artemis/domain/exam/event/ExamAttendanceCheckEvent.java
Show resolved
Hide resolved
src/test/java/de/tum/in/www1/artemis/architecture/ArchitectureTest.java
Outdated
Show resolved
Hide resolved
.../in/www1/artemis/service/connectors/gitlab/dto/GitLabPersonalAccessTokenListResponseDTO.java
Show resolved
Hide resolved
...e/tum/in/www1/artemis/service/connectors/gitlab/dto/GitLabPersonalAccessTokenRequestDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/www1/artemis/web/rest/dto/examevent/ExamWideAnnouncementEventDTO.java
Outdated
Show resolved
Hide resolved
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 looks good - few (maybe optional/debatable) changes explained in the comments
src/main/java/de/tum/in/www1/artemis/service/AssessmentService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/www1/artemis/service/connectors/athena/AthenaDTOConverterService.java
Outdated
Show resolved
Hide resolved
...main/java/de/tum/in/www1/artemis/service/connectors/athena/AthenaFeedbackSendingService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/www1/artemis/service/dto/athena/Feedback.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/www1/artemis/web/rest/StudentExamResource.java
Outdated
Show resolved
Hide resolved
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.
Tested participation and assessment of programming, text, modeling, and file upload exercises locally. Working as expected, did not encounter any null pointers or bad requests
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, thanks for implementing my many change requests 🙏🙏🙏
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.
Tested in testing session on TS5. Empty commits (empty additions/deletions/completely empty commits) work for programming exercises and empty submissions for all other exercise types also work (tested as part of an exam)
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.
tested during a testing session, works as expected for full on time submissions
found 2 issues, but guess they are not related to this pr
- When I am registered for the exam(as a student) and click on the exam button, I see an error message(course overview page)
- If the time on my machine differs from the server time, I cannot generate individual exams(as an instructor)
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.
tested in a testing session on ts5. Participated in a exam and tried various exercises and different scenarios. All work as expected.
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.
Tested on testing session ts5. Works as expected.
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.
Tested in the testing session on ts5, competencies are still working as expected
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.
Tested locally works as expected
Checklist
General
Server
Motivation and Context
The main motivation for this change is to take advantage of records, which were introduced in Java 14 and formally standardised in Java 16. Records provide a more concise and readable way to define immutable data carriers, which is the main purpose of DTOs.
Description
Steps for Testing
Prerequisites:
1 Instructor
1 Student
1 Tutor
1 Empty Course
This PR is best tested locally, as you have access to the server's logs there.
Look out for Bad Requests and Nullpointer Exceptions. They indicate that a Collection type that was never null by default before might be able to be null now.
Given the changed DTOs, try to break the communication between the client and the server (e.g. Write a complaint as a student and then answer it as an instructor to test the AssessmentUpdateDTO etc.). Please make sure to add what you've tested into your review description.
Likely edge cases include empty collections (e.g. an AssessmentUpdate with no feedbacks). The classes may have set these values to an empty collection by default, but the records do not, leading to potential errors when iterating directly over collections elsewhere in the code without first checking them for nullity.
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Review Progress
Code Review
Manual Tests
Exam Mode Test
Test Coverage