Programming exercises: Provide theia clone information on redirect
#10344
Conversation
…ia-clone-information-on-redirect
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…into feature/re-key
…ide-theia-clone-information-on-redirect
…ia-clone-information-on-redirect
…ide-theia-clone-information-on-redirect
…ia-clone-information-on-redirect
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
src/test/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResourceTest.java
Outdated
Show resolved
Hide resolved
…ia-clone-information-on-redirect
…ovide-theia-clone-information-on-redirect' into feature/programming-exercises/provide-theia-clone-information-on-redirect
src/test/javascript/spec/component/shared/code-button.component.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/javascript/spec/component/shared/code-button.component.spec.ts (1)
475-520: Consider splitting the test and using more specific assertions.The test effectively verifies the form submission, but could be improved:
- Split into separate tests for form structure and data validation.
- Use
toHaveBeenCalledOnce()consistently instead of mixing withtoHaveBeenCalled().Apply this diff to improve the test:
- expect(documentAppendChildSpy).toHaveBeenCalledOnce(); + expect(documentAppendChildSpy).toHaveBeenCalledExactlyOnceWith(form);Consider splitting into two tests:
it('should create form with correct structure', async () => { // Test form method, action, and target }); it('should include correct data in form inputs', async () => { // Test input values });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/javascript/spec/component/shared/code-button.component.spec.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
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/shared/code-button.component.spec.ts
🧠 Learnings (1)
src/test/javascript/spec/component/shared/code-button.component.spec.ts (1)
Learnt from: iyannsch
PR: ls1intum/Artemis#10344
File: src/test/javascript/spec/component/shared/code-button.component.spec.ts:0-0
Timestamp: 2025-02-16T16:00:38.131Z
Learning: In test cases, prefer removing unnecessary type assertions over adding mock properties that aren't relevant to what's being tested. This keeps tests focused and reduces maintenance burden.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: client-tests-selected
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (2)
src/test/javascript/spec/component/shared/code-button.component.spec.ts (2)
21-23: LGTM! Clean service setup.The new imports and service injection follow Angular testing best practices.
Also applies to: 32-32, 102-102
389-473: LGTM! Well-structured test cases for Theia visibility.The test cases effectively cover various scenarios for Theia button visibility using parameterized testing. Good practices include:
- Clear test descriptions
- Comprehensive coverage of edge cases
- Proper mocking of dependencies
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/webapp/app/shared/components/code-button/code-button.component.ts (2)
466-519: 🛠️ Refactor suggestionEnhance security and modernize the Online IDE launch.
The implementation needs improvements in security and modern practices:
- Replace deprecated toPromise() with firstValueFrom
- Use POST method to avoid exposing sensitive data in URL
- Simplify URL construction
- Add error handling
async startOnlineIDE() { - const artemisToken: string = (await this.accountService.getToolToken('SCORPIO').toPromise()) ?? ''; + try { + const artemisToken = await firstValueFrom(this.accountService.getToolToken('SCORPIO')) ?? ''; + const artemisUrl = new URL(window.location.href).origin; - let artemisUrl: string = ''; - if (window.location.protocol) { - artemisUrl += window.location.protocol + '//'; - } - if (window.location.host) { - artemisUrl += window.location.host; - } - - const prevAuthMech = this.selectedAuthenticationMechanism; - this.selectedAuthenticationMechanism = RepositoryAuthenticationMethod.Token; + const prevAuthMech = this.selectedAuthenticationMechanism; + this.selectedAuthenticationMechanism = RepositoryAuthenticationMethod.Token; - const data = { - appDef: this.exercise()?.buildConfig?.theiaImage ?? '', - gitUri: this.getHttpOrSshRepositoryUri(false), - gitUser: this.user.name, - gitMail: this.user.email, - artemisToken: artemisToken, - artemisUrl: artemisUrl, - }; + const data = { + appDef: this.exercise()?.buildConfig?.theiaImage ?? '', + gitUri: this.getHttpOrSshRepositoryUri(false), + gitUser: this.user.name, + gitMail: this.user.email, + artemisToken, + artemisUrl, + }; - this.selectedAuthenticationMechanism = prevAuthMech; + this.selectedAuthenticationMechanism = prevAuthMech; - const newWindow = window.open('', '_blank'); - if (!newWindow) { - return; - } + const newWindow = window.open('', '_blank'); + if (!newWindow) { + throw new Error('Failed to open new window. Please check your popup blocker settings.'); + } - newWindow.name = 'Theia-IDE'; + newWindow.name = 'Theia-IDE'; - const form = document.createElement('form'); - form.method = 'GET'; - form.action = this.theiaPortalURL; + const form = document.createElement('form'); + form.method = 'POST'; // Use POST to avoid exposing sensitive data in URL + form.action = this.theiaPortalURL; + form.target = newWindow.name; - form.target = newWindow.name; + // Create input fields for form data + Object.entries(data).forEach(([key, value]) => { + const hiddenField = document.createElement('input'); + hiddenField.type = 'hidden'; + hiddenField.name = key; + hiddenField.value = value ?? ''; + form.appendChild(hiddenField); + }); - // Loop over data element and create input fields - for (const key in data) { - if (Object.hasOwn(data, key)) { - const hiddenField = document.createElement('input'); - hiddenField.type = 'hidden'; - hiddenField.name = key; - const descriptor = Object.getOwnPropertyDescriptor(data, key); - hiddenField.value = descriptor ? descriptor.value : ''; - form.appendChild(hiddenField); - } + document.body.appendChild(form); + form.submit(); + document.body.removeChild(form); + } catch (error) { + console.error('Failed to start online IDE:', error); + this.alertService.error('artemisApp.exerciseActions.startOnlineIdeError'); } - - document.body.appendChild(form); - form.submit(); - document.body.removeChild(form); }
445-464: 🛠️ Refactor suggestionImprove type safety and error handling in Theia initialization.
The initialization logic needs improvements in type safety and error handling:
- Replace @ts-expect-error comments with proper type guards
- Add error handling for the HTTP request
private initTheia(profileInfo: ProfileInfo) { - if (profileInfo.activeProfiles?.includes(PROFILE_THEIA) && this.exercise()) { - // @ts-expect-error (The exercise is not undefined here) - this.programmingExerciseService.getTheiaConfig(this.exercise().id!).subscribe((theiaConfig) => { - // Merge the theiaConfig (containing the theiaImage) into the buildConfig - // @ts-expect-error (The exercise is not undefined here) - this.exercise().buildConfig = { ...this.exercise()?.buildConfig, ...theiaConfig }; - - // Set variables now, sanitize later on - this.theiaPortalURL = profileInfo.theiaPortalURL ?? ''; - - // Verify that all conditions are met - // @ts-expect-error (The exercise is not undefined here) - if (this.theiaPortalURL !== '' && this.exercise().allowOnlineIde && this.exercise().buildConfig?.theiaImage) { - this.theiaEnabled = true; - } - }); + const exercise = this.exercise(); + if (!profileInfo.activeProfiles?.includes(PROFILE_THEIA) || !exercise?.id) { + return; + } + + this.programmingExerciseService.getTheiaConfig(exercise.id).subscribe({ + next: (theiaConfig) => { + exercise.buildConfig = { ...exercise.buildConfig, ...theiaConfig }; + this.theiaPortalURL = profileInfo.theiaPortalURL ?? ''; + + this.theiaEnabled = Boolean( + this.theiaPortalURL && + exercise.allowOnlineIde && + exercise.buildConfig?.theiaImage + ); + }, + error: (error) => { + console.error('Failed to fetch Theia config:', error); + this.theiaEnabled = false; + this.alertService.error('artemisApp.exerciseActions.theiaConfigError'); + } + }); }
🧹 Nitpick comments (2)
src/test/javascript/spec/component/shared/code-button.component.spec.ts (2)
22-22: Remove unused import.The
ProgrammingExerciseBuildConfigimport is not used in the code.-import { ProgrammingExerciseBuildConfig } from 'app/entities/programming/programming-exercise-build.config';🧰 Tools
🪛 GitHub Check: client-tests-selected
[failure] 22-22:
'ProgrammingExerciseBuildConfig' is declared but its value is never read.🪛 GitHub Check: client-tests
[failure] 22-22:
'ProgrammingExerciseBuildConfig' is declared but its value is never read.
476-521: Enhance form submission test assertions.Consider using
toHaveBeenCalledExactlyOnceWithfor more precise assertions on form inputs.- expect(documentAppendChildSpy).toHaveBeenCalledOnce(); + expect(documentAppendChildSpy).toHaveBeenCalledExactlyOnceWith(expect.any(HTMLFormElement)); - expect(formSubmitSpy).toHaveBeenCalledOnce(); + expect(formSubmitSpy).toHaveBeenCalledExactlyOnceWith();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/de/tum/cit/aet/artemis/programming/dto/ProgrammingExerciseTheiaConfigDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java(6 hunks)src/main/webapp/app/entities/programming/programming-exercise-theia.config.ts(1 hunks)src/main/webapp/app/exercises/programming/manage/services/programming-exercise.service.ts(2 hunks)src/main/webapp/app/shared/components/code-button/code-button.component.ts(7 hunks)src/test/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResourceTest.java(1 hunks)src/test/javascript/spec/component/shared/code-button.component.spec.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/webapp/app/exercises/programming/manage/services/programming-exercise.service.ts
- src/test/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResourceTest.java
- src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java
🧰 Additional context used
📓 Path-based instructions (3)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/entities/programming/programming-exercise-theia.config.tssrc/main/webapp/app/shared/components/code-button/code-button.component.ts
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/dto/ProgrammingExerciseTheiaConfigDTO.java
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
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/shared/code-button.component.spec.ts
🧠 Learnings (1)
src/test/javascript/spec/component/shared/code-button.component.spec.ts (1)
Learnt from: iyannsch
PR: ls1intum/Artemis#10344
File: src/test/javascript/spec/component/shared/code-button.component.spec.ts:0-0
Timestamp: 2025-02-16T16:00:38.131Z
Learning: In test cases, prefer removing unnecessary type assertions over adding mock properties that aren't relevant to what's being tested. This keeps tests focused and reduces maintenance burden.
🪛 GitHub Check: client-tests-selected
src/test/javascript/spec/component/shared/code-button.component.spec.ts
[failure] 22-22:
'ProgrammingExerciseBuildConfig' is declared but its value is never read.
🪛 GitHub Check: client-tests
src/test/javascript/spec/component/shared/code-button.component.spec.ts
[failure] 22-22:
'ProgrammingExerciseBuildConfig' is declared but its value is never read.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (4)
src/main/webapp/app/entities/programming/programming-exercise-theia.config.ts (1)
1-3: LGTM! Well-structured configuration class.The class follows TypeScript naming conventions and properly marks the
theiaImageproperty as optional.src/main/java/de/tum/cit/aet/artemis/programming/dto/ProgrammingExerciseTheiaConfigDTO.java (1)
5-7: LGTM! Well-designed DTO record.The record is immutable and uses
@JsonIncludeto ensure clean JSON output by excluding empty values.src/test/javascript/spec/component/shared/code-button.component.spec.ts (1)
390-474: LGTM! Well-structured test cases for Theia button visibility.The parameterized tests effectively cover various scenarios for Theia button visibility using descriptive test cases.
src/main/webapp/app/shared/components/code-button/code-button.component.ts (1)
13-13: LGTM!The new imports are correctly organized and necessary for the Theia IDE integration.
Also applies to: 16-16, 33-33
| @@ -66,6 +67,7 @@ export class CodeButtonComponent implements OnInit { | |||
| private localStorage = inject(LocalStorageService); | |||
| private participationService = inject(ParticipationService); | |||
| private ideSettingsService = inject(IdeSettingsService); | |||
| private programmingExerciseSerice = inject(ProgrammingExerciseService); | |||
There was a problem hiding this comment.
Fix typo in service name.
The service variable name has a typo: programmingExerciseSerice should be programmingExerciseService.
- private programmingExerciseSerice = inject(ProgrammingExerciseService);
+ private programmingExerciseService = inject(ProgrammingExerciseService);📝 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.
| private programmingExerciseSerice = inject(ProgrammingExerciseService); | |
| private programmingExerciseService = inject(ProgrammingExerciseService); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/javascript/spec/component/shared/code-button.component.spec.ts (2)
475-520: Enhance form submission test with additional assertions.While the test covers the basic form submission flow, consider adding assertions for:
- The
appDefinput field value- The
gitTokeninput field value- Form cleanup after submission
Apply this enhancement:
const gitUriTest = Array.from(inputs).find((input) => input.name === 'gitUri'); expect(gitUriTest).toBeDefined(); expect(gitUriTest!.value).toBe(data.gitUri); + +const appDefTest = Array.from(inputs).find((input) => input.name === 'appDef'); +expect(appDefTest).toBeDefined(); +expect(appDefTest!.value).toBe(data.appDef); + +const gitTokenTest = Array.from(inputs).find((input) => input.name === 'gitToken'); +expect(gitTokenTest).toBeDefined(); +expect(gitTokenTest!.value).toBe(data.gitToken); + +expect(documentRemoveChildSpy).toHaveBeenCalledWith(form);
489-490: Use more specific assertion for tool token retrieval.Replace
toHaveBeenCalledOnce()withtoHaveBeenCalledExactlyOnceWith()for more precise test coverage.Apply this enhancement:
-expect(getToolTokenSpy).toHaveBeenCalledOnce(); +expect(getToolTokenSpy).toHaveBeenCalledExactlyOnceWith();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/javascript/spec/component/shared/code-button.component.spec.ts(4 hunks)src/test/javascript/spec/helpers/mocks/service/mock-programming-exercise.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/javascript/spec/helpers/mocks/service/mock-programming-exercise.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
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/shared/code-button.component.spec.ts
🧠 Learnings (1)
src/test/javascript/spec/component/shared/code-button.component.spec.ts (1)
Learnt from: iyannsch
PR: ls1intum/Artemis#10344
File: src/test/javascript/spec/component/shared/code-button.component.spec.ts:0-0
Timestamp: 2025-02-16T16:00:38.131Z
Learning: In test cases, prefer removing unnecessary type assertions over adding mock properties that aren't relevant to what's being tested. This keeps tests focused and reduces maintenance burden.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests-selected
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (3)
src/test/javascript/spec/component/shared/code-button.component.spec.ts (3)
20-23: LGTM! Well-organized imports.The new imports are correctly organized and specifically target the required Theia-related functionality.
32-32: LGTM! Proper service injection setup.The
ProgrammingExerciseServiceis correctly injected using TestBed.Also applies to: 102-102
389-473: LGTM! Comprehensive test cases for Theia visibility.The parameterized test cases effectively cover various scenarios for Theia button visibility:
- Profile activation
- Theia configuration
- Online IDE activation
- URL configuration
| // Theia requires the Build Config of the programming exercise to be set | ||
| // @ts-expect-error (The exercise is not undefined here) | ||
| this.programmingExerciseSerice.getTheiaConfig(this.exercise().id!).subscribe((theiaConfig) => { |
There was a problem hiding this comment.
can you use an exlamation mark instead of the ts-expect-error annotation here?
| // Theia requires the Build Config of the programming exercise to be set | |
| // @ts-expect-error (The exercise is not undefined here) | |
| this.programmingExerciseSerice.getTheiaConfig(this.exercise().id!).subscribe((theiaConfig) => { | |
| this.programmingExerciseSerice.getTheiaConfig(this.exercise()!.id!).subscribe((theiaConfig) => { |
There was a problem hiding this comment.
can we avoid exlamation mark fully and add a if check before? Would prefer that :D
| // Merge the theiaConfig (containing the theiaImage) into the buildConfig | ||
| // @ts-expect-error (The exercise is not undefined here) | ||
| this.exercise().buildConfig = { ...this.exercise()?.buildConfig, ...theiaConfig }; |
Deploy to TS9 for @Theia profile
Use the
artemis_admincredentials for login.Important
The following failing server-tests are not due to the code in this PR but also fail on develop
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.
As the Theia deployment is connected correctly now, you might need to take a look at your path parameters to verify them
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
Test Coverage
Summary by CodeRabbit
New Features
UI Changes