Add Booking products tests #2438
Conversation
WalkthroughThis pull request introduces several enhancements across multiple files related to product management in a testing context. Key changes include modifications to methods in the Changes
Possibly related PRs
Suggested labels
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.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (22)
tests/pw/tests/e2e/vendorBooking.spec.ts (2)
Line range hint
127-127: Fix timezone-dependent test failure.The TODO comment indicates a timezone-related failure in CI. This could lead to flaky tests.
Consider these solutions:
- Mock the system time in the test
- Make the test timezone-independent by using relative times
- Explicitly set the timezone in the test environment
Example fix:
// Before running the test, set a fixed time test.beforeAll(async ({ browser }) => { // Set timezone to UTC to ensure consistent behavior process.env.TZ = 'UTC'; }); // In the test, use relative times instead of absolute times const bookingDate = new Date(); bookingDate.setDate(bookingDate.getDate() + 1); // Book for tomorrow
Line range hint
1-127: Overall test structure looks good!Positive aspects of the test suite:
- Well-organized test cases with clear separation of concerns
- Proper use of page object model
- Good test isolation with separate browser contexts
- Comprehensive coverage of booking functionality
- Appropriate use of tags for test categorization
Minor suggestions:
- Consider adding more assertions in the test cases
- Add error handling for API calls in setup
tests/pw/tests/e2e/productsDetailsAuction.spec.ts (2)
244-245: Consider adding details to the TODO comment.The TODO comment lacks specificity about what needs to be refactored. Consider either:
- Adding specific details about the refactoring needed
- Creating a GitHub issue to track the refactoring task
- Removing the comment if it's no longer relevant
Would you like me to help create a GitHub issue to track this refactoring task?
172-173: Consider optimizing test performance.Multiple tests create new products when testing different features. While this ensures test isolation, it might impact execution time. Consider:
- Grouping related tests and reusing products where isolation isn't critical
- Using beforeEach hooks for common setup
- Implementing parallel test execution for independent test cases
Example optimization for virtual option tests:
test.describe('Product virtual options', () => { + let productId: string; + + test.beforeEach(async () => { + [, productId] = await apiUtils.createProduct(payloads.createAuctionProductRequiredFields(), payloads.vendorAuth); + }); + test('vendor can add product virtual option', async () => { - const [, productIdFull] = await apiUtils.createProduct(payloads.createAuctionProductRequiredFields(), payloads.vendorAuth); - await vendor.addProductVirtualOption(productIdFull, true); + await vendor.addProductVirtualOption(productId, true); }); });Also applies to: 177-178, 248-249, 255-256, 263-264, 296-297, 301-302
tests/pw/tests/e2e/productsDetails.spec.ts (1)
439-439: Organize and complete test coverage.Several TODO comments indicate:
- Duplicate tests that should be consolidated (EU compliance, product addons)
- Missing test coverage (advertising, rank math SEO, variation options)
- Missing update RMA options tests
Consider:
- Moving duplicate tests to shared test suites
- Creating a tracking issue for the missing test coverage
Would you like me to:
- Create a proposal for reorganizing the duplicate tests?
- Open a GitHub issue to track the missing test coverage?
Also applies to: 452-452, 494-494, 531-533
tests/pw/pages/vendorAuctionsPage.ts (1)
Line range hint
588-593: LGTM! Consider adding error handling.The implementation correctly verifies that an already added attribute cannot be added again. The method name is descriptive, and the assertions are appropriate for checking both visibility and disabled state.
Consider adding error handling for cases where the product or attribute doesn't exist:
async cantAddAlreadyAddedAttribute(productName: string, attributeName: string): Promise<void> { await this.goToAuctionProductEditById(productName); + try { await this.toBeVisible(productsVendor.attribute.savedAttribute(attributeName)); await this.toHaveAttribute(productsVendor.attribute.disabledAttribute(attributeName), 'disabled', 'disabled'); + } catch (error) { + throw new Error(`Failed to verify attribute "${attributeName}" for product "${productName}": ${error.message}`); + } }tests/pw/pages/selectors.ts (1)
5232-5238: LGTM! Consider minor improvement for consistency.The tag selector implementation looks good and follows the established patterns. However, for better consistency with other tag selectors in the codebase, consider using a single selector pattern instead of the OR condition.
- tagInput: '//select[@id="product_tag_edit"]/..//input[@class="select2-search__field"] | //select[@id="product_tag[]"]/..//input[@class="select2-search__field"]', // todo: remove previous when pr merged + tagInput: '//select[@id="product_tag[]"]/..//input[@class="select2-search__field"]',tests/pw/tests/e2e/productsDetailsBookings.spec.ts (6)
44-46: Avoid re-creating 'productIdFull'; reuse the existing one.In this test,
productIdFullis re-created even though it was created in thebeforeAllhook. This can lead to unnecessary API calls and potential inconsistencies. Consider reusing the existingproductIdFull.Modify the test to use the existing
productIdFull:- const [, productIdFull] = await apiUtils.createBookableProduct(payloads.createBookableProduct(), payloads.vendorAuth); - await vendor.addProductTitle(productIdFull, data.product.productInfo.title); + await vendor.addProductTitle(productIdFull, data.product.productInfo.title);
51-57: Ensure consistent product ID usage across tests.In tests for updating product categories, you're using both
productIdFullandproductIdBasic. For clarity and consistency, consider using a single product ID unless different setups are required.Consolidate the tests to use
productIdFull:- await vendor.addProductCategory(productIdBasic, data.product.category.categories, true); + await vendor.addProductCategory(productIdFull, data.product.category.categories, true);
84-86: Address skipped test due to Dokan issue.The test is skipped with the message 'dokan issue option does not work'. To ensure full coverage, consider addressing the underlying issue or tracking it appropriately.
Would you like assistance in diagnosing the Dokan issue or creating a GitHub issue to track it?
370-372: Avoid using 'test.slow()' without a parameter.The
test.slow()method should be used with a boolean parameter or inside a test modifier. Using it without parameters may not have the intended effect.Update the line to specify the intended behavior:
- test.slow(); + test.slow(true);
230-230: Remove unnecessary empty lines for cleaner code.There's an extra empty line here that can be removed to improve code readability.
352-352: Remove completed 'TODO' comments.The comment
// todo: vendor can't add already added attributeseems to be a placeholder. If this test is already implemented or no longer needed, consider removing the comment.tests/pw/utils/interfaces.ts (2)
255-263: Consider simplifying property names within 'duration'Since the properties are within the
durationobject, prefixes likebookingDurationmight be redundant. You could rename the properties totype,duration,unit,min, andmaxfor better readability.
266-275: Simplify property names within 'availability'The property names within the
availabilityobject could be simplified to enhance readability. For example,minimumBookingWindowIntoTheFutureDatecould be renamed tominBookingWindoworminAdvanceBooking.tests/pw/pages/vendorBookingPage.ts (4)
339-360: Improve Parameter Naming for ClarityThe parameter
neginvendorAddProductCategoryis not immediately descriptive.Consider renaming
negto something more expressive likeexpectDisabledorshouldBeDisabledto enhance readability.-async vendorAddProductCategory(category: string, multiple: boolean, neg?: boolean): Promise<void> { +async vendorAddProductCategory(category: string, multiple: boolean, expectDisabled?: boolean): Promise<void> { ... - if (neg) { + if (expectDisabled) { await this.toBeDisabled(productsVendor.category.done); return; }
428-432: Possible Unhandled Promise RejectionWhen removing the previous cover image, ensure that the actions are properly awaited and error-handled.
Consider adding error handling or checks to confirm that the image has been removed before proceeding.
157-161: Redundant Comments and Missing Error HandlingThe comments within
updateBookingProductFields(e.g.,// booking duration options) are redundant with the code itself. Additionally, there's no error handling if any of the fields fail to update.Consider removing redundant comments and adding error handling to verify each field update.
697-697: Unused ConditionhasClassinaddProductTaxThe parameter
hasClasscontrols whether to select a tax class, but it's not clear wherehasClassis set or used elsewhere.Consider evaluating if
hasClassis necessary or if it can be determined based ontax.class.tests/pw/utils/testData.ts (1)
406-406: Replace hardcoded 'addResourceId' with the actual resource IDThe
addResourceIdis currently set to'427'with a TODO comment. Please update this to use the correct dynamic value to ensure proper functionality.tests/pw/utils/payloads.ts (2)
987-990: Remove unnecessary commented-out codeLines 987-990 contain commented-out properties (
min_date_value,min_date_unit,max_date_value,max_date_unit). If these properties are no longer needed, consider removing them to enhance code readability. If they are intended for future use, you might want to add a comment explaining their purpose.
1012-1012: Ensure consistent property naming fordisplay_costThe property
display_costuses snake_case, whereas similar properties may use camelCase in the codebase (e.g.,displayCostininterfaces.ts). To maintain consistency across the codebase, consider renaming it todisplayCost.Apply this diff to update the property name:
- display_cost: '15', + displayCost: '15',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
tests/pw/pages/productsPage.ts(1 hunks)tests/pw/pages/selectors.ts(4 hunks)tests/pw/pages/vendorAuctionsPage.ts(1 hunks)tests/pw/pages/vendorBookingPage.ts(5 hunks)tests/pw/tests/e2e/productsDetails.spec.ts(4 hunks)tests/pw/tests/e2e/productsDetailsAuction.spec.ts(3 hunks)tests/pw/tests/e2e/productsDetailsBookings.spec.ts(1 hunks)tests/pw/tests/e2e/vendorBooking.spec.ts(1 hunks)tests/pw/utils/interfaces.ts(1 hunks)tests/pw/utils/payloads.ts(2 hunks)tests/pw/utils/testData.ts(2 hunks)
🔇 Additional comments (10)
tests/pw/tests/e2e/vendorBooking.spec.ts (1)
34-35: Verify test isolation with RMA settings.
The addition of RMA settings in the setup might affect other test suites. Consider:
- Resetting these settings in
afterAll - Moving this to a more specific setup if it's not required for all tests
✅ Verification successful
Test cleanup is properly handled
The RMA settings are properly managed in both test files:
vendorSettings.spec.tsresets RMA settings in itsafterAllhookvendorBooking.spec.tshas anafterAllhook for cleanup, and since it's disabling the settings, no reset is needed- Both files properly isolate their RMA settings changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if RMA settings are used in other test files
# Expected: No other test files should rely on these settings
# Search for RMA settings usage in test files
rg -t ts "_dokan_rma_settings" tests/
Length of output: 710
Script:
#!/bin/bash
# Let's check the content of vendorSettings.spec.ts to see if it handles cleanup
rg -A 10 -B 10 "_dokan_rma_settings" tests/pw/tests/e2e/vendorSettings.spec.ts
# Also check if there are any afterAll hooks in vendorBooking.spec.ts
rg -A 10 "afterAll" tests/pw/tests/e2e/vendorBooking.spec.ts
# Check the RMA settings structure in test data
rg -A 5 "rmaSettings" tests/pw/utils/dbData.ts
Length of output: 1723
tests/pw/tests/e2e/productsDetailsAuction.spec.ts (1)
16-17: LGTM! Improved variable naming for better clarity.
The renaming from productId to productIdFull and productId1 to productIdBasic makes the purpose of each variable clearer, with helpful comments indicating their differences.
tests/pw/tests/e2e/productsDetails.spec.ts (2)
19-20: Improved variable naming for better clarity!
The renaming of variables from productName to productNameFull and productName1 to productNameBasic better reflects their purpose, and the added comments provide clear context about their differences.
32-33: Address the media URL handling in GitHub Actions.
The commented code and TODO suggest an unresolved issue with media URL handling in GitHub Actions. This could affect test coverage for image-related functionality.
Let's check if this is a known issue:
Would you like me to help create a GitHub issue to track this media URL handling problem?
tests/pw/pages/selectors.ts (1)
Line range hint 5249-5283: LGTM! Well-structured booking configuration selectors.
The booking configuration selectors are well organized and follow consistent naming patterns. The selectors appropriately use IDs where available and fall back to other selector types when needed. The comments explaining options (e.g. for status values) are helpful.
tests/pw/utils/interfaces.ts (1)
276-280: Costs object is well-structured
Grouping cost-related properties under the costs object improves code organization and clarity.
tests/pw/pages/vendorBookingPage.ts (2)
323-327: Consistency in Success Messages
The success message in saveProduct differs from others.
Ensure that the success message 'Success! The product has been saved successfully.' is consistent across the application.
#!/bin/bash
# Description: Verify consistency of success messages in the codebase.
rg -F "'Success! The product has been saved successfully.'" tests/pw/pages/
# Expected: The success message should match in all relevant files.64-66: Potential Misuse of data.subUrls.frontend.vDashboard.bookingProductEdit
Ensure that productId is correctly used to construct the URL in gotoUntilNetworkidle. If productId is actually a productName, this could cause navigation issues.
Run the following script to verify that all calls to goToBookingProductEditById pass a valid productId:
tests/pw/utils/testData.ts (2)
1154-1154: Verify usage consistency of 'bookingProductEdit' method
The method bookingProductEdit(productId: string) has been added to the vDashboard. Ensure that this method is consistently implemented across the codebase and that any calls to edit booking products utilize this new method.
Run the following script to check for method usage:
#!/bin/bash
# Description: Find all usages of 'bookingProductEdit' to verify consistency.
# Expected Result: All instances align with the new method implementation.
rg 'bookingProductEdit\(.*\)'366-393: 🛠️ Refactor suggestion
Ensure all references to booking properties are updated to the new nested structure
The booking object has been reorganized with properties grouped into nested objects: duration, availability, costs, and extraOptions. Please verify that all code referencing these properties has been updated to reflect the new structure to prevent any runtime errors.
Run the following script to find any outdated references:
| await vendor.addProductShipping(productIdBasic, data.product.productInfo.shipping); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Add issue tracking and clear TODOs for skipped tests
The verification confirms that several tests are skipped with minimal documentation. Current comments only mention "has dokan issue" or "php notice" without proper tracking. Additionally, there are multiple TODO comments without actionable details.
Specific improvements needed:
- Line 213-214: Replace
test.skip(true, 'has dokan issue, php notice')with detailed tracking - Line 217-218: Same issue with shipping test skip comment
- Line 230-231: Update
test.skip(true, 'Has Dokan Issue')with specific issue details
Recommendations:
- Create dedicated issues for each PHP notice and Dokan problem
- Update skip messages with:
test.skip('BLOCKED: PHP Notice when saving shipping details - {issue-number}')
- Add TODOs with acceptance criteria:
// TODO({issue-number}): Unskip when PHP notices are resolved in shipping calculations
🔗 Analysis chain
Enhance documentation for skipped tests.
Several tests are skipped due to Dokan issues, but there's no reference to corresponding issue numbers or detailed descriptions. Consider:
- Adding issue references (e.g.,
test.skip('DOKAN-123: PHP notice when...')) - Including brief descriptions of the issues
- Adding TODOs with criteria for unskipping
Let's check if these issues are already tracked:
Also applies to: 217-218, 230-231
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related issues in the repository
gh issue list --search "in:title php notice shipping tax" --limit 10
Length of output: 70
Script:
#!/bin/bash
# Search for skipped tests and their comments in the test file
rg -B 2 "test\.skip" tests/pw/tests/e2e/productsDetailsAuction.spec.ts
# Search for any TODO or issue references in the test file
rg -i "todo|issue|dokan-|#\d+" tests/pw/tests/e2e/productsDetailsAuction.spec.ts
Length of output: 1965
| // todo: need a product with cover image | ||
| await vendor.addProductCoverImage(productName, data.product.productInfo.images.cover); | ||
| await vendor.addProductCoverImage(productName, data.product.productInfo.images.cover, true); | ||
| await vendor.addProductCoverImage(productNameFull, data.product.productInfo.images.cover); | ||
| await vendor.addProductCoverImage(productNameFull, data.product.productInfo.images.cover, true); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate image-related test setup requirements.
Multiple TODO comments indicate a need for products with specific image states. Consider creating a shared setup helper to create products with the required image states, which can be reused across these test cases.
Example helper function:
async function createProductWithImages(options: { coverImage?: boolean; galleryImages?: boolean } = {}) {
const product = { ...payloads.createProduct() };
if (options.coverImage) {
product.images = [{ src: data.product.productInfo.images.cover }];
}
if (options.galleryImages) {
product.images = [...(product.images || []), ...data.product.productInfo.images.gallery];
}
return apiUtils.createProduct(product, payloads.vendorAuth);
}Also applies to: 173-175, 185-187, 191-193
| @@ -1600,6 +1600,7 @@ export class ProductsPage extends AdminPage { | |||
| async cantAddGreaterMin(productName: string, minMaxOption: product['productInfo']['minMax']): Promise<void> { | |||
| await this.goToProductEdit(productName); | |||
| await this.clearAndType(productsVendor.minMax.minimumQuantity, minMaxOption.minimumProductQuantity); | |||
| await this.clearInputField(productsVendor.minMax.maximumQuantity); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant input field clearing
The clearInputField call is unnecessary since clearAndType on the next line already handles clearing the input field before typing the new value.
- await this.clearInputField(productsVendor.minMax.maximumQuantity);Committable suggestion skipped: line range outside the PR's diff.
| [, productIdBasic] = await apiUtils.createBookableProduct(payloads.createBookableProductRequiredFields(), payloads.vendorAuth); | ||
|
|
||
| // product with all fields | ||
| [, productIdFull] = await apiUtils.createBookableProduct(payloads.createBookableProduct(), payloads.vendorAuth); //todo: need to add all fields |
There was a problem hiding this comment.
Include all fields when creating 'productIdFull'.
The TODO comment indicates that not all fields are included in payloads.createBookableProduct(). To ensure comprehensive testing of all product fields, please update the payload to include all fields.
Do you want assistance in updating the payload to include all fields or opening a GitHub issue to track this task?
| }); | ||
|
|
||
| test.afterAll(async () => { | ||
| // await apiUtils.deleteAllProducts(payloads.vendorAuth); |
There was a problem hiding this comment.
Uncomment cleanup code to prevent residual data.
The line to delete all products is commented out in the afterAll hook. This may lead to leftover test data affecting subsequent tests. Uncomment the cleanup code to ensure a clean test environment.
Apply this change:
- // await apiUtils.deleteAllProducts(payloads.vendorAuth);
+ await apiUtils.deleteAllProducts(payloads.vendorAuth);📝 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.
| // await apiUtils.deleteAllProducts(payloads.vendorAuth); | |
| await apiUtils.deleteAllProducts(payloads.vendorAuth); |
| await this.click(productsVendor.attribute.addNew); | ||
| await this.clearAndType(productsVendor.attribute.attributeTermInput, attribute.attributeTerm); | ||
| await this.clickAndWaitForResponse(data.subUrls.ajax, productsVendor.attribute.confirmAddAttributeTerm); | ||
| await this.toBeVisible(productsVendor.attribute.selectedAttributeTerm(attribute.attributeTerm)); | ||
| } |
There was a problem hiding this comment.
Error Handling When Adding New Attribute Terms
When adding a new attribute term, there is no check to confirm that the term was successfully added before proceeding.
Add verification to ensure the attribute term is added and handle any errors accordingly.
| for (const linkedProduct of linkedProducts.crossSells) { | ||
| await this.click(productsVendor.linkedProducts.removeSelectedCrossSellProduct(linkedProduct)); | ||
| await this.press('Escape'); // shift focus from element | ||
| } |
There was a problem hiding this comment.
Incorrect Method Usage of this.press
The method this.press('Escape') might not function as intended.
Ensure that this.press is correctly implemented and that it operates within the right context. If it's supposed to interact with a specific element, you might need to specify the element or use page.keyboard.press.
- await this.press('Escape'); // shift focus from element
+ await this.page.keyboard.press('Escape'); // Shift focus from elementCommittable suggestion skipped: line range outside the PR's diff.
| async addProductTitle(productName: string, title: string): Promise<void> { | ||
| await this.goToBookingProductEditById(productName); | ||
| await this.clearAndType(bookingProductsVendor.booking.productName, title); | ||
| await this.saveProduct(); | ||
| await this.toHaveValue(bookingProductsVendor.booking.productName, title); | ||
| } |
There was a problem hiding this comment.
Inconsistent Usage of productName and productId
In the method addProductTitle, you're passing productName to goToBookingProductEditById, which expects a productId. This may lead to navigation errors if productName is not the same as productId.
Consider modifying the method to accept productId:
-async addProductTitle(productName: string, title: string): Promise<void> {
- await this.goToBookingProductEditById(productName);
+async addProductTitle(productId: string, title: string): Promise<void> {
+ await this.goToBookingProductEditById(productId);📝 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.
| async addProductTitle(productName: string, title: string): Promise<void> { | |
| await this.goToBookingProductEditById(productName); | |
| await this.clearAndType(bookingProductsVendor.booking.productName, title); | |
| await this.saveProduct(); | |
| await this.toHaveValue(bookingProductsVendor.booking.productName, title); | |
| } | |
| async addProductTitle(productId: string, title: string): Promise<void> { | |
| await this.goToBookingProductEditById(productId); | |
| await this.clearAndType(bookingProductsVendor.booking.productName, title); | |
| await this.saveProduct(); | |
| await this.toHaveValue(bookingProductsVendor.booking.productName, title); | |
| } |
| await this.clearAndType(productsVendor.addon.option.enterAnOption, addon.enterAnOption); | ||
| await this.selectByValue(productsVendor.addon.option.optionPriceType, addon.optionPriceType); | ||
| await this.clearAndType(productsVendor.addon.option.optionPriceInput, addon.optionPriceInput); | ||
| await this.check(productsVendor.addon.excludeAddons); | ||
|
|
||
| await this.saveProduct(); | ||
|
|
||
| await this.toBeVisible(productsVendor.addon.addonRow(addon.title)); | ||
| await this.click(productsVendor.addon.addonRow(addon.title)); | ||
|
|
||
| await this.toHaveSelectedValue(productsVendor.addon.type, addon.type); | ||
| await this.toHaveSelectedValue(productsVendor.addon.displayAs, addon.displayAs); | ||
| await this.toHaveValue(productsVendor.addon.titleRequired, addon.title); | ||
| await this.toHaveSelectedValue(productsVendor.addon.formatTitle, addon.formatTitle); | ||
| await this.toBeChecked(productsVendor.addon.addDescription); | ||
| await this.toHaveValue(productsVendor.addon.descriptionInput, addon.addDescription); | ||
| await this.toBeChecked(productsVendor.addon.requiredField); | ||
| // option | ||
| await this.toHaveValue(productsVendor.addon.option.enterAnOption, addon.enterAnOption); | ||
| await this.toHaveSelectedValue(productsVendor.addon.option.optionPriceType, addon.optionPriceType); | ||
| await this.toHaveValue(productsVendor.addon.option.optionPriceInput, addon.optionPriceInput); | ||
| await this.toBeChecked(productsVendor.addon.excludeAddons); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Complexity in Adding Product Addons
The method addProductAddon is lengthy and handles multiple responsibilities.
Consider breaking down the method into smaller helper functions for better maintainability.
+// Example of refactoring
+private async fillAddonDetails(addon: AddonType): Promise<void> {
+ // Fill addon details here
+}
+
async addProductAddon(productName: string, addon: product['productInfo']['addon']): Promise<void> {
await this.goToBookingProductEditById(productName);
// ...
+ await this.fillAddonDetails(addon);
// ...
}Committable suggestion skipped: line range outside the PR's diff.
tests/pw/utils/payloads.ts
Outdated
| max_duration: 10, | ||
| calendar_display_mode: 'always_visible', | ||
| enable_range_picker: true, | ||
| requires_confirmations: false, |
There was a problem hiding this comment.
Typo in property name requires_confirmations
The property requires_confirmations appears to be incorrectly named. It should likely be requires_confirmation (singular), which may align with the expected property name in the booking data structure. Using the incorrect property name might lead to unexpected behavior.
Apply this diff to correct the property name:
- requires_confirmations: false,
+ requires_confirmation: false,📝 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.
| requires_confirmations: false, | |
| requires_confirmation: false, |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/pw/utils/payloads.ts (1)
993-1040: Consider adding validation for display_costThe implementation looks good and properly extends the base bookable product configuration. The addition of
display_costprovides flexibility in pricing display.Consider adding validation to ensure
display_costis always greater than or equal to the basecost. This would prevent potential pricing inconsistencies.Example validation:
if (parseFloat(display_cost) < parseFloat(cost)) { throw new Error('Display cost cannot be less than base cost'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
tests/pw/tests/e2e/productsDetails.spec.ts(8 hunks)tests/pw/tests/e2e/productsDetailsAuction.spec.ts(6 hunks)tests/pw/tests/e2e/productsDetailsBookings.spec.ts(1 hunks)tests/pw/utils/payloads.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/pw/tests/e2e/productsDetails.spec.ts
- tests/pw/tests/e2e/productsDetailsAuction.spec.ts
- tests/pw/tests/e2e/productsDetailsBookings.spec.ts
🔇 Additional comments (1)
tests/pw/utils/payloads.ts (1)
Line range hint 965-993: Fix property name and approve structure
The method structure looks good and includes all necessary fields for a bookable product. However, there's a property name issue that needs to be fixed.
Apply this diff to fix the property name:
- requires_confirmations: false,
+ requires_confirmation: false,
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
tests/pw/tests/e2e/productsDetailsBookings.spec.ts (3)
30-30: Enhance test coverage by implementing all fields inpayloads.createBookableProduct().The TODO comment indicates incomplete test coverage. This could lead to missing edge cases or potential issues in the product creation process.
Would you like me to help implement the missing fields in the payload or create a GitHub issue to track this task?
304-307: Implement skipped resource-related tests.Multiple tests for resource management are skipped with TODO comments. This leaves a gap in testing coverage for an important feature of booking products.
Would you like me to help implement these tests or create a GitHub issue to track this task? The implementation should include:
- Creating products with resources
- Updating resource configurations
- Proper cleanup of resources
Also applies to: 309-312, 314-318
1-388: Overall test suite structure looks good.The test suite is well-organized with clear test descriptions and logical grouping of related tests. The use of beforeAll/afterAll hooks for setup and cleanup is appropriate.
Consider these improvements:
- Add error boundary tests for edge cases
- Implement data-driven tests for variations
- Add performance benchmarks for slow tests
tests/pw/pages/selectors.ts (1)
Line range hint
5249-5283: Improve organization of booking-related selectorsThe booking-related selectors are mixed with other product selectors. Consider grouping them logically for better maintainability.
Suggest reorganizing the selectors into a dedicated booking section:
booking: { // Basic booking options bookingDurationType: '#\\_wc_booking_duration_type', bookingDuration: 'input#\\_wc_booking_duration', bookingDurationUnit: '#\\_wc_booking_duration_unit', bookingDurationMin: 'input#\\_wc_booking_min_duration', bookingDurationMax: 'input#\\_wc_booking_max_duration', // Calendar options calendarDisplayMode: '#\\_wc_booking_calendar_display_mode', enableCalendarRangePicker: '#\\_wc_booking_enable_range_picker', // Booking rules requiresConfirmation: '#\\_wc_booking_requires_confirmation', canBeCancelled: '#\\_wc_booking_user_can_cancel', // ... other booking related selectors }This would:
- Improve code organization
- Make it easier to maintain booking-related selectors
- Provide better separation of concerns
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
tests/pw/pages/selectors.ts(5 hunks)tests/pw/pages/vendorAuctionsPage.ts(2 hunks)tests/pw/tests/e2e/productsDetailsAuction.spec.ts(6 hunks)tests/pw/tests/e2e/productsDetailsBookings.spec.ts(1 hunks)tests/pw/tests/e2e/storelisting.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/pw/tests/e2e/storelisting.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/pw/pages/vendorAuctionsPage.ts
- tests/pw/tests/e2e/productsDetailsAuction.spec.ts
🔇 Additional comments (1)
tests/pw/tests/e2e/productsDetailsBookings.spec.ts (1)
34-34:
Uncomment cleanup code to prevent test data pollution.
The cleanup code is commented out, which could lead to test data accumulation and potential interference between test runs.
Apply this change:
- // await apiUtils.deleteAllProducts(payloads.vendorAuth);
+ await apiUtils.deleteAllProducts(payloads.vendorAuth);Likely invalid or redundant comment.
| }); | ||
|
|
||
| test('vendor can remove booking product addon', { tag: ['@pro', '@vendor'] }, async () => { | ||
| const [, productId, , addonNames] = await apiUtils.createProductWithAddon(payloads.createAuctionProduct(), [payloads.createProductAddon()], payloads.vendorAuth); |
There was a problem hiding this comment.
Use booking product instead of auction product for addon removal test.
The test is creating an auction product instead of a booking product, which doesn't match the test context.
Apply this change:
- const [, productId, , addonNames] = await apiUtils.createProductWithAddon(payloads.createAuctionProduct(), [payloads.createProductAddon()], payloads.vendorAuth);
+ const [, productId, , addonNames] = await apiUtils.createProductWithAddon(payloads.createBookableProduct(), [payloads.createProductAddon()], payloads.vendorAuth);📝 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.
| const [, productId, , addonNames] = await apiUtils.createProductWithAddon(payloads.createAuctionProduct(), [payloads.createProductAddon()], payloads.vendorAuth); | |
| const [, productId, , addonNames] = await apiUtils.createProductWithAddon(payloads.createBookableProduct(), [payloads.createProductAddon()], payloads.vendorAuth); |
| const [responseBody, productId] = await apiUtils.createProductWithAddon(payloads.createAuctionProduct(), [payloads.createProductAddon()], payloads.vendorAuth); | ||
| await vendor.exportAddon(productId, serialize(apiUtils.getMetaDataValue(responseBody.meta_data, '_product_addons'))); |
There was a problem hiding this comment.
Use booking product instead of auction product for addon export test.
The test is creating an auction product instead of a booking product, which doesn't align with the test's purpose.
Apply this change:
- const [responseBody, productId] = await apiUtils.createProductWithAddon(payloads.createAuctionProduct(), [payloads.createProductAddon()], payloads.vendorAuth);
+ const [responseBody, productId] = await apiUtils.createProductWithAddon(payloads.createBookableProduct(), [payloads.createProductAddon()], payloads.vendorAuth);📝 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.
| const [responseBody, productId] = await apiUtils.createProductWithAddon(payloads.createAuctionProduct(), [payloads.createProductAddon()], payloads.vendorAuth); | |
| await vendor.exportAddon(productId, serialize(apiUtils.getMetaDataValue(responseBody.meta_data, '_product_addons'))); | |
| const [responseBody, productId] = await apiUtils.createProductWithAddon(payloads.createBookableProduct(), [payloads.createProductAddon()], payloads.vendorAuth); | |
| await vendor.exportAddon(productId, serialize(apiUtils.getMetaDataValue(responseBody.meta_data, '_product_addons'))); |
| test('vendor can add product virtual option', { tag: ['@pro', '@vendor'] }, async () => { | ||
| const [, productId] = await apiUtils.createProduct(payloads.createBookableProductRequiredFields(), payloads.vendorAuth); | ||
| await vendor.addProductVirtualOption(productId, true); | ||
| }); | ||
|
|
||
| test('vendor can remove product virtual option', { tag: ['@pro', '@vendor'] }, async () => { | ||
| const [, productId] = await apiUtils.createProduct({ ...payloads.createBookableProductRequiredFields(), virtual: true }, payloads.vendorAuth); | ||
| await vendor.addProductVirtualOption(productId, false); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use createBookableProduct consistently for virtual option tests.
The tests are using the generic createProduct method instead of the booking-specific method, which could miss booking-related validations.
Apply these changes:
- const [, productId] = await apiUtils.createProduct(payloads.createBookableProductRequiredFields(), payloads.vendorAuth);
+ const [, productId] = await apiUtils.createBookableProduct(payloads.createBookableProductRequiredFields(), payloads.vendorAuth);
- const [, productId] = await apiUtils.createProduct({ ...payloads.createBookableProductRequiredFields(), virtual: true }, payloads.vendorAuth);
+ const [, productId] = await apiUtils.createBookableProduct({ ...payloads.createBookableProductRequiredFields(), virtual: true }, payloads.vendorAuth);📝 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('vendor can add product virtual option', { tag: ['@pro', '@vendor'] }, async () => { | |
| const [, productId] = await apiUtils.createProduct(payloads.createBookableProductRequiredFields(), payloads.vendorAuth); | |
| await vendor.addProductVirtualOption(productId, true); | |
| }); | |
| test('vendor can remove product virtual option', { tag: ['@pro', '@vendor'] }, async () => { | |
| const [, productId] = await apiUtils.createProduct({ ...payloads.createBookableProductRequiredFields(), virtual: true }, payloads.vendorAuth); | |
| await vendor.addProductVirtualOption(productId, false); | |
| }); | |
| test('vendor can add product virtual option', { tag: ['@pro', '@vendor'] }, async () => { | |
| const [, productId] = await apiUtils.createBookableProduct(payloads.createBookableProductRequiredFields(), payloads.vendorAuth); | |
| await vendor.addProductVirtualOption(productId, true); | |
| }); | |
| test('vendor can remove product virtual option', { tag: ['@pro', '@vendor'] }, async () => { | |
| const [, productId] = await apiUtils.createBookableProduct({ ...payloads.createBookableProductRequiredFields(), virtual: true }, payloads.vendorAuth); | |
| await vendor.addProductVirtualOption(productId, false); | |
| }); |
| // tags | ||
| tags: { | ||
| tagInput: '//select[@id="product_tag"]/..//input[@class="select2-search__field"]', | ||
| searchedTag: (tagName: string) => `//li[@class="select2-results__option select2-results__option--highlighted" and normalize-space(text())="${tagName}"]`, | ||
| selectedTags: (tagName: string) => `//li[@class="select2-selection__choice" and contains(., "${tagName}")]`, | ||
| removeSelectedTags: (tagName: string) => `//li[@class="select2-selection__choice" and contains(., "${tagName}")]//span[@class="select2-selection__choice__remove"]`, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review tag selector implementation for consistency and maintainability
The tag selector implementation has been updated with the following structure:
tagInput: Uses XPath to locate the tag input field with a fallback selectorsearchedTag: Locates highlighted tag search resultsselectedTags: Finds selected tag elementsremoveSelectedTags: Locates remove buttons for selected tags
Consider the following improvements:
- tagInput: '//select[@id="product_tag"]/..//input[@class="select2-search__field"]',
+ tagInput: '.select2-search__field',
- searchedTag: (tagName: string) => `//li[@class="select2-results__option select2-results__option--highlighted" and normalize-space(text())="${tagName}"]`,
+ searchedTag: (tagName: string) => `.select2-results__option--highlighted:contains("${tagName}")`,Rationale:
- Use simpler CSS selectors where possible for better maintainability
- Avoid complex XPath when CSS selectors can achieve the same result
- Keep selector structure consistent with other parts of the codebase
Committable suggestion skipped: line range outside the PR's diff.
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
Bug Fixes
Documentation
Refactor