Adaptive learning: Visualize competencies linked to exercises correctly for exercise lecture units
#9726
Conversation
github-actions
bot
added
tests
server
WalkthroughThe changes introduced in this pull request enhance the functionality of the competency management system within the application. A new method for retrieving competency links by exercise ID is added to the Changes
Suggested labels
Suggested reviewers
Warning There were issues while running the following tools: 🔧 pmdsrc/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyExerciseLinkRepository.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java (1)Pattern 🔇 Additional comments (2)src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java (2)
The CompetencyService integration follows best practices with constructor injection and proper field encapsulation. Also applies to: 95-105
Critical: Fix validation order and improve error handling. The competency service call in Apply this diff to fix the validation order in - competencyService.addCompetencyLinksToExerciseUnits(lecture);
Course course = lecture.getCourse();
if (course == null) {
return ResponseEntity.badRequest().build();
}
User user = userRepository.getUserWithGroupsAndAuthorities();
authCheckService.checkIsAllowedToSeeLectureElseThrow(lecture, user);
+ try {
+ competencyService.addCompetencyLinksToExerciseUnits(lecture);
+ } catch (Exception e) {
+ log.error("Failed to add competency links to lecture {}: {}", lecture.getId(), e.getMessage());
+ }To eliminate code duplication and improve error handling, consider extracting the competency link addition to a private method as suggested in the previous review. Also applies to: 335-337 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: 3
🧹 Outside diff range and nitpick comments (4)
src/main/webapp/app/course/competencies/competencies-popover/competencies-popover.component.ts (1)
Line range hint
31-35: Use single quotes for string literals per style guide.According to the Angular style guide and provided coding guidelines, string literals should use single quotes.
Apply this diff to align with the style guide:
- this.navigationArray = ['/courses', `${this.courseId}`, 'competencies']; + this.navigationArray = ['/courses', '${this.courseId}', 'competencies']; - this.navigationArray = ['/course-management', `${this.courseId}`, 'competency-management']; + this.navigationArray = ['/course-management', '${this.courseId}', 'competency-management'];🧰 Tools
🪛 Biome
[error] 24-24: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
src/main/java/de/tum/cit/aet/artemis/lecture/domain/ExerciseUnit.java (2)
37-42: LGTM: Well-structured field declaration with appropriate annotations.The transient field implementation correctly handles the non-persistent nature of competency links. The initialization and annotations are properly configured.
Consider adding a more detailed Javadoc comment explaining the relationship between exercise units and competency links:
+ /** + * Temporary storage for competency links that are actually persisted in the associated exercise. + * This field is populated at runtime to provide a unified view of competencies in the lecture context. + */ @Transient @JsonSerialize
Line range hint
37-87: Excellent architectural approach for handling transient competency data.The implementation elegantly solves the UI inconsistency by:
- Using a transient field to avoid persistence conflicts
- Maintaining clean separation between exercise and exercise unit responsibilities
- Providing proper JSON serialization for API responses
This approach allows for displaying competencies in the UI while keeping the actual data properly normalized in the database.
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyService.java (1)
142-143: Consider adding parameter validation and optimizing the stream operation.
- Add null check for the lecture parameter
- Use method reference for more concise and readable code
public void addCompetencyLinksToExerciseUnits(Lecture lecture) { + if (lecture == null || lecture.getLectureUnits() == null) { + return; + } - var exerciseUnits = lecture.getLectureUnits().stream().filter(unit -> unit instanceof ExerciseUnit); + var exerciseUnits = lecture.getLectureUnits().stream().filter(ExerciseUnit.class::isInstance);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyExerciseLinkRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/domain/ExerciseUnit.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java(6 hunks)src/main/webapp/app/course/competencies/competencies-popover/competencies-popover.component.ts(1 hunks)src/test/java/de/tum/cit/aet/artemis/lecture/LectureIntegrationTest.java(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyExerciseLinkRepository.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/competency/CompetencyService.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/lecture/domain/ExerciseUnit.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/lecture/web/LectureResource.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/webapp/app/course/competencies/competencies-popover/competencies-popover.component.ts (1)
src/test/java/de/tum/cit/aet/artemis/lecture/LectureIntegrationTest.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/lecture/LectureIntegrationTest.java (2)
Learnt from: julian-christl
PR: ls1intum/Artemis#8052
File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310
Timestamp: 2024-02-23T00:03:06.365Z
Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.
Learnt from: julian-christl
PR: ls1intum/Artemis#8052
File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310
Timestamp: 2024-10-08T15:35:52.595Z
Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.
🔇 Additional comments (13)
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyExerciseLinkRepository.java (1)
5-6: LGTM!
The specific import for java.util.List follows best practices by avoiding wildcard imports.
src/main/java/de/tum/cit/aet/artemis/lecture/domain/ExerciseUnit.java (3)
14-14: LGTM: Import statements are well-organized.
The new imports are specific and necessary for the added functionality.
Also applies to: 19-21
86-87: LGTM: Setter implementation is clean and straightforward.
The method correctly assigns the provided competency links to the transient field.
78-82: LGTM: Getter implementation is consistent with field declaration.
The method correctly returns the transient field and maintains proper JSON serialization configuration.
Let's verify that this getter is used consistently across the codebase:
✅ Verification successful
Let's analyze the search results. The getCompetencyLinks() method is used across the codebase in several contexts:
-
In services and controllers:
- Used in deletion, import, and update operations
- The returned set is properly handled with null checks and defensive programming
- Services create new HashSet instances before modifications
-
In test cases:
- Used in integration tests for various unit types
- Tests handle the returned set appropriately
- No NPE-related failures in tests
-
In domain classes:
- Consistent implementation across different entity types
- Used with proper JSON annotations
- Part of the
LearningObjectinterface contract
Based on this evidence, I can generate the final response:
Verified: The getter implementation is safe and consistently used
The getCompetencyLinks() method is used safely throughout the codebase with proper null handling and defensive programming practices in services. The implementation aligns with both the interface contract and JSON serialization requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of getCompetencyLinks() to ensure they handle potential null values
rg "getCompetencyLinks\(\)" -A 3
Length of output: 17553
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyService.java (4)
10-10: LGTM: Import statements follow best practices.
The new imports are specific and necessary for the added functionality.
Also applies to: 33-34
47-59: LGTM: Constructor injection follows best practices.
The implementation correctly follows Spring's dependency injection pattern with:
- Private final field for the repository
- Constructor injection
- Proper initialization
134-141: LGTM: Well-documented method with clear purpose.
The Javadoc clearly explains the temporary nature of the links and the reasoning behind the implementation.
144-153: Consider handling potential side effects and null safety.
The current implementation has a few considerations:
- It mutates the state of ExerciseUnit objects
- The exercise null check is good, but consider logging when exercise is null for debugging
Let's verify the impact of these mutations:
Consider adding debug logging when exercise is null to help troubleshoot issues:
exerciseUnits.forEach(unit -> {
var exerciseUnit = (ExerciseUnit) unit;
var exercise = exerciseUnit.getExercise();
if (exercise != null) {
var competencyExerciseLinks = competencyExerciseLinkRepository.findByExerciseIdWithCompetency(exercise.getId());
var competencyLectureUnitLinks = competencyExerciseLinks.stream()
.map(link -> new CompetencyLectureUnitLink(link.getCompetency(), exerciseUnit, link.getWeight()))
.collect(Collectors.toSet());
exerciseUnit.setCompetencyLinks(competencyLectureUnitLinks);
+ } else {
+ log.debug("Exercise is null for ExerciseUnit {}", exerciseUnit.getId());
}
});✅ Verification successful
Mutation of ExerciseUnit's competencyLinks is safe and well-handled
The codebase analysis reveals that the mutation of competencyLinks is a standard operation that's:
- Properly encapsulated within domain models (Exercise, LectureUnit, ExerciseUnit)
- Consistently used across services (CompetencyService, LectureUnitService, ExerciseService)
- Well-handled in persistence operations with proper cleanup and re-linking
- Used extensively in tests showing it's a core part of the domain model
The null check for exercise is sufficient as is, since:
- It's a defensive check preventing NPEs
- The operation is non-critical (setting competency links)
- Adding debug logging would add noise for an expected condition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other places where ExerciseUnit's competencyLinks are accessed
rg "setCompetencyLinks|getCompetencyLinks" --type java
Length of output: 13654
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java (3)
30-30: LGTM! Clean dependency addition.
The CompetencyService integration follows best practices with proper encapsulation and constructor injection.
Also applies to: 70-71
95-95: LGTM! Constructor updated correctly.
The CompetencyService parameter is properly added and initialized, maintaining the constructor's consistency.
Also applies to: 105-105
307-307: Verify the error handling patterns in similar endpoints.
Let's verify if other endpoints in the codebase handle similar service calls with proper error handling.
Also applies to: 330-330
src/test/java/de/tum/cit/aet/artemis/lecture/LectureIntegrationTest.java (2)
130-130: LGTM! Test setup enhanced with competency linking.
The addition of competencyUtilService.linkExerciseToCompetency(competency, textExercise) properly sets up the test scenario for verifying competency links in exercise units.
301-302: LGTM! Assertion validates competency links.
The assertion correctly verifies that exercise units within the lecture have the expected competency links. The stream operation and size check ensure that exactly one competency link exists, matching the setup in the test initialization.
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyExerciseLinkRepository.java
Outdated
Show resolved
Hide resolved
src/main/webapp/app/course/competencies/competencies-popover/competencies-popover.component.ts
Outdated
Show resolved
Hide resolved
HawKhiem
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
MaximilianAnzinger
dismissed stale reviews from sachmii and HawKhiem
via
a64b3a0
github-actions
bot
removed
the
client
kevinfischer4
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
flbrgit
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
flbrgit
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
bassner
deleted the
feature/add-transient-exercise-unit-competency-links
branch
Checklist
General
Server
Motivation and Context
When a lecture contains an exercise unit it did not show the competencies linked to the exercise even if they existed. This was due to the decision not to allow linking exercise units to competencies, but only exercises themself. This PR resolves this inconsistency in the user interface.
Resolves #9714
Description
Exercise units now have a transient field that is augmented before returning the lecture unit. Thus, the links will never be persisted in the database, but can be returned to the client for a consistent view.
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
Performance Review
Code Review
Manual Tests
Test Coverage
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation