-
Notifications
You must be signed in to change notification settings - Fork 297
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
Programming exercises
: Export and import build plan from file
#7624
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code lgtm
Can you include the new Aeolus build config as well in case it is used? |
@krusche I tested it and since the whole entry in Somehow the import from file results in a completely empty form for me, can somebody confirm this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks to be pretty straightforward. Only two smaller suggestions and a question about the test.
I assumed ready for review since no longer draft, but PR-label seems to be missing.
src/main/java/de/tum/in/www1/artemis/service/export/ProgrammingExerciseExportService.java
Outdated
Show resolved
Hide resolved
...ava/de/tum/in/www1/artemis/service/programming/ProgrammingExerciseImportFromFileService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/www1/artemis/service/export/ProgrammingExerciseExportService.java
Outdated
Show resolved
Hide resolved
...ava/de/tum/in/www1/artemis/service/programming/ProgrammingExerciseImportFromFileService.java
Outdated
Show resolved
Hide resolved
...java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseTestService.java
Outdated
Show resolved
Hide resolved
This is unrelated to this PR. I tested it with the branch feature/communication/messaging-for-communication-only on ts5 and it was broken as well. The browser console reveals that it happens because options concerning the continuous plagiarism control introduced in PR #7302 are undefined. |
CPC was indeed the issue. Here is a PR with fix: #7640 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me, thanks for fixing this. I'll also test it tomorrow*
@LenaKahle Thanks for testing. |
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
WalkthroughThe changes across the Artemis platform codebase are aimed at addressing the issue of missing build plans in the export of programming exercises. The updates include the addition of a method to query build plans by their associated programming exercise ID, a service method to check for active profiles (GitLab CI or Jenkins), and updates to the export and import services to handle build plans within ZIP file exports and imports. Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... Files ignored due to filter (1)
Files selected for processing (7)
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? TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- src/test/resources/test-data/import-from-file/import-with-build-plan.zip
Files selected for processing (7)
- src/main/java/de/tum/in/www1/artemis/repository/BuildPlanRepository.java (1 hunks)
- src/main/java/de/tum/in/www1/artemis/service/ProfileService.java (1 hunks)
- src/main/java/de/tum/in/www1/artemis/service/export/ProgrammingExerciseExportService.java (7 hunks)
- src/main/java/de/tum/in/www1/artemis/service/programming/ProgrammingExerciseImportFromFileService.java (6 hunks)
- src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseBitbucketBambooIntegrationTest.java (2 hunks)
- src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseGitlabJenkinsIntegrationTest.java (3 hunks)
- src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseTestService.java (12 hunks)
Additional comments: 27
src/main/java/de/tum/in/www1/artemis/service/programming/ProgrammingExerciseImportFromFileService.java (4)
3-3: The import of
BUILD_PLAN_FILE_NAME
is necessary for the new feature to import build plans.19-20: The addition of the Logger is appropriate for logging potential issues during the import process.
51-67: The constructor now includes
profileService
andbuildPlanRepository
as parameters, which are necessary for the new build plan import functionality.106-109: The conditional check for GitLab CI or Jenkins before importing a build plan is logical, as the build plan is not relevant for other CI types like Bamboo.
src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseGitlabJenkinsIntegrationTest.java (4)
231-235: The addition of the test
importExerciseFromFile_buildPlanPresent_buildPlanSet
ensures that the build plan is correctly set during the import process. This is a crucial test for the new functionality introduced in the PR.410-416: The change in the
exportProgrammingExerciseInstructorMaterial
method to callexportProgrammingExerciseInstructorMaterial_shouldReturnFileWithBuildplan
instead of the previous method indicates that the build plan is now included in the export. This aligns with the PR's objectives.433-437: The test
testExportProgrammingExerciseInstructorMaterial_failToCreateTempDir
correctly simulates a failure in creating a temporary directory and checks the system's response to this failure scenario.440-443: The test
testExportProgrammingExerciseInstructorMaterial_embeddedFilesDontExist
is important to ensure that the export process handles the absence of embedded files gracefully.src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseBitbucketBambooIntegrationTest.java (5)
- 349-355: The method
exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan
has been renamed and now includes a boolean parameter. This change is consistent with the PR's objective to handle build plans during export. Ensure that the corresponding method definition is updated to accept the new parameter and that the method's logic correctly processes this parameter.Verification successful
The
ast-grep
command did not return any results for the methodexportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan
with a boolean parameter. This could mean that the method either does not exist or the pattern used in theast-grep
command did not match due to some discrepancy.To ensure we have the correct information, I will attempt a different approach to verify the existence and correct implementation of the method in question.
The search results confirm the existence of the method
exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan
with a boolean parameter in theProgrammingExerciseTestService
class. The method is called with bothtrue
andfalse
as arguments within theProgrammingExerciseBitbucketBambooIntegrationTest
class.Based on this information, the method has been correctly renamed and updated to include the new boolean parameter, which aligns with the PR's objective to handle build plans during the export of programming exercises.
* 438-438: The test method `exportProgrammingExerciseInstructorMaterial` is updated to include an additional boolean parameter. This change seems to be in line with the PR's objective to handle build plans during export. However, it's important to ensure that the method correctly handles the case when the build plan should not be included in the export.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the method definition has been updated and uses the new parameter correctly. ast-grep --lang java --pattern $'public void exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan(boolean $_) { $$$ }'Length of output: 233
Script:
#!/bin/bash # Search for the method 'exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan' in the test class rg "exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan" --type javaLength of output: 816
Verification successful
The output from the
rg
command has provided us with instances of theexportProgrammingExerciseInstructorMaterial
method being called within theProgrammingExerciseBitbucketBambooIntegrationTest.java
file. From the output, we can see that the method is indeed being called with an additional boolean parameter, as indicated by the calls that include five parameters, the last one being a boolean value.Here are the relevant method calls found in the test file:
programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial(HttpStatus.INTERNAL_SERVER_ERROR, true, true, true, false);
programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial(HttpStatus.INTERNAL_SERVER_ERROR, true, false, false, false);
programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial(HttpStatus.INTERNAL_SERVER_ERROR, false, true, true, false);
These calls suggest that the method has been updated to include a fifth boolean parameter, which aligns with the PR's objective to handle build plans during export. The presence of
true
orfalse
as the last argument in these calls indicates that the method can handle both cases: including the build plan in the export or not.Based on this information, the review comment is consistent with the changes in the codebase, and the method appears to correctly handle the case when the build plan should not be included in the export.
* 446-446: The test method `exportProgrammingExerciseInstructorMaterial` is updated to include an additional boolean parameter. This change seems to be in line with the PR's objective to handle build plans during export. However, it's important to ensure that the method correctly handles the case when the build plan should not be included in the export.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the method definition has been updated and handles the case when the build plan should not be included. ast-grep --lang java --pattern $'public void exportProgrammingExerciseInstructorMaterial(HttpStatus $_, boolean $_, boolean $_, boolean $_, boolean $_) { $$$ }'Length of output: 251
Script:
#!/bin/bash # Search for the updated method signature in the specified test file. rg "public void exportProgrammingExerciseInstructorMaterial\(HttpStatus, boolean, boolean, boolean, boolean\)" src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseBitbucketBambooIntegrationTest.javaLength of output: 233
Script:
#!/bin/bash # Search for any method that contains the string 'exportProgrammingExerciseInstructorMaterial' in the specified test file. rg "exportProgrammingExerciseInstructorMaterial" src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseBitbucketBambooIntegrationTest.javaLength of output: 1561
- 453-453: The method
exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan
has been renamed and now includes a boolean parameter. This change is consistent with the PR's objective to handle build plans during export. Ensure that the corresponding method definition is updated to accept the new parameter and that the method's logic correctly processes this parameter.Verification successful
Let's try using
rg
(ripgrep) to search for the methodexportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan
within the test files to confirm its existence and to check if it has been updated to include the new boolean parameter.
The search with
rg
has confirmed the existence of the methodexportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan
with a boolean parameter in theProgrammingExerciseTestService.java
file. This aligns with the PR's objective to handle build plans during export and suggests that the method has been updated accordingly.
- The method definition has been updated to include a boolean parameter.
- The method is located in the
ProgrammingExerciseTestService.java
file.Based on this information, the review comment is consistent with the changes in the codebase.
* 460-460: The test method `exportProgrammingExerciseInstructorMaterial` is updated to include an additional boolean parameter. This change seems to be in line with the PR's objective to handle build plans during export. However, it's important to ensure that the method correctly handles the case when the build plan should not be included in the export.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the method definition has been updated and uses the new parameter correctly. ast-grep --lang java --pattern $'public void exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan(boolean $_) { $$$ }'Length of output: 233
Script:
#!/bin/bash # Search for the method `exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan` in test files rg "void exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan\(boolean"Length of output: 331
Verification successful
Let's use
rg
to search for the method signature within theProgrammingExerciseBitbucketBambooIntegrationTest.java
file to verify the changes.
Let's perform a broader search for the method name without specifying the full signature to confirm its presence and parameters.
The broader search using
rg
has successfully located the methodexportProgrammingExerciseInstructorMaterial
within theProgrammingExerciseBitbucketBambooIntegrationTest.java
file. The output shows various calls to this method with different parameter values, including the one with the additional boolean parameter at the end, which is likely related to the inclusion or exclusion of the build plan in the export.Based on this output, we can confirm that the method has been updated to include an additional boolean parameter, which aligns with the PR's objective to handle build plans during export. The test cases seem to cover scenarios where the build plan is included and excluded, as indicated by the boolean parameter's different values.
Given this information, the review comment is consistent with the changes made in the codebase, and the method appears to handle the case when the build plan should not be included in the export.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the method definition has been updated and handles the case when the build plan should not be included. ast-grep --lang java --pattern $'public void exportProgrammingExerciseInstructorMaterial(HttpStatus $_, boolean $_, boolean $_, boolean $_, boolean $_) { $$$ }'Length of output: 251
Script:
#!/bin/bash # Search for the updated method signature in the ProgrammingExerciseBitbucketBambooIntegrationTest.java file rg "public void exportProgrammingExerciseInstructorMaterial\(HttpStatus, boolean, boolean, boolean, boolean\)" src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseBitbucketBambooIntegrationTest.javaLength of output: 233
Script:
#!/bin/bash # Search for the method name without specifying the full signature to confirm its presence and parameters rg "exportProgrammingExerciseInstructorMaterial" src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseBitbucketBambooIntegrationTest.javaLength of output: 1561
src/main/java/de/tum/in/www1/artemis/service/export/ProgrammingExerciseExportService.java (4)
9-17: The addition of
java.nio.charset.StandardCharsets
and the wildcard imports forjava.util.*
andjavax.xml.xpath.*
are noted. While wildcard imports can sometimes make the code less clear as to which classes are actually used, they are acceptable if a significant number of classes from the package are needed.82-98: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [85-105]
The constructor now includes
BuildPlanRepository
as a parameter, and the field is initialized. This change aligns with the PR's objective to handle the export of build plans. Ensure that theBuildPlanRepository
is being injected correctly elsewhere in the application.
91-91: The addition of the
BUILD_PLAN_FILE_NAME
constant is a good practice for maintainability and avoiding magic strings in the code.138-152: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [131-148]
The logic to export the build plan has been added. It checks if a build plan is present for the given exercise and writes it to a file named
buildPlan.txt
. This is consistent with the PR's objective. However, it's important to ensure that thebuildPlanRepository
is properly handling cases where the build plan might not exist, and that proper error handling is in place for I/O operations.src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseTestService.java (10)
6-6: The import
import static de.tum.in.www1.artemis.service.export.ProgrammingExerciseExportService.*;
is added. Please ensure that the static methods fromProgrammingExerciseExportService
are utilized within the test class.211-212: The field
BuildPlanRepository buildPlanRepository;
is autowired. Please ensure that it is used within the test class for accessing build plan data.526-537: The test method
importFromFile_buildPlanPresent_buildPlanUsed
is added to verify the import functionality with a build plan. Ensure that the logic for importing the build plan and the assertions are correct and that the test ZIP file contains the expected build plan data.1393-1396: The test method
exportInstructorAuxiliaryRepository_shouldReturnFile
is added to verify the export functionality of an instructor's auxiliary repository. Ensure that the logic for exporting the repository and the setup for the test are correct.1409-1412: The test method
exportInstructorAuxiliaryRepository_forbidden
is added to verify that exporting an instructor's auxiliary repository without proper permissions results in aFORBIDDEN
status. Ensure that the logic for expecting forbidden access and the setup for the test are correct.1426-1429: The private method
exportInstructorRepository
is added to facilitate the export of an instructor's repository. Ensure that the logic for exporting the repository and the setup for the test are correct, including the creation of a dummy file in the mock repository.1435-1438: The private method
exportStudentRequestedRepository
is added to facilitate the export of a student-requested repository. Ensure that the logic for exporting the repository, including the optional inclusion of test files, and the setup for the test are correct.1455-1457: The test method
exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan
is added to verify the export functionality without including a build plan. Ensure that the logic for testing the export functionality with the given parameters is correct.1464-1465: The test method
exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithBuildplan
is added to verify the export functionality including a build plan. Ensure that the logic for testing the export functionality with the given parameters is correct.1520-1523: The test method
exportProgrammingExerciseInstructorMaterial_problemStatementNull_success
is added to verify the export functionality when the problem statement is null. Ensure that the logic for exporting the instructor material and the setup for the test are correct.
src/main/java/de/tum/in/www1/artemis/repository/BuildPlanRepository.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- src/test/resources/test-data/import-from-file/import-with-build-plan.zip
Files selected for processing (7)
- src/main/java/de/tum/in/www1/artemis/repository/BuildPlanRepository.java (1 hunks)
- src/main/java/de/tum/in/www1/artemis/service/ProfileService.java (1 hunks)
- src/main/java/de/tum/in/www1/artemis/service/export/ProgrammingExerciseExportService.java (7 hunks)
- src/main/java/de/tum/in/www1/artemis/service/programming/ProgrammingExerciseImportFromFileService.java (6 hunks)
- src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseBitbucketBambooIntegrationTest.java (2 hunks)
- src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseGitlabJenkinsIntegrationTest.java (3 hunks)
- src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseTestService.java (12 hunks)
Additional comments: 29
src/main/java/de/tum/in/www1/artemis/service/ProfileService.java (1)
- 32-34: The addition of
isGitlabCiOrJenkins
method correctly implements the check for active GitLab CI or Jenkins profiles, aligning with the PR objectives.src/main/java/de/tum/in/www1/artemis/repository/BuildPlanRepository.java (1)
- 26-32: The new method
findByProgrammingExercises_Id
correctly uses JPQL to find aBuildPlan
by the ID of aProgrammingExercise
, which is necessary for the export/import functionality.src/main/java/de/tum/in/www1/artemis/service/programming/ProgrammingExerciseImportFromFileService.java (2)
51-67: The updated constructor now correctly takes
ProfileService
andBuildPlanRepository
as additional parameters, which are necessary for the new build plan import functionality.118-135: The new method
importBuildPlanIfExisting
properly handles the conditional import of a build plan based on the existence of the build plan file and the active profiles.src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseGitlabJenkinsIntegrationTest.java (3)
231-235: The new test method
importExerciseFromFile_buildPlanPresent_buildPlanSet
is added to ensure that the build plan is correctly set when importing an exercise from a file. This aligns with the PR objectives to handle the export and import of build plans for programming exercises. Ensure that this test covers all necessary scenarios, including negative cases where the build plan might not be present or incorrectly formatted.413-416: The method
exportProgrammingExerciseInstructorMaterial
has been updated to callexportProgrammingExerciseInstructorMaterial_shouldReturnFileWithBuildplan
instead ofexportProgrammingExerciseInstructorMaterial_shouldReturnFile(true)
. This change seems to reflect the new functionality of including the build plan in the export. Verify that the methodexportProgrammingExerciseInstructorMaterial_shouldReturnFileWithBuildplan
is implemented and that it correctly handles the inclusion of the build plan.Verification successful
The search results show the method
exportProgrammingExerciseInstructorMaterial_shouldReturnFile
is indeed present in theProgrammingExerciseTestService.java
file. The method takes two boolean parameters,saveEmbeddedFiles
andshouldIncludeBuildplan
. The context provided indicates that the method callsexportProgrammingExerciseInstructorMaterial
with the appropriate HTTP status and boolean flags, and it includes assertions to check the existence of the zip file, which suggests that it is testing the export functionality, including the handling of build plans when theshouldIncludeBuildplan
flag istrue
.Based on the information gathered, the method
exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithBuildplan
is implemented and callsexportProgrammingExerciseInstructorMaterial_shouldReturnFile
with parameters that indicate the build plan should be included in the export. The test checks for the existence of the zip file, which is part of verifying the export functionality.Therefore, the review comment has been verified, and the method
exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithBuildplan
is implemented and appears to correctly handle the inclusion of the build plan based on the provided context.* 436-436: The test `testExportProgrammingExerciseInstructorMaterial_failToCreateTempDir` is designed to simulate a failure in creating a temporary directory. This is a good negative test case to ensure that the system can handle unexpected I/O issues gracefully. Confirm that the test properly asserts the expected outcome when such an exception is thrown.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new method is implemented and correctly handles the build plan rg --type java 'exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithBuildplan'Length of output: 531
Script:
#!/bin/bash # Inspect the implementation of the method to verify it handles the build plan inclusion correctly rg --type java --context 10 'void exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithBuildplan\(\)'Length of output: 3354
Script:
#!/bin/bash # Inspect the implementation of the method exportProgrammingExerciseInstructorMaterial_shouldReturnFile to verify the handling of the build plan rg --type java --context 10 'void exportProgrammingExerciseInstructorMaterial_shouldReturnFile\(boolean, boolean\)'Length of output: 117
Script:
#!/bin/bash # Attempting a less specific search pattern to find the implementation of the method rg --type java --context 10 'exportProgrammingExerciseInstructorMaterial_shouldReturnFile'Length of output: 21219
src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseBitbucketBambooIntegrationTest.java (5)
352-352: The method
exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan
has been updated to include a boolean parameter. Ensure that all calls to this method have been updated to pass the correct boolean value based on whether the build plan should be included in the export.438-438: The test case
testExportProgrammingExerciseInstructorMaterial_failToCreateZip
has been updated to include additional boolean parameters. This change seems to be in line with the new method signature. Ensure that the test case is correctly testing the failure scenario when a ZIP file cannot be created.446-446: The test case
testExportProgrammingExerciseInstructorMaterial_failToCreateTempDir
has been updated to include additional boolean parameters. This change seems to be in line with the new method signature. Ensure that the test case is correctly testing the failure scenario when a temporary directory cannot be created.453-453: The test case
testExportProgrammingExerciseInstructorMaterial_embeddedFilesDontExist
has been updated to include a boolean parameter. This change seems to be in line with the new method signature. Ensure that the test case is correctly testing the scenario when embedded files do not exist.460-460: The test case
testExportProgrammingExerciseInstructorMaterial_failToExportRepository
has been updated to include additional boolean parameters. This change seems to be in line with the new method signature. Ensure that the test case is correctly testing the failure scenario when exporting a repository fails.src/main/java/de/tum/in/www1/artemis/service/export/ProgrammingExerciseExportService.java (6)
9-9: The addition of
java.nio.charset.StandardCharsets
import suggests that character encoding is being explicitly specified in file operations, which is a good practice to ensure consistent behavior across different environments.85-85: The addition of
BuildPlanRepository
is consistent with the PR's objective to handle build plan data during the export process.91-91: The introduction of
BUILD_PLAN_FILE_NAME
constant is a good practice for maintainability, as it avoids hard-coded strings and makes the codebase easier to understand and change if needed.105-105: Constructor injection is used for
BuildPlanRepository
, which is in line with best practices for dependency injection.138-152: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [131-151]
The try-catch block in the
exportProgrammingExerciseMaterialWithStudentReposOptional
method is a good practice for error handling. However, it's important to ensure that the catch block is not too generic and that it properly logs the exception for debugging purposes.
- 142-148: The code to export the build plan checks for the presence of a build plan and writes it to a file if it exists. This is in line with the PR's objectives. However, it's important to ensure that the file write operation is secure and does not lead to any potential security issues.
Verification successful
The verification process has provided the following insights:
- The
BUILD_PLAN_FILE_NAME
constant is hardcoded as"buildPlan.txt"
, ensuring that the filename is not influenced by user input and mitigating the risk of file system manipulation.- The
exportDir
is handled in a way that suggests it is a controlled environment, likely not directly influenced by user input, and is checked to be present before use.- The consistent use of
exportDir.orElseThrow()
before resolving paths indicates that the directory is expected to be a valid, existing path, which is a good security practice.Based on the information gathered, the file write operation in the context of exporting the build plan appears to be secure and does not indicate potential security issues.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the file write operation is secure and does not overwrite important files or directories. rg --type java "FileUtils.writeStringToFile" src/main/java/de/tum/in/www1/artemis/service/export/ProgrammingExerciseExportService.javaLength of output: 267
Script:
#!/bin/bash # Check the value of BUILD_PLAN_FILE_NAME constant and how exportDir is set. rg "BUILD_PLAN_FILE_NAME" src/main/java/de/tum/in/www1/artemis/service/export/ProgrammingExerciseExportService.java rg "exportDir" src/main/java/de/tum/in/www1/artemis/service/export/ProgrammingExerciseExportService.javaLength of output: 2326
src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseTestService.java (11)
6-6: The static import
import static de.tum.in.www1.artemis.service.export.ProgrammingExerciseExportService.*;
is added, which suggests that static methods or constants fromProgrammingExerciseExportService
are now being used in this test class.211-212: The
BuildPlanRepository
is added as a new dependency. Ensure that it is used appropriately in the test methods and that the necessary mocks are set up correctly.526-537: The new test method
importFromFile_buildPlanPresent_buildPlanUsed
is added. It seems to test the import functionality with a build plan present. Ensure that the build plan is correctly retrieved and asserted.1393-1396: The new test method
exportInstructorAuxiliaryRepository_shouldReturnFile
is added. It seems to test the export functionality of an instructor's auxiliary repository. Ensure that the auxiliary repository is correctly set up and the export functionality is tested properly.1409-1412: The new test method
exportInstructorAuxiliaryRepository_forbidden
is added. It seems to test the export functionality of an instructor's auxiliary repository with forbidden access. Ensure that the access restrictions are correctly tested.1426-1429: The method
exportInstructorRepository
is modified. It seems to test the export functionality of an instructor's repository. Ensure that the repository type and local repository are correctly set up and the export functionality is tested properly.1455-1456: The new test method
exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan
is added. It seems to test the export functionality of programming exercise instructor material without including the build plan. Ensure that the export functionality is tested properly and the build plan is excluded as expected.1464-1465: The new test method
exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithBuildplan
is added. It seems to test the export functionality of programming exercise instructor material with the build plan included. Ensure that the export functionality is tested properly and the build plan is included as expected.1474-1478: The new test method
exportProgrammingExerciseInstructorMaterial_shouldReturnFile
is added. It seems to test the export functionality of programming exercise instructor material. Ensure that the export functionality is tested properly, including the handling of embedded files and build plans.1520-1523: The new test method
exportProgrammingExerciseInstructorMaterial_problemStatementNull_success
is added. It seems to test the export functionality of programming exercise instructor material when the problem statement is null. Ensure that the export functionality is tested properly and the absence of a problem statement is handled as expected.1583-1584: The method
exportProgrammingExerciseInstructorMaterial
is modified. It seems to test the export functionality of programming exercise instructor material with various conditions. Ensure that the export functionality is tested properly, including the handling of embedded files and build plans.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (3)
- src/main/java/de/tum/in/www1/artemis/service/export/ProgrammingExerciseExportService.java (7 hunks)
- src/main/java/de/tum/in/www1/artemis/service/programming/ProgrammingExerciseImportFromFileService.java (6 hunks)
- src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseTestService.java (12 hunks)
Additional comments: 21
src/main/java/de/tum/in/www1/artemis/service/programming/ProgrammingExerciseImportFromFileService.java (5)
3-7: The addition of imports for
BUILD_PLAN_FILE_NAME
andStandardCharsets
is appropriate for the new functionality being introduced to handle build plan files and character encoding.19-20: The addition of
Logger
andLoggerFactory
imports indicates that logging has been introduced in the service, which is a good practice for error tracking and debugging.29-29: The addition of
BuildPlanRepository
to the imports is necessary for the new functionality to interact with build plans in the database.51-67: The addition of
ProfileService
andBuildPlanRepository
to the class dependencies and their inclusion in the constructor is necessary for the new functionality. It's good to see constructor injection being used, which is a best practice for dependency injection.107-109: The conditional logic to import the build plan only if the profile is GitLab CI or Jenkins is consistent with the PR objectives. However, it's important to ensure that the build plan is compatible with the system where it's being imported.
src/main/java/de/tum/in/www1/artemis/service/export/ProgrammingExerciseExportService.java (5)
9-9: The import of
java.nio.charset.StandardCharsets
is appropriate for specifying character encoding. This is a good practice to ensure consistent behavior across different platforms.85-85: The addition of
BuildPlanRepository
is necessary for the new functionality to export build plans. This change is in line with the PR objectives.91-91: The new constant
BUILD_PLAN_FILE_NAME
is added to define the name of the build plan file when exporting. This is a good practice for maintainability and avoiding hard-coded strings.82-98: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [95-105]
The constructor has been modified to include
BuildPlanRepository
and it is being initialized. This change is necessary for the new functionality to access build plans.
- 142-148: The logic to export the build plan as a text file if it exists is implemented correctly. It checks for the presence of a build plan and writes it to a file named
buildPlan.txt
. This aligns with the PR objectives to include build plans in the export process.src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseTestService.java (11)
211-212: Adding
BuildPlanRepository
as an autowired field is appropriate for accessing build plan data within tests. Ensure that the corresponding bean is properly configured in the test context.526-537: The new test method
importFromFile_buildPlanPresent_buildPlanUsed
is well-named and seems to test the import functionality with a build plan present. Ensure that the test covers all relevant assertions and edge cases for the import process.1393-1393: The method
exportInstructorAuxiliaryRepository_shouldReturnFile
has been modified without showing the actual changes. Ensure that the changes align with the new functionality for exporting build plans and that the test is comprehensive.1409-1409: Similar to the previous comment, ensure that the changes to
exportInstructorAuxiliaryRepository_forbidden
are appropriate and that the test still behaves as expected after the modifications.1426-1426: The method
exportInstructorRepository
has been modified. Ensure that the changes are consistent with the new export functionality and that the test accurately reflects the behavior of exporting instructor repositories.1435-1435: The method
exportStudentRequestedRepository
has been modified. Verify that the changes are in line with the new functionality for exporting repositories with or without tests and that the test is comprehensive.1448-1466: New test methods
exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan
andexportProgrammingExerciseInstructorMaterial_shouldReturnFileWithBuildplan
have been added. Ensure that these tests cover the scenarios of exporting instructor material with and without a build plan comprehensively.1504-1512: The conditional checks within the test method for including or excluding the build plan in the export are appropriate. Ensure that the test data, such as the build plan content "my build plan", matches the expected data in the actual application context.
1569-1589: The method
exportProgrammingExerciseInstructorMaterial
has been modified to include parameters for controlling the inclusion of the build plan. Verify that the changes are consistent with the new functionality and that the test accurately reflects the behavior of exporting instructor material under different conditions.Verification successful
The output from the
rg
command shows the context around theexportProgrammingExerciseInstructorMaterial
method in theProgrammingExerciseTestService.java
file. The method signature has been updated to include a new parametershouldIncludeBuildplan
, which is a boolean indicating whether the build plan should be included in the export or not.The method now has a conditional check for
problemStatementNull
. Iftrue
, it callsgenerateProgrammingExerciseWithProblemStatementNullForExport()
. Otherwise, it callsgenerateProgrammingExerciseForExport(saveEmbeddedFiles, shouldIncludeBuildplan)
, which has been refactored to accept a boolean parameter for including a build plan.This change is consistent with the PR objectives and the AI-generated summary, which mentioned that the PR includes changes to ensure that the build plan is exported as a text file when appropriate, and the
ProgrammingExerciseTestService
has been adjusted to handle the new build plan export and import features.The review comment is verified as accurate based on the provided diff output, which shows the method has been modified to include parameters for controlling the inclusion of the build plan in the export.
* 1624-1627: The method `generateProgrammingExerciseForExport` has been overloaded to accept parameters for saving embedded files and including a build plan. Ensure that the method's behavior is consistent with these new parameters and that the test setup is correct.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the changes made to the method by checking the diff for this specific method. rg --context 5 "File exportProgrammingExerciseInstructorMaterial" src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseTestService.javaLength of output: 1656
- 1643-1645: The addition of a conditional statement to include a build plan in the
generateProgrammingExerciseForExport
method is appropriate. Ensure that the build plan content "my build plan" is consistent with the expected content in the application context.
src/main/java/de/tum/in/www1/artemis/service/export/ProgrammingExerciseExportService.java
Show resolved
Hide resolved
src/main/java/de/tum/in/www1/artemis/service/export/ProgrammingExerciseExportService.java
Show resolved
Hide resolved
src/main/java/de/tum/in/www1/artemis/service/export/ProgrammingExerciseExportService.java
Show resolved
Hide resolved
...ava/de/tum/in/www1/artemis/service/programming/ProgrammingExerciseImportFromFileService.java
Show resolved
Hide resolved
...java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseTestService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested locally, works as expected with GitLab and Atlassian setup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested with newly created exercises on ts2 (gitlab+jenkins) and ts1; all combinations of imports and exports worked as expected
Checklist
General
Server
Changes affecting Programming Exercises
Motivation and Context
The build plan of a programming exercise is at the moment neither imported nor exported.
Closes #7606
Description
Export it now as a txt file and import it if the jenkins or gitlabci profile is active.
Steps for Testing
Prerequisites:
Exam Mode Testing
The logic doesn't differ for exam exercises
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Server
Summary by CodeRabbit
New Features
Enhancements
Tests
Refactor