Using exhibit in path helpers or form builder creates wrong url with extra parameters #8

Closed
mattvanhorn opened this Issue Jun 14, 2012 · 7 comments

2 participants

@mattvanhorn

when both product and order are exhibits

simple_form_for [product, order] 

generates

<form accept-charset="UTF-8" action="/products/2/orders?product_id=2" class="simple_form new_order" id="new_order" method="post" novalidate="novalidate">

I don't know why it does that, but having the product_id twice means I can't find the product from the params (since params[:product_id] == ['2','2']

new_product_order_path(product)

behaves the same way.

new_product_order_path(product.to_model)

generates the right URL, but seems to go against the simple delegation pattern - why do I have to remember where I need a model and where I need an exhibit? It's like a duck with a speech impediment. ;-)

This feels like it is probably Rails doing some kind of type-checking, but so far I can't confirm.

@codeodor
Objects on Rails member

I noticed that in one of my projects, but I didn't realize it was due to DisplayCase. The project has recently undergone a lot of changes, and I hadn't had time to track down why that was happening.

Anyway, thanks. I'll figure it out now.

But the strange thing is, I thought Rails should still just have params[:product_id] == '2' ... unless it was "product_id[]" as the field name.

@mattvanhorn

Yeah - I think that was wrong. The URL is bad, but I discovered some other weirdness in my project that is responsible for the null project. database_cleaner is somehow is deciding to truncate all my tables before the scenario finishes. I would complain, but I love the fact that using open source means I can fix these things myself.

@codeodor
Objects on Rails member

This commit to Rails from this pull request fixes the issue.

The problem is that it's not calling #to_model on the args it uses to build the URL (but it should be).

Since the only options I see are to monkey patch Rails or ask people to update, I think we should close this.

Do you have any thoughts about that?

@mattvanhorn

Now, that I know the cause I'm going to just upgrade. I guess other people can pick their own option: use to_model in the helper, monkey patch rails, or upgrade.

@mattvanhorn mattvanhorn reopened this Jun 20, 2012
@mattvanhorn

This and other subtle bugs can happen because of the exhibit not implementing eql? properly

I've been implementing it like this:

  def eql?(other)
    (self.class == other.class) && (self.to_model == other.to_model)
  end
  alias :== eql?

to solve the problem.

But generally, when the helper code calls == to uniquify the params, the exhibit on the left delegates it to the model, which happily replies that it is NOT equal to the exhibit on the right.

I'm not sure that this should be rolled into the gem, but I think the README should have a note about making sure to implement eql?/==

I'm reopening to let you decide how to handle.

@codeodor
Objects on Rails member

My first thought is that the gem should handle this.

But we probably ought to think about what it means to be equal.

Is there any reason to say they must be the same class to be equal? Or could we just say that the object should respond to the to_model method and that the result of to_model on each of the comparable objects must be equal?

What are the potential drawbacks? Any? (I can't think of any, but I'd like to hear someone play devil's advocate)

@codeodor
Objects on Rails member

I added a note to the read me to check here if you're experiencing the issue.

@codeodor codeodor closed this Aug 10, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment