Automated document downloading, with error handling

Question

I've automated downloading of CSVs from our vendors' websites. As expected with anything web-based, things can go... wrong, so I'm implementing some better error handling.

The code has been working reasonably stably for a year or so, but when failures have occurred in the download phase, the other phases have attempted to process anyway which is just silly. So now the intent is that archiving the freshly downloaded file and attempting to load it to the appropriate SQL table will only happen if the file is actually freshly downloaded.

So far,I've made some updates to use Err.Raise when a failure happens at any of the many steps along the way. These are now correctly bubbling up to the main code loop which is dropping into the ErrorHandler: as expected.

Welcome to the World of Spaghetti™:

What I don't like about how this is shaping up is that the error handler now has a bunch of Resume {label} code and it feels like I've regressed to 8th grade and am writing some Apple ][ Basic by just GoToing everywhere and even Resuming into the middle of a While loop.

I'm looking for some suggestions on how to refactor ProcessLoad to better support this method of handling things, or a better solution (that maybe doesn't involve raising errors) for tracking what the current status is and whether subsequent steps should process.

The Dilemma:

The key to my dilemma is that each file download/archive/SQL Load is an independent process. If one fails, there's no reason to abort the whole thing. The load is set up to handle the fact that I might be getting the same CSV file multiple times and to ignore (not load to SQL, though it will attempt to archive it - that's the next thing to fix) any duplicate data in it. I also have the option in my user interface to select all the files that need to be downloaded, or pick and choose just those that I want to download this particular time.

A couple of notes:

• This is existing, functional code that's in the midst of getting a rehab, so things aren't quite as consistent as one might like.
• Yes, I'm aware that I don't handle every error status yet. I don't like how this is shaping up, so I'm looking for input prior to writing more code that has to be changed.

---

I've declared WebDownloaderStatus as a class to track the current download status:

Option Compare Database
Option Explicit

'@Folder("WebDriver")

Public Enum DownloadStatuses
  FailureToCreateDriver = vbObjectError + &H1&
  FoundNoSatSurveyClinics = vbObjectError + &H2&
  SiteNotLoaded = vbObjectError + &H3&
  FailToNavigate = vbObjectError + &H4&
  NothingToDownload = vbObjectError + &H5&
  DownloadFailed = vbObjectError + &H6&
  LoginFailure = vbObjectError + &H7&
  OK = 0
End Enum

Private Type TDownloadStatus
  companyName As String
  downloadType As DownloadDataType
  downloadStatus As DownloadStatuses
  statusMessage As String
  okToContinue As Boolean
End Type

Private this As TDownloadStatus

Public Property Get okToContinue() As Boolean
  If downloadStatus >= 0 Then
    okToContinue = True
  Else
    okToContinue = False
  End If
End Property

Public Property Get companyName() As String
  companyName = this.companyName
End Property
Public Property Let companyName(ByVal value As String)
  this.companyName = value
End Property

Public Property Get downloadType() As DownloadDataType
  downloadType = this.downloadType
End Property
Public Property Let downloadType(ByVal value As DownloadDataType)
  this.downloadType = value
End Property

Public Property Get downloadStatus() As DownloadStatuses
  downloadStatus = this.downloadStatus
End Property
Public Property Let downloadStatus(ByVal value As DownloadStatuses)
  this.downloadStatus = value
End Property

Public Property Get statusMessage() As String
  statusMessage = this.statusMessage
End Property
Public Property Let statusMessage(ByVal value As String)
  this.statusMessage = value
End Property

And ProcessLoad is the main driving loop for downloading, archiving and pushing data onto the server:

Public Sub ProcessLoad(ByVal FileType As DownloadDataType, ByVal clinicList As ListBox)

  On Error GoTo ErrorHandler

  LogManager.Log InfoLevel, "----DataMoveCode.ProcessLoad----------------"

  Dim LoadExcelDataRC As Long
  Dim Index As Long
  Dim DataSources As Scripting.Dictionary
  Set DataSources = SetDataSources    'be sure to update this any time IWebDownloader.DownloadDataType changes!

  Dim downloaderStatus As WebDownloaderStatus
  Set downloaderStatus = New WebDownloaderStatus

  Dim Downloader As IWebDownloader
  Select Case FileType
    Case SnapSurvey
      LogManager.Log InfoLevel, "Loading SnapSurvey"
      Set Downloader = PrepareForSnapSurveyDownload(downloaderStatus)
    Case ApptPlus
      LogManager.Log InfoLevel, "Loading ApptPlus"
      Set Downloader = PrepareForApptPlusDownload
  End Select

  Index = 0
  While Index < clinicList.ListCount
    clinicList.Selected(Index) = True
    Select Case FileType
      Case SnapSurvey
        GetSnapSurveyFile Downloader, clinicList.Column(0, Index), clinicList.Column(1, Index), downloaderStatus
      Case ApptPlus
        GetApptPlusFile Downloader, clinicList.Column(0, Index), clinicList.Column(1, Index), downloaderStatus
    End Select

    Dim ArchiveData As ZipToolsData
    Dim Archiver As ZipTools
    ArchiveSourceDataByClinicID DataSources(FileType), clinicList.Column(0, Index), ArchiveData, Archiver

'todo: Phase 4: give better result reporting so I know if there was an error loading or just no data to be loaded.
    LoadExcelDataRC = LoadExcelData(DataSources(FileType), clinicList.Column(0, Index), clinicList.Column(1, Index))
NextClinic:
    If LoadExcelDataRC > 0 Then
      clinicList.RemoveItem (Index)
    Else
      clinicList.Selected(Index) = False
      Index = Index + 1
    End If
  Wend

CleanExit:
  If Not Downloader Is Nothing Then 'we could get here now without ever having properly set up the downloader class, so skip as necessary
    If Downloader.IsLoggedIn Then
      Downloader.LogoutOfWebPage
    End If
    Downloader.Driver.Quit
  End If

MassiveAbort:
  LogManager.Log InfoLevel, "----DataMoveCode.ProcessLoad Complete----------------"

  Exit Sub

ErrorHandler:
  With Err
    LogManager.Log ErrorLevel, Strings.Join(Array("Source: ", .Source, " Error Num: ", .Number, " Description: ", .Description), vbNullString)
    If .Number = FailureToCreateDriver Or .Number = LoginFailure Then
      Resume MassiveAbort
    ElseIf .Number = NothingToDownload Then
      Resume NextClinic
    End If
  End With

  Resume CleanExit

End Sub

It seems I may not have included quite enough code. Here is a path through all methods necessary to initially bring up the web page (prior to logging in) including all the Err.Raise to send the messages back. (No implication that there are faults with the current review posted - I just want to be sure.)

Public Function GetSnapSurveyFile(ByVal SSWD As IWebDownloader, ByVal clinicID As Integer, ByVal clinicName As String, ByRef webDLStatus As WebDownloaderStatus) As Integer

  On Error GoTo ErrorHandler
  LogManager.Log InfoLevel, "GetSnapSurveyFile: " & clinicName

  Dim SQLString As String
  SQLString = "SELECT ExcelFileName " & _
              "  FROM ExcelLoadConfig " & _
              " WHERE ClinicID = " & clinicID & _
              "   AND ExcelFileType = 'SatSurvey'"

  Dim RecSet As ADODB.Recordset
  Set RecSet = New ADODB.Recordset
  LogManager.Log TraceLevel, SQLString
  RecSet.Open Source:=SQLString, ActiveConnection:=CStr(TempVars!constring)
  While Not RecSet.EOF And Not RecSet.BOF
    Dim clinicLinkText As String
    clinicLinkText = LCase(RecSet.Fields(0))      'note: the links are case sensitive when finding them on the web site
    With SSWD
      .fileName = clinicLinkText & Tools.GetGeneralConfig("GetSnapSurveyFile", "FileExtension", "SatSurvey")
      .SiteValue(CStr(clinicLinkKey)) = clinicLinkText
      .DownloadDriver webDLStatus
    End With
    RecSet.MoveNext
  Wend

