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

Make endDate optional when downloading GoCardless transactions #241

Merged

Conversation

kyrias
Copy link
Contributor

@kyrias kyrias commented Aug 7, 2023

There appears to be no strong reason for explicitly limiting the end date to today, and it's preventing proper integration with Bank Norwegian whose value dates actually represent when the interest-free period for the transaction ends and is therefore in the future. (#237)

After this is merged we can then remove the corresponding field from the Actual application.

Copy link

@leikoilja leikoilja left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix 💥
For context, here are more reasoning and details

authors: [kyrias]
---

Stop sending `end_date` to today when downloading GoCardless transactions.

Choose a reason for hiding this comment

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

Suggested change
Stop sending `end_date` to today when downloading GoCardless transactions.
Stop sending optional `end_date` parameter to GoCardLess which is default set to `today()` when downloading transactions from GoCardless, since it filters out credit card transactions using `valueDate` for indicating when the interest-free period for the transaction ends. [Example issue for Bank on Norwegian Sweden](https://github.com/actualbudget/actual-server/pull/237#pullrequestreview-1563979522)

@trafico-bot trafico-bot bot added ✅ Approved Pull Request has been approved and can be merged and removed 🔍 Ready for Review labels Aug 8, 2023
@MatissJanis
Copy link
Member

Actually.. I think I know why we would want to keep this around: it allows to paginate the results.

Currently we always get the latest transactions. But with the current API (start + end date) we could build pagination that fetches the full transaction history. It would just make multiple API calls to get it all.

@MatissJanis
Copy link
Member

What if we made endDate optional (but still kept it around to allow the pagination implementation in UI in the future)?

@kyrias
Copy link
Contributor Author

kyrias commented Aug 8, 2023

I was originally going to make it optional but decided against it to keep the changes only to the backend, but if we make it optional and start using it again in the future then aren't we going to run into the same issue again with Bank Norwegian? Though I guess we could 'just' allow you to keep paging into later months...

@MatissJanis
Copy link
Member

This is how I'd imagine it working eventually:

The latest transactions are always loaded without endDate. Then we compare the oldest received transactions with what we had before. If there is something new - we start fetching the full history with multiple API calls until we hit the end of the history. If there is nothing new - we do not fetch the history and just stop.

This isn't a comprehensive solution though. And it has many question marks (how exactly do we perform the "are there more transactions that should be fetched"?). But alas it gives a vague understanding of how it might eventually work.

@kyrias kyrias force-pushed the gocardless-stop-sending-end_date branch from f7bdd31 to b3c6548 Compare August 8, 2023 20:34
@trafico-bot trafico-bot bot added 🔍 Ready for Review and removed ✅ Approved Pull Request has been approved and can be merged labels Aug 8, 2023
@kyrias kyrias force-pushed the gocardless-stop-sending-end_date branch from b3c6548 to a7802a4 Compare August 8, 2023 20:37
@kyrias kyrias changed the title Stop sending endDate when downloading GoCardless transactions Make endDate optional when downloading GoCardless transactions Aug 8, 2023
@kyrias
Copy link
Contributor Author

kyrias commented Aug 8, 2023

This isn't a comprehensive solution though. And it has many question marks (how exactly do we perform the "are there more transactions that should be fetched"?). But alas it gives a vague understanding of how it might eventually work.

There simply isn't any way to do that because the API wasn't meant for that use case. If we want pagination to work properly we'd need GoCardless to actually implement a pagination feature in their API, at which point we would again not need to pass this parameter.

But I've switched this to mark the argument as optional in the only place in which the type was specified. See actualbudget/actual#1493 for the other part. Would be nice to get these two merged relatively quickly so we can get the other parts ready for Bank Norwegian support. :)

@trafico-bot trafico-bot bot added ✅ Approved Pull Request has been approved and can be merged and removed 🔍 Ready for Review labels Aug 9, 2023
@MatissJanis MatissJanis merged commit 9102d97 into actualbudget:master Aug 9, 2023
6 checks passed
@MatissJanis
Copy link
Member

Thanks!

@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✨ Merged Pull Request has been merged successfully ✅ Approved Pull Request has been approved and can be merged labels Aug 9, 2023
@kyrias kyrias deleted the gocardless-stop-sending-end_date branch August 9, 2023 14:08
MMichotte pushed a commit to MMichotte/actual-server that referenced this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants