Unit-testing the importing of data into a database

Question

I have a functionality that imports data into a database, based on an Excel workbook and some meta data (both user-supplied). The functionality implements interface IFunctionality which essentially specifies an AuthId string property (used for fetching authorized AD groups for a functionality) as well as a CanExecute and Execute method.

The catch is that a Functionality must be COM-visible to be called via a legacy VB6 application and I want the COM interface as simple as it could be, and to me this means a IFunctionality implementation only has a parameterless constructor.

Below is the almost complete code (stripped a couple DAL calls and anonymized company-specifics) for one of those functionalities. I'm not going to supply DAL and Presentation layer code here, since my concerns are mainly around the Business Logic layer - but for the record I have DAL implemented with Linq to SQL and Presentation taken care of with WPF (which I just started learning).

[ComVisible(true)]
[ClassInterface(ClassInterfaceType.None)]
[ComDefaultInterface(typeof(IFunctionality))]
public class ImportFunctionality : IFunctionality
{
    private ImportWindow _window; // a WPF view

    public ImportFunctionality()
    { }

    public bool CanExecute()
    {
        return CurrentUser.IsAuthorized(AuthId);
    }

    public string AuthId
    {
        get { return GetType().ToString(); }
    }

    /// <summary>
    /// Prompts for an Excel workbook filename and creates pricing tables from workbook data.
    /// </summary>
    public void Execute()
    {
        try
        {
            if (!CanExecute()) throw new NotAuthorizedException(resx.Functionality_Execute_NotAuthorised);

            var xlData = GetExcelSourceData();
            if (xlData == null) return;

            var viewModel = GetImportSettings(xlData);
            if (viewModel == null) return;

            if (!GetUserConfirmation(viewModel)) return;
            ImportGridContent(viewModel);
        }
        catch (NotAuthorizedException exception)
        {
            MsgBox.Failure(resx.NotAuthorized, exception.Message);
        }
        catch (Exception exception)
        {
            MsgBox.Error(exception);
        }
    }

    private DataTable GetExcelSourceData()
    {
        var file = FileDialogHelper.GetOpenFileName(resx.FileDialogFilter_Excel97_2003);
        if (file == string.Empty) return null;

        return Excel8OleDbHelper.ImportExcelFile(file);
    }

    private ImportViewModel GetImportSettings(DataTable xlData)
    {
        var viewModel = new ImportViewModel(xlData);

        // todo: add metadata to viewModel constructor...
        using (var data = new ImportModel(Settings.Default.DefaultDb))
        {
            // Wrap Linq2SQL objects into UI-visible instances:
            var meta = data.LoadImportMetadata()
                            .Select(e => new ImportMetaData
                                            {
                                                // named property setters
                                            }).ToList();

            // load metadata into ViewModel:
            viewModel.SetMetadata(new ObservableCollection<ImportMetaData>(meta));
        }

        _window = new ImportWindow(viewModel);

        viewModel.WindowClosing += viewModel_WindowClosing;
        _window.ShowDialog();

        var result = viewModel.DialogResult;
        return (result == DialogResult.Ok)
                        ? viewModel
                        : null;
    }

    private bool GetUserConfirmation(ImportViewModel viewModel)
    {
        var result = MsgBox.Prompt(resx.ConfirmationRequired, resx.ImportFunctionality_ConfirmProceed);
        return (result == DialogResult.Yes);
    }

    void viewModel_WindowClosing(object sender, EventArgs e)
    {
        _window.Close();
    }

    /// <summary>
    /// Returns a <see cref="IImportData"/> implementation that corresponds to database name specified by supplied <see cref="viewModel"/>.
    /// </summary>
    /// <param name="viewModel"></param>
    /// <returns></returns>
    public IImportData GetTargetConnection(ImportViewModel viewModel)
    {
        var connectionString = Settings.Default[viewModel.SelectedDatabase].ToString();
        return viewModel.SelectedCompany == CompaniesEnum.Company1.ToString()
                                            ? new ImportDataCompany1(connectionString)
                                            : viewModel.SelectedCompany == CompaniesEnum.Company2.ToString()
                                                ? new ImportDataCompany2(connectionString)
                                                : new ImportData(connectionString)
                                                // this is begging to be refactored
                                                ;
    }

    private void ImportGridContent(ImportViewModel viewModel)
    {
        using (var data = GetTargetConnection(viewModel))
        {
            var args = new AsyncImportEventArgs(viewModel, data);
            var iterations = viewModel.GridSource.Rows.Count * viewModel.SelectedMetadata.Count();

            data.BeginTransaction();
            MsgBox.ProgressStatus<AsyncImportEventArgs>(resx.ImportFunctionality_Title, resx.ImportFunctionality_PleaseWait, DoAsyncImport, args, CancelAsyncImport, iterations);

            if (args.Success)
                data.CommitTransaction();
            else
                data.RollbackTransaction();
        }
    }

