Skip to content

Add lint hardcoded URLs in JavaScript #10894

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.

Sign up for GitHub

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

Merged
mitchellhenke merged 6 commits into from
Jul 1, 2024
Merged

Add lint hardcoded URLs in JavaScript #10894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2acfec5
Lint hard-coded URLs in JavaScript
aduth Jun 25, 2024
9c61ead
session timeout
Jul 1, 2024
22c7f09
address search URL
Jul 1, 2024
c877083
Update app/javascript/packages/session/requests.ts
Jul 1, 2024
e115d26
Update app/javascript/packs/session-timeout-ping.ts
Jul 1, 2024
20068e3
rename variables
Jul 1, 2024
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
5 changes: 3 additions & 2 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
"no-restricted-syntax": [
"error",
{
"selector": "AssignmentExpression[left.property.name='href'][right.type=/(Template)?Literal/]",
"message": "Do not assign window.location.href to a string or string template to avoid losing i18n parameters"
"selector": "AssignmentExpression[left.property.name='href'][right.type=/(Template)?Literal/],NewExpression[callee.name=URL][arguments.0.type=/(Template)?Literal/]",
"message": "Avoid hard-coded string URLs, since they will not include the current locale"
}
],
"react-hooks/exhaustive-deps": "error"
Expand All @@ -42,6 +42,7 @@
"packageDir": "."
}
],
"no-restricted-syntax": "off",
"testing-library/await-async-events": "error",
"testing-library/await-async-queries": "error",
"testing-library/await-async-utils": "error",
Expand Down
18 changes: 10 additions & 8 deletions app/javascript/packages/session/requests.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import { http, HttpResponse } from 'msw';
import { setupServer } from 'msw/node';
import type { SetupServer } from 'msw/node';
import { SESSIONS_URL, requestSessionStatus, extendSession } from './requests';
import { requestSessionStatus, extendSession } from './requests';
import type { SessionLiveStatusResponse, SessionTimedOutStatusResponse } from './requests';

const SESSIONS_URL = '/api/internal/sessions';

describe('requestSessionStatus', () => {
let server: SetupServer;

Expand All @@ -22,7 +24,7 @@ describe('requestSessionStatus', () => {
});

it('resolves to the status', async () => {
const result = await requestSessionStatus();
const result = await requestSessionStatus(SESSIONS_URL);

expect(result).to.deep.equal({ isLive: false });
});
Expand All @@ -46,7 +48,7 @@ describe('requestSessionStatus', () => {
});

it('resolves to the status', async () => {
const result = await requestSessionStatus();
const result = await requestSessionStatus(SESSIONS_URL);

expect(result).to.deep.equal({ isLive: true, timeout: new Date(timeout) });
});
Expand All @@ -65,7 +67,7 @@ describe('requestSessionStatus', () => {
});

it('resolves to the status', async () => {
const result = await requestSessionStatus();
const result = await requestSessionStatus(SESSIONS_URL);

expect(result).to.deep.equal({ isLive: false });
});
Expand All @@ -84,7 +86,7 @@ describe('requestSessionStatus', () => {
});

it('throws an error', async () => {
await expect(requestSessionStatus()).to.be.rejected();
await expect(requestSessionStatus(SESSIONS_URL)).to.be.rejected();
});
});
});
Expand All @@ -109,7 +111,7 @@ describe('extendSession', () => {
});

it('resolves to the status', async () => {
const result = await extendSession();
const result = await extendSession(SESSIONS_URL);

expect(result).to.deep.equal({ isLive: true, timeout: new Date(timeout) });
});
Expand All @@ -128,7 +130,7 @@ describe('extendSession', () => {
});

it('resolves to the status', async () => {
const result = await extendSession();
const result = await extendSession(SESSIONS_URL);

expect(result).to.deep.equal({ isLive: false });
});
Expand All @@ -147,7 +149,7 @@ describe('extendSession', () => {
});

