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
14 changes: 9 additions & 5 deletions app/controllers/idv/in_person/usps_locations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ class UspsLocationsController < ApplicationController
# retrieve the list of nearby IPP Post Office locations with a POST request
def index
response = []

begin
if IdentityConfig.store.arcgis_search_enabled
candidate = UspsInPersonProofing::Applicant.new(
Expand All @@ -26,12 +25,17 @@ def index
else
response = proofer.request_pilot_facilities
end
render json: response.to_json
rescue => err
Rails.logger.warn(err)
response = proofer.request_pilot_facilities
analytics.idv_in_person_locations_request_failure(
exception_class: err.class,
exception_message: err.message,
response_body_present: err.respond_to?(:response_body) && err.response_body.present?,
response_body: err.respond_to?(:response_body) && err.response_body,
response_status_code: err.respond_to?(:response_status) && err.response_status,
)
render json: {}, status: :internal_server_error
end

render json: response.to_json
end

def proofer
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import userEvent from '@testing-library/user-event';
import { setupServer } from 'msw/node';
import { rest } from 'msw';
import type { SetupServerApi } from 'msw/node';
import { SWRConfig } from 'swr';
import AddressSearch, { ADDRESS_SEARCH_URL, LOCATIONS_URL } from './address-search';

const DEFAULT_RESPONSE = [
Expand Down Expand Up @@ -40,10 +41,12 @@ describe('AddressSearch', () => {
const handleAddressFound = sandbox.stub();
const handleLocationsFound = sandbox.stub();
const { findByText, findByLabelText } = render(
<AddressSearch
onFoundAddress={handleAddressFound}
onFoundLocations={handleLocationsFound}
/>,
<SWRConfig value={{ provider: () => new Map() }}>
<AddressSearch
onFoundAddress={handleAddressFound}
onFoundLocations={handleLocationsFound}
/>
</SWRConfig>,
);

await userEvent.type(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,15 +139,18 @@ function useUspsLocations() {
}
}, [addressCandidates]);

const { data: locationResults, isLoading: isLoadingLocations } = useSWR(
[foundAddress],
([address]) => (address ? requestUspsLocations(address) : null),
{ keepPreviousData: true },
);
const {
data: locationResults,
isLoading: isLoadingLocations,
error,
Comment thread
eileen-nava marked this conversation as resolved.
} = useSWR([foundAddress], ([address]) => (address ? requestUspsLocations(address) : null), {
keepPreviousData: true,
});

return {
foundAddress,
locationResults,
error,
Comment thread
eileen-nava marked this conversation as resolved.
isLoading: isLoadingLocations || isLoadingCandidates,
handleAddressSearch,
validatedFieldRef,
Expand All @@ -158,18 +161,21 @@ interface AddressSearchProps {
registerField?: RegisterFieldCallback;
onFoundAddress?: (address: LocationQuery | null) => void;
onFoundLocations?: (locations: FormattedLocation[] | null | undefined) => void;
onError?: (error: Error | null) => void;
}

function AddressSearch({
registerField = () => undefined,
onFoundAddress = () => undefined,
onFoundLocations = () => undefined,
onError = () => undefined,
}: AddressSearchProps) {
const { t } = useI18n();
const spinnerButtonRef = useRef<SpinnerButtonRefHandle>(null);
const [textInput, setTextInput] = useState('');
const {
locationResults,
error,
isLoading,
handleAddressSearch: onSearch,
foundAddress,
Expand All @@ -185,6 +191,10 @@ function AddressSearch({
spinnerButtonRef.current?.toggleSpinner(isLoading);
}, [isLoading]);

useEffect(() => {
error && onError(error);
}, [error]);

useDidUpdateEffect(() => {
onFoundLocations(locationResults);

Expand All @@ -193,6 +203,7 @@ function AddressSearch({

const handleSearch = useCallback(
(event) => {
onError(null);
onFoundAddress(null);
onSearch(event, textInput);
},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
import { render } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { setupServer } from 'msw/node';
import { rest } from 'msw';
import type { SetupServerApi } from 'msw/node';
import { LOCATIONS_URL } from './in-person-location-step';
import { ADDRESS_SEARCH_URL } from './address-search';
import InPersonContext from '../context/in-person';
import InPersonLocationPostOfficeSearchStep from './in-person-location-post-office-search-step';

const DEFAULT_RESPONSE = [
{
address: '100 Main St E, Bronwood, Georgia, 39826',
location: {
latitude: 31.831686000000005,
longitude: -84.363768,
},
street_address: '100 Main St E',
city: 'Bronwood',
state: 'GA',
zip_code: '39826',
},
];

const DEFAULT_PROPS = {
toPreviousStep() {},
onChange() {},
value: {},
registerField() {},
};

describe('InPersonLocationStep', () => {
context('initial API request throws an error', () => {
let server: SetupServerApi;
beforeEach(() => {
server = setupServer(
rest.post(ADDRESS_SEARCH_URL, (_req, res, ctx) => res(ctx.json(DEFAULT_RESPONSE))),
rest.post(LOCATIONS_URL, (_req, res, ctx) => res(ctx.status(500))),
);
server.listen();
});

afterEach(() => {
server.close();
});

it('displays a 500 error if the request to the USPS API throws an error', async () => {
const { findByText, findByLabelText } = render(
<InPersonContext.Provider value={{ arcgisSearchEnabled: true }}>
<InPersonLocationPostOfficeSearchStep {...DEFAULT_PROPS} />
</InPersonContext.Provider>,
);

await userEvent.type(
await findByLabelText('in_person_proofing.body.location.po_search.address_search_label'),
'222 Merchandise Mart Plaza',
);

await userEvent.click(
await findByText('in_person_proofing.body.location.po_search.search_button'),
);

const error = await findByText('idv.failure.exceptions.internal_error');
expect(error).to.exist();
});
});

context('initial API request is successful', () => {
let server: SetupServerApi;
beforeEach(() => {
server = setupServer(
rest.post(LOCATIONS_URL, (_req, res, ctx) => res(ctx.json([{ name: 'Baltimore' }]))),
rest.post(ADDRESS_SEARCH_URL, (_req, res, ctx) => res(ctx.json(DEFAULT_RESPONSE))),
);
server.listen();
});

afterEach(() => {
server.close();
});

it('allows search by address when enabled', async () => {
const { findByText, findByLabelText } = render(
<InPersonContext.Provider value={{ arcgisSearchEnabled: true }}>
<InPersonLocationPostOfficeSearchStep {...DEFAULT_PROPS} />
</InPersonContext.Provider>,
);

await userEvent.type(
await findByLabelText('in_person_proofing.body.location.po_search.address_search_label'),
'100 main',
);
await userEvent.click(
await findByText('in_person_proofing.body.location.po_search.search_button'),
);
await findByText('in_person_proofing.body.location.po_search.results_description');
});

it('validates input and shows inline error', async () => {
const { findByText } = render(
<InPersonContext.Provider value={{ arcgisSearchEnabled: true }}>
<InPersonLocationPostOfficeSearchStep {...DEFAULT_PROPS} />
</InPersonContext.Provider>,
);

await userEvent.click(
await findByText('in_person_proofing.body.location.po_search.search_button'),
);

await findByText('in_person_proofing.body.location.inline_error');
});

it('displays no post office results if a successful search is followed by an unsuccessful search', async () => {
const { findByText, findByLabelText, queryByRole } = render(
<InPersonContext.Provider value={{ arcgisSearchEnabled: true }}>
<InPersonLocationPostOfficeSearchStep {...DEFAULT_PROPS} />
</InPersonContext.Provider>,
);

await userEvent.type(
await findByLabelText('in_person_proofing.body.location.po_search.address_search_label'),
'594 Broadway New York',
);
await userEvent.click(
await findByText('in_person_proofing.body.location.po_search.search_button'),
);

await userEvent.type(
await findByLabelText('in_person_proofing.body.location.po_search.address_search_label'),
'asdfkf',
);
await userEvent.click(
await findByText('in_person_proofing.body.location.po_search.search_button'),
);

const results = queryByRole('status', {
name: 'in_person_proofing.body.location.po_search.results_description',
});
expect(results).not.to.exist();
});
Comment thread
eileen-nava marked this conversation as resolved.
});
});
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { useState, useEffect, useCallback, useRef, useContext } from 'react';
import { useI18n } from '@18f/identity-react-i18n';
import { PageHeading } from '@18f/identity-components';
import { Alert, PageHeading } from '@18f/identity-components';
import { request } from '@18f/identity-request';
import BackButton from './back-button';
import AnalyticsContext from '../context/analytics';
Expand All @@ -21,6 +21,7 @@ function InPersonLocationPostOfficeSearchStep({ onChange, toPreviousStep, regist
null,
);
const [foundAddress, setFoundAddress] = useState<LocationQuery | null>(null);
const [uspsError, setUspsError] = useState<Error | null>(null);

// ref allows us to avoid a memory leak
const mountedRef = useRef(false);
Expand Down Expand Up @@ -77,12 +78,18 @@ function InPersonLocationPostOfficeSearchStep({ onChange, toPreviousStep, regist

return (
<>
{uspsError && (
<Alert type="error" className="margin-bottom-4">
{t('idv.failure.exceptions.internal_error')}
</Alert>
)}
Comment thread
eileen-nava marked this conversation as resolved.
<PageHeading>{t('in_person_proofing.headings.po_search.location')}</PageHeading>
<p>{t('in_person_proofing.body.location.po_search.po_search_about')}</p>
<AddressSearch
registerField={registerField}
onFoundAddress={setFoundAddress}
onFoundLocations={setLocationResults}
onError={setUspsError}
/>
{locationResults && foundAddress && (
<InPersonLocations
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@ import { rest } from 'msw';
import type { SetupServerApi } from 'msw/node';
import AnalyticsContext, { AnalyticsContextProvider } from '../context/analytics';
import InPersonLocationStep, { LOCATIONS_URL } from './in-person-location-step';
import InPersonLocationPostOfficeSearchStep from './in-person-location-post-office-search-step';
import { ADDRESS_SEARCH_URL } from './address-search';
import InPersonContext from '../context/in-person';

const DEFAULT_RESPONSE = [
{
Expand Down Expand Up @@ -67,64 +65,4 @@ describe('InPersonLocationStep', () => {

await findByText('{"selected_location":"Baltimore"}');
});

it('allows search by address when enabled', async () => {
const { findByText, findByLabelText } = render(
<InPersonContext.Provider value={{ arcgisSearchEnabled: true }}>
<InPersonLocationPostOfficeSearchStep {...DEFAULT_PROPS} />
</InPersonContext.Provider>,
);

await userEvent.type(
await findByLabelText('in_person_proofing.body.location.po_search.address_search_label'),
'100 main',
);
await userEvent.click(
await findByText('in_person_proofing.body.location.po_search.search_button'),
);
await findByText('in_person_proofing.body.location.po_search.results_description');
});

it('validates input and shows inline error', async () => {
const { findByText } = render(
<InPersonContext.Provider value={{ arcgisSearchEnabled: true }}>
<InPersonLocationPostOfficeSearchStep {...DEFAULT_PROPS} />
</InPersonContext.Provider>,
);

await userEvent.click(
await findByText('in_person_proofing.body.location.po_search.search_button'),
);

await findByText('in_person_proofing.body.location.inline_error');
});

it('displays no post office results if a successful search is followed by an unsuccessful search', async () => {
const { findByText, findByLabelText, queryByRole } = render(
<InPersonContext.Provider value={{ arcgisSearchEnabled: true }}>
<InPersonLocationPostOfficeSearchStep {...DEFAULT_PROPS} />
</InPersonContext.Provider>,
);

await userEvent.type(
await findByLabelText('in_person_proofing.body.location.po_search.address_search_label'),
'594 Broadway New York',
);
await userEvent.click(
await findByText('in_person_proofing.body.location.po_search.search_button'),
);

await userEvent.type(
await findByLabelText('in_person_proofing.body.location.po_search.address_search_label'),
'asdfkf',
);
await userEvent.click(
await findByText('in_person_proofing.body.location.po_search.search_button'),
);

const results = queryByRole('status', {
name: 'in_person_proofing.body.location.po_search.results_description',
});
expect(results).not.to.exist();
});
});
6 changes: 5 additions & 1 deletion app/javascript/packages/request/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,9 @@ export async function request<Response>(
}

const response = await window.fetch(url, { ...fetchOptions, headers, body });
return json ? response.json() : response.text();
if (response.ok) {
return json ? response.json() : response.text();
}

throw new Error(await response.json());
Comment on lines +44 to +48
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.

In order to use SWR's error feature, we need to have the promise reject.

Comment on lines +44 to +48
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.

It'd be good to have spec coverage for the new behaviors here:

  • Non-200 status code responses throw
  • What happens if the response isn't JSON? I might wonder if it'd be better to avoid using JSON as the error text. Also worth noting that if uncaught, these would be logged to NewRelic, and depending on what's returned from the endpoint JSON, it could include sensitive details we wouldn't want logged (not a problem in this case)

Copy link
Copy Markdown
Contributor

@allthesignals allthesignals Jan 26, 2023

Choose a reason for hiding this comment

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

@aduth thanks! I'll open a ticket for added coverage.

What happens if the response isn't JSON? I might wonder if it'd be better to avoid using JSON as the error text.

What's the standard approach to this? Is it better to assume text because other errors return as text? Some of it is API design — should our API return JSON error bodies? Or text? Etc.

Also, re: NewRelic, do you mean... the .json() parse attempt would fail, uncaught, and log potentially sensitive erro text ("Could not parse token someone's special info\ ")?

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.

What's the standard approach to this? Is it better to assume text because other errors return as text? Some of it is API design — should our API return JSON error bodies? Or text? Etc.

I guess my first question is what details do we want from what's thrown? It doesn't look like we really care what the detail is at the moment, since we're not using it. I think we could even go as far as removing the parameter altogether. But otherwise I think it could be fine to use a general text that's descriptive enough of what happened (e.g. something like 'Request response status was unsuccessful' 🤷 )

Also, re: NewRelic, do you mean... the .json() parse attempt would fail, uncaught, and log potentially sensitive erro text ("Could not parse token someone's special info\ ")?

I mean if the API responds with anything other than 200 status code, but sent a JSON payload like...

{ ssn: '666-12-1234' }

...we'd risk logging that in NewRelic.

I think it can also be helpful to assign a generic name for the error so that we can aggregate it more easily (i.e. otherwise how would we know how often this line of code is throwing?).

}
Loading