Skip to content
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.

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

Add bank accounts modal and table #452

Open
dahal opened this issue Jul 19, 2024 · 18 comments · May be fixed by #494
Open

Add bank accounts modal and table #452

dahal opened this issue Jul 19, 2024 · 18 comments · May be fixed by #494
Assignees
Labels
😍 ENHANCEMENT New feature or request 🙏 NEED HELP Extra attention is needed

Comments

@dahal
Copy link
Contributor

dahal commented Jul 19, 2024

We currently have placeholder modal for bank accounts, we need to complete this feature. Completing this feature means:-

  • Complete the Add bank account modal
  • Render current bank accounts on a table
  • Table will have these columns, with following headers
    • Bank name

    • Account number

    • Empty header, but rows should render Primary if primary is true, else Secondary. These primary/secondary should be rendered using Badge component. success for primary and info for secondary

    • Actions, with drop down actions (similar to API Key table)

      • Edit, this will render the same modal, with pre filled data which will allow users to update existing bank account information.
      • Delete (self-explainatory) complete the flow by completing the trpc delete request.

image

You can get to this page by going to Settings > Bank accounts after you login

@dahal dahal added 😍 ENHANCEMENT New feature or request 🙏 NEED HELP Extra attention is needed labels Jul 19, 2024
@Raju-kadel-27 Raju-kadel-27 self-assigned this Jul 19, 2024
@Raju-kadel-27 Raju-kadel-27 removed their assignment Jul 21, 2024
@Raju-kadel-27
Copy link
Contributor

Just to inform, @Alfredoeb9 is working on this issue.

@rushikeshg25
Copy link

@Raju-kadel-27 I can help in with this. Please assign me this if there is inactivity for a long time. Sry for the ping

@dahal
Copy link
Contributor Author

dahal commented Jul 24, 2024

@Alfredoeb9 are you picking this up or can @rushikeshg25 help? Please confirm.

@Alfredoeb9
Copy link
Contributor

@dahal I am picking this up just waiting for my other PR to merge before I continue on this task.

@Alfredoeb9
Copy link
Contributor

@dahal on the new bank modal screen, do I include the respected BankAccount Table schema as inputs? (beneficiaryName, beneficiaryAddress, bankName, bankAddress, companyId etc.)

@dahal
Copy link
Contributor Author

dahal commented Jul 26, 2024

Yes please. Company id is only to establish association.

@Alfredoeb9
Copy link
Contributor

Some other edge cases for the routing numbers and account number will there be a maxLength limit?
When doing a search looks like Routing numbers are always 9 digits long. Account numbers may be up to 17 digits long.

@dahal
Copy link
Contributor Author

dahal commented Jul 26, 2024

Some other edge cases for the routing numbers and account number will there be a maxLength limit? When doing a search looks like Routing numbers are always 9 digits long. Account numbers may be up to 17 digits long.

Is this universal or only specific to US accounts, if its universal we can have that validation. @Alfredoeb9

@Alfredoeb9
Copy link
Contributor

Alfredoeb9 commented Jul 26, 2024

Some other edge cases for the routing numbers and account number will there be a maxLength limit? When doing a search looks like Routing numbers are always 9 digits long. Account numbers may be up to 17 digits long.

Is this universal or only specific to US accounts, if its universal we can have that validation. @Alfredoeb9

Let dig deeper on that, it didnt specify the country

Edit: After some research looks like every country has different max digits. ex. In Germany and Austria, Germany has 8 digits, Austria has 5. In Canada max digits are 8. In India 11-character code with the first four characters representing the bank name, and the last six characters representing the branch.

With this I won't be putting a maxLength on the fields

@Alfredoeb9
Copy link
Contributor

@dahal Before I continue to connecting to backend, is this UI approved?

I added in a confirm routing number as that seems to be standard across other 'Add Bank Account' services.

image

@dahal
Copy link
Contributor Author

dahal commented Jul 26, 2024

@dahal Before I continue to connecting to backend, is this UI approved?

I added in a confirm routing number as that seems to be standard across other 'Add Bank Account' services.

image

Looks good, we dont need the company name though. @Alfredoeb9

@Alfredoeb9
Copy link
Contributor

Alfredoeb9 commented Jul 31, 2024

