From a03ae90697af4345c8feef4d82d9dd6101bcb58d Mon Sep 17 00:00:00 2001
From: Emi Matchu <emi@matchu.dev>
Date: Sun, 21 Jan 2024 00:39:20 -0800
Subject: [PATCH] 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 | 14 +++----------
 app/helpers/items_helper.rb         | 11 +++-------
 app/models/closet_hanger.rb         | 32 +++++++++++++++++++++++++++++
 app/views/items/show.html.haml      | 22 ++++++++++++--------
 4 files changed, 52 insertions(+), 27 deletions(-)

diff --git a/app/controllers/items_controller.rb b/app/controllers/items_controller.rb
index ddb2cf12..73992ff1 100644
--- a/app/controllers/items_controller.rb
+++ b/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
diff --git a/app/helpers/items_helper.rb b/app/helpers/items_helper.rb
index edce0c3a..63bc3210 100644
--- a/app/helpers/items_helper.rb
+++ b/app/helpers/items_helper.rb
@@ -78,15 +78,10 @@ module ItemsHelper
   def auction_genie_url_for(item)
     "https://www.neopets.com/genie.phtml?type=process_genie&criteria=exact&auctiongenie=#{CGI::escape item.name}"
   end
-  
-  def trading_users_header(owned, count)
-    ownership_key = owned ? 'owned' : 'wanted'
-    translate ".trading_users.header.#{ownership_key}", :count => count
-  end
 
-  def render_trading_users(owned)
-    @trading_users_by_owned[owned].map do |user|
-      link_to user.name, user_closet_hangers_path(user)
+  def render_trades(trades)
+    trades.map do |trade|
+      link_to trade.user.name, user_closet_hangers_path(trade.user)
     end.to_sentence.html_safe
   end
   
diff --git a/app/models/closet_hanger.rb b/app/models/closet_hanger.rb
index 1ade8c03..dde61fc4 100644
--- a/app/models/closet_hanger.rb
+++ b/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
diff --git a/app/views/items/show.html.haml b/app/views/items/show.html.haml
index fd861110..77d946dd 100644
--- a/app/views/items/show.html.haml
+++ b/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
-          %strong
-            = trading_users_header(owned, @trading_users_by_owned[owned].size)
-          = render_trading_users(owned)
-          %span.toggle
-            %button.more= t '.trading_users.show_more'
-            %button.less= t '.trading_users.show_less'
+      %p
+        %strong
+          = t '.trading_users.header.owned', count: @trades[:offering].size
+        = render_trades(@trades[:offering])
+        %span.toggle
+          %button.more= t '.trading_users.show_more'
+          %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