search

geminimvp / solidus_asset_variant_options

Using image assets across multiple variants
BSD 3-Clause "New" or "Revised" License
1 stars 0 forks source link

Validation on Spree::VariantImage inhibiting any image upload #4

Open bradediger opened 7 years ago

bradediger commented 7 years ago
  1. http://localhost:3000/admin/products/ruby-on-rails-jr-spaghetti/images
  2. Upload image
  3. Expected image to be uploaded, got error: "Variant images is invalid"

Due to: https://github.com/rails/rails/issues/25140

Commenting out this validation fixes:

https://github.com/geminimvp/solidus_asset_variant_options/blob/07070fbbe05c322a18ab0824b2c2553a50898177/app/models/spree/variant_image.rb#L12

bradediger commented 7 years ago

FYI, we just logged this to make a note of it while we were working today. Locally nulled out this validation for development at the moment.

fastjames commented 7 years ago

Can you verify that your image upload is properly creating the association record, with both association IDs in the database?

fastjames commented 7 years ago

My current hypothesis is that this is a bug in Rails, and as a result using the conventional

variant.images << some_image

will always result in an invalid association record. It seems irrational, since such a bug would (I think) affect a lot of apps, but I haven't found any evidence yet that the behavior is specific to our app.

bradediger commented 7 years ago

It appears to all be there:

engine_storefront_dev=# select count(*), count(image_id), count(variant_id) from spree_assets_variants;
 count | count | count 
-------+-------+-------
    45 |    45 |    45
(1 row)

All of the images that I attached during testing remained attached to the variant and could be observed through the blog interface as well as through Spree's admin products UX, so it doesn't feel to me like Rails just doesn't support the shovel operator here.

I wonder if the problem is related to the order of events within the validation / save lifecycle. I'd be perfectly wiling to believe that Rails hasn't set the IDs before the validation callback triggers.

fastjames commented 7 years ago

That's great news, because I really wanted to be wrong. I'll go back and revisit my original tests to see what might be causing the problem. I think the problem I had was within the spree_sample:load rake task so that's a good place to start.

bradediger commented 7 years ago

First place I would start looking is the Rails test suite. I would bet money that AR has a test already for the behavior you thought might have been broken (shoveling through a HMT sets all applicable IDs). Pulling that thread might lead you in the right direction to investigate the validation stuff.

Unfortunately I have not had time today to look into that, but I can add it to my list if it's not on your critical path.

fastjames commented 7 years ago

I took a look at this project and I noticed a distinct lack of :inverse_of in all the relationships. That may be important to this discussion.

fastjames commented 7 years ago

After re-examining this repo and the repo that's using it, I have come to the conclusion that I was making the whole thing up. I didn't commit any changes that removed the use of the shovel operator in either repo, so it's still working just as it did before. I'll make a PR to remove this validation and pick the whole issue back up if the problem reappears.

fastjames commented 7 years ago

A slightly more charitable guess might be that our initial overriding of Variant#images (for displaying images other than those directly associated with the Variant) was causing problems with the shovel operator. With that removed, everything works as expected now.