Execute trades job issue 145 #174

jonny5 · 2024-06-01T20:51:09Z

Issue #145

Creates a service to process pending orders and wrapper job to run that service for all pending orders.
Changed floats to decimal for consistency
Linked order to respective portfolio_transaction and portfolio_stock for an audit trail

Todo:
Enqueue the StockPricesUpdateJob once the StockPurchaseJob is completed

jonny5 · 2024-06-01T20:52:35Z

test/jobs/stock_purchase_job_test.rb

+
+    execute_calls = []
+
+    PurchaseStock.define_singleton_method(:execute) do |order|


~~wasn't sure how to properly mock things out in minitest, this was what seemed to work?~~

Did not work do not do this 😅

h-m-m · 2024-06-01T23:05:26Z

app/services/purchase_stock.rb

+  def withdrawal_amount
+    -order.purchase_cost
+  end


I like this a lot. This helps so much with keeping the rest of the code easy to read

h-m-m · 2024-06-01T23:07:24Z

test/models/order_test.rb

+  end
+
+  test "calculates purchase cost" do
+    stock = Stock.create(ticker: "EVG", price: 10.00)


I think you can use the fixture stock like you are in the previous test instead of creating a new one.

As context, we want to spare round-trip calls to the database in the test suite whenever we can. One of the toughest parts of Rails app maintenance is cutting down on all the database accesses that creep into the test suite and slow it down over time. Rails itself doesn't help you much in managing this problem

Updated to use fixture. Got it, I usually use FactoryBot for these types of tests

I like FactoryBot. We may eventually want it on this project. I like the idea of using the fixtures for now until we have a compelling reason to switch

h-m-m · 2024-06-01T23:08:06Z

test/services/purchase_stock_test.rb

+    assert_equal portfolio_stock, order.portfolio_stock
+  end
+
+  test "it updates the order status to completed" do


Do we have time this weekend to test any failure states?

Added a failure state at the bottom, required mocha gem for stubbing (seemed like a popular way in minitest? I tend to use rspec)

I wonder if there's a way we can do this without the stub you've written. I don't want to hold up the good work in this PR, though. If you're up for taking a go at rewriting it, I'm happy to pair with you to go over my concerns. If you're not up for that, then I think we should merge this PR in and we can discuss putting follow-up work in a separate issue

I won't have availability for a bit so feel free to do what seems best. The mocking + failure test are contained in the last commit so you could remove that before merging if you don't want mocha added

I've been noodling on this. For the kind of fail cases we want to test, are you talking about something like "what if there is not enough money in the account" or something like that @h-m-m ?

Yes! I think that's the main situation.

We will run into a problem at some point where the pending orders plus the new order could add up to more than what's in the account. Should that also go in this PR, or should we break that out into a follow-up PR?

I'm glad you asked this because it made me realize some of this logic should maybe live in a more traditional controller request. Can't we process the transactions themselves based on the price the user sees when they make the request. I would think the only part we need to put in a chron job is updating the price of the stock, either at the beginning or end of the trade day. I'm thinking the purchases themselves should just use that number so the user can see a more immediate error message if they don't have enough in their account. Am I making sense? Is there another reason to put the transactions themselves in a job?

- After this job runs we will want to enqueue the `StockPricesUpdateJob`, not sure about that implementation yet

- Multiple portfolio_stocks can exist for a portfolio with different purchase prices

- Use mocha for stubbing

jonny5 commented Jun 1, 2024

View reviewed changes

jonny5 force-pushed the execute-trades-job_issue-145 branch 3 times, most recently from 9273f4d to 86d3c12 Compare June 1, 2024 22:01

h-m-m reviewed Jun 1, 2024

View reviewed changes

americac and others added 8 commits June 2, 2024 09:22

Add pending scope to order model and add test

2919e89

Add purchase_stock service to handle stock purchases

8d40e67

[MIGRATION] Change portfolio_stock shares and purchase_price to decimal

52c2547

Add reference to portfolio_stock and portfolio_transaction to orders

ddeb095

Add a class execute method to service

d23e58b

Add stock purchase job

bbe21f3

- After this job runs we will want to enqueue the `StockPricesUpdateJob`, not sure about that implementation yet

Remove unique index on portfolio_stocks on portfolio and stock

2d82828

- Multiple portfolio_stocks can exist for a portfolio with different purchase prices

use fixture for stock

b73b345

jonny5 force-pushed the execute-trades-job_issue-145 branch from 86d3c12 to b73b345 Compare June 2, 2024 13:27

Add failure state test

7ddd8ae

- Use mocha for stubbing

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Execute trades job issue 145 #174

Execute trades job issue 145 #174

jonny5 commented Jun 1, 2024 •

edited

Loading

jonny5 Jun 1, 2024 •

edited

Loading

h-m-m Jun 1, 2024 •

edited

Loading

h-m-m Jun 1, 2024 •

edited

Loading

jonny5 Jun 2, 2024

h-m-m Jun 5, 2024

h-m-m Jun 1, 2024

jonny5 Jun 2, 2024

h-m-m Jun 5, 2024

jonny5 Jun 5, 2024

LizPrescott Sep 27, 2024

h-m-m Sep 27, 2024

LizPrescott Oct 8, 2024


		execute_calls = []

		PurchaseStock.define_singleton_method(:execute) do \|order\|

Execute trades job issue 145 #174

Are you sure you want to change the base?

Execute trades job issue 145 #174

Conversation

jonny5 commented Jun 1, 2024 • edited Loading

jonny5 Jun 1, 2024 • edited Loading

Choose a reason for hiding this comment

h-m-m Jun 1, 2024 • edited Loading

Choose a reason for hiding this comment

h-m-m Jun 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonny5 commented Jun 1, 2024 •

edited

Loading

jonny5 Jun 1, 2024 •

edited

Loading

h-m-m Jun 1, 2024 •

edited

Loading

h-m-m Jun 1, 2024 •

edited

Loading