A Bug Fix That Has Least Impact On Existing and Future Codes

Introduction

Many developers may think that troubleshooting and bug fixing are kind of more challenging than pure development. Yes, that is right, even with a very good architecture and source coding.

Background

I was assigned a bug which reads Builders and Contractors for Entrance and Encroachment Permits should not be mandatory.

The web project is a permit approval system based on ASP.NET MVC and Code-First Entity Framework for currently four kinds of permit applications:

• Building and Land Use
• Encroachment
• Entrance
• Sign

The validated page UI is shown as below. And the database table for Builders and Contractors has four corresponding required columns.

Code and Analysis

The Contractor model class looks like this:

[MetadataType(typeof(ContractorMetadata))]
public partial class Contractor
{

}

public class ContractorMetadata
{
    [Display(ResourceType = typeof(Rsc), Name = "ContactNameLabel")]
    [Required(ErrorMessageResourceName =
    "ErrorFieldRequiredMessage", ErrorMessageResourceType = typeof(Rsc))]
    public string ContactName { get; set; }

    [Display(ResourceType = typeof(Rsc), Name = "BusinessNameLabel")]
    [Required(ErrorMessageResourceName =
    "ErrorFieldRequiredMessage", ErrorMessageResourceType = typeof(Rsc))]
    public string BusinessName { get; set; }


    [Display(ResourceType = typeof(Rsc), Name = "PhoneCountryCodeLabel")]
    public int? PhoneCountryCode { get; set; }

    [Display(ResourceType = typeof(Rsc), Name = "PhoneAreaCodeLabel")]
    [RegularExpression(@"^(|[0-9]{3,7})$",
    ErrorMessageResourceName = "PhoneAreaCodeInvalid",
    ErrorMessageResourceType = typeof(Rsc))]
    public int? PhoneAreaCode { get; set; }

    [Display(ResourceType = typeof(Rsc), Name = "PhoneNumberLabel")]
    [Required(ErrorMessageResourceName = "ErrorFieldRequiredMessage",
    ErrorMessageResourceType = typeof(Rsc))]
    [RegularExpression(@"^(|[0-9]{3}[-. ]?[0-9]{3}[ ]?[0-9][0-9]{0,2}[ ]?[0-9]{0,3}[ ]?[0-9]{0,3})$",
    ErrorMessageResourceName = "PhoneNumberInvalid",
    ErrorMessageResourceType = typeof(Rsc))]
    public string PhoneNumber { get; set; }

    [Display(ResourceType = typeof(Rsc), Name = "PhoneExtLabel")]
    [RegularExpression(@"^(|[0-9]{0,6})$",
    ErrorMessageResourceName = "PhoneExtensionInvalid",
    ErrorMessageResourceType = typeof(Rsc))]

    public int? PhoneExtension { get; set; }

    [Display(ResourceType = typeof(Rsc), Name = "MobilePhoneCountryCodeLabel")]
    public long? MobilePhoneCountryCode { get; set; }

    [Display(ResourceType = typeof(Rsc), Name = "MobilePhoneAreaCodeLabel")]
    [RegularExpression(@"^(|[0-9]{3,7})$",
    ErrorMessageResourceName = "PhoneAreaCodeInvalid",
    ErrorMessageResourceType = typeof(Rsc))]
    public long? MobilePhoneAreaCode { get; set; }

    [Display(ResourceType = typeof(Rsc), Name = "MobilePhoneNumberLabel")]
    [Required(ErrorMessageResourceName = "ErrorFieldRequiredMessage",
    ErrorMessageResourceType = typeof(Rsc))]
    [RegularExpression(@"^(|[0-9]{3}[-. ]?[0-9]{3}[ ]?[0-9][0-9]{0,2}[ ]?[0-9]{0,3}[ ]?[0-9]{0,3})$",
    ErrorMessageResourceName = "PhoneNumberInvalid",
    ErrorMessageResourceType = typeof(Rsc))]
    public string MobilePhoneNumber { get; set; }

    [Display(ResourceType = typeof(Rsc), Name = "MobilePhoneExtLabel")]
    public int? MobilePhoneExtension { get; set; }
}

If Building & Land Use permit and Sign permit don't need any information for builders and contractors (actually they really don't), or they don't mandate the info, then there is a simple fix: just comment out those Required annotations, and modify the database table so that null values are allowed for those columns.

For sure, the fix will be verified  and the bug closed when retested. It is such a quick fix, let's lay back.

But wait:

1. Why do you, as a responsible developer, allow the Contractor table to be populated with empty rows if users don't enter anything?
2. What if users just enter phone numbers without Contact Name or Business Name, in which case the new row in the table is of non-sense?
3. What if the logic is changed for Building & Land Use permit and Sign permit for the Builders and Contractors to be mandatory?
4. What if a new permit type is added with some particular requirements?

Uh-oh, it is not a good time to lay back. Let's go back.

First, I added a new property to model class Contractor to mark if required properties are fulfilled, as in:

[MetadataType(typeof(ContractorMetadata))]
public partial class Contractor
{
    public bool RequiredPropertiesFulfilled { get; set; }
}

As this new property is just a helper to tell if the Contractor data is good, and it has nothing to do with database, it should be ignored in the mapping class:

