-
Notifications
You must be signed in to change notification settings - Fork 3
feat(davinci-client): add QRCode collector support #562
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 all commits
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,5 @@ | ||
| --- | ||
| '@forgerock/davinci-client': minor | ||
| --- | ||
|
|
||
| Add QR code collector support to davinci-client |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| /* | ||
| * Copyright (c) 2025 Ping Identity Corporation. All rights reserved. | ||
| * | ||
| * This software may be modified and distributed under the terms | ||
| * of the MIT license. See the LICENSE file for details. | ||
| */ | ||
| import type { QrCodeCollector } from '@forgerock/davinci-client/types'; | ||
|
|
||
| export default function (formEl: HTMLFormElement, collector: QrCodeCollector) { | ||
| if (collector.error) { | ||
| const errorEl = document.createElement('p'); | ||
| errorEl.innerText = `QR Code error: ${collector.error}`; | ||
| formEl.appendChild(errorEl); | ||
| return; | ||
| } | ||
|
|
||
| const container = document.createElement('div'); | ||
|
|
||
| const img = document.createElement('img'); | ||
| img.src = collector.output.src; | ||
| img.alt = 'QR Code'; | ||
| img.setAttribute('data-testid', 'qr-code-image'); | ||
| container.appendChild(img); | ||
|
|
||
| if (collector.output.fallbackText) { | ||
| const fallback = document.createElement('p'); | ||
| fallback.innerText = `Manual code: ${collector.output.fallbackText}`; | ||
| fallback.setAttribute('data-testid', 'qr-code-fallback'); | ||
| container.appendChild(fallback); | ||
| } | ||
|
|
||
| formEl.appendChild(container); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| /* | ||
| * Copyright (c) 2025 Ping Identity Corporation. All rights reserved. | ||
| * | ||
| * This software may be modified and distributed under the terms | ||
| * of the MIT license. See the LICENSE file for details. | ||
| */ | ||
| import { expect, test } from '@playwright/test'; | ||
| import { asyncEvents } from './utils/async-events.js'; | ||
| import { password, username } from './utils/demo-user.js'; | ||
|
|
||
| const qrCodeUrl = | ||
| '/?clientId=c12743f9-08e8-4420-a624-71bbb08e9fe1&acr_values=9da1b93991bcd577947da228ad4c741f'; | ||
|
|
||
| test('QR code renders after navigating through device registration flow', async ({ page }) => { | ||
| const { navigate } = asyncEvents(page); | ||
|
|
||
| await navigate(qrCodeUrl); | ||
|
|
||
| // Step 1: Login | ||
| await page.getByRole('button', { name: 'USER_LOGIN' }).click(); | ||
| await page.waitForEvent('requestfinished'); | ||
|
|
||
| await page.getByLabel('Username').fill(username); | ||
| await page.getByLabel('Password').fill(password); | ||
| await page.getByRole('button', { name: 'Sign On' }).click(); | ||
| await page.waitForEvent('requestfinished'); | ||
|
|
||
| // Step 2: Select device registration | ||
| await page.getByRole('button', { name: 'DEVICE_REGISTRATION' }).click(); | ||
| await page.waitForEvent('requestfinished'); | ||
|
|
||
| // Step 3: Choose "Mobile App" from the device selection screen | ||
| await expect(page.getByText('MFA Device Selection - Registration')).toBeVisible(); | ||
| await page.getByRole('button', { name: 'Mobile App' }).click(); | ||
| await page.waitForEvent('requestfinished'); | ||
|
|
||
| // Step 4: QR code should now be visible | ||
| const qrImage = page.locator('[data-testid="qr-code-image"]'); | ||
| await expect(qrImage).toBeVisible({ timeout: 10000 }); | ||
|
|
||
| // Verify the image has a base64-encoded src | ||
| const src = await qrImage.getAttribute('src'); | ||
| expect(src).toBeTruthy(); | ||
| expect(src).toContain('data:image/png;base64,'); | ||
|
|
||
| // Verify fallback text is displayed if present | ||
| const fallback = page.locator('[data-testid="qr-code-fallback"]'); | ||
| const fallbackVisible = await fallback.isVisible(); | ||
| if (fallbackVisible) { | ||
| const fallbackText = await fallback.textContent(); | ||
| expect(fallbackText).toBeTruthy(); | ||
| } | ||
|
Comment on lines
+49
to
+52
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. Strengthen fallback-text assertion to validate actual code content.
Suggested assertion tweak if (fallbackVisible) {
- const fallbackText = await fallback.textContent();
- expect(fallbackText).toBeTruthy();
+ const fallbackText = (await fallback.textContent()) ?? '';
+ const manualCode = fallbackText.replace(/^Manual code:\s*/i, '').trim();
+ expect(manualCode).not.toEqual('');
}🤖 Prompt for AI Agents |
||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -29,6 +29,7 @@ import type { | |||||||||||||||||||||||||||||||||||||
| AutoCollectors, | ||||||||||||||||||||||||||||||||||||||
| SingleValueAutoCollectorTypes, | ||||||||||||||||||||||||||||||||||||||
| ObjectValueAutoCollectorTypes, | ||||||||||||||||||||||||||||||||||||||
| QrCodeCollectorBase, | ||||||||||||||||||||||||||||||||||||||
| } from './collector.types.js'; | ||||||||||||||||||||||||||||||||||||||
| import type { | ||||||||||||||||||||||||||||||||||||||
| DeviceAuthenticationField, | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -38,6 +39,7 @@ import type { | |||||||||||||||||||||||||||||||||||||
| MultiSelectField, | ||||||||||||||||||||||||||||||||||||||
| PhoneNumberField, | ||||||||||||||||||||||||||||||||||||||
| ProtectField, | ||||||||||||||||||||||||||||||||||||||
| QrCodeField, | ||||||||||||||||||||||||||||||||||||||
| ReadOnlyField, | ||||||||||||||||||||||||||||||||||||||
| RedirectField, | ||||||||||||||||||||||||||||||||||||||
| SingleSelectField, | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -678,7 +680,7 @@ export function returnObjectValueCollector( | |||||||||||||||||||||||||||||||||||||
| * @returns {NoValueCollector} The constructed NoValueCollector object. | ||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||
| export function returnNoValueCollector< | ||||||||||||||||||||||||||||||||||||||
| Field extends ReadOnlyField, | ||||||||||||||||||||||||||||||||||||||
| Field extends ReadOnlyField | QrCodeField, | ||||||||||||||||||||||||||||||||||||||
| CollectorType extends NoValueCollectorTypes = 'NoValueCollector', | ||||||||||||||||||||||||||||||||||||||
| >(field: Field, idx: number, collectorType: CollectorType) { | ||||||||||||||||||||||||||||||||||||||
| let error = ''; | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -713,6 +715,31 @@ export function returnReadOnlyCollector(field: ReadOnlyField, idx: number) { | |||||||||||||||||||||||||||||||||||||
| return returnNoValueCollector(field, idx, 'ReadOnlyCollector'); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||
| * @function returnQrCodeCollector - Creates a QrCodeCollector object for displaying QR code images. | ||||||||||||||||||||||||||||||||||||||
| * @param {QrCodeField} field - The field object containing key, content, type, and optional fallbackText. | ||||||||||||||||||||||||||||||||||||||
| * @param {number} idx - The index to be used in the id of the QrCodeCollector. | ||||||||||||||||||||||||||||||||||||||
| * @returns {QrCodeCollectorBase} The constructed QrCodeCollector object. | ||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||
| export function returnQrCodeCollector(field: QrCodeField, idx: number): QrCodeCollectorBase { | ||||||||||||||||||||||||||||||||||||||
|
Collaborator
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. Can we reuse the generic |
||||||||||||||||||||||||||||||||||||||
| const base = returnNoValueCollector(field, idx, 'QrCodeCollector'); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| let error = base.error || ''; | ||||||||||||||||||||||||||||||||||||||
| if (!('key' in field) || !field.key) { | ||||||||||||||||||||||||||||||||||||||
| error = `${error}Key is not found in the field object. `; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||||
| ...base, | ||||||||||||||||||||||||||||||||||||||
| error: error || null, | ||||||||||||||||||||||||||||||||||||||
| output: { | ||||||||||||||||||||||||||||||||||||||
| ...base.output, | ||||||||||||||||||||||||||||||||||||||
| src: field.content || '', | ||||||||||||||||||||||||||||||||||||||
| fallbackText: field.fallbackText || '', | ||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+732
to
+739
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. Normalize On Line 736, Suggested patch return {
...base,
error: error || null,
output: {
...base.output,
+ label: typeof base.output.label === 'string' ? base.output.label : '',
src: field.content || '',
fallbackText: field.fallbackText || '',
},
};📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||
| * @function returnValidator - Creates a validator function based on the provided collector | ||||||||||||||||||||||||||||||||||||||
| * @param {ValidatedTextCollector | ObjectValueCollectors | MultiValueCollectors | AutoCollectors} collector - The collector to which the value will be validated | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
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.
🧩 Analysis chain
🌐 Web query:
In Playwright, is using page.waitForEvent('requestfinished') after a click considered flaky, and what are the recommended alternatives for deterministic synchronization in E2E tests?💡 Result:
Yes—
page.waitForEvent('requestfinished')after aclick()is often flaky/racy in E2E tests because:requestfinishedfires for any request that completes (including redirects and even HTTP error responses like 404/503), so you can easily synchronize on the “wrong” request when multiple requests are in flight. [1]Recommended deterministic alternatives
Wait for a specific response (preferred when an API call is the real “done” signal)
Register the wait before the click and match by URL/predicate:
Wait for URL / navigation deterministically (preferred when the user-visible result is navigation)
Use
page.waitForURL()(Playwright explicitly recommends URL waiting patterns for navigation sync). [3]Typical race-safe pattern:
Wait on user-visible UI state via “web-first” assertions (preferred for true E2E behavior)
Playwright recommends asserting what the user sees; these assertions auto-wait/retry until the condition is met. [2]
If you must sync with network, target the request/response—not “any finished request”
Prefer
waitForResponse/waitForRequestwith a predicate overrequestfinished(which is broad and easy to mismatch). [1]Source list
[1] Playwright docs: request event order and meaning of
requestfinished(incl. redirects and HTTP error responses still ending inrequestfinished). (playwright.dev)[2] Playwright docs: Best Practices—test user-visible behavior; use web-first assertions that auto-wait. (playwright.dev)
[3] Playwright docs: Navigations—“Waiting for navigation” using
page.waitForURL(). (playwright.dev)Citations:
Unscoped
requestfinishedwaits introduce race conditions and test flakiness.On lines 21, 26, 30, and 35,
page.waitForEvent('requestfinished')resolves on any finished request (including unrelated redirects and HTTP error responses). Starting the wait after the click creates a classic race: if the network activity completes before the listener is registered, the test will hang until timeout.Replace with UI-state waits (web-first assertions that auto-retry) or scoped response waits matched by URL:
Stabilization pattern
🤖 Prompt for AI Agents