feat: add flag to edited messages and content history tracking #35
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change adds Changes
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 RuboCop (1.73)spec/services/whatsapp/incoming_message_baileys_service_spec.rbError: 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a flag to indicate when a message has been edited and adds tracking for the message’s content history. Key changes include:
- Modifications to the WhatsApp incoming message service to update and store the edit state and content history.
- Updates to the tests to verify the new edited message functionality.
- Changes to the message model to support the new attributes in the content_attributes store.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| spec/services/whatsapp/incoming_message_baileys_service_spec.rb | Added test cases to validate edited message content updates and history. |
| app/services/whatsapp/incoming_message_baileys_service.rb | Updated service logic to set is_edited, manage content history and update content. |
| app/models/message.rb | Extended the store attributes to include is_edited and content_history. |
| message = @raw_message.dig(:update, :message, :editedMessage, :message) | ||
| if message.blank? | ||
| @raw_message = @raw_message.dig(:update, :message, :editedMessage) | ||
| if @raw_message[:message].blank? |
There was a problem hiding this comment.
The code assumes @raw_message is not nil before checking @raw_message[:message]. This might lead to a NoMethodError if the dig returns nil; add an explicit nil check for @raw_message.
| if @raw_message[:message].blank? | |
| if @raw_message.nil? || @raw_message[:message].blank? |
Copilot uses AI. Check for mistakes.
| @raw_message = @raw_message.dig(:update, :message, :editedMessage) | ||
| if @raw_message[:message].blank? |
There was a problem hiding this comment.
[nitpick] Reassigning @raw_message to a nested value can obscure the original data and impact readability. Consider using a local variable to store the nested message data instead.
| @raw_message = @raw_message.dig(:update, :message, :editedMessage) | |
| if @raw_message[:message].blank? | |
| edited_message = @raw_message.dig(:update, :message, :editedMessage) | |
| if edited_message[:message].blank? |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @CayoPOliveira)
app/services/whatsapp/incoming_message_baileys_service.rb line 321 at r1 (raw file):
end def update_message_content
this name seems clearer, WDYT
Suggestion:
def handle_edited_messageapp/services/whatsapp/incoming_message_baileys_service.rb line 338 at r1 (raw file):
end @message.update!(content: content) end
Please review the whole logic for this function, i believe it's unnecessarily complex.
app/models/message.rb line 111 at r1 (raw file):
:external_created_at, :story_sender, :story_id, :external_error, :translations, :in_reply_to_external_id, :is_unsupported, :is_reaction, :is_edited, :content_history], coder: JSON
Don't forget to refactor names.
Suggestion:
# [:is_edited] : Used to indicated edited message and edit history
store :content_attributes, accessors: [:submitted_email, :items, :submitted_values, :email, :in_reply_to, :deleted,
:external_created_at, :story_sender, :story_id, :external_error,
:translations, :in_reply_to_external_id, :is_unsupported,
:is_reaction, :is_edited, :edit_history], coder: JSONspec/services/whatsapp/incoming_message_baileys_service_spec.rb line 731 at r1 (raw file):
expect(message.content_history).to be_nil described_class.new(inbox: inbox, params: params).perform
Not needed.
Suggestion:
}
described_class.new(inbox: inbox, params: params).performspec/services/whatsapp/incoming_message_baileys_service_spec.rb line 738 at r1 (raw file):
end it 'updates the message content for an already edited message' do
NIT
Suggestion:
it 'updates the message content and the edit history' dospec/services/whatsapp/incoming_message_baileys_service_spec.rb line 753 at r1 (raw file):
} expect(message.content_attributes).to eq({ 'is_edited' => true, 'content_history' => ['Old message content'] })
Why check this?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/services/whatsapp/incoming_message_baileys_service.rb (1)
322-323:⚠️ Potential issueAdd null check before accessing nested structures
The code directly reassigns
@raw_messageto a deeply nested value without first checking if the intermediate objects exist. This could lead to a NoMethodError if any part of the path is nil.- @raw_message = @raw_message.dig(:update, :message, :editedMessage) - if @raw_message[:message].blank? + edited_message = @raw_message.dig(:update, :message, :editedMessage) + if edited_message.nil? || edited_message[:message].blank?
🧹 Nitpick comments (1)
app/services/whatsapp/incoming_message_baileys_service.rb (1)
331-337: Well-structured message edit history trackingThe implementation correctly sets the edit flag and manages the content history array, including handling both initial edits and subsequent edits elegantly.
However, consider adding a maximum limit to the history array to prevent unlimited growth over time.
@message.is_edited = true if @message.content_history.blank? @message.content_history = [@message.content] else + # Prevent unlimited growth of history array + @message.content_history = @message.content_history.last(9) if @message.content_history.size >= 10 @message.content_history.push(@message.content) end @message.update!(content: content)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/models/message.rb(1 hunks)app/services/whatsapp/incoming_message_baileys_service.rb(1 hunks)spec/services/whatsapp/incoming_message_baileys_service_spec.rb(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/services/whatsapp/incoming_message_baileys_service.rb (1)
app/models/message.rb (1)
content(189-194)
🔇 Additional comments (5)
app/models/message.rb (2)
105-107: Good documentation of the new attributesThe comments clearly explain the purpose of the new
is_editedandcontent_historyattributes.
110-111: LGTM: Properly extending the content_attributes storeThe new attributes are correctly added to the existing content_attributes JSON store, allowing them to be accessed and modified like other message attributes.
app/services/whatsapp/incoming_message_baileys_service.rb (1)
328-329: LGTM: Good defensive check for contentProperly validating that content exists before proceeding with the update.
spec/services/whatsapp/incoming_message_baileys_service_spec.rb (2)
728-736: LGTM: Good test coverage for initial edit behaviorThe test case properly verifies that the message is marked as edited and the original content is stored in the history when a message is edited for the first time.
738-759: LGTM: Comprehensive test for multiple editsGood test case that verifies the correct behavior when a message that has already been edited is edited again, ensuring the content history accumulates properly.
…_history for tracking edits
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @gabrieljablonski)
app/models/message.rb line 111 at r1 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Don't forget to refactor names.
Done.
app/services/whatsapp/incoming_message_baileys_service.rb line 321 at r1 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
this name seems clearer, WDYT
Done
Ok
app/services/whatsapp/incoming_message_baileys_service.rb line 338 at r1 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Please review the whole logic for this function, i believe it's unnecessarily complex.
Done.
spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 731 at r1 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Not needed.
Done.
spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 753 at r1 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Why check this?
LGTM Is for test the message.update was done, but I can take away
There was a problem hiding this comment.
-(questions/changes requested) +(review needed)
Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @gabrieljablonski)
There was a problem hiding this comment.
Reviewed 2 of 3 files at r4, 3 of 3 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @CayoPOliveira)
Pull Request Template
Description
Please include a summary of the change and issue(s) fixed. Also, mention relevant motivation, context, and any dependencies that this change requires.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
Checklist:
This change is
Summary by CodeRabbit