public class ContractorMap : EntityTypeConfiguration<contractor>
{
    public ContractorMap()
    {
        this.HasKey(t => t.ContractorId);

        this.Property(t => t.ContactName)
            .IsRequired()
            .HasMaxLength(200);

        this.Property(t => t.BusinessName)
            .IsRequired()
            .HasMaxLength(200);

        this.Property(t => t.PhoneNumber)
            .IsRequired()
            .HasMaxLength(15);

        ......

        this.Ignore(t => t.RequiredPropertiesFulfilled);
    }
}

Then, we need a custom model binder to make the model Required annotations behave conditionally for different permit types:

public class CustomModelBinder : DefaultModelBinder
{
    public override object BindModel
    (ControllerContext controllerContext, ModelBindingContext bindingContext)
    {
        var modelName = bindingContext.ModelName;
        Type modelType = bindingContext.ModelType;
        if (modelType == typeof(Models.Contractor))
        {
            var permitType = bindingContext.ModelName.Split('.')[0];
            //the mvc view is intentionally added with HtmlTemplatePrefix so that
            //bindingContext.ModelName looks like
            //Encroachment.DescriptionOfActivity.Contractors[0]

            if (permitType != null && (
                permitType.Equals("Encroachment",
                StringComparison.CurrentCultureIgnoreCase) ||
                permitType.Equals("Entrance",
                StringComparison.CurrentCultureIgnoreCase))
                )
            {
                var contractor = base.BindModel(controllerContext, bindingContext);
                if (bindingContext.ModelState.IsValid)
                {
                    //if user entered all the required info, just let it go.
                    (contractor as Models.Contractor).RequiredPropertiesFulfilled =
                                                                                               true;
                }
                else
                {
                    var contractorKeys = bindingContext.ModelState.Keys.Where
                    (x => x.Contains("Contractors"));
                    if (contractorKeys != null && contractorKeys.Count() > 0)
                    {
                        foreach (var k in contractorKeys)
                        {
                            bindingContext.ModelState[k].Errors.Clear();
                        }
                    }
                    //check if required fields have been entered or not.
                    //If not, mark it as not fulfilled, and remove it by
                    //function PurgeApplicationDataContractors
                                    //before application is submitted.
                    FlagDescriptionDataContractorFulfilledness
                    (contractor as Models.Contractor, controllerContext,
                                    bindingContext);
                }
                return contractor;
            }
        }
        return base.BindModel(controllerContext, bindingContext);
    }
}

In addition to other reasons, the very important one to choose a custom model binder to fix this issue is that it has a ModelBindingContext parameter passed in for us to easily get the required properties:

private IList<string>
GetBindingContextModelRequiredKeys(ModelBindingContext bindingContext)
{
    var requiredKeys = new List<string>();
    foreach (var k in bindingContext.PropertyMetadata.Keys)
    {
        if (bindingContext.PropertyMetadata[k].ModelType == typeof(short) ||
        bindingContext.PropertyMetadata[k].ModelType == typeof(long)) continue;
        if (bindingContext.PropertyMetadata[k].IsRequired)
        {
            requiredKeys.Add(k);
        }
    }

    return requiredKeys;
}

If a Contractor is entered with all valid information, then just save it. If requiredness is not satisfied, we need to do more things before going further.

We use the following function to determine if a Contractor model is fulfilled or not:

private void FlagDescriptionDataContractorFulfilledness
(Models.Contractor contractor,
ControllerContext controllerContext, ModelBindingContext bindingContext)
{
    if (contractor == null) return;
    var requiredKeys = GetBindingContextModelRequiredKeys(bindingContext);
    var nonFulfilledProperties = contractor.GetType().GetProperties().Where
    (x => x.GetValue(contractor) == null).Select(x => x.Name);
    var intersect = requiredKeys.Intersect(nonFulfilledProperties);

    contractor.RequiredPropertiesFulfilled = intersect == null;
}

And then, remove those contractors that were not entered with required information before saving to database so that no non-sense contractor's information will be populated into the Contractor table:

private void PurgeApplicationDataContractors(IEnumerable<Models.Contractor> contractors)
{
    if (contractors == null) return;
    for (var i = contractors.Count; i > 0; i--)
    {
        var c = contractors[i - 1];
        if (!c.RequiredPropertiesFulfilled) contractors.Remove(c);
    }
}

Don't forget to add the custom binders to the application model binders dictionary:

protected void Application_Start()
{
    ModelBinders.Binders.Add(typeof(Models.Contractor), new CustomModelBinder());
}

Now, we can see it work perfect with no or little impact on the existing codes. We don't have to remove the required properties (from the logic perspective, the requiredness makes sense). If in the future, Contractor is needed for other permit types and it requires the Contractor required properties, nothing has to be changed; if it does not mandate Contractor, the permit type can be easily added to the if-condition in the model binder, and it keeps working fine as expected.

With this fix, we will not have any more concerns in the foreseeable future.

One may argue that IValidatableObject can be inherited to make a custom validation. But I have not found a way to distinguish between permit types and get Contractor required properties from within the model class. Though a session variable can be used to carry the permit type name, it seems that there is not an easy way to get the required properties which are defined in the related mapping class.

Points of Interest

ASP.NET MVC ModelBinder is powerful. It carries so much information by ControllerContext and ModelBindingContext for developers to write their own logic into the codes.