feat: process webhook whatsapp await response #21
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request refactors the WhatsApp webhook processing and adds new functionality. The controller now delegates payload processing to a new method that handles conditional synchronous (using Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as WhatsappController
participant Job as WhatsappEventsJob
participant Logger
Client->>Controller: POST /webhooks/whatsapp
Controller->>Controller: Check awaitResponse parameter
alt awaitResponse present
Controller->>Job: perform_now(params)
alt Exception Raised: InvalidWebhookVerifyToken
Controller-->>Client: 401 Unauthorized
else Exception Raised: MessageNotFoundError
Controller-->>Client: 404 Not Found
else Exception Raised: StandardError
Controller->>Logger: Log error message
Controller-->>Client: 400 Bad Request
else No Exception
Controller-->>Client: 200 OK
end
else awaitResponse absent
Controller->>Job: perform_later(params)
Controller-->>Client: 200 OK
end
sequenceDiagram
participant Service as IncomingMessageBaileysService
participant MessageDB
participant Logger
Service->>Service: process_messages_update(updates)
loop For each update
Service->>Service: valid_update_message?(update)
alt Message is valid
Service->>Service: update_status(update)
Service->>Service: update_message_content(update)
else Message is invalid
Service->>Logger: Log error (raise MessageNotFoundError)
end
end
Poem
🪧 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 (
|
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
spec/controllers/webhooks/whatsapp_controller_spec.rb:26
- [nitpick] Consider using consistent capitalization for 'WhatsApp' instead of 'whatsApp' to align with naming conventions.
it 'calls the whatsApp events job asynchronously with perform_later when awaitResponse is not present' do
spec/controllers/webhooks/whatsapp_controller_spec.rb:68
- [nitpick] Consider renaming the description to 'calls the WhatsApp events job synchronously' for consistent capitalization.
it 'calls the whatsApp events job synchronously' do
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
spec/controllers/webhooks/whatsapp_controller_spec.rb:26
- [nitpick] Consider updating 'whatsApp' to 'WhatsApp' for consistent capitalization across the tests.
it 'calls the whatsApp events job asynchronously with perform_later when awaitResponse is not present' do
app/controllers/webhooks/whatsapp_controller.rb:15
- [nitpick] The parameter 'awaitResponse' uses camelCase; if it is not constrained by an external API, consider renaming it to the more conventional Ruby snake_case 'await_response'.
if params[:awaitResponse].present?
There was a problem hiding this comment.
8000 Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @gabrieljablonski)
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (4)
spec/controllers/webhooks/whatsapp_controller_spec.rb (1)
96-105: Test for generic error handling looks good.The test correctly verifies that when a generic
StandardErroroccurs during synchronous processing:
- The error is properly logged
- The response has the appropriate
:bad_request(400) statusConsider also testing the response body to ensure proper error information is returned to the client.
app/controllers/webhooks/whatsapp_controller.rb (1)
14-29: Consider improving the condition for synchronous execution.The condition
if params[:awaitResponse].present?will trigger synchronous execution as long as the parameter exists, regardless of its value. You might want to check for a truthy value instead.- if params[:awaitResponse].present? + if ActiveModel::Type::Boolean.new.cast(params[:awaitResponse])This ensures that synchronous execution only happens when
awaitResponseis explicitly set to a truthy value (true,"true","1", etc.), providing more predictable behavior.app/services/whatsapp/incoming_message_baileys_service.rb (2)
56-61: Consider conditional execution of update methods.In
handle_update, bothupdate_statusandupdate_message_contentare always called, but they may not always be needed for every update. Consider checking if the update contains status or content changes before calling these methods.def handle_update raise MessageNotFoundError unless valid_update_message? - update_status - update_message_content + update_status if @raw_update.dig(:update, :status).present? + update_message_content if @raw_update.dig(:update, :message, :editedMessage).present? endThis would make the method more efficient by only performing updates when necessary.
2-3: Consider movingMessageNotFoundErrorclass definition.The
InvalidWebhookVerifyTokenerror class is defined at the top of the file, but theMessageNotFoundErroris not defined here despite being used in this service. Consider defining it in this class for consistency, or in a shared location if it's used across multiple files.class Whatsapp::IncomingMessageBaileysService < Whatsapp::IncomingMessageBaseService class InvalidWebhookVerifyToken < StandardError; end + class MessageNotFoundError < StandardError; end
🛑 Comments failed to post (1)
app/services/whatsapp/incoming_message_baileys_service.rb (1)
75-80: 🛠️ Refactor suggestion
Add nil checks to prevent potential errors.
The
update_message_contentmethod extracts content from deeply nested objects without checking if all intermediate objects exist, which could lead to nil errors.def update_message_content - message = @raw_update.dig(:update, :message, :editedMessage, :message) - content = message[:conversation] || message.dig(:extendedTextMessage, :text) + message = @raw_update.dig(:update, :message, :editedMessage, :message) + return unless message.present? + + content = message[:conversation] || message.dig(:extendedTextMessage, :text) @message_update.update!(content: content) if content.present? endThis change adds a nil check to prevent errors when accessing properties of potentially nil objects.
📝 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.def update_message_content message = @raw_update.dig(:update, :message, :editedMessage, :message) return unless message.present? content = message[:conversation] || message.dig(:extendedTextMessage, :text) @message_update.update!(content: content) if content.present? end
There was a problem hiding this comment.
-work-in-progress +(review needed)
assign review @gabrieljablonski
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status:
There was a problem hiding this comment.
-(review needed) +(questions/changes requested)
Reviewed 1 of 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @CayoPOliveira)
spec/controllers/webhooks/whatsapp_controller_spec.rb line 0 at r2 (raw file):
Specs are failing.
Failures:
1) Webhooks::WhatsappController POST /webhooks/whatsapp/{:phone_number} when awaitResponse param is present returns not_found when MessageNotFoundError is raised
Failure/Error: allow(Webhooks::WhatsappEventsJob).to receive(:perform_now).and_raise(Whatsapp::IncomingMessageBaileysService::MessageNotFoundError)
NameError:
uninitialized constant Whatsapp::IncomingMessageBaileysService::MessageNotFoundError
# ./spec/controllers/webhooks/whatsapp_controller_spec.rb:87:in `block (4 levels) in <top (required)>'
# /usr/share/rvm/gems/ruby-3.3.3/gems/webmock-3.23.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'
2) Webhooks::WhatsappController POST /webhooks/whatsapp/{:phone_number} when awaitResponse param is present logs error and returns bad_request when StandardError is raised
Failure/Error: expect(response).to have_http_status(:bad_request)
expected the response to have status code :bad_request (400) but it was :internal_server_error (500)
# ./spec/controllers/webhooks/whatsapp_controller_spec.rb:102:in `block (4 levels) in <top (required)>'
# /usr/share/rvm/gems/ruby-3.3.3/gems/webmock-3.23.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'
Finished in 1.48 seconds (files took 3.15 seconds to load)
10 examples, 2 failures
Failed examples:
rspec ./spec/controllers/webhooks/whatsapp_controller_spec.rb:86 # Webhooks::WhatsappController POST /webhooks/whatsapp/{:phone_number} when awaitResponse param is present returns not_found when MessageNotFoundError is raised
rspec ./spec/controllers/webhooks/whatsapp_controller_spec.rb:94 # Webhooks::WhatsappController POST /webhooks/whatsapp/{:phone_number} when awaitResponse param is present logs error and returns bad_request when StandardError is raised
spec/controllers/webhooks/whatsapp_controller_spec.rb line 34 at r2 (raw file):
expect(response).to have_http_status(:ok) end
NIT
We usually won't be updating existing specs too much, but in this case let's take the opportunity to improve this as well.
Suggestion:
it 'calls the whatsapp events job asynchronously with perform_later when awaitResponse is not present' do
allow(Webhooks::WhatsappEventsJob).to receive(:perform_later)
post '/webhooks/whatsapp/123221321', params: { content: 'hello' }
expect(Webhooks::WhatsappEventsJob).to have_received(:perform_later)
expect(response).to have_http_status(:ok)
endspec/controllers/webhooks/whatsapp_controller_spec.rb line 64 at r2 (raw file):
expect(response).to have_http_status(:ok) end
NIT
same as above.
Suggestion:
it 'processes the webhook normally' do
allow(Webhooks::WhatsappEventsJob).to receive(:perform_later)
post '/webhooks/whatsapp/+1234567890', params: { content: 'hello' }
expect(Webhooks::WhatsappEventsJob).to have_received(:perform_later)
expect(response).to have_http_status(:ok)
endspec/controllers/webhooks/whatsapp_controller_spec.rb line 71 at r2 (raw file):
allow(Webhooks::WhatsappEventsJob).to receive(:perform_now) expect(Webhooks::WhatsappEventsJob).to receive(:perform_now)
Update to match previous suggestions (have_received)
spec/controllers/webhooks/whatsapp_controller_spec.rb line 78 at r2 (raw file):
end it 'returns unauthorized when InvalidWebhookVerifyToken is raised' do
use status codes instead for descriptions.
same for specs below.
Suggestion:
it 'returns 401 when InvalidWebhookVerifyToken is raised' doapp/controllers/webhooks/whatsapp_controller.rb line 17 at r2 (raw file):
def perform_whatsapp_events_job if params[:awaitResponse].present?
For consistency.
Suggestion:
if params[:await_response].present?app/controllers/webhooks/whatsapp_controller.rb line 24 at r2 (raw file):
end head :ok end
NIT
Suggestion:
def perform_whatsapp_events_job
perform_sync if params[:awaitResponse].present?
return if performed?
Webhooks::WhatsappEventsJob.perform_later(params.to_unsafe_hash)
head :ok
endapp/controllers/webhooks/whatsapp_controller.rb line 35 at r2 (raw file):
Rails.logger.error("Error processing WhatsApp webhook: #{e.message}") head :bad_request end
NIT
Usually we shouldn't rescue from StandardError, since it might mask errors.
Best to let it bubble up. Server will return a 500 error by default.
Suggestion:
def perform_sync
Webhooks::WhatsappEventsJob.perform_now(params.to_unsafe_hash)
rescue Whatsapp::IncomingMessageBaileysService::InvalidWebhookVerifyToken
head :unauthorized
rescue Whatsapp::IncomingMessageBaileysService::MessageNotFoundError
head :not_found
endThere was a problem hiding this comment.
-(questions/changes requested) +(review needed)
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gabrieljablonski)
app/controllers/webhooks/whatsapp_controller.rb line 17 at r2 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
For consistency.
Baileys-API follows PascalCase naming conventions.
spec/controllers/webhooks/whatsapp_controller_spec.rb line at r2 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Specs are failing.
Failures: 1) Webhooks::WhatsappController POST /webhooks/whatsapp/{:phone_number} when awaitResponse param is present returns not_found when MessageNotFoundError is raised Failure/Error: allow(Webhooks::WhatsappEventsJob).to receive(:perform_now).and_raise(Whatsapp::IncomingMessageBaileysService::MessageNotFoundError) NameError: uninitialized constant Whatsapp::IncomingMessageBaileysService::MessageNotFoundError # ./spec/controllers/webhooks/whatsapp_controller_spec.rb:87:in `block (4 levels) in <top (required)>' # /usr/share/rvm/gems/ruby-3.3.3/gems/webmock-3.23.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>' 2) Webhooks::WhatsappController POST /webhooks/whatsapp/{:phone_number} when awaitResponse param is present logs error and returns bad_request when StandardError is raised Failure/Error: expect(response).to have_http_status(:bad_request) expected the response to have status code :bad_request (400) but it was :internal_server_error (500) # ./spec/controllers/webhooks/whatsapp_controller_spec.rb:102:in `block (4 levels) in <top (required)>' # /usr/share/rvm/gems/ruby-3.3.3/gems/webmock-3.23.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>' Finished in 1.48 seconds (files took 3.15 s 8000 econds to load) 10 examples, 2 failures Failed examples: rspec ./spec/controllers/webhooks/whatsapp_controller_spec.rb:86 # Webhooks::WhatsappController POST /webhooks/whatsapp/{:phone_number} when awaitResponse param is present returns not_found when MessageNotFoundError is raised rspec ./spec/controllers/webhooks/whatsapp_controller_spec.rb:94 # Webhooks::WhatsappController POST /webhooks/whatsapp/{:phone_number} when awaitResponse param is present logs error and returns bad_request when StandardError is raised
Done Fixed
There was a problem hiding this comment.
-(review needed) +(approved for merge)
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status:
f74777c to
e97596b
Compare
df62cfb
into
feat/process-baileys-api-messages-update
…20) * feat: add message update processing in IncomingMessageBaileysService * feat: implement message update handling in IncomingMessageBaileysService * feat: add MessageNotFoundError for handling invalid update messages in IncomingMessageBaileysService * chore: specs for message.update events and bug fixes * chore: refactor message update handling in IncomingMessageBaileysService * chore: nit remove redundant comment * chore: enhance logging for unsupported message update statuses in IncomingMessageBaileysService and disabled metrics * chore: message status update logic with transition checks * chore: update status mapping for PENDING to sent in status_mapper * chore: update status_mapper comments and fix case statement for status codes * fix: logging for unsupported message updates in update_message_content method * test: add specs for unsupported status transitions in messages.update event * refactor: ensure message status is reloaded before assertion in messages.update event spec * refactor: status variable in status_mapper method * refactor: rename status_transition_allowed method to status_transition_allowed? * refactor: streamline message creation in specs by using let! for setup * feat: process webhook whatsapp await response (#21) * feat: enhance WhatsApp webhook processing and error responses handling * chore: correct spelling of 'WhatsApp' in webhook controller specs * refactor: rename webhook processing method and improve error handling * chore: update error handling in WhatsApp controller specs for specific exceptions * refactor: remove handling for StandardError in WhatsApp controller specs * refactor: simplify perform_whatsapp_events_job method * chore: update response status from not_found to bad_request for invalid message * refactor: update expectations for job processing in WhatsApp controller specs
This change is
Summary by CodeRabbit
New Features
Tests