Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Delegatee switch back top header #3466

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix: Delegatee switch back top header #3466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3ad9acb
intial commit
Feb 1, 2025
76a91e8
test fix
Feb 1, 2025
52bf8d7
test fix
Feb 1, 2025
043b49d
test fix
Feb 1, 2025
23d61da
test fix
Feb 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/app/shared/components/delegated-acc-message/delegated-acc-message.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,4 @@
<span class="delegated-acc"> You're now managing {{ delegateeName | ellipsis: 15 }}'s account </span>
<span class="delegated-acc">
You're now managing {{ delegateeName | ellipsis : 15 }}'s account
<u class="u-link" [ngClass]="{ disabled: !isConnected$ | async }" (click)="switchBack()">Switch back</u>
</span>
Comment on lines +1 to +4
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

⚠️ Potential issue

Punch It Right: Fix the Async Pipe and Click Guard

Boss, the style is cool but be careful! Wrap the async pipe evaluation in parentheses so the negation applies to its result—not the observable itself. Use this:

<u class="u-link" [ngClass]="{ 'disabled': !(isConnected$ | async) }" (click)="(isConnected$ | async) ? switchBack() : null">Switch back</u>

This way you ensure that the disabled style is applied correctly and the click event only fires when connected.

8 changes: 8 additions & 0 deletions src/app/shared/components/delegated-acc-message/delegated-acc-message.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,11 @@
font-weight: bold;
margin-top: env(safe-area-inset-top);
}

.u-link {
color: inherit;
}

.u-link.disabled {
color: gray;
}
Comment on lines +16 to +22
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Style with a Bang: Enhance Pointer Visuals

Superstar, your link styles are almost legendary! To further impress, add visual cues by setting cursor: pointer; for interactive links and cursor: not-allowed; for the disabled state. For example:

 .u-link {
-  color: inherit;
+  color: inherit;
+  cursor: pointer;
 }
 
 .u-link.disabled {
-  color: gray;
+  color: gray;
+  cursor: not-allowed;
 }

This will ensure that users know when the element is clickable and when it's not, just like a Rajinikanth punch!

📝 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.

Suggested change
.u-link {
color: inherit;
}
.u-link.disabled {
color: gray;
}
.u-link {
color: inherit;
cursor: pointer;
}
.u-link.disabled {
color: gray;
cursor: not-allowed;
}

12 changes: 6 additions & 6 deletions src/app/shared/components/delegated-acc-message/delegated-acc-message.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { ComponentFixture, TestBed, async, fakeAsync, flush, waitForAsync } from '@angular/core/testing';
import { ComponentFixture, TestBed, waitForAsync } from '@angular/core/testing';
import { IonicModule } from '@ionic/angular';
import { DelegatedAccMessageComponent } from './delegated-acc-message.component';
import { AuthService } from 'src/app/core/services/auth.service';
import { EllipsisPipe } from '../../pipes/ellipses.pipe';
import { RouterTestingModule } from '@angular/router/testing';
import { of } from 'rxjs';
Comment on lines +6 to +7
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

NetworkService missing its spotlight in the test arena!

I checked the component, and it's clear that NetworkService plays a crucial role there. However, the test file doesn't cast this actor, leaving the switch-back functionality without complete coverage. Ensure you add a proper mock or stub for NetworkService in the test setup for the component.

  • The component (src/app/shared/components/delegated-acc-message/delegated-acc-message.component.ts) correctly injects NetworkService.
  • The spec file (src/app/shared/components/delegated-acc-message/delegated-acc-message.component.spec.ts) lacks a setup or mock for NetworkService.
🔗 Analysis chain

Mind-blowing additions, but where's the network service test setup, partner?

You've added RouterTestingModule for navigation testing, but I notice you haven't set up the NetworkService that's needed for the new switchBack functionality. Just like how I make a grand entrance in every scene, your tests should cover all the important actors!

Let me help you verify if NetworkService is used in the component:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if NetworkService is used in the component
ast-grep --pattern 'class DelegatedAccMessageComponent {
  $$$
  constructor($$$) {
    $$$
  }
  $$$
}'

Length of output: 106

Script:

#!/bin/bash
# Search for NetworkService usage in the component implementation file
rg "NetworkService" src/app/shared/components/delegated-acc-message/delegated-acc-message.component.ts

Length of output: 290

Script:

#!/bin/bash
# Check if NetworkService is mocked or provided in the test configuration inside the spec file
rg "NetworkService" src/app/shared/components/delegated-acc-message/delegated-acc-message.component.spec.ts

