Node Controllers and 'out-of-the-box' index views


(Danny) #1

After using PT for the past week I really wish nodes would behave more like Rails framework scaffolds in that they have:

  • dedicated controllers (that inherit from PushType) allowing for a more conventional way to route, pass parameters and make variables available in specific views. I mean how do you even link to an index node view whilst passing it a search query that narrows down the items displayed in that view. Think Article-Node and a search parameter filtering out non-matches. This can be done easily in rails in the controller action. Lack of a controller makes this impossible.
  • as well as views (in addition to what is currently the show view equivalent). An index view at the very least would be extremely useful and would make more sense from a user point of view. It makes no sense to want multiple ArticleList nodes (which serves an an index view) but the “New ArticleList” button is always there ready to be (mistakenly) clicked.

Improvements Discussion
(Aaron Russell) #2

I’ve thought about this a lot. The current way of doing anything in the request cycle is to use before_node_action filters. These allow you to do anything you’d normally do in a Rails controller, so your example of searching and filtering is possible:

before_node_action only: :article_list do
  # Assuming you have a Article.by_filter_params scope
  @articles = @node.children.published.by_filter_params(params[:filter]).page(params[:page]).per(10)
end

So we can load data and prepare the action however we like. The questions I ask are:

  1. Is the ApplicationController the right place for a whole load of before_node_action filters?
  2. Do we need something more, like an explicit controller class for each node type where we can put this same kind of code?

On the first question I think definitely the ApplicationController is not the right place for this. It’s polluting the parent controller class with a bunch of methods that get inherited all the way down, and besides, it just looks a bit icky.

So quite soon I’d like to see a PushType base controller so we can generate our own controller, inherit from PushType::BaseController and put all our filters in one neat sensible place.

On the second question - do we need an actual controller for each node - I’m yet to be convinced. I’m not ruling it out, and I’m happy to listen to arguments for this, but here’s a few thoughts:

  • I’ve built some pretty big sites with PushType, and I’m yet to come across a scenario where before_node_action haven’t been up to the task. I’ve also yet to come across a situation where there are just too many filters and having them all in one place becomes a mess.
  • In my experience, most filters end up being very small - like one or two lines of code just loading some data in to an instance variable. Many node types don’t even need any filtering. So we could end up with a load of controller classes doing very little - or even nothing.
  • PushType isn’t trying to recreate any part of Rails - in fact it’s trying to get out of Rails’ way as much as possible. For complex user scenarios where you genuinely do need a controller, just use Rails. Generate a controller and create a route for it.

So this is where I am with controllers. I’ve mulled it over quite a bit and just yet to be convinced. Like I say though - happy to hear other views.

Index views

I don’t actually understand what you’re suggesting here. Can you clarify?


(Danny) #3

Absolutely not in my opinion. I think we should stick to the way Rails does it as to not confuse new PT users (by users I mean developers). More on this below.

I am for controllers for each node, or at least the option to create controllers for specific nodes because:

  • following Rails convention is a good idea. Scaffolded Rails controllers are stock standard controllers as they are, the only difference being the nouns used to describe the controller class and variables inside, which is why I think leaving dedicated controllers out because “they tend to be small” is not a strong argument, possibly. Rails generates mostly duplicated controllers for every scaffold to stick to the pattern. Which brings me to the next point,
  • Ease of use: Why go against the grain with PT? As a first time user this confused me a lot and it felt very unnatural to put everything in the application_controller. Sticking to convention thereby reduces confusion for first time users. Rails developers are accustomed to seeking the whatever_controller.rb for any controlling logic related to the whatever model. It would decrease the size of the learning curve to have PushType follow the same idea.
  • separate controllers make routing more transparent as controller actions are clearly laid out. My search problem would not have come up in the first place if PT had dedicated controllers for each node. The following quote and my response are related to this point: [quote=“aaron, post:2, topic:207”]
    In my experience, most filters end up being very small - like one or two lines of code just loading some data in to an instance variable. Many node types don’t even need any filtering. So we could end up with a load of controller classes doing very little - or even nothing.
    [/quote]
    What if users wanted to add additional related routes? Using the Article example, a user might want to implement a liking/upvoting mechanism. Having a dedicated controller would allow them to create additional Article routes the same way they would do it with any other Rails scaffold. These routes would be stored in the articles_controller.rb which neatly contains all actions to do with articles.

