Add: add vendor payment tests #53
Conversation
WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
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/tests/e2e/payments.spec.ts (1)
28-28: Remove commented-out code for code cleanlinessThe commented-out line
// await dbUtils.updateUserMeta(VENDOR_ID, 'dokan_profile_settings', { payment: [] });may not be needed. Removing unused code helps keep the codebase clean and maintainable.tests/pw/pages/paymentsPage.ts (2)
220-220: Review the Commented-Out AssertionThe assertion
await this.notToHaveCount(paymentSettingsVendor.paymentMethods.noOfPaymentMethods, 0);is commented out. If this check is necessary to validate that payment methods are rendering properly, consider uncommenting it. If it's no longer needed, consider removing it to keep the codebase clean.
266-266: Consider Removing Commented-Out CodeThe line
// await this.clickIfVisible(paymentSettingsVendor.disconnectAccount);is commented out. If this action is no longer required, consider removing the line. If it's needed for future functionality, ensure it's tracked appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- 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 (19)
tests/pw/utils/helpers.ts (2)
472-473: LGTM: Well-implemented utility methodThe
isPlainObjectmethod is correctly implemented and provides a useful utility for checking if a value is a plain object.The method effectively:
- Checks if the value is not null.
- Verifies that the value is of type 'object'.
- Ensures the value is not an array.
This implementation will be helpful in distinguishing plain objects from other types, including arrays and null values.
497-497: LGTM: Improved consistency with new utility methodThe modification to use the
isPlainObjectmethod indeepMergeObjectsis a good improvement.This change:
- Enhances code consistency by utilizing the newly defined utility method.
- Improves readability by using a descriptive method name instead of a direct type check.
- Makes the code more maintainable, as any future changes to object type checking can be centralized in the
isPlainObjectmethod.Good job on refactoring to use the new utility method!
tests/pw/utils/dbData.ts (1)
1423-1446: New payment settings added for vendor testing.The new
paymentSettingsobject has been added to thetestData.dokanobject, which appears to be used for testing purposes. This addition provides test data for various payment methods including PayPal, bank transfer, custom payment, and Skrill.However, there are a few points to consider:
Sensitive Data: The test data includes what appears to be mock sensitive information such as account numbers and IBANs. Ensure that this test data is not accidentally used in any public-facing or production environments.
Consistency: The
declarationfield in the bank settings is set to 'on'. Consider using a boolean value (true/false) for consistency with other boolean fields in the configuration.Custom Payment: The purpose of the
dokan_custompayment method is not clear. Consider adding a comment to explain its usage or provide more descriptive field names.To ensure this test data doesn't contain any real sensitive information, run the following script:
If this script returns any matches, please double-check that they are indeed mock data and not real information.
✅ Verification successful
Sensitive data in test settings verified as mock data.
The shell script confirmed that the email addresses and account numbers within the
paymentSettingsobject are mock data. No real sensitive information was found, ensuring that the test data is safe for use in non-production environments.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential real sensitive data in payment settings # Test: Look for common patterns of real account numbers or emails rg -i '\b(?:\d{10,}|[a-z0-9._%+-]+@[a-z0-9.-]+\.[a-z]{2,})\b' tests/pw/utils/dbData.tsLength of output: 269
tests/pw/tests/e2e/payments.spec.ts (12)
6-9: Approved: New imports are appropriateThe added imports from
@utils/dbUtils,@utils/dbData, and@utils/helpersare necessary for the database interactions and helper functions used in the tests.
83-83: Duplicate Issue: Avoid direct database manipulation in testsAs previously noted, using
dbUtils.updateUserMetadirectly can bypass application logic. Consider using appropriateapiUtilsmethods to ensure tests simulate real-world scenarios.
88-88: Duplicate Issue: Avoid direct database manipulation in testsRefer to the earlier comment about using
apiUtilsinstead of direct database updates for modifying user settings.
93-93: Duplicate Issue: Avoid direct database manipulation in testsSame concern as previously mentioned regarding direct database updates.
98-98: Duplicate Issue: Avoid direct database manipulation in testsPlease consider using API methods instead of direct database manipulation as discussed earlier.
103-103: Duplicate Issue: Avoid direct database manipulation in testsThis is the same issue previously noted with using
dbUtils.updateUserMetadirectly.
113-113: Duplicate Issue: Avoid direct database manipulation in testsAs before, consider using
apiUtilsmethods for updating user settings.
118-118: Duplicate Issue: Avoid direct database manipulation in testsRefer to the earlier comments about using API methods over direct database updates.
123-123: Duplicate Issue: Avoid direct database manipulation in testsSame concern applies here regarding direct manipulation of the database.
128-128: Duplicate Issue: Avoid direct database manipulation in testsPlease use
apiUtilsfor modifying user settings to ensure consistency with previous comments.
133-133: Duplicate Issue: Avoid direct database manipulation in testsConsider the earlier advice about using API methods for updating user settings.
138-138: Duplicate Issue: Avoid direct database manipulation in testsAs previously mentioned, avoid using direct database updates in tests.
tests/pw/pages/paymentsPage.ts (4)
19-21: Correct Implementation of thegoToWcPaymentSettings()MethodThe
goToWcPaymentSettings()method correctly navigates to the WooCommerce payment settings page usingawait this.goIfNotThere(data.subUrls.backend.wc.paymentSettings);.
25-25: Consistent Update to UsegoToWcPaymentSettings()The calls to
goToWcPaymentSettings()in the payment setup methods ensure that each method navigates to the correct WooCommerce payment settings page before proceeding.Also applies to: 41-41, 66-66, 94-94, 141-141, 165-165
248-252: Correct Implementation of thesetBasicPayment()MethodThe
setBasicPayment()method accurately navigates to the specified payment settings page and updates the payment email. The method ensures that the changes are saved and verifies success.
255-260: Proper Addition ofdisconnectBasicPayment()MethodThe
disconnectBasicPayment()method effectively handles the disconnection of a basic payment method by navigating to the appropriate settings page and performing the disconnect action.
| import { dbData } from '@utils/dbData'; | ||
| import { helpers } from '@utils/helpers'; | ||
|
|
||
| const { VENDOR_ID } = process.env; |
There was a problem hiding this comment.
Add error handling for undefined VENDOR_ID
VENDOR_ID is retrieved from process.env, but there is no check to ensure it is defined. To prevent potential runtime errors during test execution, consider adding validation or a default value if VENDOR_ID is not set.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
tests/pw/tests/e2e/payments.spec.ts (5)
92-94: Approve new PayPal removal test with suggestionThe new test for removing the PayPal payment method is a good addition, improving coverage of the PayPal functionality. However, consider using API calls instead of direct database manipulation for setting up the initial state:
await apiUtils.setStoreSettings({ payment: { paypal: { email: '[email protected]' } } }, payloads.vendorAuth);This approach would make the test more representative of real-world scenarios.
98-104: Approve bank payment method tests with suggestions
The tests for adding and updating bank payment methods are good additions, improving coverage of the functionality.
However, consider using API calls instead of direct database manipulation:
await apiUtils.setStoreSettings({ payment: { bank: emptyBankSettings } }, payloads.vendorAuth);This approach would make the tests more representative of real-world scenarios and help catch potential issues in the application's data handling.
The use of
helpers.emptyObjectValues()is a good practice for ensuring clean test data.
107-109: Approve bank payment method removal test with suggestionThe new test for removing the bank payment method is a good addition, improving coverage of the functionality. However, consider using API calls instead of direct database manipulation for setting up the initial state:
await apiUtils.setStoreSettings(dbData.testData.dokan.paymentSettings.bank, payloads.vendorAuth);This approach would make the test more representative of real-world scenarios.
112-125: Approve Skrill payment method tests with suggestionsThe new tests for adding, updating, and removing the Skrill payment method are excellent additions, improving coverage of the functionality. However, consider using API calls instead of direct database manipulation:
await apiUtils.setStoreSettings({ payment: { skrill: { email: '' } } }, payloads.vendorAuth); await apiUtils.setStoreSettings({ payment: { skrill: { email: '[email protected]' } } }, payloads.vendorAuth);This approach would make the tests more representative of real-world scenarios and help catch potential issues in the application's data handling.
Also, ensure that the Skrill payment method is properly tagged as a pro feature, as indicated by the
@protag in the test descriptions.
127-139: Approve custom payment method tests with suggestionsThe new tests for adding, updating, and removing the custom payment method are excellent additions, improving coverage of the functionality. However, consider using API calls instead of direct database manipulation:
await apiUtils.setStoreSettings({ payment: { dokan_custom: { value: '' } } }, payloads.vendorAuth); await apiUtils.setStoreSettings({ payment: { dokan_custom: { value: '0123456789' } } }, payloads.vendorAuth);This approach would make the tests more representative of real-world scenarios and help catch potential issues in the application's data handling.
Also, ensure that the custom payment method is properly tagged as a pro feature, as indicated by the
@protag in the test descriptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- 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)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/pw/pages/paymentsPage.ts
🔇 Additional comments (5)
tests/pw/tests/e2e/payments.spec.ts (3)
Line range hint
1-139: Overall assessment of payment method testsThe changes in this file significantly improve the test coverage for various payment methods, including PayPal, bank transfer, Skrill, and custom payment methods. The addition of tests for adding, updating, and removing each payment method is commendable and aligns well with the PR objectives.
However, there are some areas for improvement:
Consider using API calls instead of direct database manipulation throughout the tests. This would make the tests more representative of real-world scenarios and help catch potential issues in the application's data handling.
Ensure proper error handling for the
VENDOR_IDenvironment variable to prevent potential runtime errors.Review the commented-out code in the
beforeAllhook and either implement or remove it.Verify that all pro features are correctly tagged with
@proin the test descriptions.These changes will enhance the reliability and maintainability of the test suite while ensuring it accurately reflects real-world usage of the application.
6-10:⚠️ Potential issueAdd error handling for undefined
VENDOR_IDThe
VENDOR_IDis retrieved fromprocess.envwithout any validation. To prevent potential runtime errors during test execution, consider adding validation or a default value ifVENDOR_IDis not set.Example:
const VENDOR_ID = process.env.VENDOR_ID || throw new Error('VENDOR_ID is not set');
28-34:⚠️ Potential issueReview database operations in setup and teardown
There's a commented-out line in the
beforeAllhook:// await dbUtils.updateUserMeta(VENDOR_ID, 'dokan_profile_settings', { payment: [] });If this operation is necessary for test setup, consider uncommenting and implementing it. If not, remove the comment to keep the code clean.
In the
afterAllhook, direct database manipulation is used instead of API calls:await dbUtils.updateUserMeta(VENDOR_ID, 'dokan_profile_settings', dbData.testData.dokan.paymentSettings); // await apiUtils.setStoreSettings(payloads.defaultStoreSettings, payloads.vendorAuth);While this approach might be faster, it bypasses application logic and validation layers. Consider using API calls to ensure tests accurately reflect real-world scenarios and catch potential issues in the application's data handling.
tests/pw/feature-map/feature-map.yml (2)
187-197: LGTM! Changes align well with PR objectives.The modifications to vendor permissions for payment methods are consistent with the PR's goal of adding vendor payment tests. These changes expand vendor capabilities in managing various payment methods (PayPal, bank, Skrill, and custom), allowing them to update and remove payment methods in addition to adding them.
Line range hint
1-1037: Overall changes look good and are well-focused.The modifications in this file are concentrated in the "Payments" section and align well with the PR objective of adding vendor payment tests. The changes expand vendor capabilities for managing various payment methods, which should facilitate more comprehensive testing of vendor payment functionalities.
No unintended modifications or inconsistencies were found in other parts of the file, maintaining the integrity of existing feature permissions for different user roles and pages.
| // await apiUtils.setStoreSettings({ payment: { paypal: { email: '' } } }, payloads.vendorAuth); | ||
| await dbUtils.updateUserMeta(VENDOR_ID, 'dokan_profile_settings', { payment: { paypal: { email: '' } } }); | ||
| await vendor.addBasicPayment({ ...data.vendor.payment, methodName: 'paypal' }); | ||
| }); | ||
|
|
||
| test('vendor can update paypal payment method', { tag: ['@lite', '@vendor'] }, async () => { | ||
| await dbUtils.updateUserMeta(VENDOR_ID, 'dokan_profile_settings', { payment: { paypal: { email: '[email protected]' } } }); | ||
| await vendor.addBasicPayment({ ...data.vendor.payment, methodName: 'paypal' }); | ||
| }); |
There was a problem hiding this comment.
Consider using API calls for more realistic tests
-
The test now uses direct database manipulation instead of API calls:
await dbUtils.updateUserMeta(VENDOR_ID, 'dokan_profile_settings', { payment: { paypal: { email: '' } } });
While this approach might be faster, it bypasses application logic and validation layers. Consider using API calls to ensure tests accurately reflect real-world scenarios and catch potential issues in the application's data handling.
-
The new test for updating the PayPal payment method is a good addition, improving coverage of the PayPal functionality.
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
Chores