feat: log out of portal when no JWTLoginToken exist in localstorage #383
Conversation
WalkthroughThis pull request introduces a comprehensive enhancement to the notifications and providers management system. The changes focus on integrating provider data with notification handling, adding new GraphQL queries, updating service methods, and modifying the frontend components. The modifications improve type safety, expand data retrieval capabilities, and provide more flexible filtering options for users when interacting with notifications and providers. Changes
Sequence DiagramsequenceDiagram
participant User
participant NotificationsComponent
participant ProvidersService
participant GraphQL
User->>NotificationsComponent: Select Application
NotificationsComponent->>ProvidersService: getProvidersAndNotifications()
ProvidersService->>GraphQL: Query Providers and Notifications
GraphQL-->>ProvidersService: Return Response
ProvidersService-->>NotificationsComponent: Populate Providers and Notifications
NotificationsComponent->>User: Display Updated Data
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
kshitij-k-osmosys
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
apps/portal/src/app/views/applications/applications.service.ts (1)
Line range hint
39-58: Consider enhancing token validation.While not part of the current changes, given the PR's focus on JWT token handling, consider these improvements to the
decodeTokenmethod:
- Add token expiration validation
- Consider making this a static method since it doesn't use instance properties
- Add signature verification if security is critical
Example enhancement:
static decodeToken(token: string): { payload: any; isValid: boolean; error?: string; } { try { const [header, payload, signature] = token.split('.'); const decodedPayload = JSON.parse(atob(payload)); // Add expiration check const isExpired = decodedPayload.exp && Date.now() >= decodedPayload.exp * 1000; return { payload: decodedPayload, isValid: !isExpired, error: isExpired ? 'Token has expired' : undefined }; } catch (error) { return { payload: null, isValid: false, error: `Invalid token: ${error.message}` }; } }apps/portal/src/app/views/providers/providers.service.ts (2)
12-40: Combine response interfaces to reduce duplication.The interfaces
GetProviderNotificationResponseandGetProviderArchivedNotificationResponseshare a similar structure. Consider creating a generic interface or type to handle both cases, which will improve maintainability and reduce code duplication.You can define a generic interface like this:
interface GetProviderResponse<T> { providers: { providers?: Provider[]; total?: number; offset?: number; limit?: number; }; notifications: { notifications?: T[]; total?: number; offset?: number; limit?: number; }; }Then, you can use it with the specific notification types:
type GetProviderNotificationResponse = GetProviderResponse<Notification>; type GetProviderArchivedNotificationResponse = GetProviderResponse<ArchivedNotification>;
53-118: Refactor to eliminate duplicated logic ingetProvidersAndNotifications.The method contains duplicated code in both the
ifandelseblocks, particularly in processing the response and handling errors. Refactoring the common logic into helper functions will enhance readability and maintainability.You can refactor the mapping and error handling like this:
private handleResponse<T>( response: ApolloQueryResult<T>, isArchived: boolean, ): ProviderAndNotificationResponse { // Common mapping logic here } private handleError(error: any): Observable<never> { const errorMessage: string = error.message; throw new Error(errorMessage); }Then, update your method:
getProvidersAndNotifications( variables: unknown, inputToken: string, archivedNotificationToggle: boolean, ): Observable<ProviderAndNotificationResponse> { const query = archivedNotificationToggle ? GetProvidersAndArchivedNotifications : GetProvidersAndNotifications; return this.graphqlService.query(query, variables, inputToken).pipe( map((response) => this.handleResponse(response, archivedNotificationToggle), ), catchError((error) => this.handleError(error)), ); }apps/portal/src/app/views/notifications/notifications.component.ts (1)
Line range hint
212-222: Simplify error handling ingetJWTLoginTokenmethod.Throwing and catching an error within the same method can be simplified. Instead, return
nulldirectly whenuserDatais not found, and handle JSON parsing errors separately.Apply this diff to simplify the method:
getJWTLoginToken() { try { const userData = localStorage.getItem('osmoXUserData'); if (userData) { const userToken = JSON.parse(userData).token; return userToken; } - throw new Error('User data not found in localStorage'); + this.messageService.add({ + key: 'tst', + severity: 'warn', + summary: 'Warning', + detail: 'User data not found in localStorage', + }); + return null; } catch (error) { this.messageService.add({ key: 'tst', severity: 'error', summary: 'Error', detail: `Error parsing user data: ${error.message}`, }); return null; } }apps/portal/src/app/views/notifications/notifications.service.ts (1)
Line range hint
47-47: Apply similar type safety improvements to getArchivedNotifications.For consistency, consider applying the same type safety improvements to the
getArchivedNotificationsmethod.- getArchivedNotifications(variables, inputToken): Observable<NotificationResponse> { + getArchivedNotifications(variables: unknown, inputToken: string): Observable<NotificationResponse> {apps/portal/src/app/graphql/graphql.queries.ts (1)
97-154: LGTM! Well-structured combined query.The query effectively combines providers and notifications data with proper pagination, sorting, and comprehensive field selection.
Consider query optimization.
Consider making some fields optional or adding field selection arguments to optimize response size based on usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/portal/src/app/graphql/graphql.queries.ts(1 hunks)apps/portal/src/app/views/applications/applications.service.ts(1 hunks)apps/portal/src/app/views/notifications/notifications.component.html(2 hunks)apps/portal/src/app/views/notifications/notifications.component.ts(10 hunks)apps/portal/src/app/views/notifications/notifications.service.ts(1 hunks)apps/portal/src/app/views/providers/provider.model.ts(1 hunks)apps/portal/src/app/views/providers/providers.service.ts(1 hunks)
🔇 Additional comments (7)
apps/portal/src/app/views/applications/applications.service.ts (1)
22-22: LGTM! Type safety improvements are well implemented.The explicit type annotations for both parameters enhance type safety:
variables: unknownis safer than an implicit typeinputToken: stringproperly enforces the token typeapps/portal/src/app/views/notifications/notifications.component.ts (1)
486-494: Review date handling logic in filters.Adding one day to
selectedToDatemight unintentionally include extra records due to time differences. Consider setting the time to the end of the day or using date libraries for more precise control.Example adjustment:
if (this.selectedToDate) { combinedNotificationFilters.push({ field: 'createdOn', operator: 'lt', - value: new Date( - new Date(this.selectedToDate).setDate(this.selectedToDate.getDate() + 1), - ).toISOString(), + value: new Date( + this.selectedToDate.setHours(23, 59, 59, 999), + ).toISOString(), }); }apps/portal/src/app/views/providers/provider.model.ts (1)
4-11: Ensure correct typing of date fields inProviderinterface.The
createdOnandupdatedOnfields are typed asDate. If the GraphQL response returns ISO string dates, consider typing these fields asstringor transforming them intoDateobjects after receiving the data.Adjust the interface or transform the data accordingly:
export interface Provider { providerId: number; name: string; channelType: number; - createdOn: Date; - updatedOn: Date; + createdOn: string; + updatedOn: string; status: number; }Or transform the strings to
Dateobjects when processing the response.✅ Verification successful
Use
stringtype for date fields in theProviderinterfaceThe codebase shows a consistent pattern of handling dates as ISO strings in GraphQL responses, with explicit conversion to
Dateobjects when needed. The suggested change to usestringtype forcreatedOnandupdatedOnfields aligns with this pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find GraphQL schema files echo "=== Looking for GraphQL schema files ===" fd -e graphql -e gql # Search for Provider type definition in GraphQL schemas echo -e "\n=== Searching for Provider type in GraphQL schemas ===" rg "type Provider" -A 10 # Search for Provider-related queries echo -e "\n=== Looking for Provider queries ===" rg "query.*Provider" -A 5 # Look for date handling patterns echo -e "\n=== Searching for date transformation patterns ===" rg "new Date\(" -A 2Length of output: 24551
apps/portal/src/app/views/notifications/notifications.service.ts (1)
32-32: LGTM! Type safety improvements.The explicit type annotations for parameters enhance type safety and maintainability.
apps/portal/src/app/graphql/graphql.queries.ts (1)
156-213: LGTM! Consistent query structure.The archived notifications query maintains consistency with the active notifications query structure.
Verify the need for duplicate provider fields.
Since both queries fetch the same provider fields, consider extracting the provider query into a separate operation if these queries are rarely used together.
✅ Verification successful
Keep the current query structure
The verification shows that these queries are used in a toggle-based manner for switching between active and archived notifications views. While they're never used simultaneously, keeping them structured similarly makes the service layer implementation cleaner and more maintainable. The duplication of provider fields is justified by the tight coupling of these related use cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if these queries are used together or separately rg -l "GetProvidersAndNotifications|GetProvidersAndArchivedNotifications" --type tsLength of output: 189
Script:
#!/bin/bash # Check how these queries are used in the service rg -A 5 -B 5 "GetProvidersAndArchivedNotifications" apps/portal/src/app/views/providers/providers.service.ts # Look for other queries that might use provider fields ast-grep --pattern 'providers { providers { applicationId channelType createdOn name providerId status updatedOn }'Length of output: 1366
apps/portal/src/app/views/notifications/notifications.component.html (2)
9-11: LGTM! Provider selection dropdown.The dropdown implementation maintains UI consistency while switching from channel types to providers.
55-57: LGTM! Updated event handler.The event handler properly loads both providers and notifications for the selected application.
Portal PR Checklist
Pre-requisites
PR Details
PR details have been updated as per the given format (see below)
feat: add admin login page)Additional Information
ready for reviewshould be added if the PR is ready to be reviewed)Note: Reviewer should ensure that the checklist and description have been populated and followed correctly, and the PR should be merged only after resolving all conversations and verifying that CI checks pass.
Description:
Add function
authService.logoutUserto log users out when LoginToken check fails i.e. when there is no JWTLoginToken in localstorageSummary by CodeRabbit
New Features
Bug Fixes
Refactor