feat: update portal for handling JWT Token API changes #332
Conversation
WalkthroughA new GraphQL query Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 5
🧹 Outside diff range and nitpick comments (7)
apps/portal/src/app/views/applications/application.model.ts (3)
1-8: LGTM! Consider using a more specific type forstatus.The
Applicationinterface is well-structured and uses appropriate types for its properties. However, thestatusproperty could benefit from a more specific type definition.Consider using an enum or a union type for the
statusproperty to improve type safety and readability. For example:export enum ApplicationStatus { PENDING = 0, APPROVED = 1, REJECTED = 2, // Add other status values as needed } export interface Application { // ... other properties status: ApplicationStatus; }This change would make the possible status values explicit and prevent invalid status assignments.
10-15: LGTM! Consider adding JSDoc comments for better documentation.The
ApplicationResponseinterface is well-structured and appropriate for handling paginated application data.To improve documentation, consider adding JSDoc comments to describe the purpose of each property. For example:
export interface ApplicationResponse { /** Array of Application objects */ applications: Application[]; /** Total number of applications available */ total: number; /** Starting index of the current page */ offset: number; /** Maximum number of items per page */ limit: number; }This addition would enhance the interface's self-documentation and improve developer experience when working with this type.
1-15: Great addition to the codebase! This file aligns well with the PR objectives.This new file,
application.model.ts, provides well-structured interfaces for handling application data, which is crucial for the portal updates related to JWT Token API changes mentioned in the PR objectives. TheApplicationandApplicationResponseinterfaces will ensure type safety and consistency when working with application data throughout the portal.A few minor suggestions have been made to further improve the code:
- Consider using an enum for the
statusproperty in theApplicationinterface.- Add JSDoc comments to the
ApplicationResponseinterface for better documentation.These interfaces will be particularly useful when implementing the new
GetApplicationsGraphQL query mentioned in the AI-generated summary, as they provide a clear structure for the expected response data.As you continue to implement the portal updates, ensure that these interfaces are used consistently across the application, particularly in the new
ApplicationsServiceclass mentioned in the AI-generated summary. This will help maintain a cohesive and type-safe approach to handling application data throughout the portal.apps/portal/src/app/graphql/graphql.queries.ts (1)
33-57: LGTM! Consider adding sorting flexibility.The
GetApplicationsquery is well-structured and implements pagination and filtering effectively. The response includes all necessary fields for application data and pagination metadata.Consider adding sorting options as parameters to make the query more flexible. This would allow clients to sort by different fields and choose the sort order. For example:
query GetApplications($limit: Int!, $offset: Int!, $filters: [UniversalFilter!], $sortBy: String, $sortOrder: SortOrder) { applications( options: { limit: $limit offset: $offset sortBy: $sortBy sortOrder: $sortOrder filters: $filters } ) { # ... (rest of the query remains the same) } }This change would require corresponding updates in the backend to handle these new sorting parameters.
apps/portal/src/app/views/notifications/notifications.component.html (1)
215-217: LGTM! Consider adding ARIA attributes for better accessibility.The addition of this paragraph improves user guidance by providing clear instructions when no application is selected. This is a good UX enhancement.
Consider adding ARIA attributes to improve accessibility. Here's a suggested modification:
- <p *ngIf="!selectedApplication && !loading"> + <p *ngIf="!selectedApplication && !loading" role="alert" aria-live="polite"> Please select an application via the dropdown to view notifications </p>This change will ensure screen readers announce the message when it appears, improving the experience for users relying on assistive technologies.
apps/portal/src/app/views/notifications/notifications.component.ts (2)
142-146: Review the necessity of displaying a success message after setting applications.Displaying a success message every time applications are successfully fetched might not enhance the user experience and could clutter the notification area.
Consider removing this success message unless there's a specific case where the user needs confirmation. This will help keep the user interface clean and focused on critical alerts.
199-200: Address the TODO: Find a better workaround than usingsetupConfig().The TODO comment indicates that the current use of
setupConfig()is a temporary solution. Improving this will enhance the code's maintainability and clarity.I can help refactor the initialization logic to eliminate the need for
setupConfig(). This might involve:
- Removing unnecessary delays.
- Ensuring that all necessary data is fetched and set before components load.
- Utilizing Angular's lifecycle hooks appropriately.
Would you like me to provide a refactored example or open a GitHub issue to track this enhancement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- apps/portal/src/app/graphql/graphql.queries.ts (1 hunks)
- apps/portal/src/app/views/applications/application.model.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 (7 hunks)
🧰 Additional context used
🪛 Biome
apps/portal/src/app/views/notifications/notifications.component.ts
[error] 76-76: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead(lint/complexity/noBannedTypes)
🔇 Additional comments (7)
apps/portal/src/app/graphql/graphql.queries.ts (1)
59-65: LGTM! Improved security by removingallKeysfrom login response.The
LoginUsermutation has been simplified to only return a token. This change aligns with the PR objectives and improves security by not exposing potentially sensitive key information during the login process.To ensure this change is consistent across the codebase, let's verify that no other parts of the code are expecting the
allKeysfield in the login response:If this search returns any results, we should review those occurrences to ensure they're updated to work with the new login response structure.
apps/portal/src/app/views/applications/applications.service.ts (4)
1-6: LGTM: Imports are appropriate and well-organized.The imports cover all necessary dependencies for an Angular service using GraphQL and RxJS. They are correctly organized and there are no unused imports.
8-15: LGTM: Well-structured interface for GraphQL response.The
GetApplicationsResponseinterface accurately models the expected GraphQL response structure. The use of optional properties is a good practice, allowing for flexibility in the response.
16-19: LGTM: Service class declaration follows Angular best practices.The
ApplicationsServiceis correctly decorated with@Injectableand provided in 'root', making it a singleton across the application. This is appropriate for a data service.
20-20: LGTM: Constructor uses dependency injection correctly.The constructor properly injects the
GraphqlServiceas a private member, following Angular best practices for dependency injection.apps/portal/src/app/views/notifications/notifications.component.html (2)
Line range hint
1-217: Overall, the changes improve user experience and align with PR objectives.The modifications to this template file enhance the user interface for application selection and provide better guidance. These changes support the PR's goal of updating the portal for JWT Token API changes by ensuring proper application selection before displaying notifications.
Key improvements:
- Updated condition for displaying the application dropdown.
- Added user guidance when no application is selected.
These changes contribute to a more intuitive and user-friendly interface for managing notifications.
56-56: LGTM! Verify data source for applications.The condition for displaying the dropdown has been updated to use
applications.length. This change aligns with the introduction of the newApplicationsServicementioned in the PR summary.To ensure consistency, please verify that:
- The
applicationsarray is properly populated by theApplicationsService.- The
allApplicationsListis no longer used elsewhere in the component.You can run the following script to check for any remaining usage of
allApplicationsList:
apps/portal/src/app/views/notifications/notifications.component.ts
Outdated
Show resolved
Hide resolved
apps/portal/src/app/views/notifications/notifications.component.ts
Outdated
Show resolved
Hide resolved
apps/portal/src/app/views/notifications/notifications.component.ts
Outdated
Show resolved
Hide resolved
kshitij-k-osmosys
added
ready for review
help wanted
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/portal/src/app/views/notifications/notifications.component.ts (1)
93-133: LGTM with suggestion: Robust implementation of getApplications method.The
getApplicationsmethod is well-structured with proper error handling using RxJScatchError. However, consider moving the success message inside the subscription callback to ensure it's only displayed when applications are successfully fetched.Consider this modification:
.subscribe((applicationResponse: ApplicationResponse) => { // Fetch list of applications for dropdown this.applications = applicationResponse.applications.map((obj) => ({ // Name to display and ID to return upon selection label: obj.name, value: obj.applicationId, })); this.selectedApplication = this.applications[0].value; + this.messageService.add({ + key: 'tst', + severity: 'success', + summary: 'Success', + detail: 'Applications set', + }); }); -this.messageService.add({ - key: 'tst', - severity: 'success', - summary: 'Success', - detail: 'Applications set', -});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- apps/portal/src/app/views/applications/applications.service.ts (1 hunks)
- apps/portal/src/app/views/notifications/notifications.component.html (1 hunks)
- apps/portal/src/app/views/notifications/notifications.component.ts (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/portal/src/app/views/applications/applications.service.ts
- apps/portal/src/app/views/notifications/notifications.component.html
🔇 Additional comments (5)
apps/portal/src/app/views/notifications/notifications.component.ts (5)
7-7: LGTM: Import for ApplicationsService added.The import for
ApplicationsServiceis correctly added and is necessary for the new application fetching functionality.
88-90: LGTM: Improved initialization sequence.The
ngOnInitmethod has been correctly updated to an async method. The sequence of awaitinggetApplicationsbeforeloadNotificationsLazyensures that applications are loaded before notifications, addressing the previous issue of redundant application fetching.
258-259: LGTM: Improved notification fetching with async/await.The update to use async/await for notification fetching is a good improvement. It enhances readability and is consistent with the method's async signature.
Line range hint
1-290: Overall improvements to NotificationsComponent.The changes to
NotificationsComponentsignificantly enhance its functionality and error handling:
- Improved initialization sequence in
ngOnInit.- New
getApplicationsmethod for fetching application data.- Enhanced
loadNotificationsLazymethod with better error handling and application filtering.- Consistent use of async/await for asynchronous operations.
These changes address previous issues and improve the overall structure of the component. The suggestions provided in earlier comments will further refine the implementation.
Line range hint
171-196: LGTM with a note: Improved loadNotificationsLazy method.The changes to
loadNotificationsLazyare well-implemented:
- The async modification is appropriate.
- Early return when no application is selected prevents unnecessary API calls.
- The application ID filter ensures notifications are fetched for the selected application only.
However, the TODO comment on line 186 needs attention:
Regarding the TODO comment, let's investigate the potential console error:
Please review the results of this search to identify and address the source of the console error mentioned in the TODO comment.
✅ Verification successful
Verified: Improved
loadNotificationsLazymethod.The changes to
loadNotificationsLazyare well-implemented:
- The
asyncmodification is appropriate for handling asynchronous operations.- Early return when no application is selected prevents unnecessary API calls.
- The application ID filter ensures notifications are fetched for the selected application only.
Additionally, the TODO comment regarding console errors does not correspond to any existing
console.errororthrowstatements within the method, indicating that the issue has been resolved. It is recommended to remove the TODO comment to maintain code clarity.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any error logging related to user login or application selection rg -i "console\.error|throw.*error" --type typescriptLength of output: 90
Script:
#!/bin/bash # Search for any error logging related to user login or application selection in TypeScript files rg -i "console\.error|throw.*error" -g "*.ts" -g "*.tsx"Length of output: 6803
Script:
#!/bin/bash # Search for console.error or throw statements within loadNotificationsLazy method in notifications.component.ts ast-grep --lang typescript --pattern $' class $_ { $$$ loadNotificationsLazy($_) { $$$ console.error($_); throw $_; $$$ } $$$ } ' apps/portal/src/app/views/notifications/notifications.component.tsLength of output: 221
apps/portal/src/app/views/notifications/notifications.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
apps/portal/src/app/views/notifications/notifications.component.ts (3)
48-48: Consider adding type definition forapplicationsproperty.For better type safety and code clarity, it's recommended to explicitly define the type of the
applicationsproperty. Based on its usage in thegetApplicationsmethod, you could define it as follows:applications: { label: string; value: string }[] = [];This will help prevent potential type-related errors and improve code readability.
93-144: LGTM: Well-implemented application fetching logic with proper error handling.The
getApplicationsmethod is well-structured with appropriate error handling using RxJScatchError. The transformation of application data for dropdown use is correct, and the sequencing withloadNotificationsLazyis logical.One minor suggestion for improvement:
Consider using thefinalizeoperator from RxJS to ensure thatloadNotificationsLazyis always called, even if an error occurs:.pipe( catchError((error) => { // ... existing error handling ... return of(null); }), finalize(() => { this.loadNotificationsLazy({ first: 0, rows: this.pageSize }); }) ) .subscribe((applicationResponse: ApplicationResponse | null) => { // ... existing logic ... // Remove the loadNotificationsLazy call from here });This ensures that notifications are always attempted to be loaded, regardless of the success or failure of the application fetch.
🧰 Tools
🪛 Biome
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
190-218: LGTM: Improved notification loading with application filtering.The changes to
loadNotificationsLazymethod are well-implemented. The addition of login token retrieval and the check for a selected application ensure that notifications are only loaded under the correct conditions. The application ID filter correctly limits the notifications to the selected application.Minor suggestion for improvement:
Consider adding a user-friendly message when no application is selected:if (!this.selectedApplication) { this.loading = false; this.messageService.add({ key: 'tst', severity: 'info', summary: 'No Application Selected', detail: 'Please select an application to view notifications.', }); return; }This will provide better feedback to the user about why notifications aren't being loaded.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/portal/src/app/views/notifications/notifications.component.ts (8 hunks)
🧰 Additional context used
🪛 Biome
apps/portal/src/app/views/notifications/notifications.component.ts
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 296-296: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
apps/portal/src/app/views/notifications/notifications.component.ts (4)
7-8: LGTM: New imports added for application handling.The new imports for
ApplicationsServiceandApplicationResponseare correctly added and align with the changes in application handling implemented in this component.
84-84: LGTM: ApplicationsService correctly injected.The
ApplicationsServiceis properly injected into the constructor, which is consistent with the new application handling functionality implemented in this component.
89-89: LGTM: Applications fetched on component initialization.Calling
getApplications()in thengOnInitmethod ensures that the list of applications is fetched when the component initializes, which is the correct approach for initializing component data.
Line range hint
281-304: LGTM: Improved error handling and state management in notification subscription.The changes to the notification subscription logic are well-implemented:
- The addition of the
catchErroroperator provides robust error handling.- The loading state is properly managed, ensuring the UI accurately reflects the current state.
- The null check for the notification response prevents potential runtime errors.
These improvements enhance the reliability and user experience of the notification loading process.
🧰 Tools
🪛 Biome
[error] 296-296: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/portal/src/app/views/notifications/notifications.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/portal/src/common/constants/notification.ts (2)
22-23: LGTM! Consider using an enum for better type safety.The addition of new channel types (AWS_SES and SMS_SNS) is consistent with the existing structure and follows the sequential numbering. This aligns well with the PR objectives.
Consider refactoring the
ChannelTypeobject into a TypeScript enum for better type safety and autocompletion. For example:export enum ChannelType { UNKNOWN = -1, SMTP = 1, // ... other channel types ... AWS_SES = 11, SMS_SNS = 12, }This change would provide better type checking and make it easier to maintain the channel types in the future.
104-110: LGTM! Consider clarifying the providerName for SMS_SNS.The new entry for SMS_SNS in the ChannelTypeMap is well-structured and consistent with existing entries. The properties are appropriate for an SMS service using AWS SNS, and the use of the AWS icon for the providerIcon maintains consistency with other AWS services in the map.
Consider changing the
providerNamefrom "SMS SNS" to "AWS SNS" for clarity and consistency with other AWS services in the map. This would make it clearer that it's an AWS service without repeating the channel type. Here's the suggested change:[ChannelType.SMS_SNS]: { serviceIcon: 'assets/icons/sms.svg', altText: 'SMS', - providerName: 'SMS SNS', + providerName: 'AWS SNS', providerIcon: 'assets/icons/aws.png', style: 'sms', },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/portal/src/app/views/notifications/notifications.component.html (1 hunks)
- apps/portal/src/common/constants/notification.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/portal/src/app/views/notifications/notifications.component.html
🔇 Additional comments (1)
apps/portal/src/common/constants/notification.ts (1)
97-103: LGTM! AWS_SES entry is well-structured and consistent.The new entry for AWS_SES in the ChannelTypeMap is well-structured and consistent with existing entries. The properties are appropriate for an AWS SES email service, and the use of the AWS icon for the providerIcon maintains consistency with other AWS services in the map.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
apps/portal/src/app/views/applications/applications.service.ts (1)
42-60: LGTM: Well-structured token decoding method with proper error handling.The
decodeTokenmethod is well-implemented with appropriate error handling. The use ofeslint-disable-next-lineforclass-methods-use-thisis justified if this method doesn't use any instance properties or methods.Minor suggestion: Consider making this method static since it doesn't use any instance properties. This would make it clear that the method doesn't depend on the instance state and can be called without instantiating the class.
- // eslint-disable-next-line class-methods-use-this - decodeToken(token: string): string { + static decodeToken(token: string): string { // ... (rest of the method remains the same) }This change would allow the method to be called as
ApplicationsService.decodeToken(token)without needing an instance of the class.apps/portal/src/app/views/notifications/notifications.component.ts (2)
48-48: Consider adding a type annotation to theapplicationsproperty.For improved type safety and code clarity, consider adding a type annotation to the
applicationsproperty. Based on its usage in thegetApplicationsmethod, you could use:applications: { label: string; value: string }[] = [];This will make the structure of the array items explicit and help catch potential type-related errors during development.
Line range hint
284-292: Consider improving error handling in notification fetching.While the addition of error handling using
catchErroris a good improvement, the current implementation returnsof(null), which might lead to unexpected behavior in the subscription. Consider re-throwing the error after logging it and setting the loading state:catchError((error) => { this.messageService.add({ key: 'tst', severity: 'error', summary: 'Error', detail: `There was an error while loading notifications. Reason: ${error.message}`, }); this.loading = false; throw error; // Re-throw the error }),This change ensures that the subscription's error handler is invoked, allowing for more consistent error handling throughout the component.
🧰 Tools
🪛 Biome
[error] 296-296: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/portal/src/app/views/applications/applications.service.ts (1 hunks)
- apps/portal/src/app/views/notifications/notifications.component.ts (8 hunks)
🧰 Additional context used
🪛 Biome
apps/portal/src/app/views/notifications/notifications.component.ts
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 296-296: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (7)
apps/portal/src/app/views/applications/applications.service.ts (2)
1-15: LGTM: Imports and interface declaration are well-structured.The imports cover all necessary dependencies for the service's functionality. The
GetApplicationsResponseinterface accurately defines the structure of the GraphQL query response, which will help with type checking and code clarity.
16-20: LGTM: Class declaration and dependency injection are correct.The
ApplicationsServiceis properly decorated with@Injectableand provided in the root, ensuring it's available as a singleton throughout the application. The constructor correctly injects theGraphqlService, which will be used for making GraphQL queries.apps/portal/src/app/views/notifications/notifications.component.ts (5)
7-8: LGTM: New imports added for application handling.The addition of
ApplicationsServiceandApplicationResponseimports aligns with the new application handling functionality in the component.
89-89: LGTM: Simplified initialization inngOnInit.The change to directly call
this.getApplications()inngOnInitsimplifies the component's initialization process.
93-144: LGTM: Improved application fetching and error handling.The refactoring of
getApplicationsmethod has significantly improved the application fetching process and error handling. The use of avariablesobject for querying, fetching the login token, and transforming applications into selectable options are all good improvements.🧰 Tools
🪛 Biome
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
191-218: LGTM: Improved robustness inloadNotificationsLazy.The changes in
loadNotificationsLazymethod have improved its robustness:
- Using
getJWTLoginTokento fetch the login token ensures consistency with other parts of the component.- The check for
selectedApplicationprevents unnecessary API calls when no application is selected.- Updating the date formatting to use
toISOString()ensures consistent date representation.These changes collectively enhance the reliability and consistency of the notification loading process.
Also applies to: 267-267
295-304: LGTM: Improved subscription handling for notification fetching.The changes in the subscription handling for notification fetching have improved its robustness:
- Checking for both
notificationResponseandnotificationResponse.notificationsensures that the code doesn't attempt to access properties of a null object.- Setting default values when the response is null prevents potential errors and ensures a consistent state.
These changes enhance the reliability of the notification loading process and improve error resilience.
🧰 Tools
🪛 Biome
[error] 296-296: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
kshitij-k-osmosys
added
ready for review
and removed
help wanted
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:
Update portal for handling JWT Token API changes
Related changes:
Screenshots:
On initial load when user logins
Pending actions:
NA
Additional notes:
Commit cef5ee2 for refactoring code
Summary by CodeRabbit
Release Notes
New Features
GetApplicationsfor retrieving application data with pagination and filtering options.ApplicationsServiceto manage application data retrieval.AWS_SESfor email notifications andSMS_SNSfor SMS notifications.Bug Fixes
Documentation