refactor: enhance abuse reports and vendor reports functionality #2906
Conversation
- Improved error handling in the AbuseReportsPage to continue execution despite AJAX failures, providing fallback options for selecting filters and handling bulk actions. - Updated selectors in the selectors.ts file for better flexibility in identifying report elements. - Added comprehensive checks in VendorReportsPage for rendering, navigation, and data validation, including responsiveness and accessibility tests. - Introduced new methods for filtering and exporting reports, ensuring robust interaction with the analytics dashboard. - Adjusted test cases to use safer actions and improved logging for better traceability during test execution.
WalkthroughExpanded Playwright tests and selectors for reports and abuse flows. Added robust fallbacks in abuseReportsPage, broadened selectors, introduced a comprehensive VendorReportsPage with many methods, created a new vendor_reports.spec.ts suite, adjusted an abuse report spec to use a safer action, and extended testData with an analytics reports path. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test
participant P as AbuseReportsPage
participant S as Selectors
participant SRV as Server
T->>P: selectReason(reasonName)
P->>SRV: Try AJAX-based selection
alt AJAX success
SRV-->>P: Options loaded
P->>P: Select option by reason name
else AJAX fails
P->>S: Find by label/text fallback
alt Label match
P->>P: Select matched option
else No match
P->>P: Select first available option
end
end
P->>SRV: Apply filter (AJAX)
alt Apply fails
P->>P: Click non-AJAX fallback
end
P->>P: Wait for domcontentloaded
P->>P: Guarded results check
sequenceDiagram
autonumber
participant T as E2E Tests
participant V as VendorReportsPage
participant R as Reports UI
participant API as ApiUtils
T->>V: navigateToAnalyticsPage(type)
V->>R: Go to /analytics/{type}
V->>R: Validate heading/content
T->>V: testDateRangeFilter(type)
V->>R: Open picker, apply range
T->>V: verifyChartsDisplay(type)
V->>R: Probe chart selectors
T->>V: exportAnalyticsReport(type)
V->>R: Click export (multi-selector)
T->>V: validateReportsWithAPI(API)
V->>API: Fetch vendor-dashboard data
API-->>V: Response
V-->>T: Validation result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
CC: @mrabbani Please review and merge this PR for the review abuse reports test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
tests/pw/utils/testData.ts (1)
1184-1184: Add a semantic alias for vDashboard reports; keep reports_new for compatibilityreports_new is referenced at tests/pw/pages/vendorReportsPage.ts:18 — add a clearer alias in tests/pw/utils/testData.ts (around line 1184) and retain reports_new to avoid breaking tests.
- reports_new: 'dashboard/reports/?path=%2Fanalytics%2Fproducts', + // Prefer semantic name; keep `reports_new` as a temporary alias. + reportsAnalyticsProducts: 'dashboard/reports/?path=%2Fanalytics%2Fproducts', + reports_new: 'dashboard/reports/?path=%2Fanalytics%2Fproducts',Optional more flexible alternative:
- reports_new: 'dashboard/reports/?path=%2Fanalytics%2Fproducts', + // Generic builder for analytics sub-sections (e.g., 'products', 'overview', 'orders'). + reportsAnalyticsPath: (subPath: string = 'products') => `dashboard/reports/?path=${encodeURIComponent(`/analytics/${subPath}`)}`,tests/pw/pages/selectors.ts (3)
4706-4706: Broader H1 XPath is okay; prefer role-based, exact-name heading for stability.Current fallbacks like //div[contains(@Class,"reports")]//h1 are generic and risk false positives. Using Playwright’s role selector tightens intent and supports i18n/case-variance.
Consider:
- reportsText: '//h1[normalize-space()="Reports"] | //h1[normalize-space()="Analytics"] | //div[contains(@class, "reports")]//h1 | //div[contains(@class, "analytics")]//h1', + reportsText: 'role=heading[level=1][name=/^(Reports|Analytics)$/i]',Please verify this still matches in both classic “Reports” and new “Analytics” views across themes/builds.
4757-4757: Order the export triggers from most specific to least generic.Leading with .dokan-right may match non-export UI. Prefer text/role-first, then class fallbacks.
- exportStatements: '.dokan-right, button:has-text("Export"), a:has-text("Export"), .export-button', + exportStatements: 'button:has-text("Export"), a:has-text("Export"), .export-button, .dokan-right',If possible in code using these selectors, migrate to role=button[name=/^export$/i] for best resilience.
7398-7401: Tighten “Report” triggers to avoid catching “Reports” navigation; simplify reason selection.a:has-text("Report") and button:has-text("Report") can match unrelated “Reports” links. Also, the reason XPath assumes DOM proximity. Prefer role-based name matching and label association.
- reportAbuse: 'a.dokan-report-abuse-button, button.dokan-report-abuse-button, .report-abuse-button, a:has-text("Report"), button:has-text("Report")', + reportAbuse: 'a.dokan-report-abuse-button, button.dokan-report-abuse-button, .report-abuse-button, role=button[name=/^report$/i], role=link[name=/^report$/i]',- reportReasonByName: (reasonName: string) => `//input[@value='${reasonName}']/.. | //label[contains(text(), '${reasonName}')]/input/.. | //div[contains(text(), '${reasonName}')]/input/..`, + reportReasonByName: (reasonName: string) => `role=checkbox[name=${reasonName}]`,If role selector isn’t feasible everywhere, fallback to Playwright’s label association in code (e.g., page.getByLabel(reasonName)) rather than DOM-relative XPath. Please run the affected abuseReportsPage flows to confirm no accidental matches on “Reports” menus.
tests/pw/tests/e2e/vendor_reports.spec.ts (2)
29-68: Consider consolidating repetitive navigation testsLines 29-68 contain 10 nearly identical tests that only differ in the page parameter. This creates maintenance overhead.
Consider using a data-driven approach:
const analyticsPages = [ 'products', 'revenue', 'orders', 'variations', 'categories', 'coupons', 'taxes', 'stock', 'customers', 'downloads' ]; analyticsPages.forEach(page => { test(`vendor can navigate to analytics ${page} page`, { tag: ['@lite', '@vendor'] }, async () => { await vendor.navigateToAnalyticsPage(page); } ); });
174-176: Performance test timeout may be too generousThe test expects page load within 10 seconds, which is quite lenient for a performance test. Modern web applications should typically load much faster.
Consider stricter performance thresholds:
- // Expect page to load within 10 seconds + // Expect page to load within 3 seconds for optimal UXtests/pw/pages/vendorReportsPage.ts (5)
17-49: Consider extracting selectors to avoid duplicationThe method uses multiple hard-coded selector strings that could be moved to the selectors file for better maintainability.
Consider moving these selectors to
selectors.ts:- const hasContent = await this.isVisible('h1, h2, h3, .page-title, .dashboard-content, .reports-content'); + const hasContent = await this.isVisible(vendorReports.contentSelectors.headingsAndContent);
359-359: Hard-coded status value may fail in non-English environmentsThe hard-coded 'completed' status might not work in localized environments.
Consider making this configurable:
- await this.page.selectOption(selector, 'completed'); + await this.selectByValue(selector, data.orderStatus?.completed || 'completed');
436-446: Avoid silent exception handling in loopsThe empty catch blocks in the loop could hide legitimate errors.
At minimum, add debug logging:
} catch (error) { - // Continue to next selector + console.debug(`Selector ${selector} not found: ${error.message}`); continue; }
514-515: Keyboard navigation test could be more comprehensiveThe current implementation only tests a single Tab press. Consider testing complete keyboard navigation flow.
// Check for keyboard navigation await this.page.keyboard.press('Tab'); const focusedElement = await this.page.evaluate(() => document.activeElement?.tagName); console.log(`Keyboard navigation test: focused element type is ${focusedElement}`); + + // Test reverse navigation + await this.page.keyboard.press('Shift+Tab'); + const previousElement = await this.page.evaluate(() => document.activeElement?.tagName); + console.log(`Reverse navigation: focused element type is ${previousElement}`);
485-488: Endpoint confirmed — replace hardcoded path with API constant'/dokan/v1/vendor-dashboard/sales' is defined at tests/pw/utils/apiEndPoints.ts:271 (getVendorSalesReport) and referenced in tests/pw/tests/api/vendorDashborad.spec.ts; replace the hardcoded string in tests/pw/pages/vendorReportsPage.ts:485 with apiEndPoints.getVendorSalesReport to avoid drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/pw/pages/abuseReportsPage.ts(1 hunks)tests/pw/pages/selectors.ts(3 hunks)tests/pw/pages/vendorReportsPage.ts(2 hunks)tests/pw/tests/e2e/abuseReports.spec.ts(1 hunks)tests/pw/tests/e2e/vendor_reports.spec.ts(1 hunks)tests/pw/utils/testData.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/pw/tests/e2e/vendor_reports.spec.ts (3)
tests/pw/pages/vendorReportsPage.ts (1)
VendorReportsPage(10-518)tests/pw/utils/apiUtils.ts (1)
ApiUtils(9-2486)tests/pw/utils/testData.ts (1)
data(63-2915)
tests/pw/pages/abuseReportsPage.ts (3)
tests/pw/utils/testData.ts (1)
data(63-2915)tests/pw/utils/interfaces.ts (1)
product(45-468)tests/pw/utils/helpers.ts (1)
helpers(7-554)
tests/pw/pages/vendorReportsPage.ts (2)
tests/pw/pages/selectors.ts (1)
selector(3-8231)tests/pw/utils/apiUtils.ts (1)
ApiUtils(9-2486)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
🔇 Additional comments (4)
tests/pw/tests/e2e/abuseReports.spec.ts (1)
64-67: Good safety improvement for bulk actionsSwitching from 'delete' to 'mark-as-read' is a prudent change that prevents destructive operations during testing.
tests/pw/pages/abuseReportsPage.ts (3)
87-98: Excellent resilience pattern for filter selectionThe multi-level fallback approach (AJAX → label selection → first option) ensures test continuity across different environment configurations.
169-176: Smart safeguard against destructive bulk operationsThe change to select only the first checkbox instead of all prevents accidentally deleting all abuse reports from the database - a critical improvement for test safety.
223-236: Good defensive programming for report reason selectionThe fallback to select any available reason when the specific one isn't found ensures test continuity while still logging the issue.
| await this.page.selectOption(selector, { index: 1 }); | ||
| await this.waitForLoadState('networkidle'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential issue with selectOption API usage
Using this.page.selectOption() directly breaks the abstraction pattern used elsewhere in the class, which typically uses helper methods.
Use the existing helper method pattern:
- await this.page.selectOption(selector, { index: 1 });
+ await this.selectByNumber(selector, 1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await this.page.selectOption(selector, { index: 1 }); | |
| await this.waitForLoadState('networkidle'); | |
| await this.selectByNumber(selector, 1); | |
| await this.waitForLoadState('networkidle'); |
🤖 Prompt for AI Agents
In tests/pw/pages/vendorReportsPage.ts around lines 336-337, the code calls
this.page.selectOption(selector, { index: 1 }) which breaks the class
abstraction; replace that direct page call with the class helper used elsewhere
(e.g., this.selectOption(selector, { index: 1 })) so the page interaction
follows the same helper pattern, then keep the await
this.waitForLoadState('networkidle') call after it.
| expect(loadTime).toBeLessThan(10000); | ||
| console.log(`${report} analytics page loaded in ${loadTime}ms`); |
There was a problem hiding this comment.
Assertion in page object violates separation of concerns
Using expect() directly in a page object method breaks the pattern where assertions should be in test files.
Move the assertion to the test file:
- // Expect page to load within 10 seconds
- expect(loadTime).toBeLessThan(10000);
+ // Return load time for assertion in test
+ return { report, loadTime };Then in the test file:
const results = await vendor.testPageLoadPerformance();
results.forEach(({ report, loadTime }) => {
expect(loadTime).toBeLessThan(10000);
});| import { test, request, Page } from '@playwright/test'; | ||
| import { VendorReportsPage } from '@pages/vendorReportsPage'; | ||
| import { ApiUtils } from '@utils/apiUtils'; | ||
| import { data } from '@utils/testData'; | ||
|
|
||
| test.describe('Vendor Reports test', () => { | ||
| let vendor: VendorReportsPage; | ||
| let vPage: Page; | ||
| let apiUtils: ApiUtils; | ||
|
|
||
| test.beforeAll(async ({ browser }) => { | ||
| const vendorContext = await browser.newContext(data.auth.vendorAuth); | ||
| vPage = await vendorContext.newPage(); | ||
| vendor = new VendorReportsPage(vPage); | ||
|
|
||
| apiUtils = new ApiUtils(await request.newContext()); | ||
| }); | ||
|
|
||
| test.afterAll(async () => { | ||
| await vPage.close(); | ||
| await apiUtils.dispose(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding environment setup/teardown for test data
The test suite doesn't appear to have any setup for ensuring test data exists (products, orders, etc.) before running analytics/reports tests. This could lead to unreliable tests depending on the state of the test environment.
Consider adding test data setup in beforeAll:
test.beforeAll(async ({ browser }) => {
const vendorContext = await browser.newContext(data.auth.vendorAuth);
vPage = await vendorContext.newPage();
vendor = new VendorReportsPage(vPage);
apiUtils = new ApiUtils(await request.newContext());
+
+ // Ensure test data exists for reliable report validation
+ // Example: Create sample products, orders, etc. if needed
+ // await apiUtils.createProduct(...);
+ // await apiUtils.createOrder(...);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { test, request, Page } from '@playwright/test'; | |
| import { VendorReportsPage } from '@pages/vendorReportsPage'; | |
| import { ApiUtils } from '@utils/apiUtils'; | |
| import { data } from '@utils/testData'; | |
| test.describe('Vendor Reports test', () => { | |
| let vendor: VendorReportsPage; | |
| let vPage: Page; | |
| let apiUtils: ApiUtils; | |
| test.beforeAll(async ({ browser }) => { | |
| const vendorContext = await browser.newContext(data.auth.vendorAuth); | |
| vPage = await vendorContext.newPage(); | |
| vendor = new VendorReportsPage(vPage); | |
| apiUtils = new ApiUtils(await request.newContext()); | |
| }); | |
| test.afterAll(async () => { | |
| await vPage.close(); | |
| await apiUtils.dispose(); | |
| }); | |
| import { test, request, Page } from '@playwright/test'; | |
| import { VendorReportsPage } from '@pages/vendorReportsPage'; | |
| import { ApiUtils } from '@utils/apiUtils'; | |
| import { data } from '@utils/testData'; | |
| test.describe('Vendor Reports test', () => { | |
| let vendor: VendorReportsPage; | |
| let vPage: Page; | |
| let apiUtils: ApiUtils; | |
| test.beforeAll(async ({ browser }) => { | |
| const vendorContext = await browser.newContext(data.auth.vendorAuth); | |
| vPage = await vendorContext.newPage(); | |
| vendor = new VendorReportsPage(vPage); | |
| apiUtils = new ApiUtils(await request.newContext()); | |
| // Ensure test data exists for reliable report validation | |
| // Example: Create sample products, orders, etc. if needed | |
| // await apiUtils.createProduct(...); | |
| // await apiUtils.createOrder(...); | |
| }); | |
| test.afterAll(async () => { | |
| await vPage.close(); | |
| await apiUtils.dispose(); | |
| }); |
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/vendor_reports.spec.ts around lines 1 to 22, the suite
lacks deterministic test-data setup for products/orders needed by vendor
analytics which can make tests flaky; add API-driven setup in beforeAll using
the existing ApiUtils to create the minimal entities (product(s), order(s), any
required vendor associations) with unique identifiers and persist their IDs to
variables on the test scope, and add corresponding teardown in afterAll to
delete those entities (or mark them test-only) so tests are isolated and
repeatable; ensure setup awaits API responses and handles already-existing
resources idempotently (check by unique name/timestamp) so the reports pages
always have predictable data when tests run.
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