Skip to content

[ISSUE-169] fix: us link account form #487

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
merged 2 commits into from
Oct 25, 2024
Merged

[ISSUE-169] fix: us link account form #487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
07f84b5
fix(link-account): clear errors on account details
jjramirezn Oct 25, 2024
2af999c
refactor(cashout): better error handling for create external account
jjramirezn Oct 25, 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
53 changes: 31 additions & 22 deletions src/app/api/bridge/external-account/create-external-account/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,31 +65,40 @@ export async function POST(request: NextRequest) {
body: JSON.stringify(body),
})

const data = await response.json()

if (!response.ok) {
if (data.code && data.code == 'duplicate_external_account') {
// in case the bridge account already exists
//
// sending back error responses is not currently common across app
// in how we deliver errors from backend -> frontend
// sends back an object like:
// {
// id: '4c537540-80bf-41dd-a528-dbe39a4',
// code: 'duplicate_external_account',
// message: 'An external account with the same information has already been added for this customer'
// }
//
// TODO: standardize error responses across backend
return new NextResponse(JSON.stringify(data), {
status: response.status,
headers: {
'Content-Type': 'application/json',
},
})
try {
const data = await response.json()
if (data.code && data.code == 'duplicate_external_account') {
// in case the bridge account already exists
//
// sending back error responses is not currently common across app
// in how we deliver errors from backend -> frontend
// sends back an object like:
// {
// id: '4c537540-80bf-41dd-a528-dbe39a4',
// code: 'duplicate_external_account',
// message: 'An external account with the same information has already been added for this customer'
// }
//
// TODO: standardize error responses across backend
return new NextResponse(JSON.stringify(data), {
status: response.status,
headers: {
'Content-Type': 'application/json',
},
})
}
} catch (e) {
console.error('Error creating external account', e)
}
Comment on lines +69 to 93
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling to provide more informative responses.

While the try-catch block for JSON parsing is good, the error handling could be more informative:

  1. The catch block's error logging could include the response status and URL
  2. Consider adding type safety for the error response structure
 try {
     const data = await response.json()
+    // Add type safety for error response
+    type BridgeError = {
+        id: string;
+        code: string;
+        message: string;
+    }
+    const errorData = data as BridgeError;
-    if (data.code && data.code == 'duplicate_external_account') {
+    if (errorData.code && errorData.code === 'duplicate_external_account') {
         return new NextResponse(JSON.stringify(data), {
             status: response.status,
             headers: {
                 'Content-Type': 'application/json',
             },
         })
     }
 } catch (e) {
-    console.error('Error creating external account', e)
+    console.error(`Error creating external account (status: ${response.status})`, e)
 }

Committable suggestion was skipped due to low confidence.

throw new Error(`HTTP error! status: ${response.status}`)
return new NextResponse('', {
Copy link
Contributor

@panosfilianos panosfilianos Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

question (non-blocking): is an empty data field here enough to handle all errors to function it returns it too? wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

as an additional note that's visible in the TODOs in these diffs: we need a std way to handle these errors and propagate that error state in the whole UI. I'm currently creating a prototype of what a solution could look like in peanut-wallet.

tl;dr: don't waste too much time on this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the empty data field is not enough and I love that you are doing something about the error state, it is needed

Copy link
Contributor

Choose a reason for hiding this comment

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

cool!
this is not the scope to fix that and I have a Task and TODOs around that.

status: response.status,
headers: {
'Content-Type': 'application/json',
},
})
Comment on lines +94 to +99
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider preserving error details in non-duplicate error cases.

The current implementation returns an empty response for non-duplicate errors, which might make debugging harder for clients.

-return new NextResponse('', {
+return new NextResponse(JSON.stringify({
+    code: 'external_account_error',
+    message: `Failed to create external account (status: ${response.status})`
+}), {
     status: response.status,
     headers: {
         'Content-Type': 'application/json',
     },
 })
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return new NextResponse('', {
status: response.status,
headers: {
'Content-Type': 'application/json',
},
})
return new NextResponse(JSON.stringify({
code: 'external_account_error',
message: `Failed to create external account (status: ${response.status})`
}), {
status: response.status,
headers: {
'Content-Type': 'application/json',
},
})

}
const data = await response.json()
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling for successful response parsing.

The JSON parsing of the successful response should also be wrapped in a try-catch block to prevent unhandled exceptions.

-const data = await response.json()
+let data;
+try {
+    data = await response.json()
+} catch (e) {
+    console.error('Error parsing successful response:', e)
+    return new NextResponse('Internal Server Error', { status: 500 })
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const data = await response.json()
let data;
try {
data = await response.json()
} catch (e) {
console.error('Error parsing successful response:', e)
return new NextResponse('Internal Server Error', { status: 500 })
}


return new NextResponse(JSON.stringify(data), {
status: 200,
Expand Down
3 changes: 2 additions & 1 deletion src/components/Global/LinkAccountComponent/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ export const GlobaLinkAccountComponent = ({ accountNumber, onCompleted }: IGloba
setValue: setAccountDetailsValue,
getValues: getAccountDetailsValue,
setError: setAccountDetailsError,
clearErrors: clearAccountDetailsErrors,
handleSubmit: handleAccountDetailsSubmit,
} = useForm<IRegisterAccountDetails>({
mode: 'onChange',
Expand Down Expand Up @@ -368,7 +369,7 @@ export const GlobaLinkAccountComponent = ({ accountNumber, onCompleted }: IGloba
value={accountDetailsWatch('country')}
onChange={(value: any) => {
setAccountDetailsValue('country', value, { shouldValidate: true })
setAccountDetailsError('country', { message: undefined })
clearAccountDetailsErrors('country')
}}
error={accountDetailsErrors.country?.message}
/>
Expand Down
48 changes: 26 additions & 22 deletions src/utils/cashout.utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,31 +187,35 @@ export async function createExternalAccount(
}),
})

const data = await response.json()

if (!response.ok) {
if (data.code && data.code == 'duplicate_external_account') {
// if bridge account already exists for this iban
// but somehow not stored in our DB (this should never happen in
// prod, but can happen if same email used in prod & staging)
//
// returns: not interfaces.IBridgeAccount type, but
// a currently undefined error type on the data field of interfaces.IResponse
// of format:
// {
// id: '4c537540-80bf-41dd-a528-d79a4',
// code: 'duplicate_external_account',
// message: 'An external account with the same information has already been added for this customer'
// }
//
// TODO: HTTP responses need to be standardized client wide
return {
success: false,
data,
} as interfaces.IResponse
try {
const data = await response.json()
if (data.code && data.code == 'duplicate_external_account') {
// if bridge account already exists for this iban
// but somehow not stored in our DB (this should never happen in
// prod, but can happen if same email used in prod & staging)
//
// returns: not interfaces.IBridgeAccount type, but
// a currently undefined error type on the data field of interfaces.IResponse
// of format:
// {
// id: '4c537540-80bf-41dd-a528-d79a4',
// code: 'duplicate_external_account',
// message: 'An external account with the same information has already been added for this customer'
// }
//
// TODO: HTTP responses need to be standardized client wide
return {
success: false,
data,
} as interfaces.IResponse
}
} catch (error) {
console.error('Error creating external account', response)
throw new Error('Unexpected error')
}
throw new Error('Failed to create external account (duplicate account)')
}
const data = await response.json()

return {
success: true,
Expand Down