Deleting a variant has no direct effect on a shopfront

[RachL]

Description

If you delete a variant, it keeps showing up on the shopfront until you save a second time changes on your product.

If someone attempts to add this deleted variant to cart it gets a infinite loader

I saw this while testing #3619 but could reproduced it on French production

Steps to Reproduce

1. I deleted a variant that already existed on a staging server
2. I saw that it was correctly deleted from the OC but not the shopfront (I cleared my browser cache several times)
3. I've added a new variant to my product. At this stage - as I saved again - the variant previously deleted disappear from the shop. The new variant showed up correctly
4. I deleted this new variant. Again it did not showed up in the OC, but on the shopfront yes.
5. I change the price of another variant, hit save. Then my variant is correctly deleted from shop front.

I reproduce all this on French product but this time I also tried to add the deleted variant to card. I got an infinite loader on the add to cart button:

Reloading the page stop the loader, but my variant is still showing up. It's the v2 variant bottom page here: https://www.openfoodfrance.org/hub-demo-open-food-france/shop

Animated Gif/Screenshot

Severity

bug-s2: a non-critical feature is broken, no workaround

Your Environment

• Version used: v1.28
• Browser name and version: Firefox 65
• Operating System and version (desktop or mobile): Ubuntu 18.10

[luisramos0]

This looks like a cache problem. With the new feature to disable cache we can disable cache and see if this happens with the cache disabled :-)

[mkllnk]

This could be a duplicate of #2111. In that case it's no cache problem.

[mkllnk]

Changing the product a second time is a workaround, isn't it? Or first remove it from the order cycle and then delete it.

[mkllnk]

I looked into this and we have all the code to update the cache when a variant is destroyed. But we never call destroy, we call delete for a soft-delete. And that method doesn't touch the cache.

The current implementation:

openfoodnetwork/app/models/spree/variant_decorator.rb

Lines 127 to 143 in 130ba64

	def destruction
	if is_master?
	exchange_variants(:reload).destroy_all
	yield
	product.refresh_products_cache
	else
	OpenFoodNetwork::ProductsCache.variant_destroyed(self) do
	# Remove this association here instead of using dependent: :destroy because
	# dependent-destroy acts before this around_filter is called, so ProductsCache
	# has no way of knowing which exchanges the variant was a member of.
	exchange_variants(:reload).destroy_all
	# Destroy the variant
	yield
	end
	end

Where we should apply it:

openfoodnetwork/app/models/spree/variant_decorator.rb

Lines 100 to 111 in 130ba64

	def delete
	if product.variants == [self] # Only variant left on product
	errors.add :product, I18n.t(:spree_variant_product_error)
	false
	else
	transaction do
	self.update_column(:deleted_at, Time.zone.now)
	ExchangeVariant.where(variant_id: self).destroy_all
	self
	end
	end
	end

This would be solved with Spree 2 because we get rid of our soft-delete implementation. Spree 2 always soft deletes when calling destroy.

I'm calling it a day now. Feel free to work on this issue. Otherwise I may have time tomorrow to work on this.

[RachL]

Changing the product a second time is a workaround, isn't it? Or first remove it from the order cycle and then delete it.

Yes you are right. I'm downgrading to s3.

[sauloperez]

Thanks for the investigation @mkllnk. I think I can take it and fix it given my last week's cache deep-dive.

[sauloperez]

#3639 fixes this in master but would you mind testing it in v2 as well? I just checked and there is no bug in v2. I want to be extra sure though. Thanks for taking care of it @mkllnk.

[mkllnk]

I'm not sure whom you asked to test it @sauloperez. Did you mean @RachL or me to test this?

@RachL would you mind testing this when you are next testing v2 anyway?

[RachL]

@mkllnk will do!

[RachL]

@mkllnk @sauloperez good catch, the issue remains in v2. I will open an issue for it.

[RachL]

@mkllnk @sauloperez ok nevermind when I retested again to write the issue i couldn't reproduced it.

[luisramos0]

have you enabled cache in the backoffice?

[RachL]

@luisramos0 it is enabled by default I guess? Because I just saw that the bow is ticked, so I guess it was ticked when I did my test?

[luisramos0]

yeah, if it is enabled now, I think it's safe to assume it was enabled when you tested.