Sending sent messages to the RabbitMQ queue #256
Conversation
Summary of ChangesHello @AntonKun, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the messaging system by introducing the capability to publish all messages sent via the API to a RabbitMQ queue. This change aligns the processing of outgoing messages with that of incoming messages, enabling a unified and robust event-driven architecture. The implementation involves adding new helper functions to categorize message types and meticulously construct event payloads, which are then integrated into existing message sending endpoints to ensure comprehensive coverage and data consistency. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully adds the functionality to publish sent messages to a RabbitMQ queue, which is a great enhancement for event processing. The implementation is mostly correct, but I've identified several areas for improvement regarding code duplication, readability, and robustness. My main concern is the significant code duplication across the various Send... handlers, for which I've suggested a refactoring approach. Additionally, I've provided recommendations for the new helper functions getMessageType and publishSentMessageEvent to make them more idiomatic, robust by adding logging for silent failures, and more accurate by dynamically determining message properties instead of using hardcoded values.
| token := r.Context().Value("userinfo").(Values).Get("Token") | ||
| userID := r.Context().Value("userinfo").(Values).Get("Id") | ||
| s.publishSentMessageEvent(token, userID, txtid, recipient, msgid, msg, resp.Timestamp) |
There was a problem hiding this comment.
This block of code to retrieve user information and publish the message event is duplicated across at least 11 Send... handlers (e.g., SendAudio, SendImage, SendVideo, etc.). This violates the DRY (Don't Repeat Yourself) principle and makes the code harder to maintain, as any future change to this logic will require updating it in many places.
Consider refactoring this into a helper function that encapsulates this logic. For example, you could modify publishSentMessageEvent to accept the *http.Request and extract the user info itself. This would reduce the duplicated code in each handler to a single function call.
| messageEvent := map[string]interface{}{ | ||
| "Info": messageInfo, | ||
| "Message": msg, | ||
| "IsEphemeral": false, | ||
| "IsViewOnce": false, | ||
| "IsViewOnceV2": false, | ||
| "IsViewOnceV2Extension": false, | ||
| "IsDocumentWithCaption": false, | ||
| "IsLottieSticker": false, | ||
| "IsBotInvoke": false, | ||
| "IsEdit": false, | ||
| "SourceWebMsg": nil, | ||
| "UnavailableRequestID": "", | ||
| "RetryCount": 0, | ||
| "NewsletterMeta": nil, | ||
| "RawMessage": rawMessage, | ||
| } |
There was a problem hiding this comment.
Several boolean flags in the messageEvent map are hardcoded to false, but they could be determined dynamically from the message object. This would make the published event more accurate and reflect the true nature of the sent message. Specifically, IsViewOnce, IsDocumentWithCaption, and IsLottieSticker can be set based on the message content.
messageEvent := map[string]interface{}{
"Info": messageInfo,
"Message": msg,
"IsEphemeral": false,
"IsViewOnce": msg.GetViewOnceMessage() != nil,
"IsViewOnceV2": false,
"IsViewOnceV2Extension": false,
"IsDocumentWithCaption": msg.GetDocumentMessage() != nil && msg.GetDocumentMessage().GetCaption() != "",
"IsLottieSticker": msg.GetStickerMessage() != nil && msg.GetStickerMessage().GetIsLottie(),
"IsBotInvoke": false,
"IsEdit": false,
"SourceWebMsg": nil,
"UnavailableRequestID": "",
"RetryCount": 0,
"NewsletterMeta": nil,
"RawMessage": rawMessage,
}| func (s *server) getMessageType(msg *waE2E.Message) string { | ||
| if msg.GetConversation() != "" { | ||
| return "text" | ||
| } | ||
| if msg.GetExtendedTextMessage() != nil { | ||
| return "text" | ||
| } | ||
| if msg.GetImageMessage() != nil { | ||
| return "image" | ||
| } | ||
| if msg.GetVideoMessage() != nil { | ||
| return "video" | ||
| } | ||
| if msg.GetAudioMessage() != nil { | ||
| return "audio" | ||
| } | ||
| if msg.GetDocumentMessage() != nil { | ||
| return "document" | ||
| } | ||
| if msg.GetStickerMessage() != nil { | ||
| return "sticker" | ||
| } | ||
| if msg.GetContactMessage() != nil { | ||
| return "contact" | ||
| } | ||
| if msg.GetLocationMessage() != nil { | ||
| return "location" | ||
| } | ||
| // Note: Poll messages are handled by BuildPollCreation and the type is determined from RawMessage | ||
| if msg.GetButtonsMessage() != nil || msg.GetButtonsResponseMessage() != nil { | ||
| return "buttons" | ||
| } | ||
| if msg.GetListMessage() != nil || msg.GetListResponseMessage() != nil { | ||
| return "list" | ||
| } | ||
| if msg.GetTemplateMessage() != nil { | ||
| return "template" | ||
| } | ||
| return "text" | ||
| } |
There was a problem hiding this comment.
This series of if statements can be refactored into a switch statement. Using switch is more idiomatic and often more readable in Go for handling a sequence of conditions.
func (s *server) getMessageType(msg *waE2E.Message) string {
switch {
case msg.GetConversation() != "":
return "text"
case msg.GetExtendedTextMessage() != nil:
return "text"
case msg.GetImageMessage() != nil:
return "image"
case msg.GetVideoMessage() != nil:
return "video"
case msg.GetAudioMessage() != nil:
return "audio"
case msg.GetDocumentMessage() != nil:
return "document"
case msg.GetStickerMessage() != nil:
return "sticker"
case msg.GetContactMessage() != nil:
return "contact"
case msg.GetLocationMessage() != nil:
return "location"
// Note: Poll messages are handled by BuildPollCreation and the type is determined from RawMessage
case msg.GetButtonsMessage() != nil || msg.GetButtonsResponseMessage() != nil:
return "buttons"
case msg.GetListMessage() != nil || msg.GetListResponseMessage() != nil:
return "list"
case msg.GetTemplateMessage() != nil:
return "template"
default:
return "text"
}
}| if client == nil { | ||
| return |
There was a problem hiding this comment.
The function returns silently if the client is not found. It's better to log a warning in this case to make debugging easier if events are not being published as expected.
if client == nil {
log.Warn().Str("txtid", txtid).Msg("Cannot publish sent message event: client not found")
return
}| if client.Store != nil && client.Store.LIDs != nil { | ||
| ctx := context.Background() | ||
|
|
||
| // Get sender LID | ||
| if !senderJID.IsEmpty() { | ||
| if lid, err := client.Store.LIDs.GetLIDForPN(ctx, senderJID); err == nil && !lid.IsEmpty() { | ||
| senderLID = lid | ||
| } | ||
| } | ||
|
|
||
| // Get recipient LID (only for non-group chats) | ||
| if !isGroup && !recipient.IsEmpty() { | ||
| if lid, err := client.Store.LIDs.GetLIDForPN(ctx, recipient); err == nil && !lid.IsEmpty() { | ||
| recipientLID = lid | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Errors from client.Store.LIDs.GetLIDForPN are currently ignored. While not critical, logging these errors can be helpful for debugging issues related to LIDs. Also, the empty lines can be removed for cleaner formatting.
if client.Store != nil && client.Store.LIDs != nil {
ctx := context.Background()
// Get sender LID
if !senderJID.IsEmpty() {
if lid, err := client.Store.LIDs.GetLIDForPN(ctx, senderJID); err == nil && !lid.IsEmpty() {
senderLID = lid
} else if err != nil {
log.Warn().Err(err).Str("jid", senderJID.String()).Msg("Failed to get LID for sender")
}
}
// Get recipient LID (only for non-group chats)
if !isGroup && !recipient.IsEmpty() {
if lid, err := client.Store.LIDs.GetLIDForPN(ctx, recipient); err == nil && !lid.IsEmpty() {
recipientLID = lid
} else if err != nil {
log.Warn().Err(err).Str("jid", recipient.String()).Msg("Failed to get LID for recipient")
}
}
}
1 participant
Changes in connection with #245
Adding messages sent via API to the RabbitMQ queue will allow them to be processed from the queue, just like received messages. I tried to make the structure of the event similar to the case when the message would simply be sent from the WA application.