Stock filter

[Shpigford]

• Don't show the stock filter until typing
• Make sure transactions are properly connecting to securities
• Figure out how/if to handle free-form text

[Shpigford]

@josefarias Working with HotwireCombobox and running in to a couple of issues that I'd love your input on if you have a chance.

First is that it shows an empty dropdown when clicking in the box.

Second is that after it can't find anything, it shows the last items it could find, instead of clearing out the list of stocks.

This is problematic because we want to allow people to free type in items if we don't have the stock they're wanting in our list.

[josefarias]

Hey @Shpigford, of course!

it shows an empty dropdown when clicking in the box

Right so what you're seeing is an empty turbo frame that fetches the options asynchronously. It's being made visible by some of the custom CSS applied to the combobox. I've shared some suggestions below that should keep the style you want while keeping the frame invisible.

after it can't find anything, it shows the last items it could find, instead of clearing out the list of stocks

I think this is because you're conditionally rendering the async_combobox_options. You always want to render those — it's how the client is notified of the latest results. Which in this case the results happen to be an empty set, but the client still needs to know it's empty so it can clear out the previous results.

we want to allow people to free type in items if we don't have the stock they're wanting in our list.

I see you're already set up for this, but I wanted to mention we now have a short-hand for when name_when_new matches the original name. It's free_text: true.

Here's the whole diff that makes it work as expected for me locally. Let me know if you run into trouble!

PS — you might wanna try the autocomplete: :both option (it's the default), I think it usually makes for the most expected autocomplete behavior. But I'd have to try it with real ticker options to be sure it's what you want (db seeds only contain four options).

CleanShot.2024-10-27.at.23.27.25.mp4

diff --git a/app/assets/stylesheets/application.tailwind.css b/app/assets/stylesheets/application.tailwind.css
index 441f8d8..d3168cf 100644
--- a/app/assets/stylesheets/application.tailwind.css
+++ b/app/assets/stylesheets/application.tailwind.css
@@ -131,9 +131,18 @@
   }
 
   .hw-combobox__listbox {
-    @apply absolute top-[160%] right-0 w-full border border-black/20 bg-white rounded shadow-xs p-1 z-30;
+    @apply absolute top-[160%] right-0 w-full bg-transparent rounded z-30;
   }
 
+  .hw_combobox__pagination__wrapper {
+    @apply h-px;
+
+    &:only-child {
+      @apply bg-transparent;
+    }
+  }
+
+  --hw-border-color: rgba(0, 0, 0, 0.2);
   --hw-handle-width: 20px;
   --hw-handle-height: 20px;
   --hw-handle-offset-right: 0px;
@@ -153,4 +162,4 @@
   .scrollbar::-webkit-scrollbar-thumb:hover {
     background: #a6a6a6;
   }
-}
\ No newline at end of file
+}
diff --git a/app/controllers/account/trades_controller.rb b/app/controllers/account/trades_controller.rb
index e1a958b..8b5a878 100644
--- a/app/controllers/account/trades_controller.rb
+++ b/app/controllers/account/trades_controller.rb
@@ -34,9 +34,7 @@ class Account::TradesController < ApplicationController
   end
 
   def securities
-    return unless params[:q].present?
-    @security_query = params[:q]
-    @pagy, @securities = pagy(Security.order(:name).search(@security_query), limit: 20)
+    @pagy, @securities = pagy(Security.order(:name).search(params[:q]), limit: 20)
   end
 
   private
diff --git a/app/views/account/trades/_form.html.erb b/app/views/account/trades/_form.html.erb
index 268072a..283a772 100644
--- a/app/views/account/trades/_form.html.erb
+++ b/app/views/account/trades/_form.html.erb
@@ -8,7 +8,7 @@
       <%= form.select :type, options_for_select([%w[Buy buy], %w[Sell sell], %w[Deposit transfer_in], %w[Withdrawal transfer_out], %w[Interest interest]], "buy"), { label: t(".type") }, { data: { "trade-form-target": "typeInput" } } %>
       <div data-trade-form-target="tickerInput">
         <div class="form-field combobox">
-          <%= form.combobox :ticker, securities_account_trades_path(entry.account), label: t(".holding"), placeholder: t(".ticker_placeholder"), autocomplete: :list, name_when_new: "account_entry[ticker]" %>
+          <%= form.combobox :ticker, securities_account_trades_path(entry.account), label: t(".holding"), placeholder: t(".ticker_placeholder"), autocomplete: :list, free_text: true %>
         </div>
       </div>
 
diff --git a/app/views/account/trades/securities.turbo_stream.erb b/app/views/account/trades/securities.turbo_stream.erb
index 331cac8..75a08db 100644
--- a/app/views/account/trades/securities.turbo_stream.erb
+++ b/app/views/account/trades/securities.turbo_stream.erb
@@ -1,6 +1,3 @@
-<% if @securities.present? %>
-<%= async_combobox_options @securities, 
-    render_in: { partial: "account/trades/tickers" },
-    next_page: @pagy.next
-  %>
-<% end %>
+<%= async_combobox_options @securities,
+      render_in: { partial: "account/trades/tickers" },
+      next_page: @pagy.next %>

