Development: Introduce text module API
#10257
Conversation
github-actions
bot
added
tests
server
WalkthroughThis pull request refactors text exercise and submission handling across multiple modules by replacing direct repository and service dependencies with optional API interfaces. Constructor signatures, field declarations, and method implementations were updated to use types such as Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resource
participant OptionalAPI as Optional Text API
participant Service as Underlying Service/Repository
Client->>Resource: Invoke text submission/exercise operation
Resource->>OptionalAPI: Check for API presence
alt API present
OptionalAPI->>Service: Process request (import/export/feedback)
Service-->>OptionalAPI: Return data/result
OptionalAPI-->>Resource: Forward response
Resource-->>Client: Return successful response
else API missing
OptionalAPI-->>Resource: Throw ApiNotPresentException
Resource-->>Client: Return error response
end
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: 1
🧹 Nitpick comments (18)
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.java (1)
95-95: Track the TODO comment about client form data.The TODO comment indicates a potential improvement in data handling that should be tracked.
Would you like me to create an issue to track this TODO item for implementing client form data reception through IrisMessageResource?
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java (1)
174-184: Consider using a builder pattern or parameter object for constructor injection.The constructor has a large number of parameters (20+), which makes it hard to maintain and understand. Consider refactoring to use either:
- A builder pattern
- Parameter objects grouping related dependencies
- Splitting the class into smaller, more focused components
This would improve readability and maintainability.
Example approach using parameter objects:
-public ParticipationResource(ParticipationService participationService, - ProgrammingExerciseParticipationService programmingExerciseParticipationService, - CourseRepository courseRepository, /* ... more parameters ... */ - Optional<TextFeedbackApi> textFeedbackApi, - ModelingExerciseFeedbackService modelingExerciseFeedbackService) { +public ParticipationResource( + ParticipationServices participationServices, + RepositoryServices repositoryServices, + FeedbackServices feedbackServices) {Where
ParticipationServices,RepositoryServices, andFeedbackServicesare parameter objects grouping related dependencies.src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseDeletionService.java (1)
85-89: Consider breaking down the constructor parameters.While the constructor injection is correct, the number of parameters (16) suggests this class might have too many responsibilities.
Consider splitting this service into smaller, more focused services. For example:
- ExamExerciseDeletionService
- ProgrammingExerciseDeletionService
- TextExerciseDeletionService
src/main/java/de/tum/cit/aet/artemis/athena/service/AthenaDTOConverterService.java (2)
47-47: Consider avoiding Optional fieldsSome guidelines recommend passing in a concrete or null object rather than keeping
Optionalas a field. This helps keep the handling localized. However, if module presence is truly optional, this may be acceptable.
120-121: Prefer a functional Optional patternInstead of
isPresent()followed byget(), consider a more streamlined approach:-if (feedback.getReference() != null && textApi.isPresent()) { - feedbackTextBlock = textApi.get().findById(feedback.getReference()).orElse(null); -} +feedbackTextBlock = textApi + .flatMap(api -> Optional.ofNullable(feedback.getReference()) + .flatMap(api::findById)) + .orElse(null);This reduces branching and leverages
Optionalmethods more effectively.src/main/java/de/tum/cit/aet/artemis/plagiarism/service/TextPlagiarismDetectionService.java (1)
44-44: Optional as a fieldConsider whether always injecting a concrete API or a no-op would simplify usage instead of storing an
Optional.src/main/java/de/tum/cit/aet/artemis/assessment/web/ExampleSubmissionResource.java (1)
66-66: Optional field usageStoring an
Optionalin a field is a design choice; consider providing a null-object pattern or mandatory injection for clarity.src/main/java/de/tum/cit/aet/artemis/text/api/TextFeedbackApi.java (1)
14-14: Add JavaDoc documentation for class and method.Please add JavaDoc documentation to:
- Class level: Document the purpose and responsibility of this API
- Method level: Document parameters, return value, and any exceptions
+/** + * API for handling text exercise feedback operations. + */ public class TextFeedbackApi extends AbstractTextApi { + /** + * Handles a non-graded feedback request for a text exercise. + * + * @param participation The student participation to process + * @param textExercise The text exercise context + * @return The updated student participation + */ public StudentParticipation handleNonGradedFeedbackRequest(StudentParticipation participation, TextExercise textExercise) {Also applies to: 22-24
src/main/java/de/tum/cit/aet/artemis/text/api/TextSubmissionApi.java (1)
16-16: Add JavaDoc documentation and clarify method behavior.
- Add class-level JavaDoc to document the API's purpose
- The
handleTextSubmissionmethod has a side effect of hiding details. This should be explicit in the method name or documented.+/** + * API for managing text submissions, including creation, retrieval, and processing operations. + */ public class TextSubmissionApi extends AbstractTextApi { + /** + * Processes a text submission and hides sensitive details for the user. + * + * @param textSubmission The submission to process + * @param exercise The associated exercise + * @param user The user context + * @return The processed submission with hidden details + */ public TextSubmission handleTextSubmission(TextSubmission textSubmission, TextExercise exercise, User user) {Also applies to: 35-39
src/main/java/de/tum/cit/aet/artemis/text/api/TextExerciseImportApi.java (1)
17-17: Improve method naming and add documentation.
- Add class-level JavaDoc
- The overloaded
importTextExercisemethods could be confusing. Consider renaming the second method to be more specific.+/** + * API for importing text exercises, including finding and copying exercises with their associated data. + */ public class TextExerciseImportApi extends AbstractTextApi { - public Optional<TextExercise> importTextExercise(final long templateExerciseId, final TextExercise exerciseToCopy) { + public Optional<TextExercise> importTextExerciseFromTemplate(final long templateExerciseId, final TextExercise exerciseToCopy) { final Optional<TextExercise> optionalOriginalTextExercise = textExerciseRepository.findWithExampleSubmissionsAndResultsById(templateExerciseId); return optionalOriginalTextExercise.map(textExercise -> textExerciseImportService.importTextExercise(textExercise, exerciseToCopy)); }Also applies to: 40-43
src/main/java/de/tum/cit/aet/artemis/text/api/TextApi.java (1)
20-20: Add documentation and consider returning saved entity.
- Add class-level JavaDoc
- The
savemethod should return the saved entity to follow JPA repository pattern and allow access to any generated IDs or updated fields.+/** + * Primary API for text exercise operations, providing access to text exercises and blocks. + * This API serves as the main entry point for text-related functionality. + */ public class TextApi extends AbstractTextApi { - public void save(TextExercise exercise) { - textExerciseRepository.save(exercise); + public TextExercise save(TextExercise exercise) { + return textExerciseRepository.save(exercise); }Also applies to: 55-57
src/main/java/de/tum/cit/aet/artemis/text/api/TextSubmissionImportApi.java (1)
38-45: Consider adding transaction boundary.While the implementation is correct, consider adding
@Transactional(readOnly = true)to the method since it's primarily a read operation with a single write at the end.+ @Transactional(readOnly = true) public TextSubmission importStudentSubmission(long submissionId, long exerciseId, Map<Long, GradingInstruction> gradingInstructionCopyTracker) {src/main/java/de/tum/cit/aet/artemis/text/api/TextSubmissionExportApi.java (2)
42-44: Add JavaDoc for public methods.Public methods should be documented with JavaDoc, including parameters and potential exceptions.
+ /** + * Saves a text submission to a file. + * + * @param submission The text submission to save + * @param studentLogin The student's login identifier + * @param submissionsFolderName The target folder name + * @throws IOException If there's an error writing to the file + */ public void saveSubmissionToFile(TextSubmission submission, String studentLogin, String submissionsFolderName) throws IOException {
56-64: Refactor complex conditional logic.The method contains nested conditionals and multiple operations. Consider extracting the block computation logic into a separate method.
public void prepareTextBlockForExampleSubmission(long exampleSubmissionId) { Optional<TextSubmission> textSubmission = textSubmissionRepository.findWithEagerResultsAndFeedbackAndTextBlocksById(exampleSubmissionId); - if (textSubmission.isPresent() && textSubmission.get().getLatestResult() == null - && (textSubmission.get().getBlocks() == null || textSubmission.get().getBlocks().isEmpty())) { - TextSubmission submission = textSubmission.get(); - textBlockService.computeTextBlocksForSubmissionBasedOnSyntax(submission); - textBlockService.saveAll(submission.getBlocks()); - } + textSubmission.ifPresent(this::computeAndSaveTextBlocksIfNeeded); + } + + private void computeAndSaveTextBlocksIfNeeded(TextSubmission submission) { + if (submission.getLatestResult() == null && + (submission.getBlocks() == null || submission.getBlocks().isEmpty())) { + textBlockService.computeTextBlocksForSubmissionBasedOnSyntax(submission); + textBlockService.saveAll(submission.getBlocks()); + } }src/main/java/de/tum/cit/aet/artemis/assessment/service/ExampleSubmissionService.java (1)
137-141: Consider extracting API access pattern.The pattern of accessing optional API and throwing exception is repeated across the codebase. Consider extracting it into a utility method.
+ private <T> T getRequiredApi(Optional<T> api, Class<?> apiClass) { + return api.orElseThrow(() -> new ApiNotPresentException(apiClass, PROFILE_CORE)); + } if (exercise instanceof TextExercise) { - var api = textSubmissionImportApi.orElseThrow(() -> new ApiNotPresentException(TextSubmissionApi.class, PROFILE_CORE)); + var api = getRequiredApi(textSubmissionImportApi, TextSubmissionApi.class); TextSubmission textSubmission = api.importStudentSubmission(submissionId, exercise.getId(), gradingInstructionCopyTracker); newExampleSubmission.setSubmission(textSubmission); }src/main/java/de/tum/cit/aet/artemis/athena/web/AthenaResource.java (1)
168-172: Consider consolidating API presence checks.The current implementation checks for API presence separately for each API. Consider consolidating these checks to provide a more cohesive error message when multiple APIs are missing.
- var api = textApi.orElseThrow(() -> new ApiNotPresentException(TextApi.class, PROFILE_CORE)); - var submissionApi = textSubmissionApi.orElseThrow(() -> new ApiNotPresentException(TextSubmissionApi.class, PROFILE_CORE)); - - return getFeedbackSuggestions(exerciseId, submissionId, api::findByIdElseThrow, submissionApi::findByIdElseThrow, + return getFeedbackSuggestions(exerciseId, submissionId, + textApi.orElseThrow(() -> new ApiNotPresentException(TextApi.class, PROFILE_CORE))::findByIdElseThrow, + textSubmissionApi.orElseThrow(() -> new ApiNotPresentException(TextSubmissionApi.class, PROFILE_CORE))::findByIdElseThrow, athenaFeedbackSuggestionsService::getTextFeedbackSuggestions);src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.java (1)
117-121: Consider improving parameter formatting for better readability.The constructor parameters are correctly defined but could be formatted better for readability.
- ModelingExerciseImportService modelingExerciseImportService, Optional<TextExerciseImportApi> textExerciseImportApi, QuizExerciseRepository quizExerciseRepository, - QuizExerciseImportService quizExerciseImportService, LectureRepository lectureRepository, LectureImportService lectureImportService, - LectureUnitRepository lectureUnitRepository, LectureUnitImportService lectureUnitImportService, CourseCompetencyRepository courseCompetencyRepository, - ProgrammingExerciseTaskRepository programmingExerciseTaskRepository, GradingCriterionRepository gradingCriterionRepository, - CompetencyExerciseLinkRepository competencyExerciseLinkRepository, CompetencyLectureUnitLinkRepository competencyLectureUnitLinkRepository) { + ModelingExerciseImportService modelingExerciseImportService, + Optional<TextExerciseImportApi> textExerciseImportApi, + QuizExerciseRepository quizExerciseRepository, + QuizExerciseImportService quizExerciseImportService, + LectureRepository lectureRepository, + LectureImportService lectureImportService, + LectureUnitRepository lectureUnitRepository, + LectureUnitImportService lectureUnitImportService, + CourseCompetencyRepository courseCompetencyRepository, + ProgrammingExerciseTaskRepository programmingExerciseTaskRepository, + GradingCriterionRepository gradingCriterionRepository, + CompetencyExerciseLinkRepository competencyExerciseLinkRepository, + CompetencyLectureUnitLinkRepository competencyLectureUnitLinkRepository) {src/main/java/de/tum/cit/aet/artemis/core/service/export/CourseExamExportService.java (1)
425-426: Consider handling missing API case explicitly.The current implementation silently skips text exercise export when the API is not present. Consider logging a warning or adding it to exportErrors for better visibility.
- case TextExercise textExercise -> textSubmissionExportApi.ifPresent(api -> exportedExercises - .add(api.exportTextExerciseWithSubmissions(textExercise, submissionsExportOptions, exerciseExportDir, exportErrors, reportData))); + case TextExercise textExercise -> { + if (textSubmissionExportApi.isEmpty()) { + logMessageAndAppendToList( + "Skipped export of text exercise '" + textExercise.getTitle() + "' (id: " + textExercise.getId() + "): Text submission export API not available", + exportErrors, + null + ); + } else { + textSubmissionExportApi.get().exportTextExerciseWithSubmissions( + textExercise, + submissionsExportOptions, + exerciseExportDir, + exportErrors, + reportData + ); + } + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ExampleSubmissionService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/assessment/web/ExampleSubmissionResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/athena/service/AthenaDTOConverterService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/athena/web/AthenaResource.java(6 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/core/exception/ApiNotPresentException.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/export/CourseExamExportService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamImportService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/StudentExamService.java(6 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseDeletionService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationTeamWebsocketService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.java(6 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/settings/IrisSettingsService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/iris/web/IrisTextExerciseChatSessionResource.java(7 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/service/TextPlagiarismDetectionService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/text/api/AbstractTextApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/text/api/TextApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/text/api/TextExerciseImportApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/text/api/TextFeedbackApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/text/api/TextSubmissionApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/text/api/TextSubmissionExportApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/text/api/TextSubmissionImportApi.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaFeedbackSendingServiceTest.java(4 hunks)src/test/java/de/tum/cit/aet/artemis/text/architecture/TextApiArchitectureTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/de/tum/cit/aet/artemis/text/api/AbstractTextApi.java
🧰 Additional context used
📓 Path-based instructions (24)
src/test/java/de/tum/cit/aet/artemis/text/architecture/TextApiArchitectureTest.java (1)
Pattern 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/main/java/de/tum/cit/aet/artemis/core/exception/ApiNotPresentException.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/text/api/TextFeedbackApi.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/text/api/TextSubmissionImportApi.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/text/api/TextSubmissionApi.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/plagiarism/service/TextPlagiarismDetectionService.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/web/ParticipationTeamWebsocketService.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/iris/web/IrisTextExerciseChatSessionResource.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/service/ExerciseDeletionService.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/exam/service/StudentExamService.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/iris/service/session/IrisTextExerciseChatSessionService.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/atlas/service/LearningObjectImportService.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/athena/service/AthenaDTOConverterService.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/assessment/service/ExampleSubmissionService.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/text/api/TextSubmissionExportApi.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/text/api/TextApi.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/core/service/export/CourseExamExportService.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/text/api/TextExerciseImportApi.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/web/ParticipationResource.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/assessment/web/ExampleSubmissionResource.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/exam/service/ExamImportService.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/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaFeedbackSendingServiceTest.java (1)
Pattern 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/main/java/de/tum/cit/aet/artemis/iris/service/settings/IrisSettingsService.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/athena/web/AthenaResource.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
📓 Learnings (2)
src/main/java/de/tum/cit/aet/artemis/exam/service/StudentExamService.java (2)
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:296-300
Timestamp: 2024-11-12T12:51:40.391Z
Learning: When reviewing code changes in `StudentExamService.saveSubmission`, if the PR aims to improve readability without changing logic, avoid suggesting changes that alter logic, such as adding exceptions in the default case of switch statements.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:266-267
Timestamp: 2024-11-12T12:51:58.050Z
Learning: When reviewing code in this project, avoid suggesting code changes that are outside the scope of the PR.
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.java (1)
Learnt from: alexjoham
PR: ls1intum/Artemis#9455
File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401
Timestamp: 2024-11-12T12:51:58.050Z
Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (61)
src/main/java/de/tum/cit/aet/artemis/iris/service/settings/IrisSettingsService.java (3)
13-13: LGTM! Import changes align with modularization.The addition of
OptionalandTextApiimports supports the transition from direct repository dependency to optional API interface.Also applies to: 52-52
76-76: LGTM! Field and constructor changes follow best practices.The changes:
- Use constructor injection
- Maintain immutability with
finalfield- Use
Optionalto make the text module dependency optionalAlso applies to: 78-86
384-387: LGTM! Method changes handle optional API correctly.The changes properly handle the optional text API using
ifPresentwhile maintaining the same functionality.src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.java (3)
3-4: LGTM! Import statements are well-organized.The new imports are properly organized and follow Java conventions by avoiding star imports.
Also applies to: 13-13, 36-36
51-51: LGTM! Constructor injection and Optional API usage are well implemented.The changes follow best practices:
- Constructor injection is used for dependency management
- Optional aligns with the modularization effort
Also applies to: 63-77
90-90: LGTM! Proper error handling with Optional API.The change properly handles the case when TextApi is not present, providing a clear error message that includes the required profile.
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java (4)
102-102: LGTM!The import is correctly placed and follows Java import best practices by using a specific import rather than a wildcard.
170-170: LGTM!The field declaration correctly uses
Optionalfor the API dependency, following best practices for handling optional services. The field is properly declared as final and follows naming conventions.
210-210: LGTM!The field initialization is correctly implemented, properly assigning the constructor parameter to the final field.
424-425: LGTM!The code elegantly handles the optional text feedback API using functional programming style. It correctly:
- Uses
mapto handle the API call when present- Falls back to the original participation when the API is absent
- Maintains immutability by not modifying the original participation
src/main/java/de/tum/cit/aet/artemis/iris/web/IrisTextExerciseChatSessionResource.java (10)
3-4: Import forPROFILE_CORE
This static import is utilized later forApiNotPresentExceptioninitialization at lines 72, 100, and 122. No issues observed.
8-8: Adoption ofOptional
IntroducingOptionalaligns with the new approach of conditionally available APIs. No concerns here.
19-19: New exception import
ApiNotPresentExceptionimport facilitates clear error signaling whenTextApiis unavailable. Looks good.
29-29: NewTextApiimport
Brings in the API for text exercises. This is consistent with the PR’s objective of modularizing text functionality.
46-46: Use ofOptional<TextApi>
MaintainingtextApias anOptionalhelps gracefully handle its absence. Code is in line with the new design.
52-53: Constructor injection withOptional<TextApi>
AcceptingtextApias an optional dependency is a clean way to manage the optional module. No issues identified.
59-59: Field assignment
Straightforward assignment oftextApito the class field. Implementation is correct.
72-72: Optional usage for exercise retrieval
UsingorElseThrowprovides explicit handling whenTextApiis missing. The pattern is concise and clear.
100-100: Same pattern oforElseThrow
Once again, handling a missingTextApiconsistently. No issues spotted.
122-122: Consistent error handling
Reusing the same approach to retrieveTextApiensures uniformity. Implementation is sound.src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseDeletionService.java (3)
41-41: LGTM! Good use of Optional API.The change from direct service dependency to
Optional<TextApi>aligns well with the modular API approach and follows Java best practices for handling optional dependencies.Also applies to: 75-75
167-170: LGTM! Proper null-safe operation handling.The use of
Optional.ifPresent()ensures null-safe operation cancellation for text exercises. This is a good practice as it:
- Prevents NPEs
- Makes the optional nature of the text module explicit
- Follows the new modular architecture
150-218: Consider adding transaction boundaries.The delete operation performs multiple database operations but lacks explicit transaction management.
Consider adding
@Transactionalto ensure atomic deletion:+@Transactional public void delete(long exerciseId, boolean deleteStudentReposBuildPlans, boolean deleteBaseReposBuildPlans) {This ensures that if any step fails, all changes are rolled back, maintaining data consistency.
src/main/java/de/tum/cit/aet/artemis/athena/service/AthenaDTOConverterService.java (5)
4-6: Static imports and Optional usage are acceptableNo immediate concerns with these import changes. They comply with the "no star imports" guideline and follow the new optional-based design.
26-26: Custom exception importIntroducing
ApiNotPresentExceptionis a consistent approach for handling the absence of the text module.
33-33: Import forTextApiThis aligns with the shift to an API-based approach for text operations.
53-55: Constructor now acceptsOptional<TextApi>This is consistent with the modular approach. Verify that unit tests cover scenarios where
textApiis empty.
70-70: UsingorElseThrowCalling
orElseThrowfor the absent module is clear and ensures a correct fail-fast mechanism.src/main/java/de/tum/cit/aet/artemis/plagiarism/service/TextPlagiarismDetectionService.java (3)
26-26: Introduced imports for custom exception and optional export APIThese new imports match the refactoring to handle missing text submission exports gracefully.
Also applies to: 34-34
52-54: Updated constructor signaturePassing
Optional<TextSubmissionExportApi>ensures dynamic behavior depending on module availability. Confirm that the missing-API scenario is tested.
131-132: Fail-fast withorElseThrowGood approach to highlight that text submission export isn't available if the API is missing. Ensure higher-level handlers display a clear error message.
src/main/java/de/tum/cit/aet/artemis/assessment/web/ExampleSubmissionResource.java (4)
27-27: Imports forApiNotPresentExceptionandTextSubmissionExportApiNo issues. These imports support the new optional module pattern.
Also applies to: 39-39
69-70: New constructor parametersIncluding
Optional<TextSubmissionExportApi>is consistent with the rest of the refactoring. Confirm that attempts to use the API when absent are properly tested.Also applies to: 74-74
134-135: Preparing text blocksUsing
orElseThrowcorrectly fails if the API isn't available, rather than silently ignoring missing functionality.
166-167: Retrieving text submissionAgain, the
orElseThrowapproach provides immediate feedback if the API is missing. Logging or user-facing error messages might help pinpoint issues.src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationTeamWebsocketService.java (5)
37-37: Import for ApiNotPresentException is appropriate
This new import aligns with the refactoring towards optional APIs and explicit exception handling.
52-52: Import for TextSubmissionApi is consistent with modular design
Using theTextSubmissionApipromotes cleaner separation of concerns and avoids direct repository dependencies.
72-72: Optional dependency is well-introduced
DeclaringOptional<TextSubmissionApi>is a clean approach to handle the absence of the text API without breaking other workflows.
85-85: Constructor parameter injection is aptly refactored
PassingOptional<TextSubmissionApi>into the constructor ensures compliance with dependency injection best practices.Also applies to: 92-92
219-220: Graceful handling of missing textSubmissionApi
ThrowingApiNotPresentExceptionprevents silent issues when the text API is unavailable. This is a good pattern for optional module behavior.src/test/java/de/tum/cit/aet/artemis/text/architecture/TextApiArchitectureTest.java (1)
1-11: Architecture test class adheres to test naming and structure
This new test extends the established architecture test framework. No issues found.src/main/java/de/tum/cit/aet/artemis/core/exception/ApiNotPresentException.java (1)
1-19: New exception class is well-defined
This custom runtime exception clearly indicates missing API profiles and helps guide users to enable the correct Spring profile.src/main/java/de/tum/cit/aet/artemis/text/api/TextSubmissionImportApi.java (1)
17-28: LGTM! Well-structured class with proper dependency injection.The class follows best practices:
- Uses constructor injection as recommended
- Follows single responsibility principle
- Has appropriate Spring annotations
src/main/java/de/tum/cit/aet/artemis/athena/web/AthenaResource.java (4)
69-71: LGTM! Well-structured dependency injection.The change from direct repository dependencies to optional APIs follows best practices for dependency injection and modularity.
168-172: LGTM! Robust error handling for optional APIs.The implementation correctly handles the optional APIs by throwing
ApiNotPresentExceptionwhen the required APIs are not present. This provides clear error messaging and maintains the contract with the client.
69-71: LGTM! Well-structured optional API dependencies.The use of Optional for API dependencies follows best practices for handling optional components in a modular system.
92-99: LGTM! Clean constructor injection.The constructor properly handles optional dependencies through constructor injection, following Spring best practices.
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamImportService.java (2)
207-211: LGTM! Clean pattern matching and error handling.The implementation effectively uses pattern matching in the switch case and properly handles the optional API with appropriate error handling. The code maintains consistency with other exercise type handlers.
207-211: LGTM! Clean handling of optional API in switch expression.The implementation correctly:
- Uses pattern matching in switch expression
- Handles the optional API presence check
- Maintains consistent error handling with other cases
src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.java (4)
90-90: LGTM! Well-structured dependency injection.The change from direct repository dependencies to optional APIs follows best practices for dependency injection and modularity.
207-211: LGTM! Robust error handling for optional APIs.The implementation correctly handles the optional API by throwing
ApiNotPresentExceptionwhen the required API is not present. The code maintains consistency with other exercise type handlers.
90-90: LGTM! Well-placed optional API dependency.The field declaration follows the established pattern for optional API dependencies.
207-211: LGTM! Clean error handling for optional API.The implementation properly handles the optional API presence check and maintains consistent error handling with other cases.
src/main/java/de/tum/cit/aet/artemis/core/service/export/CourseExamExportService.java (4)
70-70: LGTM! Well-structured dependency injection.The change from direct repository dependencies to optional APIs follows best practices for dependency injection and modularity.
425-426: LGTM! Appropriate handling of optional API for export operations.The implementation correctly uses
ifPresent()for the optional API instead of throwing an exception. This is appropriate for export operations where the absence of the API should be handled gracefully by skipping the export.
70-70: LGTM! Well-structured optional API dependency.The field declaration follows the established pattern for optional API dependencies.
85-86: LGTM! Clean constructor injection.The constructor properly handles the optional dependency through constructor injection.
src/main/java/de/tum/cit/aet/artemis/exam/service/StudentExamService.java (4)
38-38: LGTM!The new imports are correctly added to support the API-based approach.
Also applies to: 76-76
113-113: LGTM!The field change correctly implements the optional API-based approach, replacing the direct repository dependency.
130-130: LGTM!The constructor changes correctly implement constructor injection for the optional API.
Also applies to: 141-141
318-318: LGTM!The text submission handling correctly uses the optional API with proper error handling when the API is not present.
.../java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaFeedbackSendingServiceTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ExampleSubmissionService.java (1)
137-139: Enhance error handling and documentation.While the implementation is correct, consider these improvements:
- Make the error message more specific about which functionality is unavailable
- Add JavaDoc to document the API dependency requirement
Apply this diff to enhance the implementation:
+ /** + * Imports a student submission as an example submission. + * Requires TextSubmissionImportApi to be present when handling TextExercise submissions. + * + * @throws ApiNotPresentException if TextSubmissionImportApi is not available for TextExercise + */ public ExampleSubmission importStudentSubmissionAsExampleSubmission(Long submissionId, Exercise exercise) { // ... existing code ... if (exercise instanceof TextExercise) { - var api = textSubmissionImportApi.orElseThrow(() -> new ApiNotPresentException(TextSubmissionApi.class, PROFILE_CORE)); + var api = textSubmissionImportApi.orElseThrow(() -> + new ApiNotPresentException(TextSubmissionApi.class, "Text submission import functionality is not available in " + PROFILE_CORE)); TextSubmission textSubmission = api.importStudentSubmission(submissionId, exercise.getId(), gradingInstructionCopyTracker); newExampleSubmission.setSubmission(textSubmission); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ExampleSubmissionService.java(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ExampleSubmissionService.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
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ExampleSubmissionService.java (3)
18-18: LGTM: Clean import declarations.The new imports are well-organized and follow Java conventions.
Also applies to: 27-28
46-46: LGTM: Well-structured field declaration.Good use of Optional for API dependency, promoting loose coupling and proper handling of optional modules.
52-54: LGTM: Clean constructor injection.Constructor properly handles the optional API dependency, following dependency injection best practices.
Also applies to: 59-59
helios-aet
bot
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
# Conflicts: # src/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaFeedbackSendingServiceTest.java
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
There was a problem hiding this comment.
Tested on TS6.
- Create a text exercise: Works fine
- Update the exercise: Works fine
- Make a submission: Works fine
- Create an example submission: Works fine
- Import the student submission as example submission: I made a submission with Test User 1 but somehow couldn't find it to import
- Import the exercise into a new course: Works fine but I wanted to ask whether it is an expected behavior to allow to import exercises of a course into that same course. I tried it and it still works, even though it shows Internal Server Error
There was a problem hiding this comment.
Create a text exercise: Works fine
Update the exercise: Works fine
Make a submission: Works fine
Create an example submission: Works fine
Import the student submission as example submission: I made a submission with Test User 3 but somehow couldn't find it to import
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/core/service/export/CourseExamExportService.java (1)
425-426: Consider handling the case when the API is not present.The current implementation silently skips text exercise export when the API is not present. Consider logging a warning message to help with debugging.
- case TextExercise textExercise -> textSubmissionExportApi.ifPresent(api -> exportedExercises - .add(api.exportTextExerciseWithSubmissions(textExercise, submissionsExportOptions, exerciseExportDir, exportErrors, reportData))); + case TextExercise textExercise -> { + if (textSubmissionExportApi.isEmpty()) { + log.warn("Skipping export of text exercise '{}' (id: {}) because text submission export API is not present", + textExercise.getTitle(), textExercise.getId()); + } + textSubmissionExportApi.ifPresent(api -> exportedExercises + .add(api.exportTextExerciseWithSubmissions(textExercise, submissionsExportOptions, exerciseExportDir, exportErrors, reportData))); + }src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.java (1)
207-211: Enhance error message for API not present exception.While the implementation is correct, consider providing a more descriptive error message that explains the impact of the missing API.
- var api = textExerciseImportApi.orElseThrow(() -> new ApiNotPresentException(TextExerciseImportApi.class, PROFILE_CORE)); + var api = textExerciseImportApi.orElseThrow(() -> + new ApiNotPresentException(TextExerciseImportApi.class, PROFILE_CORE, + "Text exercise import functionality is not available. Please ensure the text module is enabled."));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/export/CourseExamExportService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseDeletionService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/service/TextPlagiarismDetectionService.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/de/tum/cit/aet/artemis/plagiarism/service/TextPlagiarismDetectionService.java
- src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java
- src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseDeletionService.java
🧰 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/core/service/export/CourseExamExportService.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.java
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: Analyse
🔇 Additional comments (6)
src/main/java/de/tum/cit/aet/artemis/core/service/export/CourseExamExportService.java (3)
51-51: LGTM!The import of
TextSubmissionExportApialigns with the modularization effort.
70-70: LGTM!The field declaration using
Optional<TextSubmissionExportApi>follows best practices for handling optional dependencies.
84-85: LGTM!The constructor properly injects the optional text submission export API and initializes the field correctly.
Also applies to: 91-91
src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.java (3)
40-40: LGTM! Import changes align with modularization.The addition of
ApiNotPresentExceptionandTextExerciseImportApiimports supports the transition to a modular API-based approach.Also applies to: 64-64
90-90: LGTM! Field declaration follows best practices.The use of
Optional<TextExerciseImportApi>correctly handles the optional dependency, and the field is properly declared as final.
117-121: LGTM! Constructor changes follow dependency injection best practices.The constructor properly handles the optional text exercise API dependency through constructor injection.
Also applies to: 129-129
|
@HawKhiem @HanyangXu0508 Thanks a lot for testing. I just added it to the testing steps but did you set valid dates for exercise? Also, did you assess the submission first? A submission can only be the example submission if it already has been graded. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/assessment/web/ExampleSubmissionResource.java (1)
134-135: Consider extracting API error handling to improve consistency.The error handling for missing API is duplicated. Consider extracting it to a helper method for better maintainability.
+ private <T> T getTextApi(Class<T> apiClass) { + return (T) textSubmissionExportApi.orElseThrow( + () -> new ApiNotPresentException(apiClass, PROFILE_CORE)); + } - textSubmissionExportApi.orElseThrow(() -> new ApiNotPresentException(TextSubmissionExportApi.class, PROFILE_CORE)) + getTextApi(TextSubmissionExportApi.class) .prepareTextBlockForExampleSubmission(exampleSubmission.getSubmission().getId()); - Optional<TextSubmission> textSubmission = textSubmissionExportApi.orElseThrow(() -> new ApiNotPresentException(TextSubmissionExportApi.class, PROFILE_CORE)) + Optional<TextSubmission> textSubmission = getTextApi(TextSubmissionExportApi.class) .getSubmissionForExampleSubmission(exampleSubmission.getSubmission().getId());Also applies to: 166-167
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/assessment/web/ExampleSubmissionResource.java(5 hunks)
🧰 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/assessment/web/ExampleSubmissionResource.java
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: server-style
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/assessment/web/ExampleSubmissionResource.java (3)
27-27: LGTM! Clean dependency management.The introduction of
TextSubmissionExportApias an optional dependency promotes loose coupling and modular design.Also applies to: 39-39, 66-66
68-75: LGTM! Clean constructor injection.The constructor properly follows dependency injection best practices with clear parameter naming and initialization.
134-136: LGTM! Clean API integration pattern.The implementation properly handles the optional API dependency and delegates text block preparation appropriately.
helios-aet
bot
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
Checklist
General
Server
Motivation and Context
As part of the server modularization, we "facade" the service/repository declaration and method calls to module-external classes via a module API provided by the respective module.
Description
These changes add the module API for text exercises.
Out of scope: Adding a
PROFILE_TEXTto disable the module on server start. This requires a lot of frontend changes and testing.Steps for Testing
Basically try to test all the functionality of text exercises regarding submissions, courses, and import/export.
Exam Mode Testing
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Summary by CodeRabbit
New Features
Refactor
Tests