Don't add duplicate hangers now that closet import can specify a list

Bug report that this resolves:

...However, when I was using the "Import from SDB" tool just a few
minutes ago, it ended up adding EVERY neocash item into the "Not
In A List" section, regardless if I already that item imported
into my "Your Items". So, basically.. I had duplicates of
everything and it would not allow me to move them around into
separate catergories or anything. I know that every other time i've
used the import tool, it would only add NEW items that are not
currently already in my lists yet.

app/controllers/neopets_pages_controller.rb

@@ -3,7 +3,7 @@ class NeopetsPagesController < ApplicationController
 
   before_filter :authenticate_user!, :build_neopets_page
 
-  rescue_from ClosetPage::ParseError, :with => :on_parse_error
+  rescue_from ClosetPage::ParseError, with: :on_parse_error
 
   def create
     if @page_params && @page_params[:source]
@@ -12,25 +12,25 @@ class NeopetsPagesController < ApplicationController
       @neopets_page.source = @page_params[:source]
 
       messages = [t('neopets_pages.create.success',
-                    :index => @neopets_page.index)]
+                    index: @neopets_page.index)]
 
       saved_counts = @neopets_page.save_hangers!
       any_created = saved_counts[:created] > 0
       any_updated = saved_counts[:updated] > 0
       if any_created && any_updated
         created_msg = t('neopets_pages.create.created_and_updated_hangers.created_msg',
-                        :count => saved_counts[:created])
+                        count: saved_counts[:created])
         updated_msg = t('neopets_pages.create.created_and_updated_hangers.updated_msg',
-                        :count => saved_counts[:updated])
+                        count: saved_counts[:updated])
         messages << t('neopets_pages.create.created_and_updated_hangers.text',
-                      :created_msg => created_msg,
-                      :updated_msg => updated_msg)
+                      created_msg: created_msg,
+                      updated_msg: updated_msg)
       elsif any_created
         messages << t('neopets_pages.create.created_hangers',
-                      :count => saved_counts[:created])
+                      count: saved_counts[:created])
       elsif any_updated
         messages << t('neopets_pages.create.updated_hangers',
-                      :count => saved_counts[:updated])
+                      count: saved_counts[:updated])
       elsif @neopets_page.hangers.size > 1 # saw items, but at same quantities
         messages << t('neopets_pages.create.no_changes')
       else # no items recognized
@@ -39,16 +39,16 @@ class NeopetsPagesController < ApplicationController
 
       unless @neopets_page.unknown_item_names.empty?
         messages << t('neopets_pages.create.unknown_items',
-                      :item_names => @neopets_page.unknown_item_names.to_sentence,
-                      :count => @neopets_page.unknown_item_names.size)
+                      item_names: @neopets_page.unknown_item_names.to_sentence,
+                      count: @neopets_page.unknown_item_names.size)
       end
 
       if @neopets_page.last?
-        messages << t('neopets_pages.create.done', :name => @neopets_page.name)
+        messages << t('neopets_pages.create.done', name: @neopets_page.name)
         destination = user_closet_hangers_path(current_user)
       else
         messages << t('neopets_pages.create.next_page',
-                      :next_index => (@neopets_page.index + 1))
+                      next_index: (@neopets_page.index + 1))
         destination = {action: :new, index: @neopets_page.index + 1,
                        list_id: @neopets_page.list_id}
       end
@@ -56,7 +56,7 @@ class NeopetsPagesController < ApplicationController
       flash[:success] = messages.join(' ')
       redirect_to destination
     else
-      redirect_to :action => :new
+      redirect_to action: :new
     end
   end
 
@@ -82,7 +82,7 @@ class NeopetsPagesController < ApplicationController
   def on_parse_error(e)
     Rails.logger.info "Neopets page parse error: #{e.message}"
     flash[:alert] = t('neopets_pages.create.parse_error')
-    render :action => :new
+    render action: :new
   end
 end
 

app/models/closet_page.rb

@@ -35,7 +35,7 @@ class ClosetPage
   end
 
   def save_hangers!
-    counts = {:created => 0, :updated => 0}
+    counts = {created: 0, updated: 0}
     ClosetHanger.transaction do
       @hangers.each do |hanger|
         if hanger.new_record?
@@ -164,17 +164,29 @@ class ClosetPage
     )
 
     # Create closet hanger from each item, and remove them from the reference
-    # lists
-    hangers_scope = @closet_list ? @closet_list.hangers :
-                                   @user.closet_hangers.unlisted.where(owned: @hangers_owned)
+    # lists.
+    # We don't want to insert duplicate hangers of what a user owns if they
+    # already have it in another list (e.g. imports to Items You Own *after*
+    # curating their Up For Trade list), so we check for the hanger's presence
+    # in *all* items the user owns or wants (whichever is appropriate for this
+    # request).
+    hangers_scope = @user.closet_hangers.where(owned: @hangers_owned)
     @hangers = items.map do |item|
       data = items_data[:id].delete(item.id) ||
         items_data[:thumbnail_url].delete(item.thumbnail_url)
       hanger = hangers_scope.find_or_initialize_by_item_id(item.id)
-      hanger.user = @user
-      hanger.owned = @hangers_owned
-      hanger.list = @closet_list
-      hanger.quantity = data[:quantity]
+
+      # We also don't want to move existing hangers from other lists, so only
+      # set the list if the hanger is new.
+      hanger.list = @closet_list if hanger.new_record?
+
+      # Finally, we don't want to update the quantity of hangers in those other
+      # lists, either, so only update quantity if it's in this list. (This will
+      # be true for some existing hangers and all new hangers. This is also the
+      # only value that could change for existing hangers; if nothing changes,
+      # it was an existing hanger from another list.)
+      hanger.quantity = data[:quantity] if hanger.list == @closet_list
+
       hanger
     end