-
Notifications
You must be signed in to change notification settings - Fork 54
Add Validation to Order Creation #1038
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
da266a3
9615d13
338af91
327dd0f
947a8a0
90cec25
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,6 +54,28 @@ def existing_transaction_type | |
| order_transaction_type == :debit ? "buy" : "sell" | ||
| end | ||
|
|
||
| def sufficient_funds? | ||
| return true unless buy? | ||
| return true unless number_of_shares_valid_for_calculations? | ||
|
|
||
| current_balance_cents = (user.portfolio&.cash_balance || 0) * 100 | ||
|
|
||
| fee_already_subtracted = user.orders.pending.exists? ? PortfolioTransaction::TRANSACTION_FEE_CENTS : 0 | ||
| balance_without_reservations = current_balance_cents + purchase_cost + fee_already_subtracted | ||
|
|
||
| total_needed = purchase_cost + transaction_fee | ||
|
|
||
| balance_without_reservations >= total_needed | ||
|
||
| end | ||
|
|
||
| def sufficient_shares? | ||
| return true unless sell? | ||
| return true unless number_of_shares_valid_for_calculations? | ||
|
|
||
| current_shares = user.portfolio&.shares_owned(stock_id) || 0 | ||
| shares <= current_shares | ||
|
||
| end | ||
|
|
||
| private | ||
|
|
||
| def number_of_shares_valid_for_calculations? | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,11 @@ def self.execute(...) | |
| def execute | ||
| return unless order.pending? | ||
|
|
||
| unless valid_order? | ||
| cancel_order! | ||
| return | ||
| end | ||
|
|
||
| ActiveRecord::Base.transaction do | ||
| create_portfolio_transaction | ||
| create_portfolio_stock | ||
|
|
@@ -54,4 +59,18 @@ def update_order_status | |
| def purchase_cost | ||
| order.purchase_cost | ||
| end | ||
|
|
||
| def valid_order? | ||
| if order.buy? | ||
| order.sufficient_funds? | ||
| elsif order.sell? | ||
| order.sufficient_shares? | ||
| end | ||
|
|
||
| true | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because this is outside of the if block this function will always return true and that isn't what we want we'd want it to return the result of
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good callout I need to call return |
||
| end | ||
|
|
||
| def cancel_order! | ||
| order.cancel! | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -146,4 +146,32 @@ class ExecuteOrderTest < ActiveSupport::TestCase | |
| total_shares = portfolio.shares_owned(stock.id) | ||
| assert_equal 6, total_shares | ||
| end | ||
|
|
||
| test "cannot create buy order with insufficient funds" do | ||
| portfolio = create(:portfolio) | ||
| portfolio.portfolio_transactions.create!(amount_cents: 500, transaction_type: :deposit) | ||
| stock = create(:stock, price_cents: 100) | ||
|
|
||
| order = build(:order, :pending, :buy, shares: 10, stock: stock, user: portfolio.user) | ||
|
|
||
| assert_raises(ActiveRecord::RecordInvalid) do | ||
| order.save! | ||
| end | ||
|
|
||
| assert order.errors[:shares].first.include?("Insufficient funds") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question as below, is this the error message we will get? |
||
| end | ||
|
|
||
| test "cannot create sell order with insufficient shares" do | ||
| portfolio = create(:portfolio) | ||
| stock = create(:stock, price_cents: 100) | ||
| create(:portfolio_stock, portfolio: portfolio, stock: stock, shares: 5, purchase_price: 100) | ||
|
|
||
| order = build(:order, :pending, :sell, shares: 10, stock: stock, user: portfolio.user) | ||
|
|
||
| assert_raises(ActiveRecord::RecordInvalid) do | ||
| order.save! | ||
| end | ||
|
|
||
| assert order.errors[:base].first.include?("Cannot sell more shares than you own") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the error message we'd get I don't see it added as an error when we do the check. |
||
| end | ||
| end | ||
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.
I think the purpose of this function here here is not calculating "if" the fee was already calculated but rather what fee we need to add
maybe a better name could be
fee_to_charge