Checking if an associated object has failed to save/create

Question

I smell something wrong here, but am not quite sure what the most idiomatic way to solve the problem.

I have a controller for (job)listings and it has a method called apply. This method is used when someone has a resume and is clicking to apply for the job associated with the listing.

def apply
 set_resume
 if @resume.apply_to(@listing)
 redirect_to @listing
 else
 redirect_to @listing, alert: 'Unable to apply for job'
 end
end

This calls a method on the resume object:

def apply_to(listing)
 jobapp = Jobapp.new(resume: self, listing: listing)
 jobapp.save
end

Everything above works just fine - it just seems inelegant (and I don't get the errors associated with the jobapp object, although that's not vitally important).

If I use the following method in place of the above method:

def apply_to(listing)
 Jobapp.create(resume: self, listing: listing)
end

The conditional if @resume.apply_to(@listing) always returns true, even if the Jobapp failed to be created.

What is a cleaner way of achieving this task?

Answer 1

Given the current setup, the simplest thing would probably be to raise an exception using #save!:

def apply_to(listing)
 jobapp = Jobapp.new(resume: self, listing: listing)
 jobapp.save! # note the bang (!)
end

And in the controller

def apply
 set_resume
 @resume.apply_to(@listing)
 redirect_to @listing
rescue ActiveRecordError
 redirect_to @listing, alert: 'Unable to apply for job'
end

It may be worth the effort to rename your method to apply_to! (also with a bang) to make it clear that it can raise an exception.

---

As for why create doesn't work, it's not that it returns true - it returns the new record (and all objects except nil and false are considered "true"). To check if create actually managed to persist that record in the database, you can check record.persisted?. If true, then it was saved just fine.

So using your create version of the method, you could do

def apply
 set_resume
 jobapp = @resume.apply_to(@listing)
 if jobapp.persisted?
 redirect_to @listing
 else
 redirect_to @listing, alert: 'Unable to apply for job'
 end
end

This would also let you access jobapp.errors, if you want.

---

A few other things caught my eye:

1. set_resume should probably be in a before-filter. Or be a method that just returns a record, leaving it up to the caller to stick it in an instance variable (or not)

2. I imagine a jobapp belongs to a listing (and/or a resume). If so, it'd be cleaner to build it like so:

   jobapp = owner.jobapps.new(other: foo)
   

Given that it's this simple, though, it might be something you could just do in the controller.

It's never quite clear where a join model like jobapp (which I assume is basically a join model) should be created. Should it be in the Resume model or in the Listing model? Either one makes sense. So the Solomonic solution is do neither (at least for now), and handle it in the controller:

def apply
 jobapp = @listing.jobapps.new(resume: @resume)
 if jobapp.save
 redirect_to @listing
 else
 redirect_to @listing, alert: 'Unable to apply for job'
 end
end

Lastly, you might want to tweak that error message. It seems a little "blunt" to me. Though it's the server that failed, it sounds as if the user is unable to apply for the job. "Failed to save job application" might be more appropriate

• Ecnalyr: Your assumptions were correct and you were definitely a step ahead of me with the idea of, ". . . Either one makes sense. So the Solomonic solution is do neither (at least for now), and handle it in the controller. . ." I like this.
• Flambino: @Ecnalyr Yeah, it's the sort of solution I keep forgetting to use myself, because you always feel it belongs in a model - but it works, and really makes a lot of sense to have it in the controller. And if, some day, you need more logic, you can start by extracting it into a method or a service object.