Appearance settings tests #2458
Conversation
WalkthroughThe pull request introduces modifications across multiple files, focusing on enhancing environment configurations with new reCAPTCHA keys and a Mapbox variable. The E2E API test workflow has been updated to include these environment variables. Additionally, permissions for various features have been revised to emphasize admin functionalities, particularly in managing store appearance and vendor capabilities. New methods and properties have been added to support these changes, while existing structures remain largely intact. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (14)
tests/pw/types/environment.d.ts (1)
37-38: Add JSDoc comments to document the reCAPTCHA environment variables.While the type definitions are correct, adding JSDoc comments would improve maintainability by documenting the purpose and expected format of these credentials.
+ /** Google reCAPTCHA v2 site key for the frontend */ RECAPTCHA_SITE_KEY: string; + /** Google reCAPTCHA v2 secret key for backend verification */ RECAPTCHA_SECRET_KEY: string;tests/pw/.env.example (2)
1-20: Consider adding validation instructions for API keys.While the configuration template is clear, it would be helpful to add comments about how to validate these API keys, especially for the newly added reCAPTCHA configuration.
Add validation instructions like:
RECAPTCHA_SITE_KEY=recaptcha_site_key [reCAPTCHA site key] RECAPTCHA_SECRET_KEY=recaptcha_secret_key [reCAPTCHA secret key] +# Validate reCAPTCHA keys at: https://www.google.com/recaptcha/admin
1-20: Consider grouping related API configurations.The configuration could be more organized by grouping related API configurations together (e.g., grouping authentication-related configs like reCAPTCHA with other security settings).
Consider reorganizing the plugin configuration section to group related settings:
# Plugin Configuration +# User Authentication ADMIN=John_Doe [Admin username] ADMIN_PASSWORD=AdminPass123 [Password for the admin account] VENDOR=David_Johnson [Vendor username] VENDOR2=jhonathon_Smith [Vendor username] CUSTOMER=Michael_Williams [Customer username] USER_PASSWORD=Passw0rd123 [Password for all other users] + +# Security & Authentication APIs +RECAPTCHA_SITE_KEY=recaptcha_site_key [reCAPTCHA site key] +RECAPTCHA_SECRET_KEY=recaptcha_secret_key [reCAPTCHA secret key] + +# Dokan Configuration DOKAN_PRO=true [Dokan pro active status] LICENSE_KEY=license_key [Dokan License key] + +# Integration APIs GMAP=map_key [Google Maps API key] TALKJS_APP_ID=talkjs_app_id [TalkJS App ID] TALKJS_APP_SECRET=talkjs_app_secret [TalkJS App Secret] VONAGE_API_KEY=vonage_key [Vonage SMS API key] VONAGE_API_SECRET=vonage_secret [Vonage SMS API secret] FB_APP_ID=facebook_app_id [Facebook App ID] FB_APP_SECRET=facebook_app_secret [Facebook App secret] PRINTFUL_APP_ID=printful_app_id [Printful App ID] PRINTFUL_APP_SECRET=printful_app_secret [Printful App secret] -RECAPTCHA_SITE_KEY=recaptcha_site_key [reCAPTCHA site key] -RECAPTCHA_SECRET_KEY=recaptcha_secret_key [reCAPTCHA secret key]tests/pw/pages/basePage.ts (2)
1484-1488: Add JSDoc documentation for better maintainability.The implementation looks good but would benefit from JSDoc documentation describing the method's purpose, parameters, and return type.
Add this documentation above the method:
+/** + * Asserts that an element exists in the DOM. + * @param selector - The selector to locate the element + * @throws {Error} When the element does not exist + */ async toExists(selector: string) {
1607-1611: Add JSDoc documentation for better maintainability.The implementation looks good but would benefit from JSDoc documentation describing the method's purpose, parameters, and return type.
Add this documentation above the method:
+/** + * Asserts that an element does not exist in the DOM. + * @param selector - The selector to locate the element + * @throws {Error} When the element exists + */ async notToExists(selector: string) {tests/pw/feature-map/feature-map.yml (1)
297-317: LGTM! Well-structured feature flags for store appearance settings.The feature flags are well-organized and follow a consistent pattern. Each setting has both enable and disable capabilities, providing complete control over the features.
Consider the following architectural improvements:
- Group related settings together (e.g., all map-related settings, all template-related settings) by reordering the flags
- Add validation rules for mutually exclusive settings (e.g., map API source can't be both Google Maps and Mapbox)
- Consider adding default value indicators in the comments to improve maintainability
tests/pw/utils/payloads.ts (3)
3953-3996: Consider improving time handling in store operation hoursThe 24/7 operation configuration using "12:00 am" to "11:59 pm" could lead to edge cases:
- Potential issues during midnight transition
- Possible timezone-related problems
- Missing validation for opening time being before closing time
Consider these improvements:
storeOpenClose: { store_open_close: { enabled: 'yes', time: { monday: { status: 'open', - opening_time: ['12:00 am'], - closing_time: ['11:59 pm'], + opening_time: ['00:00'], + closing_time: ['24:00'], }, // Apply similar changes to other days }, open_notice: 'Store is open', close_notice: 'Store is closed', }, }
3723-3723: Remove unnecessary blank lineThe blank line affects code readability and should be removed.
3802-3802: Remove unnecessary blank linesMultiple blank lines affect code readability and should be removed.
Also applies to: 3887-3887
tests/pw/pages/storeAppearance.ts (3)
75-80: Address the TODO comment insetBannerSizeThere is a
//todocomment indicating that more assertions should be added to fully verify the banner size settings. Consider adding these to enhance test coverage.Would you like assistance in implementing additional assertions for this method?
83-91: Address the TODO comment inviewStoreSideBarFromThemeThere's a
// todo: add more assertionscomment in theelseblock. Adding these assertions will improve the test's effectiveness in verifying the store sidebar's absence.Would you like assistance in adding additional assertions?
105-111: Use consistent existence checks inviewFontAwesomeLibraryIn
viewFontAwesomeLibrary, you usetoExistsandnotToExiststo check for the presence of elements. For consistency and clarity, consider usingtoBeVisibleandnotToBeVisible, unless specifically checking for element existence without considering visibility.Alternatively, if the intention is to check for the presence of the library in the page's resources, consider clarifying this in the method's comments.
tests/pw/tests/e2e/storeAppearance.spec.ts (2)
37-37: Provide a justification for skipping the Google reCAPTCHA testThe test case is skipped using
test.skipin line 37 without an explanation. Including a comment detailing why the test is skipped (e.g., pending implementation, environmental constraints) will improve maintainability and clarity.Apply this diff to add a comment:
test.skip(`admin can ${status} Google reCAPTCHA validation`, { tag: ['@lite', '@admin'] }, async () => { + // Skipping due to reCAPTCHA keys not being available in the test environment. await dbUtils.updateOptionValue(dbData.dokan.optionName.appearance, { recaptcha_enable_status: status === 'enable' ? 'on' : 'off' }); await admin.viewGoogleRecaptcha(status as 'enable' | 'disable', data.predefined.vendorStores.vendor1); });
65-65: Add assertions to verify banner size settingThe test on line 65 sets the banner size using
admin.setBannerSize(data.storeBanner);but does not include assertions to confirm the change. Consider adding verification steps to ensure the banner size is updated correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
.github/workflows/e2e_api_tests.yml(1 hunks)tests/pw/.env.example(1 hunks)tests/pw/README.MD(1 hunks)tests/pw/feature-map/feature-map.yml(2 hunks)tests/pw/pages/basePage.ts(2 hunks)tests/pw/pages/selectors.ts(4 hunks)tests/pw/pages/storeAppearance.ts(1 hunks)tests/pw/tests/e2e/_auth.setup.ts(2 hunks)tests/pw/tests/e2e/storeAppearance.spec.ts(1 hunks)tests/pw/types/environment.d.ts(1 hunks)tests/pw/utils/dbData.ts(3 hunks)tests/pw/utils/payloads.ts(11 hunks)tests/pw/utils/testData.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/pw/utils/testData.ts
🔇 Additional comments (25)
tests/pw/types/environment.d.ts (1)
37-38: Ensure secure handling of reCAPTCHA credentials.
These are sensitive credentials that should be properly secured:
- Ensure these values are not committed to version control
- Verify that the
.env.examplefile contains placeholder values - Update documentation to guide developers on obtaining and configuring these keys
✅ Verification successful
reCAPTCHA credentials are properly secured with room for documentation improvement
The verification shows good security practices in place:
.env.examplecontains appropriate placeholder values- No
.envfiles are committed to the repository - No hardcoded reCAPTCHA keys found in the codebase
- Basic documentation exists in
tests/pw/README.MD
However, the documentation could be enhanced with:
- Instructions for obtaining reCAPTCHA keys from Google
- Steps for proper configuration in the testing environment
- Security best practices for handling these credentials
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify proper handling of sensitive credentials
# Check if .env.example exists and contains placeholder values
echo "Checking .env.example for reCAPTCHA placeholders..."
if [ -f "tests/pw/.env.example" ]; then
rg "RECAPTCHA_(SITE|SECRET)_KEY=[^']" "tests/pw/.env.example"
fi
# Check if .env is properly gitignored
echo "Verifying .env is gitignored..."
if [ -f ".gitignore" ]; then
rg "tests/pw/\.env$" ".gitignore"
fi
# Check documentation for setup instructions
echo "Checking for setup documentation..."
if [ -f "tests/pw/README.MD" ]; then
rg -A 5 "RECAPTCHA" "tests/pw/README.MD"
fi
Length of output: 1021
Script:
#!/bin/bash
# Let's check if there are any committed .env files or if these keys appear in any committed files
# Check for any committed .env files
echo "Checking for committed .env files..."
fd -H "\.env$" --exclude ".env.example"
# Search for potential hardcoded reCAPTCHA keys in the codebase
echo "Checking for potential hardcoded reCAPTCHA keys..."
rg -g '!{*.example,*.sample}' "RECAPTCHA_(SITE|SECRET)_KEY\s*=\s*['\"]([^'\"]+)['\"]"
# Check if there's any documentation about reCAPTCHA setup in other locations
echo "Checking for additional documentation..."
fd -t f -e md -e MD | xargs rg -l "reCAPTCHA"
Length of output: 483
tests/pw/.env.example (2)
1-20: LGTM! Well-structured plugin configuration section.
The plugin configuration section maintains consistent formatting with clear inline comments. Each sensitive credential and API key uses appropriate placeholder values.
19-20: Verify reCAPTCHA configuration in related test files.
The addition of reCAPTCHA environment variables is well-documented. However, we should ensure that these variables are properly utilized in the test implementations.
tests/pw/tests/e2e/_auth.setup.ts (1)
Line range hint 1-5: Verify required payload properties exist
The new changes depend on payloads.storeOpenClose and payloads.createStoreReview. Let's ensure these are properly defined in the payloads file.
✅ Verification successful
Required payload properties are properly defined and used
The verification confirms that:
payloads.storeOpenCloseexists with required properties (enabled, time, monday status)payloads.createStoreReviewexists with required properties (title, content, rating)- Both payloads are correctly used in the test file for API calls to update store and create reviews
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if required payload properties exist in payloads.ts
rg -A 5 "storeOpenClose|createStoreReview" tests/pw/utils/payloads.ts
Length of output: 393
Script:
#!/bin/bash
# Let's check the actual usage of these payloads in the test file
rg -A 5 "storeOpenClose|createStoreReview" tests/pw/tests/e2e/_auth.setup.ts
Length of output: 967
tests/pw/README.MD (3)
131-131: LGTM: Spelling correction for LICENSE_KEY
The spelling correction from licence_key to license_key improves consistency with standard American English spelling conventions.
124-142: Documentation is well-structured and comprehensive
The environment variables section is well-organized with:
- Clear variable names
- Descriptive comments in square brackets
- Logical grouping of related configurations
- Consistent formatting throughout
141-142: Verify reCAPTCHA configuration documentation
The addition of reCAPTCHA configuration variables is well-documented with clear descriptions. However, let's ensure these variables are properly utilized in the codebase.
✅ Verification successful
Let me gather more specific information about the reCAPTCHA implementation to verify its proper usage.
Based on the results, I can now provide a final verification response:
reCAPTCHA configuration is properly integrated in the codebase
The reCAPTCHA configuration variables are well-documented and properly utilized:
- Environment variables are correctly typed in
types/environment.d.ts - Configuration is properly set in
dbData.tsunder appearance settings - reCAPTCHA token validation is implemented in the contact seller form handler in
Ajax.php - Hidden input field for reCAPTCHA token exists in the store contact form template
- The variables are included in the
.env.examplefile
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of reCAPTCHA environment variables in the codebase
# Search for reCAPTCHA configuration usage
echo "Searching for reCAPTCHA environment variable usage..."
rg -l "RECAPTCHA_(SITE|SECRET)_KEY"
# Search for reCAPTCHA implementation
echo "Searching for reCAPTCHA implementation..."
rg -l "recaptcha"
# Check if the variables are included in the example environment file
echo "Checking .env.example file..."
cat tests/pw/.env.example
Length of output: 3090
Script:
#!/bin/bash
# Check the type definitions and actual usage of reCAPTCHA variables
echo "Checking environment type definitions..."
cat tests/pw/types/environment.d.ts
echo -e "\nChecking reCAPTCHA implementation in database utilities..."
cat tests/pw/utils/dbData.ts
echo -e "\nChecking reCAPTCHA implementation in store contact form..."
cat templates/widgets/store-contact-form.php
echo -e "\nChecking reCAPTCHA implementation in Ajax handler..."
cat includes/Ajax.php
Length of output: 99825
.github/workflows/e2e_api_tests.yml (1)
49-49: Verify MAPBOX secret configuration.
The addition of the MAPBOX environment variable looks good. However, ensure that:
- The MAPBOX secret is configured in GitHub repository settings
- The secret contains a valid Mapbox API key
tests/pw/utils/dbData.ts (1)
223-224: LGTM! Well-documented configuration options.
The added comments clearly document the valid values for each setting, improving code maintainability and developer experience.
Also applies to: 232-232, 236-236, 247-247
tests/pw/feature-map/feature-map.yml (3)
302-305: Verify reCAPTCHA and contact form dependencies.
The reCAPTCHA and contact form settings might require additional configurations.
Let's verify the related configuration files:
✅ Verification successful
Let me check the actual implementation details of these configurations.
Dependencies are properly configured and integrated
The verification shows that both reCAPTCHA and contact form features are well-integrated with proper configurations:
- reCAPTCHA v3 integration is complete with:
- Site key and Secret key configuration fields
- Hidden token field in the contact form
- Documentation links for setup guidance
- Store contact form is properly implemented with:
- Form template with required fields
- Privacy policy integration
- reCAPTCHA token integration
- Proper nonce verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for reCAPTCHA and contact form configurations
# Search for reCAPTCHA configuration
echo "Searching for reCAPTCHA configuration..."
rg -l "recaptcha|captcha" --type yaml --type php
# Search for contact form templates
echo "Searching for contact form templates..."
rg -l "contact.*form|store.*contact" --type php --type html --type twig
Length of output: 851
Script:
#!/bin/bash
# Check reCAPTCHA implementation
echo "=== ReCAPTCHA Implementation ==="
rg -A 5 "recaptcha|captcha" includes/Admin/Settings.php
echo "\n=== Contact Form Widget Implementation ==="
cat templates/widgets/store-contact-form.php
echo "\n=== Store Contact Form Settings ==="
rg -A 5 "contact.*form|store.*contact" includes/Admin/Settings.php
Length of output: 5189
297-301: Verify map API source configuration dependencies.
The map-related settings might have dependencies on API keys and configurations.
Let's verify the related configuration files:
✅ Verification successful
Map API source configuration dependencies are properly handled
The codebase shows a well-structured implementation of map API configurations:
- Map API source selection (Google Maps/Mapbox) is managed through settings with proper conditional handling
- API keys are stored securely using WordPress options:
- Google Maps:
gmap_api_keyindokan_appearanceoptions - Mapbox:
mapbox_access_tokenindokan_appearanceoptions
- Google Maps:
- Required scripts and styles are loaded conditionally based on the selected provider
- API key validation is implemented through
dokan_has_map_api_key()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for map API configuration files and settings
# Search for map-related configuration files
echo "Searching for map configuration files..."
rg -l "map.*api|google.*maps|mapbox" --type yaml --type php
# Search for API key configurations
echo "Searching for API key configurations..."
rg "GOOGLE_MAPS_API_KEY|MAPBOX_ACCESS_TOKEN" --type yaml --type php
Length of output: 945
Script:
#!/bin/bash
# Let's examine the content of relevant files to understand the map API configuration dependencies
# Check store-form.php for map settings
echo "=== Checking store settings form ==="
rg -A 5 "map.*api|google.*maps|mapbox" templates/settings/store-form.php
# Check Assets.php for API key handling
echo -e "\n=== Checking Assets.php for API configurations ==="
rg -A 5 "map.*api|google.*maps|mapbox" includes/Assets.php
# Check Admin/Settings.php for map settings
echo -e "\n=== Checking Admin Settings for map configurations ==="
rg -A 5 "map.*api|google.*maps|mapbox" includes/Admin/Settings.php
# Check for environment variables or constants
echo -e "\n=== Checking for map API constants or configurations ==="
rg "define.*(?i)(GOOGLE_MAPS_API|MAPBOX.*TOKEN)" includes/
Length of output: 5353
306-309: Ensure store header templates are properly defined.
The store header template settings reference four different layouts.
Let's verify the template files exist:
✅ Verification successful
Let's verify the actual content of these files to check if they define the mentioned layouts.
Store header templates are properly defined and implemented.
The verification shows that all four layouts (default, layout1, layout2, and layout3) are properly defined in the codebase:
- The templates are defined in the admin settings (
includes/Admin/Settings.php) with corresponding image assets - The template implementation exists in
templates/store-header.phpwith proper conditional logic for each layout - The system correctly handles different layouts with specific styling and structure variations (e.g., profile image shapes, banner handling)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for store header template files
# Search for store header template files
echo "Searching for store header templates..."
rg -l "store.*header.*template|store.*header.*layout" --type php --type html --type twig
Length of output: 318
Script:
#!/bin/bash
# Check content of relevant files for layout definitions
echo "=== Checking templates/store-header.php ==="
rg -A 5 "layout[1-3]|default" templates/store-header.php
echo -e "\n=== Checking templates/settings/store-form.php ==="
rg -A 5 "layout[1-3]|default" templates/settings/store-form.php
echo -e "\n=== Checking includes/Admin/Settings.php ==="
rg -A 5 "layout[1-3]|default" includes/Admin/Settings.php
echo -e "\n=== Checking for template related functions ==="
ast-grep --pattern 'function $FUNC($$$) {
$$$
$TEMPLATE
$$$
}' --lang php
Length of output: 17424
tests/pw/pages/selectors.ts (4)
6025-6025: LGTM! Added banner help text selector.
The addition of the banner help text selector improves test coverage for the store banner UI component.
7384-7396: LGTM! Improved profile info selectors organization.
The changes enhance selector specificity and maintainability by:
- Using more semantic class names
- Grouping related selectors logically
- Following consistent naming conventions
7528-7529: LGTM! Added store sidebar selector.
The addition of the store sidebar widget area selector follows established patterns and uses semantic class names.
7540-7551: LGTM! Added comprehensive store-related selectors.
The additions enhance test coverage by adding selectors for:
- Store contact form
- Store map
- Store open/close time
- Google reCAPTCHA
- Font Awesome library
The selectors follow consistent naming conventions and use semantic class names.
tests/pw/pages/storeAppearance.ts (7)
15-16: Method gotoSingleStore is implemented correctly
The method correctly navigates to the single store page using the provided store name.
39-47: Method viewGoogleRecaptcha is implemented correctly
The method correctly checks the visibility of Google reCAPTCHA based on the status parameter.
50-57: Method viewStoreContactFormOnStoreSidebar is implemented correctly
The method appropriately checks the visibility of the store contact form based on the status parameter.
60-63: Method viewStoreHeaderTemplate is implemented correctly
The method correctly verifies the class of the store profile info box based on the selected template.
66-73: Method viewStoreOpenCloseTimeOnStoreSidebar is implemented correctly
The method accurately checks the visibility of the store open-close time based on the status parameter.
94-102: Method viewVendorInfoOnSingleStorePage is implemented correctly
The method appropriately checks the visibility of vendor information elements based on the status parameter.
30-37: Validate API parameter handling in viewMapAPISource
Currently, the method assumes that the api parameter will only be 'Google Maps' or 'Mapbox'. Consider adding input validation or a default case to handle unexpected values, preventing potential runtime errors.
Run the following script to ensure that viewMapAPISource is called with valid parameters elsewhere in the codebase:
tests/pw/tests/e2e/storeAppearance.spec.ts (2)
70-70: Verify logic of 'enable_theme_store_sidebar' option
In line 70, the enable_theme_store_sidebar option is set as follows:
enable_theme_store_sidebar: status === 'enable' ? 'off' : 'on'This means when status is 'enable', the option is set to 'off', and vice versa. This might be counterintuitive. Please verify if this logic correctly reflects the intended behavior.
77-77: Confirm correctness of 'hide_vendor_info' settings
In line 77, the hide_vendor_info is assigned based on the status:
hide_vendor_info: status === 'enable' ? { email: '', phone: '', address: '' } : { email: 'email', phone: 'phone', address: 'address' }Assigning empty strings when status is 'enable' may not effectively hide vendor information. Please check if this implementation achieves the desired effect of showing or hiding vendor info.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tests/pw/tests/e2e/storeAppearance.spec.ts (1)
43-86: Consider enhancing test coverage with edge casesThe current implementation provides good coverage for basic enable/disable scenarios. Consider adding tests for:
- Invalid configurations
- Boundary conditions for banner size
- Race conditions when rapidly toggling settings
Example test cases to consider:
test('admin cannot set invalid banner dimensions', async () => { // Test negative values, zero, extremely large values }); test('admin cannot set conflicting store settings', async () => { // Test mutually exclusive settings });tests/pw/feature-map/feature-map.yml (2)
304-309: Consider adding granular controls for store header templates.The store header template settings are well-defined but could benefit from more granular controls:
- Consider adding preview capabilities for each template
- Add ability to customize individual elements within each template
- Consider adding custom template support
310-315: Enhance store sidebar and vendor info settings.The store sidebar and vendor info settings are logically grouped but could be improved:
- Consider adding granular controls for specific sidebar elements
- Add ability to reorder sidebar elements
- Consider adding custom sidebar widget support
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
tests/pw/.env.example(1 hunks)tests/pw/README.MD(1 hunks)tests/pw/feature-map/feature-map.yml(2 hunks)tests/pw/pages/selectors.ts(4 hunks)tests/pw/tests/e2e/storeAppearance.spec.ts(1 hunks)tests/pw/utils/testData.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/pw/README.MD
- tests/pw/.env.example
- tests/pw/utils/testData.ts
- tests/pw/pages/selectors.ts
🔇 Additional comments (5)
tests/pw/tests/e2e/storeAppearance.spec.ts (3)
7-20: LGTM! Test setup correctly configured for admin context
The setup is properly configured with admin authentication and appropriate cleanup in the afterAll hook.
36-41: Verify reCAPTCHA environment setup
The reCAPTCHA validation tests are skipped. Based on the PR context, this might be related to pending environment variable setup (RECAPTCHA_SITE_KEY, RECAPTCHA_SECRET_KEY).
Let's check the environment configuration:
✅ Verification successful
Let me check the environment configuration file to see if these variables are required or optional.
Tests skipped due to required reCAPTCHA configuration
The tests are intentionally skipped as they require valid reCAPTCHA credentials. The environment type definitions confirm that both RECAPTCHA_SITE_KEY and RECAPTCHA_SECRET_KEY are required environment variables for these tests. To run these tests:
- Set up
RECAPTCHA_SITE_KEYenvironment variable with a valid Google reCAPTCHA site key - Set up
RECAPTCHA_SECRET_KEYenvironment variable with a valid Google reCAPTCHA secret key
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for reCAPTCHA configuration
rg -i "recaptcha" --type ts
Length of output: 2076
Script:
#!/bin/bash
# Check environment configuration
cat tests/pw/types/environment.d.ts
Length of output: 2101
29-34: Verify reason for skipped map API source tests
The map API source tests are currently skipped. This might be due to pending implementation or environment setup requirements.
Let's check if there are any related TODOs or implementation gaps:
tests/pw/feature-map/feature-map.yml (2)
316-317: Verify FontAwesome library integration.
The FontAwesome library settings are minimal but sufficient. However:
- Consider adding version control for the FontAwesome library
- Add ability to load only required icons to optimize performance
✅ Verification successful
Based on the search results, I can see that FontAwesome is extensively used throughout the codebase for icons, with both Free and Pro versions (fas, far, fab prefixes). The implementation appears to be properly integrated with clear version control through CSS classes and consistent usage patterns.
FontAwesome integration is properly implemented with sufficient version control and optimized loading.
- FontAwesome is loaded through CSS classes with proper prefixes (
fas,far,fab) indicating version control - Icons are loaded on-demand through CSS classes rather than including the entire library
- The implementation follows FontAwesome's recommended best practices for performance optimization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check FontAwesome implementation details
# Search for FontAwesome version and loading mechanism
rg -A 5 'fontawesome.*\.(css|js)'
# Check for icon usage optimization
rg '\bfa-[a-z-]+\b'
Length of output: 66334
297-303: Verify map-related settings are complete and secure.
The map-related settings look good but consider the following security aspects:
- Ensure API keys for both Google Maps and Mapbox are properly validated and stored securely
- Consider adding rate limiting for the reCAPTCHA validation to prevent abuse
✅ Verification successful
Based on the search results, I can now provide a complete verification of the security concerns:
API keys and reCAPTCHA implementation are properly secured
The codebase shows proper implementation of API key handling and validation:
-
Map API keys (Google Maps and Mapbox) are:
- Stored securely in WordPress options
- Retrieved via
dokan_get_option()function - Validated before use
- Not hardcoded in the codebase
-
reCAPTCHA implementation includes:
- Proper server-side validation via
dokan_handle_recaptcha_validation() - Rate limiting through token validation
- Secure key storage using WordPress options
- Client-side validation before form submission
- Proper server-side validation via
-
Additional security measures:
- Keys are stored with
secret_textflag - API requests use nonces for CSRF protection
- Input validation before using API keys
- Error handling for invalid/missing keys
- Keys are stored with
The implementation follows security best practices for handling sensitive credentials and API integrations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for secure storage of API keys in the codebase
# Search for potential hardcoded API keys
rg -i '(google|mapbox|recaptcha).*key.*=.*["\'].*["\']'
# Search for proper key validation
ast-grep --pattern 'function validate$_Key($_) {
$$$
}'
Length of output: 116
Script:
#!/bin/bash
# Search for API key related configurations and validations
rg -i "(google|mapbox|recaptcha).*(key|token|secret)" -A 3
# Search for map and reCAPTCHA related configuration files
fd -e php -e js -e yml -e yaml -e json -e env.example | grep -i "map\|captcha\|config"
# Search for validation related to API keys
rg -i "validate.*(key|token|secret)" -A 3
Length of output: 119959
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
tests/pw/pages/basePage.ts (3)
1484-1488: Add JSDoc documentation for better maintainability.The implementation looks good but would benefit from documentation.
Add JSDoc comment to document the method:
+ /** + * Asserts that an element matching the selector exists in the DOM + * @param selector - The selector to check for existence + */ async toExists(selector: string) { const exists = await this.isLocatorExists(selector); expect(exists).toBe(true); }
1607-1611: Add JSDoc documentation for better maintainability.The implementation looks good but would benefit from documentation.
Add JSDoc comment to document the method:
+ /** + * Asserts that an element matching the selector does not exist in the DOM + * @param selector - The selector to check for non-existence + */ async notToExists(selector: string) { const exists = await this.isLocatorExists(selector); expect(exists).toBe(false); }
Line range hint
1484-1611: Consider adding timeout options for consistency.The new assertion methods would benefit from having timeout options like other assertion methods in the class.
Consider adding timeout options:
- async toExists(selector: string) { + async toExists(selector: string, options?: { timeout?: number } | undefined) { const exists = await this.isLocatorExists(selector); expect(exists).toBe(true); } - async notToExists(selector: string) { + async notToExists(selector: string, options?: { timeout?: number } | undefined) { const exists = await this.isLocatorExists(selector); expect(exists).toBe(false); }tests/pw/utils/payloads.ts (1)
3954-3997: Fix indentation while maintaining the correct structure.The store hours configuration is functionally correct but needs consistent indentation with the rest of the file.
Apply this formatting:
storeOpenClose: { store_open_close: { enabled: 'yes', time: { monday: { status: 'open', opening_time: ['12:00 am'], closing_time: ['11:59 pm'], }, tuesday: { status: 'open', opening_time: ['12:00 am'], closing_time: ['11:59 pm'], }, // ... rest of the days }, open_notice: 'Store is open', close_notice: 'Store is closed', }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
tests/pw/feature-map/feature-map.yml(2 hunks)tests/pw/pages/basePage.ts(2 hunks)tests/pw/pages/selectors.ts(4 hunks)tests/pw/utils/payloads.ts(11 hunks)tests/pw/utils/testData.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/pw/utils/testData.ts
- tests/pw/pages/selectors.ts
🔇 Additional comments (5)
tests/pw/feature-map/feature-map.yml (2)
297-317: LGTM! Well-structured store appearance features.
The admin capabilities for store appearance settings are well-organized with:
- Clear enable/disable toggles for each feature
- Proper scoping of permissions to admin-only actions
- Consistent [lite] tagging
- Comprehensive coverage of UI elements
671-671: LGTM! Consistent page naming.
The page name change from "Feature lock" to "Feature Lock" improves naming consistency across the feature map.
tests/pw/utils/payloads.ts (3)
3724-3724: LGTM! Note the side effect on product publishing.
The featured property is correctly added. Note that disabling this property also affects the ability to publish products directly.
3803-3803: LGTM! Shop URL can be empty by default.
The shop_url property is correctly added with an empty string default, which is appropriate for store initialization.
3888-3888: LGTM! Terms & Conditions toggle is properly configured.
The enable_tnc property is correctly added with 'on' value to enable Terms & Conditions for the store.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
tests/pw/feature-map/feature-map.yml (1)
297-317: LGTM! Consider grouping related features for better maintainability.The store appearance features are comprehensive and well-defined. However, consider grouping related features together for better maintainability.
Consider reorganizing the features into logical groups:
admin: # Map Settings - admin can enable store map on store sidebar [lite]: true - admin can disable store map on store sidebar [lite]: true - admin can set map api source (Google Maps) [lite]: true - admin can set map api source (Mapbox) [lite]: true + map_settings: + admin can enable store map on store sidebar [lite]: true + admin can disable store map on store sidebar [lite]: true + admin can set map api source (Google Maps) [lite]: true + admin can set map api source (Mapbox) [lite]: true # Security Settings + security_settings: + admin can enable Google reCAPTCHA validation [lite]: true + admin can disable Google reCAPTCHA validation [lite]: true # Store Layout Settings + layout_settings: + admin can set store header template (default) [lite]: true + admin can set store header template (layout1) [lite]: true + admin can set store header template (layout2) [lite]: true + admin can set store header template (layout3) [lite]: true # Sidebar Settings + sidebar_settings: + admin can enable store contact form on store sidebar [lite]: true + admin can disable store contact form on store sidebar [lite]: true + admin can enable store open-close time on store sidebar [lite]: true + admin can disable store open-close time on store sidebar [lite]: true + admin can enable store sidebar from theme [lite]: true + admin can disable store sidebar from theme [lite]: true + admin can enable vendor info on single store page [lite]: true + admin can disable vendor info on single store page [lite]: true # Asset Settings + asset_settings: + admin can enable Dokan FontAwesome library [lite]: true + admin can disable Dokan FontAwesome library [lite]: truetests/pw/utils/payloads.ts (4)
Line range hint
3789-4062: Consider using 24-hour format and standardizing time handlingThe store hours implementation could be improved in the following ways:
- Use 24-hour format (00:00 to 23:59) instead of 12-hour format to avoid AM/PM ambiguity
- Consider using ISO 8601 time format for better time zone handling
- Consider adding validation to ensure closing time is after opening time
Example implementation:
store_open_close: { enabled: 'yes', time: { monday: { status: 'open', - opening_time: ['12:00 am'], - closing_time: ['11:59 pm'], + opening_time: ['00:00'], + closing_time: ['23:59'], }, // Apply similar changes to other days } }
4100-4102: Remove commented code about unsupported rating functionalityThe commented code about store rating should be removed since it's not supported by the API. If this information needs to be preserved, consider moving it to documentation or adding a more descriptive comment about the API limitation.
- // rating: { - // rating: '4.00', // todo: not supported on create api, has separate api store reviews - // count: 1, - // }, + // Note: Store rating is handled through a separate store reviews API
4120-4127: Document payment gateway requirements separatelyInstead of keeping commented payment gateway configurations in the test payload, consider creating a separate documentation file or configuration template for supported and planned payment gateways.
Consider moving these configurations to a separate file like
payment-gateway-templates.ts:export const paymentGatewayTemplates = { moipConnect: { enabled: boolean; }, razorpay: { enabled: boolean; }, stripeExpress: { enabled: boolean; }, customMethod: { withdrawMethodName: string; withdrawMethodType: string; value: string; } };
4189-4190: Make commission values configurable for test scenariosConsider making the commission values configurable through test configuration or environment variables to support different test scenarios.
- admin_commission: '5', - admin_additional_fee: '5', + admin_commission: process.env.DEFAULT_ADMIN_COMMISSION || '5', + admin_additional_fee: process.env.DEFAULT_ADMIN_FEE || '5',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
tests/pw/feature-map/feature-map.yml(2 hunks)tests/pw/pages/basePage.ts(2 hunks)tests/pw/pages/selectors.ts(5 hunks)tests/pw/utils/payloads.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/pw/pages/basePage.ts
- tests/pw/pages/selectors.ts
🔇 Additional comments (1)
tests/pw/feature-map/feature-map.yml (1)
680-680: LGTM! Title case correction improves consistency.
The change from "Feature lock" to "Feature Lock" follows proper title case convention.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
tests/pw/utils/payloads.ts (1)
3896-3896: Remove unused or empty 'categories' arrayThe
categoriesproperty is assigned an empty object{}within an array. If no categories are specified, consider removing this property or ensuring it is correctly populated.Apply this diff to remove or correct the categories:
- categories: [{}], + // categories can be added here if neededtests/pw/tests/e2e/catalogmode.spec.ts (1)
58-62: Remove unnecessary comment or address the issueThere's a comment
// resetfollowed by the reset operation. If there's more to be added or if this is a placeholder, please complete it or remove the comment.tests/pw/README.MD (1)
9-34: Fix markdown formatting issuesThe table of contents section has inconsistent list indentation and invalid link fragments.
Apply these changes:
- Standardize list indentation to 2 spaces
- Update link fragments to match heading IDs
- Remove spaces inside link text
Example fix:
- [Pre-requisites :pushpin:](#pre-requisites) - - [Install Node](#install-node) + [Pre-requisites :pushpin:](#pre-requisites) + - [Install Node](#install-node)🧰 Tools
🪛 Markdownlint (0.35.0)
10-10: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
11-11: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
12-12: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
17-17: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
18-18: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
19-19: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
20-20: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
25-25: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
26-26: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
27-27: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
28-28: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
29-29: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
34-34: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
24-24: null
Spaces inside link text(MD039, no-space-in-links)
9-9: null
Link fragments should be valid(MD051, link-fragments)
10-10: null
Link fragments should be valid(MD051, link-fragments)
11-11: null
Link fragments should be valid(MD051, link-fragments)
12-12: null
Link fragments should be valid(MD051, link-fragments)
16-16: null
Link fragments should be valid(MD051, link-fragments)
17-17: null
Link fragments should be valid(MD051, link-fragments)
18-18: null
Link fragments should be valid(MD051, link-fragments)
19-19: null
Link fragments should be valid(MD051, link-fragments)
20-20: null
Link fragments should be valid(MD051, link-fragments)
24-24: null
Link fragments should be valid(MD051, link-fragments)
25-25: null
Link fragments should be valid(MD051, link-fragments)
26-26: null
Link fragments should be valid(MD051, link-fragments)
27-27: null
Link fragments should be valid(MD051, link-fragments)
28-28: null
Link fragments should be valid(MD051, link-fragments)
29-29: null
Link fragments should be valid(MD051, link-fragments)
33-33: null
Link fragments should be valid(MD051, link-fragments)
34-34: null
Link fragments should be valid(MD051, link-fragments)
tests/pw/utils/dbData.ts (1)
223-236: Consider adding validation for map API keysSimilar to reCAPTCHA keys, the map API keys (Google Maps and Mapbox) should be validated.
Add validation:
function validateMapApiKey(key: string | undefined, provider: 'google_maps' | 'mapbox'): string { if (!key && process.env.NODE_ENV === 'production') { console.warn(`${provider} API key not set in production environment`); } return key ?? ''; }Apply the validation:
- gmap_api_key: GMAP ?? '', - mapbox_access_token: MAPBOX ?? '', + gmap_api_key: validateMapApiKey(GMAP, 'google_maps'), + mapbox_access_token: validateMapApiKey(MAPBOX, 'mapbox'),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
tests/pw/README.MD(2 hunks)tests/pw/tests/e2e/_env.setup.ts(1 hunks)tests/pw/tests/e2e/catalogmode.spec.ts(2 hunks)tests/pw/tests/e2e/euCompliance.spec.ts(1 hunks)tests/pw/tests/e2e/storeAppearance.spec.ts(1 hunks)tests/pw/tests/e2e/vendor.spec.ts(1 hunks)tests/pw/utils/dbData.ts(4 hunks)tests/pw/utils/payloads.ts(15 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/pw/tests/e2e/_env.setup.ts
🧰 Additional context used
🪛 Markdownlint (0.35.0)
tests/pw/README.MD
10-10: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
11-11: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
12-12: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
17-17: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
18-18: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
19-19: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
20-20: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
25-25: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
26-26: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
27-27: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
28-28: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
29-29: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
34-34: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
24-24: null
Spaces inside link text
(MD039, no-space-in-links)
9-9: null
Link fragments should be valid
(MD051, link-fragments)
10-10: null
Link fragments should be valid
(MD051, link-fragments)
11-11: null
Link fragments should be valid
(MD051, link-fragments)
12-12: null
Link fragments should be valid
(MD051, link-fragments)
16-16: null
Link fragments should be valid
(MD051, link-fragments)
17-17: null
Link fragments should be valid
(MD051, link-fragments)
18-18: null
Link fragments should be valid
(MD051, link-fragments)
19-19: null
Link fragments should be valid
(MD051, link-fragments)
20-20: null
Link fragments should be valid
(MD051, link-fragments)
24-24: null
Link fragments should be valid
(MD051, link-fragments)
25-25: null
Link fragments should be valid
(MD051, link-fragments)
26-26: null
Link fragments should be valid
(MD051, link-fragments)
27-27: null
Link fragments should be valid
(MD051, link-fragments)
28-28: null
Link fragments should be valid
(MD051, link-fragments)
29-29: null
Link fragments should be valid
(MD051, link-fragments)
33-33: null
Link fragments should be valid
(MD051, link-fragments)
34-34: null
Link fragments should be valid
(MD051, link-fragments)
🔇 Additional comments (12)
tests/pw/tests/e2e/euCompliance.spec.ts (1)
Line range hint 132-152: Address skipped tests and incomplete coverage.
There are several skipped tests (test.skip) related to product EU compliance data and a TODO comment indicating incomplete test coverage. This could lead to untested functionality and potential issues in production.
Let's check the current test coverage:
Would you like help with:
- Implementing the skipped product EU compliance tests?
- Creating a comprehensive test plan for the remaining test cases mentioned in the TODO?
Also applies to: 166-168
tests/pw/utils/payloads.ts (4)
3789-3797: Verify the data type changes for 'featured', 'enabled', and 'trusted' properties
The properties featured, enabled, and trusted have been updated to use string values 'yes' instead of booleans. Please verify that all parts of the codebase that consume these properties are updated accordingly to handle string values.
To ensure consistency, check if other functions or modules expect these properties as booleans. If necessary, update them to handle string values or consider keeping the original boolean data types.
3813-3815: Ensure correct handling of 'store_open_close.enabled' as a string
The enabled property within store_open_close is now a string 'yes' instead of a boolean. Verify that any code that checks this property can handle the string value appropriately.
Consider updating conditional checks that rely on store_open_close.enabled to compare with the string value 'yes' or adjust it to use booleans for consistency.
Line range hint 3853-3877: Verify the consistency of 'show_email' property and missing 'store_tnc'
- The
show_emailproperty is assigned the string'yes'. Ensure that this is consistent with how other boolean-like settings are handled. - The
store_tnc(Terms and Conditions) property appears to be missing in this segment. Verify if this omission is intentional.
If store_tnc is required, consider adding it back to maintain completeness of the store settings.
3952-3956: Ensure 'enable_tnc' and 'store_tnc' are properly set
The enable_tnc property is set to 'on', and store_tnc is assigned a value. Confirm that these settings are correctly applied and that the front-end logic handles the 'on' string appropriately.
Double-check that the Terms and Conditions functionality works as expected with these settings.
tests/pw/tests/e2e/vendor.spec.ts (1)
29-33: Confirm the correctness of updating and resetting the option value
In the test for vendor registration with address fields enabled, the previous options are no longer stored before updating, and the reset operation directly sets the option value back. Verify that this change does not affect other tests or global settings.
Ensure that skipping the storage of previous settings does not introduce side effects. If necessary, consider storing the previous state to maintain test isolation.
tests/pw/tests/e2e/catalogmode.spec.ts (1)
58-62: Ensure proper resetting of catalog mode settings after the test
The test directly updates the option value to disable hiding product prices and then resets it back. Confirm that this approach does not interfere with other tests and that the reset operation accurately restores the original setting.
Consider storing the original setting before changing it and restoring it afterward to prevent potential side effects.
tests/pw/tests/e2e/storeAppearance.spec.ts (2)
17-18: Rename 'adminContext' variable for clarity
The variable adminContext is correctly initialized with admin authentication, but ensure that aPage is consistently used as the admin page throughout the test suite for clarity.
92-93:
Correct the logic for enabling and disabling FontAwesome library
The condition seems inverted where enabling is set to 'off' and disabling is set to 'on'. Verify that the disable_dokan_fontawesome option is correctly set based on the status.
Apply this diff to correct the logic:
await dbUtils.updateOptionValue(
dbData.dokan.optionName.appearance,
- { disable_dokan_fontawesome: status === 'enable' ? 'off' : 'on' }
+ { disable_dokan_fontawesome: status === 'enable' ? 'on' : 'off' }
);This ensures that when the status is 'enable', the FontAwesome library is disabled, and vice versa.
Likely invalid or redundant comment.
tests/pw/README.MD (1)
132-134: LGTM: New environment variables are well documented
The new environment variables for Mapbox and reCAPTCHA are properly documented with clear descriptions.
Also applies to: 142-143
tests/pw/utils/dbData.ts (2)
228-230: Add validation for reCAPTCHA environment variables
The reCAPTCHA keys should be validated before use.
1432-1452: LGTM: Vendor info objects are properly structured
The new unhideVendorInfo and unhideVendorEuInfo objects mirror the existing hide vendor info structure, maintaining consistency.
| await apiUtils.updateStore(VENDOR_ID, { show_email: 'yes' }, payloads.adminAuth); | ||
| await dbUtils.updateOptionValue(dbData.dokan.optionName.appearance, { hide_vendor_info: status === 'enable' ? { email: '', phone: '', address: '' } : { email: 'email', phone: 'phone', address: 'address' } }); | ||
| await admin.viewVendorInfoOnSingleStorePage(status as 'enable' | 'disable', data.predefined.vendorStores.vendor1); |
There was a problem hiding this comment.
Handle potential issues with hardcoded IDs and environment variables
In lines 84-86, the test updates the store using VENDOR_ID from the environment variables. Ensure that VENDOR_ID is properly defined and does not lead to issues in different test environments.
Consider adding error handling or fallback mechanisms if VENDOR_ID is undefined. Alternatively, retrieve the vendor ID dynamically within the test setup.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
tests/pw/tests/e2e/storeAppearance.spec.ts (1)
9-10: Consider using typed environment variables.The environment variable
VENDOR_IDis destructured without type checking.-const { VENDOR_ID } = process.env; +const VENDOR_ID = process.env.VENDOR_ID as string;tests/pw/utils/payloads.ts (2)
3820-3861: Consider using TypeScript enums for status values.The store open/close configuration uses string literals for status values. Using enums would provide better type safety.
enum StoreStatus { OPEN = 'open', CLOSED = 'close' } // Usage in payload: status: StoreStatus.OPEN
4150-4193: Consider validating time formats.The store open/close times are hardcoded without validation. Consider adding time format validation.
const isValidTimeFormat = (time: string): boolean => { return /^(0?[1-9]|1[0-2]):[0-5][0-9] (am|pm)$/i.test(time); }; // Usage in payload: opening_time: isValidTimeFormat(time) ? time : '12:00 am'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
tests/pw/e2e.config.ts(1 hunks)tests/pw/tests/e2e/_auth.setup.ts(3 hunks)tests/pw/tests/e2e/storeAppearance.spec.ts(1 hunks)tests/pw/utils/payloads.ts(20 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/pw/tests/e2e/_auth.setup.ts
🔇 Additional comments (8)
tests/pw/tests/e2e/storeAppearance.spec.ts (3)
1-7: LGTM! Clean and well-organized imports.
The imports are properly organized, bringing in required test utilities and data.
11-21: LGTM! Well-structured test suite setup.
The test suite setup properly initializes admin context and required utilities.
82-86:
Handle potential undefined VENDOR_ID.
The test uses VENDOR_ID directly without checking if it's defined, which could cause runtime errors.
-await apiUtils.updateStore(VENDOR_ID, { ...payloads.storeResetFields, show_email: 'yes' }, payloads.adminAuth);
+if (!VENDOR_ID) {
+ throw new Error('VENDOR_ID environment variable is required');
+}
+await apiUtils.updateStore(VENDOR_ID, { ...payloads.storeResetFields, show_email: 'yes' }, payloads.adminAuth);Likely invalid or redundant comment.
tests/pw/utils/payloads.ts (3)
3763-3764: LGTM! Proper role and notification settings.
The store creation payload correctly sets the seller role and vendor notification.
3788-3792: LGTM! Essential store settings properly configured.
The store settings for terms & conditions, featured status, and trust level are well defined.
4139-4149: LGTM! Clear store reset fields configuration.
The store reset fields are well-defined with appropriate default values.
tests/pw/e2e.config.ts (2)
28-28: Review necessity of increased expect timeout
The expect timeout has been increased from 10s to 15s globally. This is a significant increase that could hide UI responsiveness issues. Consider:
- Is this increase necessary for all assertions?
- Could specific tests override the timeout instead?
- Are there slow UI responses that need optimization?
Let's identify assertions that might be hitting timeouts:
24-24: Investigate root cause for increased test timeout
The test timeout has been increased from 40s to 45s in non-CI mode. While this might fix failing tests, it could be masking underlying performance issues. Consider:
- Investigating why tests need longer to complete
- Profiling slow tests to identify bottlenecks
- Adding performance monitoring to track test execution times
Let's identify potentially slow tests:
✅ Verification successful
Based on the search results, I can now provide a final response:
Timeout increase appears justified due to test complexity
The timeout increase from 40s to 45s in non-CI mode appears reasonable given the codebase context:
-
The tests involve complex interactions including:
- Multiple waitFor conditions for UI elements
- Network request/response handling
- File upload operations
- Modal dialogs and alerts
- State transitions and animations
-
The timeout is only increased in non-CI mode, maintaining stricter 35s limit for CI environments
-
The codebase shows consistent usage of proper wait patterns rather than arbitrary delays:
- Specific element wait conditions
- Network request completion waits
- State-based waits
- Event-based waits
The timeout increase appears to accommodate legitimate test complexity rather than mask performance issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find tests that are taking longer than expected
# Look for tests using waitFor, wait, or timeout related calls
rg -A 2 "waitFor|page\.wait|setTimeout" tests/e2e/
Length of output: 103
Script:
#!/bin/bash
# Let's first find the correct test directory structure
fd -t d "test|tests|e2e"
# Then search for test files
fd -e ts -e js "test|spec"
# Finally, let's look for wait/timeout patterns in these files
rg -A 2 "waitFor|page\.wait|setTimeout|test\.setTimeout"
Length of output: 120965
Script:
#!/bin/bash
# Let's search for test timeouts and wait patterns in the Playwright test files
rg -A 2 "waitFor|page\.wait|setTimeout|test\.setTimeout|timeout:" tests/pw/
Length of output: 16447
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
tests/pw/tests/e2e/liveSearch.spec.ts (1)
16-17: LGTM! Consider improving test isolation.The refactoring to use
dbDataconstants improves maintainability. However, consider moving the widget setup to individual test cases if they have different requirements, following the principle of test isolation.Example refactor:
async function setupLiveSearchWidget(type: 'suggestion' | 'autoload') { await dbUtils.updateOptionValue(dbData.dokanWidgets.names.liveSearch, dbData.dokanWidgets.values.liveSearchWidget); await dbUtils.updateOptionValue('sidebars_widgets', { ...dbData.sidebarWidgets, 'sidebar-1': [dbData.dokanWidgets.widgets.liveSearch] }); if (type === 'autoload') { await dbUtils.updateOptionValue(dbData.dokan.optionName.liveSearch, { live_search_option: 'old_live_search' }); } }tests/pw/tests/e2e/privacyPolicy.spec.ts (1)
19-20: Add comments explaining widget setup purpose.While the code is correct, adding comments would help explain why this specific widget setup is necessary for the tests. Consider documenting the relationship between the store contact form widget and the privacy policy tests.
Example:
// Setup store contact form widget in sidebar for privacy policy tests // This widget is required to test customer-vendor communication features await dbUtils.updateOptionValue(/*...*/);tests/pw/tests/e2e/storeAppearance.spec.ts (1)
90-91: Remove commented code and TODO.The commented code and TODO comment suggest incomplete implementation.
Either implement the fix or create an issue to track it:
- // await apiUtils.updateStore(VENDOR_ID, { show_email: 'yes' }, payloads.adminAuth); // todo: apply this after issue fix - await apiUtils.updateStore(VENDOR_ID, { ...payloads.storeResetFields, show_email: 'yes' }, payloads.adminAuth); + await apiUtils.updateStore(VENDOR_ID, { ...payloads.storeResetFields, show_email: 'yes' }, payloads.adminAuth);tests/pw/tests/e2e/commission.spec.ts (1)
Line range hint
89-101: Implement or remove skipped subscription product tests.Multiple tests are skipped with a TODO comment about implementing
createDokanSubscriptionProduct. This reduces test coverage.Would you like help implementing the
createDokanSubscriptionProductfunction? I can help create a proper implementation that:
- Creates a subscription product for testing
- Handles cleanup in afterAll
- Provides proper error handling
tests/pw/utils/payloads.ts (2)
Line range hint
3788-3818: Consider using enums for payment method configurationThe payment method configuration uses hardcoded string values. Consider using TypeScript enums to make the code more maintainable and type-safe.
+ enum PaymentMethodType { + PAYPAL = 'paypal', + BANK = 'bank', + SKRILL = 'skrill', + CUSTOM = 'dokan_custom' + } payment: { - paypal: { + [PaymentMethodType.PAYPAL]: { email: '[email protected]', }, // ... other payment methods }
4139-4149: Consider adding validation for store reset fieldsThe store reset fields contain boolean values represented as strings ('yes'). Consider adding validation to ensure only valid values are accepted.
+ const VALID_BOOLEAN_STRING = ['yes', 'no'] as const; + type BooleanString = typeof VALID_BOOLEAN_STRING[number]; storeResetFields: { - featured: 'yes', + featured: BooleanString, // ... other fields }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
tests/pw/tests/e2e/_auth.setup.ts(3 hunks)tests/pw/tests/e2e/commission.spec.ts(1 hunks)tests/pw/tests/e2e/liveSearch.spec.ts(1 hunks)tests/pw/tests/e2e/privacyPolicy.spec.ts(1 hunks)tests/pw/tests/e2e/storeAppearance.spec.ts(1 hunks)tests/pw/tests/e2e/vendorProductSubscription.spec.ts(1 hunks)tests/pw/utils/dbData.ts(5 hunks)tests/pw/utils/payloads.ts(21 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/pw/tests/e2e/_auth.setup.ts
- tests/pw/utils/dbData.ts
🔇 Additional comments (6)
tests/pw/tests/e2e/privacyPolicy.spec.ts (1)
26-26: Consider enhancing cleanup comprehensiveness.
The cleanup restores the sidebar widgets but consider also resetting any other state modified by the tests to ensure complete test isolation.
Let's verify what other cleanup might be needed:
tests/pw/tests/e2e/vendorProductSubscription.spec.ts (1)
Line range hint 35-101: Multiple skipped tests indicate incomplete implementation.
Most test cases are skipped, suggesting incomplete implementation. This could impact test coverage.
Let's check the test coverage:
Would you like help implementing these skipped test cases?
tests/pw/tests/e2e/storeAppearance.spec.ts (1)
9-9:
Add environment variable validation.
The VENDOR_ID is destructured from process.env without validation, which could lead to runtime errors if undefined.
Add validation at the start of the test suite:
-const { VENDOR_ID } = process.env;
+const VENDOR_ID = process.env.VENDOR_ID;
+if (!VENDOR_ID) {
+ throw new Error('VENDOR_ID environment variable is required');
+}Likely invalid or redundant comment.
tests/pw/tests/e2e/commission.spec.ts (1)
16-16:
Avoid hardcoding subscription product ID.
Similar to the issue in vendorProductSubscription.spec.ts, initializing subscriptionProductId with a hardcoded value ('1') creates a tight coupling to specific test data.
Consider:
- Creating the subscription product as part of the test setup
- Retrieving an existing subscription product ID dynamically
- Using environment variables for test data
-let subscriptionProductId: string = '1';
+let subscriptionProductId: string;tests/pw/utils/payloads.ts (2)
3763-3764: LGTM: Role and notification settings are properly configured
The role is correctly set to 'seller' and vendor notification is enabled with 'yes', which follows best practices for store creation.
3876-3891: Verify commission calculation logic
The commission settings include both flat and percentage rates. Need to verify that these values are properly handled in the actual commission calculation logic.
✅ Verification successful
Based on the search results, I can now provide a final response about the commission calculation verification:
Commission calculation implementation is properly handled
The codebase has a robust commission calculation system with:
- Dedicated Commission classes handling different commission types (Fixed, Percentage, Combined, Category-based)
- Comprehensive test coverage verifying commission calculations for all scenarios
- Proper handling of both flat and percentage rates through:
Calculator::calculate_commission()method- Formula classes that implement commission calculations
- Commission model that stores both admin_commission and vendor_earning values
The commission values in the test payload (flat: 7, percentage: 7) will be properly processed by this system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for commission calculation implementation
# Expected: Find files containing commission calculation logic
# Search for commission calculation logic
ast-grep --pattern 'function $_($$) {
$$$
admin_commission
$$$
}'
# Search for commission related tests
rg -l "commission.*calculation|calculate.*commission" --type=test
Length of output: 170
Script:
#!/bin/bash
# Let's try a more comprehensive search for commission-related logic
# Search for files containing commission calculation related terms
rg -i "admin.*commission|calculate.*commission|commission.*calculation" -g '!{*.lock,*.md}'
# Search for commission-related functions using AST
ast-grep --pattern 'function $name($$$) {
$$$
commission
$$$
}'
# Search specifically in test files with proper extension patterns
rg -i "commission" -g '*.test.*' -g '*.spec.*'
Length of output: 128886
Script:
#!/bin/bash
# Let's search for commission calculation implementation in PHP files
rg -i "calculate.*commission|commission.*calculation" -g "*.php" -A 5 -B 5
# Search for commission-related test assertions
rg -i "assert.*commission" -g "*.php"
# Search for commission-related class methods
ast-grep --pattern 'class $_ {
$$$
function $name($$$) {
$$$
commission
$$$
}
$$$
}'
Length of output: 34678
| @@ -9,7 +9,7 @@ test.describe('Product subscriptions test', () => { | |||
| let customer: VendorProductSubscriptionPage; | |||
| let vPage: Page, cPage: Page; | |||
| let apiUtils: ApiUtils; | |||
| let subscriptionId: string; | |||
| let subscriptionId: string = '1'; | |||
There was a problem hiding this comment.
Avoid hardcoding subscription ID.
Initializing subscriptionId with a hardcoded value ('1') creates a tight coupling to specific test data and may cause test failures if this ID doesn't exist.
Consider:
- Creating the subscription as part of the test setup
- Retrieving an existing subscription ID dynamically
- Using environment variables for test data
-let subscriptionId: string = '1';
+let subscriptionId: string;Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
tests/pw/pages/storeAppearancePage.ts (2)
75-80: Complete the TODO for banner size assertions.The method contains an incomplete TODO comment for additional assertions.
Would you like me to help implement additional assertions for banner size validation? For example:
- Verify banner dimensions in the UI
- Validate size constraints
- Check image scaling behavior
82-91: Complete the TODO for store sidebar assertions.The method contains an incomplete TODO comment for additional assertions.
Would you like me to help implement additional assertions for the store sidebar? For example:
- Verify sidebar position
- Check widget visibility
- Validate responsive behavior
tests/pw/utils/payloads.ts (1)
2254-2254: Consider using dynamic email generation instead of hardcoded valueReplace the hardcoded email with a dynamically generated one using faker or environment variables for better maintainability.
- email : '[email protected]', + email : faker.internet.email(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
tests/pw/fixtures/page.ts(3 hunks)tests/pw/pages/storeAppearancePage.ts(1 hunks)tests/pw/tests/e2e/privacyPolicy.spec.ts(2 hunks)tests/pw/tests/e2e/storeAppearance.spec.ts(1 hunks)tests/pw/utils/payloads.ts(22 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/pw/tests/e2e/privacyPolicy.spec.ts
🧰 Additional context used
🪛 Biome (1.9.4)
tests/pw/pages/storeAppearancePage.ts
[error] 11-13: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (6)
tests/pw/tests/e2e/storeAppearance.spec.ts (2)
21-25:
Uncomment and handle database setup operations.
The database setup operations are commented out, which might affect test reliability.
- // await dbUtils.updateOptionValue(dbData.dokanWidgets.names.storeLocation, dbData.dokanWidgets.values.storeLocationWidget);
- // await dbUtils.updateOptionValue(dbData.dokanWidgets.names.storeOpenClose, dbData.dokanWidgets.values.storeOpenCloseWidget);
- // await dbUtils.updateOptionValue(dbData.dokanWidgets.names.storeContactForm, dbData.dokanWidgets.values.storeContactFormWidget);
- // await dbUtils.updateOptionValue('sidebars_widgets', { 'sidebar-store': [dbData.dokanWidgets.widgets.storeLocation, dbData.dokanWidgets.widgets.storeOpenClose, dbData.dokanWidgets.widgets.storeContactForm] });
+ try {
+ await dbUtils.updateOptionValue(dbData.dokanWidgets.names.storeLocation, dbData.dokanWidgets.values.storeLocationWidget);
+ await dbUtils.updateOptionValue(dbData.dokanWidgets.names.storeOpenClose, dbData.dokanWidgets.values.storeOpenCloseWidget);
+ await dbUtils.updateOptionValue(dbData.dokanWidgets.names.storeContactForm, dbData.dokanWidgets.values.storeContactFormWidget);
+ await dbUtils.updateOptionValue('sidebars_widgets', { 'sidebar-store': [dbData.dokanWidgets.widgets.storeLocation, dbData.dokanWidgets.widgets.storeOpenClose, dbData.dokanWidgets.widgets.storeContactForm] });
+ } catch (error) {
+ console.error('Failed to setup test environment:', error);
+ throw error;
+ }Likely invalid or redundant comment.
90-91: Address the TODO comment for API issue.
There's a TODO comment about an API issue that needs to be resolved.
tests/pw/fixtures/page.ts (1)
44-44: LGTM!
The changes correctly integrate the new StoreAppearancePage class into the test fixtures.
Also applies to: 114-114, 311-311
tests/pw/utils/payloads.ts (3)
3763-3764: LGTM! Vendor role and notification settings are properly configured
The role and notification settings are correctly defined for vendor creation.
3807-3818: LGTM! Payment method configurations are well structured
The payment method configurations are properly defined with clear structure for both active and potential future payment methods.
4139-4193: LGTM! Store open/close settings are comprehensive
The store open/close configuration is well structured with:
- Complete coverage for all days of the week
- Properly formatted time settings
- Appropriate default notices
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
tests/pw/tests/e2e/storeAppearance.spec.ts (4)
22-25: Remove or document commented database operationsThese commented database operations seem important for the test setup. Either remove them if they're no longer needed or document why they're temporarily disabled.
36-36: Document reason for skipped testsSeveral test cases are marked with
test.skipwithout explanation. Please document why these tests are skipped and create tracking issues for enabling them.Also applies to: 43-43, 50-50
90-91: Address TODO comment and simplify API callThe TODO comment indicates a pending API issue fix, but the workaround uses spread operator with additional fields.
Consider simplifying the API call once the issue is fixed:
- // await apiUtils.updateStore(VENDOR_ID, { show_email: 'yes' }, payloads.adminAuth); // todo: apply this after api issue fix - await apiUtils.updateStore(VENDOR_ID, { ...payloads.storeResetFields, show_email: 'yes' }, payloads.adminAuth); + await apiUtils.updateStore(VENDOR_ID, { show_email: 'yes' }, payloads.adminAuth);
35-101: Consider refactoring test structure for better maintainabilityThe test suite uses multiple similar forEach loops for testing different features. This pattern could be refactored to be more maintainable and reduce code duplication.
Consider using a test case factory:
interface TestCase { feature: string; options: { [key: string]: any }; viewMethod: keyof StoreAppearancePage; values: string[]; } const testCases: TestCase[] = [ { feature: 'store map on store sidebar', options: { store_map: (status) => status === 'enable' ? 'on' : 'off' }, viewMethod: 'viewStoreMapOnStoreSidebar', values: ['enable', 'disable'] }, // Add other test cases... ]; testCases.forEach(({ feature, options, viewMethod, values }) => { values.forEach((value: string) => { test(`admin can ${value} ${feature}`, { tag: ['@lite', '@admin'] }, async () => { const optionValue = typeof options[Object.keys(options)[0]] === 'function' ? { [Object.keys(options)[0]]: options[Object.keys(options)[0]](value) } : options; await dbUtils.updateOptionValue(dbData.dokan.optionName.appearance, optionValue); await admin[viewMethod](value as any, data.predefined.vendorStores.vendor1); }); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/pw/tests/e2e/storeAppearance.spec.ts(1 hunks)
🔇 Additional comments (2)
tests/pw/tests/e2e/storeAppearance.spec.ts (2)
9-9:
Add environment variable validation
The VENDOR_ID environment variable is critical for the tests but lacks validation. Add a check to ensure it's defined before running the tests.
Add this validation at the beginning of the test suite:
const { VENDOR_ID } = process.env;
+if (!VENDOR_ID) {
+ throw new Error('VENDOR_ID environment variable is required for store appearance tests');
+}Likely invalid or redundant comment.
16-25: 🛠️ Refactor suggestion
Add error handling for setup operations
The test setup lacks error handling for critical operations like browser context creation and database updates.
Add proper error handling:
test.beforeAll(async ({ browser }) => {
+ try {
const adminContext = await browser.newContext(data.auth.adminAuth);
aPage = await adminContext.newPage();
admin = new StoreAppearancePage(aPage);
apiUtils = new ApiUtils(await request.newContext());
// Uncomment and handle these operations if needed
// await dbUtils.updateOptionValue(dbData.dokanWidgets.names.storeLocation, dbData.dokanWidgets.values.storeLocationWidget);
// await dbUtils.updateOptionValue(dbData.dokanWidgets.names.storeOpenClose, dbData.dokanWidgets.values.storeOpenCloseWidget);
// await dbUtils.updateOptionValue(dbData.dokanWidgets.names.storeContactForm, dbData.dokanWidgets.values.storeContactFormWidget);
// await dbUtils.updateOptionValue('sidebars_widgets', { 'sidebar-store': [dbData.dokanWidgets.widgets.storeLocation, dbData.dokanWidgets.widgets.storeOpenClose, dbData.dokanWidgets.widgets.storeContactForm] });
+ } catch (error) {
+ console.error('Failed to setup test environment:', error);
+ throw error;
+ }
});Likely invalid or redundant comment.
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
RECAPTCHA_SITE_KEYandRECAPTCHA_SECRET_KEY.Store Appearancemanagement with new functionalities for admins, including visibility controls for maps, contact forms, and vendor information.BasePageclass.MAPBOXfor managing Mapbox Maps API settings.StoreAppearancePageclass for improved store management interactions.Bug Fixes
LICENSE_KEYin configuration files.Documentation
Tests