harleyttd (Harley Trung)

If we get here, it's probably because @payform_item_set.save fails right? (again im not sure if << ever fails), so why don't you dump @payform_item_set.errors.full_messages out to flash[:error], because only admin can see this it maybe easier to see why it fails.

Also, does "<<" save anything? I thought not but this usage hints so. Shouldn't it be:
@payform_item_set.payform_items << @payform_items first, and then
@payform_item_set.save after that?

Hope you don't mind explain if some of the questions are stupid=)

You can just use:
users = User.find params[:user_ids]
and then go through each user instead of each user_id

This will throw an exception if any of the id is invalid -- a desired behavior because your code is doing the same. However it's more efficient because you don't have to go create payforms for some users half way and roll back later if you see one invalid user.

Good idea moving it to before_save.
Do you need a newline at the end of [IP Address changed ...] ? Just wondering
Also .last could be more readable than [-1] =)

In _user_options.html.erb: The "Return to your current shift report" link should only be shown when there exists such a report. In fact, "user_has_active_shift?" returns true, so shouldn't current_user.current_shift.report always NOT nil? Thus I don't think the check for that is needed. The link code should be simply:

<%=link_to "Return to your current shift report", current_user.current_shift.report %>

====== In shifts_controller.rb: Does "<<" operator ever fail? Even so, in this case we don't really handle the fail case anyway, consider moving the long 2nd part of if condition in to the if block itself =)

"break" in a while or for loop is OK if it's used properly: to short-circuit a loop and it looks clear what it does. alternatively, you can use a flag and check that flag in the while/for loop condition, which i thought not more attractive. yet we don't want to run the whole loop if we can end it early.

and oops it's not "people_count < 0", but rather "people_count < prioritized_location.min_staff"

changes look good

The comment has since been removed.