CleanExit:
  If RecSet.State <> adStateClosed Then
    RecSet.Close
  End If

  Exit Function

ErrorHandler:

  Resume CleanExit

End Function

Note that OP is aware of SQL injection and the dangers of building the query string this way vs a parameterized query. However the parameter being built into the string comes from the database itself and is passed in with no opportunity for an end user to make a malicious change to it. There are only two of us with access to the database and neither of us wants to put in the effort to clean up after a malicious attack. Should this be changed? Yes. Probably won't happen today.

.DownloadDriver is implemented here:

Private Sub IWebDownloader_DownloadDriver(ByRef webDLStatus As WebDownloaderStatus)

  On Error GoTo ErrorHandler
  LogManager.Log TraceLevel, "SnapSurveyWebDownloader.IWebDownloader_DownloadDriver"
  If Not this.IsPageLoaded Then
    IWebDownloader_LoadWebPage webDLStatus
  End If

  If this.IsPageLoaded Then
    If Not this.IsLoggedIn Then
      IWebDownloader_LoginToWebPage webDLStatus
    End If
    If this.IsLoggedIn Then
      If IWebDownloader_NavigateToDownloadLocation(webDLStatus) Then
        IWebDownloader_DownloadFile webDLStatus
      End If
    End If
  End If

CleanExit:
  Exit Sub

ErrorHandler:
  With webDLStatus
    Err.Raise .downloadStatus, Err.Source, .statusMessage
  End With
  Resume CleanExit

End Sub

And the page itself is loaded here:

Private Sub IWebDownloader_LoadWebPage(ByRef webDLStatus As WebDownloaderStatus)

'' NOTE: If downloads fail with err.Number = 13, it's likely that a new ChromeDriver is needed
''  Check here for an updated version:
''     https://sites.google.com/a/chromium.org/chromedriver/downloads
''  Put the chromedriver.exe file here:
''     C:\Users\<user>\AppData\Local\SeleniumBasic
''  note that from <user> down are hidden directories and "Find" won't search them.

  LogManager.Log DebugLevel, "SnapSurveyWebDownloader.IWebDownloader_LoadWebPage"
  On Error GoTo ErrorHandler

  Dim startTime As Single
  startTime = Timer
  this.IsPageLoaded = this.Driver.Get(this.URL, timeout:=60000)   'give a generous 30 seconds for the page to load
  Dim EndTime As Single
  EndTime = Timer
  this.Driver.Timeouts.ImplicitWait = CLng((EndTime - startTime) * 2000)  'dynamically adjust the timeout to the current page load speed
  LogManager.Log InfoLevel, "Driver timeout set to: " & CStr(this.Driver.Timeouts.ImplicitWait)

CleanExit:
  Exit Sub

ErrorHandler:
  this.IsPageLoaded = False
  webDLStatus.downloadStatus = SiteNotLoaded
  With Err
    LogManager.Log ErrorLevel, "SnapSurvey page failed to load. Number: " & .Number & " Description: " & .Description
    If .Number = 13 Then
      LogManager.Log ErrorLevel, "  >>>> it's likely that there's a new version of ChromeDriver needed"
    End If
    webDLStatus.statusMessage = "SnapSurvey page failed to load. Number: " & .Number & " Description: " & .Description
    .Raise webDLStatus.downloadStatus, "SnapSurveyWebDownloader.IWebDownloader_LoadWebPage", webDLStatus.statusMessage
  End With
  Resume CleanExit

End Sub

