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: Fix issues with sign in page redirection and video view #3404

Merged
merged 13 commits into from
Jan 7, 2025
Merged

fix: Fix issues with sign in page redirection and video view #3404

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8c5ba46
Minor
bistaastha Jan 6, 2025
7899bd9
Minor
bistaastha Jan 6, 2025
8a5beec
Minor
bistaastha Jan 6, 2025
395851b
Minor
bistaastha Jan 6, 2025
a531116
Minor
bistaastha Jan 7, 2025
8af0e15
Minor
bistaastha Jan 7, 2025
96d7496
Minor
bistaastha Jan 7, 2025
48d5d8b
Minor
bistaastha Jan 7, 2025
a5b9a90
Minor
bistaastha Jan 7, 2025
7562cc6
Minor
bistaastha Jan 7, 2025
a2d1f3e
Minor
bistaastha Jan 7, 2025
a7de94c
Minor
bistaastha Jan 7, 2025
41fc18a
Minor
bistaastha Jan 7, 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
2 changes: 2 additions & 0 deletions src/app/auth/sign-in/sign-in.page.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
oncanplay="this.play()"
poster="../../../assets/images/video-default-background.png"
loop
playsinline
autoplay
Comment on lines +12 to +13
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Mind it! The video attributes are spot on, but let's make it more accessible, shall we?

The playsinline and autoplay attributes are perfect additions for iOS devices, making the video play smoothly like my action sequences! 🎬

But wait! Let me tell you something important. For better accessibility, like how I make my movies enjoyable for everyone:

 <video
   class="sign-in__video-container__video"
   width="100%"
   height="auto"
   onloadedmetadata="this.muted = true"
   oncanplay="this.play()"
   poster="../../../assets/images/video-default-background.png"
   loop
   playsinline
   autoplay
+  aria-label="Fyle mobile app introduction"
+  preload="metadata"
 >

These changes will:

  • Add screen reader support with aria-label
  • Optimize loading with preload="metadata"
📝 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
playsinline
autoplay
<video
class="sign-in__video-container__video"
width="100%"
height="auto"
onloadedmetadata="this.muted = true"
oncanplay="this.play()"
poster="../../../assets/images/video-default-background.png"
loop
playsinline
autoplay
aria-label="Fyle mobile app introduction"
preload="metadata"
>

>
<source src="/assets/videos/mobile-onboarding.mp4" type="video/mp4" />
</video>
Comment on lines 14 to 16
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 ah? Let's make this video work everywhere like my movies!

For maximum compatibility across browsers, like how my movies reach every corner of the world:

 <video
   ...
 >
   <source src="/assets/videos/mobile-onboarding.mp4" type="video/mp4" />
+  <source src="/assets/videos/mobile-onboarding.webm" type="video/webm" />
+  <!-- Fallback content for browsers that don't support video -->
+  <p>Your browser doesn't support HTML video. Here's a <a href="/assets/videos/mobile-onboarding.mp4">link to the video</a> instead.</p>
 </video>

Adding WebM format and fallback content ensures our video plays everywhere, just like how I never disappoint my fans! 🌟

Committable suggestion skipped: line range outside the PR's diff.

