destroy method does not appear to be working

I'm using ruby 1.8.7 and rails 3.0.3 going through the Agile Rails dev book from PragProg

This destroy method on my ProductsController does not delete my product and I don't understand why.

Here is my first cut that I expected to 'just work'

  def destroy
    @product = Product.find(params[:id])
    @product.destroy
    respond_to do |format|
      format.html { redirect_to(products_url) }
      format.xml  { head :ok }
    end
  end

But my test to assert the Product.count went down failed.

If I change to use the delete class method like this:

  def destroy
    Product.delete(params[:id])
    respond_to do |format|
      format.html { redirect_to(products_url) }
      format.xml  { head :ok }
    end
  end

my test passes.

Here is the test

  test "should destroy product" do
    assert_difference('Product.count', -1) do
      if @product.referenced_by_line_item
        @product.line_items.remove_all
      end
      delete :destroy, :id => @product.to_param
    end

    assert_redirected_to products_path
  end

I have a before_destroy method on my Product model class

  def referenced_by_line_item
    if line_items.count.zero?
      return true
    else
      errors.add(:base, 'Line Items present')
      return false
    end
  end

Any ideas what I'm doing wrong here? From reading the docs (and searching other questions her开发者_开发问答e) I expect @product.destroy and Product.delete(id) to do the same thing.

Thanks!

You're calling referenced_by_line_item before you do anything. If there are line items, it's returning false, so then you don't delete the line items. I don't think that's the behavior you want. If you changed it to:

  if !@product.referenced_by_line_item
    @product.line_items.remove_all
  end
  delete :destroy, :id => @product.to_param

then @product.destroy would probably work.

Although it would probably make more sense to reverse the logic in referenced_by_line_item. It would make more sense to say, if there are line items associated with this product, then this product is referenced by a line item. I think your logic is backwards.

Also, boolean methods should end with a '?'

I see the complication here though. You want it to return false when there are line items so that it doesn't save, but the return value is inconsistent with the method name. Maybe something like this would be better:

def no_line_items_present?
  if line_items.count > 0
    # add errors
    return false
  else
    return true
  end
end

Then you can say:

  @product.line_items.remove_all unless @product.no_line_items_present?
  delete :destroy, :id => @product.to_param

Although I don't see the point in checking for line items before deciding to remove them except for a small performance increase.

delete and destroy have a major difference - delete just deletes the row, without calling callbacks, as described in http://www.nickpeters.net/2007/12/21/delete-vs-destroy/

This being the case, using the delete bypasses your before_destroy method, which when run is adding errors to the object. When you use delete, it ignores the errors, and thus changes the count.