From a7574f0864775871c96a7d208669963324248d19 Mon Sep 17 00:00:00 2001 From: Matchu Date: Tue, 25 Jun 2013 23:40:02 -0700 Subject: [PATCH] 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 | 28 ++++++++++----------- app/models/closet_page.rb | 28 +++++++++++++++------ 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/app/controllers/neopets_pages_controller.rb b/app/controllers/neopets_pages_controller.rb index 8bdeb717..dbc27078 100644 --- a/app/controllers/neopets_pages_controller.rb +++ b/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 diff --git a/app/models/closet_page.rb b/app/models/closet_page.rb index 54ff999c..e529124f 100644 --- a/app/models/closet_page.rb +++ b/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