Refactor/vendor balance raw query #2430
Conversation
WalkthroughThe changes in this pull request introduce several new classes and methods within the Dokan framework, focusing on data management for vendor balances and the overall data store structure. Key additions include the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
🧰 Additional context used🪛 LanguageTooldocs/data-store.md[uncategorized] ~20-~20: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION) [uncategorized] ~105-~105: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION) [uncategorized] ~106-~106: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION) [typographical] ~106-~106: Two consecutive dots (DOUBLE_PUNCTUATION) [style] ~165-~165: ‘New Record’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_NEW_RECORD) [uncategorized] ~191-~191: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION) [uncategorized] ~192-~192: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION) 🪛 GitHub Check: Run PHPCS inspectionincludes/Models/DataStore/BaseDataStore.php[warning] 82-82: [warning] 88-88: [warning] 88-88: [warning] 257-257: includes/Models/DataStore/VendorBalanceStore.php[warning] 89-89: tests/php/src/CustomAssertion/DBAssertionTrait.php[warning] 81-81: 🔇 Additional comments (13)includes/Models/DataStore/DataStoreInterface.php (2)
The namespace follows PSR-4 autoloading standard and the BaseModel import is appropriate for the interface requirements.
The interface includes commented-out methods (get, query, count) that seem essential for data retrieval operations. Please clarify:
includes/Order/Hooks.php (2)
The import statement for the
The replacement of direct database queries with the
Let's verify that this refactoring pattern is consistently applied across the codebase: ✅ Verification successfulLet me check the actual implementation of these usages to ensure consistency. Let me check one final thing to ensure there are no direct SQL updates in the VendorBalance class itself. Based on the comprehensive analysis, I can now provide the final response: Refactoring pattern is consistently applied across the codebase The VendorBalance model is properly encapsulating all balance updates through its model layer:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for any remaining direct updates to dokan_vendor_balance table
rg "UPDATE.*dokan_vendor_balance" --type php
# Search for other usages of VendorBalance::update_by_transaction
rg "VendorBalance::update_by_transaction" --type php
Length of output: 348 Script: #!/bin/bash
# Check the implementation context of VendorBalance::update_by_transaction calls
rg "VendorBalance::update_by_transaction" -B 5 -A 5 --type php
# Also check if there are any other methods in VendorBalance class that might be doing balance updates
ast-grep --pattern 'class VendorBalance {
$$$
public static function $_($$$) {
$$$
UPDATE $$$
$$$
}
$$$
}'
Length of output: 2252 Script: #!/bin/bash
# Check the VendorBalance class implementation
rg "class VendorBalance" -A 50 --type php
# Also check for any remaining raw SQL updates in the codebase
rg "wpdb.*UPDATE.*balance" --type php
Length of output: 10645 includes/Vendor/Vendor.php (1)
Let's ensure the VendorBalance model is properly integrated across the codebase. ✅ Verification successfulLet me gather more information about the VendorBalance model implementation. Let me check one final aspect to verify the complete integration. VendorBalance model integration is properly implemented and consistent The VendorBalance model is well-integrated across the codebase with:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify VendorBalance model implementation and usage
# Test 1: Check if VendorBalance model exists
echo "Checking VendorBalance model implementation..."
ast-grep --pattern 'class VendorBalance'
# Test 2: Check usage of get_total_balance_by_vendor method
echo "Checking get_total_balance_by_vendor usage..."
rg "VendorBalance::get_total_balance_by_vendor"
# Test 3: Check for any remaining raw vendor balance queries
echo "Checking for remaining raw vendor balance queries..."
rg "dokan_vendor_balance"
Length of output: 3063 Script: #!/bin/bash
# Check VendorBalance model implementation and its methods
echo "Checking VendorBalance model implementation..."
ast-grep --pattern 'class VendorBalance {
$$$
}'
# Check for references to VendorBalance in test files
echo "Checking test coverage..."
rg -A 5 "class VendorBalanceModelTest"
# Check for any direct database queries that might bypass the model
echo "Checking direct balance queries..."
rg -A 2 "SELECT.*dokan_vendor_balance" --type php
Length of output: 1435 Script: #!/bin/bash
# Check VendorBalance model's store implementation
echo "Checking VendorBalance store implementation..."
ast-grep --pattern 'class VendorBalanceStore {
$$$
}'
# Check for model usage in other classes
echo "Checking VendorBalance usage..."
rg "use WeDevs\\Dokan\\Models\\VendorBalance"
# Check for any model method calls
echo "Checking model method calls..."
rg "VendorBalance::" -A 2
Length of output: 5378 includes/Models/DataStore/VendorBalanceStore.php (3)
In the Please review the format specifiers:
Confirm that each field's data type matches its format specifier to prevent potential data handling issues.
You are appending $earning_status = dokan_withdraw_get_active_order_status();
$earning_status[] = 'approved';Ensure that the Consider checking: if ( ! in_array( 'approved', $earning_status, true ) ) {
$earning_status[] = 'approved';
}
The 🧰 Tools🪛 GitHub Check: Run PHPCS inspection[warning] 89-89: includes/Models/VendorBalance.php (1)
The cache key and group in $cache_group = "seller_order_data_{$vendor_id}";
$cache_key = "seller_earnings_{$vendor_id}_{$on_date->format('Y_m_d')}";Verify that these keys are unique enough to prevent collisions and that they align with the caching strategy used elsewhere in the application. Run the following script to check for potential cache key collisions: This will help identify any overlapping cache keys that might cause unexpected behavior. ✅ Verification successfulLet me correct the syntax and search more broadly for cache-related patterns to verify the caching strategy. Based on the analysis of the cache usage patterns in the codebase, I can now provide a final response: Cache keys are well-structured and follow consistent patterns The cache implementation in
The codebase shows similar patterns in other vendor-related caching:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Search for cache key usages to ensure uniqueness.
# Expectation: Cache keys for vendor earnings are unique and consistent.
rg -A 2 "Cache::(get|set)" --php
Length of output: 442 Script: #!/bin/bash
# Search for cache key usages to ensure uniqueness
rg "Cache::(get|set)" -A 2
# Also search for alternative cache implementations
rg "cache_[gs]et|wp_cache|transient" -A 2
Length of output: 36476 includes/Models/DataStore/BaseDataStore.php (3)
The method Consider reviewing the 🧰 Tools🪛 GitHub Check: Run PHPCS inspection[warning] 88-88: [warning] 88-88:
Ensure Correct Format Handling in The static analysis tool warns about replacement variables without valid placeholders. Verify that the Ensure that the
Potential SQL Injection Risk in When constructing the WHERE clause, column names ( Apply this diff to sanitize the column names: // Generate the WHERE clause securely using $wpdb->prepare() with placeholders.
- $where[] = $wpdb->prepare( "{$key} IN ({$placeholders})", ...$value );
+ $where[] = $wpdb->prepare( "%i IN ({$placeholders})", $key, ...$value );Alternatively, ensure that
🧰 Tools🪛 GitHub Check: Run PHPCS inspection[warning] 257-257: includes/Order/functions.php (1)
The import of 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 (
|
![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.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (36)
includes/Models/DataStore/DataStoreInterface.php (3)
7-12: Replace "DOKAN_SINCE" placeholder with actual version number.The @SInCE tag contains a placeholder "DOKAN_SINCE" which should be replaced with the actual version number where this interface is introduced.
25-34: Fix incorrect documentation in read() method.The PHPDoc comment mentions "download permission" which appears to be copied from another context and should be updated to reflect the generic nature of this interface.
23-58: Enhance type safety and exception handling documentation.Consider the following improvements:
- Add return type declarations to methods (void)
- Standardize exception documentation:
- Use
@throwsinstead of@throw- Specify the exception class being thrown
Here's how the methods should be declared:
- public function create( BaseModel &$model ); + public function create( BaseModel &$model ): void; - public function read( BaseModel &$model ); + public function read( BaseModel &$model ): void; - public function update( BaseModel &$model ); + public function update( BaseModel &$model ): void; - public function delete( BaseModel &$model ); + public function delete( BaseModel &$model ): void;tests/php/src/CustomAssertion/DBAssertionTrait.php (1)
78-78: Add PHPDoc documentation for the new method.For consistency with other methods in the trait, please add a PHPDoc block documenting the purpose, parameters, and return type of the
assertDatabaseMissingmethod.Add this documentation block above the method:
+ /** + * Assert that a table does not contain any rows matching the specified criteria. + * + * @param string $table The name of the table (without the prefix). + * @param array $data An associative array of field-value pairs to match. + * @return void + */ public function assertDatabaseMissing( string $table, array $data = [] ): void {docs/data-store.md (6)
9-13: Enhance the overview section with WooCommerce context.The overview effectively introduces the data layer abstraction. Consider adding a brief explanation of how this architecture aligns with or differs from WooCommerce's data layer, since you're following a similar pattern to
WC_Product.
90-90: Fix parameter name in PHPDoc.The parameter name in the PHPDoc comment is incorrect. It shows
$date_createdbut the method uses$date_updated.-* @param string|int|DateTime $date_created +* @param string|int|DateTime $date_updated
54-56: Add validation in setter methods.Consider adding validation in the setter methods to ensure data integrity. For example, the
set_namemethod should validate that the name is not empty and meets any length requirements.public function set_name( $name ) { + if ( empty( $name ) ) { + throw new \InvalidArgumentException( 'Department name cannot be empty.' ); + } + if ( strlen( $name ) > 100 ) { + throw new \InvalidArgumentException( 'Department name cannot exceed 100 characters.' ); + } return $this->set_prop( 'name', $name ); }
155-163: Remove unnecessary empty lines.Multiple consecutive empty lines don't add value to the documentation. Consider removing them to maintain consistency.
165-183: Enhance usage examples with error handling.Consider adding examples that demonstrate:
- Error handling for failed operations
- Bulk operations
- Common queries and filters
Example addition:
try { $department = new Department( $department_id ); $department->set_name('Department 2'); $department->save(); } catch (\InvalidArgumentException $e) { // Handle validation errors } catch (\Exception $e) { // Handle other errors }🧰 Tools
🪛 LanguageTool
[style] ~165-~165: ‘New Record’ might be wordy. Consider a shorter alternative.
Context: ... ## Uses of Models ### Create a New Record ```php $department = new Department(); ...(EN_WORDINESS_PREMIUM_NEW_RECORD)
188-192: Consider fixing the database field typo.While the current solution elegantly handles the
perticularstypo through the data store layer, it would be better to fix the typo in the database schema to prevent confusion and maintain code quality.Would you like help creating a migration script to fix this typo?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~191-~191: Loose punctuation mark.
Context: ..._to_db_data( BaseModel &$model ): array`: Prepare data for saving a BaseModel to ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~192-~192: Loose punctuation mark.
Context: ...e database. -map_db_raw_to_model_data: Maps database raw data to model data.(UNLIKELY_OPENING_PUNCTUATION)
tests/php/src/Models/VendorBalanceModelTest.php (4)
8-8: Remove unused import.The
WeDevs\DokanPro\Modules\DeliveryTime\StorePickup\Vendorclass is imported but never used in the test file.
15-32: Uncomment the debit assertion and fix formatting.
- The commented out debit assertion should be included to ensure the debit value is correctly persisted:
$this->assertDatabaseCount( 'dokan_vendor_balance', 1, [ 'id' => $vendor_balance->get_id(), - // 'debit' => 100, + 'debit' => 100, 'trn_id' => 1, 'trn_type' => VendorBalance::TRN_TYPE_DOKAN_ORDERS, ] );
- The assertion parameters should be properly aligned for better readability.
63-88: Consider using data providers for comprehensive test coverage.The test thoroughly verifies all fields, but could be enhanced by using PHPUnit data providers to test different scenarios (e.g., different dates, statuses, transaction types). This would make the test more maintainable and comprehensive.
Example structure:
/** * @dataProvider provideVendorBalanceData */ public function test_read_method($particulars, $debit, $status, $date) { // Test implementation } public function provideVendorBalanceData() { return [ 'completed_order' => ['Order #123', 100, 'wc-completed', '2020-01-01'], 'pending_order' => ['Order #124', 200, 'wc-pending', '2020-01-02'], // Add more test cases ]; }
90-110: Enhance test coverage for update_by_transaction method.The test only verifies updating the debit amount. Consider adding test cases for:
- Updating other fields (credit, status, etc.)
- Error cases (invalid transaction ID, type)
- Edge cases (zero amounts, null values)
includes/Commission.php (4)
9-9: Remove unused import.The
Vendorclass is imported but not used in the code.-use WeDevs\DokanPro\Modules\DeliveryTime\StorePickup\Vendor;
102-105: LGTM! Consider extracting the transaction type to a constant.The refactoring improves code maintainability by moving database operations to the VendorBalance model. However, the transaction type 'dokan_orders' could be extracted to a constant for better maintainability.
+ private const TRANSACTION_TYPE_ORDERS = 'dokan_orders'; + public function calculate_gateway_fee( $order_id ) { // ... existing code ... VendorBalance::update_by_transaction( $tmp_order->get_id(), - 'dokan_orders', + self::TRANSACTION_TYPE_ORDERS, [ 'debit' => (float) $net_amount ] );
Line range hint
13-24: Fix indentation in if-else blocks.The indentation in the if-else blocks is inconsistent, making the code harder to read.
if loyalty_years >= 5: discount = 0.15 - elif loyalty_years >= 3: - discount = 0.1 - else: - discount = 0 - discounted_amount = amount - (amount * discount) - if discount > 0: - final_amount = discounted_amount + 20 - else: - final_amount = discounted_amount - return final_amount + elif loyalty_years >= 3: + discount = 0.1 + else: + discount = 0 + discounted_amount = amount - (amount * discount) + if discount > 0: + final_amount = discounted_amount + 20 + else: + final_amount = discounted_amount + return final_amount
Based on the search results, I can now provide a final response since we have found all occurrences of the misspelled methods:
Fix typo in method names
set_order_qunatityandget_order_qunatityThe following methods in
includes/Commission.phphave spelling errors and need to be renamed to use the correct spelling "quantity":
set_order_qunatity()→set_order_quantity()get_order_qunatity()→get_order_quantity()The misspelled methods are used in 6 locations within the same file:
- Method declarations (lines 99-108)
- Usage in
get_earning_by_order()method- Usage in
prepare_for_calculation()method🔗 Analysis chain
Line range hint
1-1000: Fix typo in method names.The methods
set_order_qunatityandget_order_qunatityhave a typo in their names (should be "quantity" instead of "qunatity").Let's verify the usage of these methods to ensure we catch all occurrences:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all occurrences of the misspelled methods rg -i "qunatity" --type phpLength of output: 562
includes/Vendor/Vendor.php (2)
790-790: LGTM! Good refactoring of vendor balance logic.The change effectively moves the vendor balance calculation to the
VendorBalancemodel, improving code organization and maintainability.This architectural change:
- Improves separation of concerns by moving balance logic to a dedicated model
- Makes the code more maintainable by centralizing vendor balance calculations
- Reduces duplication by reusing the
VendorBalancemodel's functionality
Line range hint
813-875: Consider refactoring get_balance method to use VendorBalance model.For consistency with the
get_earningsrefactoring, consider moving the balance calculation logic to theVendorBalancemodel in a future update.Benefits:
- Maintains consistency with the new architecture
- Centralizes all balance-related calculations in the VendorBalance model
- Makes the code more maintainable
includes/Models/BaseModel.php (6)
14-14: Remove the unnecessary comment on line 14The comment
// wc_get_product()appears unrelated to thesave()method and may have been left unintentionally. Consider removing it to keep the code clean.
23-23: Correct the typographical error "THe" to "The" in documentationIn the docblocks on lines 23 and 37, "THe data store" should be corrected to "The data store" for proper grammar.
Also applies to: 37-37
47-47: Typo in parameter description: "date" should be "data"In the documentation for the
deletemethod, correct the typo in the parameter description:- * @param bool $force_delete Should the date be deleted permanently. + * @param bool $force_delete Should the data be deleted permanently.
55-55: Correct the type hint in documentationIn the docblock, replace
DatawithWC_Datafor accurate type hinting:- * @param Data $this The data object being deleted. + * @param WC_Data $this The data object being deleted.
76-76: Typographical error in documentation: "raws" should be "rows"In the docblock summary for the
delete_bymethod, correct the typo:- * Delete raws from the database. + * Delete rows from the database.
78-78: Ensure consistent quoting in array key examplesIn the parameter description for the
delete_bymethod, add quotes around'status'for consistency:- * @param array $data Array of args to delete an object, e.g. `array( 'id' => 1, status => ['draft', 'cancelled'] )` or `array( 'id' => 1, 'status' => 'publish' )`. + * @param array $data Array of args to delete an object, e.g. `array( 'id' => 1, 'status' => ['draft', 'cancelled'] )` or `array( 'id' => 1, 'status' => 'publish' )`.includes/Models/VendorBalance.php (3)
141-143: Validate transaction type inset_trn_typemethodThe
set_trn_typemethod currently does not validate the transaction type being set. It's important to ensure that only valid transaction types are assigned to prevent potential errors or inconsistencies.Consider adding validation to check that
$typeis one of the allowed constants:if ( ! in_array( $type, [ self::TRN_TYPE_DOKAN_ORDERS, self::TRN_TYPE_DOKAN_WITHDRAW, self::TRN_TYPE_DOKAN_REFUND ], true ) ) { throw new \InvalidArgumentException( 'Invalid transaction type provided.' ); } $this->set_prop( 'trn_type', $type );This ensures that only predefined transaction types are accepted and helps maintain data integrity.
49-52: Review redundant call to$this->set_id( $id )in the constructorIn the constructor, the ID is being set twice: once in the parent constructor and again using
$this->set_id( $id );. This may be redundant if the parent constructor already handles the ID assignment.Verify if the parent
BaseModelconstructor sets the ID. If it does, you can remove the redundant call:public function __construct( int $id = 0 ) { parent::__construct( $id ); - $this->set_id( $id ); $this->data_store = apply_filters( $this->get_hook_prefix() . 'data_store', new VendorBalanceStore() ); if ( $this->get_id() > 0 ) { $this->data_store->read( $this ); } }This simplifies the constructor and avoids unnecessary method calls.
221-223: Consider adding validation for status inset_statusmethodThe
set_statusmethod does not currently validate the status being set. To prevent invalid statuses from being assigned, it's advisable to implement validation.Add validation to check if
$statusis one of the allowed values:$allowed_statuses = ['pending', 'completed', 'cancelled']; // Replace with actual allowed statuses if ( ! in_array( $status, $allowed_statuses, true ) ) { throw new \InvalidArgumentException( 'Invalid status provided.' ); } $this->set_prop( 'status', $status );This ensures consistency and reliability in the status values assigned to vendor balances.
Also applies to: 231-233
includes/Models/DataStore/BaseDataStore.php (6)
7-7: Remove Unused Use StatementAutomattic\WooCommerce\Pinterest\API\Base.The use statement for
Automattic\WooCommerce\Pinterest\API\Baseis not utilized within this file. Removing it will clean up the code and prevent any confusion.Apply this diff to remove the unused use statement:
use Automattic\WooCommerce\Admin\API\Reports\SqlQuery; -use Automattic\WooCommerce\Pinterest\API\Base; use Exception;
57-59: Selective PHPCS Disablement Recommended.The comment
@phpcs:disable WordPress.DB.PreparedSQL.InterpolatedNotPrepareddisables PHPCS checks for the entire method. It's better to disable it only for the specific lines where necessary to avoid missing other potential issues.Apply this change to selectively disable PHPCS:
- * @phpcs:disable WordPress.DB.PreparedSQL.InterpolatedNotPrepared * + * @phpcs:disable WordPress.DB.PreparedSQL.InterpolatedNotPrepared.WordPress * @throws Exception Throw exception if invalid entity is passed. */Then, re-enable PHPCS after the specific code block.
310-312: UseinstanceofOperator for Type Checking.Using
is_a()for type checking can be less performant and is generally discouraged in favor of theinstanceofoperator.Apply this diff to use
instanceof:if ( is_a( $val, 'WC_DateTime' ) ) { + if ( $val instanceof \WC_DateTime ) { $val = $val->date( $this->get_date_format_for_field( $db_field_name ) ); }This provides a more standard and efficient type check.
403-405: Remove Redundant Comment inget_id_field_name().The comment
// 'id';seems redundant and may have been left unintentionally.Apply this diff to remove the redundant comment:
return apply_filters( $this->get_hook_prefix() . 'id_field_name', 'id' ); // 'id'; - return apply_filters( $this->get_hook_prefix() . 'id_field_name', 'id' );
65-67: Provide More Descriptive Exception Messages.The exception message
'Invalid entity item.'may not provide sufficient information for debugging.Consider enhancing the exception message to include the entity ID or other relevant details, ensuring sensitive information is not exposed.
$message = $this->get_hook_prefix() . ' : ' . __( 'Invalid entity item with ID: ', 'dokan-lite' ) . $model->get_id();
349-351: Unused Parameter$db_field_nameinget_date_format_for_field().The parameter
$db_field_nameis not utilized within the method. If this method is intended to support different date formats for different fields, implement the necessary logic; otherwise, consider removing the parameter.Apply this diff to remove the unused parameter:
- protected function get_date_format_for_field( string $db_field_name ): string { + protected function get_date_format_for_field(): string { return 'Y-m-d H:i:s'; }Also, update any method calls accordingly.
includes/Order/functions.php (1)
273-281: Localize the 'New order' stringTo support internationalization, wrap the 'New order' string in a localization function to allow for translations.
Apply this diff to localize the string:
- $vendor_balance->set_particulars( 'New order' ); + $vendor_balance->set_particulars( __( 'New order', 'dokan-lite' ) );
🛑 Comments failed to post (17)
tests/php/src/CustomAssertion/DBAssertionTrait.php (2)
79-81:
⚠️ Potential issueFix incorrect assertion logic.
The current implementation uses
assertGreaterThanOrEqual(0, $rows_count)which will always pass since a count cannot be negative. To assert that records are missing, we should verify that the count equals zero.Apply this fix:
public function assertDatabaseMissing( string $table, array $data = [] ): void { $rows_count = $this->getDatabaseCount( $table, $data ); - $this->assertGreaterThanOrEqual( 0, $rows_count, "{$rows_count} rows found in `$table` for given data " . json_encode( $data ) ); + $this->assertEquals( 0, $rows_count, "Found {$rows_count} unexpected rows in `$table` matching " . json_encode( $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.$rows_count = $this->getDatabaseCount( $table, $data ); $this->assertEquals( 0, $rows_count, "Found {$rows_count} unexpected rows in `$table` matching " . json_encode( $data ) );🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 81-81:
json_encode() is discouraged. Use wp_json_encode() instead.
81-81: 💡 Codebase verification
🛠️ Refactor suggestion
Replace all instances of json_encode() with wp_json_encode() in DBAssertionTrait
Found 3 instances where
json_encode()should be replaced withwp_json_encode():
- Line:
$this->assertGreaterThanOrEqual( 1, $rows_count, "No rows found in$tablefor given data " . json_encode( $data ) );- Line:
$this->assertEquals( $count, $rows_count, "No rows found in$tablefor given data " . json_encode( $data ) );- Line:
$this->assertGreaterThanOrEqual( 0, $rows_count, "{$rows_count} rows found in$tablefor given data " . json_encode( $data ) );🔗 Analysis chain
Use wp_json_encode() instead of json_encode().
WordPress provides
wp_json_encode()as a safer alternative tojson_encode(). It includes additional checks and filters.Let's check for other occurrences of
json_encode()in the trait:Apply this change consistently across all methods:
- $this->assertGreaterThanOrEqual( 0, $rows_count, "{$rows_count} rows found in `$table` for given data " . json_encode( $data ) ); + $this->assertGreaterThanOrEqual( 0, $rows_count, "{$rows_count} rows found in `$table` for given data " . wp_json_encode( $data ) );🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other json_encode usages in this file rg "json_encode" "tests/php/src/CustomAssertion/DBAssertionTrait.php"Length of output: 452
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 81-81:
json_encode() is discouraged. Use wp_json_encode() instead.docs/data-store.md (1)
116-127:
⚠️ Potential issueFix inconsistent field names in DepartmentStore.
There's an inconsistency in the field names:
get_fields_with_formatusesupdated_at- The getter method is named
get_date_updated- The model uses
date_updatedThis inconsistency could lead to issues with data mapping.
public function get_fields_with_format(): array { return [ 'name' => '%s', 'date_created' => '%s', - 'updated_at' => '%s', + 'date_updated' => '%s', ]; }📝 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_fields_with_format(): array { return [ 'name' => '%s', 'date_created' => '%s', 'date_updated' => '%s', ]; } public function get_date_updated( $context = 'view' ) { return $this->get_prop('date_updated', $context); } }tests/php/src/Models/VendorBalanceModelTest.php (4)
112-137: 🛠️ Refactor suggestion
Add tearDown cleanup for Mockery.
To prevent memory leaks and ensure proper test isolation, add Mockery cleanup in tearDown:
protected function tearDown(): void { Mockery::close(); parent::tearDown(); }
13-13: 🛠️ Refactor suggestion
Improve test data management.
Consider adding:
- A setUp method to handle common test data
- Helper methods for creating vendor balance objects
- Constants for common test values
Example:
private const TEST_VENDOR_ID = 1; private const TEST_DEBIT = 100; protected function setUp(): void { parent::setUp(); $this->vendor_balance = $this->createTestVendorBalance(); } private function createTestVendorBalance(array $overrides = []): VendorBalance { $defaults = [ 'particulars' => 'test', 'debit' => self::TEST_DEBIT, 'vendor_id' => self::TEST_VENDOR_ID, // ... other default values ]; $data = array_merge($defaults, $overrides); $balance = new VendorBalance(); foreach ($data as $key => $value) { $method = "set_$key"; $balance->$method($value); } return $balance; }
34-50:
⚠️ Potential issueFix spelling error in column name.
The column name 'perticulars' is misspelled in the assertion. It should be 'particulars':
$this->assertDatabaseHas( 'dokan_vendor_balance', [ - 'perticulars' => 'test123', + 'particulars' => 'test123', ] );📝 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_update_method() { $vendor_balance = new VendorBalance(); $vendor_balance->save(); $this->assertEmpty( $vendor_balance->get_particulars() ); // Change the particulars $vendor_balance->set_particulars( 'test123' ); $vendor_balance->save(); // Assert changes are applied to the database. $this->assertDatabaseHas( 'dokan_vendor_balance', [ 'particulars' => 'test123', ] ); }
139-181:
⚠️ Potential issueUse exact assertion for balance calculation.
The current test uses
assertGreaterThan(110, $total_balance)which could pass even if the calculation is incorrect. Since we know the exact expected value from the transactions:
- Order 1: +100 (debit)
- Order 2: +60 (debit)
- Refund: -20 (credit)
- Withdraw: -30 (credit)
Total: 110Replace with an exact assertion:
- $this->assertGreaterThan(110, $total_balance); + $this->assertEquals(110, $total_balance);📝 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_total_balance_by_vendor_method() { $trn_id = 1; $vendor_balance = new VendorBalance(); $vendor_balance->set_particulars( 'test' ); $vendor_balance->set_debit( 100 ); $vendor_balance->set_status( 'wc-completed' ); $vendor_balance->set_trn_id( $trn_id++ ); $vendor_balance->set_trn_type( VendorBalance::TRN_TYPE_DOKAN_ORDERS ); $vendor_balance->set_vendor_id( $this->seller_id1 ); $vendor_balance->set_trn_date( '2020-01-01' ); $vendor_balance->set_balance_date( '2020-01-01' ); $vendor_balance->save(); $vendor_balance->set_id( 0 ); $vendor_balance->set_debit( 0 ); $vendor_balance_2 = clone $vendor_balance; $vendor_balance_2->set_trn_id( $trn_id++ ); $vendor_balance_2->set_debit( 60 ); $vendor_balance_2->save(); $vendor_balance_refund = clone $vendor_balance; $vendor_balance_refund->set_trn_id( $trn_id++ ); $vendor_balance_refund->set_trn_type( VendorBalance::TRN_TYPE_DOKAN_REFUND ); $vendor_balance_refund->set_status( 'approved' ); $vendor_balance_refund->set_credit( 20 ); $vendor_balance_refund->save(); $vendor_balance_withdraw = clone $vendor_balance; $vendor_balance_withdraw->set_trn_id( $trn_id++ ); $vendor_balance_withdraw->set_trn_type( VendorBalance::TRN_TYPE_DOKAN_WITHDRAW ); $vendor_balance_withdraw->set_status( 'approved' ); $vendor_balance_withdraw->set_credit( 30 ); $vendor_balance_withdraw->save(); $total_balance = VendorBalance::get_total_balance_by_vendor( $this->seller_id1 ); // 100 + 60 - 20 - 30 = 110 $this->assertEquals(110, $total_balance); }includes/Models/DataStore/VendorBalanceStore.php (2)
68-68:
⚠️ Potential issueSecurity Concern: Use
$wpdb->prepare()for SQL Query ConstructionThe SQL query in
get_total_balance()incorporates dynamic variables that, while partially sanitized usingesc_sql(), may still be vulnerable to SQL injection. It's recommended to use$wpdb->prepare()for all dynamic parts of the query to ensure proper sanitization.Consider refactoring the query to utilize placeholders with
$wpdb->prepare(). Here's how you might adjust the code:Replace the construction of
$statuses_strand$trn_types_str:- $statuses_str = "'" . implode( "', '", esc_sql( $earning_status ) ) . "'"; - $trn_types_str = "'" . implode( "', '", esc_sql( $trn_types ) ) . "'"; + $statuses_placeholders = implode( ',', array_fill( 0, count( $earning_status ), '%s' ) ); + $trn_types_placeholders = implode( ',', array_fill( 0, count( $trn_types ), '%s' ) );Modify the
whereclauses to use$wpdb->prepare():- $this->add_sql_clause( 'where', " AND trn_type IN ($trn_types_str)" ); - $this->add_sql_clause( 'where', " AND status IN ($statuses_str)" ); + $this->add_sql_clause( 'where', $wpdb->prepare( " AND trn_type IN ( $trn_types_placeholders )", $trn_types ) ); + $this->add_sql_clause( 'where', $wpdb->prepare( " AND status IN ( $statuses_placeholders )", $earning_status ) );By incorporating these changes, you ensure that all dynamic values are properly escaped, enhancing the security of your SQL queries.
Also applies to: 76-84, 86-91
24-24:
⚠️ Potential issueTypographical Error: Correct 'perticulars' to 'particulars'
The term 'perticulars' appears to be misspelled throughout the code. The correct spelling is 'particulars'. Ensuring accurate spelling enhances code readability and prevents confusion.
Apply the following changes to correct the spelling:
Update the field name in
get_fields_with_format():- 'perticulars' => '%s', + 'particulars' => '%s',Update the method name and its usage:
- protected function get_perticulars( VendorBalance $model, string $context = 'edit' ): string { + protected function get_particulars( VendorBalance $model, string $context = 'edit' ): string { return $model->get_particulars( $context ); }Update the documentation comments:
- * Used to get perticulars through the get_perticulars method by the DataStore. + * Used to get particulars through the get_particulars method by the DataStore.Also applies to: 43-44, 49-50
includes/Models/BaseModel.php (1)
60-60:
⚠️ Potential issueValidate
$this->object_typebefore using in filter nameEnsure that
$this->object_typecontains only expected values to prevent any unintended behavior or security issues when constructing the filter name inapply_filters. Consider sanitizing or validating the property before use.includes/Models/VendorBalance.php (2)
36-36:
⚠️ Potential issueCorrect the misspelling of 'perticulars' to 'particulars'
The property name 'perticulars' is misspelled throughout the code. The correct spelling is 'particulars'. This misspelling can lead to confusion and potential bugs when other developers interact with the code.
Apply the following diff to correct the misspellings:
- 'perticulars' => '', + 'particulars' => '',Update all occurrences in method definitions and usages:
- return $this->get_prop( 'perticulars', $context ); + return $this->get_prop( 'particulars', $context ); - $this->set_prop( 'perticulars', $note ); + $this->set_prop( 'particulars', $note );Ensure that all references to 'perticulars' are updated to 'particulars' throughout the codebase.
Also applies to: 151-153, 161-163, 171-173
284-284:
⚠️ Potential issuePotential issue with date modification using
modify( $on_date )The usage of
modify( $on_date )on thedokan_current_datetime()object may not set$on_dateto the intended date. Instead, it may modify the current datetime relative to$on_date, leading to unexpected results.Consider creating a new
DateTimeinstance with the provided date:- $on_date = $on_date && strtotime( $on_date ) ? dokan_current_datetime()->modify( $on_date ) : dokan_current_datetime(); + $on_date = $on_date && strtotime( $on_date ) ? dokan_current_datetime( new \DateTime( $on_date ) ) : dokan_current_datetime();Or directly instantiate a
DateTimeobject:- $on_date = $on_date && strtotime( $on_date ) ? dokan_current_datetime()->modify( $on_date ) : dokan_current_datetime(); + $on_date = $on_date && strtotime( $on_date ) ? new \DateTime( $on_date ) : dokan_current_datetime();Ensure that
$on_dateaccurately represents the intended date for balance calculation.Committable suggestion skipped: line range outside the PR's diff.
includes/Models/DataStore/BaseDataStore.php (4)
257-257: 🛠️ Refactor suggestion
PHPCS Warning: Replacement Variables Without Valid Placeholders.
The static analysis tool warns about replacement variables without valid placeholders. Ensure that all placeholders in the query match the variables provided.
Review the placeholders in the
$wpdb->prepare()statement to match the number and type of variables.🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 257-257:
Replacement variables found, but no valid placeholders found in the query.
198-205:
⚠️ Potential issueCorrect Variable Name for Field Formats.
There is an inconsistency in variable naming.
$field_formatis used instead of$fields_format, which may lead to undefined variable issues.Apply this diff to correct the variable name:
$fields_format = $this->get_fields_with_format(); - $field_format[ $this->get_id_field_name() ] = $this->get_id_field_format(); + $fields_format[ $this->get_id_field_name() ] = $this->get_id_field_format(); $data_format = [];Ensure that
$fields_formatis used consistently throughout the method.📝 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.$fields_format = $this->get_fields_with_format(); $fields_format[ $this->get_id_field_name() ] = $this->get_id_field_format(); $data_format = []; foreach ( $data_to_update as $key => $value ) { $data_format[] = $fields_format[ $key ]; }
394-396:
⚠️ Potential issueCompatibility Issue with
str_starts_with().The function
str_starts_with()is available from PHP 8.0 onwards. If the project supports older PHP versions, this could cause compatibility issues.Apply this diff to use
strpos()instead:if ( ! str_starts_with( $table_name, $wpdb->prefix ) ) { + if ( strpos( $table_name, $wpdb->prefix ) !== 0 ) { $table_name = $wpdb->prefix . $table_name; }This ensures compatibility with PHP versions earlier than 8.0.
Committable suggestion skipped: line range outside the PR's diff.
81-85:
⚠️ Potential issueEnsure Correct Use of Placeholders in
$wpdb->prepare.In the WHERE clause, the format variable
$formatis inserted directly into the query string. This can lead to improper placeholder usage. Instead, use a standard placeholder like%dor%s.Apply this diff to fix the placeholder:
$this->add_sql_clause( 'where', $wpdb->prepare( - " AND {$id_field_name} = $format", + " AND {$id_field_name} = {$format}", $model->get_id() ) );Ensure that
$formatcontains a valid placeholder like%dor%s.Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 82-82:
Replacement variables found, but no valid placeholders found in the query.includes/Order/functions.php (1)
281-281: 🛠️ Refactor suggestion
Add error handling for the
save()methodEnsure that the
save()method's success or failure is properly handled to prevent data inconsistencies.Consider checking the return value of
save()and handling potential errors:if ( ! $vendor_balance->save() ) { // Handle the error, e.g., log the error or throw an exception }
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
Release Notes
New Features
VendorBalancemodel for managing vendor financial data.VendorBalanceStorefor handling vendor balance data retrieval and manipulation.Bug Fixes
Documentation
Tests
VendorBalancemodel, covering various functionalities.Chores