feat: REST API endpoints for Orders, Taxes, Shipping and Payment Management #2412
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces several new PHP classes and test suites to enhance the REST API functionality within the Dokan plugin. Key additions include controllers for managing country data, orders, order actions, order notes, order refunds, payment gateways, shipping methods, and tax classes. Each controller implements specific permission checks to ensure that only authorized users can access or modify resources. Additionally, comprehensive test classes are added to validate the functionality of these controllers, ensuring robust error handling and compliance with expected behaviors. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
mralaminahamed
added
Needs: Testing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (52)
tests/php/src/REST/TaxClassControllerTest.php (3)
19-25: 🛠️ Refactor suggestion
Enhance route registration test coverage.
The current test only verifies the GET method. Consider expanding the test to:
- Verify all supported HTTP methods
- Assert route parameters and arguments
- Validate permission callback registration
Here's a suggested enhancement:
public function test_ensure_route_is_registered() { $routes = $this->server->get_routes( $this->namespace ); $full_route = $this->get_route( 'taxes/classes' ); $this->assertArrayHasKey( $full_route, $routes ); - $this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] ); + $route = $routes[$full_route][0]; + + // Verify supported methods + $this->assertArrayHasKey( 'GET', $route['methods'] ); + + // Verify permission callback exists + $this->assertArrayHasKey( 'permission_callback', $route ); + $this->assertTrue( is_callable( $route['permission_callback'] ) ); + + // Verify arguments schema + $this->assertArrayHasKey( 'args', $route ); }Committable suggestion was skipped due to low confidence.
30-38: 🛠️ Refactor suggestion
Expand test coverage for tax classes retrieval.
The current test only covers the basic success case. Consider adding:
- Test cases for different user roles (admin, customer, guest)
- Validation of response data structure and content
- Error cases (unauthorized access, invalid requests)
- Edge cases (empty tax classes, maximum limits)
Here's a suggested enhancement:
public function test_get_tax_classes() { wp_set_current_user( $this->seller_id1 ); $response = $this->get_request( 'taxes/classes' ); $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); $this->assertIsArray( $data ); + + // Validate data structure + foreach ($data as $tax_class) { + $this->assertArrayHasKey('name', $tax_class); + $this->assertArrayHasKey('slug', $tax_class); + } } + +/** + * @testWith ["administrator", 200] + * ["shop_manager", 200] + * ["customer", 403] + * [null, 401] + */ +public function test_get_tax_classes_permissions($role, $expected_status) { + if ($role) { + $user_id = $this->factory->user->create(['role' => $role]); + wp_set_current_user($user_id); + } + + $response = $this->get_request('taxes/classes'); + $this->assertEquals($expected_status, $response->get_status()); +} + +public function test_get_tax_classes_empty() { + wp_set_current_user($this->seller_id1); + + // Remove all tax classes first + update_option('woocommerce_tax_classes', ''); + + $response = $this->get_request('taxes/classes'); + $this->assertEquals(200, $response->get_status()); + $this->assertEmpty($response->get_data()); +}📝 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_get_tax_classes() { wp_set_current_user( $this->seller_id1 ); $response = $this->get_request( 'taxes/classes' ); $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); $this->assertIsArray( $data ); // Validate data structure foreach ($data as $tax_class) { $this->assertArrayHasKey('name', $tax_class); $this->assertArrayHasKey('slug', $tax_class); } } /** * @testWith ["administrator", 200] * ["shop_manager", 200] * ["customer", 403] * [null, 401] */ public function test_get_tax_classes_permissions($role, $expected_status) { if ($role) { $user_id = $this->factory->user->create(['role' => $role]); wp_set_current_user($user_id); } $response = $this->get_request('taxes/classes'); $this->assertEquals($expected_status, $response->get_status()); } public function test_get_tax_classes_empty() { wp_set_current_user($this->seller_id1); // Remove all tax classes first update_option('woocommerce_tax_classes', ''); $response = $this->get_request('taxes/classes'); $this->assertEquals(200, $response->get_status()); $this->assertEmpty($response->get_data()); }
8-12: 💡 Codebase verification
Now I have enough context to provide the final response:
The @group annotation
dokan-tax-classes-v3appears to be incorrectThe TaxClassController implementation in
includes/REST/TaxClassController.phpclearly uses thedokan/v1namespace, which matches the test file's namespace. While there are v3 endpoints in the codebase, they are specifically for order-related functionality (OrderController, OrderNoteController, etc.). The tax classes API is intentionally implemented as a v1 endpoint.Recommendation:
- Update the test group annotation from
@group dokan-tax-classes-v3to@group dokan-tax-classes-v1to maintain consistency with the actual implementation.🔗 Analysis chain
Version mismatch between @group annotation and namespace.
The @group annotation suggests v3 (
dokan-tax-classes-v3), but the namespace is set to v1. This inconsistency could lead to confusion. Please verify the correct version and update either the annotation or the namespace accordingly.
Let me gather more context about the tax-related endpoints and their versioning by checking the actual controller implementation.
Let me check the actual implementation of the TaxClassController to understand the versioning strategy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for version consistency across tax-related files # Expected: All tax-related endpoints should use the same version # Check for tax-related routes and their versions rg -A 5 "namespace.*dokan/v[0-9].*tax" --type php rg -A 5 "@group.*tax.*v[0-9]" --type phpLength of output: 499
Script:
#!/bin/bash # Check for the actual TaxClass controller implementation and its versioning rg -l "class.*TaxClass.*Controller" --type php # Look for tax-related REST route registrations ast-grep --pattern 'register_rest_route($_, "dokan/v$_/tax"'Length of output: 194
Script:
#!/bin/bash # Check the content of TaxClassController.php for namespace and route registration cat includes/REST/TaxClassController.php # Also check if there are any other v3 endpoints in the codebase for context rg "dokan/v3" --type phpLength of output: 2437
tests/php/src/REST/TaxControllerTest.php (2)
19-25:
⚠️ Potential issueExpand route registration test coverage.
The current test only verifies GET method registration. For a tax management API, we should also:
- Verify other expected HTTP methods (POST/PUT/DELETE)
- Validate route parameters and schema
- Verify that unauthorized HTTP methods are not registered
Here's a suggested enhancement:
public function test_ensure_route_is_registered() { $routes = $this->server->get_routes( $this->namespace ); $full_route = $this->get_route( 'taxes' ); $this->assertArrayHasKey( $full_route, $routes ); - $this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] ); + $route_data = $routes[$full_route][0]; + + // Verify allowed methods + $this->assertTrue( $route_data['methods']['GET'] ); + $this->assertTrue( $route_data['methods']['POST'] ); + $this->assertTrue( $route_data['methods']['PUT'] ); + $this->assertTrue( $route_data['methods']['DELETE'] ); + + // Verify schema + $this->assertArrayHasKey( 'schema', $route_data ); + $this->assertEquals( 'dokan_tax_rate', $route_data['schema']['title'] ); + + // Verify args for GET + $this->assertArrayHasKey( 'args', $route_data ); + $this->assertArrayHasKey( 'context', $route_data['args'] ); }Committable suggestion was skipped due to low confidence.
30-39:
⚠️ Potential issueEnhance tax retrieval test coverage.
The current test only performs basic validation. Consider adding:
- Test data setup with known tax rates
- Specific assertions about tax data structure
- Error cases (unauthorized users, invalid requests)
- Vendor-specific tax data validation
Here's a suggested enhancement:
public function test_get_taxes() { + // Setup test data + $tax_rate = array( + 'country' => 'US', + 'state' => 'AL', + 'rate' => '7.5', + 'name' => 'State Tax', + 'priority' => 1, + 'compound' => false, + ); + $tax_rate_id = \WC_Tax::_insert_tax_rate($tax_rate); + wp_set_current_user( $this->seller_id1 ); $response = $this->get_request( 'taxes' ); $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); $this->assertIsArray( $data ); - // Add more assertions based on expected tax data + + // Verify tax data structure + $this->assertArrayHasKey( 'id', $data[0] ); + $this->assertArrayHasKey( 'country', $data[0] ); + $this->assertArrayHasKey( 'state', $data[0] ); + $this->assertArrayHasKey( 'rate', $data[0] ); + $this->assertArrayHasKey( 'name', $data[0] ); + + // Verify tax data values + $this->assertEquals( $tax_rate['country'], $data[0]['country'] ); + $this->assertEquals( $tax_rate['rate'], $data[0]['rate'] ); + + // Clean up + \WC_Tax::_delete_tax_rate($tax_rate_id); } + +/** + * Test tax retrieval with unauthorized user + */ +public function test_get_taxes_unauthorized() { + wp_set_current_user( 0 ); + $response = $this->get_request( 'taxes' ); + $this->assertEquals( 401, $response->get_status() ); +} + +/** + * Test vendor-specific tax data + */ +public function test_get_vendor_specific_taxes() { + wp_set_current_user( $this->seller_id1 ); + + // Setup vendor-specific tax settings + update_user_meta( $this->seller_id1, 'dokan_tax_rate', '8.5' ); + + $response = $this->get_request( 'taxes' ); + $data = $response->get_data(); + + $this->assertEquals( '8.5', $data[0]['rate'] ); +}📝 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_get_taxes() { // Setup test data $tax_rate = array( 'country' => 'US', 'state' => 'AL', 'rate' => '7.5', 'name' => 'State Tax', 'priority' => 1, 'compound' => false, ); $tax_rate_id = \WC_Tax::_insert_tax_rate($tax_rate); wp_set_current_user( $this->seller_id1 ); $response = $this->get_request( 'taxes' ); $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); $this->assertIsArray( $data ); // Verify tax data structure $this->assertArrayHasKey( 'id', $data[0] ); $this->assertArrayHasKey( 'country', $data[0] ); $this->assertArrayHasKey( 'state', $data[0] ); $this->assertArrayHasKey( 'rate', $data[0] ); $this->assertArrayHasKey( 'name', $data[0] ); // Verify tax data values $this->assertEquals( $tax_rate['country'], $data[0]['country'] ); $this->assertEquals( $tax_rate['rate'], $data[0]['rate'] ); // Clean up \WC_Tax::_delete_tax_rate($tax_rate_id); } /** * Test tax retrieval with unauthorized user */ public function test_get_taxes_unauthorized() { wp_set_current_user( 0 ); $response = $this->get_request( 'taxes' ); $this->assertEquals( 401, $response->get_status() ); } /** * Test vendor-specific tax data */ public function test_get_vendor_specific_taxes() { wp_set_current_user( $this->seller_id1 ); // Setup vendor-specific tax settings update_user_meta( $this->seller_id1, 'dokan_tax_rate', '8.5' ); $response = $this->get_request( 'taxes' ); $data = $response->get_data(); $this->assertEquals( '8.5', $data[0]['rate'] ); }tests/php/src/REST/PaymentGatewaysControllerTest.php (3)
14-25: 🛠️ Refactor suggestion
Enhance route registration test coverage.
While the basic route registration is tested, consider adding:
- Validation of route parameters and schema
- Tests for other HTTP methods if supported (POST, PUT, DELETE)
- Verification of permission callbacks
public function test_ensure_route_is_registered() { $routes = $this->server->get_routes($this->namespace); $full_route = $this->get_route('payment_gateways'); $route_data = $routes[$full_route][0]; $this->assertArrayHasKey($full_route, $routes); $this->assertNestedContains(['methods' => ['GET' => true]], $routes[$full_route]); // Verify schema $this->assertArrayHasKey('schema', $route_data); // Verify permissions $this->assertArrayHasKey('permission_callback', $route_data); }
27-39: 🛠️ Refactor suggestion
Enhance payment gateways list test coverage.
The current test only covers the happy path. Consider adding:
- Response structure validation
- Error cases (unauthorized access, no gateways configured)
- Pagination testing if supported
public function test_get_payment_gateways() { wp_set_current_user($this->seller_id1); $response = $this->get_request('payment_gateways'); $this->assertEquals(200, $response->get_status()); $data = $response->get_data(); // Validate structure $this->assertArrayHasKey('id', $data[0]); $this->assertArrayHasKey('title', $data[0]); $this->assertArrayHasKey('enabled', $data[0]); // Test unauthorized access wp_set_current_user(0); $response = $this->get_request('payment_gateways'); $this->assertEquals(401, $response->get_status()); }
41-54: 🛠️ Refactor suggestion
Improve single gateway test robustness.
The test uses hardcoded 'bacs' gateway ID and lacks error cases. Consider:
- Using test data constants or setup fixtures
- Testing invalid gateway IDs
- Validating complete response structure
private const TEST_GATEWAY = 'bacs'; public function test_get_payment_gateway() { wp_set_current_user($this->seller_id1); // Test valid gateway $response = $this->get_request('payment_gateways/' . self::TEST_GATEWAY); $this->assertEquals(200, $response->get_status()); $data = $response->get_data(); // Validate complete structure $this->assertEquals(self::TEST_GATEWAY, $data['id']); $this->assertArrayHasKey('title', $data); $this->assertArrayHasKey('description', $data); $this->assertArrayHasKey('enabled', $data); $this->assertArrayHasKey('settings', $data); // Test invalid gateway $response = $this->get_request('payment_gateways/invalid_gateway'); $this->assertEquals(404, $response->get_status()); }tests/php/src/REST/DataCountriesControllerTest.php (3)
45-54: 🛠️ Refactor suggestion
Expand country states test coverage.
The test should include:
- Verification of specific state data structure
- Invalid country code handling
- Error scenarios
public function test_get_country_states() { wp_set_current_user( $this->seller_id1 ); $response = $this->get_request( 'data/countries/US' ); $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); $this->assertArrayHasKey( 'states', $data ); $this->assertNotEmpty( $data['states'] ); + + // Verify specific states exist with correct format + $states = $data['states']; + $this->assertArrayHasKey('CA', $states); + $this->assertEquals('California', $states['CA']); + $this->assertArrayHasKey('NY', $states); + $this->assertEquals('New York', $states['NY']); } + +/** + * Test invalid country code for states endpoint. + */ +public function test_get_country_states_invalid_country() { + wp_set_current_user( $this->seller_id1 ); + + $response = $this->get_request( 'data/countries/INVALID' ); + $this->assertEquals( 404, $response->get_status() ); +}Committable suggestion was skipped due to low confidence.
30-40: 🛠️ Refactor suggestion
Enhance countries endpoint test coverage.
The test verifies basic functionality but should be expanded to include:
- Specific country data structure validation
- Unauthorized access scenarios
- Pagination if supported
public function test_get_countries() { wp_set_current_user( $this->seller_id1 ); $response = $this->get_request( 'data/countries' ); $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); $this->assertIsArray( $data ); $this->assertNotEmpty( $data ); - // Add more assertions based on expected country data + + // Verify country data structure + $first_country = reset($data); + $this->assertArrayHasKey('code', $first_country); + $this->assertArrayHasKey('name', $first_country); + + // Known country verification + $us_exists = false; + foreach ($data as $country) { + if ($country['code'] === 'US') { + $us_exists = true; + $this->assertEquals('United States', $country['name']); + break; + } + } + $this->assertTrue($us_exists, 'US should exist in countries list'); } +/** + * Test unauthorized access to countries endpoint. + */ +public function test_get_countries_unauthorized() { + wp_set_current_user( 0 ); + + $response = $this->get_request( 'data/countries' ); + $this->assertEquals( 401, $response->get_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.public function test_get_countries() { wp_set_current_user( $this->seller_id1 ); $response = $this->get_request( 'data/countries' ); $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); $this->assertIsArray( $data ); $this->assertNotEmpty( $data ); // Verify country data structure $first_country = reset($data); $this->assertArrayHasKey('code', $first_country); $this->assertArrayHasKey('name', $first_country); // Known country verification $us_exists = false; foreach ($data as $country) { if ($country['code'] === 'US') { $us_exists = true; $this->assertEquals('United States', $country['name']); break; } } $this->assertTrue($us_exists, 'US should exist in countries list'); } /** * Test unauthorized access to countries endpoint. */ public function test_get_countries_unauthorized() { wp_set_current_user( 0 ); $response = $this->get_request( 'data/countries' ); $this->assertEquals( 401, $response->get_status() ); }
19-25: 🛠️ Refactor suggestion
Consider expanding route registration test coverage.
The current test verifies basic route registration and GET method support. Consider adding:
- Negative tests for invalid routes
- Verification of route arguments and callbacks
- Tests for other HTTP methods to ensure they're not allowed
public function test_ensure_route_is_registered() { $routes = $this->server->get_routes( $this->namespace ); $full_route = $this->get_route( 'data/countries' ); $this->assertArrayHasKey( $full_route, $routes ); $this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] ); + + // Verify route arguments + $this->assertArrayHasKey( 'args', $routes[$full_route][0] ); + + // Verify other methods are not allowed + $this->assertArrayNotHasKey( 'POST', $routes[$full_route][0]['methods'] ); + $this->assertArrayNotHasKey( 'PUT', $routes[$full_route][0]['methods'] ); + $this->assertArrayNotHasKey( 'DELETE', $routes[$full_route][0]['methods'] ); + + // Test invalid route + $invalid_route = $this->get_route( 'data/invalid' ); + $this->assertArrayNotHasKey( $invalid_route, $routes ); }📝 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_ensure_route_is_registered() { $routes = $this->server->get_routes( $this->namespace ); $full_route = $this->get_route( 'data/countries' ); $this->assertArrayHasKey( $full_route, $routes ); $this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] ); // Verify route arguments $this->assertArrayHasKey( 'args', $routes[$full_route][0] ); // Verify other methods are not allowed $this->assertArrayNotHasKey( 'POST', $routes[$full_route][0]['methods'] ); $this->assertArrayNotHasKey( 'PUT', $routes[$full_route][0]['methods'] ); $this->assertArrayNotHasKey( 'DELETE', $routes[$full_route][0]['methods'] ); // Test invalid route $invalid_route = $this->get_route( 'data/invalid' ); $this->assertArrayNotHasKey( $invalid_route, $routes ); }tests/php/src/REST/ShippingMethodControllerTest.php (2)
32-42:
⚠️ Potential issueExpand test coverage for shipping methods retrieval
The current test only covers the happy path. Consider adding:
- Tests for different user roles (guest, customer, admin)
- Validation of response structure and required fields
- Error cases (e.g., invalid authentication)
public function test_get_shipping_methods() { - wp_set_current_user( $this->seller_id1 ); + $test_cases = [ + 'guest' => [ + 'user_id' => 0, + 'expected_status' => 401 + ], + 'customer' => [ + 'user_id' => $this->customer, + 'expected_status' => 403 + ], + 'seller' => [ + 'user_id' => $this->seller_id1, + 'expected_status' => 200 + ] + ]; - $response = $this->get_request( 'shipping_methods' ); + foreach ($test_cases as $case => $config) { + wp_set_current_user($config['user_id']); + $response = $this->get_request('shipping_methods'); + $this->assertEquals( + $config['expected_status'], + $response->get_status(), + "Failed asserting response status for $case user" + ); - $this->assertEquals( 200, $response->get_status() ); - $data = $response->get_data(); - $this->assertIsArray( $data ); - $this->assertNotEmpty( $data ); - // Add more assertions based on expected shipping method data + if ($config['expected_status'] === 200) { + $data = $response->get_data(); + $this->assertIsArray($data); + $this->assertNotEmpty($data); + + // Verify required fields for each shipping method + foreach ($data as $method) { + $this->assertArrayHasKey('id', $method); + $this->assertArrayHasKey('title', $method); + $this->assertArrayHasKey('enabled', $method); + $this->assertIsBool($method['enabled']); + } + } + } }Committable suggestion was skipped due to low confidence.
47-56:
⚠️ Potential issueImprove single shipping method test robustness
Current implementation has several limitations:
- Relies on 'flat_rate' existing without proper setup
- Lacks error case testing
- Minimal response validation
public function test_get_shipping_method() { - wp_set_current_user( $this->seller_id1 ); + // Setup + $expected_methods = [ + 'flat_rate' => [ + 'id' => 'flat_rate', + 'title' => 'Flat Rate', + 'enabled' => true + ] + ]; + add_filter('woocommerce_shipping_methods', function($methods) use ($expected_methods) { + return array_merge($methods, $expected_methods); + }); - // Assuming 'flat_rate' is a valid shipping method ID - $response = $this->get_request( 'shipping_methods/flat_rate' ); + $test_cases = [ + 'invalid_id' => [ + 'method_id' => 'non_existent', + 'expected_status' => 404 + ], + 'valid_id' => [ + 'method_id' => 'flat_rate', + 'expected_status' => 200 + ] + ]; - $this->assertEquals( 200, $response->get_status() ); - $data = $response->get_data(); - $this->assertEquals( 'flat_rate', $data['id'] ); + wp_set_current_user($this->seller_id1); + + foreach ($test_cases as $case => $config) { + $response = $this->get_request("shipping_methods/{$config['method_id']}"); + + $this->assertEquals( + $config['expected_status'], + $response->get_status(), + "Failed asserting response status for $case" + ); + + if ($config['expected_status'] === 200) { + $data = $response->get_data(); + $expected = $expected_methods[$config['method_id']]; + + $this->assertEquals($expected['id'], $data['id']); + $this->assertEquals($expected['title'], $data['title']); + $this->assertEquals($expected['enabled'], $data['enabled']); + } + } }📝 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_get_shipping_method() { // Setup $expected_methods = [ 'flat_rate' => [ 'id' => 'flat_rate', 'title' => 'Flat Rate', 'enabled' => true ] ]; add_filter('woocommerce_shipping_methods', function($methods) use ($expected_methods) { return array_merge($methods, $expected_methods); }); $test_cases = [ 'invalid_id' => [ 'method_id' => 'non_existent', 'expected_status' => 404 ], 'valid_id' => [ 'method_id' => 'flat_rate', 'expected_status' => 200 ] ]; wp_set_current_user($this->seller_id1); foreach ($test_cases as $case => $config) { $response = $this->get_request("shipping_methods/{$config['method_id']}"); $this->assertEquals( $config['expected_status'], $response->get_status(), "Failed asserting response status for $case" ); if ($config['expected_status'] === 200) { $data = $response->get_data(); $expected = $expected_methods[$config['method_id']]; $this->assertEquals($expected['id'], $data['id']); $this->assertEquals($expected['title'], $data['title']); $this->assertEquals($expected['enabled'], $data['enabled']); } } }includes/REST/TaxController.php (3)
11-18:
⚠️ Potential issueReplace "DOKAN_SINCE" placeholder with actual version number.
The
@sincetag contains a placeholder that needs to be replaced with the actual version number where this class is introduced.- * @since DOKAN_SINCE + * @since 3.9.0📝 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./** * Dokan REST API Taxes controller class. * * @since 3.9.0 * * @package WeDevs\Dokan\REST */ class TaxController extends WC_REST_Taxes_Controller {
53-60: 🛠️ Refactor suggestion
Enhance permission checks with specific capabilities.
The current vendor permission check only verifies if the user is a seller. For tax management, consider adding specific capability checks to ensure the vendor has the required permissions to manage tax settings.
protected function check_vendor_permission(): bool { - return dokan_is_user_seller( dokan_get_current_user_id() ); + $user_id = dokan_get_current_user_id(); + return dokan_is_user_seller( $user_id ) && + user_can( $user_id, 'dokan_manage_tax' ); }This ensures that vendors not only need to be sellers but also have the specific capability to manage taxes.
📝 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./** * Check vendor permission. * * @return bool */ protected function check_vendor_permission(): bool { $user_id = dokan_get_current_user_id(); return dokan_is_user_seller( $user_id ) && user_can( $user_id, 'dokan_manage_tax' ); }
27-51: 💡 Codebase verification
⚠️ Potential issueBased on the search results, I can now provide a final response as we have sufficient information about permission check implementations across various controllers in the codebase.
Permission checks for write operations are required for consistency with other controllers
Looking at other controllers in the codebase, particularly
OrderNoteControllerV3,OrderRefundControllerV3, andOrderControllerV3, they implement full CRUD permission checks. TheTaxControllershould follow the same pattern for consistency. Specifically:
create_item_permissions_check: Implemented in OrderNoteControllerV3, OrderRefundControllerV3, OrderControllerV3update_item_permissions_check: Implemented in OrderControllerV3delete_item_permissions_check: Implemented in OrderRefundControllerV3, OrderControllerV3The implementation should follow the same pattern as other controllers:
public function create_item_permissions_check($request) { if (!$this->check_vendor_permission()) { return new WP_Error('dokan_rest_cannot_create', __('Sorry, you are not allowed to create resources.', 'dokan-lite'), array('status' => rest_authorization_required_code())); } return true; }🔗 Analysis chain
Implement permission checks for write operations.
The current implementation only includes permission checks for reading tax resources (
get_itemsandget_item). According to the PR objectives, CRUD operations should be supported. Please implement permission checks for create, update, and delete operations.Consider adding these methods:
create_item_permissions_checkupdate_item_permissions_checkdelete_item_permissions_checkLet's verify if other controllers implement these permission checks:
Let me try a different approach to search for permission check methods in the controllers.
This should help us understand:
- What controller files exist in the codebase
- What permission check methods are implemented in these controllers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for create/update/delete permission checks in other controllers ast-grep --pattern 'function $_(create|update|delete)_item_permissions_check'Length of output: 79
Script:
#!/bin/bash # Search for controller files fd "Controller\.php$" --type f # Then search for permission check methods in these files rg "function.*_permissions_check" --type php -A 5Length of output: 31481
includes/REST/ShippingMethodController.php (3)
11-18:
⚠️ Potential issueReplace DOKAN_SINCE placeholder with actual version.
The
@since DOKAN_SINCEtag needs to be replaced with the actual version number where this class was introduced.
58-60: 🛠️ Refactor suggestion
Consider caching the vendor permission check result.
Since the vendor status is unlikely to change during a single request, consider caching the result to avoid multiple database queries:
+ /** + * Cached result of vendor permission check. + * + * @var bool|null + */ + private ?bool $is_vendor = null; protected function check_vendor_permission(): bool { - return dokan_is_user_seller( dokan_get_current_user_id() ); + if ( $this->is_vendor === null ) { + $this->is_vendor = dokan_is_user_seller( dokan_get_current_user_id() ); + } + return $this->is_vendor; }📝 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./** * Cached result of vendor permission check. * * @var bool|null */ private ?bool $is_vendor = null; protected function check_vendor_permission(): bool { if ( $this->is_vendor === null ) { $this->is_vendor = dokan_is_user_seller( dokan_get_current_user_id() ); } return $this->is_vendor; }
33-51: 🛠️ Refactor suggestion
Consider reducing code duplication in permission checks.
The permission check methods share almost identical code. Consider extracting the common logic into a reusable method:
+ /** + * Handle common permission check logic. + * + * @param string $message Error message for unauthorized access. + * @return WP_Error|boolean + */ + protected function handle_permission_check( string $message ): WP_Error|bool { + if ( ! $this->check_vendor_permission() ) { + return new WP_Error( + 'dokan_rest_cannot_view', + $message, + array( 'status' => rest_authorization_required_code() ) + ); + } + return true; + } public function get_items_permissions_check( $request ) { - if ( ! $this->check_vendor_permission() ) { - return new WP_Error( 'dokan_rest_cannot_view', __( 'Sorry, you cannot list resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) ); - } - return true; + return $this->handle_permission_check( __( 'Sorry, you cannot list resources.', 'dokan-lite' ) ); } public function get_item_permissions_check( $request ) { - if ( ! $this->check_vendor_permission() ) { - return new WP_Error( 'dokan_rest_cannot_view', __( 'Sorry, you cannot view this resource.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) ); - } - return true; + return $this->handle_permission_check( __( 'Sorry, you cannot view this resource.', 'dokan-lite' ) ); }📝 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./** * Handle common permission check logic. * * @param string $message Error message for unauthorized access. * @return WP_Error|boolean */ protected function handle_permission_check( string $message ): WP_Error|bool { if ( ! $this->check_vendor_permission() ) { return new WP_Error( 'dokan_rest_cannot_view', $message, array( 'status' => rest_authorization_required_code() ) ); } return true; } public function get_items_permissions_check( $request ) { return $this->handle_permission_check( __( 'Sorry, you cannot list resources.', 'dokan-lite' ) ); } /** * Check if a given request has access to read a shipping method. * * @param WP_REST_Request $request Full details about the request. * @return WP_Error|boolean */ public function get_item_permissions_check( $request ) { return $this->handle_permission_check( __( 'Sorry, you cannot view this resource.', 'dokan-lite' ) ); }includes/REST/PaymentGatewayController.php (1)
33-38: 🛠️ Refactor suggestion
Consider improving error messages and reducing code duplication.
The permission check methods could be enhanced:
- Error messages could be more specific about the required vendor permissions
- Consider extracting the common logic to reduce duplication
- Add @return tags in docblocks
/** * Check whether a given request has permission to read payment gateways. * * @param WP_REST_Request $request Full details about the request. + * @return WP_Error|boolean WP_Error if user lacks permission, true otherwise. */ public function get_items_permissions_check( $request ) { if ( ! $this->check_vendor_permission() ) { - return new WP_Error( 'dokan_rest_cannot_view', __( 'Sorry, you cannot list resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) ); + return new WP_Error( + 'dokan_rest_cannot_view', + __( 'Sorry, you need vendor payment menu access permissions to view payment gateways.', 'dokan-lite' ), + array( 'status' => rest_authorization_required_code() ) + ); } return true; }Also applies to: 46-51
tests/php/src/REST/OrderActionsControllerV3Test.php (2)
18-71: 🛠️ Refactor suggestion
Consider adding more test scenarios for comprehensive coverage.
While the current tests cover basic functionality, consider adding tests for:
- Unauthorized access (non-seller trying to access actions)
- Non-existent order scenarios
- Different action types using a data provider
- Concurrent action execution edge cases
Example implementation for unauthorized access test:
/** * Test unauthorized access to order actions. */ public function test_get_order_actions_unauthorized() { wp_set_current_user( $this->customer ); // Set as regular customer $order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create(); $response = $this->get_request( "orders/{$order_id}/actions" ); $this->assertEquals( 403, $response->get_status() ); }
28-31: 🛠️ Refactor suggestion
Strengthen assertions for order actions response structure.
The current assertions only check for key existence. Consider adding more specific assertions about the expected data structure and values.
$this->assertIsArray( $data ); -$this->assertArrayHasKey( 'send_order_details', $data ); -$this->assertArrayHasKey( 'send_order_details_admin', $data ); -$this->assertArrayHasKey( 'regenerate_download_permissions', $data ); +$this->assertArrayHasKey( 'send_order_details', $data ); +$this->assertIsString( $data['send_order_details'] ); +$this->assertArrayHasKey( 'send_order_details_admin', $data ); +$this->assertIsString( $data['send_order_details_admin'] ); +$this->assertArrayHasKey( 'regenerate_download_permissions', $data ); +$this->assertIsString( $data['regenerate_download_permissions'] );📝 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->assertIsArray( $data ); $this->assertArrayHasKey( 'send_order_details', $data ); $this->assertIsString( $data['send_order_details'] ); $this->assertArrayHasKey( 'send_order_details_admin', $data ); $this->assertIsString( $data['send_order_details_admin'] ); $this->assertArrayHasKey( 'regenerate_download_permissions', $data ); $this->assertIsString( $data['regenerate_download_permissions'] );tests/php/src/REST/OrderRefundsControllerV3Test.php (2)
19-67: 🛠️ Refactor suggestion
Consider expanding test coverage for comprehensive validation.
While the happy path is well tested, consider adding the following test scenarios:
- Error cases (invalid order ID, insufficient permissions)
- Validation of all refund response properties
- Business rules validation (e.g., refund amount cannot exceed order total)
Here's a suggested approach for additional test methods:
public function test_create_refund_with_invalid_order_id() { wp_set_current_user($this->seller_id1); $response = $this->post_request('orders/999999/refunds', [ 'amount' => 10, 'reason' => 'testing' ]); $this->assertEquals(404, $response->get_status()); } public function test_create_refund_exceeding_order_total() { wp_set_current_user($this->seller_id1); $order_id = $this->factory()->order->create_order_with_fees_and_shipping(); $order = wc_get_order($order_id); $total = $order->get_total(); $response = $this->post_request("orders/{$order_id}/refunds", [ 'amount' => $total + 100, 'reason' => 'testing' ]); $this->assertEquals(400, $response->get_status()); }
54-66: 🛠️ Refactor suggestion
Enhance response validation.
The current assertions only check for the presence of keys and total values. Consider validating all relevant properties of the refund response.
$response = $this->get_request('orders/' . $order->get_id() . '/refunds/' . $refund->get_id()); $data = $response->get_data(); $this->assertEquals(200, $response->get_status()); + // Validate common refund properties + $expected_properties = ['id', 'date_created', 'reason', 'refunded_by']; + foreach ($expected_properties as $property) { + $this->assertArrayHasKey($property, $data); + } + // Validate line items $this->assertContains('line_items', array_keys($data)); $this->assertEquals(-1, $data['line_items'][0]['total']); + $this->assertEquals(1, $data['line_items'][0]['quantity']); + $this->assertArrayHasKey('name', $data['line_items'][0]); // Validate fee lines $this->assertContains('fee_lines', array_keys($data)); $this->assertEquals(-10, $data['fee_lines'][0]['total']); + $this->assertArrayHasKey('name', $data['fee_lines'][0]); // Validate shipping lines $this->assertContains('shipping_lines', array_keys($data)); $this->assertEquals(-20, $data['shipping_lines'][0]['total']); + $this->assertArrayHasKey('method_title', $data['shipping_lines'][0]);📝 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.$response = $this->get_request( 'orders/' . $order->get_id() . '/refunds/' . $refund->get_id() ); $data = $response->get_data(); $this->assertEquals( 200, $response->get_status() ); // Validate common refund properties $expected_properties = ['id', 'date_created', 'reason', 'refunded_by']; foreach ($expected_properties as $property) { $this->assertArrayHasKey($property, $data); } // Validate line items $this->assertContains( 'line_items', array_keys( $data ) ); $this->assertEquals( -1, $data['line_items'][0]['total'] ); $this->assertEquals(1, $data['line_items'][0]['quantity']); $this->assertArrayHasKey('name', $data['line_items'][0]); // Validate fee lines $this->assertContains( 'fee_lines', array_keys( $data ) ); $this->assertEquals( -10, $data['fee_lines'][0]['total'] ); $this->assertArrayHasKey('name', $data['fee_lines'][0]); // Validate shipping lines $this->assertContains( 'shipping_lines', array_keys( $data ) ); $this->assertEquals( -20, $data['shipping_lines'][0]['total'] ); $this->assertArrayHasKey('method_title', $data['shipping_lines'][0]);tests/php/src/REST/OrderNotesControllerTestV3.php (4)
15-36: 🛠️ Refactor suggestion
Add test coverage for edge cases in order note creation.
The current test only covers the happy path. Consider adding test cases for:
- Unauthorized access (non-seller trying to add notes)
- Invalid order ID
- Empty or invalid note content
Add these test methods:
public function test_create_order_note_unauthorized() { wp_set_current_user( $this->customer ); $order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create(); $response = $this->post_request( "orders/{$order_id}/notes", ['note' => 'Test note'] ); $this->assertEquals( 403, $response->get_status() ); } public function test_create_order_note_invalid_order() { wp_set_current_user( $this->seller_id1 ); $response = $this->post_request( "orders/999999/notes", ['note' => 'Test note'] ); $this->assertEquals( 404, $response->get_status() ); } public function test_create_order_note_empty_content() { wp_set_current_user( $this->seller_id1 ); $order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create(); $response = $this->post_request( "orders/{$order_id}/notes", ['note' => ''] ); $this->assertEquals( 400, $response->get_status() ); }
11-14: 🛠️ Refactor suggestion
Consider adding a setUp method to reduce code duplication.
Each test method starts with similar setup code. Consider extracting this to a setUp method:
protected $order_id; protected function setUp(): void { parent::setUp(); wp_set_current_user( $this->seller_id1 ); $this->order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create(); }This would make the test methods more focused and reduce duplication.
41-60:
⚠️ Potential issueMake note order assertions more robust.
The current test assumes a specific order of notes in the response. Since the order might not be guaranteed, consider either:
- Adding an ORDER BY clause in the controller
- Making the assertions order-independent
Here's how to make the assertions more robust:
- $this->assertEquals( 'Note 1', $data[0]['note'] ); - $this->assertFalse( $data[0]['customer_note'] ); - $this->assertEquals( 'Note 2', $data[1]['note'] ); - $this->assertTrue( $data[1]['customer_note'] ); + $notes = array_column($data, 'note'); + $this->assertContains('Note 1', $notes); + $this->assertContains('Note 2', $notes); + + $note1 = current(array_filter($data, function($note) { + return $note['note'] === 'Note 1'; + })); + $note2 = current(array_filter($data, function($note) { + return $note['note'] === 'Note 2'; + })); + + $this->assertFalse($note1['customer_note']); + $this->assertTrue($note2['customer_note']);📝 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_get_order_notes() { wp_set_current_user( $this->seller_id1 ); $order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create(); // Add some notes wc_create_order_note( $order_id, 'Note 1' ); wc_create_order_note( $order_id, 'Note 2', 1, true ); $response = $this->get_request( "orders/{$order_id}/notes" ); $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); $this->assertCount( 2, $data ); $notes = array_column($data, 'note'); $this->assertContains('Note 1', $notes); $this->assertContains('Note 2', $notes); $note1 = current(array_filter($data, function($note) { return $note['note'] === 'Note 1'; })); $note2 = current(array_filter($data, function($note) { return $note['note'] === 'Note 2'; })); $this->assertFalse($note1['customer_note']); $this->assertTrue($note2['customer_note']); }
65-80: 🛠️ Refactor suggestion
Enhance delete note test coverage and documentation.
The test should validate the response data structure and handle edge cases. Also, document the purpose of the
forceparameter.public function test_delete_order_note() { wp_set_current_user( $this->seller_id1 ); $order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create(); $note_id = wc_create_order_note( $order_id, 'Test note' ); $response = $this->delete_request( "orders/{$order_id}/notes/{$note_id}", array( 'force' => true ) ); $this->assertEquals( 200, $response->get_status() ); + $data = $response->get_data(); + + // Verify response structure + $this->assertArrayHasKey('deleted', $data); + $this->assertTrue($data['deleted']); // Verify the note is deleted $this->assertEmpty( wc_get_order_notes( [ 'id' => $note_id ] ) ); + + // Verify deleting again returns 404 + $response = $this->delete_request( + "orders/{$order_id}/notes/{$note_id}", + array( 'force' => true ) + ); + $this->assertEquals( 404, $response->get_status() ); }Also add a docblock explaining the force parameter:
/** * Test deleting an order note. + * + * @since 3.0.0 + * + * The 'force' parameter is used to permanently delete the note + * instead of moving it to trash. In WP REST API, this is a + * common pattern for delete operations. */ public function test_delete_order_note() {📝 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./** * Test deleting an order note. * * @since 3.0.0 * * The 'force' parameter is used to permanently delete the note * instead of moving it to trash. In WP REST API, this is a * common pattern for delete operations. */ public function test_delete_order_note() { wp_set_current_user( $this->seller_id1 ); $order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create(); $note_id = wc_create_order_note( $order_id, 'Test note' ); $response = $this->delete_request( "orders/{$order_id}/notes/{$note_id}", array( 'force' => true ) ); $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); // Verify response structure $this->assertArrayHasKey('deleted', $data); $this->assertTrue($data['deleted']); // Verify the note is deleted $this->assertEmpty( wc_get_order_notes( [ 'id' => $note_id ] ) ); // Verify deleting again returns 404 $response = $this->delete_request( "orders/{$order_id}/notes/{$note_id}", array( 'force' => true ) ); $this->assertEquals( 404, $response->get_status() ); }includes/REST/Manager.php (1)
237-246: 💡 Codebase verification
Now I have enough information to provide the final response:
All controller files are present except ShippingStatusController.php
The verification shows that 9 out of 10 controller files exist in the codebase:
- DataCountryController.php
- ShippingMethodController.php
- TaxClassController.php
- TaxController.php
- PaymentGatewayController.php
- OrderControllerV3.php
- OrderNoteControllerV3.php
- OrderRefundControllerV3.php
- OrderActionControllerV3.php
However,
ShippingStatusController.phpis missing from the codebase despite being referenced in the Manager.php class map. This needs to be addressed.🔗 Analysis chain
Verify existence of new controller files.
Let's ensure all new controller files are present in the codebase.
Let me analyze the results. The output shows that only 4 out of 10 controller files were found in the codebase:
- OrderControllerV3.php
- OrderNoteControllerV3.php
- OrderRefundControllerV3.php
- OrderActionControllerV3.php
However, let me verify with a different search strategy to ensure we're not missing any files due to path specifics.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if all new controller files exist # Test: Check existence of new controller files for controller in DataCountry ShippingMethod ShippingStatus TaxClass Tax PaymentGateway OrderControllerV3 OrderNoteControllerV3 OrderRefundControllerV3 OrderActionControllerV3; do fd -t f "${controller}.php$" -E "vendor/*" doneLength of output: 2407
Script:
#!/bin/bash # Search in includes/REST directory specifically since that's where the files should be ls -la includes/REST/DataCountry*.php includes/REST/ShippingMethod*.php includes/REST/ShippingStatus*.php includes/REST/TaxClass*.php includes/REST/Tax*.php includes/REST/PaymentGateway*.php 2>/dev/null # Also do a broader search with ripgrep to find any references to these controllers rg -l "class (DataCountry|ShippingMethod|ShippingStatus|TaxClass|Tax|PaymentGateway)Controller" --type phpLength of output: 1245
includes/REST/TaxClassController.php (1)
14-14:
⚠️ Potential issueReplace placeholder 'DOKAN_SINCE' with actual version number in
@sincetagThe
@sinceannotation in the class documentation uses the placeholderDOKAN_SINCE. Please replace it with the actual version number of the plugin in which this class was introduced.includes/REST/OrderRefundControllerV3.php (7)
14-14:
⚠️ Potential issueReplace placeholder
DOKAN_SINCEwith the actual version numberThe
@since DOKAN_SINCEtag in the class documentation should be updated to reflect the actual version number before release.
75-85:
⚠️ Potential issueHandle invalid refund IDs gracefully
In
get_item_permissions_check, after retrieving the refund object, you check if it exists. Ensure that if the refund does not exist or theparent_idis invalid, appropriate errors are returned.
95-105:
⚠️ Potential issueValidate refund existence before authorization in
delete_item_permissions_checkEnsure that the refund object is valid before proceeding with order authorization checks to prevent errors when
get_parent_id()is called on a non-existent refund.
33-114: 🛠️ Refactor suggestion
Consider adding capability checks for enhanced security
While you are checking if the current user is a seller, you might also want to check if they have specific capabilities required for refund operations to ensure robust security.
33-39: 🛠️ Refactor suggestion
Use consistent status codes and messages in authorization checks
In the
check_order_authorizationmethod, you return aWP_Errorwith status code403. Consider usingrest_authorization_required_code()instead of hardcoding403to maintain consistency with WordPress REST API practices.Apply this diff to use the consistent authorization code:
return new WP_Error( 'dokan_rest_unauthorized_order', __( 'You do not have permission to access this order', 'dokan-lite' ), - array( 'status' => 403 ) + array( 'status' => rest_authorization_required_code() ) );📝 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.protected function check_order_authorization( int $order_id ) { $vendor_id = dokan_get_seller_id_by_order( $order_id ); if ( $vendor_id !== dokan_get_current_user_id() ) { return new WP_Error( 'dokan_rest_unauthorized_order', __( 'You do not have permission to access this order', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) ); } return true; }
61-67:
⚠️ Potential issueEnsure
order_idvalidation increate_item_permissions_checkSimilar to
get_items_permissions_check, make sure thatorder_idis validated increate_item_permissions_checkto prevent potential issues.Apply this diff to add
order_idvalidation:public function create_item_permissions_check( $request ) { if ( ! $this->check_vendor_permission() ) { return new WP_Error( 'dokan_rest_cannot_create', __( 'Sorry, you are not allowed to create resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) ); } + if ( empty( $request['order_id'] ) ) { + return new WP_Error( + 'dokan_rest_invalid_order_id', + __( 'Order ID is required.', 'dokan-lite' ), + array( 'status' => 400 ) + ); + } return $this->check_order_authorization( $request['order_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.public function create_item_permissions_check( $request ) { if ( ! $this->check_vendor_permission() ) { return new WP_Error( 'dokan_rest_cannot_create', __( 'Sorry, you are not allowed to create resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) ); } if ( empty( $request['order_id'] ) ) { return new WP_Error( 'dokan_rest_invalid_order_id', __( 'Order ID is required.', 'dokan-lite' ), array( 'status' => 400 ) ); } return $this->check_order_authorization( $request['order_id'] ); }
47-53:
⚠️ Potential issueValidate the presence of
order_idin the requestIn
get_items_permissions_check, ensure thatorder_idis provided in the request to avoid undefined index notices or errors.Apply this diff to validate
order_id:public function get_items_permissions_check( $request ) { if ( ! $this->check_vendor_permission() ) { return new WP_Error( 'dokan_rest_cannot_view', __( 'Sorry, you cannot list resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) ); } + if ( empty( $request['order_id'] ) ) { + return new WP_Error( + 'dokan_rest_invalid_order_id', + __( 'Order ID is required.', 'dokan-lite' ), + array( 'status' => 400 ) + ); + } return $this->check_order_authorization( $request['order_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.public function get_items_permissions_check( $request ) { if ( ! $this->check_vendor_permission() ) { return new WP_Error( 'dokan_rest_cannot_view', __( 'Sorry, you cannot list resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) ); } if ( empty( $request['order_id'] ) ) { return new WP_Error( 'dokan_rest_invalid_order_id', __( 'Order ID is required.', 'dokan-lite' ), array( 'status' => 400 ) ); } return $this->check_order_authorization( $request['order_id'] ); }includes/REST/OrderNoteControllerV3.php (4)
115-131:
⚠️ Potential issueHandle Unexpected Response Types in
create_itemIn the
create_itemmethod, after callingparent::create_item( $request ), consider checking if$responseis an instance ofWP_REST_Responsebefore proceeding to use its methods to prevent potential errors.Apply this check:
if ( is_wp_error( $response ) ) { return $response; } if ( ! ( $response instanceof WP_REST_Response ) ) { return new WP_Error( 'dokan_rest_invalid_response', esc_html__( 'Invalid response from parent method.', 'dokan-lite' ), array( 'status' => 500 ) ); } if ( $response->get_status() === 201 ) { // Proceed with updating the note author }
146-158: 🛠️ Refactor suggestion
Improve Data Handling in
update_note_authorConsider validating and sanitizing user data before updating the comment to enhance security and data integrity.
Implement data sanitization:
if ( $creating && $user instanceof \WP_User && dokan_is_user_seller( $user_id ) ) { $data = array( 'comment_ID' => $note->comment_ID, 'user_id' => $user_id, 'comment_author' => sanitize_text_field( $user->display_name ), 'comment_author_email' => sanitize_email( $user->user_email ), ); wp_update_comment( wp_slash( $data ) ); // Update comment meta to indicate it's a vendor note update_comment_meta( (int) $note->comment_ID, 'dokan_vendor_note', 1 ); }
93-93:
⚠️ Potential issueEscape Error Messages in
delete_item_permissions_checkFor consistency and security, use
esc_html__in thedelete_item_permissions_checkmethod.Apply this diff:
-return new WP_Error( 'dokan_rest_cannot_delete', __( 'Sorry, you are not allowed to delete this resource.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) ); +return new WP_Error( 'dokan_rest_cannot_delete', esc_html__( 'Sorry, you are not allowed to delete this resource.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) );📝 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.return new WP_Error( 'dokan_rest_cannot_delete', esc_html__( 'Sorry, you are not allowed to delete this resource.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) );
51-51:
⚠️ Potential issueEscape Error Messages for Security
In
get_items_permissions_check, consider usingesc_html__instead of__to properly escape the error message and enhance security by preventing potential HTML injection.Apply this diff to address the issue:
-return new WP_Error( 'dokan_rest_cannot_view', __( 'Sorry, you cannot list resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) ); +return new WP_Error( 'dokan_rest_cannot_view', esc_html__( 'Sorry, you cannot list resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) );📝 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.return new WP_Error( 'dokan_rest_cannot_view', esc_html__( 'Sorry, you cannot list resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) );includes/REST/OrderActionControllerV3.php (4)
66-83: 🛠️ Refactor suggestion
Add return type declarations for consistency and type safety
Consider adding return type declarations to your methods to enhance code clarity and enforce type safety. This will help developers understand the expected return values and can prevent potential bugs.
Apply these diffs to add return type declarations:
- protected function check_order_authorization( int $order_id ) { + protected function check_order_authorization( int $order_id ): bool|\WP_Error {- public function get_items_permissions_check( $request ) { + public function get_items_permissions_check( $request ): bool|\WP_Error {- public function create_item_permissions_check( $request ) { + public function create_item_permissions_check( $request ): bool|\WP_Error {- public function get_items( $request ) { + public function get_items( $request ): WP_REST_Response|\WP_Error {- public function create_item( $request ) { + public function create_item( $request ): WP_REST_Response|\WP_Error {- private function process_order_action( WC_Order $order, string $action ) { + private function process_order_action( WC_Order $order, string $action ): bool|\WP_Error {Also applies to: 91-97, 105-111, 128-139, 148-170, 206-299
206-299: 🛠️ Refactor suggestion
Handle unexpected actions more gracefully in
process_order_actionWhen an unrecognized action is received, consider providing more context in the error message to aid in debugging. Logging the unexpected action could also help in monitoring and resolving potential issues.
155-155:
⚠️ Potential issueSanitize the 'action' parameter from the request
Ensure that the 'action' parameter retrieved from the request is properly sanitized to prevent potential security issues related to untrusted input.
Apply this diff to sanitize the 'action' parameter:
- $action = $request['action']; + $action = sanitize_text_field( $request['action'] );📝 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.$action = sanitize_text_field( $request['action'] );
278-296:
⚠️ Potential issueUse
has_actioninstead ofdid_actionto check for custom order actionsIn the
process_order_actionmethod, thedefaultcase usesdid_action()to check if a custom order action exists. However,did_action()checks how many times an action has been fired, not whether it's registered. To verify if an action hook exists, you should usehas_action()instead.Apply this diff to fix the issue:
- if ( did_action( 'woocommerce_order_action_' . sanitize_title( $action ) ) ) { + if ( has_action( 'woocommerce_order_action_' . sanitize_title( $action ) ) ) {📝 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.default: if ( has_action( 'woocommerce_order_action_' . sanitize_title( $action ) ) ) { /** * Fires when a custom order action is being processed. * * This hook allows third-party plugins to add custom order actions * and define their behavior when triggered through the API. * * @since DOKAN_SINCE * * @param WC_Order $order The order object for which the action is being performed. * * @hook woocommerce_order_action_{$action} */ do_action( 'woocommerce_order_action_' . sanitize_title( $action ), $order ); } else { return new WP_Error( 'dokan_rest_invalid_order_action', esc_html__( 'Invalid order action.', 'dokan-lite' ), array( 'status' => 400 ) ); } }includes/REST/OrderControllerV3.php (3)
175-180: 🛠️ Refactor suggestion
Optimize query performance by indexing
_dokan_vendor_idmeta keyThe meta query in
prepare_objects_queryfilters orders by_dokan_vendor_id, which can lead to slow queries on large datasets because meta queries are less efficient. To improve performance, consider adding a database index on the_dokan_vendor_idmeta key.
306-311:
⚠️ Potential issueEnsure filter is removed even if an exception occurs
In the
perform_vendor_actionmethod, if an exception is thrown within the$action()callable, theremove_filtercall will not execute, leaving the filter attached and potentially affecting subsequent operations. To ensure the filter is always removed, use atry...finallyblock.Apply this diff to fix the issue:
private function perform_vendor_action( callable $action ) { add_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] ); - $result = $action(); - remove_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] ); + try { + $result = $action(); + } finally { + remove_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] ); + } return $result; }📝 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.private function perform_vendor_action( callable $action ) { add_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] ); try { $result = $action(); } finally { remove_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] ); } return $result; }
286-286:
⚠️ Potential issueRename parameter to avoid using reserved keyword "object"
In PHP 7 and later,
objectis a reserved keyword. Using$objectas a parameter name can lead to syntax errors or unexpected behavior. Please rename the$objectparameter to something like$data_objectto avoid conflicts.Apply this diff to fix the issue:
-public function prepare_object_for_response( $object, $request ): WP_REST_Response { +public function prepare_object_for_response( $data_object, $request ): WP_REST_Response {Also, update all references to
$objectwithin the method accordingly.Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 286-286:
It is recommended not to use reserved keyword "object" as function parameter name. Found: $objecttests/php/src/REST/OrdersControllerV3Test.php (4)
91-105: 🛠️ Refactor suggestion
Add missing return type declarations for consistency
Some test methods lack return type declarations while others include
: void. For consistency and clarity, consider adding: voidto methods that do not return a value.Also applies to: 221-245
250-285: 🛠️ Refactor suggestion
Refactor duplicated code in meta data tests
The methods
test_collection_param_include_meta,test_collection_param_include_meta_empty, andtest_collection_param_exclude_metacontain similar setup and assertion code. Refactoring the common code into helper methods will improve maintainability and reduce duplication.Also applies to: 290-326, 331-366
655-656:
⚠️ Potential issueFix incorrect discount calculation in test
The expected total is calculated with a 5% discount (
* 0.95), but the coupon provides a 10% discount. Update the calculation to reflect the correct discount rate.Apply this diff to correct the calculation:
- $expected_total = (float) ( $product->get_price() * 2 ) * 0.95; // 10% discount + $expected_total = (float) ( $product->get_price() * 2 ) * 0.9; // 10% discount📝 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.$expected_total = (float) ( $product->get_price() * 2 ) * 0.9; // 10% discount $this->assertEquals( $expected_total, $data['total'] );
570-575:
⚠️ Potential issueCorrect meta data access in line items
Within the loop over
$order['line_items'], the assertions check'meta_data'in$orderinstead of$line_item. To accurately verify each line item's meta data, update the references to$line_item.Apply this diff to fix the assertions:
- $this->assertArrayHasKey( 'meta_data', $order ); - $this->assertGreaterThanOrEqual( 3, count( $order['meta_data'] ) ); + $this->assertArrayHasKey( 'meta_data', $line_item ); + $this->assertGreaterThanOrEqual( 3, count( $line_item['meta_data'] ) );📝 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->assertArrayHasKey( 'line_items', $order ); foreach ( $order['line_items'] as $line_item ) { $this->assertArrayHasKey( 'meta_data', $line_item ); $this->assertGreaterThanOrEqual( 3, count( $line_item['meta_data'] ) );
There was a problem hiding this comment.
Actionable comments posted: 48
🧹 Outside diff range and nitpick comments (21)
tests/php/src/REST/TaxClassControllerTest.php (1)
1-39: Missing test cases for critical functionality.The test suite should include additional test methods for:
- Tax class creation (POST)
- Tax class updates (PUT)
- Tax class deletion (DELETE)
- Validation of tax class data
- Rate limits and quotas
- Integration with WooCommerce tax settings
Would you like me to provide a comprehensive test suite implementation covering these scenarios?
tests/php/src/REST/TaxControllerTest.php (1)
7-12: Enhance class documentation.Consider adding more comprehensive PHPDoc for the test class, including:
- A description of what aspects of the Tax API are being tested
- Any prerequisites or setup requirements
- The API version being tested
/** + * Class TaxControllerTest + * + * Tests the v1 REST API endpoints for tax management, including: + * - Route registration + * - Tax data retrieval + * + * @since 1.0.0 * @group dokan-taxes */includes/REST/DataCountryController.php (2)
11-18: Replace DOKAN_SINCE placeholder with actual version number.The
@since DOKAN_SINCEplaceholder should be replaced with the actual version number where this feature is being introduced.- * @since DOKAN_SINCE + * @since 3.9.0
33-38: Consider enhancing error message with more context.While the permission check is secure, the error message could be more specific about the required role/permission.
- return new WP_Error( 'dokan_rest_cannot_view', __( 'Sorry, you cannot list resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) ); + return new WP_Error( + 'dokan_rest_cannot_view', + __( 'Sorry, you need to be a vendor to access country data.', 'dokan-lite' ), + array( 'status' => rest_authorization_required_code() ) + );tests/php/src/REST/DataCountriesControllerTest.php (1)
7-13: Consider enhancing class documentation.While the @group annotation is helpful, consider adding a class-level PHPDoc that describes the purpose and scope of these tests.
/** + * Class DataCountriesControllerTest + * + * Tests for the REST API endpoints handling country and state data retrieval. + * Covers endpoints under 'dokan/v1/data/countries'. + * * @group dokan-data-countries */tests/php/src/REST/ShippingMethodControllerTest.php (1)
1-11: Add strict types declaration and class documentation.Consider enhancing the file header with strict types declaration and adding comprehensive class-level documentation.
<?php +declare(strict_types=1); namespace WeDevs\Dokan\Test\REST; use WeDevs\Dokan\Test\DokanTestCase; /** + * Class ShippingMethodControllerTest + * + * Tests for shipping method REST API endpoints to ensure proper functionality + * of shipping method retrieval and route registration. + * * @group dokan-shipping-methods */ class ShippingMethodControllerTest extends DokanTestCase {includes/REST/TaxController.php (1)
18-61: Consider adding vendor-specific tax functionality.The controller currently only implements permission checks while inheriting all functionality from WC_REST_Taxes_Controller. Consider whether vendor-specific tax features are needed, such as:
- Filtering tax rates by vendor
- Adding vendor-specific tax settings
- Implementing vendor tax reports endpoints
This would make the controller more aligned with vendor-specific requirements mentioned in the PR objectives.
includes/REST/TaxClassController.php (2)
11-25: Replace DOKAN_SINCE placeholder with actual version number.The documentation block contains a placeholder
@since DOKAN_SINCEwhich should be replaced with the actual version number where this feature is being introduced.
1-61: Consider integrating with a centralized permission system.Since this is part of a larger REST API enhancement, consider implementing a centralized permission management system that can be reused across all new controllers (Orders, Taxes, Shipping, Payment). This would ensure consistent permission checking logic and make it easier to maintain and modify permissions in the future.
Key considerations for a centralized system:
- Create a shared trait or base class for common permission checks
- Implement capability-based permissions that can be filtered
- Add support for custom roles and multi-vendor scenarios
includes/REST/ShippingMethodController.php (1)
5-7: Consider organizing use statements alphabetically.While the current implementation works correctly, organizing use statements alphabetically improves readability and maintainability.
-use WC_REST_Shipping_Methods_Controller; -use WP_Error; -use WP_REST_Request; +use WC_REST_Shipping_Methods_Controller; +use WP_Error; +use WP_REST_Request;includes/REST/PaymentGatewayController.php (2)
33-38: Consider using more specific error messages.While the current error messages are functional, they could be more specific to help users understand why they can't access payment gateway information.
- __( 'Sorry, you cannot list resources.', 'dokan-lite' ) + __( 'Sorry, you do not have permission to view payment gateways.', 'dokan-lite' ) - __( 'Sorry, you cannot view this resource.', 'dokan-lite' ) + __( 'Sorry, you do not have permission to view this payment gateway.', 'dokan-lite' )Also applies to: 46-51
18-61: Consider implementing prepare_item_for_response method.Since this is a vendor-specific endpoint, you might want to customize the response data to only include information relevant to vendors. Consider overriding the
prepare_item_for_responsemethod from the parent class to filter sensitive gateway settings.Example implementation:
/** * Prepare a payment gateway for response. * * @param WC_Payment_Gateway $item Payment gateway instance. * @param WP_REST_Request $request Request object. * @return WP_REST_Response */ public function prepare_item_for_response( $item, $request ) { $response = parent::prepare_item_for_response( $item, $request ); $data = $response->get_data(); // Remove sensitive data that vendors shouldn't see unset( $data['settings'] ); // Add vendor-specific data if needed $response->set_data( $data ); return $response; }tests/php/src/REST/OrderActionsControllerV3Test.php (1)
7-11: Enhance test class documentation.Consider adding a class-level PHPDoc block with a description and the
@coversannotation to explicitly indicate which class is being tested./** + * Class OrderActionsControllerV3Test + * + * Tests for order actions REST API endpoints in Dokan plugin. + * + * @covers \WeDevs\Dokan\REST\OrderActionsControllerV3 * @group dokan-orders-v3 * @group dokan-order-actions-v3 */tests/php/src/REST/OrderRefundsControllerV3Test.php (1)
16-19: Enhance PHPDoc with expected response structure.The method documentation should include details about the expected response format and the significance of negative totals in refunds.
/** * Test if line, fees and shipping items are all included in refund response. + * + * @testdox Refund response should include line items, fees, and shipping with negative totals + * + * @return void + * + * Expected response structure: + * { + * "line_items": [{"total": "-1.00", ...}], + * "fee_lines": [{"total": "-10.00", ...}], + * "shipping_lines": [{"total": "-20.00", ...}] + * } */tests/php/src/REST/OrderNotesControllerTestV3.php (1)
1-81: Consider implementing a comprehensive test matrix.While the current tests cover basic CRUD operations, consider implementing a comprehensive test matrix that includes:
- Permission levels (vendor, customer, admin, guest)
- Edge cases (empty values, invalid IDs, malformed requests)
- Business logic variations (note types, visibility settings)
- Response format consistency
- Error message validation
This will ensure the API behaves consistently and securely across all scenarios.
includes/REST/Manager.php (1)
237-246: Consider architectural improvements for better maintainability.As the number of controllers grows, consider these architectural improvements:
- Group controllers by domain (Orders, Shipping, etc.) using sub-managers
- Implement a versioning strategy that allows multiple versions to coexist
- Consider lazy loading of controllers to improve performance
Example structure:
namespace WeDevs\Dokan\REST; abstract class BaseManager { protected function registerControllers() {} } class OrderManager extends BaseManager { protected function registerControllers() { // Register only order-related controllers } }Would you like me to provide a detailed implementation plan for these architectural improvements?
includes/REST/OrderNoteControllerV3.php (2)
86-86: Correct grammatical error in method commentThe doc comment for
delete_item_permissions_checkcontains a grammatical error.Current:
* Check if a given request has access delete a order note.Suggested correction:
* Check if a given request has access to delete an order note.
142-142: Clarify the method description in the doc commentThe description for
update_note_authorshould more accurately reflect its functionality.Current:
* Update a single order note.Suggested:
* Update the author information of an order note after creation.includes/REST/OrderActionControllerV3.php (1)
178-196: Ensure Proper Localization for Custom Order ActionsThe method
get_available_order_actions_for_orderallows filtering of the$actionsarray via thewoocommerce_order_actionsfilter. While default actions are localized, custom actions added by third-party plugins may not be. To maintain consistency, ensure that any additional actions added through the filter are properly localized using translation functions.Consider adding a note in the documentation:
/** * Filters the list of available order actions. * * This filter allows you to add or remove order actions for the Dokan vendor dashboard. * Note: Ensure that custom actions are localized using translation functions. * * @since DOKAN_SINCE * * @param array $actions The list of available order actions. * @param WC_Order $order The order object. */includes/REST/OrderControllerV3.php (1)
171-173: Redundant vendor check in query preparationThe check for
! dokan_is_user_seller( $user )before returning$argsmight be redundant if non-vendor users are already restricted from accessing this endpoint. Evaluate if this condition is necessary.tests/php/src/REST/OrdersControllerV3Test.php (1)
110-113: Address the TODO regarding meta queriesIn the
test_orders_get_allmethod, there's a TODO comment indicating that meta queries are not working in test cases. Consider resolving this issue to ensure comprehensive testing of meta query functionality in order retrieval.Would you like assistance in addressing the meta query issue in the test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
- includes/REST/DataCountryController.php (1 hunks)
- includes/REST/Manager.php (1 hunks)
- includes/REST/OrderActionControllerV3.php (1 hunks)
- includes/REST/OrderControllerV3.php (1 hunks)
- includes/REST/OrderNoteControllerV3.php (1 hunks)
- includes/REST/OrderRefundControllerV3.php (1 hunks)
- includes/REST/PaymentGatewayController.php (1 hunks)
- includes/REST/ShippingMethodController.php (1 hunks)
- includes/REST/TaxClassController.php (1 hunks)
- includes/REST/TaxController.php (1 hunks)
- tests/php/src/REST/DataCountriesControllerTest.php (1 hunks)
- tests/php/src/REST/OrderActionsControllerV3Test.php (1 hunks)
- tests/php/src/REST/OrderNotesControllerTestV3.php (1 hunks)
- tests/php/src/REST/OrderRefundsControllerV3Test.php (1 hunks)
- tests/php/src/REST/OrdersControllerV3Test.php (1 hunks)
- tests/php/src/REST/PaymentGatewaysControllerTest.php (1 hunks)
- tests/php/src/REST/ShippingMethodControllerTest.php (1 hunks)
- tests/php/src/REST/TaxClassControllerTest.php (1 hunks)
- tests/php/src/REST/TaxControllerTest.php (1 hunks)
🧰 Additional context used
🪛 GitHub Check: Run PHPCS inspection
includes/REST/OrderControllerV3.php
[warning] 286-286:
It is recommended not to use reserved keyword "object" as function parameter name. Found: $object
🔇 Additional comments (24)
tests/php/src/REST/TaxClassControllerTest.php (1)
1-13:⚠️ Potential issueNamespace mismatch between test class and route.
The test class uses namespace 'dokan/v1' but is grouped under '@group dokan-tax-classes-v3'. This inconsistency could lead to confusion. Either update the namespace to v3 or clarify why v1 is being used for a v3 feature.
Let's verify the namespace usage across the codebase:
tests/php/src/REST/TaxControllerTest.php (1)
1-5: LGTM! File structure follows PSR-4 standards.The namespace and use statements are properly organized.
includes/REST/DataCountryController.php (2)
1-9: LGTM! Well-structured file header with proper security check.The namespace, imports, and security check are properly implemented following WordPress coding standards.
20-25: Verify API version alignment with PR objectives.The PR objectives mention different versions for different endpoints (v3 for Orders, v1 for Tax/Shipping/Payment). Since this is a data/country controller, please confirm if v1 is the intended version for this endpoint.
✅ Verification successful
Based on the search results, I can now generate the final response as I have sufficient information to verify the API versioning concern.
API version (v1) is correctly aligned for the DataCountryController
The version
dokan/v1is consistent with other data and configuration-related endpoints in the codebase:
- All data controllers use v1:
DataCountryController,DummyDataController- All configuration controllers use v1:
TaxController,ShippingMethodController,PaymentGatewayController- Only Order-related controllers have v2 and v3 versions, which aligns with the PR objectives
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other API version declarations to ensure consistency rg -g '*.php' 'protected \$namespace = .dokan/v[0-9]+.' includes/REST/Length of output: 1965
tests/php/src/REST/PaymentGatewaysControllerTest.php (2)
1-10: LGTM! Well-structured test class setup.The class follows PHP best practices with proper namespace, inheritance, and test group annotation.
12-13: LGTM! Correct API version used.The namespace 'dokan/v1' aligns with the PR objectives for payment gateway endpoints.
tests/php/src/REST/DataCountriesControllerTest.php (2)
1-6: LGTM! File structure follows PSR standards.
1-54: Verify integration with shipping zone functionality.Since this endpoint provides country/state data for shipping zones (as mentioned in PR objectives), consider adding tests that verify the data format matches what's expected by the shipping zone management endpoints.
includes/REST/TaxController.php (3)
1-9: LGTM! File structure follows best practices.The file structure follows WordPress coding standards with proper namespace, imports, and security measures.
53-60: Add documentation for helper functions and verify their existence.The method uses Dokan helper functions. Let's add proper documentation and verify these functions exist.
/** * Check vendor permission. * + * @uses dokan_is_user_seller() Checks if the user is a seller + * @uses dokan_get_current_user_id() Gets the current user ID * * @return bool */✅ Verification successful
Let me gather more information about these helper functions to provide accurate documentation.
Documentation suggestion verified and can be improved further
Based on the implementation details found, here's a more accurate documentation suggestion:
/** * Check vendor permission. * + * @uses dokan_is_user_seller() Checks if a user has the 'dokandar' capability + * @uses dokan_get_current_user_id() Gets the current user ID, handles vendor staff cases * * @return bool */The helper functions exist in
includes/functions.phpwith proper implementations:
dokan_get_current_user_id()handles special logic for vendor staff usersdokan_is_user_seller()checks for the 'dokandar' capability🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify existence of Dokan helper functions # Expect: Function definitions or references in core files # Search for helper function definitions rg "function (dokan_is_user_seller|dokan_get_current_user_id)\b" --type php # Search for documentation of these functions rg "@function (dokan_is_user_seller|dokan_get_current_user_id)\b" --type phpLength of output: 285
Script:
#!/bin/bash # Get the implementation of these helper functions with context rg "function dokan_(is_user_seller|get_current_user_id)" -B 2 -A 6 includes/functions.phpLength of output: 547
20-25: Verify API versioning strategy consistency.The namespace is set to 'dokan/v1' which aligns with the PR objectives for Tax Management APIs (v1). However, let's verify this is consistent with other new controllers.
✅ Verification successful
Let me try a different search strategy to verify the API versioning consistency.
The previous search might have failed due to escaping issues. This new approach should help us find both explicit namespace definitions and route registrations to verify the API versioning strategy.
Let me fix the shell script syntax and try one more time.
The previous script had quote matching issues. This version uses a more precise regex pattern and removes the problematic parentheses in the command.
Let me try one final approach with simpler patterns.
I've simplified the patterns to avoid any syntax issues with quotes and made the search more basic but still effective for our verification needs.
API versioning is consistent with the codebase patterns
The namespace
dokan/v1inTaxController.phpis correct and consistent with the codebase versioning patterns:
All tax-related controllers use
dokan/v1:
TaxController.php: v1TaxClassController.php: v1- Other related endpoints like
PaymentGatewayController.phpandDataCountryController.php: v1The codebase shows clear versioning patterns:
- v1: Used for most basic features and new standalone features
- v2: Used for upgraded features (e.g.,
StoreSettingControllerV2,WithdrawControllerV2)- v3: Specifically used for order-related endpoints (e.g.,
OrderControllerV3,OrderNoteControllerV3)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check API versioning consistency across new controllers # Expect: All tax-related endpoints use v1, while orders use v3 # Search for namespace definitions in REST controllers rg -B 1 -A 1 "protected \$namespace = '(dokan|wc)/" --type phpLength of output: 66
Script:
#!/bin/bash # Search for namespace property definitions in PHP files rg -B 2 -A 2 '\$namespace\s*=\s*["\'].*["\']' --type php # Also search for any route registrations that might reveal API versions rg -B 2 -A 2 'register_rest_route\(' --type phpLength of output: 93
Script:
#!/bin/bash # Search for namespace property definitions in PHP files rg -B 2 -A 2 '\$namespace\s*=\s*[\'"][^\'"]*[\'"]' --type php # Also search for any route registrations that might reveal API versions rg -B 2 -A 2 'register_rest_route' --type phpLength of output: 93
Script:
#!/bin/bash # Search for namespace definitions rg '\$namespace' --type php # Search for route registrations rg 'register_rest_route' --type phpLength of output: 9328
includes/REST/TaxClassController.php (2)
1-9: LGTM! Proper namespace, imports and security check.The file follows best practices with proper namespace declaration, necessary imports, and includes the ABSPATH security check to prevent direct file access.
27-38: Consider validating the request parameter.While the permission check is implemented correctly, consider validating the
$requestparameter even though it's not currently used. This future-proofs the method in case request parameters are added later.Let's check if there are any request parameters being used in the parent class:
includes/REST/ShippingMethodController.php (2)
20-25: LGTM! Proper REST API versioning.The namespace follows REST API versioning best practices with a clear vendor prefix and version number.
1-61: Verify REST API endpoint registration.Let's ensure this controller is properly registered in the REST API manager.
✅ Verification successful
Let me gather more information about the REST API registration by checking the class map in the Manager.php file.
Let me fix the regex and try again to check the REST API class map.
Let me check if the ShippingMethodController is registered elsewhere in the Manager.php file since it's not in the initial class map.
Let me verify how these registered controllers are initialized in the Manager class.
Based on the gathered information, I can now provide the final response:
REST API endpoint registration is properly implemented ✅
The ShippingMethodController is correctly registered in the REST API system:
- It's included in the REST API class map in
Manager.php- The Manager class properly initializes all registered controllers through the
register_rest_routesmethod- The registration happens during WordPress's
rest_api_inithook- The controller extends
WC_REST_Shipping_Methods_Controllerand sets its namespace to 'dokan/v1'🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ShippingMethodController registration rg -l "ShippingMethodController" | xargs rg "get_rest_api_class_map|rest_api_init"Length of output: 332
Script:
#!/bin/bash # Check the REST API class map content rg -A 20 "get_rest_api_class_map\(\) {" includes/REST/Manager.phpLength of output: 187
Script:
#!/bin/bash # Check the REST API class map content with fixed regex rg -A 20 "private function get_rest_api_class_map" includes/REST/Manager.phpLength of output: 2198
Script:
#!/bin/bash # Check for ShippingMethodController registration in Manager.php rg -B 5 -A 5 "ShippingMethodController" includes/REST/Manager.phpLength of output: 1462
Script:
#!/bin/bash # Check the registration mechanism in Manager.php rg -B 5 -A 10 "register_rest_routes" includes/REST/Manager.phpLength of output: 1327
includes/REST/PaymentGatewayController.php (2)
1-9: LGTM! File structure follows best practices.The file includes proper namespace declaration, necessary imports, and security check.
20-25: Verify API version number.The PR objectives mention this is a new Payment Gateway API (v1), so the namespace version is correct. However, let's verify there are no other payment gateway endpoints in different versions.
✅ Verification successful
Based on the search results, I can now generate the final response since I have sufficient information about the API versioning across the codebase.
API version (v1) is consistent with related endpoints
The payment gateway endpoint version is correctly set to v1, which aligns with other related endpoints in the codebase:
- Other payment/financial related endpoints like
WithdrawControllerandReverseWithdrawalControlleruse v1- Store settings and tax-related endpoints also use v1
- This is the first payment gateway controller implementation, so starting with v1 is appropriate
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing payment gateway endpoint definitions rg -i "payment.*gateway.*controller" --type php # Search for dokan API endpoint registrations rg "namespace.*dokan/v" --type phpLength of output: 3656
tests/php/src/REST/OrderActionsControllerV3Test.php (1)
37-54: 🛠️ Refactor suggestionVerify actual effects of order actions.
The test only verifies the API response but doesn't confirm that the action was actually performed. Consider:
- Adding assertions to verify the email was sent for 'send_order_details'
- Checking order meta or status changes
- Using WordPress testing hooks to verify action triggers
public function test_apply_order_action() { wp_set_current_user( $this->seller_id1 ); $order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create(); + // Hook into email sending action to verify it's triggered + $email_sent = false; + add_action('woocommerce_email_send_order_details', function() use (&$email_sent) { + $email_sent = true; + }); + $response = $this->post_request( "orders/{$order_id}/actions", ['action' => 'send_order_details'] ); $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); $this->assertArrayHasKey( 'message', $data ); $this->assertEquals( 'Order action applied successfully.', $data['message'] ); $this->assertEquals( 'send_order_details', $data['action'] ); + $this->assertTrue($email_sent, 'Email should have been triggered'); }tests/php/src/REST/OrderRefundsControllerV3Test.php (1)
1-14: LGTM! Well-structured test class setup.The class follows PHPUnit best practices with proper namespacing, inheritance, and test grouping annotations.
tests/php/src/REST/OrderNotesControllerTestV3.php (1)
1-11: LGTM! Well-structured test file setup.The file follows proper PHP practices with clear namespace declaration and appropriate test grouping.
includes/REST/OrderRefundControllerV3.php (1)
112-114: Vendor Permission Check Implementation is CorrectThe
check_vendor_permissionmethod correctly determines if the current user is a vendor, ensuring that only vendor users can access these endpoints.includes/REST/OrderActionControllerV3.php (1)
221-224: Check for Existence of Email Classes Before InvocationIn the
process_order_actionmethod, when resending emails, the code assumes that the email classesWC_Email_Customer_InvoiceandWC_Email_New_Orderare available. If another plugin unhooks or removes these email classes, it could lead to errors. Consider verifying the existence of these classes before calling their methods.Run the following script to confirm that the email classes are available:
Also applies to: 252-257
includes/REST/OrderControllerV3.php (2)
38-49: Permission checks are correctly implementedThe
check_permissionmethod effectively verifies vendor permissions and returns appropriate error messages for different actions. This ensures secure access control.
85-103: Properly associating orders with the current vendorWhen creating a new order, the code correctly sets the
_dokan_vendor_idmeta to associate the order with the current vendor. This is essential for accurate order management.
| public function test_ensure_route_is_registered() { | ||
| $routes = $this->server->get_routes( $this->namespace ); | ||
| $full_route = $this->get_route( 'taxes/classes' ); | ||
|
|
||
| $this->assertArrayHasKey( $full_route, $routes ); | ||
| $this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance route registration test coverage.
The current test only verifies GET method support. Based on REST API best practices, consider adding tests for:
- Other HTTP methods (POST, PUT, DELETE) if supported
- Route parameters and their constraints
- Authentication requirements
Here's a suggested enhancement:
public function test_ensure_route_is_registered() {
$routes = $this->server->get_routes( $this->namespace );
$full_route = $this->get_route( 'taxes/classes' );
$this->assertArrayHasKey( $full_route, $routes );
// Test all supported methods
$supported_methods = ['GET', 'POST', 'PUT', 'DELETE'];
foreach ($supported_methods as $method) {
$this->assertNestedContains(
[ 'methods' => [ $method => true ] ],
$routes[ $full_route ]
);
}
// Test permission callback existence
$this->assertArrayHasKey(
'permission_callback',
$routes[ $full_route ][0]
);
}| public function test_get_tax_classes() { | ||
| wp_set_current_user( $this->seller_id1 ); | ||
|
|
||
| $response = $this->get_request( 'taxes/classes' ); | ||
|
|
||
| $this->assertEquals( 200, $response->get_status() ); | ||
| $data = $response->get_data(); | ||
| $this->assertIsArray( $data ); | ||
| } |
There was a problem hiding this comment.
Insufficient test coverage for tax classes retrieval.
The current test only verifies successful retrieval with 200 status. Consider adding tests for:
- Error scenarios (unauthorized access, invalid requests)
- Response data structure validation
- Different user roles (admin, customer, guest)
- Empty tax classes scenario
Here's a suggested enhancement:
public function test_get_tax_classes() {
// Test unauthorized access
wp_set_current_user( 0 );
$response = $this->get_request( 'taxes/classes' );
$this->assertEquals( 401, $response->get_status() );
// Test with seller
wp_set_current_user( $this->seller_id1 );
$response = $this->get_request( 'taxes/classes' );
$this->assertEquals( 200, $response->get_status() );
$data = $response->get_data();
// Validate response structure
$this->assertIsArray( $data );
foreach ( $data as $tax_class ) {
$this->assertArrayHasKey( 'name', $tax_class );
$this->assertArrayHasKey( 'slug', $tax_class );
}
}
public function test_get_tax_classes_empty() {
// Setup empty tax classes
delete_option( 'woocommerce_tax_classes' );
wp_set_current_user( $this->seller_id1 );
$response = $this->get_request( 'taxes/classes' );
$this->assertEquals( 200, $response->get_status() );
$this->assertEmpty( $response->get_data() );
}| /** | ||
| * Test getting taxes. | ||
| */ | ||
| public function test_get_taxes() { | ||
| wp_set_current_user( $this->seller_id1 ); | ||
|
|
||
| $response = $this->get_request( 'taxes' ); | ||
|
|
||
| $this->assertEquals( 200, $response->get_status() ); | ||
| $data = $response->get_data(); | ||
| $this->assertIsArray( $data ); | ||
| // Add more assertions based on expected tax data | ||
| } |
There was a problem hiding this comment.
Enhance test coverage for tax retrieval.
The current test implementation is incomplete. Consider adding:
- Test cases for different user roles (admin, customer, unauthorized)
- Validation of the tax data structure and required fields
- Error case testing (invalid requests, unauthorized access)
- Pagination and filtering tests if supported
Here's a suggested expansion of the test cases:
/**
* Test getting taxes with different user roles.
*
* @dataProvider user_role_provider
*/
public function test_get_taxes_with_different_roles($role, $expected_status) {
$user_id = $this->factory->user->create(['role' => $role]);
wp_set_current_user($user_id);
$response = $this->get_request('taxes');
$this->assertEquals($expected_status, $response->get_status());
}
/**
* Test tax data structure and required fields.
*/
public function test_get_taxes_data_structure() {
wp_set_current_user($this->seller_id1);
$response = $this->get_request('taxes');
$data = $response->get_data();
$this->assertArrayHasKey('tax_rates', $data);
foreach ($data['tax_rates'] as $tax_rate) {
$this->assertArrayHasKeys([
'id',
'country',
'state',
'rate',
'name',
'priority',
'compound',
'shipping',
'order'
], $tax_rate);
}
}
/**
* Data provider for user role tests.
*/
public function user_role_provider() {
return [
['administrator', 200],
['seller', 200],
['customer', 403],
['subscriber', 403]
];
}| /** | ||
| * Test ensuring the route is registered. | ||
| * | ||
| * @return void | ||
| */ | ||
| public function test_ensure_route_is_registered() { | ||
| $routes = $this->server->get_routes( $this->namespace ); | ||
| $full_route = $this->get_route( 'taxes' ); | ||
|
|
||
| $this->assertArrayHasKey( $full_route, $routes ); | ||
| $this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] ); | ||
| } |
There was a problem hiding this comment.
Expand route registration test coverage.
The test only verifies GET method support, but according to the PR objectives, the tax management API should support CRUD operations. Consider adding assertions for other HTTP methods (POST, PUT, DELETE) that would be needed for tax rate management.
public function test_ensure_route_is_registered() {
$routes = $this->server->get_routes( $this->namespace );
$full_route = $this->get_route( 'taxes' );
$this->assertArrayHasKey( $full_route, $routes );
- $this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] );
+ $this->assertNestedContains(
+ [
+ 'methods' => [
+ 'GET' => true,
+ 'POST' => true,
+ 'PUT' => true,
+ 'DELETE' => true
+ ]
+ ],
+ $routes[ $full_route ]
+ );
}📝 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.
| /** | |
| * Test ensuring the route is registered. | |
| * | |
| * @return void | |
| */ | |
| public function test_ensure_route_is_registered() { | |
| $routes = $this->server->get_routes( $this->namespace ); | |
| $full_route = $this->get_route( 'taxes' ); | |
| $this->assertArrayHasKey( $full_route, $routes ); | |
| $this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] ); | |
| } | |
| /** | |
| * Test ensuring the route is registered. | |
| * | |
| * @return void | |
| */ | |
| public function test_ensure_route_is_registered() { | |
| $routes = $this->server->get_routes( $this->namespace ); | |
| $full_route = $this->get_route( 'taxes' ); | |
| $this->assertArrayHasKey( $full_route, $routes ); | |
| $this->assertNestedContains( | |
| [ | |
| 'methods' => [ | |
| 'GET' => true, | |
| 'POST' => true, | |
| 'PUT' => true, | |
| 'DELETE' => true | |
| ] | |
| ], | |
| $routes[ $full_route ] | |
| ); | |
| } |
| <?php | ||
|
|
||
| namespace WeDevs\Dokan\Test\REST; | ||
|
|
||
| use WeDevs\Dokan\Test\DokanTestCase; | ||
|
|
||
| /** | ||
| * @group dokan-taxes | ||
| */ | ||
| class TaxControllerTest extends DokanTestCase { | ||
|
|
||
| protected $namespace = 'dokan/v1'; | ||
|
|
||
| /** | ||
| * Test ensuring the route is registered. | ||
| * | ||
| * @return void | ||
| */ | ||
| public function test_ensure_route_is_registered() { | ||
| $routes = $this->server->get_routes( $this->namespace ); | ||
| $full_route = $this->get_route( 'taxes' ); | ||
|
|
||
| $this->assertArrayHasKey( $full_route, $routes ); | ||
| $this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] ); | ||
| } | ||
|
|
||
| /** | ||
| * Test getting taxes. | ||
| */ | ||
| public function test_get_taxes() { | ||
| wp_set_current_user( $this->seller_id1 ); | ||
|
|
||
| $response = $this->get_request( 'taxes' ); | ||
|
|
||
| $this->assertEquals( 200, $response->get_status() ); | ||
| $data = $response->get_data(); | ||
| $this->assertIsArray( $data ); | ||
| // Add more assertions based on expected tax data | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide a final response:
Test coverage needs expansion beyond basic endpoints
After analyzing the codebase, I found that:
- Current test coverage only includes basic route registration and GET endpoints for both tax rates and tax classes
- The controllers implement permission checks that are not fully tested
- The
TaxClassControllerhas delete functionality without corresponding test coverage - Integration with orders and shipping components is not evident in the test suite
Required test additions:
- Permission check tests for both controllers
- Delete operation tests for tax classes
- Vendor-specific tax settings validation
- Integration tests with order calculations
Note: The review comment was correct in identifying gaps in test coverage. The implemented controllers have more functionality than what's currently being tested, and integration aspects are not covered in the test suite.
🔗 Analysis chain
Verify test coverage aligns with PR objectives.
The current test file only covers basic tax rate retrieval. Based on the PR objectives, please ensure test coverage for:
- Tax class management operations
- Vendor-specific tax settings
- Integration with orders and shipping components
- Permission checks for vendor authorization
Let me gather more information about the test coverage by examining the contents of these files.
This will help me verify if the test coverage aligns with the implemented functionality and PR objectives. Let me analyze the actual implementations and test coverage before making a final assessment.
Let me check for order and shipping integration tests to complete the verification.
This will help verify the integration aspects mentioned in the review comment before making the final assessment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other tax-related test files and endpoints
echo "Checking for tax-related test files..."
fd -e php -p "Tax.*Test\.php$" tests/
echo "Checking for tax-related endpoint implementations..."
rg -l "class.*Tax.*Controller" includes/
Length of output: 440
Script:
#!/bin/bash
# Check contents of tax-related test files and their corresponding controllers
echo "=== TaxClassControllerTest.php ==="
cat tests/php/src/REST/TaxClassControllerTest.php
echo -e "\n=== TaxController.php ==="
cat includes/REST/TaxController.php
echo -e "\n=== TaxClassController.php ==="
cat includes/REST/TaxClassController.php
Length of output: 4694
Script:
#!/bin/bash
# Check for tax-related code in orders and shipping tests
echo "=== Searching for tax handling in order tests ==="
rg -l "tax" tests/php/src/Orders --type php
echo -e "\n=== Searching for tax handling in shipping tests ==="
rg -l "tax" tests/php/src/Shipping --type php
echo -e "\n=== Checking for tax-related methods in order controller ==="
rg "function.*tax" includes/REST/OrderController.php
Length of output: 574
| protected function check_order_authorization( int $order_id ) { | ||
| $order = wc_get_order( $order_id ); | ||
|
|
||
| if ( ! $order ) { | ||
| return new WP_Error( 'dokan_rest_invalid_order_id', __( 'Invalid order ID.', 'dokan-lite' ), array( 'status' => 404 ) ); | ||
| } | ||
|
|
||
| if ( $order->get_meta( 'has_sub_order' ) ) { | ||
| return new WP_Error( 'dokan_rest_invalid_order', __( 'Sorry, this is a parent order', 'dokan-lite' ), array( 'status' => 404 ) ); | ||
| } | ||
|
|
||
| $vendor_id = dokan_get_seller_id_by_order( $order_id ); | ||
| if ( $vendor_id !== dokan_get_current_user_id() ) { | ||
| return new WP_Error( 'dokan_rest_unauthorized_order', __( 'You do not have permission to access this order', 'dokan-lite' ), array( 'status' => 403 ) ); | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize Order Retrieval to Avoid Redundant Calls
In the check_order_authorization method, the order is retrieved using wc_get_order( $order_id ). Later, in both the get_items and create_item methods, the same order is retrieved again using wc_get_order( $order_id ). To improve performance and reduce redundancy, consider returning the WC_Order object upon successful authorization. This way, you can reuse the order object without additional retrievals.
Apply this diff to modify the return value:
protected function check_order_authorization( int $order_id ) {
$order = wc_get_order( $order_id );
// ... existing checks ...
- return true;
+ return $order;
}Then update the get_items method:
public function get_items( $request ) {
$order_id = $request['id'];
- $auth_check = $this->check_order_authorization( $order_id );
- if ( is_wp_error( $auth_check ) ) {
- return $auth_check;
- }
-
- $order = wc_get_order( $order_id );
+ $order = $this->check_order_authorization( $order_id );
+ if ( is_wp_error( $order ) ) {
+ return $order;
+ }
$actions = $this->get_available_order_actions_for_order( $order );
return rest_ensure_response( $actions );
}Similarly, update the create_item method to utilize the returned order object.
Committable suggestion was skipped due to low confidence.
| public function get_items( $request ): WP_REST_Response { | ||
| return $this->perform_vendor_action( | ||
| function () use ( $request ) { | ||
| return parent::get_items( $request ); | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying repetitive code in CRUD methods
The CRUD methods (get_items, create_item, etc.) all use perform_vendor_action to wrap parent methods. To reduce code duplication, consider abstracting this pattern or using a loop or decorator to handle these methods dynamically.
| * | ||
| * @return WP_REST_Response | ||
| */ | ||
| public function prepare_object_for_response( $object, $request ): WP_REST_Response { |
There was a problem hiding this comment.
Avoid using reserved keyword 'object' as a parameter name
In PHP 7 and above, object is a reserved keyword. Using it as a parameter name can lead to parsing errors. Consider renaming the parameter to $order for clarity and to prevent potential issues.
Apply this diff to fix the issue:
-public function prepare_object_for_response( $object, $request ): WP_REST_Response {
+public function prepare_object_for_response( $order, $request ): WP_REST_Response {📝 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 prepare_object_for_response( $object, $request ): WP_REST_Response { | |
| public function prepare_object_for_response( $order, $request ): WP_REST_Response { |
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 286-286:
It is recommended not to use reserved keyword "object" as function parameter name. Found: $object
| $expected_total = (float) ( $product->get_price() * 2 ) * 0.95; // 10% discount | ||
| $this->assertEquals( $expected_total, $data['total'] ); |
There was a problem hiding this comment.
Fix the discount calculation in the expected total
In the test_orders_create_with_vendor_coupon method, the expected total is calculated using a multiplier of 0.95, which corresponds to a 5% discount. Since the coupon provides a 10% discount, the multiplier should be 0.9.
Apply this diff to correct the calculation:
- $expected_total = (float) ( $product->get_price() * 2 ) * 0.95; // 10% discount
+ $expected_total = (float) ( $product->get_price() * 2 ) * 0.9; // 10% discount📝 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.
| $expected_total = (float) ( $product->get_price() * 2 ) * 0.95; // 10% discount | |
| $this->assertEquals( $expected_total, $data['total'] ); | |
| $expected_total = (float) ( $product->get_price() * 2 ) * 0.9; // 10% discount | |
| $this->assertEquals( $expected_total, $data['total'] ); |
| wp_set_current_user( $this->seller_id1 ); | ||
|
|
||
| // Update commission settings for dokan. | ||
| $commission_options = array( | ||
| 'commission_type' => 'percentage', | ||
| 'admin_percentage' => 10, | ||
| 'additional_fee' => 0, | ||
| ); | ||
| update_option( 'dokan_selling', $commission_options ); | ||
|
|
||
| $order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create(); | ||
| $response = $this->get_request( 'orders/' . $order_id ); | ||
|
|
||
| $this->assertEquals( 200, $response->get_status() ); | ||
|
|
||
| $order = $response->get_data(); | ||
|
|
||
| $this->assertArrayHasKey( 'line_items', $order ); | ||
|
|
||
| foreach ( $order['line_items'] as $line_item ) { | ||
| $this->assertArrayHasKey( 'meta_data', $order ); | ||
| $this->assertGreaterThanOrEqual( 3, count( $order['meta_data'] ) ); | ||
|
|
There was a problem hiding this comment.
Correct the access to 'meta_data' in line items
Within the test_order_commission_by_vendor method, the assertions inside the loop over $order['line_items'] are accessing $order['meta_data'] instead of $line_item['meta_data']. To accurately validate each line item's metadata, the code should access $line_item['meta_data'].
Apply this diff to fix the metadata assertions:
- $this->assertArrayHasKey( 'meta_data', $order );
- $this->assertGreaterThanOrEqual( 3, count( $order['meta_data'] ) );
+ $this->assertArrayHasKey( 'meta_data', $line_item );
+ $this->assertGreaterThanOrEqual( 3, count( $line_item['meta_data'] ) );📝 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.
| wp_set_current_user( $this->seller_id1 ); | |
| // Update commission settings for dokan. | |
| $commission_options = array( | |
| 'commission_type' => 'percentage', | |
| 'admin_percentage' => 10, | |
| 'additional_fee' => 0, | |
| ); | |
| update_option( 'dokan_selling', $commission_options ); | |
| $order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create(); | |
| $response = $this->get_request( 'orders/' . $order_id ); | |
| $this->assertEquals( 200, $response->get_status() ); | |
| $order = $response->get_data(); | |
| $this->assertArrayHasKey( 'line_items', $order ); | |
| foreach ( $order['line_items'] as $line_item ) { | |
| $this->assertArrayHasKey( 'meta_data', $order ); | |
| $this->assertGreaterThanOrEqual( 3, count( $order['meta_data'] ) ); | |
| wp_set_current_user( $this->seller_id1 ); | |
| // Update commission settings for dokan. | |
| $commission_options = array( | |
| 'commission_type' => 'percentage', | |
| 'admin_percentage' => 10, | |
| 'additional_fee' => 0, | |
| ); | |
| update_option( 'dokan_selling', $commission_options ); | |
| $order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create(); | |
| $response = $this->get_request( 'orders/' . $order_id ); | |
| $this->assertEquals( 200, $response->get_status() ); | |
| $order = $response->get_data(); | |
| $this->assertArrayHasKey( 'line_items', $order ); | |
| foreach ( $order['line_items'] as $line_item ) { | |
| $this->assertArrayHasKey( 'meta_data', $line_item ); | |
| $this->assertGreaterThanOrEqual( 3, count( $line_item['meta_data'] ) ); |
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (8)
includes/REST/OrderNoteControllerV3.php (1)
91-91: Fix grammatical error in docblockIn the docblock for
delete_item_permissions_check, there's a grammatical error: "Check if a given request has access delete a order note." It should be "Check if a given request has access to delete an order note."Apply this diff to correct the error:
/** - * Check if a given request has access delete a order note. + * Check if a given request has access to delete an order note. * * @param WP_REST_Request $request Full details about the request. * @return bool|WP_Error */tests/php/src/REST/DataCountriesControllerTest.php (2)
168-185: Assert status code in concurrent requests test.In
test_concurrent_requests, consider asserting that each response has a status code of200to ensure all requests are successful, in addition to comparing the response data.Apply this diff to include the assertions:
foreach ( $responses as $response ) { + $this->assertEquals( 200, $response->get_status() ); $this->assertEquals( $first_response->get_data(), $response->get_data() ); }
96-105: Verify country code and name in empty states test.In
test_get_states_empty_states, you're verifying that thestatesarray is empty for countryFR. Consider also asserting that thecodeandnamein the response match the expected country to ensure the correct country data is returned.Apply this diff to include the assertions:
$this->assertArrayHasKey( 'states', $data ); + $this->assertEquals( 'FR', $data['code'] ); + $this->assertEquals( 'France', $data['name'] ); $this->assertEmpty( $data['states'] );tests/php/src/REST/OrderNoteControllerV3Test.php (2)
95-118: Enhance test assertions to verify note contentWhile the test checks the structure of the returned notes, it does not verify the actual content. To ensure the correct notes are retrieved, consider adding assertions to check the content of each note.
Example additions:
// Verify notes are returned $this->assertCount( 2, $data ); // Verify the content of the notes $this->assertEquals( 'Test note 1', $data[0]['note'] ); $this->assertEquals( 'Test note 2', $data[1]['note'] );
316-344: Rename test method to reflect sequential executionThe method
test_concurrent_notessuggests testing concurrent operations, but the notes are created sequentially in a loop. Since PHP executes code sequentially in this context, consider renaming the method to better reflect the test's purpose.Apply this diff to rename the method:
/** - * Test concurrent note operations + * Test multiple note creation */ -public function test_concurrent_notes() { +public function test_multiple_note_creation() {tests/php/src/REST/OrderRefundControllerV3Test.php (3)
194-210: Ensure full refund tests expect correct status codes.The
test_create_full_refundmethod expects a 500 status code when creating a full refund. Typically, a full refund should either be processed successfully (201 Created) or return a client error like 400 Bad Request if not allowed. Relying on a 500 Internal Server Error may not be appropriate as it indicates a server-side issue.Consider adjusting the test to expect a more suitable status code or handling full refund scenarios correctly in the API.
169-189: Enhance assertions for invalid refund amount tests.In the
test_create_refund_invalid_amountmethod, while you are asserting the status codes, adding assertions to check the error codes and messages will ensure that the API provides clear feedback and that the correct error handling paths are tested.
315-315: Verify refund creation before proceeding with deletion.In the
test_delete_refundmethod, after creating a refund, it's good practice to check that the refund was created successfully before attempting to delete it. This will make the test more robust and easier to debug if refund creation fails.Consider adding an assertion to verify the refund ID is valid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- includes/REST/Manager.php (1 hunks)
- includes/REST/OrderActionControllerV3.php (1 hunks)
- includes/REST/OrderNoteControllerV3.php (1 hunks)
- includes/REST/OrderRefundControllerV3.php (1 hunks)
- tests/php/src/REST/DataCountriesControllerTest.php (1 hunks)
- tests/php/src/REST/OrderActionControllerV3Test.php (1 hunks)
- tests/php/src/REST/OrderNoteControllerV3Test.php (1 hunks)
- tests/php/src/REST/OrderRefundControllerV3Test.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- includes/REST/Manager.php
🔇 Additional comments (3)
tests/php/src/REST/OrderNoteControllerV3Test.php (1)
1-345: Well-structured test class with comprehensive coverageThe
OrderNoteControllerV3Testclass is well-designed, providing thorough coverage of various scenarios related to order note management. The tests include authorization checks, input validation, successful operations, and error handling, ensuring robust verification of the controller's functionality.includes/REST/OrderActionControllerV3.php (1)
313-320: Incorrect Placement ofrequiredin JSON SchemaThe
requiredkeyword should be placed at the root level of the schema, not within theactionproperty definition. This ensures compliance with JSON Schema Draft-04 standards.tests/php/src/REST/OrderRefundControllerV3Test.php (1)
208-210:⚠️ Potential issueInvestigate unexpected 500 error when creating a full refund.
In the
test_create_full_refundmethod, asserting a 500 Internal Server Error when attempting to create a full refund might indicate an issue with the refund process. A 500 error suggests an unhandled exception or server error, which is not expected for this operation. Consider reviewing why a full refund results in a 500 error and ensure that the API handles this case gracefully, possibly with a more appropriate status code like 400 Bad Request.Please run the following script to investigate any discrepancies in handling full refunds:
Verify whether the API returns a 500 error or handles the full refund appropriately.
✅ Verification successful
Let me analyze the test file to better understand the context since the curl command didn't provide useful information. Let me generate some scripts to investigate the test case and related code.
Let me check the actual controller implementation to understand how the refund is being handled.
Based on the code analysis, I can now provide a final response regarding the 500 error assertion in the test case.
The 500 error assertion in the test is correct and expected behavior
The test is asserting a 500 error with the code 'woocommerce_rest_cannot_create_order_refund' which is the correct behavior because:
The OrderRefundControllerV3 extends WooCommerce's WC_REST_Order_Refunds_Controller and performs additional vendor permission checks before allowing refund operations.
The controller implements strict authorization checks through:
check_vendor_permission()check_order_authorization()- Various permission check methods for different operations
When a refund creation fails due to authorization or validation issues, WooCommerce's parent controller returns a 500 error with the specific error code, which is the expected behavior being tested.
The test is correctly verifying that unauthorized or invalid refund attempts are properly rejected with the appropriate error response.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if full refunds are handled properly without resulting in a server error. # Test: Attempt to create a full refund via the REST API and check the response. # Expected Result: The API should allow full refunds or return a clear error message without a 500 status code. curl -X POST "https://your-site.com/wp-json/dokan/v3/orders/{$this->order_id}/refunds" \ -H "Content-Type: application/json" \ -d '{"amount": "'$total'", "reason": "Testing full refund handling"}' \ -u seller_username:seller_password \ -iLength of output: 7712
Script:
#!/bin/bash # First, let's look at the full test method and surrounding context rg -B 5 -A 10 "test_create_full_refund" tests/php/src/REST/OrderRefundControllerV3Test.php # Then let's check the actual controller implementation fd -e php OrderRefundControllerV3.php # Also check for any related refund handling code rg -l "create.*refund|process.*refund" --type phpLength of output: 843
Script:
#!/bin/bash # Check the controller implementation for refund handling cat includes/REST/OrderRefundControllerV3.php # Also check the Order Hooks implementation since it appeared in the refund-related files cat includes/Order/Hooks.phpLength of output: 28321
| public function get_items_permissions_check( $request ) { | ||
| if ( ! $this->check_vendor_permission() ) { | ||
| return new WP_Error( 'dokan_rest_cannot_view', __( 'Sorry, you cannot list resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) ); | ||
| } | ||
|
|
||
| return $this->check_order_authorization( $request['order_id'] ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type hints for $request parameters
Consider adding type hints for the $request parameter in your methods to enhance code clarity and enable static analysis tools to catch type-related issues.
Apply this diff to add the WP_REST_Request type hint:
-public function get_items_permissions_check( $request ) {
+public function get_items_permissions_check( WP_REST_Request $request ) {
-public function create_item_permissions_check( $request ) {
+public function create_item_permissions_check( WP_REST_Request $request ) {
-public function get_item_permissions_check( $request ) {
+public function get_item_permissions_check( WP_REST_Request $request ) {
-public function delete_item_permissions_check( $request ) {
+public function delete_item_permissions_check( WP_REST_Request $request ) {
-public function create_item( $request ) {
+public function create_item( WP_REST_Request $request ) {Also applies to: 68-74, 82-88, 96-102, 119-119
| $vendor_id = dokan_get_seller_id_by_order( $order_id ); | ||
| if ( $vendor_id !== dokan_get_current_user_id() ) { | ||
| return new WP_Error( 'dokan_rest_unauthorized_order', __( 'You do not have permission to access this order', 'dokan-lite' ), array( 'status' => 403 ) ); | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Ensure $vendor_id is valid before comparison
In check_order_authorization, ensure that $vendor_id is valid before comparing it with the current user ID to prevent potential issues when $vendor_id is null or false.
Apply this diff to add a validity check for $vendor_id:
$vendor_id = dokan_get_seller_id_by_order( $order_id );
+if ( empty( $vendor_id ) ) {
+ return new WP_Error( 'dokan_rest_invalid_vendor', __( 'Invalid vendor ID.', 'dokan-lite' ), array( 'status' => 403 ) );
+}
if ( $vendor_id !== dokan_get_current_user_id() ) {
return new WP_Error( 'dokan_rest_unauthorized_order', __( 'You do not have permission to access this order', 'dokan-lite' ), array( 'status' => 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.
| $vendor_id = dokan_get_seller_id_by_order( $order_id ); | |
| if ( $vendor_id !== dokan_get_current_user_id() ) { | |
| return new WP_Error( 'dokan_rest_unauthorized_order', __( 'You do not have permission to access this order', 'dokan-lite' ), array( 'status' => 403 ) ); | |
| } | |
| return true; | |
| $vendor_id = dokan_get_seller_id_by_order( $order_id ); | |
| if ( empty( $vendor_id ) ) { | |
| return new WP_Error( 'dokan_rest_invalid_vendor', __( 'Invalid vendor ID.', 'dokan-lite' ), array( 'status' => 403 ) ); | |
| } | |
| if ( $vendor_id !== dokan_get_current_user_id() ) { | |
| return new WP_Error( 'dokan_rest_unauthorized_order', __( 'You do not have permission to access this order', 'dokan-lite' ), array( 'status' => 403 ) ); | |
| } | |
| return true; |
| public function test_get_items_success() { | ||
| wp_set_current_user( $this->seller_id1 ); | ||
| $response = $this->get_request( $this->base_route ); | ||
|
|
||
| $this->assertEquals( 200, $response->get_status() ); | ||
| $data = $response->get_data(); | ||
|
|
||
| // Verify response structure | ||
| $this->assertIsArray( $data ); | ||
| $this->assertNotEmpty( $data ); | ||
|
|
||
| // Check first country format | ||
| $first_country = reset( $data ); | ||
| $this->assertArrayHasKey( 'code', $first_country ); | ||
| $this->assertArrayHasKey( 'name', $first_country ); | ||
|
|
||
| // Verify some known countries exist | ||
| $country_codes = array_column( $data, 'code' ); | ||
| $this->assertContains( 'US', $country_codes ); | ||
| $this->assertContains( 'GB', $country_codes ); | ||
| $this->assertContains( 'CA', $country_codes ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding tests for pagination and filtering in countries endpoint.
In test_get_items_success, you're testing the retrieval of all countries. If the endpoint supports pagination or filtering, consider adding tests to verify these functionalities to ensure comprehensive coverage.
| public function test_get_states_special_characters() { | ||
| wp_set_current_user( $this->seller_id1 ); | ||
|
|
||
| $response = $this->get_request( $this->base_route . '/US@#' ); | ||
| $this->assertEquals( 404, $response->get_status() ); | ||
| } |
There was a problem hiding this comment.
Add assertion for error code in special characters test.
In test_get_states_special_characters, you assert that the status code is 404. Consider adding an assertion to verify the error code returned in the response to ensure consistent error handling.
Apply this diff to include the assertion:
$this->assertEquals( 404, $response->get_status() );
+ $this->assertEquals( 'woocommerce_rest_data_invalid_location', $response->get_data()['code'] );📝 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_get_states_special_characters() { | |
| wp_set_current_user( $this->seller_id1 ); | |
| $response = $this->get_request( $this->base_route . '/US@#' ); | |
| $this->assertEquals( 404, $response->get_status() ); | |
| } | |
| public function test_get_states_special_characters() { | |
| wp_set_current_user( $this->seller_id1 ); | |
| $response = $this->get_request( $this->base_route . '/US@#' ); | |
| $this->assertEquals( 404, $response->get_status() ); | |
| $this->assertEquals( 'woocommerce_rest_data_invalid_location', $response->get_data()['code'] ); | |
| } |
| private function perform_vendor_action( callable $action ) { | ||
| add_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] ); | ||
| $result = $action(); | ||
| remove_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] ); | ||
| return $result; | ||
| } |
There was a problem hiding this comment.
Ensure filter is always removed in perform_vendor_action
In the perform_vendor_action method, if an exception or error occurs during the execution of the $action callable, the remove_filter function may not be called, leaving the filter attached and potentially affecting other operations. To ensure that the filter is always removed, wrap the action execution within a try-finally block.
Apply this diff to modify the method:
private function perform_vendor_action( callable $action ) {
add_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] );
- $result = $action();
- remove_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] );
+ try {
+ $result = $action();
+ } finally {
+ remove_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] );
+ }
return $result;
}📝 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.
| private function perform_vendor_action( callable $action ) { | |
| add_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] ); | |
| $result = $action(); | |
| remove_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] ); | |
| return $result; | |
| } | |
| private function perform_vendor_action( callable $action ) { | |
| add_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] ); | |
| try { | |
| $result = $action(); | |
| } finally { | |
| remove_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] ); | |
| } | |
| return $result; | |
| } |
| if ( $order->get_meta( 'has_sub_order' ) ) { | ||
| return new WP_Error( 'dokan_rest_invalid_order', __( 'Sorry, this is a parent order', 'dokan-lite' ), array( 'status' => 404 ) ); | ||
| } |
There was a problem hiding this comment.
Use Appropriate HTTP Status Code
Returning a 404 Not Found status code for a parent order may not be appropriate since the order exists but cannot be accessed in this context. Consider using a 403 Forbidden status code to indicate that the user is not permitted to access parent orders.
| $this->create_refund( $this->order_id, 25 ); | ||
|
|
||
| $response = $this->get_request( "orders/{$this->order_id}/refunds" ); | ||
| var_dump( $response->get_data() ); |
There was a problem hiding this comment.
Remove var_dump debugging statement.
The var_dump statement at line 135 appears to be a leftover from debugging and should be removed to keep the test code clean and output free from unnecessary information.
Apply this diff to remove the var_dump statement:
- var_dump( $response->get_data() );📝 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.
| var_dump( $response->get_data() ); |
| ] | ||
| ); | ||
|
|
||
| var_dump( $response->get_data() ); |
There was a problem hiding this comment.
Remove var_dump debugging statement.
The var_dump statement at line 237 is unnecessary in the committed test code and should be removed to maintain code cleanliness.
Apply this diff to remove the var_dump statement:
- var_dump( $response->get_data() );📝 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.
| var_dump( $response->get_data() ); |
| [ 'force' => true ] | ||
| ); | ||
|
|
||
| var_dump( $response->get_data() ); |
There was a problem hiding this comment.
Remove var_dump debugging statement.
The var_dump statement at line 322 should be removed to prevent unwanted output during test execution.
Apply this diff to remove the var_dump statement:
- var_dump( $response->get_data() );📝 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.
| var_dump( $response->get_data() ); |
| private function create_refund( $order_id, $amount ) { | ||
| $refund = wc_create_refund( | ||
| [ | ||
| 'amount' => $amount, | ||
| 'reason' => 'Test refund', | ||
| 'order_id' => $order_id, | ||
| ] | ||
| ); | ||
|
|
||
| return $refund->get_id(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling in create_refund helper method.
The create_refund method does not handle cases where the refund creation fails (e.g., returns false or a WP_Error). Adding error handling will make tests more robust and provide clearer failure messages.
Apply this diff to include error handling:
private function create_refund( $order_id, $amount ) {
$refund = wc_create_refund(
[
'amount' => $amount,
'reason' => 'Test refund',
'order_id' => $order_id,
]
);
+ if ( is_wp_error( $refund ) ) {
+ $this->fail( 'Failed to create refund: ' . $refund->get_error_message() );
+ } elseif ( ! $refund ) {
+ $this->fail( 'Failed to create refund: Unknown error.' );
+ }
return $refund->get_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.
| private function create_refund( $order_id, $amount ) { | |
| $refund = wc_create_refund( | |
| [ | |
| 'amount' => $amount, | |
| 'reason' => 'Test refund', | |
| 'order_id' => $order_id, | |
| ] | |
| ); | |
| return $refund->get_id(); | |
| } | |
| private function create_refund( $order_id, $amount ) { | |
| $refund = wc_create_refund( | |
| [ | |
| 'amount' => $amount, | |
| 'reason' => 'Test refund', | |
| 'order_id' => $order_id, | |
| ] | |
| ); | |
| if ( is_wp_error( $refund ) ) { | |
| $this->fail( 'Failed to create refund: ' . $refund->get_error_message() ); | |
| } elseif ( ! $refund ) { | |
| $this->fail( 'Failed to create refund: Unknown error.' ); | |
| } | |
| return $refund->get_id(); | |
| } |
mralaminahamed
changed the base branch from
develop
to
update/vendor-dashboard-structure
All Submissions:
Changes proposed in this Pull Request:
This PR introduces new REST API endpoints to enhance the Dokan plugin's capabilities:
Order Management APIs (v3):
Tax Management APIs (v1):
Shipping Management APIs (v1):
Payment Gateway APIs (v1):
Each endpoint implements proper permission checks and follows WordPress REST API standards.
Related Pull Request(s)
N/A
Closes
N/A
How to test the changes in this Pull Request:
Orders:
Taxes:
Shipping:
Payment:
Changelog entry
Title: Enhanced REST API endpoints for vendor operations
Added comprehensive REST API endpoints for vendors to manage orders, taxes, shipping, and payments through standardized interfaces. New endpoints include order management with refund capabilities, tax rate configuration, shipping method setup, and payment gateway integration.
Before Changes
Vendors had limited API access to manage core functionalities like orders, taxes, shipping, and payments.
After Changes
Vendors can now programmatically:
All endpoints implement proper authorization and follow WordPress REST API best practices.
PR Self Review Checklist:
Summary by CodeRabbit
Release Notes
New Features
Tests