Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/controllers/verify_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ def app_data

{
base_path: idv_app_path,
start_over_url: idv_session_path,
cancel_url: idv_cancel_path,
completion_url: completion_url,
initial_values: initial_values,
enabled_step_names: IdentityConfig.store.idv_api_enabled_steps,
Expand Down
67 changes: 67 additions & 0 deletions app/javascript/packages/components/button-to.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import sinon from 'sinon';
import { render } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import ButtonTo from './button-to';

describe('ButtonTo', () => {
beforeEach(() => {
const csrf = document.createElement('meta');
csrf.name = 'csrf-token';
csrf.content = 'token-value';
document.body.appendChild(csrf);
});

it('renders props passed through to Button', () => {
const { getByRole } = render(
<ButtonTo url="" method="" isUnstyled>
Click me
</ButtonTo>,
);

const button = getByRole('button', { name: 'Click me' }) as HTMLButtonElement;

expect(button.type).to.equal('button');
expect(button.classList.contains('usa-button')).to.be.true();
expect(button.classList.contains('usa-button--unstyled')).to.be.true();
});

it('creates a form in the body outside the root container', () => {
const { container, getByRole } = render(
<ButtonTo url="/" method="delete" isUnstyled>
Click me
</ButtonTo>,
);

const form = document.querySelector('form')!;
expect(form).to.be.ok();
expect(container.contains(form)).to.be.false();
return Promise.all([
new Promise<void>((resolve) => {
form.addEventListener('submit', (event) => {
event.preventDefault();
expect(Object.fromEntries(new window.FormData(form))).to.deep.equal({
_method: 'delete',
authenticity_token: 'token-value',
});
resolve();
});
}),
userEvent.click(getByRole('button')),
]);
});

it('submits to form on click', async () => {
const { getByRole } = render(
<ButtonTo url="" method="" isUnstyled>
Click me
</ButtonTo>,
);

const form = document.querySelector('form')!;
sinon.stub(form, 'submit');

await userEvent.click(getByRole('button'));

expect(form.submit).to.have.been.calledOnce();
});
});
49 changes: 49 additions & 0 deletions app/javascript/packages/components/button-to.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { useRef } from 'react';
import { createPortal } from 'react-dom';
import Button from './button';
import type { ButtonProps } from './button';

interface ButtonToProps extends ButtonProps {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would we ever need to add params here that could be posted? or is that the kind of thing we'd add if/when we need it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would we ever need to add params here that could be posted? or is that the kind of thing we'd add if/when we need it?

Possibly? I liken this to Rails' button_to, which (TIL) supports a params hash for just that purpose. But I heavily subscribe to the school of YAGNI, so I'm not in any hurry to add it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YAGNI 👍

/**
* URL to which the user should navigate.
*/
url: string;

/**
* Form method button should submit as.
*/
method: string;
}

/**
* Component which renders a button that navigates to the specified URL via form, with method
* parameterized as a hidden input and including authenticity token. The form is rendered to the
* document root, to avoid conflicts with nested forms.
*/
function ButtonTo({ url, method, children, ...buttonProps }: ButtonToProps) {
const formRef = useRef<HTMLFormElement>(null);
const csrfRef = useRef<HTMLInputElement>(null);

function submitForm() {
const csrf = document.querySelector<HTMLMetaElement>('meta[name="csrf-token"]')?.content;
if (csrf && csrfRef.current) {
csrfRef.current.value = csrf;
}
formRef.current?.submit();
}

return (
<Button {...buttonProps} onClick={submitForm}>
{children}
{createPortal(
<form ref={formRef} method="post" action={url}>
<input type="hidden" name="_method" value={method} />
<input ref={csrfRef} type="hidden" name="authenticity_token" />
</form>,
document.body,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need some sort of key to reference these at the document body? so that two ButtonTo's on the same page don't overwrite each other's forms?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need some sort of key to reference these at the document body? so that two ButtonTo's on the same page don't overwrite each other's forms?

If there were two ButtonTo, each would have and submit their own form, so I don't think they'd conflict with each other.

The markup would look like...

<body>
  <div id="app-root">
    <button></button>
    <button></button>
  </div>
  <form></form>
  <form></form>
</body>

)}
</Button>
);
}

export default ButtonTo;
1 change: 1 addition & 0 deletions app/javascript/packages/components/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
export { default as Alert } from './alert';
export { default as Button } from './button';
export { default as ButtonTo } from './button-to';
export { default as BlockLink } from './block-link';
export { default as Icon } from './icon';
export { default as FullScreen } from './full-screen';
Expand Down
44 changes: 0 additions & 44 deletions app/javascript/packages/document-capture/components/button-to.jsx

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { render } from '@testing-library/react';
import { Provider as UploadContextProvider } from '../context/upload';
import StartOverOrCancel from './start-over-or-cancel';

describe('StartOverOrCancel', () => {
it('omits start over link when in hybrid flow', () => {
const { getByText } = render(
<UploadContextProvider
flowPath="hybrid"
endpoint=""
csrf=""
method="POST"
backgroundUploadURLs={{}}
>
<StartOverOrCancel />
</UploadContextProvider>,
);

expect(() => getByText('doc_auth.buttons.start_over')).to.throw();
expect(getByText('links.cancel')).to.be.ok();
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { useContext } from 'react';
import { StartOverOrCancel as FlowStartOverOrCancel } from '@18f/identity-verify-flow';
import UploadContext from '../context/upload';

function StartOverOrCancel() {
const { flowPath } = useContext(UploadContext);

return <FlowStartOverOrCancel canStartOver={flowPath !== 'hybrid'} />;
}

export default StartOverOrCancel;
10 changes: 0 additions & 10 deletions app/javascript/packages/document-capture/context/upload.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ const UploadContext = createContext({
backgroundUploadURLs: /** @type {Record<string,string>} */ ({}),
backgroundUploadEncryptKey: /** @type {CryptoKey=} */ (undefined),
flowPath: /** @type {FlowPath} */ ('standard'),
startOverURL: /** @type {string} */ (''),
cancelURL: /** @type {string} */ (''),
Comment on lines -12 to -13
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome that we can clean up the existing flow by using this too

csrf: /** @type {string?} */ (null),
});

Expand Down Expand Up @@ -76,8 +74,6 @@ UploadContext.displayName = 'UploadContext';
* @prop {string?} csrf CSRF token to send as parameter to upload implementation.
* @prop {Record<string,any>=} formData Extra form data to merge into the payload before uploading
* @prop {FlowPath} flowPath The user's session flow path, one of "standard" or "hybrid".
* @prop {string} startOverURL URL to application DELETE path for session restart.
* @prop {string} cancelURL URL to application path for session cancel.
* @prop {ReactNode} children Child elements.
*/

Expand All @@ -96,8 +92,6 @@ function UploadContextProvider({
csrf,
formData,
flowPath,
startOverURL,
cancelURL,
children,
}) {
const uploadWithCSRF = (payload) =>
Expand All @@ -117,8 +111,6 @@ function UploadContextProvider({
backgroundUploadEncryptKey,
isMockClient,
flowPath,
startOverURL,
cancelURL,
csrf,
}),
[
Expand All @@ -129,8 +121,6 @@ function UploadContextProvider({
backgroundUploadEncryptKey,
isMockClient,
flowPath,
startOverURL,
cancelURL,
csrf,
],
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,18 @@ describe('addSearchParams', () => {
expect(actual).to.equal(expected);
});

it('adds search params to an existing search fragment', () => {
const params = '?a=1&b=1';
const expected = '?a=1&b=2&c=3';
it('accepts URL as a path', () => {
const url = '/example';
const expected = `${window.location.origin}/example?a=1&b=2&c=3`;

const actual = addSearchParams(params, { b: 2, c: 3 });
const actual = addSearchParams(url, { a: 1, b: 2, c: 3 });

expect(actual).to.equal(expected);
});

it('adds search params to an empty URL', () => {
const params = '';
const expected = '?a=1&b=2&c=3';
const expected = `${window.location.origin}/?a=1&b=2&c=3`;

const actual = addSearchParams(params, { a: 1, b: 2, c: 3 });

Expand Down
24 changes: 6 additions & 18 deletions app/javascript/packages/url/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,13 @@
* Given a URL or a string fragment of search parameters and an object of parameters, returns a
* new URL or search parameters with the parameters added.
*
* @param urlOrParams Original URL or search parameters.
* @param url Original URL.
* @param params Search parameters to add.
*
* @return Modified URL or search parameters.
* @return Modified URL.
*/
export function addSearchParams(urlOrParams: string, params: Record<string, any>): string {
let parsedURLOrParams: URL | URLSearchParams;
let searchParams: URLSearchParams;

try {
parsedURLOrParams = new URL(urlOrParams);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason we moved away from the URL class? I'd love to be able to rely on something like that instead of having to build our own

Copy link
Copy Markdown
Contributor Author

@aduth aduth May 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason we moved away from the URL class? I'd love to be able to rely on something like that instead of having to build our own

I went through a few iterations of this. The main problem with the URL class is that this function is meant to be quite flexible with its input values (absolute URLs, paths, querystring-only), and it's hard to map each of those to a corresponding return value of the same form using URL.

That being said, I couldn't actually find any existing usage of the querystring parameter input, so we could adjust this to assume that both the input and output would be a full URL.

Copy link
Copy Markdown
Contributor Author

@aduth aduth May 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, to be more specific about why it was changed here: We had been initializing startOverURL and cancelURL with paths. A URL constructor won't parse a path fragment on its own (new URL('/foo') throws). We could adjust this to parse relative to the current URL (e.g. new URL('/foo', window.location.href)), but at that point is when it becomes harder to determine the logic around how to return the expected value (parsedURL.href ? parsedURL.pathname + parsedURL.search ? How do we determine which to use?).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That being said, I couldn't actually find any existing usage of the querystring parameter input, so we could adjust this to assume that both the input and output would be a full URL.

Simplifying supports simplifies the implementation! 0f3f511

searchParams = parsedURLOrParams.searchParams;
} catch {
parsedURLOrParams = new URLSearchParams(urlOrParams);
searchParams = parsedURLOrParams;
}

Object.entries(params).forEach(([key, value]) => searchParams.set(key, value));

const result = parsedURLOrParams.toString();
return parsedURLOrParams instanceof URLSearchParams ? `?${result}` : result;
export function addSearchParams(url: string, params: Record<string, any>): string {
const parsedURL = new URL(url, window.location.href);
Object.entries(params).forEach(([key, value]) => parsedURL.searchParams.set(key, value));
return parsedURL.toString();
}
22 changes: 22 additions & 0 deletions app/javascript/packages/verify-flow/context/flow-context.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { createContext } from 'react';

const FlowContext = createContext({
/**
* URL to path for session restart.
*/
startOverURL: '',

/**
* URL to path for session cancel.
*/
cancelURL: '',

/**
* Current step name.
*/
currentStep: '',
});

FlowContext.displayName = 'FlowContext';

export default FlowContext;
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ const SecretsContext = createContext({
setItems: (async () => {}) as SetItems,
});

SecretsContext.displayName = 'SecretsContext';

const pick = (obj: object, keys: string[]) =>
Object.fromEntries(keys.map((key) => [key, obj[key]]));

Expand Down
2 changes: 2 additions & 0 deletions app/javascript/packages/verify-flow/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
export { default as FlowContext } from './context/flow-context';
export { SecretsContextProvider } from './context/secrets-context';
export { default as StartOverOrCancel } from './start-over-or-cancel';
export { default as VerifyFlow } from './verify-flow';

export type { SecretValues } from './context/secrets-context';
Expand Down
Loading