3
\$\begingroup\$

In my Ruby on Rails 6 app:

I have a User class built with Devise, and I scaffolded a List class where each List should belong_to a User.

I want Lists created through my app to be automatically assigned to the logged-in user. This should happen on the back end, after the rest of the new List fields have been posted to the server.

I was able to make this happen by adding a line to the default list-params definition in the scaffolded controller:

def list_params
  params[:list].merge!(:user_id => current_user.id.to_s)
  params.require(:list).permit(:title, :active, :user_id)
end

My questions are:

  • Am I reinventing the wheel somehow with this approach?
  • Does modifying the list_params in this way affect the application outside of creating and updating Lists?
  • Have I introduced any potential security issues by doing things this way?

For reference, here is the rest of the controller:

class ListsController < ApplicationController
  before_action :authenticate_user!
  before_action :set_list, only: [:show, :edit, :update, :destroy]

# GET /lists
# GET /lists.json
def index
  @lists = List.all
end

# GET /lists/1
# GET /lists/1.json
def show
end

# GET /lists/new
def new
  @list = List.new
end

# GET /lists/1/edit
def edit
end

# POST /lists
# POST /lists.json
def create
  @list = List.new(list_params)

  respond_to do |format|
    if @list.save
      format.html { redirect_to @list, notice: "List was successfully created." }
      format.json { render :show, status: :created, location: @list }
    else
      format.html { render :new }
      format.json { render json: @list.errors, status: :unprocessable_entity }
    end
  end
end

# PATCH/PUT /lists/1
# PATCH/PUT /lists/1.json
def update
  respond_to do |format|
    if @list.update(list_params)
      format.html { redirect_to @list, notice: "List was successfully updated." }
      format.json { render :show, status: :ok, location: @list }
    else
      format.html { render :edit }
      format.json { render json: @list.errors, status: :unprocessable_entity }
    end
  end
end

# DELETE /lists/1
# DELETE /lists/1.json
def destroy
  @list.destroy
  respond_to do |format|
    format.html { redirect_to lists_url, notice: "List was successfully destroyed." }
    format.json { head :no_content }
  end
end

private

# Use callbacks to share common setup or constraints between actions.
def set_list
  @list = List.find(params[:id])
end

# Only allow a list of trusted parameters through.
def list_params
  params[:list].merge!(:user_id => current_user.id.to_s)
  params.require(:list).permit(:title, :active, :user_id)
end
end

And here is the form template:

<%= form_with(model: list, local: true) do |form| %>
    <% if list.errors.any? %>
      <div id="error_explanation">
        <h2><%= pluralize(list.errors.count, "error") %> prohibited this list from being saved:</h2>

        <ul>
          <% list.errors.full_messages.each do |message| %>
            <li><%= message %></li>
          <% end %>
        </ul>
      </div>
    <% end %>

    <div class="field">
      <%= form.label :title %>
      <%= form.text_field :title %>
    </div>

    <div class="field">
      <%= form.label :active %>
      <%= form.check_box :active %>
    </div>


    <div class="actions">
      <%= form.submit %>
    </div>
  <% end %>
\$\endgroup\$
1
\$\begingroup\$

Why do we need user_id in params at all? Probably, there is a need to add some context to a current scope. And yes, you're trying to write straightforward logic in a different way.

Here is some suggested refactoring:

class ListsController < ApplicationController
  def new
    build_resource
  end

  def create
    build_resource

    respond_to do |format|
      if update_resource
        # ...
      else
        # ...
      end
    end
  end

  def update
    update_resource
    # ...
  end

  def destroy
    destroy_resource
  end

  private

  def permitted_params
    params.require(:list).permit(:title, :active)
  end

  def resource
    @resource ||= current_user.lists.find(params[:id])
  end

  def build_resource
    @resource = current_user.lists.new
  end

  def update_resource
    resource.update(permitted_params)
  end

  def destroy_resource
    resource.destroy
  end
end

For those pure-CRUD controllers common logic can be easily moved into concern. We've created a gem some time ago https://github.com/cimon-io/unobtrusive_resources

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.