Module/form manager #2098
Module/form manager #2098
Conversation
… block instead of generic sub section
…nd made Single Step Product Edit page as default refactor: replaced product add/edit page code with OOP fix: added Dokan Pro checks for `product_vendors_can_create_tags` and `product_category_style` admin settings
… dokan_product_edit_meta_data, this will save some executions refactor: removed update_post_meta and get_post_meta references
…Manager.php to prevent direct access to the files
…o module/form_manager
shashwatahalder01
added
Needs: Dev Review
|
Scenario: Screenshot: |
|
Scenario: Screenshot: |
|
Scenario: Screenshot: |
|
scenario: Scenario: |
|
Scenario: PHP Warning: Screenshot: |
|
Scenario: Screenshot: |
|
Scenario: Screenshot: |
|
Scenario: Steps to reproduce:
|
|
Scenario: Steps to reproduce:
|
|
Scenario: Screenshot: |
|
Scenario: Steps to reproduce:
|
|
Scenario: Steps to reproduce:
|
|
Scenario: Steps to reproduce:
|
|
Scenario: Steps to reproduce:
|
|
Scenario: Steps to reproduce:
Screencasthttps://jam.dev/c/6f77f720-749a-4aec-864f-67f0e0e72247 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tests/pw/pages/productsPage.ts (1)
Line range hint 321-373: Consider extracting common product description logic.
The product description handling code is duplicated across three methods. Consider extracting this into a shared helper method to improve maintainability and reduce duplication.
+ private async addProductDescription(product: product['simple'] | product['downloadable'] | product['virtual']): Promise<void> {
+ await this.typeFrameSelector(
+ productsVendor.description.descriptionIframe,
+ productsVendor.description.descriptionHtmlBody,
+ product.description
+ );
+ }
async vendorAddSimpleProduct(product: product['simple'] | product['variable'] | product['simpleSubscription'] | product['external'], productPopup = false): Promise<void> {
// ... existing code ...
- await this.typeFrameSelector(productsVendor.description.descriptionIframe, productsVendor.description.descriptionHtmlBody, product.description);
+ await this.addProductDescription(product);
// ... existing code ...
}
async vendorAddDownloadableProduct(product: product['downloadable'], productPopup = false): Promise<void> {
// ... existing code ...
- await this.typeFrameSelector(productsVendor.description.descriptionIframe, productsVendor.description.descriptionHtmlBody, product.description);
+ await this.addProductDescription(product);
// ... existing code ...
}
async vendorAddVirtualProduct(product: product['simple'], productPopup = false): Promise<void> {
// ... existing code ...
- await this.typeFrameSelector(productsVendor.description.descriptionIframe, productsVendor.description.descriptionHtmlBody, product.description);
+ await this.addProductDescription(product);
// ... existing code ...
}tests/pw/utils/testData.ts (2)
215-215: Consider using a constant for the repeated description value.
The implementation consistently adds the description property across all product types, which is good. However, the string 'test long description' is repeated multiple times. Consider defining it as a constant at the top of the file to improve maintainability.
+ const TEST_PRODUCT_DESCRIPTION = 'test long description';
simple: {
- description: 'test long description',
+ description: TEST_PRODUCT_DESCRIPTION,
},
downloadable: {
- description: 'test long description',
+ description: TEST_PRODUCT_DESCRIPTION,
},
// ... apply similar changes to other product typesAlso applies to: 228-228, 248-248, 261-261, 281-281, 293-293, 310-310, 332-332
Line range hint 1-1000: Consider splitting this large test data file into smaller, domain-specific modules.
While not directly related to the current changes, the file has grown quite large and contains test data for many different features. Consider restructuring it to improve maintainability:
- Split into domain-specific modules (e.g.,
productTestData.ts,vendorTestData.ts, etc.) - Extract common configuration into a separate config file
- Create a central index file to re-export all test data
This would make the test data easier to maintain and navigate.
Example structure:
// src/tests/pw/utils/testData/index.ts
export * from './productTestData';
export * from './vendorTestData';
export * from './customerTestData';
// ... etc
// src/tests/pw/utils/testData/productTestData.ts
export const productTestData = {
simple: { /* ... */ },
variable: { /* ... */ },
// ... etc
};
// src/tests/pw/utils/testData/config.ts
export const testConfig = {
// common configuration values
};📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- dokan.php (3 hunks)
- tests/pw/pages/productsPage.ts (3 hunks)
- tests/pw/pages/selectors.ts (2 hunks)
- tests/pw/pages/settingsPage.ts (0 hunks)
- tests/pw/utils/dbData.ts (0 hunks)
- tests/pw/utils/schemas.ts (1 hunks)
- tests/pw/utils/testData.ts (8 hunks)
💤 Files with no reviewable changes (2)
- tests/pw/pages/settingsPage.ts
- tests/pw/utils/dbData.ts
🔇 Additional comments (9)
dokan.php (3)
60-60: LGTM!
The property documentation follows PHPDoc standards and maintains consistency with existing property documentation.
263-263: LGTM!
The constant definition is secure and follows the existing pattern of using __DIR__ for absolute paths.
381-381: Verify integration with existing components.
The new ProductForm initialization appears to be part of the module/form manager feature. Since this could affect vendor product management, including discount scheduling:
- Ensure it properly integrates with:
- Vendor dashboard
- Product management features
- Discount scheduling system
- Verify it addresses the backdated discount schedule issue (#2926)
Let's verify the integration points:
tests/pw/pages/productsPage.ts (3)
321-321: LGTM: Product description handling added to simple product creation.
The implementation correctly adds product description support using the iframe selector pattern, maintaining consistency with the rest of the codebase.
342-342: LGTM: Product description handling added to downloadable product creation.
The implementation mirrors the simple product description handling, maintaining consistency across product types.
373-373: LGTM: Product description handling added to virtual product creation.
The implementation follows the same pattern as other product types, ensuring consistent behavior across the product management system.
tests/pw/utils/schemas.ts (1)
1749-1749: LGTM: Type enhancement for category IDs.
The change to allow both number and string types for category IDs is a valid enhancement.
tests/pw/pages/selectors.ts (2)
231-231: Improved selector efficiency by combining related selectors.
The updated selector .dokan-admin-notice.dokan-info, .dokan-admin-notice.dokan-alert combines two previously separate selectors into one, which improves maintainability by keeping related selectors together.
3967-3970: LGTM! Added new product status and visibility selectors.
The new selectors for product status and visibility are properly added to the otherOptions section:
productStatus: '#status',
visibility: '#\\_visibility',
purchaseNote: '#\\_purchase_note',
enableProductReviews: '#comment_status',… on vendor dashboard
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (3)
assets/src/less/products.less (1)
472-499: LGTM: Well-structured form layout implementation.The shipping dimension options layout is well-organized with proper spacing and width calculations. The 23.3% width with 2% margin for form groups ensures proper alignment in the grid system.
Consider using CSS custom properties (variables) for the repeated values:
.dokan-shipping-dimention-options { .dokan-form-group { - width: 23.3%; + width: var(--form-group-width, 23.3%); - margin-right: 2%; + margin-right: var(--form-group-margin, 2%); } }includes/Assets.php (1)
1040-1063: Track refactoring of variation-related strings.The variation-related strings are properly implemented, but there's a TODO comment indicating they should be moved to Dokan Pro Assets class.
Would you like me to create a GitHub issue to track the planned refactoring of these strings to the Dokan Pro Assets class?
includes/ProductForm/Init.php (1)
269-269: Address the TODO: Add support for creating tagsThere's a TODO comment indicating that support for creating tags needs to be added. Implement this functionality to enhance the product tagging feature.
Would you like assistance in implementing support for creating tags, or should I open a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
assets/src/less/products.less(3 hunks)includes/Assets.php(8 hunks)includes/ProductForm/Init.php(1 hunks)
🔇 Additional comments (8)
assets/src/less/products.less (1)
284-286: LGTM: Clean utility class implementation.
The .dokan-hide class follows standard naming conventions and uses the correct CSS property for hiding elements.
includes/Assets.php (4)
9-9: LGTM: Import statement follows PSR-4 standard.
The import statement for ProductFormFactory is properly namespaced and follows PSR-4 autoloading conventions.
522-522: LGTM: Proper dependency management.
The addition of 'jquery-tiptip' as a dependency is appropriate as it's a WooCommerce-provided script needed for tooltip functionality.
979-993: LGTM: Proper internationalization of UI strings.
The added localization strings are:
- Properly internationalized using WordPress i18n functions
- Correctly escaped where necessary
- Clear and user-friendly
1209-1209: LGTM: Proper filter implementation with sensible default.
The product types filter is properly implemented with 'simple' as the default value, allowing for easy extension through the WordPress filter system.
includes/ProductForm/Init.php (3)
581-583: Check sanitize_callback parameters for Downloads field
The sanitize_callback for the Downloads field accepts multiple parameters ($file_names, $file_urls, $file_hashes), which may not align with the expected callback signature. Ensure that the callback function receives the appropriate parameters as per the implementation.
Please confirm that the form handling logic passes these parameters correctly to the sanitize_callback. If not, adjust the function to accept a single $value parameter or modify the form processing accordingly.
92-94: Verify sanitize_callback parameters for Slug field
The sanitize_callback for the Slug field accepts multiple parameters ($slug, $product_id, $status, $post_parent), which may not match the expected callback signature. Ensure that this function is called with the correct parameters during form processing.
Confirm that the form processing code provides these parameters to the sanitize_callback. If not, adjust the callback to match the expected usage.
223-231: Ensure proper parameter passing in sanitize_callback for Categories field
The sanitize_callback function for the Categories field accepts $categories and $product as parameters. Verify that the form processing logic supplies these parameters when invoking the callback.
Please ensure that the callback receives the correct parameters during execution. If necessary, adjust the form handling code to pass the required arguments.
| @@ -972,7 +998,7 @@ | |||
|
|
|||
| .instruction-inside { | |||
| text-align: center; | |||
| padding-top: 40px; | |||
| padding-top: 80px; | |||
There was a problem hiding this comment.
💡 Codebase verification
Padding change appears inconsistent with responsive design patterns
The codebase shows multiple declarations of .instruction-inside with different padding approaches:
- One instance uses percentage-based padding (20% top, 30% bottom)
- Another uses the new fixed 80px padding-top
- These different padding styles appear in the same file without clear responsive breakpoints
The change from percentage-based padding to fixed pixels could cause layout issues across different viewport sizes, especially since the file already contains mobile-specific media queries (@media max-width: 430px).
Recommend:
- Maintain consistent padding strategy (either percentage or fixed)
- If fixed padding is required, add appropriate media queries for smaller screens
- Consider reverting to percentage-based padding for better responsiveness
🔗 Analysis chain
Verify the increased padding impact on layout.
The padding-top has been significantly increased from 20px to 80px. While this might improve visual spacing, please verify that it doesn't cause any layout issues or excessive whitespace in different viewport sizes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any responsive design considerations for the instruction-inside class
# Look for media queries or responsive adjustments that might affect this padding
rg -A 5 "instruction-inside.*{" --type less
rg "@media.*{" --type less
Length of output: 262
Script:
#!/bin/bash
# Fix the regex syntax and search for instruction-inside class and media queries
rg -A 5 "instruction-inside" --type less
rg "@media" --type less
Length of output: 4309
| public function get_product_fields() { | ||
| $temp_fields = []; | ||
| foreach ( ProductFormFactory::get_fields() as $field_id => $field ) { | ||
| $temp_fields[ $field_id ] = $field->toArray(); | ||
| } | ||
|
|
||
| return json_encode( $temp_fields ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for JSON encoding.
While the method is well-structured, it should handle potential JSON encoding failures to prevent silent errors.
Consider this improvement:
public function get_product_fields() {
$temp_fields = [];
foreach ( ProductFormFactory::get_fields() as $field_id => $field ) {
$temp_fields[ $field_id ] = $field->toArray();
}
- return json_encode( $temp_fields );
+ $json = json_encode( $temp_fields );
+ if ( json_last_error() !== JSON_ERROR_NONE ) {
+ return '{}'; // Return empty object as fallback
+ }
+ return $json;
}📝 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_product_fields() { | |
| $temp_fields = []; | |
| foreach ( ProductFormFactory::get_fields() as $field_id => $field ) { | |
| $temp_fields[ $field_id ] = $field->toArray(); | |
| } | |
| return json_encode( $temp_fields ); | |
| } | |
| public function get_product_fields() { | |
| $temp_fields = []; | |
| foreach ( ProductFormFactory::get_fields() as $field_id => $field ) { | |
| $temp_fields[ $field_id ] = $field->toArray(); | |
| } | |
| $json = json_encode( $temp_fields ); | |
| if ( json_last_error() !== JSON_ERROR_NONE ) { | |
| return '{}'; // Return empty object as fallback | |
| } | |
| return $json; | |
| } |
| * @since DOKAN_SINCE | ||
| */ |
There was a problem hiding this comment.
Replace placeholder 'DOKAN_SINCE' with the actual version number
The @since DOKAN_SINCE tag in the class docblock should be replaced with the actual version number before release for accurate documentation.
| * @since DOKAN_SINCE | ||
| * | ||
| * @return void |
There was a problem hiding this comment.
Update @since DOKAN_SINCE in method docblock
The @since DOKAN_SINCE tag in the __construct() method docblock should be updated with the appropriate version number.
| * @since DOKAN_SINCE | ||
| * | ||
| * @return void | ||
| */ |
There was a problem hiding this comment.
Ensure @since DOKAN_SINCE tags are updated
All occurrences of @since DOKAN_SINCE in the method docblocks should be replaced with the actual version number to maintain accurate documentation.
Also applies to: 45-48, 55-58, 88-91, 107-110
| Elements::DOWNLOADABLE, [ | ||
| 'title' => __( 'Downloadable', 'dokan-lite' ), | ||
| 'description' => __( 'Virtual products are intangible and are not shipped.', 'dokan-lite' ), | ||
| 'field_type' => 'checkbox', | ||
| 'name' => '_downloadable', | ||
| 'additional_properties' => [ | ||
| 'value' => 'yes', | ||
| ], | ||
| 'sanitize_callback' => function ( $value ) { | ||
| return ! empty( $value ) && 'yes' === $value; | ||
| }, | ||
| ] | ||
| ); |
There was a problem hiding this comment.
Inconsistent use of checkbox values: 'yes' vs 'on'
The checkboxes for Elements::DOWNLOADABLE and Elements::VIRTUAL use 'value' => 'yes' in additional_properties, and the sanitize_callback checks for 'yes'. However, the dependency_condition checks for 'value' => 'on'. This inconsistency may lead to dependency conditions not working as expected.
Update the dependency_condition values to match the checkbox values:
// For Date On Sale From field
- 'value' => 'on',
+ 'value' => 'yes',
// For Date On Sale To field
- 'value' => 'on',
+ 'value' => 'yes',
// For Downloadable Options section
- 'value' => 'on',
+ 'value' => 'yes',Also applies to: 368-381
| 'title' => __( 'Downloadable', 'dokan-lite' ), | ||
| 'description' => __( 'Virtual products are intangible and are not shipped.', 'dokan-lite' ), |
There was a problem hiding this comment.
Descriptions for 'Downloadable' and 'Virtual' fields are swapped
The description for the Downloadable field describes virtual products, and the description for the Virtual field describes downloadable products. Please swap the descriptions to match the correct fields.
Apply this diff to fix the descriptions:
// For the 'Downloadable' field
- 'description' => __( 'Virtual products are intangible and are not shipped.', 'dokan-lite' ),
+ 'description' => __( 'Downloadable products give access to a file upon purchase.', 'dokan-lite' ),
// For the 'Virtual' field
- 'description' => __( 'Downloadable products give access to a file upon purchase.', 'dokan-lite' ),
+ 'description' => __( 'Virtual products are intangible and are not shipped.', 'dokan-lite' ),Also applies to: 370-371
| /** | ||
| * Maximum number of tags a vendor can add. | ||
| * | ||
| * @since 3.3.7 | ||
| * | ||
| * @args int -1 default to unlimited | ||
| */ | ||
| $maximum_tags_select_length = apply_filters( 'dokan_product_tags_select_max_length', -1 ); | ||
|
|
||
| if ( $maximum_tags_select_length !== -1 && count( $tags ) > $maximum_tags_select_length ) { | ||
| // translators: %s: maximum tag length | ||
| return sprintf( __( 'You can only select %s tags', 'dokan-lite' ), number_format_i18n( $maximum_tags_select_length ) ); | ||
| } | ||
|
|
||
| // todo: add support for creating tags | ||
| return array_map( | ||
| function ( $tag ) { | ||
| if ( is_numeric( $tag ) ) { | ||
| return absint( $tag ); | ||
| } else { | ||
| return sanitize_text_field( $tag ); | ||
| } | ||
| }, (array) $tags |
There was a problem hiding this comment.
Handle errors properly in sanitize_callback for Tags field
In the sanitize_callback function for the Tags field, returning a string error message is not appropriate for error handling. Instead, consider returning a WP_Error object or using a validation mechanism to handle the error.
Apply this diff to correct the error handling:
- return sprintf( __( 'You can only select %s tags', 'dokan-lite' ), number_format_i18n( $maximum_tags_select_length ) );
+ return new \WP_Error(
+ 'too_many_tags',
+ sprintf( __( 'You can only select %s tags', 'dokan-lite' ), number_format_i18n( $maximum_tags_select_length ) )
+ );📝 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.
| /** | |
| * Maximum number of tags a vendor can add. | |
| * | |
| * @since 3.3.7 | |
| * | |
| * @args int -1 default to unlimited | |
| */ | |
| $maximum_tags_select_length = apply_filters( 'dokan_product_tags_select_max_length', -1 ); | |
| if ( $maximum_tags_select_length !== -1 && count( $tags ) > $maximum_tags_select_length ) { | |
| // translators: %s: maximum tag length | |
| return sprintf( __( 'You can only select %s tags', 'dokan-lite' ), number_format_i18n( $maximum_tags_select_length ) ); | |
| } | |
| // todo: add support for creating tags | |
| return array_map( | |
| function ( $tag ) { | |
| if ( is_numeric( $tag ) ) { | |
| return absint( $tag ); | |
| } else { | |
| return sanitize_text_field( $tag ); | |
| } | |
| }, (array) $tags | |
| /** | |
| * Maximum number of tags a vendor can add. | |
| * | |
| * @since 3.3.7 | |
| * | |
| * @args int -1 default to unlimited | |
| */ | |
| $maximum_tags_select_length = apply_filters( 'dokan_product_tags_select_max_length', -1 ); | |
| if ( $maximum_tags_select_length !== -1 && count( $tags ) > $maximum_tags_select_length ) { | |
| // translators: %s: maximum tag length | |
| return new \WP_Error( | |
| 'too_many_tags', | |
| sprintf( __( 'You can only select %s tags', 'dokan-lite' ), number_format_i18n( $maximum_tags_select_length ) ) | |
| ); | |
| } | |
| // todo: add support for creating tags | |
| return array_map( | |
| function ( $tag ) { | |
| if ( is_numeric( $tag ) ) { | |
| return absint( $tag ); | |
| } else { | |
| return sanitize_text_field( $tag ); | |
| } | |
| }, (array) $tags |
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
https://github.com/getdokan/dokan-pro/pull/2883
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
Bug Fixes
Improvements
Chores