Add product form tests #2383
Add product form tests #2383
Conversation
WalkthroughThe pull request introduces significant updates to the feature map configuration and various test files, enhancing vendor capabilities in product management across multiple pages. Key changes include disabling administrative features for the Dokan plugin, enabling vendor functionalities for managing products, and introducing new testing methods for product creation and editing. The updates also enhance the flexibility of test setups and ensure thorough validation of product-related functionalities. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 12
🧹 Outside diff range and nitpick comments (15)
tests/pw/tests/api/orderDownloads.spec.ts (2)
20-20: Good implementation of the imported test data!The usage of
data.image.avatarin place of a hardcoded image path is well done. It enhances the test's maintainability and reusability.For even better clarity, consider adding a brief comment explaining what
data.image.avatarrepresents, e.g.:// Upload a test avatar image for the downloadable product const [responseBody] = await apiUtils.uploadMedia(data.image.avatar, payloads.mimeTypes.png, payloads.adminAuth);
Line range hint
1-62: Consider enhancing error handling and assertions in the test suite.The overall structure and implementation of the tests are good. Here are some suggestions to further improve the test suite:
Error Handling: Consider adding try-catch blocks or using test.fail() to handle potential errors more gracefully, especially in the
beforeAllhook where multiple async operations are performed.Specific Assertions: While using schemas for validation is excellent, consider adding more specific assertions for critical fields. For example, in the "get all order downloads" test, you could assert the presence of the newly created download.
Cleanup: Consider adding a
test.afterAllhook to clean up created resources (products, orders, downloads) to ensure a clean state for future test runs.Parameterized Tests: If applicable, consider using test.describe.parallel() and parameterized tests to improve test execution speed and coverage.
Example of a more specific assertion:
test('get all order downloads', { tag: ['@lite', '@v2'] }, async () => { const [response, responseBody] = await apiUtils.get(endPoints.getAllOrderDownloads(orderId)); expect(response.ok()).toBeTruthy(); expect(responseBody).toBeTruthy(); expect(responseBody).toMatchSchema(schemas.orderDownloadsSchema.orderDownloadsSchema); // Add a specific assertion expect(responseBody.some(download => download.product_id === downloadableProductId)).toBeTruthy(); });tests/pw/tests/e2e/products.spec.ts (2)
74-76: LGTM! Consider adding assertions for critical elements.The new test for verifying that vendors can view the add new product page is a valuable addition. It aligns well with the PR objectives and follows the existing test structure.
To enhance the test's robustness, consider adding specific assertions for critical elements on the add new product page. For example:
test('vendor can view add new product page', { tag: ['@lite', '@vendor'] }, async () => { await vendor.vendorAddNewProductRenderProperly(); // Add assertions for critical elements, e.g.: // await expect(page.locator('#product-name')).toBeVisible(); // await expect(page.locator('#product-type')).toBeVisible(); });
Line range hint
1-174: Overall great job! Consider improving test isolation.The changes in this file align well with the PR objectives of enhancing product form testing. The tests are well-organized, properly tagged, and cover a wide range of scenarios for both admin and vendor roles.
To further improve the test suite, consider enhancing test isolation:
- Use
test.describe.configure({ mode: 'parallel' })at the top of the file to enable parallel test execution if not already implemented elsewhere.- Instead of creating products in
beforeAll, consider creating them in individual tests or intest.beforeEach()hooks for better isolation.- Ensure each test cleans up its own data to prevent interdependencies between tests.
Example:
test.describe.configure({ mode: 'parallel' }); test.describe('Product functionality test', () => { // ... existing setup ... test.beforeEach(async ({ browser }) => { // Setup for each test }); test.afterEach(async () => { // Cleanup after each test }); // ... existing tests ... });These changes can improve test reliability and make it easier to run tests in parallel, potentially reducing overall execution time.
tests/pw/feature-map/feature-map.yml (4)
Line range hint
320-321: Improved vendor control over reverse withdrawals.The new features added to the "Reverse Withdrawal" page for vendors are beneficial:
- Vendors can now filter reverse withdrawals by date, improving their ability to track and manage these transactions.
- The ability to pay reverse withdrawal balance gives vendors more control over their financial operations.
These additions enhance the vendor's financial management capabilities within the platform.
Consider adding a feature for vendors to export filtered reverse withdrawal data for better record-keeping and financial reporting. This could be implemented as follows:
vendor can filter reverse withdrawals by date [lite]: true vendor can pay reverse withdrawal balance [lite]: true + vendor can export filtered reverse withdrawal data [lite]: true
Line range hint
328-335: Valuable addition of catalog mode functionality.The new features in the "Catalog Mode" section provide important capabilities:
- Admins and vendors can set catalog mode, giving them control over how products are displayed.
- Options to disable hiding product prices in catalog mode for both admins and vendors.
- Vendors can set catalog mode at both store-wide and single product levels.
- Customers can view products in catalog mode.
These features offer flexibility in product presentation, which can be beneficial for various business strategies.
To enhance this feature, consider adding an option for admins to set default catalog mode behavior that vendors can then override. This could be implemented as:
admin can set catalog mode [lite]: true admin can disable hide product price in catalog mode [lite]: true + admin can set default catalog mode behavior [lite]: true vendor can set catalog mode (storewide) [lite]: true vendor can set catalog mode (single product) [lite]: true vendor can disable hide product price in catalog mode [lite]: true + vendor can override default catalog mode behavior [lite]: true
Line range hint
1003-1004: Enhanced vendor subscription options with non-recurring packs.The addition of non-recurring subscription pack purchase options for vendors is a positive change:
- Vendors can now buy non-recurring subscription packs during registration.
- Vendors can also purchase these packs on the subscription page.
These features provide flexibility for vendors in choosing and managing their subscription status.
While the addition of non-recurring subscription packs is valuable, consider implementing recurring subscription options in the future for vendors who prefer automatic renewals. This could be done by changing the following lines:
vendor can buy recurring subscription pack (on registration): false vendor can buy recurring subscription pack (on subscription page): false + vendor can buy recurring subscription pack (on registration): true + vendor can buy recurring subscription pack (on subscription page): trueAdditionally, you might want to add an option for vendors to switch between recurring and non-recurring subscription types:
vendor can switch subscription: true + vendor can switch between recurring and non-recurring subscriptions: true
Line range hint
1-1089: Overall positive enhancements to vendor capabilities and platform flexibility.This update to the feature map configuration file brings several valuable improvements to the Dokan platform:
- Significantly enhanced product management capabilities for vendors.
- Improved financial control for vendors through reverse withdrawal features.
- Introduction of catalog mode functionality, offering more flexibility in product display.
- Addition of non-recurring subscription options for vendors.
These changes align well with the PR objective of enhancing product form functionality and generally improve the vendor experience on the platform. The updates provide more control and flexibility to vendors in managing their products, finances, and subscriptions.
As the platform continues to evolve, consider the following architectural suggestions:
- Implement a modular approach to feature additions, allowing for easier enabling/disabling of specific capabilities.
- Consider creating a vendor settings API that can be easily extended as new features are added.
- Ensure that new features are consistently implemented across different areas of the platform (e.g., subscription management, product management) to maintain a cohesive user experience.
- As the feature set grows, consider implementing feature flags for gradual rollout and A/B testing of new capabilities.
tests/pw/tests/e2e/productsDetails.spec.ts (3)
9-9: Remove unused import 'serialize' from 'php-serialize'The imported function
serializefrom'php-serialize'is not used anywhere in the code and can be safely removed to clean up the imports.Apply this diff to remove the unused import:
- import { serialize } from 'php-serialize';
11-11: Remove unused constant 'CATEGORY_ID'The constant
CATEGORY_IDis imported fromprocess.envbut is not used in the code. It can be safely removed to tidy up the code.Apply this diff to remove the unused constant:
- const { CATEGORY_ID } = process.env;
31-32: Resolve TODO or remove commented-out codeThere are commented-out lines with a TODO comment indicating that
mediaUrlis not working on GitHub Actions. Leaving commented-out code can clutter the codebase. Consider resolving the issue withmediaUrlor remove the commented-out code if it's no longer needed.tests/pw/pages/productsPage.ts (4)
374-375: Avoid Potential Confusion with Variable NamelengthAt line 374, you're destructuring
lengthfromproductsVendor.rma. Sincelengthis commonly associated with arrays and strings, using it as a variable name could cause confusion. Consider renaming it to something more descriptive, such asrmaLengthordurationLength.
388-391: Address TODO Comment Regarding EU Compliance FieldsThere is a TODO comment about checking for the Germanized plugin, and the code related to EU compliance fields is commented out. Please implement the necessary checks or remove the commented code if it's no longer needed.
Would you like assistance in implementing this functionality or creating a GitHub issue to track this task?
668-668: Typo in Error Message VerificationThe error message
'Error! Description is a required field..'contains an extra period at the end. Verify if this matches the actual error message displayed by the application. If it's a typo, update it to'Error! Description is a required field.'.
779-779: Address TODO Comment for Additional AssertionsAt line 779, there's a TODO comment:
// todo: add more assertions to all product edit test, customer pov. Enhancing your tests with additional assertions from the customer's point of view will improve test coverage and reliability.Would you like help drafting these additional assertions, or should we open a GitHub issue to track this enhancement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- tests/pw/feature-map/feature-map.yml (2 hunks)
- tests/pw/pages/productsPage.ts (3 hunks)
- tests/pw/tests/api/orderDownloads.spec.ts (2 hunks)
- tests/pw/tests/e2e/products.spec.ts (3 hunks)
- tests/pw/tests/e2e/productsDetails.spec.ts (1 hunks)
- tests/pw/utils/payloads.ts (2 hunks)
🔇 Additional comments (5)
tests/pw/tests/api/orderDownloads.spec.ts (1)
10-10: Excellent use of centralized test data!The addition of
import { data } from '@utils/testData';is a positive change. It improves the test's flexibility by using a dynamic data source instead of a hardcoded image path. This follows the best practice of centralizing test data, making it easier to maintain and update across multiple tests.tests/pw/tests/e2e/products.spec.ts (2)
161-163: Implement or provide context for the skipped test.A new test has been added to verify that vendors cannot add products without required fields, but it's currently skipped.
Could you please provide context on why this test is skipped? If it's due to pending implementation:
- Consider adding a TODO comment explaining the reason and any blockers.
- If possible, implement the test to enhance coverage of edge cases.
Here's a suggestion for implementing the test:
test("vendor can't add product without required fields", { tag: ['@lite', '@vendor'] }, async () => { await vendor.addProductWithoutRequiredFields(data.product.simple); // Add assertions to verify the product wasn't added and error messages are displayed // await expect(page.locator('.error-message')).toBeVisible(); // await expect(page.locator('.error-message')).toContainText('Required fields are missing'); });If there are known issues preventing the implementation, please add a comment explaining the situation.
28-28: Verify the intention behind commenting out product deletion.The line for deleting all products in the
afterAllhook has been commented out. This could potentially lead to test data pollution between test runs.Could you please clarify the reasoning behind this change? If it's intentional, consider adding a comment explaining why product deletion is skipped. If it's unintentional, please uncomment the line to ensure proper cleanup after tests.
To verify the impact, you can run the following script:
tests/pw/feature-map/feature-map.yml (1)
Line range hint
94-129: Excellent enhancement of vendor product management capabilities!The new features added to the "Products" page for vendors significantly improve their ability to manage products effectively. These additions cover a wide range of functionalities, including:
- Adding various product types (simple, variable, subscription, external, group, downloadable, virtual)
- Advanced product filtering and searching
- Import/export capabilities
- Duplicate product functionality
- Detailed product management (title, permalink, pricing, discounts)
These enhancements align well with the PR objective of improving product form functionality and will greatly benefit vendors using the Dokan platform.
tests/pw/pages/productsPage.ts (1)
665-665: Verify Error Message Selector and TextAt line 665, you're asserting on
productsVendor.NoTitleErrorfor the error message'This field is required'. Ensure thatNoTitleErroris the correct selector and matches the application's actual DOM structure. Additionally, confirm that the error message text is accurate.
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
New Features
Bug Fixes
Tests