Topic: create new and update in same function as new?

the update_attributes and save would be called twice for a record that already exist. what is the better way?  thanks?!



  def blog_post_new
         @blogpost = BlogPost.new
     
         if request.post? 

           @blogpost = BlogPost.find(:first, :conditions => ['id = ? and user_id = ?',params[:id], current_user.id]) || BlogPost.new(params[:blogpost])

           @blogpost.update_attributes(params[:blogpost])
         
           if @blogpost.save

             redirect_to :action => 'index'

           else

             render :url => "#{blog_new_post_url()}"

           end
        end
  end

Last edited by bbqplate (2008-02-02 13:50:33)

Re: create new and update in same function as new?

def blog_post_new
  if @blogpost = current_user.blogposts.find(params[:id])
    @blogpost.attributes = params[:blogpost]
  else
    @blogpost = Blogpost.new(params[:blogpost])
  end

  if @blogpost.save
    redirect_to :action => "index"
  else
    render :action => "blog_post_new"
  end
end

Last edited by Duplex (2008-02-02 14:08:01)

Re: create new and update in same function as new?

hi, thanks for the quick reply!

some questions if i may ask....

the "current_user.blogposts.find(params[:id])" statement....does that ensure that the find for params[:id] will only be valid if the user_id for the blogposts belong to the current_user?

Re: create new and update in same function as new?

exactly. it adds a condition to the find behind the scenes to do that. doing finds on associated objects like this is considered best practice therefore wink

Last edited by Duplex (2008-02-02 14:49:42)

Re: create new and update in same function as new?

Won't this do the same thing?

def blog_post_new
  @blogpost = params[:id] ? current_user.blog_posts.find(params[:id]) :
    current_user.blog_posts.build(params[:blogpost])
  if request.post?
    @blogpost.attributes = params[:blogpost]
    if @blogpost.save
      redirect_to :action => 'index'
    end
  end
end

Note that we are always using an association to narrow the scope; current_user.blog_posts.find(...) instead of BlogPost.find(...); and current_user.blog_posts.build(...) instead of BlogPost.new(...). Not only is it much shorter and easier to read, in this case (because you are dealing with users) it is also more secure.

The decision on whether to create a new blogpost or update an existing one is made by checking for the existence of params[:id].

Finally, there's no need to render the blog_post_new action (you had render :url => "#{blog_new_post_url()}") in case of failing validation; rendering that is the default action, so just don't do anything.

NB: This should also work as your edit action.

Last edited by fabio (2008-02-02 15:03:27)

Re: create new and update in same function as new?

great code and question!


@blogpost = params[:id] ? current_user.blog_posts.find(params[:id]) : current_user.blog_posts.build(params[:blogpost])



is this saying if params[:id] is valid  (hence a record that already exist) assign @blogpost to current_user.blog_post.find....else create a new blogpost if params[:id] is not a valid number?




fabio wrote:

Won't this do the same thing?

def blog_post_new
  @blogpost = params[:id] ? current_user.blog_posts.find(params[:id]) :
    current_user.blog_posts.build(params[:blogpost])
  if request.post?
    @blogpost.attributes = params[:blogpost]
    if @blogpost.save
      redirect_to :action => 'index'
    end
  end
end

Note that we are always using an association to narrow the scope; current_user.blog_posts.find(...) instead of BlogPost.find(...); and current_user.blog_posts.build(...) instead of BlogPost.new(...). Not only is it much shorter and easier to read, in this case (because you are dealing with users) it is also more secure.

The decision on whether to create a new blogpost or update an existing one is made by checking for the existence of params[:id].

Finally, there's no need to render the blog_post_new action (you had render :url => "#{blog_new_post_url()}") in case of failing validation; rendering that is the default action, so just don't do anything.

NB: This should also work as your edit action.

Re: create new and update in same function as new?

That's not really correct. it will only check weither an id value was passed, and then either look for the corresponding record, or create a fresh one. That doesn't take into account the possibility that some user plays with the id value, and uses an invalid id, which would lead to your app throwing a nil error. I would rather do something like this:

def blog_post_new
  if params[:id] && @blogpost = current_user.blogposts.find(params[:id])
    # there's an id and it is valid -> update attributes
    @blogpost.attributes = params[:blogpost]
  elsif !params[:id]
    #there is no id -> build a new record.
    @blogpost = current_user.blogposts.build(params[:blogpost])
  else
   # there's an id, but its not valid -> set error message and show the form again
   flash[:error] = "No Record found for this ID"
   render :action => "new" #or what the action with the form is called.
  end

  if @blogpost.save
    redirect_to :action => "index"
  else
    flash[:error] = "Save failed."
  end
end


Depends how solid your code should be. It's no security threat, but rather smoothing your apps response to errors.

Last edited by Duplex (2008-02-02 16:10:20)

Re: create new and update in same function as new?

bbqplate wrote:

is this saying if params[:id] is valid  (hence a record that already exist) assign @blogpost to current_user.blog_post.find....else create a new blogpost if params[:id] is not a valid number?

Not exactly. It checks to see that a parameter with the name of "id" was passed and pulls up a blogpost with that ID if it was, or builds a new blogpost if it wasn't. It then assigns the result of either to @blogpost. The idea being that the edit and update actions will pass in an ID, but new and create wont.

Duplex wrote:

That's not really correct . . .

You are right, I wasn't doing exception handling for simplicity's sake. You could just do this, however:

def blog_post_new
  @blogpost = params[:id] ? current_user.blog_posts.find(params[:id]) :
    current_user.blog_posts.build(params[:blogpost])
  if request.post?
    @blogpost.attributes = params[:blogpost]
    if @blogpost.save
      redirect_to :action => 'index'
    end
  end
# AR::Base#find raises RecordNotFound when an invalid ID is passed
rescue ActiveRecord::RecordNotFound
  # If somebody is passing in invalid IDs, give them as little info as possible to 'hack' you:
  flash[:notice] = "Something went wrong with your request. Your request has been logged and an administrator has been advised."
end