Expand Down
2 changes: 1 addition & 1 deletion src/app/auth/sign-in/sign-in.page.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ describe('SignInPage', () => {
component.ionViewWillEnter();
expect(component.hardwareBackButtonAction).toEqual(mockSubscription);
expect(platformHandlerService.registerBackButtonAction).toHaveBeenCalledOnceWith(
BackButtonActionPriority.MEDIUM,
BackButtonActionPriority.ABSOLUTE,
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Mind it! The test file is not in sync with the actual implementation, partner!

The sign-in page uses MEDIUM priority in the actual implementation, but the test file is trying to be the superstar with ABSOLUTE priority. Let's keep it real and match the test with the implementation!

  • src/app/auth/sign-in/sign-in.page.ts:291: Uses BackButtonActionPriority.MEDIUM
  • src/app/auth/sign-in/sign-in.page.spec.ts:232: Incorrectly uses BackButtonActionPriority.ABSOLUTE
🔗 Analysis chain

Mind it! The back button priority change needs verification.

The change from MEDIUM to ABSOLUTE priority could affect other components' back button handling. This might be too powerful, like my punch!

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other components using BackButtonActionPriority
ast-grep --pattern 'BackButtonActionPriority.$_'

Length of output: 2761

component.goBack
);
});
Expand Down
2 changes: 1 addition & 1 deletion src/app/auth/sign-in/sign-in.page.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ export class SignInPage implements OnInit {
} else {
this.currentStep = SignInPageState.SELECT_SIGN_IN_METHOD;
}
const priority = BackButtonActionPriority.MEDIUM;
const priority = BackButtonActionPriority.ABSOLUTE;
this.hardwareBackButtonAction = this.platformHandlerService.registerBackButtonAction(priority, this.goBack);
}
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Mind it! The state management needs more style!

The state transitions are working, but we can make them SUPER by adding validation and persistence!

Consider creating a dedicated state manager:

interface SignInState {
  currentStep: SignInPageState;
  email?: string;
  isLoading: boolean;
}

class SignInStateManager {
  private state: SignInState;
  
  canTransition(from: SignInPageState, to: SignInPageState): boolean {
    // Add validation logic
    return true;
  }
  
  transition(to: SignInPageState): void {
    if (this.canTransition(this.state.currentStep, to)) {
      this.state.currentStep = to;
    }
  }
}

Also applies to: 293-295


Expand Down
22 changes: 16 additions & 6 deletions src/app/auth/verify/verify.page.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { of, throwError } from 'rxjs';
import { apiEouRes } from 'src/app/core/mock-data/extended-org-user.data';
import { getElementBySelector, getTextContent } from 'src/app/core/dom-helpers';
import { VerifyPageState } from './verify.enum';
import { UserEventService } from 'src/app/core/services/user-event.service';

