(core) Fix bugs when both welcomeTour and docTour are available

Summary:
- Unify where in the code tours get initiated.
- Avoid start a new tour while one is being started or is in progress.
- Ignore welcome tour when on a doc that has a doc tour.
- Fix tours when starting with a special page like Access Rules.
- Remove mention of the no-longer-present "Give Feedback" button in the last
  message of the welcome tour.

Test Plan:
Add a browser test case that docTour preempts the welcome tour and shows no errors
(this test case fails in multiple ways without the changes).

Reviewers: georgegevoian

Reviewed By: georgegevoian

Differential Revision: https://phab.getgrist.com/D3330

app/client/components/GristDoc.ts

@@ -35,11 +35,12 @@ import {DocPageModel} from 'app/client/models/DocPageModel';
 import {UserError} from 'app/client/models/errors';
 import {urlState} from 'app/client/models/gristUrlState';
 import {getFilterFunc, QuerySetManager} from 'app/client/models/QuerySet';
-import {getUserOrgPrefObs, getUserOrgPrefsObs} from 'app/client/models/UserPrefs';
+import {getUserOrgPrefObs, getUserOrgPrefsObs, markAsSeen} from 'app/client/models/UserPrefs';
 import {App} from 'app/client/ui/App';
 import {DocHistory} from 'app/client/ui/DocHistory';
 import {startDocTour} from "app/client/ui/DocTour";
 import {showDocSettingsModal} from 'app/client/ui/DocumentSettings';
+import {isTourActive} from "app/client/ui/OnBoardingPopups";
 import {IPageWidget, toPageWidget} from 'app/client/ui/PageWidgetPicker';
 import {IPageWidgetLink, linkFromId, selectBy} from 'app/client/ui/selectBy';
 import {startWelcomeTour} from 'app/client/ui/welcomeTour';
