Fix/8122 8127 profile page telegram sub #3897
Conversation
WalkthroughReworks the switcher to bind via a getter and handle full change events; updates profile template buttons and switch bindings; adds savedTelegramIsNotify and complex submit/cancel/switch logic to persist or roll back Telegram notification state; and expands unit tests to cover telegram flows and switcher behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Switch as Switch (Telegram)
participant Page as UbsUserProfilePageComponent
participant Dialog as Confirm Dialog
participant Service as ClientProfileService
participant Nav as Telegram URL
User->>Switch: Toggle
Switch-->>Page: onSwitchChanged(newValue) /* full event forwarded */
Page->>Page: Set form.control telegramIsNotify = newValue (optimistic)
Page->>Dialog: Ask user to confirm change
alt Confirmed
Page->>Service: saveToServer({ telegramIsNotify: newValue })
alt Success
Service-->>Page: Updated profile
Page->>Page: Patch form, set savedTelegramIsNotify = newValue, mark pristine
opt Enabling requires bot
Page->>Nav: Open Telegram bot URL
end
else Error
Service-->>Page: Error
Page->>Page: Rollback form.control telegramIsNotify to savedTelegramIsNotify
end
else Cancelled
Page->>Page: Rollback form.control telegramIsNotify to savedTelegramIsNotify
end
sequenceDiagram
autonumber
actor User
participant Page as UbsUserProfilePageComponent
participant Service as ClientProfileService
participant Snackbar as Snackbar
User->>Page: Click Save (button)
Page->>Page: Validate form, compute payload, compare telegramIsNotify with savedTelegramIsNotify
alt Relevant changes present
Page->>Service: postDataClientProfile(normalized payload)
Service-->>Page: Updated profile
Page->>Page: Patch form with server data, mark pristine
Page->>Snackbar: Show "saved" message
else No relevant changes
Page->>Page: No-op
end
User->>Page: Click Cancel (button)
Page->>Page: Revert telegramIsNotify to savedTelegramIsNotify
Page->>Service: Reload user data
Service-->>Page: Server profile
Page->>Page: Reset form with server values
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/app/ubs/ubs-user/components/ubs-user-profile-page/ubs-switcher/ubs-switcher.component.spec.ts (4)
42-89: Tests cover the core onChange behavior correctly.The tests verify that:
- The method emits the new checked state via
switchChanged- The checkbox is reset to match
isChecked(controlled component pattern)However, note that the test organization is inconsistent—some tests verify single aspects (lines 43-65, 67-76) while line 78-89 verifies both behaviors together. Consider consolidating or ensuring each test has a clear single responsibility.
91-98: Consider removing this redundant test.This test only verifies that
onChangedoesn't throw, which is already implicitly covered by all the otheronChangetests. It doesn't add meaningful value.Apply this diff to remove the test:
- it('should cast event.target to HTMLInputElement', () => { - component.isChecked = false; - - const mockCheckbox = { checked: true } as HTMLInputElement; - const mockEvent = { target: mockCheckbox } as unknown as Event; - - expect(() => component.onChange(mockEvent)).not.toThrow(); - });
101-111: Consider removing or enhancing these shallow property tests.These tests only verify that you can assign and read back property values, which TypeScript already guarantees at compile time. Consider:
- Removing them entirely if they don't test meaningful behavior
- Or enhancing them to test actual behavior, such as how changes to these properties affect the component's rendered state or interactions
113-118: Consider removing this basic type-checking test.This test only verifies that
switchChangedis defined and is anEventEmitter, which TypeScript already guarantees. The emission behavior is already thoroughly tested in theonChangemethod tests (lines 42-89), so this test adds minimal value.If you choose to keep it, consider simplifying to just:
describe('Output properties', () => { it('should have switchChanged EventEmitter', () => { expect(component.switchChanged).toBeDefined(); - expect(component.switchChanged instanceof EventEmitter).toBe(true); }); });src/app/ubs/ubs-user/components/ubs-user-profile-page/ubs-user-profile-page.component.spec.ts (1)
884-965: Comprehensive test coverage for switch behavior.The tests thoroughly cover the telegram notification switch scenarios: enabling with confirmation, canceling, and disabling. The test setup properly initializes
savedTelegramIsNotify,userProfile, and form state.Optional: Consider extracting the repeated
userProfilesetup (lines 889-899, 917-927, 943-953) into a helper function orbeforeEachblock to reduce duplication:function createTestUserProfile(telegramIsNotify: boolean): UserProfile { return { recipientName: 'Test', recipientSurname: 'User', recipientEmail: 'test@example.com', recipientPhone: '+380991234567', alternateEmail: '', addressDto: [], telegramIsNotify, hasPassword: true, botList: [{ link: 'https://t.me/testbot', type: 'telegram' }] }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/ubs/ubs-user/components/ubs-user-profile-page/ubs-switcher/ubs-switcher.component.spec.ts(2 hunks)src/app/ubs/ubs-user/components/ubs-user-profile-page/ubs-user-profile-page.component.spec.ts(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (18.x)
🔇 Additional comments (3)
src/app/ubs/ubs-user/components/ubs-user-profile-page/ubs-switcher/ubs-switcher.component.spec.ts (1)
25-40: LGTM! displayChecked getter tests are well-structured.The tests comprehensively cover all cases for the getter (true, false, and undefined), and the assertions are correct.
src/app/ubs/ubs-user/components/ubs-user-profile-page/ubs-user-profile-page.component.spec.ts (2)
849-862: LGTM! Good edge case coverage.The tests properly verify
setUrlToBotbehavior with both populated and empty bot lists.
486-487: Test setup properly reflects telegram notification integration.The tests correctly initialize
telegramIsNotifyform control, setsavedTelegramIsNotify, mark form as dirty, and configure service mocks for the submit flow.Also applies to: 497-497, 501-501, 503-503, 507-507, 522-539
| describe('onChange method', () => { | ||
| it('should emit true when checkbox is checked', () => { | ||
| component.isChecked = false; | ||
| spyOn(component.switchChanged, 'emit'); | ||
|
|
||
| const mockCheckbox = { checked: true } as HTMLInputElement; | ||
| const mockEvent = { target: mockCheckbox } as unknown as Event; | ||
|
|
||
| component.onChange(mockEvent); | ||
|
|
||
| expect(component.switchChanged.emit).toHaveBeenCalledWith(true); | ||
| }); | ||
|
|
||
| it('should emit false when checkbox is unchecked', () => { | ||
| component.isChecked = true; | ||
| spyOn(component.switchChanged, 'emit'); | ||
|
|
||
| const mockCheckbox = { checked: false } as HTMLInputElement; | ||
| const mockEvent = { target: mockCheckbox } as unknown as Event; | ||
|
|
||
| component.onChange(mockEvent); | ||
|
|
||
| expect(component.switchChanged.emit).toHaveBeenCalledWith(false); | ||
| }); | ||
|
|
||
| it('should reset checkbox.checked to isChecked value', () => { | ||
| component.isChecked = true; | ||
|
|
||
| const mockCheckbox = { checked: false } as HTMLInputElement; | ||
| const mockEvent = { target: mockCheckbox } as unknown as Event; | ||
|
|
||
| component.onChange(mockEvent); | ||
|
|
||
| expect(mockCheckbox.checked).toBe(true); | ||
| }); | ||
|
|
||
| it('should handle event when isChecked is false', () => { | ||
| component.isChecked = false; | ||
| spyOn(component.switchChanged, 'emit'); | ||
|
|
||
| const mockCheckbox = { checked: true } as HTMLInputElement; | ||
| const mockEvent = { target: mockCheckbox } as unknown as Event; | ||
|
|
||
| component.onChange(mockEvent); | ||
|
|
||
| expect(mockCheckbox.checked).toBe(false); | ||
| expect(component.switchChanged.emit).toHaveBeenCalledWith(true); | ||
| }); | ||
|
|
||
| it('should cast event.target to HTMLInputElement', () => { | ||
| component.isChecked = false; | ||
|
|
||
| const mockCheckbox = { checked: true } as HTMLInputElement; | ||
| const mockEvent = { target: mockCheckbox } as unknown as Event; | ||
|
|
||
| expect(() => component.onChange(mockEvent)).not.toThrow(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify edge case handling in onChange.
The tests don't cover scenarios where event.target might be null/undefined or when isChecked is undefined during onChange. Please verify how the component handles these edge cases and consider adding tests if these scenarios are possible.
Run the following script to examine the implementation:
🏁 Script executed:
#!/bin/bash
# Description: Check the onChange implementation for edge case handling
# Find the component implementation and look for onChange method
fd -e ts ubs-switcher.component.ts --exec cat {} | grep -A 10 "onChange"Length of output: 206
🏁 Script executed:
#!/bin/bash
# Locate isChecked declaration and default
fd -e ts ubs-switcher.component.ts --exec cat {} | rg -n "isChecked"Length of output: 104
Handle potential undefined event.target and isChecked. Guard against event.target being null before casting to HTMLInputElement, initialize the @Input() isChecked with a default value (e.g. = false), and add tests covering these edge cases.
| it('should call "goToTelegramUrl" correctly', () => { | ||
| const windowOpenSpy = spyOn(window, 'open'); | ||
| component.telegramBotURL = 'https://t.me/testbot'; | ||
| component.goToTelegramUrl(); | ||
| expect(windowOpenSpy).toHaveBeenCalledWith('https://t.me/testbot', '_blank'); | ||
| }); |
There was a problem hiding this comment.
Rename test to avoid duplicate test description.
A test with the same description "should call 'goToTelegramUrl' correctly" already exists at line 244. While the new test provides better coverage (by verifying window.open is called with correct parameters), the duplicate name creates confusion.
Suggested rename:
- it('should call "goToTelegramUrl" correctly', () => {
+ it('should open telegram bot URL in new window', () => {
const windowOpenSpy = spyOn(window, 'open');
component.telegramBotURL = 'https://t.me/testbot';
component.goToTelegramUrl();
expect(windowOpenSpy).toHaveBeenCalledWith('https://t.me/testbot', '_blank');
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should call "goToTelegramUrl" correctly', () => { | |
| const windowOpenSpy = spyOn(window, 'open'); | |
| component.telegramBotURL = 'https://t.me/testbot'; | |
| component.goToTelegramUrl(); | |
| expect(windowOpenSpy).toHaveBeenCalledWith('https://t.me/testbot', '_blank'); | |
| }); | |
| it('should open telegram bot URL in new window', () => { | |
| const windowOpenSpy = spyOn(window, 'open'); | |
| component.telegramBotURL = 'https://t.me/testbot'; | |
| component.goToTelegramUrl(); | |
| expect(windowOpenSpy).toHaveBeenCalledWith('https://t.me/testbot', '_blank'); | |
| }); |
🤖 Prompt for AI Agents
In
src/app/ubs/ubs-user/components/ubs-user-profile-page/ubs-user-profile-page.component.spec.ts
around lines 842 to 847, the test description duplicates an existing one at line
244; rename this test to a unique, descriptive string (for example: "should open
Telegram URL in a new tab" or similar) to avoid confusion and keep the assertion
verifying window.open called with the component.telegramBotURL and '_blank'
unchanged.
|
| this.isEditing = false; | ||
| this.clientProfileService | ||
| .getDataClientProfile() | ||
| .pipe(take(1)) |
There was a problem hiding this comment.
Take(1) is redundant for http.get
| } | ||
|
|
||
| const formSwitch = !!this.userForm.get('telegramIsNotify')?.value; | ||
| const serverSwitch = !!this.savedTelegramIsNotify; |
There was a problem hiding this comment.
!! is applied to boolean variable
| this.userForm.get('telegramIsNotify')?.setValue(true, { emitEvent: false }); | ||
|
|
||
| if (this.isEditing) { | ||
| if (this.savedTelegramIsNotify !== true) { |
There was a problem hiding this comment.
!this.savedTelegramIsNotify
2 participants
Changed the logic of the switcher
Added the logic of comparing the switcher status with the saved status
Added tests
Fixed the 404 error
Added the selection confirmation window
Edited the page logic for the correct operation of the notification subscription functionality
Edited the logic of the Save changes button
Summary by CodeRabbit
New Features
Bug Fixes
Tests