-
Notifications
You must be signed in to change notification settings - Fork 15
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
Changes from all commits
8c5ba46
7899bd9
8a5beec
395851b
a531116
8af0e15
96d7496
48d5d8b
a5b9a90
7562cc6
a2d1f3e
a7de94c
41fc18a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,8 @@ | |
oncanplay="this.play()" | ||
poster="../../../assets/images/video-default-background.png" | ||
loop | ||
playsinline | ||
autoplay | ||
> | ||
<source src="/assets/videos/mobile-onboarding.mp4" type="video/mp4" /> | ||
</video> | ||
Comment on lines
14
to
16
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! 🌟
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 - expect(platformHandlerService.registerBackButtonAction).toHaveBeenCalledTimes(1);
+ expect(platformHandlerService.registerBackButtonAction).toHaveBeenCalledOnceWith(
+ jasmine.any(Function),
+ BackButtonActionPriority.MEDIUM
+ ); 📝 Committable suggestion
Suggested change
|
||||||||||||
}); | ||||||||||||
|
||||||||||||
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); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||||||||||||
|
@@ -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(); | ||||||||||||
|
@@ -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); | ||||||||||||
}); | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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], | ||
|
@@ -54,6 +57,10 @@ describe('VerifyPage', () => { | |
}, | ||
}, | ||
}, | ||
{ | ||
provide: UserEventService, | ||
useValue: userEventServiceSpy, | ||
}, | ||
], | ||
}).compileComponents(); | ||
|
||
|
@@ -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', () => { | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Add test cases for all scenarios to make your error handling as powerful as a Rajinikanth punch! 🌟 🔗 Analysis chainStyle-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 executedThe 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
|
||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -461,6 +461,8 @@ export class TasksComponent implements OnInit { | |
|
||
await cardAddedModal.present(); | ||
await cardAddedModal.onDidDismiss(); | ||
|
||
this.doRefresh(); | ||
Comment on lines
+464
to
+465
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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
+ });
|
||
}); | ||
} | ||
|
||
|
There was a problem hiding this comment.
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
andautoplay
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:
These changes will:
aria-label
preload="metadata"
📝 Committable suggestion