Reimplement Spree's simple_product in OFN factory #2493
Conversation
|
On |
|
A lot of tests still fail because of |
There was a problem hiding this comment.
Hey, the simple_product was just an extension of base_product with on_hand = 5.
I am pretty sure tests will depend on that number (5) to pass. How are we planning to address that? Have you considered not changing from simple to base but instead create a new simple product that still extends base product and would address the "on_hand" thing in a spree2 friendly manner?
There was a problem hiding this comment.
While this fixes some tests, it doesn't fix the underlying problem that we are relying on Spree's factories here. This change is typical shotgun surgery. The solution would be to create our own simple_product. Long term, I would like to only use our own factories. I always find it annoying to look for a factory, not finding it in our code base and then needing to search the Spree modules to find it.
https://refactoring.guru/smells/shotgun-surgery
There was a problem hiding this comment.
Agree with @mkllnk, watching for shotgun surgery will make things more changeable in the future. And we know more upgrades will come.
|
Ok so I should instead create our own product factory, is that right ? |
|
and call it |
|
Should I take into account https://github.com/openfoodfoundation/openfoodnetwork/pull/2417/files and keep a |
|
@HugsDaniel I think you should keep what's in there. on_hand? I believe we will still have on_hand available after the upgrade. even if only an adapter. |
HugsDaniel
force-pushed
the
2433-change-from-simple-product-to-base-product
branch
from
7ab57b7 to
eab04e0
Compare
HugsDaniel
changed the title
|
Yes @HugsDaniel. You can assume that there will be methods called |
spec/factories.rb
Outdated
| @@ -526,4 +530,5 @@ | |||
| user.spree_roles << Spree::Role.find_or_create_by_name!('admin') | |||
| end | |||
| end | |||
|
|
|||
There was a problem hiding this comment.
I guess this slipped in accidentally?
There was a problem hiding this comment.
Yes it did, sorry about that
What? Why?
Closes #2433
As explained in the PR, Spree's product factory got rid of
simple_productin favor ofbase_product, making all of our tests usingsimple_productfail.I reimplemented the Spree<2.0
simple_productfactory inside OFNspec/factories.rb, inheriting from OFN's override of Spree'sbase_productfactory.As before, a
simple_productis an extension of abase_productwithon_hand 5.What should we test?
All tests using Spree's
product_factorydon't fail onsimple_product.Release notes
Spree Upgrade - Reimplementation in OFN of Spree's
simple_product, inheriting frombase_product, as a test helper.Changelog Category: Changed
How is this related to the Spree upgrade?
Spree 2.0 removed
simple_productin theirproduct_factory, so it has been reimplemented in OFN factories to enable its use in test suite.