it('throws an error', async () => {
await expect(extendSession()).to.be.rejected();
await expect(extendSession(SESSIONS_URL)).to.be.rejected();
});
});
});
12 changes: 6 additions & 6 deletions app/javascript/packages/session/requests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,6 @@ interface SessionTimedOutStatus {

export type SessionStatus = SessionLiveStatus | SessionTimedOutStatus;

export const SESSIONS_URL = new URL('/api/internal/sessions', window.location.href).toString();
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Jul 1, 2024

Choose a reason for hiding this comment

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

API endpoints are more consistent than the ones that have locales in them, so it could have been fine if we wanted right? This is just for consistency?

Copy link
Copy Markdown
Contributor Author

@mitchellhenke mitchellhenke Jul 1, 2024

Choose a reason for hiding this comment

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

The session endpoints don't include any user-facing language currently so there aren't any issues right now, so passing in the URL is just for the localization. The other upside is we get path validation with Rails.

I"m not sure I understand what you mean by API endpoints being more consistent.

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Jul 1, 2024

Choose a reason for hiding this comment

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

consistent meaning they don't change based on locale param

Copy link
Copy Markdown
Contributor Author

@mitchellhenke mitchellhenke Jul 1, 2024

Choose a reason for hiding this comment

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

ah, got it. I think generally, yes, but there are cases like the country support endpoint that return translated content depending on the requested locale.

Copy link
Copy Markdown
Contributor

@aduth aduth Jul 1, 2024

Choose a reason for hiding this comment

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

I had originally knowingly implemented this without the path helper based on the original comment that we don't need the endpoint to be localized, but agree it's nice to use path helpers for validation and having a single source of truth for routes. And avoids having to create an exception.


function mapSessionStatusResponse<R extends SessionLiveStatusResponse>(
response: R,
): SessionLiveStatus;
Expand Down Expand Up @@ -83,20 +81,22 @@ function handleUnauthorizedStatusResponse(error: ResponseError) {
/**
* Request the current session status. Returns a promise resolving to the current session status.
*
* @param sessionsURL The URL for the session API
* @return A promise resolving to the current session status
*/
export const requestSessionStatus = (): Promise<SessionStatus> =>
request<SessionStatusResponse>(SESSIONS_URL)
export const requestSessionStatus = (sessionsURL: string): Promise<SessionStatus> =>
request<SessionStatusResponse>(sessionsURL)
.catch(handleUnauthorizedStatusResponse)
.then(mapSessionStatusResponse);

/**
* Request that the current session be kept alive. Returns a promise resolving to the updated
* session status.
*
* @param sessionsURL The URL for the session API
* @return A promise resolving to the updated session status.
*/
export const extendSession = (): Promise<SessionStatus> =>
request<SessionStatusResponse>(SESSIONS_URL, { method: 'PUT' })
export const extendSession = (sessionsURL: string): Promise<SessionStatus> =>
request<SessionStatusResponse>(sessionsURL, { method: 'PUT' })
.catch(handleUnauthorizedStatusResponse)
.then(mapSessionStatusResponse);
4 changes: 3 additions & 1 deletion app/javascript/packs/document-capture.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ interface AppRootData {
previousStepUrl: string;
docAuthSelfieDesktopTestMode: string;
locationsUrl: string;
addressSearchUrl: string;
}

const appRoot = document.getElementById('document-capture-form')!;
Expand Down Expand Up @@ -109,6 +110,7 @@ const {
previousStepUrl,
docAuthSelfieDesktopTestMode,
locationsUrl: locationsURL,
addressSearchUrl: addressSearchURL,
} = appRoot.dataset as DOMStringMap & AppRootData;

let parsedUsStatesTerritories = [];
Expand All @@ -125,7 +127,7 @@ const App = composeComponents(
value: {
inPersonURL,
locationsURL,
addressSearchURL: new URL('/api/addresses', window.location.href).toString(),
addressSearchURL,
inPersonOutageMessageEnabled: inPersonOutageMessageEnabled === 'true',
inPersonOutageExpectedUpdateDate,
inPersonFullAddressEntryEnabled: inPersonFullAddressEntryEnabled === 'true',
Expand Down
11 changes: 6 additions & 5 deletions app/javascript/packs/session-timeout-ping.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,17 @@ const defaultTime = '60';
const frequency = parseInt(warningEl?.dataset.frequency || defaultTime, 10) * 1000;
const warning = parseInt(warningEl?.dataset.warning || defaultTime, 10) * 1000;
const start = parseInt(warningEl?.dataset.start || defaultTime, 10) * 1000;
const timeoutUrl = warningEl?.dataset.timeoutUrl!;
const timeoutURL = warningEl?.dataset.timeoutUrl!;
const sessionsURL = warningEl?.dataset.sessionsUrl!;

const modal = document.querySelector<ModalElement>('lg-modal.session-timeout-modal')!;
const keepaliveEl = document.getElementById('session-keepalive-btn');
const countdownEls: NodeListOf<CountdownElement> = modal.querySelectorAll('lg-countdown');

function success({ isLive, timeout }: SessionStatus) {
if (!isLive) {
if (timeoutUrl) {
forceRedirect(timeoutUrl);
if (timeoutURL) {
forceRedirect(timeoutURL);
}
return;
}
Expand All @@ -43,12 +44,12 @@ function success({ isLive, timeout }: SessionStatus) {
setTimeout(ping, nextPingTimeout);
}

const ping = () => requestSessionStatus().then(success);
const ping = () => requestSessionStatus(sessionsURL).then(success);

function keepalive() {
modal.hide();
countdownEls.forEach((countdownEl) => countdownEl.stop());
extendSession();
extendSession(sessionsURL);
}

keepaliveEl?.addEventListener('click', keepalive, false);
Expand Down
1 change: 1 addition & 0 deletions app/views/idv/shared/_document_capture.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
how_to_verify_url: idv_how_to_verify_url,
previous_step_url: @previous_step_url,
locations_url: idv_in_person_usps_locations_url,
address_search_url: '',
Comment thread
zachmargolis marked this conversation as resolved.
} %>
<%= simple_form_for(
:doc_auth,
Expand Down
1 change: 1 addition & 0 deletions app/views/session_timeout/_ping.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<%= tag.div id: 'session-timeout-cntnr',
data: {
timeout_url: new_user_session_url(timeout: :session, request_id: sp_session[:request_id]),
sessions_url: api_internal_sessions_path,
warning: warning,
start: start,
frequency: frequency,
Expand Down