I just got burned.
My fault. My responsibility.
The Lesson
I'm skipping right to the lesson that I learned from today: Don't assume that you know what a control does. Oh, and test thoroughly.The Background
Let me give you some of the back story. I was tasked with creating an internal website to help distribute some gift cards. The site was meant to replace reams of paper that had been used in the past. The requirements were simple: have an employee digitally "sign" something stating that they did indeed receive their gift card.I had a nice, complicated solution that gave everyone the "personalized" feel that HR was desiring. Due to other complications out of my control, I had to scale back to a much simpler site that simply asked for the person's employee number and last four digits of their SSN. The work I did on the more complicated site was not a loss. It did show us some issues with our Active Directory data and uncovered an error with a system that HR used.
I tested the site with four or five of my coworkers, and it worked as expected. I presented it to HR, and they absolutely loved it. The design was elegant and captured the holiday spirit.
The Launch
The day before the launch, HR gave me a new set of data. I cleaned it up and got it into my database. Everything was looking good. We posted the link on HR's internal website, and they prepared packets for all the directors (60+ directors distributing to 2400+ employees.)On the day of the launch, I had to attend a meeting. When I got back to my desk, I had quite a few telephone calls, emails, and help tickets concerning the website. Oh, no, I thought.
The Problem
The error reports were from people whom had leading zeroes in either their employee numbers or the last-four digits of their SSN. It didn't make any sense to me. The range control was returning false when checking the value for validity. It would not allow them to "sign" for their cards.Why are the values being rejected?
What I Had Done
I had to make the site using ASP.NET Web Forms. I would have preferred MVC, but I had a constraint that made me feel that web forms was the better way to go. On the page, I used two validation controls per field. I used the Required Field Validator and the Range Validator controls. Both fields, employee number and last-four of SSN, were both numbers, so range validator made sense to me. Both had the minimum value of 1 and the maximum value were 99999 and 9999 respectively. Here's a sample tag:<asp:RangeValidator ID="rvEmployeeNumber" runat="server" ControlToValidate="txtEmployeeNumber" Display="Dynamic" Text="*Must be Numeric" MinimumValue="1" MaximumValue="99999"></asp:RangeValidator>
Investigation
I pulled up the documentation (http://msdn.microsoft.com/en-us/library/system.web.ui.webcontrols.rangevalidator(v=vs.100).aspx) and reviewed it. I assumed* that the validation control would default to a number, since it is a range after all. I located the type property, and saw that there were five types: String, Integer, Double, Date, and Currency.
I used a test page to add five controls with a range validator of each type. I didn't think that I needed to test a control with the type of Date, but it's best to check. The three number types gave no issue. The Date range validator broke as I expected, since the minimum and maximum values weren't even date values. The String range validator recreated the error condition on the site. We have a winner.
So, the default type for the Range Validator control is String. To confirm, I removed the Type attribute, and output the default value to a label on the page. The default type was, indeed, String. D'oh!
What I Should Have Done
Since the employee number and the last four digits of the SSN aren't numbers meant to use with calculations, it does make sense to treat them as strings. After all, I do store them as strings in the database for that very reason.
A more correct way to use the Range Validation control would have been this way:
<asp:RangeValidator ID="rvEmployeeNumber" runat="server" ControlToValidate="txtEmployeeNumber" Display="Dynamic" Text="*Must be Numeric" Type="String" MinimumValue="00001" MaximumValue="99999"></asp:RangeValidator>
Do:
- Set the Type attribute. Even though the default is String, set it, so that others will know that you meant to use the type of String.
- Add the leading zeroes.
- Remember, it's a string, not a number.
I'm not certain what my biggest mistake was. I thought of the input as a number, and gave no thought to the leading zeroes. If I had written the client-side validation code, I would have used a regular expression that made certain that it was x characters in length and that each character was a digit (0 - 9). To be truly thorough, I should have run all the employee numbers through the validation logic.
The Brightside
Even with that snafu, the feedback has been very positive, though I am still sore from my mistake. It has made the card distribution go smoother, and it will help the Accounting department. Hopefully, I have reduced paper waste, and cut down the time spent by HR and Accounting.I don't consider myself arrogant, but I can be rather prideful of my code. This was a good lesson in humility, and I did not enjoy it...
