Development: Migrate user settings to use standalone, inject and signals
#9930
Conversation
github-actions
bot
added
the
client
SimonEntholzer
changed the title
Development: Refactor user settings to use standalone and signalsDevelopment: Migrate user settings to use standalone, inject and signals
SimonEntholzer
requested review from
iyannsch,
meltemarsl,
BBesrour and
ole-ve
WalkthroughThis pull request introduces significant modernization to the user settings components in an Angular application. The changes primarily focus on adopting Angular's new signal-based reactivity, converting components to standalone mode, and simplifying dependency injection. Key modifications include updating state management, removing the Changes
Suggested labels
Suggested reviewers
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 5
🧹 Outside diff range and nitpick comments (14)
src/main/webapp/app/shared/user-settings/ssh-settings/ssh-user-settings.component.ts (1)
72-81: Optimize Signal Updates inrefreshSshKeysMethodThe
refreshSshKeysmethod callsthis.sshPublicKeys.set()twice, which can be optimized. By mapping overpublicKeysbefore setting them, you can update thesshPublicKeyssignal in a single operation and ensure that bothsshPublicKeysandsshUserSettingsService.sshKeyscontain the updated keys with thehasExpiredproperty.Suggested refactor:
private refreshSshKeys() { this.sshUserSettingsService .getSshPublicKeys() .pipe( tap((publicKeys: UserSshPublicKey[]) => { + const updatedKeys = publicKeys.map((key) => ({ + ...key, + hasExpired: key.expiryDate && dayjs().isAfter(dayjs(key.expiryDate)), + })); + this.sshPublicKeys.set(updatedKeys); + this.sshUserSettingsService.sshKeys = updatedKeys; this.keyCount.set(publicKeys.length); this.isLoading.set(false); }), ) .subscribe({ error: () => { this.isLoading.set(false); this.alertService.error('artemisApp.userSettings.sshSettingsPage.loadKeyFailure'); }, }); }src/main/webapp/app/shared/user-settings/science-settings/science-settings.component.ts (1)
Line range hint
53-55: Store Subscription to Feature Toggle for Proper CleanupThe subscription to
getFeatureToggleActiveis not assigned tofeatureToggleActiveSubscription, preventing it from being unsubscribed inngOnDestroy. Assign the subscription to ensure proper cleanup and prevent potential memory leaks.Suggested fix:
- this.featureToggleService.getFeatureToggleActive(FeatureToggle.Science).subscribe((active) => { + this.featureToggleActiveSubscription = this.featureToggleService.getFeatureToggleActive(FeatureToggle.Science).subscribe((active) => { this.featureToggleActive = active; });src/test/javascript/spec/component/account/account-information.component.spec.ts (1)
Test coverage needs improvement
The component has several methods that lack test coverage:
setUserImage()method for handling image uploaddeleteUserImage()for removing profile picturestriggerUserImageFileInput()for triggering file input- Error handling scenarios for profile picture operations
- Success scenarios for profile picture operations
Key areas to test:
- Image upload workflow with modal interaction
- Profile picture removal functionality
- Error handling for both upload and remove operations
- Success scenarios and state updates
- Interaction with UserSettingsService and AccountService
🔗 Analysis chain
Line range hint
1-58: LGTM! Test setup follows best practices.The test configuration properly uses mocks and follows the jest testing guidelines. The removal of TranslatePipeMock aligns with the move towards standalone components.
Let's verify test coverage for the component:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for AccountInformationComponent # Look for untested methods or signals in the component ast-grep --pattern 'class AccountInformationComponent { $$$ }'Length of output: 190
Script:
#!/bin/bash # Let's try a different approach to find the component file and its content fd "account-information.component.ts" --type f --exclude "*.spec.ts"Length of output: 162
Script:
#!/bin/bash # Search for any test files related to account information fd "account-information" --type f | grep -i "spec"Length of output: 129
Script:
#!/bin/bash # Use ripgrep to find test cases in the spec file rg "it\(" "src/test/javascript/spec/component/account/account-information.component.spec.ts" -A 3Length of output: 275
Script:
#!/bin/bash # Look for methods in the component that might need testing rg "^[[:space:]]*[a-zA-Z]+\(.*\).*{" "src/test/javascript/spec/component/account/account-information.component.spec.ts" -A 2Length of output: 250
Script:
#!/bin/bash # Let's examine the actual component file to see what methods need testing cat "src/main/webapp/app/shared/user-settings/account-information/account-information.component.ts"Length of output: 5205
Script:
#!/bin/bash # Fix the regex to look for methods in the spec file rg "^\s+\w+\s*\(" "src/test/javascript/spec/component/account/account-information.component.spec.ts" -A 2Length of output: 604
src/test/javascript/spec/component/account/user-settings-container.component.spec.ts (1)
51-51: Verify signal behavior in testsThe test assertions have been correctly updated to use method calls for signals (
localVCEnabled()). However, consider adding test cases for:
- Signal initialization
- Signal updates through component lifecycle
Add these test cases:
it('should initialize signals with default values', () => { expect(comp.localVCEnabled()).toBeFalse(); }); it('should update localVCEnabled signal when profile changes', () => { profileServiceMock.getProfileInfo.mockReturnValue(of({ activeProfiles: [] })); comp.ngOnInit(); expect(comp.localVCEnabled()).toBeFalse(); profileServiceMock.getProfileInfo.mockReturnValue(of({ activeProfiles: [PROFILE_LOCALVC] })); comp.ngOnInit(); expect(comp.localVCEnabled()).toBeTrue(); });Also applies to: 58-58
src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.html (1)
11-19: Consider extracting user profile section to a componentThe user profile section could be extracted into a separate component to improve reusability and maintainability.
Consider creating a new component:
@Component({ selector: 'jhi-user-profile-header', template: ` @if (user()) { <div> @if (user()?.name) { <span id="user-header">{{ user()?.name }}</span> } <br /> <span id="login">{{ user()?.login }}</span> </div> } `, standalone: true }) export class UserProfileHeaderComponent { @Input() user = input<User | null>(null); }src/main/webapp/app/shared/user-settings/user-settings.directive.ts (1)
Line range hint
1-100: Consider implementing OnDestroy to handle subscription cleanupThe directive contains subscriptions in the
loadSettingmethod but doesn't implementOnDestroyto clean them up, which could lead to memory leaks.Add the following implementation:
- export abstract class UserSettingsDirective implements OnInit { + export abstract class UserSettingsDirective implements OnInit, OnDestroy { + private destroy$ = new Subject<void>(); + + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + } protected loadSetting(): void { - this.userSettingsService.loadSettings(this.userSettingsCategory).subscribe({ + this.userSettingsService.loadSettings(this.userSettingsCategory) + .pipe(takeUntil(this.destroy$)) + .subscribe({src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.component.ts (1)
Line range hint
31-31: Consider initializing the signal with a more descriptive default valueThe signal initialization could be more explicit about its purpose.
- programmingLanguageToIde: WritableSignal<Map<ProgrammingLanguage, Ide>> = signal(new Map([[ProgrammingLanguage.EMPTY, this.PREDEFINED_IDE[0]]])); + programmingLanguageToIde: WritableSignal<Map<ProgrammingLanguage, Ide>> = signal( + new Map([[ProgrammingLanguage.EMPTY, this.PREDEFINED_IDE[0]]], // Default IDE for empty programming language + ));src/main/webapp/app/shared/user-settings/account-information/account-information.component.ts (2)
Line range hint
39-42: Implement proper subscription cleanupThe
authStateSubscriptionis not being cleaned up properly.Add the following cleanup:
- export class AccountInformationComponent implements OnInit { + export class AccountInformationComponent implements OnInit, OnDestroy { private authStateSubscription: Subscription; + ngOnDestroy(): void { + if (this.authStateSubscription) { + this.authStateSubscription.unsubscribe(); + } + }
Line range hint
82-91: Consider using a shared error handling serviceThe error handling logic is duplicated across multiple methods. Consider extracting it to a shared service.
Create a new service:
@Injectable({ providedIn: 'root' }) export class ErrorHandlingService { constructor(private alertService: AlertService) {} handleHttpError(error: HttpErrorResponse): void { const errorMessage = error.error?.title || error.headers?.get('x-artemisapp-alert'); if (errorMessage) { this.alertService.addAlert({ type: AlertType.DANGER, message: errorMessage, disableTranslation: true, }); } } }Then update the error handling methods to use this service:
- private onProfilePictureUploadError(error: HttpErrorResponse) { - const errorMessage = error.error ? error.error.title : error.headers?.get('x-artemisapp-alert'); - if (errorMessage) { - this.alertService.addAlert({ - type: AlertType.DANGER, - message: errorMessage, - disableTranslation: true, - }); - } - } + private onProfilePictureUploadError(error: HttpErrorResponse) { + this.errorHandlingService.handleHttpError(error); + }src/main/webapp/app/shared/user-settings/vcs-access-tokens-settings/vcs-access-tokens-settings.component.html (1)
36-38: Add aria-label for better accessibility.While the copy functionality is well implemented, the button could benefit from an accessibility improvement.
<button [cdkCopyToClipboard]="currentUser()!.vcsAccessToken || ''" (cdkCopyToClipboardCopied)="onCopyFinished($event)" [class.btn-success]="wasCopied()" class="btn btn-secondary btn-sm me-2" id="copy-token-button" + aria-label="Copy access token to clipboard" type="button" >src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.html (2)
118-121: Consider extracting complex conditional logic to component method.The nested conditional logic for button disable state is complex and could be hard to maintain. Consider moving this logic to the component.
-[disabled]="!(displayedSshKey() && ((selectedOption() === 'useExpiration' && displayedExpiryDate() && isExpiryDateValid()) || selectedOption() !== 'useExpiration'))" +[disabled]="!isFormValid()"Add to component:
isFormValid(): boolean { return this.displayedSshKey() && ((this.selectedOption() === 'useExpiration' && this.displayedExpiryDate() && this.isExpiryDateValid()) || this.selectedOption() !== 'useExpiration'); }
75-77: Consider adding a maximum date limit.While the minimum date is properly set to prevent past dates, consider adding a maximum date limit to prevent unreasonably far future dates.
[pickerType]="DateTimePickerType.CALENDAR" [shouldDisplayTimeZoneWarning]="false" [min]="currentDate()" +[max]="getMaxAllowedDate()"src/test/javascript/spec/component/account/ssh-user-settings-key-details.component.spec.ts (1)
130-161: Consider consolidating OS-specific test cases.The OS detection tests are well-written but could be more maintainable using a parameterized approach.
Consider refactoring to use a test data structure:
const testCases = [ { os: 'Windows', userAgent: 'Mozilla/5.0 (Windows NT 10.0; Win64; x64)', expected: 'cat ~/.ssh/id_ed25519.pub | clip' }, { os: 'MacOS', userAgent: 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7)', expected: 'pbcopy < ~/.ssh/id_ed25519.pub' }, // ... other cases ]; testCases.forEach(({ os, userAgent, expected }) => { it(`should detect ${os}`, () => { jest.spyOn(window.navigator, 'userAgent', 'get').mockReturnValue(userAgent); comp.ngOnInit(); expect(comp.copyInstructions()).toBe(expected); }); });src/test/javascript/spec/component/account/vcs-access-token-settings.component.spec.ts (1)
62-82: Consider separating concerns in token creation tests.The token creation tests mix multiple concerns. Consider splitting the validation and creation logic into separate test cases for better clarity.
Example structure:
describe('token creation', () => { describe('validation', () => { it('should validate expiry date', () => { // date validation tests }); }); describe('creation', () => { it('should create token with valid date', () => { // token creation tests }); }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (23)
src/main/webapp/app/account/settings/settings.component.ts(1 hunks)src/main/webapp/app/app-routing.module.ts(1 hunks)src/main/webapp/app/app.module.ts(0 hunks)src/main/webapp/app/shared/user-settings/account-information/account-information.component.ts(1 hunks)src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.component.ts(1 hunks)src/main/webapp/app/shared/user-settings/notification-settings/notification-settings.component.ts(2 hunks)src/main/webapp/app/shared/user-settings/science-settings/science-settings.component.ts(1 hunks)src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.html(5 hunks)src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.ts(4 hunks)src/main/webapp/app/shared/user-settings/ssh-settings/ssh-user-settings.component.html(3 hunks)src/main/webapp/app/shared/user-settings/ssh-settings/ssh-user-settings.component.ts(4 hunks)src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.html(2 hunks)src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts(2 hunks)src/main/webapp/app/shared/user-settings/user-settings.directive.ts(2 hunks)src/main/webapp/app/shared/user-settings/user-settings.module.ts(0 hunks)src/main/webapp/app/shared/user-settings/user-settings.route.ts(1 hunks)src/main/webapp/app/shared/user-settings/vcs-access-tokens-settings/vcs-access-tokens-settings.component.html(4 hunks)src/main/webapp/app/shared/user-settings/vcs-access-tokens-settings/vcs-access-tokens-settings.component.ts(6 hunks)src/test/javascript/spec/component/account/account-information.component.spec.ts(1 hunks)src/test/javascript/spec/component/account/ssh-user-settings-key-details.component.spec.ts(4 hunks)src/test/javascript/spec/component/account/ssh-user-settings.component.spec.ts(4 hunks)src/test/javascript/spec/component/account/user-settings-container.component.spec.ts(1 hunks)src/test/javascript/spec/component/account/vcs-access-token-settings.component.spec.ts(7 hunks)
💤 Files with no reviewable changes (2)
- src/main/webapp/app/app.module.ts
- src/main/webapp/app/shared/user-settings/user-settings.module.ts
🧰 Additional context used
📓 Path-based instructions (21)
src/test/javascript/spec/component/account/account-information.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/app-routing.module.ts (1)
src/main/webapp/app/shared/user-settings/user-settings.route.ts (1)
src/main/webapp/app/account/settings/settings.component.ts (1)
src/test/javascript/spec/component/account/user-settings-container.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/user-settings/ssh-settings/details/ssh-user-settings-key-details.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/user-settings/user-settings-container/user-settings-container.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/user-settings/ssh-settings/ssh-user-settings.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/user-settings/account-information/account-information.component.ts (1)
src/test/javascript/spec/component/account/ssh-user-settings-key-details.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/user-settings/vcs-access-tokens-settings/vcs-access-tokens-settings.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/test/javascript/spec/component/account/ssh-user-settings.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/user-settings/notification-settings/notification-settings.component.ts (1)
src/main/webapp/app/shared/user-settings/user-settings.directive.ts (1)
src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts (1)
src/test/javascript/spec/component/account/vcs-access-token-settings.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/user-settings/ssh-settings/ssh-user-settings.component.ts (1)
src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.component.ts (1)
src/main/webapp/app/shared/user-settings/vcs-access-tokens-settings/vcs-access-tokens-settings.component.ts (1)
src/main/webapp/app/shared/user-settings/science-settings/science-settings.component.ts (1)
src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.ts (1)
📓 Learnings (6)
src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.html (1)
Learnt from: SimonEntholzer
PR: ls1intum/Artemis#9478
File: src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.ts:104-111
Timestamp: 2024-11-12T12:51:46.554Z
Learning: In `ssh-user-settings-key-details.component.ts`, changing the error handling code by adding optional chaining (`?.`) and replacing `.indexOf()` with `.includes()` may alter semantics and should be avoided.
src/main/webapp/app/shared/user-settings/ssh-settings/ssh-user-settings.component.html (2)
Learnt from: SimonEntholzer
PR: ls1intum/Artemis#9478
File: src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.ts:104-111
Timestamp: 2024-11-12T12:51:46.554Z
Learning: In `ssh-user-settings-key-details.component.ts`, changing the error handling code by adding optional chaining (`?.`) and replacing `.indexOf()` with `.includes()` may alter semantics and should be avoided.
Learnt from: Hialus
PR: ls1intum/Artemis#9454
File: src/main/webapp/app/shared/user-settings/ssh-settings/ssh-user-settings.component.html:114-119
Timestamp: 2024-11-12T12:51:58.050Z
Learning: In `src/main/webapp/app/shared/user-settings/ssh-settings/ssh-user-settings.component.html`, when providing commands for copying SSH public keys, always include instructions for macOS and Windows users in addition to Linux.
src/main/webapp/app/shared/user-settings/user-settings.directive.ts (1)
Learnt from: SimonEntholzer
PR: ls1intum/Artemis#9397
File: src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts:1-1
Timestamp: 2024-11-12T12:51:46.554Z
Learning: In this project, refrain from suggesting the replacement of the `inject()` function with constructor injection in components.
src/main/webapp/app/shared/user-settings/ssh-settings/ssh-user-settings.component.ts (1)
Learnt from: SimonEntholzer
PR: ls1intum/Artemis#9478
File: src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.ts:104-111
Timestamp: 2024-11-12T12:51:46.554Z
Learning: In `ssh-user-settings-key-details.component.ts`, changing the error handling code by adding optional chaining (`?.`) and replacing `.indexOf()` with `.includes()` may alter semantics and should be avoided.
src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.component.ts (1)
Learnt from: janthoXO
PR: ls1intum/Artemis#9201
File: src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.component.html:12-23
Timestamp: 2024-11-12T12:51:40.391Z
Learning: In Angular components, using the OnPush change detection strategy can mitigate performance issues related to method calls in templates, as it reduces unnecessary checks by only updating the view when specific conditions are met.
src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.ts (1)
Learnt from: SimonEntholzer
PR: ls1intum/Artemis#9478
File: src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.ts:104-111
Timestamp: 2024-11-12T12:51:46.554Z
Learning: In `ssh-user-settings-key-details.component.ts`, changing the error handling code by adding optional chaining (`?.`) and replacing `.indexOf()` with `.includes()` may alter semantics and should be avoided.
🔇 Additional comments (20)
src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.ts (2)
Line range hint 94-106: Consistent error handling by avoiding change to .includes()
In the error handling code, the use of .indexOf(errorKey) > -1 is consistent with previous practices and avoids potential semantic changes that may arise from using .includes(). Based on prior learnings, changing from .indexOf() to .includes() can alter the behavior if errorKey is undefined or null. The current implementation is correct and should be maintained as is.
125-125: Ensure optional chaining does not alter error handling semantics
Using optional chaining (?.) in this.displayedExpiryDate()?.isValid() may bypass error handling if displayedExpiryDate is undefined or null. According to previous learnings, altering error-handling code with optional chaining can change semantics and should be carefully reviewed to ensure it doesn't introduce unintended behavior.
Run the following script to verify that displayedExpiryDate is handled correctly throughout the codebase:
src/main/webapp/app/shared/user-settings/notification-settings/notification-settings.component.ts (1)
32-33: Visibility Change Enhances Encapsulation
Changing the visibility of faSave and faInfoCircle from public to protected improves encapsulation and aligns with Angular best practices by restricting access to these properties within the component and its subclasses.
src/main/webapp/app/shared/user-settings/vcs-access-tokens-settings/vcs-access-tokens-settings.component.ts (1)
35-50: Proper Use of Signals for Reactive State Management
The conversion of properties to signals (currentUser, expiryDate, validExpiryDate, wasCopied, and edit) enhances the component's reactivity and aligns with modern Angular practices. Accessing and setting signal values are correctly implemented.
src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts (1)
26-31: Effective Use of Signals and Improved Property Visibility
Introducing signals for currentUser, localVCEnabled, and isAtLeastTutor improves reactivity. Changing the visibility of faUser to protected enhances encapsulation. The component adheres to Angular best practices.
src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.html (2)
6-11: LGTM! Proper use of new Angular control flow syntax
The template correctly uses @if instead of *ngIf and properly handles null checks for the user image.
Line range hint 32-41: LGTM! Proper permission checks
The template correctly uses signals for permission checks (localVCEnabled() and isAtLeastTutor()).
src/main/webapp/app/shared/user-settings/user-settings.route.ts (1)
13-13: Verify route integration with parent module
The empty path ('') and renamed export to UserSettingsRoutes could affect route resolution depending on how this routes file is imported in the parent module.
Let's verify the route integration:
Also applies to: 15-15
✅ Verification successful
Let me check how these routes are integrated in the parent module.
Let me check the actual user settings route implementation to complete the verification.
Route integration is properly configured and safe
The route integration is correctly implemented:
- The
UserSettingsRoutesis properly loaded inapp-routing.module.tsunder the 'user-settings' path with correct authority checks - The empty path in
user-settings.route.tsis intentional as it:- Serves as the container component route
- Has a child route that redirects empty paths to 'account'
- No route path conflicts exist as the empty paths are properly scoped within their respective modules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how these routes are imported and used in the parent module
# Find route imports in the app
rg -l "UserSettingsRoutes|userSettingsState" --type ts
# Check for potential route path conflicts
rg "path: ''" --type ts
Length of output: 4716
Script:
#!/bin/bash
# Check the integration in app-routing.module.ts
cat src/main/webapp/app/app-routing.module.ts
Length of output: 10708
Script:
#!/bin/bash
# Check the user settings route implementation
cat src/main/webapp/app/shared/user-settings/user-settings.route.ts
Length of output: 3732
src/test/javascript/spec/component/account/ssh-user-settings.component.spec.ts (3)
66-66: LGTM: Proper signal usage in tests
The test cases correctly reflect the transition to signals by using method calls (e.g., keyCount(), sshPublicKeys()) instead of property access.
Also applies to: 73-75
89-89: LGTM: Error handling test updated for signals
The error handling test case properly validates the signal's default state.
49-49: Verify translation functionality
The removal of declarations array might affect translation handling in tests.
Let's verify the translation setup:
✅ Verification successful
Let me gather more information about the test file and its dependencies.
Translation functionality is properly configured
The test file correctly handles translations by:
- Importing and using MockTranslateService from the mocks directory
- Providing TranslateService with MockTranslateService in the TestBed configuration
- Setting up the translation language in beforeEach
The empty declarations array is intentional as the component's dependencies are handled through imports and providers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check translation setup in other test files
# Find other test files using TranslatePipeMock
rg -l "TranslatePipeMock" --type ts
# Check translation-related imports in component
ast-grep --pattern 'import { $_ } from "@ngx-translate/core"'
Length of output: 5417
Script:
#!/bin/bash
# Check the content of the specific test file
cat src/test/javascript/spec/component/account/ssh-user-settings.component.spec.ts
# Check for TranslateModule or TranslatePipe in the test file
rg "TranslateModule|TranslatePipe" src/test/javascript/spec/component/account/ssh-user-settings.component.spec.ts
# Check the mock translation service usage
rg "MockTranslateService|TranslatePipeMock" src/test/javascript/spec/component/account/ssh-user-settings.component.spec.ts
Length of output: 4337
src/main/webapp/app/shared/user-settings/user-settings.directive.ts (1)
14-16: LGTM! Clean dependency injection using inject() function
The transition to using the inject() function for dependency injection aligns with modern Angular practices.
src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.component.ts (1)
17-20: LGTM! Well-structured standalone component configuration
The component is properly configured as standalone with appropriate imports and OnPush change detection strategy.
src/main/webapp/app/shared/user-settings/account-information/account-information.component.ts (1)
19-22: LGTM! Proper standalone component setup
The component is correctly configured as standalone with necessary imports.
src/main/webapp/app/shared/user-settings/ssh-settings/ssh-user-settings.component.html (2)
4-7: LGTM! Proper usage of new Angular control flow syntax.
The code correctly uses the new @if syntax and appropriately handles the loading and empty states.
46-46: LGTM! Proper implementation of list iteration.
The code correctly uses the new @for syntax with proper tracking and index declaration.
src/main/webapp/app/shared/user-settings/vcs-access-tokens-settings/vcs-access-tokens-settings.component.html (1)
87-87: LGTM! Proper usage of new Angular control flow syntax.
The code correctly uses the new @if syntax for edit mode handling.
src/test/javascript/spec/component/account/ssh-user-settings-key-details.component.spec.ts (1)
80-85: LGTM! Well-structured transition to signals.
The changes correctly reflect the transition from properties to signals, with proper usage of the set method for signal updates and method calls for state access.
src/test/javascript/spec/component/account/vcs-access-token-settings.component.spec.ts (1)
141-144: Verify error handling completeness.
The error handling test could be more comprehensive. Consider testing different error scenarios and their respective user feedback.
src/main/webapp/app/app-routing.module.ts (1)
106-113: LGTM! Well-structured route configuration.
The new user settings route is properly configured with:
- Lazy loading for better performance
- Correct authority check for security
- Clear organization within the routing structure
...app/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/user-settings/ssh-settings/ssh-user-settings.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/user-settings/science-settings/science-settings.component.ts
Show resolved
Hide resolved
...p/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/test/javascript/spec/component/shared/user-settings/user-settings.directive.spec.ts (1)
Line range hint
89-148: Consider enhancing test organization and readability.While the test coverage is comprehensive, consider these improvements:
- Group related test setup in shared
beforeEachblocks- Extract common test data to constants
- Use descriptive test case names that follow the given/when/then pattern
Example refactor:
describe('Service methods with Category Notification Settings', () => { const notificationSettingsForTesting: NotificationSetting[] = [notificationSettingA, notificationSettingB]; + + // Common test setup + let loadSettingsSuccessSpy: jest.SpyInstance; + let extractSettingsSpy: jest.SpyInstance; + + beforeEach(() => { + loadSettingsSuccessSpy = jest.spyOn(userSettingsService, 'loadSettingsSuccessAsSettingsStructure'); + extractSettingsSpy = jest.spyOn(userSettingsService, 'extractIndividualSettingsFromSettingsStructure'); + }); describe('test loadSettings', () => { - it('should call userSettingsService to load Settings', () => { + it('given valid settings, when initialized, then loads and processes settings', () => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/javascript/spec/component/shared/user-settings/user-settings.directive.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/component/shared/user-settings/user-settings.directive.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/user-settings/user-settings.directive.spec.ts (1)
32-33: LGTM! Constructor changes align with modern Angular practices.
The simplified constructor correctly reflects the migration to using Angular's inject() function in the base class.
Let's verify the base class implementation:
✅ Verification successful
Constructor changes correctly reflect base class migration to inject()
The base UserSettingsDirective class has been properly refactored to use Angular's inject() function for all dependencies:
userSettingsService = inject(UserSettingsService)alertService = inject(AlertService)changeDetector = inject(ChangeDetectorRef)
Therefore, the simplified mock component constructor is correct and aligns with the base implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the base UserSettingsDirective uses inject() instead of constructor injection
# Search for the base directive implementation
ast-grep --pattern 'class UserSettingsDirective {
$$$
userSettingsService = inject($$$);
$$$
}'
Length of output: 227
Script:
#!/bin/bash
# Let's try a broader search to find the UserSettingsDirective implementation
# First, find the file containing UserSettingsDirective
fd "user-settings.directive.ts" --type f --exclude "*.spec.ts"
# Then let's examine its content
rg "class UserSettingsDirective" -A 10
Length of output: 1323
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/test/javascript/spec/component/settings/ide/ide-settings.component.spec.ts (2)
Line range hint
52-57: Enhance test assertions specificity.While the test assertions are good, they could be more specific:
- Use
toContainEntries()for Map comparisons instead oftoEqual()- Use
toBeTrue()instead oftoEqual(true)- expect(component.programmingLanguageToIde()).toEqual(loadedIdePreferences); + expect(component.programmingLanguageToIde()).toContainEntries([...loadedIdePreferences.entries()]);
Line range hint
33-108: Consider adding tests for error scenarios.The test suite would benefit from additional coverage for:
- Error handling when service calls fail
- Validation of empty or invalid inputs
- Edge cases in IDE preference management
Example scenarios to consider:
- Service throwing errors
- Invalid programming language inputs
- Network failures during save/load operations
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/javascript/spec/component/settings/ide/ide-settings.component.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/component/settings/ide/ide-settings.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/settings/ide/ide-settings.component.spec.ts (1)
9-9: LGTM! Good practice using a dedicated mock for translations.
The addition of TranslateTestingModule from a dedicated mocks directory follows good testing practices by properly isolating translation dependencies.
src/test/javascript/spec/component/settings/ide/ide-settings.component.spec.ts
Show resolved
Hide resolved
sachmii
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
SimonEntholzer
dismissed stale reviews from ole-ve, iyannsch, sachmii, coderabbitai[bot], and HawKhiem
via
cd4dbc9
SimonEntholzer
removed
the
deployment-error
sachmii
added
deploy:artemis-test3
deploy:artemis-test4
and removed
deploy:artemis-test3
labels
|
github-actions
bot
added
the
deployment-error
sachmii
added
deploy:artemis-test1
and removed
deployment-error
sachmii
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Client
Motivation and Context
We want to migrate all our components to standalone, to use inject for services, and to use signals.
Description
Migrates the user settings components to the new client guidelines.
Steps for Testing
Go to the user settings and check everything works as before. (See screenshots)
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
Release Notes
New Features
Improvements
Bug Fixes
Chores
UserSettingsModuleto streamline component management.