Rails Ruby on Rails → 3.0.2

Comments and changes to this ticket

• You flagged this item as spam.
  

  Ellis Berner June 14th, 2010 @ 09:34 PM

  • no changes were found...

• You flagged this item as spam.
  

  Ellis Berner June 14th, 2010 @ 09:35 PM

  err typo. Should be 'loading up /post' (not posts)

• 

  Jeff Kreeftmeijer June 15th, 2010 @ 06:07 AM

  I've attached a patch that removes <%= link_to 'Back', posts_path %> from the edit and new actions' views when using the generator with the --singleton option for master.

  It includes tests for this specific change and some tests for running the generator with the --singleton option, since they didn't exist yet.

  • singleton_generator.diff 8 KB
• 

  Jeff Kreeftmeijer June 16th, 2010 @ 02:18 PM

  There were some unnecessary tests in test_singleton_scaffold_on_invoke that I directly copied from test_scaffold_on_invoke and some trailing whitespaces in my previous patch. Here's a new one. :)

  • singleton_generator2.diff 7.3 KB
• 

  Jeff Kreeftmeijer June 16th, 2010 @ 02:44 PM

  Aaand another one. This is the same as the previous patch, but adds parentheses around the arguments of the assert_match calls to avoid warnings when running the tests (Like I did for the rest of the scaffold_generator tests in #4872). :)

  • singleton_generator3.diff 7.3 KB
• 

  José Valim June 19th, 2010 @ 11:46 PM

  • Milestone cleared.

  Awesome mate! Just two quick notes:

  1)

  <%%= link_to 'Show', @<%= singular_name %> %> <%= '|' unless options[:singleton] %>
  <% unless options[:singleton] -%>
  <%%= link_to 'Back', <%= plural_name %>_path %>
  <% end -%>
  

  Could be better written as:

  <%%= link_to 'Show', @<%= singular_name %> %>
  <% unless options[:singleton] -%>
    | <%%= link_to 'Back', <%= plural_name %>_path %>
  <% end -%>
  

  2) And could you please check that:

  <%%= link_to 'Show', @<%= singular_name %> %>
  <%%= link_to 'Edit', edit_<%= singular_name %>_path(@<%= singular_name %>) %>
  

  In fact generates the proper route in singleton cases? I'm sure the second case does not, but we need to check the first.

  And nice additions (assert_no_instance_method and assert_not_in_file)!

• 

  Jeff Kreeftmeijer June 20th, 2010 @ 11:34 PM

  Thanks José, I've updated my patch and changed the views like you explained in your first note. The previous way of doing it was to keep the generated view exactly the same as before, but I think I like your approach more after all.

  About your second note: I'll check that out and will create a new ticket if find something, but that doesn't really have anything to do with this patch, right?

  • singleton_generator4.diff 7.3 KB
