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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
11 changes: 4 additions & 7 deletions src/app/auth/sign-in/sign-in.page.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -228,18 +228,15 @@ describe('SignInPage', () => {
spyOn(component, 'goBack');
component.ionViewWillEnter();
expect(component.hardwareBackButtonAction).toEqual(mockSubscription);
expect(platformHandlerService.registerBackButtonAction).toHaveBeenCalledOnceWith(
BackButtonActionPriority.MEDIUM,
component.goBack
);
expect(platformHandlerService.registerBackButtonAction).toHaveBeenCalledTimes(1);
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 test expectation needs more power, partner!

The test only verifies the call count but not the actual parameters passed to registerBackButtonAction. Let's make it more robust!

-    expect(platformHandlerService.registerBackButtonAction).toHaveBeenCalledTimes(1);
+    expect(platformHandlerService.registerBackButtonAction).toHaveBeenCalledOnceWith(
+      jasmine.any(Function),
+      BackButtonActionPriority.MEDIUM
+    );
📝 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
expect(platformHandlerService.registerBackButtonAction).toHaveBeenCalledTimes(1);
expect(platformHandlerService.registerBackButtonAction).toHaveBeenCalledOnceWith(
jasmine.any(Function),
BackButtonActionPriority.MEDIUM
);

});

describe('goBack(): ', () => {
it('should navigate to ENTER_EMAIL when current step is ENTER_PASSWORD', () => {
spyOn(component, 'changeState');

component.currentStep = SignInPageState.ENTER_PASSWORD;
component.goBack();
component.goBack(component.currentStep);
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, partner! The test readability needs a boost!

The test cases would be clearer if we explicitly show what currentStep is being passed. Let's make it more readable!

-      component.goBack(component.currentStep);
+      const currentStep = SignInPageState.ENTER_PASSWORD;
+      component.goBack(currentStep);

Apply similar changes to other test cases for consistency.

Also applies to: 249-249, 259-259


expect(component.changeState).toHaveBeenCalledWith(SignInPageState.ENTER_EMAIL);
expect(backButtonService.showAppCloseAlert).not.toHaveBeenCalled();
Expand All @@ -249,7 +246,7 @@ describe('SignInPage', () => {
spyOn(component, 'changeState');

component.currentStep = SignInPageState.SELECT_SIGN_IN_METHOD;
component.goBack();
component.goBack(component.currentStep);

expect(backButtonService.showAppCloseAlert).toHaveBeenCalledTimes(1);
expect(component.changeState).not.toHaveBeenCalled();
Expand All @@ -259,7 +256,7 @@ describe('SignInPage', () => {
spyOn(component, 'changeState');

component.currentStep = SignInPageState.ENTER_EMAIL;
component.goBack();
component.goBack(component.currentStep);

expect(component.changeState).toHaveBeenCalledWith(SignInPageState.SELECT_SIGN_IN_METHOD);
});
Expand Down
9 changes: 6 additions & 3 deletions src/app/auth/sign-in/sign-in.page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -266,8 +266,8 @@ export class SignInPage implements OnInit {
await this.loginInfoService.addLoginInfo(deviceInfo.appVersion, new Date());
}

goBack(): void {
switch (this.currentStep) {
goBack(currentStep: SignInPageState): void {
switch (currentStep) {
case SignInPageState.ENTER_EMAIL:
this.changeState(SignInPageState.SELECT_SIGN_IN_METHOD);
break;
Expand All @@ -285,8 +285,11 @@ export class SignInPage implements OnInit {
} else {
this.currentStep = SignInPageState.SELECT_SIGN_IN_METHOD;
}
const fn = (): void => {
this.goBack(this.currentStep);
};
const priority = BackButtonActionPriority.MEDIUM;
this.hardwareBackButtonAction = this.platformHandlerService.registerBackButtonAction(priority, this.goBack);
this.hardwareBackButtonAction = this.platformHandlerService.registerBackButtonAction(priority, fn);
}

changeState(state: SignInPageState): void {
Expand Down
22 changes: 16 additions & 6 deletions src/app/auth/verify/verify.page.spec.ts
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
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-2.component.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ export function TestCases2(getTestBed) {
});

it('handleEnrollmentSuccess(): should handle enrollment success and trigger subsequent actions', fakeAsync(() => {
spyOn(component, 'doRefresh');
corporateCreditCardExpenseService.clearCache.and.returnValue(of(null));

const mockPopover = {
Expand Down
2 changes: 2 additions & 0 deletions src/app/fyle/dashboard/tasks/tasks.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,8 @@ export class TasksComponent implements OnInit {

await cardAddedModal.present();
await cardAddedModal.onDidDismiss();

this.doRefresh();
Comment on lines +464 to +465
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 change looks good, but let's make it even better!

The addition of this.doRefresh() after card enrollment is a stylish move, partner! It ensures the tasks list stays fresh like my morning coffee. But let me tell you something - we can make it even more powerful!

Consider adding error handling to make it super-robust:

     await cardAddedModal.onDidDismiss();
-    this.doRefresh();
+    this.doRefresh().catch((error) => {
+      console.error('Failed to refresh tasks after card enrollment:', error);
+      // Show user-friendly error message if needed
+    });

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

});
}

Expand Down
Loading