drhenner / ror_ecommerce

Ruby on Rails Ecommerce platform, perfect for your small business solution.
www.ror-e.com
MIT License
1.21k stars 409 forks source link

Useless validations #96

Closed carlosipe closed 11 years ago

carlosipe commented 11 years ago

First of all, thank you! I'm learning a lot of Rails reading your code :+1: (and sometimes fighting with it).

I'm just wondering what is the sense of validating age and names in user_model. I see you don't allow old people (> 110) neither names containing numbers.

What's the goal here? Clearly there are not security reason involved and I think it just adds useless lines of code, maybe needless bugs and nothing in exchange.

I think validations should mainly take care of security issues, validating names or ages doesn't guarantee that you are going to receive real data, so I don't see the point.

Thanks!

drhenner commented 11 years ago

First thanks for the feedback...

I'd argue validations are more than security issues (I assume you would agree that a nil name is not allowed in many fields). That being said for birth dates I normally have a form field default to an invalid result. Thus ensuring a user at the very least tried to set a valid year. All being said is it critical to even have birthdate in most e-commerce apps? Nope.

I could easily remove the field and be happy. That said, with the field being in the app I think having at least logical bounds makes sense. The answer really is in the hands of the business owner and not the developer.

I'm guessing most businesses will remove the field altogether. I treat ror_ecommerce as a starter app that a business guy/developer can flex into what they want to make. If the validation is not important to a business removing it works for me.

OK now that I have written this much I have another idea... What about making the value === NIL if the person put invalid data? That seems to be best for the business because they don't have a lot of invalid data. Also the form is not rejected and hence no negative feedback to the user.

Thoughts?

carlosipe commented 11 years ago

Oh, I agree with you. Validations are more than for preventing security issues, and I think it's nice having logical bounds for some inputs. Validating numericality of age could be important (you could need to target people older than 30 for some task). But >110 and <2 validations don't make sense at all.

You can't validate the user enter real data, so you have to deal with people entering fake data (and there could be legit reasons to do it). In any case, what's the goal with bothering your user if he says he's 111 years old (are you enforcing real data? No; are you establishing logical bounds? neither). I think you are just making useless computation (not expensive, though), making your code longer and therefore more difficult to read (I guess this is more important, mainly because the user of ror_ecommerce is likely going to modify all these things and need to understand your code before).

My point has nothing to see with these validations in concrete, but with the philosophy behind validations. I think validations (which can bother your user) should only exist when absolutely necessary. You risk bothering your user with a validation that doesn't benefit you, and bothering your code reader with a validation that doesn't matter. Personally, I always prefer automatic sanitizing over returning a failing validation when possible.

About the NIL option, I'm not sure, it depends on what are going to use these fields for. If you are going to do calculations with ages I guess you should ensure numericality at least...

Finally, I want to thank you again. I'm starting to use ror_ecommerce actively :+1: and I wanted to say hello before starting making pull requests or giving real feedback (concrete things more than arguing on code style)


The origin of the question: I got caught by the name validator in your code, I was integrating omniauth with your user model, and at the moment I was using ajax without showing the validation errors and it took me a while to realize that carlos2 wasn't a valid name, so I wonder 'why carlos2 is not a valid name? what's the security reason or "logical bound" that the user model is trying to enforce?')

drhenner commented 11 years ago

BTW: you should probably use the rails4 branch if you are starting something new. I'm waiting for the official release of rails4 to merge it into master.

Also: I would Argue carlos2 is an invalid name. Just as 12345678 is an invalid name. I believe most countries don't allow you to have a number in your name. In fact I think "Chad Johnson" (NFL football player) tried this and then just spelled out 85 in spanish for his name change. That is another story but interesting. =) NOTE: if there were a username then something like carlos2 would be perfectly fine.

I prefer the NIL result than "bad data" much of the time but that depends on the app.

Why is 2 - 110 not a logical bound for age? I would say no user will be 2 years old but these days 5 years old is not 100% out of the question. As for 110, There comes an age where we all are dead, 110 is close enough. All this said I think I'll just remove birth date all together because any business that needs birth date can add it for themselves.

Hope this comes across as non-confronting. Sometimes text sounds a bit direct. I do appreciate the feedback.

carlosipe commented 11 years ago

I'll take a look then, I didn't how stable the rails4 branch was.

I agree that carlos2 could be considered an invalid name, but that's not what I am discussing about. My point is: who cares?, "martin" is an invalid name when applied to me too and since you're not checking I'm telling you my real name, why do you spend time checking it contains numbers or not?

In the case of the name, you could have a reason (I don't see it, though) but the age case is more clear: you spent time controlling that I don't answer a number > 110, creating three new methods in your model, writing translations files for error messages (which forces me to do extra work translating) and it doesn't worth at all because it doesn't make you any difference if I say I'm 109 or I say I'm 140.

I'm a lazy programmer, and I like minimalistic code. I have nothing against you refusing more-than-110-years-old guys as users, but writing that code doesn't worth the time spent on it and worst: it opens the possibility of bugs. I think the code should only have the minimum necessary to work. I've seen a lot of times innocent validations on names which let people out (example: invalidating names containing '-' which should be valid). And personally I hate over-validations on passwords...

So, my original question was: you spend time creating methods, checking regular expressions, making tests, writing transaltions, assuming the possibility of new bugs and what do you get in exchange?

But don't take it too seriously, it became in a conversation longer than it deserves for a tiny point like this. I appreciate you took the time to answer.

PS: My english is a little rusty, I hope my words didn't sound confronting (If I ever sound too direct or plainly aggressive is just bad English, not bad intentions)

EDIT: I don't know why answering from my email messed up the format and removed all the line breaks

drhenner commented 11 years ago

So I think part of our difference is "our point of view"

I tend to develop with a business hat on first. Then I throw on the developer hat (along with a case of Mountain Dew and snacks)

From a business standpoint I want to do my best capturing good data. S0 @ typ0 in @ n@me might not be that bad but 12345 as a name is obviously bad. IMO it's best to make sure someone puts in a valid name. Sure you can not control a fictitious name but that doesn't mean you should have a complete free field. Every business has different rules but every business I have worked at has wanted a validator on the name field. Customer service likes real names.

I can't control an incorrect street address but that doesn't mean I should ignore all validation on an address. It just depends on where you draw the line and every business guy making the rules will change them.

I 100% agree with is the possibility of adding a bug for validating something like age. To be honest I added age for marketing reasons but now the birthdate field has actually been more of a pain on my side. There have been bugs in the past especially when ruby jumped from 1.8 to 1.9. You have convinced me to remove birthdate altogether.

The password validator is a double edge sword. Too simple and you make things too easy to crack. Too difficult and it is painful and customers will leave. I'd say I'm a bit more simple than I should be for some businesses but just fine for other businesses.

I need to run, love the feedback. Any thing else you find issues with?

carlosipe commented 11 years ago

Yes, it's clear that we have different point of views of how programming should be done. I like minimalism because I think simplicity leads to better adaptability, better readability, etc. I don't think the developer hat is incompatible with the business hat (I am pragmatic enough). Anyway, it was nice to talk with you about this.


In regard to other issues, yes, I'll be opening an issue ticket in a few days about payments. I really like a simple solution like Stripe (and I read you like it too) but unfortunately it is not available outside the US. In the project I'm currently working on, I'll need to be able to process payments in US, Argentina and Brasil. I think I will be working on a branch to handle asynchronous payments (IPN) and I'm willing to make it really modular, to make it easy to plug and play with ror_ecommerce. When I have a better idea of what I need I'll open a branch and a ticket to discuss implementation details. Thanks!

drhenner commented 11 years ago

Looks like Stripe is starting beta programs outside the US. take a look at drhenner/stripe_commerce if that interests you.

I'm closing this as an issue...