Add Printful module tests #2432
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several changes across multiple files, primarily enhancing testing capabilities and integrating Printful functionalities. Key modifications include updates to the Changes
Possibly related PRs
Suggested reviewers
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: 8
🧹 Outside diff range and nitpick comments (14)
tests/pw/tests/e2e/printful.spec.ts (3)
6-6: Document why tests are skipped.The test suite is marked with
skipbut there's no explanation why. Add a comment explaining the reason (e.g., if it requires specific setup, is work in progress, or has known issues).+// TODO: Enable tests once Printful API credentials are configured in the test environment test.describe.skip('Printful test', () => {
11-16: Add prerequisite checks in beforeAll hook.The setup assumes Printful vendor authentication will work but doesn't validate it. Consider adding checks for required environment variables and authentication success.
test.beforeAll(async ({ browser }) => { + // Verify required environment variables + if (!process.env.PRINTFUL_APP_ID || !process.env.PRINTFUL_APP_SECRET) { + throw new Error('Printful credentials not configured'); + } const customerContext = await browser.newContext(data.auth.vendorAuth); vPage = await customerContext.newPage(); vendor = new PrintfulPage(vPage); apiUtils = new ApiUtils(await request.newContext()); });
18-21: Add error handling in cleanup.The
afterAllhook should handle potential errors during cleanup to ensure resources are properly released even if one operation fails.test.afterAll(async () => { - await vPage.close(); - await apiUtils.dispose(); + try { + await vPage?.close(); + } finally { + await apiUtils?.dispose(); + } });tests/pw/pages/privacyPolicyPage.ts (1)
Line range hint
11-53: Consider adding JSDoc comments to document the class and methods.While the code is well-structured, adding JSDoc comments would improve documentation and provide better IDE support. This is particularly important for test code that other team members will maintain.
Example improvement:
+/** + * Page object representing the Privacy Policy page and its interactions. + * Handles vendor contact form and privacy policy related test scenarios. + */ export class PrivacyPolicyPage extends CustomerPage { constructor(page: Page) { super(page); } + /** + * Contacts a vendor through the store contact form. + * @param storeName - The name of the store to contact + * @param storeContactData - Contact form data including name, email, and message + */ async contactVendor(storeName: string, storeContactData: storeContactData) {tests/pw/pages/printfulPage.ts (2)
9-12: Remove unnecessary constructorThe constructor can be removed as it only calls the parent constructor with the same parameter, which TypeScript does automatically.
export class PrintfulPage extends VendorPage { - constructor(page: Page) { - super(page); - }🧰 Tools
🪛 Biome
[error] 10-12: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
15-20: Address TODO comment and consider renaming method
- The TODO comment suggests incomplete implementation regarding connect/disconnect visibility checks.
- Consider renaming the method to something more concise like
verifySettingsPageElements().Would you like me to help implement the connect/disconnect visibility check logic?
tests/pw/tests/e2e/shortcodes.spec.ts (1)
50-56: Consider enhancing test cleanup reliabilityGood practices observed:
- Setting up required database state before test
- Cleaning up after test to prevent test pollution
However, consider these improvements for better reliability:
test.describe('Shortcodes test', () => { let admin: ShortcodePage; let vendor: ShortcodePage; let customer: ShortcodePage; let aPage: Page, vPage: Page, cPage: Page; let apiUtils: ApiUtils; + let originalSubscriptionSettings: any; test.beforeAll(async ({ browser }) => { // ... existing setup ... + // Store original settings + originalSubscriptionSettings = await dbUtils.getOptionValue(dbData.dokan.optionName.vendorSubscription); }); test.afterAll(async () => { // ... existing cleanup ... + // Ensure settings are always reset + await dbUtils.setOptionValue(dbData.dokan.optionName.vendorSubscription, originalSubscriptionSettings); }); test('vendor can view Dokan subscription packs (shortcode)', { tag: ['@pro', '@admin'] }, async () => { await dbUtils.updateOptionValue(dbData.dokan.optionName.vendorSubscription, { enable_pricing: 'on' }); const [responseBody, pageId] = await apiUtils.createPage(payloads.dokanSubscriptionPackShortcode, payloads.adminAuth); - await vendor.viewDokanSubscriptionPacks(responseBody.link); - await apiUtils.deletePage(pageId, payloads.adminAuth); - - // reset settings - await dbUtils.setOptionValue(dbData.dokan.optionName.vendorSubscription, dbData.dokan.vendorSubscriptionSettings); + try { + await vendor.viewDokanSubscriptionPacks(responseBody.link); + } finally { + await apiUtils.deletePage(pageId, payloads.adminAuth); + } });This refactor:
- Moves settings cleanup to
afterAllto ensure it runs even if tests fail- Adds try/finally to ensure page cleanup happens even if test fails
- Stores original settings to restore exact state
tests/pw/tests/e2e/settings.spec.ts (1)
143-145: Consider adding error handling.The test implementation follows the established pattern and is well-placed within the settings test suite. However, consider adding error handling for potential API failures.
test('admin can set Dokan Printful settings', { tag: ['@pro', '@admin'] }, async () => { - await admin.setDokanPrintfulSettings(data.dokanSettings.printful); + try { + await admin.setDokanPrintfulSettings(data.dokanSettings.printful); + } catch (error) { + console.error('Failed to set Printful settings:', error); + throw error; + } });tests/pw/tests/e2e/_env.setup.ts (1)
272-274: Consider adding validation and error handlingThe test follows the established pattern for setting Dokan settings, which is good. However, consider these improvements:
- Add an assertion to verify the option was set correctly
- Add error handling for potential database operation failures
Here's a suggested implementation:
setup('admin set dokan printful settings', { tag: ['@pro'] }, async () => { - await dbUtils.setOptionValue(dbData.dokan.optionName.printful, dbData.dokan.printful); + try { + await dbUtils.setOptionValue(dbData.dokan.optionName.printful, dbData.dokan.printful); + const savedValue = await dbUtils.getOptionValue(dbData.dokan.optionName.printful); + expect(savedValue).toEqual(dbData.dokan.printful); + } catch (error) { + console.error('Failed to set Printful settings:', error); + throw error; + } });tests/pw/utils/interfaces.ts (1)
1814-1830: The interface structure looks good, but could benefit from additional documentation and type safety.The
printfulinterface is well-organized with clear property names and appropriate types. However, consider these improvements:Consider applying these enhancements:
// printful Settings printful: { + /** Title displayed in the settings section */ settingTitle: string; + /** Client ID for Printful API authentication */ clientId: string; + /** Secret key for Printful API authentication */ secretKey: string; + /** UI Customization Settings */ popupTitle: string; popupTextColor: string; popupBackgroundColor: string; tabBackgroundColor: string; activeTabBackgroundColor: string; sizeGuideButtonText: string; buttonTextColor: string; + /** Measurement Settings */ primaryMeasurementUnit: string; + /** Product Options - Consider using a more specific type */ - optionNames: string[]; - optionValues: string[]; + optionNames: Array<'size' | 'color' | 'style'>; + optionValues: Array<string>; saveSuccessMessage: string; };tests/pw/feature-map/feature-map.yml (1)
806-821: Consider expanding test coverage for the Printful module.The Printful module test scenarios could benefit from additional coverage in the following areas:
- Error handling scenarios (e.g., failed connection attempts)
- Edge cases in shipping combinations
- Product synchronization features
tests/pw/utils/dbData.ts (1)
1088-1102: Consider adding validation for color values.The Printful configuration includes several color-related settings that would benefit from validation to ensure they contain valid hex color codes.
Consider adding validation or using a type system to ensure color values are valid hex codes:
printful: { app: '', size_guide_sub_section: '', popup_title: 'Size Guide', - popup_text_color: '#000000', - popup_bg_color: '#FFFFFF', - tab_bg_color: '#EEEEEE', - active_tab_bg_color: '#DDDDDD', + popup_text_color: validateHexColor('#000000'), + popup_bg_color: validateHexColor('#FFFFFF'), + tab_bg_color: validateHexColor('#EEEEEE'), + active_tab_bg_color: validateHexColor('#DDDDDD'), size_guide_button_text: 'Size Guide', - button_text_color: '#1064A9', + button_text_color: validateHexColor('#1064A9'), primary_measurement_unit: 'inches', app_id: PRINTFUL_APP_ID ?? '', app_secret: PRINTFUL_APP_SECRET ?? '', dashboard_menu_manager: [], },Add this helper function:
function validateHexColor(color: string): string { const hexRegex = /^#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})$/; if (!hexRegex.test(color)) { throw new Error(`Invalid hex color code: ${color}`); } return color; }tests/pw/pages/basePage.ts (1)
1739-1755: Consider extracting the color constant to improve maintainability.The RGB color value 'rgb(33, 150, 243)' is hardcoded in both methods. Consider extracting it to a class constant or configuration value for better maintainability.
export class BasePage { + private static readonly VENDOR_DASHBOARD_SWITCHER_ENABLED_COLOR = 'rgb(33, 150, 243)'; async enableSwitcherVendorDashboard(selector: string): Promise<void> { selector = /^(\/\/|\(\/\/)/.test(selector) ? `${selector}//span` : `${selector} span`; const value = await this.getElementBackgroundColor(selector); - if (!value.includes('rgb(33, 150, 243)')) { + if (!value.includes(BasePage.VENDOR_DASHBOARD_SWITCHER_ENABLED_COLOR)) { await this.click(selector); } } async disableSwitcherVendorDashboard(selector: string): Promise<void> { selector = /^(\/\/|\(\/\/)/.test(selector) ? `${selector}//span` : `${selector} span`; const value = await this.getElementBackgroundColor(selector); - if (value.includes('rgb(33, 150, 243)')) { + if (value.includes(BasePage.VENDOR_DASHBOARD_SWITCHER_ENABLED_COLOR)) { await this.click(selector); } } }tests/pw/pages/selectors.ts (1)
5932-5942: Consider using consistent indentation for better readability.The indentation in this section appears to be inconsistent with the rest of the file. Consider adjusting to match the surrounding code.
- store: 'li.store > a', - addons: 'li.product-addon > a', - payment: 'li.payment > a', - verification: 'li.verification > a', - deliveryTime: 'li.delivery-time > a', - shipping: 'li.shipping > a', - shipStation: 'li.shipstation > a', - socialProfile: 'li.social > a', - rma: 'li.rma > a', - printful: 'li.printful a', - storeSEO: 'li.seo > a', + store: 'li.store > a', + addons: 'li.product-addon > a', + payment: 'li.payment > a', + verification: 'li.verification > a', + deliveryTime: 'li.delivery-time > a', + shipping: 'li.shipping > a', + shipStation: 'li.shipstation > a', + socialProfile: 'li.social > a', + rma: 'li.rma > a', + printful: 'li.printful a', + storeSEO: 'li.seo > a',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
tests/pw/.gitignore(1 hunks)tests/pw/feature-map/feature-map.yml(2 hunks)tests/pw/pages/basePage.ts(1 hunks)tests/pw/pages/colorsPage.ts(2 hunks)tests/pw/pages/printfulPage.ts(1 hunks)tests/pw/pages/privacyPolicyPage.ts(1 hunks)tests/pw/pages/selectors.ts(4 hunks)tests/pw/pages/settingsPage.ts(1 hunks)tests/pw/pages/withdrawsPage.ts(1 hunks)tests/pw/tests/e2e/_env.setup.ts(1 hunks)tests/pw/tests/e2e/colors.spec.ts(1 hunks)tests/pw/tests/e2e/liveChat.spec.ts(2 hunks)tests/pw/tests/e2e/printful.spec.ts(1 hunks)tests/pw/tests/e2e/settings.spec.ts(1 hunks)tests/pw/tests/e2e/shortcodes.spec.ts(2 hunks)tests/pw/types/environment.d.ts(1 hunks)tests/pw/utils/dbData.ts(5 hunks)tests/pw/utils/interfaces.ts(1 hunks)tests/pw/utils/testData.ts(5 hunks)
🧰 Additional context used
🪛 Biome
tests/pw/pages/printfulPage.ts
[error] 10-12: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (27)
tests/pw/tests/e2e/colors.spec.ts (1)
23-23:
Please provide more context about the skipped test.
The test skip seems unrelated to this PR's objective of adding Printful module tests. A few concerns:
- The skip reason "need to fix" is too vague. What specifically needs to be fixed?
- There's no reference to a tracking issue or JIRA ticket.
- This change appears to be outside the scope of adding Printful module tests.
Let's check if there are any related issues:
Please either:
- Add a detailed comment explaining what needs fixing and create a tracking issue, or
- Remove this change and handle it in a separate PR focused on fixing the color palette tests.
tests/pw/tests/e2e/printful.spec.ts (2)
1-5: LGTM! Well-structured imports following best practices.
The imports are properly organized, following the page object model pattern and maintaining good separation of concerns between UI and API utilities.
6-46: Verify test coverage against feature requirements.
Let's ensure all Printful module features are covered by the test suite.
tests/pw/types/environment.d.ts (1)
35-36: LGTM! Environment variables follow established patterns.
The new Printful-related environment variables are correctly typed and follow the existing naming conventions for third-party integration credentials.
tests/pw/pages/privacyPolicyPage.ts (1)
2-2: LGTM! Import path standardization looks good.
The change from relative to absolute import path using the @pages alias improves maintainability and follows TypeScript best practices.
tests/pw/.gitignore (2)
193-193: 🛠️ Refactor suggestion
Review and refine the ignore patterns
The current ignore patterns might need refinement:
- The relative path
../../plugins/navigates outside the test directory, which could be fragile. Consider using an absolute path or environment variable. - The pattern
_test.spec.tsmight be too broad and could accidentally ignore important test files. - Having both
plugins/and../../plugins/seems redundant.
Consider this alternative approach:
-../../plugins/
-_test.spec.ts
-_test.js
-_auth.spec.ts
+# Specific test files to ignore
+temp_*.spec.ts
+temp_*.js
+# Use environment variable or absolute path for plugins
+${DOKAN_PLUGINS_DIR}Likely invalid or redundant comment.
193-193: Verify the impact on Printful test files
Let's ensure these ignore patterns don't accidentally exclude the new Printful module tests.
✅ Verification successful
No Printful test files are excluded by the new ignore pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if any Printful-related test files would be ignored
# List all Printful-related test files
echo "Printful-related test files:"
fd -t f "printful.*\.spec\.ts$"
# Check which files would be ignored by the patterns
echo -e "\nFiles that would be ignored:"
git check-ignore $(fd -t f ".*\.spec\.ts$") | grep -i "printful"
Length of output: 4827
tests/pw/pages/printfulPage.ts (1)
39-65: Refactor hard-coded values and improve selectors
- Extract RGB values into constants
- Move span selectors to the selectors file
- Consider adding verification of actual shipping functionality
+const ENABLED_COLOR = 'rgb(33, 150, 243)';
+const DISABLED_COLOR = 'rgb(204, 204, 204)';
async enableShipping(shipping: 'printful' | 'marketplace' | 'both'): Promise<void> {
await this.goIfNotThere(data.subUrls.frontend.vDashboard.settingsPrintful);
switch (shipping) {
case 'printful':
await this.enableSwitcherVendorDashboard(vendorPrintful.printfulShipping);
- await this.toHaveBackgroundColor(vendorPrintful.printfulShipping + '//span', 'rgb(33, 150, 243)');
+ await this.toHaveBackgroundColor(vendorPrintful.printfulShippingToggle, ENABLED_COLOR);
break;✅ Verification successful
Refactor hard-coded values and improve selectors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if shipping functionality is tested elsewhere
rg -l "shipping.*test|test.*shipping" tests/
Length of output: 452
tests/pw/tests/e2e/liveChat.spec.ts (2)
6-6: LGTM!
The import statement is properly placed with other utility imports and is used within the test setup.
25-25: Verify the necessity of permalink rewrite.
The permalink rewrite command has been added after updating the live chat user meta. While this is a common requirement when URL structures change, we should verify if this is necessary for all live chat test scenarios.
Consider documenting the reason for adding the permalink rewrite in a code comment to clarify its necessity in the test setup.
tests/pw/pages/colorsPage.ts (1)
70-70: LGTM! Good improvement in test reliability.
Replacing hover simulation with direct class manipulation is a more reliable approach for testing state changes. This change reduces test flakiness as it doesn't depend on browser-specific hover behavior.
tests/pw/tests/e2e/shortcodes.spec.ts (1)
6-7: LGTM! Clean import additions
The new database utility imports are well-organized and follow the established import pattern.
tests/pw/pages/withdrawsPage.ts (1)
228-228: LGTM! Method name change improves clarity.
The rename from enableSwitcherDisbursement to enableSwitcherVendorDashboard better reflects the context and aligns with the method name in the BasePage class.
Let's verify that all occurrences of the old method name have been updated:
✅ Verification successful
Change Verified: All method occurrences have been updated successfully.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining instances of the old method name
# Expected: No occurrences of 'enableSwitcherDisbursement' should be found
echo "Searching for old method name..."
rg "enableSwitcherDisbursement"
echo "Searching for new method name for context..."
rg "enableSwitcherVendorDashboard"
Length of output: 975
tests/pw/feature-map/feature-map.yml (2)
810-816: LGTM: Vendor features cover essential functionality.
The vendor features appropriately cover:
- Account connectivity (connect/disconnect)
- Shipping method configuration
- Menu page visibility
817-820: Verify if customer features are intentionally disabled.
All customer-related features are set to false. Please confirm if this is intentional or if these features should be enabled as part of the implementation.
✅ Verification successful
Customer features are intentionally disabled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Printful customer features are implemented in the codebase
# Search for Printful-related customer functionality
rg -l "printful.*customer|customer.*printful" --type php
# Search for Printful purchase-related code
rg -l "printful.*purchase|purchase.*printful" --type php
Length of output: 117
Script:
#!/bin/bash
# Description: Check if Printful customer features are implemented in YAML and JavaScript files
# Search for Printful-related customer functionality in YAML files
rg -l "printful.*customer|customer.*printful" --type yml
# Search for Printful-related customer functionality in JavaScript files
rg -l "printful.*customer|customer.*printful" --type js
# Search for Printful purchase-related code in YAML files
rg -l "printful.*purchase|purchase.*printful" --type yml
# Search for Printful purchase-related code in JavaScript files
rg -l "printful.*purchase|purchase.*printful" --type js
Length of output: 289
Script:
#!/bin/bash
# Description: Check if Printful customer features are implemented in YAML and JavaScript files
# Search for Printful-related customer functionality in YAML files
rg -l "printful.*customer|customer.*printful" --type yaml
# Search for Printful-related customer functionality in JavaScript files
rg -l "printful.*customer|customer.*printful" --type js
# Search for Printful purchase-related code in YAML files
rg -l "printful.*purchase|purchase.*printful" --type yaml
# Search for Printful purchase-related code in JavaScript files
rg -l "printful.*purchase|purchase.*printful" --type js
Length of output: 309
tests/pw/utils/dbData.ts (3)
1-1: LGTM! Environment variables are properly structured.
The addition of Printful-related environment variables follows the existing pattern and maintains consistency with other third-party integrations.
905-906: LGTM! Proper handling of TalkJS credentials.
The implementation uses nullish coalescing operator for safe fallbacks, consistent with best practices for handling potentially undefined environment variables.
1146-1146: LGTM! Module properly added to the list.
The 'printful' module is correctly added to the modules array, maintaining alphabetical ordering.
tests/pw/pages/basePage.ts (2)
1739-1747: LGTM! The enableSwitcherVendorDashboard method is well implemented.
The method correctly handles both XPath and CSS selectors, and follows the established pattern for switcher methods in the class.
1748-1755: LGTM! The disableSwitcherVendorDashboard method is well implemented.
The method maintains consistency with the enable method and properly handles the switcher state.
tests/pw/utils/testData.ts (5)
32-33: LGTM: Environment variables properly defined
The Printful integration credentials are correctly added as environment variables following the established pattern.
1087-1088: LGTM: Vendor dashboard Printful settings properly configured
The Printful settings paths are correctly added to the vendor dashboard configuration:
- Settings page path follows the established pattern
- OAuth authorization URL is properly defined for authentication
1917-1919: LGTM: Module statistics correctly updated
The module statistics are properly updated to reflect the addition of the Printful module:
- Total modules increment is accurate
- Video links and product management counts are consistently updated
1952-1952: LGTM: Printful module properly added to modules list
The Printful module is correctly added to the modules array, maintaining alphabetical ordering.
2440-2456: LGTM: Printful settings configuration is comprehensive
The Printful settings configuration is well-structured and includes all necessary components:
- API credentials configuration
- UI customization options
- Measurement preferences
- Success messages
tests/pw/pages/selectors.ts (2)
2548-2563: LGTM! The Printful settings selectors are well-structured.
The implementation follows the existing selector patterns and uses appropriate locator strategies. The selectors cover all necessary UI elements for Printful integration including authentication, size guide settings and color picker functionality.
2017-2017: LGTM! The Printful menu item selector is properly added.
The selector for the Printful menu item is correctly added to the settings menu section.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (14)
tests/pw/.env.example (1)
17-18: Enhance documentation for Printful integrationThe comments could be more descriptive about the Printful integration requirements and where to obtain these credentials.
Consider updating the comments to include:
- Purpose of the Printful integration
- Link to Printful's API documentation
- Steps to obtain these credentials from Printful dashboard
tests/pw/tests/e2e/liveSearch.spec.ts (1)
Line range hint
26-46: Consider improving test isolationWhile the tests are well-structured, modifying database state during test execution (
updateOptionValue) could lead to flaky tests if they're run in parallel or if a test fails mid-execution.Consider these improvements:
test.describe('Live search test', () => { + test.describe.configure({ mode: 'serial' }); // Ensure sequential execution + // ... existing code ... test('customer can search product using live search (autoload content)', { tag: ['@pro', '@customer'] }, async () => { - await dbUtils.updateOptionValue(dbData.dokan.optionName.liveSearch, { live_search_option: 'old_live_search' }); + // Move DB updates to beforeAll/beforeEach hooks for better isolation await customer.searchByLiveSearch(data.predefined.simpleProduct.product1.name, true); });tests/pw/pages/printfulPage.ts (1)
10-12: Remove unnecessary constructorSince the constructor only calls
super(page)without any additional initialization, it can be safely removed. The parent class constructor will be called automatically.- constructor(page: Page) { - super(page); - }🧰 Tools
🪛 Biome
[error] 10-12: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
tests/pw/pages/menuManagerPage.ts (1)
67-68: Consider adding error handling for save operation failures.While the implementation is correct, consider adding try-catch blocks to handle potential save operation failures gracefully. This would improve test reliability and debugging.
- await this.clickAndWaitForResponseAndLoadState(data.subUrls.ajax, settingsAdmin.saveChanges); + try { + await this.clickAndWaitForResponseAndLoadState(data.subUrls.ajax, settingsAdmin.saveChanges); + } catch (error) { + throw new Error(`Failed to save menu rename changes: ${error.message}`); + }tests/pw/pages/productAdvertisingPage.ts (2)
Line range hint
108-127: Consider using TypeScript union type for filter actions.The
actionparameter could be more type-safe by using a union type instead of a string.Consider this improvement:
-async filterAdvertisedProduct(input: string, action: string) { +type FilterAction = 'by-store' | 'by-creation'; +async filterAdvertisedProduct(input: string, action: FilterAction) {
Line range hint
147-173: Enhance type safety for bulk actions.The
actionparameter inproductAdvertisingBulkActioncould benefit from type safety, and the optionalproductNameparameter suggests this method might be doing too much.Consider these improvements:
-async productAdvertisingBulkAction(action: string, productName?: string) { +type BulkAction = 'delete' | 'expire'; // add other valid actions +async productAdvertisingBulkAction(action: BulkAction, productName?: string) {Also consider splitting the method into two separate methods for better single responsibility:
performBulkAction(action: BulkAction)performBulkActionOnProduct(action: BulkAction, productName: string)tests/pw/pages/euCompliancePage.ts (2)
Line range hint
71-72: Address TODO comments for incomplete assertions.Several TODO comments indicate missing assertions across different test cases. These gaps in test coverage should be addressed to ensure comprehensive validation of the EU compliance settings.
Would you like me to help generate the additional assertions for each case? Here's what we could verify:
- For
euVendor: Verify field visibility and proper data persistence- For
euVendorReg: Validate registration form field presence- For
euCustomer: Check customer-specific field states- For
germanizedSupport: Verify Germanized support features- For
overrideInvoice: Validate invoice number override functionalityAlso applies to: 81-82, 87-88, 91-92, 95-96
Line range hint
1-199: Well-structured test implementation following best practices.The test implementation demonstrates several strengths:
- Clear organization by user roles (admin, vendor, customer)
- Consistent use of wait strategies for async operations
- Comprehensive assertions after state changes
- Proper error handling through parent class methods
Consider extracting common EU compliance validation logic into shared helper methods to reduce duplication across test cases.
tests/pw/pages/reverseWithdrawsPage.ts (4)
Line range hint
43-44: Remove commented-out code.The commented-out line for clearing filters appears to be obsolete. If this code is no longer needed, it should be removed to maintain code cleanliness.
- //await this.clickAndWaitForResponse(data.subUrls.api.dokan.reverseWithdraws, reverseWithdrawAdmin.filters.clearFilter);
Line range hint
71-74: Remove commented-out alternative implementation.These commented lines appear to be an alternative implementation for vendor selection. If the current implementation is preferred, remove these comments to improve code readability.
- // await this.toContainText(reverseWithdrawAdmin.addReverseWithdrawal.searchedResult, reverseWithdrawal.store); - // await this.press(data.key.arrowDown); - // await this.press(data.key.enter);
Line range hint
42-83: Consider refactoring API endpoints into a configuration file.Methods like
filterReverseWithdrawsandaddReverseWithdrawaluse hard-coded API endpoints fromdata.subUrls.api.dokan. Consider moving these endpoints to a centralized configuration file for better maintainability.Example configuration structure:
// config/endpoints.ts export const API_ENDPOINTS = { dokan: { reverseWithdraws: '/reverse-withdraws', stores: '/stores', products: '/products' } };
Line range hint
42-52: Consider adding error handling for API responses.The methods
filterReverseWithdrawsandvendorFilterReverseWithdrawalsmake API calls but don't handle potential error responses. Consider adding try-catch blocks with appropriate error handling.Example implementation:
async filterReverseWithdraws(vendorName: string) { try { await this.goto(data.subUrls.backend.dokan.reverseWithdraws); await this.click(reverseWithdrawAdmin.filters.filterByStore); const response = await this.typeAndWaitForResponse( data.subUrls.api.dokan.reverseWithdraws, reverseWithdrawAdmin.filters.filterInput, vendorName ); if (!response.ok()) { throw new Error(`API request failed with status ${response.status()}`); } await this.clickAndWaitForResponseAndLoadState( data.subUrls.api.dokan.reverseWithdraws, reverseWithdrawAdmin.filters.filteredResult(vendorName) ); await this.toBeVisible(reverseWithdrawAdmin.reverseWithdrawCell(vendorName)); } catch (error) { console.error('Failed to filter reverse withdraws:', error); throw error; } }Also applies to: 156-165
tests/pw/pages/vendorVerificationsPage.ts (1)
98-99: Consider adding error handling for failed updatesWhile the implementation is correct, consider adding error handling for scenarios where the update fails. This would improve test robustness.
- await this.clickAndWaitForResponseAndLoadState(data.subUrls.ajax, settingsAdmin.saveChanges); - await this.toContainText(settingsAdmin.dokanUpdateSuccessMessage, data.dokanSettings.vendorVerification.saveSuccessMessage); + try { + await this.clickAndWaitForResponseAndLoadState(data.subUrls.ajax, settingsAdmin.saveChanges); + await this.toContainText(settingsAdmin.dokanUpdateSuccessMessage, data.dokanSettings.vendorVerification.saveSuccessMessage); + } catch (error) { + throw new Error(`Failed to save verification method changes: ${error.message}`); + }tests/pw/pages/selectors.ts (1)
Line range hint
1981-2018: Add missing comma after saveSuccessMessageThere is a syntax error - missing comma after the
saveSuccessMessageproperty in the settings menu section.Apply this diff to fix the syntax error:
- saveSuccessMessage: 'Setting has been saved successfully.' + saveSuccessMessage: 'Setting has been saved successfully.',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
tests/pw/.env.example(1 hunks)tests/pw/README.MD(1 hunks)tests/pw/pages/catalogModePage.ts(1 hunks)tests/pw/pages/colorsPage.ts(1 hunks)tests/pw/pages/euCompliancePage.ts(1 hunks)tests/pw/pages/menuManagerPage.ts(5 hunks)tests/pw/pages/printfulPage.ts(1 hunks)tests/pw/pages/productAdvertisingPage.ts(1 hunks)tests/pw/pages/reverseWithdrawsPage.ts(1 hunks)tests/pw/pages/selectors.ts(6 hunks)tests/pw/pages/settingsPage.ts(28 hunks)tests/pw/pages/vendorVerificationsPage.ts(5 hunks)tests/pw/tests/e2e/liveSearch.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/pw/pages/colorsPage.ts
🧰 Additional context used
🪛 Gitleaks
tests/pw/README.MD
131-131: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Biome
tests/pw/pages/printfulPage.ts
[error] 10-12: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (19)
tests/pw/.env.example (1)
17-18: Verify environment variable usage
Let's ensure these variables are properly referenced throughout the codebase.
✅ Verification successful
Environment variables are properly integrated and used
The verification confirms proper integration of PRINTFUL_APP_ID and PRINTFUL_APP_SECRET:
- Correctly typed in
tests/pw/types/environment.d.ts - Properly imported and used in database utilities:
tests/pw/utils/testData.ts: Used for client configurationtests/pw/utils/dbData.ts: Safely handled with nullish coalescing
- No hardcoded values found that should use these variables instead
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of Printful environment variables
# Check TypeScript type definitions
echo "Checking TypeScript definitions..."
rg "PRINTFUL_APP" tests/pw/types/
# Check database utility usage
echo "Checking database utilities..."
rg "PRINTFUL_APP" tests/pw/utils/
# Check for any hardcoded values that should use these env vars
echo "Checking for hardcoded values..."
rg --type php "printful.*['\"].*['\"]" -g '!tests/pw/.env.example'
Length of output: 1046
tests/pw/tests/e2e/liveSearch.spec.ts (2)
Line range hint 7-24: Well-structured test setup and teardown!
The test suite follows best practices with:
- Isolated browser context for customer tests
- Proper database state management
- Clean resource cleanup
1-5: Verify the removal of ApiUtils dependency
The removal of ApiUtils import suggests an architectural change. Let's verify this change is consistent across the test suite.
tests/pw/pages/printfulPage.ts (1)
14-20: Handle connection state verification properly
The commented TODO indicates uncertainty about the connection state verification. This could lead to flaky tests.
Consider checking the current connection state first and then verifying the appropriate button:
async vendorPrintfulSettingsRenderProperly(): Promise<void> {
await this.goIfNotThere(data.subUrls.frontend.vDashboard.settingsPrintful);
- // await this.toBeVisible(vendorPrintful.connectToPrintful); //todo: connect or disconnect
+ const isConnected = await this.isVisible(vendorPrintful.disconnectToPrintful);
+ if (isConnected) {
+ await this.toBeVisible(vendorPrintful.disconnectToPrintful);
+ } else {
+ await this.toBeVisible(vendorPrintful.connectToPrintful);
+ }
await this.toBeVisible(vendorPrintful.printfulShipping);
await this.toBeVisible(vendorPrintful.marketplaceShipping);
}tests/pw/pages/catalogModePage.ts (1)
30-30: LGTM! Selector standardization improves maintainability.
The change to use settingsAdmin.saveChanges aligns with the broader effort to standardize selectors across settings pages.
Let's verify the standardized selector usage across other files:
✅ Verification successful
Standardized selector is consistently used across settings pages
The verification confirms that settingsAdmin.saveChanges is widely adopted across multiple test files, with only one commented-out instance of the old-style selector pattern in menuManagerPage.ts. The change aligns perfectly with the established standardization pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the standardized selector usage across settings pages
# Expected: Multiple files using the same standardized selector
# Search for the new standardized selector
echo "Files using the standardized selector:"
rg "settingsAdmin\.saveChanges" --type ts
# Search for any remaining old-style selectors
echo -e "\nFiles potentially still using old-style selectors:"
rg "settingsAdmin\..*\..*SaveChanges" --type ts
Length of output: 6226
tests/pw/pages/menuManagerPage.ts (4)
34-35: LGTM! Good use of async handling.
The change properly uses clickAndWaitForResponseAndLoadState to ensure the save action completes before proceeding with assertions.
45-46: LGTM! Consistent implementation with the activate case.
The deactivate case properly mirrors the activate case's implementation pattern, maintaining code consistency.
109-110: LGTM! Comprehensive verification of menu reordering.
The implementation properly verifies the reordering operation in both admin and frontend contexts with appropriate index tracking.
132-133: Consider expanding reset verification coverage.
The current implementation only verifies one menu's state after reset. Consider adding assertions for all affected menus to ensure the reset operation completely restores default settings.
tests/pw/pages/euCompliancePage.ts (1)
59-59: LGTM! Selector standardization improves maintainability.
The change to use settingsAdmin.saveChanges aligns with the broader effort to standardize selectors across settings pages, making the codebase more maintainable.
tests/pw/pages/vendorVerificationsPage.ts (5)
33-34: LGTM: Save changes implementation is correct
The implementation correctly uses the common save changes selector and properly verifies the success message.
81-82: LGTM: Proper implementation with status code verification
The implementation correctly handles the creation response (201) and saves changes with proper verification.
114-115: LGTM: Proper implementation with status code verification
The implementation correctly handles the deletion response (204) and saves changes with proper verification.
Line range hint 191-191: Address TODO comments and potential timing issues
There are several TODO comments indicating areas that need improvement:
- Line 191: Need to resolve filter reset visibility
- Line 207: Investigate why goto doesn't reload page in verification tests
- Line 284: Need to resolve reload requirement
These comments suggest potential timing or state management issues that should be addressed to improve test reliability.
#!/bin/bash
# Search for similar patterns in other test files
echo "Searching for similar reload patterns..."
rg "await this.reload\(\)" tests/
echo "Searching for similar goto patterns..."
rg "await this.goto.*verification" tests/Also applies to: 207-207, 284-284
56-57: Consider adding a wait between status update and save
While the implementation is functionally correct, there might be a race condition between the method status update and save changes operation. Consider adding a brief wait or ensuring the status update is complete before saving.
tests/pw/pages/settingsPage.ts (3)
105-105: Great job standardizing the save button selector!
The consistent use of settingsAdmin.saveChanges across all settings methods improves maintainability and reduces code duplication.
Also applies to: 142-142, 202-202, 229-229, 244-244, 267-267, 282-282, 300-300, 314-314, 327-327, 342-342, 358-358, 381-381, 405-405, 424-424, 445-445, 464-464, 482-482, 511-511, 526-526, 550-550, 582-582, 602-602, 627-627, 642-642, 658-658, 685-685, 707-707, 720-720
662-687: LGTM! Well-structured Printful settings method.
The method follows the established pattern of other settings methods and includes proper validation of the success message.
678-682: Add waits for color picker interactions.
This comment was previously made and is still valid. The color picker interactions could be flaky without proper waits, especially in CI environments.
tests/pw/pages/selectors.ts (1)
2519-2532: LGTM: Printful settings selectors are well organized
The Printful settings selectors are well structured and follow the existing pattern in the codebase. The selectors cover all the key UI elements needed for Printful integration:
- Basic settings like client ID and secret key
- Size guide configuration
- Color picker functionality
- Measurement unit settings
tests/pw/.env.example
Outdated
| PRINTFUL_APP_ID=facebook_app_id [Printful App ID] | ||
| PRINTFUL_APP_SECRET=facebook_app_secret [Printful App secret] |
There was a problem hiding this comment.
Security: Update example credential values
The example values for Printful credentials currently use Facebook-related placeholders (facebook_app_id, facebook_app_secret), which could lead to confusion and potential security issues if developers accidentally use Facebook credentials.
Apply this diff to use Printful-specific example values:
-PRINTFUL_APP_ID=facebook_app_id [Printful App ID]
-PRINTFUL_APP_SECRET=facebook_app_secret [Printful App secret]
+PRINTFUL_APP_ID=printful_app_id [Printful API key for authentication]
+PRINTFUL_APP_SECRET=printful_app_secret [Printful API secret for secure requests]📝 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.
| PRINTFUL_APP_ID=facebook_app_id [Printful App ID] | |
| PRINTFUL_APP_SECRET=facebook_app_secret [Printful App secret] | |
| PRINTFUL_APP_ID=printful_app_id [Printful API key for authentication] | |
| PRINTFUL_APP_SECRET=printful_app_secret [Printful API secret for secure requests] |
| // enable shipping | ||
| async enableShipping(shipping: 'printful' | 'marketplace' | 'both'): Promise<void> { | ||
| await this.goIfNotThere(data.subUrls.frontend.vDashboard.settingsPrintful); | ||
|
|
||
| switch (shipping) { | ||
| case 'printful': | ||
| await this.enableSwitcherVendorDashboard(vendorPrintful.printfulShipping); | ||
| await this.toHaveBackgroundColor(vendorPrintful.printfulShipping + '//span', 'rgb(33, 150, 243)'); | ||
| break; | ||
|
|
||
| case 'marketplace': | ||
| await this.disableSwitcherVendorDashboard(vendorPrintful.printfulShipping); | ||
| await this.toHaveBackgroundColor(vendorPrintful.printfulShipping + '//span', 'rgb(204, 204, 204)'); | ||
| await this.enableSwitcherVendorDashboard(vendorPrintful.marketplaceShipping); | ||
| await this.toHaveBackgroundColor(vendorPrintful.marketplaceShipping + '//span', 'rgb(33, 150, 243)'); | ||
| break; | ||
|
|
||
| case 'both': | ||
| await this.enableSwitcherVendorDashboard(vendorPrintful.printfulShipping); | ||
| await this.enableSwitcherVendorDashboard(vendorPrintful.marketplaceShipping); | ||
| await this.toHaveBackgroundColor(vendorPrintful.printfulShipping + '//span', 'rgb(33, 150, 243)'); | ||
| await this.toHaveBackgroundColor(vendorPrintful.marketplaceShipping + '//span', 'rgb(33, 150, 243)'); | ||
| break; | ||
|
|
||
| default: | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve shipping configuration robustness
Several improvements can make this method more maintainable and reliable:
- Extract magic RGB values into named constants
- Consider more reliable ways to verify enabled/disabled states
- Add error handling for UI state changes
+const UI_COLORS = {
+ ENABLED: 'rgb(33, 150, 243)',
+ DISABLED: 'rgb(204, 204, 204)'
+} as const;
async enableShipping(shipping: 'printful' | 'marketplace' | 'both'): Promise<void> {
+ try {
await this.goIfNotThere(data.subUrls.frontend.vDashboard.settingsPrintful);
switch (shipping) {
case 'printful':
await this.enableSwitcherVendorDashboard(vendorPrintful.printfulShipping);
- await this.toHaveBackgroundColor(vendorPrintful.printfulShipping + '//span', 'rgb(33, 150, 243)');
+ await this.toHaveBackgroundColor(vendorPrintful.printfulShipping + '//span', UI_COLORS.ENABLED);
break;
case 'marketplace':
await this.disableSwitcherVendorDashboard(vendorPrintful.printfulShipping);
- await this.toHaveBackgroundColor(vendorPrintful.printfulShipping + '//span', 'rgb(204, 204, 204)');
+ await this.toHaveBackgroundColor(vendorPrintful.printfulShipping + '//span', UI_COLORS.DISABLED);
await this.enableSwitcherVendorDashboard(vendorPrintful.marketplaceShipping);
- await this.toHaveBackgroundColor(vendorPrintful.marketplaceShipping + '//span', 'rgb(33, 150, 243)');
+ await this.toHaveBackgroundColor(vendorPrintful.marketplaceShipping + '//span', UI_COLORS.ENABLED);
break;
case 'both':
await this.enableSwitcherVendorDashboard(vendorPrintful.printfulShipping);
await this.enableSwitcherVendorDashboard(vendorPrintful.marketplaceShipping);
- await this.toHaveBackgroundColor(vendorPrintful.printfulShipping + '//span', 'rgb(33, 150, 243)');
- await this.toHaveBackgroundColor(vendorPrintful.marketplaceShipping + '//span', 'rgb(33, 150, 243)');
+ await this.toHaveBackgroundColor(vendorPrintful.printfulShipping + '//span', UI_COLORS.ENABLED);
+ await this.toHaveBackgroundColor(vendorPrintful.marketplaceShipping + '//span', UI_COLORS.ENABLED);
break;
default:
break;
}
+ } catch (error) {
+ throw new Error(`Failed to configure shipping settings: ${error.message}`);
+ }
}Consider using a more reliable way to verify the enabled/disabled states:
private async verifyShippingState(selector: string, enabled: boolean): Promise<void> {
// Check multiple properties to ensure robust state verification
const element = await this.page.locator(selector);
await element.waitFor({ state: 'visible' });
const isEnabled = await element.evaluate((el) => {
const styles = window.getComputedStyle(el);
return {
backgroundColor: styles.backgroundColor,
ariaChecked: el.getAttribute('aria-checked'),
classList: Array.from(el.classList)
};
});
// Verify multiple properties instead of just background color
expect(isEnabled.ariaChecked).toBe(enabled ? 'true' : 'false');
expect(isEnabled.backgroundColor).toBe(enabled ? UI_COLORS.ENABLED : UI_COLORS.DISABLED);
}| @@ -18,7 +18,7 @@ export class ProductAdvertisingPage extends AdminPage { | |||
| async recreateProductAdvertisementPaymentViaSettingsSave() { | |||
| await this.goToDokanSettings(); | |||
| await this.click(selector.admin.dokan.settings.menus.productAdvertising); | |||
| await this.clickAndWaitForResponse(data.subUrls.ajax, selector.admin.dokan.settings.productAdvertising.productAdvertisingSaveChanges); | |||
| await this.clickAndWaitForResponse(data.subUrls.ajax, selector.admin.dokan.settings.saveChanges); | |||
There was a problem hiding this comment.
💡 Codebase verification
Standardization in progress, but needs completion
The codebase shows mixed usage of save changes selectors:
- Found standardized
selector.admin.dokan.settings.saveChangesin:tests/pw/pages/reverseWithdrawsPage.tstests/pw/pages/productAdvertisingPage.ts
- Found legacy selector in:
tests/pw/pages/menuManagerPage.ts(usingsettingsAdmin.menuManager.menuManagerSaveChanges)
🔗 Analysis chain
LGTM! Good move towards standardization.
The change to use a generic saveChanges selector aligns well with the standardization effort across different settings pages.
Let's verify the consistent usage of this selector across other settings pages:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the standardization of save changes selector across settings pages
# Expected: All settings pages should use the generic selector.admin.dokan.settings.saveChanges
# Search for any remaining specific save changes selectors
rg -t typescript "productAdvertisingSaveChanges|settingsSaveChanges|catalogModeSaveChanges|euComplianceSaveChanges|menuManagerSaveChanges|reverseWithdrawsSaveChanges|vendorVerificationsSaveChanges" tests/pw/pages/
# Search for the standardized selector usage
rg -t typescript "selector\.admin\.dokan\.settings\.saveChanges" tests/pw/pages/
Length of output: 368
Script:
#!/bin/bash
# Let's try without the typescript file type flag since it's not recognized
# Search for any remaining specific save changes selectors
rg "productAdvertisingSaveChanges|settingsSaveChanges|catalogModeSaveChanges|euComplianceSaveChanges|menuManagerSaveChanges|reverseWithdrawsSaveChanges|vendorVerificationsSaveChanges" tests/pw/pages/
# Search for the standardized selector usage
rg "selector\.admin\.dokan\.settings\.saveChanges" tests/pw/pages/
Length of output: 706
* Add a product details test * Add product details virtual options test * Add Followed Store tests * Add testdata & payloads for auction tests * Add Auction product details test * Add new tests * Add product details tests * Fix live chat test * Fix test issues * Fix yml * Fix auction tests * Fix flaky tests * Fix locator issue * Fix PR reviews * Fix a flaky test * Update packages * Fix delivery time tests for cart & checkout block * Update feature map * Add a condition * chore: Release Version 3.13.1 * Add Printful module tests (getdokan#2432) * Fix tests * remove .only * Fix 2 flaky tests * Add printful tests * Skip printful tests * Skipped a test * Fix a locator * Update a locator * Fix PR reviews * Fix pr Reviews * Skip some tests --------- 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
Release Notes
New Features
Bug Fixes
These updates significantly enhance the integration with Printful, improving both vendor and customer experiences.