I agree 100%, there is no point recreating Rails. What I would like to see though is PushType integrating well with Rails, seamlessly if possible as to not confuse users*.

*I might be overhyping the “ease of use” argument, possibly because the lack of documentation made it harder to use.


(Danny) #4

Let me clarify: Currently, PT generates one view for us which is basically the equivalent of a Rails show.html.erb view, displaying a single record. What I am suggesting are automatic index views associated with each node that list all records of that particular node. Though I think the part that needed clarification was this bit:

The current method to achieve an index view is to create a NodeList node. e.g. to create an index view for Article node we would create an ArticleList node and exactly one ArticleList record. Then, in order to list Articles on this node, we have to make all Articles children of the ArticleList record (which in itself may not be possible because our content structure might require Articles to be children of another node type). The index view thereby becomes a database record. It makes no sense to create additional ArticleList records as the sole purpose of that node and record was to recreate the index view. I hope my explanation makes sense.

Now that I think of it… we could work around the “wasting the parent_id relationship on ArticleList nodes”-problem by creating a custom variable to be displayed in that ArticleList view, like @articles = Article.all. This means we don’t have to make all Articles children of ArticleList. The question then becomes whether we should have to do that in order to achieve something as simple as an index view. I’m not sure if that undermines my previous argument, I would much prefer to have this built in.


(Aaron Russell) #5

I need to flip this around because I think the way PushType currently does things is conventional Rails, and the alternative would be far less conventional. It might be worth trying to explain how this currently works and I think you’ll see where I’m coming from.

For rendering nodes, PushType inserts one single route into your application, which looks like this:

#routes.rb
get '*permalink' => 'front_end#show', as: 'node'

This rule should be at the end of your routes file and acts as a catchall. Throw any url at the route and it will pass the permalink to PushType::FrontEndController#show, which will attempt to find a node with that permalink and render that node.

At this point I would insist that this is not going against Rails convention in any way. You have a resource (a node) which you are identifying with a permalink and you are calling an action to show/render that resource. Maybe it’s confusing because you can’t see the front end controller in your app - it’s hidden in the PushType engine and maybe it feels like there’s some “magic” going on. But there’s not. It’s actually a pretty simple and standard pattern.

Alternatives?

Going back to our routes file above, we need to understand that at the routing level Rails knows nothing about the resource or the node you want to access - it just has a permalink. So it’s impossible to conditionally call a ArticlesController or a EventsController using standard Rails routing.

To do this, we’d need to build some kind of middleware that loads the node, works out what type of node it is and then calls the relevant controller. At this point we’re already way off the beaten path when it comes to Rails convention. Our controllers wouldn’t be ActionControllers, they would be some kind of PushTypeController that is made to feel and behave a bit like ActionControllers. We’re basically recreating Rails functionality… which goes back to the point in my first reply.

We could do all this… I haven’t ruled this out. Just haven’t been convinced that it’s worth it.


A couple of other points to reply to:

Use a Rails controller. PushType comes with the worlds greatest web application framework for free :smiley:. Seriously PushType, will never be better than Rails for developing all the bespoke and unique functionality that every website needs. PushType’s responsiblity is purely for managing content and rendering content. Use Rails for everything else.

I think this is key. I have neglected the docs a bit, and there is nothing in the docs on what we’ve talked about in this topic. I’ll try and prioritise this.


(Danny) #6

Thanks for the detailed response, @aaron. Once again a better behind-the-scenes understanding of PushType made me agree with your points! :slight_smile:


(Aaron Russell) #7

:smile:

You won’t be the last person to suggest controllers. This’ll come up again. I actually think at some point PushType probably will have some kind of node specific controllers, but sometimes it’s worth waiting for the obviously right solution to stare you in the face. Until then I don’t think the current approach is the wrong approach.