refactor: store support modules test cases for old ui #2919
Conversation
…icket handling - Updated the `toBeVisible` method to use a locator's waitFor for improved visibility checks. - Added a new `formatOrderId` method to format order IDs with commas. - Modified `viewOrderReferenceNumberOnSupportTicket` to utilize the formatted order ID for visibility assertions. - Adjusted selectors for better specificity in the `selectors.ts` file.
- Added descriptive test steps to improve clarity and organization in the store support test suite. - Refactored test cases for admin, customer, and vendor interactions with support tickets to include step descriptions. - Improved overall readability and maintainability of the test code by grouping related actions within test steps.
WalkthroughBasePage.toBeVisible now performs an explicit locator.waitFor(state: 'visible') with a default 15s timeout; many selectors were adjusted and new "newMenus" and date/customer filter selectors added; StoreSupportsPage gains formatOrderId and uses formatted IDs when checking order references; e2e tests were wrapped with test.step blocks; a frontend route path was changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test
participant BasePage as BasePage
participant Locator as Playwright Locator
Test->>BasePage: toBeVisible(selector, options?)
BasePage->>Locator: locator(selector)
BasePage->>Locator: waitFor(state: "visible", timeout: options?.timeout ?? 15000)
Note right of Locator #DDEFEF: explicit wait replaces expect(...).toBeVisible
Locator-->>BasePage: resolve / reject
BasePage-->>Test: return / throw
sequenceDiagram
autonumber
participant Test as Test
participant StorePage as StoreSupportsPage
participant Selectors as selectors.ts
participant BasePage as BasePage
participant Page as DOM
Test->>StorePage: viewOrderReferenceNumberOnSupportTicket(ticketId, orderId)
StorePage->>StorePage: formatOrderId(orderId) -> formattedOrderId
StorePage->>Selectors: build selector for orderReferenceText(formattedOrderId)
StorePage->>BasePage: toBeVisible(spanSelector)
BasePage->>Page: locator.waitFor(state: "visible")
Page-->>BasePage: visible
BasePage-->>StorePage: done
StorePage->>Selectors: build selector for orderReferenceLink(formattedOrderId)
StorePage->>BasePage: toBeVisible(linkSelector)
BasePage->>Page: locator.waitFor(state: "visible")
Page-->>BasePage: visible
BasePage-->>StorePage: done
StorePage-->>Test: done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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 (9)
tests/pw/pages/basePage.ts (1)
1521-1524: Switch to explicit wait is fine; trim unused option and align typing.This keeps behavior but drops Playwright’s assertion messaging. If that’s desired, OK. The options type includes visible?: boolean which isn’t used here and isn’t a valid expect option—simplify to timeout only.
- async toBeVisible(selector: string, options?: { timeout?: number; visible?: boolean }) { + async toBeVisible(selector: string, options?: { timeout?: number }) { const locator = this.page.locator(selector); await locator.waitFor({ state: 'visible', timeout: options?.timeout ?? 15000 }); }Tip: if you want assertion breadcrumbs in traces, consider expect(locator).toBeVisible({ timeout: ... }) instead of waitFor, or wrap waitFor in toPass for consistent reporting.
tests/pw/pages/selectors.ts (2)
893-893: Scope is good; prefer pure CSS/locator chaining over legacy>>.
p.search-box >> input#post-search-inputworks, but>>is mainly for mixing selector engines. Use a descendant CSS or locator chaining for consistency and future-proofing.Apply this diff:
- searchTicket: 'p.search-box >> input#post-search-input', + searchTicket: 'p.search-box input#post-search-input',
7161-7164: Scope selectors to exact-match to avoid order-ID prefix collisions.Call sites already pass a formatted ID (tests/pw/pages/storeSupportsPage.ts:478–491 — formatOrderId returns Number(...).toLocaleString()); update selectors in tests/pw/pages/selectors.ts:7161–7164.
- orderReferenceText: (orderId: string) => - `//strong[contains(normalize-space(), "Referenced Order #${orderId}")]`, - orderReferenceLink: (orderId: string) => - `//strong[contains(normalize-space(), "Referenced Order #${orderId}")]/..`, + orderReferenceText: (orderId: string) => + `//span[contains(@class,"order-reference")]//strong[normalize-space()=concat("Referenced Order #", "${orderId}")]`, + orderReferenceLink: (orderId: string) => + `//span[contains(@class,"order-reference")]//a[./strong[normalize-space()=concat("Referenced Order #", "${orderId}")]]`,If the UI appends extra text, anchor the link by a stable attribute (e.g., href containing
/view-order/${orderId}) instead.tests/pw/pages/storeSupportsPage.ts (3)
478-480: Avoid environment-dependent number formatting; enforce integer formatting and validate input.
toLocaleString()without an explicit locale can render different separators across CI/hosts and may include fractional parts if a string like "123.0" is passed, causing flaky selectors.Apply this diff:
- formatOrderId(orderId: number | string): string { - return Number(orderId).toLocaleString(); // adds commas automatically - } + formatOrderId(orderId: number | string, locale = 'en-US'): string { + const n = Number.parseInt(String(orderId), 10); + if (Number.isNaN(n)) { + throw new Error(`formatOrderId: invalid orderId "${orderId}"`); + } + return new Intl.NumberFormat(locale, { useGrouping: true, maximumFractionDigits: 0 }).format(n); + }
477-477: Fix misleading comment placement.The comment says “customer cant send message…” but the following methods are unrelated helpers/view assertions.
Apply this diff:
- // customer cant send message to closed support ticket + // helpers for order reference rendering
482-492: Update order-reference selectors to tolerate NBSP and locale grouping separatorsTwo XPath selectors in tests/pw/pages/selectors.ts use normalize-space()/contains (around lines 6063 and 7162) and will fail when order numbers include non‑breaking spaces (U+00A0/U+202F) or locale grouping separators. Make the match tolerant.
- Preferred: strip separators from the numeric part in XPath using substring-after() + translate(). Example (replace ${DIGITS_ONLY} with the digits-only form of the expected orderId):
//strong[ starts-with(normalize-space(.), "Referenced Order #") and translate(substring-after(normalize-space(.), "#"), ' \u00A0\u202F,.' , '') = '${DIGITS_ONLY}' ]
- Alternative: use Playwright locator hasText with a regex that allows digits + separators, e.g. new RegExp('Referenced Order #\s*[\d\s,\.\u00A0\u202F]+') and build the regex from the expected orderId digits.
tests/pw/pages/selectors.ts — occurrences ~lines 6063 and 7162.
tests/pw/tests/e2e/storeSupports.spec.ts (3)
40-42: Step title nit: clarify scope.The step says “for vendor” but
enableStoreSupportModuleverifies admin, vendor, and customer surfaces. Rename for accuracy.Apply this diff:
- await test.step('Enable store support module for vendor', async () => { + await test.step('Enable store support module (end-to-end surfaces)', async () => {
130-134: Shadowed variable name reduces readability.
const [, supportTicketId]shadows the outersupportTicketId. Use a distinct name.Apply this diff:
- const [, supportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, author: CUSTOMER_ID, meta: { store_id: VENDOR_ID } }); - await admin.storeSupportBulkAction('close', supportTicketId); + const [, newSupportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, author: CUSTOMER_ID, meta: { store_id: VENDOR_ID } }); + await admin.storeSupportBulkAction('close', newSupportTicketId);
146-159: Sanity-check required env IDs to fail fast.If
VENDOR_ID,CUSTOMER_ID, orPRODUCT_IDare unset, API calls create malformed data.Add a guard at the top of the suite:
test.describe('Store Support test (customer)', () => { + if (!VENDOR_ID || !CUSTOMER_ID || !PRODUCT_ID) { + test.fail(true, 'Required env vars VENDOR_ID, CUSTOMER_ID, PRODUCT_ID are missing'); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/pw/pages/basePage.ts(1 hunks)tests/pw/pages/selectors.ts(3 hunks)tests/pw/pages/storeSupportsPage.ts(1 hunks)tests/pw/tests/e2e/storeSupports.spec.ts(3 hunks)tests/pw/utils/testData.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/pw/tests/e2e/storeSupports.spec.ts (3)
tests/pw/utils/payloads.ts (1)
payloads(9-5094)tests/pw/utils/testData.ts (2)
admin(61-61)data(63-2915)tests/pw/pages/storeSupportsPage.ts (1)
StoreSupportsPage(15-510)
⏰ 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: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (3)
tests/pw/utils/testData.ts (1)
1159-1161: Encode orderReceivedDetails params & confirm route change
- Replace with encoded params:
- orderReceivedDetails: (orderId: string, orderKey: string) => `classic-checkout/order-received/${orderId}/?key=${orderKey}`, + orderReceivedDetails: (orderId: string, orderKey: string) => `classic-checkout/order-received/${encodeURIComponent(orderId)}/?key=${encodeURIComponent(orderKey)}`,
- Confirm the switch to "classic-checkout/order-received" is intentional and consistent across callers (check data.subUrls.frontend.orderReceived and other references to "order-received"). See tests/pw/utils/testData.ts (~Lines 1129, 1159–1161) and search the codebase for additional usages.
tests/pw/pages/selectors.ts (1)
3075-3080: Header locators via:has-text()— LGTM.This anchors headers by visible labels and removes reliance on brittle classes/structure.
tests/pw/tests/e2e/storeSupports.spec.ts (1)
211-215: Order reference check relies on locale; confirm selectors tolerate grouping differences.Given the formatter now groups digits, ensure the selector for order reference matches with commas/dots/NBSP as applicable.
Run the same selector inspection suggested earlier to verify it’s regex/locale-agnostic.
| await test.step('Deactivate store support module and cleanup', async () => { | ||
| await apiUtils.activateModules(payloads.moduleIds.storeSupport, payloads.adminAuth); | ||
| await aPage.close(); | ||
| await apiUtils.dispose(); | ||
| }); |
There was a problem hiding this comment.
Cleanup step calls activate instead of deactivate.
Label says “Deactivate…”, but activateModules is called. This leaves the module enabled after the suite.
Apply this diff:
- await test.step('Deactivate store support module and cleanup', async () => {
- await apiUtils.activateModules(payloads.moduleIds.storeSupport, payloads.adminAuth);
+ await test.step('Deactivate store support module and cleanup', async () => {
+ await apiUtils.deactivateModules(payloads.moduleIds.storeSupport, payloads.adminAuth);
await aPage.close();
await apiUtils.dispose();
});📝 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 test.step('Deactivate store support module and cleanup', async () => { | |
| await apiUtils.activateModules(payloads.moduleIds.storeSupport, payloads.adminAuth); | |
| await aPage.close(); | |
| await apiUtils.dispose(); | |
| }); | |
| await test.step('Deactivate store support module and cleanup', async () => { | |
| await apiUtils.deactivateModules(payloads.moduleIds.storeSupport, payloads.adminAuth); | |
| await aPage.close(); | |
| await apiUtils.dispose(); | |
| }); |
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/storeSupports.spec.ts around lines 32 to 36, the cleanup
step is mislabeled as "Deactivate..." but calls apiUtils.activateModules; change
this to call apiUtils.deactivateModules with the same arguments
(payloads.moduleIds.storeSupport, payloads.adminAuth) so the store support
module is actually disabled during cleanup, keeping the aPage.close() and
apiUtils.dispose() calls as-is.
| test('admin can reply to support ticket as vendor', { tag: ['@pro', '@admin'] }, async () => { | ||
| await admin.replySupportTicket(supportTicketId, data.storeSupport.chatReply.asVendor); | ||
| await test.step('Reply to support ticket as vendor', async () => { | ||
| await admin.replySupportTicket(supportTicketId, data.storeSupport.chatReply.asVendor); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Reply-as-vendor test never selects vendor author.
replySupportTicket defaults replier = 'admin'. The test doesn’t pass 'vendor', so it replies as admin.
Apply this diff:
- await admin.replySupportTicket(supportTicketId, data.storeSupport.chatReply.asVendor);
+ await admin.replySupportTicket(supportTicketId, data.storeSupport.chatReply.asVendor, 'vendor');📝 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.
| test('admin can reply to support ticket as vendor', { tag: ['@pro', '@admin'] }, async () => { | |
| await admin.replySupportTicket(supportTicketId, data.storeSupport.chatReply.asVendor); | |
| await test.step('Reply to support ticket as vendor', async () => { | |
| await admin.replySupportTicket(supportTicketId, data.storeSupport.chatReply.asVendor); | |
| }); | |
| }); | |
| test('admin can reply to support ticket as vendor', { tag: ['@pro', '@admin'] }, async () => { | |
| await test.step('Reply to support ticket as vendor', async () => { | |
| await admin.replySupportTicket(supportTicketId, data.storeSupport.chatReply.asVendor, 'vendor'); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/storeSupports.spec.ts around lines 93 to 97, the test
titled "admin can reply to support ticket as vendor" calls replySupportTicket
but doesn't pass the replier argument so it uses the default 'admin'; update the
call to pass 'vendor' as the replier (e.g.,
admin.replySupportTicket(supportTicketId, data.storeSupport.chatReply.asVendor,
'vendor')) so the test actually posts the reply as the vendor.
| await test.step('Deactivate store support module and cleanup', async () => { | ||
| await apiUtils.activateModules(payloads.moduleIds.storeSupport, payloads.adminAuth); | ||
| await vPage.close(); | ||
| await apiUtils.dispose(); | ||
| }); |
There was a problem hiding this comment.
Vendor suite cleanup also calls activate instead of deactivate and leaks the admin page.
Two issues:
- Should deactivate per label.
aPageisn’t closed here.
Apply this diff:
- await test.step('Deactivate store support module and cleanup', async () => {
- await apiUtils.activateModules(payloads.moduleIds.storeSupport, payloads.adminAuth);
- await vPage.close();
- await apiUtils.dispose();
- });
+ await test.step('Deactivate store support module and cleanup', async () => {
+ await apiUtils.deactivateModules(payloads.moduleIds.storeSupport, payloads.adminAuth);
+ await vPage.close();
+ await aPage.close();
+ await apiUtils.dispose();
+ });📝 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 test.step('Deactivate store support module and cleanup', async () => { | |
| await apiUtils.activateModules(payloads.moduleIds.storeSupport, payloads.adminAuth); | |
| await vPage.close(); | |
| await apiUtils.dispose(); | |
| }); | |
| await test.step('Deactivate store support module and cleanup', async () => { | |
| await apiUtils.deactivateModules(payloads.moduleIds.storeSupport, payloads.adminAuth); | |
| await vPage.close(); | |
| await aPage.close(); | |
| await apiUtils.dispose(); | |
| }); |
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/storeSupports.spec.ts around lines 263 to 267, the cleanup
step incorrectly calls activateModules instead of deactivating the store support
module and fails to close the admin page; change the call to the corresponding
deactivate (e.g., apiUtils.deactivateModules or
apiUtils.deactivateModuleByLabel) using payloads.moduleIds.storeSupport and
payloads.adminAuth, and ensure you close the admin page (aPage.close()) in this
step before disposing apiUtils and closing vPage so no pages are leaked.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/pw/tests/e2e/storeSupports.spec.ts (1)
198-203: Minor naming clarity: avoid shadowing type name.Variable responseBody has the same name as its type; rename for clarity.
- let responseBody: responseBody; + let orderResponse: responseBody; @@ - const orderKey = responseBody.order_key; - await customer.storeSupport(orderId + ',' + orderKey, data.customer.getSupport, 'order-received'); + const orderKey = orderResponse.order_key; + await customer.storeSupport(`${orderId},${orderKey}`, data.customer.getSupport, 'order-received');tests/pw/pages/selectors.ts (3)
893-894: Avoid brittle [1] index; anchor to container (simpler and more stable).Using [1] makes this fragile if another search box appears. Anchor to the search-box container with a CSS selector instead.
- searchTicket: '(//p[contains(@class, "search-box")]/input[@id="post-search-input"])[1]', + searchTicket: 'p.search-box #post-search-input',
3075-3080: CSS :has-text usage looks good; confirm Playwright version supports it.These rely on Playwright’s CSS engine extension (:has-text). If your CI uses an older Playwright, fall back to XPath equivalents.
Fallback (only if needed):
- itemColumn: '#dokan_commission_box th:has-text("Item")', + itemColumn: '//div[@id="dokan_commission_box"]//th[normalize-space()="Item"]', - typeColumn: '#dokan_commission_box th:has-text("Type")', + typeColumn: '//div[@id="dokan_commission_box"]//th[normalize-space()="Type"]', - rateColumn: '#dokan_commission_box th:has-text("Rate")', + rateColumn: '//div[@id="dokan_commission_box"]//th[normalize-space()="Rate"]', - qtyColumn: '#dokan_commission_box th:has-text("Qty")', + qtyColumn: '//div[@id="dokan_commission_box"]//th[normalize-space()="Qty"]', - commissionColumn: '#dokan_commission_box th:has-text("Commission")', + commissionColumn: '//div[@id="dokan_commission_box"]//th[normalize-space()="Commission"]',Also applies to: 3087-3087
7161-7165: Make order reference selectors consistent across vendor and customer.Customer uses contains(normalize-space(), …) while vendor (Line 6062-6065) still uses exact match. Align both to reduce flakiness from whitespace/label changes.
Suggested vendor update (outside this hunk):
// vendor.vSupport.supportTicketDetails.orderReference orderReferenceText: (orderId: string) => `//strong[contains(normalize-space(), "Referenced Order #${orderId}")]`, orderReferenceLink: (orderId: string) => `//strong[contains(normalize-space(), "Referenced Order #${orderId}")]/..`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/pw/pages/selectors.ts(4 hunks)tests/pw/tests/e2e/storeSupports.spec.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/pw/tests/e2e/storeSupports.spec.ts (2)
tests/pw/utils/payloads.ts (1)
payloads(9-5094)tests/pw/pages/storeSupportsPage.ts (1)
StoreSupportsPage(15-510)
⏰ 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: e2e tests (1, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (5)
tests/pw/tests/e2e/storeSupports.spec.ts (5)
32-36: Cleanup step calls activate instead of deactivate.Label says “Deactivate…”, but activateModules is called; module remains enabled after the suite.
Apply this diff:
- await test.step('Deactivate store support module and cleanup', async () => { - await apiUtils.activateModules(payloads.moduleIds.storeSupport, payloads.adminAuth); + await test.step('Deactivate store support module and cleanup', async () => { + await apiUtils.deactivateModules(payloads.moduleIds.storeSupport, payloads.adminAuth); await aPage.close(); await apiUtils.dispose(); });
93-97: Reply-as-vendor test posts as admin due to missing arg.replySupportTicket defaults to 'admin'; pass 'vendor' to exercise the vendor path.
Apply this diff:
- await admin.replySupportTicket(supportTicketId, data.storeSupport.chatReply.asVendor); + await admin.replySupportTicket(supportTicketId, data.storeSupport.chatReply.asVendor, 'vendor');
263-267: Vendor suite cleanup: activates instead of deactivating, and leaks the admin page.
- Should deactivate per label.
- Close both vPage and aPage to avoid page leaks.
Apply this diff:
- await test.step('Deactivate store support module and cleanup', async () => { - await apiUtils.activateModules(payloads.moduleIds.storeSupport, payloads.adminAuth); - await vPage.close(); - await apiUtils.dispose(); - }); + await test.step('Deactivate store support module and cleanup', async () => { + await apiUtils.deactivateModules(payloads.moduleIds.storeSupport, payloads.adminAuth); + await vPage.close(); + await aPage.close(); + await apiUtils.dispose(); + });
70-72: No changes required.data.storeSupport.titleandpayloads.createSupportTicket.titleboth equal"test support ticket".
199-203: storeSupport handles ‘order-received’ format correctly
Theorder-receivedcase splits the input on comma intoorderIdandorderKeybefore callingorderReceivedDetails, so no change is needed.
| ticketId: '(//div[contains(@class, "dokan-summary-info") and contains(@class, "ticket-id")])[1]', | ||
| vendorInfo: '(//div[contains(@class, "dokan-summary-info") and contains(@class, "dokan-vendor-info")])[1]', |
There was a problem hiding this comment.
Stabilize ticket summary locators; drop [1] and scope to the summary container.
Indexing [1] is order‑dependent and flaky. Scope by parent to make it deterministic.
- ticketId: '(//div[contains(@class, "dokan-summary-info") and contains(@class, "ticket-id")])[1]',
- vendorInfo: '(//div[contains(@class, "dokan-summary-info") and contains(@class, "dokan-vendor-info")])[1]',
+ ticketId: 'div.dokan-chat-summary-holder .dokan-summary-info.ticket-id',
+ vendorInfo: 'div.dokan-chat-summary-holder .dokan-summary-info.dokan-vendor-info',📝 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.
| ticketId: '(//div[contains(@class, "dokan-summary-info") and contains(@class, "ticket-id")])[1]', | |
| vendorInfo: '(//div[contains(@class, "dokan-summary-info") and contains(@class, "dokan-vendor-info")])[1]', | |
| ticketId: 'div.dokan-chat-summary-holder .dokan-summary-info.ticket-id', | |
| vendorInfo: 'div.dokan-chat-summary-holder .dokan-summary-info.dokan-vendor-info', |
🤖 Prompt for AI Agents
In tests/pw/pages/selectors.ts around lines 930-931, the ticket summary locators
use an index ([1]) which is order-dependent and flaky; remove the trailing [1]
and instead scope each locator to the summary container selector (find the
summary container element once and build ticketId and vendorInfo as children of
that container) so the locators become deterministic and reference the summary
parent rather than relying on global indexing.
| await test.step('Create support ticket for testing', async () => { | ||
| [, supportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, author: CUSTOMER_ID, meta: { store_id: VENDOR_ID } }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Harden ticket creation against missing/typed env IDs.
process.env values may be undefined and are strings; pass numeric IDs with sane fallbacks to prevent API errors.
Apply this pattern (adapt per call site):
- [, supportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, author: CUSTOMER_ID, meta: { store_id: VENDOR_ID } });
+ const authorId = Number(CUSTOMER_ID ?? payloads.createSupportTicket.author);
+ const storeId = Number(VENDOR_ID ?? payloads.createSupportTicket.meta.store_id);
+ [, supportTicketId] = await apiUtils.createSupportTicket({
+ ...payloads.createSupportTicket,
+ author: authorId,
+ meta: { store_id: storeId }
+ });Repeat for other createSupportTicket calls in this file (including those with order_id).
Run to quickly spot any remaining unsafe usages:
Also applies to: 106-109, 131-133, 224-227, 256-259, 327-329, 333-336
🏁 Script executed:
#!/bin/bash
rg -nC1 "createSupportTicket\(" tests/pw/tests/e2e/storeSupports.spec.tsLength of output: 3546
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' tests/pw/tests/e2e/storeSupports.spec.tsLength of output: 9593
Ensure numeric env IDs in createSupportTicket calls
Coerce process.env.VENDOR_ID and process.env.CUSTOMER_ID to numbers with a fallback to the payload defaults (and convert orderId for calls that include it) to avoid passing undefined or string values into apiUtils.createSupportTicket. Apply at every invocation in tests/pw/tests/e2e/storeSupports.spec.ts:
- [, supportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, author: CUSTOMER_ID, meta: { store_id: VENDOR_ID } });
+ const authorId = Number(CUSTOMER_ID ?? payloads.createSupportTicket.author);
+ const storeId = Number(VENDOR_ID ?? payloads.createSupportTicket.meta.store_id);
+ [, supportTicketId] = await apiUtils.createSupportTicket({
+ ...payloads.createSupportTicket,
+ author: authorId,
+ meta: { store_id: storeId }
+ });For calls with order_id:
- [, supportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, author: CUSTOMER_ID, meta: { store_id: VENDOR_ID, order_id: orderId } });
+ const authorId = Number(CUSTOMER_ID ?? payloads.createSupportTicket.author);
+ const storeId = Number(VENDOR_ID ?? payloads.createSupportTicket.meta.store_id);
+ const orderIdNum = Number(orderId);
+ [, supportTicketId] = await apiUtils.createSupportTicket({
+ ...payloads.createSupportTicket,
+ author: authorId,
+ meta: { store_id: storeId, order_id: orderIdNum }
+ });Update all createSupportTicket calls in this file accordingly.
📝 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 test.step('Create support ticket for testing', async () => { | |
| [, supportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, author: CUSTOMER_ID, meta: { store_id: VENDOR_ID } }); | |
| }); | |
| await test.step('Create support ticket for testing', async () => { | |
| const authorId = Number(CUSTOMER_ID ?? payloads.createSupportTicket.author); | |
| const storeId = Number(VENDOR_ID ?? payloads.createSupportTicket.meta.store_id); | |
| [, supportTicketId] = await apiUtils.createSupportTicket({ | |
| ...payloads.createSupportTicket, | |
| author: authorId, | |
| meta: { store_id: storeId } | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/storeSupports.spec.ts around lines 26-28 (and apply to
every createSupportTicket invocation in this file), ensure process.env.VENDOR_ID
and process.env.CUSTOMER_ID are coerced to numbers with fallback to the payload
defaults before passing to apiUtils.createSupportTicket; specifically, replace
uses of VENDOR_ID and CUSTOMER_ID with Number(process.env.VENDOR_ID ??
VENDOR_ID) and Number(process.env.CUSTOMER_ID ?? CUSTOMER_ID) respectively, and
for calls that include order_id also convert orderId with
Number(process.env.ORDER_ID ?? orderId) (or similar local default) so you never
pass undefined or strings into createSupportTicket.
| await test.step('Create closed support ticket and reopen', async () => { | ||
| const [, closedSupportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, status: 'closed', author: CUSTOMER_ID, meta: { store_id: VENDOR_ID } }); | ||
| await admin.reopenSupportTicket(closedSupportTicketId); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Status value consistency: 'closed' vs 'close'.
payloads.updateSupportTicketStatus uses 'close' while tests create tickets with status 'closed'. Ensure API accepts 'closed'; otherwise tickets may not land in the Closed tab.
Use this to confirm expected status literals:
If API expects 'close', update the test calls:
- const [, closedSupportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, status: 'closed', author: CUSTOMER_ID, meta: { store_id: VENDOR_ID } });
+ const [, closedSupportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, status: 'close', author: Number(CUSTOMER_ID ?? 2), meta: { store_id: Number(VENDOR_ID ?? 8) } });Also applies to: 224-227, 319-322, 333-336
🏁 Script executed:
#!/bin/bash
# Inspect ApiUtils createSupportTicket/status handling and payloads
rg -nC3 "createSupportTicket\(|updateSupportTicketStatus|status[:=]\s*'close|closed'" tests/pw utils | sed -n '1,200p'Length of output: 16086
🏁 Script executed:
#!/bin/bash
# find createSupportTicket in payloads.ts
rg -n "createSupportTicket:" -n tests/pw/utils/payloads.ts -C5
# inspect CUSTOMER_ID and VENDOR_ID definitions
sed -n '1,80p' tests/pw/tests/e2e/storeSupports.spec.tsLength of output: 3803
Use consistent status literal 'close' in createSupportTicket payloads
Replace all status: 'closed' occurrences in your createSupportTicket calls with status: 'close' (e.g. in tests at lines 123–126, 224–227, 257–260, 319–322, 333–336) to match payloads.updateSupportTicketStatus.status and ensure tickets are properly closed.
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/storeSupports.spec.ts around lines 123 to 126, the test
uses status: 'closed' when creating support tickets but the codebase expects the
literal 'close'; update this and all other createSupportTicket payloads
mentioned (lines 224–227, 257–260, 319–322, 333–336) to use status: 'close' so
they match payloads.updateSupportTicketStatus.status and ensure tickets are
treated as closed.
- Added new selectors for the updated UI in `selectors.ts`. - Enhanced `vendorStoreSupportRenderProperly` method to handle both old and new UI structures. - Introduced a new test suite for the React UI, covering various vendor interactions with support tickets. - Updated test data to include new URL for the store support page.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (8)
tests/pw/pages/selectors.ts (1)
930-931: Still need to scope ticket summary locators.We’re back to the same issue I flagged earlier: indexing with
[1]keeps these selectors order‑fragile. Please scope bothticketIdandvendorInfounder the summary holder (e.g.,div.dokan-chat-summary-holder .dokan-summary-info.ticket-id/.dokan-vendor-info) so they reliably target the intended elements without positional dependence.tests/pw/tests/e2e/storeSupports.spec.ts (7)
31-33: Coerce env IDs before creating tickets.
CUSTOMER_ID/VENDOR_IDcome straight fromprocess.env; when unset you overwrite the payload defaults withundefined, and when set they remain strings. Both cases break the API call. Convert them to numbers and fall back to the payload defaults before passing them.- await test.step('Create support ticket for testing', async () => { - [, supportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, author: CUSTOMER_ID, meta: { store_id: VENDOR_ID } }); - }); + await test.step('Create support ticket for testing', async () => { + const authorId = Number(CUSTOMER_ID ?? payloads.createSupportTicket.author); + const storeId = Number(VENDOR_ID ?? payloads.createSupportTicket.meta.store_id); + [, supportTicketId] = await apiUtils.createSupportTicket({ + ...payloads.createSupportTicket, + author: authorId, + meta: { store_id: storeId }, + }); + });Apply the same pattern anywhere else you call
createSupportTicket(including order-linked payloads whereorder_idalso needsNumber(...)). Based on learnings.
37-41: Fix admin cleanup step.The step is labeled “Deactivate…” but still calls
activateModules, leaving the feature on after the suite. Swap todeactivateModulesso later tests don’t inherit the module state.await test.step('Deactivate store support module and cleanup', async () => { - await apiUtils.activateModules(payloads.moduleIds.storeSupport, payloads.adminAuth); + await apiUtils.deactivateModules(payloads.moduleIds.storeSupport, payloads.adminAuth); await aPage.close(); await apiUtils.dispose(); });
99-101: Passvendorwhen replying as vendor.
replySupportTicketdefaultsreplierto'admin', so this test still replies as the admin. Provide the third argument ('vendor') to exercise the vendor path.await test.step('Reply to support ticket as vendor', async () => { - await admin.replySupportTicket(supportTicketId, data.storeSupport.chatReply.asVendor); + await admin.replySupportTicket(supportTicketId, data.storeSupport.chatReply.asVendor, 'vendor'); });
128-131: Use the API’sstatusliteral.Elsewhere we update ticket status with
'close', but these pre-created tickets still pass'closed'. The mismatch prevents them from showing under Closed. Replace everystatus: 'closed'payload in this file with'close'(including the customer/vendor/new UI sections).- const [, closedSupportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, status: 'closed', author: CUSTOMER_ID, meta: { store_id: VENDOR_ID } }); + const authorId = Number(CUSTOMER_ID ?? payloads.createSupportTicket.author); + const storeId = Number(VENDOR_ID ?? payloads.createSupportTicket.meta.store_id); + const [, closedSupportTicketId] = await apiUtils.createSupportTicket({ + ...payloads.createSupportTicket, + status: 'close', + author: authorId, + meta: { store_id: storeId }, + });Mirror the same numeric coercion fix from Line 33 and adjust every other
status: 'closed'call accordingly. Based on learnings.
229-232: Closed-ticket env/type fixes.Same two issues: use
'close'and number-fallback IDs (author,store_id,order_id). Update all repeated patterns in this suite.
268-272: Vendor cleanup still activates & leaves admin page open.This block should deactivate the module and close both
vPageandaPage. Otherwise the module remains enabled and the admin context leaks.await test.step('Deactivate store support module and cleanup', async () => { - await apiUtils.activateModules(payloads.moduleIds.storeSupport, payloads.adminAuth); - await vPage.close(); - await apiUtils.dispose(); + await apiUtils.deactivateModules(payloads.moduleIds.storeSupport, payloads.adminAuth); + await vPage.close(); + await aPage.close(); + await apiUtils.dispose(); });
382-386: React cleanup mirrors prior bug.Switch to
deactivateModules, close both admin/vendor pages, then dispose.await test.step('Deactivate store support module and cleanup', async () => { - await apiUtils.activateModules(payloads.moduleIds.storeSupport, payloads.adminAuth); - await vPage.close(); - await apiUtils.dispose(); + await apiUtils.deactivateModules(payloads.moduleIds.storeSupport, payloads.adminAuth); + await vPage.close(); + await aPage.close(); + await apiUtils.dispose(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/pw/pages/selectors.ts(5 hunks)tests/pw/pages/storeSupportsPage.ts(2 hunks)tests/pw/tests/e2e/storeSupports.spec.ts(4 hunks)tests/pw/utils/testData.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/pw/utils/testData.ts
🧰 Additional context used
🧬 Code graph analysis (1)
tests/pw/tests/e2e/storeSupports.spec.ts (1)
tests/pw/utils/payloads.ts (1)
payloads(9-5094)
⏰ 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: e2e tests (3, 3)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
| const [, supportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, author: CUSTOMER_ID, meta: { store_id: VENDOR_ID } }); | ||
| await admin.storeSupportBulkAction('close', supportTicketId); |
There was a problem hiding this comment.
Apply the env-id fix here too.
This payload still injects raw env strings/undefined. Mirror the Number(...) fallback approach here (and anywhere else you create tickets).
- const [, supportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, author: CUSTOMER_ID, meta: { store_id: VENDOR_ID } });
+ const authorId = Number(CUSTOMER_ID ?? payloads.createSupportTicket.author);
+ const storeId = Number(VENDOR_ID ?? payloads.createSupportTicket.meta.store_id);
+ const [, ticketId] = await apiUtils.createSupportTicket({
+ ...payloads.createSupportTicket,
+ author: authorId,
+ meta: { store_id: storeId },
+ });
+ await admin.storeSupportBulkAction('close', ticketId);Rename the inner supportTicketId to avoid shadowing the outer variable. Based on learnings.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/storeSupports.spec.ts around lines 136-137, the
createSupportTicket payload injects raw env strings/undefined and the inner
supportTicketId shadows an outer variable; change meta to use Number(...) with a
safe fallback (e.g. meta: { store_id: Number(VENDOR_ID) || 0 }) to ensure a
numeric id, rename the destructured result to avoid shadowing (e.g. const [,
createdSupportTicketId]) and use that new name in the subsequent
admin.storeSupportBulkAction call; apply the same Number(...) fallback pattern
anywhere else in the code where tickets are created.
| [, responseBody, orderId] = await apiUtils.createOrderWithStatus(PRODUCT_ID, { ...payloads.createOrder, customer_id: CUSTOMER_ID }, data.order.orderStatus.completed, payloads.vendorAuth); | ||
| }); | ||
|
|
||
| await test.step('Create support ticket for the order', async () => { | ||
| [, supportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, author: CUSTOMER_ID, meta: { store_id: VENDOR_ID, order_id: orderId } }); | ||
| }); |
There was a problem hiding this comment.
Normalize order payload IDs.
PRODUCT_ID, CUSTOMER_ID, VENDOR_ID, and orderId suffer the same string/undefined problem. Convert each to numbers (with payload fallbacks) before sending. Remember to fix meta.order_id in every order-linked ticket creation call.
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/storeSupports.spec.ts around lines 158 to 163, the
PRODUCT_ID, CUSTOMER_ID, VENDOR_ID, and orderId may be strings or undefined when
used in request payloads; convert each to numbers with safe fallbacks before
sending (e.g., Number(PRODUCT_ID) || fallbackId) and ensure all payload fields
that expect numeric IDs receive the converted numbers; also update the support
ticket meta to use meta.order_id (not meta.orderId) and pass the numeric orderId
value in every order-linked ticket creation call.
| [, supportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, author: CUSTOMER_ID, meta: { store_id: VENDOR_ID } }); | ||
| await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, status: 'closed', author: CUSTOMER_ID, meta: { store_id: VENDOR_ID } }); | ||
| }); |
There was a problem hiding this comment.
Vendor suite ticket creation still raw env + wrong status.
Normalize author/store_id, use 'close', and ensure other calls in this suite (including the React UI describe) follow suit.
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/storeSupports.spec.ts around lines 262 to 264, the
vendor-suite support ticket creation is using the customer author and a raw env
mapping for store_id and the status value is incorrect; change the author to the
vendor identity and normalize the meta.store_id to the vendor ID (use the same
VENDOR_ID for both author/store_id as appropriate for vendor tests), change the
status value from 'closed' to 'close', and update any other ticket-creation
calls in this describe block (including the React UI describe) to use the same
normalized author/store_id and 'close' status so the suite is consistent.
| await test.step('Create support tickets for vendor testing', async () => { | ||
| [, supportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, author: CUSTOMER_ID, meta: { store_id: VENDOR_ID } }); | ||
| await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, status: 'closed', author: CUSTOMER_ID, meta: { store_id: VENDOR_ID } }); | ||
| }); |
There was a problem hiding this comment.
React suite repeats the raw env + status issues.
Apply the same numeric coercion and 'close' literal fixes here (and everywhere below in this describe).
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/storeSupports.spec.ts around lines 375 to 378, the test
uses raw environment IDs and the wrong status literal; coerce vendor/store IDs
to numbers (e.g., Number(VENDOR_ID) or +VENDOR_ID) when setting meta.store_id
and anywhere else in this describe that passes env IDs, and change the
support-ticket status from 'closed' to the expected 'close' literal; apply these
two fixes consistently throughout this describe block.
| const [, closedSupportTicketId] = await apiUtils.createSupportTicket({ ...payloads.createSupportTicket, status: 'closed', author: CUSTOMER_ID, meta: { store_id: VENDOR_ID } }); | ||
| await vendor.vendorReopenSupportTicket(closedSupportTicketId); | ||
| }); |
There was a problem hiding this comment.
Also normalize IDs & status here.
Ensure this and the other ticket factories in the React suite coerce env values and use 'close'.
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/storeSupports.spec.ts around lines 439-441, the support
ticket created uses an incorrect status value ('closed') and may be passing
unnormalized env IDs; change the payload to use status: 'close' and coerce
CUSTOMER_ID and VENDOR_ID to the canonical type used across the React test suite
(e.g., Number(CUSTOMER_ID) and Number(VENDOR_ID) or String(...) if your
factories expect strings), and apply the same normalization to the other ticket
factory usages in the React suite so all factories consistently coerce env
values and use 'close' as the status.
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
Bug Fixes
Features
Tests
Refactor