CODEWITHSUNDEEP
ruby-on-railsrestruby-on-rails-4
Yuval Karmi
Yuval Karmi

Reputation: 26713

Rails has_one build_association deletes record before save

So this has been asked previously, but with no satisfying answers.

Consider two models, User, and Subscription associated as such:

class User < ActiveRecord::Base
      has_one :subscription, dependent: :destroy
end

class Subscription < ActiveRecord::Base
      belongs_to :user
end

Inside of SubscriptionsController, I have a new action that looks like this

def new
    user = User.find(params[:user_id])
    @subscription = user.build_subscription
end

Given that a subscription already exists for a user record, I'm faced with the following problem:

user.build_subscription is destructive, meaning that simply visiting the new action actually destroys the association, thereby losing the current subscription record.

Now, I could simply check for the subscription's existence and redirect like this:

def new
    user = User.find(params[:user_id])
    if user.subscription.present?
        redirect_to root_path
    else
        @subscription = user.build_subscription
    end
end

But that doesn't seem all that elegant.

Here's my question

Shouldn't just building a tentative record for an association not be destructive? Doesn't that violate RESTful routing, since new is accessed with a GET request, which should not modify the record?

Or perhaps I'm doing something wrong. Should I be building the record differently? Maybe via Subscription.new(user_id: user.id)? Doesn't seem to make much sense.

Would much appreciate an explanation as to why this is implemented this way and how you'd go about dealing with this.

Thanks!

Upvotes: 12

Views: 2030

Answers (4)

Anissa
Anissa

Reputation: 21

As of 2018 Richard Peck's solution worked for me:

 #app/models/user.rb
 Class User < ActiveRecord::Base

    def build_a_subscription
       if subscription
           subscription
       else
           build_subscription
       end
    end
  end

My issue was that a user controller didn't have a new method, because users came from an api or from a seed file. So mine looked like:

#app/controllers/subscriptions_controller.rb
def update
  @user = User.find(params[:id])
  @user.build_a_subscription
  if @user.update_attributes(user_params)
    redirect_to edit_user_path(@user), notice: 'User was successfully updated.'
  else
    render :edit
  end
end

And I was finally able to have the correct singular version of subscriptions in my fields_for, so :subscription verses :subscriptions

#app/views
<%= f.fields_for :subscription do |sub| %>
  <%= render 'subscription', f: sub %>
<% end %>

Before I could only get the fields_for to show in the view if I made subscriptions plural. And then it wouldn't save. But now, everything works.

Upvotes: 0

DrewB
DrewB

Reputation: 3341

I came across this today and agree that deleting something from the db when you call build is a very unexpected outcome (caused us to have bad data). As you suggested, you can work around if very easily by simply doing Subscription.new(user: user). I personally don't think that is much less readable then user.build_subscription.

Upvotes: 1

Richard Peck
Richard Peck

Reputation: 76774

It depends on what you want to do

Thoughts

From what you've posted, it seems the RESTful structure is still valid for you. You're calling the new action on the subscriptions controller, which, by definition, means you're making a new subscription (not loading a current subscription)?

You have to remember that Rails is basically just a group of Ruby classes, with instance methods. This means that you don't need to keep entirely to the RESTful structure if it doesn't suit

I think your issue is how you're handling the request / action:

def new
    user = User.find(params[:user_id])
    @subscription = user.build_subscription
end

@subscription is building a new ActiveRecord object, but doesn't need to be that way. You presumably want to change the subscription (if they have one), or create an association if they don't

Logic

Perhaps you could include some logic in an instance method:

#app/models/user.rb
Class User < ActiveRecord::Base

    def build
       if subscription
           subscription
       else
           build_subscription
       end
    end

end

#app/controllers/subscriptions_controller.rb
def new
    user = User.find(params[:user_id])
    @subscription = user.build
end

This will give you a populated ActiveRecord, either with data from the subscription, or the new ActiveRecord object.

View

In the view, you can then use a select box like this:

#app/views/subscriptions/new.html.erb
<%= form_for @subscription do |f| %>
    <%= "User #{params[:user_id]}'s subscription: %>
    <%= f.collection_select :subscription_id, Subscription.all,:id , :name %>
<% end %>

They are my thoughts, but I think you want to do something else with your code. If you give me some comments on this answer, we can fix it accordingly!

Upvotes: 3

23tux
23tux

Reputation: 14736

I also always thought, that a user.build_foobar would only be written to the db, if afterwards a user.save is called. One question: After calling user.build_subscription, is the old subscription still in the database?

What is the output user.persisted? and user.subscription.persisted?, after calling user.build_subscription?

Your method to check if a subscription is present, is IMHO absolutely ok and valid.

Upvotes: 1

Related Questions