← Back to Upcase

Intermediate workshop 3rd video assignment


(Charlieanna) #1

@halogenandtoast
Just like you created a concern for the model I created a concern for the controllers called Shoutable.

But all I could do is

module Shoutable
  def build_shout(content)
    current_user.shouts.build(content: content)
  end
end 

class TextShoutsController < ApplicationController
  include Shoutable
  def create
	  content = build_content
		shout = build_shout(content)
		if shout.save
	  else
	  	flash.alert = "Could not shout"
	  end
	  redirect_to dashboard_path
	end

The create method is also duplicated but should I move that too because redirect_to and the create method I feel should be in the controller


(Matthew Mongeau) #2

I tend to agree that the redirect_to and the create action should be in the controller. One thing you might consider is trying to merge the TextShoutsController and PhotoShoutsController into 1 controller. You can do this if you update your form to pass along the name of the content class and use a sprinkling of metaprogramming to get the class (i.e. TextShout or PhotoShout). In addition to that, strong_parameters has a method #permit! which allows you to permit any attributes. Since we’d be okay permitting any of the attributes on the content class this makes it so we don’t have to keep track of which content classes contain what fields. I’d see how far you can get with trying to merge TextShoutsController and PhotoShoutsController into a single ShoutsController.


(Charlieanna) #3

Ya I could try that. Will let you know once I try that.

Thanks.


(Matteo Giaccone) #4

Hi @halogenandtoast, what do you think about this refactoring?

The forms here app/views/dashboards/show.html.erb looks like:

<%= form_for @dashboard.new_text_shout, url: shouts_path do |form| %>
  <%= form.text_field :body, placeholder: 'Shout content here' %>
  <%= hidden_field_tag :shout_type, 'text' %>
  <%= form.submit 'Shout!' %>
<% end %>

<%= form_for @dashboard.new_photo_shout, url: shouts_path do |form| %>
  <%= form.file_field :image %>
  <%= hidden_field_tag :shout_type, 'photo' %>
  <%= form.submit 'Shout!' %>
<% end %>

<%= render @dashboard.timeline %>

And the controller app/controllers/shouts_controller.rb is the only controllers for shouts refactored like this:

class ShoutsController < ApplicationController
  def show
    @shout = Shout.find(params[:id])
  end

  def create
    content = build_content
    shout = current_user.shouts.build(content: content)
    unless shout.save
      flash.alert = "Could not shout."
    end
    redirect_to dashboard_path
  end

  private

  def shout_type
    params[:shout_type]
  end

  def build_content
    eval("#{shout_type}Shout".classify).new(eval("#{shout_type}_shout_parameters"))
  end

  def text_shout_parameters
    params.require(:text_shout).permit(:body)
  end

  def photo_shout_parameters
    params.require(:photo_shout).permit(:image)
  end
end

My repo is here: https://github.com/matjack1/intermediate-rails

Thank you for the feedback!


(Charlieanna) #5

@matjack1 you nailed it.

Even I tried a solution but with a similar project.

http://forum.thoughtbot.com/t/combining-two-controllers-into-one/1603

You can use permit! to do away with the parameters method. And use params[:content_type]. This was suggested by @halogenandtoast. Even I am waiting for his reply.


(Matteo Giaccone) #6

Thank you @charlieanna!

Yes, I used shout_type, instead of content_type, but the principle is the same as yours. I think your solution is better than mine because it works also when editing the form (you take the class name, while I have hardcoded the type).

If it’s the good approach I’ll refactor following your approach. Do you have a public repo for your project? It would be interesting for me to take a look. Thank you!


(Charlieanna) #7

Also notice the permit! I used as suggested by Goose.


(Matteo Giaccone) #8

Yes, I noticed it, but I prefer to permit the fields per single type, so I can better check the parameters and not allow everything together.


(Charlieanna) #9

Yes even I was confused with the idea of using permit! so let us wait for Matt’s reply. @halogenandtoast you listening?


(Matthew Mongeau) #10

The usage of permit! should be sparingly but I think in the case of the TextShout and PhotoShout they represent the part of the shout the user has control over and therefor I am okay with using permit!. The other thing I like doing here is changing the form_for a bit to something like:

<%= shout_form_for(TextContent.new) do |form| %>
  <%= form.text_field :body, placeholder: "Shout here" %>
  <%= form.submit "Shout!" %>
<% end %>

And then I use the following helper:

module ShoutHelper
  def shout_form_for(content, &block)
    form_for(Shout.new(content: content)) do |form|
      form.hidden_field(:content_type) +
        form.fields_for(:content, &block)
    end
  end
end

And then in my controller:

  def create
    shout = current_user.shouts.create(shout_params)
    redirect_to dashboard_path
  end

  private

  def shout_params
    { content: shout_content }
  end

  def shout_content
    content_class.new(content_params)
  end

  def content_class
    params[:shout][:content_type].constantize
  end

  def content_params
    params[:shout][:content].permit!
  end

I use this format because it’s pretty versatile and will grow well with the application if the existing content types change.


(Raul Murciano) #11

Ah, this is a very nice refactoring. The extraction to the helper wasn’t obvious to me, thank you for posting it Matthew!


(Matteo Giaccone) #12

Yes, thank you very much for sharing the refactor.

I don’t get how you can pass the TextContent object to the content of the Shout and everything works magically :smile:

I had the same doubt in the ShoutsController#create, I don’t understand where you see the match between content and content_type. I don’t know if it’s clear my doubt. Thanks!


(Raul Murciano) #13

This line:

form_for(Shout.new(content: content)) do |form|

assigns the content to the new Shout, so this Shout’s content_type field will take the TextShout or PhotoShout value, according to the argument passed to shout_form_for.

In the controller, the content_class method takes the content_type param from the hidden field and uses constantize to generate the corresponding class (i.e: converts a "TextShout" string to a TextShout class). That class is used in shout_content to generate the object that will be assigned as content to the new Shout.


(Matteo Giaccone) #14

Ok @raul, thank you very much!

It was also unclear the polymorphic association. Rails works out automatically the content_type and content_id just pasing the TextContent.new object. I was also missing this last bit of magic!

This screencast made it clear to me: http://railscasts.com/episodes/154-polymorphic-association-revised (sorry for the paywall :frowning:), much more than the Rails documentation.