Skip to content

fix: Propagate error when bridge account already exists for IBAN #423

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 1 commit into from
Oct 8, 2024
Merged

fix: Propagate error when bridge account already exists for IBAN #423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 20 additions & 0 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 @@ -68,6 +68,26 @@ export async function POST(request: NextRequest) {
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
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: nice comment!

return new NextResponse(JSON.stringify(data), {
status: response.status,
headers: {
'Content-Type': 'application/json',
},
})
}
Comment on lines +71 to +90
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Inconsistent Handling of 'duplicate_external_account' Errors Detected

The error handling for duplicate_external_account is implemented in src/app/api/bridge/external-account/create-external-account/route.ts but is not consistently handled in other parts of the codebase, such as:

  • src/utils/cashout.utils.ts
  • src/components/Global/LinkAccountComponent/index.tsx

This inconsistency may lead to unpredictable behaviors and complicate error management.

Recommendations:

  1. Standardize Error Handling:

    • Ensure that duplicate_external_account errors are handled uniformly across all modules.

  2. Refactor Existing Implementations:

    • Update src/utils/cashout.utils.ts and src/components/Global/LinkAccountComponent/index.tsx to follow the same error handling pattern as in route.ts.

  3. Track Standardization Efforts:

    • Create a separate issue to address the standardization of error responses across the backend.
🔗 Analysis chain

Approve changes with suggestions for improvement

The new error handling for duplicate external accounts is a good addition. It properly handles the specific case and maintains consistency with the API response.

However, there are a few points to consider:

  1. The comment block (lines 74-83) is quite verbose. Consider refactoring it to be more concise while still conveying the necessary information.

  2. The TODO comment about standardizing error responses across the backend is important. I recommend creating a separate issue to track this task for a more comprehensive solution.

Here's a suggestion for a more concise comment:

// Handle case where bridge account already exists
// TODO: Standardize error responses across backend
// Current response structure:
// {
//   id: string,
//   code: 'duplicate_external_account',
//   message: string
// }

Would you like me to create a GitHub issue for standardizing error responses across the backend?

Let's verify if this new error handling is consistently used across the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent handling of 'duplicate_external_account' error

# Test 1: Search for other occurrences of 'duplicate_external_account'
echo "Searching for 'duplicate_external_account' occurrences:"
rg --type typescript "duplicate_external_account"

# Test 2: Search for similar error handling patterns
echo "\nSearching for similar error handling patterns:"
ast-grep --lang typescript --pattern $'if ($DATA.code && $DATA.code == $_) {
  return new NextResponse(JSON.stringify($DATA), {
    status: $_.status,
    headers: {
      "Content-Type": "application/json",
    },
  })
}'

Length of output: 542

Script:

#!/bin/bash
# Description: Check for consistent handling of 'duplicate_external_account' error without specifying file type

# Test 1: Search for other occurrences of 'duplicate_external_account'
echo "Searching for 'duplicate_external_account' occurrences across all files:"
rg "duplicate_external_account"

# Test 2: Search for similar error handling patterns without specifying language
echo "\nSearching for similar error handling patterns across all files:"
ast-grep --lang typescript --pattern $'if ($DATA.code && $DATA.code == $_) {
  return new NextResponse(JSON.stringify($DATA), {
    status: $_.status,
    headers: {
      "Content-Type": "application/json",
    },
  })
}'

Length of output: 1160

Script:

#!/bin/bash
# Description: Verify consistent handling of 'duplicate_external_account' in identified files

# Define the files to check based on previous search results
FILES=(
  "src/utils/cashout.utils.ts"
  "src/components/Global/LinkAccountComponent/index.tsx"
  "src/app/api/bridge/external-account/create-external-account/route.ts"
)

# Loop through each file and extract the error handling block for 'duplicate_external_account'
for FILE in "${FILES[@]}"; do
  echo "Extracting error handling for 'duplicate_external_account' in $FILE:"
  ast-grep --lang typescript --pattern $'if ($DATA.code && $DATA.code == "duplicate_external_account") { $$$ }' "$FILE"
done

Length of output: 1451

throw new Error(`HTTP error! status: ${response.status}`)
}

Expand Down
17 changes: 15 additions & 2 deletions src/components/Global/LinkAccountComponent/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import Icon from '../Icon'
import { useAuth } from '@/context/authContext'
import { Divider } from '@chakra-ui/react'
import { isIBAN } from 'validator'
import { IBridgeAccount, IResponse } from '@/interfaces'

const steps = [{ label: 'Step 1: Enter bank account' }, { label: 'Step 2: Provide details' }]

Expand Down Expand Up @@ -172,13 +173,25 @@ export const GlobaLinkAccountComponent = ({ accountNumber, onCompleted }: IGloba
throw new Error('Customer ID is missing')
}

