Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add variant_sku and price #29

Open
savrand opened this issue Aug 21, 2014 · 15 comments
Open

Add variant_sku and price #29

savrand opened this issue Aug 21, 2014 · 15 comments

Comments

@savrand
Copy link

savrand commented Aug 21, 2014

I have added to csv fields as it is indicated https://github.com/autotelik/datashift_spree/wiki/Spree-Import-Export-Tips#variants
variant_sku . before saving
https://github.com/autotelik/datashift/blob/master/lib/loaders/loader_base.rb#L346
I output @load_object.variants and it shows me variants with filled sku as in csv but after saving they are changed to standard ones.

@autotelik
Copy link
Owner

Ok this is a bug in datashift_spree products loader

See method add _ options _ variants

It sets the sku automatically on the variant regardless, need to add a check and only auto generate the sku if not present in the spreadsheet

Sent from Yahoo Mail on Android

@autotelik
Copy link
Owner

Hmmm maybe im talking nonsense... seems actually as long as variant_sku column appear AFTER variants column this should not matter as the auto sku should be over written

@autotelik
Copy link
Owner

I cannot change at moment but I believe bug in line 136 - 138 of product loader

else
super
end

The call to super will not work in this case as the column name is actually 'sku' not 'variant_sku' so specific code needs adding here to assign the sku to the Variant

@autotelik
Copy link
Owner

something like .....

if(@load_object.variants.size > 1)
@load_object.variants.each_with_index {|v, i| v.sku = "#{current_value}_#{i}" }
else
@load_object.variants.first.sku = current_value.to_s
end

@load_object.save

@fredericboivin
Copy link

This is still an issue.

I don't completely understand all the code so I'm not sure where to stand but from what I gather, the variant_sku are correctly assigned but not saved.

When the loop is done, @load_object.save is called, which attemps to save the master product only, so it seems like the Variant SKUs are getting replaced locally but the @load_object affects only the master and the changes are not being persisted.

Since they use the same code, I guess it affects variant_price and count_on_hand equally.

Any suggestions?

@autotelik
Copy link
Owner

Some changes to fix count_on_hand for Spree 2 have literally just been checked in.

Sorry bit hard to follow the trail ..can you specify exactly which fields are not being persisted as you expect .. On Variants ?

sku, price and count_on_hand ? For what version of Spree ? And using master branch from github (not gem) ?

thanks
tom

@fredericboivin
Copy link

I'm using the latest version of everything :

Datashift v0.16.0@d561aa (from github)
Datashift_spree v.0.6.0@d6e271 (from github)

Spree 2.4.3

And I'm trying to assign specific SKUs to variant by using variant_sku, "sku1|sku2|sku3"

I've found the issue though :

SKU are not updated because the children (Spree::Variant) are updated locally through the parent (Spree::Product) and Spree::Product does not accepts_nested_attributes for Spree::Variant so the changes on the children are not persisted when saving the parent (@load_object.save)

So it's possible to replace @load_object.save by

@load_object.variants.each do |i|
i.save
end

Or we could use v.update_attributes( sku: values[i].to_s") instead.

I don't know the project well enough to know the best way to fix it, what are your thoughts?

(English is not my mother tongue so sorry if it's not exactly clear)

I'm referring to this part of the code

elsif(current_method_detail.operator?('variant_sku') && current_value)
      if(@load_object.variants.size > 0)

        if(current_value.to_s.include?(Delimiters::multi_assoc_delim))

          # Check if we processed Option Types and assign  per option
          values = current_value.to_s.split(Delimiters::multi_assoc_delim)

          if(@load_object.variants.size == values.size)
            @load_object.variants.each_with_index {|v, i| v.sku = values[i].to_s}
            @load_object.variants.each do |i|
              i.save
            end
            @load_object.save
          else
            puts "WARNING: SKU entries did not match number of Variants - None Set"
          end

        end
      else
        super
      end

@redglory
Copy link
Contributor

redglory commented Mar 4, 2015

Hum... I've not changed any code regarding 'variant_sku' method and I got my custom variant sku's saved...

@fredericboivin
Copy link

I will post a more detailed use case later today with a CSV sample, dependencies version, log entries from Datashift and resulting SQL.

@fredericboivin
Copy link

Sample CSV

SKU Name ShippingCategory Price Option Types Option Types Option Types variant_sku
ILJR0247 TSHIRT PROMO DEFAUT 100 color:Q; size:M color:Q; size:G color:A; size:TP 10583856

Resultings SKU :

| ILJR0247_1
| ILJR0247_2
| ILJR0247_3

Variant_SKU is processed without problem but values aren't overwritten. From the logs, it's clear that after looping over @load_object. variants, the SKUs are replaced locally but the changes aren't persisted when the Product is saved.

If you look at a relevant part of the log, you see that no UPDATE is being called on any variant.

http://pastebin.com/uLLc8NTC

Could you post a sample of your working CSV ? Also, which version of Rails/Spree are you running?

I might be doing something wrong but still, if I save the Product.variants explicitly in the 'variant_sku' block, it works

I'm curious about your thoughts on this

@autotelik
Copy link
Owner

redglory kindly commited some fixes all around this yesterday (936b96e)

Have you tried pulling this latest from master ? if this does not fix your problem just let me know and I'll try and go through your examples/logs.

@autotelik
Copy link
Owner

@load_object.variants.each_with_index {|v, i| v.sku = values[i].to_s }
@load_object.save

I assume save on Parent product peculates down to associations but given what you see perhaps not

I don't have time to dev/test this right now, so if you're desperate ..try changing

Line 154 .... @load_object.variants.each_with_index {|v, i| v.sku = values[i].to_s }

To save the Variant itself, something like :

@load_object.variants.each_with_index do |v, i|
v.sku = values[i].to_s
begin
v.save!
rescue => x
logger.error "Warning SKU not set on Variant #{v.inspect} #{e.message}"
end

cheers
tom

@fredericboivin
Copy link

I will try again with the master branch and get back to you

Cheers

@joeswann
Copy link

Just ran into pretty much this issue with spree 3.0.0 and variant_price

Changing line 112 to this seemed to do the trick
@load_object.variants.each_with_index {|v, i| v.update_attributes(price: values[i].to_f) }

@ping0xFF
Copy link

ping0xFF commented Mar 8, 2016

@joeswann Thanks, that fixed it per your suggestion.

@load_object.variants.each_with_index {|v, i| v.update_attributes(sku: values[i].to_f) }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants