General:Make Request Feedback a standalone component
#9323
Conversation
…he repository view
github-actions
bot
added
the
client
undernagruzez
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
undernagruzez
changed the title
General:Make Request Feedback a standalone component
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
github-actions
bot
added
tests
server
WalkthroughThe changes involve enhancements to error handling, validation logic, and feedback mechanisms across multiple components in the codebase. Key updates include improved error messages in 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: 28
🧹 Outside diff range comments (5)
src/main/webapp/app/overview/exercise-details/exercise-buttons.module.ts (1)
Line range hint
1-17: Summary: Changes align with PR objectives, pending verification.The changes in this file contribute to the PR's goal of making the Request Feedback feature a standalone component. The new
RequestFeedbackButtonComponentis imported and added to the module, which aligns with the objective of enhancing modularity and reusability.However, to ensure full compliance with Angular best practices and maintain consistency within the module, we need to verify if
RequestFeedbackButtonComponentis indeed a standalone component. Once confirmed, we can be certain that the implementation fully meets the PR objectives and follows the correct Angular architecture.After confirming the component's status, consider the following to further improve the module's architecture:
- If
RequestFeedbackButtonComponentis standalone, document this decision and consider gradually migrating other components to standalone for consistency.- If it's not standalone, adjust its integration into the module to match other components.
These steps will ensure that the module remains consistent and maintainable as it evolves.
src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.html (1)
Line range hint
234-244: Consider using the new feedback button component for text and modeling exercises.For consistency and to fully leverage the new standalone component, consider refactoring the feedback request button for text and modeling exercises to use the
<jhi-request-feedback-button>component. This would improve maintainability and ensure a uniform approach across different exercise types.Replace the current implementation with:
@if (exercise.allowFeedbackRequests && athenaEnabled && (exercise.type === ExerciseType.TEXT || exercise.type === ExerciseType.MODELING)) { <jhi-request-feedback-button [exercise]="exercise" [smallButtons]="smallButtons" [disabled]="!gradedParticipation?.submissions?.last()?.submitted || isGeneratingFeedback" ></jhi-request-feedback-button> }Note: You may need to adjust the
<jhi-request-feedback-button>component to handle thedisabledproperty and the specific behavior for text and modeling exercises.src/test/java/de/tum/cit/aet/artemis/exercise/participation/ParticipationIntegrationTest.java (1)
Line range hint
1659-1719: Good addition, but needs alignment with recent changesThe new test method
whenFeedbackRequestedAndRateLimitStillUnknownDueRequestsInProgress_thenFail()is a valuable addition to cover an important edge case. The overall structure and setup of the test are well done.There are a couple of points that need attention:
The error message assertion still uses the old generic message "preconditions not met". This should be updated to a more specific message, consistent with the changes made in other tests.
The number of Athena results generated (5) is inconsistent with the change made earlier in the file where it was increased to 20.
Please apply the following changes:
- Update the error message:
request.putAndExpectError("/api/exercises/" + programmingExercise.getId() + "/request-feedback", null, HttpStatus.BAD_REQUEST, "rateLimitUnknown");
- Align the number of Athena results:
for (int i = 0; i < MAX_ATHENA_RESULTS; i++) { // ... existing code ... }Make sure to use the
MAX_ATHENA_RESULTSconstant suggested earlier.src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.ts (2)
Line range hint
113-133: Refactor conditional logic for settingtheiaEnabledto improve clarityThe current logic sets
theiaEnabledtotrueand then conditionally sets it tofalsebased on multiple checks. It's clearer and more maintainable to initializetheiaEnabledtofalseand set it totrueonly if all conditions are met.Apply this diff to refactor the logic:
if (profileInfo.activeProfiles?.includes(PROFILE_THEIA) && this.programmingExercise) { - this.theiaEnabled = true; - - // Set variables now, sanitize later on - this.theiaPortalURL = profileInfo.theiaPortalURL ?? ''; - - // Verify that Theia's portal URL is set - if (this.theiaPortalURL === '') { - this.theiaEnabled = false; - } - - // Verify that the exercise allows the online IDE - if (!this.programmingExercise.allowOnlineIde) { - this.theiaEnabled = false; - } - - // Verify that the exercise has a theia blueprint configured - if (!this.programmingExercise.buildConfig?.theiaImage) { - this.theiaEnabled = false; - } + // Initialize as false + this.theiaEnabled = false; + + // Set variables now, sanitize later on + this.theiaPortalURL = profileInfo.theiaPortalURL ?? ''; + + // Verify that all conditions are met to enable Theia + if ( + this.theiaPortalURL !== '' && + this.programmingExercise.allowOnlineIde && + this.programmingExercise.buildConfig?.theiaImage + ) { + this.theiaEnabled = true; + } }
Line range hint
343-370: Fix logical error in the assignment oflatestResultThe assignment of
latestResultuses a logical AND (&&), which can result inlatestResultbeing assigned a boolean value (false) rather than the expected result object. This may cause unexpected behavior when accessing properties likelatestResult?.score.Apply this diff to correct the assignment:
-const latestResult = this.gradedParticipation?.results && this.gradedParticipation.results.find(({ assessmentType }) => assessmentType === AssessmentType.AUTOMATIC); +const latestResult = this.gradedParticipation?.results?.find(({ assessmentType }) => assessmentType === AssessmentType.AUTOMATIC);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (17)
- src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java (3 hunks)
- src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseCodeReviewFeedbackService.java (2 hunks)
- src/main/webapp/app/exercises/programming/shared/code-editor/actions/code-editor-actions.component.html (1 hunks)
- src/main/webapp/app/exercises/programming/shared/code-editor/actions/code-editor-actions.component.ts (3 hunks)
- src/main/webapp/app/exercises/programming/shared/code-editor/code-editor.module.ts (2 hunks)
- src/main/webapp/app/exercises/programming/shared/code-editor/container/code-editor-container.component.html (1 hunks)
- src/main/webapp/app/overview/exercise-details/exercise-buttons.module.ts (1 hunks)
- src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.html (1 hunks)
- src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.ts (4 hunks)
- src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.html (1 hunks)
- src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.ts (1 hunks)
- src/main/webapp/i18n/de/exercise.json (1 hunks)
- src/main/webapp/i18n/en/exercise.json (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/exercise/participation/ParticipationIntegrationTest.java (3 hunks)
- src/test/javascript/spec/component/exam/participate/exam-navigation-sidebar.component.spec.ts (2 hunks)
- src/test/javascript/spec/component/overview/exercise-details/request-feedback-button/request-feedback-button.component.spec.ts (1 hunks)
- src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (16)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.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_importssrc/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseCodeReviewFeedbackService.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_importssrc/main/webapp/app/exercises/programming/shared/code-editor/actions/code-editor-actions.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/programming/shared/code-editor/actions/code-editor-actions.component.ts (1)
src/main/webapp/app/exercises/programming/shared/code-editor/code-editor.module.ts (1)
src/main/webapp/app/exercises/programming/shared/code-editor/container/code-editor-container.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/overview/exercise-details/exercise-buttons.module.ts (1)
src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.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/overview/exercise-details/exercise-details-student-actions.component.ts (1)
src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.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/overview/exercise-details/request-feedback-button/request-feedback-button.component.ts (1)
src/main/webapp/i18n/de/exercise.json (1)
Pattern
src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".src/test/java/de/tum/cit/aet/artemis/exercise/participation/ParticipationIntegrationTest.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: truesrc/test/javascript/spec/component/exam/participate/exam-navigation-sidebar.component.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}src/test/javascript/spec/component/overview/exercise-details/request-feedback-button/request-feedback-button.component.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
🪛 Biome
src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.ts
[error] 60-60: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 62-62: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 83-83: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🔇 Additional comments (26)
src/main/webapp/app/overview/exercise-details/exercise-buttons.module.ts (2)
9-9: LGTM: Import statement follows Angular style guide.The import statement for
RequestFeedbackButtonComponentis correctly formatted and follows Angular's style guide. The component name uses PascalCase as required by the coding guidelines.
12-12: Verify if RequestFeedbackButtonComponent is a standalone component.The
RequestFeedbackButtonComponentis added to theimportsarray, which is typically used for modules. This suggests that it might be a standalone component (a feature introduced in Angular 14+). If this is the case, the current implementation is correct.However, for consistency with other components in this module (which are in the
declarationsarray), consider the following options:
- If
RequestFeedbackButtonComponentis not a standalone component, move it to thedeclarationsarray.- If it is a standalone component, consider converting other components to standalone as well for consistency across the module.
Could you confirm if
RequestFeedbackButtonComponentis indeed a standalone component? This will help ensure the module structure is correct and consistent.✅ Verification successful
Verified:
RequestFeedbackButtonComponentis a standalone component. Adding it to theimportsarray is correct.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if RequestFeedbackButtonComponent is a standalone component # Test: Search for the component definition rg --type typescript -A 5 'export class RequestFeedbackButtonComponent'Length of output: 108
Script:
#!/bin/bash # Description: Verify if RequestFeedbackButtonComponent is a standalone component # Test: Search for the component definition within .ts files rg 'export class RequestFeedbackButtonComponent' --glob '*.ts'Length of output: 235
Script:
#!/bin/bash # Description: Check if RequestFeedbackButtonComponent is a standalone component # Search for 'standalone: true' in the component's TypeScript file rg 'standalone:\s*true' src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.tsLength of output: 154
src/main/webapp/app/exercises/programming/shared/code-editor/actions/code-editor-actions.component.html (2)
1-3: LGTM! New request feedback button aligns with PR objectives.The addition of the standalone
<jhi-request-feedback-button>component is in line with the PR objectives to enhance modularity and reusability. The conditional rendering ensures the button is only displayed when an exercise is present, which is a good practice.A few observations:
- The @if syntax is correctly used, adhering to the new Angular template syntax guidelines.
- The null-safe navigation operator (?.) is appropriately used to prevent errors if
participation()is null.- The non-null assertion operator (!) is used when passing the exercise to the component, which assumes the exercise will always be defined when the condition is true.
1-3: Overall changes enhance UI functionality and align with PR objectives.The addition of the request feedback button at the beginning of the template enhances the user interface by providing easy access to the feedback feature. This change, combined with the existing action buttons, creates a comprehensive set of controls for the code editor component.
Key points:
- The new feedback button is correctly placed before other action buttons, giving it prominence.
- Existing functionality for handling conflicts, refreshing, and submitting remains intact.
- The conditional rendering ensures appropriate display of buttons based on the current state.
These changes successfully implement the PR objective of adding the request feedback button to the code repository view for non-exam settings, while maintaining the existing functionality.
src/main/webapp/app/exercises/programming/shared/code-editor/container/code-editor-container.component.html (1)
Line range hint
9-9: Excellent use of the new Angular syntax!The template correctly uses the new @if syntax instead of the older *ngIf, which aligns perfectly with the provided coding guidelines. This usage is consistent throughout the file.
Also applies to: 72-72
src/main/webapp/app/exercises/programming/shared/code-editor/code-editor.module.ts (4)
27-27: LGTM: Import of RequestFeedbackButtonComponentThe import statement for
RequestFeedbackButtonComponentis correctly implemented and follows the Angular style guide. The component's location in the file structure (overview/exercise-details) seems appropriate for its functionality.
Line range hint
27-39: Changes align well with PR objectivesThe integration of
RequestFeedbackButtonComponentas a standalone component in this module aligns perfectly with the PR objectives of enhancing modularity and reusability of the request feedback feature. The changes are minimal and focused, suggesting a clean implementation that doesn't disrupt existing functionality.Key observations:
- The component is correctly imported and added to the
importsarray, indicating its standalone nature.- No other parts of the module were modified, maintaining the integrity of existing components.
- The implementation follows Angular best practices for integrating standalone components.
These changes should contribute to improved maintainability and clearer separation of concerns in the codebase.
Line range hint
27-39: Verify implementation of visibility logic for exam modeWhile the integration of
RequestFeedbackButtonComponentlooks good, it's important to ensure that the visibility logic mentioned in the PR objectives is correctly implemented. Specifically:
- The button should be visible in the code repository view for non-exam settings.
- The button should not appear in exam mode.
Since this logic is not present in the current file, it's likely implemented within the
RequestFeedbackButtonComponentitself or in its parent component.To confirm the correct implementation of this logic, you can run the following script:
#!/bin/bash # Description: Check for exam mode visibility logic in RequestFeedbackButtonComponent # Test: Search for conditional rendering based on exam mode echo "Checking for exam mode visibility logic in RequestFeedbackButtonComponent:" rg --type typescript -A 5 $'(ngIf|\\[hidden\\]|\\*ngIf).*exam' src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.ts # Test: Search for property or method related to exam mode echo "Checking for exam mode related properties or methods:" rg --type typescript -A 5 $'(isExamMode|examMode|inExam)' src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.tsPlease ensure that the visibility logic is correctly implemented to meet the PR objectives.
39-39: Confirm standalone component configurationThe addition of
RequestFeedbackButtonComponentto theimportsarray suggests that it's implemented as a standalone component. This aligns with the PR objective of making the Request Feedback feature more modular and reusable.To ensure full compliance with Angular's standalone component pattern:
- Verify that
RequestFeedbackButtonComponentis decorated with@Componentand includesstandalone: truein its configuration.- Confirm that the component declares all its dependencies in its own
importsarray.You can run the following script to check the component's configuration:
src/test/javascript/spec/component/exam/participate/exam-navigation-sidebar.component.spec.ts (4)
23-23: LGTM: Import statement for ArtemisSharedCommonModule.The import statement for ArtemisSharedCommonModule is correctly placed and necessary for the changes in the TestBed configuration.
Line range hint
1-280: Summary and RecommendationThe changes to this test file are minimal, focusing on adding the ArtemisSharedCommonModule to the test configuration. This aligns with the PR objective of making the Request Feedback a standalone component. However, to ensure the integrity of the test suite and the component's functionality, I recommend the following actions before approving this PR:
- Run all suggested verification steps mentioned in the previous comments.
- If all tests pass and no unexpected behaviors are observed, this change can be approved.
- If any issues arise, please address them and update the test cases accordingly.
Once these steps are completed and any potential issues are resolved, this change can be confidently merged.
Line range hint
1-280: Ensure comprehensive testing after adding ArtemisSharedCommonModule.While no changes were made to the existing test cases, the addition of ArtemisSharedCommonModule could potentially affect the component's behavior. To ensure the integrity of the test suite:
- Run all related tests, not just for this file, to catch any potential side effects.
- Manually verify the ExamNavigationSidebarComponent's behavior in the actual application to ensure it still functions as expected.
Run the following commands to perform a comprehensive test:
#!/bin/bash # Run all tests related to the exam module npm test -- --testPathPattern=src/test/javascript/spec/component/exam # Run the entire test suite to catch any potential side effects npm testAfter running these tests, please report any failures or unexpected behaviors.
37-37: Verify the impact of ArtemisSharedCommonModule on existing tests.The addition of MockModule(ArtemisSharedCommonModule) to the imports array is consistent with the existing pattern. However, please ensure that this addition doesn't introduce any unintended side effects on the existing tests.
To verify the impact, please run the following command and check if all tests still pass:
src/main/webapp/app/exercises/programming/shared/code-editor/actions/code-editor-actions.component.ts (2)
17-17: LGTM: Import statement added correctly.The import for the
Participationmodel is properly placed and follows the coding guidelines.
Line range hint
1-324: Summary: Changes align with PR objectives.The modifications to this file are minimal and focused, supporting the PR objective of making the request feedback a standalone component. The addition of the
participationinput property allows the component to receive necessary data, which is likely used in the request feedback functionality.The changes do not alter existing logic, maintaining the integrity of the component while enhancing its capabilities. This aligns well with the PR summary, which emphasizes improved modularity and reusability.
To ensure that the
participationproperty is used correctly in the component's template or in child components, please run the following command:This will help verify that the new input property is being utilized as intended in the component's view or passed down to child components.
✅ Verification successful
Participation Property Usage Verified
The
participationinput property is correctly utilized in the component's template. It is properly accessed to conditionally render elements and to pass necessary data to thejhi-request-feedback-buttoncomponent, aligning with the PR's objective of enhancing functionality through modular components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the participation property in the component's template and child components rg --type html 'participation' src/main/webapp/app/exercises/programming/shared/code-editor/actions/Length of output: 461
src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.html (1)
Line range hint
1-280: Overall implementation looks good!The changes in this file align well with the PR objectives and adhere to the coding guidelines. The new
@ifand@forAngular syntax is correctly used throughout the file. The structure is clear, well-organized, and the logic seems sound.src/main/webapp/i18n/en/exercise.json (1)
171-171: Approved: Message key and content updated for clarity.The change from "notEnoughPoints" to "noSubmissionExists" with the message "You have to submit your work at least once." is a good improvement. It provides a clearer explanation of the requirement for requesting feedback, aligning with the PR objectives.
To ensure consistency across all language files, please run the following script:
This will help identify if similar updates are needed in other language files.
src/main/webapp/i18n/de/exercise.json (1)
171-171: LGTM! Translation is correct and follows guidelines.The German translation for the message when a user attempts to send a feedback request without any submissions is correct and follows the coding guidelines. It uses the informal "du" form as required, maintaining consistency with other translations in the file.
src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts (2)
78-78: New component import looks good.The import statement for the RequestFeedbackButtonComponent has been added correctly.
127-127: RequestFeedbackButtonComponent added to declarations.The RequestFeedbackButtonComponent has been properly added to the declarations array in the TestBed configuration using MockComponent.
src/test/java/de/tum/cit/aet/artemis/exercise/participation/ParticipationIntegrationTest.java (1)
27-27: LGTM: Import for @disabled annotation addedThe addition of the @disabled import is appropriate and aligns with the coding guidelines for using JUnit 5 features. This suggests that a test method will be disabled, which can be useful for temporarily skipping tests or marking tests that are not yet ready for execution.
src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.ts (1)
32-35:⚠️ Potential issueUse '@input' and '@output' decorators for inputs and outputs
Currently, inputs and outputs are declared using
inputandoutputfunctions, which is not standard Angular practice. Use@Input()and@Output()decorators to declare component inputs and outputs.-isGeneratingFeedback = input<boolean>(); +@Input() isGeneratingFeedback!: boolean; -smallButtons = input<boolean>(false); +@Input() smallButtons = false; -exercise = input.required<Exercise>(); +@Input() exercise!: Exercise; -generatingFeedback = output<void>(); +@Output() generatingFeedback = new EventEmitter<void>();Also, make sure to import
EventEmitterfrom@angular/core:+import { Component, OnInit, inject, Input, Output, EventEmitter } from '@angular/core';Likely invalid or redundant comment.
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseCodeReviewFeedbackService.java (1)
231-231: Verify the correctness of theBadRequestAlertExceptionconstructorAt line 231, you have added
trueas a fourth parameter to theBadRequestAlertExceptionconstructor. Ensure that this constructor exists and accepts a boolean as the fourth parameter.Run the following script to verify the constructor usage:
src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.ts (1)
Line range hint
258-277: MethodrequestFeedback()implementation looks goodThe
requestFeedback()method correctly handles condition checks, user confirmations, and service calls with proper success and error handling.src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java (1)
416-416: Verify the logic for checking if a feedback request has already been sentThe condition checks if
participationIndividualDueDateis not null and if the current date is after it, then throws an exception stating"Request has already been sent". Ensure this logic accurately determines whether a feedback request has already been sent, as passing the individual due date might not necessarily indicate that a request was previously sent.src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.html (1)
3-3: EnsureExerciseTypeis accessible in the templateIn line 3,
ExerciseType.TEXTis used within the template. Please verify that theExerciseTypeenum is properly imported and exposed in the component class so it's accessible in the template.You can confirm this by checking the component class and ensuring that
ExerciseTypeis a public property:import { ExerciseType } from 'app/entities/exercise.model'; @Component({ // ... }) export class RequestFeedbackButtonComponent { public ExerciseType = ExerciseType; // ... }
.../app/exercises/programming/shared/code-editor/container/code-editor-container.component.html
Show resolved
Hide resolved
...webapp/app/exercises/programming/shared/code-editor/actions/code-editor-actions.component.ts
Show resolved
Hide resolved
src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.html
Show resolved
Hide resolved
src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts
Show resolved
Hide resolved
| @@ -75,6 +75,7 @@ | |||
| import { MockResizeObserver } from '../../helpers/mocks/service/mock-resize-observer'; | |||
| import { MonacoEditorModule } from 'app/shared/monaco-editor/monaco-editor.module'; | |||
| import { CodeEditorMonacoComponent } from 'app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component'; | |||
| import { RequestFeedbackButtonComponent } from 'app/overview/exercise-details/request-feedback-button/request-feedback-button.component'; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Summary of changes and suggestions for improvement.
The addition of RequestFeedbackButtonComponent to the imports and declarations is a good start for integrating this new feature. However, to ensure robust testing and maintain the high quality of the codebase, consider the following improvements:
- Add specific tests for RequestFeedbackButtonComponent to verify its behavior within the CodeEditorContainerComponent.
- Update existing tests if RequestFeedbackButtonComponent interacts with or affects the behavior of other components.
- If applicable, add tests for any new functionality introduced by RequestFeedbackButtonComponent.
These additions will help maintain the comprehensive test coverage evident in the rest of the file.
Also applies to: 127-127
| // submissions.results is always undefined so this is neccessary | ||
| return ( | ||
| this.gradedParticipation.submissions.last()?.id === | ||
| this.gradedParticipation?.results.filter((result) => result.assessmentType == AssessmentType.AUTOMATIC_ATHENA).first()?.submission?.id | ||
| ); |
There was a problem hiding this comment.
Potential runtime errors due to non-standard first() and last() methods on arrays
JavaScript arrays do not have first() and last() methods by default, which could lead to runtime errors. Consider using array indexing to access the first and last elements.
Apply this diff to fix the issue:
-return (
- this.gradedParticipation.submissions.last()?.id ===
- this.gradedParticipation?.results.filter((result) => result.assessmentType == AssessmentType.AUTOMATIC_ATHENA).first()?.submission?.id
-);
+const latestSubmissionId = this.gradedParticipation.submissions?.[this.gradedParticipation.submissions.length - 1]?.id;
+const firstAthenaResultSubmissionId = this.gradedParticipation?.results.filter((result) => result.assessmentType === AssessmentType.AUTOMATIC_ATHENA)?.[0]?.submission?.id;
+return latestSubmissionId === firstAthenaResultSubmissionId;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // submissions.results is always undefined so this is neccessary | |
| return ( | |
| this.gradedParticipation.submissions.last()?.id === | |
| this.gradedParticipation?.results.filter((result) => result.assessmentType == AssessmentType.AUTOMATIC_ATHENA).first()?.submission?.id | |
| ); | |
| // submissions.results is always undefined so this is necessary | |
| const latestSubmissionId = this.gradedParticipation.submissions?.[this.gradedParticipation.submissions.length - 1]?.id; | |
| const firstAthenaResultSubmissionId = this.gradedParticipation?.results.filter((result) => result.assessmentType === AssessmentType.AUTOMATIC_ATHENA)?.[0]?.submission?.id; | |
| return latestSubmissionId === firstAthenaResultSubmissionId; |
| @@ -382,7 +381,7 @@ private ResponseEntity<StudentParticipation> handleExerciseFeedbackRequest(Exerc | |||
| throw new BadRequestAlertException("Not intended for the use in exams", "participation", "preconditions not met"); | |||
| } | |||
| if (exercise.getDueDate() != null && now().isAfter(exercise.getDueDate())) { | |||
| throw new BadRequestAlertException("The due date is over", "participation", "preconditions not met"); | |||
| throw new BadRequestAlertException("The due date is over", "participation", "feedbackRequestAfterDueDate", true); | |||
There was a problem hiding this comment.
Consider using AccessForbiddenException instead of BadRequestAlertException
Requesting feedback after the due date is a forbidden action. Using AccessForbiddenException, which results in a 403 Forbidden status code, would more accurately represent the access restriction compared to BadRequestAlertException (400 Bad Request).
| @@ -393,7 +392,7 @@ | |||
| StudentParticipation participation = (exercise instanceof ProgrammingExercise) | |||
| ? programmingExerciseParticipationService.findStudentParticipationByExerciseAndStudentId(exercise, principal.getName()) | |||
| : studentParticipationRepository.findByExerciseIdAndStudentLogin(exercise.getId(), principal.getName()) | |||
| .orElseThrow(() -> new ResourceNotFoundException("Participation not found")); | |||
| .orElseThrow(() -> new BadRequestAlertException("Participation not found", "participation", "noSubmissionExists", true)); | |||
There was a problem hiding this comment.
Use EntityNotFoundException for missing participation
When a participation is not found, it's appropriate to throw an EntityNotFoundException, which corresponds to a 404 Not Found status code. This provides a clearer indication of the error compared to BadRequestAlertException (400 Bad Request).
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java
Show resolved
Hide resolved
...app/overview/exercise-details/request-feedback-button/request-feedback-button.component.html
Show resolved
Hide resolved
EneaGore
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
undernagruzez
changed the base branch from
develop
to
feature/programming-exercises/revised-feedback
undernagruzez
changed the base branch from
feature/programming-exercises/revised-feedback
to
develop
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
|
Implemented in #refactoring of preli |
!! Can be tested only on TS1 !!
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
Currently, the request feedback button is embedded within other components, making it less reusable and harder to maintain. Error handling around this feature takes place both on the client and server side. Now, it only happens on the server side. Additionally, students do not have the ability to request feedback directly from the code repository view in non-exam settings.
Description
Refactored the request feedback button into a standalone component to improve modularity and reusability.
Enhanced error handling to provide clearer feedback to users when issues occur during feedback requests.
Added the request feedback button to the code repository view for non-exam settings, allowing students to request feedback directly from there.
Ensured the button did not appear in exam mode to maintain exam integrity.
Steps for Testing
Prerequisites:
1 Instructor
1 Student
1 Programming Exercise with Feedback Requests enabled
Log in as an instructor.
Create a programming exercise with Feedback Requests enabled(set the due date, choose manual assessment, tick the checkboxes for Athena feedback suggestions and feedback requests)
Log in to Artemis as a student.
Navigate to the programming exercise.
Open the code repository view (non-exam setting).
Verify that the request feedback button is visible.
Click the request feedback button.
Confirm that a feedback request is successfully submitted and a confirmation message is displayed.
Exam Mode Testing
Prerequisites:
1 Instructor
1 Student
1 Exam with a Programming Exercise
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
Exam Mode Test
Performance Tests
Test Coverage
Info: ✅ ❌ in Confirmation (assert/expect) have to be adjusted manually, also delete trivial files!
Client
Server
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Improvements
User Interface
Localization
Testing