OP notes that this is a good example of why code comments are so dangerous - the code gives a generous 60 seconds before timing out, however the comment says only 30 seconds. I'll just remove that comment and escort the OP to the door...

Answer 1

I am going to add another answer to address the additional code, rather than review my original answer. The additional code did confirm my assumptions but the remainder of the advice is still valid.

While I can comment on some of the coding aspects, I don't have the actual experience in downloading to anticipate all errors or problems. So, I may leave breadcrumbs to some solutions rather than anything specific.

IWebDownloader_LoadWebPage

This is the lowest level, so any issues that bubble up from here are going to get harder to fix at the higher levels.

If I assume that On Error is a valid approach here, we can still modify the error handler in the first instance:

ErrorHandler:
  this.IsPageLoaded = False
  webDLStatus.downloadStatus = SiteNotLoaded
  With Err
    LogManager.Log ErrorLevel, "SnapSurvey page failed to load. Number: " & .Number & " Description: " & .Description
    If .Number = 13 Then
      LogManager.Log ErrorLevel, "  >>>> it's likely that there's a new version of ChromeDriver needed"
    End If
    webDLStatus.statusMessage = "SnapSurvey page failed to load. Number: " & .Number & " Description: " & .Description
    .Raise webDLStatus.downloadStatus, "SnapSurveyWebDownloader.IWebDownloader_LoadWebPage", webDLStatus.statusMessage
  End With
  Resume CleanExit

becomes

ErrorHandler:
  this.IsPageLoaded = False
  webDLStatus.downloadStatus = SiteNotLoaded
  With Err
    LogManager.Log ErrorLevel, "SnapSurvey page failed to load. Number: " & .Number & " Description: " & .Description
    If .Number = 13 Then
      LogManager.Log ErrorLevel, "  >>>> it's likely that there's a new version of ChromeDriver needed"
    End If
    webDLStatus.statusMessage = "SnapSurvey page failed to load. Number: " & .Number & " Description: " & .Description
  End With
  Resume CleanExit

You will notice that the only thing I did was remove the Raise. This means that your routine will now have an orderly exit rather than an abortive one.

You set this.IsPageLoaded in the code, but do not check it before running to the next part of the code. You can avoid one error call by simply checking if this.IsPageLoaded is False. What are the other ways the .Get can error out?

IWebDownloader_DownloadDriver

Similar to the previous section:

ErrorHandler:
  With webDLStatus
    Err.Raise .downloadStatus, Err.Source, .statusMessage
  End With
  Resume CleanExit

becomes

ErrorHandler:
  Resume CleanExit

Which means, that you could optimise even more:

Sub IWebDownloader_DownloadDriver(ByRef webDLStatus As WebDownloaderStatus)
  On Error GoTo CleanExit ' was ErrorHandler
[...]
CleanExit:
End Sub

This is a real soft fail, but you don't do anything except raise an error (an abortive rather than a controlled exit). Because of this, I assume that you are merely passing through any error from LoadWebPage.

Now you can simply check for errors in webDLStatus. Actually, you don't have to check for anything, because webDLStatus is set by the lower routines. So your new code could be:

Sub IWebDownloader_DownloadDriver(ByRef webDLStatus As WebDownloaderStatus)
  LogManager.Log TraceLevel, "SnapSurveyWebDownloader.IWebDownloader_DownloadDriver"
  If Not this.IsPageLoaded Then
    IWebDownloader_LoadWebPage webDLStatus
  End If

  If this.IsPageLoaded Then
    If Not this.IsLoggedIn Then
      IWebDownloader_LoginToWebPage webDLStatus
    End If
    If this.IsLoggedIn Then
      If IWebDownloader_NavigateToDownloadLocation(webDLStatus) Then
        IWebDownloader_DownloadFile webDLStatus
      End If
    End If
  End If
End Sub

This means this routine is simply a flow logic control point.

GetSnapSurveyFile