@@ -132,6 +133,9 @@ export class GristDoc extends DisposableWithEvents {
 
   public readonly userOrgPrefs = getUserOrgPrefsObs(this.docPageModel.appModel);
 
+  // If the doc has a docTour. Used also to enable the UI button to restart the tour.
+  public readonly hasDocTour: Computed<boolean>;
+
   private _actionLog: ActionLog;
   private _undoStack: UndoStack;
   private _lastOwnActionGroup: ActionGroupWithCursorPos|null = null;
@@ -140,6 +144,7 @@ export class GristDoc extends DisposableWithEvents {
   private _rightPanelTool = createSessionObs(this, "rightPanelTool", "none", RightPanelTool.guard);
   private _viewLayout: ViewLayout|null = null;
   private _showGristTour = getUserOrgPrefObs(this.userOrgPrefs, 'showGristTour');
+  private _seenDocTours = getUserOrgPrefObs(this.userOrgPrefs, 'seenDocTours');
 
   constructor(
     public readonly app: App,
@@ -162,6 +167,9 @@ export class GristDoc extends DisposableWithEvents {
     // Maintain the MetaRowModel for the global document info, including docId and peers.
     this.docInfo = this.docModel.docInfoRow;
 
+    this.hasDocTour = Computed.create(this, use =>
+      use(this.docModel.allTableIds.getObservable()).includes('GristDocTour'));
+
     const defaultViewId = this.docInfo.newDefaultViewId;
 
     // Grainjs observable for current view id, which may be a string such as 'code'.
@@ -217,24 +225,39 @@ export class GristDoc extends DisposableWithEvents {
     }));
 
     // Start welcome tour if flag is present in the url hash.
+    let tourStarting = false;
     this.autoDispose(subscribe(urlState().state, async (_use, state) => {
-      if (state.welcomeTour || state.docTour || this._shouldAutoStartWelcomeTour()) {
-        // On boarding tours were not designed with mobile support in mind. Disable until fixed.
-        if (isNarrowScreen()) {
-          return;
-        }
-        await this._waitForView();
-        await delay(0); // we need to wait an extra bit.
+      // Onboarding tours were not designed with mobile support in mind. Disable until fixed.
+      if (isNarrowScreen()) {
+        return;
+      }
+      // If we have an active tour (or are in the process of starting one), don't start a new one.
+      if (tourStarting || isTourActive()) {
+        return;
+      }
+      const autoStartDocTour = this.hasDocTour.get() && !this._seenDocTours.get()?.includes(this.docId());
+      const docTour = state.docTour || autoStartDocTour;
+      const welcomeTour = state.welcomeTour || this._shouldAutoStartWelcomeTour();
 
-        // Remove any tour-related hash-tags from the URL. So #repeat-welcome-tour and
-        // #repeat-doc-tour are used as triggers, but will immediately disappear.
-        await urlState().pushUrl({welcomeTour: false, docTour: false},
-          {replace: true, avoidReload: true});
+      if (welcomeTour || docTour) {
+        tourStarting = true;
+        try {
+          await this._waitForView();
+          await delay(0); // we need to wait an extra bit.
 
-        if (!state.docTour) {
-          startWelcomeTour(() => this._showGristTour.set(false));
-        } else {
-          await startDocTour(this.docData, this.docComm, () => null);
+          // Remove any tour-related hash-tags from the URL. So #repeat-welcome-tour and
+          // #repeat-doc-tour are used as triggers, but will immediately disappear.
+          await urlState().pushUrl({welcomeTour: false, docTour: false},
+            {replace: true, avoidReload: true});
+
+          if (!docTour) {
+            startWelcomeTour(() => this._showGristTour.set(false));
+          } else {
+            const onFinishCB = () => (autoStartDocTour && markAsSeen(this._seenDocTours, this.docId()));
+            await startDocTour(this.docData, this.docComm, onFinishCB);
+          }
+        } finally {
+          tourStarting = false;
         }
       }
     }));
@@ -801,6 +824,10 @@ export class GristDoc extends DisposableWithEvents {
    * Waits for a view to be ready
    */
   private async _waitForView() {
+    // For pages like ACL's, there isn't a view instance to wait for.
+    if (!this.viewModel.activeSection.peek().getRowId()) {
+      return;
+    }
     const view = await waitObs(this.viewModel.activeSection.peek().viewInstance);
     await view?.getLoadingDonePromise();
     return view;
@@ -929,7 +956,11 @@ export class GristDoc extends DisposableWithEvents {
    * For first-time users on personal org, start a welcome tour.
    */
   private _shouldAutoStartWelcomeTour(): boolean {
-    // TODO: decide what to do when both a docTour and grist welcome tour are available.
+    // When both a docTour and grist welcome tour are available, show only the docTour, leaving
+    // the welcome tour for another doc (e.g. a new one).
+    if (this.hasDocTour.get()) {
+      return false;
+    }
 
     // Only show the tour if one is on a personal org and can edit. This excludes templates (on
     // the Templates org, which may have their own tour) and team sites (where user's intended

app/client/models/UserPrefs.ts

@@ -58,3 +58,23 @@ function makePrefFunctions<P extends keyof PrefsTypes>(prefsTypeName: P) {
 
 export const {getPrefsObs: getUserOrgPrefsObs, getPrefObs: getUserOrgPrefObs} = makePrefFunctions('userOrgPrefs');
 export const {getPrefsObs: getUserPrefsObs, getPrefObs: getUserPrefObs} = makePrefFunctions('userPrefs');
+
+
+// For preferences that store a list of items (such as seen docTours), this helper updates the
+// preference to add itemId to it (e.g. to avoid auto-starting the docTour again in the future).
+// prefKey is used only to log a more informative warning on error.
+export function markAsSeen<T>(seenIdsObs: Observable<T[] | undefined>, itemId: T) {
+  const seenIds = seenIdsObs.get() || [];
+  try {
+    if (!seenIds.includes(itemId)) {
+      const seen = new Set(seenIds);
+      seen.add(itemId);
+      seenIdsObs.set([...seen].sort());
+    }
+  } catch (e) {
+    // If we fail to save this preference, it's probably not worth alerting the user about,
+    // so just log to console.
+    // tslint:disable-next-line:no-console
+    console.warn("Failed to save preference in markAsSeen", e);
+  }
+}

app/client/ui/OnBoardingPopups.ts

@@ -80,6 +80,11 @@ export function startOnBoarding(messages: IOnBoardingMsg[], onFinishCB: () => vo
   ctl.start().catch(reportError);
 }
 
+// Returns whether some tour is currently active.
+export function isTourActive(): boolean {
+  return !tourSingleton.isEmpty();
+}
+
 class OnBoardingError extends Error {
   public name = 'OnBoardingError';
   constructor(message: string) {

app/client/ui/Tools.ts

@@ -1,7 +1,6 @@
 import {GristDoc} from 'app/client/components/GristDoc';
-import {loadGristDoc} from 'app/client/lib/imports';
 import {urlState} from 'app/client/models/gristUrlState';
-import {getUserOrgPrefObs} from 'app/client/models/UserPrefs';
+import {getUserOrgPrefObs, markAsSeen} from 'app/client/models/UserPrefs';
 import {showExampleCard} from 'app/client/ui/ExampleCard';
 import {examples} from 'app/client/ui/ExampleInfo';
 import {createHelpTools, cssLinkText, cssPageEntry, cssPageEntryMain, cssPageEntrySmall,
@@ -14,7 +13,7 @@ import {cssLink} from 'app/client/ui2018/links';
 import {menuAnnotate} from 'app/client/ui2018/menus';
 import {confirmModal} from 'app/client/ui2018/modals';
 import {userOverrideParams} from 'app/common/gristUrls';
-import {Computed, Disposable, dom, makeTestId, Observable, observable, styled} from 'grainjs';
+import {Disposable, dom, makeTestId, Observable, observable, styled} from 'grainjs';
 
 const testId = makeTestId('test-tools-');
 
@@ -22,8 +21,6 @@ export function tools(owner: Disposable, gristDoc: GristDoc, leftPanelOpen: Obse
   const docPageModel = gristDoc.docPageModel;
   const isOwner = docPageModel.currentDoc.get()?.access === 'owners';
   const isOverridden = Boolean(docPageModel.userOverride.get());
-  const hasDocTour = Computed.create(owner, use =>
-    use(gristDoc.docModel.allTableIds.getObservable()).includes('GristDocTour'));
   const canViewAccessRules = observable(false);
   function updateCanViewAccessRules() {
     canViewAccessRules.set((isOwner && !isOverridden) ||
@@ -106,20 +103,12 @@ export function tools(owner: Disposable, gristDoc: GristDoc, leftPanelOpen: Obse
       );
     }),
     // Show the 'Tour of this Document' button if a GristDocTour table exists.
-    dom.maybe(hasDocTour, () =>
+    dom.maybe(gristDoc.hasDocTour, () =>
       cssSplitPageEntry(
         cssPageEntryMain(
           cssPageLink(cssPageIcon('Page'),
             cssLinkText('Tour of this Document'),
-            automaticHelpTool(
-              async ({markAsSeen}) => {
-                const gristDocModule = await loadGristDoc();
-                await gristDocModule.startDocTour(gristDoc.docData, gristDoc.docComm, markAsSeen);
-              },
-              gristDoc,
-              "seenDocTours",
-              gristDoc.docId()
-            ),
+            urlState().setLinkUrl({docTour: true}),
             testId('doctour'),
           ),
         ),
@@ -165,24 +154,7 @@ function automaticHelpTool(
       return;
     }
 
-    // When the help is closed, if it's the first time it's dismissed, save this fact, to avoid
-    // showing it automatically again in the future.
-    function markAsSeen() {
-      try {
-        if (!seenIds.includes(itemId)) {
-          const seen = new Set(seenIds);
-          seen.add(itemId);
-          prefObs.set([...seen].sort());
-        }
-      } catch (e) {
-        // If we fail to save this preference, it's probably not worth alerting the user about,
-        // so just log to console.
-        // tslint:disable-next-line:no-console
-        console.warn("Failed to save userPref " + prefKey, e);
-      }
-    }
-
-    showFunc({elem, reopen, markAsSeen});
+    showFunc({elem, reopen, markAsSeen: () => markAsSeen(prefObs, itemId)});
   }
 
   return [

app/client/ui/welcomeTour.ts

@@ -63,8 +63,7 @@ export const welcomeTour: IOnBoardingMsg[] = [
     selector: '.tour-help-center',
     title: 'Flying higher',
     body: () => [
-      dom('p', 'Use ', Key(GreyIcon('Help'), 'Help Center'), ' for documentation, videos, and tutorials.'),
-      dom('p', 'Use ', Key(GreyIcon('Feedback'), 'Give Feedback'), ' for issues or questions.'),
+      dom('p', 'Use ', Key(GreyIcon('Help'), 'Help Center'), ' for documentation or questions.'),
     ],
     placement: 'right',
   },