New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add metrics collection for opening issueish in pane or browser #1694

Merged
merged 8 commits into from Sep 19, 2018

Conversation

4 participants
@kuychaco
Member

kuychaco commented Sep 18, 2018

Description of the Change

There are currently six ways of viewing details for an issueish from our package (at least as far as I can tell... please let me know if you can think of another!). I have instrumented each of them and added a corresponding test.

Added the following events and metadata:

  • clicking an issueish link in an IssueishDetailView opens another IssueishDetailView in a new tab
    'open-issueish-in-pane', {from: 'issueish-link', target: 'new-tab'}
  • cmd-clicking an issueish link in an IssueishDetailView opens another IssueishDetailView in the current tab
    'open-issueish-in-pane', {from: 'issueish-link', target: 'current-tab'}
  • clicking an issueish item in the IssueishListView opens an IssueishDetailView in a new tab
    'open-issueish-in-pane', {from: 'issueish-list'}
  • pasting an issueish link in the OpenIssueishDialog opens an IssueishDetailView in a new tab
    'open-issueish-in-pane', {from: 'dialog'}
  • shift-clicking an issueish link in an IssueishDetailView opens the issueish page on dotcom
    'open-issueish-in-browser', {from: 'issueish-link'}
  • clicking the issueish reference in the header of the IssueishDetailView opens the corresponding issueish page on dotcom
    'open-issueish-in-browser', {from: 'issueish-header'}

In the process I also caught a bug -- cmd-clicking an issueish link used to throw the following error:

Uncaught TypeError: _issueishDetailItem2.default.opener is not a function Thrown From: [github](https://github.com/atom/github) package 0.19.0 At /Users/kuychaco/.atom/packages/github/lib/views/issueish-link.js:70 TypeError: _issueishDetailItem2.default.opener is not a function at openInNewTab (/packages/github/lib/views/issueish-link.js:70:37) at openIssueishLinkInNewTab (/packages/github/lib/views/issueish-link.js:40:5) at Object.handleClickEvent (/packages/github/lib/views/issueish-link.js:29:5) at BareGithubDotcomMarkdown.handleClick (/packages/github/lib/views/github-dotcom-markdown.js:101:18)

Alternate Designs

Rather than having separate events for viewing the issueish in a pane or browser, I considered using only one event type of open-issueish and adding target pane/browser information to the metadata. I decided to keep them separate because there is a clear distinction between the intention as well as the impact of the two types of events, namely that open-issueish-in-pane keeps you in Atom whereas open-issueish-in-browser takes you out of Atom.

Benefits

These changes will shed light on behavior when viewing PR and issue information, potentially inform decisions around product and project work, as well as help us assess the impact of future changes we make.

Here are some questions we might get insight into from collecting these metrics:

  • How frequently are people leaving Atom to view information in the browser?
    • How does this change as we add more GitHub information and features into Atom?
  • What entry points are being used to view issueish information?
  • Are people using the open issue dialog to specify a PR/issue to view? Currently you can only access this using a command github:open-issue-or-pull-request
  • Are there discoverability issues for any of the entry points listed above? What is the impact of taking steps to improve discoverability?
  • What are the usage statistics for the issue list view in the GitHub tab?
  • What are the usage statistics for viewing issues/PRs that are referenced as part of an issue/PR that is being viewed?

Possible Drawbacks

None that I can think of...

Applicable Issues

#1591

kuychaco added some commits Sep 14, 2018

Add event recording for opening an issueish in a pane or browser
Added the following events and metadata:

'open-issueish-in-pane', {from: 'issueish-link', target: 'current-tab'}
'open-issueish-in-pane', {from: 'issueish-link', target: 'new-tab'}
'open-issueish-in-pane', {from: 'issueish-list'}
'open-issueish-in-pane', {from: 'dialog'}
'open-issueish-in-browser', {from: 'issueish-link'}
'open-issueish-in-browser', {from: 'issueish-header'}
@annthurium

thanks for working on this, @kuychaco !

  • In general, thank you for being so thorough about your unit tests.

  • I like the extra params you used for addEvent - it makes it really clear where the user came from and where they're going which should be a boon for our analysts.

@@ -93,7 +94,9 @@ export default class IssueishSearchesController extends React.Component {
this.props.workingDirectory,
),
{pending: true, searchAllPanes: true},
);
).then(() => {

This comment has been minimized.

@annthurium

annthurium Sep 18, 2018

Contributor

Hmm...maybe the reporter should have an async helper for recording async events. 🤔

@annthurium

annthurium Sep 18, 2018

Contributor

Hmm...maybe the reporter should have an async helper for recording async events. 🤔

This comment has been minimized.

@kuychaco

kuychaco Sep 18, 2018

Member

Possibly! Personally, I think using addEvent like this is just fine -- it's clear and readable. That said, definitely open to trying it out and seeing how it feels / if there are additional benefits to using an async helper.

@kuychaco

kuychaco Sep 18, 2018

Member

Possibly! Personally, I think using addEvent like this is just fine -- it's clear and readable. That said, definitely open to trying it out and seeing how it feels / if there are additional benefits to using an async helper.

assert.isTrue(openLinkInBrowserStub.calledWith(href));
});
describe('opening issueish links', function() {

This comment has been minimized.

@annthurium

annthurium Sep 18, 2018

Contributor

would it make sense to add a unit test here to assert that if the link opening fails, we don't record an event?

@annthurium

annthurium Sep 18, 2018

Contributor

would it make sense to add a unit test here to assert that if the link opening fails, we don't record an event?

This comment has been minimized.

@kuychaco

kuychaco Sep 18, 2018

Member

done! 👍

@kuychaco

kuychaco Sep 18, 2018

Member

done! 👍

Feature Sprint : 27 August - 14 September 2018 : v0.20.0 automation moved this from In Progress 🔧 to QA Review 🔬 Sep 18, 2018

@kuychaco kuychaco moved this from QA Review 🔬 to In Progress 🔧 in Feature Sprint : 27 August - 14 September 2018 : v0.20.0 Sep 18, 2018

@smashwilson smashwilson added the metrics label Sep 18, 2018

kuychaco added some commits Sep 18, 2018

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 18, 2018

Coverage Status

Coverage increased (+0.3%) to 80.507% when pulling d883d53 on ku-pr-metrics into aec3bce on master.

coveralls commented Sep 18, 2018

Coverage Status

Coverage increased (+0.3%) to 80.507% when pulling d883d53 on ku-pr-metrics into aec3bce on master.

kuychaco added some commits Sep 18, 2018

@kuychaco kuychaco merged commit 315ffef into master Sep 19, 2018

6 checks passed

ci/circleci: beta Your tests passed on CircleCI!
Details
ci/circleci: dev Your tests passed on CircleCI!
Details
ci/circleci: stable Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 80.507%
Details

Feature Sprint : 27 August - 14 September 2018 : v0.20.0 automation moved this from In Progress 🔧 to Merged ☑️ Sep 19, 2018

@kuychaco kuychaco deleted the ku-pr-metrics branch Sep 19, 2018

@annthurium annthurium referenced this pull request Oct 20, 2018

Open

v0.21.0-0 QA Review #1752

0 of 11 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment