-
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 all 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 |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
|
|
||
| /* eslint-disable */ | ||
| export interface AppState { | ||
| isActive: boolean; | ||
| } | ||
|
|
||
| // Minimal shape of AppInfo used by the app. | ||
| export interface AppInfo { | ||
| version: string; | ||
| } | ||
|
|
||
| // Use a loose event shape so this mock is compatible with both | ||
| // 'appStateChange' (which has isActive) and 'appUrlOpen' (which has url), | ||
| // avoiding type errors in application code during tests. | ||
| export type AppListener = (event: { [key: string]: any }) => void | Promise<void>; | ||
|
|
||
| export const App = { | ||
| addListener(_eventName: string, _listener: AppListener): Promise<{ remove: () => void }> { | ||
| // In tests we don't actually hook into any real app lifecycle, | ||
| // so just return a no-op remover. | ||
| return Promise.resolve({ remove: () => {} }); | ||
| }, | ||
|
|
||
| // Simple stub for App.getInfo used by DeviceService. | ||
| getInfo(): Promise<AppInfo> { | ||
| return Promise.resolve({ version: '1.2.3' }); | ||
| }, | ||
|
|
||
| exitApp(): Promise<void> { | ||
| return Promise.resolve(); | ||
| }, | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| /* eslint-disable */ | ||
|
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 Blanket eslint-disable - use with caution, boss! The 🧹 Cleanup suggestionSince line 1 already disables all rules, remove the redundant directive on line 11: export interface PushNotificationEvent {
- // eslint-disable-next-line `@typescript-eslint/no-explicit-any`
[key: string]: any;
}🤖 Prompt for AI Agents |
||
| export interface PermissionStatus { | ||
| receive: string; | ||
| } | ||
|
|
||
| export interface Token { | ||
| value?: string; | ||
| } | ||
|
|
||
| export interface PushNotificationEvent { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| [key: string]: any; | ||
| } | ||
|
|
||
| export type PushNotificationListener = (event: PushNotificationEvent) => void; | ||
|
|
||
|
Comment on lines
2
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. Boss, lint police will halt the show — move type declarations into .model.ts (one per file). CI already fails because this mock defines multiple interfaces/types in a non-.model.ts file. Split each type into its own 🛠️ Suggested change (re-export types from model files)-export interface PermissionStatus {
- receive: string;
-}
-
-export interface Token {
- value?: string;
- // Allow additional properties so the type is flexible in tests.
- [key: string]: any;
-}
-
-export interface PushNotificationEvent {
- notification?: {
- data?: any;
- };
-}
-
-export type PushNotificationListener = (event: PushNotificationEvent) => void;
+export type { PermissionStatus } from './permission-status.model';
+export type { Token } from './token.model';
+export type { PushNotificationEvent } from './push-notification-event.model';
+export type { PushNotificationListener } from './push-notification-listener.model';Example model file: // __mocks__/@capacitor/token.model.ts
export type Token = { value?: string; [key: string]: unknown };🧰 Tools🪛 GitHub Actions: Lint[error] 1-1: ESLint: Interfaces should be defined only in files with .model.ts extension (custom-rules/prefer-semantic-extension-name) 🪛 GitHub Check: Run linters[failure] 17-17: [failure] 17-17: [failure] 13-13: [failure] 11-11: [failure] 11-11: [failure] 8-8: [failure] 5-5: [failure] 5-5: [failure] 1-1: 🤖 Prompt for AI Agents |
||
| export const PushNotifications = { | ||
| requestPermissions(): Promise<PermissionStatus> { | ||
| return Promise.resolve({ receive: 'denied' }); | ||
| }, | ||
|
|
||
| checkPermissions(): Promise<PermissionStatus> { | ||
| return Promise.resolve({ receive: 'denied' }); | ||
| }, | ||
|
|
||
| register(): Promise<void> { | ||
| return Promise.resolve(); | ||
| }, | ||
|
|
||
| unregister(): Promise<void> { | ||
| return Promise.resolve(); | ||
| }, | ||
|
|
||
| addListener(_eventName: string, _listener: any): Promise<{ remove: () => void }> { | ||
| return Promise.resolve({ remove: () => {} }); | ||
| }, | ||
| }; | ||
|
|
||
| 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); | ||
|
|
@@ -164,6 +167,12 @@ export class AppComponent implements OnInit, AfterViewInit { | |
| }); | ||
| }); | ||
|
|
||
| // Initialize push notification click listener early to catch notifications | ||
| // that launched the app from a killed state | ||
| if (Capacitor.isNativePlatform()) { | ||
| this.pushNotificationService.initializeNotificationClickListener(); | ||
| } | ||
|
|
||
| // Handle app shortcuts (Home Screen Quick Actions) | ||
| if (Capacitor.isNativePlatform()) { | ||
| AppShortcuts.addListener('click', (event) => { | ||
|
|
@@ -242,7 +251,6 @@ export class AppComponent implements OnInit, AfterViewInit { | |
| this.footerService.selectionMode$.subscribe((isEnabled) => { | ||
| this.showFooter = !isEnabled; | ||
| }); | ||
|
|
||
| // This was done as a security fix for appknox | ||
| // eslint-disable-next-line | ||
| if ((window as any) && (window as any).localStorage) { | ||
|
|
@@ -293,6 +301,7 @@ export class AppComponent implements OnInit, AfterViewInit { | |
| this.userEventService.onLogout(() => { | ||
| this.trackingService.onSignOut(); | ||
| this.freshChatService.destroy(); | ||
| this.pushNotificationService.unregister(); | ||
|
Contributor
Author
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. Unregisters a token on log out |
||
| 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 |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| export interface MobilePushNotificationSettings { | ||
| enabled: boolean; | ||
| allowed: boolean; | ||
| unsubscribed_events: string[]; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -185,6 +185,24 @@ describe('OrgUserService', () => { | |
| }); | ||
| }); | ||
|
|
||
| it('sendDeviceToken(): should fetch existing tokens and append new token when sending to backend', (done) => { | ||
| const token = 'test-device-token'; | ||
| const existingTokens: string[] = ['existing-token-1']; | ||
| const expectedTokens = [...existingTokens, token]; | ||
|
|
||
| spenderPlatformV1ApiService.get.and.returnValue(of({ data: { tokens: existingTokens } } as any)); | ||
| spenderPlatformV1ApiService.post.and.returnValue(of({})); | ||
|
|
||
| orgUserService.sendDeviceToken(token).subscribe((res) => { | ||
| expect(spenderPlatformV1ApiService.get).toHaveBeenCalledWith('/device_token'); | ||
| expect(spenderPlatformV1ApiService.post).toHaveBeenCalledWith('/device_token', { | ||
| data: { tokens: expectedTokens }, | ||
| }); | ||
| 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 |
|---|---|---|
|
|
@@ -101,6 +101,34 @@ export class OrgUserService { | |
| ); | ||
| } | ||
|
|
||
| getDeviceTokens(): Observable<string[]> { | ||
| return this.spenderPlatformV1ApiService | ||
| .get<PlatformApiResponse<{ tokens: string[] }>>('/device_token') | ||
| .pipe(map((response) => response.data?.tokens ?? [])); | ||
| } | ||
|
|
||
| sendDeviceToken(token: string): Observable<unknown> { | ||
| const payload = { | ||
| data: { | ||
| tokens: [token], | ||
| }, | ||
| }; | ||
|
|
||
| return this.spenderPlatformV1ApiService.post('/device_token', payload); | ||
| // return this.getDeviceTokens().pipe( | ||
| // map((existingTokens) => { | ||
| // const tokens = existingTokens ?? []; | ||
| // tokens.push(token); | ||
| // return tokens; | ||
| // }), | ||
| // switchMap((tokens) => | ||
| // this.spenderPlatformV1ApiService.post('/device_token', { | ||
| // data: { tokens }, | ||
| // }), | ||
| // ), | ||
| // ); | ||
| } | ||
|
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 | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -421,6 +421,11 @@ export class PlatformOrgSettingsService { | |||||||||||||||||||||||||
| auto_report_submission_settings: { | ||||||||||||||||||||||||||
| expense_grouping_dimensions: incoming.auto_report_submission_settings?.expense_grouping_dimensions || [], | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| mobile_notification_settings: { | ||||||||||||||||||||||||||
| allowed: incoming.mobile_notification_settings && incoming.mobile_notification_settings.allowed, | ||||||||||||||||||||||||||
| enabled: incoming.mobile_notification_settings && incoming.mobile_notification_settings.enabled, | ||||||||||||||||||||||||||
| unsubscribed_events: incoming.mobile_notification_settings && incoming.mobile_notification_settings.unsubscribed_events, | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
|
Comment on lines
+424
to
+428
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. Boss, avoid truthy-but-empty 🔥 Suggested fix- mobile_notification_settings: {
- allowed: incoming.mobile_notification_settings && incoming.mobile_notification_settings.allowed,
- enabled: incoming.mobile_notification_settings && incoming.mobile_notification_settings.enabled,
- unsubscribed_events: incoming.mobile_notification_settings && incoming.mobile_notification_settings.unsubscribed_events,
- },
+ mobile_notification_settings: incoming.mobile_notification_settings
+ ? {
+ allowed: incoming.mobile_notification_settings.allowed ?? false,
+ enabled: incoming.mobile_notification_settings.enabled ?? false,
+ unsubscribed_events: incoming.mobile_notification_settings.unsubscribed_events ?? [],
+ }
+ : undefined,📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Object.keys(orgSettings).forEach((settingsType) => { | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
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.
Fix the lint blockers in the App mock, boss.
CI is already red on explicit
any, unused params, and the emptyremovebody. Tighten the type and mark params used so the show goes on.🛠️ Suggested fix
🧰 Tools
🪛 GitHub Actions: Lint
[error] 15-15: Unexpected any. Specify a different type
@typescript-eslint/no-explicit-any🪛 GitHub Check: Run linters
[failure] 21-21:
Unexpected empty method 'remove'
[failure] 18-18:
'_listener' is defined but never used
[failure] 18-18:
'_eventName' is defined but never used
[failure] 15-15:
Unexpected any. Specify a different type
🤖 Prompt for AI Agents