fix: retry WooCommerce SMS when every recipient send fails

[akzmoudud]

Closes getdokan/texty-pro#30

Problem

WC\Base::send() wrote the _texty_{id} idempotency flag before dispatching and ignored the gateway result. A transient gateway/network error left the order flagged "sent" forever, so the customer/admin SMS was silently lost and never retried. Two secondary defects compounded it: the flag was written with add_meta_data (append, not replace), and a failed send produced no signal.

Fix

• Capture the gateway result. Track $any_sent across the recipient loop (! is_wp_error( $result ) && false !== $result).
• Roll back on total failure. If every recipient failed, delete_meta_data( $meta_key ) and return false, so the next matching status change retries.
• Preserve at-most-once. A partial success keeps the flag — recipients who already got the SMS are not messaged again.
• No meta bloat. Claim with update_meta_data so exactly one _texty_{id} row exists per order/notification.
• Hook order preserved. texty_after_notification still fires in its original position (before the rollback), so the texty_before_notification / texty_after_notification pairing is unchanged.

The _texty_{id} meta key is unchanged (stable contract).

Acceptance criteria

• All recipients fail → _texty_{id} left unset → next matching status change retries.
• At least one recipient succeeds → flag stays set (no duplicate to successful recipients).
• Exactly one _texty_{id} meta row per order/notification (update_meta_data).
• Status flip-flop (processing → on-hold → processing) still fires each distinct notification id once.

Verification

• php -l clean; composer phpcs clean (0 errors) on the changed file.
• Manual UI repro: Twilio active with an invalid Auth Token → set order to Processing → send fails → _texty_order_customer_processing is absent (was permanently 1 before). Restore the token, re-fire Processing → SMS now sends.

Known limitations (out of scope, per the issue)

• Partial-failure retry: a recipient that fails inside an otherwise-successful batch is not retried (the issue intentionally keeps the flag on any success).
• Residual race: read-then-claim is still not atomic, so two concurrent woocommerce_order_status_changed firings could both claim and both send. Documented in the issue as an optional wp_cache_add() follow-up.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Reduced duplicate notification sends when order status changes happen at the same time.
  • Improved retry behavior so notifications can be sent again if all delivery attempts fail.
  • Kept successful sends marked as sent to avoid repeated notifications.

[coderabbitai]

📝 Walkthrough

Walkthrough

WC\Base::send() now updates the notification meta key before sending, tracks whether any recipient send succeeds, and deletes the meta claim when every send attempt fails so later status changes can retry.

Changes

WooCommerce SMS send claim

Layer / File(s)	Summary
Meta claim before send includes/Notifications/WC/Base.php	The send path switches from adding a new meta row to updating the existing idempotency key and initializes $any_sent before the gateway loop.
Recipient outcome tracking and claim rollback includes/Notifications/WC/Base.php	The gateway loop sets $any_sent only on non-error, non-false results, and the method deletes the meta key and returns false when no recipient send succeeds.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

A bunny hopped through byte and box,
Claimed the flag, then checked the knocks.
If every send fell into gloom,
The retry carrot sprouted bloom. 🐰

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely matches the main change: retrying WooCommerce SMS after total send failure.
Linked Issues check	✅ Passed	The changes satisfy #30 by rolling back the flag on total failure, preserving it on partial success, and using update_meta_data to keep one row.
Out of Scope Changes check	✅ Passed	The PR stays focused on the WooCommerce SMS idempotency and retry fix without introducing unrelated changes.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/failed-wc-sms-marked-sent

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@includes/Notifications/WC/Base.php`:
- Around line 142-145: The claim set in Base::sendNotification should be
released when no recipients remain after filtering, because the early return
path leaves the notification marked as sent even though nothing was delivered.
Update the no-recipient branch in Base::sendNotification so it clears the
_texty_{id} meta before returning, while keeping the existing claim/save
behavior for the normal send path.

🪄 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

Run ID: 82784f18-cbf2-4aaa-be27-cf4c54fd09b1

📥 Commits

Reviewing files that changed from the base of the PR and between 5d20bb9 and b75fe2a.

📒 Files selected for processing (1)

• includes/Notifications/WC/Base.php

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Release the claim on the no-recipient path.

Line 144 persists _texty_{id}, but Line 169 returns when all recipients are filtered out. That leaves the notification marked sent even though nobody received an SMS, so later matching status changes will not retry.

Proposed fix

         if ( ! $recipients ) {
+            $this->order->delete_meta_data( $meta_key );
+            $this->order->save_meta_data();
+
             return false;
         }

Also applies to: 168-170

🤖 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 `@includes/Notifications/WC/Base.php` around lines 142 - 145, The claim set in
Base::sendNotification should be released when no recipients remain after
filtering, because the early return path leaves the notification marked as sent
even though nothing was delivered. Update the no-recipient branch in
Base::sendNotification so it clears the _texty_{id} meta before returning, while
keeping the existing claim/save behavior for the normal send path.