6
\$\begingroup\$

The actions I'm wondering about are members, add_student, and remove_student. They work just fine, but they aren't RESTful and it feels like they should be handled (perhaps) by the StudentsController.

Note that members and show are the same. The only difference is that the members.html.erb view iterates over and displays @classroom.students while show.html.erb displays info about the classroom itself.

class ClassroomsController < ApplicationController

  # GET /classrooms
  def index
    @classrooms = Classroom.all.order('name ASC')
  end

  # GET /classrooms/1
  def show
    @classroom = Classroom.find(params[:id])

    authorize @classroom
  end

  # GET /classrooms/1/members
  def members
    @classroom = Classroom.find(params[:id])

    authorize @classroom
  end

  # GET /classrooms/new
  def new
    @classroom = Classroom.new
    @classroom.build_avatar

    authorize @classroom
  end

  # GET /classrooms/1/edit
  def edit
    @classroom = Classroom.find(params[:id])
    @classroom.build_avatar unless @classroom.avatar

    authorize @classroom
  end

  # POST /classrooms
  def create
    @classroom = Classroom.new(classroom_params)

    authorize @classroom

    if @classroom.save
      redirect_to classrooms_path, notice: 'Classroom was successfully created.'
    else
      render action: "new"
    end
  end

  # PUT /classrooms/1
  def update
    @classroom = Classroom.find(params[:id])

    authorize @classroom

    if @classroom.update(classroom_params)
      redirect_to edit_classroom_path(@classroom), notice: 'Classroom was successfully updated.'
    else
      render action: "edit"
    end
  end

  def add_student
    @classroom = Classroom.find(params[:id])
    @student = Student.find(params[:classroom][:student_ids][0])

    authorize @classroom

    @classroom.students << @student

    respond_to do |format|
      @count = @classroom.students.count
      format.js
    end
  end

  def remove_student
    @classroom = Classroom.find(params[:id])
    @student = Student.find(params[:classroom][:student_ids][0])

    authorize @classroom

    @classroom.students.delete(@student)

    respond_to do |format|
      @count = @classroom.students.count
      format.js
    end
  end

  # DELETE /classrooms/1
  def destroy
    @classroom = Classroom.find(params[:id])

    authorize @classroom

    @classroom.destroy

    redirect_to classrooms_path
  end

  private

  def classroom_params
    params.require(:classroom).permit(:name, :description, avatar_attributes: [:id, :avatar, :_destroy],
      :student_ids => [])
  end

end

Here are the models:

# student.rb
class Student < ActiveRecord::Base
  has_many :classroom_memberships, :dependent => :destroy, :inverse_of => :student
  has_many :classrooms, :through => :classroom_memberships
end

# classroom.rb
class Classroom < ActiveRecord::Base
  has_many :classroom_memberships, :inverse_of => :classroom, :dependent => :destroy
  has_many :munchkins, :through => :classroom_memberships
end
\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

The actions I'm wondering about are members, add_student, and remove_student. They work just fine, but they aren't RESTful [...]

True, they're not part the usual CRUD actions that can be expressed with HTTP verbs. But then neither are the default new or edit actions, so it's not like going beyond CRUD is automatically illegal.

Even so, associations do always present a bit of challenge. Are you adding the student to the class, or the class to the student?

However, in this case it appears that you already have a join model: ClassroomMembership. So going back to the CRUD actions, consider what you're creating when you call add_student: A membership.

So what you might want to do is simply make a ClassroomMembershipsController, since that's the resource you're manipulating. Said controller can then have the usual RESTful CRUD actions, keeping StudentsController or ClassroomController pretty clean.

This is just an idea, though. Again, if it's simpler to keep your actions on the ClassroomController then do that. But conceptually what you're doing is manipulating the association model.

\$\endgroup\$
4
  • \$\begingroup\$ Adding a ClassroomMembershipsController makes a lot of sense! Definitely a more RESTful approach. Follow-up question: ClassroomMemberships don't have their own views--so I guess I just render the Classroom view from the ClassroomMembershipsController and that's kosher? \$\endgroup\$
    – Rob Sobers
    Commented May 21, 2015 at 2:59
  • \$\begingroup\$ @RobSobers Or you can just do a redirect_to @membership.classroom. A controller doesn't need to have views; its purpose is merely to respond to requests, and sending a redirect is a response. The default destroy action doesn't have a view either, since it just redirects. Incidentally, I'd consider the name Enrollment for the join model, only because it sounds a little more straightforward. But renaming a resource is a chore and has knock-on effect, so don't do it lightly. It can be done of course, just know what you're getting yourself into :) \$\endgroup\$
    – Flambino
    Commented May 21, 2015 at 8:23
  • \$\begingroup\$ Good point. And I suppose in this case, I won't even need to redirect since both add_student and remove_student respond with JavaScript, which will be sent to whichever page is making the request. \$\endgroup\$
    – Rob Sobers
    Commented May 21, 2015 at 12:19
  • \$\begingroup\$ @RobSobers Sure. In that case, I often find it easiest to respond with plain HTTP status codes, and no actual content. E.g. instead of render ... or redirect_to ..., you can just call head :created (when a membership was created), head :conflict if the membership already exists, etc. etc.. See httpstatus.es for more. There are plenty of standardised response codes, that carry a lot of meaning \$\endgroup\$
    – Flambino
    Commented May 21, 2015 at 13:23
3
\$\begingroup\$

You could refactor add_student and remove_student using blocks

  def add_student
    find_class_and_student do
      @classroom.students << @student      
    end
  end

  def remove_student
    find_class_and_student do
      @classroom.students.delete(@student)
    end
  end

  private

  def find_class_and_student
     @classroom = Classroom.find(params[:id])
     @student = Student.find(params[:classroom][:student_ids][0])        
     authorize @classroom

     yield

     respond_to do |format|
        @count = @classroom.students.count
        format.js
     end   
  end

This is just pulling your code inside some other actions.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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