-
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 all 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,39 @@ 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_to_charge = user.orders.pending.exists? ? PortfolioTransaction::TRANSACTION_FEE_CENTS : 0 | ||
| balance_without_reservations = current_balance_cents + purchase_cost + fee_to_charge | ||
|
|
||
| total_needed = purchase_cost + transaction_fee | ||
|
|
||
| return true if balance_without_reservations >= total_needed | ||
|
|
||
| formatted_balance = format_money(current_balance_cents) | ||
| formatted_cost = format_money(total_needed) | ||
| errors.add(:shares, "Insufficient funds. You have #{formatted_balance} but need #{formatted_cost}") | ||
|
|
||
| false | ||
| 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 | ||
|
|
||
| return true if shares <= current_shares | ||
|
|
||
| formatted_shares = (current_shares % 1).zero? ? current_shares.to_i : current_shares | ||
|
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. I don't think we need this formatting because this is being run by a job so the user will not see an error message here at all. |
||
| errors.add(:base, "Cannot sell more shares than you own (#{formatted_shares} available)") | ||
| false | ||
| 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,15 @@ def update_order_status | |
| def purchase_cost | ||
| order.purchase_cost | ||
| end | ||
|
|
||
| def valid_order? | ||
| return order.sufficient_funds? if order.buy? | ||
| return order.sufficient_shares? if order.sell? | ||
|
|
||
| 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'm confused with this line here, I'm not sure what this is calculating. How is the balance something we're getting from adding the purchase cose and fees to be charged?