2
\$\begingroup\$

I am currently using a belongs_to relationship in my Rails application to link a map and a group an object that way I can assign those maps to the user based on the group they are in.

I have this mapgroup model.

class Mapgroup < ApplicationRecord
  belongs_to :map, optional: true
  belongs_to :group, optional: true
end

I am getting the map from a dropdown menu in the form but I don't want a dropdown for the group because the use is already in that group.

I opted to assign the group_id in the url to the mapgroup in the create method.

def create
  @mapgroup = Mapgroup.create(mapgroup_params)
  @mapgroup.group_id = params[:group_id] <-- right here

  respond_to do |format|
    if @mapgroup.save
      format.html { redirect_to groups_path, notice: 'Mapgroup crated' }
    else
      format.html { render :new, notice: 'That shit failed' }
    end
  end
end

Is this a good practice? I have done this in previous apps but it seems a bit...smelly. Is there a more elegant way to do this?

\$\endgroup\$
2
  • \$\begingroup\$ It's fine to assign attributes based onn the params. The one thing I would change about this would be to use .new instead of create. Since you call save later, if you use create you are saving twice which is unencessary. Also, I realize it's just a joke, but instead of returning some generic error message, you should include the validation errors from the model (@mapgroup.errors.full_messages) \$\endgroup\$ – max pleaner Mar 17 '18 at 23:52
  • \$\begingroup\$ If params[:group_id] comes from a URL param, i.e., /groups/:group_id/maps, I think it is important that you write it this way (as a way to prevent POSTing to a group that does not match the URL) \$\endgroup\$ – Pat Newell Apr 16 '18 at 10:34
1
\$\begingroup\$

You have two issues here:

  • need to find parent model first, and then build association (as group_id could be wrong)
  • create would try to save record, so you're trying to save twice. Use build/new instead.
    def create
      @group = Group.find(params[:group_id])
      @mapgroup = @group.mapgroups.build(mapgroup_params)

      respond_to do |format|
        if @mapgroup.save
          format.html { redirect_to groups_path, notice: 'Mapgroup crated' }
        else
          format.html { render :new, notice: 'That shit failed' }
        end
      end
    end
\$\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.