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

Development: Introduce text module API #10257

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

Conversation

ole-ve
Copy link
Contributor

@ole-ve ole-ve commented Feb 4, 2025

Checklist

General

Server

  • I strictly followed the principle of data economy for all database calls.
  • I strictly followed the server coding and design guidelines.
  • I documented the Java code using JavaDoc style.

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_TEXT to 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.

  1. Log in to Artemis
  2. Create a text exercise
  3. Update the exercise
  4. Make a submission
  5. Create an example submission
  6. Import the student submission as example submission
  7. Import the exercise into a new course
  8. Delete the exercise in both courses

Exam Mode Testing

  1. Log in to Artemis
  2. Create a regular text exercise
  3. Create an exam
  4. Create a text exercise in the exam
  5. Import the created text exercise from 2 into the exam
  6. Participate in the exam and make submissions

Review Progress

Performance Review

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Exam Mode Test

  • Test 1
  • Test 2

Summary by CodeRabbit

  • New Features
    • Introduced new APIs for managing text exercises, submissions, feedback, and import/export operations, enhancing content management.
  • Refactor
    • Streamlined text processing by replacing direct data access with modular, optional API interfaces to improve reliability and error handling.
    • Enhanced integration across exam, participation, and plagiarism detection processes for a more robust experience.
  • Tests
    • Updated test suites and added architectural tests to validate the new text management workflows.

@ole-ve ole-ve self-assigned this Feb 4, 2025
@ole-ve ole-ve requested a review from a team as a code owner February 4, 2025 11:37
@github-actions github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) assessment Pull requests that affect the corresponding module athena Pull requests that affect the corresponding module atlas Pull requests that affect the corresponding module core Pull requests that affect the corresponding module exam Pull requests that affect the corresponding module exercise Pull requests that affect the corresponding module iris Pull requests that affect the corresponding module plagiarism Pull requests that affect the corresponding module text Pull requests that affect the corresponding module labels Feb 4, 2025
@ole-ve ole-ve mentioned this pull request Feb 4, 2025
11 tasks
Copy link

coderabbitai bot commented Feb 4, 2025

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 PMD (7.8.0)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ExampleSubmissionService.java

[ERROR] Error at ruleset.xml:58:5
56|
57|
58|
^^^^^ Unable to find referenced rule BooleanInstantiation; perhaps the rule name is misspelled?

59|
60|
[WARN] Warning at ruleset.xml:66:5
64|
65|
66|
^^^^^ Use Rule name category/java/bestpractices.xml/DefaultLabelNotLastInSwitch instead of the deprecated Rule name category/java/bestpractices.xml/DefaultLabelNotLastInSwitchStmt. PMD 8.0.0 will remove support for this deprecated Rule name usage.

67|
68|
[ERROR] Error at ruleset.xml:71:5
69|
70|
71|
^^^^^ Unable to find referenced rule DontImportJavaLang; perhaps the rule name is misspelled?

72|
73|
[ERROR] Error at ruleset.xml:75:5
73|
74|
75|
^^^^^ Unable to find referenced rule DuplicateImports; perhaps the rule name is misspelled?

76|
77|
[ERROR] Error at ruleset.xml:78:5
76|
77|
78|
^^^^^ Unable to find referenced rule EmptyFinallyBlock; perhaps the rule name is misspelled?

79|
80|
[ERROR] Error at ruleset.xml:79:5
77|
78|
79|
^^^^^ Unable to find referenced rule EmptyIfStmt; perhaps the rule name is misspelled?

80|
81|
[ERROR] Error at ruleset.xml:81:5
79|
80|
81|
^^^^^ Unable to find referenced rule EmptyInitializer; perhaps the rule name is misspelled?

82|
83|
[ERROR] Error at ruleset.xml:82:5
80|
81|
82|
^^^^^ Unable to find referenced rule EmptyStatementBlock; perhaps the rule name is misspelled?

83|
84|
[ERROR] Error at ruleset.xml:83:5
81|
82|
83|
^^^^^ Unable to find referenced rule EmptyStatementNotInLoop; perhaps the rule name is misspelled?