At first glance, it seems your error handler is simply taking the passed-through error and creating a soft fail. A big issue here is that once an error is raised, you forcibly exit the loop and stop processing all the other records.

Now that we have stopped raising errors in the lower-level you can handle a download error (as reported through the download status) in a more controlled way. Perhaps If webDLStatus.DownloadStatus > 0 Then Move.EOF if you really want to abort the loop.

You also don't check for errors in the database access functions. What happens if it can't find clinicLinkKey (and where is this set anyway)? These are the sort of errors that can be anticipated. I suggest that you actively trap these errors and update webDLStatus to reflect the outcome. Of course, this is in a loop and you only have one status report going out.

As per the previous comment, because you don't do anything in your error handler except go for a clean exit, you can (if you need to keep On Error) just go to CleanExit in the first place.

Answer 2

Some basics

Trivial and not really going to affect your current code, but consider setting DownloadStatuses as flaggable errors (instead of H1,H2,H3, ... use H1, H2, H4, ...). That way you can flag multiple errors at the one time.

VBA has limitations on user-defined types (e.g. Private Type TDownloadStatus), particularly how they can be used in Collections. I always use Classes now - just as easy to code and modern machines don't sweat the extra load. Perhaps this would be a considerations if you were dealing with big data.

• As an added bonus, you can make your enum part of the class, not the main code, thus encapsulating more code.
• Having said the above, I do see that you have actually used WebDownloaderStatus class to encapsulate the UDT (for reflection?) which addresses the points in my main paragraph. Why not just run with the Class and have the elements of the Type as private members of your class? You have effectively added an extra layer of code - but for what real value?

Be really critical on yourself and review how logic code is written. Classic example that I see often:

Public Property Get okToContinue() As Boolean
  If downloadStatus >= 0 Then
    okToContinue = True
  Else
    okToContinue = False
  End If
End Property

should be:

Public Property Get okToContinue() As Boolean
    okToContinue = (downloadStatus >= 0)
End Property

Error handling (basic philosophy): If you can foresee errors, you should always explicitly handle them, rather than leaving it to exceptions (on in VBA's case, On Error/Err.Raise).

Is this complete? Set DataSources = SetDataSources, but I don't see SetDataSources defined anywhere. I assume this is a global object?

Double check your properties in the WebDownloaderStatus class - you have not defined DownLoadDataType in the code in the OP, so should this be a Let or a Set?

Dealing with the basics helps cut some of the Spaghetti. This makes it clearer to address your main question - the program flow.

Error Handling and flow

Your Select Case FileType do not include a Case Else statement, but you also do not validate FileType on entry to the function. This is one area where an error can occur, because it means a possible path for Downloader = Nothing, which your other code does not directly address.

You can refactor your working subroutines (I assume they are Sub because you don't use any return values) to become functions, and they can return an error code. Even better, they could return a WebDownloaderStatus object, which holds all the relevant information to help your main code flow.

And then it hits me - the obvious. I assume the Get~File subroutines fill downloaderStatus because I assume downloaderStatus is passed ByRef (it is an object after all). (See comment above about returning it from a function instead) - but downloaderStatus is not used to check results.

Rather than throw errors in the subordinate routines, you can simply pass the errors back in the downloaderStatus object and let your main code decide how to handle it. I reckon that, with proper use of the downloaderStatus object you could get rid of any need for On Error.

Should Get~File return a download status, or should it return the Downloader? Could be 6 of one, half dozen of the other - but this minor refactoring will help with a logical program flow even if it does not appear to make any significant change to the outcome or efficiency.

Important next step

Having gone through my thoughts above, your Spaghetti is not the problem itself, it is a symptom of other problems in your subordinate code.

Review your subordinate code and make it safe. Create these routines in a way that any errors are caught (or averted in the first place) and reported through the status object. After all, this is why you developed WebDownloaderStatus class.

Once you have done that, you will find that your ProcessLoad will become cleaner and maintainable.