• 

  José Valim June 20th, 2010 @ 11:44 PM

  Oh, now I see why you used the previous syntax, to keep the generated view pretty. Maybe we could do it inline? Something like this:

  <%%= link_to 'Show', @<%= singular_name %> %><% unless options[:singleton] -%>  | <%%= link_to 'Back', <%= plural_name %>_path %><% end -%>
  

  Sorry that I missed it earlier. :(

  And about the paths, it's related to this patch, because if the following is not valid for a singleton resource:

  <%%= link_to 'Edit', edit_<%= singular_name %>_path(@<%= singular_name %>) %>
  

  It means it should output this instead:

  <%%= link_to 'Edit', edit_<%= singular_name %>_path %>
  

• 

  Prem Sichanugrist (sikachu) June 23rd, 2010 @ 05:30 PM

  • State changed from “new” to “open”

  So, since Jeff's ticket didn't apply on master anymore, I've update his ticket to reflect the changes on the generator on master, while retaining his ownership.

  I've also took advice from José on the edit_<%= singular_name %>_path %> without argument thing, and write my another patch to fix it. (Also, doing some cleaning to make the output look nicer.)

  I've attach both patches below:

  • 0001-removed-the-index-links-from-the-generator-s-view-te.patch 2.3 KB
• 

  Prem Sichanugrist (sikachu) June 23rd, 2010 @ 05:41 PM

  I actually found out some more bug with it, will provide another patch soon ..

• 

  Prem Sichanugrist (sikachu) June 23rd, 2010 @ 06:33 PM

  Urgh.

  I actually stumbled across the usability bug of this patch on the controller side. Consider this snippets generated:

  class TopicsController < ApplicationController
  
    # GET /topics/1
    # GET /topics/1.xml
    def show
      @topic = Topic.find(params[:id])
  
      respond_to do |format|
        format.html # show.html.erb
        format.xml  { render :xml => @topic }
      end
    end
  
    # GET /topics/new
    # GET /topics/new.xml
    def new
      @topic = Topic.new
  
      respond_to do |format|
        format.html # new.html.erb
        format.xml  { render :xml => @topic }
      end
    end
  
    # GET /topics/1/edit
    def edit
      @topic = Topic.find(params[:id])
    end
  
    # POST /topics
    # POST /topics.xml
    def create
      @topic = Topic.new(params[:topic])
  
      respond_to do |format|
        if @topic.save
          format.html { redirect_to(@topic, :notice => 'Topic was successfully created.') }
          format.xml  { render :xml => @topic, :status => :created, :location => @topic }
        else
          format.html { render :action => "new" }
          format.xml  { render :xml => @topic.errors, :status => :unprocessable_entity }
        end
      end
    end
  
    # PUT /topics/1
    # PUT /topics/1.xml
    def update
      @topic = Topic.find(params[:id])
  
      respond_to do |format|
        if @topic.update_attributes(params[:topic])
          format.html { redirect_to(@topic, :notice => 'Topic was successfully updated.') }
          format.xml  { head :ok }
        else
          format.html { render :action => "edit" }
          format.xml  { render :xml => @topic.errors, :status => :unprocessable_entity }
        end
      end
    end
  
    # DELETE /topics/1
    # DELETE /topics/1.xml
    def destroy
      @topic = Topic.find(params[:id])
      @topic.destroy
  
      respond_to do |format|
        format.html { redirect_to(topics_url) }
        format.xml  { head :ok }
      end
    end
  end
  

  There're some issues from this generated controller:

  1. On the show and edit option, the params[:id] would not be assigned, as the URL for it would be /topic. So, what would be the best record to shown here? Topic.last?
     
  2. On the destroy action, what should we redirect the user to after destroyed? I mean, I know everybody will change it, but what would be the best? redirect_to '/'?

  I'd love to discuss more about this. ;)

  BTW, I've found out there's a bug in the _form.html.erb generated by the generator. I've attached the replacement patch to fix it.

  • 0002-silence-some-of-the-erb-tag-whitespace-and-calling-e.patch 3.4 KB
• 

  José Valim June 23rd, 2010 @ 07:14 PM

  Hey Prem! Nice work!

  About the controller, I don't think there is anything we can do to make it work by default. Usually, singleton resources are nested or depend on something in the session. That said, maybe we could show a message to the user saying that singleton only provides a stub, he still needs to work on controller + tests in order to have it working.

  What do you think?

• 

  Prem Sichanugrist (sikachu) June 24th, 2010 @ 05:37 PM

  Urgh ..

  So, after a long discussion with José, it turned out that we should remove the --singleton option for the time being. Because of:

  1. Singleton resource usually tied to another resource. For example, User has_one Profile. The controller generator doesn't actually know how should it find profile object. So, the test will be failed.
     
  2. I've tried to add argument to the --singleton option, to be --singleton=current_user. It turned out to be more ugly, and there're too many conditions going on in the controller file itself, make it hard to maintain.
     
  3. I've also tried stub out the find method as José suggest, but then the test case provided with the scaffold will be fail. This is not good, as some user expects the test cases to be pass after they generate the scaffold.

  I've proposed to make another generator as well, such as rails g singleton_generator current_user profile fullname:string, but then I don't think it would be welcomed by the core team. So maybe it could come as the plugin.

  So, thanks to everybody who's involved in this ticket, especially Jeff. I'm sure we'll find out the best solution soon.

  • 0001-remove-the-singeleton-option-from-scaffold-generator.patch 8.7 KB
• 

  Repository June 24th, 2010 @ 07:21 PM

  • State changed from “open” to “resolved”

  (from [67ee6c38b9b112eabe37d5869c23210b9ebf453c]) Remove the --singeleton option from scaffold generator.

  It turned out to be that scaffold for singeleton resource will always depend on another model, and it's not possible at the moment to make the application tests pass after generate the singeleton scafold. So, it would be better to remove it for now and probably provide another generator, such as singeleton_scaffold, in which also require the depended model name.

  [#4863 state:resolved]

  Signed-off-by: José Valim jose.valim@gmail.com
  http://github.com/rails/rails/commit/67ee6c38b9b112eabe37d5869c2321...

• 

  Jeremy Kemper October 15th, 2010 @ 11:01 PM

  • Milestone set to “3.0.2”
  • Importance changed from “” to “Low”

  [bulk edit]