check_tos() causes infinite loop on Checkout

[dgoerger]

Case:

• admin does not require users to enter a phone number
• user account created without a phone number and never accepts ToS
• admin changes App Settings to require phone number
• checkoutperson tries to check something out for said user
• hits code block:

  def check_tos(user)
    return true if user.terms_of_service_accepted

    user.terms_of_service_accepted = params[:terms_of_service_accepted].present?
    return user.terms_of_service_accepted ? user.save : (flash[:error] = "You must confirm that the user accepts the Terms of Service.") && false
  end

If I'm parsing this correctly, the return statement says:

return {
  if user.terms_of_service_accepted
    user.save #invalid without user phone number, will return false
  else
    (flash[:error] = "You must confirm that the user accepts the Terms of Service.") && false #should we be returning strings AND booleans in the same call? is this kosher coding? why don't we set the flash where this is called from?
  end
}

... i.e., there's no way to return true on this function in the above scenario. I'm not sure why the app is spiralling into an infinite loop instead of hitting the redirect_to :back and return and bouncing back to the manage page, but apparently, at least sometimes, it is.

[orenyk]

Related to #954 and #675 :-(. We'll see what the best way to fix this is, and, if feasible, back-port this to v3.4.

[orenyk]

I can see us solving this in a number of ways:

1. Temporarily set require_phone to false during check_tos
2. Add dummy data to all users missing phone numbers (e.g. (555) 555-5555) whenever the require_phone setting is changed to false. The admin should be presented with both a confirmation box and warning stating the number of records that have been modified, and all users with dummy data should be somehow flagged as "missing phone number" (this could tie into flagging users as in Feature Request: Marking patrons that have lost equipment / "Patron has lost equipment" option in check in procedures  #460)
3. Alternatively, add an additional field to the User model, missing_phone that defaults to false but is set to true for all users that lack a phone number when the require_phone setting is set to true. We can then override the validation for those users, thereby requiring all new users to input a phone number. The users could still be flagged and the admin warned as in option 2.

I'm leaning towards option 3, it's a little clunky but should work. We could add code in the migration to add the missing_phone column that will, if require_phone is set to true go through all existing users and appropriately set the missing_phone parameter. We could also add a UI element on the user show view to reflect it.

[orenyk]

Also, verified on master (although it doesn't infinite loop, just redirects)

[dgoerger]

What would be the downside of bypassing user validations when marking ToS as signed (in this method) ?

[orenyk]

That's basically what I was trying to get at with option 1 above. I don't really see a problem with it, except it doesn't help us if we ever try to update the user somewhere else with similar results. I think modifying the skip_phone_validation method in the User model to return true if missing_phone is set as true is a good way to avoid generally breaking our model when we set require_phone to true with existing users. I also don't think it will be to hard to implement; I was pretty close to working code before I had to pause tonight. Does that make sense?

[orenyk]

I think this is now finished; I added a migration to add a missing_phone parameter to the users table which automatically flags no_phone users as missing_phone if the require_phone setting is true and updated the AppConfigController#update method to flag no_phone users if you change require_phone to true. Now testing that this resolves the issue.

[orenyk]

It works! Issuing PR now :-)