84|
85|
[ERROR] Error at ruleset.xml:84:5
82|
83|
84|
^^^^^ Unable to find referenced rule EmptySwitchStatements; perhaps the rule name is misspelled?

85|
86|
[ERROR] Error at ruleset.xml:85:5
83|
84|
85|
^^^^^ Unable to find referenced rule EmptySynchronizedBlock; perhaps the rule name is misspelled?

86|
87|
[ERROR] Error at ruleset.xml:86:5
84|
85|
86|
^^^^^ Unable to find referenced rule EmptyTryBlock; perhaps the rule name is misspelled?

87|
88|
[ERROR] Error at ruleset.xml:87:5
85|
86|
87|
^^^^^ Unable to find referenced rule EmptyWhileStmt; perhaps the rule name is misspelled?

88|
89|
[ERROR] Error at ruleset.xml:90:5
88|
89|
90|
^^^^^ Unable to find referenced rule ExcessiveClassLength; perhaps the rule name is misspelled?

91|
92|
[ERROR] Error at ruleset.xml:91:5
89|
90|
91|
^^^^^ Unable to find referenced rule ExcessiveMethodLength; perhaps the rule name is misspelled?

92|
93|
[ERROR] Error at ruleset.xml:106:5
104|
105|
106|
^^^^^ Unable to find referenced rule ImportFromSamePackage; perhaps the rule name is misspelled?

107|
108|
[ERROR] Error at ruleset.xml:119:5
117|
118|
119|
^^^^^ Unable to find referenced rule MissingBreakInSwitch; perhaps the rule name is misspelled?

120|
121|
[WARN] Warning at ruleset.xml:124:5
122|
123|
124|
^^^^^ Use Rule name category/java/errorprone.xml/NonCaseLabelInSwitch instead of the deprecated Rule name category/java/errorprone.xml/NonCaseLabelInSwitchStatement. PMD 8.0.0 will remove support for this deprecated Rule name usage.

125|
126|
[ERROR] Error at ruleset.xml:134:9
132|
133| // It's okay to use short variable names in DTOs, e.g. "id" or "name"
134| ./de/tum/in/www1/artemis/web/rest/dto/.
^^^^^^^^^^^^^^^^ Unexpected element 'exclude-pattern' in rule ShortVariable

135|
136|
[ERROR] Error at ruleset.xml:137:5
135|
136|
137|
^^^^^ Unable to find referenced rule SimplifyBooleanAssertion; perhaps the rule name is misspelled?

138|
139|
[WARN] Warning at ruleset.xml:184:5
182|
183|
184|
^^^^^ Use Rule name category/ecmascript/errorprone.xml/InaccurateNumericLiteral instead of the deprecated Rule name category/ecmascript/errorprone.xml/InnaccurateNumericLiteral. PMD 8.0.0 will remove support for this deprecated Rule name usage.

185|
186|
[ERROR] Cannot load ruleset category/vm/bestpractices.xml: Cannot resolve rule/ruleset reference 'category/vm/bestpractices.xml'. Make sure the resource is a valid file or URL and is on the CLASSPATH. Use --debug (or a fine log level) to see the current classpath.
[WARN] Progressbar rendering conflicts with reporting to STDOUT. No progressbar will be shown. Try running with argument -r to output the report to a file instead.

Walkthrough

This pull request refactors text submission and exercise handling by replacing direct repository and service dependencies with optional API interfaces. Several modules—including assessment, exam, atlas, exercise, iris, plagiarism, and text—have been updated to use Optional APIs such as TextSubmissionImportApi, TextApi, TextFeedbackApi, and others. New API classes encapsulate text operations, and a new exception (ApiNotPresentException) is introduced to handle cases when an API is not available. Constructor signatures, method calls, and tests have been adjusted accordingly, centralizing text-related operations and improving error handling.

Changes