[Shpigford]

@josefarias Annnnnd amazing. That all works perfectly. 🙌 Thank you!

[Shpigford]

@zachgoll I'm having trouble figuring out why those tests are failing.

[zachgoll]

@Shpigford the system tests are just inputting a regular ticker string, which means the new select won't "select" the actual security to load the Security ID into the form field. Then, the form submission fails.

Here's a diff that fixes:

maybe $ git diff test/system/trades_test.rb 
diff --git a/test/system/trades_test.rb b/test/system/trades_test.rb
index c9f7835..bcff415 100644
--- a/test/system/trades_test.rb
+++ b/test/system/trades_test.rb
@@ -16,7 +16,9 @@ class TradesTest < ApplicationSystemTestCase
 
     open_new_trade_modal
 
-    fill_in "Ticker symbol", with: "NVDA"
+    fill_in "Ticker symbol", with: "AAPL"
+    select_combobox_option("Apple")
+
     fill_in "Date", with: Date.current
     fill_in "Quantity", with: shares_qty
     fill_in "account_entry[price]", with: 214.23
@@ -27,7 +29,7 @@ class TradesTest < ApplicationSystemTestCase
 
     within_trades do
       assert_text "Purchase 10 shares of AAPL"
-      assert_text "Buy #{shares_qty} shares of NVDA"
+      assert_text "Buy #{shares_qty} shares of AAPL"
     end
   end
 
@@ -38,6 +40,7 @@ class TradesTest < ApplicationSystemTestCase
 
     select "Sell", from: "Type"
     fill_in "Ticker symbol", with: aapl.ticker
+    select_combobox_option(aapl.security.name)
     fill_in "Date", with: Date.current
     fill_in "Quantity", with: aapl.qty
     fill_in "account_entry[price]", with: 215.33
@@ -64,4 +67,10 @@ class TradesTest < ApplicationSystemTestCase
     def visit_account_trades
       visit account_url(@account, tab: "transactions")
     end
+
+    def select_combobox_option(text)
+      within "#account_entry_ticker-hw-listbox" do
+        find("li", text: text).click
+      end
+    end
 end

[zachgoll]

@josefarias just curious, but is that diff I shared above a reasonable way to run a system test against HW Combobox?

Or have you been recommending something a bit more stable than that?

[Shpigford]

@zachgoll Looks like those changes didn't get the tests to pass (at least didn't for me).

[zachgoll]

@Shpigford in 4c5d102 that exchange_mic NOT NULL filter caused our fixture securities to no longer show up in the dropdown. So we'll need to populate those to get the test to pass:

maybe $ git diff test/fixtures/securities.yml 
diff --git a/test/fixtures/securities.yml b/test/fixtures/securities.yml
index 20c676a..16d48a5 100644
--- a/test/fixtures/securities.yml
+++ b/test/fixtures/securities.yml
@@ -1,7 +1,9 @@
 aapl:
   ticker: AAPL
   name: Apple
+  exchange_mic: XNAS
 
 msft:
   ticker: MSFT
   name: Microsoft
+  exchange_mic: XNAS

[josefarias]

@josefarias just curious, but is that diff I shared above a reasonable way to run a system test against HW Combobox?

Or have you been recommending something a bit more stable than that?

Looks good! Here's what I use for the gem's test suite:

find("li[role=option]", exact_text: text).click

But I think you have enough specificity going on considering you're scoping by the listbox id.

[josefarias]

@Shpigford @zachgoll you might want to have some additional processing here.

Because you're returning search results on company names (in addition to ticker symbols), and also allowing free-text entry, people might enter e.g. "Apple". They'll see that the combobox returned an equivalent result, and blur the combobox without selecting anything.

At that point the combobox will think they meant to enter the literal string "Apple", and not "AAPL". I think that'll result in a Security with ticker "Apple" being created globally. But I haven't taken the time to really understand this builder class yet.

Might not be a problem in practice, just wanted to point that out.

[zachgoll]

This is a tricky one because for self-hosters who won't have the securities loaded yet, we'd still need to support the free-text entry.

That said, I think @Shpigford is working on a slightly different data model for securities + prices right now that may make it so we can be more strict with this input—i.e. only allowing IDs to be passed to the input's value and throwing a required client-side validation if not selected.

[josefarias]

That new data model solution sounds good!

If and when you switch to that, changing the combobox to autocomplete: :both (the default) instead of :list will always select the best match, too. So it'll help with the required UX.

[zachgoll]

@josefarias awesome, will do. Thanks for the heads up!

[zachgoll]

@josefarias just curious, but is that diff I shared above a reasonable way to run a system test against HW Combobox?
Or have you been recommending something a bit more stable than that?

Looks good! Here's what I use for the gem's test suite:

find("li[role=option]", exact_text: text).click

But I think you have enough specificity going on considering you're scoping by the listbox id.

@josefarias awesome, thank you!