-
Notifications
You must be signed in to change notification settings - Fork 357
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
DSA Notifications #4376
DSA Notifications #4376
Conversation
allows us to resolve linked comments or dsa reports that might be connected to a notification through the graphql API.
allows us to use translation keys when creating the text for notifications.
shows as an example how notifications can have linked content
await ctx.notifications.create(ctx.tenant.id, ctx.tenant.locale, { | ||
targetUserID: comment.authorID!, | ||
comment, | ||
type: NotificationType.COMMENT_FEATURED, | ||
}); | ||
|
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.
@marcushaddon and @kabeaty, this is what it looks like to publish a notification for a user. All you need is access to the ctx.notifications
which is available for every query and mutation in Coral's API.
The content of the notification is also malleable and has many optional fields. It can also be extended to provide new interface types for various kinds of notifications. It's all very flexible to whatever needs to be inside the notification.
As well, there is a convenient NotificationType
to categorize the notifications and is infinite in how many kinds it can be, but adds a type that makes it easy to create/sort/filter notifications in the future.
This is what I mean by a one-liner
to publish a notification anywhere. This isn't relevant for either of your DSA work, but it shows how minimal it will be to send new notifications throughout both of your's code.
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.
Very nice initial framework for notifications to keep building on and extending!
show active bell icon when new notifications are available
this allows us to clear the notifications red dot when the user has viewed it and also in future, if we want to, hook it into a websocket or similar live update-able value.
f872d90
to
cd33c2b
Compare
# Conflicts: # server/src/core/server/models/dsaReport/report.ts
allows us to retain state of comment snapshotted at the time the notification was created.
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.
Echoing my previous review, fantastic framework for notifications set up here! Just left a few small comments/questions throughout.
client/src/core/client/stream/tabs/Notifications/NotificationContainer.css
Outdated
Show resolved
Hide resolved
client/src/core/client/ui/components/icons/ActiveNotificationBellIcon.tsx
Show resolved
Hide resolved
The `publicID` was changed to `referenceID` on the DSA report model. Update the report details model on the notifications to match.
What does this PR do?
Preliminarily adds notifications to the DSA epic branch.
These changes will impact:
What changes to the GraphQL/Database Schema does this PR introduce?
Notification
type and corresponding Connection typesDoes this PR introduce any new environment variables or feature flags?
No.
If any indexes were added, were they added to
INDEXES.md
?This index creates the uniqueness constraint for the
tenantID
andid
fields on the notifications collectionThis index speeds up the retrieval of notifications by
tenantID
,ownerID
, andcreatedAt
which is the most common way of retrieving notifications for pagination in the notifications tab on the stream.How do I test this PR?
Admin > Configure > General
Notifications
for updates on these actionsWhere any tests migrated to React Testing Library?
No
How do we deploy this PR?