Add & Refactor vendor payment tests #2387
Conversation
WalkthroughThe changes encompass a comprehensive update to the feature map configuration, significantly altering user permissions for various roles, particularly enhancing vendor capabilities while restricting certain admin functionalities. Key modifications include the removal of admin permissions for managing Dokan plugins, the introduction of new vendor permissions for payment and shipping methods, and adjustments to product management rights. Additionally, new methods and structures have been introduced in related test files to align with these permission changes, focusing on database interactions and enhancing test coverage. 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: 4
🧹 Outside diff range and nitpick comments (3)
tests/pw/pages/paymentsPage.ts (1)
223-245: LGTM: New method centralizes payment settings navigationThe new
goToPaymentSettingsmethod effectively centralizes the navigation logic for different payment methods. This improves code organization and makes it easier to add new payment methods in the future.Consider adding a default case that throws an error for unknown method names to catch potential issues early:
default: - break; + throw new Error(`Unknown payment method: ${methodName}`);tests/pw/utils/helpers.ts (1)
Line range hint
497-502: LGTM: ImproveddeepMergeObjectswithisPlainObjectThe use of the
isPlainObjectutility function improves the readability and maintainability of thedeepMergeObjectsfunction. This change makes the code more modular and easier to understand.Consider extracting the array merging logic into a separate method for better modularity:
mergeArrayOrReplace(target: any[], source: any[]): any[] { if (this.isPlainObject(target[0]) && this.isPlainObject(source[0])) { return this.deepMergeArrays(target, source); } return source; } // Then in deepMergeObjects: } else if (Array.isArray(source[key]) && Array.isArray(target[key])) { result[key] = this.mergeArrayOrReplace(target[key], source[key]); } else { result[key] = source[key]; }This extraction would make the
deepMergeObjectsfunction even more readable and easier to maintain.tests/pw/tests/e2e/payments.spec.ts (1)
28-28: Remove Commented-Out Code for ClarityThe line is commented out:
// await dbUtils.updateUserMeta(VENDOR_ID, 'dokan_profile_settings', { payment: [] });If this code is no longer needed, consider removing it to keep the codebase clean. If you plan to use it later, add a comment explaining its purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- tests/pw/feature-map/feature-map.yml (1 hunks)
- tests/pw/pages/paymentsPage.ts (7 hunks)
- tests/pw/tests/e2e/payments.spec.ts (3 hunks)
- tests/pw/utils/dbData.ts (1 hunks)
- tests/pw/utils/helpers.ts (2 hunks)
- tests/pw/utils/testData.ts (0 hunks)
💤 Files with no reviewable changes (1)
- tests/pw/utils/testData.ts
🧰 Additional context used
🪛 Biome
tests/pw/utils/helpers.ts
[error] 470-470: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (28)
tests/pw/pages/paymentsPage.ts (6)
19-21: LGTM: Method renamed for clarityThe renaming of
goToPaymentSettingstogoToWcPaymentSettingsimproves the clarity of the method's purpose. The implementation remains correct and concise.
25-25: LGTM: Method call updated correctlyThe method call has been correctly updated to
goToWcPaymentSettings, maintaining consistency with the earlier method renaming.
41-41: LGTM: Consistent method call updates across payment setup methodsThe method calls in
setupStripeConnect,setupPaypalMarketPlace,setupMangoPay,setupRazorpay, andsetupStripeExpresshave been correctly updated togoToWcPaymentSettings. This change maintains consistency with the earlier method renaming and ensures the correct navigation method is called in all payment setup functions.Also applies to: 66-66, 94-94, 141-141, 165-165
247-254: LGTM: New method simplifies basic payment setupThe new
addBasicPaymentmethod effectively replaces the previoussetBasicPaymentSettingsmethod. It simplifies the process of adding basic payment methods and correctly utilizes the newgoToPaymentSettingsmethod for navigation. The implementation is consistent with other payment setting methods in the class.
255-260: LGTM: New method for removing basic payment methodsThe new
removeBasicPaymentmethod is a valuable addition, complementing theaddBasicPaymentmethod by providing functionality to disconnect basic payment methods. It correctly uses thegoToPaymentSettingsmethod for navigation and follows the established pattern of other methods in the class.
263-266: LGTM: Method renamed for consistencyThe renaming of
setBankTransfertoaddBankTransferimproves consistency with other method names in the class, such asaddBasicPayment.Regarding the commented-out line:
// await this.clickIfVisible(paymentSettingsVendor.disconnectAccount);Is this a planned feature or a reminder for future implementation? Consider adding a TODO comment if it's intended for future use, or remove it if it's no longer needed.
tests/pw/utils/helpers.ts (2)
472-473: LGTM:isPlainObjectimplementationThe
isPlainObjectfunction is well-implemented and provides a useful utility for checking if a value is a plain object. It correctly excludes null values and arrays, which is important for type checking in JavaScript/TypeScript.
Line range hint
469-502: Summary of changes in helpers.tsThe changes to this file are generally positive:
- The addition of
emptyObjectValuesprovides a useful utility, though it could benefit from the suggested refactoring for improved readability and flexibility.- The
isPlainObjectfunction is a well-implemented and valuable addition.- The modification to
deepMergeObjectsusingisPlainObjectenhances code readability and maintainability.These changes align well with the PR objectives of refactoring and enhancing the testing framework. They improve code modularity and provide useful utilities for object manipulation, which are likely to be beneficial in various testing scenarios.
🧰 Tools
🪛 Biome
[error] 470-470: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
tests/pw/feature-map/feature-map.yml (5)
Line range hint
1001-1001: Excellent addition to prevent self-reviews!The new permission "vendor can't review own store: true" is a great addition to maintain the integrity of the review system. This prevents vendors from potentially creating biased or misleading reviews for their own stores, which enhances the trustworthiness of the platform for customers.
187-197: LGTM! Consider security implications.The new permissions for vendors to update and remove various payment methods (PayPal, bank, Skrill, and custom) provide greater flexibility for vendors to manage their finances. This is a positive change that can improve the user experience for vendors.
However, it's crucial to ensure that proper security measures are in place. Please run the following script to verify if there are any security checks implemented for these new permissions:
Line range hint
1153-1188: Vendor verification process improvements, but clarification needed on some settings.The new permissions for admins to manage verification methods and requests, as well as the enhanced abilities for vendors to submit and manage their verification requests, are excellent improvements to the vendor verification process.
However, I noticed a few settings that are set to false and might need clarification:
- "vendor need all required method to be verified to get verification badge: false"
- "vendor need to be verified only one method when no required method is exists: false"
- "vendor address verification gets reset when he update address: false"
Could you please provide more context on why these settings are false? It seems counterintuitive that vendors don't need all required methods to be verified to get a verification badge, or that they don't need to be verified by at least one method when no required method exists.
To better understand these settings, please run the following script to check for any recent discussions or changes related to these verification rules:
Line range hint
1137-1151: Subscription management improvements, but clarification needed.The new permissions for admins to cancel subscriptions (immediately or at the end of the current period) provide better control over vendor subscriptions. Additionally, allowing vendors to buy non-recurring subscription packs offers more flexibility.
However, I noticed that vendors cannot buy recurring subscription packs (both on registration and on the subscription page). Is this an intentional change in the business model or a potential oversight?
Please clarify the intention behind disabling recurring subscription purchases for vendors. If this is intentional, consider updating the documentation to reflect this change. If not, we may need to investigate further.
Line range hint
1190-1214: Wholesale features look great, but there's a contradiction in price visibility.The new wholesale features, including admin management of wholesale customers, vendor ability to create wholesale products, and customer options to become wholesale customers, are excellent additions to the platform.
However, there's a contradiction in the visibility of wholesale prices for customers:
- "customer can see wholesale price on shop archive: true"
- "customer can't see wholesale price on shop archive: true"
These two settings contradict each other. We need to clarify which one is correct to ensure consistent behavior in the application.
Please resolve this contradiction by setting only one of these to true based on the intended behavior:
- customer can see wholesale price on shop archive: true - customer can't see wholesale price on shop archive: true + customer can see wholesale price on shop archive: true + customer can't see wholesale price on shop archive: falseOr vice versa, depending on the desired behavior.
To help identify any recent changes or discussions about wholesale price visibility, please run the following script:
tests/pw/utils/dbData.ts (1)
1423-1446: LGTM! New payment settings object added.The new
paymentSettingsobject has been correctly added to thetestData.dokanobject. It includes configurations for various payment methods (PayPal, bank transfer, custom Dokan method, and Skrill), which is consistent with the AI-generated summary.A few observations:
- The structure is well-organized and easy to read.
- Each payment method has its own sub-object with relevant fields.
- Sensitive information (like email addresses and account numbers) is using placeholder values, which is a good practice for test data.
To ensure this change doesn't conflict with any existing payment settings, let's verify if there are any other payment-related configurations in the file:
tests/pw/tests/e2e/payments.spec.ts (14)
6-8: Imports for Database Utilities and HelpersThe imports
dbUtils,dbData, andhelpersare appropriately added to support the new database interactions and helper functions used in the tests.
10-10: Ensure Environment VariableVENDOR_IDis Properly ConfiguredThe
VENDOR_IDis retrieved fromprocess.env. Please verify that this environment variable is set correctly in all testing environments to prevent unexpected errors.
33-33: Verify Reset of Payment Settings inafterAllHookThe
afterAllhook updates the user meta:await dbUtils.updateUserMeta(VENDOR_ID, 'dokan_profile_settings', dbData.testData.dokan.paymentSettings);Ensure that
dbData.testData.dokan.paymentSettingscontains the correct default values to reset the payment settings after the tests.
98-99: Verify Helper Function Outputs for AccuracyYou're using:
await dbUtils.updateUserMeta(VENDOR_ID, 'dokan_profile_settings', helpers.emptyObjectValues(dbData.testData.dokan.paymentSettings.bank));Ensure that
helpers.emptyObjectValuescorrectly processesdbData.testData.dokan.paymentSettings.bankto produce the desired test data.
87-90:⚠️ Potential issueConsistent Use of Application APIs Over Direct DB Updates
Similar to previous feedback, avoid direct database manipulations:
await dbUtils.updateUserMeta(VENDOR_ID, 'dokan_profile_settings', { payment: { paypal: { email: '[email protected]' } } });Use application APIs or UI actions to update payment methods to ensure tests reflect actual user interactions.
92-94:⚠️ Potential issuePrevent Bypassing Business Logic with Direct DB Updates
Directly removing payment methods via
dbUtils.updateUserMetamay skip important business logic checks. Use the application's standard mechanisms to remove payment methods.
102-104:⚠️ Potential issueUse Application Interfaces for Updating Payment Methods
Updating payment methods should be done through application interfaces to maintain test integrity and simulate real user behavior.
108-109:⚠️ Potential issueAvoid Direct Database Manipulation in Tests
Once again, directly updating the database can lead to tests that do not accurately reflect user interactions.
113-114:⚠️ Potential issueUse Application APIs Instead of Direct DB Updates
For adding Skrill payment methods, avoid direct database updates:
await dbUtils.updateUserMeta(VENDOR_ID, 'dokan_profile_settings', { payment: { skrill: { email: '' } } });
118-119:⚠️ Potential issueMaintain Test Consistency with Application-Level Actions
Updating Skrill payment methods should utilize the application's APIs or UI:
await dbUtils.updateUserMeta(VENDOR_ID, 'dokan_profile_settings', { payment: { skrill: { email: '[email protected]' } } });
123-124:⚠️ Potential issueDirect Database Updates May Lead to Inaccurate Test Results
Removing Skrill payment methods via direct database manipulation may not trigger all necessary application logic.
128-129:⚠️ Potential issueSimulate User Actions Instead of Modifying the Database Directly
For custom payment methods, setting values directly in the database bypasses validation and other logic.
133-134:⚠️ Potential issueEnsure Test Reliability by Avoiding Database Shortcuts
Updating custom payment methods should be performed through the application's interfaces.
138-139:⚠️ Potential issueAccurate Test Simulation Requires Application-Level Interactions
Removing custom payment methods directly from the database does not accurately simulate user behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
tests/pw/tests/e2e/payments.spec.ts (7)
6-10: Approve new imports and environment variable with a suggestion.The new imports and environment variable are appropriate for the refactoring of vendor payment tests. They align well with the PR objectives.
Consider adding a comment explaining the purpose of the
VENDOR_IDenvironment variable for better code readability:// VENDOR_ID is used to identify the vendor in database operations const { VENDOR_ID } = process.env;
33-34: Approve afterAll hook changes with a suggestion.The changes in the afterAll hook are appropriate. Using dbUtils to reset the database state after tests is a good practice.
Consider adding a comment explaining why the apiUtils line is commented out and if it should be removed:
await dbUtils.updateUserMeta(VENDOR_ID, 'dokan_profile_settings', dbData.testData.dokan.paymentSettings); // The following line is no longer needed as we're using direct database updates instead of API calls // await apiUtils.setStoreSettings(payloads.defaultStoreSettings, payloads.vendorAuth);
82-85: Approve changes to PayPal payment method test with a suggestion.The shift from apiUtils to dbUtils for setting up the test state is consistent with the overall changes in this PR. This approach provides more direct control over the database state.
Consider adding a comment explaining the reason for this change:
// Direct database update for more precise test state control await dbUtils.updateUserMeta(VENDOR_ID, 'dokan_profile_settings', { payment: { paypal: { email: '' } } });
98-99: Approve changes to bank payment method addition test with a suggestion.The use of dbUtils and the helpers.emptyObjectValues function is a good approach for setting up a clean initial state. This change is consistent with the overall refactoring in this file.
Consider adding a comment explaining the purpose of emptying the object values:
// Set up an empty bank payment configuration to ensure a clean initial state await dbUtils.updateUserMeta(VENDOR_ID, 'dokan_profile_settings', helpers.emptyObjectValues(dbData.testData.dokan.paymentSettings.bank));
112-125: Approve addition of Skrill payment method tests with a suggestion.The new tests for adding, updating, and removing the Skrill payment method are valuable additions to the test suite. They enhance test coverage for the pro version and follow the same pattern as other tests in this file.
Consider extracting the Skrill-specific setup into a helper function to reduce duplication:
const setupSkrillPayment = async (email: string) => { await dbUtils.updateUserMeta(VENDOR_ID, 'dokan_profile_settings', { payment: { skrill: { email } } }); }; // Usage in tests: test('vendor can add skrill payment method', { tag: ['@pro', '@vendor'] }, async () => { await setupSkrillPayment(''); await vendor.addBasicPayment({ ...data.vendor.payment, methodName: 'skrill' }); }); // Similar for update and remove tests
127-139: Approve addition of custom payment method tests with a suggestion.The new tests for adding, updating, and removing the custom payment method are valuable additions to the test suite. They enhance test coverage for the pro version and follow the same pattern as other tests in this file.
Similar to the Skrill tests, consider extracting the custom payment-specific setup into a helper function:
const setupCustomPayment = async (value: string) => { await dbUtils.updateUserMeta(VENDOR_ID, 'dokan_profile_settings', { payment: { dokan_custom: { value } } }); }; // Usage in tests: test('vendor can add custom payment method', { tag: ['@pro', '@vendor'] }, async () => { await setupCustomPayment(''); await vendor.addBasicPayment({ ...data.vendor.payment, methodName: 'custom' }); }); // Similar for update and remove tests
Line range hint
1-139: Overall assessment of changes to payments.spec.tsThe refactoring of this test file aligns well with the PR objectives of enhancing the testing framework for vendor payments. Key improvements include:
- Shift from API calls to direct database operations, providing more precise control over the test environment.
- Addition of new tests for updating payment methods, enhancing overall test coverage.
- Consistent approach across different payment methods (PayPal, bank transfer, Skrill, and custom).
These changes should lead to more reliable and comprehensive testing of the vendor payment functionality.
To further improve the test suite:
- Consider creating a helper file for common database setup operations to reduce duplication across tests.
- Evaluate the possibility of running these tests in parallel to improve execution time, ensuring that each test properly isolates its database state.
- Consider adding more edge cases and error scenarios to further enhance the robustness of the test suite.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/pw/tests/e2e/payments.spec.ts (3 hunks)
🔇 Additional comments (4)
tests/pw/tests/e2e/payments.spec.ts (4)
87-90: Approve addition of PayPal payment method update test.The new test for updating the PayPal payment method is a valuable addition. It enhances test coverage and follows the same pattern as other tests in this file.
92-94: Approve changes to PayPal payment method removal test.The modification to use dbUtils for setting up the initial state in the PayPal payment method removal test is consistent with the overall approach in this file. This change provides more direct control over the test environment.
102-104: Approve addition of bank payment method update test.The new test for updating the bank payment method is a valuable addition to the test suite. It enhances test coverage and follows the same pattern as other tests in this file.
107-109: Approve changes to bank payment method removal test.The modification to use dbUtils for setting up the initial state in the bank payment method removal test is consistent with the overall approach in this file. This change provides more direct control over the test environment.
* Add: add product form tests (getdokan#2385) * Add: add product form tests (download options, inventory) * Fix : revert unwanted changes * Add & Refactor vendor payment tests (getdokan#2387) * Add: add vendor payment tests * Update: update feature map * Update: remove test.only * Add $data parameter to dokan_update_vendor hook (getdokan#2386) * Add $data parameter to dokan_update_vendor hook - Added $data array as second parameter to dokan_update_vendor action hook - Updated doc block for dokan_update_vendor hook to reflect new parameter - Added doc block for dokan_before_update_vendor hook * update: hook doc blocks * update:hook metadata with since attribute * chore: Release Version 3.12.4 --------- Co-authored-by: Al Amin Ahamed <[email protected]> Co-authored-by: Shazahanul Islam Shohag <[email protected]>
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
Documentation
Tests
Chores