From 9e141817798e3d59ac795255dd2f99f6b6470b86 Mon Sep 17 00:00:00 2001 From: Andy Grunwald Date: Fri, 13 Sep 2024 13:21:01 +0200 Subject: [PATCH 1/4] sync cmd: Rework view layer to show the single steps that are processed --- cmd/sync.go | 334 +++++++++++++++++++++++++---------------- go.mod | 1 + go.sum | 2 + lib/browser/browser.go | 8 +- lib/browser/oauth2.go | 8 +- lib/utils/utils.go | 14 +- 6 files changed, 220 insertions(+), 147 deletions(-) diff --git a/cmd/sync.go b/cmd/sync.go index 424e880..e7cf252 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -18,6 +18,7 @@ import ( "github.com/charmbracelet/bubbles/spinner" tea "github.com/charmbracelet/bubbletea" "github.com/charmbracelet/lipgloss" + "github.com/pkg/errors" "github.com/spf13/cobra" "github.com/spf13/viper" ) @@ -138,85 +139,95 @@ func RunSyncCommand(cmd *cobra.Command, cmdArgs []string) { } func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksum, localOICDBSchemaChecksum string, vaultProvider *vault.Provider1Password, documentArchive *archive.DocumentArchive, recipeParser *parser.RecipeParser, buchhalterAPIClient *repository.BuchhalterAPIClient) { - p.Send(viewMsgStatusUpdate{ - title: "Build archive index", - hasError: false, - }) + p.Send(utils.ViewStatusUpdateMsg{Message: "Building archive index"}) logger.Info("Building document archive index ...") err := documentArchive.BuildArchiveIndex() if err != nil { logger.Error("Error building document archive index", "error", err) - p.Send(viewMsgStatusUpdate{ - title: "Building document archive index", - hasError: true, - shouldQuit: false, - }) + p.Send(utils.ViewStatusUpdateMsg{Err: fmt.Errorf("error building document archive index: %w", err)}) } + p.Send(utils.ViewStatusUpdateMsg{ + Message: "Building archive index", + Completed: true, + }) // Check for OICDB schema updates - p.Send(viewMsgStatusUpdate{ - title: "Checking for OICDB schema updates ...", - hasError: false, - }) + p.Send(utils.ViewStatusUpdateMsg{Message: "Checking for OICDB schema updates"}) logger.Info("Checking for OICDB schema updates ...", "local_checksum", localOICDBSchemaChecksum) err = buchhalterAPIClient.UpdateOpenInvoiceCollectorDBSchemaIfAvailable(localOICDBSchemaChecksum) if err != nil { logger.Error("Error checking for OICDB schema updates", "error", err) - p.Send(viewMsgStatusUpdate{ - title: "Checking for OICDB schema updates", - hasError: true, - shouldQuit: false, - }) + p.Send(utils.ViewStatusUpdateMsg{Err: fmt.Errorf("error checking for OICDB schema updates: %w", err)}) } + p.Send(utils.ViewStatusUpdateMsg{ + Message: "Checking for OICDB schema updates", + Completed: true, + }) developmentMode := viper.GetBool("dev") if !developmentMode { // Check for OICDB repository updates - p.Send(viewMsgStatusUpdate{ - title: "Checking for OICDB repository updates ...", - hasError: false, - }) + p.Send(utils.ViewStatusUpdateMsg{Message: "Checking for OICDB repository updates"}) logger.Info("Checking for OICDB repository updates ...", "local_checksum", localOICDBChecksum) err = buchhalterAPIClient.UpdateOpenInvoiceCollectorDBIfAvailable(localOICDBChecksum) if err != nil { logger.Error("Error checking for OICDB repository updates", "error", err) - p.Send(viewMsgStatusUpdate{ - title: "Checking for OICDB repository updates", - hasError: true, - shouldQuit: false, - }) + p.Send(utils.ViewStatusUpdateMsg{Err: fmt.Errorf("error for OICDB repository updates: %w", err)}) } + p.Send(utils.ViewStatusUpdateMsg{ + Message: "Checking for OICDB repository updates", + Completed: true, + }) } + statusUpdateMessage := "Loading recipes and credentials for suppliers" + if len(supplier) > 0 { + statusUpdateMessage = fmt.Sprintf("Loading recipe and credentials for supplier `%s`", supplier) + } + p.Send(utils.ViewStatusUpdateMsg{ + Message: statusUpdateMessage, + }) recipesToExecute, err := prepareRecipes(logger, supplier, vaultProvider, recipeParser) - // No credentials found for supplier/recipes - if len(recipesToExecute) == 0 || err != nil { - logger.Error("No recipes found for suppliers", "supplier", supplier, "error", err) - p.Send(viewMsgStatusUpdate{ - title: "No recipes found for suppliers", - hasError: true, - shouldQuit: true, + if err != nil { + // No error logging needed. This is done in `prepareRecipes` + p.Send(utils.ViewStatusUpdateMsg{ + Err: fmt.Errorf("error loading recipes: %w", err), + ShouldQuit: true, + }) + return + } + + // No pair of credentials found for supplier/recipes + if len(recipesToExecute) == 0 { + loggingErrorMessage := "No recipes found for suppliers" + if len(supplier) > 0 { + loggingErrorMessage = fmt.Sprintf("No recipe found for supplier `%s`", supplier) + } + logger.Error(loggingErrorMessage, "supplier", supplier, "error", err) + p.Send(utils.ViewStatusUpdateMsg{ + Err: errors.New(loggingErrorMessage), + ShouldQuit: true, }) return } + p.Send(utils.ViewStatusUpdateMsg{ + Message: statusUpdateMessage, + Completed: true, + }) - var t string recipeCount := len(recipesToExecute) if recipeCount == 1 { - t = fmt.Sprintf("Running one recipe for supplier %s ...", recipesToExecute[0].recipe.Supplier) + statusUpdateMessage = fmt.Sprintf("Running one recipe for supplier `%s` ...", recipesToExecute[0].recipe.Supplier) logger.Info("Running one recipe ...", "supplier", recipesToExecute[0].recipe.Supplier) } else { - t = fmt.Sprintf("Running recipes for %d suppliers ...", recipeCount) + statusUpdateMessage = fmt.Sprintf("Running recipes for %d suppliers ...", recipeCount) logger.Info("Running recipes for multiple suppliers ...", "num_suppliers", recipeCount) } - p.Send(viewMsgStatusUpdate{ - title: t, - hasError: false, - }) - p.Send(viewMsgProgressUpdate{Percent: 0.001}) + p.Send(utils.ViewStatusUpdateMsg{Message: statusUpdateMessage}) + p.Send(utils.ViewProgressUpdateMsg{Percent: 0.001}) buchhalterDocumentsDirectory := viper.GetString("buchhalter_documents_directory") buchhalterConfigDirectory := viper.GetString("buchhalter_config_directory") @@ -232,34 +243,43 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu for i := range recipesToExecute { startTime := time.Now() stepCountInCurrentRecipe = len(recipesToExecute[i].recipe.Steps) - p.Send(viewMsgStatusUpdate{ - title: "Downloading invoices from " + recipesToExecute[i].recipe.Supplier + ":", - hasError: false, - }) // Load username, password, totp from vault + p.Send(utils.ViewStatusUpdateMsg{ + Message: fmt.Sprintf("Requesting credentials from vault for supplier `%s`", recipesToExecute[i].recipe.Supplier), + }) logger.Info("Requesting credentials from vault", "supplier", recipesToExecute[i].recipe.Supplier) recipeCredentials, err := vaultProvider.GetCredentialsByItemId(recipesToExecute[i].vaultItemId) if err != nil { - // TODO Implement better error handling logger.Error(vaultProvider.GetHumanReadableErrorMessage(err)) - fmt.Println(vaultProvider.GetHumanReadableErrorMessage(err)) + // TODO Let `GetHumanReadableErrorMessage` return a proper error + p.Send(utils.ViewStatusUpdateMsg{ + Err: errors.New(vaultProvider.GetHumanReadableErrorMessage(err)), + }) continue } + p.Send(utils.ViewStatusUpdateMsg{ + Message: fmt.Sprintf("Requested credentials from vault for supplier `%s`", recipesToExecute[i].recipe.Supplier), + Completed: true, + }) + p.Send(utils.ViewStatusUpdateMsg{Message: fmt.Sprintf("Downloading invoices from `%s`", recipesToExecute[i].recipe.Supplier)}) logger.Info("Downloading invoices ...", "supplier", recipesToExecute[i].recipe.Supplier, "supplier_type", recipesToExecute[i].recipe.Type) switch recipesToExecute[i].recipe.Type { case "browser": browserDriver, err := browser.NewBrowserDriver(logger, recipeCredentials, buchhalterDocumentsDirectory, documentArchive, buchhalterMaxDownloadFilesPerReceipt) if err != nil { - // TODO Implement better error handling - fmt.Println(err) + p.Send(utils.ViewStatusUpdateMsg{ + Err: fmt.Errorf("error downloading invoices from %s: %w", recipesToExecute[i].recipe.Supplier, err), + }) + // TODO Should we continue here or abort? } // Send the browser context to the view layer // This is needed in case of an external abort signal (e.g. CTRL+C). p.Send(updateBrowserContext{ctx: browserDriver.GetContext()}) + // TODO Catch error here recipeResult = browserDriver.RunRecipe(p, totalStepCount, stepCountInCurrentRecipe, baseCountStep, recipesToExecute[i].recipe) if ChromeVersion == "" { ChromeVersion = browserDriver.ChromeVersion @@ -272,14 +292,17 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu case "client": clientDriver, err := browser.NewClientAuthBrowserDriver(logger, recipeCredentials, buchhalterConfigDirectory, buchhalterDocumentsDirectory, documentArchive) if err != nil { - // TODO Implement better error handling - fmt.Println(err) + p.Send(utils.ViewStatusUpdateMsg{ + Err: fmt.Errorf("error downloading invoices from %s: %w", recipesToExecute[i].recipe.Supplier, err), + }) + // TODO Should we continue here or abort? } // Send the browser context to the view layer // This is needed in case of an external abort signal (e.g. CTRL+C). p.Send(updateBrowserContext{ctx: clientDriver.GetContext()}) + // TODO Catch error here recipeResult = clientDriver.RunRecipe(p, totalStepCount, stepCountInCurrentRecipe, baseCountStep, recipesToExecute[i].recipe) if ChromeVersion == "" { ChromeVersion = clientDriver.ChromeVersion @@ -299,6 +322,7 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu NewFilesCount: recipeResult.NewFilesCount, } RunData = append(RunData, rdx) + // TODO Check for recipeResult.LastErrorMessage p.Send(viewMsgRecipeDownloadResultMsg{ duration: time.Since(startTime), @@ -307,30 +331,37 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu errorMessage: recipeResult.LastErrorMessage, }) logger.Info("Downloading invoices ... completed", "supplier", recipesToExecute[i].recipe.Supplier, "supplier_type", recipesToExecute[i].recipe.Type, "duration", time.Since(startTime), "new_files", recipeResult.NewFilesCount) + invoiceLabel := "invoices" + if recipeResult.NewFilesCount == 1 { + invoiceLabel = "invoice" + } + p.Send(utils.ViewStatusUpdateMsg{ + Message: fmt.Sprintf("Downloaded %d %s from `%s`", recipeResult.NewFilesCount, invoiceLabel, recipesToExecute[i].recipe.Supplier), + Completed: true, + }) baseCountStep += stepCountInCurrentRecipe } // If we have a premium user run, upload the documents to the buchhalter API logger.Info("Checking if we have a premium subscription to Buchhalter API ...") + p.Send(utils.ViewStatusUpdateMsg{ + Message: "Checking if a premium subscription to Buchhalter API exists", + }) user, err := buchhalterAPIClient.GetAuthenticatedUser() if err != nil { logger.Error("Error retrieving authenticated user", "error", err) - p.Send(viewMsgStatusUpdate{ - title: "Retrieving authenticated user", - hasError: true, - shouldQuit: false, - }) + p.Send(utils.ViewStatusUpdateMsg{Err: fmt.Errorf("error retrieving a premium subscription to Buchhalter API: %w", err)}) } if user != nil && len(user.User.ID) > 0 { - uiDocumentUploadMessage := "Uploading documents to Buchhalter API ..." + statusUpdateMessage = "Uploading documents to Buchhalter API" if len(supplier) > 0 { - uiDocumentUploadMessage = fmt.Sprintf("Uploading documents of supplier %s to Buchhalter API ...", supplier) + statusUpdateMessage = fmt.Sprintf("Uploading documents of supplier `%s` to Buchhalter API", supplier) } - p.Send(viewMsgStatusUpdate{ - title: uiDocumentUploadMessage, - hasError: false, - }) + p.Send(utils.ViewStatusUpdateMsg{Message: statusUpdateMessage}) + + countUploadedFiles := 0 + countSkippedExistFiles := 0 fileIndex := documentArchive.GetFileIndex() for fileChecksum, fileInfo := range fileIndex { // If the user is only working on a specific supplier, skip the upload of documents for other suppliers @@ -349,6 +380,7 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu // If the file exists already, skip it if result { logger.Info("Uploading document to Buchhalter API ... exists already", "file", fileInfo.Path, "checksum", fileChecksum) + countSkippedExistFiles++ continue } logger.Info("Uploading document to Buchhalter API ... does not exist already", "file", fileInfo.Path, "checksum", fileChecksum) @@ -359,28 +391,50 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu logger.Error("Error uploading document to Buchhalter API", "file", fileInfo.Path, "supplier", fileInfo.Supplier, "error", err) continue } + countUploadedFiles++ + } + documentsLabel := "documents" + if countUploadedFiles == 1 { + documentsLabel = "document" + } + statusUpdateMessage = fmt.Sprintf("Uploaded %d %s to Buchhalter API (%d skipped, because they already exist)", countUploadedFiles, documentsLabel, countSkippedExistFiles) + if len(supplier) > 0 { + statusUpdateMessage = fmt.Sprintf("Uploaded %d %s of supplier `%s` to Buchhalter API (%d skipped, because they already exist)", countUploadedFiles, documentsLabel, supplier, countSkippedExistFiles) } + p.Send(utils.ViewStatusUpdateMsg{ + Message: statusUpdateMessage, + Completed: true, + }) } else { logger.Info("Skipping document upload to Buchhalter API due to missing premium subscription") + p.Send(utils.ViewStatusUpdateMsg{ + Message: "Skipping document upload to Buchhalter API due to missing premium subscription", + Completed: true, + }) } - + // TODO Move to own function/bubbletea cmd alwaysSendMetrics := viper.GetBool("buchhalter_always_send_metrics") if !developmentMode && alwaysSendMetrics { logger.Info("Sending usage metrics to Buchhalter API", "always_send_metrics", alwaysSendMetrics, "development_mode", developmentMode) + p.Send(utils.ViewStatusUpdateMsg{Message: "Sending usage metrics to Buchhalter API"}) err = buchhalterAPIClient.SendMetrics(RunData, cliVersion, ChromeVersion, vaultProvider.Version, recipeParser.OicdbVersion) if err != nil { logger.Error("Error sending usage metrics to Buchhalter API", "error", err) - p.Send(viewMsgStatusUpdate{ - title: "Sending usage metrics to Buchhalter API", - hasError: true, - shouldQuit: false, + p.Send(utils.ViewStatusUpdateMsg{ + Err: fmt.Errorf("error sending usage metrics to Buchhalter API: %w", err), + ShouldQuit: true, }) + return } - p.Send(viewMsgQuit{}) + p.Send(utils.ViewStatusUpdateMsg{ + Message: "Sending usage metrics to Buchhalter API", + Completed: true, + ShouldQuit: true, + }) } else if developmentMode { - p.Send(viewMsgQuit{}) + p.Send(viewQuitMsg{}) } else { p.Send(viewMsgModeUpdate{ @@ -391,6 +445,8 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu } } +// TODO Distinguish between "no recipes found" and "no credentials found" +// TODO Rename function to something more meaningful func prepareRecipes(logger *slog.Logger, supplier string, vaultProvider *vault.Provider1Password, recipeParser *parser.RecipeParser) ([]recipeToExecute, error) { var r []recipeToExecute @@ -474,12 +530,17 @@ var ( type viewModel struct { mode string - // Direct output on the screen + // UI + actionsCompleted []string + actionInProgress string + actionError string + actionDetails string + spinner spinner.Model + currentAction string details string showProgress bool progress progress.Model - spinner spinner.Model results []viewMsgRecipeDownloadResultMsg quitting bool hasError bool @@ -500,8 +561,8 @@ type updateBrowserContext struct { ctx context.Context } -// viewMsgQuit initiates the shutdown sequence for the bubbletea application. -type viewMsgQuit struct{} +// viewQuitMsg initiates the shutdown sequence for the bubbletea application. +type viewQuitMsg struct{} // viewMsgRecipeDownloadResultMsg registers a recipe download result in the bubbletea application. type viewMsgRecipeDownloadResultMsg struct { @@ -535,22 +596,6 @@ type viewMsgModeUpdate struct { details string } -// viewMsgStatusUpdate updates the status message in the bubbletea application. -// "Status" represents the current action being performed by the app. -// -// Examples: Building index, Executing recipe, etc. -type viewMsgStatusUpdate struct { - title string - hasError bool - shouldQuit bool -} - -// viewMsgProgressUpdate updates the progress bar in the bubbletea application. -// "Percent" represents the percentage of the progress bar. -type viewMsgProgressUpdate struct { - Percent float64 -} - type tickMsg time.Time // initialModel returns the model for the bubbletea application. @@ -562,14 +607,12 @@ func initialModel(logger *slog.Logger, vaultProvider *vault.Provider1Password, b s.Style = spinnerStyle m := viewModel{ - mode: "sync", - currentAction: "Initializing...", - details: "Loading...", - showProgress: true, - progress: progress.New(progress.WithGradient("#9FC131", "#DBF227")), - spinner: s, - results: make([]viewMsgRecipeDownloadResultMsg, numLastResults), - hasError: false, + mode: "sync", + showProgress: true, + progress: progress.New(progress.WithGradient("#9FC131", "#DBF227")), + spinner: s, + results: make([]viewMsgRecipeDownloadResultMsg, numLastResults), + hasError: false, vaultProvider: vaultProvider, buchhalterAPIClient: buchhalterAPIClient, @@ -586,9 +629,7 @@ func initialModel(logger *slog.Logger, vaultProvider *vault.Provider1Password, b // Init initializes the bubbletea application. // Returns an initial command for the application to run. func (m viewModel) Init() tea.Cmd { - return tea.Sequence( - m.spinner.Tick, - ) + return m.spinner.Tick } // Update updates the bubbletea application model. @@ -639,12 +680,34 @@ func (m viewModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil + case utils.ViewStatusUpdateMsg: + m.actionInProgress = msg.Message + m.actionDetails = msg.Details + + if msg.Completed { + // TODO Add error also to completed if it doesn't quit? + m.actionsCompleted = append(m.actionsCompleted, msg.Message) + m.actionInProgress = "" + } + + if msg.Err != nil { + m.actionError = msg.Err.Error() + } + + if msg.ShouldQuit { + return m, func() tea.Msg { + return viewQuitMsg{} + } + } + + return m, nil + case updateBrowserContext: m.logger.Info("Updating browser context") m.browserCtx = msg.ctx return m, nil - case viewMsgQuit: + case viewQuitMsg: m.logger.Info("Initiating shutdown sequence") mn := quit(m) @@ -667,37 +730,18 @@ func (m viewModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } return m, nil - case viewMsgStatusUpdate: - m.currentAction = msg.title - if msg.hasError { - m.hasError = true - } - - if msg.shouldQuit { - return m, tea.Quit - } - return m, nil - case viewMsgModeUpdate: - m.currentAction = msg.title - m.details = msg.details + m.actionInProgress = msg.title + m.actionDetails = msg.details + m.mode = msg.mode m.showProgress = false return m, nil - case viewMsgProgressUpdate: - cmd := m.progress.SetPercent(msg.Percent) - return m, cmd - - case utils.ViewMsgProgressUpdate: + case utils.ViewProgressUpdateMsg: cmd := m.progress.SetPercent(msg.Percent) return m, cmd - case utils.ViewMsgStatusAndDescriptionUpdate: - m.currentAction = msg.Title - m.details = msg.Description - return m, nil - case tickMsg: if m.progress.Percent() == 1.0 { m.showProgress = false @@ -732,16 +776,39 @@ func (m viewModel) View() string { textStyleGrayBold(fmt.Sprintf("Using OICDB %s and CLI %s", m.recipeParser.OicdbVersion, cliVersion)), ) + "\n") - if !m.hasError { - s.WriteString(m.spinner.View() + m.currentAction) - s.WriteString(helpStyle.Render(" " + m.details)) - } else { - s.WriteString(errorStyle.Render("ERROR: " + m.currentAction)) - s.WriteString(helpStyle.Render(" " + m.details)) + for _, actionCompleted := range m.actionsCompleted { + s.WriteString(checkMark.Render() + " " + textStyleBold(actionCompleted) + "\n") + } + + if len(m.actionInProgress) > 0 { + if len(m.actionDetails) > 0 { + s.WriteString(m.spinner.View() + textStyleBold(m.actionInProgress)) + s.WriteString(helpStyle.Render(" " + m.actionDetails)) + } else { + s.WriteString(m.spinner.View() + textStyleBold(m.actionInProgress) + "\n") + } + } + + if len(m.actionError) > 0 { + s.WriteString(errorkMark.Render() + " " + errorStyle.Render(m.actionError) + "\n") + if len(m.actionDetails) > 0 { + s.WriteString(helpStyle.Render(" " + m.actionDetails)) + } } s.WriteString("\n") + if len(m.currentAction) > 0 { + if !m.hasError { + s.WriteString(m.spinner.View() + m.currentAction) + s.WriteString(helpStyle.Render(" " + m.details)) + } else { + s.WriteString(errorStyle.Render("ERROR: " + m.currentAction)) + s.WriteString(helpStyle.Render(" " + m.details)) + } + s.WriteString("\n") + } + if m.showProgress { s.WriteString(m.progress.View() + "\n\n") } @@ -788,10 +855,11 @@ func quit(m viewModel) viewModel { m.showProgress = false } else { - m.currentAction = "Thanks for using buchhalter.ai!" + m.actionInProgress = "Thanks for using buchhalter.ai!" + m.actionDetails = "HAVE A NICE DAY! :)" + m.quitting = true m.showProgress = false - m.details = "HAVE A NICE DAY! :)" } // Stopping the browser instance diff --git a/go.mod b/go.mod index fb06e94..9718420 100644 --- a/go.mod +++ b/go.mod @@ -41,6 +41,7 @@ require ( github.com/muesli/cancelreader v0.2.2 // indirect github.com/muesli/termenv v0.15.2 // indirect github.com/pelletier/go-toml/v2 v2.2.2 // indirect + github.com/pkg/errors v0.9.1 // indirect github.com/rivo/uniseg v0.4.7 // indirect github.com/sagikazarmark/locafero v0.6.0 // indirect github.com/sagikazarmark/slog-shim v0.1.0 // indirect diff --git a/go.sum b/go.sum index b7e042a..37c34b3 100644 --- a/go.sum +++ b/go.sum @@ -84,6 +84,8 @@ github.com/orisano/pixelmatch v0.0.0-20220722002657-fb0b55479cde h1:x0TT0RDC7UhA github.com/orisano/pixelmatch v0.0.0-20220722002657-fb0b55479cde/go.mod h1:nZgzbfBr3hhjoZnS66nKrHmduYNpc34ny7RK4z5/HM0= github.com/pelletier/go-toml/v2 v2.2.2 h1:aYUidT7k73Pcl9nb2gScu7NSrKCSHIDE89b3+6Wq+LM= github.com/pelletier/go-toml/v2 v2.2.2/go.mod h1:1t835xjRzz80PqgE6HHgN2JOsmgYu/h4qDAS4n929Rs= +github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= +github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= diff --git a/lib/browser/browser.go b/lib/browser/browser.go index d29faee..c3a1f52 100644 --- a/lib/browser/browser.go +++ b/lib/browser/browser.go @@ -151,9 +151,9 @@ func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountI n := 1 var result utils.RecipeResult for _, step := range recipe.Steps { - p.Send(utils.ViewMsgStatusAndDescriptionUpdate{ - Title: fmt.Sprintf("Downloading invoices from %s (%d/%d):", recipe.Supplier, n, stepCountInCurrentRecipe), - Description: step.Description, + p.Send(utils.ViewStatusUpdateMsg{ + Message: fmt.Sprintf("Downloading invoices from `%s` (%d/%d):", recipe.Supplier, n, stepCountInCurrentRecipe), + Details: step.Description, }) stepResultChan := make(chan utils.StepResult, 1) @@ -265,7 +265,7 @@ func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountI } } cs = (float64(baseCountStep) + float64(n)) / float64(totalStepCount) - p.Send(utils.ViewMsgProgressUpdate{Percent: cs}) + p.Send(utils.ViewProgressUpdateMsg{Percent: cs}) n++ } diff --git a/lib/browser/oauth2.go b/lib/browser/oauth2.go index 9feb6c6..7e78088 100644 --- a/lib/browser/oauth2.go +++ b/lib/browser/oauth2.go @@ -136,9 +136,9 @@ func (b *ClientAuthBrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, n := 1 var result utils.RecipeResult for _, step := range recipe.Steps { - p.Send(utils.ViewMsgStatusAndDescriptionUpdate{ - Title: fmt.Sprintf("Downloading invoices from %s (%d/%d):", recipe.Supplier, n, stepCountInCurrentRecipe), - Description: step.Description, + p.Send(utils.ViewStatusUpdateMsg{ + Message: fmt.Sprintf("Downloading invoices from %s (%d/%d):", recipe.Supplier, n, stepCountInCurrentRecipe), + Details: step.Description, }) stepResultChan := make(chan utils.StepResult, 1) @@ -202,7 +202,7 @@ func (b *ClientAuthBrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, } cs = (float64(baseCountStep) + float64(n)) / float64(totalStepCount) - p.Send(utils.ViewMsgProgressUpdate{Percent: cs}) + p.Send(utils.ViewProgressUpdateMsg{Percent: cs}) n++ } diff --git a/lib/utils/utils.go b/lib/utils/utils.go index eea9c6c..1023654 100644 --- a/lib/utils/utils.go +++ b/lib/utils/utils.go @@ -20,16 +20,18 @@ const ( randomStringCharset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" ) -// ViewMsgProgressUpdate updates the progress bar in the bubbletea application. +// ViewProgressUpdateMsg updates the progress bar in the bubbletea application. // "Percent" represents the percentage of the progress bar. -type ViewMsgProgressUpdate struct { +type ViewProgressUpdateMsg struct { Percent float64 } -// ViewMsgStatusAndDescriptionUpdate updates the status and description in the bubbletea application. -type ViewMsgStatusAndDescriptionUpdate struct { - Title string - Description string +type ViewStatusUpdateMsg struct { + Message string + Details string + Err error + Completed bool + ShouldQuit bool } // RecipeResult represents the result of a single recipe execution. From 0d291c0be2d7fb28324d458897c9c90f65006f60 Mon Sep 17 00:00:00 2001 From: Andy Grunwald Date: Fri, 13 Sep 2024 19:44:53 +0200 Subject: [PATCH 2/4] sync cmd: Display errors that don't abort the program as normal UI actions --- cmd/sync.go | 86 +++++++++++++++++++++++++++++------------- lib/browser/browser.go | 7 ++-- lib/browser/oauth2.go | 7 ++-- lib/utils/utils.go | 12 ++++++ 4 files changed, 79 insertions(+), 33 deletions(-) diff --git a/cmd/sync.go b/cmd/sync.go index e7cf252..351ed95 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -145,12 +145,16 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu err := documentArchive.BuildArchiveIndex() if err != nil { logger.Error("Error building document archive index", "error", err) - p.Send(utils.ViewStatusUpdateMsg{Err: fmt.Errorf("error building document archive index: %w", err)}) + p.Send(utils.ViewStatusUpdateMsg{ + Err: fmt.Errorf("error building document archive index: %w", err), + Completed: true, + }) + } else { + p.Send(utils.ViewStatusUpdateMsg{ + Message: "Building archive index", + Completed: true, + }) } - p.Send(utils.ViewStatusUpdateMsg{ - Message: "Building archive index", - Completed: true, - }) // Check for OICDB schema updates p.Send(utils.ViewStatusUpdateMsg{Message: "Checking for OICDB schema updates"}) @@ -159,12 +163,16 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu err = buchhalterAPIClient.UpdateOpenInvoiceCollectorDBSchemaIfAvailable(localOICDBSchemaChecksum) if err != nil { logger.Error("Error checking for OICDB schema updates", "error", err) - p.Send(utils.ViewStatusUpdateMsg{Err: fmt.Errorf("error checking for OICDB schema updates: %w", err)}) + p.Send(utils.ViewStatusUpdateMsg{ + Err: fmt.Errorf("error checking for OICDB schema updates: %w", err), + Completed: true, + }) + } else { + p.Send(utils.ViewStatusUpdateMsg{ + Message: "Checking for OICDB schema updates", + Completed: true, + }) } - p.Send(utils.ViewStatusUpdateMsg{ - Message: "Checking for OICDB schema updates", - Completed: true, - }) developmentMode := viper.GetBool("dev") if !developmentMode { @@ -175,12 +183,16 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu err = buchhalterAPIClient.UpdateOpenInvoiceCollectorDBIfAvailable(localOICDBChecksum) if err != nil { logger.Error("Error checking for OICDB repository updates", "error", err) - p.Send(utils.ViewStatusUpdateMsg{Err: fmt.Errorf("error for OICDB repository updates: %w", err)}) + p.Send(utils.ViewStatusUpdateMsg{ + Err: fmt.Errorf("error for OICDB repository updates: %w", err), + Completed: true, + }) + } else { + p.Send(utils.ViewStatusUpdateMsg{ + Message: "Checking for OICDB repository updates", + Completed: true, + }) } - p.Send(utils.ViewStatusUpdateMsg{ - Message: "Checking for OICDB repository updates", - Completed: true, - }) } statusUpdateMessage := "Loading recipes and credentials for suppliers" @@ -254,7 +266,8 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu logger.Error(vaultProvider.GetHumanReadableErrorMessage(err)) // TODO Let `GetHumanReadableErrorMessage` return a proper error p.Send(utils.ViewStatusUpdateMsg{ - Err: errors.New(vaultProvider.GetHumanReadableErrorMessage(err)), + Err: errors.New(vaultProvider.GetHumanReadableErrorMessage(err)), + Completed: true, }) continue } @@ -270,9 +283,10 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu browserDriver, err := browser.NewBrowserDriver(logger, recipeCredentials, buchhalterDocumentsDirectory, documentArchive, buchhalterMaxDownloadFilesPerReceipt) if err != nil { p.Send(utils.ViewStatusUpdateMsg{ - Err: fmt.Errorf("error downloading invoices from %s: %w", recipesToExecute[i].recipe.Supplier, err), + Err: fmt.Errorf("error downloading invoices from %s: %w", recipesToExecute[i].recipe.Supplier, err), + Completed: true, }) - // TODO Should we continue here or abort? + // TODO Should we continue here or abort? !!! } // Send the browser context to the view layer @@ -293,9 +307,10 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu clientDriver, err := browser.NewClientAuthBrowserDriver(logger, recipeCredentials, buchhalterConfigDirectory, buchhalterDocumentsDirectory, documentArchive) if err != nil { p.Send(utils.ViewStatusUpdateMsg{ - Err: fmt.Errorf("error downloading invoices from %s: %w", recipesToExecute[i].recipe.Supplier, err), + Err: fmt.Errorf("error downloading invoices from %s: %w", recipesToExecute[i].recipe.Supplier, err), + Completed: true, }) - // TODO Should we continue here or abort? + // TODO Should we continue here or abort? !!! } // Send the browser context to the view layer @@ -351,7 +366,10 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu user, err := buchhalterAPIClient.GetAuthenticatedUser() if err != nil { logger.Error("Error retrieving authenticated user", "error", err) - p.Send(utils.ViewStatusUpdateMsg{Err: fmt.Errorf("error retrieving a premium subscription to Buchhalter API: %w", err)}) + p.Send(utils.ViewStatusUpdateMsg{ + Err: fmt.Errorf("error retrieving a premium subscription to Buchhalter API: %w", err), + Completed: true, + }) } if user != nil && len(user.User.ID) > 0 { statusUpdateMessage = "Uploading documents to Buchhalter API" @@ -531,7 +549,7 @@ type viewModel struct { mode string // UI - actionsCompleted []string + actionsCompleted []utils.UIAction actionInProgress string actionError string actionDetails string @@ -607,6 +625,8 @@ func initialModel(logger *slog.Logger, vaultProvider *vault.Provider1Password, b s.Style = spinnerStyle m := viewModel{ + actionsCompleted: []utils.UIAction{}, + mode: "sync", showProgress: true, progress: progress.New(progress.WithGradient("#9FC131", "#DBF227")), @@ -684,13 +704,20 @@ func (m viewModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.actionInProgress = msg.Message m.actionDetails = msg.Details - if msg.Completed { - // TODO Add error also to completed if it doesn't quit? - m.actionsCompleted = append(m.actionsCompleted, msg.Message) + if msg.Completed && msg.Err == nil { + m.actionsCompleted = append(m.actionsCompleted, utils.UIAction{ + Message: msg.Message, + Style: utils.UIActionStyleSuccess, + }) m.actionInProgress = "" } - if msg.Err != nil { + if msg.Completed && msg.Err != nil && !msg.ShouldQuit { + m.actionsCompleted = append(m.actionsCompleted, utils.UIAction{ + Message: msg.Err.Error(), + Style: utils.UIActionStyleError, + }) + } else if msg.Err != nil { m.actionError = msg.Err.Error() } @@ -777,7 +804,12 @@ func (m viewModel) View() string { ) + "\n") for _, actionCompleted := range m.actionsCompleted { - s.WriteString(checkMark.Render() + " " + textStyleBold(actionCompleted) + "\n") + switch actionCompleted.Style { + case utils.UIActionStyleSuccess: + s.WriteString(checkMark.Render() + " " + textStyleBold(actionCompleted.Message) + "\n") + case utils.UIActionStyleError: + s.WriteString(errorkMark.Render() + " " + errorStyle.Render(actionCompleted.Message) + "\n") + } } if len(m.actionInProgress) > 0 { diff --git a/lib/browser/browser.go b/lib/browser/browser.go index c3a1f52..e7e5b9f 100644 --- a/lib/browser/browser.go +++ b/lib/browser/browser.go @@ -103,8 +103,9 @@ func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountI ctx := b.browserCtx defer b.browserCancel() - // get chrome version for metrics - if b.ChromeVersion == "" { + // Get chrome version for metrics + b.ChromeVersion = strings.TrimSpace(b.ChromeVersion) + if len(b.ChromeVersion) == 0 { err := chromedp.Run(ctx, chromedp.Tasks{ chromedp.Navigate("chrome://version"), chromedp.Text(`#version`, &b.ChromeVersion, chromedp.NodeVisible), @@ -117,7 +118,7 @@ func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountI } b.logger.Info("Starting chrome browser driver ... completed ", "recipe", recipe.Supplier, "recipe_version", recipe.Version, "chrome_version", b.ChromeVersion) - // create download directories + // Create download directories var err error b.downloadsDirectory, b.documentsDirectory, err = utils.InitSupplierDirectories(b.buchhalterDocumentsDirectory, recipe.Supplier) if err != nil { diff --git a/lib/browser/oauth2.go b/lib/browser/oauth2.go index 7e78088..2b45e19 100644 --- a/lib/browser/oauth2.go +++ b/lib/browser/oauth2.go @@ -109,8 +109,9 @@ func (b *ClientAuthBrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, ctx := b.browserCtx defer b.browserCancel() - // get chrome version for metrics - if b.ChromeVersion == "" { + // Get chrome version for metrics + b.ChromeVersion = strings.TrimSpace(b.ChromeVersion) + if len(b.ChromeVersion) == 0 { err := chromedp.Run(ctx, chromedp.Tasks{ chromedp.Navigate("chrome://version"), chromedp.Text(`#version`, &b.ChromeVersion, chromedp.NodeVisible), @@ -123,7 +124,7 @@ func (b *ClientAuthBrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, } b.logger.Info("Starting client auth chrome browser driver ... completed ", "recipe", recipe.Supplier, "recipe_version", recipe.Version, "chrome_version", b.ChromeVersion) - // create download directories + // Create download directories var err error b.downloadsDirectory, b.documentsDirectory, err = utils.InitSupplierDirectories(b.buchhalterDocumentsDirectory, recipe.Supplier) if err != nil { diff --git a/lib/utils/utils.go b/lib/utils/utils.go index 1023654..686d785 100644 --- a/lib/utils/utils.go +++ b/lib/utils/utils.go @@ -52,6 +52,18 @@ type StepResult struct { Break bool } +type UIActionStyle string + +const ( + UIActionStyleSuccess UIActionStyle = "success" + UIActionStyleError UIActionStyle = "error" +) + +type UIAction struct { + Message string + Style UIActionStyle +} + func InitSupplierDirectories(buchhalterDirectory, supplier string) (string, string, error) { downloadsDirectory := filepath.Join(buchhalterDirectory, "_tmp", supplier) documentsDirectory := filepath.Join(buchhalterDirectory, supplier) From 91b7d365bd19723981e2089e911ea1e8a5a492da Mon Sep 17 00:00:00 2001 From: Andy Grunwald Date: Fri, 13 Sep 2024 20:48:51 +0200 Subject: [PATCH 3/4] sync cmd: Add better error handling for unexpected cases --- cmd/sync.go | 44 ++++++++++++++++++++++++++++---------- lib/browser/browser.go | 48 ++++++++++++++++++++++-------------------- lib/browser/oauth2.go | 23 ++++++++++++-------- lib/parser/parser.go | 2 -- 4 files changed, 72 insertions(+), 45 deletions(-) diff --git a/cmd/sync.go b/cmd/sync.go index 351ed95..a10cabb 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -282,19 +282,28 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu case "browser": browserDriver, err := browser.NewBrowserDriver(logger, recipeCredentials, buchhalterDocumentsDirectory, documentArchive, buchhalterMaxDownloadFilesPerReceipt) if err != nil { + logger.Error("Error initializing a new browser driver", "error", err, "supplier", recipesToExecute[i].recipe.Supplier) p.Send(utils.ViewStatusUpdateMsg{ - Err: fmt.Errorf("error downloading invoices from %s: %w", recipesToExecute[i].recipe.Supplier, err), + Err: fmt.Errorf("error initializing a new browser driver for supplier `%s`: %w", recipesToExecute[i].recipe.Supplier, err), Completed: true, }) - // TODO Should we continue here or abort? !!! + // We skip this supplier and continue with the next one + continue } // Send the browser context to the view layer // This is needed in case of an external abort signal (e.g. CTRL+C). p.Send(updateBrowserContext{ctx: browserDriver.GetContext()}) - // TODO Catch error here - recipeResult = browserDriver.RunRecipe(p, totalStepCount, stepCountInCurrentRecipe, baseCountStep, recipesToExecute[i].recipe) + recipeResult, err = browserDriver.RunRecipe(p, totalStepCount, stepCountInCurrentRecipe, baseCountStep, recipesToExecute[i].recipe) + if err != nil { + logger.Error("Error running browser recipe", "error", err, "supplier", recipesToExecute[i].recipe.Supplier) + p.Send(utils.ViewStatusUpdateMsg{ + Err: fmt.Errorf("error running browser recipe for supplier `%s`: %w", recipesToExecute[i].recipe.Supplier, err), + Completed: true, + }) + // We fall through, because recipeResult might be set and contains additional information + } if ChromeVersion == "" { ChromeVersion = browserDriver.ChromeVersion } @@ -306,19 +315,28 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu case "client": clientDriver, err := browser.NewClientAuthBrowserDriver(logger, recipeCredentials, buchhalterConfigDirectory, buchhalterDocumentsDirectory, documentArchive) if err != nil { + logger.Error("Error initializing a new client auth browser driver", "error", err, "supplier", recipesToExecute[i].recipe.Supplier) p.Send(utils.ViewStatusUpdateMsg{ - Err: fmt.Errorf("error downloading invoices from %s: %w", recipesToExecute[i].recipe.Supplier, err), + Err: fmt.Errorf("error initializing a new client auth browser for supplier `%s`: %w", recipesToExecute[i].recipe.Supplier, err), Completed: true, }) - // TODO Should we continue here or abort? !!! + // We skip this supplier and continue with the next one + continue } // Send the browser context to the view layer // This is needed in case of an external abort signal (e.g. CTRL+C). p.Send(updateBrowserContext{ctx: clientDriver.GetContext()}) - // TODO Catch error here - recipeResult = clientDriver.RunRecipe(p, totalStepCount, stepCountInCurrentRecipe, baseCountStep, recipesToExecute[i].recipe) + recipeResult, err = clientDriver.RunRecipe(p, totalStepCount, stepCountInCurrentRecipe, baseCountStep, recipesToExecute[i].recipe) + if err != nil { + logger.Error("Error running browser recipe", "error", err, "supplier", recipesToExecute[i].recipe.Supplier) + p.Send(utils.ViewStatusUpdateMsg{ + Err: fmt.Errorf("error running browser recipe for supplier `%s`: %w", recipesToExecute[i].recipe.Supplier, err), + Completed: true, + }) + // We fall through, because recipeResult might be set and contains additional information + } if ChromeVersion == "" { ChromeVersion = clientDriver.ChromeVersion } @@ -328,6 +346,8 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu // In case of an external abort signal (e.g. CTRL+C), bubbletea will call `chromedp.Cancel()`. } + // TODO recipeResult can be empty! (not nil, but without values) + rdx := repository.RunDataSupplier{ Supplier: recipesToExecute[i].recipe.Supplier, Version: recipesToExecute[i].recipe.Version, @@ -338,7 +358,6 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu } RunData = append(RunData, rdx) - // TODO Check for recipeResult.LastErrorMessage p.Send(viewMsgRecipeDownloadResultMsg{ duration: time.Since(startTime), newFilesCount: recipeResult.NewFilesCount, @@ -391,7 +410,7 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu logger.Info("Uploading document to Buchhalter API ...", "file", fileInfo.Path, "checksum", fileChecksum) result, err := buchhalterAPIClient.DoesDocumentExist(fileChecksum) if err != nil { - // TODO Implement better error handling + // Skip the file if we can't check the existence of the document in the API logger.Error("Error checking if document exists already in Buchhalter API", "file", fileInfo.Path, "checksum", fileChecksum, "error", err) continue } @@ -405,7 +424,10 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu err = buchhalterAPIClient.UploadDocument(fileInfo.Path, fileInfo.Supplier) if err != nil { - // TODO Implement better error handling + p.Send(utils.ViewStatusUpdateMsg{ + Err: fmt.Errorf("error uploading document `%s` from `%s` to Buchhalter API: %w", fileInfo.Path, fileInfo.Supplier, err), + Completed: true, + }) logger.Error("Error uploading document to Buchhalter API", "file", fileInfo.Path, "supplier", fileInfo.Supplier, "error", err) continue } diff --git a/lib/browser/browser.go b/lib/browser/browser.go index e7e5b9f..4313b7e 100644 --- a/lib/browser/browser.go +++ b/lib/browser/browser.go @@ -36,13 +36,11 @@ type BrowserDriver struct { documentArchive *archive.DocumentArchive buchhalterDocumentsDirectory string + downloadsDirectory string + documentsDirectory string ChromeVersion string - // TODO Check if those are needed - downloadsDirectory string - documentsDirectory string - browserCtx context.Context browserCancel context.CancelFunc recipeTimeout time.Duration @@ -97,7 +95,7 @@ func (b *BrowserDriver) GetContext() context.Context { return b.browserCtx } -func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountInCurrentRecipe int, baseCountStep int, recipe *parser.Recipe) utils.RecipeResult { +func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountInCurrentRecipe int, baseCountStep int, recipe *parser.Recipe) (utils.RecipeResult, error) { b.logger.Info("Starting chrome browser driver ...", "recipe", recipe.Supplier, "recipe_version", recipe.Version) ctx := b.browserCtx @@ -111,19 +109,25 @@ func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountI chromedp.Text(`#version`, &b.ChromeVersion, chromedp.NodeVisible), }) if err != nil { - // TODO Implement error handling - panic(err) + b.logger.Error("Error while determining the Chrome version", "error", err.Error()) + p.Send(utils.ViewStatusUpdateMsg{ + Err: fmt.Errorf("error while determining the Chrome version: %w", err), + Completed: true, + }) + // We fall through here, because we can still continue without the Chrome version } b.ChromeVersion = strings.TrimSpace(b.ChromeVersion) } b.logger.Info("Starting chrome browser driver ... completed ", "recipe", recipe.Supplier, "recipe_version", recipe.Version, "chrome_version", b.ChromeVersion) + var result utils.RecipeResult + // Create download directories var err error b.downloadsDirectory, b.documentsDirectory, err = utils.InitSupplierDirectories(b.buchhalterDocumentsDirectory, recipe.Supplier) if err != nil { - // TODO Implement error handling - fmt.Println(err) + b.logger.Error("Error while creating download directory", "error", err.Error(), "documents_directory", b.buchhalterDocumentsDirectory, "supplier", recipe.Supplier) + return result, fmt.Errorf("error while creating download directory: %w", err) } b.logger.Info("Download directories created", "downloads_directory", b.downloadsDirectory, "documents_directory", b.documentsDirectory) @@ -139,8 +143,8 @@ func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountI }), }) if err != nil { - // TODO Implement error handling - panic(err) + b.logger.Error("Error while configuring the download behavior of chrome", "error", err.Error(), "downloads_directory", b.downloadsDirectory) + return result, fmt.Errorf("error while configuring the download behavior of chrome: %w", err) } // Disable downloading images for performance reasons @@ -150,7 +154,6 @@ func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountI var cs float64 n := 1 - var result utils.RecipeResult for _, step := range recipe.Steps { p.Send(utils.ViewStatusUpdateMsg{ Message: fmt.Sprintf("Downloading invoices from `%s` (%d/%d):", recipe.Supplier, n, stepCountInCurrentRecipe), @@ -163,7 +166,6 @@ func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountI if step.When.URL != "" { var currentURL string if err := chromedp.Run(ctx, chromedp.Location(¤tURL)); err != nil { - // TODO implement better error handling b.logger.Error("Failed to get current URL", "error", err.Error()) // Skipping step @@ -236,10 +238,10 @@ func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountI } err = utils.TruncateDirectory(b.downloadsDirectory) if err != nil { - // TODO Implement error handling - fmt.Println(err) + b.logger.Error("Error while truncating the download directory", "error", err.Error(), "downloads_directory", b.downloadsDirectory) + return result, fmt.Errorf("error while truncating the download directory: %w", err) } - return result + return result, nil } case <-time.After(b.recipeTimeout): @@ -253,16 +255,16 @@ func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountI } err = utils.TruncateDirectory(b.downloadsDirectory) if err != nil { - // TODO Implement error handling - fmt.Println(err) + b.logger.Error("Error while truncating the download directory", "error", err.Error(), "downloads_directory", b.downloadsDirectory) + return result, fmt.Errorf("error while truncating the download directory: %w", err) } - // Imagine we run the `downloadALl` step, we download 2 files and then the recipe times out. + // Imagine we run the `downloadAll` step, we download 2 files and then the recipe times out. // It is bad that the recipe timed out, however, we still want to process with the 2 new downloaded documents. // Process in this context means to move the files to the documents directory and add them to the document archive. // Thats why we don't abort if the recipe timed out in this stage. if !(step.Action == "downloadAll" && b.downloadedFilesCount > 0) { - return result + return result, nil } } cs = (float64(baseCountStep) + float64(n)) / float64(totalStepCount) @@ -272,10 +274,10 @@ func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountI err = utils.TruncateDirectory(b.downloadsDirectory) if err != nil { - // TODO Implement error handling - fmt.Println(err) + b.logger.Error("Error while truncating the download directory", "error", err.Error(), "downloads_directory", b.downloadsDirectory) + return result, fmt.Errorf("error while truncating the download directory: %w", err) } - return result + return result, nil } func (b *BrowserDriver) stepOpen(ctx context.Context, step parser.Step) utils.StepResult { diff --git a/lib/browser/oauth2.go b/lib/browser/oauth2.go index 2b45e19..dd2feb3 100644 --- a/lib/browser/oauth2.go +++ b/lib/browser/oauth2.go @@ -103,7 +103,7 @@ func (b *ClientAuthBrowserDriver) GetContext() context.Context { return b.browserCtx } -func (b *ClientAuthBrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountInCurrentRecipe int, baseCountStep int, recipe *parser.Recipe) utils.RecipeResult { +func (b *ClientAuthBrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountInCurrentRecipe int, baseCountStep int, recipe *parser.Recipe) (utils.RecipeResult, error) { b.logger.Info("Starting client auth chrome browser driver ...", "recipe", recipe.Supplier, "recipe_version", recipe.Version) ctx := b.browserCtx @@ -117,25 +117,30 @@ func (b *ClientAuthBrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, chromedp.Text(`#version`, &b.ChromeVersion, chromedp.NodeVisible), }) if err != nil { - // TODO Implement error handling - panic(err) + b.logger.Error("Error while determining the Chrome version", "error", err.Error()) + p.Send(utils.ViewStatusUpdateMsg{ + Err: fmt.Errorf("error while determining the Chrome version: %w", err), + Completed: true, + }) + // We fall through here, because we can still continue without the Chrome version } b.ChromeVersion = strings.TrimSpace(b.ChromeVersion) } b.logger.Info("Starting client auth chrome browser driver ... completed ", "recipe", recipe.Supplier, "recipe_version", recipe.Version, "chrome_version", b.ChromeVersion) + var result utils.RecipeResult + // Create download directories var err error b.downloadsDirectory, b.documentsDirectory, err = utils.InitSupplierDirectories(b.buchhalterDocumentsDirectory, recipe.Supplier) if err != nil { - // TODO Implement error handling - fmt.Println(err) + b.logger.Error("Error while creating download directory", "error", err.Error(), "documents_directory", b.buchhalterDocumentsDirectory, "supplier", recipe.Supplier) + return result, err } b.logger.Info("Download directories created", "downloads_directory", b.downloadsDirectory, "documents_directory", b.documentsDirectory) var cs float64 n := 1 - var result utils.RecipeResult for _, step := range recipe.Steps { p.Send(utils.ViewStatusUpdateMsg{ Message: fmt.Sprintf("Downloading invoices from %s (%d/%d):", recipe.Supplier, n, stepCountInCurrentRecipe), @@ -186,7 +191,7 @@ func (b *ClientAuthBrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, NewFilesCount: b.newFilesCount, } if lastStepResult.Break { - return result + return result, nil } } @@ -199,7 +204,7 @@ func (b *ClientAuthBrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, LastStepDescription: step.Description, NewFilesCount: b.newFilesCount, } - return result + return result, nil } cs = (float64(baseCountStep) + float64(n)) / float64(totalStepCount) @@ -207,7 +212,7 @@ func (b *ClientAuthBrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, n++ } - return result + return result, nil } func (b *ClientAuthBrowserDriver) stepOauth2Setup(step parser.Step) utils.StepResult { diff --git a/lib/parser/parser.go b/lib/parser/parser.go index 4ed628a..ba489bb 100644 --- a/lib/parser/parser.go +++ b/lib/parser/parser.go @@ -39,7 +39,6 @@ type Database struct { } type Recipe struct { - // TODO Rename Prodiver to Supplier Supplier string `json:"supplier"` Domains []string `json:"domains"` Version string `json:"version"` @@ -169,7 +168,6 @@ func validateRecipes(buchhalterConfigDirectory string) (bool, error) { return true, nil } - // TODO Create our own error type here (validation error) errorMessageParts := []string{} for _, errorDescription := range result.Errors() { errorMessageParts = append(errorMessageParts, errorDescription.String()) From 56578a59180c4760c9f2885b47dac1a782403a22 Mon Sep 17 00:00:00 2001 From: Andy Grunwald Date: Fri, 13 Sep 2024 21:01:17 +0200 Subject: [PATCH 4/4] sync cmd: Remove `fmt.Println` and add error handling --- lib/browser/browser.go | 8 +++----- lib/browser/oauth2.go | 25 ++++++++++--------------- 2 files changed, 13 insertions(+), 20 deletions(-) diff --git a/lib/browser/browser.go b/lib/browser/browser.go index 4313b7e..db45aa7 100644 --- a/lib/browser/browser.go +++ b/lib/browser/browser.go @@ -137,9 +137,8 @@ func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountI WithDownloadPath(b.downloadsDirectory). WithEventsEnabled(true), chromedp.ActionFunc(func(ctx context.Context) error { - // TODO Implement error handling - _ = b.waitForLoadEvent(ctx) - return nil + err = b.waitForLoadEvent(ctx) + return err }), }) if err != nil { @@ -465,8 +464,7 @@ func (b *BrowserDriver) stepTransform(step parser.Step) utils.StepResult { case "unzip": zipFiles, err := utils.FindFiles(b.downloadsDirectory, ".zip") if err != nil { - // TODO improve error handling - fmt.Println(err) + return utils.StepResult{Status: "error", Message: fmt.Sprintf("Error while finding zip files: %s", err)} } for _, s := range zipFiles { b.logger.Debug("Executing recipe step ... unzipping file", "action", step.Action, "source", s, "destination", b.downloadsDirectory) diff --git a/lib/browser/oauth2.go b/lib/browser/oauth2.go index dd2feb3..a192684 100644 --- a/lib/browser/oauth2.go +++ b/lib/browser/oauth2.go @@ -271,8 +271,8 @@ func (b *ClientAuthBrowserDriver) stepOauth2Authenticate(ctx context.Context, re verifier, challenge, err := utils.Oauth2Pkce(b.oauth2PkceVerifierLength) if err != nil { - // TODO implement error handling - fmt.Println(err) + b.logger.Error("Error while creating the OAuth2 Pkce", "error", err.Error()) + return utils.StepResult{Status: "error", Message: fmt.Sprintf("error while creating the OAuth2 Pkce: %s", err.Error())} } state := utils.RandomString(20) @@ -309,29 +309,27 @@ func (b *ClientAuthBrowserDriver) stepOauth2Authenticate(ctx context.Context, re return utils.StepResult{Status: "error", Message: "error while logging in: " + err.Error()} } - /** Check for 2FA authentication */ + // Check for 2FA authentication var faNodes []*cdp.Node err = chromedp.Run(ctx, b.run(5*time.Second, chromedp.WaitVisible(`#form-input-passcode`, chromedp.ByID)), chromedp.Nodes("#form-input-passcode", &faNodes, chromedp.AtLeast(0)), ) - if err != nil { b.logger.Error("Error while logging in", "error", err.Error()) return utils.StepResult{Status: "error", Message: "error while logging in: " + err.Error()} } - /** Insert 2FA code */ + // Insert 2FA code if len(faNodes) > 0 { err = chromedp.Run(ctx, chromedp.SendKeys("#form-input-passcode", credentials.Totp, chromedp.ByID), chromedp.Click("#form-submit", chromedp.ByID), ) - } - - if err != nil { - b.logger.Error("Error while logging in", "error", err.Error()) - return utils.StepResult{Status: "error", Message: "error while logging in: " + err.Error()} + if err != nil { + b.logger.Error("Error while logging in (2FA)", "error", err.Error()) + return utils.StepResult{Status: "error", Message: "error while logging in (2fa): " + err.Error()} + } } /** Request access token */ @@ -340,7 +338,6 @@ func (b *ClientAuthBrowserDriver) stepOauth2Authenticate(ctx context.Context, re chromedp.Sleep(2*time.Second), chromedp.Location(&u), ) - if err != nil { b.logger.Error("Error while requesting access token", "error", err.Error()) return utils.StepResult{Status: "error", Message: "error while logging in: " + err.Error()} @@ -404,8 +401,7 @@ func (b *ClientAuthBrowserDriver) stepOauth2PostAndGetItems(ctx context.Context, var jsr interface{} err := json.Unmarshal(body, &jsr) if err != nil { - // TODO Implement better error handling - panic(err) + return utils.StepResult{Status: "error", Message: fmt.Sprintf("Error while parsing JSON: %s", err), Break: true} } ids := extractJsonValue(jsr, step.ExtractDocumentIds) @@ -435,8 +431,7 @@ func (b *ClientAuthBrowserDriver) stepOauth2PostAndGetItems(ctx context.Context, } downloadSuccessful, err := b.doRequest(ctx, url, step.DocumentRequestMethod, step.DocumentRequestHeaders, f, nil) if err != nil { - // TODO implement error handling - fmt.Println(err) + return utils.StepResult{Status: "error", Message: fmt.Sprintf("Error while downloading invoices: %s", err.Error())} } if !downloadSuccessful { return utils.StepResult{Status: "error", Message: "Error while downloading invoices"}