-
-
Notifications
You must be signed in to change notification settings - Fork 100
🚀 Add bank account transaction integration #132
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
Conversation
@KMKoushik :) |
I have also added i18n support for this feature in another PR that is based on this PR: #89. If this gets merged I can open a PR with i18n support for this aswell. https://github.com/alexanderwassbjer/split-pro/tree/feature/i18n-support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good overall with some comments. please lemme know what you think!
tysm for this PR really appreciate it. this is really awesome!
think it's best to merge in v2, test thoroughly and release it with other features! (i'll create a new branch)
.env.example
Outdated
|
||
# GoCardless options | ||
NEXT_PUBLIC_GOCARDLESS_ENABLED= | ||
NEXT_PUBLIC_GOCARDLESS_COUNTRY= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so it will work on one specific country alone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it should work world wide. The bank provider picker in the settings page fetch all bank providers. It works with either one country or undefined which then returns all the bank providers.
But because the list can be long in that case, I added the env to have a option to only fetch some.
If you have another approach to this, feel free to tell me.
One way is to have a country picker first and then the providers but it feels a bit weird..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have default currency option, we can get the country from that currency!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find the default currency? If you mean the currency that is set by the user when they try to add a expense it feels a bit weird to use that because it can change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for now, it only works with one country in the package I use. https://github.com/nordigen/nordigen-node/blob/9d535ca9d788115d4e199c749432733be3ea6cdd/lib/api/institutions.js#L23
That can be fixed, either in the package or that we have a country picker first.
But if we think of the user case, will it be necessary to have multiple countries at the same instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to stick with the env and use it either with no value to fetch all bank providers or filter it based on the env. I have created a PR on the Nordigen-node package and hoping for a merge so we can use it.
nordigen/nordigen-node#63
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you submit this PR to my fork? :)
@alexanderwassbjer changed the base to v2! |
Awesome! Iam happy you liked it. It would be nice if it was released in v2! |
ofcourse this is going to be star feature! |
Co-authored-by: Fredrik Olsson <[email protected]>
Fix bug with floating numbers on exact split calculator
@alexanderwassbjer
|
94bc6d7
to
421b40e
Compare
@alexanderwassbjer awesome! Feel free to add me on Discord #_krokosik |
<Checkbox | ||
checked={multipleTransactions?.some( | ||
(cItem) => cItem.transactionId === item.transactionId, | ||
)} | ||
disabled={alreadyAdded} | ||
onCheckedChange={createCheckboxHandler(item)} | ||
className="h-6 w-6 md:h-4 md:w-4" | ||
/> | ||
<Button className="flex items-center gap-4" variant="ghost" disabled={alreadyAdded}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not very intuitive to me. With such a list, especially on mobile you just see checkboxes and may not be aware that it is possible to fill in the expense details using just a single transaction.
On the other hand, given a list of things to select, I would expect that clicking a row will toggle the checkbox and it selects a single transaction instead.
I will take the opportunity to lay out in this comment my main user flow icks:
- the new UI is nice but it severely cluttters the previously minimal add expense UI
- it kind of behaves like a separate entity to the original flow, especially with multiple transactions. This is especially bad since it is not obvious that the split outlined above is actually applied
- when selecting multiple transactions, there is no way to see the total sum and the fact that there are multiple expenses created is surprising to me.
- the add container gets expanded and overflows the screen, putting either the sponsor button or the transaction selection
Here are some loose thoughts I have on next steps, which are up to debate of course:
- the transaction selection should stay out of the user's way - I would propose putting it into an
AppDrawer
triggered by alandmark
icon to the right of the sponsor button - inside of it, the user can select multiple transactions. You may want to consider the scroll-area component from shadcn, but maybe the appdrawer handles that already
- regarding multiple transactions, in the current flow, you cannot modify them individually, which implies to me that you are creating one expense out of multiple transactions (for example buying groceries in several stores). So instead of creating multiple expenses, I would just update the add page with the total sum AND a title that concatenates all the transaction name. As I doubt it will be a sensible name, we can focus the title input after closing the modal.
Let me know what do you think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve updated the flow so that transaction selection is now moved out of the main form — there’s a button next to the Sponsor button which opens an AppDrawer. Inside the drawer you can select one or multiple transactions (either by clicking the checkbox or the entire row). When you’re done, there’s a submit button that adds them. This should make the main add expense view feel less cluttered and more focused.
But I actually think that the transaction button next to the sponsor button does not feel like a function button. I think it should be redesigned so it easier to understand that it's a feature.
Regarding the multiple transactions flow: I don’t agree with the idea of merging them into a single expense. In my implementation, each selected transaction is added as its own expense. The reason is that it allows you to later review, edit, or remove individual expenses. If we instead created a combined expense, that granularity would be lost.
From my perspective (and the way I use this in my own household finances with my partner), the ability to keep the transactions separate is essential. For example, if we mistakenly add the wrong transaction, it’s very easy to spot and remove the specific one. If everything were merged into a single expense, we would no longer use the feature since we couldn’t make such corrections without removing the whole combined expense.
I understand your point about the total sum not being visible at the moment — that’s a valid concern, and I think we could address it by showing a summary of the selected transactions somewhere in the AppDrawer before submitting in either this version of the feature or in v2 of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding multiple transactions:
I agree that creating multiple separate expenses would be nice, but this is the component for adding a single expense and I believe it should stay that way for consistent user experience. In my opinion, if you want granularity, you should just add them one by one, since the way you described, you still need to go over each individual expense.
I strongly feel that this page should primarily support adding a single expense. The button is now at a very convenient position for a finger to do it fast and the added convenience, especially that cancelling its consequences when done by mistake (very easy now) is quite tedious, should be possible in some other way.
Perhaps we could have a checkbox/dialog asking whether we want to merge the selected transactions into the edit form or submit seperately. The latter should show some clear information that it is going to happen (multiple expenses with the current split settings will be created). Another issue is that users can submit the expenses even without getting to the split type selection.
All in all, I just can't accept the multiple transaction in the current state and I don't have a good idea for its implementation. I propose first merging a singular transactions as the PR is vast and then we can iterate once we get a good idea on how to implement that.
src/components/AddExpense/BankTransactions/BankingTransactionList.tsx
Outdated
Show resolved
Hide resolved
src/components/AddExpense/BankTransactions/BankingTransactionList.tsx
Outdated
Show resolved
Hide resolved
prisma/migrations/20250922183043_add_cached_bank_data_relation/migration.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More UI comments. Especially that we have a responsiveness regression. Here is the comparison with the main branch
screenrecording-2025-10-02_20-57-10.mp4
@alexanderwassbjer thank you for all the hard work! I think that feature wise the branch is now ready to merge and we will polish it out+add multiple transactions as other developments in 1.6! Once the CI checks pass, we can go ahead and finally merge this. |
Implemented an integration that uses GoCardless api to fetch bank account transactions. Then when you want to add an expense you can either fill it manually or you can pick from your transactions and it will automatically be added as an expense. There is also a function to "multi add" multiple transactions at the same time.
If a transaction already has been added to a group you cant add it once again.
GoCardless page
Other changes:
Screenshots: