Skip to content

Wallets#181

Merged
rogelioLpz merged 19 commits intomainfrom
wallets
Sep 17, 2021
Merged

Wallets#181
rogelioLpz merged 19 commits intomainfrom
wallets

Conversation

@rogelioLpz
Copy link
Copy Markdown
Member

@rogelioLpz rogelioLpz commented Aug 6, 2021

closes #180

@rogelioLpz rogelioLpz added the enhancement New feature or request label Aug 6, 2021
@rogelioLpz rogelioLpz self-assigned this Aug 6, 2021
@rogelioLpz rogelioLpz requested a review from matin as a code owner August 6, 2021 20:08
@rogelioLpz rogelioLpz marked this pull request as draft August 6, 2021 20:08
Comment thread tests/resources/test_savings.py Outdated

# STEP 2 : DEPOSIT MONEY IN SAVING
deposit = WalletTransaction.create(
wallet_id=saving.id,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

en este caso si sería wallet_uri

Comment thread tests/resources/test_savings.py Outdated
Comment on lines +61 to +36
entries: List[BalanceEntry] = BalanceEntry.all(
funding_instrument_uri=wallet_uri
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmmm no, no sería funding_instrument_uri, sería wallet_id

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

y para el default, sería wallet_id=default

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 13, 2021

Codecov Report

Merging #181 (db29180) into main (5a86635) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #181   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           27        29    +2     
  Lines          661       726   +65     
=========================================
+ Hits           661       726   +65     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cuenca/__init__.py 100.00% <ø> (ø)
cuenca/http/client.py 100.00% <100.00%> (ø)
cuenca/resources/__init__.py 100.00% <100.00%> (ø)
cuenca/resources/balance_entries.py 100.00% <100.00%> (ø)
cuenca/resources/base.py 100.00% <100.00%> (ø)
cuenca/resources/savings.py 100.00% <100.00%> (ø)
cuenca/resources/wallet_transactions.py 100.00% <100.00%> (ø)
cuenca/version.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a86635...db29180. Read the comment docs.

Comment thread cuenca/http/client.py Outdated

if data is not None:
for key, value in data.items():
if isinstance(value, (dt.date, dt.datetime)):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is redundant:

In [19]: isinstance(dt.datetime.now(), dt.date)
Out[19]: True

Comment thread cuenca/http/client.py
if data is not None:
for key, value in data.items():
if isinstance(value, (dt.date, dt.datetime)):
data[key] = value.isoformat()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missing the timezone:

In [28]: value = dt.datetime.now()

In [29]: value.isoformat()
Out[29]: '2021-08-13T17:49:16.368700'

In [30]: value.tzinfo is None
Out[30]: True

In [31]: default_tz = dt.timezone.utc

In [32]: if value.tzinfo is None:
    ...:     value = value.astimezone(default_tz)
    ...:

In [33]: value.isoformat()
Out[33]: '2021-08-13T22:49:16.368700+00:00'

not sure what default_tz would be in this case: UTC or America/Mexico_City

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Si es necesario indicar el timezone?
Entiendo que todo se maneja en UTC

Comment thread cuenca/resources/savings.py Outdated
_resource: ClassVar = 'savings'
name: str
category: SavingCategory
goal_amount: int
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can goal be None?


@property
def wallet(self) -> Optional['Wallet']:
return cast('Wallet', retrieve_uri(self.wallet_uri))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

does this work?

Wallet isn't here:

# avoid circular imports
resource_classes = [
ApiKey,
Account,
Arpc,
BalanceEntry,
BillPayment,
Card,
CardActivation,
CardTransaction,
CardValidation,
Commission,
Deposit,
LoginToken,
Saving,
ServiceProvider,
Statement,
Transfer,
UserCredential,
UserLogin,
WalletTransaction,
WhatsappTransfer,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Wallet es la clase base, Saving es el resource

@rogelioLpz rogelioLpz marked this pull request as ready for review September 1, 2021 00:30
@alexviquez
Copy link
Copy Markdown
Contributor

hay que poner contexto o asignar un issue al PR @rogelioLpz

Copy link
Copy Markdown
Contributor

@alexviquez alexviquez left a comment

Choose a reason for hiding this comment

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

unos comments

Comment on lines +29 to 32
wallet_id: str

@property # type: ignore
def related_transaction(self) -> Transaction:
Copy link
Copy Markdown
Contributor

@alexviquez alexviquez Sep 6, 2021

Choose a reason for hiding this comment

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

tal y como related_transaction y funding_instruments deberíamos poder hacer fetch del wallet desde el balance_entry

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No aplica. Debido a que los balance_entries de la cuenta default van tener el valor default y eso no apunta a ningún recurso

Comment thread cuenca/resources/base.py Outdated
Comment thread requirements.txt Outdated
Comment thread tests/resources/test_wallet_transactions.py Outdated
@rogelioLpz rogelioLpz merged commit 7453795 into main Sep 17, 2021
@rogelioLpz rogelioLpz deleted the wallets branch September 17, 2021 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resources Saving y WalletTransaction

4 participants