Moves Menu query logic to component ruby #2441
Conversation
app/furniture/marketplace/product.rb
Outdated
| @@ -44,6 +44,8 @@ class Product < Record | |||
| scope :with_menu_tag, -> { joins(:tags).merge(Tag.menu_tag) } | |||
| scope :without_menu_tag, -> { where.not(id: with_menu_tag) } | |||
| scope :sort_alpha, -> { order(name: :asc) } | |||
| scope :menu_tag_product, -> { with_all_rich_text.merge(unarchived).merge(sort_alpha) } | |||
| scope :menu_product, -> { with_all_rich_text.merge(unarchived).merge(without_menu_tag).merge(sort_alpha) } | |||
There was a problem hiding this comment.
A few things about these two new scopes raise questions for me:
- why are you
mergeing scopes from the same model? i don't think you need nor want to do that; you can just do{... unarchived.without_menu_tag.sort_alpha ...} - do we want to encode eager-loading as part of a scope? we don't typically do that, because it makes the scope only suitable for that one particular use case, and bloated for other purposes; in this example, if you wanted to count the menu_products, this scope would be unnecessarily eager-loading all rich text in order to do that count
- these are scopes on a Product model, so adding
productto the scope names seems redundant (i.e.Product.menuis a better scope name thanProduct.menu_product) - it seems like the
menu_tag_productscope is defined as a superset of themenu_product, which doesn't seem right
There was a problem hiding this comment.
do we want to encode eager-loading as part of a scope
I think this makes sense to do now that I've moved the scope to methods on the component
app/furniture/marketplace/tag.rb
Outdated
| @@ -5,6 +5,8 @@ class Tag < Record | |||
| belongs_to :marketplace, inverse_of: :tags | |||
| has_many :product_tags, inverse_of: :tag, dependent: :destroy | |||
| has_many :products, through: :product_tags, inverse_of: :tags | |||
| has_many :menu_tag_products, -> { menu_tag_product }, through: :product_tags, source: :product, inverse_of: :tags | |||
| has_many :menu_products, -> { menu_product }, through: :product_tags, source: :product, inverse_of: :tags | |||
There was a problem hiding this comment.
I see the menu_tag_products relationship used in this PR, but not menu_products.
I definitely do not want to use a relationship that uses a scope that includes eager-loading, that doesn't seem right to me.
rosschapman
changed the title
There was a problem hiding this comment.
@anaulin Here's another stab where these queries (scope combos) live on the component since they are very specific to the menu interface. Notionally it feels like heavy lifting for a "view" component, but it might feel similarly awkward if we hoisted this logic to the Marketplace controller. Maybe something like a Marketplace::Menus would be a domain object model to move toward to encapsulate this responsibility. Then the concept of tag and menu are disassociated from the model's public interface.
psuedo-code:
class Marketplace
class Menus
has_many menus, through: tag|product_tag, -> (order: asc)
has_many products, through: marketplace
scope all_products, ...
def menus_ordered
end
def products_for_menu(menu)
end
end
endThere was a problem hiding this comment.
I like this a lot. If these scope combinations are only used in this component, putting them in the component seems great; it makes the ERB for the component cleaner without cluttering the model scopes.
No description provided.