diff --git a/app/client/ui/AccountPage.ts b/app/client/ui/AccountPage.ts index 6d1b1b2e..1a297711 100644 --- a/app/client/ui/AccountPage.ts +++ b/app/client/ui/AccountPage.ts @@ -7,7 +7,6 @@ import {MFAConfig} from 'app/client/ui/MFAConfig'; import {pagePanels} from 'app/client/ui/PagePanels'; import {createTopBarHome} from 'app/client/ui/TopBar'; import {transientInput} from 'app/client/ui/transientInput'; -import {buildNameWarningsDom, checkName} from 'app/client/ui/WelcomePage'; import {bigBasicButton, bigPrimaryButtonLink} from 'app/client/ui2018/buttons'; import {cssBreadcrumbs, cssBreadcrumbsLink, separator} from 'app/client/ui2018/breadcrumbs'; import {labeledSquareCheckbox} from 'app/client/ui2018/checkbox'; @@ -218,6 +217,31 @@ function confirmPwdResetModal(userEmail: string) { }); } +/** + * We allow alphanumeric characters and certain common whitelisted characters (except at the start), + * plus everything non-ASCII (for non-English alphabets, which we want to allow but it's hard to be + * more precise about what exactly to allow). + */ +// eslint-disable-next-line no-control-regex +const VALID_NAME_REGEXP = /^(\w|[^\u0000-\u007F])(\w|[- ./'"()]|[^\u0000-\u007F])*$/; + +/** + * Test name against various rules to check if it is a valid username. + */ +export function checkName(name: string): boolean { + return VALID_NAME_REGEXP.test(name); +} + +/** + * Builds dom to show marning messages to the user. + */ +function buildNameWarningsDom() { + return cssWarning( + "Names only allow letters, numbers and certain special characters", + testId('username-warning'), + ); +} + const cssContainer = styled('div', ` display: flex; justify-content: center; @@ -308,3 +332,7 @@ const cssEmail = styled('div', ` const cssLoginMethod = styled(cssFlexGrow, ` word-break: break-word; `); + +const cssWarning = styled('div', ` + color: red; +`); diff --git a/app/client/ui/WelcomePage.ts b/app/client/ui/WelcomePage.ts index 1df2f9da..f3c40935 100644 --- a/app/client/ui/WelcomePage.ts +++ b/app/client/ui/WelcomePage.ts @@ -1,4 +1,4 @@ -import { Computed, Disposable, dom, domComputed, DomContents, input, MultiHolder, Observable, styled } from "grainjs"; +import { Disposable, dom, domComputed, DomContents, input, MultiHolder, Observable, styled } from "grainjs"; import { handleSubmit, submitForm } from "app/client/lib/formUtils"; import { AppModel, reportError } from "app/client/models/AppModel"; @@ -6,12 +6,11 @@ import { getLoginUrl, getSignupUrl, urlState } from "app/client/models/gristUrlS import { AccountWidget } from "app/client/ui/AccountWidget"; import { AppHeader } from 'app/client/ui/AppHeader'; import * as BillingPageCss from "app/client/ui/BillingPageCss"; -import * as forms from "app/client/ui/forms"; import { pagePanels } from "app/client/ui/PagePanels"; import { createUserImage } from 'app/client/ui/UserImage'; import { cssMemberImage, cssMemberListItem, cssMemberPrimary, cssMemberSecondary, cssMemberText } from 'app/client/ui/UserItem'; -import { basicButtonLink, bigBasicButton, bigBasicButtonLink, bigPrimaryButton, bigPrimaryButtonLink, +import { basicButtonLink, bigBasicButtonLink, bigPrimaryButton, bigPrimaryButtonLink, cssButton } from "app/client/ui2018/buttons"; import { colors, mediaSmall, testId, vars } from "app/client/ui2018/cssVars"; import { getOrgName, Organization } from "app/common/UserAPI"; @@ -26,18 +25,9 @@ function _redirectToSiblingPage(name: string) { window.location.assign(url.href); } -// Redirect to result.redirectUrl is set, otherwise fail -function _redirectOnSuccess(result: any) { - const redirectUrl = result.redirectUrl; - if (!redirectUrl) { - throw new Error('form failed to redirect'); - } - window.location.assign(redirectUrl); -} - function handleSubmitForm( pending: Observable, - onSuccess: (v: any) => void = _redirectOnSuccess, + onSuccess: (v: any) => void, onError?: (e: unknown) => void ): (elem: HTMLFormElement) => void { return handleSubmit(pending, submitForm, onSuccess, onError); @@ -45,7 +35,6 @@ function handleSubmitForm( export class WelcomePage extends Disposable { - private _currentUserName = this._appModel.currentUser && this._appModel.currentUser.name || ''; private _orgs: Organization[]; private _orgsLoaded = Observable.create(this, false); @@ -75,8 +64,6 @@ export class WelcomePage extends Disposable { domComputed(urlState().state, (state) => ( state.welcome === 'signup' ? dom.create(this._buildSignupForm.bind(this)) : state.welcome === 'verify' ? dom.create(this._buildVerifyForm.bind(this)) : - state.welcome === 'user' ? dom.create(this._buildNameForm.bind(this)) : - state.welcome === 'info' ? dom.create(this._buildInfoForm.bind(this)) : state.welcome === 'teams' ? dom.create(this._buildOrgPicker.bind(this)) : state.welcome === 'select-account' ? dom.create(this._buildAccountPicker.bind(this)) : null @@ -84,39 +71,6 @@ export class WelcomePage extends Disposable { )); } - // TODO: This can be removed, since the 'user' page is no longer part of any flow. - private _buildNameForm(owner: MultiHolder) { - let inputEl: HTMLInputElement; - let form: HTMLFormElement; - const value = Observable.create(owner, checkName(this._currentUserName) ? this._currentUserName : ''); - const isNameValid = Computed.create(owner, value, (use, val) => checkName(val)); - const pending = Observable.create(owner, false); - - // delayed focus - setTimeout(() => inputEl.focus(), 10); - - return form = dom( - 'form', - { method: "post", action: location.href }, - handleSubmitForm(pending), - cssLabel('Your full name, as you\'d like it displayed to your collaborators.'), - inputEl = cssInput( - value, { onInput: true, }, - { name: "username" }, - // TODO: catch isn't needed, so either remove or propagate errors from _submitForm. - dom.onKeyDown({Enter: () => isNameValid.get() && form.requestSubmit()}), - ), - dom.maybe((use) => use(value) && !use(isNameValid), buildNameWarningsDom), - cssButtonGroup( - bigPrimaryButton( - 'Continue', - dom.boolAttr('disabled', (use) => Boolean(use(value) && !use(isNameValid)) || use(pending)), - testId('continue-button') - ), - ) - ); - } - private _buildSignupForm(owner: MultiHolder) { let inputEl: HTMLInputElement; const pending = Observable.create(owner, false); @@ -234,61 +188,6 @@ export class WelcomePage extends Disposable { ); } - /** - * Builds a form to ask the new user a few questions. - * - * TODO: This can be removed, since the 'info' page is no longer part of any flow. - */ - private _buildInfoForm(owner: MultiHolder) { - const allFilled = Observable.create(owner, false); - const whereObs = Observable.create(owner, ''); - const pending = Observable.create(owner, false); - - return forms.form({method: "post", action: location.href }, - handleSubmitForm(pending), - (elem) => { setTimeout(() => elem.focus(), 0); }, - forms.text('Please help us serve you better by answering a few questions.'), - forms.question( - forms.text('Where do you plan to use Grist?'), - forms.checkboxItem([{name: 'use_work'}], 'Work'), - forms.checkboxItem([{name: 'use_school'}], 'School'), - forms.checkboxItem([{name: 'use_personal'}], 'Personal'), - forms.textBox({name: 'company'}, - dom.hide(use => !use(whereObs)), - dom.attr('placeholder', (use) => use(whereObs) === 'school' ? 'Your School' : 'Your Company') - ), - ), - forms.question( - forms.text('What brings you to Grist?'), - forms.checkboxItem([{name: 'reason_problem'}], 'Solve a particular problem or need'), - forms.checkboxItem([{name: 'reason_tool'}], 'Find a better tool than the one I am using'), - forms.checkboxItem([{name: 'reason_curious'}], 'Just curious about a new product'), - forms.checkboxOther([{name: 'reason_other'}], {name: 'other_reason', placeholder: 'Other...'}), - ), - forms.question( - forms.text('What kind of industry do you work in?'), - forms.textBox({name: 'industry', placeholder: 'Your answer'}), - ), - forms.question( - forms.text('What is your role?'), - forms.textBox({name: 'role', placeholder: 'Your answer'}), - ), - dom.on('change', (e, form) => { - allFilled.set(forms.isFormFilled(form, ['use_*', 'reason_*', 'industry', 'role'])); - whereObs.set(form.use_work.checked ? 'work' : form.use_school.checked ? 'school' : ''); - }), - cssButtonGroup( - cssButtonGroup.cls('-right'), - bigBasicButton('Continue', - cssButton.cls('-primary', allFilled), - dom.boolAttr('disabled', pending), - {tabIndex: '0'}, - testId('continue-button')), - ), - testId('info-form'), - ); - } - private async _fetchOrgs() { this._orgs = await this._appModel.api.getOrgs(true); this._orgsLoaded.set(true); @@ -412,7 +311,6 @@ const textStyle = ` color: ${colors.dark}; `; -const cssLabel = styled('label', textStyle); // TODO: there's probably a much better way to style labels with a bit of // space between them and things they are not the label for? const cssSeparatedLabel = styled('label', textStyle + ' margin-top: 20px;'); @@ -427,10 +325,6 @@ const cssButtonGroup = styled('div', ` } `); -const cssWarning = styled('div', ` - color: red; -`); - const cssInput = styled(input, BillingPageCss.inputStyle); const cssOrgButton = styled(bigPrimaryButtonLink, ` @@ -444,29 +338,3 @@ const cssOrgButton = styled(bigPrimaryButtonLink, ` margin-top: 16px; } `); - -/** - * We allow alphanumeric characters and certain common whitelisted characters (except at the start), - * plus everything non-ASCII (for non-English alphabets, which we want to allow but it's hard to be - * more precise about what exactly to allow). - */ -// eslint-disable-next-line no-control-regex -const VALID_NAME_REGEXP = /^(\w|[^\u0000-\u007F])(\w|[- ./'"()]|[^\u0000-\u007F])*$/; - -/** - * Test name against various rules to check if it is a valid username. Returned obj has `.valid` set - * to true if all passes, otherwise it has the `.flag` set the the first failing test. - */ -export function checkName(name: string): boolean { - return VALID_NAME_REGEXP.test(name); -} - -/** - * Builds dom to show marning messages to the user. - */ -export function buildNameWarningsDom() { - return cssWarning( - "Names only allow letters, numbers and certain special characters", - testId('username-warning'), - ); -} diff --git a/app/client/ui/WelcomeQuestions.ts b/app/client/ui/WelcomeQuestions.ts index 4adc17e9..58b66931 100644 --- a/app/client/ui/WelcomeQuestions.ts +++ b/app/client/ui/WelcomeQuestions.ts @@ -25,7 +25,7 @@ export function showWelcomeQuestions(userPrefsObs: Observable) { const submitUrl = new URL(window.location.href); submitUrl.pathname = '/welcome/info'; - return BaseAPI.requestJson(submitUrl.href, + return BaseAPI.request(submitUrl.href, {method: 'POST', body: JSON.stringify({use_cases, use_other})}); } diff --git a/app/common/BaseAPI.ts b/app/common/BaseAPI.ts index 063355ae..0ac910c4 100644 --- a/app/common/BaseAPI.ts +++ b/app/common/BaseAPI.ts @@ -37,12 +37,18 @@ export class BaseAPI { }; } - // Make a JSON request to the given URL, and read the esponse as JSON. Handles errors, and + // Make a JSON request to the given URL, and read the response as JSON. Handles errors, and // counts pending requests in the same way as BaseAPI methods do. public static requestJson(url: string, init: RequestInit = {}): Promise { return new BaseAPI().requestJson(url, init); } + // Make a request to the given URL, and read the response. Handles errors, and + // counts pending requests in the same way as BaseAPI methods do. + public static request(url: string, init: RequestInit = {}): Promise { + return new BaseAPI().request(url, init); + } + private static _numPendingRequests: number = 0; protected fetch: typeof fetch; diff --git a/app/common/gristUrls.ts b/app/common/gristUrls.ts index 0e36f482..2623df63 100644 --- a/app/common/gristUrls.ts +++ b/app/common/gristUrls.ts @@ -20,7 +20,7 @@ export const HomePage = StringUnion('all', 'workspace', 'templates', 'trash'); export type IHomePage = typeof HomePage.type; // TODO: Remove 'user' and 'info', since those pages are no longer part of any flow. -export const WelcomePage = StringUnion('user', 'info', 'teams', 'signup', 'verify', 'select-account'); +export const WelcomePage = StringUnion('teams', 'signup', 'verify', 'select-account'); export type WelcomePage = typeof WelcomePage.type; export const AccountPage = StringUnion('account'); @@ -286,7 +286,7 @@ export function decodeUrl(gristConfig: Partial, location: Locat if (map.has('m')) { state.mode = OpenDocMode.parse(map.get('m')); } if (map.has('account')) { state.account = AccountPage.parse(map.get('account')) || 'account'; } if (map.has('billing')) { state.billing = BillingSubPage.parse(map.get('billing')) || 'billing'; } - if (map.has('welcome')) { state.welcome = WelcomePage.parse(map.get('welcome')) || 'user'; } + if (map.has('welcome')) { state.welcome = WelcomePage.parse(map.get('welcome')); } if (sp.has('billingPlan')) { state.params!.billingPlan = sp.get('billingPlan')!; } if (sp.has('billingTask')) { state.params!.billingTask = BillingTask.parse(sp.get('billingTask')); diff --git a/app/gen-server/lib/HomeDBManager.ts b/app/gen-server/lib/HomeDBManager.ts index 63778743..800e75db 100644 --- a/app/gen-server/lib/HomeDBManager.ts +++ b/app/gen-server/lib/HomeDBManager.ts @@ -428,9 +428,7 @@ export class HomeDBManager extends EventEmitter { needsSave = true; // If we are turning off the isFirstTimeUser flag, then right // after this transaction commits is a great time to trigger - // any automation for first logins - the user has logged in - // and gone through the welcome process (so they've had a - // chance to set a name) + // any automation for first logins if (!props.isFirstTimeUser) { isWelcomed = true; } } if (needsSave) { diff --git a/app/server/lib/FlexServer.ts b/app/server/lib/FlexServer.ts index cbe5c774..d5c852a1 100644 --- a/app/server/lib/FlexServer.ts +++ b/app/server/lib/FlexServer.ts @@ -1135,69 +1135,22 @@ export class FlexServer implements GristServer { this._redirectToLoginWithoutExceptionsMiddleware, ]; - // These are some special-purpose pre-sign-up welcome pages, with no middleware. - this.app.get(['/welcome/signup', '/welcome/verify'], expressWrap(async (req, resp, next) => { + // These are some special-purpose welcome pages, with no middleware. + this.app.get(/\/welcome\/(signup|verify|teams|select-account)/, expressWrap(async (req, resp, next) => { return this._sendAppPage(req, resp, {path: 'app.html', status: 200, config: {}, googleTagManager: true}); })); - /** - * TODO: Add '/welcome/teams' and '/welcome/select-account' to the above route handler, - * and remove this one, since those are the only welcome paths that are part of current UI flows. - */ - this.app.get('/welcome/:page', ...middleware, expressWrap(async (req, resp, next) => { - return this._sendAppPage(req, resp, {path: 'app.html', status: 200, config: {}, googleTagManager: true}); - })); - - /** - * TODO: We should now be able to remove this route, since the only remaining welcome path - * is GET /welcome/teams, and we already redirect there from the welcomeNewUser middleware. - * - * Leaving this alone for now, just in case, but we should remove this soon. - */ - this.app.post('/welcome/:page', ...middleware, expressWrap(async (req, resp, next) => { - const mreq = req as RequestWithLogin; + this.app.post('/welcome/info', ...middleware, expressWrap(async (req, resp, next) => { const userId = getUserId(req); - const domain = mreq.org; - let redirectPath: string = '/'; - - if (req.params.page === 'user') { - // The /welcome/user page is no longer part of any flow, but if visited, will still submit - // here and redirect. The full name is now part of the sign-up page, so we no longer - // need to prompt new users for their name here. - const name: string|undefined = req.body && req.body.username || undefined; - - // Reset isFirstTimeUser flag, used to redirect a new user to the /welcome/user page. - await this._dbManager.updateUser(userId, {name, isFirstTimeUser: false}); - - // This is a good time to set another flag (showNewUserQuestions), to show a popup with - // welcome question(s) to this new user. Both flags are scoped to the user, but - // isFirstTimeUser has a dedicated DB field because it predates userPrefs. Note that the - // updateOrg() method handles all levels of prefs (for user, user+org, or org). - await this._dbManager.updateOrg(getScope(req), 0, {userPrefs: {showNewUserQuestions: true}}); - - } else if (req.params.page === 'info') { - // The /welcome/info page is no longer part of any flow, but if visited, will still submit - // here and redirect. The new form with new-user questions appears in a modal popup. It - // also posts here to save answers, but ignores the response. - const user = getUser(req); - const row = {...req.body, UserID: userId, Name: user.name, Email: user.loginEmail}; - this._recordNewUserInfo(row) - .catch(e => { - // If we failed to record, at least log the data, so we could potentially recover it. - log.rawWarn(`Failed to record new user info: ${e.message}`, {newUserQuestions: row}); - }); - } - - // redirect to teams page if users has access to more than one org. Otherwise redirect to - // personal org. - const result = await this._dbManager.getMergedOrgs(userId, userId, domain || null); - const orgs = (result.status === 200) ? result.data : null; - if (orgs && orgs.length > 1) { - redirectPath = '/welcome/teams'; - } + const user = getUser(req); + const row = {...req.body, UserID: userId, Name: user.name, Email: user.loginEmail}; + this._recordNewUserInfo(row) + .catch(e => { + // If we failed to record, at least log the data, so we could potentially recover it. + log.rawWarn(`Failed to record new user info: ${e.message}`, {newUserQuestions: row}); + }); - const redirectUrl = this.getMergedOrgUrl(mreq, redirectPath); - resp.json({redirectUrl}); + resp.status(200).send(); }), // Add a final error handler that reports errors as JSON. jsonErrorHandler); diff --git a/test/nbrowser/Smoke.ts b/test/nbrowser/Smoke.ts index c621e574..c4c27310 100644 --- a/test/nbrowser/Smoke.ts +++ b/test/nbrowser/Smoke.ts @@ -14,10 +14,6 @@ async function openMainPage() { await driver.get(`${server.getHost()}`); while (true) { // eslint-disable-line no-constant-condition try { - const url = await driver.getCurrentUrl(); - if (url.match(/welcome\//)) { - await driver.findContent('button', /Continue/).click(); - } if (await driver.findContent('button', /Create Empty Document/).isPresent()) { return; }