Registration Field Guide Fix?

[AviFS]

In reference to:

https://rodauth.jeremyevans.net/rdoc/files/doc/guides/registration_field_rdoc.html

I know you are committed to maintaining your projects at zero issues, so I felt this was more appropriate here as I don't strictly know it to be a bug.

In both example A, and example B, validating the presence of the name field appears as:

    # Validate presence of the name field
    unless name = param_or_nil("name")
      throw_error_status(422, "name", "must be present")
    end

and

# Validate presence of the name field
throw_error_status(422, "name", "must be present") unless param_or_nil("name")

Now, in principle this does validate the presence of the name field, however in practice I found I was using it to check that the name field was filled in. Not simply that the parameter was submitted.

After some initial confusion, I ended up using:

# Validate presence of the name field
throw_error_status(422, "name", "must be present") if param("name").empty?

which I might recommend for the guide, if this is the intended behavior. (Or at least a note that this doesn't confirm the form input was filled out, only that the form input exists.)

Detail

That method is defined in the source as

def param(key)
  param_or_nil(key).to_s
end

So in effect it checks if it's nil, or if it's empty.

I'd be remiss not to thank you for your projects and your work maintaining them!

[jeremyevans]

@janko Making this change to the guide seems reasonable, but since you authored this guide, I wanted to check with you as to whether the use of param_or_nil is intentional here. It seems reasonable that you could use param_or_nil for a field that wasn't required, but the guide says at the top: such as requiring the user to also enter their full name or their company’s name. Alternatively, we could change requiring to allowing at the top.

[jeremyevans]

Thank you for pointing that out. We do want the examples to be correct. In this case, I think it's better to have the assignment separately, so I committed a change for that.

[AviFS]

I'm thinking through what I'd do as in this case, where I wanted to assign a first name and a last name on account creation, if I used part A. I think I'd prefer the one-line version of the if statements.

# Validate presence of the name fields
throw_error_status(422, "first_name", "must be present") if param("first_name").empty?
throw_error_status(422, "last_name", "must be present") if param("last_name").empty?

# Assign the new fields to the account record
account[:first_name] = param("first_name")
account[:first_name] = param("last_name")

This would also make it more in line with part B, where a "one-line if" is used and the record is created with param("name") without assigning it to a variable.

This is now completely beside the point, but I also used:

throw_error_status(invalid_field_error_status, "first_name", "...") if param("first_name").empty?

as opposed to throw_error_status(422, ...). I don't know if this is desired, or if 422 is a sufficiently used code that using it as is is fine.

[AviFS]

Thank you for pointing that out. We do want the examples to be correct. In this case, I think it's better to have the assignment separately, so I committed a change for that.

I'm sorry, I missed your reply. Thank you for doing this again and your responsiveness. I suggested the above to be more consistent with the second example, which uses param("name") in both places rather than assigning (and uses an inlined if statement). I think it'd be best if whatever approach the guide uses was consistent between the two examples, since they're both meant to do the same thing, just that A) inserts the fields into the same table and B) inserts the fields into a different table. However I fully defer to and respect your judgement!

[jeremyevans]

I'm fine with the approach you recommend. Can you please submit a pull request for it?

[AviFS]

Gladly. I've ended up making the change as minimal as possible to janko's original, just removing the name assignment and checking if param("name").empty? but not inlining the conditional. I'm torn on having the longer comment (I liked that the code annotations in the guide are descriptive but simple enough to be copy/pasted as they are, rather than asking implicitly to be customized) but couldn't figure out where else to put it, and can't directly delete something you wrote. And I'm sure it doesn't really matter that much.

As for the throw_error_status did you prefer the use of 422 directly, or of invalid_field_error_status? Even aside from which is better code, there might be an argument one way or the other on the basis of exposition, and what would be less confusing to newcomers. Either way is fine by me, I just want to present the possibility.

Hope to hear from @janko as well before it's merged, as the author of the guide. However, the pull request is submitted. I'll gladly update it based on any updates from these two points, but I want to leave it submitted so you, Jeremy, feel like you can finish and conclude this whenever.