const data = await utils.createExternalAccount(
const response: IResponse = await utils.createExternalAccount(
customerId,
accountType as 'iban' | 'us',
accountDetails,
address,
accountOwnerName
)

if (!response.success) {
if (response.data && response.data.code == 'duplicate_external_account' ) {
// bridge account already exists for this IBAN
const errorMessage = 'An external account with the same information has already been added for this customer'
throw new Error(errorMessage)

}
throw new Error('Creating Bridge account failed')
}
Comment on lines +184 to +192
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

Use strict equality for error code comparison

In line 185, consider using strict equality (===) when comparing response.data.code to 'duplicate_external_account' to avoid unexpected type coercion and adhere to best practices.

Apply this diff to make the change:

- if (response.data && response.data.code == 'duplicate_external_account' ) {
+ if (response.data && response.data.code === 'duplicate_external_account' ) {
📝 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
if (!response.success) {
if (response.data && response.data.code == 'duplicate_external_account' ) {
// bridge account already exists for this IBAN
const errorMessage = 'An external account with the same information has already been added for this customer'
throw new Error(errorMessage)
}
throw new Error('Creating Bridge account failed')
}
if (!response.success) {
if (response.data && response.data.code === 'duplicate_external_account' ) {
// bridge account already exists for this IBAN
const errorMessage = 'An external account with the same information has already been added for this customer'
throw new Error(errorMessage)
}
throw new Error('Creating Bridge account failed')
}


const data: IBridgeAccount = response.data

await utils.createAccount(
user?.user?.userId ?? '',
Expand All @@ -193,7 +206,7 @@ export const GlobaLinkAccountComponent = ({ accountNumber, onCompleted }: IGloba
onCompleted ? onCompleted() : setCompletedLinking(true)
} catch (error) {
console.error('Error during the submission process:', error)
setErrorState({ showError: true, errorMessage: 'An error occurred. Please try again later' })
setErrorState({ showError: true, errorMessage: String(error)})
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

Improve error messaging by using error.message

When setting the error message in setErrorState, consider using error.message instead of String(error) to provide a cleaner and more user-friendly error message.

Apply this diff to make the change:

- setErrorState({ showError: true, errorMessage:  String(error)})
+ setErrorState({ showError: true, errorMessage: error.message})

Committable suggestion was skipped due to low confidence.

} finally {
setLoadingState('Idle')
}
Expand Down
5 changes: 5 additions & 0 deletions src/interfaces/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ import { interfaces as peanutInterfaces } from '@squirrel-labs/peanut-sdk'

export type RecipientType = 'address' | 'ens' | 'iban' | 'us'

export interface IResponse {
success: boolean
data?: any
}

export interface IUserBalance {
chainId: string
address: string
Expand Down
31 changes: 27 additions & 4 deletions src/utils/cashout.utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ export async function createExternalAccount(
accountDetails: any,
address: any,
accountOwnerName: string
): Promise<interfaces.IBridgeAccount> {
): Promise<interfaces.IResponse> {
try {
const response = await fetch(`/api/bridge/external-account/create-external-account?customerId=${customerId}`, {
method: 'POST',
Expand All @@ -187,13 +187,36 @@ export async function createExternalAccount(
}),
})

const data = await response.json()

Comment on lines +190 to +191
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

Check response.ok before parsing JSON

Calling await response.json() before verifying response.ok can lead to runtime errors if the response body is empty or not valid JSON. It's safer to check response.ok before attempting to parse the response body.

Suggested fix:

-            const data = await response.json()

+            let data;
+            if (!response.ok) {
+                // Handle error without parsing JSON
+            } else {
+                data = await response.json();
+            }

Alternatively, parse the JSON within the conditional blocks to ensure data is only accessed when the response is OK.

Committable suggestion was skipped due to low confidence.

if (!response.ok) {
if (data.code && data.code == 'duplicate_external_account') {
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use strict equality for string comparison

For type safety and to prevent unexpected type coercion, use === instead of == when comparing strings in TypeScript.

Suggested fix:

+                if (data.code && data.code === 'duplicate_external_account') {
📝 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
if (data.code && data.code == 'duplicate_external_account') {
if (data.code && data.code === 'duplicate_external_account') {

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this is a nitpick and wont change anth here, but generally good advice

Copy link
Contributor

Choose a reason for hiding this comment

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

@Hugo0, understood. Thank you for your feedback.

(_/_)/(｡•́︿•̀｡)

// 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
}
Comment on lines +193 to +212
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

Consistent error handling and return types

The function createExternalAccount is expected to return a Promise<interfaces.IResponse>. However, when an error occurs, the function throws an exception instead of returning an IResponse object with success: false. This inconsistency can lead to unexpected behavior for callers expecting a consistent response structure.

Suggested fix:

Modify both the error handling within the if (!response.ok) block and the catch block to return a standardized error response. For example:

+                let errorData = await response.json();
                if (errorData.code && errorData.code === 'duplicate_external_account') {
                    // Return structured error response
                    return {
                        success: false,
                        data: errorData,
                    } as interfaces.IResponse;
                }
-               throw new Error('Failed to create external account')
+               return {
+                   success: false,
+                   data: { message: 'Failed to create external account', details: errorData },
+               } as interfaces.IResponse;
            }

...

        } catch (error) {
            console.error('Error:', error);
-           throw new Error(`Failed to create external account. Error: ${error}`);
+           return {
+               success: false,
+               data: { message: `Failed to create external account. Error: ${error}` },
+           } as interfaces.IResponse;
        }

This ensures that the function consistently returns an IResponse object regardless of success or failure.

Committable suggestion was skipped due to low confidence.

⚠️ Potential issue

Ensure data is defined before accessing properties

If response.json() fails or returns undefined, accessing data.code could result in a runtime error. Ensure that data is properly defined and handle cases where parsing the JSON might fail.

Suggested fix:

+                let errorData;
+                try {
+                    errorData = await response.json();
+                } catch (parseError) {
+                    console.error('Error parsing JSON:', parseError);
+                    errorData = { message: 'Unknown error occurred' };
+                }
+                if (errorData.code && errorData.code === 'duplicate_external_account') {
                    // Return structured error response
                    return {
                        success: false,
-                       data
+                       data: errorData,
                    } as interfaces.IResponse;
                }

Committable suggestion was skipped due to low confidence.

throw new Error('Failed to create external account')
}

const data = await response.json()

return data as interfaces.IBridgeAccount
return {
success: true,
data: data as interfaces.IBridgeAccount
} as interfaces.IResponse
} catch (error) {
console.error('Error:', error)
throw new Error(`Failed to create external account. Error: ${error}`)
Expand Down