describe('VerifyPage', () => {
let component: VerifyPage;
Expand All @@ -19,12 +20,14 @@ describe('VerifyPage', () => {
let routerAuthService: jasmine.SpyObj<RouterAuthService>;
let authService: jasmine.SpyObj<AuthService>;
let trackingService: jasmine.SpyObj<TrackingService>;
let userEventService: jasmine.SpyObj<UserEventService>;

beforeEach(waitForAsync(() => {
const routerSpy = jasmine.createSpyObj('Router', ['navigate']);
const routerAuthServiceSpy = jasmine.createSpyObj('RouterAuthService', ['emailVerify']);
const authServiceSpy = jasmine.createSpyObj('AuthService', ['newRefreshToken']);
const trackingServiceSpy = jasmine.createSpyObj('TrackingService', ['emailVerified', 'onSignin']);
const userEventServiceSpy = jasmine.createSpyObj('UserEventService', ['logout']);

TestBed.configureTestingModule({
declarations: [VerifyPage],
Expand Down Expand Up @@ -54,6 +57,10 @@ describe('VerifyPage', () => {
},
},
},
{
provide: UserEventService,
useValue: userEventServiceSpy,
},
],
}).compileComponents();

Expand All @@ -63,6 +70,7 @@ describe('VerifyPage', () => {
routerAuthService = TestBed.inject(RouterAuthService) as jasmine.SpyObj<RouterAuthService>;
authService = TestBed.inject(AuthService) as jasmine.SpyObj<AuthService>;
trackingService = TestBed.inject(TrackingService) as jasmine.SpyObj<TrackingService>;
userEventService = TestBed.inject(UserEventService) as jasmine.SpyObj<UserEventService>;
}));

it('should create', () => {
Expand Down Expand Up @@ -107,13 +115,15 @@ describe('VerifyPage', () => {
const error = {
status: 404,
};
const logoutSpy = spyOn(component, 'logout');
component.handleError(error);
expect(router.navigate).toHaveBeenCalledOnceWith([
'/',
'auth',
'pending_verification',
{ orgId: 'orNVthTo2Zyo' },
]);
expect(logoutSpy).toHaveBeenCalledTimes(1);
Comment on lines +118 to +120
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

💡 Codebase verification

Mind it! Your error handling needs more power, machan!

The verification reveals three different paths in the error handler, but the test only covers one! Here's what we're missing, style-ah style-ah:

  • Status 422: Should navigate to 'auth/disabled'
  • Status 440: Should navigate to 'auth/pending_verification' with correct params
  • Other status codes: Currently tested, correctly calls logout()

Add test cases for all scenarios to make your error handling as powerful as a Rajinikanth punch! 🌟

🔗 Analysis chain

Style-ah style-ah! But let's verify the error scenarios, machan!

The error handling test now redirects to logout instead of a specific route. But we should verify all possible error scenarios to ensure robust error handling.

Add test cases for other HTTP status codes that might occur during verification:

it('should handle network errors gracefully', () => {
  const error = { status: 0 };  // Network error
  const logoutSpy = spyOn(component, 'logout');
  component.handleError(error);
  expect(logoutSpy).toHaveBeenCalledTimes(1);
});
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for all error handling scenarios in the codebase
rg -B 2 -A 2 "handleError.*404" --type ts

Length of output: 43

Script:

#!/bin/bash
# Search for handleError implementation
ast-grep --pattern 'handleError($$$) { $$$ }'

# Search for error handling in verify page
rg -B 2 -A 4 "handleError" src/app/auth/verify/verify.page.ts

# Search for HTTP error imports and usage
rg -B 2 -A 2 "HttpErrorResponse" src/app/auth/verify/

Length of output: 599

Script:

#!/bin/bash
# Get the complete handleError method implementation with more context
rg -B 2 -A 10 "handleError.*void" src/app/auth/verify/verify.page.ts

# Also check for logout method usage
rg -B 2 -A 2 "logout\(" src/app/auth/verify/verify.page.ts

Length of output: 780

});
});

it('logout(): should log out the user', () => {
component.logout();
expect(userEventService.logout).toHaveBeenCalledTimes(1);
expect(router.navigate).toHaveBeenCalledOnceWith(['/', 'auth', 'sign_in']);
});
bistaastha marked this conversation as resolved.
Show resolved Hide resolved
});
11 changes: 9 additions & 2 deletions src/app/auth/verify/verify.page.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { RouterAuthService } from 'src/app/core/services/router-auth.service';
import { switchMap, tap } from 'rxjs/operators';
import { AuthService } from 'src/app/core/services/auth.service';
import { TrackingService } from '../../core/services/tracking.service';
import { UserEventService } from 'src/app/core/services/user-event.service';

@Component({
selector: 'app-verify',
Expand All @@ -16,7 +17,8 @@ export class VerifyPage implements OnInit {
private routerAuthService: RouterAuthService,
private authService: AuthService,
private router: Router,
private trackingService: TrackingService
private trackingService: TrackingService,
private userEventService: UserEventService
) {}

ngOnInit(): void {
Expand All @@ -43,7 +45,12 @@ export class VerifyPage implements OnInit {
} else if (err.status === 440) {
this.router.navigate(['/', 'auth', 'pending_verification', { hasTokenExpired: true, orgId }]);
} else {
this.router.navigate(['/', 'auth', 'pending_verification', { orgId }]);
this.logout();
}
}

logout(): void {
this.userEventService.logout();
this.router.navigate(['/', 'auth', 'sign_in']);
}
}
1 change: 1 addition & 0 deletions src/app/fyle/dashboard/tasks/tasks.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,7 @@ export class TasksComponent implements OnInit {
const popoverResponse = (await addCorporateCardPopover.onDidDismiss()) as OverlayResponse<{ success: boolean }>;

if (popoverResponse.data?.success) {
this.doRefresh();
this.handleEnrollmentSuccess();
}
}
Expand Down
Loading