Move more of the trade-fetching logic into the model

It was a bit tricky to figure out the right API for this, since I'm
looking ahead to the possibility of splitting these across multiple
pages with more detail, like we do in DTI 2020.

What I like about this API is that the caller gets to apply, or not
apply, whatever scopes they want to the underlying hanger set (like
`includes` or `order`), without violating the usual syntax by e.g.
passing it as a parameter to a method.

app/controllers/items_controller.rb

@@ -58,19 +58,11 @@ class ItemsController < ApplicationController
 
     respond_to do |format|
       format.html do
+        @trades = @item.closet_hangers.trading.includes(:user).user_is_active.
+          order('users.last_trade_activity_at DESC').to_trades
+
         @contributors_with_counts = @item.contributors_with_counts
 
-        trading_closet_hangers = @item.closet_hangers.trading.includes(:user).
-          user_is_active.order('users.last_trade_activity_at DESC')
-
-        owned_trading_hangers = trading_closet_hangers.filter { |c| c.owned? }
-        wanted_trading_hangers = trading_closet_hangers.filter { |c| c.wanted? }
-
-        @trading_users_by_owned = {
-          true => owned_trading_hangers.map(&:user).uniq,
-          false => wanted_trading_hangers.map(&:user).uniq,
-        }
-
         if user_signed_in?
           # Empty arrays are important so that we can loop over this and still
           # show the generic no-list case

app/helpers/items_helper.rb

@@ -79,14 +79,9 @@ module ItemsHelper
     "https://www.neopets.com/genie.phtml?type=process_genie&criteria=exact&auctiongenie=#{CGI::escape item.name}"
   end
 
-  def trading_users_header(owned, count)
+  def render_trades(trades)
-    ownership_key = owned ? 'owned' : 'wanted'
+    trades.map do |trade|
-    translate ".trading_users.header.#{ownership_key}", :count => count
+      link_to trade.user.name, user_closet_hangers_path(trade.user)
-  end
-
-  def render_trading_users(owned)
-    @trading_users_by_owned[owned].map do |user|
-      link_to user.name, user_closet_hangers_path(user)
     end.to_sentence.html_safe
   end
   

app/models/closet_hanger.rb

@@ -153,6 +153,38 @@ class ClosetHanger < ApplicationRecord
     # If quantity is zero and there's no hanger, good. Do nothing.
   end
 
+  # Use this with a scoped relation to convert it into a list of trades, e.g.
+  # `item.hangers.trading.to_trades`.
+  #
+  # A trade includes the user who's trading, and the available closet hangers
+  # (which you can use to get e.g. the list name).
+  #
+  # We don't preload anything here - if you want user names or list names, you
+  # should `includes` them in the hanger scope first, to avoid extra queries!
+  Trade = Struct.new('Trade', :user_id, :hangers) do
+    def user
+      # Take advantage of `includes(:user)` on the hangers, if applied.
+      hangers.first.user
+    end
+  end
+  def self.to_trades
+    # Let's ensure that the `trading` filter is applied, to avoid data leaks.
+    # (I still recommend doing it at the call site too for clarity, though!)
+    all_trading_hangers = trading.to_a
+
+    owned_hangers = all_trading_hangers.filter(&:owned?)
+    wanted_hangers = all_trading_hangers.filter(&:wanted?)
+
+    # Group first into offering vs seeking, then by user.
+    offering, seeking = [owned_hangers, wanted_hangers].map do |hangers|
+      hangers.group_by(&:user_id).map do |user_id, user_hangers|
+        Trade.new(user_id, user_hangers)
+      end
+    end
+
+    {offering: offering, seeking: seeking}
+  end
+
   protected
 
   def list_belongs_to_user

app/views/items/show.html.haml

@@ -29,14 +29,20 @@
           ✨⏳️
           %i We now only show recently-updated lists here!
           ⏳️✨
-      - [true, false].each do |owned|
+      %p
-        %p
+        %strong
-          %strong
+          = t '.trading_users.header.owned', count: @trades[:offering].size
-            = trading_users_header(owned, @trading_users_by_owned[owned].size)
+        = render_trades(@trades[:offering])
-          = render_trading_users(owned)
+        %span.toggle
-          %span.toggle
+          %button.more= t '.trading_users.show_more'
-            %button.more= t '.trading_users.show_more'
+          %button.less= t '.trading_users.show_less'
-            %button.less= t '.trading_users.show_less'
+      %p
+        %strong
+          = t '.trading_users.header.wanted', count: @trades[:seeking].size
+        = render_trades(@trades[:seeking])
+        %span.toggle
+          %button.more= t '.trading_users.show_more'
+          %button.less= t '.trading_users.show_less'
 
   - if user_signed_in?
     #your-items-form