@dahal are users able to create infinite amount of bank accounts, with the exception that only one can be true for primary and all others as false?

With the current schema only two accounts can be created, one for primary and one for non-primary, I can't create any other primary=false bank accounts.

I don't think prisma @@unique allows for one true and all others as false.

Solution: On the server I can run a check to see if any primary column is true and if there are then return back an error.

@dahal
Copy link
Contributor Author

dahal commented Jul 31, 2024

@dahal are users able to create infinite amount of bank accounts, with the exception that only one can be true for primary and all others as false?

With the current schema only two accounts can be created, one for primary and one for non-primary, I can't create any other primary=false bank accounts.

I don't think prisma @@unique allows for one true and all others as false.

Solution: On the server I can run a check to see if any primary column is true and if there are then return back an error.

I think one primary and secondary is fine for now with the proper validation in place. When editing the existing account, and if user decide to toggle primary to secondary and vice versa, please update other record to toggle as accordingly. Meaning

  • When toggling secondary => primary, toggle existing primary => secondary
  • When toggling primary => secondary, toggle existing secondary => primary

Show a ConfirmDialog with the message Please note that other primary/secondary account will be set as secondary/primary, are you sure you want to proceed?
@Alfredoeb9, please order primary/secondary and secondary/primary as according to the action you are taking.

@Alfredoeb9
Copy link
Contributor

@dahal I am not able to auto set primary -> secondary and secondary -> primary getting Error: Unique constraint failed on the fields: (companyId, primary)

Coming from BankAccount schema: @@unique([companyId, primary], name: "unique_primary_account")

How should we proceed? I could remove the unique constraints and implement the auto setting from primary -> seconary and vise-versa but then users are going to able to create infinite about of accounts unless custom code is placed on backend to check if any other accouts are primary and return an error in that case.

@dahal
Copy link
Contributor Author

dahal commented Aug 8, 2024

@dahal I am not able to auto set primary -> secondary and secondary -> primary getting Error: Unique constraint failed on the fields: (companyId, primary)

Coming from BankAccount schema: @@unique([companyId, primary], name: "unique_primary_account")

How should we proceed? I could remove the unique constraints and implement the auto setting from primary -> seconary and vise-versa but then users are going to able to create infinite about of accounts unless custom code is placed on backend to check if any other accouts are primary and return an error in that case.

@Alfredoeb9 we probably can run a raw sql query for this, something like

BEGIN;

-- Step 1: Select the current primary account and set it to false
UPDATE BankAccount
SET primary = FALSE
WHERE companyId = :companyId
AND primary = TRUE;

-- Step 2: Select the current non-primary account and set it to true
UPDATE BankAccount
SET primary = TRUE
WHERE companyId = :companyId
AND primary = FALSE
LIMIT 1;

COMMIT;

@Alfredoeb9
Copy link
Contributor

Alfredoeb9 commented Aug 19, 2024

can you reassign please @dahal

@Alfredoeb9
Copy link
Contributor

Alfredoeb9 commented Aug 20, 2024

@dahal I tried running a raw sql string using queryRaw and executeRaw as provided in the docs but no avail. I recieve the same error message as above Unique constraint failed on the fields: (companyId, primary)

I also tried putting query in a $transaction but did not work.

I also tried setting the primary value to null then switching to respected value but would get a different error as null cannot be assigned to boolean.

This feature is basically finished just need to find a solution for this last automatic primary switch but I'm not sure it is possible due to the @@unique constriant on a boolean column.

@dahal
Copy link
Contributor Author

dahal commented Aug 20, 2024

@dahal I tried running a raw sql string using queryRaw and executeRaw as provided in the docs but no avail. I recieve the same error message as above Unique constraint failed on the fields: (companyId, primary)

I also tried putting query in a $transaction but did not work.

I also tried setting the primary value to null then switching to respected value but would get a different error as null cannot be assigned to boolean.

This feature is basically finished just need to find a solution for this last automatic primary switch but I'm not sure it is possible due to the @@unique constriant on a boolean column.

Lets remove the db level validation in that case. @Alfredoeb9

@Alfredoeb9 Alfredoeb9 linked a pull request Aug 22, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
😍 ENHANCEMENT New feature or request 🙏 NEED HELP Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants