Topic: Doing things the Rails way

I have an order form where the user will input hundreds of text fields at once, which is generated by an inventory helper function (thanks again ryanb!). Since I do not want to list what I have in my inventory here, since this is for new orders, I am forcing the quantity to zero (or a saved amount in the case of a previously open order). This seems like kind of a silly way to do it since I am querying one table to generate the form, but the data entered will be saved to a different table.


 for item in type.finished_items
...<table> ...
quantity = '0'
if x = find_by_finished_item_id_and_order_id(@item.id, @order.id
quantity = x.qty
end
table_str << text_field("item[]", "qty", :size => '4', :value => quantity)
... </table> ...
end

This creates params{item] which creates a hash, with many different hashes inside of of it, which each of those hashes pointing to one more hash, like this:
 "item"=>{"6603"=>{"qty"=>"0"}, "6624"=>{"qty"=>"0"}, "6525"=>{"qty"=>"4"}, ...

Because I am not updating to the model that created this form, I want to manipulate the params hash myself so I can update to the correct model. I do have it sort of working but I am pretty sure I am not doing it the proper RoR way. What I have right now is:

@order = find_order
    keys = Array.new(params[:item].keys)

#Removes the values from the params hash and dumps them into an array.
    qtys = Array.new
    params[:item].values.each {|x| qtys.push(x["qty"])}

#Go through each product key
    params[:item].keys.each {|key|

#check if an entry already exists for this order and product
      if x = FinishedLineItem.find_by_finished_item_id_and_order_id(key, @order.id)

#assign the new value to the old quantity
        x.qty = qtys[keys.index(key)]
        if x.qty > 0
          x.update

#if the new value is zero simply destroy the entry
        else
          x.destroy
        end

#if a previous entry does not exist, and the new value is not zero, add the entry to the DB.
      elsif qtys[keys.index(key)] != "0"
        FinishedLineItem.create(:finished_item_id => key, :qty =>qtys[keys.index(key)], :order_id => @order.id)
      end
    }


So to summarize, this code is looking at the params Hash and creating an array that is easier to work with. Then it looks to see if there is already a listed quantity for that particular order and product id. If yes, it updates the table with the new value. If the new value is zero it destroys the old row since I don't want to keep line items with a quantity of zero. If a row doesn't exist at all, it simply creates one. This seems to hit the database a little more than is needed as well.

Thanks for any advice anyone can provide!

Last edited by dahuk (2006-11-13 13:06:10)

Re: Doing things the Rails way

Whenever there's a problem like this, it helps to summarize the models and their relationship. I'm guessing Order has_many finished_line_items, and each FinishedLineItem belongs_to a finished_item. From what I can tell, you want to list all finished items providing a quantity field for the user to choose which items to add to the order. If the item is already in the order you want to display the correct quantity.

This is difficult because the interface does not parallel the models. I would consider changing the interface to make things easier. You can probably make improvements to it as well. For example, I would list all items, then make an "add item to order" button/link next to each. This will create a FinishedLineItem or add to the quantity if it already exists. If you want to keep it all on one page, you can use AJAX to do this.

Anyway, going back to your original problem. First of all this is too much logic in the controller. It is better to move this into the order model:

# in order.rb
def update_item_quantities(item_quantities)
  # ...
end

This method will handle the update, create, and destroying of the line items based on the quantity and if it already exists. This method will expect that same { 'item_id' => 'quantity' } hash you made in the action. Here's the code to go in that method:

def update_item_quantities(item_quantities)
  item_quantities.each do |item_id, quantity|
    if quantity.to_i > 0
      line_item = finished_line_items.find_or_create_by_finished_item_id(item_id)
      line_item.quantity = quantity
      line_item.save
    else
      line_items.destroy(['finished_item_id=?', item_id]) # untested!
    end
  end
end

That find_or_create_by_finished_item_id is a magic method rails provides. It pretty much does what it says. Now you need to somehow call this method in the controller with the proper hash. In fact, there's no need to transform the given params. Instead I suggest you use text_field_tag instead of the simple text_field so you can place the quantity in the hash directly. Like this:

<% for item in type.finished_items %>
  <%= text_field_tag "item_quantities[#{item.id}]", @order.quantity_of_item(item), :size => 4 %>
<% end %>

# in controller
@order = find_order
@order.update_item_quantities(params[:item_quantities])


Notice again we move the quantity fetching logic into the order model. You should move logic into models whenever you can (but never HTML). The method could look like this:

def quantity_of_item(item)
  line_item = finished_line_items.find_by_finished_item_id(item.id)
  if line_item.nil?
    0
  else
    line_item.quantity
  end
end

All of this code is completely untested, so use at your own risk.

Railscasts - Free Ruby on Rails Screencasts

Re: Doing things the Rails way

From what I can tell, you want to list all finished items providing a quantity field for the user to choose which items to add to the order. If the item is already in the order you want to display the correct quantity.

This is exactly what I am trying to do, thank you for taking the time to help me here.

I have moved my code to the model like you have described. My models look like:

class Order < ActiveRecord::Base
  has_many :finished_line_items, :class_name => "FinishedLineItem"

class FinishedLineItem < ActiveRecord::Base
  belongs_to :order, :class_name => "Order", :counter_cache => true
  belongs_to :finished_item, :class_name => "FinishedItem"

class FinishedItem < ActiveRecord::Base
  belongs_to :finished_type, :class_name => "FinishedType"
  has_many :finished_line_items, :class_name => "FinishedLineItem"


It works fine, except for an strange error I am getting if the quantity has not been updated to zero.


The line

else
        finished_line_items.destroy(['finished_item_id = ?', item_id])

is producing the following error.
Couldn't find FinishedLineItem with ID=finished_line_item = ?

It seems like that line should work, do you think it might be a bug?

I've replaced it with

line_item = self.finished_line_items.find_by_finished_item_id(item_id)
        if !line_item.nil?
          line_item.destroy
        end

and this works fine.

I don't think I will be adding an 'add item to order' button to the form right now because I think that would make it difficult for the users. If you take a look at what the order form looks like now you will see that it would be a little bit impractical for the users since they won't be able to tab through each field, especially since each order will most likely fill up most of the individual items.


http://optimal-vision.com/images/order.png

Thanks again for your help, I hope that as I learn more I will be able to help other people out as well!

Last edited by dahuk (2006-11-15 14:54:20)

Re: Doing things the Rails way

dahuk wrote:

The line

else
        finished_line_items.destroy(['finished_item_id = ?', item_id])

is producing the following error.
Couldn't find FinishedLineItem with ID=finished_line_item = ?

It seems like that line should work, do you think it might be a bug?

It's not a bug, I was just using the method incorrectly (that's what I get for not looking things up). wink

delete_all should do what you want.

line_items.delete_all(['finished_item_id=?', item_id])

Railscasts - Free Ruby on Rails Screencasts