drapergem / draper

Decorators/View-Models for Rails Applications
MIT License
5.23k stars 527 forks source link

Draper changes the behaviour of url_for when using STI models #886

Open daniel-rikowski opened 4 years ago

daniel-rikowski commented 4 years ago

I'm using Draper 4.0.1 and I have these models:

class Document < AR::Base; end;
class InputDocument < Document; end;
class OutputDocument < Document; end;

I also have a DocumentDecorator, but no dedicated decorators for the STI sub classes.

When I call url_for using a decorated InputDocument instance the wrong path helper is generated/called.

Example:

doc = InputDocument.find(1).decorate
doc.class # => DocumentDecorator
doc.object.class # => InputDocument
url_for(doc.object) # => /input_documents (EXPECTED)
url_for(doc) # => /documents (NOT EXPECTED)

I traced the url_for call through the various Rails methods. Eventually doc.model_name gets called, which seems to be the cause of this bug:

Draper doesn't delegate the model_name call to doc.object.class but to doc.class.object_class instead.

doc.object.model_name # => InputDocument
doc.object.class.model_name # => InputDocument
doc.class.object_class.model_name # => Document

Subsequentially Rails gets the wrong ActiveModel::Name and generates the wrong path helper name.

IMHO model_name should be delegated to the decorated object's actual class (or even to the instance itself?) and not the class inferred by the decorator, but I'm not sure this is the best course of action...

Alexander-Senko commented 2 months ago
url_for(doc) # => /documents (NOT EXPECTED)

I'd rather say that's what I expected in this case 🤔

Reason 1

Decorators encapsulate models isolating them from the outer code. Here, the outer code sees a document. If you'd like it to see an input one, just define InputDocumentDecorator.

Reason 2

A quite common use case for decorators is to follow controllers hierarchy. Thus, one model class can be decorated with different decorators depending on the controller scope. In this case it seems quite natural for URLs to match decorator names and not the model ones.