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

Append-only wallet transactions #21

Merged
merged 7 commits into from
Feb 22, 2017
Merged

Conversation

kjagiello
Copy link
Member

@kjagiello kjagiello commented Feb 18, 2017

Wallet transactions should never be altered once created. Should solve #19.

@kjagiello kjagiello added this to the 2.0.0 milestone Feb 18, 2017
@kjagiello kjagiello self-assigned this Feb 18, 2017
@kjagiello kjagiello requested a review from flaeppe February 18, 2017 23:27
@kjagiello kjagiello force-pushed the feature/append-only-wallet-trxs branch from d72481f to e1db6d0 Compare February 18, 2017 23:30
@codecov-io
Copy link

codecov-io commented Feb 19, 2017

Codecov Report

Merging #21 into develop will increase coverage by 0.07%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop      #21      +/-   ##
===========================================
+ Coverage    87.71%   87.79%   +0.07%     
===========================================
  Files           73       73              
  Lines         2410     2417       +7     
  Branches       126      125       -1     
===========================================
+ Hits          2114     2122       +8     
+ Misses         278      276       -2     
- Partials        18       19       +1
Impacted Files Coverage Δ
src/foobar/rest/serializers/wallet.py 100% <ø> (ø)
src/foobar/tests/rest/test_purchase.py 98.36% <ø> (-0.03%)
src/wallet/enums.py 100% <100%> (ø)
src/wallet/signals/handlers.py 100% <100%> (ø)
src/wallet/tests/factories.py 100% <100%> (ø)
src/wallet/api.py 100% <100%> (ø)
src/wallet/models.py 100% <100%> (+1.78%)
src/foobar/tests/rest/test_wallet.py 100% <100%> (ø)
src/foobar/tests/test_views.py 100% <100%> (ø)
src/foobar/tests/test_api.py 100% <100%> (ø)
... and 3 more

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 1c7e1ef...6ec06cd. Read the comment docs.

For example, rejected transactions does not contribute to
the wallet balance.
For example, rejected transactions do not contribute to the wallet
balance.
Copy link
Member

Choose a reason for hiding this comment

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

Is there a spelling mistake here, shouldn't it say: ...example, pending transactions do ...?
As that is what we check at line 84

"""Withdraw given amount from the wallet.

Throw InsufficientFunds if there is not enough money in the wallet.
"""
assert amount.amount >= 0, "Cannot withdraw a negative amount of money"
assert amount.amount > 0, "The amount must be non-negative."
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer positive instead of non-negative. Otherwise it could seem like 0 is OK, which it is not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fix incoming soon!

"""Deposit given amount into the wallet."""
assert amount.amount >= 0, "Cannot deposit a negative amount of money"
assert amount.amount > 0, "The amount must be non-negative."
Copy link
Member

Choose a reason for hiding this comment

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

Same as previously mentioned

trx_status=enums.TrxStatus.FINALIZED,
amount=amount,
trx_type=TrxType.PENDING if pending else TrxType.FINALIZED,
amount=-amount,
reference=reference
Copy link
Member

@flaeppe flaeppe Feb 19, 2017

Choose a reason for hiding this comment

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

What if pending=False and reference=None? Is it necessary to cover this case?

@@ -13,14 +25,13 @@ def transfer_currency(apps, schema_editor):
for wallet in Wallet.objects.all():
incoming = WalletTrx.objects.filter(
wallet=wallet.id,
trx_status=enums.TrxStatus.FINALIZED,
trx_type=enums.TrxType.INCOMING
trx_status=TrxStatus.FINALIZED,
Copy link
Member

Choose a reason for hiding this comment

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

Isn't TrxStatus.PENDING interesting here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Incoming transactions that are PENDING do not contribute to the wallet balance.

elif not created:
# A transaction has been updated, so we simply recalculate
# the wallet balance.
wallet_obj.balance = wallet_obj.transactions.balance()
wallet_obj.save()
Copy link
Member

Choose a reason for hiding this comment

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

Do we always need to call save() here?

Copy link
Member Author

Choose a reason for hiding this comment

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

True, true

Copy link
Member Author

@kjagiello kjagiello Feb 19, 2017

Choose a reason for hiding this comment

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

No, definitely not! Good catch.

@kjagiello kjagiello force-pushed the feature/append-only-wallet-trxs branch from e1db6d0 to 4c03ea3 Compare February 20, 2017 10:21
@kjagiello kjagiello changed the title Append only wallet transactions Append-only wallet transactions Feb 22, 2017
@kjagiello kjagiello merged commit 4e7bd10 into develop Feb 22, 2017
@kjagiello kjagiello deleted the feature/append-only-wallet-trxs branch March 12, 2017 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants