Add PHPUnit tests for dismiss-issue REST endpoint (single & large-batch modes) - #1715
Merged
pattonwebz merged 7 commits intoMay 28, 2026
Conversation
Tests cover both single-issue and large-batch dismiss modes: - Single issue dismissed by authorized user (success case) - Single issue dismissed by unauthorized user (403 failure) - Large batch dismissed by user with edit permission on all posts (success with bulk update) - Large batch dismissed by user with partial authorization (403 failure before bulk query) - Large batch dismissed by user with no authorization (403 failure) The tests verify: - Proper permission checking via edit_post capability - Correct response status codes and data - Database state changes (single vs batch updates) - Fail-safe behavior (no partial updates on permission failure)
Contributor
📝 WalkthroughWalkthroughAdds per-row permission checks to the dismiss_issue largeBatch path and PHPUnit tests that cover single-issue and large-batch dismiss scenarios for fully authorized, partially authorized, and unauthorized users. ChangesDismiss-Issue Endpoint Implementation & Tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 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)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a suite of PHPUnit tests for the dismiss-issue REST API endpoint, covering single and batch dismissal scenarios across different authorization levels. Feedback highlights a strict type check failure in an assertion where a boolean response is compared to an integer. Additionally, a potential security vulnerability was identified where the API's permission check for batch dismissals may be insufficient, potentially allowing unauthorized users to dismiss issues. Finally, several unused static properties and a dead setup method were found and should be removed to clean up the test suite.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds PHPUnit coverage for the POST /accessibility-checker/v1/dismiss-issue/{issue_id} REST endpoint to validate permission gating and database updates for both single-issue dismiss and largeBatch dismiss behavior.
Changes:
- Added new REST endpoint tests for single-issue dismiss success (authorized) and failure (unauthorized).
- Added new REST endpoint tests for
largeBatchdismiss success (all posts editable) and failure (partial/none editable), including DB state verification.
Comments suppressed due to low confidence (2)
tests/phpunit/includes/classes/RestApiEndpointsTest.php:684
- Same capability issue as the single-issue test: these posts are created as
publishfor the limited user, but the limited user only hasedit_posts(notedit_published_posts), soedit_postchecks may fail and make this test flaky/incorrect. Consider making these posts drafts or extending the limited user's caps for the duration of the test.
$post_1 = self::factory()->post->create(
[
'post_type' => 'post',
'post_status' => 'publish',
'post_author' => self::$limited_id,
'post_title' => 'Batch Post 1',
'post_content' => 'Batch Content 1',
]
);
$post_2 = self::factory()->post->create(
[
'post_type' => 'post',
'post_status' => 'publish',
'post_author' => self::$limited_id,
'post_title' => 'Batch Post 2',
'post_content' => 'Batch Content 2',
]
);
tests/phpunit/includes/classes/RestApiEndpointsTest.php:770
- This test intends to model a "partial authorization" batch, but the limited user's post is created with
post_status=publish. Since the limited user only hasedit_posts, they may not haveedit_poston their own published post, turning this into a "no authorization" scenario and not validating the intended behavior. Use a draft status for the limited-owned post (or grantedit_published_posts) so exactly one row is editable.
$limited_post = self::factory()->post->create(
[
'post_type' => 'post',
'post_status' => 'publish',
'post_author' => self::$limited_id,
'post_title' => 'Limited Batch Post',
'post_content' => 'Limited Batch Content',
]
);
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/phpunit/includes/classes/RestApiEndpointsTest.php (1)
87-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
edit_published_poststo the limited-user fixture fordismiss-issueauthorization
tests/phpunit/includes/classes/RestApiEndpointsTest.phpcreates the limited user with onlyedit_posts(line 91). The REST route’s permission check usescurrent_user_can( 'edit_post', $post_id ), and forpost_status => publishthat meta-cap maps to requiringedit_published_postsas well—so the “authorized” dismiss cases can hit 403.Suggested fix
$user = new WP_User( self::$limited_id ); $user->add_cap( 'edit_posts' ); + $user->add_cap( 'edit_published_posts' );🤖 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 `@tests/phpunit/includes/classes/RestApiEndpointsTest.php` around lines 87 - 92, The limited test user fixture (self::$limited_id) only has edit_posts added via new WP_User(...)->add_cap('edit_posts'), but dismiss-issue authorization for published posts uses current_user_can('edit_post', $post_id) which requires the meta-cap edit_published_posts; update the fixture in RestApiEndpointsTest (around where self::$limited_id is created) to also add_cap('edit_published_posts') to the WP_User instance so the limited user can pass edit_post checks on published posts (i.e., call add_cap for edit_published_posts on the WP_User created for self::$limited_id).
🤖 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.
Outside diff comments:
In `@tests/phpunit/includes/classes/RestApiEndpointsTest.php`:
- Around line 87-92: The limited test user fixture (self::$limited_id) only has
edit_posts added via new WP_User(...)->add_cap('edit_posts'), but dismiss-issue
authorization for published posts uses current_user_can('edit_post', $post_id)
which requires the meta-cap edit_published_posts; update the fixture in
RestApiEndpointsTest (around where self::$limited_id is created) to also
add_cap('edit_published_posts') to the WP_User instance so the limited user can
pass edit_post checks on published posts (i.e., call add_cap for
edit_published_posts on the WP_User created for self::$limited_id).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0971c6a2-f5b3-430a-a426-0f332bc20643
📒 Files selected for processing (1)
tests/phpunit/includes/classes/RestApiEndpointsTest.php
Before executing the bulk object-based UPDATE, load all issue rows matching the object and verify the current user has edit_post capability for each row's post ID. If any issue in the batch belongs to a post the user cannot edit, return rest_forbidden immediately without modifying any data. Only when all permission checks pass does the single bulk UPDATE run. This avoids thousands of individual UPDATE queries while ensuring no partial dismissals can occur when a user lacks access to part of a batch.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
includes/classes/class-rest-api.php (1)
1245-1291:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUpdate only the vetted IDs in the bulk write.
Line 1277 switches back to
WHERE ... object = %s, so there is still a TOCTOU auth gap: a row inserted for the same object after the permission scan but before theUPDATEwill be dismissed without ever being checked. The bulk write should target the exactids from$issue_rows(or run under a lock/transaction) so the authorized set and updated set stay identical.As per coding guidelines, "Follow WordPress security best practices (sanitization, validation, nonces) in all PHP code" and "Implement proper capability checks for all admin operations and AJAX requests in PHP".Suggested direction
+ $issue_ids = array_map( 'intval', wp_list_pluck( $issue_rows, 'id' ) ); + $id_placeholders = implode( ', ', array_fill( 0, count( $issue_ids ), '%d' ) ); // Update all issues with the same object in one query after auth checks pass. // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching -- Direct update required, no caching needed. $result = $wpdb->query( $wpdb->prepare( - 'UPDATE %i SET ignre = %d, ignre_user = %d, ignre_date = %s, ignre_reason = %s, ignre_comment = %s, ignre_global = %d WHERE siteid = %d AND object = %s', - $table_name, - $ignre, - $ignre_user, - $ignre_date, - $ignre_reason, - $ignre_comment, - $ignre_global, - $site_id, - $object + "UPDATE %i SET ignre = %d, ignre_user = %d, ignre_date = %s, ignre_reason = %s, ignre_comment = %s, ignre_global = %d WHERE siteid = %d AND id IN ( {$id_placeholders} )", + array_merge( + [ + $table_name, + $ignre, + $ignre_user, + $ignre_date, + $ignre_reason, + $ignre_comment, + $ignre_global, + $site_id, + ], + $issue_ids + ) ) );🤖 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/classes/class-rest-api.php` around lines 1245 - 1291, The bulk UPDATE currently uses WHERE ... object = %s and can update rows inserted after the permission scan; instead collect the vetted IDs from $issue_rows into a sanitized $issue_ids array (e.g., map to (int) and ensure non-empty) and change the UPDATE in the $wpdb->query call to target WHERE id IN (%d,...) AND siteid = %d (or include object if still needed) so only the originally vetted rows (from $issue_rows) are updated; alternatively wrap the read+update in a transaction/lock, but the preferred fix is to use the explicit id list ($issue_ids) in the UPDATE query to close the TOCTOU gap (referencing $issue_rows, $issue_ids, $wpdb->query, $table_name, $site_id, $object).
🤖 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.
Outside diff comments:
In `@includes/classes/class-rest-api.php`:
- Around line 1245-1291: The bulk UPDATE currently uses WHERE ... object = %s
and can update rows inserted after the permission scan; instead collect the
vetted IDs from $issue_rows into a sanitized $issue_ids array (e.g., map to
(int) and ensure non-empty) and change the UPDATE in the $wpdb->query call to
target WHERE id IN (%d,...) AND siteid = %d (or include object if still needed)
so only the originally vetted rows (from $issue_rows) are updated; alternatively
wrap the read+update in a transaction/lock, but the preferred fix is to use the
explicit id list ($issue_ids) in the UPDATE query to close the TOCTOU gap
(referencing $issue_rows, $issue_ids, $wpdb->query, $table_name, $site_id,
$object).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03c3825d-6275-4eb9-ba99-c5858cc9907f
📒 Files selected for processing (1)
includes/classes/class-rest-api.php
- Use draft posts in single and batch authorized tests so the limited user (edit_posts only, no edit_published_posts) can pass the edit_post capability check on their own posts - Fix assertSame(1, ...) -> assertTrue() for the ignre field since the endpoint returns bool $is_ignoring, not int 1 All 12 tests now pass (95 assertions).
…arge-batch-dismiss-rest-tests-and-updates
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/phpunit/includes/classes/RestApiEndpointsTest.php (1)
345-508: ⚡ Quick winDead code:
wpSetUpBeforeClass_DismissIssuesis never called.The method
wpSetUpBeforeClass_DismissIssuesand the static properties$dismiss_test_posts/$dismiss_test_issuesare never used. WordPress's test framework only auto-invokeswpSetUpBeforeClass—custom suffixes like_DismissIssuesrequire an explicit call fromwpSetUpBeforeClass.Since all five test methods create their own fixtures inline, this code can be removed entirely, or you can call it from
wpSetUpBeforeClassand refactor the tests to use the shared fixtures.Option A: Remove the dead code
- /** - * Dismiss issue test data: post ID to object mapping. - * - * `@var` array - */ - protected static $dismiss_test_posts = []; - - /** - * Dismiss issue test data: issue IDs created for batch testing. - * - * `@var` array - */ - protected static $dismiss_test_issues = []; - - /** - * Set up dismiss-issue test fixtures. - * - * `@param` WP_UnitTest_Factory $factory Factory instance. - * `@return` void - */ - public static function wpSetUpBeforeClass_DismissIssues( $factory ) { - // ... entire method body ... - }Option B: Wire up the fixtures
public static function wpSetUpBeforeClass( $factory ) { // Ensure posts are scannable by plugin. update_option( 'edac_post_types', [ 'post' ] ); // Ensure plugin DB table exists for tests (normally created via admin_init). ( new \EDAC\Admin\Update_Database() )->edac_update_database(); self::$admin_id = $factory->user->create( [ 'role' => 'administrator' ] ); self::$limited_id = $factory->user->create( [ 'role' => 'subscriber' ] ); self::$subscriber_id = $factory->user->create( [ 'role' => 'subscriber' ] ); // Give limited user edit_posts but not edit_others_posts so they cannot edit this post. $user = new WP_User( self::$limited_id ); $user->add_cap( 'edit_posts' ); self::$post_id = $factory->post->create( [ 'post_type' => 'post', 'post_status' => 'publish', 'post_author' => self::$admin_id, 'post_title' => 'EDAC PHPUnit Post', 'post_content' => '<main><h1>Title</h1><p>Img without alt <img src="/wp-includes/images/media/default.png"></p></main>', ] ); + + // Initialize dismiss-issue fixtures. + self::wpSetUpBeforeClass_DismissIssues( $factory ); }🤖 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 `@tests/phpunit/includes/classes/RestApiEndpointsTest.php` around lines 345 - 508, The wpSetUpBeforeClass_DismissIssues method and the static properties $dismiss_test_posts and $dismiss_test_issues are dead code because WordPress only auto-calls wpSetUpBeforeClass; either remove wpSetUpBeforeClass_DismissIssues and the two properties entirely (Option A) or invoke it from the class's wpSetUpBeforeClass and refactor tests to use the shared fixtures (Option B); locate the method wpSetUpBeforeClass_DismissIssues and the properties $dismiss_test_posts / $dismiss_test_issues and implement one of these two fixes consistently across the test class.
🤖 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.
Nitpick comments:
In `@tests/phpunit/includes/classes/RestApiEndpointsTest.php`:
- Around line 345-508: The wpSetUpBeforeClass_DismissIssues method and the
static properties $dismiss_test_posts and $dismiss_test_issues are dead code
because WordPress only auto-calls wpSetUpBeforeClass; either remove
wpSetUpBeforeClass_DismissIssues and the two properties entirely (Option A) or
invoke it from the class's wpSetUpBeforeClass and refactor tests to use the
shared fixtures (Option B); locate the method wpSetUpBeforeClass_DismissIssues
and the properties $dismiss_test_posts / $dismiss_test_issues and implement one
of these two fixes consistently across the test class.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf744dbf-6a4f-4331-bcb2-9725dc771d1a
📒 Files selected for processing (2)
includes/classes/class-rest-api.phptests/phpunit/includes/classes/RestApiEndpointsTest.php
…-tests-and-updates
4 tasks
This was referenced Jul 28, 2026
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
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.
Summary
Adds comprehensive PHPUnit tests for the
POST /accessibility-checker/v1/dismiss-issue/{issue_id}REST endpoint covering both single-issue and large-batch dismiss modes.Tests Added
All tests added to
RestApiEndpointsTest:test_single_issue_dismiss_authorized_useredit_poston the post dismisses a single issueignre=1, reason, comment, and user recordedtest_single_issue_dismiss_unauthorized_useredit_postattempts to dismiss a single issuetest_large_batch_dismiss_authorized_on_alllargeBatch=truelarge_batch=true, all issues in batch updated via single bulk UPDATEtest_large_batch_dismiss_authorized_on_sometest_large_batch_dismiss_unauthorized_on_allWhat This Verifies
edit_post) blocks access before any DB writeSummary by CodeRabbit
Bug Fixes
Tests