1.file

class TodoListsController < ApplicationController
  before_action :set_current_user
  
  def index
    sort_by = params[:sort] || 'created_at'
    sort_order = (params[:asc] == 'false') ? 'ASC' : 'DESC'
    # This seems like it might be vulnerable to SQL injection.  It might be better to whitelist the sortby column.
    @lists = TodoList.where(user_id: User.current_user.id).order("#{sort_by} #{sort_order}")
  end
  
  def show
    @list = TodoList.find(params[:id])
  end
  
  # This method should be on a TodoItem Controller.  It is weird to be finding a TodoItem by the id param, when that id param should be
  # mapping to a TodoList given the route.  Additionally, we're not rescuing anything here if complete! raises.
  def complete_item
    TodoItem.find(params[:id]).complete!
    redirect_to :back
  end

  def new
    @list = TodoList.new
  end

  def create
    # Again, it would be better to whitelist the permissible params here using strong parameters
    @list = TodoList.new(params[:todo_list])

    if @list.save!
      redirect_to todo_lists_url
    # we'll never get to the else branch when if saving fails here as we'll raise.  We need to either rescue, or use save instead of save!
    # If save fails, it might be more descriptive to include the validation error instead of "error saving list", and to keep that
    # for when we raise.
    else
      flash[:notice] = "Error saving list"
      redirect_to new_todo_list_url
    end
  end
  
  # I am not sure what the value is in this method.  It doesn't seem to be saving us anything, and if it's really useful,
  # it should be moved to ApplicationController.
  # Additionally, I would remove it from the before filter and rewrite it as
  # def current_user
  #   @current_user ||= User.set_current_user(session[:user_id])
  # so we don't need to look it up on every controller action, even when we're not using it.
  def set_current_user
    User.set_current_user(session[:user_id])
  end
end

2.file

class TodoList < ApplicationRecord
  has_many :todo_items
  belongs_to :user
  
  before_save :set_usr
  
  def formatted_title
    title + (todo_items.empty? ' (Complete)' : '')
  end
  
  # If we were to keep this method, I would suggest renaming it to 'set_user'.  In the case we moved these
  # functions to ActionController, I would explicitly pass in current_user,
  # ie TodoList.new(params[:todo_list].merge(user_id: current_user.id)).
  def set_usr
    self.user_id = User.current_user.id
    save
  end
end

3.file

class User < ApplicationRecord
  has_many :todo_lists
  
  # I don't think a method like this should exist in the context of the model,
  # and as in the comment before, I would move this to ApplicationController.
  # Additionally, this could be reduced to memoization, if @_current_user ||= value
  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

4.file

class TodoItem < ApplicationRecord
  belongs_to :todo_list

  # It might be good to validate other fields as well, at the very least we know done is a boolean.
  # completed_at might be more descriptive than 'done'.
  validates :title, presence: true

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

5.file

# These two files contain a large amount of duplication.  I would extract all this out to a common partial and
# render it from both pages.
<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>

6.file

<%-# 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 %>