add api test case#567
Conversation
tested upto 6.7 & version bump
…sure-issue Data Exposure limited response
…load-issue File download issue
…file-download-issue- Frontend file download issue
…oy-test Auto deploy by github
…oy-test direct deploy
Refactored compatibility-checker.php to dynamically detect and handle multiple Pro package slugs during activation, deactivation, and compatibility checks. Updated logic to prevent activation and deactivate any incompatible Pro package (version < 3.0.0) by iterating over all possible Pro plugin slugs. Also updated config/app.php to use PM_PRO_VERSION for versioning.
Replaced PM_PRO_VERSION with PM_VERSION in config/app.php for version consistency. Cleaned up language file by removing translator comments and a redundant text domain in one string.
Deleted an unused import in Singletonable.php and replaced $wpdb->prefix . $table with $full_table_name in a database check within Commands.php. Also removed the .cursor/rules/wp-project-manager.mdc file.
Refactored translation functions to use esc_html__ and esc_html_e for improved escaping and translator comments. Updated Textdomain class to load texts using pm_load_texts and enhanced get_text to support nested keys and named sprintf. Removed redundant load_plugin_textdomain call from Frontend class to streamline initialization.
Replaces direct access to $_GET for 'incomplete_task_page' with WP_Router's request object for improved request handling and consistency.
Added api-schema.json to .distignore and .gitattributes for export-ignore. Updated changelog for v3.0.0 with improvements, fixes, and disclosures. Revised readme.txt to reflect new stable tag, PHP requirement, and other metadata for WordPress.org compliance.
…nvention_changes Fix/naming convention changes
Replaces the custom pm_load_texts() function with wedevs_pm_load_texts(), which now returns an empty array and is marked as deprecated. All translations are now handled using standard WordPress internationalization functions.
Renamed the plugin from 'weDevs Project Manager' to 'Project Manager' throughout cpm.php and readme.txt. Updated documentation, feature descriptions, FAQs, and third-party service disclosures for consistency and clarity. Improved plugin metadata, changelog, and external service sections to align with WordPress.org standards.
Changed the text domain for 'Add List' from 'cpm' to 'wedevs-project-manager' for consistency. Added a translator comment for the 'Request failed: %s' string to improve localization clarity.
Changed the i18n text domain in a comment for consistency and updated error message formatting in project-ai-create-form.vue to concatenate the status code outside the translation string.
Removed unused or duplicate translation strings, corrected text domains, and standardized some message formats in wedevs-project-manager.php for improved consistency and maintainability.
Set 'dry-run' to false in the deploy workflow to allow actual deployments. Added a privacy policy section to readme.txt describing Appsero SDK telemetry data collection and user consent.
Bumped version to 3.0.1 in cpm.php and package.json. Removed the .cursorindexingignore development file. Updated changelog and readme to reflect the removal of development files.
Downgrades version numbers in cpm.php and package.json from 3.0.1 to 3.0.0. Removes the 3.0.1 changelog entry from changelog.txt and readme.txt, reverting to the previous release state.
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (4)
tests/api/test-task-lists.php-137-168 (1)
137-168:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen sorting test behavior checks and cleanup for test isolation.
The sorting test only checks status 200 and creates an extra task-list row that is not explicitly cleaned up. This can hide broken sorting logic and leave residual rows between tests.
Suggested hardening
class PM_Task_Lists_API_Test extends PM_API_Test_Case { protected $project_id; protected $task_list_id; + protected $extra_task_list_ids = []; @@ $second_list_id = $wpdb->insert_id; + $this->extra_task_list_ids[] = $second_list_id; @@ $response = $this->server->dispatch($request); $this->assertEquals(200, $response->get_status()); + + $updated_order = $wpdb->get_var($wpdb->prepare( + "SELECT `order` FROM {$wpdb->prefix}pm_boards WHERE id = %d", + $this->task_list_id + )); + $this->assertEquals(1, (int) $updated_order); @@ public function tearDown() { global $wpdb; + if (!empty($this->extra_task_list_ids)) { + foreach ($this->extra_task_list_ids as $id) { + $wpdb->delete($wpdb->prefix . 'pm_boards', ['id' => $id]); + } + } $wpdb->delete($wpdb->prefix . 'pm_boards', ['id' => $this->task_list_id]); $wpdb->delete($wpdb->prefix . 'pm_projects', ['id' => $this->project_id]); parent::tearDown(); }Also applies to: 202-206
🤖 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/api/test-task-lists.php` around lines 137 - 168, Update test_task_list_sorting to assert that the new ordering was persisted (e.g., query $wpdb for the two rows with IDs $second_list_id and $this->task_list_id and assert their 'order' values match the request) and also validate response body if available from the POST to '/pm/v2/projects/{project_id}/lists/sorting'; finally ensure test isolation by deleting the inserted row via $wpdb->delete (using $second_list_id) or otherwise cleaning it up in tearDown so the extra task-list row does not remain after the test.tests/api/test-tasks.php-15-26 (1)
15-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd error handling for database inserts in setUp.
If
$wpdb->insert()fails, subsequent tests will fail with confusing errors. Consider verifying that inserts succeeded and that$wpdb->insert_idis valid.🛡️ Proposed fix
// Create a test project -$wpdb->insert( +$result = $wpdb->insert( $wpdb->prefix . 'pm_projects', [ 'title' => 'Test Project', 'description' => 'Test project for tasks', 'status' => 0, 'created_by' => $this->admin_user, 'created_at' => current_time('mysql'), 'updated_at' => current_time('mysql') ] ); +$this->assertNotFalse($result, 'Failed to create test project'); $this->project_id = $wpdb->insert_id; +$this->assertGreaterThan(0, $this->project_id, 'Invalid project ID');Apply similar checks for task list and task inserts.
🤖 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/api/test-tasks.php` around lines 15 - 26, In setUp, after calling $wpdb->insert(...) for creating the project, check the return value and $wpdb->insert_id to ensure the insert succeeded; if the insert returned false or insert_id is empty/zero, fail early (throw or use a test assertion) with a clear message so downstream tests don't run on invalid state; apply the same checks for the task list and task inserts (verify $wpdb->insert() !== false and a valid $wpdb->insert_id before assigning $this->project_id / $this->task_list_id / $this->task_id).tests/api/test-tasks.php-96-112 (1)
96-112:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrack and clean up the created task.
The test creates a new task but doesn't track its ID for cleanup. This leaves orphaned records in the database after each test run, which can accumulate and affect test performance or cause issues with unique constraints.
🧹 Proposed fix
Track the created task ID as a class property and clean it up in
tearDown():protected $project_id; protected $task_list_id; protected $task_id; +protected $created_task_ids = [];In the test:
$response = $this->server->dispatch($request); $this->assertEquals(201, $response->get_status()); $data = $response->get_data(); $this->assertEquals('New Test Task', $data['data']['title']); +$this->created_task_ids[] = $data['data']['id'];In
tearDown():public function tearDown() { global $wpdb; + foreach ($this->created_task_ids as $task_id) { + $wpdb->delete($wpdb->prefix . 'pm_boards', ['id' => $task_id]); + } $wpdb->delete($wpdb->prefix . 'pm_boards', ['id' => $this->task_id]);🤖 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/api/test-tasks.php` around lines 96 - 112, The test_create_task creates a task but doesn't record its ID for cleanup; update test_create_task to capture the created task ID from $response->get_data()['data']['id'] (e.g. push into a class property like $this->created_task_ids) and implement tearDown() to iterate over $this->created_task_ids and delete each created task (via the appropriate deletion helper for your app or WP helper) so tests remove orphaned records; reference test_create_task and tearDown to locate where to store and clean the IDs, and use $this->project_id/$this->task_list_id context if needed when deleting.tests/api/test-tasks.php-203-211 (1)
203-211:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClean up the inserted assignee record.
The test inserts an assignee record but never cleans it up in
tearDown(). This leaves orphaned records in thepm_assigneestable after each test run.🧹 Proposed fix
Track the inserted assignee and clean it up:
protected $project_id; protected $task_list_id; protected $task_id; +protected $test_assignee_ids = [];In the test:
// First attach user global $wpdb; $wpdb->insert( $wpdb->prefix . 'pm_assignees', [ 'assigned_to' => $this->editor_user, 'assignable_id' => $this->task_id, 'assignable_type' => 'task', 'project_id' => $this->project_id ] ); +$this->test_assignee_ids[] = $wpdb->insert_id;In
tearDown():public function tearDown() { global $wpdb; + foreach ($this->test_assignee_ids as $assignee_id) { + $wpdb->delete($wpdb->prefix . 'pm_assignees', ['id' => $assignee_id]); + } $wpdb->delete($wpdb->prefix . 'pm_boards', ['id' => $this->task_id]);🤖 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/api/test-tasks.php` around lines 203 - 211, The test inserts a row into pm_assignees but never removes it; modify the test to capture and store the inserted row ID (use $wpdb->insert_id after calling $wpdb->insert in the test where you insert with assigned_to => $this->editor_user, assignable_id => $this->task_id, assignable_type => 'task', project_id => $this->project_id) into a property like $this->assignee_id, then in tearDown() delete that record (use $wpdb->delete on $wpdb->prefix . 'pm_assignees' with the id or the same identifying columns) and null out the property to ensure cleanup even if the insert failed.
🧹 Nitpick comments (10)
tests/api/test-files.php (1)
141-148: ⚡ Quick winAssert explicit auth failure code instead of “not 200”.
assertNotEquals(200, ...)can pass on unrelated server errors. Assert401/403(and optionally error code) so this test verifies permission behavior, not just failure.Suggested fix
- $this->assertNotEquals(200, $response->get_status()); + $this->assertContains($response->get_status(), [401, 403]);🤖 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/api/test-files.php` around lines 141 - 148, Replace the vague assertion in test_get_files_without_permission so it explicitly checks for an authorization failure: call $this->server->dispatch($request) as before but assert the response status is 401 or 403 (e.g., $this->assertContains($response->get_status(), [401,403]) or separate assertions), and optionally decode $response->get_data() to assert the expected REST error code/message; modify the test_get_files_without_permission function and use $response->get_status() and $response->get_data() to validate permission-specific behavior instead of assertNotEquals(200,...).tests/api/test-milestones.php (4)
170-181: ⚡ Quick winAssert specific error status code for permission failure.
The test uses
assertNotEquals(201)to check for permission denial, but this could pass with any status code. For a permission error, you should assert 401 (Unauthorized) or 403 (Forbidden), depending on your API's convention.✅ Proposed fix to assert specific status code
$response = $this->server->dispatch($request); -$this->assertNotEquals(201, $response->get_status()); +// Use 401 for Unauthorized or 403 for Forbidden based on your API convention +$this->assertContains($response->get_status(), [401, 403]);🤖 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/api/test-milestones.php` around lines 170 - 181, The test method test_create_milestone_without_permission should assert a specific permission error status instead of assertNotEquals(201); replace the generic assertion with an explicit check on $response->get_status() (e.g. assertEquals(403, $response->get_status()) if your API uses Forbidden for lacking permissions, or assertEquals(401, ...) if it uses Unauthorized) so the test fails only when the API returns the wrong permission status.
78-94: ⚡ Quick winClean up the created milestone in this test.
The test creates a new milestone but does not clean it up. This leaves test data in the database and could cause issues if tests are run multiple times or if other tests expect a clean state.
🧹 Proposed fix to add cleanup
public function test_create_milestone() { wp_set_current_user($this->admin_user); $request = new WP_REST_Request('POST', '/pm/v2/projects/' . $this->project_id . '/milestones'); $request->set_body_params([ 'title' => 'New Milestone', 'description' => 'New milestone description', 'start_date' => date('Y-m-d'), 'end_date' => date('Y-m-d', strtotime('+30 days')) ]); $response = $this->server->dispatch($request); $this->assertEquals(201, $response->get_status()); $data = $response->get_data(); $this->assertEquals('New Milestone', $data['data']['title']); + + // Clean up the created milestone + global $wpdb; + $wpdb->delete($wpdb->prefix . 'pm_boards', ['id' => $data['data']['id']]); }🤖 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/api/test-milestones.php` around lines 78 - 94, The test_create_milestone creates a milestone but never deletes it; after retrieving the created milestone id from $response->get_data()['data']['id'], dispatch a DELETE WP_REST_Request to '/pm/v2/projects/' . $this->project_id . '/milestones/' . $milestone_id (using the same $this->server) and assert successful deletion (e.g. 200/204) so the created record is removed; alternatively, call any existing helper like a delete_milestone method in tearDown to remove the created milestone if present.
154-165: ⚡ Quick winAssert specific error status code instead of "not 201".
The test uses
assertNotEquals(201)to check for failure, but this could pass with any status code (200, 400, 403, 500, etc.). For a validation error, you should assert a specific error code, typically 400 for bad request.✅ Proposed fix to assert specific status code
$response = $this->server->dispatch($request); -$this->assertNotEquals(201, $response->get_status()); +$this->assertEquals(400, $response->get_status());🤖 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/api/test-milestones.php` around lines 154 - 165, In test_create_milestone_without_title, replace the generic assertNotEquals(201, $response->get_status()) with a specific assertion for the expected validation error status (e.g., assertEquals(400, $response->get_status())) so the test verifies a Bad Request response; update the assertion in the test_create_milestone_without_title method to assert the exact HTTP status code returned for missing required title.
138-149: ⚡ Quick winVerify that privacy was actually set.
The test only checks for a 200 status but does not verify that the privacy value was actually updated in the response or database. Consider adding an assertion to confirm the privacy field was set to the expected value.
✅ Proposed enhancement to verify privacy value
$response = $this->server->dispatch($request); $this->assertEquals(200, $response->get_status()); + +// Verify privacy was actually set +$data = $response->get_data(); +$this->assertEquals(1, $data['data']['privacy']);🤖 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/api/test-milestones.php` around lines 138 - 149, The test_milestone_privacy test only asserts a 200 status; extend it to verify the privacy was actually updated by either inspecting the response body or querying the model/DB for the milestone with $this->milestone_id; after dispatching $request and asserting 200, decode $response->get_data() (or load the milestone record) and assert that the privacy field equals the expected value (1) so the test confirms the update occurred.tests/api/test-users.php (1)
58-70: ⚡ Quick winMutation tests should assert persisted side effects, not only status codes.
These three tests currently pass even if the endpoint returns success without changing DB state. Add DB-level assertions for add/remove/role-update outcomes.
Example assertions to add
global $wpdb; // add user $count = (int) $wpdb->get_var( $wpdb->prepare( "SELECT COUNT(*) FROM {$wpdb->prefix}pm_assignees WHERE project_id = %d AND assigned_to = %d AND assignable_type = %s", $this->project_id, $this->subscriber_user, 'project' ) ); $this->assertSame(1, $count); // remove user $count = (int) $wpdb->get_var( $wpdb->prepare( "SELECT COUNT(*) FROM {$wpdb->prefix}pm_assignees WHERE project_id = %d AND assigned_to = %d AND assignable_type = %s", $this->project_id, $this->editor_user, 'project' ) ); $this->assertSame(0, $count);Also applies to: 75-82, 87-98
🤖 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/api/test-users.php` around lines 58 - 70, The test test_add_users_to_project currently only asserts the response status; update it (and the analogous tests around lines 75-82 and 87-98) to assert DB-side effects by querying the pm_assignees table via global $wpdb after calling $this->server->dispatch($request); specifically, use $wpdb->get_var with $wpdb->prepare to count rows where project_id = $this->project_id, assigned_to = $this->subscriber_user (or $this->editor_user for removals), and assignable_type = 'project', then assert the expected counts (e.g., 1 for add, 0 for remove) and for role-update tests assert the role_id stored for that assignee matches the new role_id.tests/api/test-projects.php (1)
269-281: ⚡ Quick winAI endpoint test currently accepts regression statuses.
Allowing
400and503as unconditional pass statuses makes this test low-signal. Prefer skipping when dependency is unavailable, and assert success otherwise.💡 Suggested pattern
$response = $this->server->dispatch($request); - - // AI endpoint might not be available in test environment - $this->assertContains($response->get_status(), [200, 201, 400, 503]); + $status = $response->get_status(); + if ($status === 503) { + $this->markTestSkipped('AI service unavailable in test environment.'); + } + $this->assertContains($status, [200, 201]);🤖 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/api/test-projects.php` around lines 269 - 281, The test method test_ai_generate_project should not treat 400/503 as passing; change it so that after dispatching the request you detect if the AI dependency is unavailable (response status 400 or 503) and call $this->markTestSkipped(...) with an explanatory message, otherwise assert the response status is a success (200 or 201); update the logic inside test_ai_generate_project (the WP_REST_Request creation, $this->server->dispatch(...) and downstream assertions) so skipped statuses cause a skip and only 200/201 are treated as passing.tests/api/test-tasks.php (3)
154-165: ⚡ Quick winConsider verifying the status was actually changed.
The test only checks for a 200 status code but doesn't verify that the task status was actually updated. Consider adding an assertion to check the status in the response data.
✨ Suggested enhancement
$response = $this->server->dispatch($request); $this->assertEquals(200, $response->get_status()); +$data = $response->get_data(); +$this->assertEquals(1, $data['data']['status'], 'Task status should be updated to 1');🤖 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/api/test-tasks.php` around lines 154 - 165, test_change_task_status only asserts HTTP 200 but doesn't verify the task's status changed; after dispatching the request in test_change_task_status, decode the response body or re-fetch the task using the existing API (or Task model) and assert that the task with id $this->task_id now has status == 1 (or that the response data contains status == 1), so the test both checks the HTTP response and validates the actual state change.
226-238: ⚡ Quick winConsider verifying the filter results.
The test sends filter parameters but only checks for a 200 status code. Consider verifying that the returned tasks actually match the filter criteria (e.g., check that all returned tasks have
status = 0).✨ Suggested enhancement
$response = $this->server->dispatch($request); $this->assertEquals(200, $response->get_status()); +$data = $response->get_data(); +$this->assertArrayHasKey('data', $data); +// Verify all returned tasks match the filter criteria +foreach ($data['data'] as $task) { + $this->assertEquals(0, $task['status'], 'All tasks should have status = 0'); +}🤖 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/api/test-tasks.php` around lines 226 - 238, The test_filter_tasks currently only asserts the HTTP status; update it to fetch and inspect the response payload from $response (use $response->get_data() or json_decode($response->get_body(), true) depending on response type) and assert that the returned items are tasks and each has 'status' === 0 and 'assigned_to' matches $this->admin_user (iterate over the task array and assert per-item conditions); keep the existing WP_REST_Request setup and $response status assertion but add these content assertions to ensure the filter actually worked.
182-193: ⚡ Quick winConsider verifying the user was actually attached.
The test only checks for a 200 status code but doesn't verify that the user assignment was actually created. Consider querying the database or checking the response data to confirm the attachment succeeded.
✨ Suggested enhancement
$response = $this->server->dispatch($request); $this->assertEquals(200, $response->get_status()); + +// Verify the assignment was created +global $wpdb; +$assignee = $wpdb->get_row($wpdb->prepare( + "SELECT * FROM {$wpdb->prefix}pm_assignees WHERE assigned_to = %d AND assignable_id = %d AND assignable_type = 'task'", + $this->editor_user, + $this->task_id +)); +$this->assertNotNull($assignee, 'User should be assigned to the task');🤖 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/api/test-tasks.php` around lines 182 - 193, The test test_attach_users_to_task only asserts a 200 status; after dispatching the PUT you should verify the editor was actually attached by fetching the updated task or assignment list and asserting the user appears: after $response = $this->server->dispatch($request) call the same API (e.g. GET '/pm/v2/projects/{project_id}/tasks/{task_id}' or the endpoint that returns task users) or load assignments from the DB for $this->task_id, then assert the returned data contains $this->editor_user (or its ID) — update test_attach_users_to_task to perform that additional fetch and an assertion on the returned users collection.
🤖 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 `@tests/api/test-activities.php`:
- Around line 104-136: The current tests test_get_activities_by_resource_type
and test_get_activities_by_resource_id only assert that the response is an
array/non-empty so they pass even if filters are ignored; update each test to
seed at least one activity that does NOT match the requested filter (e.g.,
create an activity with a different resource_type or resource_id using the same
setup helpers), then after dispatching the request iterate over $data['data']
and assert every returned item's resource_type (in
test_get_activities_by_resource_type) or resource_id and resource_type (in
test_get_activities_by_resource_id) equals the requested value and that the
non-matching seeded activity is not present; use the existing test helpers/ids
($this->project_id, $this->task_id, $this->server, WP_REST_Request) to locate
where to add the extra seed and the per-item assertions.
- Around line 85-99: test_get_activities_with_pagination currently only asserts
200 and presence of 'data' but doesn't validate pagination; seed multiple
activities for $this->project_id (e.g., create 3 activities via your project
activity factory/helper used elsewhere), then issue WP_REST_Request GETs to
'/pm/v2/projects/' . $this->project_id . '/activities' with different query
params (per_page=1&page=1 and per_page=1&page=2) using $this->server->dispatch,
assert each response status is 200, assert each response data array length
equals the requested per_page (1), assert the returned items differ between
page=1 and page=2, and assert pagination headers like X-WP-Total (from
$response->get_headers()) equals the total seeded count to confirm correct
pagination semantics.
- Around line 15-58: In setUp() ensure each $wpdb->insert() is checked and the
test fails immediately if an insert returned false: after the project insert
(which sets $this->project_id), after the board/task insert (which sets
$this->task_id), and after the activity insert (which sets $this->activity_id)
verify the insert result and/or that $wpdb->insert_id is truthy; if not, call
$this->fail() (or throw) with a clear message indicating which insert failed so
fixtures cannot silently produce invalid IDs for the rest of the test.
In `@tests/api/test-comments.php`:
- Around line 169-170: The current assertion uses $this->assertNotEquals(201,
$response->get_status()), which can still pass on 5xx server errors; replace it
with a strict 4xx check by asserting the status is between 400 and 499 (e.g. use
$status = $response->get_status(); then $this->assertGreaterThanOrEqual(400,
$status) and $this->assertLessThan(500, $status) or a single
$this->assertTrue($status >= 400 && $status < 500)); update the assertion
referencing $response->get_status() and remove the assertNotEquals call.
- Around line 190-194: tearDown() currently only deletes the seeded comment
($this->comment_id) and leaves comments created by test_create_comment() behind;
update tearDown to remove all pm_comments rows related to the test task (e.g.
delete where task_id = $this->task_id) in addition to the seeded id. Use $wpdb
to either run a prepared DELETE query (via $wpdb->prepare and $wpdb->query) or
fetch comment IDs for the task and loop $wpdb->delete over them; reference
tearDown(), $wpdb, $this->comment_id, $this->task_id, pm_comments, and
test_create_comment() when making the change.
- Around line 187-188: Replace the weak assertion at the end of the test that
uses assertNotEquals(200, $response->get_status()) with a strict check for an
authorization failure: assert that $response->get_status() is either 401
(unauthorized) or 403 (forbidden). Locate the assertion using the
$response->get_status() call in the test and change it to explicitly verify the
expected permission status (e.g., assertTrue(in_array($response->get_status(),
[401, 403])) or two separate assertEquals checks), so the test fails on server
errors like 500 but passes only for proper auth failures.
In `@tests/api/test-files.php`:
- Around line 81-100: The test_upload_file test creates an attachment
($test_attachment_id) but never deletes it; update test_upload_file to remove
the created attachment after the request (e.g., call
wp_delete_attachment($test_attachment_id, true) or register it for deletion in
tearDown) so the media/post fixture doesn't leak, and apply the same cleanup to
the other test that creates an attachment (the test around lines 166-171) to
ensure all created attachments are removed after each test run.
- Around line 1-3: This new PHP test file lacks the required GPL-2.0+ license
header; add the standard project license comment block at the top of the file
(immediately after the <?php tag) following the same header format used in other
PHP sources, ensuring it declares "GPL-2.0+" and includes the
copyright/year/author lines as per project conventions so that the class
declaration (PM_Files_API_Test extends PM_API_Test_Case) remains unchanged.
In `@tests/api/test-milestones.php`:
- Around line 14-25: In setUp() of tests/api/test-milestones.php the code
doesn't verify the success of $wpdb->insert, so
$this->project_id/$this->milestone_id can be 0/false; after calling
$wpdb->insert(...) for the project and for the milestone check the insert return
(or $wpdb->insert_id) and fail early if it indicates failure—e.g. capture the
result, assert it's truthy or call $this->fail with a message that includes
$wpdb->last_error, and only then assign $this->project_id/$this->milestone_id
from $wpdb->insert_id; this ensures clear test failures and avoids subsequent
cryptic invalid-ID errors.
- Around line 118-133: The test_delete_milestone is deleting the shared fixture
$this->milestone_id (created in setUp()), causing test-order dependencies and
double-delete in tearDown(); fix it by creating a dedicated milestone inside
test_delete_milestone (e.g., call the existing fixture helper or factory to
create a new milestone and store its id in a local variable like
$temp_milestone_id), use that id in the WP_REST_Request URI and subsequent DB
assertions, and ensure tearDown() still only cleans up the original
$this->milestone_id (or track created test-specific IDs so they are removed once
and not by tearDown()).
In `@tests/api/test-projects.php`:
- Around line 71-86: The teardown currently only removes $this->project_id so
tests that create extra projects (e.g., test_create_project) leave rows behind;
update the test class to track all created project IDs (add a property like
$created_project_ids and push new IDs inside test_create_project and any other
tests that create projects) and modify tearDown() to loop through
$this->created_project_ids (and $this->project_id if still used) to delete each
project via the same deletion method used elsewhere (e.g., wp_delete_post or the
project model's delete function), then clear the array so every created project
is removed after each test run.
- Around line 286-297: The test_update_project_without_permission test uses
assertNotEquals(200, $response->get_status()), which is too weak; update the
assertion to assert the expected authorization failure code instead (for example
use $this->assertEquals(403, $response->get_status()) or
assertTrue(in_array($response->get_status(), [401,403])) to explicitly verify
auth failure), referencing the test method
test_update_project_without_permission and the $response variable so the test
fails only on unexpected statuses.
- Around line 174-214: The class contains duplicate PHPUnit method names—another
test_update_project and test_delete_project are redeclared—so rename these
duplicate methods to unique names (e.g., change test_update_project to
test_update_project_v2 or test_update_project_api and change test_delete_project
to test_delete_project_v2 or test_delete_project_api_delete) and update any
references if used elsewhere; keep the test bodies, assertions,
wp_set_current_user calls and request endpoints (methods test_update_project and
test_delete_project in this file) unchanged aside from the method identifiers so
PHP class loading no longer fails.
In `@tests/api/test-task-lists.php`:
- Around line 1-3: Add the required GPL-2.0+ file header to this new PHP test
file to meet licensing rules: insert the standard GPL-2.0+ comment block at the
top of tests/api/test-task-lists.php before the opening <?php so the file
explicitly declares the license; ensure the header appears above the declaration
of class PM_Task_Lists_API_Test (which extends PM_API_Test_Case) and follows the
project's canonical GPL-2.0+ header text and formatting.
- Around line 189-200: The test test_create_task_list_without_permission
currently uses assertNotEquals(201, $response->get_status()) which is too broad;
change it to assert the exact permission failure status (e.g. assertEquals(403,
$response->get_status())) after dispatching the WP_REST_Request in order to
ensure the unauthorized create attempt via server->dispatch returns the expected
auth error code rather than any other error.
In `@tests/api/test-tasks.php`:
- Around line 1-3: Add the required GPL-2.0+ license header at the top of the
PHP file containing the PM_Tasks_API_Test class (tests/api/test-tasks.php) so
the file begins with the standard multi-line GPL-2.0+ comment block before the
opening <?php tag; ensure the header references GPL-2.0+ and includes
author/project/year info per project convention and keep the class declaration
(class PM_Tasks_API_Test extends PM_API_Test_Case) and existing code unchanged
below it.
In `@tests/api/test-users.php`:
- Around line 13-24: In setUp(), immediately verify that each $wpdb->insert call
succeeds instead of letting later tests fail ambiguously: wrap the $wpdb->insert
for the project (and the similar insert around lines 27-37) with assertions such
as $this->assertNotFalse($wpdb->insert(...), 'Failed to insert project/user
fixture') and then assert $wpdb->insert_id is non-empty before assigning
$this->project_id (and equivalent $this->user_id), so failed inserts fail fast
and clearly; update the code around the $wpdb->insert calls and assignments to
$this->project_id/$this->user_id accordingly.
- Around line 169-179: The test method test_non_admin_cannot_add_users currently
uses assertNotEquals(201, $response->get_status()) which is too weak; change it
to assertEquals(403, $response->get_status()) (or the explicit auth failure
status your API returns) to assert an authorization failure. Locate the
test_non_admin_cannot_add_users function and replace the assertNotEquals(201,
$response->get_status()) call with an assertion that the response status is the
expected auth error (e.g., assertEquals(403, $response->get_status())), keeping
the existing request setup using WP_REST_Request and $this->server->dispatch.
---
Minor comments:
In `@tests/api/test-task-lists.php`:
- Around line 137-168: Update test_task_list_sorting to assert that the new
ordering was persisted (e.g., query $wpdb for the two rows with IDs
$second_list_id and $this->task_list_id and assert their 'order' values match
the request) and also validate response body if available from the POST to
'/pm/v2/projects/{project_id}/lists/sorting'; finally ensure test isolation by
deleting the inserted row via $wpdb->delete (using $second_list_id) or otherwise
cleaning it up in tearDown so the extra task-list row does not remain after the
test.
In `@tests/api/test-tasks.php`:
- Around line 15-26: In setUp, after calling $wpdb->insert(...) for creating the
project, check the return value and $wpdb->insert_id to ensure the insert
succeeded; if the insert returned false or insert_id is empty/zero, fail early
(throw or use a test assertion) with a clear message so downstream tests don't
run on invalid state; apply the same checks for the task list and task inserts
(verify $wpdb->insert() !== false and a valid $wpdb->insert_id before assigning
$this->project_id / $this->task_list_id / $this->task_id).
- Around line 96-112: The test_create_task creates a task but doesn't record its
ID for cleanup; update test_create_task to capture the created task ID from
$response->get_data()['data']['id'] (e.g. push into a class property like
$this->created_task_ids) and implement tearDown() to iterate over
$this->created_task_ids and delete each created task (via the appropriate
deletion helper for your app or WP helper) so tests remove orphaned records;
reference test_create_task and tearDown to locate where to store and clean the
IDs, and use $this->project_id/$this->task_list_id context if needed when
deleting.
- Around line 203-211: The test inserts a row into pm_assignees but never
removes it; modify the test to capture and store the inserted row ID (use
$wpdb->insert_id after calling $wpdb->insert in the test where you insert with
assigned_to => $this->editor_user, assignable_id => $this->task_id,
assignable_type => 'task', project_id => $this->project_id) into a property like
$this->assignee_id, then in tearDown() delete that record (use $wpdb->delete on
$wpdb->prefix . 'pm_assignees' with the id or the same identifying columns) and
null out the property to ensure cleanup even if the insert failed.
---
Nitpick comments:
In `@tests/api/test-files.php`:
- Around line 141-148: Replace the vague assertion in
test_get_files_without_permission so it explicitly checks for an authorization
failure: call $this->server->dispatch($request) as before but assert the
response status is 401 or 403 (e.g.,
$this->assertContains($response->get_status(), [401,403]) or separate
assertions), and optionally decode $response->get_data() to assert the expected
REST error code/message; modify the test_get_files_without_permission function
and use $response->get_status() and $response->get_data() to validate
permission-specific behavior instead of assertNotEquals(200,...).
In `@tests/api/test-milestones.php`:
- Around line 170-181: The test method test_create_milestone_without_permission
should assert a specific permission error status instead of
assertNotEquals(201); replace the generic assertion with an explicit check on
$response->get_status() (e.g. assertEquals(403, $response->get_status()) if your
API uses Forbidden for lacking permissions, or assertEquals(401, ...) if it uses
Unauthorized) so the test fails only when the API returns the wrong permission
status.
- Around line 78-94: The test_create_milestone creates a milestone but never
deletes it; after retrieving the created milestone id from
$response->get_data()['data']['id'], dispatch a DELETE WP_REST_Request to
'/pm/v2/projects/' . $this->project_id . '/milestones/' . $milestone_id (using
the same $this->server) and assert successful deletion (e.g. 200/204) so the
created record is removed; alternatively, call any existing helper like a
delete_milestone method in tearDown to remove the created milestone if present.
- Around line 154-165: In test_create_milestone_without_title, replace the
generic assertNotEquals(201, $response->get_status()) with a specific assertion
for the expected validation error status (e.g., assertEquals(400,
$response->get_status())) so the test verifies a Bad Request response; update
the assertion in the test_create_milestone_without_title method to assert the
exact HTTP status code returned for missing required title.
- Around line 138-149: The test_milestone_privacy test only asserts a 200
status; extend it to verify the privacy was actually updated by either
inspecting the response body or querying the model/DB for the milestone with
$this->milestone_id; after dispatching $request and asserting 200, decode
$response->get_data() (or load the milestone record) and assert that the privacy
field equals the expected value (1) so the test confirms the update occurred.
In `@tests/api/test-projects.php`:
- Around line 269-281: The test method test_ai_generate_project should not treat
400/503 as passing; change it so that after dispatching the request you detect
if the AI dependency is unavailable (response status 400 or 503) and call
$this->markTestSkipped(...) with an explanatory message, otherwise assert the
response status is a success (200 or 201); update the logic inside
test_ai_generate_project (the WP_REST_Request creation,
$this->server->dispatch(...) and downstream assertions) so skipped statuses
cause a skip and only 200/201 are treated as passing.
In `@tests/api/test-tasks.php`:
- Around line 154-165: test_change_task_status only asserts HTTP 200 but doesn't
verify the task's status changed; after dispatching the request in
test_change_task_status, decode the response body or re-fetch the task using the
existing API (or Task model) and assert that the task with id $this->task_id now
has status == 1 (or that the response data contains status == 1), so the test
both checks the HTTP response and validates the actual state change.
- Around line 226-238: The test_filter_tasks currently only asserts the HTTP
status; update it to fetch and inspect the response payload from $response (use
$response->get_data() or json_decode($response->get_body(), true) depending on
response type) and assert that the returned items are tasks and each has
'status' === 0 and 'assigned_to' matches $this->admin_user (iterate over the
task array and assert per-item conditions); keep the existing WP_REST_Request
setup and $response status assertion but add these content assertions to ensure
the filter actually worked.
- Around line 182-193: The test test_attach_users_to_task only asserts a 200
status; after dispatching the PUT you should verify the editor was actually
attached by fetching the updated task or assignment list and asserting the user
appears: after $response = $this->server->dispatch($request) call the same API
(e.g. GET '/pm/v2/projects/{project_id}/tasks/{task_id}' or the endpoint that
returns task users) or load assignments from the DB for $this->task_id, then
assert the returned data contains $this->editor_user (or its ID) — update
test_attach_users_to_task to perform that additional fetch and an assertion on
the returned users collection.
In `@tests/api/test-users.php`:
- Around line 58-70: The test test_add_users_to_project currently only asserts
the response status; update it (and the analogous tests around lines 75-82 and
87-98) to assert DB-side effects by querying the pm_assignees table via global
$wpdb after calling $this->server->dispatch($request); specifically, use
$wpdb->get_var with $wpdb->prepare to count rows where project_id =
$this->project_id, assigned_to = $this->subscriber_user (or $this->editor_user
for removals), and assignable_type = 'project', then assert the expected counts
(e.g., 1 for add, 0 for remove) and for role-update tests assert the role_id
stored for that assignee matches the new role_id.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a594c9e1-a62c-45c0-b531-a9e0035023aa
📒 Files selected for processing (9)
tests/api/README.mdtests/api/test-activities.phptests/api/test-comments.phptests/api/test-files.phptests/api/test-milestones.phptests/api/test-projects.phptests/api/test-task-lists.phptests/api/test-tasks.phptests/api/test-users.php
✅ Files skipped from review due to trivial changes (1)
- tests/api/README.md
| $wpdb->insert( | ||
| $wpdb->prefix . 'pm_projects', | ||
| [ | ||
| 'title' => 'Test Project', | ||
| 'description' => 'Test project for activities', | ||
| 'status' => 0, | ||
| 'created_by' => $this->admin_user, | ||
| 'created_at' => current_time('mysql'), | ||
| 'updated_at' => current_time('mysql') | ||
| ] | ||
| ); | ||
| $this->project_id = $wpdb->insert_id; | ||
|
|
||
| // Create a test task | ||
| $wpdb->insert( | ||
| $wpdb->prefix . 'pm_boards', | ||
| [ | ||
| 'title' => 'Test Task', | ||
| 'project_id' => $this->project_id, | ||
| 'type' => 'task', | ||
| 'status' => 0, | ||
| 'order' => 0, | ||
| 'created_by' => $this->admin_user, | ||
| 'created_at' => current_time('mysql'), | ||
| 'updated_at' => current_time('mysql') | ||
| ] | ||
| ); | ||
| $this->task_id = $wpdb->insert_id; | ||
|
|
||
| // Create a test activity | ||
| $wpdb->insert( | ||
| $wpdb->prefix . 'pm_activities', | ||
| [ | ||
| 'actor_id' => $this->admin_user, | ||
| 'action' => 'created', | ||
| 'action_type' => 'task', | ||
| 'resource_id' => $this->task_id, | ||
| 'resource_type' => 'task', | ||
| 'project_id' => $this->project_id, | ||
| 'created_at' => current_time('mysql'), | ||
| 'updated_at' => current_time('mysql') | ||
| ] | ||
| ); | ||
| $this->activity_id = $wpdb->insert_id; |
There was a problem hiding this comment.
Fail fast on fixture insert failures in setUp().
On Line 15, Line 29, and Line 45, $wpdb->insert() return values are not checked. If one insert fails, later tests fail in misleading ways (or pass for the wrong reason) with invalid IDs.
Suggested fix
- $wpdb->insert(
+ $inserted = $wpdb->insert(
$wpdb->prefix . 'pm_projects',
[
'title' => 'Test Project',
'description' => 'Test project for activities',
'status' => 0,
'created_by' => $this->admin_user,
'created_at' => current_time('mysql'),
'updated_at' => current_time('mysql')
]
);
- $this->project_id = $wpdb->insert_id;
+ $this->assertNotFalse($inserted, 'Failed to insert test project fixture');
+ $this->project_id = (int) $wpdb->insert_id;
+ $this->assertGreaterThan(0, $this->project_id);
- $wpdb->insert(
+ $inserted = $wpdb->insert(
$wpdb->prefix . 'pm_boards',
[
'title' => 'Test Task',
'project_id' => $this->project_id,
'type' => 'task',
'status' => 0,
'order' => 0,
'created_by' => $this->admin_user,
'created_at' => current_time('mysql'),
'updated_at' => current_time('mysql')
]
);
- $this->task_id = $wpdb->insert_id;
+ $this->assertNotFalse($inserted, 'Failed to insert test task fixture');
+ $this->task_id = (int) $wpdb->insert_id;
+ $this->assertGreaterThan(0, $this->task_id);
- $wpdb->insert(
+ $inserted = $wpdb->insert(
$wpdb->prefix . 'pm_activities',
[
'actor_id' => $this->admin_user,
'action' => 'created',
'action_type' => 'task',
'resource_id' => $this->task_id,
'resource_type' => 'task',
'project_id' => $this->project_id,
'created_at' => current_time('mysql'),
'updated_at' => current_time('mysql')
]
);
- $this->activity_id = $wpdb->insert_id;
+ $this->assertNotFalse($inserted, 'Failed to insert test activity fixture');
+ $this->activity_id = (int) $wpdb->insert_id;
+ $this->assertGreaterThan(0, $this->activity_id);📝 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.
| $wpdb->insert( | |
| $wpdb->prefix . 'pm_projects', | |
| [ | |
| 'title' => 'Test Project', | |
| 'description' => 'Test project for activities', | |
| 'status' => 0, | |
| 'created_by' => $this->admin_user, | |
| 'created_at' => current_time('mysql'), | |
| 'updated_at' => current_time('mysql') | |
| ] | |
| ); | |
| $this->project_id = $wpdb->insert_id; | |
| // Create a test task | |
| $wpdb->insert( | |
| $wpdb->prefix . 'pm_boards', | |
| [ | |
| 'title' => 'Test Task', | |
| 'project_id' => $this->project_id, | |
| 'type' => 'task', | |
| 'status' => 0, | |
| 'order' => 0, | |
| 'created_by' => $this->admin_user, | |
| 'created_at' => current_time('mysql'), | |
| 'updated_at' => current_time('mysql') | |
| ] | |
| ); | |
| $this->task_id = $wpdb->insert_id; | |
| // Create a test activity | |
| $wpdb->insert( | |
| $wpdb->prefix . 'pm_activities', | |
| [ | |
| 'actor_id' => $this->admin_user, | |
| 'action' => 'created', | |
| 'action_type' => 'task', | |
| 'resource_id' => $this->task_id, | |
| 'resource_type' => 'task', | |
| 'project_id' => $this->project_id, | |
| 'created_at' => current_time('mysql'), | |
| 'updated_at' => current_time('mysql') | |
| ] | |
| ); | |
| $this->activity_id = $wpdb->insert_id; | |
| $inserted = $wpdb->insert( | |
| $wpdb->prefix . 'pm_projects', | |
| [ | |
| 'title' => 'Test Project', | |
| 'description' => 'Test project for activities', | |
| 'status' => 0, | |
| 'created_by' => $this->admin_user, | |
| 'created_at' => current_time('mysql'), | |
| 'updated_at' => current_time('mysql') | |
| ] | |
| ); | |
| $this->assertNotFalse($inserted, 'Failed to insert test project fixture'); | |
| $this->project_id = (int) $wpdb->insert_id; | |
| $this->assertGreaterThan(0, $this->project_id); | |
| // Create a test task | |
| $inserted = $wpdb->insert( | |
| $wpdb->prefix . 'pm_boards', | |
| [ | |
| 'title' => 'Test Task', | |
| 'project_id' => $this->project_id, | |
| 'type' => 'task', | |
| 'status' => 0, | |
| 'order' => 0, | |
| 'created_by' => $this->admin_user, | |
| 'created_at' => current_time('mysql'), | |
| 'updated_at' => current_time('mysql') | |
| ] | |
| ); | |
| $this->assertNotFalse($inserted, 'Failed to insert test task fixture'); | |
| $this->task_id = (int) $wpdb->insert_id; | |
| $this->assertGreaterThan(0, $this->task_id); | |
| // Create a test activity | |
| $inserted = $wpdb->insert( | |
| $wpdb->prefix . 'pm_activities', | |
| [ | |
| 'actor_id' => $this->admin_user, | |
| 'action' => 'created', | |
| 'action_type' => 'task', | |
| 'resource_id' => $this->task_id, | |
| 'resource_type' => 'task', | |
| 'project_id' => $this->project_id, | |
| 'created_at' => current_time('mysql'), | |
| 'updated_at' => current_time('mysql') | |
| ] | |
| ); | |
| $this->assertNotFalse($inserted, 'Failed to insert test activity fixture'); | |
| $this->activity_id = (int) $wpdb->insert_id; | |
| $this->assertGreaterThan(0, $this->activity_id); |
🤖 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/api/test-activities.php` around lines 15 - 58, In setUp() ensure each
$wpdb->insert() is checked and the test fails immediately if an insert returned
false: after the project insert (which sets $this->project_id), after the
board/task insert (which sets $this->task_id), and after the activity insert
(which sets $this->activity_id) verify the insert result and/or that
$wpdb->insert_id is truthy; if not, call $this->fail() (or throw) with a clear
message indicating which insert failed so fixtures cannot silently produce
invalid IDs for the rest of the test.
| public function test_get_activities_with_pagination() { | ||
| wp_set_current_user($this->admin_user); | ||
|
|
||
| $request = new WP_REST_Request('GET', '/pm/v2/projects/' . $this->project_id . '/activities'); | ||
| $request->set_query_params([ | ||
| 'per_page' => 10, | ||
| 'page' => 1 | ||
| ]); | ||
|
|
||
| $response = $this->server->dispatch($request); | ||
|
|
||
| $this->assertEquals(200, $response->get_status()); | ||
| $data = $response->get_data(); | ||
| $this->assertArrayHasKey('data', $data); | ||
| } |
There was a problem hiding this comment.
Pagination test does not currently validate pagination semantics.
On Line 85-99, the test only checks HTTP 200 and data key. With a single seeded activity, this still passes even if per_page/page is ignored.
Add enough fixtures for multiple pages and assert behavior (e.g., page sizes and differing result sets across pages).
🤖 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/api/test-activities.php` around lines 85 - 99,
test_get_activities_with_pagination currently only asserts 200 and presence of
'data' but doesn't validate pagination; seed multiple activities for
$this->project_id (e.g., create 3 activities via your project activity
factory/helper used elsewhere), then issue WP_REST_Request GETs to
'/pm/v2/projects/' . $this->project_id . '/activities' with different query
params (per_page=1&page=1 and per_page=1&page=2) using $this->server->dispatch,
assert each response status is 200, assert each response data array length
equals the requested per_page (1), assert the returned items differ between
page=1 and page=2, and assert pagination headers like X-WP-Total (from
$response->get_headers()) equals the total seeded count to confirm correct
pagination semantics.
| public function test_get_activities_by_resource_type() { | ||
| wp_set_current_user($this->admin_user); | ||
|
|
||
| $request = new WP_REST_Request('GET', '/pm/v2/projects/' . $this->project_id . '/activities'); | ||
| $request->set_query_params([ | ||
| 'resource_type' => 'task' | ||
| ]); | ||
|
|
||
| $response = $this->server->dispatch($request); | ||
|
|
||
| $this->assertEquals(200, $response->get_status()); | ||
| $data = $response->get_data(); | ||
| $this->assertIsArray($data['data']); | ||
| } | ||
|
|
||
| /** | ||
| * Test: Get activities by resource ID | ||
| */ | ||
| public function test_get_activities_by_resource_id() { | ||
| wp_set_current_user($this->admin_user); | ||
|
|
||
| $request = new WP_REST_Request('GET', '/pm/v2/projects/' . $this->project_id . '/activities'); | ||
| $request->set_query_params([ | ||
| 'resource_id' => $this->task_id, | ||
| 'resource_type' => 'task' | ||
| ]); | ||
|
|
||
| $response = $this->server->dispatch($request); | ||
|
|
||
| $this->assertEquals(200, $response->get_status()); | ||
| $data = $response->get_data(); | ||
| $this->assertNotEmpty($data['data']); | ||
| } |
There was a problem hiding this comment.
Filter tests can pass even when filter logic is broken.
On Line 104-136, the tests assert array/non-empty only. Since setup seeds one matching activity, both tests pass even if resource_type / resource_id filters are ignored.
Seed at least one non-matching activity and assert every returned row matches the requested filter(s) (and that non-matching rows are excluded).
🤖 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/api/test-activities.php` around lines 104 - 136, The current tests
test_get_activities_by_resource_type and test_get_activities_by_resource_id only
assert that the response is an array/non-empty so they pass even if filters are
ignored; update each test to seed at least one activity that does NOT match the
requested filter (e.g., create an activity with a different resource_type or
resource_id using the same setup helpers), then after dispatching the request
iterate over $data['data'] and assert every returned item's resource_type (in
test_get_activities_by_resource_type) or resource_id and resource_type (in
test_get_activities_by_resource_id) equals the requested value and that the
non-matching seeded activity is not present; use the existing test helpers/ids
($this->project_id, $this->task_id, $this->server, WP_REST_Request) to locate
where to add the extra seed and the per-item assertions.
| $this->assertNotEquals(201, $response->get_status()); | ||
| } |
There was a problem hiding this comment.
Tighten validation assertion to 4xx, not just “not 201”.
At Line 169, assertNotEquals(201, ...) can still pass on server errors (5xx), masking regressions in validation handling.
Suggested fix
- $this->assertNotEquals(201, $response->get_status());
+ $status = $response->get_status();
+ $this->assertGreaterThanOrEqual(400, $status);
+ $this->assertLessThan(500, $status);📝 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.
| $this->assertNotEquals(201, $response->get_status()); | |
| } | |
| $status = $response->get_status(); | |
| $this->assertGreaterThanOrEqual(400, $status); | |
| $this->assertLessThan(500, $status); | |
| } |
🤖 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/api/test-comments.php` around lines 169 - 170, The current assertion
uses $this->assertNotEquals(201, $response->get_status()), which can still pass
on 5xx server errors; replace it with a strict 4xx check by asserting the status
is between 400 and 499 (e.g. use $status = $response->get_status(); then
$this->assertGreaterThanOrEqual(400, $status) and $this->assertLessThan(500,
$status) or a single $this->assertTrue($status >= 400 && $status < 500)); update
the assertion referencing $response->get_status() and remove the assertNotEquals
call.
| $this->assertNotEquals(200, $response->get_status()); | ||
| } |
There was a problem hiding this comment.
Permission test should assert unauthorized/forbidden, not just “not 200”.
At Line 187, assertNotEquals(200, ...) is too broad and can pass on unrelated failures (e.g., 500). This weakens the permission contract check.
Suggested fix
- $this->assertNotEquals(200, $response->get_status());
+ $status = $response->get_status();
+ $this->assertContains($status, [401, 403]);🤖 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/api/test-comments.php` around lines 187 - 188, Replace the weak
assertion at the end of the test that uses assertNotEquals(200,
$response->get_status()) with a strict check for an authorization failure:
assert that $response->get_status() is either 401 (unauthorized) or 403
(forbidden). Locate the assertion using the $response->get_status() call in the
test and change it to explicitly verify the expected permission status (e.g.,
assertTrue(in_array($response->get_status(), [401, 403])) or two separate
assertEquals checks), so the test fails on server errors like 500 but passes
only for proper auth failures.
| <?php | ||
|
|
||
| class PM_Task_Lists_API_Test extends PM_API_Test_Case { |
There was a problem hiding this comment.
Add the GPL-2.0+ file header for compliance.
This new PHP file does not include a GPL-2.0+ license header, which creates a compliance gap for distributed source files.
Suggested header
<?php
+/**
+ * REST API tests for task-lists endpoint.
+ *
+ * `@package` WP_Project_Manager
+ * `@license` GPL-2.0-or-later
+ */As per coding guidelines, **/*.php: PHP must be >= 7.2 and follow GPL-2.0+ license.
📝 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.
| <?php | |
| class PM_Task_Lists_API_Test extends PM_API_Test_Case { | |
| <?php | |
| /** | |
| * REST API tests for task-lists endpoint. | |
| * | |
| * `@package` WP_Project_Manager | |
| * `@license` GPL-2.0-or-later | |
| */ | |
| class PM_Task_Lists_API_Test extends PM_API_Test_Case { |
🤖 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/api/test-task-lists.php` around lines 1 - 3, Add the required GPL-2.0+
file header to this new PHP test file to meet licensing rules: insert the
standard GPL-2.0+ comment block at the top of tests/api/test-task-lists.php
before the opening <?php so the file explicitly declares the license; ensure the
header appears above the declaration of class PM_Task_Lists_API_Test (which
extends PM_API_Test_Case) and follows the project's canonical GPL-2.0+ header
text and formatting.
| public function test_create_task_list_without_permission() { | ||
| wp_set_current_user($this->subscriber_user); | ||
|
|
||
| $request = new WP_REST_Request('POST', '/pm/v2/projects/' . $this->project_id . '/task-lists'); | ||
| $request->set_body_params([ | ||
| 'title' => 'Unauthorized Task List' | ||
| ]); | ||
|
|
||
| $response = $this->server->dispatch($request); | ||
|
|
||
| $this->assertNotEquals(201, $response->get_status()); | ||
| } |
There was a problem hiding this comment.
Tighten unauthorized-create assertion to expected auth failure code.
assertNotEquals(201, ...) is too broad; it will pass even on unexpected server errors. Assert the exact permission failure (typically 401 or 403) to prevent false positives.
Suggested assertion change
- $this->assertNotEquals(201, $response->get_status());
+ $this->assertContains($response->get_status(), [401, 403]);📝 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.
| public function test_create_task_list_without_permission() { | |
| wp_set_current_user($this->subscriber_user); | |
| $request = new WP_REST_Request('POST', '/pm/v2/projects/' . $this->project_id . '/task-lists'); | |
| $request->set_body_params([ | |
| 'title' => 'Unauthorized Task List' | |
| ]); | |
| $response = $this->server->dispatch($request); | |
| $this->assertNotEquals(201, $response->get_status()); | |
| } | |
| public function test_create_task_list_without_permission() { | |
| wp_set_current_user($this->subscriber_user); | |
| $request = new WP_REST_Request('POST', '/pm/v2/projects/' . $this->project_id . '/task-lists'); | |
| $request->set_body_params([ | |
| 'title' => 'Unauthorized Task List' | |
| ]); | |
| $response = $this->server->dispatch($request); | |
| $this->assertContains($response->get_status(), [401, 403]); | |
| } |
🤖 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/api/test-task-lists.php` around lines 189 - 200, The test
test_create_task_list_without_permission currently uses assertNotEquals(201,
$response->get_status()) which is too broad; change it to assert the exact
permission failure status (e.g. assertEquals(403, $response->get_status()))
after dispatching the WP_REST_Request in order to ensure the unauthorized create
attempt via server->dispatch returns the expected auth error code rather than
any other error.
| <?php | ||
|
|
||
| class PM_Tasks_API_Test extends PM_API_Test_Case { |
There was a problem hiding this comment.
Add GPL-2.0+ license header.
Per coding guidelines, PHP files must include a GPL-2.0+ license header.
📄 Proposed license header
<?php
+/**
+ * PM Tasks API Test
+ *
+ * `@package` WP_Project_Manager
+ * `@license` GPL-2.0+
+ */
class PM_Tasks_API_Test extends PM_API_Test_Case {As per coding guidelines: "PHP must be >= 7.2 and follow GPL-2.0+ license"
📝 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.
| <?php | |
| class PM_Tasks_API_Test extends PM_API_Test_Case { | |
| <?php | |
| /** | |
| * PM Tasks API Test | |
| * | |
| * `@package` WP_Project_Manager | |
| * `@license` GPL-2.0+ | |
| */ | |
| class PM_Tasks_API_Test extends PM_API_Test_Case { |
🤖 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/api/test-tasks.php` around lines 1 - 3, Add the required GPL-2.0+
license header at the top of the PHP file containing the PM_Tasks_API_Test class
(tests/api/test-tasks.php) so the file begins with the standard multi-line
GPL-2.0+ comment block before the opening <?php tag; ensure the header
references GPL-2.0+ and includes author/project/year info per project convention
and keep the class declaration (class PM_Tasks_API_Test extends
PM_API_Test_Case) and existing code unchanged below it.
| $wpdb->insert( | ||
| $wpdb->prefix . 'pm_projects', | ||
| [ | ||
| 'title' => 'Test Project', | ||
| 'description' => 'Test project for user management', | ||
| 'status' => 0, | ||
| 'created_by' => $this->admin_user, | ||
| 'created_at' => current_time('mysql'), | ||
| 'updated_at' => current_time('mysql') | ||
| ] | ||
| ); | ||
| $this->project_id = $wpdb->insert_id; |
There was a problem hiding this comment.
Fail fast when fixture inserts fail in setUp().
If either insert fails, later assertions can fail for the wrong reason. Assert DB seeding succeeds right where it happens.
Suggested patch
- $wpdb->insert(
+ $project_inserted = $wpdb->insert(
$wpdb->prefix . 'pm_projects',
[
'title' => 'Test Project',
'description' => 'Test project for user management',
'status' => 0,
'created_by' => $this->admin_user,
'created_at' => current_time('mysql'),
'updated_at' => current_time('mysql')
]
);
+ $this->assertNotFalse($project_inserted, 'Failed to seed pm_projects');
$this->project_id = $wpdb->insert_id;
+ $this->assertGreaterThan(0, $this->project_id, 'Invalid seeded project_id');
- $wpdb->insert(
+ $assignee_inserted = $wpdb->insert(
$wpdb->prefix . 'pm_assignees',
[
'assigned_to' => $this->editor_user,
'assignable_id' => $this->project_id,
'assignable_type' => 'project',
'project_id' => $this->project_id,
'created_at' => current_time('mysql'),
'updated_at' => current_time('mysql')
]
);
+ $this->assertNotFalse($assignee_inserted, 'Failed to seed pm_assignees');Also applies to: 27-37
🤖 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/api/test-users.php` around lines 13 - 24, In setUp(), immediately
verify that each $wpdb->insert call succeeds instead of letting later tests fail
ambiguously: wrap the $wpdb->insert for the project (and the similar insert
around lines 27-37) with assertions such as
$this->assertNotFalse($wpdb->insert(...), 'Failed to insert project/user
fixture') and then assert $wpdb->insert_id is non-empty before assigning
$this->project_id (and equivalent $this->user_id), so failed inserts fail fast
and clearly; update the code around the $wpdb->insert calls and assignments to
$this->project_id/$this->user_id accordingly.
| public function test_non_admin_cannot_add_users() { | ||
| wp_set_current_user($this->subscriber_user); | ||
|
|
||
| $request = new WP_REST_Request('POST', '/pm/v2/projects/' . $this->project_id . '/users'); | ||
| $request->set_body_params([ | ||
| 'users' => [$this->editor_user] | ||
| ]); | ||
|
|
||
| $response = $this->server->dispatch($request); | ||
|
|
||
| $this->assertNotEquals(201, $response->get_status()); |
There was a problem hiding this comment.
Permission test is too weak with assertNotEquals(201, ...).
This can pass on unrelated failures (e.g., 500). Assert an auth failure status explicitly.
Suggested patch
- $this->assertNotEquals(201, $response->get_status());
+ $this->assertContains(
+ $response->get_status(),
+ [401, 403],
+ 'Non-admin user should receive an authorization error'
+ );🤖 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/api/test-users.php` around lines 169 - 179, The test method
test_non_admin_cannot_add_users currently uses assertNotEquals(201,
$response->get_status()) which is too weak; change it to assertEquals(403,
$response->get_status()) (or the explicit auth failure status your API returns)
to assert an authorization failure. Locate the test_non_admin_cannot_add_users
function and replace the assertNotEquals(201, $response->get_status()) call with
an assertion that the response status is the expected auth error (e.g.,
assertEquals(403, $response->get_status())), keeping the existing request setup
using WP_REST_Request and $this->server->dispatch.
c7cec45 to
2972c93
Compare
Summary by CodeRabbit