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

Initial pass at Plaid EU #1555

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Initial pass at Plaid EU #1555

wants to merge 6 commits into from

Conversation

Shpigford
Copy link
Member

No description provided.

@Shpigford Shpigford requested a review from zachgoll December 18, 2024 17:04
@Shpigford
Copy link
Member Author

@zachgoll First high-level pass at adding EU support. You've got a better handle of how the Plaid stuff works, so let me know if I'm off base here.

Copy link
Collaborator

@zachgoll zachgoll left a comment

Choose a reason for hiding this comment

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

I reached out to Plaid and they said there is no way to figure out whether an "Item" is US or EU just based on the Plaid Item data.

So the biggest thing we're missing currently here is that identifying piece of data stored on PlaidItem. I think since there are only 2 possible clients, we could consider a field called plaid_region that is a string and in our model we could probably validate that too:

class PlaidItem < ApplicationRecord
  enum :plaid_region, { us: "us", eu: "eu" }
end

Which would then allow us to easily access it like, some_item.eu?

Otherwise, I think EU should work largely the same as US and I believe our existing implementation will properly handle these multi-currency accounts fine.

.env.example Outdated Show resolved Hide resolved
db/schema.rb Show resolved Hide resolved
app/models/concerns/plaidable.rb Show resolved Hide resolved
app/views/accounts/new/_method_selector.html.erb Outdated Show resolved Hide resolved
@Shpigford
Copy link
Member Author

@zachgoll Take a look now.

@Shpigford Shpigford requested a review from zachgoll December 19, 2024 15:20
Copy link
Collaborator

@zachgoll zachgoll left a comment

Choose a reason for hiding this comment

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

Yeah, I like this strategy of passing it from the Stimulus controller. Should work nicely!

t.datetime "updated_at", null: false
t.index ["chat_id"], name: "index_messages_on_chat_id"
t.index ["user_id"], name: "index_messages_on_user_id"
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like we're still getting the extra tables here. You may need a full rails db:migrate:reset since db:reset will just rebuild based on this existing schema.rb and not perform the migrations

@@ -5,6 +5,7 @@ def create
Current.family.plaid_items.create_from_public_token(
plaid_item_params[:public_token],
item_name: item_name,
region: plaid_item_params[:region]
Copy link
Collaborator

Choose a reason for hiding this comment

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

The create_from_public_token method on PlaidItem will need to be updated to accept this param

@@ -1,6 +1,9 @@
class PlaidItem < ApplicationRecord
include Plaidable, Syncable

enum :plaid_region, { us: "us", eu: "eu" }
validates :plaid_region, inclusion: { in: plaid_regions.keys }
Copy link
Collaborator

Choose a reason for hiding this comment

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

The enum should have this inclusion validation already built-in, so we can remove the second validates here

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.

2 participants