/app\controllers\todo_list_controller.rb

app\controllers\todo_list_controller.rb

class TodoListsController < ApplicationController
  before_action :set_current_user
  
  def index
    sort_by = params[:sort] || 'created_at'
    sort_order = (params[:asc] == 'false') ? 'ASC' : 'DESC'
    # COMMENT: Feeding unsanitized query params as string into the database is always A VERY BAD IDEA. Luckily in this example
    # the attack vector seems restricted to the sorting, but who knows if there is any bug in the ActiveRecord order method
    # that allows to widen the scale of the attach to a full SQL injection code.
    # 
    # The proper way to do it is what is done in the sort_order, where we always control what strings are fed to the database.
    @lists = TodoList.where(user_id: User.current_user.id).order("#{sort_by} #{sort_order}")
  end
  
  def show
    # We must check for user ownership, otherwise a malicious user could see all the other users TodoList items just changing
    # the id in the url (something dead easy to do if the id is the typical sequential integer of most SQL databases).
    @list = TodoList.find(params[:id])
  end
  
  # Mark an item as done and return user back from whence they came!
  def complete_item
    # We must check for user ownership, otherwise a malicious user could complete the other users TodoList items just changing
    # the id in the url (something dead easy to do if the id is the typical sequential integer of most SQL databases).
    TodoItem.find(params[:id]).complete!
    redirect_to :back
  end

  def new
    @list = TodoList.new
  end

  def create
    # I don't see any strong params here ;) Again, unsanitized input is a no-no.
    @list = TodoList.new(params[:todo_list])

    if @list.save!
      redirect_to todo_lists_url
    else
      flash[:notice] = "Error saving list"
      redirect_to new_todo_list_url
    end
  end
  
  def set_current_user
    User.set_current_user(session[:user_id])
end

app\models\todo_item.rb

# COMMENT: From the complete! method, we could deduce that the database has a boolean "done" field for
# TodoItem records. 
#
# I know that not defining the fields each model contains and define only the relations and validations is the recommended
# way to go in ActiveRecord, but it makes me frown every time I see it. 
#
# The model is esentially a contract, to expose a domain model data to be manipulated from the rest of the code. Not knowing
# what its inside this model at first sight, and having to resort to look at methods or at another external tool (like a database
# schema dump) to know what fields is supposed to hold, which are allowed values and ranges, etc. makes zero sense IMHO.
#
# Luckily for me, most of the time I've been working in Mongoid backed apps, where is mandatory to define the fields (as Mongo
# is schemaless) :)
#
# This is just my opinion, but I would gladly have preferred that given the tendency of Rails core developers to love magic
# and convention over configuration, that the default would be something like Django South, where migrations are done
# automatically to reflect the changes you do in the model fields.

class TodoItem < ApplicationRecord
  belongs_to :todo_list

  validates :title, presence: true

  # Completes to do item, saving to database.
  def complete!
    update!(done: true)
  end
end

app\models\todo_list.rb

class TodoList < ApplicationRecord
  has_many :todo_items
  belongs_to :user
  
  before_save :set_usr
  
  # COMMENT: Assuming "title" is a TodoList field holding the title of the list, I don't see which value this method
  # coud add. Probably the developer meant something along the lines of "todo_items.where(done: true).empty?" instead.
  # And this will match for lists without any items in it, so an extra check to take in account that would be good to do.
  def formatted_title
    title + (todo_items.empty? ' (Complete)' : '')
  end
  
  # Assign this list to the currently signed in user when it's createdç
  #
  # COMMENT:Lots of thing wrong in the set_user method:
  #
  # 1 - Never use "save" in an before_save callback (in fact never user it un any database-related callback, except perhaps
  #     in after_create, and there are very few use cases where this would make sense, like bootstraping some data coming from
  #     some slow asynchronous source.
  #
  # 2 - Even ignoring the "save", this method does not make much sense to me. We are coupling the new todo_list to the current
  #     user, and what happens if we want to create todo_lists in some process in backend that is not tied to a user session? or
  #     if we want some admin to create todo_lists in behalf of another user? I don't like this method the slightest, we are
  #     coupling too much the model domain with some specific business logic that is bound to evolve over time.
  #
  #     In my experience, would be better to skip this method and replace it for some tests that test that the controllers 
  #     are injecting the current user when creating the todo_list, taking in acccount also that is the tests coverage is not
  #     enough and somebody slips and tries to enter a todo_list without user, the FK validations will raise an error and
  #     we will have a rollbar (or any other error reporting system in place... because you are using one, for sure ;) )
  #
  #     At the very least, I will change it to "self.user_id ||= User.current_user_id" to only set it as default if it was empty.
  #
  # 3 - As a minor hindrance, far below the other two, I woudl change the naming. No reason to abbreviate set_usr instead
  #     of set_user. If possible, its better to use the complete model or field names to be 100% sure what are we referring to.
  def set_usr
    self.user_id = User.current_user.id
    save
  end
end

app\models\user.rb

class User < ApplicationRecord
  has_many :todo_lists
  
# COMMENT: I have two problems with the chosen mechanism to store the current user: the first one conceptual, and the second
# one technical/practical.
#
# 1 - Conceptual problem: Current user does not belong to the User class. A current user it a current user inside a scope, being
#     it a web request, some code in a job, or any other scope.
  
#     It does not make sense conceptually to store it as a class instance variable that is shared by all the code that uses that model.
#     Every scope should keep track of the current user on his own. The usual method in rails controllers is store it as an
#     instance variable at controller level automatically on each request throuhg a before_action callback.
#
#     If you want to avoid database hits between requests for the same user, you could use some caching mechanism like Redis
#     to store the user (and say hi to the wonderful new world of errors and logic handling related to cache invalidation).
#
# 2 - Technical problem: Despite the conceptual problem, using a class instance variable to keep this data smells like problems
#     to me. For sure it work depending on the setup, but if we are using multithreaded servers or other methods that access
#     the same code at once, we will overwrite the current_user constantly and will create a mess as a result. I'm not 100%
#     sure of what could trigger that behaviour, but I'm pretty confident that given time I could design an scenario where
#     this approach will fail.
  
  
  def self.set_current_user(id)
    return if @_current_user.present?
    @_current_user = User.find(id)
  end
  
  # Avoid a DB query each time we need the currently logged in user
  def self.current_user
    @_current_user
  end
end

app\views\todo_lists\index.erb

<%-#COMMENT: Former app\views\todo_lists\list.erb, but that name was wrong if you want the Rails "magic" of convention over configuration
to happen here  -%>
<%-# Display the current user's lists along with any uncompleted items -%>

<% @lists.each do |list| %>
  <h2><%= list.formatted_title %></h2>
  <ul>
    <% list.todo_items.where(done: false).each do |item| %>
      <li><% item.title %> - <%= link_to "Done!", complete_item_todo_lists(item), method: :put %></li>
    <% end %>
  </ul>
<% end %>

app\views\todo_lists\show.erb

<h2><%= @list.formatted_title %></h2>
<ul>
  <% @list.todo_items.where(done: false).each do |item| %>
    <li><% item.title %> - <%= link_to "Done!", complete_item_todo_lists(item), method: :put %></li>
  <% end %>
</ul>