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

BUG Variation - currently is hard coded to look for a has_one relation to Product #106

Open
jsirish opened this issue Jul 18, 2020 · 3 comments
Milestone

Comments

@jsirish
Copy link
Member

jsirish commented Jul 18, 2020

Describe the bug
Currently there are multiple methods where a Variation is looking for a has_one Product. Throughout the rest of the Foxy modules, we've tried to keep this more flexible so that multiple classes in the same project could have ecommerce functionality - such as a Product, Event, etc - without having to extend a base Product class

Expected behavior
Multiple, unrelated classes could have Foxy extensions such as Purchasable and Shippable applied to them and still work with Variations

Additional context
We could explore a generic has_one Product => SiteTree::class relationship. This would limit products to being Pages, but there are already instances across Foxy modules where we've made this assumption. So assuming that would work, it would be an acceptable implementation based on the current code structure.

@jsirish jsirish added this to the 1.3.0 milestone Jul 18, 2020
@muskie9
Copy link
Member

muskie9 commented Jul 18, 2020

I think this would have to be more polymorphic and do 'Product' => DataObject::class similar to the example in the Silverstripe docs for Polymorphic has_one. I think with how this is currently setup, the assumption is the relation name vs the class on the actual relation. The has_one has to be implemented by the developer per project, so it could end up being any class, however if there is no common base class issues could arise.

I could be misremembering, but I'm for polymorphic relations if they work as they allow the flexibility we're looking for.

@jsirish
Copy link
Member Author

jsirish commented Jul 18, 2020

The polymorphic works in some cases. However, the issue we run into is when we want to query by a particular field, such as Code, that does not exist on that base class, be it DataObject or SiteTree. I ran into this recently with the Inventory checks for Variations.

Somehow we've made it this far without hard coding a base class in most cases, silverstripe-foxy-products being the exception (by design).

There is this method in FoxyHelper that gets a list of the array of classes spec'd as "Products", which is used in the FoxyHelper->getProducts() method. Maybe there is an opportunity here:

https://github.com/dynamic/silverstripe-foxy/blob/1.2/src/Model/FoxyHelper.php#L163

@muskie9
Copy link
Member

muskie9 commented Jul 18, 2020

I think something similar would work at this point and re-visiting how we can support the queries in a better way is worth another look. Now that you mention the field issue I recall why we have it setup this way. We could explore storing the "Owner" type class in a Varchar on a variation.

This type of setup would be similar to what ElementalArea does. We may have to review how this would then be leveraged, but that would give us the class of the related product. Depending on the relation calls, we could determine how that would or wouldn't be useful in those situations.

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

2 participants