Development: Introduce lti module API
#10295
Conversation
github-actions
bot
added
tests
server
WalkthroughThis pull request replaces several LTI-related dependencies across the application with a new API, Changes
Sequence Diagram(s)Manual Assessment Submission FlowsequenceDiagram
participant Client
participant AssessmentService
participant LtiApi
participant "LTI Handler" as UnderlyingService
Client->>AssessmentService: submitManualAssessment(participation)
AssessmentService->>LtiApi: ifPresent(onNewResult(participation))
LtiApi->>UnderlyingService: onNewResult(participation)
User Initialization FlowsequenceDiagram
participant Client
participant UserResource
participant LtiApi
participant "LTI Check Service" as LtiCheckService
Client->>UserResource: initializeUser(user)
UserResource->>LtiApi: isLtiCreatedUser(user)
LtiApi->>LtiCheckService: isLtiCreatedUser(user)
Online Course Configuration FlowsequenceDiagram
participant Client
participant CourseResource
participant LtiApi
participant "Online Course Config Service" as OnlineCourseService
Client->>CourseResource: updateCourse(courseUpdate)
CourseResource->>LtiApi: createOnlineCourseConfiguration(courseUpdate)
LtiApi->>OnlineCourseService: createOnlineCourseConfiguration(courseUpdate)
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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 comments (1)
src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (1)
309-316: Consider adding explicit error handling for missing LTI API.When a course is marked as online but the LTI API is not available, the code silently continues without creating the configuration. Consider throwing an appropriate exception to provide better feedback.
Apply this diff to improve error handling:
if (courseUpdate.isOnlineCourse() != existingCourse.isOnlineCourse()) { if (courseUpdate.isOnlineCourse() && ltiApi.isPresent()) { ltiApi.get().createOnlineCourseConfiguration(courseUpdate); } - else { + else if (courseUpdate.isOnlineCourse()) { + throw new BadRequestAlertException("Cannot create online course: LTI module is not enabled", Course.ENTITY_NAME, "ltiNotEnabled"); + } + else { courseUpdate.setOnlineCourseConfiguration(null); } }
🧹 Nitpick comments (5)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java (2)
28-29: Well-structured architectural test modifications.The test modifications effectively incorporate ignored class filtering while maintaining the original architectural constraints. The implementation follows ArchUnit best practices and maintains good readability.
Consider adding JavaDoc to document the purpose of ignored classes and their impact on the architectural rules.
public abstract class AbstractModuleAccessArchitectureTest extends AbstractArchitectureTest implements ModuleArchitectureTest { + /** + * Tests that classes outside the module can only access API, domain, or DTO classes, + * except for specifically ignored classes that may have different architectural requirements. + */ @Test void shouldOnlyAccessApiDomainDto() {Also applies to: 36-37, 42-43
58-60: Add JavaDoc to document the purpose and usage of getIgnoredClasses.The method is well-implemented but lacks documentation. Add JavaDoc to explain:
- The purpose of ignored classes in architectural tests
- When and how subclasses should override this method
- The significance of the default empty set
+ /** + * Returns a set of classes that should be excluded from architectural rule checks. + * This method can be overridden by subclasses to specify classes that have special + * architectural requirements or exceptions to the standard module rules. + * + * @return A Set of Class<?> objects to be ignored in architectural tests + */ protected Set<Class<?>> getIgnoredClasses() { return Set.of(); }src/main/java/de/tum/cit/aet/artemis/core/exception/ApiNotPresentException.java (1)
16-18: Consider enhancing the error message format.The error message is informative but could be more user-friendly.
- super(String.format("Api %s is not enabled, because Spring profile %s is not enabled. Did you enable it?", api.getName(), profile)); + super(String.format("API '%s' is not available because Spring profile '%s' is not enabled. Please enable the required profile.", api.getSimpleName(), profile));src/main/java/de/tum/cit/aet/artemis/lti/api/LtiApi.java (1)
40-54: Add JavaDoc documentation for public methods.The public methods lack documentation explaining their purpose and parameters.
+ /** + * Handles new result submission for LTI. + * @param participation The student participation containing the result + */ public void onNewResult(StudentParticipation participation) { ltiNewResultService.onNewResult(participation); } + /** + * Checks if a user was created through LTI. + * @param user The user to check + * @return true if the user was created through LTI, false otherwise + */ public boolean isLtiCreatedUser(User user) { return ltiService.isLtiCreatedUser(user); } + /** + * Creates online course configuration for LTI. + * @param course The course to configure + */ public void createOnlineCourseConfiguration(Course course) { onlineCourseConfigurationService.createOnlineCourseConfiguration(course); } + /** + * Finds LTI resource launches for a specific user and exercise. + * @param user The user to find launches for + * @param exercise The exercise to find launches for + * @return Collection of LTI resource launches + */ public Collection<LtiResourceLaunch> findByUserAndExercise(User user, Exercise exercise) { return lti13ResourceLaunchRepository.findByUserAndExercise(user, exercise); }src/main/java/de/tum/cit/aet/artemis/text/service/TextAssessmentService.java (1)
60-93: Consider breaking down the large method into smaller, focused methods.The
prepareSubmissionForAssessmentmethod handles multiple responsibilities and could be split into smaller methods for better maintainability.Consider extracting the following methods:
+ private Result createOrLoadResult(TextSubmission textSubmission, @Nullable Result result) { + if (result != null) { + final List<Feedback> assessments = feedbackRepository.findByResult(result); + result.setFeedbacks(assessments); + result.setSubmission(textSubmission); + return result; + } + + result = new Result(); + result.setParticipation(textSubmission.getParticipation()); + resultService.createNewRatedManualResult(result); + result.setCompletionDate(null); + result = resultRepository.save(result); + result.setSubmission(textSubmission); + textSubmission.addResult(result); + submissionRepository.save(textSubmission); + return result; + } + private void prepareTextBlocks(TextSubmission textSubmission) { + final var textBlocks = textBlockService.findAllBySubmissionId(textSubmission.getId()); + textSubmission.setBlocks(textBlocks); + + if (textSubmission.getBlocks() == null || !isInitialized(textSubmission.getBlocks()) || textSubmission.getBlocks().isEmpty()) { + textBlockService.computeTextBlocksForSubmissionBasedOnSyntax(textSubmission); + } + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/main/java/de/tum/cit/aet/artemis/assessment/service/AssessmentService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/core/exception/ApiNotPresentException.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/UserResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminCourseResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/lti/api/AbstractLtiApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lti/api/LtiApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingAssessmentService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/quiz/service/QuizStatisticService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/text/service/TextAssessmentService.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/lti/architecture/LtiApiArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/de/tum/cit/aet/artemis/lti/api/AbstractLtiApi.java
🧰 Additional context used
📓 Path-based instructions (2)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
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/exception/ApiNotPresentException.javasrc/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.javasrc/main/java/de/tum/cit/aet/artemis/assessment/service/AssessmentService.javasrc/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminCourseResource.javasrc/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingAssessmentService.javasrc/main/java/de/tum/cit/aet/artemis/lti/api/LtiApi.javasrc/main/java/de/tum/cit/aet/artemis/quiz/service/QuizStatisticService.javasrc/main/java/de/tum/cit/aet/artemis/text/service/TextAssessmentService.javasrc/main/java/de/tum/cit/aet/artemis/core/web/UserResource.javasrc/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
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/lti/architecture/LtiApiArchitectureTest.javasrc/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java
📓 Learnings (1)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (2)
Learnt from: julian-christl
PR: ls1intum/Artemis#9322
File: src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java:0-0
Timestamp: 2024-11-12T12:51:51.201Z
Learning: In Artemis, an `ExerciseGroup` always has an associated `Exam`, so `exerciseGroup.exam` is never null.
Learnt from: julian-christl
PR: ls1intum/Artemis#9322
File: src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java:170-172
Timestamp: 2024-11-12T12:51:46.554Z
Learning: In Artemis, `exercise.exerciseGroup` may be null, as not all exercises belong to an `ExerciseGroup`.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (26)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java (1)
3-4: LGTM: Import statements are well-organized.The new imports are appropriately placed and follow Java conventions. The static imports enhance code readability for the predicate usage.
Also applies to: 10-10
src/test/java/de/tum/cit/aet/artemis/lti/architecture/LtiApiArchitectureTest.java (1)
9-20: LGTM! The architecture test class is well-structured.The implementation correctly extends
AbstractModuleAccessArchitectureTestand provides appropriate overrides for module package definition and ignored classes.src/main/java/de/tum/cit/aet/artemis/core/exception/ApiNotPresentException.java (1)
6-9: LGTM! Documentation is clear and descriptive.The JavaDoc clearly explains the purpose of the exception.
src/main/java/de/tum/cit/aet/artemis/lti/api/LtiApi.java (2)
20-22: LGTM! Proper Spring configuration.The class is correctly configured with appropriate Spring annotations and profile.
32-38: LGTM! Constructor follows dependency injection best practices.All dependencies are injected via constructor, following the guidelines.
src/main/java/de/tum/cit/aet/artemis/text/service/TextAssessmentService.java (1)
39-45: LGTM! Constructor properly updated for LTI API integration.The constructor correctly integrates the Optional and follows dependency injection guidelines.
src/main/java/de/tum/cit/aet/artemis/core/web/UserResource.java (3)
34-34: LGTM!Import statement for
LtiApiis correctly added.
68-68: LGTM!The field declaration and constructor parameter have been correctly updated to use
Optional<LtiApi>. The constructor initialization follows best practices by using constructor injection.Also applies to: 72-77
147-147: LGTM!The LTI user check has been correctly updated to use the new
LtiApi. The logic maintains the same behavior while using the new API.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingAssessmentService.java (3)
33-33: LGTM!Import statement for
LtiApiis correctly added.
48-52: LGTM!The constructor parameters and super constructor call have been correctly updated to use
Optional<LtiApi>. The changes maintain proper dependency injection.
127-127: LGTM!The LTI result submission has been correctly updated to use the new
LtiApi. The lambda expression maintains readability while using the new API.src/main/java/de/tum/cit/aet/artemis/quiz/service/QuizStatisticService.java (3)
20-20: LGTM!Import statement for
LtiApiis correctly added.
47-47: LGTM!The field declaration and constructor parameter have been correctly updated to use
Optional<LtiApi>. The constructor follows best practices by using constructor injection.Also applies to: 51-58
111-111: LGTM!The LTI result submission has been correctly updated to use the new
LtiApi. The lambda expression maintains readability while using the new API.src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminCourseResource.java (3)
47-47: LGTM!Import statement for
LtiApiis correctly added.
77-77: LGTM!The field declaration and constructor parameter have been correctly updated to use
Optional<LtiApi>. The constructor follows best practices by using constructor injection.Also applies to: 80-87
143-145: LGTM!The online course configuration has been correctly updated to use the new
LtiApi. The logic maintains the same behavior while using the new API.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java (1)
34-34: LGTM! The LTI service migration is consistent.The changes correctly replace
LtiNewResultServicewithLtiApiacross imports, field declarations, constructor parameters, and method calls. The migration maintains the same functionality while aligning with the server modularization efforts.Also applies to: 55-55, 66-66, 71-71, 194-194
src/main/java/de/tum/cit/aet/artemis/assessment/service/AssessmentService.java (1)
36-36: LGTM! The LTI service migration is consistent.The changes correctly replace
LtiNewResultServicewithLtiApiacross imports, field declarations, constructor parameters, and method calls. The migration maintains the same functionality while aligning with the server modularization efforts.Also applies to: 64-64, 72-72, 84-84, 246-250
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (1)
65-65: LGTM! The LTI service migration is consistent.The changes correctly replace
LtiNewResultServicewithLtiApiacross imports, field declarations, constructor parameters, and method calls. The migration maintains the same functionality while aligning with the server modularization efforts.Also applies to: 91-91, 133-136, 144-144, 192-194
src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseService.java (1)
76-76: LGTM! The LTI service migration is consistent.The changes correctly replace
Lti13ResourceLaunchRepositorywithLtiApiacross imports, field declarations, constructor parameters, and method calls. The migration maintains the same functionality while aligning with the server modularization efforts.Also applies to: 108-108, 140-140, 152-152, 198-201
src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (4)
122-122: LGTM!The import for
LtiApiis correctly placed and aligns with the PR's objective of introducing the LTI module API.
150-150: LGTM!The field declaration is well-designed:
- Uses
Optionalto handle cases where LTI is not enabled- Follows immutability best practices with
final- Maintains proper encapsulation with
private
193-199: LGTM!The constructor changes follow best practices:
- Uses constructor injection as per coding guidelines
- Maintains alphabetical parameter ordering
- Properly handles optional LTI dependency
203-203: LGTM!The field assignment is correctly implemented and follows the class's initialization pattern.
ole-ve
dismissed stale reviews from HanyangXu0508, MaximilianAnzinger, and coderabbitai[bot]
via
e9c320d
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseService.java (1)
88-88: Consider splitting this service into smaller, more focused services.The
ExerciseServiceclass has grown quite large and handles multiple responsibilities (exercises, teams, statistics, competencies, etc.). Consider breaking it down into smaller, more focused services to improve maintainability and adhere to the Single Responsibility Principle.Potential split:
ExerciseManagementService: Core exercise CRUD operationsExerciseTeamService: Team-related operationsExerciseStatisticsService: Statistics calculationsExerciseCompetencyService: Competency-related operationssrc/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (2)
193-199: Consider reducing the constructor’s parameter list.
This constructor has many parameters, which can hamper readability and maintainability. You might consider refactoring to a builder/factory pattern or applying the single-responsibility principle to reduce complexity.
310-312: Use the functional style for Optionals.
Rather than callingisPresent()and thenget(), consider usingltiApi.ifPresent(api -> api.createOnlineCourseConfiguration(courseUpdate));for cleaner, more idiomatic code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseService.java(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
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/web/CourseResource.javasrc/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseService.java
🧠 Learnings (1)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (2)
Learnt from: julian-christl
PR: ls1intum/Artemis#9322
File: src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java:0-0
Timestamp: 2024-11-12T12:51:51.201Z
Learning: In Artemis, an `ExerciseGroup` always has an associated `Exam`, so `exerciseGroup.exam` is never null.
Learnt from: julian-christl
PR: ls1intum/Artemis#9322
File: src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java:170-172
Timestamp: 2024-11-12T12:51:46.554Z
Learning: In Artemis, `exercise.exerciseGroup` may be null, as not all exercises belong to an `ExerciseGroup`.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
- GitHub Check: Mend Security Check
🔇 Additional comments (13)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (5)
65-65: LGTM!The import statement follows the coding guidelines by using a specific import rather than a star import.
91-91: LGTM!The field declaration follows best practices:
- Uses
finalmodifier for immutability- Has private access following the principle of least privilege
- Correctly uses
Optionalto handle potential absence of LTI functionality
133-136: LGTM!The constructor changes follow the coding guidelines:
- Uses constructor injection for dependency management
- Maintains consistent parameter ordering and formatting
144-144: LGTM!The field initialization is correctly done in the constructor and is consistent with the parameter changes.
192-193: LGTM!The LTI service usage is correctly updated to use the new API while maintaining the same functionality:
- Proper type checking for
ProgrammingExerciseStudentParticipation- Correct null-safety check using
Optional.isPresent()- Appropriate method call through the new API facade
src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseService.java (5)
76-76: LGTM!The import is correctly placed and follows Java best practices by using a specific import rather than a wildcard.
108-108: LGTM!The field declaration follows best practices:
- Appropriate access modifier (private)
- Immutability (final)
- Proper use of Optional for dependency injection
138-144: LGTM!The constructor changes follow the coding guidelines:
- Uses constructor injection
- Maintains parameter ordering
- Consistent with field declaration
152-152: LGTM!The field initialization is consistent with the constructor parameter.
198-201: LGTM!The changes successfully encapsulate the LTI resource check behind the new API while maintaining functional equivalence. This aligns well with the modularization objectives.
src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (3)
122-122: Explicit import usage is consistent.
No issues identified. The explicit import ofLtiApialigns well with the guideline to avoid wildcard imports.
150-150: Optional dependency field injection looks fine.
Declaring this dependency asprivate final Optional<LtiApi>respects the principle of injecting dependencies via the constructor.
203-203: Straightforward assignment.
No issues identified regardingthis.ltiApi = ltiApi;.
Checklist
General
Server
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 lti.
Steps for Testing
Can not be tested on TS0 (lti is disabled)
Basically try to test all the functionality of lti.
Exam Mode Testing
not relevant - lti is not used for exam-related functionality
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Summary by CodeRabbit