From 35fda66975764fed3521cd7eec35443adcd1ac2c Mon Sep 17 00:00:00 2001 From: f1ames Date: Fri, 18 Aug 2023 10:30:20 +0200 Subject: [PATCH] fix: improve error notifications for better UX --- src/commands/bootstrap-configuration.ts | 7 ++-- src/commands/synchronize.ts | 5 ++- src/constants.ts | 3 +- src/utils/commands.ts | 3 +- src/utils/errors.ts | 16 ++------ src/utils/globals.ts | 18 +++++++++ src/utils/policy-puller.ts | 47 +++++++++++++++++++++- src/utils/runtime-context.ts | 6 ++- src/utils/tooltip.ts | 52 ++++++++++++++++++++++--- src/utils/validation.ts | 11 +++--- 10 files changed, 134 insertions(+), 34 deletions(-) diff --git a/src/commands/bootstrap-configuration.ts b/src/commands/bootstrap-configuration.ts index 43cf019..234a65e 100644 --- a/src/commands/bootstrap-configuration.ts +++ b/src/commands/bootstrap-configuration.ts @@ -2,6 +2,7 @@ import { Uri, commands, window } from 'vscode'; import { canRun } from '../utils/commands'; import { getWorkspaceConfig, getWorkspaceFolders } from '../utils/workspace'; import { createDefaultConfigFile } from '../utils/validation'; +import { raiseInfo, raiseWarning } from '../utils/errors'; import logger from '../utils/logger'; import type { Folder } from '../utils/workspace'; @@ -27,17 +28,17 @@ export function getBootstrapConfigurationCommand() { const currentConfig = await getWorkspaceConfig(folder); if (currentConfig.type === 'file') { - window.showInformationMessage(`Local '${currentConfig.fileName}' configuration file already exists, opening it.`); + raiseInfo(`Local '${currentConfig.fileName}' configuration file already exists, opening it.`); return currentConfig.path; } if (currentConfig.type === 'config') { - window.showWarningMessage(`Shared '${currentConfig.path}' configuration file already exists, opening it.`); + raiseWarning(`Shared '${currentConfig.path}' configuration file already exists, opening it.`); return currentConfig.path; } if (currentConfig.type === 'remote') { - window.showWarningMessage(`Remote '${currentConfig.fileName}' configuration file already exists, opening it.`); + raiseWarning(`Remote '${currentConfig.fileName}' configuration file already exists, opening it.`); return currentConfig.path; } diff --git a/src/commands/synchronize.ts b/src/commands/synchronize.ts index 6a12dc5..1d33e30 100644 --- a/src/commands/synchronize.ts +++ b/src/commands/synchronize.ts @@ -1,6 +1,7 @@ -import { window, commands } from 'vscode'; +import { commands } from 'vscode'; import { canRun } from '../utils/commands'; import { COMMANDS, COMMAND_NAMES } from '../constants'; +import { raiseWarning } from '../utils/errors'; import globals from '../utils/globals'; import type { RuntimeContext } from '../utils/runtime-context'; @@ -11,7 +12,7 @@ export function getSynchronizeCommand(context: RuntimeContext) { } if (!globals.user.isAuthenticated) { - window.showWarningMessage(`You are not authenticated, cannot synchronize policies. Run ${COMMAND_NAMES.LOGIN} to authenticate first.}`); + raiseWarning(`You are not authenticated, cannot synchronize policies. Run ${COMMAND_NAMES.LOGIN} to authenticate first.}`); return null; } diff --git a/src/constants.ts b/src/constants.ts index 19f51fd..af939ed 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -44,5 +44,6 @@ export const COMMAND_NAMES = { export const STATUS_BAR_TEXTS = { DEFAULT: '🔍 Monokle', - VALIDATING: '🤔 Validating...', + VALIDATING: '🤔 Monokle: Validating...', + ERROR: '❌ Monokle: Needs attention...', }; diff --git a/src/utils/commands.ts b/src/utils/commands.ts index d439937..ce0acfd 100644 --- a/src/utils/commands.ts +++ b/src/utils/commands.ts @@ -1,10 +1,11 @@ import { workspace, window } from 'vscode'; import { SETTINGS } from '../constants'; +import { raiseInfo } from './errors'; export function canRun() { const isEnabled = workspace.getConfiguration(SETTINGS.NAMESPACE).get(SETTINGS.ENABLED); if (!isEnabled) { - window.showInformationMessage('Monokle is disabled for this workspace. Enable it in the settings.'); + raiseInfo('Monokle is disabled for this workspace. Enable it in the settings.'); } return isEnabled; diff --git a/src/utils/errors.ts b/src/utils/errors.ts index f48ff6b..8f512c7 100644 --- a/src/utils/errors.ts +++ b/src/utils/errors.ts @@ -1,7 +1,7 @@ import { MessageOptions, window } from 'vscode'; import { SETTINGS } from '../constants'; import globals from './globals'; -import type { WorkspaceFolderConfig, Folder } from './workspace'; +import type { WorkspaceFolderConfig } from './workspace'; // Important: Notification promises resolves only after interaction with it (like closing), // so in most cases we don't want to wait for it to not block rest of the flow. @@ -11,8 +11,8 @@ export type ErrorAction = { callback: () => void | Promise }; -export async function raiseInvalidConfigError(config: WorkspaceFolderConfig, owner: Folder) { - let errorMsg: string; +export function getInvalidConfigError(config: WorkspaceFolderConfig) { + let errorMsg = ''; if (config.type === 'file') { errorMsg = `Your local configuration file, under '${config.path}' path, is invalid.`; @@ -27,15 +27,7 @@ export async function raiseInvalidConfigError(config: WorkspaceFolderConfig, own errorMsg = `Your remote configuration file from '${globals.remotePolicyUrl}' is invalid.`; } - if (!errorMsg) { - return null; - } - - return raiseError(`${errorMsg} Resources in '${owner.name}' folder will not be validated.`); -} - -export async function raiseCannotGetPolicyError(msg: string, actions?: ErrorAction[]) { - return raiseError(`${msg} Remote policy cannot be fetched and resources will not be validated.`, actions); + return errorMsg; } export async function raiseError(msg: string, actions: ErrorAction[] = [], options: MessageOptions = {}) { diff --git a/src/utils/globals.ts b/src/utils/globals.ts index 2704128..00cb691 100644 --- a/src/utils/globals.ts +++ b/src/utils/globals.ts @@ -2,9 +2,16 @@ import { workspace } from 'vscode'; import { DEFAULT_REMOTE_POLICY_URL, SETTINGS } from '../constants'; import { getAuthenticator } from './authentication'; import { getSynchronizer } from './synchronization'; +import { Folder } from './workspace'; + +export type FolderStatus = { + valid: boolean; + error?: string; +}; class Globals { private _storagePath: string = ''; + private statuses: Record = {}; private _authenticator: Awaited> = null; private _synchronizer: Awaited> = null; @@ -52,6 +59,17 @@ class Globals { return this._synchronizer.getPolicy(path); } + getFolderStatus(folder: Folder) { + return this.statuses[folder.id]; + } + + setFolderStatus(folder: Folder, error?: string) { + this.statuses[folder.id] = { + valid: !error, + error, + }; + } + setAuthenticator(authenticator: Awaited>) { this._authenticator = authenticator; } diff --git a/src/utils/policy-puller.ts b/src/utils/policy-puller.ts index 857e9ab..a297909 100644 --- a/src/utils/policy-puller.ts +++ b/src/utils/policy-puller.ts @@ -1,3 +1,4 @@ +import { rm } from 'fs/promises'; import { getWorkspaceFolders } from './workspace'; import { getSynchronizer } from './synchronization'; import logger from './logger'; @@ -67,13 +68,55 @@ export class PolicyPuller { try { const policy = await this._synchronizer.synchronize(folder.uri.fsPath, globals.user.token); logger.log('fetchPolicyFiles', policy); + globals.setFolderStatus(folder); } catch (error) { - logger.error(error); - // @TODO update status bar + const errorDetails = this.getErrorDetails(error); + + logger.error('fetchPolicyFiles', error, errorDetails); + + if (errorDetails.type === 'NO_PROJECT' || errorDetails.type === 'NO_POLICY') { + const outdatedPolicy = await this._synchronizer.getPolicy(folder.uri.fsPath); + + if (outdatedPolicy.path) { + await rm(outdatedPolicy.path, { force: true }); + } + } + + globals.setFolderStatus(folder, errorDetails.message); } } this._isPulling = false; this._pullPromise = undefined; } + + private getErrorDetails(err: any) { + const message = err.message || ''; + + let type = 'UNKNOWN'; + + if (message.length === 0) { + return { + type, + message: '' + }; + } + + if (message.includes('Cannot fetch user data')) { + type = 'NO_USER'; + } + + if (message.includes('does not belong to any project')) { + type = 'NO_PROJECT'; + } + + if (message.includes('does not have policy defined')) { + type = 'NO_POLICY'; + } + + return { + type, + message: `${type}: ${message}` + }; + } } diff --git a/src/utils/runtime-context.ts b/src/utils/runtime-context.ts index b79505c..5030b39 100644 --- a/src/utils/runtime-context.ts +++ b/src/utils/runtime-context.ts @@ -1,6 +1,6 @@ import { STATUS_BAR_TEXTS } from '../constants'; -import { getTooltipContent } from './tooltip'; +import { getTooltipData } from './tooltip'; import { getAuthenticator } from './authentication'; import { getSynchronizer } from './synchronization'; import type { Disposable, ExtensionContext, StatusBarItem } from 'vscode'; @@ -58,7 +58,9 @@ export class RuntimeContext { } async updateTooltip() { - this._statusBarItem.tooltip = await getTooltipContent(); + const tooltipData = await getTooltipData(); + this._statusBarItem.text = tooltipData.status === 'ok' ? STATUS_BAR_TEXTS.DEFAULT : STATUS_BAR_TEXTS.ERROR; + this._statusBarItem.tooltip = tooltipData.content; } registerDisposables(disposables: Disposable[]) { diff --git a/src/utils/tooltip.ts b/src/utils/tooltip.ts index 15acb76..0f323d5 100644 --- a/src/utils/tooltip.ts +++ b/src/utils/tooltip.ts @@ -1,7 +1,14 @@ import { MarkdownString } from 'vscode'; import { getWorkspaceConfig, getWorkspaceFolders } from './workspace'; import { getValidationResult } from './validation'; +import { COMMAND_NAMES } from '../constants'; import globals from './globals'; +import logger from './logger'; + +export type TooltipData = { + content: string | MarkdownString; + status: 'ok' | 'error'; +}; export function getTooltipContentDefault() { if (!globals.enabled) { @@ -11,18 +18,32 @@ export function getTooltipContentDefault() { return 'Monokle extension initializing...'; } -export async function getTooltipContent() { +export async function getTooltipData(): Promise { if (!globals.enabled) { - return 'Monokle extension disabled'; + return { + content: 'Monokle extension disabled', + status: 'ok', + }; } + let isStatusOk = true; + const folders = getWorkspaceFolders(); const folderList = await Promise.all(folders.map(async (folder) => { const config = await getWorkspaceConfig(folder); const configType = config.type === 'file' || config.type === 'config' ? 'local' : config.type; + const folderStatus = globals.getFolderStatus(folder); + const errorStatus = folderStatus?.error ?? ''; + const fixTip = getFixTip(errorStatus); + + logger.log('folderStatus', folderStatus, fixTip); + if (!config.isValid) { - return `**${folder.name}**: invalid ${configType} config`; + isStatusOk = false; + + return `**${folder.name}**: invalid ${configType} config` + + (fixTip ? `
_Fix suggestion_: ${fixTip}` : ''); } const validationResult = await getValidationResult(folder.id); @@ -46,8 +67,27 @@ export async function getTooltipContent() { activeUserText = `

Logged in as **${globals.user.email}**`; } - const content = new MarkdownString(`${folderList.join('
')}${activeUserText}

Show validation panel`); + const content = new MarkdownString(`${folderList.join('
')}${activeUserText}

Click to show validation panel`); content.supportHtml = true; - return content; -} \ No newline at end of file + return { + content, + status: isStatusOk ? 'ok' : 'error', + }; +} + +function getFixTip(err: string) { + if (err.startsWith('NO_USER')) { + return `Try logging again with _${COMMAND_NAMES.LOGIN}_ command.`; + } + + if (err.startsWith('NO_PROJECT')) { + return 'Add to project in Monokle Cloud.'; + } + + if (err.startsWith('NO_POLICY')) { + return 'Add policy in Monokle Cloud related project.'; + } + + return ''; +} diff --git a/src/utils/validation.ts b/src/utils/validation.ts index 2e10d4c..f2c4601 100644 --- a/src/utils/validation.ts +++ b/src/utils/validation.ts @@ -5,7 +5,7 @@ import { Uri } from 'vscode'; import { Document } from 'yaml'; import { getWorkspaceConfig, getWorkspaceResources } from './workspace'; import { VALIDATION_FILE_SUFFIX, DEFAULT_CONFIG_FILE_NAME, TMP_POLICY_FILE_SUFFIX } from '../constants'; -import { raiseInvalidConfigError } from './errors'; +import { getInvalidConfigError } from './errors'; import logger from '../utils/logger'; import globals from './globals'; import type { Folder } from './workspace'; @@ -67,11 +67,10 @@ export async function validateFolder(root: Folder): Promise { const workspaceConfig = await getWorkspaceConfig(root); if (workspaceConfig.isValid === false) { - if (workspaceConfig.type === 'remote') { - return null; // Error will be already shown by policy puller. + if (workspaceConfig.type !== 'remote') { + // For remote config, error is already send from policy puller. + globals.setFolderStatus(root, getInvalidConfigError(workspaceConfig)); } - - raiseInvalidConfigError(workspaceConfig, root); return null; } @@ -101,6 +100,8 @@ export async function validateFolder(root: Folder): Promise { logger.log(root.name, 'resultsFilePath', resultsFilePath); + globals.setFolderStatus(root); + return Uri.file(resultsFilePath); }