feat: toggle active/archived notifications on portal #368
Conversation
WalkthroughA new GraphQL query, Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
apps/portal/src/app/views/notifications/notification.model.ts (1)
Line range hint
1-32: Reduce code duplication using interface inheritanceBoth
NotificationandArchivedNotificationinterfaces share identical properties. Consider creating a base interface to reduce duplication and improve maintainability.Consider refactoring like this:
export interface BaseNotification { id: number; channelType: number; data: Record<string, unknown>; deliveryStatus: number; result?: Record<string, unknown> | null; createdOn: Date; updatedOn: Date; createdBy: string; updatedBy: string; status: number; } export interface Notification extends BaseNotification {} export interface ArchivedNotification extends BaseNotification {} export interface NotificationResponse { notifications: Notification[]; total: number; offset: number; limit: number; }This approach:
- Reduces code duplication
- Makes it easier to maintain common properties
- Allows for future extension of specific interfaces if needed
apps/portal/src/app/graphql/graphql.queries.ts (1)
33-61: Consider using GraphQL fragments for shared fieldsThe
GetArchivedNotificationsquery shares many fields withGetNotifications. Consider extracting these common fields into a GraphQL fragment to improve maintainability and reduce duplication.Here's how you could refactor this:
+const NotificationFields = gql` + fragment NotificationFields on BaseNotification { + channelType + createdBy + createdOn + data + deliveryStatus + result + status + updatedBy + updatedOn + } +`; export const GetNotifications = gql` query GetNotifications($limit: Int!, $offset: Int!, $filters: [UniversalFilter!]) { notifications( options: { limit: $limit offset: $offset sortBy: "createdOn" sortOrder: DESC filters: $filters } ) { notifications { id + ...NotificationFields - channelType - createdBy - createdOn - data - deliveryStatus - result - status - updatedBy - updatedOn } total offset limit } } + ${NotificationFields} `; export const GetArchivedNotifications = gql` query GetArchivedNotifications($limit: Int!, $offset: Int!, $filters: [UniversalFilter!]) { archivedNotifications( options: { limit: $limit offset: $offset sortBy: "createdOn" sortOrder: DESC filters: $filters } ) { archivedNotifications { notificationId + ...NotificationFields - channelType - createdBy - createdOn - data - deliveryStatus - result - status - updatedBy - updatedOn } total offset limit } } + ${NotificationFields} `;apps/portal/src/app/views/notifications/notifications.service.ts (2)
17-24: Consider adding interface documentation.The interface structure is well-defined and maintains consistency with
GetNotificationsResponse. Consider adding JSDoc comments to document the interface purpose and properties.+/** + * Response interface for archived notifications GraphQL query + */ interface GetArchivedNotificationsResponse { archivedNotifications: { archivedNotifications?: ArchivedNotification[]; total?: number; offset?: number; limit?: number; }; }
77-80: Consider enhancing error handling.The current error handling could be more informative for debugging purposes.
Consider this enhancement:
catchError((error) => { - const errorMessage: string = error.message; - throw new Error(errorMessage); + const errorMessage = `Failed to fetch archived notifications: ${error.message}`; + console.error(errorMessage, error); + throw new Error(errorMessage); }),apps/portal/src/app/views/notifications/notifications.component.ts (2)
344-344: Simplify conditional checks using optional chainingYou can simplify the conditional statements by using optional chaining, which enhances readability and conciseness.
Change:
if (notificationResponse && notificationResponse.notifications) {To:
if (notificationResponse?.notifications) {This change ensures that if
notificationResponseis null or undefined, the condition safely evaluates to false without accessingnotifications.Also applies to: 373-373
🧰 Tools
🪛 Biome (1.9.4)
[error] 344-344: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
328-384: Refactor to eliminate duplicated code in notification fetchingThe logic for fetching notifications and handling errors in both branches of the
ifstatement is largely duplicated. Consider refactoring to reduce code duplication by extracting the common functionality into a separate method or by parameterizing the differences.For example, you could create a private method like
fetchNotificationsthat accepts the fetch function as a parameter:private fetchNotifications( fetchFunction: (variables: any, token: string) => Observable<NotificationResponse | null>, variables: any, token: string ) { fetchFunction(variables, token) .pipe( 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; return of(null); }), ) .subscribe((notificationResponse: NotificationResponse | null) => { if (notificationResponse?.notifications) { // pagination is handled by p-table component of primeng this.notifications = notificationResponse.notifications; this.totalRecords = notificationResponse.total; } else { this.notifications = []; this.totalRecords = 0; } this.loading = false; }); }Then modify your
loadNotificationsLazymethod:if (this.archivedNotificationToggle) { this.fetchNotifications(this.notificationService.getArchivedNotifications.bind(this.notificationService), variables, loginToken); } else { this.fetchNotifications(this.notificationService.getNotifications.bind(this.notificationService), variables, loginToken); }This refactoring improves maintainability and reduces potential errors from having to update multiple places when changes are needed.
🧰 Tools
🪛 Biome (1.9.4)
[error] 344-344: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 373-373: 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 (4)
apps/portal/src/app/graphql/graphql.queries.ts(1 hunks)apps/portal/src/app/views/notifications/notification.model.ts(1 hunks)apps/portal/src/app/views/notifications/notifications.component.ts(2 hunks)apps/portal/src/app/views/notifications/notifications.service.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/portal/src/app/views/notifications/notifications.component.ts
[error] 344-344: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 373-373: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
apps/portal/src/app/graphql/graphql.queries.ts (1)
33-61: Verify GraphQL schema compatibility
Let's ensure the GraphQL schema properly defines these new types and that all queries using notification fields are consistent.
✅ Verification successful
Let me gather more specific information about the GraphQL schema and queries.
Let me check one more aspect to verify the schema and type definitions.
GraphQL schema and implementation are properly aligned
The GraphQL query GetArchivedNotifications is correctly implemented and matches the entity definitions:
- The entity
ArchivedNotificationin the backend defines all the fields used in the query - The interface
ArchivedNotificationin the frontend matches the query response structure - The service implementation properly handles the query response and maps it to the expected format
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other GraphQL queries using notification fields
# and verify schema compatibility
# Search for other GraphQL queries related to notifications
rg -l "notification" --type ts
# Search specifically for GraphQL type definitions
ast-grep --pattern 'type $_ {
$$$
notification
$$$
}'
Length of output: 4524
Script:
#!/bin/bash
# Search for GraphQL schema definitions and queries related to archived notifications
rg -A 10 "type ArchivedNotification" --type ts
rg -A 10 "input.*Notification" --type ts
rg "query.*archivedNotifications" --type ts
Length of output: 2952
Script:
#!/bin/bash
# Search for the ArchivedNotification entity definition and its usage
rg -A 15 "class ArchivedNotification" --type ts
rg -A 10 "interface.*ArchivedNotification" --type ts
Length of output: 6833
apps/portal/src/app/views/notifications/notifications.service.ts (2)
4-4: LGTM! Import statements are properly structured.
The new imports for archived notifications functionality are correctly added and follow Angular's style guide.
Also applies to: 6-6
52-82: Verify consistency between notification types.
The implementation looks good, but let's verify that both regular and archived notifications maintain consistent behavior.
✅ Verification successful
Let me fix the regex patterns and gather more information.
Let me check the regular notification handling to compare with the archived implementation.
Based on the gathered information, I can now provide the final response:
Implementation maintains consistency between regular and archived notifications
The verification confirms that both regular and archived notifications:
- Return the same
NotificationResponseinterface - Have matching field mappings (all essential fields are preserved)
- Handle errors consistently with the same error handling pattern
- Maintain consistent pagination structure (total, offset, limit)
The only difference is the expected field renaming from id to notificationId in archived notifications, which is correctly handled in the mapping.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency between regular and archived notification handling
# Search for notification-related GraphQL queries
echo "Checking GraphQL queries structure:"
rg -A 10 "query.*Notifications.*{"
# Search for notification type definitions
echo "Checking notification type definitions:"
rg -A 10 "type.*Notification.*{"
# Search for notification-related components
echo "Checking component implementations:"
rg -A 5 "notifications\.(archived)?notifications"
Length of output: 1283
Script:
#!/bin/bash
# Search for notification-related files and their content
echo "Checking notification service implementation:"
rg -l "NotificationService" | xargs cat
echo "Checking GraphQL query definitions:"
fd -e graphql | xargs cat
echo "Checking notification interfaces/types:"
rg -A 10 "interface.*Notification"
Length of output: 5017
Script:
#!/bin/bash
# Search for the regular notification handling implementation
echo "Checking regular notification handling:"
rg -A 20 "getNotifications.*Observable.*NotificationResponse"
# Search for any mapping between regular and archived notifications
echo "Checking notification mapping:"
rg -A 10 "map.*notification"
Length of output: 5439
apps/portal/src/app/views/notifications/notifications.component.ts (1)
98-99: Loading notifications after toggling the archive state
The code correctly reloads notifications when the archive toggle is activated or deactivated, ensuring the displayed notifications reflect the current selection.
apps/portal/src/app/views/notifications/notifications.service.ts
Outdated
Show resolved
Hide resolved
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.service.ts (1)
52-83: LGTM! Consider adding type safety to the mapping function.The implementation follows the established patterns and correctly handles null cases. Consider adding explicit type annotation for better type safety:
- notifications: archivedNotificationArray?.map((item) => ({ + notifications: archivedNotificationArray?.map((item: ArchivedNotification) => ({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/portal/src/app/views/notifications/notifications.service.ts(3 hunks)
🔇 Additional comments (2)
apps/portal/src/app/views/notifications/notifications.service.ts (2)
4-4: LGTM! Import statements are properly structured.
The new imports for archived notifications functionality are correctly added and follow Angular's style guide.
Also applies to: 6-6
17-24: LGTM! Interface definition is well-structured.
The new interface maintains consistency with the existing pattern and properly defines the expected response structure for archived notifications.
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 toggle to fetch from either notifications table or from archive table
Related changes:
ArchivedNotificationmodel to fetch archive datagetArchivedNotificationsNotificationScreenshots:
Video Demo
https://github.com/user-attachments/assets/edf0be80-8c34-45aa-bc39-e02347d9fef1
Active
Archived
Pending actions:
NA
Additional notes:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation