Programming exercises: Fix routing to repository view
#10228
Conversation
…re/update-angular19
…re/update-angular19
…re/update-angular19
…re/update-angular19
…re/update-angular19
To fix pdf-preview-enlarged-canvas.component.spec.ts tests and pdf-preview-thumbnail-grid.component.spec.ts tests
…re/update-angular19
github-actions
bot
added
tests
server
SimonEntholzer
changed the title
Programming Exercises: Fix routing to repository viewProgramming exercises: Fix routing to repository view
WalkthroughThis pull request updates the routing configurations and repository access logic across the application. URL segments and parameter names have been simplified by removing redundant parts (such as an extra Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Button as "jhi-code-button"
participant Router as "Angular Router"
participant RepoView as "RepositoryViewComponent"
User->>Button: Click repository button
Button->>Router: Navigate using routerLink array (with repositoryId & context)
Router->>RepoView: Load Repository View with extracted parameters
RepoView->>Router: Retrieve parameters (isInCourseManagement, repositoryId)
RepoView-->>User: Render repository interface
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)src/main/webapp/app/exam/manage/exam-management.route.ts (1)📓 Learnings (1)src/main/webapp/app/exam/manage/exam-management.route.ts (1)⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (3)
✨ Finishing Touches
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: 3
🧹 Nitpick comments (16)
src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.ts (1)
76-76: Consider using Angular Router events for more robust route checking.While
router.url.includes()works, consider using Angular's Router events for more reliable route state management:-this.isInCourseManagement = this.router.url.includes('course-management'); +this.router.events.pipe( + filter((event): event is NavigationEnd => event instanceof NavigationEnd) +).subscribe((event) => { + this.isInCourseManagement = event.url.includes('course-management'); +});Don't forget to clean up the subscription in ngOnDestroy to prevent memory leaks:
private routerSubscription: Subscription; ngOnDestroy() { this.routerSubscription?.unsubscribe(); }src/main/webapp/app/localvc/commit-details-view/commit-details-view.component.ts (2)
76-76: Consider an enum or constant for repositoryType checks
Hardcoding'USER'is understandable, but consider anenumor constant to improve maintainability and avoid typos in multiple components.
130-131: Align naming in the diff report with repositoryId usage
UsingparticipationIdForLeftCommitandparticipationIdForRightCommitto storerepositoryIdmight be confusing. If feasible, rename them in the underlying model for clarity.src/main/webapp/app/shared/components/code-button/code-button.component.ts (5)
99-102: copyEnabled interplay is tidy but watch UX edge cases
Enabling copying is good for user convenience—ensure the UI gracefully handles edge cases (e.g., user tries to copy an expired token).
126-145: Reactive constructor with 'effect' is advanced but take care with watchers
The effect blocks keep the constructor logic clean. Minimize side effects in these watchers to ensure maintainability.
171-171: Use a meaningful docstring for configureTooltips
While this line is a small addition, consider adding or updating docstrings to clarify how tooltips are configured.
258-263: Loading VCS tokens for all participations is thorough
This loop usage is ideal. Consider parallelization only if performance becomes an issue.
270-283: Graceful fallback if tokens are not found
Your error handling for 404 and 403 is straightforward. Possibly use a more user-friendly message than “403 Forbidden.”src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCInfoContributor.java (1)
33-34: LGTM! Enhanced authentication mechanism flexibility.The ordered list of authentication mechanisms provides better flexibility and control compared to boolean flags.
Consider adding a comment explaining the significance of the authentication order:
+ /** + * Ordered list of authentication mechanisms for repository access. + * Order determines the priority: first mechanism is tried first. + * Supported values: password, token, ssh + */ @Value("${artemis.version-control.repository-authentication-mechanisms:password,token,ssh}") private List<String> orderedAuthenticationMechanisms;src/main/webapp/app/shared/layouts/profiles/profile-info.model.ts (1)
33-33: LGTM! Frontend model aligns with backend changes.The ProfileInfo model correctly reflects the backend changes to authentication mechanisms.
Consider adding JSDoc for better type documentation:
+ /** + * Ordered list of authentication mechanisms available for repository access. + * Example: ['password', 'token', 'ssh'] + */ public authenticationMechanisms?: string[];src/main/webapp/app/localvc/vcs-repository-access-log-view/vcs-repository-access-log-view.component.ts (1)
Line range hint
24-28: Consider renaming the variable for clarity.The variable
participationIdis derived fromrepositoryIdin the route params, which could be confusing. Consider renaming it to match its source or add a comment explaining the mapping.- private readonly participationId = computed(() => { - const participationId = this.params().repositoryId; - if (participationId) { - return Number(participationId); + private readonly participationId = computed(() => { + // Using repositoryId as participationId for backward compatibility + const repositoryId = this.params().repositoryId; + if (repositoryId) { + return Number(repositoryId);src/main/webapp/app/shared/layouts/profiles/profile.service.ts (1)
73-73: LGTM! Good architectural improvement.Replacing boolean flags with an array of authentication mechanisms provides more flexibility and extensibility for handling multiple authentication methods.
src/main/webapp/app/localvc/commit-history/commit-history.component.ts (1)
33-33: Consider separating the dual roles of repositoryId.While the comment clarifies the dual purpose, having a single property serve as both
participationIdandrepositoryIdcould lead to confusion. Consider using a discriminated union type or separate properties for better type safety and clarity.- repositoryId?: number; // acts as both participationId (USER repositories) and repositoryId (AUXILIARY repositories), undefined for TEMPLATE, SOLUTION and TEST + type RepositoryIdentifier = + | { type: 'USER'; participationId: number } + | { type: 'AUXILIARY'; repositoryId: number } + | { type: 'TEMPLATE' | 'SOLUTION' | 'TEST' }; + repositoryIdentifier?: RepositoryIdentifier;src/main/webapp/app/localvc/repository-view/repository-view.component.ts (1)
77-77: Add documentation for isInCourseManagement property.The purpose and usage of this property are not clear. Please add JSDoc comments explaining:
- What this flag controls
- When it should be true/false
- Any side effects of its value
+ /** + * Indicates whether the repository view is being accessed from course management. + * This affects the behavior of certain UI elements and routing. + */ isInCourseManagement = false;src/main/webapp/app/exercises/programming/shared/code-editor/actions/code-editor-actions.component.ts (1)
65-65: Add type annotation and consider property initialization.The routerLink property could be improved for better type safety and initialization.
- routerLink: string; + routerLink = this.router.url; ngOnInit(): void { - this.routerLink = this.router.url; this.conflictStateSubscription = this.conflictServiceAlso applies to: 91-91
src/main/webapp/app/shared/components/code-button/code-button.component.html (1)
8-10: Consider performance implications of method bindings.Converting property bindings to method calls (
loading(),smallButtons(),hideLabelMobile()) may impact performance as these methods will be called during every change detection cycle.Consider using properties with setters/getters or observables if reactive updates are needed:
-[buttonLoading]="loading()" -[smallButton]="smallButtons()" -[hideLabelMobile]="hideLabelMobile()" +[buttonLoading]="loading$ | async" +[smallButton]="smallButtons$ | async" +[hideLabelMobile]="hideLabelMobile$ | async"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/main/resources/config/application-artemis.ymlis excluded by!**/*.ymlsrc/main/resources/config/application-localvc.ymlis excluded by!**/*.yml
📒 Files selected for processing (31)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/gitlab/GitlabInfoContributor.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCInfoContributor.java(3 hunks)src/main/webapp/app/detail-overview-list/components/programming-auxiliary-repository-buttons-detail/programming-auxiliary-repository-buttons-detail.component.html(1 hunks)src/main/webapp/app/detail-overview-list/components/programming-repository-buttons-detail/programming-repository-buttons-detail.component.html(1 hunks)src/main/webapp/app/exam/manage/exam-management.route.ts(3 hunks)src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.html(1 hunks)src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.ts(1 hunks)src/main/webapp/app/exercises/programming/manage/programming-exercise-management-routing.module.ts(2 hunks)src/main/webapp/app/exercises/programming/participate/programming-repository-routing.module.ts(3 hunks)src/main/webapp/app/exercises/programming/shared/code-editor/actions/code-editor-actions.component.ts(4 hunks)src/main/webapp/app/exercises/shared/dashboards/tutor/exercise-assessment-dashboard.component.html(1 hunks)src/main/webapp/app/exercises/shared/exercise-scores/exercise-scores.component.html(1 hunks)src/main/webapp/app/exercises/shared/participation/participation.component.html(1 hunks)src/main/webapp/app/localvc/commit-details-view/commit-details-view.component.ts(7 hunks)src/main/webapp/app/localvc/commit-history/commit-history.component.ts(4 hunks)src/main/webapp/app/localvc/repository-view/repository-view.component.html(1 hunks)src/main/webapp/app/localvc/repository-view/repository-view.component.ts(2 hunks)src/main/webapp/app/localvc/vcs-repository-access-log-view/vcs-repository-access-log-view.component.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(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/main/webapp/app/shared/layouts/profiles/profile-info.model.ts(1 hunks)src/main/webapp/app/shared/layouts/profiles/profile.service.ts(1 hunks)src/main/webapp/app/shared/user-settings/user-settings.route.ts(1 hunks)src/main/webapp/i18n/de/exercise-actions.json(0 hunks)src/main/webapp/i18n/en/exercise-actions.json(0 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCInfoContributorTest.java(2 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(9 hunks)
💤 Files with no reviewable changes (3)
- src/main/webapp/i18n/de/exercise-actions.json
- src/main/webapp/i18n/en/exercise-actions.json
- src/test/javascript/spec/component/overview/exercise-details/exercise-details-student-actions.component.spec.ts
✅ Files skipped from review due to trivial changes (2)
- src/main/webapp/app/detail-overview-list/components/programming-repository-buttons-detail/programming-repository-buttons-detail.component.html
- src/main/webapp/app/exercises/shared/dashboards/tutor/exercise-assessment-dashboard.component.html
🧰 Additional context used
📓 Path-based instructions (26)
src/main/webapp/app/exercises/programming/participate/programming-repository-routing.module.ts (1)
src/main/webapp/app/detail-overview-list/components/programming-auxiliary-repository-buttons-detail/programming-auxiliary-repository-buttons-detail.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/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.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/localvc/vcs-repository-access-log-view/vcs-repository-access-log-view.component.ts (1)
src/main/webapp/app/shared/user-settings/user-settings.route.ts (1)
src/main/webapp/app/shared/layouts/profiles/profile-info.model.ts (1)
src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.ts (1)
src/main/webapp/app/exercises/programming/manage/programming-exercise-management-routing.module.ts (1)
src/main/webapp/app/localvc/repository-view/repository-view.component.ts (1)
src/main/webapp/app/localvc/repository-view/repository-view.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/java/de/tum/cit/aet/artemis/programming/service/gitlab/GitlabInfoContributor.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/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
src/main/webapp/app/exercises/shared/exercise-scores/exercise-scores.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/shared/participation/participation.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/layouts/profiles/profile.service.ts (1)
src/main/webapp/app/exercises/programming/shared/code-editor/actions/code-editor-actions.component.ts (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCInfoContributor.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/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/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/localvc/commit-details-view/commit-details-view.component.ts (1)
src/main/webapp/app/exam/manage/exam-management.route.ts (1)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/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/main/webapp/app/localvc/commit-history/commit-history.component.ts (1)
📓 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 (7)
- GitHub Check: server-style
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
🔇 Additional comments (63)
src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.ts (1)
70-70: LGTM! Property follows Angular style guidelines.The
isInCourseManagementproperty follows camelCase naming convention and is properly typed as boolean.src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.html (3)
13-13: LGTM! Improved null handling.The nullish coalescing operator (
??) is more precise than the logical OR (||) for handling undefined/null repository URIs.
14-18: Verify route paths across the application.The conditional routing logic looks correct, but let's verify the route paths are consistent with the rest of the application:
✅ Verification successful
Routes are properly registered and consistent ✅
The conditional routing paths match their corresponding route registrations in the router configuration files. Both the course management and regular view routes follow the established patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar repository view routes echo "Checking repository view routes..." rg "repository.*USER.*participation" "src/main/webapp/app" # Check if these route patterns are registered echo "Checking route registration..." rg "path:.*programming-exercises.*repository.*USER" "src/main/webapp/app" rg "path:.*exercises.*repository" "src/main/webapp/app"Length of output: 1920
9-9: Verify heading hierarchy.The added h1 heading might disrupt the document outline as there's already an h5 heading above it. Consider adjusting the heading levels for proper document structure.
src/main/webapp/app/localvc/commit-details-view/commit-details-view.component.ts (7)
10-10: Approach to import GitDiffReportComponent looks good
This import aligns well with Angular's standalone component approach.
26-26: Validate potential undefined repositoryId usage
The optional property helps avoid runtime errors in some contexts, but please ensure all references handle cases whererepositoryIdisundefinedorNaN(e.g., parse failures).
36-36: Confirm no fallback for repositoryType
SettingrepositoryTypeto a mandatory string type is fine, but please verify that any missing route parameter won't cause an undefined state downstream. Currently you default to'USER'inngOnInit, which seems safe.
61-61: Gracefully handle missing or invalid repositoryId
If the route param is missing or invalid,Number(params['repositoryId'])may becomeNaN. A quick check or default would prevent potential errors.
63-63: Neat fallback for repositoryType
Using?? 'USER'is a concise way to supply a default. This promotes reliable behavior when the parameter is undefined.
78-79: Check for missing repositoryId path parameter
These lines rely onthis.repositoryIdbeing present ifrepositoryType === 'USER'. Verify that unexpected scenarios (no param) won’t break the logic.
115-115: Beware of non-existent commit hashes
Usingthis.previousCommit.hash!orthis.currentCommit.hash!could break if they are unexpectedlyundefined. Add safety checks or ensure the route always provides valid commits.src/main/webapp/app/shared/components/code-button/code-button.component.ts (27)
1-1: Use of new Angular 19+ imports is correct
Importinginput,signal, andeffectaligns with the new signals architecture. This modernizes state handling.
26-33: Router and new service imports are consistent with Angular style
These additional imports appear relevant to routing logic and the new signals-based approach.
34-38: Enum for distinct authentication states is a good practice
This strongly types the authentication approach and eliminates string-literal checks throughout the code.
60-60: Implements OnInit with new Angular features
Switching away fromOnChangesto purely reactive signals is cohesive with the newinput()pattern.
69-70: Injected AlertService and Router
Injecting the router and alert services looks appropriate for handling navigation and user feedback here.
75-84: Excellent usage of required reactive inputs
Usinginput.required<T>()ensures the properties are bound properly, preventing runtime errors when they must exist.
85-88: Flexible authentication array with fallback state
This approach is simpler than storing multiple booleans. Keep an eye on how the states are toggled to avoid confusion.
89-91: Token presence and validity checks recognized
Storing user token flags keeps logic straightforward, but confirm they remain updated if tokens change outside of this component.
95-98: LocalVC signals follow new Angular best practices
Using signals for dynamic toggles (localVC vs. GitLab) is a solid approach, especially with effect-based watchers.
104-105: Properly storing and retrieving profile info URLs
EnsuringsshSettingsUrlandvcsTokenSettingsUrlare assigned upon loading profile info fosters clarity.
161-163: Dynamically filtering authenticationMechanisms from profile
Good job mapping the server-provided list to your strongly typed enum.
167-168: Setting localVCEnabled and gitlabVCEnabled with signals
This pattern fosters reactive updates across components—clean approach.
183-184: SSH usage toggled correctly
Switching states by settingthis.currentState = States.SSHis straightforward. Remember to confirm user keys exist first.
189-198: Differentiating token usage for course-management vs. exam scenarios
This block’s logic is well-structured. Ensure tests cover both contexts (course vs. exam) to confirm correct token usage.
202-204: HTTPS password fallback is clean
The password approach is nice for users without tokens or SSH. Keep an eye on potential future deprecation.
209-210: Local storage usage is minimal and effective
Storing just the currentState is straightforward, limiting stored data.
218-218: Checking activeParticipation before falling back to repositoryUri
This method ensures a consistent source for the repository URL. No further suggestions.
221-233: Appropriate onClick state restoration
Retrieving the stored state from local storage is user-friendly. The subsequent calls effectively maintain the chosen clone method.
241-253: SSH URL transformation logic is robust
Regex-based replacement is correct for typical URL shapes. Ensure thorough testing if Git URLs deviate from expected patterns.
267-268: Documentation is concise
The docstring clearly states the logic for loading or creating new tokens.
290-316: Method getUsedToken is well-scoped
Returning either user or participation tokens is neatly segregated. TheisInCourseManagementcheck is effective.
339-340: SSH clone URL generation uses existing template
This is consistent with your approach to removing user credentials from SSH URLs.
369-369: Retrieving the IDE with fallback logic is beneficial
This block ensures a robust default for any unrecognized or unsupported programming language.
377-377: Practice mode switching is user-friendly
Switching between normal and practice modes on the fly covers typical exam/course transitions.
384-394: Getters for useToken, useSsh, and usePassword are clear
These computed props simplify template or function logic by clarifying which authentication approach is active.
411-422: Tooltip configuration updated for local VC scenario
The string replacement logic for tooltips is handy. Just ensurewindow.location.originbehaves as expected in non-browser contexts or ephemeral environments.
424-433: Headline generation method is well organized
This code elegantly addresses rated vs. practice repositories, benefiting clarity for the user.src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCInfoContributorTest.java (3)
5-9: Importing List and ReflectionTestUtils is appropriate
Bringing injava.util.ListandReflectionTestUtilssuits your test scenario for checking internal states.
18-21: Setting orderedAuthenticationMechanisms for the subject under test
Using reflection to configure private fields is fine for verifying this logic. Ensure consistent naming with the production code.
29-29: Asserting the new authenticationMechanisms key is correct
Replacing the old boolean-based checks with a list-based approach is consistent with the broader shift.src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCInfoContributor.java (1)
47-48: LGTM! Clean implementation of the authentication mechanism info.The implementation correctly exposes the authentication mechanisms to the frontend.
src/main/java/de/tum/cit/aet/artemis/programming/service/gitlab/GitlabInfoContributor.java (1)
33-34: LGTM! Consistent with LocalVCInfoContributor changes.The implementation maintains consistency with LocalVCInfoContributor while preserving the deprecation notice.
src/main/webapp/app/shared/user-settings/user-settings.route.ts (1)
78-81: LGTM! Good security enhancement.The addition of role-based access control and route protection for VCS token settings is a good security practice. The route is now properly restricted to admin, instructor, editor, and TA roles.
src/main/webapp/app/exercises/programming/manage/programming-exercise-management-routing.module.ts (2)
28-65: LGTM! Good refactoring for route organization.Extracting common repository routes into
repositorySubRoutesfollows DRY principles and improves maintainability. The routes are well-structured with proper access control and lazy loading.
185-189: LGTM! Consistent route configuration.The reuse of
repositorySubRoutesensures consistent configuration across different repository paths.src/main/webapp/app/localvc/commit-history/commit-history.component.ts (1)
55-60: LGTM! Clean and clear repository type handling.The changes improve the code by:
- Using nullish coalescing for default repository type
- Clear conditional logic for different repository types
src/main/webapp/app/exercises/programming/shared/code-editor/actions/code-editor-actions.component.ts (1)
7-7: LGTM! Clean import organization.The changes follow Angular's best practices for:
- Importing router-related modules
- Declaring imports in the component decorator
Also applies to: 28-28
src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.ts (1)
2-2: LGTM! Simplified routing dependencies.Good cleanup of unused Router service, keeping only the necessary RouterLink directive for template usage.
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
246-246: LGTM! The new constant improves authentication flexibility.The addition of
INFO_CODE_BUTTON_AUTHENTICATION_MECHANISMSsupports the shift from boolean flags to a list-based approach for authentication mechanisms.Let's verify the constant's usage across the codebase:
✅ Verification successful
✓ Constant is correctly implemented and used consistently
The
INFO_CODE_BUTTON_AUTHENTICATION_MECHANISMSconstant is properly used in both version control implementations (Gitlab and LocalVC) to expose supported authentication methods. The implementation:
- Maintains consistent behavior across VCS providers
- Includes proper test coverage
- Supports configurable authentication mechanisms with sensible defaults (password, token, ssh)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of the new constant and related authentication mechanisms rg "INFO_CODE_BUTTON_AUTHENTICATION_MECHANISMS" --type javaLength of output: 1011
Script:
#!/bin/bash # Check the implementation of InfoContributor classes ast-grep --pattern 'class $_ implements InfoContributor { $$$ contribute($_, $_) { $$$ } $$$ }'Length of output: 233
Script:
#!/bin/bash # Check the implementation details of the InfoContributor classes rg -A 10 "class GitlabInfoContributor|class LocalVCInfoContributor" --type java rg "orderedAuthenticationMechanisms" -B 2 -A 2 --type javaLength of output: 7727
src/test/javascript/spec/component/shared/code-button.component.spec.ts (4)
38-39: LGTM! Good test setup for authentication states.The addition of
localStorageStateandrouterproperly sets up the test environment for the new authentication mechanism.
55-55: LGTM! Comprehensive authentication mechanisms configuration.The
authenticationMechanismsarray in the mock profile info correctly reflects the supported authentication methods.
113-116: LGTM! Proper mocking of local storage.The implementation correctly mocks local storage for testing authentication state persistence.
Line range hint
222-250: LGTM! Thorough token handling test.The test case properly verifies:
- Token placeholder display for security
- Actual token usage in repository URLs
- Team participation scenarios
src/main/webapp/app/exam/manage/exam-management.route.ts (2)
268-269: LGTM! Improved repository route structure.The changes properly integrate
repositorySubRoutesfor both repository type and repository ID paths, maintaining consistent routing patterns.Also applies to: 272-273
578-579: LGTM! Consistent route structure maintained.The same repository routing pattern is correctly applied to exercise group routes, ensuring consistency across the application.
Also applies to: 581-582
src/main/webapp/app/detail-overview-list/components/programming-auxiliary-repository-buttons-detail/programming-auxiliary-repository-buttons-detail.component.html (1)
10-10: LGTM! Simplified repository route path.The removal of the redundant 'repo' segment from
routerLinkForRepositoryViewproperly aligns with the new routing structure while maintaining all necessary navigation information.src/main/webapp/app/localvc/repository-view/repository-view.component.html (1)
50-51: Verify the empty router link array behavior.The change from
'.'to[]forrouterLinkForRepositoryViewmight affect the navigation behavior. While this aligns with the PR's objective to fix routing, an empty array typically means staying on the current route. Please verify this is the intended behavior.Consider using a relative path array (e.g.,
['.']) if you want to explicitly indicate staying on the current route, as it makes the intention clearer.src/main/webapp/app/shared/components/code-button/code-button.component.html (2)
18-28: LGTM! Improved warning conditions.The updated conditional checks for warnings are more specific and handle edge cases better. The use of
Statesenum and additional checks forisInCourseManagementanduserTokenPresentimproves code clarity.
44-54: LGTM! Well-structured authentication mechanism handling.The iteration over
authenticationMechanismwith separate conditions for each type improves maintainability and makes it easier to add new authentication mechanisms in the future.src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.html (1)
130-133: Validate empty repositoryUri behavior.While the array syntax for
routerLinkForRepositoryViewis good, settingrepositoryUrito an empty string might cause issues if the component expects a valid URI.Please verify that:
- The component handles empty repository URIs gracefully
- This doesn't affect repository access functionality
src/main/webapp/app/exercises/shared/exercise-scores/exercise-scores.component.html (1)
321-321: LGTM! Improved routing with relative paths.The use of relative paths in
routerLinkForRepositoryViewimproves maintainability and makes the routing more flexible.src/main/webapp/app/exercises/shared/participation/participation.component.html (1)
123-132: Validate non-null assertion on repositoryUri.The addition of the null check for
getRepositoryLink(row, value)improves robustness. However, the non-null assertion (!) ongetRepositoryLink(row, value)assumes the function always returns a non-null value when the condition is true.Please verify that:
getRepositoryLink()always returns a non-null value in this context- Consider using optional chaining (
?.) instead if there's any possibility of null values
src/main/webapp/app/exercises/programming/participate/programming-repository-routing.module.ts
Show resolved
Hide resolved
src/main/webapp/app/localvc/repository-view/repository-view.component.ts
Show resolved
Hide resolved
SimonEntholzer
changed the base branch from
develop
to
feature/programming-exercises/code-button-configuration
…' into feature/programming-exercises/repository-view-routing
github-actions
bot
removed
server
krusche
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
…view-routing # Conflicts: # src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.html # src/main/webapp/app/exercises/shared/exercise-scores/exercise-scores.component.html # src/main/webapp/app/exercises/shared/participation/participation.component.html # src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.html
Checklist
General
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Changes affecting Programming Exercises
Motivation and Context
The routing to the repository view is broken in multiple places, and is very convoluted.
Description
Fixes the routing, also fixes #9843
Steps for Testing
Basically make sure, that in any place with a code button, the routing to the repository works correctly.
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
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
Refactor