Pull Request Template #10
Conversation
|
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.69.1)GemfileObsolete configuration: app/builders/messages/message_builder.rbObsolete configuration: app/controllers/api/v1/accounts/conversations_controller.rbObsolete configuration:
WalkthroughThis pull request introduces comprehensive enhancements to the application's WhatsApp integration, messaging, and deployment capabilities. The changes span multiple components, including CI/CD configuration, message handling services, webhook processing, and validation mechanisms. Key additions include support for unofficial WhatsApp Cloud API, Brazilian phone number validation, improved message status tracking, and more flexible webhook handling. The modifications aim to enhance the robustness and flexibility of the messaging infrastructure, with a particular focus on WhatsApp channel interactions. Changes
Sequence DiagramsequenceDiagram
participant Client
participant WhatsAppCloudService
participant MessageBuilder
participant WebhookListener
participant WebhookJob
Client->>WhatsAppCloudService: Send Message
WhatsAppCloudService->>MessageBuilder: Build Message
MessageBuilder-->>WhatsAppCloudService: Processed Message
WhatsAppCloudService->>WebhookListener: Trigger Webhook
WebhookListener->>WebhookJob: Queue Webhook Delivery
WebhookJob->>External: Send Webhook
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
ok |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (29)
app/services/whatsapp/incoming_message_base_service.rb (3)
60-68: Consider processing all errors.
Currently, only the first error (state[:errors].first) is used in creating the failed status message. If the API can return multiple errors, capturing them all may provide more comprehensive diagnostic data.
125-126: Refactor to simplify file attachment logic.
Consider extracting file attachment creation into a separate private helper method to make the code more readable.
164-166: Introduction of a default :incoming message type.
Defining a dedicated method to set the default message type is a clean approach. If future expansions require additional message types, consider a more flexible configuration mechanism.spec/services/whatsapp/incoming_message_service_spec.rb (1)
100-119: Consider enhancing the failed status test coverage.While the test correctly verifies the status change and message count, consider these improvements:
- Add expectations to verify that the error message content matches the error details
- Extract the error details structure to a helper or shared context for reuse
- Rename the test to indicate it also verifies error message creation (e.g., "updates status to failed and creates error message")
Example enhancement:
- it 'update status message to failed' do + it 'updates status to failed and creates error message' do # ... existing setup ... described_class.new(inbox: whatsapp_channel.inbox, params: status_params).perform expect(last_conversation.messages.count).to eq(2) expect(Message.find_by!(source_id: source_id).status).to eq('failed') + error_message = last_conversation.messages.last + expect(error_message.content).to include('abc') # Verify error message content + expect(error_message.content).to include('123') # Verify error code endapp/models/concerns/brazilian_number_validator.rb (1)
11-11: Minor grammatical refinement in Portuguese error message (optional).
“Número esta inválido” might be changed to “Número está inválido” to use the accented “á”.- record.errors.add(attribute, (options[:message] || 'Número esta inválido ou é um celular e precisa ter o nono digito!')) + record.errors.add(attribute, (options[:message] || 'Número está inválido ou é um celular e precisa ter o nono dígito!'))app/models/contact.rb (1)
37-37: Consider ensuring consistent phone validations.You've introduced a Brazilian-specific phone number validation on top of an existing format validation. If phone numbers outside Brazil are expected in your system, consider clarifying or conditioning these validations (e.g., only apply the Brazilian validator if a phone number is known to be from Brazil). Otherwise, this might block valid international numbers.
.gitlab-ci.yml (1)
32-32: Store the Dokku remote URL in a CI variable.Exposing credentials or host addresses in the repository is brittle and might be a security concern if this is a private target. Consider using GitLab’s CI/CD variables for “GIT_REMOTE_URL.”
Gemfile (1)
148-148: Pin or specify a compatible phonelib version.You might want to pin this gem (e.g., '~> 0.8') to ensure consistent behavior across deployments. Maintaining a precise version range helps avoid unexpected regressions.
app/services/whatsapp/incoming_message_whatsapp_cloud_service.rb (3)
41-44: Sanitize/escape the sender’s name in group chats.When constructing the group message content, a malicious or malformed @sender.name could cause unwanted formatting (e.g. Markdown injection). Consider sanitizing or escaping @sender.name to improve safety.
58-63: Validate from_id usage in outgoing_message_type?.You rely on from_id matching the channel’s phone_number without additional checks. If from_id or phone_number is missing or malformed, the method might incorrectly return nil or a false negative.
65-73: Confirm you are returning a boolean.Returning nil from activity_message_type? is treated as false, which might be intentional. If you prefer a strict boolean return, consider explicitly returning false to maintain clarity in conditionals.
app/services/whatsapp/providers/whatsapp_cloud_service.rb (2)
55-60: Evaluate the HTTP method approach.message_update_http_method returns :post, and message_path uses messages_path. If you plan to allow other HTTP methods such as PATCH or PUT for updates, consider making message_update_http_method more dynamic or configurable.
88-93: Consider input sanitization for text messages.When sending text messages via send_text_message, format_content might prepend user-generated data. If the destination interprets Markdown, name injection remains a possibility. Evaluate whether stricter input validation or escaping is warranted.
app/builders/messages/message_builder.rb (4)
91-92: Handle missing sender record.If no contact is found for the given sender_id, consider a fallback or error to prevent a nil sender.
117-120: Assigning :progress status on pending messages is good, but formalize the logic.params_status_peding? sets the status to :progress if certain conditions are met. Confirm that this aligns with any official transitions or workflow states in Whatsapp.
138-144: Check for merges that might override existing attributes.The chaining of .merge(...) calls adds new attributes to message_params. If a future attribute name conflicts, it could unintentionally overwrite existing data. Keep an eye on naming collisions.
146-147: Correct the spelling of 'params_status_peding?'.It appears you intended to write “params_status_pending?” which would be more descriptive.
- def params_status_peding? + def params_status_pending?app/jobs/webhook_job.rb (1)
4-5: Provide fallback or validation for custom methods.method uses a default of :post, but if callers supply an unrecognized HTTP verb, the job might behave unexpectedly. Consider validating or mapping supported methods.
lib/webhooks/trigger.rb (1)
2-7: Ensure method flexibility and consider error checks
Using default parameters for method and headers is straightforward. However, it might be beneficial to validate or sanitize the provided HTTP method to safeguard against invalid inputs. Otherwise, this approach looks clean and aligns well with typical REST patterns.app/services/whatsapp/providers/base_service.rb (1)
42-44: Disable for official WhatsApp Cloud API
Returning false is logical if you assume official APIs by default. Consider making it overrideable or configurable if multiple APIs might need dynamic detection.app/services/whatsapp/providers/whatsapp_360_dialog_service.rb (1)
52-54: Check for missing or null source_id.
Accessing message[:source_id] without verifying that :source_id exists could lead to errors. Consider handling nil or empty values in a gracefully.def message_path(message) - "#{api_base_path}/messages/#{message[:source_id]}" + source_id = message[:source_id] + return nil unless source_id + "#{api_base_path}/messages/#{source_id}"app/listeners/webhook_listener.rb (1)
86-98: Ensure proper payload validation.
While deep_symbolizing keys is helpful, consider verifying that the payload still contains the expected keys (e.g., :message_type, :event, :status) before proceeding. This practice reduces the risk of triggering unexpected behavior with malformed payloads.app/javascript/dashboard/routes/dashboard/settings/inbox/channels/CloudWhatsapp.vue (2)
131-131: Default URL could be further validated or clarified.
Currently defaulting to “https://graph.facebook.com” is a straightforward choice. If you expect user-provided hosts, consider clarifying that in UI tooltips or docs.
146-146: Augment required rule with a URL validator.
You might consider adding a more robust validator (e.g., isURL) to ensure actual URL formatting instead of just checking for presence.import { required } from 'vuelidate/lib/validators'; +import { required, url as isURL } from 'vuelidate/lib/validators'; validations: { ... - url: { required }, + url: { required, isURL }, },spec/services/whatsapp/providers/whatsapp_cloud_service_spec.rb (1)
69-77: Ensure test coverage for edge cases.While this test confirms that whatsapp_cloud_api_unofficial? returns true given a valid URL, it is advisable to also test scenarios such as missing or malformed URLs to ensure robust validation logic.
app/models/conversation.rb (1)
110-110: Clarify method usage.The last_message_in_messaging_window? call might benefit from an explanatory comment regarding the relationship between time windows and channel restrictions, especially if we add more channel-specific conditions in the future.
app/models/message.rb (1)
79-81: Enhance readability by extracting conditions.The scope logic is dense, with multiple comparisons in a single statement. Extracting the condition into a named method or clarifying the reasoning in a comment would improve readability.
-scope :to_read, lambda { |datetime| - where('EXTRACT(EPOCH FROM updated_at) <= (?) and message_type = 0 and status < 2', datetime.to_i.succ) -} +scope :to_read, lambda { |datetime| + cutoff = datetime.to_i.succ + where('EXTRACT(EPOCH FROM updated_at) <= (?) AND message_type = ? AND status < ?', cutoff, message_types[:incoming], statuses[:delivered]) +}spec/models/conversation_spec.rb (1)
511-526: Validate negative scenarios.The new test confirms returning true under valid conditions. Consider adding a test case for an invalid URL or missing provider_config to confirm that can_reply? doesn’t return true unintentionally.
app/javascript/dashboard/i18n/locale/en/inboxMgmt.json (1)
214-218: Fix grammatical and capitalization issues in the URL field strings.The URL field addition is well-structured and aligns with the WhatsApp Cloud API integration. However, there are some text issues to address:
"URL": { - "LABEL": "Whatsapp Cloud API URL", - "PLACEHOLDER": "Please enter an Whatsapp Cloud API URL or use default https://graph.facebook.com", + "LABEL": "WhatsApp Cloud API URL", + "PLACEHOLDER": "Please enter a WhatsApp Cloud API URL or use default https://graph.facebook.com", "ERROR": "This field is required" },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
.gitlab-ci.yml(1 hunks)Gemfile(1 hunks)app/builders/messages/message_builder.rb(3 hunks)app/controllers/api/v1/accounts/conversations_controller.rb(1 hunks)app/javascript/dashboard/i18n/locale/en/inboxMgmt.json(1 hunks)app/javascript/dashboard/routes/dashboard/settings/inbox/channels/CloudWhatsapp.vue(4 hunks)app/jobs/update_last_seen_job.rb(1 hunks)app/jobs/webhook_job.rb(1 hunks)app/listeners/webhook_listener.rb(1 hunks)app/models/channel/whatsapp.rb(1 hunks)app/models/concerns/brazilian_number_validator.rb(1 hunks)app/models/contact.rb(1 hunks)app/models/contact_inbox.rb(1 hunks)app/models/conversation.rb(1 hunks)app/models/inbox.rb(1 hunks)app/models/message.rb(3 hunks)app/services/whatsapp/incoming_message_base_service.rb(7 hunks)app/services/whatsapp/incoming_message_whatsapp_cloud_service.rb(2 hunks)app/services/whatsapp/providers/base_service.rb(1 hunks)app/services/whatsapp/providers/whatsapp_360_dialog_service.rb(1 hunks)app/services/whatsapp/providers/whatsapp_cloud_service.rb(1 hunks)app/services/whatsapp/send_on_whatsapp_service.rb(1 hunks)config/initializers/rest_client/request.rb(1 hunks)lib/webhooks/trigger.rb(1 hunks)spec/jobs/agent_bots/webhook_job_spec.rb(1 hunks)spec/jobs/webhook_job_spec.rb(1 hunks)spec/models/conversation_spec.rb(1 hunks)spec/services/whatsapp/incoming_message_service_spec.rb(1 hunks)spec/services/whatsapp/providers/whatsapp_cloud_service_spec.rb(1 hunks)
🔇 Additional comments (40)
app/services/whatsapp/incoming_message_base_service.rb (7)
35-35: Good addition of set_message_type call.
Setting the message type early in the process avoids confusion and ensures uniformity in how messages are created.
47-50: Proper usage of a transaction block.
Wrapping the updates to the message within a transaction ensures that the creation of a failure message is rolled back if any subsequent operations fail, maintaining data integrity.
51-56: Conditional handling of 'deleted' status.
Overwriting the message content to indicate that a message was deleted is acceptable as long as the original content is intentionally replaced. Confirm that this aligns with user expectations for message history.
108-108: Assigning sender reference.
Linking the contact inbox contact to @sender appears consistent and ensures accurate attribution of the message origin.
112-112: Conversation retrieval logic.
This approach appropriately reuses the most recent conversation or creates a new one, preventing duplicates.
135-136: Check for missing location fields.
Ensure that location data (like 'url', 'address', 'name', 'latitude', 'longitude') is present. If any field can be missing, handle it gracefully.
145-145: Adopting instance variable for message_type.
Using @message_type clarifies the source of the type parameter and avoids confusion when creating messages.
app/models/inbox.rb (1)
145-147: Consider a safer fallback retrieval for the agent reply time window.
Currently, this line uses channel.additional_attributes['agent_reply_time_window'].to_i, which will return 0 when the key is not present. That might unintentionally override the intended fallback of 24 hours if the attribute is missing or nil. If the desired fallback is 24 hours, consider using something like:
def messaging_window
- api? ? channel.additional_attributes['agent_reply_time_window'].to_i : 24
+ return 24 unless api?
+ channel.additional_attributes&.fetch('agent_reply_time_window', 24).to_i
endThis approach ensures that if the attribute is missing or set to nil, the default of 24 hours is maintained.
spec/services/whatsapp/incoming_message_service_spec.rb (2)
79-96: LGTM! Well-structured test for read status updates.
The test case follows good practices with clear variable naming, proper test data setup, and explicit status change verification.
Line range hint 4-7: Verify webhook stubbing configuration.
The test stubs the webhook configuration request but doesn't verify if the stub is actually used. Consider adding expectations to verify the webhook interaction:
before do
stub_request(:post, 'https://waba.360dialog.io/v1/configs/webhook')
+ .to_return(status: 200, body: "", headers: {})
end
+
+ after do
+ expect(WebMock).to have_requested(:post, 'https://waba.360dialog.io/v1/configs/webhook')
+ endapp/jobs/update_last_seen_job.rb (2)
1-3: Great use of background job for asynchronous tasks.
The class inheritance from ApplicationJob and the specified queue_as :default are aligned with Rails conventions, ensuring that the job is queued and processed asynchronously. This promotes a scalable approach when handling multiple concurrent jobs.
4-11: Validate concurrency handling when updating message statuses.
When updating multiple messages in a loop (line 8-9), consider potential race conditions if other processes or jobs modify the same messages concurrently. Although Active Record ensures row-level updates, verifying concurrency strategies (e.g., optimistic locking or patch updates) might be beneficial for systems with high write contention.
app/models/contact_inbox.rb (1)
67-67: Enhanced WhatsApp source ID support with substring check.
Allowing '@g.us' in the source_id ensures group conversation coverage. This aligns well if the broader application needs to handle group chats. Confirm that your UI and business logic also handle group chat states effectively.
app/controllers/api/v1/accounts/conversations_controller.rb (1)
96-96: Asynchronous update can improve responsiveness but verify job success.
Calling UpdateLastSeenJob.perform_later asynchronously prevents blocking. However, ensure you handle potential job failures, especially if the conversation or messages are modified while the job is in the queue. Incorporating re-try or error-handling logic in the job can help maintain consistency.
.gitlab-ci.yml (2)
15-15: Handle missing or non-prefixed tags gracefully.
Relying on the “v” prefix in tags (e.g., “v1.0.0”) can fail if the tag doesn’t match that format. Consider adding a fallback or validating the extracted tag to handle unexpected cases.
18-23: Validate Docker image references before pushing.
While this process is standard, ensure that both the versioned and latest Docker image references are valid. A missing or blank “TAG_NAME” could push an undesired tag, potentially overwriting a precious image.
app/services/whatsapp/send_on_whatsapp_service.rb (1)
93-99: Improve UUID matching & consider E.164 format consistency.
- The current UUID regex is strictly lowercase. Include uppercase characters if needed:
/^[0-9A-Fa-f]{8}-[0-9A-Fa-f]{4}-.../ - Using “sub('+', '')” removes only the first “+”. If the string somehow contained multiple pluses, it wouldn’t remove them.
- Removing the plus sign might conflict with the standard E.164 format, especially if your system depends on phone numbers retaining “+.” Reassess whether you want to strip it for Brazilian phone numbers or if the service or external API forbids the plus sign.
app/services/whatsapp/incoming_message_whatsapp_cloud_service.rb (3)
7-28: Handle potential nil references and strengthen group-based contact creation.
In the group message section, there's a reliance on @processed_params[:messages].first[:from] and contact_params[:wa_id]. If the :messages array is empty or the :wa_id is missing, this could result in a NoMethodError or a mismatch in the contact creation logic. Consider adding guard clauses or default fallbacks to ensure robust handling for unexpected scenarios.
46-49: Avoid potential NoMethodError on :wa_id.
If contact_params[:wa_id] is unexpectedly nil, the include?("@g.us") call will raise an exception. Consider using contact_params[:wa_id]&.include?("@g.us") or a short-circuit check to prevent errors.
51-56: Confirm default message_type handling.
By defaulting @message_type to :activity and only overriding it if activity_message_type? returns false, the logic may inadvertently classify certain messages as activities. Ensure that the fallback scenario is justified, and consider logging or clarifying the reason for classifying as :activity by default.
app/services/whatsapp/providers/whatsapp_cloud_service.rb (2)
47-53: Check for presence of message[:status] and message[:source_id].
The message_update_payload method references message[:status] and message[:source_id]. If these keys are missing or nil, it may cause unexpected behavior downstream. Ensure you handle missing keys gracefully.
101-104: Ensure sender availability in format_content.
If sender is missing or has no name, you fallback to message.content. If the usage scenario requires the presence of a name, consider logging or adding a default name to avoid confusion in logs or user interfaces.
app/builders/messages/message_builder.rb (3)
79-84: Gracefully handle nil contact in group_incoming? scenario.
If contact_sender returns nil, messages might end up with a nil sender. This could cause confusion or errors later on. Consider defining a fallback or logging the scenario when the contact is not found.
86-90: Confirm that checking @conversation.contact.email is sufficient for group detection.
The code looks for @g.us in the email to decide if it’s a group. Make sure you are not missing other relevant checks, or if the contact’s email can be absent or in a format that doesn’t contain @g.us.
122-124: Ensure source_id is valid.
In source_id_param, you rely solely on the presence of @params[:source_id]. Consider type or format checks if source_id is used in subsequent logic or external API calls.
spec/jobs/agent_bots/webhook_job_spec.rb (1)
18-18: Test covers the new parameters well.
Validating that Webhooks::Trigger.execute is called with url, payload, :post, and the headers ensures coverage of the expanded signature. Good job adding the additional arguments in the expectations.
spec/jobs/webhook_job_spec.rb (1)
18-19: Good test coverage for updated parameters
The test now correctly expects the new parameters passed to Webhooks::Trigger.execute, ensuring consistency with the updated method signature.
app/services/whatsapp/providers/base_service.rb (3)
22-24: Abstract method for updating message payload
Raising an exception here is a good pattern for enforcing implementation in subclasses. Ensure child classes override this properly.
26-28: HTTP method choice
Defaulting to :put is fine for updates. Just confirm that all downstream calls handle PUT semantics as expected (e.g., idempotency).
30-32: Abstract method for message path
Like the message update payload, this enforces that each provider must define its own path. Good for maintainability.
app/models/channel/whatsapp.rb (2)
53-53: Delegation approach appears consistent.
Delegating these methods to the provider service centralizes logic in one place, which improves modularity and maintainability. Ensure that the delegated methods exist on all relevant provider services, or handle any exceptions if some providers don’t implement them.
59-61: Validate the presence of underlying keys.
These methods (message_path, message_update_payload, message_update_http_method) rely on underlying keys in the payload. Make sure that the provider service gracefully handles missing or unexpected keys to avoid potential runtime errors or exceptions.
app/services/whatsapp/providers/whatsapp_360_dialog_service.rb (1)
48-50: Validate required keys before constructing the message update payload.
If message[:status] is missing or nil, this might cause issues in downstream processes. Consider adding a validation step or defaulting to a known status.
app/listeners/webhook_listener.rb (1)
102-102: Approach is consistent and clear.
Delivering WhatsApp-specific webhooks here maintains a clean separation of concerns among different webhook flows. This ensures better maintainability and clarity in code.
app/javascript/dashboard/routes/dashboard/settings/inbox/channels/CloudWhatsapp.vue (3)
88-107: Conditional advanced toggle is well-implemented.
Toggling the “advanced” mode to enable/disable the URL input helps prevent accidental edits. If future requirements arise, you might also consider adding inline validation to alert users when the URL isn’t in a valid format.
134-134: Advanced flag logic is straightforward.
Initializing “advanced” to false ensures the feature is off by default. This approach is user-friendly.
168-168: Payload extension is well-structured.
Storing the user-specified URL under provider_config is consistent with how other settings are stored. If certain providers do not use this URL, consider verifying its necessity before sending.
app/models/conversation.rb (1)
104-105: Confirm no unintended bypass of other checks.
Introducing an immediate return for unofficial WhatsApp Cloud channels may skip other important checks in can_reply?. Evaluate whether additional gating conditions (e.g., conversation status, permission checks) are needed before returning true.
app/models/message.rb (2)
64-64: Evaluate potential concurrency considerations.
Introducing the progress status might require ensuring that concurrent updates (e.g., from webhooks) don't incorrectly overwrite progress states. Confirm that transitions to or from progress are synchronized with your business logic.
150-150: Expose complete status data in webhooks.
Including status in the webhook data is great for tracking message progress. Be sure to confirm that downstream systems can handle the new progress state correctly.
|
|
||
| phone = Phonelib.parse(value) | ||
|
|
||
| return if phone.country != 'BR' || phone.valid? || phone.type != :mobile || phone.local_number.scan(/\d/).join.length >= 11 |
There was a problem hiding this comment.
Logical operator mismatch may prevent intended validation checks.
The condition merges multiple requirements (country == 'BR', phone.valid?, type == :mobile, local_number length >= 11) with the OR operator (||). Currently, if phone.valid? is true or phone.type != :mobile is false, the method simply returns and avoids adding an error. This appears reversed from the intended logic of rejecting invalid phone numbers. You might want to invert or separate conditions to ensure phone meets all the intended criteria.
- return if phone.country != 'BR' || phone.valid? || phone.type != :mobile || phone.local_number.scan(/\d/).join.length >= 11
+ return if phone.country == 'BR' && phone.valid? && phone.type == :mobile && phone.local_number.scan(/\d/).join.length >= 11📝 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.
| return if phone.country != 'BR' || phone.valid? || phone.type != :mobile || phone.local_number.scan(/\d/).join.length >= 11 | |
| return if phone.country == 'BR' && phone.valid? && phone.type == :mobile && phone.local_number.scan(/\d/).join.length >= 11 |
| "#{api_base_path}/v13.0/#{media_id}" | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistent API versions and domain checks.
The code uses "v13.0" in media_url, phone_id_path, and "v14.0" in business_account_path. This version mismatch can lead to inconsistent behavior. Also, the domain check in whatsapp_cloud_api_unofficial? might be too simplistic if the URL partly includes “graph.facebook.com.” Consider centralizing the API version in a configuration or environment variable and making the domain check more cautious.
Also applies to: 43-45
| def facebook_api_base_path | ||
| ENV.fetch('WHATSAPP_CLOUD_BASE_URL', 'https://graph.facebook.com') | ||
| end | ||
|
|
||
| def api_base_path | ||
| whatsapp_channel.provider_config['url'] || facebook_api_base_path | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate API path building.
facebook_api_base_path, api_base_path, phone_id_path, business_account_path, and messages_path each build dynamic URLs with different versions (v13.0 vs v14.0). Consider unifying them with a single version strategy or environment-driven approach to reduce maintenance complexity.
Also applies to: 75-79, 83-83
| # https://gist.github.com/bluefuton/5851093 | ||
| require 'json' | ||
| require 'uri' | ||
|
|
||
| RestClient::Request.class_eval do | ||
| def default_headers | ||
| Rails.logger.info ">>>>>>>>>>>>>>>>>>> WHATSAPP_CHANNEL_URLS: #{ENV.fetch('WHATSAPP_CHANNEL_URLS', nil)}" | ||
| @urls = JSON.parse(ENV.fetch('WHATSAPP_CHANNEL_URLS', '{}')).with_indifferent_access | ||
| Rails.logger.info ">>>>>>>>>>>>>>>>>>> @urls: #{@urls}" | ||
| uri = URI(@url) | ||
| key = "#{uri.scheme}://#{uri.host}" | ||
| Rails.logger.info ">>>>>>>>>>>>>>>>>>> @urls[#{key}]: #{@urls[key]}" | ||
| headers = {} | ||
| if @urls[key] | ||
| headers.store(@urls[key][:header_name], @urls[key][:header_value]) | ||
| Rails.logger.info ">>>>>>>>>>>>>>>>>>> make_headers for #{@urls[key]} with params #{headers}" | ||
| end | ||
| headers | ||
| end |
There was a problem hiding this comment.
Avoid logging environment variables in production
Logging the entire WHATSAPP_CHANNEL_URLS environment variable can expose sensitive information in production logs. Consider restricting detailed logging to development or debug mode for security.
Apply a selective logging approach as follows (only logs in development):
def default_headers
- Rails.logger.info ">>>>>>>>>>>>>>>>>>> WHATSAPP_CHANNEL_URLS: #{ENV.fetch('WHATSAPP_CHANNEL_URLS', nil)}"
+ Rails.logger.debug ">>>>>>>>>>>>>>>>>>> WHATSAPP_CHANNEL_URLS: #{ENV.fetch('WHATSAPP_CHANNEL_URLS', nil)}" if Rails.env.development?Committable suggestion skipped: line range outside the PR's diff.
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:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Enhancements
Tests