Programming exercises: Provide theia clone information on redirect
#9379
Conversation
WalkthroughThe changes introduce significant updates to the JWT handling and token management in a Java and TypeScript application. Key modifications include the addition of a Changes
Assessment against linked issues
Possibly related issues
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
|
github-actions
bot
added
tests
server
…ia-clone-information-on-redirect
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range comments (4)
src/test/javascript/spec/helpers/mocks/service/mock-programming-exercise.service.ts (1)
Line range hint
1-35: Consider enhancing type safety and consistency across mock methods.The
MockProgrammingExerciseServiceclass is well-structured and consistent in its approach to mocking service methods. To further improve its robustness and usefulness in tests, consider the following suggestions:
Use TypeScript's generic types with
of()to ensure type safety. For example:getBuildConfig = (exerciseId: number) => of<ProgrammingExerciseBuildConfig>({});Consider creating a base interface or type for the service, which this mock class can implement. This would ensure that the mock service stays in sync with the actual service interface.
For methods that return empty objects, consider using
of(null)orof(undefined)instead ofof({}), unless an empty object is the expected return type.Add JSDoc comments to methods, especially for those with complex parameter structures, to improve readability and maintainability.
These changes would enhance the overall quality and reliability of the mock service in test scenarios.
src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.ts (2)
Line range hint
117-121: LGTM! Consider refactoring for code reuse.The addition of code to set
localVCEnabledandathenaEnabledbased on active profiles aligns with the PR objectives and follows Angular best practices.Consider refactoring the profile checking logic into a separate method to reduce code duplication:
private setProfileFlags(profiles: string[] | undefined) { this.localVCEnabled = profiles?.includes(PROFILE_LOCALVC) ?? false; this.athenaEnabled = profiles?.includes(PROFILE_ATHENA) ?? false; } ngOnInit(): void { // ... existing code ... if (this.exercise.type === ExerciseType.PROGRAMMING) { this.profileService.getProfileInfo().subscribe((profileInfo) => { this.setProfileFlags(profileInfo.activeProfiles); }); } else if (this.exercise.type === ExerciseType.TEXT) { this.editorLabel = 'openTextEditor'; this.profileService.getProfileInfo().subscribe((profileInfo) => { this.setProfileFlags(profileInfo.activeProfiles); }); } // ... rest of the method ... }This refactoring improves code reuse and maintainability.
Also applies to: 126-128
Duplicate Assignments Detected for
athenaEnabled
- The property
athenaEnabledis assigned multiple times withinexercise-details-student-actions.component.ts(Lines 11-24), which may be redundant.Please review these assignments to ensure they are both necessary and intentional.
🔗 Analysis chain
Line range hint
52-53: LGTM! Please clarify the purpose of new properties.The removal of Theia-related properties and the addition of
localVCEnabledandathenaEnabledproperties align with the PR objectives.Could you please clarify the specific use cases for
localVCEnabledandathenaEnabled? This will help ensure that these new properties are used appropriately throughout the component.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of localVCEnabled and athenaEnabled echo "Usage of localVCEnabled:" rg '\blocalVCEnabled\b' src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.ts echo "Usage of athenaEnabled:" rg '\bathenaEnabled\b' src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.tsLength of output: 680
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java (1)
Line range hint
1-1010: Consider refactoring for improved maintainability.While the current implementation is functional and follows a consistent pattern, consider the following suggestions for future improvements:
Split the controller: This file is quite large and handles many different operations. Consider splitting it into smaller, more focused controller classes based on functionality (e.g.,
ProgrammingExerciseCreationController,ProgrammingExerciseUpdateController, etc.).Extract common operations: Some operations, like authorization checks and error handling, are repeated across multiple methods. Consider extracting these into separate service methods to reduce duplication and improve maintainability.
Use of constants: Consider defining constants for common string literals (e.g., error messages, entity names) to improve consistency and ease of maintenance.
These suggestions are not critical for the current functionality but could help improve the overall structure and maintainability of the codebase in the long term.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (11)
- src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseBuildConfigRepository.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java (6 hunks)
- src/main/webapp/app/exercises/programming/manage/services/programming-exercise.service.ts (2 hunks)
- src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.html (0 hunks)
- src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.ts (1 hunks)
- src/main/webapp/app/shared/components/code-button/code-button.component.html (1 hunks)
- src/main/webapp/app/shared/components/code-button/code-button.component.ts (5 hunks)
- src/test/javascript/spec/component/overview/exercise-details/exercise-details-student-actions.component.spec.ts (0 hunks)
- src/test/javascript/spec/component/shared/code-button.component.spec.ts (5 hunks)
- src/test/javascript/spec/helpers/mocks/service/mock-programming-exercise.service.ts (2 hunks)
💤 Files not reviewed due to no reviewable changes (2)
- src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.html
- src/test/javascript/spec/component/overview/exercise-details/exercise-details-student-actions.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (9)
src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.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/repository/ProgrammingExerciseBuildConfigRepository.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/web/ProgrammingExerciseResource.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/manage/services/programming-exercise.service.ts (1)
src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.ts (1)
src/main/webapp/app/shared/components/code-button/code-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/shared/components/code-button/code-button.component.ts (1)
src/test/javascript/spec/component/shared/code-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/helpers/mocks/service/mock-programming-exercise.service.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}}
📓 Learnings (1)
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java (1)
Learnt from: Hialus PR: ls1intum/Artemis#8607 File: src/main/java/de/tum/in/www1/artemis/web/rest/programming/ProgrammingExerciseResource.java:64-64 Timestamp: 2024-06-15T20:04:18.637Z Learning: For the Artemis project, import statements are automatically managed by formatters and should not be commented on.
🪛 Biome
src/main/webapp/app/shared/components/code-button/code-button.component.ts
[error] 144-144: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 409-409: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 413-413: 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)
🔇 Additional comments (21)
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseBuildConfigRepository.java (1)
38-47: LGTM! The new method enhances repository functionality.The addition of the
findByExerciseIdElseThrowmethod is a valuable enhancement to the repository. It follows our coding guidelines, maintains consistency with existing methods, and provides a useful abstraction for error handling when retrieving build configs.src/test/javascript/spec/helpers/mocks/service/mock-programming-exercise.service.ts (1)
5-5: LGTM: Import statement added correctly.The new import for
ProgrammingExerciseBuildConfigis properly added and consistent with the existing import style.src/main/webapp/app/shared/components/code-button/code-button.component.html (1)
Line range hint
1-106: Overall assessment: Good implementation with room for minor improvementsThe changes in this file successfully integrate the online IDE button into the existing code button component. The implementation aligns well with the PR objectives and follows Angular best practices. The conditional rendering and security measures are appropriately implemented.
A few minor suggestions were made to improve usability and user experience, particularly for mobile devices and during the IDE startup process. These improvements would further enhance the seamless integration of Theia into the Artemis exercise flow.
src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.ts (1)
Line range hint
1-524: Overall, the changes look good and align with the PR objectives.The modifications to this component successfully remove Theia-related functionality and integrate new features as intended. The code adheres to Angular best practices and the provided coding guidelines.
A few minor suggestions have been made for improvement:
- Organizing imports alphabetically
- Clarifying the purpose of new properties
- Refactoring profile checking logic for better code reuse
These suggestions aim to enhance code readability and maintainability. Great job on implementing these changes!
src/main/webapp/app/exercises/programming/manage/services/programming-exercise.service.ts (1)
664-666: LGTM: New methodgetBuildConfigadded correctly.The new
getBuildConfigmethod is well-implemented and consistent with the existing codebase. It follows the established patterns for HTTP requests and return types.src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java (2)
501-514: LGTM! New endpoint for retrieving build config implemented correctly.The new
getBuildConfigmethod is well-implemented:
- It uses proper access control with
@EnforceAtLeastStudentInExercise.- The request is logged for debugging purposes.
- It correctly retrieves the build config from the repository.
- The response is properly constructed and returned.
The implementation looks good and follows the existing patterns in the class.
Line range hint
165-178: LGTM! Constructor updated correctly to include new repository.The constructor has been properly updated to include the new
ProgrammingExerciseBuildConfigRepository:
- The new parameter is added in a logical position.
- The parameter is correctly assigned to the class field in the constructor body.
- The change maintains the existing code style and structure.
This update is consistent with the addition of the new
getBuildConfigmethod.src/main/webapp/app/shared/components/code-button/code-button.component.ts (5)
13-13: Import Statement Updated CorrectlyImporting
PROFILE_THEIAis appropriate for enabling Theia IDE integration.
16-16: faDesktop Icon Imported SuccessfullyThe
faDesktopicon is correctly imported for use in the component.
19-19: ProgrammingExerciseService Injected ProperlyThe
ProgrammingExerciseServiceis correctly imported and injected into the constructor for fetching build configurations.Also applies to: 92-92
75-76: Properties Added for Theia IntegrationNew properties
theiaEnabledandtheiaPortalURLare added to manage the state and URL for Theia IDE, following camelCase naming conventions.
81-81: faDesktop Icon Assigned CorrectlyThe
faDesktopicon is correctly assigned to be used in the template.src/test/javascript/spec/component/shared/code-button.component.spec.ts (9)
8-8: Import statement is appropriate.The classes
ExerciseandExerciseTypeare correctly imported and utilized in the test code.
32-36: Import statements are necessary and correct.The imported entities are required for the new tests and are appropriately used.
43-43: Declaration ofprogrammingExerciseServiceis proper.This service is needed for injecting and spying on methods within the tests.
51-52: Spy instances forgetProfileInfoandgetBuildConfigare correctly declared.These spies are essential for mocking the return values in the tests.
97-104:exerciseobject is well-defined for testing purposes.The properties are appropriately set to simulate various scenarios in the tests.
127-127: MockingProgrammingExerciseServiceusingMockProgrammingExerciseService.This aligns with the guideline to mock irrelevant dependencies using
NgMocks.
129-134: Proper initialization and mocking ofgetProfileInfo.The use of
compileComponents().then()ensures the component is compiled before mocking, which is acceptable in this context.
140-140: Injection ofProgrammingExerciseServiceinto the test bed is appropriate.This allows for effective spying and mocking of its methods.
478-559: Parameterized tests effectively cover multiple scenarios.The use of
it.eachprovides comprehensive coverage for different configurations, enhancing test reliability.
.../de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseBuildConfigRepository.java
Show resolved
Hide resolved
src/test/javascript/spec/helpers/mocks/service/mock-programming-exercise.service.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.html
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.java
Show resolved
Hide resolved
src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range comments (1)
src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.ts (1)
Issues Found: Remove or update references to
startOnlineIDEincode-button.component.html.The
startOnlineIDEmethod is still being referenced insrc/main/webapp/app/shared/components/code-button/code-button.component.html. Please remove or update this reference to prevent potential runtime errors.🔗 Analysis chain
Line range hint
1-341: LGTM! Verify template file for any lingering references.The removal of the
startOnlineIDEmethod aligns with the PR objectives of removing Theia-related functionality.Please run the following script to check if there are any remaining references to the removed
startOnlineIDEmethod in the component's template file:
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/main/webapp/app/shared/components/code-button/code-button.component.ts (5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/shared/components/code-button/code-button.component.ts (1)
🪛 Biome
src/main/webapp/app/shared/components/code-button/code-button.component.ts
[error] 144-144: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Outdated
Show resolved
Hide resolved
dmytropolityka
changed the title
Programming: Provide theia clone information on redirectProgramming: Provide theia clone information on redirect
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Outdated
Show resolved
Hide resolved
github-actions
bot
added
core
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (5)
- src/main/java/de/tum/cit/aet/artemis/core/web/open/PublicUserJwtResource.java (2 hunks)
- src/main/webapp/app/core/auth/account.service.ts (1 hunks)
- src/main/webapp/app/shared/components/code-button/code-button.component.ts (5 hunks)
- src/test/javascript/spec/component/shared/code-button.component.spec.ts (5 hunks)
- src/test/javascript/spec/helpers/mocks/service/mock-account.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/main/java/de/tum/cit/aet/artemis/core/web/open/PublicUserJwtResource.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/core/auth/account.service.ts (1)
src/main/webapp/app/shared/components/code-button/code-button.component.ts (1)
src/test/javascript/spec/component/shared/code-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/helpers/mocks/service/mock-account.service.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/shared/components/code-button/code-button.component.ts
[error] 364-364: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
src/test/javascript/spec/component/shared/code-button.component.spec.ts
[error] 600-600: 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)
🪛 GitHub Check: client-tests-selected
src/test/javascript/spec/component/shared/code-button.component.spec.ts
[failure] 601-601:
Element implicitly has an 'any' type because expression of type 'string' can't be used to index type '{ appDef: string; gitUri: string; gitToken: string; }'.
🪛 GitHub Check: client-tests
src/test/javascript/spec/component/shared/code-button.component.spec.ts
[failure] 601-601:
Element implicitly has an 'any' type because expression of type 'string' can't be used to index type '{ appDef: string; gitUri: string; gitToken: string; }'.
🔇 Additional comments (5)
src/main/java/de/tum/cit/aet/artemis/core/web/open/PublicUserJwtResource.java (3)
30-30: LGTM: New imports are appropriate and follow guidelines.The new imports for
RequestParamandEnforceAtLeastStudentare correctly added and specific to the new functionality. They adhere to the guideline of avoiding star imports.Also applies to: 37-37
Line range hint
1-173: Summary: Changes align well with PR objectives, with minor suggestions for improvement.The implementation of the new
reKeymethod inPublicUserJwtResourcealigns well with the PR objectives of streamlining the user experience and minimizing clicks for accessing the online IDE. The changes are focused, well-implemented, and adhere to the provided coding guidelines.Key points:
- The new method provides a way to re-key the user's authentication token, which can be returned as a bearer token or set as a cookie.
- The implementation follows good practices such as the single responsibility principle and proper use of annotations.
- Minor suggestions for improvement include adding Javadoc comments and using more descriptive parameter names.
- A security verification is recommended to ensure the new endpoint doesn't introduce vulnerabilities.
Overall, the changes effectively contribute to the goal of enhancing the integration of Theia within the Artemis platform, improving the user experience for programming exercises.
99-115: Verify security implications of the new re-key endpoint.While the new
reKeymethod is well-implemented and doesn't affect existing functionality, it's crucial to ensure that this new endpoint doesn't introduce any security vulnerabilities. Please verify the following:
- The
@EnforceAtLeastStudentannotation provides sufficient access control for this sensitive operation.- The token generation process in
jwtCookieService.buildLoginCookie(true)is secure and uses appropriate expiration times.- Proper logging is in place for this operation to track potential misuse.
- Consider rate limiting this endpoint to prevent abuse.
To help verify the security of this new endpoint, you can run the following commands:
src/main/webapp/app/core/auth/account.service.ts (1)
Line range hint
1-390: Overall assessment: Changes are well-implemented and align with PR objectivesThe addition of the
rekeyCookieToBearerTokenmethod to the AccountService is a well-executed change that supports the PR's goal of enhancing the Theia User-Flow integration. The implementation adheres to Angular style guidelines and integrates smoothly with the existing service without introducing breaking changes.To ensure the changes work as expected across the codebase:
This will help verify that the new authentication method is properly integrated and that there are no outstanding TODOs related to the Theia integration.
src/main/webapp/app/shared/components/code-button/code-button.component.ts (1)
403-406: 🧹 Nitpick (assertive)Inform the user if the IDE window cannot be opened
If the call to
window.openfails (e.g., due to a popup blocker), the user may not understand why nothing happened. Consider adding a user-friendly message to inform them.Modify the code as follows:
if (!newWindow) { + alert('Please disable your popup blocker to open the online IDE.'); return; }This will help users troubleshoot the issue more easily.
Likely invalid or redundant comment.
src/test/javascript/spec/helpers/mocks/service/mock-account.service.ts
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/web/open/PublicUserJwtResource.java
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Outdated
Show resolved
Hide resolved
src/test/javascript/spec/component/shared/code-button.component.spec.ts
Outdated
Show resolved
Hide resolved
src/test/javascript/spec/component/shared/code-button.component.spec.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/main/webapp/app/shared/components/code-button/code-button.component.ts (5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/shared/components/code-button/code-button.component.ts (1)
🪛 Biome
src/main/webapp/app/shared/components/code-button/code-button.component.ts
[error] 382-384: Expected a statement but instead found '}
async startOnlineIDE()'.Expected a statement here.
(parse)
[error] 396-396: Illegal return statement outside of a function
(parse)
[error] 364-364: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🪛 GitHub Check: client-tests-selected
src/main/webapp/app/shared/components/code-button/code-button.component.ts
[failure] 382-382:
Declaration or statement expected.
[failure] 384-384:
Unexpected keyword or identifier.
[failure] 384-384:
';' expected.
🪛 GitHub Check: client-tests
src/main/webapp/app/shared/components/code-button/code-button.component.ts
[failure] 382-382:
Declaration or statement expected.
[failure] 384-384:
Unexpected keyword or identifier.
[failure] 384-384:
';' expected.
🪛 GitHub Check: client-compilation
src/main/webapp/app/shared/components/code-button/code-button.component.ts
[failure] 382-382:
Declaration or statement expected.
[failure] 384-384:
Unexpected keyword or identifier.
[failure] 384-384:
';' expected.
🔇 Additional comments (3)
src/main/webapp/app/shared/components/code-button/code-button.component.ts (3)
385-392: Validate Data Before Form SubmissionEnsure that all required data fields are present before submitting the form to prevent unexpected errors.
Consider adding checks to validate the
dataobject:if (!data.appDef || !data.gitUri || !data.artemisToken) { alert('Missing required data to start the Online IDE.'); return; }
76-77: 🛠️ Refactor suggestionRename
theiaPortalURLto Follow CamelCase ConventionPer the coding guidelines, property names should be in camelCase. The property
theiaPortalURLshould be renamed totheiaPortalUrlfor consistency.Apply this diff to rename the property:
- theiaPortalURL: string; + theiaPortalUrl: string;Remember to update all references to this property in the code.
Likely invalid or redundant comment.
401-402:⚠️ Potential issueConsider Using
POSTMethod for Submitting Sensitive DataThe
gitTokenandartemisTokenare sensitive information and should not be sent via aGETrequest, as query parameters may be logged or exposed in browser history. Switching toPOSTenhances security by transmitting data in the request body.Apply this diff to change the form method to
POST:- form.method = 'GET'; + form.method = 'POST'; form.action = this.theiaPortalURL;Ensure that the server endpoint accepts
POSTrequests and handles the data appropriately.Likely invalid or redundant comment.
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/components/code-button/code-button.component.ts
Outdated
Show resolved
Hide resolved
janthoXO
added
stacked-pr
github-actions
bot
added
the
deployment-error
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.java (1)
28-28: Consider caching strategy for build configurations.
While the JSON serialization is properly handled with @JsonIgnoreProperties, consider implementing caching for frequently accessed build configurations to improve performance, especially since this configuration will be more frequently accessed with the enhanced Theia integration. This could be achieved using Spring's caching abstractions or a distributed cache like Redis for better scalability.
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java (1)
503-516: Consider enhancing error handling and logging.
The endpoint is well-implemented with proper security, documentation, and response handling. However, consider these improvements:
- Add more detailed error handling for specific scenarios
- Enhance logging to include user context for better traceability
Example enhancement:
@GetMapping("programming-exercises/{exerciseId}/build-config")
@EnforceAtLeastStudentInExercise
public ResponseEntity<ProgrammingExerciseBuildConfig> getBuildConfig(@PathVariable long exerciseId) {
- log.debug("REST request to get build config of ProgrammingExercise : {}", exerciseId);
+ var user = userRepository.getUserWithGroupsAndAuthorities();
+ log.debug("REST request to get build config of ProgrammingExercise : {} by user : {}", exerciseId, user.getLogin());
var buildConfig = programmingExerciseBuildConfigRepository.findByExerciseIdElseThrow(exerciseId);
return ResponseEntity.ok().body(buildConfig);
}📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- src/main/java/de/tum/cit/aet/artemis/core/config/websocket/WebsocketConfiguration.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/aet/artemis/core/config/websocket/WebsocketConfiguration.java
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.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/programming/web/ProgrammingExerciseResource.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
📓 Learnings (1)
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java (3)
Learnt from: Hialus
PR: ls1intum/Artemis#8607
File: src/main/java/de/tum/in/www1/artemis/web/rest/programming/ProgrammingExerciseResource.java:64-64
Timestamp: 2024-06-15T20:04:18.637Z
Learning: For the Artemis project, import statements are automatically managed by formatters and should not be commented on.
Learnt from: Hialus
PR: ls1intum/Artemis#8607
File: src/main/java/de/tum/in/www1/artemis/web/rest/programming/ProgrammingExerciseResource.java:64-64
Timestamp: 2024-10-08T15:35:42.972Z
Learning: For the Artemis project, import statements are automatically managed by formatters and should not be commented on.
Learnt from: Hialus
PR: ls1intum/Artemis#8607
File: src/main/java/de/tum/in/www1/artemis/web/rest/programming/ProgrammingExerciseResource.java:64-64
Timestamp: 2024-10-08T15:35:48.767Z
Learning: For the Artemis project, import statements are automatically managed by formatters and should not be commented on.
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java (1)
120-121: LGTM! Field declaration and constructor injection are well implemented.
The changes follow the coding guidelines by:
- Using constructor injection
- Properly declaring the field as private final
- Initializing the field in the constructor
Also applies to: 165-167, 178-178
iyannsch
added
deploy:artemis-test9
github-actions
bot
added
lock:artemis-test9
and removed
deploy:artemis-test9
github-actions
bot
added
the
config-change
|
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. |
iyannsch
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
…ia-clone-information-on-redirect
…ide-theia-clone-information-on-redirect
…ture/programming-exercises/provide-theia-clone-information-on-redirect
github-actions
bot
removed
the
core
…ture/programming-exercises/provide-theia-clone-information-on-redirect
Tip
This PR stacks on top of
feature/bearer-supportandfeature/re-keyDeploy to TS9 for @Theia profile
Use the
artemis_admincredentials for login.Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Changes affecting Programming Exercises
Motivation and Context
The Theia User-Flow aims to integrate seamlessly into the existing Artemis exercise flow. When users click on the Open Online IDE button, we want to require as few clicks as possible before the exercise can be worked. In #8723, we defined that no further login should be required - at least for working with the repository.
In separate repositories, I already implemented the Theia LandingPage which can accept the git clone token for the user and instantly spawn a new session with the repo cloned using the token.
Fixes #9357
Description
This PR clears up the UI, moving the
Open in Online IDEbutton into the existing<Code>button. Catering to the required data-sharing, the user's clone token, the user's exercise repo, and the configured Theia Settings are passed to the Landing Page.Steps for Testing
Prerequisites:
Codebutton contains theOpen Online IDEbutton.appDefshould match your selected ImagegitUrishould match your clone URL and the git tokenartemisTokenshould be existent but you don't have to verify thatHere, you can see a screenshot highlighting the relevant parts.
Exam Mode Testing
Make sure that none of that functionality is visible in the Exam Mode.
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
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests