-
Notifications
You must be signed in to change notification settings - Fork 673
test(billing): cover recovery banner permissions #14713
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| import { expect } from '@playwright/test' | ||
| import type { Locator, Page } from '@playwright/test' | ||
|
|
||
| import { BaseDialog } from '@e2e/fixtures/components/BaseDialog' | ||
| import { waitForCloudApp } from '@e2e/fixtures/cloudAppFixture' | ||
|
|
||
| export class WorkspaceBillingSettings extends BaseDialog { | ||
| readonly content: Locator | ||
| readonly statusBanner: Locator | ||
|
|
||
| constructor(page: Page) { | ||
| super(page, page.getByTestId('settings-dialog')) | ||
| this.content = this.root.getByRole('main') | ||
| this.statusBanner = this.content.getByRole('status') | ||
| } | ||
|
|
||
| async open(appUrl: string): Promise<void> { | ||
| await this.page.goto(appUrl) | ||
| await waitForCloudApp(this.page) | ||
| await expect( | ||
| this.page.getByRole('status', { name: 'Loading ComfyUI' }) | ||
| ).toBeHidden() | ||
| await this.page.getByRole('button', { name: 'Close dialog' }).click() | ||
| await this.page | ||
| .getByRole('button', { name: /^Settings/ }) | ||
| .first() | ||
| .click() | ||
| await expect(this.root).toBeVisible() | ||
| await this.root | ||
| .locator('nav') | ||
| .getByRole('button', { name: 'Plan & Credits', exact: true }) | ||
| .click() | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,148 @@ | ||||||||||||||
| import { expect } from '@playwright/test' | ||||||||||||||
| import type { | ||||||||||||||
| PaymentPortalRequest, | ||||||||||||||
| PaymentPortalResponse | ||||||||||||||
| } from '@comfyorg/ingest-types' | ||||||||||||||
|
|
||||||||||||||
| import { cloudAppFixture as test } from '@e2e/fixtures/cloudAppFixture' | ||||||||||||||
| import { WorkspaceBillingSettings } from '@e2e/fixtures/components/WorkspaceBillingSettings' | ||||||||||||||
| import { | ||||||||||||||
| PAYMENT_FAILED_TEAM_BILLING_STATUS, | ||||||||||||||
| PAUSED_TEAM_BILLING_STATUS | ||||||||||||||
| } from '@e2e/fixtures/data/cloudWorkspace' | ||||||||||||||
| import { CLOUD_SELF_EMAIL } from '@e2e/fixtures/helpers/CloudAuthHelper' | ||||||||||||||
| import { APP_URL, setupCloudApp } from '@e2e/fixtures/utils/cloudAppSetup' | ||||||||||||||
| import { jsonRoute } from '@e2e/fixtures/utils/jsonRoute' | ||||||||||||||
| import { member, workspace } from '@e2e/fixtures/utils/workspaceMocks' | ||||||||||||||
|
|
||||||||||||||
| test.describe('Team billing recovery visibility', { tag: '@cloud' }, () => { | ||||||||||||||
| test.describe('team owner with a failed payment', () => { | ||||||||||||||
| let settings: WorkspaceBillingSettings | ||||||||||||||
| let portalRequests: PaymentPortalRequest[] | ||||||||||||||
|
|
||||||||||||||
| test.beforeEach(async ({ page }) => { | ||||||||||||||
| portalRequests = [] | ||||||||||||||
| await page.addInitScript(() => { | ||||||||||||||
| window.open = (url) => { | ||||||||||||||
| document.documentElement.dataset.openedUrl = String(url) | ||||||||||||||
| return window | ||||||||||||||
| } | ||||||||||||||
| }) | ||||||||||||||
| await setupCloudApp(page, { | ||||||||||||||
| workspace: workspace('team', 'owner'), | ||||||||||||||
| members: [ | ||||||||||||||
| member({ | ||||||||||||||
| email: CLOUD_SELF_EMAIL, | ||||||||||||||
| role: 'owner', | ||||||||||||||
| is_original_owner: true | ||||||||||||||
| }) | ||||||||||||||
| ], | ||||||||||||||
| features: { | ||||||||||||||
| billing_control_enabled: true, | ||||||||||||||
| v1_payment_recovery: true | ||||||||||||||
| } | ||||||||||||||
| }) | ||||||||||||||
| await page.route('**/api/billing/status', (route) => | ||||||||||||||
| route.fulfill(jsonRoute(PAYMENT_FAILED_TEAM_BILLING_STATUS)) | ||||||||||||||
| ) | ||||||||||||||
| await page.route('**/api/billing/payment-portal', (route) => { | ||||||||||||||
| portalRequests.push(route.request().postDataJSON()) | ||||||||||||||
| return route.fulfill( | ||||||||||||||
| jsonRoute({ | ||||||||||||||
| url: 'https://billing.stripe.com/session/e2e-recovery' | ||||||||||||||
| } satisfies PaymentPortalResponse) | ||||||||||||||
| ) | ||||||||||||||
| }) | ||||||||||||||
| settings = new WorkspaceBillingSettings(page) | ||||||||||||||
| await settings.open(APP_URL) | ||||||||||||||
| }) | ||||||||||||||
|
|
||||||||||||||
| test('TB-22A exposes payment recovery and sends the current return URL', async ({ | ||||||||||||||
| page | ||||||||||||||
| }) => { | ||||||||||||||
| await expect(settings.statusBanner).toContainText('Payment failed') | ||||||||||||||
| const returnUrl = page.url() | ||||||||||||||
|
|
||||||||||||||
| await settings.statusBanner | ||||||||||||||
| .getByRole('button', { name: 'Update payment' }) | ||||||||||||||
| .click() | ||||||||||||||
|
|
||||||||||||||
| expect(portalRequests).toEqual([{ return_url: returnUrl }]) | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this intermittently read Current-head browser evidence: #14713 (comment) |
||||||||||||||
| await expect | ||||||||||||||
| .poll(() => page.locator('html').getAttribute('data-opened-url')) | ||||||||||||||
| .toBe('https://billing.stripe.com/session/e2e-recovery') | ||||||||||||||
| }) | ||||||||||||||
| }) | ||||||||||||||
|
|
||||||||||||||
| test.describe('team member with a failed payment', () => { | ||||||||||||||
| let settings: WorkspaceBillingSettings | ||||||||||||||
|
|
||||||||||||||
| test.beforeEach(async ({ page }) => { | ||||||||||||||
| await setupCloudApp(page, { | ||||||||||||||
| workspace: workspace('team', 'member'), | ||||||||||||||
| members: [ | ||||||||||||||
| member({ | ||||||||||||||
| email: 'creator@test.comfy.org', | ||||||||||||||
| role: 'owner', | ||||||||||||||
| is_original_owner: true | ||||||||||||||
| }), | ||||||||||||||
| member({ email: CLOUD_SELF_EMAIL, role: 'member' }) | ||||||||||||||
| ], | ||||||||||||||
| features: { | ||||||||||||||
| billing_control_enabled: true, | ||||||||||||||
| v1_payment_recovery: true | ||||||||||||||
| } | ||||||||||||||
| }) | ||||||||||||||
| await page.route('**/api/billing/status', (route) => | ||||||||||||||
| route.fulfill(jsonRoute(PAYMENT_FAILED_TEAM_BILLING_STATUS)) | ||||||||||||||
| ) | ||||||||||||||
| settings = new WorkspaceBillingSettings(page) | ||||||||||||||
| await settings.open(APP_URL) | ||||||||||||||
| }) | ||||||||||||||
|
|
||||||||||||||
| test('TB-22B keeps payment failure details and recovery private', async () => { | ||||||||||||||
| await expect(settings.statusBanner).toHaveCount(0) | ||||||||||||||
| await expect( | ||||||||||||||
| settings.content.getByRole('button', { name: 'Update payment' }) | ||||||||||||||
| ).toHaveCount(0) | ||||||||||||||
| await expect(settings.content).not.toContainText('Payment failed') | ||||||||||||||
| }) | ||||||||||||||
| }) | ||||||||||||||
|
|
||||||||||||||
| test.describe('team member with a paused subscription', () => { | ||||||||||||||
| let settings: WorkspaceBillingSettings | ||||||||||||||
|
|
||||||||||||||
| test.beforeEach(async ({ page }) => { | ||||||||||||||
| await setupCloudApp(page, { | ||||||||||||||
| workspace: workspace('team', 'member'), | ||||||||||||||
| members: [ | ||||||||||||||
| member({ | ||||||||||||||
| email: 'creator@test.comfy.org', | ||||||||||||||
| role: 'owner', | ||||||||||||||
| is_original_owner: true | ||||||||||||||
| }), | ||||||||||||||
| member({ email: CLOUD_SELF_EMAIL, role: 'member' }) | ||||||||||||||
| ], | ||||||||||||||
| features: { | ||||||||||||||
| billing_control_enabled: true, | ||||||||||||||
| v1_payment_recovery: true | ||||||||||||||
| } | ||||||||||||||
| }) | ||||||||||||||
| await page.route('**/api/billing/status', (route) => | ||||||||||||||
| route.fulfill(jsonRoute(PAUSED_TEAM_BILLING_STATUS)) | ||||||||||||||
| ) | ||||||||||||||
| settings = new WorkspaceBillingSettings(page) | ||||||||||||||
| await settings.open(APP_URL) | ||||||||||||||
| }) | ||||||||||||||
|
|
||||||||||||||
| test('TB-22C shows a role-safe pause notice without billing controls', async () => { | ||||||||||||||
| await expect(settings.statusBanner).toContainText('Subscription paused') | ||||||||||||||
| await expect(settings.statusBanner).toContainText( | ||||||||||||||
| "Ask your workspace owner to restore the workspace's subscription." | ||||||||||||||
| ) | ||||||||||||||
| await expect( | ||||||||||||||
| settings.statusBanner.getByRole('button', { name: 'Update payment' }) | ||||||||||||||
| ).toHaveCount(0) | ||||||||||||||
|
Comment on lines
+143
to
+145
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Check recovery-control absence in all Plan & Credits content. The current locator excludes Proposed fix- settings.statusBanner.getByRole('button', { name: 'Update payment' })
+ settings.content.getByRole('button', { name: 'Update payment' })📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| }) | ||||||||||||||
| }) | ||||||||||||||
| }) | ||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 13518
🌐 Web query:
Playwright page.waitForRequest wait before click pattern💡 Result:
The recommended pattern for waiting for a request after an action (such as a click) in Playwright is to initiate the wait before performing the action and to use Promise.all to handle both concurrently [1][2][3]. This approach prevents race conditions where the request completes before the test starts listening for it [2][4]. Recommended Pattern Use Promise.all to register the listener and trigger the action simultaneously [2][5]. This ensures Playwright is actively "armed" to intercept the request as soon as it is sent [2]. const [request] = await Promise.all([ page.waitForRequest('/api/target-endpoint'), page.locator('button').click, ]); Why this pattern is necessary If you perform the action first (e.g., await page.click) and then wait (e.g., await page.waitForRequest), there is a risk that the request will be sent and processed by the browser before your test code begins listening for it [2][4]. This leads to intermittent timeout errors, which may appear non-deterministic depending on network speed or browser engine behavior [4][6]. Alternative approach If you do not want to use Promise.all, you can still achieve the same result by creating the promise first without awaiting it, then triggering the action, and finally awaiting the promise [1][7]: // 1. Start waiting for request before the action (no await here) const requestPromise = page.waitForRequest('/api/target-endpoint'); // 2. Perform the action await page.locator('button').click; // 3. Wait for the promise to resolve const request = await requestPromise; Both methods are functionally equivalent, but the Promise.all syntax is often preferred for its readability and conciseness [1][2][5]. Additional Considerations - Predicates: You can use a predicate function instead of a URL string to match requests based on status, headers, or body content (e.g., req.postDataJSON) [5][8][3]. - waitForResponse: In many cases, it is more reliable to wait for the response (
page.waitForResponse) rather than the request, as the response confirms that the server has successfully received and processed the action [4]. The same Promise.all pattern applies towaitForResponse[9][5].Citations:
Wait for the payment-portal request before reading its body.
Line 67 reads
portalRequestsimmediately after the click, while the route handler is executed asynchronously. Use Playwright’s wait-before-click pattern for the API request and assert from the intercepted request body.🤖 Prompt for AI Agents