Programming exercises: Fix an issue with access tokens for team exercises
#9802
Conversation
github-actions
bot
added
tests
server
SimonEntholzer
changed the title
Programming exercise: Fix VCS access tokens for team exercisesProgramming exercises: Fix VCS access tokens for team exercises
WalkthroughThe changes in this pull request involve modifications to several classes to enhance the handling of team exercises and VCS access tokens. The Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
src/test/java/de/tum/cit/aet/artemis/core/authentication/UserAccountLocalVcsIntegrationTest.java (1)
41-46: Consider enhancing test method naming and documentationWhile the test implementation follows the class's consistent pattern, consider these improvements:
- Rename the method to be more specific about what it verifies:
-void getParticipationVcsAccessTokenByUserForTeamExercise() throws Exception { +void shouldGetVcsAccessTokenForTeamExerciseParticipation() throws Exception {
- Add JavaDoc to document the test's purpose and scenario:
/** * Verifies that VCS access tokens can be retrieved for team exercise participations. * This test ensures that students associated with the team are properly loaded * when fetching participation data for team exercises. */src/main/java/de/tum/cit/aet/artemis/programming/service/ParticipationVcsAccessTokenService.java (1)
31-35: LGTM: Constructor properly updatedThe constructor is correctly modified to include the new dependency while maintaining clean code principles.
Consider breaking the long parameter list into multiple lines for better readability:
- public ParticipationVcsAccessTokenService(ParticipationVCSAccessTokenRepository participationVCSAccessTokenRepository, - ProgrammingExerciseStudentParticipationRepository programmingExerciseStudentParticipationRepository, TeamRepository teamRepository) { + public ParticipationVcsAccessTokenService( + ParticipationVCSAccessTokenRepository participationVCSAccessTokenRepository, + ProgrammingExerciseStudentParticipationRepository programmingExerciseStudentParticipationRepository, + TeamRepository teamRepository) {src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseUtilService.java (1)
745-758: Fix JavaDoc parameter description for team parameter.The implementation looks good and follows the established patterns for test utility methods. However, the JavaDoc parameter description for
teamis incorrect as it describes it as "The login of the user" when it's actually a Team object.Apply this diff to fix the JavaDoc:
/** * Adds programming submission to provided programming exercise. The provided login is used to access or create a participation. * * @param exercise The exercise to which the submission should be added. * @param submission The submission which should be added to the programming exercise. - * @param team The login of the user used to access or create an exercise participation. + * @param team The team object used to access or create a team participation. * @return The created programming submission. */src/test/java/de/tum/cit/aet/artemis/core/user/util/UserTestService.java (2)
928-956: Add test annotation and documentation.The test method is missing:
- The
@Testannotation- JavaDoc documentation explaining the test's purpose and steps
Add the following before the method:
+ /** + * Test that verifies the creation and retrieval of VCS access tokens for team exercises: + * 1. Creates a team exercise and adds a user to a team + * 2. Verifies that token is not found initially + * 3. Creates a new token and verifies it can be retrieved + * 4. Cleans up all created entities + */ + @Test public void getAndCreateParticipationVcsAccessTokenForTeamExercise() throws Exception {
928-956: Consider extracting common setup and cleanup logic.The test shares similar setup and cleanup patterns with
getAndCreateParticipationVcsAccessToken. Consider extracting these into helper methods to improve maintainability and reduce code duplication.Consider creating the following helper methods:
private ProgrammingSubmission setupTeamExerciseSubmission(User user) { var course = courseUtilService.addEmptyCourse(); var exercise = programmingExerciseUtilService.addProgrammingExerciseToCourse(course); exercise.setMode(ExerciseMode.TEAM); exerciseTestRepository.save(exercise); courseRepository.save(course); User tutor = userTestRepository.findOneByLogin(TEST_PREFIX + "tutor1").orElseThrow(); Team team = teamUtilService.createTeam(Set.of(user), tutor, exercise, "team1"); var submission = (ProgrammingSubmission) new ProgrammingSubmission() .commitHash("abc") .type(SubmissionType.MANUAL) .submitted(true); return programmingExerciseUtilService.addProgrammingSubmissionToTeamExercise(exercise, submission, team); } private void cleanupSubmission(ProgrammingSubmission submission) { submissionRepository.delete(submission); participationVCSAccessTokenRepository.deleteAll(); participationRepository.deleteById(submission.getParticipation().getId()); teamUtilService.deleteAll(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
src/main/java/de/tum/cit/aet/artemis/programming/service/ParticipationVcsAccessTokenService.java(5 hunks)src/test/java/de/tum/cit/aet/artemis/core/authentication/UserAccountLocalVcsIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/core/user/util/UserTestService.java(4 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/team/TeamUtilService.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseUtilService.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/main/java/de/tum/cit/aet/artemis/programming/service/ParticipationVcsAccessTokenService.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/core/authentication/UserAccountLocalVcsIntegrationTest.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/test/java/de/tum/cit/aet/artemis/core/user/util/UserTestService.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/test/java/de/tum/cit/aet/artemis/exercise/team/TeamUtilService.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/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseUtilService.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
📓 Learnings (1)
src/test/java/de/tum/cit/aet/artemis/core/authentication/UserAccountLocalVcsIntegrationTest.java (1)
Learnt from: SimonEntholzer
PR: ls1intum/Artemis#8929
File: src/test/java/de/tum/in/www1/artemis/authentication/UserLocalVcIntegrationTest.java:29-33
Timestamp: 2024-11-12T12:51:51.200Z
Learning: When addressing missing test coverage, ensure that new tests specifically validate the functionality introduced in the PR, such as token-based authentication.
🔇 Additional comments (10)
src/test/java/de/tum/cit/aet/artemis/core/authentication/UserAccountLocalVcsIntegrationTest.java (2)
44-44: Verify test service implementation follows guidelines
Please ensure that UserTestService.getAndCreateParticipationVcsAccessTokenForTeamExercise() follows these test guidelines:
- Uses AssertJ's
assertThat()for assertions - Tracks database query count for performance monitoring
Let's verify the implementation:
#!/bin/bash
# Description: Check UserTestService implementation
# Check for assertThat usage
rg -A 5 "getAndCreateParticipationVcsAccessTokenForTeamExercise" src/test/java
# Check for query count tracking
rg "TestTransaction|@Sql" src/test/java/de/tum/cit/aet/artemis/core/user/util/UserTestService.java41-46: 🛠️ Refactor suggestion
Consider adding edge case tests for team exercises
Given that this PR fixes issues with VCS token loading for team exercises, consider adding additional test cases to verify edge scenarios:
- Token retrieval when a student is part of multiple teams
- Token retrieval when a team has no students
- Token retrieval after team membership changes
Let's check if these scenarios are covered elsewhere:
src/main/java/de/tum/cit/aet/artemis/programming/service/ParticipationVcsAccessTokenService.java (4)
13-13: LGTM: Clean dependency addition
The TeamRepository dependency is properly added following the constructor injection pattern and immutability principles.
Also applies to: 29-30
Line range hint 1-100: Verify test coverage for team exercise scenarios
The implementation aligns with the PR objective of fixing VCS token loading for team exercises. However, let's verify the test coverage mentioned in the PR description.
#!/bin/bash
# Look for related test files and coverage
rg -l "ParticipationVcsAccessTokenService.*team" --type java --glob "*Test.java"90-99: 🛠️ Refactor suggestion
Improve robustness and performance of team student loading
The current implementation has several potential issues:
- Possible NPE if
findWithStudentsByIdreturns empty Optional - Potential N+1 query problem if called for multiple participations
- Complex nested Optional handling
Consider this safer and more efficient implementation:
private void loadTeamStudentsForTeamExercise(StudentParticipation participation) {
- if (participation.getTeam().isPresent()) {
- participation.getTeam().get().setStudents(teamRepository.findWithStudentsById(participation.getTeam().get().getId()).get().getStudents());
- }
+ participation.getTeam().ifPresent(team ->
+ teamRepository.findWithStudentsById(team.getId())
+ .ifPresent(loadedTeam -> team.setStudents(loadedTeam.getStudents()))
+ );
}Let's check for similar patterns in the codebase that might need similar improvements:
#!/bin/bash
# Look for similar Optional unwrapping patterns that might need improvement
ast-grep --pattern 'if ($x.isPresent()) {
$$$
$x.get()
$$$
}'62-62: Consider transaction boundaries for team loading
While the placement of loadTeamStudentsForTeamExercise calls is correct, loading team data might require explicit transaction handling to prevent LazyInitializationException.
Let's check if there are any transaction annotations in the codebase:
Consider adding @Transactional(readOnly = true) to these methods if not already handled at a higher level.
Also applies to: 81-81
src/test/java/de/tum/cit/aet/artemis/exercise/team/TeamUtilService.java (2)
Line range hint 1-253: Consider enhancing test isolation and data management
While the class provides good utility for test data generation, consider these architectural improvements:
- Implement a TestContext pattern to track and clean up created entities
- Add methods to verify the state after operations
- Consider using builder pattern for more flexible team creation
Example TestContext implementation:
@TestComponent
public class TeamTestContext {
private final Set<Team> createdTeams = new HashSet<>();
public void trackTeam(Team team) {
createdTeams.add(team);
}
@AfterEach
public void cleanup() {
// Clean up only teams created in the current test
createdTeams.forEach(team -> {
team.getStudents().clear();
team.setOwner(null);
team.setExercise(null);
});
teamRepo.deleteAll(createdTeams);
createdTeams.clear();
}
}Let's analyze the current test patterns:
#!/bin/bash
# Description: Analyze test patterns and isolation
# Find test classes using this utility
echo "Test classes using TeamUtilService:"
rg "class.*extends.*Test.*\{" -A 10 -B 2 $(rg -l "TeamUtilService" --type java)
# Check for potential test isolation issues
echo "\nPotential test isolation issues:"
rg "@Test" -A 20 -B 2 $(rg -l "TeamUtilService" --type java) | rg -v "@AfterEach|@After"247-253: 🛠️ Refactor suggestion
Consider revising the database access approach
The deleteAll() method has several concerns that should be addressed:
- Direct database access violates the test guidelines. Consider using a mock repository or moving this logic to a dedicated cleanup service.
- Missing transaction management could lead to inconsistent state.
- Potential test isolation issues if other tests are running concurrently.
Consider this alternative implementation:
- public void deleteAll() {
- teamRepo.deleteAll();
- }
+ @Transactional
+ public void deleteAll() {
+ // First, clean up related entities to prevent foreign key violations
+ var teams = teamRepo.findAll();
+ teams.forEach(team -> {
+ team.getStudents().clear();
+ team.setOwner(null);
+ team.setExercise(null);
+ });
+ teamRepo.saveAll(teams);
+ teamRepo.deleteAll();
+ }Let's verify the usage context and potential impact:
src/test/java/de/tum/cit/aet/artemis/core/user/util/UserTestService.java (2)
49-53: LGTM!
The new imports are properly organized and necessary for implementing team exercise functionality.
98-100: LGTM!
The new autowired fields are properly annotated and follow the class's organization pattern.
Also applies to: 138-140
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
alekspetrov9e
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
flbrgit
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
flbrgit
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
changed the title
Programming exercises: Fix VCS access tokens for team exercisesProgramming exercises: Fix an issue with access tokens for team exercises
Checklist
General
Server
Changes affecting Programming Exercises
Motivation and Context
For programming team exercises, the vcs access token did not load, as the team's students were not loaded when fetching the participation.
Description
Fix the issue by loading the team's students, when the exercise is a team exercise.
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Server
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests