Programming exercises: Improve code button

[SimonEntholzer]

Checklist

General

• I tested all changes and their related features with all corresponding user types on a test server.
• Language: I followed the guidelines for inclusive, diversity-sensitive, and appreciative language.
• I chose a title conforming to the naming conventions for pull requests.

Server

• Important: I implemented the changes with a very good performance and prevented too many (unnecessary) and too complex database calls.
• I strictly followed the principle of data economy for all database calls.
• I strictly followed the server coding and design guidelines.
• I documented the Java code using JavaDoc style.

Client

• Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data (e.g. using paging).
• I strictly followed the principle of data economy for all client-server REST calls.
• I strictly followed the client coding and design guidelines.
• Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
• I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
• I documented the TypeScript code using JSDoc style.
• I added multiple screenshots/screencasts of my UI changes.
• I translated all newly inserted strings into English and German.

Changes affecting Programming Exercises

• High priority: I tested all changes and their related features with all corresponding user types on a test server configured with the integrated lifecycle setup (LocalVC and LocalCI).
• I tested all changes and their related features with all corresponding user types on a test server configured with Gitlab and Jenkins.

Motivation and Context

The code-button code is quite complex and bug prone.

Description

Fixes #9972
Adds the following features:

• in the .yml configuration admins can configure which authentication methods to use in a simple way, by providing an array, e.g. ['password', 'ssh', 'token']. The code button will also show these mechanisms in that order.
• The code button will remember the last used state, and use it as the default when opening the button.
• Makes a clear separation of token usage between courses and course-management routes. For course routes, the code-button uses the participation token, as this is the students view. For the instructor view, it always uses the user token.

Steps for Testing

Prerequisites:

1. Create a programming exercise
2. Participate as a student, by using the code button to clone the repository (SSH and via token)
3. As the instructor go to Course Management -> Exercises -> Participations, and use the clone button. Check if the token missing message is shown if your user does not have a user token (Settings -> VCS token)
4. Create a team programming exercise, create a team, and test the above steps

Exam Mode Testing

1. Create an exam with a programming exercise, participate as a student, and as an instructor check that you can use the code button in the participations view.

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

Code Review

• Code Review 1
• Code Review 2

Manual Tests

• Test 1
• Test 2

Test Coverage

Screenshots

E.g. authentication set to ['ssh', 'token'], so the password/https option is not available:

Summary by CodeRabbit

Release Notes

• Authentication

  • Introduced a new configuration for authentication mechanisms.
  • Removed specific version control access token settings.

• User Interface

  • Updated repository link handling in various components.
  • Refined code button functionality across different views.
  • Enhanced visibility of programming exam summary section.
  • Removed repository password display options.

• Localization

  • Removed deprecated password-related translation entries.

• Access Control

  • Enhanced route protection for user settings based on user roles.

These changes enhance the authentication and repository access mechanisms, providing more flexible and streamlined user interactions.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (4)

src/main/webapp/app/shared/components/code-button/code-button.component.ts (3)

85-86: Extract localStorage key as a constant.

The localStorage key 'code-button-state' is hardcoded in multiple places. Extract it as a constant for better maintainability.

+private readonly LOCAL_STORAGE_KEY = 'code-button-state';

private storeToLocalStorage() {
-    this.localStorage.store('code-button-state', this.currentState);
+    this.localStorage.store(this.LOCAL_STORAGE_KEY, this.currentState);
}

Also applies to: 209-210

---

85-86: Initialize currentState in constructor.

The currentState initialization from localStorage should be moved to the constructor for better state management.

constructor() {
    this.isInCourseManagement = this.router.url.includes('course-management');
+   this.currentState = this.localStorage.retrieve(this.LOCAL_STORAGE_KEY) || States.Password;

    effect(async () => {
        // ...
    });
}

onClick() {
-   this.currentState = this.localStorage.retrieve('code-button-state') || States.Password;

    if (this.useSsh) {
        this.useSshUrl();
    }
    if (this.useToken) {
        this.useHttpsToken();
    }
    if (this.usePassword) {
        this.useHttpsPassword();
    }
}

Also applies to: 221-232

---

282-284: Centralize error handling and improve error messages.

Error handling is scattered and error messages could be more descriptive.

+private handleError(error: HttpErrorResponse, context: string) {
+    if (error.status === 403) {
+        this.alertService.warning(`artemisApp.exerciseActions.${context}.forbidden`);
+    } else if (error.status === 404) {
+        this.alertService.warning(`artemisApp.exerciseActions.${context}.notFound`);
+    } else {
+        this.alertService.error(`artemisApp.exerciseActions.${context}.error`);
+    }
+    throw error;
+}

loadParticipationVcsAccessToken(participation: ProgrammingExerciseStudentParticipation) {
    return this.accountService.getVcsAccessToken(participation.id).subscribe({
        next: (res: HttpResponse<string>) => {
            if (res.body) {
                participation.vcsAccessToken = res.body;
                this.copyEnabled = this.useToken;
            }
        },
        error: (error: HttpErrorResponse) => {
            if (error.status === 404) {
                this.createNewParticipationVcsAccessToken(participation);
            }
-           if (error.status == 403) {
-               this.alertService.warning('403 Forbidden');
-           }
+           this.handleError(error, 'token');
        },
    });
}

src/test/javascript/spec/component/shared/code-button.component.spec.ts (1)

148-158: Improve test description and assertions.

The test description could be more descriptive, and assertions could be more specific.

-it('should not load participation vcsAccessToken when it already exists in participation', async () => {
+it('should skip loading vcsAccessToken from server when participation already has a valid token', async () => {
    participation.vcsAccessToken = 'vcpat-1234';
    fixture.componentRef.setInput('participations', [participation]);

    await component.ngOnInit();
    component.onClick();

-   expect(component.user.vcsAccessToken).toEqual(vcsToken);
+   expect(component.user.vcsAccessToken).toBe(vcsToken);
    expect(getVcsAccessTokenSpy).not.toHaveBeenCalled();
    expect(createVcsAccessTokenSpy).not.toHaveBeenCalled();
+   expect(participation.vcsAccessToken).toBe('vcpat-1234');
});

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 62b2f71 and 83da1fc.

📒 Files selected for processing (4)

• src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.html (1 hunks)
• src/main/webapp/app/shared/components/code-button/code-button.component.html (2 hunks)
• src/main/webapp/app/shared/components/code-button/code-button.component.ts (9 hunks)
• src/test/javascript/spec/component/shared/code-button.component.spec.ts (9 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.html
• src/main/webapp/app/shared/components/code-button/code-button.component.html

🧰 Additional context used

📓 Path-based instructions (2)

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/main/webapp/app/shared/components/code-button/code-button.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

📓 Learnings (1)

src/main/webapp/app/shared/components/code-button/code-button.component.ts (1)

Learnt from: SimonEntholzer
PR: ls1intum/Artemis#10147
File: src/main/webapp/app/shared/components/code-button/code-button.component.ts:1-1
Timestamp: 2025-01-18T08:49:30.693Z
Learning: In Angular 19+ projects, component inputs should use the new input() function from @angular/core instead of the @Input() decorator, as it's part of the new signals architecture that improves performance and type safety.

⏰ Context from checks skipped due to timeout of 90000ms (6)

• GitHub Check: server-tests
• GitHub Check: client-tests-selected
• GitHub Check: client-tests
• GitHub Check: Analyse
• GitHub Check: Build and Push Docker Image
• GitHub Check: Build .war artifact

🔇 Additional comments (2)

src/main/webapp/app/shared/components/code-button/code-button.component.ts (1)

241-254: 🛠️ Refactor suggestion

Enhance security of credential handling in URIs.

The current implementation exposes credentials in the URI. Consider using a more secure approach.

src/test/javascript/spec/component/shared/code-button.component.spec.ts (1)

184-186: Use toHaveBeenCalledOnce for better test readability.

Replace toHaveBeenCalled with toHaveBeenCalledOnce for better test readability.

-expect(getVcsAccessTokenSpy).toHaveBeenCalled();
+expect(getVcsAccessTokenSpy).toHaveBeenCalledOnce();

[SimonEntholzer]

Code looks good overall. Just two minor remarks from my side. I also have a general question: Since personal access tokens are not used for students anymore, would it make sense to add a guard to the /user-settings/vcs-token path, so that students can't access it via the url? This is currently still possible.

That's a good point, this would make sense now. I added it that students can now no longer access these settings via path.

[chrisknedl]

Code. Thanks for implementing my suggestions!

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCInfoContributorTest.java (2)

16-21: Enhance test method clarity and maintainability.

1. The test method name testContribute is too generic. Consider renaming it to describe the specific behavior being tested, e.g., shouldContributeAuthenticationMechanismsToInfo.

2. Consider extracting the authentication mechanisms list as a constant or documenting why these specific values were chosen:

private static final List<String> EXPECTED_AUTHENTICATION_MECHANISMS = List.of("password", "token", "ssh");

3. The use of reflection might indicate that orderedAuthenticationMechanisms should be passed through the constructor or a setter method instead.

---

29-29: Enhance assertion specificity and error messages.

The current assertion could be more robust. Consider:

assertThat(info.getDetails())
    .as("Info details should not be null")
    .isNotNull()
    .containsKey("authenticationMechanisms");

assertThat(info.getDetails().get("authenticationMechanisms"))
    .as("Authentication mechanisms should match the configured values")
    .isNotNull()
    .isInstanceOf(List.class)
    .asList()
    .containsExactlyElementsOf(authenticationMechanisms);

This provides:

• Null checks
• Type verification
• More descriptive error messages
• Exact list comparison

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f114d21 and 0bd00f0.

📒 Files selected for processing (1)

• src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCInfoContributorTest.java (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCInfoContributorTest.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: true

⏰ Context from checks skipped due to timeout of 90000ms (9)

• GitHub Check: Codacy Static Code Analysis
• GitHub Check: server-tests
• GitHub Check: client-tests-selected
• GitHub Check: client-tests
• GitHub Check: client-style
• GitHub Check: server-style
• GitHub Check: Build and Push Docker Image
• GitHub Check: Build .war artifact
• GitHub Check: Analyse

🔇 Additional comments (1)

src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCInfoContributorTest.java (1)

Line range hint 1-13: LGTM! Clean imports and proper class structure.

The imports are well-organized, and the test class follows proper naming conventions.

[dfuchss]

Code looks good overall. Just two minor remarks from my side. I also have a general question: Since personal access tokens are not used for students anymore, would it make sense to add a guard to the /user-settings/vcs-token path, so that students can't access it via the url? This is currently still possible.

That's a good point, this would make sense now. I added it that students can now no longer access these settings via path.

The API is still accessible for Instructors and Teaching Assistants, right? Because we rely on this API.

[SimonEntholzer]

Code looks good overall. Just two minor remarks from my side. I also have a general question: Since personal access tokens are not used for students anymore, would it make sense to add a guard to the /user-settings/vcs-token path, so that students can't access it via the url? This is currently still possible.

That's a good point, this would make sense now. I added it that students can now no longer access these settings via path.

The API is still accessible for Instructors and Teaching Assistants, right? Because we rely on this API.

Yes, that only affects users which exclusively have the student role.

The API itself wasn't touched at all here tbh., only the routing to the token settings. So TAs and instructors are not affected in any way.

[az108]

Code reapprove 👍