Add shipstation tests #2444
Add shipstation tests #2444
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces significant updates to the feature map configuration, enhancing permissions and capabilities for various user roles, particularly vendors and admins. Key changes include new permissions for managing auction integration, color scheme customization, delivery times, and various modules related to shipping and payments. Additionally, new API endpoints and testing functionalities are added to support these changes, ensuring comprehensive coverage for the newly implemented features. Changes
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (19)
tests/pw/tests/e2e/shipstation.spec.ts (2)
5-9: Track the temporary skip condition.The test suite is currently skipped with the comment "remove after pr is merged". This should be tracked to ensure it's not forgotten.
Would you like me to create a GitHub issue to track the removal of this skip condition after the PR is merged?
10-18: Add error handling in the setup process.While the lifecycle hooks follow Playwright's best practices, consider adding try-catch blocks for better error handling.
test.beforeAll(async ({ browser }) => { + try { const vendorContext = await browser.newContext(data.auth.vendorAuth); vPage = await vendorContext.newPage(); vendor = new ShipstationPage(vPage); + } catch (error) { + console.error('Failed to setup ShipStation test:', error); + throw error; + } });tests/pw/pages/shipstationPage.ts (1)
9-12: Remove unnecessary constructor.The constructor can be removed as it only calls the parent constructor without additional initialization. TypeScript will automatically create this constructor.
export class ShipstationPage 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)
tests/pw/tests/api/shipstation.spec.ts (3)
14-14: Add environment variable validationConsider adding validation for the
VENDOR_IDenvironment variable to fail fast if it's not set.-const { VENDOR_ID } = process.env; +const { VENDOR_ID } = process.env; +if (!VENDOR_ID) { + throw new Error('VENDOR_ID environment variable is required'); +}
17-17: Track test skip removalThe skip directive needs to be removed after PR merge. Consider adding a GitHub issue to track this.
Would you like me to create a GitHub issue to track the removal of this skip directive?
1-69: Consider adding integration test scenariosThe current tests verify individual API endpoints in isolation. Consider adding integration test scenarios that:
- Create credentials
- Use those credentials to create settings
- Verify the complete workflow
Example integration test:
test('complete ShipStation setup workflow', { tag: ['@pro'] }, async () => { // Create credentials const [credResponse] = await apiUtils.post(endPoints.createShipstationCredential, { data: { vendor_id: VENDOR_ID } }); expect(credResponse.status()).toBe(201); // Create settings using the credentials const [settingsResponse] = await apiUtils.post(endPoints.createShipstationOrderStatusSettings, { data: { ...payloads.shipstationOrderStatusSettings, vendor_id: VENDOR_ID } }); expect(settingsResponse.status()).toBe(201); // Verify the complete setup const [verifyResponse] = await apiUtils.get(endPoints.getShipstationCredential(VENDOR_ID)); expect(verifyResponse.status()).toBe(200); });tests/pw/tests/e2e/vendorSettings.spec.ts (3)
115-119: Consider using try-finally for cleanup operationsWhile the cleanup approach is consistent with other tests, consider wrapping the test in a try-finally block to ensure cleanup runs even if the test fails:
test('vendor can set min-max settings', { tag: ['@pro', '@vendor'] }, async () => { - await vendor.setStoreSettings(data.vendor.vendorInfo, 'min-max'); - - // disable min-max - await dbUtils.updateOptionValue(dbData.dokan.optionName.selling, { enable_min_max_quantity: 'off', enable_min_max_amount: 'off' }); + try { + await vendor.setStoreSettings(data.vendor.vendorInfo, 'min-max'); + } finally { + // disable min-max + await dbUtils.updateOptionValue(dbData.dokan.optionName.selling, { enable_min_max_quantity: 'off', enable_min_max_amount: 'off' }); + } });
Line range hint
1-150: Consider using nested describe blocks for better test organizationThe tests are well-organized with tags, but readability could be improved by using nested
test.describe()blocks to group related tests:test.describe('Vendor settings test', () => { + test.describe('vendor view tests', () => { test('vendor can view store settings menu page', ...); // other view tests + }); + test.describe('store settings tests', () => { test('vendor can set store banner settings', ...); // other settings tests + }); });
Line range hint
120-122: Enhance ShipStation test coverageGiven that this PR focuses on ShipStation tests, consider adding more test cases to cover:
- Invalid credentials handling
- API connection failures
- Different shipping service configurations
- Rate calculation scenarios
Example test structure:
test('vendor cannot save invalid ShipStation credentials', { tag: ['@pro', '@vendor'] }, async () => { const invalidCredentials = { ...data.vendor.shipStation, apiKey: 'invalid' }; await vendor.expectShipStationError(invalidCredentials); }); test('vendor can update existing ShipStation settings', { tag: ['@pro', '@vendor'] }, async () => { // Test updating existing configuration }); test('vendor can configure different shipping services', { tag: ['@pro', '@vendor'] }, async () => { // Test various shipping service configurations });tests/pw/pages/vendorSettingsPage.ts (1)
450-466: Consider refactoring for improved maintainability.A few suggestions to enhance the code:
- Use template literals for better readability in status checks
- Extract common status handling logic into a helper method
- Define HTTP status codes as constants
Here's a suggested refactor:
+ private async handleOrderStatus(status: string, input: string, dropdown?: string): Promise<void> { + if (dropdown) { + await this.click(dropdown); + } + await this.clearAndType(input, status); + await this.toContainText(settingsShipStation.result, status); + await this.press(data.key.enter); + } + const HTTP_CREATED = 201; async setShipStation(shipStation: vendor['shipStation']): Promise<void> { await this.goIfNotThere(data.subUrls.frontend.vDashboard.settingsShipstation); // export order statuses const allStatus = await this.getMultipleElementTexts(settingsShipStation.selectedStatus); - const statusIsSelected = allStatus.includes(`×${shipStation.status}`); + const statusIsSelected = allStatus.includes(`×${shipStation.status}`); if (!statusIsSelected) { - await this.clearAndType(settingsShipStation.exportOrderStatusesInput, shipStation.status); - await this.toContainText(settingsShipStation.result, shipStation.status); - await this.press(data.key.enter); + await this.handleOrderStatus(shipStation.status, settingsShipStation.exportOrderStatusesInput); } // shipped order status - await this.click(settingsShipStation.shippedOrderStatusDropdown); - await this.clearAndType(settingsShipStation.shippedOrderStatusInput, shipStation.status); - await this.toContainText(settingsShipStation.result, shipStation.status); - await this.press(data.key.enter); + await this.handleOrderStatus( + shipStation.status, + settingsShipStation.shippedOrderStatusInput, + settingsShipStation.shippedOrderStatusDropdown + ); - await this.clickAndAcceptAndWaitForResponse(data.subUrls.api.dokan.shipstation, settingsShipStation.saveChanges, 201); + await this.clickAndAcceptAndWaitForResponse(data.subUrls.api.dokan.shipstation, settingsShipStation.saveChanges, HTTP_CREATED); await this.toBeVisible(settingsShipStation.saveSuccessMessage); }tests/pw/utils/apiEndPoints.ts (1)
378-385: Consider grouping with other shipping-related endpoints.The ShipStation endpoint implementations look good and follow the established patterns. However, consider moving these endpoints near other shipping-related endpoints (like
shipping-status) for better code organization and maintainability.tests/pw/feature-map/feature-map.yml (1)
1108-1108: Remove commented out duplicate test case.This test case is already defined elsewhere in the feature map. Remove the commented line to maintain cleaner code.
- # vendor can view Shipstation settings menu page: truetests/pw/utils/schemas.ts (1)
2959-2971: Consider enhancing the ShipStation schemas with additional validations.The schema definitions look good but could benefit from some additional validations:
For
shipstationCredentialSchema:
- Consider adding string length or format validations for the API keys
- Add
.min(1)to ensure non-empty stringsFor
shipstationOrderStatusSettingSchema:
- Consider adding
.min(1)toexport_statusesarray to ensure at least one status is selected- Add validation for allowed order status values
Here's the suggested implementation:
shipstationSchema: { shipstationCredentialSchema: z.object({ key_id: z.string().or(z.number()), - consumer_key: z.string(), - consumer_secret: z.string(), + consumer_key: z.string().min(1, 'Consumer key is required'), + consumer_secret: z.string().min(1, 'Consumer secret is required'), }), shipstationOrderStatusSettingSchema: z.object({ vendor_id: z.string().or(z.number()), - export_statuses: z.array(z.string()), - shipped_status: z.string(), + export_statuses: z.array(z.string()).min(1, 'At least one export status is required'), + shipped_status: z.enum(['completed', 'processing', 'on-hold'], { + errorMap: () => ({ message: 'Invalid shipped status' }), + }), }), },tests/pw/utils/testData.ts (2)
1249-1249: Consider consolidating ShipStation-related configurations.The ShipStation configurations are currently scattered across different sections. Consider grouping them under a dedicated
shipstationobject for better maintainability and easier updates.Example structure:
shipstation: { api: 'dokan/v1/shipstation', settings: { // vendor settings // store settings // other shipstation specific configurations } }
Line range hint
1-1500: Consider implementing data factories for complex test scenarios.While the current approach to test data management is solid, consider implementing the Factory pattern for generating complex test scenarios. This would:
- Make test data creation more maintainable
- Allow for easier composition of test scenarios
- Provide better type safety through TypeScript interfaces
Example implementation:
interface ShipStationTestData { api: string; credentials: { username: string; password: string; }; // other shipstation specific types } class ShipStationDataFactory { static create(overrides?: Partial<ShipStationTestData>): ShipStationTestData { return { api: 'dokan/v1/shipstation', credentials: { username: faker.internet.userName(), password: faker.internet.password() }, ...overrides }; } }tests/pw/utils/payloads.ts (2)
4877-4880: Add TypeScript type definitions and validation.The
createCredentialpayload would benefit from:
- Type definitions for the vendor_id parameter
- Additional validation fields commonly required for API credentials
createCredential: { - vendor_id: '', + vendor_id: string | number; + api_key?: string; + api_secret?: string; + is_test_mode?: boolean; },
4881-4886: Add type safety and documentation for order status settings.The order status settings would be more maintainable with:
- Type definitions for the parameters
- Constants for the WooCommerce order statuses
- JSDoc comments explaining the purpose
+ /** ShipStation order status mapping configuration */ shipstationOrderStatusSettings: { - vendor_id: '', - export_statuses: ['wc-pending', 'wc-processing', 'wc-on-hold', 'wc-completed', 'wc-cancelled'], - shipped_status: 'wc-completed', + vendor_id: string | number; + export_statuses: WooCommerceOrderStatus[]; + shipped_status: WooCommerceOrderStatus; }, + /** Valid WooCommerce order statuses */ + type WooCommerceOrderStatus = + | 'wc-pending' + | 'wc-processing' + | 'wc-on-hold' + | 'wc-completed' + | 'wc-cancelled';tests/pw/pages/selectors.ts (2)
6548-6559: Fix inconsistent indentation in ShipStation credentials section.The indentation is inconsistent in this section. Some lines use 12 spaces while others use 8 spaces.
Apply consistent 12 space indentation:
generateCredentials: 'button#dokan-shipstation-generate-credentials-btn', generateSuccessMessage: '//div[@id="swal2-html-container" and normalize-space()="API credentials generated successfully."]', revokeCredentials: 'button#dokan-shipstation-revoke-credentials-btn', confirmRevoke: 'button.swal2-confirm', revokeSuccessMessage: '//div[@id="swal2-html-container" and normalize-space()="API credentials revoked successfully."]', credentials: { authenticationKey: '//label[normalize-space()="Authentication Key"]/..//code', consumerKey: '//label[normalize-space()="Consumer Key"]/..//code', consumerSecret: '//label[normalize-space()="Consumer Secret"]/..//code', },
6560-6568: Use consistent selector patterns for ShipStation settings.The selectors use a mix of CSS and XPath patterns. Consider using consistent XPath selectors for better maintainability.
Apply consistent XPath selectors:
- selectedStatus: '//label[@for="dokan-shipstation-export-statuses"]/..//li[@class="select2-selection__choice"]', - exportOrderStatusesInput: '//label[normalize-space()="Export Order Statuses"]/..//span[@class="select2-selection select2-selection--multiple"]//input[@class="select2-search__field"]', - shippedOrderStatusDropdown: '.select2-selection__arrow', - shippedOrderStatusInput: '(//input[@class="select2-search__field"])[2]', - result: '.select2-results__option.select2-results__option--highlighted', + selectedStatus: '//label[@for="dokan-shipstation-export-statuses"]/..//li[@class="select2-selection__choice"]', + exportOrderStatusesInput: '//label[normalize-space()="Export Order Statuses"]/..//span[@class="select2-selection select2-selection--multiple"]//input[@class="select2-search__field"]', + shippedOrderStatusDropdown: '//span[@class="select2-selection__arrow"]', + shippedOrderStatusInput: '(//input[@class="select2-search__field"])[2]', + result: '//li[contains(@class,"select2-results__option") and contains(@class,"select2-results__option--highlighted")]', saveChanges: '#dokan-store-shipstation-form-submit', saveSuccessMessage: '//div[@id="swal2-html-container" and normalize-space()="Settings saved successfully."]',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
tests/pw/feature-map/feature-map.yml(1 hunks)tests/pw/package.json(1 hunks)tests/pw/pages/selectors.ts(1 hunks)tests/pw/pages/shipstationPage.ts(1 hunks)tests/pw/pages/vendorSettingsPage.ts(2 hunks)tests/pw/tests/api/shipstation.spec.ts(1 hunks)tests/pw/tests/e2e/shipstation.spec.ts(1 hunks)tests/pw/tests/e2e/vendorSettings.spec.ts(1 hunks)tests/pw/utils/apiEndPoints.ts(1 hunks)tests/pw/utils/apiUtils.ts(1 hunks)tests/pw/utils/payloads.ts(1 hunks)tests/pw/utils/schemas.ts(1 hunks)tests/pw/utils/testData.ts(1 hunks)
🧰 Additional context used
🪛 Biome
tests/pw/pages/shipstationPage.ts
[error] 10-12: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (12)
tests/pw/tests/e2e/shipstation.spec.ts (2)
1-4: LGTM! Clean and well-organized imports.
The imports follow good practices with clear separation between Playwright utilities and custom project modules.
2-2: Verify ShipstationPage implementation.
Let's verify that the ShipstationPage class exists and implements the required methods.
✅ Verification successful
Let me try a different approach to locate the ShipstationPage implementation by searching for the file and its contents.
ShipstationPage implementation is verified and complete
The ShipstationPage class is properly implemented in tests/pw/pages/shipstationPage.ts with both required methods:
generateShipStationCredentials()revokeShipStationCredentials()
The class extends VendorPage and the import path @pages/shipstationPage correctly resolves to the implementation file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ShipstationPage class implementation
# Expected: Find the ShipstationPage class with generateShipStationCredentials and revokeShipStationCredentials methods
# Search for the ShipstationPage class definition
echo "Searching for ShipstationPage class..."
ast-grep --pattern 'export class ShipstationPage {
$$$
}'
# Search for the required methods
echo "Searching for required methods..."
ast-grep --pattern 'generateShipStationCredentials($$$) {
$$$
}'
ast-grep --pattern 'revokeShipStationCredentials($$$) {
$$$
}'
Length of output: 361
Script:
#!/bin/bash
# First, let's find the actual file path
echo "Searching for shipstationPage file..."
fd -t f -i "shipstation(page)?\.(ts|js|tsx|jsx)$"
# Then let's examine its contents if found
echo -e "\nSearching for class implementation and methods..."
rg -l "class.*ShipstationPage" --type ts
rg "class.*ShipstationPage" -A 10 --type ts
# Let's also check the actual import path from the test file
echo -e "\nChecking the import path resolution..."
cat "tests/pw/tests/e2e/shipstation.spec.ts"
Length of output: 2372
tests/pw/pages/shipstationPage.ts (2)
1-8: LGTM! Clean imports and selector setup.
The imports are well-organized and the selector constant improves maintainability.
17-17: Verify API endpoint usage across the codebase.
Let's ensure the ShipStation API endpoint is consistently used across the codebase.
Also applies to: 29-29
✅ Verification successful
Let me verify the API endpoint usage further by checking the data configuration.
Let me check the data configuration file to see how the API endpoint is defined.
Let me check the API endpoint configuration in the data file.
Based on the verification results, I can now provide the final response:
ShipStation API endpoint is consistently used across the codebase
The ShipStation API endpoint (data.subUrls.api.dokan.shipstation) is consistently used in all relevant locations:
- Generate credentials in shipstationPage.ts
- Revoke credentials in shipstationPage.ts
- Save settings in vendorSettingsPage.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ShipStation API endpoint usage
rg -t ts -t php "dokan.shipstation" -A 2
Length of output: 2085
Script:
#!/bin/bash
# Search for API endpoint configuration
rg -t ts "api.*dokan.*shipstation" -A 2
Length of output: 978
Script:
#!/bin/bash
# Search for the data configuration file and its content
fd -e ts data.ts -x cat {}
Length of output: 131074
Script:
#!/bin/bash
# Search for API endpoint configuration in data.ts
rg -t ts "subUrls.*api.*shipstation" -A 2
Length of output: 980
tests/pw/package.json (1)
16-16: LGTM! Verify auth_setup project configuration.
The new script follows the established pattern and integrates well with existing setup scripts.
Let's verify the auth_setup project configuration exists:
✅ Verification successful
Configuration for auth_setup project exists and looks correct
The auth_setup project is properly configured in e2e.config.ts:
- Matches
_auth.setup.tstest files - Has correct dependency on
site_setup - Includes appropriate retry configuration
- Follows the established pattern of other setup projects
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if auth_setup project is configured in e2e.config.ts
# Search for auth_setup project configuration
rg -A 5 "projects.*auth_setup" "tests/pw/e2e.config.ts"
Length of output: 55
Script:
#!/bin/bash
# Let's first check if the config file exists and examine its content
cat tests/pw/e2e.config.ts
# If the file is not found, let's search for it in case the path is different
fd -t f "e2e.config.ts"
Length of output: 6707
tests/pw/pages/vendorSettingsPage.ts (1)
83-89: LGTM! Clean implementation of conditional visibility checks.
The code effectively determines the state of ShipStation credentials and shows appropriate UI elements based on that state.
tests/pw/utils/apiEndPoints.ts (1)
378-385: Verify API endpoint structure.
Let's verify that these endpoint structures match the API documentation and existing implementation.
✅ Verification successful
API endpoint structure is consistent and verified
The endpoint structures in apiEndPoints.ts match the API routes documented in the test coverage tags and are properly implemented in the test suite. The endpoints follow RESTful conventions and maintain consistency:
-
Credentials endpoints:
- GET/DELETE:
/dokan/v1/shipstation/credentials/{vendorId} - POST:
/dokan/v1/shipstation/credentials/create
- GET/DELETE:
-
Order status endpoints:
- GET/DELETE:
/dokan/v1/shipstation/order-statuses/{vendorId} - POST:
/dokan/v1/shipstation/order-statuses
- GET/DELETE:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in API endpoint structure and verify against existing implementations
# Check for similar endpoint patterns
rg -A 2 "credentials/\$\{.*\}" --type ts
# Check for other shipping integration endpoints
rg -A 2 "shipping.*credentials" --type ts
# Check for order status endpoints structure
rg -A 2 "order-statuses/\$\{.*\}" --type ts
Length of output: 130
Script:
#!/bin/bash
# Let's try different search patterns to find API endpoint definitions
# Search for shipstation related endpoints
rg "shipstation" --type ts -A 2 -B 2
# Search for any API endpoint patterns with SERVER_URL
rg "SERVER_URL.*dokan/v1" --type ts -A 2 -B 2
# Look for files that might contain API documentation or route definitions
fd -e ts -e js -e php "route|api|endpoint" --type f
Length of output: 50644
Script:
#!/bin/bash
# Let's check the API implementation and tests for these endpoints
# Look for shipstation API implementation
rg -B 2 -A 2 "shipstation/credentials|shipstation/order-statuses" --type php
# Check test coverage for these endpoints
cat tests/pw/tests/api/shipstation.spec.ts
Length of output: 3568
tests/pw/feature-map/feature-map.yml (1)
1109-1110: Consider adding more test cases for ShipStation integration.
While the current test cases cover the basic credential management functionality, consider adding tests for:
- Validation of generated credentials
- Error handling for invalid credentials
- Testing the actual ShipStation API integration
- Verifying order synchronization between platforms
tests/pw/utils/schemas.ts (1)
2957-2958: LGTM!
The empty line and comment provide good separation and documentation for the ShipStation schema section.
tests/pw/utils/apiUtils.ts (2)
1536-1539: LGTM!
The comment block follows the established pattern and clearly indicates the purpose of the methods.
1540-1558: Verify test coverage and endpoint definitions.
Ensure that:
- These new methods are covered by tests
- The corresponding endpoints are properly defined in the endpoints file
✅ Verification successful
Integration verified with proper test coverage and endpoint definitions
The methods are well-integrated with:
- Complete test coverage in
shipstation.spec.tswith proper assertions - Correctly defined endpoints in
apiEndPoints.tsfollowing the API structure:/dokan/v1/shipstation/credentials/create/dokan/v1/shipstation/credentials/{vendorId}/dokan/v1/shipstation/order-statuses/dokan/v1/shipstation/order-statuses/{vendorId}
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test coverage and endpoint definitions
# Check for test files using these methods
echo "Checking for test coverage..."
rg -l "createShipstationCredential|deleteShipstationCredential|createShipstationOrderStatusSettings|deleteShipstationOrderStatusSettings" tests/
# Check for endpoint definitions
echo "Checking for endpoint definitions..."
rg -l "createShipstationCredential|deleteShipstationCredential|createShipstationOrderStatusSettings|deleteShipstationOrderStatusSettings" tests/pw/utils/apiEndPoints.ts
Length of output: 597
Script:
#!/bin/bash
# Let's examine the actual test coverage and endpoint definitions
# Check the test implementation
echo "=== Test Implementation ==="
rg -B2 -A2 "createShipstationCredential|deleteShipstationCredential|createShipstationOrderStatusSettings|deleteShipstationOrderStatusSettings" tests/pw/tests/api/shipstation.spec.ts
# Check the endpoint definitions
echo -e "\n=== Endpoint Definitions ==="
rg "createShipstationCredential|deleteShipstationCredential|createShipstationOrderStatusSettings|deleteShipstationOrderStatusSettings" tests/pw/utils/apiEndPoints.ts
Length of output: 2167
tests/pw/utils/testData.ts (1)
1249-1249: LGTM! The ShipStation API endpoint is well-structured.
The endpoint follows the established naming conventions and is correctly placed within the Dokan API namespace.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
tests/pw/pages/shipstationPage.ts (1)
9-12: Remove unnecessary constructor.The constructor only calls super without adding any additional functionality. In TypeScript/JavaScript, when a class extends another class and doesn't need additional constructor logic, the constructor can be omitted as it will be automatically inherited.
export class ShipStationPage extends VendorPage { - constructor(page: Page) { - super(page); - }🧰 Tools
🪛 Biome (1.9.4)
[error] 10-12: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
tests/pw/feature-map/feature-map.yml (1)
1108-1111: LGTM! Consider adding more test cases for comprehensive coverage.The basic test cases for ShipStation integration look good, covering the essential credential management functionality.
Consider adding the following test cases for more comprehensive coverage:
+ vendor can't generate ShipStation credentials with invalid input: true + vendor can't use revoked ShipStation credentials: true + vendor can view ShipStation credentials error messages: true + vendor can sync orders with ShipStation: true + vendor can handle ShipStation API errors gracefully: truetests/pw/utils/schemas.ts (2)
2960-2964: Consider adding security annotations for sensitive fields.The credential schema contains sensitive authentication fields. Consider adding JSDoc comments or custom Zod refinements to:
- Mark these fields as sensitive in logs
- Ensure proper handling in error messages
- Add validation for minimum security requirements
shipStationCredentialSchema: z.object({ key_id: z.string().or(z.number()), - consumer_key: z.string(), - consumer_secret: z.string(), + consumer_key: z.string() + .min(8, "Consumer key must be at least 8 characters") + .describe("Sensitive credential - do not log"), + consumer_secret: z.string() + .min(12, "Consumer secret must be at least 12 characters") + .describe("Sensitive credential - do not log"), }),
2966-2970: Add enum validation for status fields.The order status settings schema could benefit from enum validation to ensure only valid statuses are used.
shipStationOrderStatusSettingSchema: z.object({ vendor_id: z.string().or(z.number()), - export_statuses: z.array(z.string()), - shipped_status: z.string(), + export_statuses: z.array( + z.enum([ + 'wc-pending', + 'wc-processing', + 'wc-on-hold', + 'wc-completed', + // Add other valid statuses + ]) + ), + shipped_status: z.enum([ + 'wc-completed', + 'wc-shipped', + // Add other valid shipped statuses + ]), }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
tests/pw/feature-map/feature-map.yml(2 hunks)tests/pw/pages/shipstationPage.ts(1 hunks)tests/pw/pages/vendorSettingsPage.ts(2 hunks)tests/pw/pages/vendorShippingPage.ts(1 hunks)tests/pw/tests/api/shipstation.spec.ts(1 hunks)tests/pw/tests/e2e/shipstation.spec.ts(1 hunks)tests/pw/tests/e2e/vendorSettings.spec.ts(2 hunks)tests/pw/utils/apiEndPoints.ts(2 hunks)tests/pw/utils/apiUtils.ts(1 hunks)tests/pw/utils/payloads.ts(2 hunks)tests/pw/utils/schemas.ts(1 hunks)tests/pw/utils/testData.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/pw/pages/vendorShippingPage.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/pw/pages/vendorSettingsPage.ts
- tests/pw/tests/api/shipstation.spec.ts
- tests/pw/tests/e2e/shipstation.spec.ts
- tests/pw/tests/e2e/vendorSettings.spec.ts
- tests/pw/utils/payloads.ts
- tests/pw/utils/testData.ts
🧰 Additional context used
🪛 Biome (1.9.4)
tests/pw/pages/shipstationPage.ts
[error] 10-12: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (9)
tests/pw/pages/shipstationPage.ts (3)
1-8: LGTM! Clean import organization.
The imports are well-structured and the selector organization follows good practices.
14-22: 🛠️ Refactor suggestion
Add JSDoc documentation and error handling.
The method implementation looks good but could benefit from:
- JSDoc documentation to describe the method's purpose, parameters, and return value
- Error handling as suggested in the previous review
+/**
+ * Generates ShipStation credentials and verifies the success of the operation.
+ * @throws {Error} If credential generation fails or verification steps fail
+ */
async generateShipStationCredentials() {
+ try {
await this.goIfNotThere(data.subUrls.frontend.vDashboard.settingsShipStation);
await this.clickAndAcceptAndWaitForResponse(data.subUrls.api.dokan.shipStation, settingsShipStation.generateCredentials, 201);
await this.toBeVisible(settingsShipStation.generateSuccessMessage);
await this.toBeVisible(settingsShipStation.revokeCredentials);
await this.multipleElementVisible(settingsShipStation.credentials);
+ } catch (error) {
+ throw new Error(`Failed to generate ShipStation credentials: ${error.message}`);
+ }
}24-34: 🛠️ Refactor suggestion
Add JSDoc documentation and error handling.
The method implementation looks good but could benefit from:
- JSDoc documentation to describe the method's purpose
- Error handling and blank line removal as suggested in the previous review
+/**
+ * Revokes ShipStation credentials and verifies the success of the operation.
+ * @throws {Error} If credential revocation fails or verification steps fail
+ */
async revokeShipStationCredentials() {
+ try {
await this.goIfNotThere(data.subUrls.frontend.vDashboard.settingsShipStation);
-
await this.click(settingsShipStation.revokeCredentials);
await this.clickAndAcceptAndWaitForResponse(data.subUrls.api.dokan.shipStation, settingsShipStation.confirmRevoke);
await this.toBeVisible(settingsShipStation.revokeSuccessMessage);
await this.toBeVisible(settingsShipStation.generateCredentials);
await this.multipleElementNotVisible(settingsShipStation.credentials);
+ } catch (error) {
+ throw new Error(`Failed to revoke ShipStation credentials: ${error.message}`);
+ }
}tests/pw/utils/apiEndPoints.ts (2)
368-368: LGTM! Improved comment formatting.
The spacing after // improves readability and follows consistent comment formatting.
378-384: LGTM! Well-structured ShipStation endpoints.
The new endpoints are well-organized, follow RESTful conventions, and maintain consistent naming patterns. The implementation includes essential CRUD operations for both credentials and order status settings.
Let's verify if all necessary ShipStation operations are covered by checking the test files:
✅ Verification successful
All ShipStation endpoints are properly covered with tests ✅
The test coverage analysis shows that:
- All 6 endpoints (GET, POST, DELETE for both credentials and order status settings) have corresponding test cases
- Each endpoint is tested for successful response and schema validation
- The tests are properly organized and use consistent patterns
- Test data and payloads are properly set up in the utility files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for ShipStation endpoints
# Search for ShipStation-related test files
echo "Searching for ShipStation test files..."
fd -e ts -e js shipstation tests/
# Search for ShipStation API usage in test files
echo "Checking API usage in tests..."
rg -l "shipstation" tests/
Length of output: 511
Script:
#!/bin/bash
# Check the test coverage by examining the API test file
echo "Checking ShipStation API test coverage..."
cat tests/pw/tests/api/shipstation.spec.ts
# Check the test data and payloads
echo -e "\nChecking test data and payloads..."
rg "shipstation" tests/pw/utils/testData.ts tests/pw/utils/payloads.ts -A 2 -B 2
Length of output: 5203
tests/pw/feature-map/feature-map.yml (1)
1108-1111: Verify integration with test implementation files.
Let's ensure all the required test implementation files are in place and properly integrated.
✅ Verification successful
Integration between feature map and test implementation is properly aligned
The verification shows complete alignment between the feature map entries and their implementations:
- All required files exist and contain ShipStation-related code
- Test implementations in
shipstation.spec.tsdirectly match the feature map entries:- "vendor can generate ShipStation credentials"
- "vendor can revoke ShipStation credentials"
- Supporting files contain corresponding selectors, API endpoints, and utility methods needed for the tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and integration of ShipStation test files
# Check for the existence of required test files
echo "Checking for required test files..."
for file in "tests/pw/pages/selectors.ts" "tests/pw/tests/e2e/shipstation.spec.ts" "tests/pw/pages/shipstationPage.ts" "tests/pw/utils/apiEndPoints.ts" "tests/pw/utils/apiUtils.ts"; do
if [ -f "$file" ]; then
echo "✓ Found $file"
# Check for ShipStation-related content
echo " Contents:"
rg -i "shipstation" "$file" || echo " No ShipStation-related content found"
else
echo "✗ Missing $file"
fi
done
Length of output: 6146
tests/pw/utils/schemas.ts (1)
2958-2959: LGTM!
The ShipStation schemas are properly integrated into the main schemas object, following the established pattern in the file.
Also applies to: 2971-2971
tests/pw/utils/apiUtils.ts (2)
1540-1543: LGTM with previous review comments.
The implementation looks correct. Please address the type definitions and error handling improvements suggested in the previous review comments.
Also applies to: 1545-1548, 1550-1553, 1555-1558
1536-1558: Implementation follows established patterns.
The ShipStation methods are well-integrated with the existing codebase:
- Follows RESTful patterns
- Uses consistent return types
- Maintains logical grouping with other integration methods
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
tests/pw/tests/e2e/productsDetailsAuction.spec.ts (1)
187-194: Consider adding test for removing relist options.To maintain consistency with other sections in the test suite (like virtual options, shipping, etc.), consider adding a test case for removing relist options:
test('vendor can remove product relist options', { tag: ['@pro', '@vendor'] }, async () => { // First enable relist options await vendor.addProductRelistingOption(productIdFull, data.product.auction); // Then remove them await vendor.removeProductRelistingOption(productIdFull); });tests/pw/pages/vendorAuctionsPage.ts (1)
252-264: Good separation of concerns, but consider adding input validation.The method effectively separates relisting options from general options, improving code organization. However, consider these improvements:
async addProductRelistingOption(productName: string, generalOption: product['auction']) { + // Validate input values + if (!generalOption.relistIfFailAfterNHours || !generalOption.relistIfNotPaidAfterNHours || !generalOption.relistAuctionDurationInH) { + throw new Error('Required relisting options are missing'); + } + await this.goToAuctionProductEditById(productName); await this.check(auctionProductsVendor.auction.enableAutomaticRelisting); await this.clearAndType(auctionProductsVendor.auction.relistIfFailAfterNHours, generalOption.relistIfFailAfterNHours); await this.clearAndType(auctionProductsVendor.auction.relistIfNotPaidAfterNHours, generalOption.relistIfNotPaidAfterNHours); await this.clearAndType(auctionProductsVendor.auction.relistAuctionDurationInH, generalOption.relistAuctionDurationInH); await this.saveProduct(); + try { await this.toBeChecked(auctionProductsVendor.auction.enableAutomaticRelisting); await this.toHaveValue(auctionProductsVendor.auction.relistIfFailAfterNHours, generalOption.relistIfFailAfterNHours); await this.toHaveValue(auctionProductsVendor.auction.relistIfNotPaidAfterNHours, generalOption.relistIfNotPaidAfterNHours); await this.toHaveValue(auctionProductsVendor.auction.relistAuctionDurationInH, generalOption.relistAuctionDurationInH); + } catch (error) { + throw new Error(`Failed to verify relisting options: ${error.message}`); + } }tests/pw/feature-map/feature-map.yml (1)
1174-1181: Consider adding error handling configurations for ShipStation Integration.The ShipStation Integration configuration should include:
- Error handling for failed credential generation/revocation
- Validation rules for ShipStation settings
- Rate limiting configurations for API calls
Also, line 1178 is commented out but the same feature is defined in the vendor settings section, which could lead to confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
tests/pw/feature-map/feature-map.yml(27 hunks)tests/pw/pages/vendorAuctionsPage.ts(1 hunks)tests/pw/tests/e2e/productsDetailsAuction.spec.ts(1 hunks)
🔇 Additional comments (4)
tests/pw/tests/e2e/productsDetailsAuction.spec.ts (1)
187-189: LGTM! Test case for enabling relist options.
The test implementation looks correct and follows the established pattern in the test suite.
tests/pw/pages/vendorAuctionsPage.ts (1)
250-251: LGTM! Good refactoring.
The removal of relisting options from this method improves code organization by maintaining single responsibility principle.
tests/pw/feature-map/feature-map.yml (2)
458-458: LGTM: ShipStation settings menu configuration is properly placed.
The addition of ShipStation settings menu visibility in the vendor settings section is consistent with the ShipStation Integration features.
752-753: Verify if all modules should be disabled by default.
All module enable/disable flags are set to false. Please confirm if this is intentional and aligns with the expected default configuration.
Run the following script to check if this is consistent with other configuration files:
Also applies to: 761-762, 778-779, 785-786, 828-829, 844-845, 879-880, 897-898, 909-910, 916-917, 925-926, 932-933, 950-951, 970-971, 1004-1005, 1014-1015, 1045-1046, 1065-1066, 1072-1073, 1079-1080, 1096-1097, 1135-1136, 1152-1153, 1175-1176, 1186-1187, 1208-1209, 1228-1229, 1274-1275, 1281-1282, 1288-1289, 1299-1300, 1307-1308, 1319-1320, 1333-1334, 1355-1356, 1396-1397, 1420-1421
✅ Verification successful
Let me analyze the history to understand if this is a deliberate configuration or a recent change.
Let me check if there's documentation or test files that can help us understand if this is the intended configuration.
All module flags being disabled by default is the correct configuration
Based on the documentation and feature map analysis:
- All module enable/disable flags are consistently set to
falsein the feature map - The readme indicates these are Pro modules (e.g., Delivery Time, ShipStation, Table Rate Shipping)
- Having them disabled by default is the expected behavior for premium/Pro features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other feature map files have similar module configurations
# Search for other feature map files
fd -e yml -e yaml . | grep -i "feature.*map" | while read -r file; do
echo "Checking $file..."
# Look for module enable/disable patterns
rg -i "can (enable|disable).*module.*:" "$file"
done
Length of output: 4603
Script:
#!/bin/bash
# Let's check the git history of this file to understand if this is a recent change
# or has been the default configuration
git log -p tests/pw/feature-map/feature-map.yml | grep -A 5 -B 5 "can.*module:"
Length of output: 0
Script:
#!/bin/bash
# Let's check if there are any configuration files or documentation that might explain
# the module management strategy
# Search for relevant documentation or configuration files
rg -i -A 5 -B 5 "module.*(enable|disable|configuration|management)" --type md --type txt
# Also check for any test files that might verify this behavior
fd -e test.js -e spec.js -e test.ts -e spec.ts . | xargs rg -i "module.*(enable|disable)"
Length of output: 2588
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/pw/utils/apiUtils.ts (1)
1540-1558: Add test coverage for ShipStation API methods.Since this is a test utility file, ensure comprehensive test coverage for these ShipStation API methods. Consider adding tests for:
- Success scenarios
- Error scenarios (e.g., invalid vendor_id)
- Edge cases (e.g., empty/null values)
Would you like me to help generate test cases for these methods?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
tests/pw/tests/e2e/productsDetailsAuction.spec.ts(1 hunks)tests/pw/utils/apiUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/pw/tests/e2e/productsDetailsAuction.spec.ts
🔇 Additional comments (1)
tests/pw/utils/apiUtils.ts (1)
1536-1539: Past review comment is still valid.
The suggestion to add detailed JSDoc documentation for the ShipStation methods block is still applicable.
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
Tests