Programming exercises: Fix code button showing HTTPS link when it should be disabled
#9696
Conversation
github-actions
bot
added
the
client
SimonEntholzer
changed the title
Integraed code lifecycle: Do not show HTTPS link when it is disabled
SimonEntholzer
changed the title
WalkthroughThe pull request updates the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/main/webapp/app/shared/components/code-button/code-button.component.ts (3)
82-97: LGTM with a suggestion for error handling.The refactoring to async/await improves readability and fixes the race condition by ensuring user data is loaded before access. However, consider adding error handling for the identity retrieval.
Consider adding try-catch block:
async ngOnInit() { + try { this.user = (await this.accountService.identity())!; this.refreshTokenState(); + } catch (error) { + console.error('Failed to load user identity:', error); + // Handle error appropriately + }🧰 Tools
🪛 Biome
[error] 89-89: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 90-90: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
89-90: Improve readability of localStorage observers.The assignments within expressions can be confusing. Consider using more explicit arrow function bodies.
- this.localStorage.observe('useSsh').subscribe((useSsh) => (this.useSsh = useSsh || false)); - this.localStorage.observe('useToken').subscribe((useToken) => (this.useToken = useToken || false)); + this.localStorage.observe('useSsh').subscribe((useSsh) => { + this.useSsh = useSsh || false; + }); + this.localStorage.observe('useToken').subscribe((useToken) => { + this.useToken = useToken || false; + });🧰 Tools
🪛 Biome
[error] 89-89: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 90-90: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Line range hint
282-289: Improve error handling in createNewVcsAccessToken.The empty error handler could miss important failure scenarios. Consider logging the error and potentially notifying the user.
createNewVcsAccessToken(participation: ProgrammingExerciseStudentParticipation) { this.accountService.createVcsAccessToken(participation!.id!).subscribe({ next: (res: HttpResponse<string>) => { if (res.body) { participation.vcsAccessToken = res.body; if (this.activeParticipation?.id == participation.id) { this.user.vcsAccessToken = res.body; } } }, - error: () => {}, + error: (error) => { + console.error('Failed to create VCS access token:', error); + // Consider showing a user-friendly error message + }, }); }🧰 Tools
🪛 Biome
[error] 89-89: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 90-90: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/webapp/app/shared/components/code-button/code-button.component.ts(1 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] 89-89: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 90-90: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (1)
src/main/webapp/app/shared/components/code-button/code-button.component.ts (1)
82-97: Verify the fix for disabled HTTPS links.
The changes appear to address the race condition by ensuring user data is loaded before access. Let's verify the fix works as expected.
Also applies to: 282-289
✅ Verification successful
Let me gather more context about the code button component and its interaction with the showCloneUrlWithoutToken flag.
Based on the gathered information, I can now generate the final response:
Changes correctly handle HTTPS link visibility and user data loading
The implementation properly handles the HTTPS link visibility through:
- Profile service correctly sets
showCloneUrlWithoutTokenfrom server configuration - Component template conditionally renders HTTPS option based on
showCloneUrlWithoutTokenflag - User data loading is properly sequenced with
await accountService.identity() - Token state is correctly synchronized with local storage and profile settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the handling of show-clone-url-without-token flag
# and related user data loading
# Check for any other instances where showCloneUrlWithoutToken might be used
rg -l "showCloneUrlWithoutToken"
# Check for any timing-related issues in user data loading
ast-grep --pattern 'accountService.identity()'
Length of output: 1179
Script:
#!/bin/bash
# Check the implementation of code-button component
rg -A 5 "showCloneUrlWithoutToken" src/main/webapp/app/shared/components/code-button/code-button.component.ts
# Check the template usage
cat src/main/webapp/app/shared/components/code-button/code-button.component.html
# Check profile service implementation
rg -A 5 "showCloneUrlWithoutToken" src/main/webapp/app/shared/layouts/profiles/profile.service.ts
Length of output: 7798
🧰 Tools
🪛 Biome
[error] 89-89: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 90-90: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
SimonEntholzer
changed the title
Programming exercises: Fix code button showing HTTPS link when they are disabled
SimonEntholzer
changed the title
Programming exercises: Fix code button showing HTTPS link when they are disabledProgramming exercises: Fix code button showing HTTPS link when it should be disabled
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
SimonEntholzer
dismissed stale reviews from dfuchss and coderabbitai[bot]
via
e7ada88
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/webapp/app/shared/components/code-button/code-button.component.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/shared/components/code-button/code-button.component.ts (1)
🔇 Additional comments (2)
src/main/webapp/app/shared/components/code-button/code-button.component.ts (2)
82-88: LGTM! Good improvement in error handling.
The conversion to async/await and addition of early return for null user case improves error handling and prevents potential null reference issues.
91-106: 🛠️ Refactor suggestion
Review state initialization order to prevent inconsistencies.
The current initialization sequence could lead to state inconsistencies:
useTokenis initialized from localStorage- A subscription to
useTokenchanges is set up - Later,
useTokenmight be overwritten based onshowCloneUrlWithoutToken
This could trigger unnecessary state updates or leave the component in an inconsistent state.
Let's verify the potential state inconsistency:
Consider reorganizing the initialization:
async ngOnInit() {
const user = await this.accountService.identity();
if (!user) {
return;
}
this.user = user;
this.refreshTokenState();
+ await this.initializeProfileSettings();
this.copyEnabled = true;
this.useSsh = this.localStorage.retrieve('useSsh') || false;
- this.useToken = this.localStorage.retrieve('useToken') || false;
+ // Only initialize useToken from localStorage if showCloneUrlWithoutToken allows it
+ const storedUseToken = this.localStorage.retrieve('useToken') || false;
+ this.useToken = this.showCloneUrlWithoutToken ? storedUseToken : true;
// ... rest of the initialization
}
+private async initializeProfileSettings(): Promise<void> {
+ const profileInfo = await firstValueFrom(this.profileService.getProfileInfo());
+ this.showCloneUrlWithoutToken = profileInfo.showCloneUrlWithoutToken ?? true;
+ // ... other profile settings
+}
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
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/test/javascript/spec/component/shared/code-button.component.spec.ts (2)
140-149: Consider adding more specific expectations.The async initialization test looks good, but could be enhanced with additional assertions to verify the component's state more thoroughly.
Consider adding these expectations:
expect(component.accessTokensEnabled).toBeTrue(); expect(component.useParticipationVcsAccessToken).toBeFalse(); expect(component.user).toBeDefined();
151-179: Consider adding error case verification.The VCS token handling tests look good, but could be enhanced with error case verification.
Consider adding a test case for other error scenarios:
it('should handle other errors when getting vcsAccessToken', async () => { getVcsAccessTokenSpy = jest.spyOn(accountService, 'getVcsAccessToken') .mockReturnValue(throwError(() => new HttpErrorResponse({ status: 500, statusText: 'Server Error' }))); stubServices(); participation.id = 1; component.useParticipationVcsAccessToken = true; component.participations = [participation]; await component.ngOnInit(); component.ngOnChanges(); expect(component.accessTokensEnabled).toBeTrue(); expect(component.user.vcsAccessToken).toBeUndefined(); expect(getVcsAccessTokenSpy).toHaveBeenCalled(); expect(createVcsAccessTokenSpy).not.toHaveBeenCalled(); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/javascript/spec/component/shared/code-button.component.spec.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (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}}
🔇 Additional comments (1)
src/test/javascript/spec/component/shared/code-button.component.spec.ts (1)
22-22: LGTM!
The addition of BehaviorSubject import is appropriate for mocking the ProfileService response.
SimonEntholzer
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
show-clone-url-without-tokenvariable set. Only tested usual code-button functionality on test server.Client
Motivation and Context
With the
show-clone-url-without-tokenvariable, showing of the HTTPs clone link can be disabled. This feature seems to have broken recently, as initially it is shown now.Description
This is cause by a race condition, where the user is not yet loaded, but accessed. Explicitly awaiting the user solves the issue.
Steps for Testing
show-clone-url-without-tokenset tofalseTokenandSSH.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
Client
Screenshots
Before the bugfix:
After the fix:
Summary by CodeRabbit
Bug Fixes
Improvements