-
Notifications
You must be signed in to change notification settings - Fork 580
fix(cloud): stop bouncing working users to /cloud/survey mid-session #12301
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
Changes from all commits
c7abc90
c717616
e88a7ae
6915d83
9dfb501
e399b57
1e9a6f5
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,91 @@ | ||
| import { fromPartial } from '@total-typescript/shoehorn' | ||
| import { beforeEach, describe, expect, test, vi } from 'vitest' | ||
|
|
||
| import { getSurveyCompletedStatus } from './auth' | ||
|
|
||
| const fetchApi = vi.fn() | ||
|
|
||
| vi.mock('@/scripts/api', () => ({ | ||
| api: { | ||
| fetchApi: (...args: unknown[]) => fetchApi(...args) | ||
| } | ||
| })) | ||
|
|
||
| vi.mock('@sentry/vue', () => ({ | ||
| addBreadcrumb: vi.fn(), | ||
| captureException: vi.fn() | ||
| })) | ||
|
|
||
| function mockResponse({ | ||
| ok, | ||
| status, | ||
| body | ||
| }: { | ||
| ok: boolean | ||
| status: number | ||
| body?: unknown | ||
| }): Response { | ||
| return fromPartial<Response>({ | ||
| ok, | ||
| status, | ||
| statusText: '', | ||
| json: async () => body | ||
| }) | ||
| } | ||
|
|
||
| describe('getSurveyCompletedStatus', () => { | ||
| beforeEach(() => { | ||
| fetchApi.mockReset() | ||
| }) | ||
|
|
||
| test('200 with non-empty value → true', async () => { | ||
| fetchApi.mockResolvedValueOnce( | ||
| mockResponse({ ok: true, status: 200, body: { value: { q1: 'a' } } }) | ||
| ) | ||
| await expect(getSurveyCompletedStatus()).resolves.toBe(true) | ||
| }) | ||
|
|
||
| test('200 with empty value → false (the only "not completed" signal)', async () => { | ||
| fetchApi.mockResolvedValueOnce( | ||
| mockResponse({ ok: true, status: 200, body: { value: {} } }) | ||
| ) | ||
| await expect(getSurveyCompletedStatus()).resolves.toBe(false) | ||
| }) | ||
|
|
||
| test('200 with null value → false', async () => { | ||
| fetchApi.mockResolvedValueOnce( | ||
| mockResponse({ ok: true, status: 200, body: { value: null } }) | ||
| ) | ||
| await expect(getSurveyCompletedStatus()).resolves.toBe(false) | ||
| }) | ||
|
|
||
| test('404 → true (do not bounce on missing key)', async () => { | ||
| fetchApi.mockResolvedValueOnce(mockResponse({ ok: false, status: 404 })) | ||
| await expect(getSurveyCompletedStatus()).resolves.toBe(true) | ||
| }) | ||
|
|
||
| test('500 → true (do not bounce on transient backend error)', async () => { | ||
| fetchApi.mockResolvedValueOnce(mockResponse({ ok: false, status: 500 })) | ||
| await expect(getSurveyCompletedStatus()).resolves.toBe(true) | ||
| }) | ||
|
|
||
| // 401/403 fall under the same "ambiguous => treat as completed" policy. | ||
| // The dedicated auth layer handles re-authentication on the next API | ||
| // call; this function deliberately does not try to disambiguate auth | ||
| // failures from other non-OK responses. Locking with tests so the | ||
| // policy can't drift back to a "throw on auth error" branch. | ||
| test('401 → true (auth layer handles re-auth on next call)', async () => { | ||
| fetchApi.mockResolvedValueOnce(mockResponse({ ok: false, status: 401 })) | ||
| await expect(getSurveyCompletedStatus()).resolves.toBe(true) | ||
| }) | ||
|
|
||
|
Comment on lines
+72
to
+81
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. 👍 Nit-positive: this is the right way to write a regression test. The 401/403 cases carry an explanatory comment that pins the policy decision to a reason — |
||
| test('403 → true (auth layer handles re-auth on next call)', async () => { | ||
| fetchApi.mockResolvedValueOnce(mockResponse({ ok: false, status: 403 })) | ||
| await expect(getSurveyCompletedStatus()).resolves.toBe(true) | ||
| }) | ||
|
|
||
| test('network rejection → true (do not bounce on network error)', async () => { | ||
| fetchApi.mockRejectedValueOnce(new TypeError('Network request failed')) | ||
| await expect(getSurveyCompletedStatus()).resolves.toBe(true) | ||
| }) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,23 +96,24 @@ export async function getSurveyCompletedStatus(): Promise<boolean> { | |
| } | ||
| }) | ||
| if (!response.ok) { | ||
| // Not an error case - survey not completed is a valid state | ||
| // Ambiguous response (404/5xx/etc). Treat as completed to avoid | ||
|
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. 👍 Nit-positive: this comment is doing what the AGENTS.md guidance asks for — it explains the user-facing why (transient hiccups bouncing working customers), not the what. Future reader will know why this looks like a swallow and won't "helpfully" revert it. Keep this style. |
||
| // bouncing working customers to /cloud/survey on transient hiccups. | ||
| // Real "not completed" only comes from a 200 with empty value. | ||
| Sentry.addBreadcrumb({ | ||
| category: 'auth', | ||
| message: 'Survey status check returned non-ok response', | ||
| level: 'info', | ||
| level: 'warning', | ||
| data: { | ||
| status: response.status, | ||
| endpoint: `/settings/${ONBOARDING_SURVEY_KEY}` | ||
| } | ||
| }) | ||
| return false | ||
| return true | ||
|
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. The PR body's resolution table still says:
…but the final commit ( This is the only thing I would call out as actually important — reviewers / approvers / future spelunkers will read the PR body and sign off on a different contract than the one being shipped. Please update the table to show 401/403 →
Contributor
Author
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. Updated the PR description — dropped the throws row, all non-OK / network now show true. Thanks for catching it. |
||
| } | ||
| const data = await response.json() | ||
| // Check if data exists and is not empty | ||
| return !isEmpty(data.value) | ||
| } catch (error) { | ||
| // Network error - still capture it as it's not thrown from above | ||
| // Network/parse failure — same policy as ambiguous HTTP responses. | ||
| Sentry.captureException(error, { | ||
| tags: { | ||
| api_endpoint: '/settings/{key}', | ||
|
|
@@ -124,7 +125,7 @@ export async function getSurveyCompletedStatus(): Promise<boolean> { | |
| }, | ||
| level: 'warning' | ||
|
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. Minor consistency nit: the non-OK HTTP branch above breadcrumbs at
Contributor
Author
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. Bumped the breadcrumb to warning in 1e9a6f5 so both ambiguous-response paths log at the same level. |
||
| }) | ||
| return false | ||
| return true | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
Minor:
body?: unknown+json: async () => body+as unknown as Responseis the kind of double-cast the project's type-safety guidance gently warns against. Acceptable for a test scaffold and I would not block on it, but a typedPartial<Response>helper (orbody: { value: unknown }since that's all the SUT reads) would keep the test more honest about the shape it depends on. Non-blocking.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.
Correction on myself — this codebase has
@total-typescript/shoehornalready in dev deps and ~70 test files use it for exactly this.fromPartial<Response>(...)keeps the same partial-shape style without theas unknown as Responsedouble cast (addimport { fromPartial } from '@total-typescript/shoehorn'at the top):Still non-blocking, just pointing at the local convention. Sorry for the noise.
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.
Switched to fromPartial in 1e9a6f5. Thanks for the pointer to the local convention.