Apply CodeRabbit review fixes to Back in Stock Notifications alpha#64329
Merged
Conversation
github-actions
Bot
added
the
plugin: woocommerce
2 tasks
opr
force-pushed
the
feature/back-in-stock-notifications
branch
from
eb9cdff to
453ee2b
Compare
Avoids 'already defined' warnings when WOOCOMMERCE_BIS_ALPHA_ENABLED is set at the wp-config level (which happens in the tests env via .wp-env.json on downstream branches).
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBack in Stock Notifications: removed the HasherHelper shim and migrated to WordPress Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (email link)
participant Controller as EmailActionController
participant Factory as Notification Factory
participant Notif as Notification
participant DB as Persistence/DB
participant Logger as Logger
rect rgba(135,206,235,0.5)
Client->>Controller: GET /?notification_id=...&email_link_action=verify|unsubscribe&email_link_action_key=...
end
Controller->>Controller: sanitize 'email_link_action' -> $action
Controller->>Factory: Factory::get_notification($notification_id)
Factory-->>Controller: Notification|null
alt Notification exists
Controller->>Notif: validate_and_maybe_process_request($notification, $action, $key)
alt action == verify
Notif->>Notif: wp_verify_fast_hash(...) -> if valid: set status VERIFIED
Notif->>DB: persist status
else action == unsubscribe
Notif->>Notif: wp_verify_fast_hash(...) -> if valid: set status CANCELLED and set cancellation source
Notif->>DB: persist status and metadata
else unknown action
Controller->>Logger: debug log if WP_DEBUG, drop request otherwise
end
Notif-->>Controller: result (redirect)
else Notification not found
Controller->>Logger: debug/early return
end
Controller-->>Client: wp_safe_redirect / response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
plugins/woocommerce/src/Internal/StockNotifications/Config.php (1)
73-98: Filter rename is acceptable under alpha gating — worth a brief note for the alpha upgrade guide.Renaming
woocommerce_customer_stock_notifications_supported_product_stock_statuses→woocommerce_customer_stock_notifications_supported_product_statusesis a breaking change for any extender that may have hooked into the original (typo'd) name. Since BIS is gated behindWOOCOMMERCE_BIS_ALPHA_ENABLED, this is fine now, but consider calling it out in the alpha change notes so anyone testing the alpha knows to update hook usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/woocommerce/src/Internal/StockNotifications/Config.php` around lines 73 - 98, The filter name was changed from the typo'd hook to 'woocommerce_customer_stock_notifications_supported_product_statuses' in Config::get_supported_product_statuses(), which is breaking for extenders; update the alpha upgrade/changes documentation to call out this rename (original 'woocommerce_customer_stock_notifications_supported_product_stock_statuses' → new 'woocommerce_customer_stock_notifications_supported_product_statuses'), mention WOOCOMMERCE_BIS_ALPHA_ENABLED gating, and include a short migration note so testers know to update any add_filter/remove_filter usages against that hook.plugins/woocommerce/tests/php/src/Internal/StockNotifications/Emails/EmailActionControllerTests.php (1)
21-55: LGTM — tests correctly reflect the split meta keys,wp_fast_hashmigration, and explicit action parameter.The verification-key test uses the
time():hashformat (matching the expiration-awareverification_action_keycontract) while the unsubscribe test uses a bare hash, consistent withcheck_unsubscribe_key()not enforcing TTL.Optional: consider adding negative cases (invalid key, mismatched action e.g. passing
'unsubscribe'against averification_action_key, and missing notification id) to guard against regressions in the action-routing logic introduced byemail_link_action. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/woocommerce/tests/php/src/Internal/StockNotifications/Emails/EmailActionControllerTests.php` around lines 21 - 55, Add negative test cases to cover invalid/mismatched action routing: extend EmailActionControllerTests by adding tests such as test_process_verification_action_with_invalid_key_returns_no_change, test_process_unsubscribe_action_with_verification_key_does_not_unsubscribe, and test_process_action_with_missing_notification_id_handles_gracefully; each should create a Notification (use Factory or Notification methods), set the opposite or invalid meta key (use 'verification_action_key' vs 'unsubscribe_action_key' and wp_fast_hash/time:wp_fast_hash variants), call EmailActionController::validate_and_maybe_process_request with mismatched action or an invalid id, and assert that the notification status and cancellation_source remain unchanged (or that the controller returns/handles the error as expected).plugins/woocommerce/src/Internal/StockNotifications/Emails/EmailActionController.php (1)
55-74: Optional: extract action tokens into class constants and consider a default branch.Two small hardening suggestions for the dispatch surface:
- The literal strings
'verify'/'unsubscribe'are now the contract between URL producers (email templates) and this consumer. A drift in either side silently no-ops. Extracting them asEmailActionController::ACTION_VERIFY/ACTION_UNSUBSCRIBEwould make the coupling explicit and greppable.- The
switchhas nodefault— unknown/typo'd actions are dropped silently with no log or notice. For an alpha-gated feature the silent fallthrough is fine, but a singlewc_get_logger()->debug(...)in adefault:branch would make mis-routed emails visible during alpha testing.Also:
$action = ''default on the signature means any existing 2-arg caller (tests?) will now silently do nothing rather than dispatch. If this method is only intended to be driven bymaybe_process_email_action, consider making$actionrequired; otherwise leave as-is.♻️ Proposed refactor sketch
class EmailActionController { + private const ACTION_VERIFY = 'verify'; + private const ACTION_UNSUBSCRIBE = 'unsubscribe'; + public function __construct() { ... } ... - switch ( $action ) { - case 'verify': + switch ( $action ) { + case self::ACTION_VERIFY: $this->process_verification_action( $notification, $email_link_action_key ); break; - case 'unsubscribe': + case self::ACTION_UNSUBSCRIBE: $this->process_unsubscribe_action( $notification, $email_link_action_key ); break; + default: + // Unknown email_link_action — silently ignore in production, log in debug. + if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) { + wc_get_logger()->debug( + sprintf( 'Unknown email_link_action "%s" for notification %d', $action, $notification->get_id() ), + array( 'source' => 'stock-notifications' ) + ); + } + break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/woocommerce/src/Internal/StockNotifications/Emails/EmailActionController.php` around lines 55 - 74, The switch in validate_and_maybe_process_request silently relies on string literals and lacks a default/logging path; extract the action tokens into class constants (e.g., ACTION_VERIFY and ACTION_UNSUBSCRIBE) and use them in the switch to make the contract explicit, add a default: branch that logs unknown actions using wc_get_logger()->debug(...) so typos or mismatches are visible, and decide whether $action should be required (if only called by maybe_process_email_action) or keep the '' default but ensure callers are updated/tested; update references to process_verification_action and process_unsubscribe_action to use the new constants.plugins/woocommerce/src/Internal/StockNotifications/Notification.php (1)
500-502: Optional: consider whether explicit guards are needed forcheck_unsubscribe_key.The
check_unsubscribe_keymethod callswp_verify_fast_hash()with the result ofget_meta( 'unsubscribe_action_key' ), which returns''when the key has never been generated or was cleared. Whilewp_verify_fast_hash()handles empty strings cleanly without emitting notices, adding an explicit guard would align with defensive programming principles. However, note that the "symmetry" withcheck_verification_key()is not exact—check_verification_key()guards via a functional check (str_contains()for a colon) rather than an explicit empty/type guard. If adding a guard here, ensure it serves the actual defensive need rather than assumed consistency with a different pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/woocommerce/src/Internal/StockNotifications/Notification.php` around lines 500 - 502, The method check_unsubscribe_key calls wp_verify_fast_hash with the value returned by get_meta('unsubscribe_action_key') which can be an empty string; add an explicit defensive guard at the start of check_unsubscribe_key to return false if get_meta('unsubscribe_action_key') is empty or not a string before calling wp_verify_fast_hash, referencing the same meta key and keeping behavior consistent with existing check_verification_key semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/woocommerce/src/Internal/StockNotifications/Config.php`:
- Around line 73-98: The filter name was changed from the typo'd hook to
'woocommerce_customer_stock_notifications_supported_product_statuses' in
Config::get_supported_product_statuses(), which is breaking for extenders;
update the alpha upgrade/changes documentation to call out this rename (original
'woocommerce_customer_stock_notifications_supported_product_stock_statuses' →
new 'woocommerce_customer_stock_notifications_supported_product_statuses'),
mention WOOCOMMERCE_BIS_ALPHA_ENABLED gating, and include a short migration note
so testers know to update any add_filter/remove_filter usages against that hook.
In
`@plugins/woocommerce/src/Internal/StockNotifications/Emails/EmailActionController.php`:
- Around line 55-74: The switch in validate_and_maybe_process_request silently
relies on string literals and lacks a default/logging path; extract the action
tokens into class constants (e.g., ACTION_VERIFY and ACTION_UNSUBSCRIBE) and use
them in the switch to make the contract explicit, add a default: branch that
logs unknown actions using wc_get_logger()->debug(...) so typos or mismatches
are visible, and decide whether $action should be required (if only called by
maybe_process_email_action) or keep the '' default but ensure callers are
updated/tested; update references to process_verification_action and
process_unsubscribe_action to use the new constants.
In `@plugins/woocommerce/src/Internal/StockNotifications/Notification.php`:
- Around line 500-502: The method check_unsubscribe_key calls
wp_verify_fast_hash with the value returned by
get_meta('unsubscribe_action_key') which can be an empty string; add an explicit
defensive guard at the start of check_unsubscribe_key to return false if
get_meta('unsubscribe_action_key') is empty or not a string before calling
wp_verify_fast_hash, referencing the same meta key and keeping behavior
consistent with existing check_verification_key semantics.
In
`@plugins/woocommerce/tests/php/src/Internal/StockNotifications/Emails/EmailActionControllerTests.php`:
- Around line 21-55: Add negative test cases to cover invalid/mismatched action
routing: extend EmailActionControllerTests by adding tests such as
test_process_verification_action_with_invalid_key_returns_no_change,
test_process_unsubscribe_action_with_verification_key_does_not_unsubscribe, and
test_process_action_with_missing_notification_id_handles_gracefully; each should
create a Notification (use Factory or Notification methods), set the opposite or
invalid meta key (use 'verification_action_key' vs 'unsubscribe_action_key' and
wp_fast_hash/time:wp_fast_hash variants), call
EmailActionController::validate_and_maybe_process_request with mismatched action
or an invalid id, and assert that the notification status and
cancellation_source remain unchanged (or that the controller returns/handles the
error as expected).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9fe39bd3-cc92-4774-bcee-f9efc39d9da1
📒 Files selected for processing (23)
.markdownlintignoreplugins/woocommerce/changelog/bis-coderabbit-review-fixesplugins/woocommerce/changelog/bis-remove-hasherhelper-shimplugins/woocommerce/phpstan-baseline.neonplugins/woocommerce/src/Internal/StockNotifications/Admin/NotificationEditPage.phpplugins/woocommerce/src/Internal/StockNotifications/Admin/Templates/html-admin-notification-create.phpplugins/woocommerce/src/Internal/StockNotifications/AsyncTasks/NotificationsProcessor.phpplugins/woocommerce/src/Internal/StockNotifications/CODERABBIT-TRIAGE-RAW.mdplugins/woocommerce/src/Internal/StockNotifications/CODERABBIT-TRIAGE.mdplugins/woocommerce/src/Internal/StockNotifications/Config.phpplugins/woocommerce/src/Internal/StockNotifications/DataRetentionController.phpplugins/woocommerce/src/Internal/StockNotifications/Emails/CustomerStockNotificationEmail.phpplugins/woocommerce/src/Internal/StockNotifications/Emails/CustomerStockNotificationVerifiedEmail.phpplugins/woocommerce/src/Internal/StockNotifications/Emails/CustomerStockNotificationVerifyEmail.phpplugins/woocommerce/src/Internal/StockNotifications/Emails/EmailActionController.phpplugins/woocommerce/src/Internal/StockNotifications/Notification.phpplugins/woocommerce/src/Internal/StockNotifications/Privacy/PrivacyEraser.phpplugins/woocommerce/src/Internal/StockNotifications/SPRINT.mdplugins/woocommerce/src/Internal/StockNotifications/Utilities/HasherHelper.phpplugins/woocommerce/templates/emails/customer-stock-notification-verified.phpplugins/woocommerce/tests/legacy/bootstrap.phpplugins/woocommerce/tests/php/src/Internal/StockNotifications/Emails/EmailActionControllerTests.phpplugins/woocommerce/tests/php/src/Internal/StockNotifications/NotificationTests.php
💤 Files with no reviewable changes (2)
- plugins/woocommerce/src/Internal/StockNotifications/Utilities/HasherHelper.php
- plugins/woocommerce/phpstan-baseline.neon
Pulls the 'verify' / 'unsubscribe' tokens shared between the three BIS email templates and EmailActionController into class constants (ACTION_VERIFY / ACTION_UNSUBSCRIBE), so a drift on either side of the URL contract is now a compile-time typo rather than a silent no-op. All three email templates that produce these links are updated to reference the same constants. Adds a default branch to the action switch that logs unknown tokens via wc_get_logger()->debug when WP_DEBUG is enabled, so mis-routed email links become visible during alpha testing instead of failing silently. Expands EmailActionControllerTests with negative cases for: invalid verification key, mismatched action key family (verification key present, unsubscribe action requested), missing notification id, and unknown action token. Addresses CodeRabbit nitpicks on PR #64329. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
plugins/woocommerce/tests/php/src/Internal/StockNotifications/Emails/EmailActionControllerTests.php (1)
103-111: Optional: preferexpectNotToPerformAssertions()overassertTrue( true ).The dummy assertion works, but PHPUnit's risky-test checks flag this pattern and the intent ("this call must not throw or mutate") is clearer with
$this->expectNotToPerformAssertions();declared before the act step.♻️ Suggested tweak
public function test_process_action_with_missing_notification_id_handles_gracefully() { $controller = new EmailActionController(); // Should not throw or emit a notice — the guard in // validate_and_maybe_process_request catches the empty id. + $this->expectNotToPerformAssertions(); $controller->validate_and_maybe_process_request( 0, 'any-key', 'verify' ); - - $this->assertTrue( true ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/woocommerce/tests/php/src/Internal/StockNotifications/Emails/EmailActionControllerTests.php` around lines 103 - 111, Replace the dummy assertion in the test_process_action_with_missing_notification_id_handles_gracefully test with PHPUnit's intent API: call $this->expectNotToPerformAssertions() before invoking EmailActionController::validate_and_maybe_process_request(0, 'any-key', 'verify') so the test expresses "no assertions expected" instead of using $this->assertTrue(true); keep the same EmailActionController instantiation and method call.plugins/woocommerce/src/Internal/StockNotifications/Emails/EmailActionController.php (1)
71-104: Optional: tighten the empty-vs-unknown action distinction.
validate_and_maybe_process_request()still accepts a default$action = '', which will silently fall through to thedefaultbranch and emitUnknown email_link_action "" for notification N. The only in-repo caller (maybe_process_email_action) already passes a sanitized non-empty value, so either making the parameter required or branching empty-vs-unknown explicitly would make the log signal more actionable during alpha triage.♻️ Suggested tweak
- public function validate_and_maybe_process_request( int $notification_id, string $email_link_action_key, string $action = '' ): void { - if ( empty( $email_link_action_key ) || empty( $notification_id ) ) { + public function validate_and_maybe_process_request( int $notification_id, string $email_link_action_key, string $action ): void { + if ( empty( $email_link_action_key ) || empty( $notification_id ) || empty( $action ) ) { return; }Feel free to defer — behaviourally this is a no-op in practice because the only public entry point feeds the
isset()-guarded GET param through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/woocommerce/src/Internal/StockNotifications/Emails/EmailActionController.php` around lines 71 - 104, The method validate_and_maybe_process_request currently has a default empty string for $action which results in the default switch branch logging an "Unknown email_link_action" for an empty value; fix by making the distinction explicit — either remove the default (make $action a required parameter) or add an early guard like if ( '' === $action ) { return; } (or log a clearer "empty email_link_action" message) before the switch, and if you make the signature non-default, update the caller maybe_process_email_action accordingly to pass the sanitized action.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@plugins/woocommerce/src/Internal/StockNotifications/Emails/EmailActionController.php`:
- Around line 71-104: The method validate_and_maybe_process_request currently
has a default empty string for $action which results in the default switch
branch logging an "Unknown email_link_action" for an empty value; fix by making
the distinction explicit — either remove the default (make $action a required
parameter) or add an early guard like if ( '' === $action ) { return; } (or log
a clearer "empty email_link_action" message) before the switch, and if you make
the signature non-default, update the caller maybe_process_email_action
accordingly to pass the sanitized action.
In
`@plugins/woocommerce/tests/php/src/Internal/StockNotifications/Emails/EmailActionControllerTests.php`:
- Around line 103-111: Replace the dummy assertion in the
test_process_action_with_missing_notification_id_handles_gracefully test with
PHPUnit's intent API: call $this->expectNotToPerformAssertions() before invoking
EmailActionController::validate_and_maybe_process_request(0, 'any-key',
'verify') so the test expresses "no assertions expected" instead of using
$this->assertTrue(true); keep the same EmailActionController instantiation and
method call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f540021a-025f-4c94-879f-881211d3fa18
📒 Files selected for processing (6)
plugins/woocommerce/changelog/64398-bis-email-action-constantsplugins/woocommerce/src/Internal/StockNotifications/Emails/CustomerStockNotificationEmail.phpplugins/woocommerce/src/Internal/StockNotifications/Emails/CustomerStockNotificationVerifiedEmail.phpplugins/woocommerce/src/Internal/StockNotifications/Emails/CustomerStockNotificationVerifyEmail.phpplugins/woocommerce/src/Internal/StockNotifications/Emails/EmailActionController.phpplugins/woocommerce/tests/php/src/Internal/StockNotifications/Emails/EmailActionControllerTests.php
✅ Files skipped from review due to trivial changes (2)
- plugins/woocommerce/changelog/64398-bis-email-action-constants
- plugins/woocommerce/src/Internal/StockNotifications/Emails/CustomerStockNotificationVerifyEmail.php
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/woocommerce/src/Internal/StockNotifications/Emails/CustomerStockNotificationEmail.php
- Add missing exit after wp_safe_redirect() in both email-action handlers so execution stops once the 302 is queued. - Guard check_unsubscribe_key against an empty stored hash, mirroring check_verification_key semantics. wp_verify_fast_hash tolerates an empty string today but the explicit early-return makes intent greppable and removes the implicit dependency. - Coerce the verification-key meta to string before str_contains so a stored null/int can never trip a TypeError. - Reword tests/legacy/bootstrap.php BIS comment to reflect that the constant is only set as a default — per-suite overrides still win. - Parenthesize the expired-verification-key timestamp arithmetic in NotificationTests for readability. - Separate empty vs unknown action in EmailActionController: empty means the caller omitted the argument (silent return); unknown keeps the debug-log branch so genuinely mis-routed tokens surface. - Replace assertTrue(true) in the no-assertion happy-path test with expectNotToPerformAssertions() to avoid PHPUnit's risky-test warning. Addresses CodeRabbit nits across PRs #64329, #64367, #64368, #64369, #64370, #64371 that share files on the feature branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/woocommerce/tests/php/src/Internal/StockNotifications/Emails/EmailActionControllerTests.php (1)
56-132: Solid negative-path coverage expansion.The new tests cover the three important new branches: invalid-key rejection, action/key mismatch (unsubscribe routed at a notification with only a verification key), zero/missing ID, and unknown action token.
expectNotToPerformAssertions()is the right call for the intentionally side-effect-free path at lines 103-112.Optional: you might add one more tiny test for the explicit-empty-
$actionearly return invalidate_and_maybe_process_request(e.g.->validate_and_maybe_process_request( $id, 'test', '' )) to lock down that branch separately from the "unknown token" branch, since they take different code paths in the controller (early return vs.defaultcase with debug log). Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/woocommerce/tests/php/src/Internal/StockNotifications/Emails/EmailActionControllerTests.php` around lines 56 - 132, Add a tiny test that covers the explicit-empty-$action early-return branch: create a product and Notification (e.g. status PENDING or ACTIVE as appropriate), save it, call EmailActionController::validate_and_maybe_process_request($id, 'test', '') and assert the notification was not mutated (e.g. compare status before/after). Name the test method something like test_process_action_with_empty_action_early_returns to make the intent clear and ensure it verifies the early-return path is distinct from the unknown-token branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@plugins/woocommerce/tests/php/src/Internal/StockNotifications/Emails/EmailActionControllerTests.php`:
- Around line 56-132: Add a tiny test that covers the explicit-empty-$action
early-return branch: create a product and Notification (e.g. status PENDING or
ACTIVE as appropriate), save it, call
EmailActionController::validate_and_maybe_process_request($id, 'test', '') and
assert the notification was not mutated (e.g. compare status before/after). Name
the test method something like
test_process_action_with_empty_action_early_returns to make the intent clear and
ensure it verifies the early-return path is distinct from the unknown-token
branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 34964822-91c4-442f-832d-ac46b268061f
📒 Files selected for processing (5)
plugins/woocommerce/src/Internal/StockNotifications/Emails/EmailActionController.phpplugins/woocommerce/src/Internal/StockNotifications/Notification.phpplugins/woocommerce/tests/legacy/bootstrap.phpplugins/woocommerce/tests/php/src/Internal/StockNotifications/Emails/EmailActionControllerTests.phpplugins/woocommerce/tests/php/src/Internal/StockNotifications/NotificationTests.php
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/woocommerce/src/Internal/StockNotifications/Notification.php
- plugins/woocommerce/tests/php/src/Internal/StockNotifications/NotificationTests.php
Line-number citations in markdown rot every time the target file is edited and give a false-precision signal — a reader searching for the cited line after a few weeks of churn lands on unrelated code. Replace the remaining ones in SPRINT.md with symbol references (method names and file stems). Also tighten the CODERABBIT-TRIAGE uncertainty-notes wording and add a banner at the top of the triage doc making it explicit that the file is a pre-GA scratchpad listed in SPRINT.md for deletion, not reference documentation — so anyone who lands on it via a search doesn't treat the triage notes as the permanent shape of the code. Addresses CodeRabbit nits on PRs #64369 (CODERABBIT-TRIAGE wording) and #64370 (SPRINT.md anchor format). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/woocommerce/src/Internal/StockNotifications/CODERABBIT-TRIAGE.md`:
- Around line 91-96: Update the summary count in the triage header to match the
breakdown: either change "Of the 11 items flagged" to "Of the 12 items flagged"
or adjust the bucket numbers so 9 + 2 + 1 totals 11; locate the header sentence
that begins "Of the 11 items flagged in the handoff as 'already identified
must-fixes'" and make the counts consistent with the listed bullet breakdown
(NotificationEditPage, Notification, Config, NotificationsProcessor,
DataRetentionController, html-admin-notification-create,
customer-stock-notification-verified, HasherHelper,
CustomerStockNotificationVerifyEmail, and CustomerStockNotificationEmail).
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 625b6d42-4fee-433b-a14d-0dab305395f6
📒 Files selected for processing (2)
plugins/woocommerce/src/Internal/StockNotifications/CODERABBIT-TRIAGE.mdplugins/woocommerce/src/Internal/StockNotifications/SPRINT.md
✅ Files skipped from review due to trivial changes (1)
- plugins/woocommerce/src/Internal/StockNotifications/SPRINT.md
The unknown-token test locks down the switch's default: path (debug-log branch). This adds a separate test for the empty-action early return that sits before the switch, so the two distinct code paths for 'caller omitted the argument' vs 'caller passed a bogus token' are each asserted in isolation. Addresses CodeRabbit nit on PR #64329. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/woocommerce/tests/php/src/Internal/StockNotifications/Emails/EmailActionControllerTests.php (1)
21-153: Optional: extract a helper to DRY the notification-arrange blocks.Five of the seven tests repeat the same 6-line arrange block (create product →
new Notification→ set product id / status / email →update_meta_data→save). A tiny private helper would make each test read as "given a PENDING notification with this key meta, when I call the controller with X, then status is Y."♻️ Proposed helper extraction
+ /** + * Arrange a persisted notification with a single action-key meta entry. + * + * `@param` string $status Initial NotificationStatus. + * `@param` string $meta_key 'verification_action_key' or 'unsubscribe_action_key'. + * `@param` string $stored_key Value to store in the meta (already hashed / time-prefixed by caller). + * `@return` int Saved notification id. + */ + private function arrange_notification( string $status, string $meta_key, string $stored_key ): int { + $product = WC_Helper_Product::create_simple_product(); + $notification = new Notification(); + $notification->set_product_id( $product->get_id() ); + $notification->set_status( $status ); + $notification->set_user_email( 'test@example.com' ); + $notification->update_meta_data( $meta_key, $stored_key ); + return $notification->save(); + }Then each test collapses to, e.g.:
$id = $this->arrange_notification( NotificationStatus::PENDING, 'verification_action_key', time() . ':' . wp_fast_hash( 'test' ) ); ( new EmailActionController() )->validate_and_maybe_process_request( $id, 'wrong-key', 'verify' ); $this->assertEquals( NotificationStatus::PENDING, Factory::get_notification( $id )->get_status() );Non-blocking — happy to defer if you prefer to keep each test fully self-contained.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/woocommerce/tests/php/src/Internal/StockNotifications/Emails/EmailActionControllerTests.php` around lines 21 - 153, Many tests duplicate the same notification setup; add a private helper (e.g., arrange_notification) to DRY creation and saving of a Notification so tests become shorter; implement a private method on the test class that accepts arguments (status, meta_key, meta_value) and performs: create product via WC_Helper_Product::create_simple_product(), instantiate Notification, call set_product_id, set_status, set_user_email, update_meta_data, save and return the saved id; then replace repeated arrange blocks with calls to this helper and keep assertions and EmailActionController::validate_and_maybe_process_request calls intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@plugins/woocommerce/tests/php/src/Internal/StockNotifications/Emails/EmailActionControllerTests.php`:
- Around line 21-153: Many tests duplicate the same notification setup; add a
private helper (e.g., arrange_notification) to DRY creation and saving of a
Notification so tests become shorter; implement a private method on the test
class that accepts arguments (status, meta_key, meta_value) and performs: create
product via WC_Helper_Product::create_simple_product(), instantiate
Notification, call set_product_id, set_status, set_user_email, update_meta_data,
save and return the saved id; then replace repeated arrange blocks with calls to
this helper and keep assertions and
EmailActionController::validate_and_maybe_process_request calls intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: bb1f91d0-262d-4df6-a9b6-74a4cc266d3d
📒 Files selected for processing (1)
plugins/woocommerce/tests/php/src/Internal/StockNotifications/Emails/EmailActionControllerTests.php
- Correct triage reconciliation count from 11 to 12 (9 + 2 + 1 = 12). - Extract a private arrange_notification() helper in EmailActionControllerTests so each test reads as 'given a notification with this status and meta, when the controller is called with X, expect status Y' instead of duplicating the 6-line setup block seven times. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace 64398-bis-email-action-constants, bis-coderabbit-review-fixes, and bis-remove-hasherhelper-shim with one bis-alpha-cr-fixes-and-cleanup entry summarising the scope of #64329. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
opr
requested review from
xristos3490
and removed request for
jorgeatorres and
prettyboymp
Removes SPRINT.md, CODERABBIT-TRIAGE.md, and CODERABBIT-TRIAGE-RAW.md from src/Internal/StockNotifications/ — these were sprint-internal scratch files always intended for pre-merge cleanup. Updates the bis-alpha-cr-fixes-and-cleanup changelog entry to reflect the deletion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
xristos3490
approved these changes
Apr 29, 2026
xristos3490
left a comment
xristos3490
left a comment
Member
There was a problem hiding this comment.
Reviewed the code over huddle. This tests well on my end, and looks good, @opr! Nice touch on removing the HasherHelper!
This was referenced Apr 29, 2026
4 tasks
kalessil
added a commit
that referenced
this pull request
May 21, 2026
* Trap wp_safe_redirect in EmailActionControllerTests to fix silent PHPUnit halt PR #64329 added `exit;` after `wp_safe_redirect()` in EmailActionController::process_verification_action() and process_unsubscribe_action(). The new EmailActionControllerTests calls validate_and_maybe_process_request() directly with valid keys, which now reaches `wp_safe_redirect + exit;` and silently terminates PHPUnit mid-run with status 0 — CI stays green while the rest of the suite never executes. Install the same `wp_redirect` filter trap already used by the sister NotificationManagementServiceTests so the controller's `exit;` is never reached during tests. The four happy-path tests (verify success, verified email dispatch, repeated-verify short-circuit, unsubscribe) wrap their SUT calls in try/catch and assert the intercept message. Production code is unchanged — wp_safe_redirect + exit is the correct WP pattern. Bug introduced in PR #64329. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add changefile(s) from automation for the following project(s): woocommerce * Monorepo: cleanup. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: woocommercebot <woocommercebot@users.noreply.github.com> Co-authored-by: Vladimir Reznichenko <kalessil@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Submission Review Guidelines:
Changes proposed in this Pull Request:
Closes RSM-436.
Resolves the must-fix CodeRabbit review comments on the Back in Stock Notifications alpha (
WOOCOMMERCE_BIS_ALPHA_ENABLED-gated) that were left unaddressed when the alpha merged. Also drops the pre-WP-6.8HasherHelpercompatibility shim now that WooCommerce requires WP 6.8, and deletes the in-tree sprint scratch docs ahead of merge.Notable items:
date_notifiedcolumn instead ofcancellation_source.email_link_action_keymeta into distinctverification_action_key/unsubscribe_action_keykeys so generating one no longer silently invalidates the other. Verification and unsubscribe email URLs now carry an explicitemail_link_actionparameter for dispatch.verify/unsubscribe) into named constants onEmailActionControllerand adds a debug log path for unknown actions.Factory::get_notification()returningfalsein the retention cron and privacy eraser, preventing fatals when any notification id is unreadable.(int) $product_id ?? 0), repairs malformed<option>tags + a missing attribute space in the admin create form, and drops an orphanedsprintfargument (no%splaceholder) in the verified email template.woocommerce_customer_stock_notifications_supported_product_stock_statuses→..._supported_product_statuseswhile the feature is still alpha-gated.HasherHelpershim now that WooCommerce requires WP 6.8.SPRINT.md,CODERABBIT-TRIAGE.md,CODERABBIT-TRIAGE-RAW.md) fromsrc/Internal/StockNotifications/ahead of merge.Bug introduced in PR #59947.
How to test the changes in this Pull Request:
Enable the alpha:
define( 'WOOCOMMERCE_BIS_ALPHA_ENABLED', true );inwp-config.php, then enable WooCommerce → Settings → Products → Customer stock notifications → Allow signups.Active.Sent.Cancelled,cancellation_source=USER, success notice on the redirect.Cancelled,cancellation_source=ADMIN,date_cancelledpopulated,date_notifiedremainsNULL(this was the bug).Select product…and a clear (×) button — both were suppressed before the attribute-space fix.Testing that has already taken place:
Manual end-to-end on a local Studio site (SQLite): signup → return-to-stock → back-in-stock email dispatch → unsubscribe all verified.
pnpm --filter=@woocommerce/plugin-woocommerce test:php:env -- --filter 'StockNotifications|Notification'runs 409 tests / 998 assertions green, including the updatedNotificationTestsandEmailActionControllerTests.phpcsand PHPStan (level 8, PHP 8.2) both clean on the changed paths; the PHPStan baseline shrinks by 8 entries forPrivacyEraser.phpnow that the null guard is in place.Milestone
Changelog entry
Changelog Entry Comment
Comment
Changelog entry was created manually and is committed in this PR:
plugins/woocommerce/changelog/bis-alpha-cr-fixes-and-cleanup(Patch/Dev). Consolidates the previous separate entries (bis-coderabbit-review-fixes,bis-remove-hasherhelper-shim,64398-bis-email-action-constants) into a single one.