-
Notifications
You must be signed in to change notification settings - Fork 15
feat: add redirection for notifications + API integrations #4079
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
base: fyle-fix-setup-keys
Are you sure you want to change the base?
Changes from 13 commits
082e946
03f0a14
f5879c7
2e73a08
060f63f
178a224
ce1f560
249829c
e112f85
b8832ae
56ea99a
be63e04
52554fb
5897bb6
d787bac
b26d6e0
88cfed2
f6ec718
a440a25
7cf5cfa
ccbf3f7
2ce020f
8d22251
53013f3
448562f
2d25b9b
2b867db
f418f33
3462fde
82f5600
f070816
721d8ac
c0ba3eb
0815453
d4f18d1
50e8495
7750455
bc25593
a575594
8d3a4ae
4cbbf17
d21b725
afaa507
e55921e
b37f3c8
b32f422
fc141cf
ba510e3
adb990b
ca32a7a
fe356a4
a44f505
67d5a95
99782e3
2603fdc
6424719
3534acc
b280e92
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 |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ import { NgClass } from '@angular/common'; | |
| import { FyConnectionComponent } from './shared/components/fy-connection/fy-connection.component'; | ||
| import { Capacitor } from '@capacitor/core'; | ||
| import { AppShortcuts } from '@capawesome/capacitor-app-shortcuts'; | ||
| import { PushNotificationService } from './core/services/push-notification.service'; | ||
|
|
||
| @Component({ | ||
| selector: 'app-root', | ||
|
|
@@ -53,6 +54,8 @@ export class AppComponent implements OnInit, AfterViewInit { | |
|
|
||
| private router = inject(Router); | ||
|
|
||
| private pushNotificationService = inject(PushNotificationService); | ||
|
|
||
| private activatedRoute = inject(ActivatedRoute); | ||
|
|
||
| private userEventService = inject(UserEventService); | ||
|
|
@@ -293,6 +296,7 @@ export class AppComponent implements OnInit, AfterViewInit { | |
| this.userEventService.onLogout(() => { | ||
| this.trackingService.onSignOut(); | ||
| this.freshChatService.destroy(); | ||
| this.pushNotificationService.unregister(); | ||
| this.isSwitchedToDelegator = false; | ||
|
Comment on lines
301
to
305
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. Handle unregister promise (and avoid double-unregister). 🐛 Suggested fix (keep call, guard it)- this.pushNotificationService.unregister();
+ void this.pushNotificationService.unregister().catch(noop);🤖 Prompt for AI Agents |
||
| this.router.navigate(['/', 'auth', 'sign_in']); | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -181,6 +181,19 @@ describe('OrgUserService', () => { | |
| }); | ||
| }); | ||
|
|
||
| it('sendDeviceToken(): should send device token to the backend', (done) => { | ||
| const token = 'test-device-token'; | ||
| spenderPlatformV1ApiService.post.and.returnValue(of({})); | ||
|
|
||
| orgUserService.sendDeviceToken(token).subscribe((res) => { | ||
| expect(spenderPlatformV1ApiService.post).toHaveBeenCalledWith('/users/device_token', { | ||
| data: { token }, | ||
| }); | ||
| expect(res).toEqual({}); | ||
| done(); | ||
| }); | ||
| }); | ||
|
Comment on lines
188
to
204
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 | 🔵 Trivial One test case? Naanga oru dharavai commit pannaa, noorudhavai test pannuven! The test covers the happy path like a hero's entry scene, but Superstar needs to see some villain-fighting scenarios too:
Also, Line 192 uses spenderPlatformV1ApiService.get.and.returnValue(
of({ data: { tokens: existingTokens } } as PlatformApiResponse<{ tokens: string[] }>)
);🎬 Additional test cases - Superstar collectionit('sendDeviceToken(): should handle empty existing tokens', (done) => {
const token = 'new-device-token';
spenderPlatformV1ApiService.get.and.returnValue(
of({ data: { tokens: [] } } as PlatformApiResponse<{ tokens: string[] }>)
);
spenderPlatformV1ApiService.post.and.returnValue(of({}));
orgUserService.sendDeviceToken(token).subscribe((res) => {
expect(spenderPlatformV1ApiService.post).toHaveBeenCalledWith('/users/device_token', {
data: { tokens: [token] },
});
done();
});
});
it('sendDeviceToken(): should handle null tokens from backend', (done) => {
const token = 'new-device-token';
spenderPlatformV1ApiService.get.and.returnValue(
of({ data: { tokens: null } } as any)
);
spenderPlatformV1ApiService.post.and.returnValue(of({}));
orgUserService.sendDeviceToken(token).subscribe((res) => {
expect(spenderPlatformV1ApiService.post).toHaveBeenCalledWith('/users/device_token', {
data: { tokens: [token] },
});
done();
});
});Based on learnings: "In the Fyle mobile app, the migration from OrgUserSettingsService to PlatformEmployeeSettingsService introduced a potential breaking change where mileage_settings could be null or undefined... indicating this is a real-world scenario that needs defensive handling." 🤖 Prompt for AI Agents |
||
|
|
||
| it('should return false if the user is not switched to a delegator', (done) => { | ||
| jwtHelperService.decodeToken.and.returnValue(accessTokenData); | ||
| // This token contains the user details such as user id, org id, org user id, roles, scopes, etc. | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -99,6 +99,16 @@ export class OrgUserService { | |||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| sendDeviceToken(token: string): Observable<unknown> { | ||||||||||||||||||||||||||||||||||||||
| const payload = { | ||||||||||||||||||||||||||||||||||||||
| data: { | ||||||||||||||||||||||||||||||||||||||
| token, | ||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| return this.spenderPlatformV1ApiService.post('/users/device_token', payload); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
110
to
130
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. 🧩 Analysis chain🏁 Script executed: # Search for device_token and /users/device_token references
rg -n -C 3 "/users/device_token|device_token" --type=tsRepository: fylein/fyle-mobile-app Length of output: 1196 🏁 Script executed: # Look for any API documentation or interface definitions related to device tokens
rg -n "tokens|deviceToken" --type=ts -A 2 -B 2 | head -100Repository: fylein/fyle-mobile-app Length of output: 372 🏁 Script executed: # Check for any test files that might show expected payload
fd -e spec.ts -e test.ts | xargs rg -l "device_token\|deviceToken" 2>/dev/nullRepository: fylein/fyle-mobile-app Length of output: 50 🏁 Script executed: # Look for any backend API contract files or documentation
fd -e "*.json" -e "*.md" -e "*.yaml" -e "*.yml" | xargs rg -l "device_token" 2>/dev/nullRepository: fylein/fyle-mobile-app Length of output: 154 🏁 Script executed: cat -n src/app/core/services/org-user.service.spec.ts | sed -n '185,200p'Repository: fylein/fyle-mobile-app Length of output: 624 Fix the payload schema to match the expected contract. The implementation sends Required fix const payload = {
data: {
- tokens: [token],
+ token,
},
};📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Comment on lines
110
to
130
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. Mind it! Duplicate tokens can sneak in like a villain! Thalaiva sees two concerns here:
🎬 Proposed fix - Superstar style sendDeviceToken(token: string): Observable<unknown> {
return this.getDeviceTokens().pipe(
map((existingTokens) => {
- const tokens = existingTokens ?? [];
- tokens.push(token);
- return tokens;
+ if (existingTokens.includes(token)) {
+ return existingTokens;
+ }
+ return [...existingTokens, token];
}),
switchMap((tokens) =>
this.spenderPlatformV1ApiService.post('/users/device_token', {
data: { tokens },
}),
),
);
}🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| getUserById(userId: string): Observable<EouApiResponse> { | ||||||||||||||||||||||||||||||||||||||
| return this.apiService.get('/eous/' + userId); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,12 +1,30 @@ | ||||||||||||||||||||
| import { Injectable } from '@angular/core'; | ||||||||||||||||||||
| import { Injectable, inject } from '@angular/core'; | ||||||||||||||||||||
| import { PushNotifications, Token } from '@capacitor/push-notifications'; | ||||||||||||||||||||
| import { DeepLinkService } from './deep-link.service'; | ||||||||||||||||||||
| import { TrackingService } from './tracking.service'; | ||||||||||||||||||||
| import { UserEventService } from './user-event.service'; | ||||||||||||||||||||
| import { OrgUserService } from './org-user.service'; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @Injectable({ | ||||||||||||||||||||
| providedIn: 'root', | ||||||||||||||||||||
| }) | ||||||||||||||||||||
| export class PushNotificationService { | ||||||||||||||||||||
| private listenersInitialized = false; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private deepLinkService: DeepLinkService = inject(DeepLinkService); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private trackingService: TrackingService = inject(TrackingService); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private userEventService: UserEventService = inject(UserEventService); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private orgUserService: OrgUserService = inject(OrgUserService); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| constructor() { | ||||||||||||||||||||
| this.userEventService.onLogout(() => { | ||||||||||||||||||||
| void this.unregister(); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| async initializePushNotifications(): Promise<void> { | ||||||||||||||||||||
| const permission = await PushNotifications.requestPermissions(); | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -16,18 +34,51 @@ export class PushNotificationService { | |||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| async unregister(): Promise<void> { | ||||||||||||||||||||
| await PushNotifications.unregister(); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private initListeners(): void { | ||||||||||||||||||||
| if (this.listenersInitialized) { | ||||||||||||||||||||
| return; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| this.listenersInitialized = true; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| PushNotifications.addListener('registration', (_token: Token) => { | ||||||||||||||||||||
| // TODO: Integrate API for sending token to backend | ||||||||||||||||||||
| this.addRegistrationListener(); | ||||||||||||||||||||
| this.addRegistrationErrorListener(); | ||||||||||||||||||||
| this.addNotificationClickListener(); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private addRegistrationListener(): void { | ||||||||||||||||||||
| PushNotifications.addListener('registration', (token: Token) => { | ||||||||||||||||||||
| const tokenValue = token?.value; | ||||||||||||||||||||
| if (tokenValue) { | ||||||||||||||||||||
| this.orgUserService.sendDeviceToken(tokenValue).subscribe(); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| this.trackingService.eventTrack('Push Notification Registered'); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private addRegistrationErrorListener(): void { | ||||||||||||||||||||
| PushNotifications.addListener('registrationError', () => undefined); | ||||||||||||||||||||
| PushNotifications.addListener('pushNotificationActionPerformed', () => undefined); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
||||||||||||||||||||
| private addRegistrationErrorListener(): void { | |
| PushNotifications.addListener('registrationError', () => undefined); | |
| PushNotifications.addListener('pushNotificationActionPerformed', () => undefined); | |
| } | |
| private addRegistrationErrorListener(): void { | |
| PushNotifications.addListener('registrationError', (error) => { | |
| this.trackingService.eventTrack('Push Notification Registration Failed', { error: error?.error }); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@src/app/core/services/push-notification.service.ts` around lines 63 - 65, The
current addRegistrationErrorListener swallows errors; update
addRegistrationErrorListener so the
PushNotifications.addListener('registrationError', ...) callback accepts the
error payload and forwards it to your app's telemetry/logging (e.g., call
this.logger.error(error) and/or this.telemetryService.captureException(error)),
falling back to console.error if no logger is available; ensure you reference
PushNotifications.addListener and addRegistrationErrorListener when making the
change so the registrationError handler records the error instead of returning
undefined.
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.
Guard deep-link parsing to avoid crash.
Line 80 can throw if the URL is malformed — wrap in try/catch so a bad payload doesn’t kill the handler.
🛡️ Suggested fix
this.trackingService.eventTrack('Push Notification Clicked', { actionType });
-const redirectParams = this.deepLinkService.getJsonFromUrl(url);
-this.deepLinkService.redirect(redirectParams);
+try {
+ const redirectParams = this.deepLinkService.getJsonFromUrl(url);
+ this.deepLinkService.redirect(redirectParams);
+} catch (error) {
+ this.trackingService.eventTrack('Push Notification Redirect Failed', { error });
+}🤖 Prompt for AI Agents
In `@src/app/core/services/push-notification.service.ts` around lines 68 - 82, The
push notification click handler in addNotificationClickListener uses
deepLinkService.getJsonFromUrl(url) and deepLinkService.redirect(redirectParams)
without guarding against malformed URLs; wrap the parsing and redirect logic in
a try/catch inside the PushNotifications.addListener callback so exceptions from
getJsonFromUrl or redirect are caught, log or handle the error (via
trackingService or a logger) and return gracefully instead of throwing; keep the
early checks for url and event the same and ensure
trackingService.eventTrack('Push Notification Clicked', { actionType }) still
runs before the guarded parse/redirect block.
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.
Unregisters a token on log out