Skip to content

Commit 4c786c0

Browse files
authored
fix(oauth): oauth_states query is now done before the WaitingForOAuth page renders (#322)
* fix(oauth): query the oauth_states before waiting for oauth page opens * docs: remove outdated comments * refactor(memberState): let the waiting component get the memberState directly * test(reducer): add tests to cover recent changes * refactor(actions): use existing action creator
1 parent ee92c2e commit 4c786c0

7 files changed

Lines changed: 84 additions & 43 deletions

File tree

src/redux/actions/Connect.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,9 +79,9 @@ export const startOauth = (member, institution) => ({
7979
payload: { member, institution },
8080
})
8181

82-
export const startOauthSuccess = (member, oauthWindowURI) => ({
82+
export const startOauthSuccess = (member, oauthWindowURI, memberState = null) => ({
8383
type: ActionTypes.START_OAUTH_SUCCESS,
84-
payload: { member, oauthWindowURI },
84+
payload: { member, oauthWindowURI, memberState },
8585
})
8686

8787
export const jobComplete = (member, job) => ({

src/redux/actions/__tests__/Connect-test.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ describe('Connect Dispatcher', () => {
7676
})
7777
expect(actions.startOauthSuccess({ guid: 'MBR-1' }, 'something.com')).toEqual({
7878
type: ActionTypes.START_OAUTH_SUCCESS,
79-
payload: { member: { guid: 'MBR-1' }, oauthWindowURI: 'something.com' },
79+
payload: { member: { guid: 'MBR-1' }, oauthWindowURI: 'something.com', memberState: null },
8080
})
8181
})
8282
})

src/redux/reducers/Connect.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ export const defaultState = {
3131
isConnectMounted: false,
3232
isOauthLoading: false, // whether or not the oauth process is starting
3333
oauthURL: null, // the URL to the oauth provider
34+
memberState: null, // pending oauth state for the active oauth member
3435
oauthErrorReason: null, // the reason there was an oauth error
3536
// whether or not there was an error *after* the user authenticated with
3637
// the provider, this means mx messed up after successful auth.
@@ -339,12 +340,14 @@ const startOauth = (state, action) => ({
339340
: STEPS.ENTER_CREDENTIALS,
340341
),
341342
currentMemberGuid: action.payload.member.guid,
343+
memberState: defaultState.memberState,
342344
selectedInstitution: action.payload.institution,
343345
})
344346
const startOauthSuccess = (state, action) => ({
345347
...state,
346348
currentMemberGuid: action.payload.member.guid,
347349
isOauthLoading: false,
350+
memberState: action.payload.memberState,
348351
members: upsertMember(state, { payload: action.payload.member }),
349352
oauthURL: action.payload.oauthWindowURI,
350353
})
@@ -359,12 +362,14 @@ const oauthError = (state, action) => ({
359362
...state,
360363
currentMemberGuid: action.payload.memberGuid,
361364
location: pushLocation(state.location, STEPS.OAUTH_ERROR),
365+
memberState: defaultState.memberState,
362366
oauthURL: defaultState.oauthURL,
363367
oauthErrorReason: action.payload.errorReason,
364368
})
365369
const retryOAuth = (state) => ({
366370
...state,
367371
location: popLocation(state),
372+
memberState: defaultState.memberState,
368373
oauthURL: defaultState.oauthURL,
369374
oauthErrorReason: defaultState.oauthErrorReason,
370375
})

src/redux/reducers/__tests__/Connect-test.js

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
loadConnectError,
1111
loadConnectSuccess,
1212
retryOAuth,
13+
startOauth,
1314
startOauthSuccess,
1415
stepToDeleteMemberSuccess,
1516
stepToMicrodeposits,
@@ -669,14 +670,30 @@ describe('Connect redux store', () => {
669670
})
670671

671672
describe('oauth actions', () => {
673+
it('should clear stale memberState with START_OAUTH', () => {
674+
const institution = { guid: 'INS-1', credentials }
675+
const beforeState = {
676+
...defaultState,
677+
memberState: { guid: 'OAS-STALE' },
678+
}
679+
680+
const afterState = reducer(beforeState, startOauth({ guid: 'MBR-1' }, institution))
681+
682+
expect(afterState.currentMemberGuid).toEqual('MBR-1')
683+
expect(afterState.selectedInstitution).toEqual(institution)
684+
expect(afterState.memberState).toEqual(defaultState.memberState)
685+
})
686+
672687
it('should finish the loader with START_OAUTH_SUCCESS', () => {
688+
const memberState = { guid: 'OAS-1' }
673689
const afterState = reducer(
674690
{ ...defaultState, isOauthLoading: true },
675-
startOauthSuccess({ guid: 'MBR-1' }, 'something.com'),
691+
startOauthSuccess({ guid: 'MBR-1' }, 'something.com', memberState),
676692
)
677693

678694
expect(afterState.isOauthLoading).toBe(false)
679695
expect(afterState.oauthURL).toBe('something.com')
696+
expect(afterState.memberState).toEqual(memberState)
680697
})
681698

682699
it('should set some oauth error state when OAUTH_COMPLETE_ERROR happens', () => {
@@ -694,6 +711,7 @@ describe('Connect redux store', () => {
694711
expect(afterState.location[afterState.location.length - 1].step).toEqual(STEPS.OAUTH_ERROR)
695712
expect(afterState.oauthURL).toEqual(null)
696713
expect(afterState.oauthErrorReason).toEqual(OAUTH_ERROR_REASONS.CANCELLED)
714+
expect(afterState.memberState).toEqual(defaultState.memberState)
697715
})
698716
})
699717

@@ -940,6 +958,7 @@ describe('Connect redux store', () => {
940958
const beforeState = {
941959
...defaultState,
942960
oauthURL: 'something.com',
961+
memberState: { guid: 'OAS-123' },
943962
location: [
944963
{ step: STEPS.SEARCH },
945964
{ step: STEPS.ENTER_CREDENTIALS },
@@ -953,6 +972,7 @@ describe('Connect redux store', () => {
953972
STEPS.ENTER_CREDENTIALS,
954973
)
955974
expect(afterState.oauthURL).toEqual(defaultState.oauthURL)
975+
expect(afterState.memberState).toEqual(defaultState.memberState)
956976
})
957977

958978
it('should clear the OAuth error and reason if we are in the oauth error step', () => {

src/views/oauth/OAuthStep.js

Lines changed: 40 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import { scrollToTop } from 'src/utilities/ScrollToTop'
2626

2727
import { DisclosureInterstitial } from 'src/views/disclosure/Interstitial'
2828
import { AnalyticEvents } from 'src/const/Analytics'
29+
import { OauthState } from 'src/const/consts'
2930
import useAnalyticsEvent from 'src/hooks/useAnalyticsEvent'
3031
import { PostMessageContext } from 'src/ConnectWidget'
3132

@@ -86,13 +87,10 @@ export const OAuthStep = React.forwardRef((props, navigationRef) => {
8687
* Called when we succesfully generate an oauth window uri for the exsting
8788
* member, or use the uri from a brand new member.
8889
*/
89-
function onStartOAuthSuccess(member, oauthWindowURI) {
90+
function onStartOAuthSuccess(member, oauthWindowURI, memberState) {
9091
setIsStartingOauth(false)
9192
setOAuthStartError(null)
92-
dispatch({
93-
type: connectActions.ActionTypes.START_OAUTH_SUCCESS,
94-
payload: { member, oauthWindowURI },
95-
})
93+
dispatch(connectActions.startOauthSuccess(member, oauthWindowURI, memberState))
9694
}
9795

9896
/**
@@ -120,6 +118,14 @@ export const OAuthStep = React.forwardRef((props, navigationRef) => {
120118
useEffect(() => {
121119
if (!isStartingOauth) return () => {}
122120

121+
const loadPendingOAuthState = (memberGuid) =>
122+
defer(() =>
123+
api.loadOAuthStates({
124+
outbound_member_guid: memberGuid,
125+
auth_status: OauthState.AuthStatus.PENDING,
126+
}),
127+
).pipe(map((states) => states?.[0]))
128+
123129
let member$
124130

125131
/**
@@ -152,14 +158,25 @@ export const OAuthStep = React.forwardRef((props, navigationRef) => {
152158
config,
153159
),
154160
)
155-
.pipe(map((resp) => resp.member))
161+
.pipe(
162+
map((resp) => resp.member),
163+
mergeMap((newMember) =>
164+
loadPendingOAuthState(newMember.guid).pipe(
165+
map((newMemberState) => ({
166+
member: newMember,
167+
oauthWindowURI: newMember.oauth_window_uri,
168+
memberState: newMemberState,
169+
})),
170+
),
171+
),
172+
)
156173
.subscribe(
157-
(member) => {
174+
({ member, oauthWindowURI, memberState }) => {
158175
sendAnalyticsEvent(AnalyticEvents.OAUTH_PENDING_MEMBER_CREATED, {
159176
institution_guid: institution.guid,
160177
institution_name: institution.name,
161178
})
162-
onStartOAuthSuccess(member, member.oauth_window_uri)
179+
onStartOAuthSuccess(member, oauthWindowURI, memberState)
163180
},
164181
(err) => onStartOAuthFail(err),
165182
)
@@ -171,12 +188,25 @@ export const OAuthStep = React.forwardRef((props, navigationRef) => {
171188
.pipe(
172189
mergeMap((existingMember) =>
173190
defer(() => api.getOAuthWindowURI(existingMember.guid, config)).pipe(
174-
map(({ oauth_window_uri }) => [existingMember, oauth_window_uri]),
191+
map(({ oauth_window_uri }) => ({
192+
member: existingMember,
193+
oauthWindowURI: oauth_window_uri,
194+
})),
195+
),
196+
),
197+
mergeMap(({ member, oauthWindowURI }) =>
198+
loadPendingOAuthState(member.guid).pipe(
199+
map((newMemberState) => ({
200+
member,
201+
oauthWindowURI,
202+
memberState: newMemberState,
203+
})),
175204
),
176205
),
177206
)
178207
.subscribe(
179-
([member, oauthWindowURI]) => onStartOAuthSuccess(member, oauthWindowURI),
208+
({ member, oauthWindowURI, memberState }) =>
209+
onStartOAuthSuccess(member, oauthWindowURI, memberState),
180210
(err) => onStartOAuthFail(err),
181211
)
182212

src/views/oauth/WaitingForOAuth.js

Lines changed: 4 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import React, { useState, useEffect } from 'react'
22
import PropTypes from 'prop-types'
3+
import { useSelector } from 'react-redux'
34
import { of, defer } from 'rxjs'
45
import { map, mergeMap, first, filter, catchError } from 'rxjs/operators'
56

@@ -32,6 +33,7 @@ export const WaitingForOAuth = ({
3233
institution_name: institution.name,
3334
})
3435

36+
const memberState = useSelector((state) => state.connect.memberState)
3537
const sendAnalyticsEvent = useAnalyticsEvent()
3638
const [disableOauthButtons, setDisableOauthButtons] = useState(true)
3739
const tokens = useTokens()
@@ -42,32 +44,8 @@ export const WaitingForOAuth = ({
4244
const clientLocale = document.querySelector('html')?.getAttribute('lang') || 'en'
4345

4446
useEffect(() => {
45-
/**
46-
* This gets the most recent PENDING oauth state for the member and polls that
47-
* state until it goes from PENDING to ERRORED or SUCCESS.
48-
*
49-
* NOTE: the pause and retreival of the most recent oauth state is a little
50-
* weird. Ideally we would know which oauth state to poll for ahead of time,
51-
* but we don't since it is created when the user visits the oauth url.
52-
*
53-
* Once that is refactored in this issue:
54-
* https://gitlab.mx.com/mx/connectivity/connect/connect-issues/-/issues/1836
55-
*
56-
* We could potentially have the member create and oauth uri endpoints return
57-
* the oauth state created and know which oauth state to retreive ahead of time.
58-
*/
59-
const oauthStateCompleted$ = of(outboundMember).pipe(
60-
mergeMap(() =>
61-
defer(() =>
62-
api.loadOAuthStates({
63-
outbound_member_guid: outboundMember.guid,
64-
auth_status: OauthState.AuthStatus.PENDING,
65-
}),
66-
).pipe(
67-
map((states) => states?.[0]),
68-
catchError(() => of(null)),
69-
),
70-
),
47+
// This polls the member state until it goes from PENDING to ERRORED or SUCCESS.
48+
const oauthStateCompleted$ = of(memberState).pipe(
7149
filter((latestState) => !!latestState),
7250
mergeMap((latestState) => pollOauthState(latestState.guid, api)),
7351
mergeMap((pollingState) => {

src/views/oauth/__tests__/WaitingForOAuth-test.tsx

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,14 @@ describe('WaitingForOAuth view', () => {
1717
outboundMember: { guid: 'MBR-123' },
1818
}
1919

20+
const preloadedState = {
21+
connect: {
22+
memberState: { ...OAUTH_STATE.oauth_state, guid: 'OAS-123' },
23+
},
24+
}
25+
2026
it('should disable the buttons when the component loads', () => {
21-
render(<WaitingForOAuth {...defaultProps} />)
27+
render(<WaitingForOAuth {...defaultProps} />, { preloadedState })
2228
const tryAgainButton = screen.getByRole('button', { name: 'Try again' })
2329
expect(
2430
screen.getByText(
@@ -32,7 +38,7 @@ describe('WaitingForOAuth view', () => {
3238
})
3339

3440
it('should enable the tryAgain button after 2 seconds and call onOAuthRetry when clicked ', async () => {
35-
const { user } = render(<WaitingForOAuth {...defaultProps} />)
41+
const { user } = render(<WaitingForOAuth {...defaultProps} />, { preloadedState })
3642
const tryAgainButton = await screen.findByRole('button', { name: 'Try again' })
3743
await waitFor(
3844
async () => {
@@ -45,7 +51,7 @@ describe('WaitingForOAuth view', () => {
4551
})
4652

4753
it('should call onOAuthSuccess if polling an oauth state was successful', async () => {
48-
render(<WaitingForOAuth {...defaultProps} />)
54+
render(<WaitingForOAuth {...defaultProps} />, { preloadedState })
4955
await waitFor(
5056
async () => {
5157
expect(defaultProps.onOAuthSuccess).toHaveBeenCalledTimes(1)
@@ -61,6 +67,7 @@ describe('WaitingForOAuth view', () => {
6167
<ApiProvider apiValue={{ ...apiValue, loadOAuthState }}>
6268
<WaitingForOAuth {...defaultProps} />
6369
</ApiProvider>,
70+
{ preloadedState },
6471
)
6572
await waitFor(
6673
async () => {
@@ -85,6 +92,7 @@ describe('WaitingForOAuth view', () => {
8592
<ApiProvider apiValue={{ ...apiValue, loadOAuthState }}>
8693
<WaitingForOAuth {...defaultProps} />
8794
</ApiProvider>,
95+
{ preloadedState },
8896
)
8997

9098
await waitFor(

0 commit comments

Comments
 (0)