From 26766fd4abb1b79439596d897482046280b8e1ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Wed, 6 Oct 2021 17:50:29 +0200 Subject: [PATCH] (core) Removing error styles from user messages Summary: Removing error styles from user messages. Only unexpected errors are styled with red icon and border. Removing reportSuccess message - leaving it for another diff. Test Plan: manual tests Reviewers: paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D3063 --- app/client/components/DocComm.ts | 6 +++--- app/client/models/HomeModel.ts | 6 +++--- app/client/models/NotifyModel.ts | 18 +----------------- app/client/models/UserManagerModel.ts | 6 ++---- app/client/models/errors.ts | 25 ++++++++++++++++--------- 5 files changed, 25 insertions(+), 36 deletions(-) diff --git a/app/client/components/DocComm.ts b/app/client/components/DocComm.ts index 8a617b98..be25f8d4 100644 --- a/app/client/components/DocComm.ts +++ b/app/client/components/DocComm.ts @@ -1,5 +1,5 @@ import {Comm, CommMessage} from 'app/client/components/Comm'; -import {reportError, UserError} from 'app/client/models/errors'; +import {reportError, reportMessage} from 'app/client/models/errors'; import {Notifier} from 'app/client/models/NotifyModel'; import {ActionGroup} from 'app/common/ActionGroup'; import {ActiveDocAPI, ApplyUAOptions, ApplyUAResult} from 'app/common/ActiveDocAPI'; @@ -198,7 +198,7 @@ export class DocComm extends Disposable implements ActiveDocAPI { } private async _doForkDoc(): Promise { - reportError(new UserError('Preparing your copy...', {key: 'forking'})); + reportMessage('Preparing your copy...', {key: 'forking'}); const {urlId, docId} = await this.fork(); // TODO: may want to preserve linkParameters in call to openDoc. const openResponse = await this._comm.openDoc(docId); @@ -209,7 +209,7 @@ export class DocComm extends Disposable implements ActiveDocAPI { this._docId = docId; this._setOpenResponse(openResponse); this.changeUrlIdEmitter.emit(urlId); - reportError(new UserError('You are now editing your own copy', {key: 'forking'})); + reportMessage('You are now editing your own copy', {key: 'forking'}); } } diff --git a/app/client/models/HomeModel.ts b/app/client/models/HomeModel.ts index 75dcb5de..197551a6 100644 --- a/app/client/models/HomeModel.ts +++ b/app/client/models/HomeModel.ts @@ -4,7 +4,7 @@ import {HomePluginManager} from 'app/client/lib/HomePluginManager'; import {ImportSourceElement} from 'app/client/lib/ImportSourceElement'; import {localStorageObs} from 'app/client/lib/localStorageObs'; import {AppModel, reportError} from 'app/client/models/AppModel'; -import {UserError} from 'app/client/models/errors'; +import {reportMessage, UserError} from 'app/client/models/errors'; import {urlState} from 'app/client/models/gristUrlState'; import {ownerName} from 'app/client/models/WorkspaceInfo'; import {IHomePage} from 'app/common/gristUrls'; @@ -215,7 +215,7 @@ export class HomeModelImpl extends Disposable implements HomeModel, ViewSettings public async restoreWorkspace(ws: Workspace) { await this._app.api.undeleteWorkspace(ws.id); await this._updateWorkspaces(); - reportError(new UserError(`Workspace "${ws.name}" restored`)); + reportMessage(`Workspace "${ws.name}" restored`); } // Creates a new doc by calling the API, and returns its docId. @@ -242,7 +242,7 @@ export class HomeModelImpl extends Disposable implements HomeModel, ViewSettings public async restoreDoc(doc: Document): Promise { await this._app.api.undeleteDoc(doc.id); await this._updateWorkspaces(); - reportError(new UserError(`Document "${doc.name}" restored`)); + reportMessage(`Document "${doc.name}" restored`); } public async pinUnpinDoc(docId: string, pin: boolean): Promise { diff --git a/app/client/models/NotifyModel.ts b/app/client/models/NotifyModel.ts index 743436f3..a4e193fe 100644 --- a/app/client/models/NotifyModel.ts +++ b/app/client/models/NotifyModel.ts @@ -14,10 +14,9 @@ import {isNarrowScreenObs, testId} from 'app/client/ui2018/cssVars'; const maxAppErrors = 5; interface INotifier { + createUserMessage(message: string, options?: INotifyOptions): INotification; // If you are looking to report errors, please do that via reportError rather // than these methods so that we have a chance to send the error to our logs. - createUserError(message: string, options?: INotifyOptions): INotification; - createUserMessage(message: string, options?: INotifyOptions): INotification; createAppError(error: Error): void; createProgressIndicator(name: string, size: string, expireOnComplete: boolean): IProgress; @@ -217,21 +216,6 @@ export class Notifier extends Disposable implements INotifier { }; } - /** - * Creates a basic toast user error. By default, expires in 10 seconds. - * Takes an options objects to configure `expireSec` and `canUserClose`. - * Set `expireSec` to 0 to prevent expiration. - * - * If you are looking to report errors, please do that via reportError so - * that we have a chance to send the error to our logs. - */ - public createUserError(message: string, options: Partial = {}): INotification { - return this.createUserMessage(message, { - level: 'error', - ...options - }); - } - /** * Creates a basic toast notification. By default, expires in 10 seconds. * Takes an options objects to configure `expireSec` and `canUserClose`. diff --git a/app/client/models/UserManagerModel.ts b/app/client/models/UserManagerModel.ts index d7b9de92..1dee2e17 100644 --- a/app/client/models/UserManagerModel.ts +++ b/app/client/models/UserManagerModel.ts @@ -1,5 +1,5 @@ import {DocPageModel} from 'app/client/models/DocPageModel'; -import {reportError, UserError} from 'app/client/models/errors'; +import {reportWarning} from 'app/client/models/errors'; import {normalizeEmail} from 'app/common/emails'; import {GristLoadConfig} from 'app/common/gristUrls'; import * as roles from 'app/common/roles'; @@ -207,9 +207,7 @@ export class UserManagerModelImpl extends Disposable implements UserManagerModel if (m === this.publicMember && access === roles.EDITOR && this._docPageModel?.gristDoc.get()?.hasGranularAccessRules()) { access = roles.VIEWER; - reportError(new UserError( - 'Public "Editor" access is incompatible with Access Rules. Reduced to "Viewer".' - )); + reportWarning('Public "Editor" access is incompatible with Access Rules. Reduced to "Viewer".'); } if (!roles.isValidRole(access)) { throw new Error(`Cannot update user to invalid role ${access}`); diff --git a/app/client/models/errors.ts b/app/client/models/errors.ts index 5ea3ee1a..a322e029 100644 --- a/app/client/models/errors.ts +++ b/app/client/models/errors.ts @@ -47,28 +47,32 @@ export function getAppErrors(): string[] { export function reportMessage(msg: string, options?: Partial) { if (_notifier && !_notifier.isDisposed()) { _notifier.createUserMessage(msg, { - level : 'message', ...options }); } } /** - * Shows notification with green border and a tick icon. + * Shows warning toast notification (with yellow styling). + */ +export function reportWarning(msg: string, options?: Partial) { + reportMessage(msg, {level: 'warning', ...options}); +} + +/** + * Shows success toast notification (with green styling). */ export function reportSuccess(msg: string, options?: Partial) { - if (_notifier && !_notifier.isDisposed()) { - _notifier.createUserMessage(msg, { - level : 'success', - ...options - }); - } + reportMessage(msg, {level: 'success', ...options}); } /** * Report an error to the user using the global Notifier instance. If the argument is a UserError * or an error with a status in the 400 range, it indicates a user error. Otherwise, it's an * application error, which the user can report to us as a bug. + * + * Not all errors will be shown as an error toast, depending on the content of the error + * this function might show a simple toast message. */ export function reportError(err: Error|string): void { log.error(`ERROR:`, err); @@ -99,6 +103,7 @@ export function reportError(err: Error|string): void { options.title = "Add users as team members first"; options.actions = []; } + // Show the error as a message _notifier.createUserMessage(message, options); } else if (err.name === 'UserError' || (typeof status === 'number' && status >= 400 && status < 500)) { // This is explicitly a user error, or one in the "Client Error" range, so treat it as user @@ -108,10 +113,12 @@ export function reportError(err: Error|string): void { if (details && details.tips && details.tips.some(tip => tip.action === 'ask-for-help')) { options.actions = ['ask-for-help']; } - _notifier.createUserError(message, options); + _notifier.createUserMessage(message, options); } else if (err.name === 'NeedUpgradeError') { + // Show the error as a message _notifier.createUserMessage(err.message, {actions: ['upgrade'], key: 'NEED_UPGRADE'}); } else if (code === 'AUTH_NO_EDIT' || code === 'ACL_DENY') { + // Show the error as a message _notifier.createUserMessage(err.message, {key: code, memos: details?.memos}); } else { // If we don't recognize it, consider it an application error (bug) that the user should be