Issue #922: WhatsApp Business Cloud API messaging as a plugin - #4606
Issue #922: WhatsApp Business Cloud API messaging as a plugin#4606joshua1234511 wants to merge 5 commits into
Conversation
Plugins can already inject buttons into the receipt via view:sales_receipt_buttons, but the invoice, quote and work order views have no hook point, so a plugin that delivers sale documents can only reach one of the four. Add the matching hooks, keeping the same name/data shape: view:sales_invoice_buttons view:sales_quote_buttons view:sales_work_order_buttons Each passes ['saleId' => $sale_id_num] and sits in the same position as the existing receipt hook, so one plugin callback can serve all four and branch on the document type. Also allow a plugin to expose an inbound provider webhook. A server-to-server delivery carries no CSRF token and no session, so the global csrf filter now excludes the pattern plugins/*/webhook. Plugins are responsible for authenticating such requests themselves (typically by verifying the provider's signature header against a shared secret) — this is documented alongside the change. While editing that line, the except list becomes an array. CodeIgniter matches every entry as \A<pattern>\z, so the previous 'login|migrate' string produced a single pattern whose inner alternatives were unanchored: 'login/anything' was CSRF-exempt. Listing the entries separately anchors each one, which closes that and keeps the new plugin pattern tight — plugins/whatsapp/webhook is exempt while plugins/whatsapp/send is not.
) Reimplements the WhatsApp messaging feature from opensourcepos#4590 as a self-contained plugin, as requested in review. No core file is touched. app/Plugins/WhatsappPlugin/ provides: - Free-form messaging page, registered as the 'whatsapp' office module with its own permission, plus a per-person modal and a conversation thread view. - A "Send via WhatsApp" button on the receipt, invoice, quote and work order, injected through the sales document view hooks. The button renders only when the sale's customer has a phone number. - Delivery of the sale document PDF — the same sales/{type}_email view core uses for the emailed attachment. - A public webhook at plugins/whatsapp/webhook for inbound replies and delivery status callbacks, authenticated by X-Hub-Signature-256 HMAC against the app secret. It fails closed: an unconfigured secret or a bad signature persists nothing, and it always answers 200 so Meta does not retry. - A conversation log table created by a plugin migration and dropped on uninstall, with the migration version reset so a re-install recreates it. - Access token and app secret encrypted at rest in plugin_config; no settings are written to app_config and initial_schema.sql is untouched. Behaviour carried over from opensourcepos#4590: the document type is whitelisted before it reaches a view path, the temp PDF uses a unique name and is always unlinked, outbound message bodies are sent verbatim (WhatsApp renders plain text), and out-of-order status callbacks cannot downgrade sent -> delivered -> read. Depends on the sale document view hooks and the plugins/*/webhook CSRF exemption. English strings only; other locales can follow.
|
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 ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesWhatsApp integration
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant WhatsAppBusinessCloud
participant WebhookController
participant WhatsAppMessage
participant WhatsAppController
participant WhatsAppConnector
WhatsAppBusinessCloud->>WebhookController: Send verified webhook event
WebhookController->>WhatsAppMessage: Log inbound message or update status
WhatsAppController->>WhatsAppConnector: Send text or document
WhatsAppConnector->>WhatsAppBusinessCloud: Submit Graph API request
WhatsAppConnector->>WhatsAppMessage: Log outbound message
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
@jekkos I'm not sure if you saw my other messages regarding this but we need to decide on how to proceed with storing plugins. I had written the Mailchimp plugin as an example plugin, so keeping it in the opensourcepos/opensourcepos repository seemed to make sense at the time, but we need to come up with a better way of organizing and maintaining them. There are two issues to answer: 1. Where is the code stored? 2. How are the plugins advertised? I'm only asking about the first question here: Private/monetized plugins should be stored and maintained in their developer's own repos. public/free plugins
How do you think we should proceed? |
|
@joshua1234511 wrote in the closed PR:
I will look at the Sales::load_sale_data() method when I review this PR. If it's safe to do so, we will make it public. A note is that we have API endpoints on the roadmap so eventually we will require plugins to use the API endpoints only to get data, which will more safely gate the core logic and make plugins more maintainable. Currently using core functions to get data has a major downside that if the contract changes, then it breaks downstream plugins that use them. |
Lets go for option 1. we can keep the source together.and then release plugins as we choose. eg next to a full bundle we can then also create a lighter package with core only |
objecttothis
left a comment
There was a problem hiding this comment.
This is just as far as I got for now. I'll work on it more later. Also, note that I just pushed a few commits to the plugin branch which assist in the module creation. Make sure that your code reflects the expected implementation of the module in the README.md for plugins. I didn't get a chance to see if you had the module icon the way it's expecting and registering the module in your install.
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
app/Plugins/WhatsappPlugin/Config/Routes.php (1)
15-15: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider POST for
sendDocument. It performs an external side effect (sends a PDF to a customer) but is reachable via GET, so it is not covered by CSRF protection and can be triggered by any cross-site image/link load in an authenticated session. Core'ssales/sendPdfhas the same shape, so this is a consistency-vs-safety tradeoff worth a deliberate decision.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Plugins/WhatsappPlugin/Config/Routes.php` at line 15, Change the sendDocument route registration to POST-only for WhatsappController::getSendDocument, ensuring the side-effecting endpoint is covered by CSRF protection. Preserve its existing URL pattern and controller parameters, and update the handler naming only if required by the framework’s POST convention.app/Plugins/WhatsappPlugin/Models/WhatsappMessage.php (1)
59-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMethod names use snake_case instead of camelCase.
get_conversation(),get_recent_conversations(), andupdate_status()use snake_case. As per coding guidelines,**/*.phpfiles should "use camelCase for variables and methods." Renaming would also require updating the call sites inWhatsappControllerandWebhookController.♻️ Proposed rename
- public function get_conversation(string $phone, int $limit = 200): ResultInterface + public function getConversation(string $phone, int $limit = 200): ResultInterface @@ - public function get_recent_conversations(int $limit = 50): array + public function getRecentConversations(int $limit = 50): array @@ - public function update_status(string $waMessageId, string $status): bool + public function updateStatus(string $waMessageId, string $status): bool🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Plugins/WhatsappPlugin/Models/WhatsappMessage.php` around lines 59 - 124, Rename the WhatsappMessage methods get_conversation, get_recent_conversations, and update_status to camelCase equivalents, then update every corresponding call site in WhatsappController and WebhookController. Preserve each method’s existing behavior and parameters while ensuring all references use the new names consistently.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Config/Filters.php`:
- Around line 76-85: Tighten the CSRF exemption in app/Config/Filters.php lines
76-85 to match only plugins/<single-segment>/webhook, preserving the comment’s
anchored/exact-path wording. Update app/Plugins/README.md lines 629-630 to
describe the same single-segment exemption behavior.
In `@app/Plugins/WhatsappPlugin/Controllers/WebhookController.php`:
- Line 63: Update the verify-token handling in WebhookController to safely read
a missing verify_token setting without triggering an undefined-key warning,
while preserving the existing invalid-token path that returns a clean 403
response.
In `@app/Plugins/WhatsappPlugin/Controllers/WhatsappController.php`:
- Around line 151-159: Update the document-generation flow in WhatsappController
and SaleDocument::renderPdf to distinguish missing phone numbers from
temporary-file/PDF creation failures instead of treating every null result as
no_phone. Return or propagate an explicit failure reason from renderPdf, then
select the corresponding UI message and accurate log entry in the controller
while preserving cart cleanup and response structure.
In `@app/Plugins/WhatsappPlugin/Libraries/SaleDocument.php`:
- Around line 86-126: Update buildData() and the number selection in renderPdf()
so receipt documents fall back to $data['receipt_num'] when receipt_number is
absent, while work_order documents populate work_order_number from the existing
work-order number source. Ensure the resolved number is used for both the PDF
display_name and buildCaption() so receipt filenames and captions are populated
correctly.
In `@app/Plugins/WhatsappPlugin/Libraries/WhatsappConnector.php`:
- Around line 157-173: Update normalizePhone so the country-prefix branch
removes at most one leading trunk zero from digits before prepending the country
code; do not use an operation that strips multiple zeros, and preserve the
existing behavior for numbers without a leading zero or when the country code is
already present.
In `@app/Plugins/WhatsappPlugin/WhatsappPlugin.php`:
- Around line 237-246: Update encryptSetting() to fail closed when encryption
throws: do not return the raw $value or permit saveSettings() to persist it, and
instead propagate an appropriate exception or otherwise abort the save operation
after logging the failure. Preserve the encrypted return path for successful
encryption and ensure token/app_secret storage cannot continue with plaintext
values.
- Around line 118-132: Make the enabled state consistent by updating runtime
guards such as connector(), controllers, and hooks to use the storage-backed
enabled setting returned by getSettings(), or remove that setting and route all
consumers through BasePlugin::isEnabled(). Ensure install(), getSettings(), and
every runtime gate share one source of truth rather than separate plugin and
global toggle values.
---
Nitpick comments:
In `@app/Plugins/WhatsappPlugin/Config/Routes.php`:
- Line 15: Change the sendDocument route registration to POST-only for
WhatsappController::getSendDocument, ensuring the side-effecting endpoint is
covered by CSRF protection. Preserve its existing URL pattern and controller
parameters, and update the handler naming only if required by the framework’s
POST convention.
In `@app/Plugins/WhatsappPlugin/Models/WhatsappMessage.php`:
- Around line 59-124: Rename the WhatsappMessage methods get_conversation,
get_recent_conversations, and update_status to camelCase equivalents, then
update every corresponding call site in WhatsappController and
WebhookController. Preserve each method’s existing behavior and parameters while
ensuring all references use the new names consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f00cb1d-ef29-4c32-8c6d-f12d501b04e8
📒 Files selected for processing (21)
app/Config/Filters.phpapp/Plugins/README.mdapp/Plugins/WhatsappPlugin/Config/Routes.phpapp/Plugins/WhatsappPlugin/Controllers/WebhookController.phpapp/Plugins/WhatsappPlugin/Controllers/WhatsappController.phpapp/Plugins/WhatsappPlugin/LICENSEapp/Plugins/WhatsappPlugin/Language/en/Module.phpapp/Plugins/WhatsappPlugin/Language/en/WhatsappPlugin.phpapp/Plugins/WhatsappPlugin/Libraries/SaleDocument.phpapp/Plugins/WhatsappPlugin/Libraries/WhatsappConnector.phpapp/Plugins/WhatsappPlugin/Migrations/20260728120000_CreateWhatsappMessagesTable.phpapp/Plugins/WhatsappPlugin/Models/WhatsappMessage.phpapp/Plugins/WhatsappPlugin/Views/config.phpapp/Plugins/WhatsappPlugin/Views/conversation.phpapp/Plugins/WhatsappPlugin/Views/form_whatsapp.phpapp/Plugins/WhatsappPlugin/Views/sale_document_button.phpapp/Plugins/WhatsappPlugin/Views/whatsapp.phpapp/Plugins/WhatsappPlugin/WhatsappPlugin.phpapp/Views/sales/invoice.phpapp/Views/sales/quote.phpapp/Views/sales/work_order.php
| public function renderPdf(int $saleId, string $type): ?array | ||
| { | ||
| $data = $this->buildData($saleId); | ||
|
|
||
| $phone = (string) ($data['customer_phone'] ?? ''); | ||
|
|
||
| if ($phone === '') { | ||
| return null; | ||
| } | ||
|
|
||
| $number = $data[$type . '_number'] ?? ''; | ||
|
|
||
| $emailLib = new Email_lib(); | ||
| $data['mimetype'] = $emailLib->getLogoMimeType(); | ||
| $data['img_tag'] = $emailLib->buildLogoImgTag(); | ||
|
|
||
| // Same view core uses for the emailed attachment. | ||
| $html = Services::renderer()->setData($data)->render("sales/{$type}_email", $data); | ||
|
|
||
| helper(['dompdf', 'file']); | ||
|
|
||
| // A unique temp name avoids a TOCTOU race with core's getSendPdf(), which | ||
| // writes to a deterministic path. The recipient still sees a friendly name. | ||
| $path = tempnam(sys_get_temp_dir(), 'wa_'); | ||
|
|
||
| if ($path === false || file_put_contents($path, create_pdf($html)) === false) { | ||
| if ($path !== false && is_file($path)) { | ||
| unlink($path); | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| return [ | ||
| 'path' => $path, | ||
| 'display_name' => lang('Sales.' . $type) . '-' . str_replace('/', '-', (string) $number) . '.pdf', | ||
| 'caption' => $this->buildCaption($data, (string) $number), | ||
| 'phone' => $phone, | ||
| 'person_id' => isset($data['customer_id']) ? (int) $data['customer_id'] : null, | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the existing sale number for receipts and populate work_order_number.
buildData() only sets invoice_number and quote_number; for receipt and work_order, $data[$type . '_number'] falls back to '', producing filenames like "Receipt-.pdf" and an empty Token_invoice_sequence in the caption. Fall back to the receipt’s existing number from $data['receipt_num'] where available, and ensure work_order has the correct work_order_number source.
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 112-112: Avoid unsafe call to unlink
Context: unlink($path)
Note: [CWE-73] External Control of File Name or Path.
(avoid-unlink)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Plugins/WhatsappPlugin/Libraries/SaleDocument.php` around lines 86 - 126,
Update buildData() and the number selection in renderPdf() so receipt documents
fall back to $data['receipt_num'] when receipt_number is absent, while
work_order documents populate work_order_number from the existing work-order
number source. Ensure the resolved number is used for both the PDF display_name
and buildCaption() so receipt filenames and captions are populated correctly.
Rename to WhatsAppPlugin, switch to plugin-specific logging, trim explanatory comments, and scope the LICENSE to the plugin author. Add the module icon: SVG asset, module_icon view, injectModuleIcon() handler, and a public IconController so the image is not auth-gated. Fix: work_order_number was never populated for work_order_email; receipt filenames fell back to an empty number; normalizePhone stripped every leading zero rather than the single trunk zero; a failed setting encryption stored the secret as plaintext; no_phone was reported for PDF generation failures; drop the redundant enabled setting so BasePlugin::isEnabled() is the only source of truth. Revert the app/Config/Filters.php comment so the PR touches no core files.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
app/Plugins/WhatsAppPlugin/LICENSE (1)
23-26: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftUse the license named in this file.
Lines 23-26 add restrictions that are not part of the MIT License. The file is therefore not an MIT License, despite its title.
Remove these terms, or rename the file as a custom license and obtain approval for its distribution terms.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Plugins/WhatsAppPlugin/LICENSE` around lines 23 - 26, Remove the added restrictions in the LICENSE file so its terms match the MIT License, including the clauses prohibiting copyright or ownership claims and requiring unmodified visible credit on every page; preserve the standard MIT license text and title.app/Plugins/WhatsAppPlugin/WhatsAppPlugin.php (2)
135-163: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestrict the persisted API base URL.
saveSettings()accepts an arbitraryapi_url, andconnector()passes it toWhatsAppConnectorfor outbound API calls. A user who can save plugin settings can redirect server-side requests to an attacker-controlled or internal host.Require HTTPS and an allowlisted WhatsApp Graph API host before saving
api_url. Also reject unknown setting keys.As per coding guidelines, “Validate and sanitize all user input.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Plugins/WhatsAppPlugin/WhatsAppPlugin.php` around lines 135 - 163, Update WhatsAppPlugin::saveSettings to reject keys not defined by the plugin, and validate api_url before persisting it. Require a valid HTTPS URL whose host matches the allowlisted WhatsApp Graph API host, while preserving the existing encryption and normalization behavior for accepted settings.Source: Coding guidelines
109-121: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not return stored secrets to the configuration form.
Lines 116 and 120 decrypt the access token and app secret. The configuration view writes both values into the browser DOM. This defeats encrypted-at-rest storage for any administrator browser session.
Render empty secret fields with a configured-state indicator. Preserve existing secrets when the field is empty. Add an explicit clear action.
Based on PR objectives, the access token and app secret require encrypted storage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Plugins/WhatsAppPlugin/WhatsAppPlugin.php` around lines 109 - 121, Update WhatsAppPlugin::getSettings and the configuration form flow so decrypted token and app_secret values are never returned to or rendered in the browser. Expose only empty secret fields plus configured-state indicators, preserve the existing encrypted secrets when submissions leave those fields empty, and add an explicit clear action that removes each secret when requested.app/Plugins/WhatsAppPlugin/Migrations/20260728120000_CreateWhatsAppMessagesTable.php (1)
43-44: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake
wa_message_idunique.
WebhookController::process()stores the Meta message ID for each inbound delivery. A retried signed webhook can insert the same message again because this is only a lookup index. Duplicate rows then appear in conversation history and message counts.Use a unique key for
wa_message_id. Keep failed outbound rows valid because nullable unique columns allow multipleNULLvalues.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Plugins/WhatsAppPlugin/Migrations/20260728120000_CreateWhatsAppMessagesTable.php` around lines 43 - 44, Change the wa_message_id index declaration in CreateWhatsAppMessagesTable to a unique key, while keeping the column nullable so multiple failed outbound rows with NULL remain valid. Do not alter the webhook or status-update logic.app/Plugins/WhatsAppPlugin/Libraries/WhatsAppConnector.php (1)
146-157: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve an explicit international prefix.
A number such as
+4420...becomes4420...before this condition. If the default country code is1, this code changes it to14420....Detect a leading
+before stripping punctuation. Do not prependdefault_country_codewhen the caller supplied an explicit international number.Proposed fix
public function normalizePhone(string $phone): string { + $hasInternationalPrefix = str_starts_with(ltrim($phone), '+'); $digits = preg_replace('/\D+/', '', $phone) ?? ''; - if ($country !== '' && ! str_starts_with($digits, $country)) { + if (! $hasInternationalPrefix && $country !== '' && ! str_starts_with($digits, $country)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Plugins/WhatsAppPlugin/Libraries/WhatsAppConnector.php` around lines 146 - 157, Update the phone normalization logic around $digits and $country to detect whether $phone begins with an explicit “+” before removing non-digit characters. Only prepend default_country_code for non-explicit numbers; preserve the existing trunk-zero handling and return behavior.app/Plugins/WhatsAppPlugin/Models/WhatsAppMessage.php (2)
51-88: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRename public model methods to camelCase.
Rename
get_conversation,get_recent_conversations, andupdate_statustogetConversation,getRecentConversations, andupdateStatus. Update the controller, connector, and webhook call sites in the same change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Plugins/WhatsAppPlugin/Models/WhatsAppMessage.php` around lines 51 - 88, Rename the public WhatsAppMessage methods get_conversation, get_recent_conversations, and update_status to getConversation, getRecentConversations, and updateStatus, respectively. Update every controller, connector, and webhook call site to use the new camelCase names while preserving existing behavior.Source: Coding guidelines
92-108: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake status progression atomic.
Two webhook requests can both read
sent. If thereadrequest writes first, a delayeddeliveredrequest can still overwrite it because its rank check used stale data.Lock the message row in a transaction before the rank check, or use a conditional update that only permits forward transitions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Plugins/WhatsAppPlugin/Models/WhatsAppMessage.php` around lines 92 - 108, Make the status transition in the WhatsAppMessage method containing the current rank check atomic: use a transaction with a row lock before reading and comparing the existing status, or perform a conditional update that only allows a higher-ranked status. Ensure delayed lower-ranked webhook requests cannot overwrite a newer status.app/Plugins/WhatsAppPlugin/Libraries/SaleDocument.php (1)
154-176: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve the active register state before rendering a document.
clear_all()deletes the current session cart beforecopy_entire_sale()loads the historical sale. The controller later callsclearCart(), so it leaves the cart empty instead of restoring the active sale. This also overwritescash_adjustment_amount.A user who sends an older document while another sale is open in the same session can lose the active cart. Render from isolated sale data, or snapshot and restore the complete
Sale_liband session state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Plugins/WhatsAppPlugin/Libraries/SaleDocument.php` around lines 154 - 176, Update the document-rendering flow around clear_all(), copy_entire_sale(), and clearCart() to isolate historical sale data from the active register state. Snapshot and restore the complete Sale_lib cart state and related session values, including cash_adjustment_amount, so sending an older document never empties or alters the user’s active sale.app/Plugins/WhatsAppPlugin/Controllers/WhatsAppController.php (1)
134-173: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse POST and CSRF protection for document delivery.
The flow sends a WhatsApp document through GET. A cross-site navigation can invoke this state-changing operation with an authenticated session.
app/Plugins/WhatsAppPlugin/Controllers/WhatsAppController.php#L134-L173: replacegetSendDocument()with a POST-only endpoint that validates CSRF.app/Plugins/WhatsAppPlugin/Views/sale_document_button.php#L27-L34: replace$.get()with a CSRF-protected POST request.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Plugins/WhatsAppPlugin/Controllers/WhatsAppController.php` around lines 134 - 173, Update app/Plugins/WhatsAppPlugin/Controllers/WhatsAppController.php lines 134-173 by replacing getSendDocument() with a POST-only, CSRF-validated endpoint while preserving its document delivery behavior. Update app/Plugins/WhatsAppPlugin/Views/sale_document_button.php lines 27-34 to replace the $.get() call with a CSRF-protected POST request that supplies the required sale data.app/Plugins/WhatsAppPlugin/Views/sale_document_button.php (1)
10-18: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse a button for the document-send control.
Keyboard activation targets the anchor. The click handler is attached to the nested
div, so keyboard users cannot send the document. Replace the anchor anddivwith one<button type="button">element.Proposed fix
-<a href="javascript:void(0);"> - <div class="btn btn-success btn-sm" id="show_whatsapp_button"> +<button type="button" class="btn btn-success btn-sm" id="show_whatsapp_button"> <?= '<span class="glyphicon glyphicon-comment"> </span>' . lang('WhatsAppPlugin.send_whatsapp') ?> - </div> -</a> +</button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Plugins/WhatsAppPlugin/Views/sale_document_button.php` around lines 10 - 18, Replace the anchor and nested div surrounding `#show_whatsapp_button` with a single button type="button" element, preserving the existing id, classes, label, and click handler so keyboard activation triggers document sending.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Plugins/WhatsAppPlugin/Config/Routes.php`:
- Line 14: Change the sendDocument route invoking
WhatsAppController::getSendDocument from GET to POST, enable the framework’s
standard CSRF protection for the endpoint, and update the corresponding button
request and controller action to use POST while preserving the existing
document-sending behavior.
In `@app/Plugins/WhatsAppPlugin/Views/config.php`:
- Around line 200-203: Update the localized values in the messages object to
emit valid JavaScript values via json_encode() with HTML-safe flags, covering
both WhatsAppPlugin.phone_id_required and WhatsAppPlugin.token_required so
apostrophes and HTML-sensitive characters cannot break the script.
In `@app/Plugins/WhatsAppPlugin/Views/form_whatsapp.php`:
- Around line 83-90: Update the localized validation messages in the messages
configuration to safely encode each lang() value for JavaScript, using
json_encode() together with the esc() helper before output. Apply this to the
phone.required, phone.number, and message.required values while preserving their
existing translation keys and validation structure.
In `@app/Plugins/WhatsAppPlugin/Views/whatsapp.php`:
- Around line 31-39: Escape both localized placeholder values in
app/Plugins/WhatsAppPlugin/Views/whatsapp.php lines 31-39 with esc() before
emitting them in the phone and message input attributes. Also escape the
localized alt value in app/Plugins/WhatsAppPlugin/Views/module_icon.php line 7
with esc().
---
Outside diff comments:
In `@app/Plugins/WhatsAppPlugin/Controllers/WhatsAppController.php`:
- Around line 134-173: Update
app/Plugins/WhatsAppPlugin/Controllers/WhatsAppController.php lines 134-173 by
replacing getSendDocument() with a POST-only, CSRF-validated endpoint while
preserving its document delivery behavior. Update
app/Plugins/WhatsAppPlugin/Views/sale_document_button.php lines 27-34 to replace
the $.get() call with a CSRF-protected POST request that supplies the required
sale data.
In `@app/Plugins/WhatsAppPlugin/Libraries/SaleDocument.php`:
- Around line 154-176: Update the document-rendering flow around clear_all(),
copy_entire_sale(), and clearCart() to isolate historical sale data from the
active register state. Snapshot and restore the complete Sale_lib cart state and
related session values, including cash_adjustment_amount, so sending an older
document never empties or alters the user’s active sale.
In `@app/Plugins/WhatsAppPlugin/Libraries/WhatsAppConnector.php`:
- Around line 146-157: Update the phone normalization logic around $digits and
$country to detect whether $phone begins with an explicit “+” before removing
non-digit characters. Only prepend default_country_code for non-explicit
numbers; preserve the existing trunk-zero handling and return behavior.
In `@app/Plugins/WhatsAppPlugin/LICENSE`:
- Around line 23-26: Remove the added restrictions in the LICENSE file so its
terms match the MIT License, including the clauses prohibiting copyright or
ownership claims and requiring unmodified visible credit on every page; preserve
the standard MIT license text and title.
In
`@app/Plugins/WhatsAppPlugin/Migrations/20260728120000_CreateWhatsAppMessagesTable.php`:
- Around line 43-44: Change the wa_message_id index declaration in
CreateWhatsAppMessagesTable to a unique key, while keeping the column nullable
so multiple failed outbound rows with NULL remain valid. Do not alter the
webhook or status-update logic.
In `@app/Plugins/WhatsAppPlugin/Models/WhatsAppMessage.php`:
- Around line 51-88: Rename the public WhatsAppMessage methods get_conversation,
get_recent_conversations, and update_status to getConversation,
getRecentConversations, and updateStatus, respectively. Update every controller,
connector, and webhook call site to use the new camelCase names while preserving
existing behavior.
- Around line 92-108: Make the status transition in the WhatsAppMessage method
containing the current rank check atomic: use a transaction with a row lock
before reading and comparing the existing status, or perform a conditional
update that only allows a higher-ranked status. Ensure delayed lower-ranked
webhook requests cannot overwrite a newer status.
In `@app/Plugins/WhatsAppPlugin/Views/sale_document_button.php`:
- Around line 10-18: Replace the anchor and nested div surrounding
`#show_whatsapp_button` with a single button type="button" element, preserving the
existing id, classes, label, and click handler so keyboard activation triggers
document sending.
In `@app/Plugins/WhatsAppPlugin/WhatsAppPlugin.php`:
- Around line 135-163: Update WhatsAppPlugin::saveSettings to reject keys not
defined by the plugin, and validate api_url before persisting it. Require a
valid HTTPS URL whose host matches the allowlisted WhatsApp Graph API host,
while preserving the existing encryption and normalization behavior for accepted
settings.
- Around line 109-121: Update WhatsAppPlugin::getSettings and the configuration
form flow so decrypted token and app_secret values are never returned to or
rendered in the browser. Expose only empty secret fields plus configured-state
indicators, preserve the existing encrypted secrets when submissions leave those
fields empty, and add an explicit clear action that removes each secret when
requested.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 71197209-3a00-4df0-be36-00271aed7d0e
⛔ Files ignored due to path filters (1)
app/Plugins/WhatsAppPlugin/whatsapp.svgis excluded by!**/*.svg
📒 Files selected for processing (18)
app/Plugins/WhatsAppPlugin/Config/Routes.phpapp/Plugins/WhatsAppPlugin/Controllers/IconController.phpapp/Plugins/WhatsAppPlugin/Controllers/WebhookController.phpapp/Plugins/WhatsAppPlugin/Controllers/WhatsAppController.phpapp/Plugins/WhatsAppPlugin/LICENSEapp/Plugins/WhatsAppPlugin/Language/en/Module.phpapp/Plugins/WhatsAppPlugin/Language/en/WhatsAppPlugin.phpapp/Plugins/WhatsAppPlugin/Libraries/SaleDocument.phpapp/Plugins/WhatsAppPlugin/Libraries/WhatsAppConnector.phpapp/Plugins/WhatsAppPlugin/Migrations/20260728120000_CreateWhatsAppMessagesTable.phpapp/Plugins/WhatsAppPlugin/Models/WhatsAppMessage.phpapp/Plugins/WhatsAppPlugin/Views/config.phpapp/Plugins/WhatsAppPlugin/Views/conversation.phpapp/Plugins/WhatsAppPlugin/Views/form_whatsapp.phpapp/Plugins/WhatsAppPlugin/Views/module_icon.phpapp/Plugins/WhatsAppPlugin/Views/sale_document_button.phpapp/Plugins/WhatsAppPlugin/Views/whatsapp.phpapp/Plugins/WhatsAppPlugin/WhatsAppPlugin.php
| messages: { | ||
| phone_id: '<?= lang('WhatsAppPlugin.phone_id_required') ?>', | ||
| token: '<?= lang('WhatsAppPlugin.token_required') ?>' | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Encode localized messages as JavaScript values.
Lines 201 and 202 inject translated text into single-quoted JavaScript strings. A translation containing an apostrophe can break the configuration script.
Use json_encode() with HTML-safe flags for each message.
Proposed fix
- phone_id: '<?= lang('WhatsAppPlugin.phone_id_required') ?>',
- token: '<?= lang('WhatsAppPlugin.token_required') ?>'
+ phone_id: <?= json_encode(lang('WhatsAppPlugin.phone_id_required'), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT) ?>,
+ token: <?= json_encode(lang('WhatsAppPlugin.token_required'), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT) ?>📝 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.
| messages: { | |
| phone_id: '<?= lang('WhatsAppPlugin.phone_id_required') ?>', | |
| token: '<?= lang('WhatsAppPlugin.token_required') ?>' | |
| } | |
| messages: { | |
| phone_id: <?= json_encode(lang('WhatsAppPlugin.phone_id_required'), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT) ?>, | |
| token: <?= json_encode(lang('WhatsAppPlugin.token_required'), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT) ?> | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Plugins/WhatsAppPlugin/Views/config.php` around lines 200 - 203, Update
the localized values in the messages object to emit valid JavaScript values via
json_encode() with HTML-safe flags, covering both
WhatsAppPlugin.phone_id_required and WhatsAppPlugin.token_required so
apostrophes and HTML-sensitive characters cannot break the script.
| messages: { | ||
| phone: { | ||
| required: "<?= lang('WhatsAppPlugin.phone_number_required') ?>", | ||
| number: "<?= lang('WhatsAppPlugin.phone') ?>" | ||
| }, | ||
| message: { | ||
| required: "<?= lang('WhatsAppPlugin.message_required') ?>" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Encode localized validation messages for JavaScript.
These lang() values are inserted directly into JavaScript string literals. A translation containing a quote or newline can break the script. Encode each value for JavaScript, such as with json_encode(), before output.
As per coding guidelines, escape output using the esc() helper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Plugins/WhatsAppPlugin/Views/form_whatsapp.php` around lines 83 - 90,
Update the localized validation messages in the messages configuration to safely
encode each lang() value for JavaScript, using json_encode() together with the
esc() helper before output. Apply this to the phone.required, phone.number, and
message.required values while preserving their existing translation keys and
validation structure.
Source: Coding guidelines
objecttothis
left a comment
There was a problem hiding this comment.
I'm really looking forward to this feature. Good work. See my comments. Let me know if you have questions or pushback.
| $this->forge->addKey('wa_message_id'); | ||
|
|
||
| // utf8mb4 so emoji and full multilingual message content can be stored. | ||
| $this->forge->createTable('whatsapp_messages', true, [ |
There was a problem hiding this comment.
utf8mb4_0900_ai_ci collation should be used here. It's the current recommendation and what the core code will migrate the core database to.
There was a problem hiding this comment.
Can you post screenshots of these views in the conversation?
objecttothis
left a comment
There was a problem hiding this comment.
@joshua1234511 Excellent. Take a look at my suggestion for the function naming and then address the two outstanding @coderabbitai recommendations, then I will merge this branch. Also, I notice you don't have a /app/Plugins/WhatsAppPlugin/README.md That could be beneficial for users of the plugin if there is anything about installation, configuration or use of the plugin that users need to know. I'm not going to hold up the PR over it, but I recommend it.
| * | ||
| * @throws ReflectionException | ||
| */ | ||
| public function log(array $data): int |
There was a problem hiding this comment.
Just a readability thought here, but you have this function defined and it's public, but in the WhatsAppController you also have a private function with a different signature called log(). They are referenced differently with this->log() vs messageModel->log(). I recommend refactoring this function to something like storeWhatsAppMessage()
|
@joshua1234511 I did another review and marked all the addressed conversations as resolved. Just a few more conversations to resolve. Let me know if you have questions. |
OSPOS can email a customer a sale document but cannot message them on WhatsApp, which is the primary channel for many merchants. #4590 implemented this by editing core — new controllers, a core library and migration, a config tab, and the same button block pasted into four sales views — and was rejected in review for exactly that reason.
This is the same feature rebuilt as a self-contained plugin at app/Plugins/WhatsappPlugin/, touching no core file. It provides a messaging page as its own permissioned office module, a "Send via WhatsApp" button on all four sale documents injected through the view hooks, delivery of the sale document PDF, and a signature-verified webhook that records inbound replies and delivery-status callbacks so the conversation view shows both sides. The conversation log is created by a plugin migration and removed on uninstall; the access token and app secret are encrypted at rest.
Closes #922 . Supersedes #4590 .
Summary by CodeRabbit