feat: notification message templates #356
Conversation
WalkthroughAdds notification templates and messages: DB migrations, Rust repositories and gRPC server handlers, protobuf and generated JS/TS bindings, gRPC client/service wrappers, GraphQL types/queries/mutations, and API wiring to create/list/update/delete templates and create/list/update messages. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as GraphQL Client / Admin UI
participant API as GraphQL Server
participant Svc as JS NotificationsService
participant gRPC as gRPC Client
participant Rust as Notifications gRPC Server
participant DB as Postgres
Client->>API: GraphQL mutation/query (template/message create/list/update/delete)
API->>Svc: call msgTemplate*/msgMessage*(...)
Svc->>gRPC: unary RPC (MsgTemplate*/MsgMessage*Request)
gRPC->>Rust: invoke RPC handler
Rust->>DB: repository SQL (INSERT/UPDATE/DELETE/SELECT)
DB-->>Rust: rows
Rust-->>gRPC: proto response (MsgTemplate/MsgMessage)
gRPC-->>Svc: RPC result
Svc-->>API: mapped result or error
API-->>Client: GraphQL response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![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 (1)
core/notifications/migrations/20251209165958_create_msg_templates_and_msg_messages.up.sql (1)
3-11: Redundant index on unique constraint.In PostgreSQL, UNIQUE constraints automatically create an index on the constrained columns. The explicit index
idx_msg_templates_name_language_code(lines 21–22) duplicates the index created by the UNIQUE constraint (line 10), adding maintenance overhead without benefit.Remove the redundant index definition:
-CREATE INDEX idx_msg_templates_name_language_code - ON msg_templates (name, language_code); - CREATE INDEX idx_msg_messages_usernameAlso applies to: 21-22
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/notifications/migrations/20251209165958_create_msg_templates_and_msg_messages.down.sql(1 hunks)core/notifications/migrations/20251209165958_create_msg_templates_and_msg_messages.up.sql(1 hunks)
🔇 Additional comments (3)
core/notifications/migrations/20251209165958_create_msg_templates_and_msg_messages.up.sql (2)
13-19: Add foreign key constraints for referential integrity.The
usernameandsent_bycolumns appear to be user identifiers but lack foreign key constraints. Without referential integrity, orphaned records could accumulate if users are deleted, and it's unclear whether these should reference auserstable.Additionally, consider whether
msg_messagesshould referencemsg_templatesif messages are associated with specific templates.Please confirm the schema design:
- Should
usernameandsent_byhave foreign key constraints to auserstable?- Should
msg_messagesinclude a foreign key reference tomsg_templates?- Should
msg_messageshave acreated_attimestamp for audit purposes?If foreign keys are needed, consider adding them in this migration to ensure data consistency from the start.
24-28: Index choices are appropriate.The indexes on
usernameandstatusare well-chosen for filtering operations. These are not redundant and support common query patterns.core/notifications/migrations/20251209165958_create_msg_templates_and_msg_messages.down.sql (1)
3-8: Rollback logic is sound.The down migration correctly drops indexes before tables and uses
IF EXISTSfor idempotency. Once the redundant index in the up migration is removed, the down migration will continue to work correctly because theIF EXISTSclause safely handles the absence of that index.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
core/notifications/src/msg_messages/repo.rs (3)
8-15: Consider using an enum for the status field.The
statusfield is currently aString, which allows any arbitrary value. Consider defining aMessageStatusenum to ensure type safety and prevent invalid status values at compile time.Additionally, consider adding a
created_atfield to track when the message was originally created, distinct fromupdated_at.
23-29: Inconsistent constructor pattern across repositories.This constructor takes references and clones them (
&PgPool,&ReadPool), whileNotificationHistory::newtakes ownership directly (pool: PgPool). Consider standardizing the constructor pattern across all repositories for consistency.
73-135: Reduce code duplication in query construction.The method has four nearly identical branches that differ only in the presence of LIMIT/OFFSET clauses. Consider consolidating this using dynamic query building or a query builder pattern.
Example refactor using dynamic SQL construction:
pub async fn list_messages( &self, limit: Option<i64>, offset: Option<i64>, ) -> Result<Vec<MsgMessage>, sqlx::Error> { let mut query = String::from( "SELECT id, username, status, sent_by, updated_at FROM msg_messages ORDER BY updated_at DESC" ); let mut bindings = vec![]; if let Some(lim) = limit { query.push_str(&format!(" LIMIT ${}", bindings.len() + 1)); bindings.push(lim); } if let Some(off) = offset { query.push_str(&format!(" OFFSET ${}", bindings.len() + 1)); bindings.push(off); } let mut q = sqlx::query_as::<_, MsgMessage>(&query); for binding in bindings { q = q.bind(binding); } Ok(q.fetch_all(self.read_pool.inner()).await?) }core/notifications/src/msg_templates/repo.rs (2)
6-14: Consider stronger typing for language_code.The
language_codefield is aString, which allows any value. Consider using a validated type or enum to ensure only valid language codes are stored.
101-163: Reduce code duplication in query construction.This method has the same four-branch duplication pattern as
list_messagesin the msg_messages repository. Consider applying the same dynamic query building approach to consolidate the logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/notifications/src/app/mod.rs(5 hunks)core/notifications/src/lib.rs(1 hunks)core/notifications/src/msg_messages/mod.rs(1 hunks)core/notifications/src/msg_messages/repo.rs(1 hunks)core/notifications/src/msg_templates/mod.rs(1 hunks)core/notifications/src/msg_templates/repo.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/notifications/src/app/mod.rs (4)
core/notifications/src/msg_templates/repo.rs (1)
new(23-28)core/notifications/src/msg_messages/repo.rs (1)
new(24-29)core/notifications/src/history/repo.rs (1)
new(15-17)core/notifications/src/user_notification_settings/repo.rs (1)
new(17-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: execute via bats
- GitHub Check: execute via buck2
- GitHub Check: Migrate Mongodb
- GitHub Check: Check SDLs
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (16)
core/notifications/src/lib.rs (1)
10-11: LGTM!The module declarations follow the established pattern and correctly integrate the new msg_templates and msg_messages modules into the crate structure.
core/notifications/src/msg_templates/mod.rs (1)
1-3: LGTM!The module structure follows Rust best practices by declaring the repo submodule and re-exporting the public types for cleaner imports.
core/notifications/src/msg_messages/mod.rs (1)
1-3: LGTM!Consistent with the msg_templates module structure, correctly exposing MsgMessage and MsgMessageRepository.
core/notifications/src/msg_messages/repo.rs (2)
31-50: LGTM!The create_message method correctly uses parameterized queries and the RETURNING clause to retrieve the inserted row.
52-71: Verify error handling for non-existent message IDs.The
fetch_onecall returns aRowNotFounderror if the message ID doesn't exist. Confirm that callers properly handle this error case, or consider defining a custom error type (e.g.,MessageNotFound) for clearer error semantics.core/notifications/src/msg_templates/repo.rs (2)
30-53: LGTM!The create_template method correctly uses parameterized queries and returns the newly created template.
55-99: Verify error handling for non-existent template IDs.Both
update_templateanddelete_templateusefetch_one, which returns aRowNotFounderror if the template ID doesn't exist. Ensure that callers handle this appropriately—either by explicitly catchingsqlx::Error::RowNotFoundor by mapping to a domain-specific error type before returning to the API layer.core/notifications/src/app/mod.rs (9)
13-14: LGTM!The imports correctly bring in the new repository types needed for the messaging feature.
27-28: LGTM!The repository fields are correctly added to the NotificationsApp struct, following the established pattern for other repositories.
45-46: LGTM!The repositories are correctly instantiated with the pool and read_pool, consistent with other repository initialization in this method.
68-69: LGTM!The repositories are properly included in the NotificationsApp construction.
392-406: LGTM!The method correctly delegates to the repository and handles the result appropriately.
408-423: LGTM!The update method properly forwards all parameters to the repository.
425-432: LGTM!The delete method correctly delegates to the repository and returns the deleted ID.
448-460: LGTM!The message creation method correctly delegates to the repository.
462-473: LGTM!The status update method properly forwards the parameters to the repository.
| #[instrument(name = "app.msg_templates_list", skip(self), err)] | ||
| pub async fn list_msg_templates( | ||
| &self, | ||
| _language_code: Option<String>, | ||
| limit: Option<i64>, | ||
| offset: Option<i64>, | ||
| ) -> Result<Vec<crate::msg_templates::MsgTemplate>, ApplicationError> { | ||
| let templates = self | ||
| .msg_template_repository | ||
| .list_templates(limit, offset) | ||
| .await?; | ||
| Ok(templates) | ||
| } |
There was a problem hiding this comment.
Remove unused parameter or implement filtering.
The _language_code parameter is accepted but never used. This creates a misleading API where callers might expect filtering by language code, but no filtering occurs. Either:
- Remove the parameter entirely until filtering is implemented, or
- Implement the filtering logic in the repository layer
The underscore prefix indicates the parameter is intentionally unused, but this suggests incomplete functionality rather than a deliberate design choice.
If filtering should be implemented, the repository's list_templates method would need to be updated to accept and apply a language_code filter in the SQL WHERE clause.
🤖 Prompt for AI Agents
In core/notifications/src/app/mod.rs around lines 434 to 446, the _language_code
parameter is accepted but unused; either remove the parameter from the function
signature and any callers (and update the instrument attribute if needed) or
implement language filtering by: adding a language_code Option<String> argument
to the repository's list_templates method, propagate that parameter through this
method, update the repository signature and its SQL/query logic to include a
WHERE clause that filters by language_code when provided, and adjust any
unit/integration tests and call sites to pass or omit the language_code
accordingly.
| #[instrument(name = "app.msg_messages_list", skip(self), err)] | ||
| pub async fn list_msg_messages( | ||
| &self, | ||
| _username: Option<String>, | ||
| _status: Option<String>, | ||
| limit: Option<i64>, | ||
| offset: Option<i64>, | ||
| ) -> Result<Vec<crate::msg_messages::MsgMessage>, ApplicationError> { | ||
| let messages = self | ||
| .msg_message_repository | ||
| .list_messages(limit, offset) | ||
| .await?; | ||
| Ok(messages) | ||
| } |
There was a problem hiding this comment.
Remove unused parameters or implement filtering.
The _username and _status parameters are accepted but never used. This creates a misleading API where callers expect filtering capabilities that don't exist. Either:
- Remove these parameters until filtering is implemented, or
- Implement the filtering logic in the repository layer
The underscore prefixes indicate intentionally unused parameters, suggesting this is incomplete functionality that should be addressed before merging.
To implement filtering, the repository's list_messages method would need to accept these parameters and apply them in SQL WHERE clauses.
🤖 Prompt for AI Agents
In core/notifications/src/app/mod.rs around lines 475-488 the parameters
_username and _status are accepted but unused, creating a misleading API; either
remove them from the function signature (and update any callers/tests and the
instrument macro to reflect the new signature) or implement filtering by
extending the repository API to accept Option<String> username and status, pass
those values through from this method to msg_message_repository.list_messages,
and update the repository SQL/ORM query to apply WHERE clauses when the options
are Some(...) (plus adjust tests).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/api/src/services/notifications/proto/notifications.proto (1)
17-448: Remove duplicate proto definition or establish single source of truth.The file
core/api/src/services/notifications/proto/notifications.protois identical tocore/notifications/proto/notifications.proto. Maintaining duplicate proto definitions creates maintenance burden and risk of divergence. Consolidate to a single source and use proto imports if cross-service definitions are needed.However, note: there is a real type inconsistency in the
statusfield across message definitions that applies to both files—MsgMessage.statususes the enum typeMsgMessageStatus, whileMsgMessageCreateRequest.statusandMsgMessageUpdateStatusRequest.statususe the string type. This inconsistency should be resolved by standardizing on a single type.
🧹 Nitpick comments (8)
core/notifications/src/grpc/server/convert.rs (1)
363-379: Consider logging unknown status values instead of silently defaulting to Pending.The fallback to
Pendingfor unrecognized status strings (Line 369) could mask data integrity issues. If an unexpected status value appears, it will be silently converted toPendingwithout any indication of the problem.+use tracing::warn; + impl From<crate::msg_messages::MsgMessage> for proto::MsgMessage { fn from(message: crate::msg_messages::MsgMessage) -> Self { let status = match message.status.as_str() { "pending" => proto::MsgMessageStatus::Pending, "accepted" => proto::MsgMessageStatus::Accepted, "revoked" => proto::MsgMessageStatus::Revoked, - _ => proto::MsgMessageStatus::Pending, + unknown => { + warn!(status = %unknown, "Unknown MsgMessage status, defaulting to Pending"); + proto::MsgMessageStatus::Pending + } };core/api/src/graphql/admin/root/mutation/notification-template-delete.ts (1)
32-34: Inconsistent error handling for input validation errors.The input validation error returns a raw object with only
message, while service errors at line 41 usemapAndParseErrorForGqlResponsewhich providesmessage,path, andcode. This inconsistency may cause issues for clients expecting a uniform error structure.Consider aligning the error format or using a consistent helper function for all error responses.
core/api/src/graphql/admin/root/mutation/notification-message-update-status.ts (2)
10-10: Consider using an enum type forstatusinstead of a raw string.Using
GT.NonNull(GT.String)allows any arbitrary string value. If there's a defined set of valid message statuses (e.g., "pending", "sent", "delivered"), consider defining a GraphQL enum type to provide:
- Input validation at the GraphQL layer
- Self-documenting API with available options
- Better type safety
34-36: Inconsistent error handling for input validation.Same pattern as in
notification-template-delete.ts- the error object is missingpathandcodefields thatmapAndParseErrorForGqlResponseprovides at line 46.core/api/src/graphql/admin/root/mutation/notification-template-create.ts (1)
41-43: Inconsistent error handling for input validation.Same pattern as other mutations - the error object at line 42 lacks
pathandcodefields present inmapAndParseErrorForGqlResponseoutput at line 56. Consider standardizing the error format across all error responses.core/api/src/graphql/admin/root/mutation/notification-message-create.ts (1)
9-11: Consider adding validation for the status field.The
statusfield accepts any string value, but the domain typeMsgMessageStatusonly allows"pending","accepted", or"revoked". Invalid values will be silently converted to"pending"by the service layer (see line 81 in core/api/src/services/notifications/index.ts). Consider either:
- Creating a GraphQL enum type for
MsgMessageStatusand using it here for compile-time validation, or- Adding runtime validation in the resolver to reject invalid status values with a clear error message.
Example with GraphQL enum:
+const MsgMessageStatusEnum = GT.Enum({ + name: "MsgMessageStatus", + values: { + PENDING: { value: "pending" }, + ACCEPTED: { value: "accepted" }, + REVOKED: { value: "revoked" }, + }, +}) + const NotificationMessageCreateInput = GT.Input({ name: "NotificationMessageCreateInput", fields: () => ({ username: { type: GT.NonNull(GT.String) }, - status: { type: GT.NonNull(GT.String) }, + status: { type: GT.NonNull(MsgMessageStatusEnum) }, sentBy: { type: GT.NonNull(GT.String) }, }), })core/api/src/graphql/admin/root/mutation/notification-template-update.ts (1)
43-49: Consider consistent error handling pattern.The error handling for
idandlanguageCodevalidation (lines 43-49) returns errors with just amessagefield, while service errors (line 63) usemapAndParseErrorForGqlResponsewhich includescodeandpathfields. This inconsistency in error structure may complicate client-side error handling.Consider wrapping validation errors through a similar mapper for consistency:
if (id instanceof Error) { - return { errors: [{ message: id.message }], success: false } + return { errors: [mapAndParseErrorForGqlResponse(id)], success: false } } if (languageCode instanceof Error) { - return { errors: [{ message: languageCode.message }], success: false } + return { errors: [mapAndParseErrorForGqlResponse(languageCode)], success: false } }core/notifications/proto/notifications.proto (1)
356-360: Follow protobuf enum naming conventions.The enum values should be prefixed with the enum name, and the zero value should represent an unspecified/unknown state for forward compatibility.
Apply this diff to align with protobuf style guide:
enum MsgMessageStatus { - PENDING = 0; - ACCEPTED = 1; - REVOKED = 2; + MSG_MESSAGE_STATUS_UNSPECIFIED = 0; + MSG_MESSAGE_STATUS_PENDING = 1; + MSG_MESSAGE_STATUS_ACCEPTED = 2; + MSG_MESSAGE_STATUS_REVOKED = 3; }As per static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
core/api/src/domain/notifications/index.types.d.ts(2 hunks)core/api/src/graphql/admin/mutations.ts(2 hunks)core/api/src/graphql/admin/queries.ts(2 hunks)core/api/src/graphql/admin/root/mutation/notification-message-create.ts(1 hunks)core/api/src/graphql/admin/root/mutation/notification-message-update-status.ts(1 hunks)core/api/src/graphql/admin/root/mutation/notification-template-create.ts(1 hunks)core/api/src/graphql/admin/root/mutation/notification-template-delete.ts(1 hunks)core/api/src/graphql/admin/root/mutation/notification-template-update.ts(1 hunks)core/api/src/graphql/admin/root/query/notification-messages.ts(1 hunks)core/api/src/graphql/admin/root/query/notification-templates.ts(1 hunks)core/api/src/graphql/admin/types/object/notification-message.ts(1 hunks)core/api/src/graphql/admin/types/object/notification-template.ts(1 hunks)core/api/src/services/notifications/grpc-client.ts(2 hunks)core/api/src/services/notifications/index.ts(4 hunks)core/api/src/services/notifications/proto/notifications.proto(2 hunks)core/api/src/services/notifications/proto/notifications_grpc_pb.d.ts(5 hunks)core/api/src/services/notifications/proto/notifications_grpc_pb.js(2 hunks)core/api/src/services/notifications/proto/notifications_pb.d.ts(2 hunks)core/notifications/proto/notifications.proto(2 hunks)core/notifications/src/grpc/server/convert.rs(1 hunks)core/notifications/src/grpc/server/mod.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
core/notifications/src/grpc/server/convert.rs (1)
core/api/src/services/notifications/proto/notifications_pb.d.ts (2)
MsgTemplate(1031-1053)MsgMessage(1066-1086)
core/api/src/services/notifications/proto/notifications_grpc_pb.d.ts (1)
core/api/src/services/notifications/proto/notifications_pb.d.ts (14)
MsgTemplateCreateRequest(1098-1118)MsgTemplateCreateResponse(1130-1145)MsgTemplateUpdateRequest(1153-1175)MsgTemplateUpdateResponse(1188-1203)MsgTemplateDeleteRequest(1211-1223)MsgTemplateDeleteResponse(1231-1243)MsgTemplatesListRequest(1251-1267)MsgTemplatesListResponse(1277-1291)MsgMessageCreateRequest(1299-1315)MsgMessageCreateResponse(1325-1340)MsgMessageUpdateStatusRequest(1348-1362)MsgMessageUpdateStatusResponse(1371-1386)MsgMessagesListRequest(1394-1408)MsgMessagesListResponse(1417-1431)
core/api/src/graphql/admin/root/mutation/notification-message-create.ts (2)
core/api/src/services/notifications/index.ts (1)
NotificationsService(93-971)core/api/src/graphql/error-map.ts (1)
mapAndParseErrorForGqlResponse(917-924)
core/api/src/graphql/admin/root/query/notification-messages.ts (2)
core/api/src/services/notifications/index.ts (1)
NotificationsService(93-971)core/api/src/graphql/error-map.ts (1)
mapAndParseErrorForGqlResponse(917-924)
core/api/src/graphql/admin/root/query/notification-templates.ts (2)
core/api/src/services/notifications/index.ts (1)
NotificationsService(93-971)core/api/src/graphql/error-map.ts (1)
mapAndParseErrorForGqlResponse(917-924)
core/api/src/domain/notifications/index.types.d.ts (3)
core/api/src/services/notifications/proto/notifications_pb.d.ts (2)
MsgTemplate(1031-1053)MsgMessage(1066-1086)core/api/src/services/notifications/grpc-client.ts (7)
msgTemplateCreate(124-128)msgTemplateUpdate(130-134)msgTemplateDelete(136-140)msgTemplatesList(142-146)msgMessageCreate(148-152)msgMessageUpdateStatus(154-158)msgMessagesList(160-164)core/api/src/domain/notifications/errors.ts (1)
NotificationsServiceError(9-9)
core/api/src/services/notifications/proto/notifications_grpc_pb.js (1)
core/api/src/services/notifications/proto/notifications_grpc_pb.d.ts (1)
NotificationsServiceService(194-194)
core/notifications/src/grpc/server/mod.rs (2)
core/notifications/src/app/mod.rs (5)
msg_template_create(393-406)msg_template_update(409-423)msg_template_delete(426-432)msg_message_create(449-460)msg_message_update_status(463-473)core/notifications/src/grpc/server/convert.rs (15)
from(10-20)from(24-28)from(32-41)from(45-62)from(66-79)from(83-88)from(92-97)from(101-122)from(126-150)from(178-183)from(187-229)from(233-245)from(291-347)from(351-360)from(364-379)
core/api/src/services/notifications/grpc-client.ts (1)
core/api/src/services/notifications/proto/notifications_pb.d.ts (14)
MsgTemplateCreateRequest(1098-1118)MsgTemplateCreateResponse(1130-1145)MsgTemplateUpdateRequest(1153-1175)MsgTemplateUpdateResponse(1188-1203)MsgTemplateDeleteRequest(1211-1223)MsgTemplateDeleteResponse(1231-1243)MsgTemplatesListRequest(1251-1267)MsgTemplatesListResponse(1277-1291)MsgMessageCreateRequest(1299-1315)MsgMessageCreateResponse(1325-1340)MsgMessageUpdateStatusRequest(1348-1362)MsgMessageUpdateStatusResponse(1371-1386)MsgMessagesListRequest(1394-1408)MsgMessagesListResponse(1417-1431)
🪛 Buf (1.61.0)
core/notifications/proto/notifications.proto
357-357: Enum value name "PENDING" should be prefixed with "MSG_MESSAGE_STATUS_".
(ENUM_VALUE_PREFIX)
357-357: Enum zero value name "PENDING" should be suffixed with "_UNSPECIFIED".
(ENUM_ZERO_VALUE_SUFFIX)
358-358: Enum value name "ACCEPTED" should be prefixed with "MSG_MESSAGE_STATUS_".
(ENUM_VALUE_PREFIX)
359-359: Enum value name "REVOKED" should be prefixed with "MSG_MESSAGE_STATUS_".
(ENUM_VALUE_PREFIX)
core/api/src/services/notifications/proto/notifications.proto
357-357: Enum value name "PENDING" should be prefixed with "MSG_MESSAGE_STATUS_".
(ENUM_VALUE_PREFIX)
357-357: Enum zero value name "PENDING" should be suffixed with "_UNSPECIFIED".
(ENUM_ZERO_VALUE_SUFFIX)
358-358: Enum value name "ACCEPTED" should be prefixed with "MSG_MESSAGE_STATUS_".
(ENUM_VALUE_PREFIX)
359-359: Enum value name "REVOKED" should be prefixed with "MSG_MESSAGE_STATUS_".
(ENUM_VALUE_PREFIX)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Migrate Mongodb
- GitHub Check: Check SDLs
- GitHub Check: execute via bats
- GitHub Check: execute via buck2
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (25)
core/api/src/graphql/admin/types/object/notification-message.ts (1)
1-31: LGTM!The GraphQL object type definition is clean and correctly defines all required fields with appropriate types. The
Timestampscalar usage forupdatedAtaligns with the proto definition.core/api/src/services/notifications/proto/notifications_grpc_pb.js (1)
1-605: Generated code - no review required.This file is auto-generated from protobuf definitions (as indicated by the header comment). The new RPC definitions and serialization handlers follow the established patterns consistently.
core/api/src/graphql/admin/queries.ts (1)
16-17: LGTM!The new notification queries are properly imported and added to the authenticated query fields, following the established patterns for admin queries.
Also applies to: 39-40
core/api/src/graphql/admin/types/object/notification-template.ts (1)
1-35: LGTM!The GraphQL object type correctly defines all template fields with appropriate types. Using the
Languagescalar forlanguageCodeensures proper validation of language codes.core/notifications/src/grpc/server/convert.rs (1)
350-361: LGTM!The
MsgTemplateconversion is straightforward with clean field mapping.core/api/src/graphql/admin/root/mutation/notification-template-delete.ts (1)
36-48: LGTM!The service call and success response handling follow the expected patterns for admin mutations.
core/api/src/graphql/admin/root/query/notification-templates.ts (2)
25-27: Throwing a plain object instead of an Error instance.
mapAndParseErrorForGqlResponsereturns a plainIErrorobject{ message, path, code }, not an Error instance. Throwing this directly may cause unexpected behavior in GraphQL error handling or logging since stack traces won't be captured.Consider wrapping in a proper Error or using GraphQL's error utilities:
if (args.languageCode instanceof Error) { - throw mapAndParseErrorForGqlResponse(args.languageCode) + const mapped = mapAndParseErrorForGqlResponse(args.languageCode) + throw new Error(mapped.message) }Alternatively, verify this is the intended pattern used elsewhere in the codebase for query resolvers.
37-47: LGTM!The service integration and response handling are implemented correctly. The pagination parameters are properly forwarded to the service layer.
core/api/src/graphql/admin/root/mutation/notification-message-update-status.ts (1)
38-52: LGTM!Service instantiation, method call, and response handling follow the established mutation patterns correctly.
core/api/src/graphql/admin/root/query/notification-messages.ts (2)
22-28: Same pattern concern: throwing plain objects.Same observation as in
notification-templates.ts-mapAndParseErrorForGqlResponsereturns a plain object, not an Error instance. Verify this is the intended pattern for query error handling.
30-39: LGTM!The service call and response handling are correctly implemented with proper error checking and pagination support.
core/api/src/graphql/admin/root/mutation/notification-template-create.ts (1)
45-62: LGTM!The service call correctly forwards all template fields, and the error/success response handling follows the established mutation patterns.
core/api/src/services/notifications/proto/notifications_grpc_pb.d.ts (1)
22-28: LGTM! Generated gRPC definitions.The TypeScript declarations for the new gRPC methods are correctly generated and follow the standard gRPC patterns. All seven new RPCs (template and message operations) are properly defined with their request/response types.
Also applies to: 130-192, 208-214, 251-271, 309-329
core/api/src/domain/notifications/index.types.d.ts (1)
84-101: LGTM! Domain types properly structured.The
MsgMessageStatus,MsgTemplate, andMsgMessagetypes are well-defined. The service methods acceptstatus: MsgMessageStatus | stringwhich provides flexibility while offering type hints for the expected values ("pending","accepted","revoked"). The service layer handles conversion and defaults invalid values appropriately.Also applies to: 186-200
core/notifications/src/grpc/server/mod.rs (2)
101-126: LGTM! Template handlers are well-implemented.The template CRUD handlers properly:
- Parse and validate UUIDs with clear error messages (lines 136-137, 167-168)
- Handle optional language_code by checking for empty strings (lines 191-195)
- Apply tracing instrumentation
- Convert domain objects to proto types
Also applies to: 128-157, 159-181, 183-211
213-232: LGTM! Message handlers follow consistent patterns.The message handlers are well-structured with:
- Proper UUID validation for updates (lines 242-243)
- Consistent error handling and tracing
- Clean conversion from domain to proto types
Note: Line 271 passes
Nonefor the first two filter parameters inlist_msg_messages. These appear to be reserved for future username/status filtering but aren't currently exposed through the API.Also applies to: 234-256, 258-280
core/api/src/services/notifications/index.ts (3)
72-91: LGTM! Status conversion functions are correctly implemented.The bidirectional conversion between
MsgMessageStatusdomain type and proto enum properly handles all three states (pending, accepted, revoked) with a safe default to pending for unknown values.
737-767: LGTM! Template and message service methods are well-structured.All methods follow consistent patterns:
- Proper request construction with proto types
- Correct handling of optional parameters (languageCode, limit, offset)
- Proper error handling via handleCommonNotificationErrors
- Clean mapping of responses to domain types
The template and message list methods correctly handle pagination parameters by checking if they're provided before setting them on the request.
Also applies to: 769-802, 804-822, 824-857, 908-937
859-883: Fix proto definition for MsgMessageCreateRequest and MsgMessageUpdateStatusRequest status field type.The proto definition defines
MsgMessageCreateRequest.statusandMsgMessageUpdateStatusRequest.statusasstringtype, whilemsgMessageStatusToProto()returns aProtoMsgMessageStatusenum andMsgMessage.statusis typed asMsgMessageStatusenum. The double-castas unknown as stringat lines 871 and 897 is necessary to work around this mismatch. Update the proto definitions to useMsgMessageStatus status = 2instead ofstring status = 2in both request messages to align with the actual data type and eliminate the unsafe cast.core/api/src/graphql/admin/mutations.ts (1)
12-16: LGTM! Mutations properly wired.The new notification template and message mutations are correctly imported and integrated into the authed mutations object, following the established pattern.
Also applies to: 31-35
core/api/src/services/notifications/grpc-client.ts (1)
30-43: LGTM! gRPC client methods properly promisified.All seven new client methods (msgTemplateCreate, msgTemplateUpdate, msgTemplateDelete, msgTemplatesList, msgMessageCreate, msgMessageUpdateStatus, msgMessagesList) follow the consistent promisification pattern and are correctly typed with their request/response pairs.
Also applies to: 124-164
core/notifications/proto/notifications.proto (3)
17-23: LGTM! RPC declarations follow standard gRPC patterns.The new RPC methods for template and message management follow consistent naming conventions and standard CRUD patterns.
362-369: LGTM! MsgTemplate message structure is appropriate.The template fields provide sufficient data for localized notification templates.
371-377: Verify missing fields in MsgMessage.The message structure lacks:
- A
created_attimestamp (onlyupdated_atis present)- A
template_idfield to link messages to templatesEnsure these omissions are intentional and align with the feature requirements.
core/api/src/services/notifications/proto/notifications_pb.d.ts (1)
1031-1573: Generated file reflects proto definition issues.This TypeScript declaration file is generated from the proto definitions. The type inconsistencies observed here (e.g.,
statusasstringin lines 1302, 1351 vs.MsgMessageStatusenum in line 1092) reflect issues in the source proto files.Fix the proto definitions in
core/notifications/proto/notifications.protoandcore/api/src/services/notifications/proto/notifications.proto, then regenerate this file.
| message MsgMessageCreateRequest { | ||
| string username = 1; | ||
| string status = 2; | ||
| string sent_by = 3; | ||
| } |
There was a problem hiding this comment.
Fix type inconsistency for status field.
The status field is defined as string here but as MsgMessageStatus enum in MsgMessage (line 374). This inconsistency will cause type mismatches in serialization and validation.
Apply this diff:
message MsgMessageCreateRequest {
string username = 1;
- string status = 2;
+ MsgMessageStatus status = 2;
string sent_by = 3;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| message MsgMessageCreateRequest { | |
| string username = 1; | |
| string status = 2; | |
| string sent_by = 3; | |
| } | |
| message MsgMessageCreateRequest { | |
| string username = 1; | |
| MsgMessageStatus status = 2; | |
| string sent_by = 3; | |
| } |
🤖 Prompt for AI Agents
In core/notifications/proto/notifications.proto around lines 422-426, the
MsgMessageCreateRequest.message has status typed as string which conflicts with
MsgMessageStatus enum used by MsgMessage (around line 374); change the field
type from string to MsgMessageStatus (keeping field number 2) so the request
uses the same enum type, then regenerate protobuf artifacts/stubs to pick up the
type change.
| message MsgMessageUpdateStatusRequest { | ||
| string id = 1; | ||
| string status = 2; | ||
| } |
There was a problem hiding this comment.
Fix type inconsistency for status field.
Same issue as MsgMessageCreateRequest: the status field should be MsgMessageStatus enum, not string.
Apply this diff:
message MsgMessageUpdateStatusRequest {
string id = 1;
- string status = 2;
+ MsgMessageStatus status = 2;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| message MsgMessageUpdateStatusRequest { | |
| string id = 1; | |
| string status = 2; | |
| } | |
| message MsgMessageUpdateStatusRequest { | |
| string id = 1; | |
| MsgMessageStatus status = 2; | |
| } |
🤖 Prompt for AI Agents
In core/notifications/proto/notifications.proto around lines 432 to 435, the
MsgMessageUpdateStatusRequest currently defines status as a string; change the
field type to the MsgMessageStatus enum (e.g., "MsgMessageStatus status = 2;")
so it matches MsgMessageCreateRequest; ensure the MsgMessageStatus enum is
available/imported in this file or the same proto and update any
references/clients/IDL consumers if necessary to use the enum type instead of a
string.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
core/api/src/services/notifications/proto/notifications.proto (1)
17-23: Proto surface is coherent; revisitstatustyping and Buf enum naming, and keep both protos in sync.The new RPCs and message types here look consistent with the server-side proto and with the higher-level TS types.
Two follow-ups (same concerns as on the server proto):
statusasstringvsMsgMessageStatusenum
MsgMessage.statususesMsgMessageStatus, but bothMsgMessageCreateRequest.statusandMsgMessageUpdateStatusRequest.statusare plainstring. That’s legal but bypasses schema-level validation and IDE support.If you prefer stronger typing (and to keep request/response aligned), consider switching these fields to the enum:
message MsgMessageCreateRequest { string username = 1;
string status = 2;
MsgMessageStatus status = 2; string sent_by = 3;}
message MsgMessageUpdateStatusRequest {
string id = 1;
string status = 2;
MsgMessageStatus status = 2;If the intent is to allow arbitrary statuses, adding a short comment to that effect would help future readers.
Buf enum naming hints
Buf is flagging
MsgMessageStatusvalues (PENDING,ACCEPTED,REVOKED) for not following the configured prefix/zero-value conventions. If Buf lint is enforced, you may need either to:
Rename the enum values, e.g.:
-enum MsgMessageStatus { - PENDING = 0; - ACCEPTED = 1; - REVOKED = 2; -} +enum MsgMessageStatus { + MSG_MESSAGE_STATUS_UNSPECIFIED = 0; + MSG_MESSAGE_STATUS_PENDING = 1; + MSG_MESSAGE_STATUS_ACCEPTED = 2; + MSG_MESSAGE_STATUS_REVOKED = 3; +}(and update generated consumers), or
Relax/override the Buf lint rule for this enum to match the rest of the file’s style.
Any change you make here should be mirrored in
core/notifications/proto/notifications.protoand followed by regenerating the TS/JS bindings.Also applies to: 355-457
core/notifications/proto/notifications.proto (1)
17-23: Server proto matches the API proto; alignstatustyping and decide on Buf enum naming.This file mirrors the API proto, so the same two points apply as already raised in earlier review comments:
statusfield type in create/update requestsPrior feedback already suggested using
MsgMessageStatusinstead ofstringforMsgMessageCreateRequest.statusandMsgMessageUpdateStatusRequest.status. If you want proto-level validation and consistent typing withMsgMessage.status, the diff would be:message MsgMessageCreateRequest { string username = 1;
string status = 2;
MsgMessageStatus status = 2; string sent_by = 3;}
message MsgMessageUpdateStatusRequest {
string id = 1;
string status = 2;
MsgMessageStatus status = 2;If you intentionally kept these as `string` for flexibility, consider documenting that choice so it’s clear this is not an oversight, and ensure the Rust app layer validates/normalizes the values before persisting.
Buf enum naming for
MsgMessageStatusBuf is warning about the value names and zero value. As with the API proto, you’ll either need to rename the enum values to satisfy your lint rules or adjust the Buf config/exception list. Any renaming here must also be reflected in:
core/api/src/services/notifications/proto/notifications.proto- Generated bindings (Rust/TS)
- Any domain/DB mapping that depends on these constants.
Also applies to: 355-457
🧹 Nitpick comments (8)
core/notifications/migrations/20251209165958_create_msg_templates_and_msg_messages.up.sql (1)
16-22: Add index forupdated_atto match query ORDER BY
msg_messagescurrently has indexes onusernameandstatus, butlist_messagesorders byupdated_at DESC. For growing tables this will require sorting the whole result set. Consider adding an index onupdated_at(or a composite index combining your usual filters withupdated_at) to keep pagination performant.Also applies to: 27-31
core/api/src/services/notifications/proto/notifications_pb.d.ts (1)
1075-1105: Align message status typing between requests, entity, and DB
MsgMessage.statusis strongly typed asMsgMessageStatus, but bothMsgMessageCreateRequest.statusandMsgMessageUpdateStatusRequest.statusare plainstring, while the DB column has a CHECK constraint restricting values to'pending' | 'accepted' | 'revoked'.This makes it easy for clients to send invalid values that only fail at the DB layer. If feasible, consider:
- Changing the proto for the request messages to use
MsgMessageStatusas well (and regenerating), or- At least constraining the higher-level API (e.g., GraphQL) to an enum that maps to
MsgMessageStatus, so only valid statuses reach the backend.This would keep the status domain consistent across layers.
Also applies to: 1326-1350, 1375-1396, 1596-1600
core/notifications/src/grpc/server/convert.rs (1)
350-382: Tighten status mapping inFrom<MsgMessage> for proto::MsgMessageThe mapping from
message.statustoproto::MsgMessageStatuscovers"pending" | "accepted" | "revoked"and falls back toPendingon anything else:let status = match message.status.as_str() { "pending" => proto::MsgMessageStatus::Pending, "accepted" => proto::MsgMessageStatus::Accepted, "revoked" => proto::MsgMessageStatus::Revoked, _ => proto::MsgMessageStatus::Pending, };Given the DB CHECK constraint, unknown values shouldn’t occur today, but silently coercing them to
Pendingwould hide data issues if new statuses are introduced or invariants are broken elsewhere. Consider logging and/or treating unknown values as an error instead of defaulting, so mismatches are visible during development and ops.(Assuming the proto
updated_atfield is “seconds since epoch,” usingupdated_at.timestamp()is appropriate.)core/api/src/graphql/admin/root/mutation/notification-template-create.ts (1)
7-82: Create mutation matches DB/service contract; only a minor error-shape nitThe input shape and nullability line up with the
msg_templatesschema and themsgTemplateCreateservice, and the resolver wiring/complexity look good. For invalidlanguageCode, you currently return{ message }directly instead of usingmapAndParseErrorForGqlResponse, so parse failures won’t carry acode/pathlike service errors do. That’s a small inconsistency but not a blocker; consider using a consistent error shape if you care about clients inspecting error codes.core/notifications/src/msg_messages/repo.rs (2)
7-14: Use a typed status enum instead of rawString
MsgMessage.statusis aString, but themsg_messages.statuscolumn is constrained to'pending' | 'accepted' | 'revoked', and the proto layer exposes aMsgMessageStatusenum. Using a raw string here means invalid statuses can be constructed all the way down to the DB before being rejected.Consider introducing a small Rust enum for message status (with
FromStr/Displayor similar) and using that inMsgMessageand the repository APIs. That will enforce valid values at the type level and keep the DB constraint as a final safeguard rather than the first line of defense.Also applies to: 30-70
72-134: Defensively handle non-positivelimit/offsetinlist_messages
list_messagestakesOption<i64>forlimit/offsetand passes any provided values straight intoLIMIT/OFFSET. If upstream ever supplies negative values, Postgres will error. It’s minor, but you may want to:
- Validate these in the app layer, or
- Treat
limit <= 0/offset < 0asNonehere before building the query.That avoids surprising SQL errors from malformed client input.
core/notifications/src/grpc/server/mod.rs (1)
13-13: New template/message RPC handlers are consistent; consider pagination bounds.The new
msg_template_*andmsg_message_*RPCs follow existing patterns (tracing,into_inner, UUID parsing,Status::frommapping, andFromconversions), and the UUID handling plus invalid-argument mapping look good.One thing to consider:
msg_templates_listandmsg_messages_listpasslimitthrough directly when> 0. If the app layer doesn’t already clamp this, it might be worth enforcing a reasonable maximum here to avoid accidentally unbounded queries from a misbehaving client.Also applies to: 101-287
core/api/src/domain/notifications/index.types.d.ts (1)
84-105: Types match proto well; consider tighteningstatustyping and clarifying defaults.The
MsgTemplate/MsgMessageshapes and the newINotificationsServicemethods line up with the proto definitions and generated bindings, which is good.Two minor points:
- Both
msgMessageCreateandmsgMessageUpdateStatusacceptstatusasMsgMessageStatus | string. That gives up most of the benefit of definingMsgMessageStatus; if you don’t expect arbitrary values here, consider narrowing toMsgMessageStatus(and possiblystatus?: MsgMessageStatusformsgMessageCreate) to get better compile-time checks.msgMessageCreatemakesstatusoptional, but the proto request field is a requiredstring. Please double-check that the service layer sets a sensible default (e.g."pending") rather than letting an empty string fall through to gRPC.Example tightening (if desired):
-type MsgMessageStatus = "pending" | "accepted" | "revoked" +type MsgMessageStatus = "pending" | "accepted" | "revoked" msgMessageCreate(args: { username: string - status?: MsgMessageStatus | string + status?: MsgMessageStatus sentBy: string }): Promise<true | NotificationsServiceError> msgMessageUpdateStatus(args: { id: string - status: MsgMessageStatus | string + status: MsgMessageStatus }): Promise<true | NotificationsServiceError>Also applies to: 164-209
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
core/api/src/domain/notifications/index.types.d.ts(2 hunks)core/api/src/graphql/admin/root/mutation/notification-message-create.ts(1 hunks)core/api/src/graphql/admin/root/mutation/notification-template-create.ts(1 hunks)core/api/src/graphql/admin/root/mutation/notification-template-update.ts(1 hunks)core/api/src/graphql/admin/types/object/notification-template.ts(1 hunks)core/api/src/services/notifications/index.ts(3 hunks)core/api/src/services/notifications/proto/notifications.proto(2 hunks)core/api/src/services/notifications/proto/notifications_pb.d.ts(2 hunks)core/notifications/migrations/20251209165958_create_msg_templates_and_msg_messages.up.sql(1 hunks)core/notifications/proto/notifications.proto(2 hunks)core/notifications/src/app/mod.rs(5 hunks)core/notifications/src/grpc/server/convert.rs(1 hunks)core/notifications/src/grpc/server/mod.rs(2 hunks)core/notifications/src/msg_messages/repo.rs(1 hunks)core/notifications/src/msg_templates/repo.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- core/api/src/graphql/admin/types/object/notification-template.ts
- core/notifications/src/app/mod.rs
- core/api/src/services/notifications/index.ts
🧰 Additional context used
🧬 Code graph analysis (4)
core/api/src/graphql/admin/root/mutation/notification-template-update.ts (2)
core/api/src/services/notifications/index.ts (1)
NotificationsService(71-989)core/api/src/graphql/error-map.ts (1)
mapAndParseErrorForGqlResponse(917-924)
core/api/src/graphql/admin/root/mutation/notification-template-create.ts (2)
core/api/src/services/notifications/index.ts (1)
NotificationsService(71-989)core/api/src/graphql/error-map.ts (1)
mapAndParseErrorForGqlResponse(917-924)
core/notifications/src/msg_templates/repo.rs (1)
core/notifications/src/msg_messages/repo.rs (7)
sqlx(36-36)sqlx(56-56)sqlx(79-79)sqlx(94-94)sqlx(108-108)sqlx(122-122)new(23-28)
core/api/src/domain/notifications/index.types.d.ts (3)
core/api/src/services/notifications/proto/notifications_pb.d.ts (2)
MsgTemplate(1031-1059)MsgMessage(1075-1095)core/api/src/services/notifications/grpc-client.ts (7)
msgTemplateCreate(124-128)msgTemplateUpdate(130-134)msgTemplateDelete(136-140)msgTemplatesList(142-146)msgMessageCreate(148-152)msgMessageUpdateStatus(154-158)msgMessagesList(160-164)core/api/src/domain/notifications/errors.ts (1)
NotificationsServiceError(9-9)
🪛 Buf (1.61.0)
core/api/src/services/notifications/proto/notifications.proto
357-357: Enum value name "PENDING" should be prefixed with "MSG_MESSAGE_STATUS_".
(ENUM_VALUE_PREFIX)
357-357: Enum zero value name "PENDING" should be suffixed with "_UNSPECIFIED".
(ENUM_ZERO_VALUE_SUFFIX)
358-358: Enum value name "ACCEPTED" should be prefixed with "MSG_MESSAGE_STATUS_".
(ENUM_VALUE_PREFIX)
359-359: Enum value name "REVOKED" should be prefixed with "MSG_MESSAGE_STATUS_".
(ENUM_VALUE_PREFIX)
core/notifications/proto/notifications.proto
357-357: Enum value name "PENDING" should be prefixed with "MSG_MESSAGE_STATUS_".
(ENUM_VALUE_PREFIX)
357-357: Enum zero value name "PENDING" should be suffixed with "_UNSPECIFIED".
(ENUM_ZERO_VALUE_SUFFIX)
358-358: Enum value name "ACCEPTED" should be prefixed with "MSG_MESSAGE_STATUS_".
(ENUM_VALUE_PREFIX)
359-359: Enum value name "REVOKED" should be prefixed with "MSG_MESSAGE_STATUS_".
(ENUM_VALUE_PREFIX)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build and test core
- GitHub Check: Migrate Mongodb
- GitHub Check: Check SDLs
- GitHub Check: execute via bats
- GitHub Check: execute via buck2
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
core/api/src/graphql/admin/root/mutation/notification-template-update.ts (1)
7-90: Update mutation wiring looks consistent and correctInput fields, nullability, and the resolver’s call into
NotificationsService.msgTemplateUpdateall mirror the create mutation and the underlying repository/DB schema. Error handling forid/languageCodeand service errors is consistent with the existing GraphQL patterns. No changes requested.
| const NotificationMessageCreateInput = GT.Input({ | ||
| name: "NotificationMessageCreateInput", | ||
| fields: () => ({ | ||
| username: { type: GT.NonNull(GT.String) }, | ||
| status: { type: GT.String }, | ||
| sentBy: { type: GT.NonNull(GT.String) }, | ||
| }), | ||
| }) | ||
|
|
||
| const NotificationMessageCreateMutation = GT.Field< | ||
| null, | ||
| GraphQLAdminContext, | ||
| { | ||
| input: { | ||
| username: string | ||
| status?: string | ||
| sentBy: string | ||
| } | ||
| } | ||
| >({ | ||
| extensions: { | ||
| complexity: 40, | ||
| }, | ||
| type: GT.NonNull(SuccessPayload), | ||
| args: { | ||
| input: { type: GT.NonNull(NotificationMessageCreateInput) }, | ||
| }, | ||
| resolve: async (_, args) => { | ||
| const { username, status, sentBy } = args.input | ||
| const notificationsService = NotificationsService() | ||
|
|
||
| const res = await notificationsService.msgMessageCreate({ | ||
| username, | ||
| status, | ||
| sentBy, | ||
| }) | ||
|
|
||
| if (res instanceof Error) { | ||
| return { errors: [mapAndParseErrorForGqlResponse(res)], success: false } | ||
| } | ||
|
|
||
| return { | ||
| errors: [], | ||
| success: true, | ||
| } | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Constrain status input to valid values and define a clear default
NotificationMessageCreateInput exposes status as an optional String, and the resolver forwards it directly into msgMessageCreate. Downstream:
- The DB column
msg_messages.statusisNOT NULLwith a CHECK on'pending' | 'accepted' | 'revoked'. - The proto
MsgMessageuses aMsgMessageStatusenum for the stored status.
Without validation here, callers can send arbitrary strings (e.g., "Pending" or "foo") and only see a DB error. When status is omitted, behavior also depends on how msgMessageCreate interprets the missing field vs the DB default.
I’d recommend either:
- Making
statusa GraphQL enum aligned with the allowed values (and the proto enum), optionally defaulting to"pending"when omitted, or - At minimum, validating
statusagainst the allowed set before calling the service and returning a clear GraphQL error if it’s invalid.
This keeps invariants at the API edge and avoids opaque DB constraint failures.
🤖 Prompt for AI Agents
In core/api/src/graphql/admin/root/mutation/notification-message-create.ts
around lines 6 to 52, the NotificationMessageCreateInput currently exposes
status as an optional free-form String which allows invalid values and relies on
downstream DB/proto constraints; change the input to use a GraphQL enum that
mirrors the MsgMessageStatus values (pending, accepted, revoked) and make the
field either non-null with a default of "pending" or keep it optional but
validate in the resolver; in the resolver, coerce or default missing status to
"pending", and if a provided status is not one of the enum values return a clear
GraphQL error (mapped via mapAndParseErrorForGqlResponse or a similar error
payload) instead of forwarding invalid strings to
notificationsService.msgMessageCreate.
| CREATE TABLE msg_templates ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| name VARCHAR NOT NULL, | ||
| language_code VARCHAR NOT NULL, | ||
| icon_name VARCHAR NOT NULL, | ||
| title TEXT NOT NULL, | ||
| body TEXT NOT NULL, | ||
| should_send_push BOOLEAN NOT NULL DEFAULT FALSE, | ||
| should_add_to_history BOOLEAN NOT NULL DEFAULT FALSE, | ||
| should_add_to_bulletin BOOLEAN NOT NULL DEFAULT FALSE, | ||
| UNIQUE (name, language_code) | ||
| ); |
There was a problem hiding this comment.
Avoid redundant index on (name, language_code)
UNIQUE (name, language_code) on msg_templates already creates a B‑tree index on those columns; the extra idx_msg_templates_name_language_code index on the same pair is redundant and adds write overhead. Consider dropping the explicit index and relying on the unique constraint (and updating the down migration accordingly).
Also applies to: 24-25
🤖 Prompt for AI Agents
In
core/notifications/migrations/20251209165958_create_msg_templates_and_msg_messages.up.sql
around lines 3-14 (and similarly for lines 24-25), the explicit B-tree index on
(name, language_code) is redundant because the UNIQUE (name, language_code)
already creates that index; remove the explicit
idx_msg_templates_name_language_code index creation from the up migration and
update the down migration to stop dropping that index (instead drop only the
table or the unique constraint as appropriate), ensuring no duplicate index is
created and write overhead is reduced.
| pub async fn list_templates( | ||
| &self, | ||
| limit: Option<i64>, | ||
| offset: Option<i64>, | ||
| ) -> Result<Vec<MsgTemplate>, sqlx::Error> { | ||
| match (limit, offset) { | ||
| (Some(limit), Some(offset)) => { | ||
| let templates = sqlx::query_as::<_, MsgTemplate>( | ||
| r#" | ||
| SELECT | ||
| id, | ||
| name, | ||
| language_code, | ||
| icon_name, | ||
| title, | ||
| body, | ||
| should_send_push, | ||
| should_add_to_history, | ||
| should_add_to_bulletin | ||
| FROM msg_templates | ||
| ORDER BY name, language_code | ||
| LIMIT $1 OFFSET $2 | ||
| "#, | ||
| ) | ||
| .bind(limit) | ||
| .bind(offset) | ||
| .fetch_all(self.read_pool.inner()) | ||
| .await?; | ||
| Ok(templates) | ||
| } | ||
| (Some(limit), None) => { | ||
| let templates = sqlx::query_as::<_, MsgTemplate>( | ||
| r#" | ||
| SELECT | ||
| id, | ||
| name, | ||
| language_code, | ||
| icon_name, | ||
| title, | ||
| body, | ||
| should_send_push, | ||
| should_add_to_history, | ||
| should_add_to_bulletin | ||
| FROM msg_templates | ||
| ORDER BY name, language_code | ||
| LIMIT $1 | ||
| "#, | ||
| ) | ||
| .bind(limit) | ||
| .fetch_all(self.read_pool.inner()) | ||
| .await?; | ||
| Ok(templates) | ||
| } | ||
| (None, Some(offset)) => { | ||
| let templates = sqlx::query_as::<_, MsgTemplate>( | ||
| r#" | ||
| SELECT | ||
| id, | ||
| name, | ||
| language_code, | ||
| icon_name, | ||
| title, | ||
| body, | ||
| should_send_push, | ||
| should_add_to_history, | ||
| should_add_to_bulletin | ||
| FROM msg_templates | ||
| ORDER BY name, language_code | ||
| OFFSET $1 | ||
| "#, | ||
| ) | ||
| .bind(offset) | ||
| .fetch_all(self.read_pool.inner()) | ||
| .await?; | ||
| Ok(templates) | ||
| } | ||
| (None, None) => { | ||
| let templates = sqlx::query_as::<_, MsgTemplate>( | ||
| r#" | ||
| SELECT | ||
| id, | ||
| name, | ||
| language_code, | ||
| icon_name, | ||
| title, | ||
| body, | ||
| should_send_push, | ||
| should_add_to_history, | ||
| should_add_to_bulletin | ||
| FROM msg_templates | ||
| ORDER BY name, language_code | ||
| "#, | ||
| ) | ||
| .fetch_all(self.read_pool.inner()) | ||
| .await?; | ||
| Ok(templates) | ||
| } | ||
| } |
There was a problem hiding this comment.
list_templates ignores language_code even though the API surface supports it
The repository’s list_templates only takes (limit, offset) and selects all rows ordered by name, language_code. Meanwhile, the proto MsgTemplatesListRequest includes languageCode, and the DB has an index on (name, language_code).
If callers expect to list templates for a specific language, this will currently be ignored. Either:
- Thread
language_codeinto this method and add aWHERE language_code = $X(using the existing index), or - Remove/ignore
languageCodefrom the RPC/GraphQL surface to avoid a misleading parameter.
Right now this mismatch is easy to overlook and can cause confusing behavior.
🤖 Prompt for AI Agents
In core/notifications/src/msg_templates/repo.rs around lines 146–243 the
list_templates implementation ignores language_code from the API surface; update
the method signature to accept an Option<String> language_code, add a WHERE
clause (e.g. WHERE language_code = $N) to the SQL when language_code is Some and
bind that parameter (preserve limit/offset binds and ordering to use the
existing (name, language_code) index), and update callers to pass through the
proto MsgTemplatesListRequest.languageCode (or alternatively remove languageCode
from the RPC schema if you choose to drop support) so the DB query matches the
API surface.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
core/api/src/graphql/admin/schema.graphql (2)
427-483: Add GraphQL descriptions for new types and inputs.The new types and inputs lack documentation strings. Other types in the schema have descriptions (e.g.,
AuditedAccount,BTCWallet,Merchant) that help API consumers understand purpose and usage. Add descriptions to:
NotificationMessageNotificationMessageCreateInputNotificationMessageUpdateStatusInputNotificationTemplateNotificationTemplateCreateInputNotificationTemplateDeleteInputNotificationTemplateUpdateInput
547-548: Align pagination pattern with existing cursor-based approach.The
notificationMessagesandnotificationTemplatesqueries uselimitandoffsetpagination, but other paginated queries in the schema (e.g.,invoices,transactions) use cursor-based pagination withafter,before,first,lastarguments. Adopting the cursor-based approach would provide consistency and better support for real-world pagination scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/api/src/graphql/admin/schema.graphql(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build and test core
- GitHub Check: Migrate Mongodb
- GitHub Check: Check SDLs
- GitHub Check: execute via bats
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: execute via buck2
| type NotificationMessage { | ||
| id: ID! | ||
| sentBy: String! | ||
| status: String! |
There was a problem hiding this comment.
Use enums for status field instead of String for type safety.
The status field in NotificationMessage is typed as String!, but the schema consistently uses enums for status-like fields (see AccountStatus, InvoicePaymentStatus, LnPaymentStatus, TxStatus). Additionally, status is optional (String) in the create input but required in the output type, creating an inconsistency.
Consider defining a NotificationStatus enum (with appropriate values) and using it across these fields:
NotificationMessage.status: NotificationStatus!NotificationMessageCreateInput.status: NotificationStatus(or make it required)NotificationMessageUpdateStatusInput.status: NotificationStatus!
Also applies to: 437-437, 443-443
|
|
||
| type NotificationTemplate { | ||
| body: String! | ||
| iconName: String! |
There was a problem hiding this comment.
Use NotificationIcon enum instead of String for iconName field.
The schema already defines a comprehensive NotificationIcon enum (lines 373–425) with 30+ icon values. The iconName field should reference this enum for type safety and validation, rather than accepting arbitrary strings.
Apply this diff:
type NotificationTemplate {
body: String!
- iconName: String!
+ iconName: NotificationIcon!
id: ID!
languageCode: Language!
name: String!
shouldAddToBulletin: Boolean!
shouldAddToHistory: Boolean!
shouldSendPush: Boolean!
title: String!
}
input NotificationTemplateCreateInput {
body: String!
- iconName: String!
+ iconName: NotificationIcon!
languageCode: Language!
name: String!
shouldAddToBulletin: Boolean!
shouldAddToHistory: Boolean!
shouldSendPush: Boolean!
title: String!
}
input NotificationTemplateUpdateInput {
body: String!
- iconName: String!
+ iconName: NotificationIcon!
id: ID!
languageCode: Language!
name: String!
shouldAddToBulletin: Boolean!
shouldAddToHistory: Boolean!
shouldSendPush: Boolean!
title: String!
}Also applies to: 460-460, 475-475
🤖 Prompt for AI Agents
In core/api/src/graphql/admin/schema.graphql at lines 448, 460 and 475, replace
the iconName field type from String! to the existing NotificationIcon! enum;
update each field declaration to use NotificationIcon! (e.g. "iconName:
NotificationIcon!") so schema validation and type-safety use the predefined enum
instead of arbitrary strings. Ensure no additional imports are needed since
NotificationIcon is defined in the same file and run schema validation to
confirm there are no remaining String usages for iconName elsewhere.
|
@tiankii Below are my comments/questions:
|
You will find code related to integration (message delivery) in the Frontend/Integration PR: Frontend/Integrations: Card invitations #353 |
The code for the methods related to the Invitation flow (Invitation sent, KYC Passed, KYC Fail, Invitation Revoked) is still in progress. |
I understand. You still use the MarketingNotification mutation for message delivery. The added tables are for persisting delivered messages and templates. |
You haven't addressed my questions though. I understand it's in progress but how do you intend for them to work? |
…cation-message-templates
We have created a document that maps the integration events across the components. https://docs.google.com/document/d/1LqlChHAUpYg0iKY69L1fvoLLzRuiAlivj3kaHtDbwu4/edit?usp=sharing We have identified and added placeholders on the Admin Panel, Mobile App, and Blink KYC components, where we will do API calls to "Card Platform". We need the "Card backend service" deployed and accessible on the blink-backend project to start coding and testing the integration. |
…ate status, and return pagination totals
…nation total, template status, message templateId)
3 participants
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.