ref(assistant): Clean up code (#7907)

Make it easier to understand, fix some issues with analytics events

src/sentry/static/sentry/app/actionCreators/guides.jsx

@@ -1,5 +1,6 @@
 import {Client} from '../api';
 import GuideActions from '../actions/guideActions';
+import HookStore from '../stores/hookStore';
 
 const api = new Client();
 
@@ -24,11 +25,11 @@ export function nextStep() {
   GuideActions.nextStep();
 }
 
-export function closeGuide() {
-  GuideActions.closeGuide();
+export function closeGuideOrSupport() {
+  GuideActions.closeGuideOrSupport();
 }
 
-export function markUseful(guideId, useful) {
+export function recordFinish(guideId, useful) {
   api.request('/assistant/', {
     method: 'PUT',
     data: {
@@ -37,9 +38,15 @@ export function markUseful(guideId, useful) {
       useful,
     },
   });
+  HookStore.get('analytics:event').forEach(cb =>
+    cb('assistant.guide_finished', {
+      guide: guideId,
+      useful,
+    })
+  );
 }
 
-export function dismiss(guideId) {
+export function recordDismiss(guideId, step) {
   api.request('/assistant/', {
     method: 'PUT',
     data: {
@@ -47,4 +54,10 @@ export function dismiss(guideId) {
       status: 'dismissed',
     },
   });
+  HookStore.get('analytics:event').forEach(cb =>
+    cb('assistant.guide_dismissed', {
+      guide: guideId,
+      step,
+    })
+  );
 }

src/sentry/static/sentry/app/actions/guideActions.jsx

@@ -1,7 +1,7 @@
 import Reflux from 'reflux';
 
 let GuideActions = Reflux.createActions([
-  'closeGuide',
+  'closeGuideOrSupport',
   'fetchSucceeded',
   'nextStep',
   'registerAnchor',

src/sentry/static/sentry/app/components/assistant/guideDrawer.jsx

@@ -3,7 +3,7 @@ import React from 'react';
 import styled from 'react-emotion';
 import Button from '../buttons/button';
 import {t} from '../../locale';
-import {markUseful, nextStep} from '../../actionCreators/guides';
+import {recordFinish, nextStep} from '../../actionCreators/guides';
 import CueIcon from './cueIcon';
 import CloseIcon from './closeIcon';
 import AssistantContainer from './assistantContainer';
@@ -17,8 +17,8 @@ export default class GuideDrawer extends React.Component {
     onDismiss: PropTypes.func.isRequired,
   };
 
-  handleUseful = useful => {
-    markUseful(this.props.guide.id, useful);
+  handleFinish = useful => {
+    recordFinish(this.props.guide.id, useful);
     this.props.onFinish();
   };
 
@@ -55,7 +55,7 @@ export default class GuideDrawer extends React.Component {
                 <Button
                   priority="success"
                   size="small"
-                  onClick={() => this.handleUseful(true)}
+                  onClick={() => this.handleFinish(true)}
                 >
                   {t('Yes')} &nbsp; &#x2714;
                 </Button>
@@ -63,7 +63,7 @@ export default class GuideDrawer extends React.Component {
                   priority="success"
                   size="small"
                   style={{marginLeft: '0.25em'}}
-                  onClick={() => this.handleUseful(false)}
+                  onClick={() => this.handleFinish(false)}
                 >
                   {t('No')} &nbsp; &#x2716;
                 </Button>

src/sentry/static/sentry/app/components/assistant/helper.jsx

@@ -3,7 +3,12 @@ import Reflux from 'reflux';
 import createReactClass from 'create-react-class';
 import styled from 'react-emotion';
 import {t} from '../../locale';
-import {dismiss, closeGuide, fetchGuides, nextStep} from '../../actionCreators/guides';
+import {
+  closeGuideOrSupport,
+  fetchGuides,
+  nextStep,
+  recordDismiss,
+} from '../../actionCreators/guides';
 import SupportDrawer from './supportDrawer';
 import GuideDrawer from './guideDrawer';
 import GuideStore from '../../stores/guideStore';
@@ -20,13 +25,10 @@ const AssistantHelper = createReactClass({
 
   getInitialState() {
     return {
-      isDrawerOpen: false,
-      // currentGuide and currentStep are obtained from GuideStore.
-      // Even though this component doesn't need currentStep, it's
-      // child GuideDrawer does, and it's cleaner for only the parent
-      // to subscribe to GuideStore and pass down the guide and step,
-      // rather than have both parent and child subscribe to GuideStore.
       currentGuide: null,
+      // currentStep is applicable to the Need-Help button too. When currentGuide
+      // is null, if currentStep is 0 the Need-Help button is cued, and if it's > 0
+      // the support widget is open.
       currentStep: 0,
     };
   },
@@ -35,100 +37,63 @@ const AssistantHelper = createReactClass({
     fetchGuides();
   },
 
-  onGuideStateChange(data) {
-    let newState = {
-      currentGuide: data.currentGuide,
-      currentStep: data.currentStep,
-    };
-    if (this.state.currentGuide != data.currentGuide) {
-      newState.isDrawerOpen = false;
+  componentDidUpdate(prevProps, prevState) {
+    if (this.state.currentGuide && !prevState.currentGuide) {
+      HookStore.get('analytics:event').forEach(cb =>
+        cb('assistant.guide_cued', {
+          guide: this.state.currentGuide.id,
+          cue: this.state.currentGuide.cue,
+        })
+      );
     }
-    this.setState(newState);
-  },
-
-  handleDrawerOpen() {
-    let {currentGuide} = this.state;
-    this.setState({
-      isDrawerOpen: true,
-    });
-    nextStep();
-    HookStore.get('analytics:event').forEach(cb =>
-      cb('assistant.guide_opened', {
-        guide: currentGuide.id,
-        cue: currentGuide.cue,
-      })
-    );
   },
 
-  handleSupportDrawerClose() {
+  onGuideStateChange(data) {
     this.setState({
-      isDrawerOpen: false,
+      currentGuide: data.currentGuide,
+      currentStep: data.currentStep,
     });
   },
 
-  handleDismiss(e) {
-    dismiss(this.state.currentGuide.id);
-    closeGuide();
-    HookStore.get('analytics:event').forEach(cb =>
-      cb('assistant.guide_dismissed', {
-        guide: this.state.currentGuide.id,
-        cue: this.state.currentGuide.cue,
-        step: this.state.currentStep,
-      })
-    );
+  // Terminology:
+  // - A guide can be FINISHED by answering whether or not is was useful in the last step.
+  // - A guide can be DISMISSED by x-ing out of it at any time (including when it's cued).
+  // In both cases we consider it CLOSED.
+  handleGuideDismiss(e) {
+    e.stopPropagation();
+    recordDismiss(this.state.currentGuide.id, this.state.currentStep);
+    closeGuideOrSupport();
   },
 
   render() {
-    // isDrawerOpen and currentGuide/currentStep live in different places and are updated
-    // non-atomically. So we need to guard against the inconsistent state of the drawer
-    // being open and a guide being present, but currentStep not updated yet.
-    // If this gets too complicated, it would be better to move isDrawerOpen into
-    // GuideStore so we can update the state atomically in onGuideStateChange.
-    let showDrawer = false;
-    let {currentGuide, currentStep, isDrawerOpen} = this.state;
-    let isGuideCued = currentGuide !== null;
-
-    if (isGuideCued) {
-      HookStore.get('analytics:event').forEach(cb =>
-        cb('assistant.guide_cued', {
-          guide: currentGuide.id,
-          cue: currentGuide.cue,
-        })
-      );
-    }
-
-    const cueText = isGuideCued ? currentGuide.cue : t('Need Help?');
-    if (isDrawerOpen && (!isGuideCued || currentStep > 0)) {
-      showDrawer = true;
-    }
+    let {currentGuide, currentStep} = this.state;
+    const cueText = (currentGuide && currentGuide.cue) || t('Need Help?');
 
     return (
       <StyledHelper>
-        {showDrawer &&
-          this.state.currentGuide && (
+        {currentGuide !== null &&
+          currentStep > 0 && (
             <GuideDrawer
               guide={currentGuide}
               step={currentStep}
-              onFinish={closeGuide}
-              onDismiss={this.handleDismiss}
+              onFinish={closeGuideOrSupport}
+              onDismiss={this.handleGuideDismiss}
             />
           )}
 
-        {showDrawer &&
-          !this.state.currentGuide && (
-            <SupportDrawer onClose={this.handleSupportDrawerClose} />
-          )}
+        {currentGuide === null &&
+          currentStep > 0 && <SupportDrawer onClose={closeGuideOrSupport} />}
 
-        {!showDrawer && (
+        {!currentStep && (
           <StyledAssistantContainer
-            onClick={this.handleDrawerOpen}
+            onClick={nextStep}
             className="assistant-cue"
-            hasGuide={this.state.currentGuide}
+            hasGuide={currentGuide}
           >
-            <CueIcon hasGuide={this.state.currentGuide} />
-            <StyledCueText hasGuide={this.state.currentGuide}>{cueText}</StyledCueText>
-            {isGuideCued && (
-              <div style={{display: 'flex'}} onClick={this.handleDismiss}>
+            <CueIcon hasGuide={currentGuide} />
+            <StyledCueText hasGuide={currentGuide}>{cueText}</StyledCueText>
+            {currentGuide && (
+              <div style={{display: 'flex'}} onClick={this.handleGuideDismiss}>
                 <CloseIcon />
               </div>
             )}

src/sentry/static/sentry/app/components/assistant/supportDrawer.jsx

@@ -106,7 +106,7 @@ const SupportDrawer = createReactClass({
       <StyledAssistantContainer hasResults={hasResults}>
         <StyledAssistantInputRow>
           <CueIcon />
-          <StyledSearchContainer onSubmit={this.handleSubmit}>
+          <StyledSearchContainer>
             <StyledSearchIcon src="icon-search" />
             <StyledInput
               type="text"
@@ -164,7 +164,7 @@ const StyledSearchIcon = styled(InlineSvg)`
   color: ${p => p.theme.gray1};
 `;
 
-const StyledSearchContainer = styled('form')`
+const StyledSearchContainer = styled('div')`
   position: relative;
   margin-left: 0.5em;
   display: flex;

src/sentry/static/sentry/app/stores/guideStore.jsx

@@ -20,7 +20,7 @@ const GuideStore = Reflux.createStore({
       currentStep: 0,
     };
     this.listenTo(GuideActions.fetchSucceeded, this.onFetchSucceeded);
-    this.listenTo(GuideActions.closeGuide, this.onCloseGuide);
+    this.listenTo(GuideActions.closeGuideOrSupport, this.onCloseGuideOrSupport);
     this.listenTo(GuideActions.nextStep, this.onNextStep);
     this.listenTo(GuideActions.registerAnchor, this.onRegisterAnchor);
     this.listenTo(GuideActions.unregisterAnchor, this.onUnregisterAnchor);
@@ -31,27 +31,27 @@ const GuideStore = Reflux.createStore({
     this.updateCurrentGuide();
   },
 
-  onCloseGuide() {
-    let {currentGuide, guidesSeen} = this.state;
-    guidesSeen.add(currentGuide.id);
+  // This handles both closing a guide and the support drawer.
+  onCloseGuideOrSupport() {
+    let {currentGuide} = this.state;
+    if (currentGuide) {
+      this.state.guidesSeen.add(currentGuide.id);
+    }
     this.updateCurrentGuide();
-    HookStore.get('analytics:event').forEach(cb =>
-      cb('assistant.guide_closed', {
-        guide: currentGuide.id,
-        cue: currentGuide.cue,
-      })
-    );
   },
 
   onNextStep() {
     this.state.currentStep += 1;
     this.trigger(this.state);
-    HookStore.get('analytics:event').forEach(cb =>
-      cb('assistant.guide_next', {
-        guide: this.state.currentGuide.id,
-        cue: this.state.currentGuide.cue,
-      })
-    );
+    if (this.state.currentGuide) {
+      let eventName =
+        this.state.currentStep == 1 ? 'assistant.guide_opened' : 'assistant.guide_next';
+      HookStore.get('analytics:event').forEach(cb =>
+        cb(eventName, {
+          guide: this.state.currentGuide.id,
+        })
+      );
+    }
   },
 
   onRegisterAnchor(anchor) {
@@ -87,11 +87,9 @@ const GuideStore = Reflux.createStore({
       );
     }
 
-    if (bestGuide !== this.state.currentGuide) {
-      this.state.currentGuide = bestGuide;
-      this.state.currentStep = 0;
-      this.trigger(this.state);
-    }
+    this.state.currentGuide = bestGuide;
+    this.state.currentStep = 0;
+    this.trigger(this.state);
   },
 });
 

tests/js/spec/components/assistant/__snapshots__/helper.spec.jsx.snap

@@ -50,11 +50,3 @@ exports[`Helper renders guide drawer 1`] = `
   />
 </StyledHelper>
 `;
-
-exports[`Helper renders support drawer 1`] = `
-<StyledHelper>
-  <SupportDrawer
-    onClose={[Function]}
-  />
-</StyledHelper>
-`;

tests/js/spec/components/assistant/helper.spec.jsx

@@ -1,5 +1,8 @@
+import {ThemeProvider} from 'emotion-theming';
+import theme from 'app/utils/theme';
+
 import React from 'react';
-import {shallow} from 'enzyme';
+import {shallow, mount} from 'enzyme';
 import AssistantHelper from 'app/components/assistant/helper';
 
 describe('Helper', function() {
@@ -8,13 +11,19 @@ describe('Helper', function() {
     expect(wrapper).toMatchSnapshot();
   });
 
-  it('renders support drawer', function() {
-    let wrapper = shallow(<AssistantHelper />);
+  it('renders support drawer', async function() {
+    let wrapper = mount(
+      <ThemeProvider theme={theme}>
+        <AssistantHelper />
+      </ThemeProvider>
+    );
     wrapper
       .find('.assistant-cue')
       .first()
       .simulate('click');
-    expect(wrapper).toMatchSnapshot();
+    await tick();
+    wrapper.update();
+    expect(wrapper.find('SupportDrawer')).toHaveLength(1);
   });
 
   it('renders guide drawer', function() {

tests/js/spec/stores/guideStore.spec.jsx

@@ -52,7 +52,7 @@ describe('GuideStore', function() {
     expect(GuideStore.state.currentStep).toEqual(1);
     GuideStore.onNextStep();
     expect(GuideStore.state.currentStep).toEqual(2);
-    GuideStore.onCloseGuide();
+    GuideStore.onCloseGuideOrSupport();
     expect(GuideStore.state.guidesSeen).toEqual(new Set([1]));
   });
 });