    protected void DoAsyncImport(AsyncProgressArgs<AsyncImportEventArgs> e)
    {
        // Lengthy operation involving DAL calls and periodically updating the view
        // ...

        // Update the view:
        e.UpdateProgress(resx.ProgressCompleted, maxProgressValue);
        e.UpdateMessage(resx.ImportFunctionality_TitleCompleted);

    }

    private void CancelAsyncImport(AsyncImportEventArgs e)
    {
        e.Success = false;
        e.Data.RollbackTransaction();
        MsgBox.Info(resx.ImportFunctionality_Cancelled_Title, resx.ImportFunctionality_Cancelled_Msg);
    }

    #region nested types

    /// <summary>
    /// Encapsulates parameters passed to async method for importing pricing tables.
    /// </summary>
    public class AsyncImportEventArgs : EventArgs
    {
        public ImportViewModel ViewModel { get; private set; }
        public IImportData Data { get; private set; }
        public bool Success { get; set; }

        public AsyncImportEventArgs(ImportViewModel dialog, IImportData data)
        {
            ViewModel = dialog;
            Data = data;
        }
    }

    #endregion
}

The questions I have are the following:

1. If I wanted to write unit tests for this functionality, what would I need to do in order to make the code unit-testable? I have no issues making methods public, as they wouldn't be exposed to COM anyway (because only IFunctionality is accessible through COM). Or perhaps I should make them protected, and derive from this class so I could test it (the test wrapper class would expose public methods that call into the protected ones; tests would call those exposed public methods and it could even have dependencies injected into its constructor - am I thinking the right way about this?).
2. If I wanted to write unit tests for this functionality, I couldn't write a test for the Execute method, right? I'd have to test the more specialized code and figure out how to mock the DAL calls? And for CanExecute, I'd need a way to get rid of the CurrentUser dependency and inject some dummy provider that doesn't hit the database nor Active Directory, right?
3. Is this code "clean"? (easy to read, easy to understand, easy to maintain) I guess the sole fact that I'm asking this, answers for itself... How could it be made cleaner then?
4. Are static classes and methods hindering anything? (like CurrentUser.IsAuthorized, FileDialogHelper.GetOpenFileName, and Excel8OleDbHelper.ImportExcelFile) - MsgBox lives in the Presentation layer and it aims at replacing the ugly default message box, I think the static class is warranted in that particular case.
5. Sharp eyes will have noticed I initiate a db transaction, do some work while showing progress (the process can take anywhere between a minute and 2 hours, depending on inputs) and then committing or rolling back the transaction only after the ProgressMsgBox has closed. This leaves the affected tables unnecessarily locked, longer than they need to be. How can I work around this? (hmm now that I'm thinking about it, I could have the ViewModel raise an event when progress completes and handle it in this class to commit or rollback as needed... but I'd have to make data a property of the class... makes sense? Then I guess the functionality class would need to implement IDisposable and rely on the client code to properly dispose its resources, no?)

Answer 1

The points you've specified in your question actually show that you (almost) see issues in your code yourselves, so thumbs up for that. I'll only expand on your points plus add a couple:

• your class is doing too much. You've noted that it has to be accessible from COM, but it doesn't mean that this exact class (business logic, BL) should be accessible from COM. What you should do is create a separate class/interface for accessing BL from COM (in terms of patterns it is called façade), and that class should wire up and issue calls to your BL, so that BL is not designed with restrictions/requirements imposed by COM;
• you should rewrite all your static classes/helpers like CurrentUser, FileDialogHelper and Excel8OleDbHelper to be instance classes implementing certain interfaces, and use them from main "functionality" class via interfaces only;
• do not use UI in business logic (MsgBox.Failure, _window.ShowDialog, MsgBox.Prompt, etc). The code that uses business logic should be responsible for providing data needed, and showing errors/messages to customer, and business logic should not reference any class from UI namespaces;
• you should not expose private methods as public/protected in order to unit-test them. Write as many unit tests (that use only public interface of your class) as you need to check all use cases of your class. Requirement "all use cases" ensures that they will verify all private methods as well;
• transactions should not depend on UI. If you need to expose progress of long-term operation - expose an event for that, but don't block on it. If you need a confirmation for final import - just expose another method that UI can call when user confirmed the import;
• extract DAL-related code into separate class with its own interface;
• wire up all the classes either manually in the COM-exposed class or using one of the IoC frameworks;
• test each class separately, mocking all external dependencies (that you should reference via interface) with one of the mocking frameworks (Moq, Rhino Mocks, etc)
• you haven't shown import code, but I would suggest looking into bulk upload methods, e.g. like suggested here.