Length of output: 107

import { apiEouRes } from 'src/app/core/mock-data/extended-org-user.data';
import { getElementBySelector, getTextContent } from 'src/app/core/dom-helpers';

describe('DelegatedAccMessageComponent', () => {
let component: DelegatedAccMessageComponent;
Expand All @@ -13,9 +14,10 @@ describe('DelegatedAccMessageComponent', () => {

beforeEach(waitForAsync(() => {
const authServiceSpy = jasmine.createSpyObj('AuthService', ['getEou']);
authServiceSpy.getEou.and.returnValue(of(apiEouRes));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

The transformation from Promise to Observable is stylish, but incomplete!

Like how I transform from a simple bus conductor to a superstar, you've transformed the auth service mock from Promise to Observable. But where are the test cases for the new switchBack feature? The RouterTestingModule is ready for action, but no one's called "Action!" yet!

Add these test cases to make your test suite as powerful as my punch dialogues:

it('should handle switch back when connected', () => {
  component.isConnected$ = of(true);
  component.switchBack();
  // Add router navigation assertions
});

it('should not switch back when disconnected', () => {
  component.isConnected$ = of(false);
  component.switchBack();
  // Verify no navigation occurs
});

Also applies to: 20-20

TestBed.configureTestingModule({
declarations: [DelegatedAccMessageComponent, EllipsisPipe],
imports: [IonicModule.forRoot()],
imports: [IonicModule.forRoot(), RouterTestingModule],
providers: [
{
provide: AuthService,
Expand All @@ -26,8 +28,6 @@ describe('DelegatedAccMessageComponent', () => {

fixture = TestBed.createComponent(DelegatedAccMessageComponent);
component = fixture.componentInstance;
authService = TestBed.inject(AuthService) as jasmine.SpyObj<AuthService>;
authService.getEou.and.returnValue(Promise.resolve(apiEouRes));
fixture.detectChanges();
}));

Expand All @@ -38,7 +38,7 @@ describe('DelegatedAccMessageComponent', () => {
it("should display delegatee's name", () => {
component.delegateeName = 'Abhishek Jain';
fixture.detectChanges();
expect(getTextContent(getElementBySelector(fixture, '.delegated-acc'))).toEqual(
expect(fixture.nativeElement.querySelector('.delegated-acc').textContent).toContain(
`You're now managing Abhishek Jain's account`
);
Comment on lines +41 to 43
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

The selector change is sharp, but let's make it sharper!

You've changed the element selection like how I changed the style of action scenes in cinema! But remember, in testing, like in movies, we need to be precise with our expectations.

Let's make it even more precise:

const messageElement = fixture.nativeElement.querySelector('.delegated-acc');
expect(messageElement).toBeTruthy();
expect(messageElement.textContent.trim()).toBe(`You're now managing Abhishek Jain's account`);

});
Expand Down
30 changes: 25 additions & 5 deletions src/app/shared/components/delegated-acc-message/delegated-acc-message.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,40 @@
import { Component, OnInit } from '@angular/core';
import { Component, OnInit, EventEmitter } from '@angular/core';
import { AuthService } from '../../../core/services/auth.service';
import { from } from 'rxjs';
import { from, concat, Observable } from 'rxjs';
import { shareReplay } from 'rxjs/operators';
import { Router } from '@angular/router';
import { NetworkService } from '../../../core/services/network.service';

@Component({
selector: 'app-delegated-acc-message',
templateUrl: './delegated-acc-message.component.html',
styleUrls: ['./delegated-acc-message.component.scss'],
})
export class DelegatedAccMessageComponent implements OnInit {
delegateeName;
delegateeName: string;

constructor(private authService: AuthService) {}
isConnected$: Observable<boolean>;

ngOnInit() {
constructor(private authService: AuthService, private router: Router, private networkService: NetworkService) {}

ngOnInit(): void {
from(this.authService.getEou()).subscribe((res) => {
this.delegateeName = res.us.full_name;
});
this.setupNetworkWatcher();
}

setupNetworkWatcher(): void {
const networkWatcherEmitter = this.networkService.connectivityWatcher(new EventEmitter<boolean>());
this.isConnected$ = concat(this.networkService.isOnline(), networkWatcherEmitter.asObservable()).pipe(
shareReplay(1)
);
}

switchBack(): void {
if (!this.isConnected$) {
return;
}
this.router.navigate(['/', 'enterprise', 'delegated_accounts', { switchToOwn: true }]);
}
}