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