where should ActiveRecord::Base.transaction be?

Question

I have three models: List, Food, and Quantity. List and Food are associated through Quantity via has_many :through. So each Quantity has three params: food_id, list_id, and amount (an integer).

My aim is to create a new Quantity (associated with that list) each time a list is created. I wish to do this using a transaction so that all objects must be successfully created or else none will be.

My main question is: where in my code should I write this transaction? I think it should be in the List model, but I'm not sure; and if it should be in the List model, I don't know where it would be within the List model. I think it should not be in the List controller, and I found advice on a comment on Mark Daggett's blog that it could be in an independent data access object, but I'm not sure how to do that.

Secondarily: the transaction itself. It is hard to tell if my mistake is in the transaction or just its location.

In case it is relevant, I came to have this question following answers to another question, but I thought this should be a new question since I didn't find a similar one specifically about transactions.

My List model, where the transaction currently lives:

class List < ActiveRecord::Base
  has_many :quantities
  has_many :foods, :through => :quantities

  before_save { self.name = name.downcase }

  validates :days, presence: true, :numericality => { :greater_than => 0 }
  validates :name, length: { maximum: 140 }, uniqueness: { case_sensitive: false }
end

ActiveRecord::Base.transaction do
      @list = List.create
      @a = Food.all.sample(1) 
      Quantity.create(food_id: @a, list_id: @list.id, amount: rand(6))
end

There is no error, but the new Quantity doesn't get created, which makes me think that I'm doing something right in addition to at least one thing wrong.

I also tried

List.transaction do

instead of

ActiveRecord::Base.transaction do

and got the same result.

I would appreciate any direction or hints on this problem that I assume stems from a misunderstanding of a very basic point (so basic that I couldn't find anything about it in the docs). Thank you.

Rails 4.2.3, Cloud9. Development database = SQLite3, production database = postgres heroku.

Answer 1

I've updated my code according to feedback from mrodrigues:

• Edit the names of variables and parameters for clarity.
• Add rescue so that the write method returns false upon failure.
• Reorganize code so that the write method (in the service) can instantiate an object within the create method (in the controller).
• Relocate the code that actually saves a new list to the write method.

In order for this all to work, I added a def initialize method in the WriteList class, which you all may have assumed was there, but was not. I hope this was a sensible (and not wacko) thing to do.

Edit

I made these changes after I got on the right track:

• Add return list to the end of the write method.
• Instead of rescuing to false, created a new list using the params that does not save; this results in the error messages regarding the problem with the validation to read correctly.

End edit

It now is working properly for my purpose; I appreciate any further feedback on organization/structure/building for the long-term.

app/services/write_list.rb:

class WriteList

  def initialize(params)
      @params=params
  end

  def write
      ActiveRecord::Base.transaction do
          food = Food.all.sample.id
          list = List.new(days: @params[:days], name: @params[:name])
          list.save!
          Quantity.create!(food_id: food, list_id: list[:id], amount: 1+rand(6))
          return list
      end
    rescue
      return List.new(days: @params[:days], name: @params[:name])
  end
end

relevant section of app\controllers\lists_controller:

def create
  @list = WriteList.new(list_params).write
  if @list.save
    flash[:success] = "A list has been created!"
    redirect_to @list
  else
    render 'new'
  end
end

By the way: I hope my practice of posting answers with improvements over my previous answers/question is appropriate; someone please tell me if it is not. I find it very helpful for tracking my progress and gathering more feedback, and I hope others (especially beginners such as myself) may gain something from the updates.

Answer 2

I finally got my transaction working and into a service! Thank you both ilan and mrodrigues for the advice. My create new Quantity logic is out of the controller and model, I am approaching "skinny everything" code, and I have bangs to require save.

For reference, this post on John Nunemaker's Railstips helped me define the WriteList method in a way that would be recognized by the Lists controller (by using self.).

Here is the service I wrote to handle creation of Quantity upon creation of List:

class WriteList
  def self.write!(a)
     ActiveRecord::Base.transaction do
      a.save!
      @a = Food.all.sample.id
      Quantity.create!(food_id: @a, list_id: a.id, amount: rand(6))
    end
  end
end

And here is the create portion of my controller:

def create
  list = List.new(list_params) 
  if WriteList.write!(list)
    flash[:success] = "A list has been created!"
    redirect_to list
  else
    render 'new'
  end
end

I would appreciate any further tips if anything seems off to you.

Answer 3

As ilan berci told above, there's probably errors that you're not seeing. When using a transaction, is a good practice to save the records using the bang version of the methods, this way you'll get an exception when any validation doesn't pass, and then the transaction will be rollbacked.

As for for your first question, there's much discussion about where to put complexity in a Rails project. A few years ago, the motto was "skinny controllers, fat models", meaning one should extract complexity to models, making the controllers, which are more difficult to test and where the application's flow must be evident, more readable. I prefer a philosophy of "skinny everything": there's no absolute rule, you can put everything anywhere, but once it starts to get a little complex, extract it to a class that's responsible for only one or very few things. Keep things small, simple, tested, and compose them when needed.

In your case, you could create a service or use case, and simply use it in the controller:

class CreateList
  def create!
    ActiveRecord::Base.transaction do
      @list = List.create!
      @food = Food.all.sample(1) 
      Quantity.create!(food: @food, list: @list, amount: rand(6))
    end
  end
end

As you can see, it's extremely simple to test this class! Most of the "right ways" only show their real value with experience, and there's never a unique, correct way to do things, so keep trying different techniques until you can grasp your own way to solve a problem!

Answer 4

You are getting errors except you don't see them. If you want to see them then call Quantity.create! instead of Quantity.create. (or you can assign to a temporary and see them such as: q = Quantity.create(...); q.errors

The errors are because of your 2 validations in class List (bad name by the way) checking the days and name.