detect should be replaced by find_by

[lelelelemon]

ror_ecommerce/app/views/admin/rma/return_authorizations/_form.html.erb

<% return_item = @return_authorization.return_items.detect {|p| p.order_item_id == item.id } %> will issue a query to get all return_items of @return_authorization, and then detect them to find the one with specified item.id. However, use find_by will only issue the query use where condition @return_authorization.return_items.find_by(order_item_id: item.id)

[lelelelemon]

diff --git a/app/views/admin/rma/return_authorizations/_form.html.erb b/app/views/admin/rma/return_authorizations/_form.html.erb
index 05b3fe4a..0e0a56b0 100644
--- a/app/views/admin/rma/return_authorizations/_form.html.erb
+++ b/app/views/admin/rma/return_authorizations/_form.html.erb
@@ -25,7 +25,7 @@
     <% @order.order_items.each do |item| %>
       <div  id='image_<%= item.id %>'
             class="variant_form left last">
-        <% return_item = @return_authorization.return_items.detect {|p| p.order_item_id == item.id } %>
+        <% return_item = @return_authorization.return_items.find_by(order_item_id: item.id) %>
         <% return_item = return_item || ReturnItem.new( order_item: item) %>
         <% return_item.updated_by = current_user.id %>
         <%= f.fields_for :return_items, return_item do |item_form|%>

[lelelelemon]

same here

diff --git a/app/views/admin/merchandise/products/_shipping_rates_form.html.erb b/app/views/admin/merchandise/products/_shipping_rates_form.html.erb
index b95fc8b..7a6332e 100644
--- a/app/views/admin/merchandise/products/_shipping_rates_form.html.erb
+++ b/app/views/admin/merchandise/products/_shipping_rates_form.html.erb
@@ -1,6 +1,6 @@
 <% @all_shipping_rates.each do |rate| %>
     <div  id='rate_<%= rate.id %>' class="property_form left span-9 last">
-      <% checked_rate = @product.shipping_categories.detect {|c| c.shipping_rate_id == rate.id } %>
+      <% checked_rate = @product.shipping_categories.find_by(shipping_rate_id: rate.id) %>
       <div class='span-8'> 
 
         <%= check_box_tag "product[shipping_rate_ids][]", rate.id, checked_rate ? true : false %>

[lelelelemon]

diff --git a/app/views/admin/merchandise/wizards/products/_form.html.erb b/app/views/admin/merchandise/wizards/products/_form.html.erb
index 947387a..345eb2a 100644
--- a/app/views/admin/merchandise/wizards/products/_form.html.erb
+++ b/app/views/admin/merchandise/wizards/products/_form.html.erb
@@ -41,7 +41,7 @@
     <% @all_properties.each do |property| %>
       <div id='property_<%= property.id %>'
             class="property_form left last"
-            <%= "style='display:none;'" if @product.id && [email protected] {|p| p.id == property.id} %>>
+            <%= "style='display:none;'" if @product.id && [email protected]_by(id: property.id) %>>
         <%= form.fields_for :product_properties, property.product_properties.find_or_initialize_by(product_id: @product.id) do |product_property_form|%>
           <label><%= property.full_name %></label>
           <%= product_property_form.hidden_field :property_id %>

[drhenner]

   <% @order.order_items.each do |item| %>
     <div  id='image_<%= item.id %>'
         class="variant_form left last">
        <% return_item = @return_authorization.return_items.detect {|p| p.order_item_id == item.id } %>
        <% return_item = @return_authorization.return_items.find_by(order_item_id: item.id) %>

If I do a find_by I would be doing it for each order_item which by the end of this loop I'd be doing a find_by for each order_item_id in the order.

Hence if there is one return item but 20 order_items, There would be 20 SQL queries here. Yet my current code just does one SQL query. I'm up for changing this pattern but maybe by moving the logic out of the view.

[lelelelemon]

I know this will create N + 1 queries, but using detect will cause unnecessary data retrieval, either will have some problems. I think removing N + 1 is much more important.

[drhenner]

After iterating through all the order_items all the return_items will be needed. So I don't think there is a case for the @return_authorization.return_items case to have unnecessary data retrieval.

Either way I agree I think the N + 1 is more important.

Thanks for pointing all this out. You are motivating me to keep adding more things to the repo.

[Taimoor2500]

why not do this in controller @return_items_by_order_item = @return_authorization.return_items.index_by(&:order_item_id)

and in view implement <% return_item = @return_items_by_order_item[item.id] || ReturnItem.new(order_item: item) %>

all necessary data can be loaded beforer view renders