Lectures: Fix an issue when saving lecture units in guided mode without linked competency
#9758
Conversation
github-actions
bot
added
tests
server
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
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 (3)
src/test/java/de/tum/cit/aet/artemis/lecture/LectureUnitServiceTest.java (1)
92-104: Test looks good with some suggestions for improvement.The test effectively verifies the null handling of competency links. Consider these improvements:
- Extract the magic number
1Lto a constant for better maintainability- Make the saving lambda more descriptive of what it's mocking
- Consider adding more edge cases (e.g., empty list vs null)
Here's a suggested improvement:
class LectureUnitServiceTest extends AbstractSpringIntegrationIndependentTest { private static final String TEST_PREFIX = "lecuniservtst"; + private static final Long TEST_UNIT_ID = 1L; @Test void testSaveWithCompetencyLinksWithNullLinks() { AttachmentUnit lectureUnit = new AttachmentUnit(); lectureUnit.setCompetencyLinks(null); - LectureUnit savedLectureUnit = lectureUnitService.saveWithCompetencyLinks(lectureUnit, unit -> { - unit.setId(1L); - return unit; - }); + LectureUnit savedLectureUnit = lectureUnitService.saveWithCompetencyLinks(lectureUnit, + // Mock persistence by setting ID + unit -> { + unit.setId(TEST_UNIT_ID); + return unit; + }); assertThat(savedLectureUnit).isNotNull(); assertThat(savedLectureUnit.getCompetencyLinks()).isEmpty(); }src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.java (2)
256-256: Reorder null check for better efficiencyWhile adding the null check is correct, it should be placed before
Hibernate.isInitialized(links)for better efficiency, as there's no need to check initialization status of a null object.- if (Hibernate.isInitialized(links) && links != null && !links.isEmpty()) { + if (links != null && Hibernate.isInitialized(links) && !links.isEmpty()) {
Line range hint
251-262: Add JavaDoc for method parametersThe method lacks documentation for its parameters. Consider adding parameter descriptions to improve code maintainability.
/** * Saves the exercise and links it to the competencies. * * @param lectureUnit the lecture unit to save - * @param saveFunction function to save the exercise + * @param saveFunction function to save the lecture unit, typically a repository save operation * @param <T> type of the lecture unit - * @return saved exercise + * @return saved lecture unit with updated competency links */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/lecture/LectureUnitServiceTest.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.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/lecture/LectureUnitServiceTest.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
🔇 Additional comments (2)
src/test/java/de/tum/cit/aet/artemis/lecture/LectureUnitServiceTest.java (1)
14-14: LGTM!
The import statement is correctly placed and properly used in the new test method.
src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.java (1)
Line range hint 251-262: Verify test coverage and usage patterns
Let's ensure this change is properly tested and identify all callers of this method.
✅ Verification successful
Test coverage and usage patterns are adequate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage and usage patterns for saveWithCompetencyLinks
# Find test files that might contain tests for this method
echo "Looking for relevant test files..."
fd -e java -e kt --full-path "test.*LectureUnit.*Test"
# Find all usages of saveWithCompetencyLinks
echo -e "\nChecking usage patterns..."
rg "saveWithCompetencyLinks" --type java
Length of output: 5443
az108
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
Feras797
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
florian-glombik
changed the title
Lectures: Fix saving lecture unitsLectures: Fix saving lecture units without linked competency
krusche
changed the title
Lectures: Fix saving lecture units without linked competencyLectures: Fix an issue when saving lecture units in guided mode without linked competency
Checklist
General
Server
Motivation and Context
Found the bug when working on Lectures: Add status bar to lecture creation and edit mode #9655.
If Lecture Units are created within the guided mode and no competency is linked, the server throws an internal server error.
Description
This PR allows the creation of a lecture unit without linking it to a competency.
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
Screenshots
Bug
With the Bugfix