-
Notifications
You must be signed in to change notification settings - Fork 19
PY-benOff-fields Update beneficial owner to accept optional fields #279
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
e2e_tests/account_test.py
Outdated
| # Mock or assume the existence of these functions | ||
| account_id = "3344334" # Assuming this function exists |
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.
What function? Isn't this just a varible declaration?
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.
sorry it is just variable, will remove comment
e2e_tests/account_test.py
Outdated
| "trackUserId": "userId_fe6885b5815463b26f65e71095832bdd916890f7"}, | ||
| credit_limit=_credit_limit) | ||
| "trackUserId": "userId_fe6885b5815463b26f65e71095832bdd916890f7"}) | ||
| response = client.accounts.update(request) | ||
| assert response.data.type == "creditAccount" | ||
| assert response.data.attributes.get("creditLimit") == _credit_limit |
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.
Why did we need to remove the credit limit?
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.
from what i tested in postman we dont need creditLimit
| transaction_ids = [] | ||
| response = client.transactions.list(ListTransactionParams(150, 20, since="2022-10-13T16:01:19.346Z", | ||
| until="2022-11-13T16:01:19.346Z")) | ||
| until="2022-11-13T16:01:19.346Z", account_id="3344334")) |
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.
Is this change intdended?
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.
yess as this account is with benOwners
unit/models/account.py
Outdated
| payload = { | ||
| "data": { | ||
| "type": CreditAccountType, | ||
| "type": "creditAccount", # Assuming CreditAccountType resolves to "creditAccount" |
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 I'm understanding this correctly, CreditAccountType is a constant\enum that was reused. It is a much better and cleaner approach that hardcoding string values. Is there a reason to omit CreditAccountType and use the string instead?
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.
ok will return it
| beneficial_owners=[ | ||
| BeneficialOwner( | ||
| try: | ||
| request = CreateBusinessApplicationRequest( |
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.
Am I understanding that in CreateBusinessApplicationRequest there are only formatting changes?
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.
small change to do benOff optional
e2e_tests/account_test.py
Outdated
| def test_update_credit_account(): | ||
| # Mock or assume the existence of these functions | ||
| account_id = "3344334" # Assuming this function exists | ||
| account_id = "3344334" |
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.
Do we have a way to not have this id hardcoded, but create an account for the test or fetch an existing one?
No description provided.