File(s) Change Summary
src/main/java/de/tum/cit/aet/artemis/assessment/service/ExampleSubmissionService.java, ExampleSubmissionResource.java Replaced direct text submission and import/export dependencies with Optional TextSubmissionImportApi/ExportApi; updated constructors and methods to handle text operations via API calls with proper exception handling.
src/main/java/de/tum/cit/aet/artemis/athena/service/AthenaDTOConverterService.java, AthenaResource.java Removed TextExerciseRepository and TextBlockRepository; introduced Optional for text handling; refactored methods and error handling for feedback and assessment logic.
src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.java, ExamImportService.java, StudentExamService.java Eliminated direct repository/service calls for text exercises and submissions; introduced Optional and Optional; modified method signatures and control flow to throw exceptions when APIs are absent.
src/main/java/de/tum/cit/aet/artemis/core/service/export/CourseExamExportService.java, .../plagiarism/TextPlagiarismDetectionService.java Replaced direct export and plagiarism service dependencies with Optional; added conditional API checks before executing export operations.
src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseDeletionService.java, ParticipationResource.java, ParticipationTeamWebsocketService.java Transitioned from using direct TextExerciseService and TextSubmissionService to Optional/Optional interfaces; updated constructors and method calls to perform API presence checks.
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.java, IrisSettingsService.java, IrisTextExerciseChatSessionResource.java Replaced direct TextExerciseRepository usage with Optional; modified constructors and methods to include API checks and throw ApiNotPresentException when missing.
src/main/java/de/tum/cit/aet/artemis/text/api/* (AbstractTextApi, TextApi, TextExerciseImportApi, TextFeedbackApi, TextSubmissionApi, TextSubmissionExportApi, TextSubmissionImportApi) Introduced new API classes that encapsulate text-related operations (import, export, feedback, submission); establish a common base via AbstractTextApi and integrate Spring controller profiles.
src/main/java/de/tum/cit/aet/artemis/core/exception/ApiNotPresentException.java Added a new exception class to handle cases when an expected API is not available, including a descriptive error message regarding Spring profile activation.
src/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaFeedbackSendingServiceTest.java, TextApiArchitectureTest.java Updated tests to use Optional mocks instead of repository mocks; added a new architecture test for the text module to validate module dependencies.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Service
    participant OptionalApi
    Client->>Service: Request text operation (e.g., import submission)
    Service->>OptionalApi: Check API via Optional.orElseThrow()
    alt API Present
        OptionalApi-->>Service: Return text data/result
        Service-->>Client: Respond with successful outcome
    else API Absent
        Service-->>Client: Throw ApiNotPresentException
    end
Loading

Suggested labels

programming

Suggested reviewers

  • krusche
  • SimonEntholzer
  • Hialus
  • EneaGore
  • BBesrour
  • MaximilianAnzinger
  • FelixTJDietrich

Tip

🌐 Web search-backed reviews and chat
  • We have enabled web search-based reviews and chat for all users. This feature allows CodeRabbit to access the latest documentation and information on the web.
  • You can disable this feature by setting web_search: false in the knowledge_base settings.
  • Please share any feedback in the Discord discussion.
✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

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: 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:

  1. A builder pattern
  2. Parameter objects grouping related dependencies
  3. 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, and FeedbackServices are 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 fields

Some guidelines recommend passing in a concrete or null object rather than keeping Optional as 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 pattern

Instead of isPresent() followed by get(), 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 Optional methods more effectively.

src/main/java/de/tum/cit/aet/artemis/plagiarism/service/TextPlagiarismDetectionService.java (1)

44-44: Optional as a field

Consider 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 usage

Storing an Optional in 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:

  1. Class level: Document the purpose and responsibility of this API
  2. 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.

  1. Add class-level JavaDoc to document the API's purpose
  2. The handleTextSubmission method 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.

  1. Add class-level JavaDoc
  2. The overloaded importTextExercise methods 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.

  1. Add class-level JavaDoc
  2. The save method 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

📥 Commits

Reviewing files that changed from the base of the PR and between f77fb84 and c8c8426.

📒 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 Optional and TextApi imports 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 final field
  • Use Optional to make the text module dependency optional

Also applies to: 78-86


384-387: LGTM! Method changes handle optional API correctly.

The changes properly handle the optional text API using ifPresent while 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 Optional for 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:

  1. Uses map to handle the API call when present
  2. Falls back to the original participation when the API is absent
  3. Maintains immutability by not modifying the original participation
src/main/java/de/tum/cit/aet/artemis/iris/web/IrisTextExerciseChatSessionResource.java (10)

3-4: Import for PROFILE_CORE
This static import is utilized later for ApiNotPresentException initialization at lines 72, 100, and 122. No issues observed.


8-8: Adoption of Optional
Introducing Optional aligns with the new approach of conditionally available APIs. No concerns here.


19-19: New exception import
ApiNotPresentException import facilitates clear error signaling when TextApi is unavailable. Looks good.


29-29: New TextApi import
Brings in the API for text exercises. This is consistent with the PR’s objective of modularizing text functionality.


46-46: Use of Optional<TextApi>
Maintaining textApi as an Optional helps gracefully handle its absence. Code is in line with the new design.


52-53: Constructor injection with Optional<TextApi>
Accepting textApi as an optional dependency is a clean way to manage the optional module. No issues identified.


59-59: Field assignment
Straightforward assignment of textApi to the class field. Implementation is correct.


72-72: Optional usage for exercise retrieval
Using orElseThrow provides explicit handling when TextApi is missing. The pattern is concise and clear.


100-100: Same pattern of orElseThrow
Once again, handling a missing TextApi consistently. No issues spotted.


122-122: Consistent error handling
Reusing the same approach to retrieve TextApi ensures 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:

  1. Prevents NPEs
  2. Makes the optional nature of the text module explicit
  3. 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 @Transactional to 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 acceptable

No 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 import

Introducing ApiNotPresentException is a consistent approach for handling the absence of the text module.


33-33: Import for TextApi

This aligns with the shift to an API-based approach for text operations.


53-55: Constructor now accepts Optional<TextApi>

This is consistent with the modular approach. Verify that unit tests cover scenarios where textApi is empty.


70-70: Using orElseThrow

Calling orElseThrow for 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 API

These new imports match the refactoring to handle missing text submission exports gracefully.

Also applies to: 34-34


52-54: Updated constructor signature

Passing Optional<TextSubmissionExportApi> ensures dynamic behavior depending on module availability. Confirm that the missing-API scenario is tested.


131-132: Fail-fast with orElseThrow

Good 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 for ApiNotPresentException and TextSubmissionExportApi

No issues. These imports support the new optional module pattern.

Also applies to: 39-39


69-70: New constructor parameters

Including 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 blocks

Using orElseThrow correctly fails if the API isn't available, rather than silently ignoring missing functionality.


166-167: Retrieving text submission

Again, the orElseThrow approach 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 the TextSubmissionApi promotes cleaner separation of concerns and avoids direct repository dependencies.


72-72: Optional dependency is well-introduced
Declaring Optional<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
Passing Optional<TextSubmissionApi> into the constructor ensures compliance with dependency injection best practices.

Also applies to: 92-92


219-220: Graceful handling of missing textSubmissionApi
Throwing ApiNotPresentException prevents 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 ApiNotPresentException when 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:

  1. Uses pattern matching in switch expression
  2. Handles the optional API presence check
  3. 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 ApiNotPresentException when 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.

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

🧹 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:

  1. Make the error message more specific about which functionality is unavailable
  2. 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

📥 Commits

Reviewing files that changed from the base of the PR and between c8c8426 and f8ab027.

📒 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 helios-aet bot temporarily deployed to artemis-test4.artemis.cit.tum.de February 4, 2025 12:39 Inactive
@helios-aet helios-aet bot temporarily deployed to artemis-test6.artemis.cit.tum.de February 5, 2025 13:12 Inactive
coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 8, 2025
# Conflicts:
#	src/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaFeedbackSendingServiceTest.java
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 athena Pull requests that affect the corresponding module atlas Pull requests that affect the corresponding module core Pull requests that affect the corresponding module exam Pull requests that affect the corresponding module exercise Pull requests that affect the corresponding module iris Pull requests that affect the corresponding module plagiarism Pull requests that affect the corresponding module ready for review server Pull requests that update Java code. (Added Automatically!) tests text Pull requests that affect the corresponding module
Projects
Status: Ready For Review
Development

Successfully merging this pull request may close these issues.

1 participant