Modeling exercises: Inline AI feedback view
#9799
Conversation
github-actions
bot
added
server
LeonWehrhahn
changed the title
Modeling exercises: Inline AI Feedback View
WalkthroughThe changes in this pull request involve multiple updates across various files, primarily focusing on enhancing feedback handling and submission management within the modeling exercise context. Key modifications include the addition of new methods for retrieving results based on participation ID and assessment type, updates to data transfer objects to streamline feedback representation, and improvements to component logic for managing submissions and feedback views. The overall structure of the application remains intact, with new functionalities introduced to improve user interaction and data processing. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmd (7.7.0)src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingExerciseFeedbackService.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. 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: 6
🧹 Outside diff range and nitpick comments (21)
src/main/webapp/app/exercises/modeling/participate/modeling-participation.route.ts (1)
19-28: Consider adding a route resolver for better UX.To prevent showing an empty/loading state while fetching submission data, consider implementing a resolver to pre-fetch the submission data before activating the route.
Example implementation:
@Injectable({ providedIn: 'root' }) export class ModelingSubmissionResolver implements Resolve<ModelingSubmission> { constructor(private submissionService: ModelingSubmissionService) {} resolve(route: ActivatedRouteSnapshot): Observable<ModelingSubmission> { const participationId = route.paramMap.get('participationId'); const submissionId = route.paramMap.get('submissionId'); return this.submissionService.getSubmission(participationId, submissionId); } }Then update the route configuration:
{ path: 'participate/:participationId/submission/:submissionId', component: ModelingSubmissionComponent, + resolve: { + submission: ModelingSubmissionResolver + }, data: { authorities: [Authority.USER], pageTitle: 'artemisApp.modelingExercise.home.title', }, canActivate: [UserRouteAccessService], canDeactivate: [PendingChangesGuard], },src/main/java/de/tum/cit/aet/artemis/athena/dto/ModelingFeedbackDTO.java (1)
31-31: Consider adding parameter validation.While the
@NotNullannotation on thefeedbackparameter is good, consider adding validation for:
exerciseIdandsubmissionIdto ensure they are positivefeedback.getReference()to ensure it's not null or empty when requiredExample validation:
public static ModelingFeedbackDTO of( @Positive long exerciseId, @Positive long submissionId, @NotNull Feedback feedback) { Objects.requireNonNull(feedback.getReference(), "Reference must not be null"); // ... rest of the method }src/main/webapp/app/exercises/modeling/participate/modeling-participation.module.ts (1)
16-16: Consider moving RequestFeedbackButtonComponent to shared moduleThe RequestFeedbackButtonComponent is currently located in 'app/overview/exercise-details' but is being used in the modeling module. Since it's a reusable component for requesting feedback, consider:
- Moving it to the shared components directory (e.g., 'app/shared/components/feedback')
- Declaring it in ArtemisSharedComponentModule
- Importing ArtemisSharedComponentModule where needed
This would improve modularity and make the component's purpose and reusability more apparent.
src/main/webapp/app/exercises/modeling/assess/modeling-assessment.util.ts (2)
13-13: Address the TODO comment about complex type mapping.The TODO comment indicates a known issue with complexity. Given that this PR enhances the feedback mechanism, this would be a good time to address this technical debt.
Consider these improvements:
- Create a dedicated type mapping configuration file
- Use a decorator pattern to handle different diagram types
- Implement a strategy pattern for type resolution
Would you like me to provide a detailed implementation proposal for any of these approaches?
Line range hint
18-95: Refactor the nested switch statements for better maintainability.The current implementation uses nested if-else and switch statements, making it hard to maintain and extend for new diagram types. This is particularly relevant given the PR's focus on enhancing feedback visualization.
Consider this more maintainable approach:
interface ElementTypeMapping { [key: string]: { type: string; nameFormatter?: (element: any) => string }; } const UML_ELEMENT_MAPPINGS: ElementTypeMapping = { [UMLElementType.Class]: { type: 'class' }, [UMLElementType.Package]: { type: 'package' }, // ... other mappings }; const UML_RELATIONSHIP_MAPPINGS: ElementTypeMapping = { [UMLRelationshipType.ClassBidirectional]: { type: 'association', nameFormatter: (rel) => `${rel.source} <-> ${rel.target}` }, // ... other mappings }; function getElementAssessmentName(element: any, mapping: ElementTypeMapping): { type: string; name: string } { const config = mapping[element.type] || { type: '', nameFormatter: (e) => e.name || '' }; return { type: config.type, name: config.nameFormatter ? config.nameFormatter(element) : element.name }; }src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingExerciseFeedbackService.java (2)
Line range hint
171-178: Consider adding input validation and documentation.While the conversion logic is correct, consider these improvements:
- Add input validation for critical fields
- Document the purpose of the reference field
- Use @nonnull annotations where appropriate
Here's a suggested improvement:
/** * Converts an Athena feedback suggestion to a Feedback object. * * @param feedbackItem the Athena feedback suggestion + * @throws IllegalArgumentException if required fields are null * @return the Feedback object */ - private Feedback convertToFeedback(ModelingFeedbackDTO feedbackItem) { + private Feedback convertToFeedback(@NonNull ModelingFeedbackDTO feedbackItem) { + if (feedbackItem.title() == null || feedbackItem.description() == null) { + throw new IllegalArgumentException("Title and description are required"); + } Feedback feedback = new Feedback(); feedback.setText(feedbackItem.title()); feedback.setDetailText(feedbackItem.description()); feedback.setHasLongFeedbackText(false); feedback.setType(FeedbackType.AUTOMATIC); feedback.setCredits(feedbackItem.credits()); feedback.setReference(feedbackItem.reference()); return feedback; }
Line range hint
42-47: Consider operational improvements for feedback service.The service handles core functionality well, but consider these operational enhancements:
- Make the rate limit (10) configurable via properties
- Add structured logging for better monitoring of feedback generation
Example configuration property:
artemis.modeling.feedback.max-requests=10Example logging enhancement:
log.info("Generating AI feedback for exercise={} participation={} submission={}", modelingExercise.getId(), participation.getId(), submission.getId());src/main/webapp/app/exercises/shared/result/result.component.ts (4)
273-274: Consider using constants for exercise type paths.The hardcoded paths could be moved to constants to improve maintainability and reduce the risk of typos.
+ private readonly EXERCISE_TYPE_PATHS = { + [ExerciseType.TEXT]: 'text-exercises', + [ExerciseType.MODELING]: 'modeling-exercises', + } as const; + - const exerciseTypePath = this.exercise?.type === ExerciseType.TEXT ? 'text-exercises' : 'modeling-exercises'; + const exerciseTypePath = this.EXERCISE_TYPE_PATHS[this.exercise?.type!];
266-272: Enhance null safety for submission ID retrieval.The current implementation could be more explicit about handling undefined cases.
- let submissionId = result.submission?.id; - // In case of undefined result submission try the latest submission as this can happen before reloading the component - if (!submissionId) { - submissionId = result.participation?.submissions?.last()?.id; - } + const submissionId = result.submission?.id ?? result.participation?.submissions?.last()?.id; + if (!submissionId) { + console.warn('No submission ID found for result:', result); + }
275-277: Remove redundant return statement after navigation.The
return undefinedafterrouter.navigateis unnecessary as the navigation will end the method execution.this.router.navigate(['/courses', courseId, 'exercises', exerciseTypePath, this.exercise?.id, 'participate', result.participation?.id, 'submission', submissionId]); -return undefined;
265-275: Add error handling for invalid course ID.Consider adding validation for the courseId to provide better error feedback.
if (this.exercise?.type === ExerciseType.TEXT || this.exercise?.type === ExerciseType.MODELING) { const courseId = getCourseFromExercise(this.exercise)?.id; + if (!courseId) { + console.error('Unable to navigate: Invalid course ID'); + return; + }src/main/webapp/app/exercises/shared/result/result.utils.ts (2)
119-123: Add JSDoc documentation for better maintainability.The function implementation looks good and correctly handles both manual unreferenced and automatic feedback. Consider adding JSDoc documentation to explain the purpose and behavior.
+/** + * Filters and returns unreferenced feedback from an array of feedbacks. + * Unreferenced feedback includes both manual unreferenced feedback and automatic feedback without references. + * + * @param feedbacks - Array of feedback items to filter + * @returns Array of unreferenced feedback items or undefined if input is undefined + */ export const getUnreferencedFeedback = (feedbacks: Feedback[] | undefined): Feedback[] | undefined => {
119-123: Improve type safety with explicit return types.Consider using a more explicit return type to improve type safety and make the function's behavior more predictable.
-export const getUnreferencedFeedback = (feedbacks: Feedback[] | undefined): Feedback[] | undefined => { +export const getUnreferencedFeedback = (feedbacks: Feedback[] | undefined): readonly Feedback[] => { return feedbacks ? feedbacks.filter( (feedbackElement) => !feedbackElement.reference && (feedbackElement.type === FeedbackType.MANUAL_UNREFERENCED || feedbackElement.type === FeedbackType.AUTOMATIC), ) - : undefined; + : [];This change:
- Returns an empty array instead of undefined for consistency
- Makes the return value readonly to prevent accidental modifications
- Simplifies error handling for consumers as they don't need to check for undefined
src/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingSubmissionResource.java (4)
96-96: Consider breaking down the class to reduce constructor parameters.While the constructor properly initializes all dependencies, it's getting quite large with many parameters. This might indicate that the class has too many responsibilities.
Consider:
- Breaking down the class into smaller, more focused components
- Using a builder pattern or configuration object
- Creating specialized service classes for different aspects of submission handling
Also applies to: 104-105
424-445: Consider simplifying the nested stream operations.The current implementation of filtering submissions and results uses nested streams with complex conditions.
Consider extracting the filtering logic into separate methods for better readability:
- List<Submission> submissionsWithResults = submissions.stream().filter(submission -> { - submission.setParticipation(participation); - List<Result> filteredResults = submission.getResults().stream().filter(result -> { - if (ExerciseDateService.isAfterAssessmentDueDate(modelingExercise)) { - return true; - } - else { - return result.getAssessmentType() == AssessmentType.AUTOMATIC_ATHENA; - } - }).peek(Result::filterSensitiveInformation) - .sorted(Comparator.comparing(Result::getCompletionDate).reversed()) - .collect(Collectors.toList()); - if (!filteredResults.isEmpty()) { - submission.setResults(filteredResults); - return true; - } - return false; - }).collect(Collectors.toList()); + List<Submission> submissionsWithResults = submissions.stream() + .map(submission -> enrichSubmissionWithFilteredResults(submission, participation, modelingExercise)) + .filter(submission -> !submission.getResults().isEmpty()) + .collect(Collectors.toList());Add this private method:
private Submission enrichSubmissionWithFilteredResults(Submission submission, StudentParticipation participation, ModelingExercise exercise) { submission.setParticipation(participation); List<Result> filteredResults = submission.getResults().stream() .filter(result -> shouldIncludeResult(result, exercise)) .peek(Result::filterSensitiveInformation) .sorted(Comparator.comparing(Result::getCompletionDate).reversed()) .collect(Collectors.toList()); submission.setResults(filteredResults); return submission; } private boolean shouldIncludeResult(Result result, ModelingExercise exercise) { return ExerciseDateService.isAfterAssessmentDueDate(exercise) || result.getAssessmentType() == AssessmentType.AUTOMATIC_ATHENA; }
400-402: Consider externalizing error messages.Error messages are currently hardcoded in the controller. Consider moving them to a resource bundle or constants file for better maintainability and internationalization support.
Also applies to: 406-408
447-448: Consider using ResponseEntity builder pattern consistently.For better consistency with other endpoints in the class, consider using the builder pattern:
- return ResponseEntity.ok().body(submissionsWithResults); + return ResponseEntity.ok(submissionsWithResults);src/main/java/de/tum/cit/aet/artemis/assessment/repository/ResultRepository.java (1)
Line range hint
1-1000: Consider extracting business logic to a dedicated service layer.While the implementation is functionally correct, the repository has taken on significant business logic responsibilities (e.g., point calculations, assessment submissions). Consider:
- Moving calculation methods like
calculateTotalPoints,calculatePointsPerGradingCriterion, andsubmitResultto a dedicated service layer.- Creating a
ResultCalculationServicefor point-related operations.- Moving assessment-related operations to an
AssessmentService.This would:
- Improve adherence to the Single Responsibility Principle
- Make the code more maintainable and testable
- Keep the repository focused on data access concerns
src/main/webapp/app/exercises/modeling/participate/modeling-submission.component.html (3)
10-10: Inconsistent use of 'this' in template expressionsVariables in the template are inconsistently referenced with and without
this. For example:
- Line 10:
@if (isFeedbackView)- Line 28:
@if (modelingExercise.allowFeedbackRequests && (!this.modelingExercise.dueDate || !hasExerciseDueDatePassed(this.modelingExercise, this.participation)))- Line 39:
@if (!this.isFeedbackView)For consistency and to avoid confusion, please use a consistent approach when referencing component properties in the template. Preferably, omit
thiswhen accessing properties in the template.Apply this diff to make the property references consistent:
- @if (!this.isFeedbackView) { + @if (!isFeedbackView) { - @if (modelingExercise.allowFeedbackRequests && (!this.modelingExercise.dueDate || !hasExerciseDueDatePassed(this.modelingExercise, this.participation))) { + @if (modelingExercise.allowFeedbackRequests && (!modelingExercise.dueDate || !hasExerciseDueDatePassed(modelingExercise, participation))) {Also applies to: 28-28, 39-39
145-145: Redundant non-null assertion operators in conditionsIn the conditions on lines 145 and 148, non-null assertion operators are used unnecessarily:
- Line 145:
@if (!assessmentResult || !assessmentResult!.feedbacks || assessmentResult!.feedbacks!.length === 0)- Line 148:
@if (assessmentResult && assessmentResult!.feedbacks && assessmentResult!.feedbacks!.length > 0)Since you are already checking whether
assessmentResultandassessmentResult.feedbacksare null or undefined, the non-null assertions (!) are redundant and can be removed.Apply this diff to clean up the conditions:
- @if (!assessmentResult || !assessmentResult!.feedbacks || assessmentResult!.feedbacks!.length === 0) { + @if (!assessmentResult || !assessmentResult.feedbacks || assessmentResult.feedbacks.length === 0) { - @if (assessmentResult && assessmentResult!.feedbacks && assessmentResult!.feedbacks!.length > 0) { + @if (assessmentResult && assessmentResult.feedbacks && assessmentResult.feedbacks.length > 0) {Also applies to: 148-148
162-170: Simplify repeated conditional checks for 'feedback.reference'In this block, you repeatedly check
@if (feedback.reference)before displaying elements. To improve readability and reduce redundancy, consider wrapping the content within a single@if (feedback.reference)block.Refactored code:
@* Combine the repeated checks into one *@ @if (feedback.reference) { <span>{{ assessmentsNames[feedback.referenceId!]?.type }} </span> <code class="text-primary">{{ assessmentsNames[feedback.referenceId!]?.name }}</code> <br /> }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
src/main/java/de/tum/cit/aet/artemis/assessment/repository/ResultRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/athena/dto/ModelingFeedbackDTO.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingExerciseFeedbackService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingSubmissionResource.java(5 hunks)src/main/webapp/app/exercises/modeling/assess/modeling-assessment.util.ts(1 hunks)src/main/webapp/app/exercises/modeling/participate/modeling-participation.module.ts(2 hunks)src/main/webapp/app/exercises/modeling/participate/modeling-participation.route.ts(1 hunks)src/main/webapp/app/exercises/modeling/participate/modeling-submission.component.html(3 hunks)src/main/webapp/app/exercises/modeling/participate/modeling-submission.component.ts(14 hunks)src/main/webapp/app/exercises/modeling/participate/modeling-submission.service.ts(1 hunks)src/main/webapp/app/exercises/shared/result/result.component.ts(1 hunks)src/main/webapp/app/exercises/shared/result/result.utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
src/main/java/de/tum/cit/aet/artemis/assessment/repository/ResultRepository.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/athena/dto/ModelingFeedbackDTO.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/modeling/service/ModelingExerciseFeedbackService.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/modeling/web/ModelingSubmissionResource.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/exercises/modeling/assess/modeling-assessment.util.ts (1)
src/main/webapp/app/exercises/modeling/participate/modeling-participation.module.ts (1)
src/main/webapp/app/exercises/modeling/participate/modeling-participation.route.ts (1)
src/main/webapp/app/exercises/modeling/participate/modeling-submission.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/exercises/modeling/participate/modeling-submission.component.ts (1)
src/main/webapp/app/exercises/modeling/participate/modeling-submission.service.ts (1)
src/main/webapp/app/exercises/shared/result/result.component.ts (1)
src/main/webapp/app/exercises/shared/result/result.utils.ts (1)
🪛 Biome
src/main/webapp/app/exercises/modeling/participate/modeling-submission.component.ts
[error] 163-163: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (10)
src/main/webapp/app/exercises/modeling/participate/modeling-participation.route.ts (1)
19-28: LGTM! Verify component reuse strategy.
The route configuration follows Angular best practices and aligns with the PR objectives. The security guards and authorities are properly configured.
Let's verify the component handles route parameter changes correctly:
✅ Verification successful
The first search didn't find the component implementation. Let's try a broader search to locate the component and verify its subscription handling.
Route configuration is safe and component handles route parameters correctly
The ModelingSubmissionComponent implementation shows proper handling of route parameters and component lifecycle:
- The component implements OnDestroy and cleans up subscriptions properly
- Route parameters are handled safely through:
- Initial route params subscription in ngOnInit
- Proper cleanup of subscriptions in ngOnDestroy
- Safe handling of submissionId and participationId parameters
- Component reuse is handled correctly through:
- State reset between navigations
- Proper cleanup of websocket subscriptions
- Clear separation between feedback view and submission view modes
The route configuration can be safely used as is, with no risk of memory leaks or state persistence issues between route changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if ModelingSubmissionComponent implements OnDestroy and handles parameter changes
# Check for OnDestroy implementation and subscription cleanup
ast-grep --pattern 'export class ModelingSubmissionComponent implements $_ {
$$$
ngOnDestroy() {
$$$
}
$$$
}'
# Check for route parameter subscription
ast-grep --pattern 'this.route.params.subscribe'
Length of output: 6713
Script:
#!/bin/bash
# Let's check the ModelingSubmissionComponent implementation
fd "modeling-submission.component.ts" --exec cat {}
# Also check for any route parameter handling in parent components
fd "modeling-participation.component.ts" --exec cat {}
Length of output: 60446
src/main/java/de/tum/cit/aet/artemis/athena/dto/ModelingFeedbackDTO.java (1)
14-14: LGTM! Record structure aligns with feedback enhancement goals.
The change from List<String> elementIds to String reference simplifies the feedback reference structure while maintaining the DTO's single responsibility. This aligns well with the PR objective of enhancing diagram feedback display.
src/main/webapp/app/exercises/modeling/assess/modeling-assessment.util.ts (1)
Line range hint 96-120: LGTM! The filterInvalidFeedback function is well-implemented.
The function correctly handles null checks and efficiently filters invalid feedback elements.
src/main/webapp/app/exercises/modeling/participate/modeling-submission.service.ts (1)
140-147: Implementation aligns well with PR objectives.
The new method effectively supports the submission history timeline feature by retrieving submissions with their associated results, which is crucial for displaying AI feedback in the new Feedback View.
src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingExerciseFeedbackService.java (1)
176-176: LGTM - Change aligns with inline feedback requirements.
The addition of the reference field is essential for the new inline feedback feature, enabling the connection between feedback and specific diagram elements.
Let's verify the complete feedback mapping chain:
src/main/webapp/app/exercises/shared/result/result.component.ts (1)
265-275: LGTM! Implementation aligns with PR objectives.
The changes successfully implement the new navigation logic for TEXT and MODELING exercises, supporting the enhanced feedback mechanism.
src/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingSubmissionResource.java (3)
74-75: LGTM! Field declarations follow best practices.
The new fields follow constructor injection pattern and proper encapsulation principles.
Also applies to: 91-92
381-389: LGTM! Well-documented endpoint.
The Javadoc clearly describes the endpoint's purpose and behavior under different conditions.
395-419: LGTM! Comprehensive security checks.
The implementation properly validates:
- Participation existence and type
- User permissions
- Exam-specific access rules
src/main/java/de/tum/cit/aet/artemis/assessment/repository/ResultRepository.java (1)
107-108: LGTM! Method signature follows repository conventions.
The new method findFirstByParticipationIdAndAssessmentTypeOrderByCompletionDateDesc is well-designed and follows Spring Data JPA naming conventions. It appropriately extends the repository's capabilities to filter results by assessment type, which aligns with the PR's objective of enhancing the feedback mechanism.
Let's verify the usage of this new method:
✅ Verification successful
Method is used appropriately in the codebase
The search results confirm that:
- The method follows the same pattern as other repository methods for handling assessment types with results
- The codebase extensively uses AssessmentType with Results in various contexts:
- Result DTOs properly handle assessment types
- Services like ModelingSubmissionService and ProgrammingSubmissionService use assessment types for result filtering
- Assessment types are properly set and managed in test utilities
- The UI components handle different assessment types appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new method is being used appropriately in the codebase
# Test: Search for method usage and related assessment type handling
# Check for direct method usage
rg -A 3 "findFirstByParticipationIdAndAssessmentTypeOrderByCompletionDateDesc"
# Check for AssessmentType enum usage in the context of results
rg -A 3 "AssessmentType.*Result"
Length of output: 15901
src/main/webapp/app/exercises/modeling/participate/modeling-participation.module.ts
Show resolved
Hide resolved
src/main/webapp/app/exercises/modeling/assess/modeling-assessment.util.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/exercises/modeling/participate/modeling-submission.service.ts
Show resolved
Hide resolved
src/main/webapp/app/exercises/modeling/participate/modeling-submission.component.html
Outdated
Show resolved
Hide resolved
src/main/webapp/app/exercises/modeling/participate/modeling-submission.component.html
Outdated
Show resolved
Hide resolved
src/main/webapp/app/exercises/modeling/participate/modeling-submission.component.ts
Outdated
Show resolved
Hide resolved
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
LeonWehrhahn
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingExerciseFeedbackService.java (2)
120-123: Consider enhancing error handling with specific error typesThe error handling correctly updates the result state and notifies clients. However, consider differentiating between different types of failures to provide more specific feedback to users.
Consider this approach:
catch (Exception e) { log.error("Could not generate feedback for exercise ID: {} and participation ID: {}", modelingExercise.getId(), participation.getId(), e); + String errorMessage = switch (e) { + case NetworkingException ne -> "AI service is temporarily unavailable"; + case IllegalArgumentException iae -> "Invalid exercise configuration"; + default -> "AI Feedback could not be generated"; + }; automaticResult.setSuccessful(false); automaticResult.setCompletionDate(null); + automaticResult.setFeedbackText(errorMessage); participation.addResult(automaticResult); this.resultWebsocketService.broadcastNewResult(participation, automaticResult); - throw new InternalServerErrorException("Something went wrong... AI Feedback could not be generated"); + throw new InternalServerErrorException(errorMessage); }
209-232: Consider optimizing the duplicate submission checkThe validation logic is correct but could be more efficient by avoiding the need to load all submission results.
Consider adding a database query to check for existing Athena results:
+ @Query(""" + SELECT COUNT(r) > 0 + FROM Result r + WHERE r.submission.id = :submissionId + AND r.assessmentType = :assessmentType + """) + boolean hasResultWithAssessmentType(@Param("submissionId") Long submissionId, @Param("assessmentType") AssessmentType assessmentType); private void checkLatestSubmissionHasAthenaResultOrThrow(StudentParticipation participation) { Optional<Submission> submissionOptional = participationService .findExerciseParticipationWithLatestSubmissionAndResultElseThrow(participation.getId()) .findLatestSubmission(); if (submissionOptional.isEmpty()) { throw new BadRequestAlertException("No legal submissions found", "submission", "noSubmission"); } Submission submission = submissionOptional.get(); - Result latestResult = submission.getLatestResult(); - - if (latestResult != null && latestResult.getAssessmentType() == AssessmentType.AUTOMATIC_ATHENA) { + if (resultRepository.hasResultWithAssessmentType(submission.getId(), AssessmentType.AUTOMATIC_ATHENA)) { log.debug("Submission ID: {} already has an Athena result. Skipping feedback generation.", submission.getId()); throw new BadRequestAlertException( "Submission already has an Athena result", "submission", "submissionAlreadyHasAthenaResult", true ); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingExerciseFeedbackService.java(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingExerciseFeedbackService.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
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingExerciseFeedbackService.java (2)
73-73: LGTM: Good validation placement
The new validation is correctly placed before the async processing, following the fail-fast principle.
172-172: LGTM: Proper feedback reference integration
The addition of the reference field enables inline feedback display on diagrams, which aligns with the PR objectives.
src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingExerciseFeedbackService.java
Show resolved
Hide resolved
LeonWehrhahn
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
sachmii
added
deploy:artemis-test3
deploy:artemis-test1
and removed
deploy:artemis-test3
labels
sachmii
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
maximiliansoelch
added
ready to merge
maintainer-approved
krusche
changed the title
Modeling exercises: Inline AI Feedback ViewModeling exercises: Inline AI feedback view
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
When students requested AI feedback for modeling exercises in Artemis, the feedback was previously only displayed as a list in a pop-up window. This made it difficult for students to correlate feedback comments with the corresponding elements in their diagrams.
The goal of this PR is to enhance the feedback experience for modeling exercises by allowing students to view AI-generated feedback directly on the diagram, similar to how tutor feedback is presented. This provides a more intuitive and interactive way for students to receive and understand feedback.
Description
A new Feedback View for modeling exercises has been introduced to enhance the way students receive and interact with AI-generated feedback.
Accessing the Feedback View:
Key Features of the Feedback View:
Feedback Displayed Directly on the Diagram:
Unreferenced Feedback Listed Below the Diagram:
Submission History and Feedback Navigation:
Request Feedback Directly in the Editor:
Steps for Testing
Prerequisites:
Testing Steps:
Instructor Setup:
Student Participation:
Log in as a student.
Navigate to the modeling exercise.
Create a model using the modeling editor.
Request AI Feedback:
View Feedback on Diagram:
Submission History:
Access from Exercise Overview:
Exam Mode Testing
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
Screenshots
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes