From ac04a7110df84e42ad64809a4d444ebf8982a4b7 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 18 May 2018 15:42:18 -0400 Subject: [PATCH 01/64] Return canned GraphQL query responses in spec mode --- lib/relay-network-layer-manager.js | 59 +++++++++++++++++++++++++++--- test/helpers.js | 5 ++- 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/lib/relay-network-layer-manager.js b/lib/relay-network-layer-manager.js index 1ae291dcaa..0952c3eb35 100644 --- a/lib/relay-network-layer-manager.js +++ b/lib/relay-network-layer-manager.js @@ -13,11 +13,46 @@ function logRatelimitApi(headers) { console.debug(`GitHub API Rate Limit: ${remaining}/${total} — resets ${resetsIn}`); } -const tokenPerEnvironmentUrl = new Map(); +const responsesByQuery = new Map(); + +export function expectRelayQuery(operationPattern, response) { + let resolve, reject; + const promise = new Promise((resolve0, reject0) => { + resolve = () => resolve0({data: response}); + reject = reject0; + }); + + responsesByQuery.set(operationPattern.name, {promise, response, variables: operationPattern.variables || {}}); + + return {promise, resolve, reject}; +} + +export function clearRelayExpectations() { + responsesByQuery.clear(); +} + +const tokenPerURL = new Map(); +const fetchPerURL = new Map(); function createFetchQuery(url) { + if (atom.inSpecMode()) { + return function specFetchQuery(operation, variables, cacheConfig, uploadables) { + const expectation = responsesByQuery.get(operation.name); + if (!expectation) { + // eslint-disable-next-line no-console + console.log(`GraphQL query ${operation.name} was:\n ${operation.text.replace(/\n/g, '\n ')}`); + + const e = new Error(`Unexpected GraphQL query: ${operation.name}`); + e.rawStack = e.stack; + throw e; + } + return expectation.promise; + }; + } + return function fetchQuery(operation, variables, cacheConfig, uploadables) { - const currentToken = tokenPerEnvironmentUrl.get(url); + const currentToken = tokenPerURL.get(url); + return fetch(url, { method: 'POST', headers: { @@ -43,17 +78,29 @@ export default class RelayNetworkLayerManager { static getEnvironmentForHost(host, token) { host = host === 'github.com' ? 'https://api.github.com' : host; const url = host === 'https://api.github.com' ? `${host}/graphql` : `${host}/api/v3/graphql`; - const config = relayEnvironmentPerGithubHost.get(host) || {}; - let {environment, network} = config; - tokenPerEnvironmentUrl.set(url, token); + let {environment, network} = relayEnvironmentPerGithubHost.get(host) || {}; + tokenPerURL.set(url, token); if (!environment) { const source = new RecordSource(); const store = new Store(source); - network = Network.create(createFetchQuery(url)); + network = Network.create(this.getExistingFetchQuery(url)); environment = new Environment({network, store}); relayEnvironmentPerGithubHost.set(host, {environment, network}); } return environment; } + + static getExistingFetchQuery(url) { + if (!tokenPerURL.has(url)) { + return null; + } + + let fetch = fetchPerURL.get(url); + if (!fetch) { + fetch = createFetchQuery(url); + fetchPerURL.set(fetch); + } + return fetch; + } } diff --git a/test/helpers.js b/test/helpers.js index 9ae40fa868..eedbeae74c 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -7,13 +7,14 @@ import transpiler from 'atom-babel6-transpiler'; import React from 'react'; import ReactDom from 'react-dom'; import sinon from 'sinon'; +import {Directory} from 'atom'; import Repository from '../lib/models/repository'; import GitShellOutStrategy from '../lib/git-shell-out-strategy'; import WorkerManager from '../lib/worker-manager'; import ContextMenuInterceptor from '../lib/context-menu-interceptor'; import getRepoPipelineManager from '../lib/get-repo-pipeline-manager'; -import {Directory} from 'atom'; +import {clearRelayExpectations} from '../lib/relay-network-layer-manager'; assert.autocrlfEqual = (actual, expected, ...args) => { const newActual = actual.replace(/\r\n/g, '\n'); @@ -248,6 +249,8 @@ afterEach(function() { ContextMenuInterceptor.dispose(); global.sinon.restore(); + + clearRelayExpectations(); }); // eslint-disable-next-line jasmine/no-global-setup From 88c48ffde4d98fc05be71966a0bab98320e1511e Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 18 May 2018 15:43:02 -0400 Subject: [PATCH 02/64] Spec for the simple case of loading mentionable users --- test/models/user-store.test.js | 44 ++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index b6653999db..c9605bffb3 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -1,11 +1,11 @@ import dedent from 'dedent-js'; import UserStore, {NO_REPLY_GITHUB_EMAIL} from '../../lib/models/user-store'; - +import RelayNetworkLayerManager, {expectRelayQuery} from '../../lib/relay-network-layer-manager'; import {cloneRepository, buildRepository, FAKE_USER} from '../helpers'; describe('UserStore', function() { - it('loads store with users and committer in repo upon construction', async function() { + it('loads store with local git users and committer in a repo with no GitHub remote', async function() { const workdirPath = await cloneRepository('multiple-commits'); const repository = await buildRepository(workdirPath); const store = new UserStore({repository}); @@ -23,6 +23,46 @@ describe('UserStore', function() { await assert.async.deepEqual(store.committer, FAKE_USER); }); + it('loads store with mentionable users from the GitHub API in a repo with a GitHub remote', async function() { + const workdirPath = await cloneRepository('multiple-commits'); + const repository = await buildRepository(workdirPath); + + await repository.setConfig('remote.origin.url', 'git@github.com:me/stuff.git'); + await repository.setConfig('remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*'); + await repository.setConfig('remote.old.url', 'git@sourceforge.com:me/stuff.git'); + await repository.setConfig('remote.old.fetch', '+refs/heads/*:refs/remotes/old/*'); + + const {resolve, promise} = expectRelayQuery({name: 'MentionableUserQuery'}, { + repository: { + mentionableUsers: { + nodes: [ + {login: 'kuychaco', email: 'kuychaco@github.com', name: 'Katrina Uychaco'}, + {login: 'smashwilson', email: 'smashwilson@github.com', name: 'Ash Wilson'}, + {login: 'octocat', email: 'mona@lisa.com', name: 'Mona Lisa'}, + ], + pageInfo: { + hasNextPage: false, + endCursor: null, + }, + }, + }, + }); + + const store = new UserStore({repository}); + assert.deepEqual(store.getUsers(), []); + + resolve(); + await promise; + + assert.deepEqual(store.getUsers(), [ + {login: 'kuychaco', email: 'kuychaco@github.com', name: 'Katrina Uychaco'}, + {login: 'smashwilson', email: 'smashwilson@github.com', name: 'Ash Wilson'}, + {login: 'octocat', email: 'mona@lisa.com', name: 'Mona Lisa'}, + ]); + }); + + it('infers no-reply emails for users without a public email address'); + it('excludes committer and no reply user from `getUsers`', async function() { const workdirPath = await cloneRepository('multiple-commits'); const repository = await buildRepository(workdirPath); From 92b063f4f2a7723089e12690aaa42b1a707ce427 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 21 May 2018 10:45:19 -0400 Subject: [PATCH 03/64] Load users from GraphQL if a GitHub remote and token are present --- lib/models/user-store.js | 68 +++++++++++++++++++++++++++++----- test/models/user-store.test.js | 13 ++++--- 2 files changed, 65 insertions(+), 16 deletions(-) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index dd1b5d9374..f2ac51a6b1 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -1,3 +1,5 @@ +import RelayNetworkLayerManager from '../relay-network-layer-manager'; + // This is a guess about what a reasonable value is. Can adjust if performance is poor. const MAX_COMMITS = 5000; @@ -28,22 +30,29 @@ export default class UserStore { } } - loadUsers() { - this.loadUsersFromLocalRepo(); - } - - async loadUsersFromLocalRepo() { - const users = await this.repository.getAuthors({max: MAX_COMMITS}); + async loadUsers() { const committer = await this.repository.getCommitter(); this.setCommitter(committer); + + const githubRemotes = (await this.repository.getRemotes()).filter(remote => remote.isGithubRepo()); + const users = githubRemotes.length === 0 + ? await this.loadUsersFromLocalRepo() + : await this.loadUsersFromGraphQL(githubRemotes); + this.addUsers(users); this.didUpdate(); } - addUsersFromGraphQL(response) { - // TODO: [ku 3/2018] also get users from GraphQL API if available. Will need to reshape the data accordingly - // This will get called in relay query renderer callback - // this.addUsers(users); + loadUsersFromLocalRepo() { + return this.repository.getAuthors({max: MAX_COMMITS}); + } + + async loadUsersFromGraphQL(remotes) { + const mentionableUsers = await Promise.all(remotes.map(remote => getMentionableUsers(remote))); + return mentionableUsers.reduce((acc, list) => { + acc = {...acc, ...list}; + return acc; + }, {}); } addUsers(users) { @@ -77,3 +86,42 @@ export default class UserStore { }); } } + +async function getMentionableUsers(remote) { + const fetchQuery = RelayNetworkLayerManager.getExistingFetchQuery('https://api.github.com/graphql'); + if (!fetchQuery) { + // No authentication token + return []; + } + + const response = await fetchQuery({ + name: 'GetMentionableUsers', + text: ` + query GetMentionableUsers { + repository(owner: $owner, name: $name) { + mentionableUsers(first: $first, after: $after) { + nodes { + login + email + name + } + pageInfo { + hasNextPage + endCursor + } + } + } + } + `, + }, { + owner: remote.getOwner(), + name: remote.getRepo(), + first: 100, + after: null, + }); + + return response.data.repository.mentionableUsers.nodes.reduce((acc, node) => { + acc[node.email] = node.name; + return acc; + }, {}); +} diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index c9605bffb3..b54cf33f23 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -24,6 +24,8 @@ describe('UserStore', function() { }); it('loads store with mentionable users from the GitHub API in a repo with a GitHub remote', async function() { + RelayNetworkLayerManager.getEnvironmentForHost('https://api.github.com', '1234'); + const workdirPath = await cloneRepository('multiple-commits'); const repository = await buildRepository(workdirPath); @@ -32,7 +34,7 @@ describe('UserStore', function() { await repository.setConfig('remote.old.url', 'git@sourceforge.com:me/stuff.git'); await repository.setConfig('remote.old.fetch', '+refs/heads/*:refs/remotes/old/*'); - const {resolve, promise} = expectRelayQuery({name: 'MentionableUserQuery'}, { + const {resolve} = expectRelayQuery({name: 'GetMentionableUsers'}, { repository: { mentionableUsers: { nodes: [ @@ -52,12 +54,11 @@ describe('UserStore', function() { assert.deepEqual(store.getUsers(), []); resolve(); - await promise; - assert.deepEqual(store.getUsers(), [ - {login: 'kuychaco', email: 'kuychaco@github.com', name: 'Katrina Uychaco'}, - {login: 'smashwilson', email: 'smashwilson@github.com', name: 'Ash Wilson'}, - {login: 'octocat', email: 'mona@lisa.com', name: 'Mona Lisa'}, + await assert.async.deepEqual(store.getUsers(), [ + {email: 'smashwilson@github.com', name: 'Ash Wilson'}, + {email: 'kuychaco@github.com', name: 'Katrina Uychaco'}, + {email: 'mona@lisa.com', name: 'Mona Lisa'}, ]); }); From 5bea3addb8712f1f184b04dc341312fa9fc3469b Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 21 May 2018 10:51:34 -0400 Subject: [PATCH 04/64] Assert against mentionable users that aren't also in git history --- test/models/user-store.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index b54cf33f23..dfe9c59f10 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -38,9 +38,9 @@ describe('UserStore', function() { repository: { mentionableUsers: { nodes: [ - {login: 'kuychaco', email: 'kuychaco@github.com', name: 'Katrina Uychaco'}, - {login: 'smashwilson', email: 'smashwilson@github.com', name: 'Ash Wilson'}, + {login: 'annthurium', email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, {login: 'octocat', email: 'mona@lisa.com', name: 'Mona Lisa'}, + {login: 'smashwilson', email: 'smashwilson@github.com', name: 'Ash Wilson'}, ], pageInfo: { hasNextPage: false, @@ -57,8 +57,8 @@ describe('UserStore', function() { await assert.async.deepEqual(store.getUsers(), [ {email: 'smashwilson@github.com', name: 'Ash Wilson'}, - {email: 'kuychaco@github.com', name: 'Katrina Uychaco'}, {email: 'mona@lisa.com', name: 'Mona Lisa'}, + {email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, ]); }); From 5f850b4266da367a60c1f993c537037447753c25 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 21 May 2018 14:11:30 -0400 Subject: [PATCH 05/64] Match expected GraphQL queries by variables --- lib/relay-network-layer-manager.js | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/lib/relay-network-layer-manager.js b/lib/relay-network-layer-manager.js index 0952c3eb35..f63f74b92c 100644 --- a/lib/relay-network-layer-manager.js +++ b/lib/relay-network-layer-manager.js @@ -22,7 +22,9 @@ export function expectRelayQuery(operationPattern, response) { reject = reject0; }); - responsesByQuery.set(operationPattern.name, {promise, response, variables: operationPattern.variables || {}}); + const existing = responsesByQuery.get(operationPattern.name) || []; + existing.push({promise, response, variables: operationPattern.variables || {}}); + responsesByQuery.set(operationPattern.name, existing); return {promise, resolve, reject}; } @@ -37,8 +39,22 @@ const fetchPerURL = new Map(); function createFetchQuery(url) { if (atom.inSpecMode()) { return function specFetchQuery(operation, variables, cacheConfig, uploadables) { - const expectation = responsesByQuery.get(operation.name); - if (!expectation) { + const expectations = responsesByQuery.get(operation.name) || []; + const match = expectations.find(expectation => { + if (Object.keys(expectation.variables).length !== Object.keys(variables).length) { + return false; + } + + for (const key in expectation.variables) { + if (expectation.variables[key] !== variables[key]) { + return false; + } + } + + return true; + }); + + if (!match) { // eslint-disable-next-line no-console console.log(`GraphQL query ${operation.name} was:\n ${operation.text.replace(/\n/g, '\n ')}`); @@ -46,7 +62,8 @@ function createFetchQuery(url) { e.rawStack = e.stack; throw e; } - return expectation.promise; + + return match.promise; }; } From 350ff6e4b51ff188c0f9127fff79090d3538e9f3 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 21 May 2018 14:14:10 -0400 Subject: [PATCH 06/64] Fetch multiple pages of mentionable users --- lib/models/user-store.js | 81 +++++++++++++++++++--------------- test/models/user-store.test.js | 73 +++++++++++++++++++++++++++++- 2 files changed, 118 insertions(+), 36 deletions(-) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index f2ac51a6b1..7ef35c4768 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -47,12 +47,13 @@ export default class UserStore { return this.repository.getAuthors({max: MAX_COMMITS}); } - async loadUsersFromGraphQL(remotes) { - const mentionableUsers = await Promise.all(remotes.map(remote => getMentionableUsers(remote))); - return mentionableUsers.reduce((acc, list) => { - acc = {...acc, ...list}; - return acc; - }, {}); + loadUsersFromGraphQL(remotes) { + for (const remote of remotes) { + getMentionableUsers(remote, users => { + this.addUsers(users); + this.didUpdate(); + }); + } } addUsers(users) { @@ -87,41 +88,51 @@ export default class UserStore { } } -async function getMentionableUsers(remote) { +async function getMentionableUsers(remote, callback) { const fetchQuery = RelayNetworkLayerManager.getExistingFetchQuery('https://api.github.com/graphql'); if (!fetchQuery) { // No authentication token - return []; + return; } - const response = await fetchQuery({ - name: 'GetMentionableUsers', - text: ` - query GetMentionableUsers { - repository(owner: $owner, name: $name) { - mentionableUsers(first: $first, after: $after) { - nodes { - login - email - name - } - pageInfo { - hasNextPage - endCursor + let hasMore = true; + let cursor = null; + + while (hasMore) { + const response = await fetchQuery({ + name: 'GetMentionableUsers', + text: ` + query GetMentionableUsers { + repository(owner: $owner, name: $name) { + mentionableUsers(first: $first, after: $after) { + nodes { + login + email + name + } + pageInfo { + hasNextPage + endCursor + } } } } - } - `, - }, { - owner: remote.getOwner(), - name: remote.getRepo(), - first: 100, - after: null, - }); - - return response.data.repository.mentionableUsers.nodes.reduce((acc, node) => { - acc[node.email] = node.name; - return acc; - }, {}); + `, + }, { + owner: remote.getOwner(), + name: remote.getRepo(), + first: 100, + after: cursor, + }); + + const connection = response.data.repository.mentionableUsers; + + callback(connection.nodes.reduce((acc, node) => { + acc[node.email] = node.name; + return acc; + }, {})); + + cursor = connection.pageInfo.endCursor; + hasMore = connection.pageInfo.hasNextPage; + } } diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index dfe9c59f10..a588cf351b 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -34,7 +34,10 @@ describe('UserStore', function() { await repository.setConfig('remote.old.url', 'git@sourceforge.com:me/stuff.git'); await repository.setConfig('remote.old.fetch', '+refs/heads/*:refs/remotes/old/*'); - const {resolve} = expectRelayQuery({name: 'GetMentionableUsers'}, { + const {resolve} = expectRelayQuery({ + name: 'GetMentionableUsers', + variables: {owner: 'me', name: 'stuff', first: 100, after: null}, + }, { repository: { mentionableUsers: { nodes: [ @@ -62,6 +65,74 @@ describe('UserStore', function() { ]); }); + it('loads users from multiple pages from the GitHub API', async function() { + RelayNetworkLayerManager.getEnvironmentForHost('https://api.github.com', '1234'); + + const workdirPath = await cloneRepository('multiple-commits'); + const repository = await buildRepository(workdirPath); + + await repository.setConfig('remote.origin.url', 'git@github.com:me/stuff.git'); + await repository.setConfig('remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*'); + await repository.setConfig('remote.old.url', 'git@sourceforge.com:me/stuff.git'); + await repository.setConfig('remote.old.fetch', '+refs/heads/*:refs/remotes/old/*'); + + const {resolve: resolve0} = expectRelayQuery({ + name: 'GetMentionableUsers', + variables: {owner: 'me', name: 'stuff', first: 100, after: null}, + }, { + repository: { + mentionableUsers: { + nodes: [ + {login: 'annthurium', email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, + {login: 'octocat', email: 'mona@lisa.com', name: 'Mona Lisa'}, + {login: 'smashwilson', email: 'smashwilson@github.com', name: 'Ash Wilson'}, + ], + pageInfo: { + hasNextPage: true, + endCursor: 'foo', + }, + }, + }, + }); + + const {resolve: resolve1} = expectRelayQuery({ + name: 'GetMentionableUsers', + variables: {owner: 'me', name: 'stuff', first: 100, after: 'foo'}, + }, { + repository: { + mentionableUsers: { + nodes: [ + {login: 'zzz', email: 'zzz@github.com', name: 'Zzzzz'}, + {login: 'aaa', email: 'aaa@github.com', name: 'Aahhhhh'}, + ], + pageInfo: { + hasNextPage: false, + endCursor: 'bar', + }, + }, + }, + }); + + const store = new UserStore({repository}); + assert.deepEqual(store.getUsers(), []); + + resolve0(); + await assert.async.deepEqual(store.getUsers(), [ + {email: 'smashwilson@github.com', name: 'Ash Wilson'}, + {email: 'mona@lisa.com', name: 'Mona Lisa'}, + {email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, + ]); + + resolve1(); + await assert.async.deepEqual(store.getUsers(), [ + {email: 'aaa@github.com', name: 'Aahhhhh'}, + {email: 'smashwilson@github.com', name: 'Ash Wilson'}, + {email: 'mona@lisa.com', name: 'Mona Lisa'}, + {email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, + {email: 'zzz@github.com', name: 'Zzzzz'}, + ]); + }); + it('infers no-reply emails for users without a public email address'); it('excludes committer and no reply user from `getUsers`', async function() { From 3e9bc31eaa4bf44418258d7ca6ea35a54a706ce0 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 21 May 2018 14:45:03 -0400 Subject: [PATCH 07/64] Infer no-reply emails --- lib/models/user-store.js | 4 ++++ test/models/user-store.test.js | 36 +++++++++++++++++++++++++++++++--- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index 7ef35c4768..ae6d85df56 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -128,6 +128,10 @@ async function getMentionableUsers(remote, callback) { const connection = response.data.repository.mentionableUsers; callback(connection.nodes.reduce((acc, node) => { + if (node.email === '') { + node.email = `${node.login}@users.noreply.github.com`; + } + acc[node.email] = node.name; return acc; }, {})); diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index a588cf351b..b71d5a541d 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -73,8 +73,6 @@ describe('UserStore', function() { await repository.setConfig('remote.origin.url', 'git@github.com:me/stuff.git'); await repository.setConfig('remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*'); - await repository.setConfig('remote.old.url', 'git@sourceforge.com:me/stuff.git'); - await repository.setConfig('remote.old.fetch', '+refs/heads/*:refs/remotes/old/*'); const {resolve: resolve0} = expectRelayQuery({ name: 'GetMentionableUsers', @@ -133,7 +131,39 @@ describe('UserStore', function() { ]); }); - it('infers no-reply emails for users without a public email address'); + it('infers no-reply emails for users without a public email address', async function() { + RelayNetworkLayerManager.getEnvironmentForHost('https://api.github.com', '1234'); + + const workdirPath = await cloneRepository('multiple-commits'); + const repository = await buildRepository(workdirPath); + + await repository.setConfig('remote.origin.url', 'git@github.com:me/stuff.git'); + await repository.setConfig('remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*'); + + const {resolve} = expectRelayQuery({ + name: 'GetMentionableUsers', + variables: {owner: 'me', name: 'stuff', first: 100, after: null}, + }, { + repository: { + mentionableUsers: { + nodes: [ + {login: 'simurai', email: '', name: 'simurai'}, + ], + pageInfo: { + hasNextPage: false, + endCursor: null, + }, + }, + }, + }); + + const store = new UserStore({repository}); + + resolve(); + await assert.async.deepEqual(store.getUsers(), [ + {email: 'simurai@users.noreply.github.com', name: 'simurai'}, + ]); + }); it('excludes committer and no reply user from `getUsers`', async function() { const workdirPath = await cloneRepository('multiple-commits'); From d8e512e3e06f4b5522363c8a40adef2819e8cf2a Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 21 May 2018 14:56:58 -0400 Subject: [PATCH 08/64] Call addUsers once --- lib/models/user-store.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index ae6d85df56..6c0e784d00 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -35,18 +35,17 @@ export default class UserStore { this.setCommitter(committer); const githubRemotes = (await this.repository.getRemotes()).filter(remote => remote.isGithubRepo()); - const users = githubRemotes.length === 0 + githubRemotes.length === 0 ? await this.loadUsersFromLocalRepo() : await this.loadUsersFromGraphQL(githubRemotes); + } + async loadUsersFromLocalRepo() { + const users = await this.repository.getAuthors({max: MAX_COMMITS}); this.addUsers(users); this.didUpdate(); } - loadUsersFromLocalRepo() { - return this.repository.getAuthors({max: MAX_COMMITS}); - } - loadUsersFromGraphQL(remotes) { for (const remote of remotes) { getMentionableUsers(remote, users => { From 7f7d07ff65208ccb45885cdbfaa552aeec6c1b69 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 21 May 2018 15:42:53 -0400 Subject: [PATCH 09/64] Author model --- lib/models/author.js | 29 +++++++++++++++++++++++++++++ test/models/author.test.js | 19 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 lib/models/author.js create mode 100644 test/models/author.test.js diff --git a/lib/models/author.js b/lib/models/author.js new file mode 100644 index 0000000000..21da35c9a8 --- /dev/null +++ b/lib/models/author.js @@ -0,0 +1,29 @@ +export const NO_REPLY_GITHUB_EMAIL = 'noreply@github.com'; + +export default class Author { + constructor(email, fullName, login = null) { + this.email = email; + this.fullName = fullName; + this.login = login; + } + + getEmail() { + return this.email; + } + + getFullName() { + return this.fullName; + } + + getLogin() { + return this.login; + } + + isNoReply() { + return this.email === NO_REPLY_GITHUB_EMAIL; + } + + hasLogin() { + return this.login !== null; + } +} diff --git a/test/models/author.test.js b/test/models/author.test.js new file mode 100644 index 0000000000..ed2f444d8d --- /dev/null +++ b/test/models/author.test.js @@ -0,0 +1,19 @@ +import Author, {NO_REPLY_GITHUB_EMAIL} from '../../lib/models/author'; + +describe('Author', function() { + it('recognizes the no-reply GitHub email address', function() { + const a0 = new Author('foo@bar.com', 'Eh'); + assert.isFalse(a0.isNoReply()); + + const a1 = new Author(NO_REPLY_GITHUB_EMAIL, 'Whatever'); + assert.isTrue(a1.isNoReply()); + }); + + it('distinguishes authors with a GitHub handle', function() { + const a0 = new Author('foo@bar.com', 'Eh', 'handle'); + assert.isTrue(a0.hasLogin()); + + const a1 = new Author('other@bar.com', 'Nah'); + assert.isFalse(a1.hasLogin()); + }); +}); From 45f24accccee61b2f883550d3842752c35ece098 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 09:06:10 -0400 Subject: [PATCH 10/64] Construct Authors from Present.getAuthors() --- lib/models/repository-states/present.js | 6 +++-- test/models/repository.test.js | 35 +++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index 6075a609fa..cf3780eae1 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -11,6 +11,7 @@ import Hunk from '../hunk'; import HunkLine from '../hunk-line'; import DiscardHistory from '../discard-history'; import Branch, {nullBranch} from '../branch'; +import Author from '../author'; import BranchSet from '../branch-set'; import Remote from '../remote'; import Commit from '../commit'; @@ -616,8 +617,9 @@ export default class Present extends State { // For now we'll do the naive thing and invalidate anytime HEAD moves. This ensures that we get new authors // introduced by newly created commits or pulled commits. // This means that we are constantly re-fetching data. If performance becomes a concern we can optimize - return this.cache.getOrSet(Keys.authors, () => { - return this.git().getAuthors(options); + return this.cache.getOrSet(Keys.authors, async () => { + const authorMap = await this.git().getAuthors(options); + return Object.keys(authorMap).map(email => new Author(email, authorMap[email])); }); } diff --git a/test/models/repository.test.js b/test/models/repository.test.js index acab459700..69e3b8d6bd 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -536,6 +536,41 @@ describe('Repository', function() { }); }); + describe('getAuthors', function() { + it('returns user names and emails', async function() { + const workingDirPath = await cloneRepository('multiple-commits'); + const repository = new Repository(workingDirPath); + await repository.getLoadPromise(); + + await repository.git.exec(['config', 'user.name', 'Mona Lisa']); + await repository.git.exec(['config', 'user.email', 'mona@lisa.com']); + await repository.git.commit('Commit from Mona', {allowEmpty: true}); + + await repository.git.exec(['config', 'user.name', 'Hubot']); + await repository.git.exec(['config', 'user.email', 'hubot@github.com']); + await repository.git.commit('Commit from Hubot', {allowEmpty: true}); + + await repository.git.exec(['config', 'user.name', 'Me']); + await repository.git.exec(['config', 'user.email', 'me@github.com']); + await repository.git.commit('Commit from me', {allowEmpty: true}); + + const authors = await repository.getAuthors({max: 3}); + assert.lengthOf(authors, 3); + + const expected = [ + ['mona@lisa.com', 'Mona Lisa'], + ['hubot@github.com', 'Hubot'], + ['me@github.com', 'Me'], + ]; + for (const [email, fullName] of expected) { + assert.isTrue( + authors.some(author => author.getEmail() === email && author.getFullName() === fullName), + `getAuthors() output includes ${fullName} <${email}>`, + ); + } + }); + }); + describe('pull()', function() { it('updates the remote branch and merges into local branch', async function() { const {localRepoPath} = await setUpLocalAndRemoteRepositories({remoteAhead: true}); From f845cdfaaae8e82d4723d7548a022b016f373734 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 09:06:48 -0400 Subject: [PATCH 11/64] Return an Author or nullAuthor from getCommitter() --- lib/models/author.js | 42 ++++++++++++++++++++++++++++++++++ lib/models/repository.js | 6 ++++- test/models/repository.test.js | 17 +++++++------- 3 files changed, 55 insertions(+), 10 deletions(-) diff --git a/lib/models/author.js b/lib/models/author.js index 21da35c9a8..b6f29eb6ce 100644 --- a/lib/models/author.js +++ b/lib/models/author.js @@ -26,4 +26,46 @@ export default class Author { hasLogin() { return this.login !== null; } + + isPresent() { + return true; + } + + toString() { + let s = `${this.fullName} <${this.email}>`; + if (this.hasLogin()) { + s += ` @${this.login}`; + } + return s; + } } + +export const nullAuthor = { + getEmail() { + return ''; + }, + + getFullName() { + return ''; + }, + + getLogin() { + return null; + }, + + isNoReply() { + return false; + }, + + hasLogin() { + return false; + }, + + isPresent() { + return false; + }, + + toString() { + return 'null author'; + }, +}; diff --git a/lib/models/repository.js b/lib/models/repository.js index 7a3d58ca5a..b785b55f01 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -6,6 +6,7 @@ import fs from 'fs-extra'; import {getNullActionPipelineManager} from '../action-pipeline'; import CompositeGitStrategy from '../composite-git-strategy'; import Remote, {nullRemote} from './remote'; +import Author, {nullAuthor} from './author'; import Branch from './branch'; import {Loading, Absent, LoadingGuess, AbsentGuess} from './repository-states'; @@ -241,7 +242,10 @@ export default class Repository { } }); } - return committer; + + return committer.name !== null && committer.email !== null + ? new Author(committer.email, committer.name) + : nullAuthor; } } diff --git a/test/models/repository.test.js b/test/models/repository.test.js index 69e3b8d6bd..ca28ee4131 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -514,13 +514,14 @@ describe('Repository', function() { const workingDirPath = await cloneRepository('three-files'); const repository = new Repository(workingDirPath); await repository.getLoadPromise(); - assert.deepEqual(await repository.getCommitter(), { - name: FAKE_USER.name, - email: FAKE_USER.email, - }); + + const committer = await repository.getCommitter(); + assert.isTrue(committer.isPresent()); + assert.strictEqual(committer.getFullName(), FAKE_USER.name); + assert.strictEqual(committer.getEmail(), FAKE_USER.email); }); - it('returns empty object if user name or email do not exist', async function() { + it('returns a null object if user name or email do not exist', async function() { const workingDirPath = await cloneRepository('three-files'); const repository = new Repository(workingDirPath); await repository.getLoadPromise(); @@ -529,10 +530,8 @@ describe('Repository', function() { // getting the local config for testing purposes only because we don't // want to blow away global config when running tests. - assert.deepEqual(await repository.getCommitter({local: true}), { - name: null, - email: null, - }); + const committer = await repository.getCommitter({local: true}); + assert.isFalse(committer.isPresent()); }); }); From 19f9d9f24b864c51533cfd8023006224150d7a42 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 10:02:33 -0400 Subject: [PATCH 12/64] Sort and match Authors --- lib/models/author.js | 14 ++++++++++++++ test/models/author.test.js | 14 +++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/lib/models/author.js b/lib/models/author.js index b6f29eb6ce..f2f6f32660 100644 --- a/lib/models/author.js +++ b/lib/models/author.js @@ -31,6 +31,10 @@ export default class Author { return true; } + matches(other) { + return this.getEmail() === other.getEmail(); + } + toString() { let s = `${this.fullName} <${this.email}>`; if (this.hasLogin()) { @@ -38,6 +42,12 @@ export default class Author { } return s; } + + static compare(a, b) { + if (a.getFullName() < b.getFullName()) { return -1; } + if (a.getFullName() > b.getFullName()) { return 1; } + return 0; + } } export const nullAuthor = { @@ -65,6 +75,10 @@ export const nullAuthor = { return false; }, + matches(other) { + return other === this; + }, + toString() { return 'null author'; }, diff --git a/test/models/author.test.js b/test/models/author.test.js index ed2f444d8d..6f5952ec2c 100644 --- a/test/models/author.test.js +++ b/test/models/author.test.js @@ -1,4 +1,4 @@ -import Author, {NO_REPLY_GITHUB_EMAIL} from '../../lib/models/author'; +import Author, {nullAuthor, NO_REPLY_GITHUB_EMAIL} from '../../lib/models/author'; describe('Author', function() { it('recognizes the no-reply GitHub email address', function() { @@ -16,4 +16,16 @@ describe('Author', function() { const a1 = new Author('other@bar.com', 'Nah'); assert.isFalse(a1.hasLogin()); }); + + it('implements matching by email address', function() { + const a0 = new Author('same@same.com', 'Zero'); + const a1 = new Author('same@same.com', 'One'); + const a2 = new Author('same@same.com', 'Two', 'two'); + const a3 = new Author('different@same.com', 'Three'); + + assert.isTrue(a0.matches(a1)); + assert.isTrue(a0.matches(a2)); + assert.isFalse(a0.matches(a3)); + assert.isFalse(a0.matches(nullAuthor)); + }); }); From 00ae45c45ac76543b58c51e6e22211790fcff7eb Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 10:02:53 -0400 Subject: [PATCH 13/64] Store Authors sorted within the UserStore --- lib/models/user-store.js | 175 ++++++++++++++++++--------------- test/models/user-store.test.js | 82 +++++++-------- 2 files changed, 126 insertions(+), 131 deletions(-) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index 6c0e784d00..2c3211d476 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -1,10 +1,9 @@ import RelayNetworkLayerManager from '../relay-network-layer-manager'; +import Author, {nullAuthor} from './author'; // This is a guess about what a reasonable value is. Can adjust if performance is poor. const MAX_COMMITS = 5000; -export const NO_REPLY_GITHUB_EMAIL = 'noreply@github.com'; - export default class UserStore { constructor({repository, onDidUpdate}) { this.repository = repository; @@ -13,8 +12,10 @@ export default class UserStore { }); this.onDidUpdate = onDidUpdate || (() => {}); // TODO: [ku 3/2018] Consider using Dexie (indexDB wrapper) like Desktop and persist users across sessions - this.users = {}; - this.committer = {}; + + this.allUsers = new Map(); + this.users = []; + this.committer = nullAuthor; this.populate(); } @@ -35,32 +36,110 @@ export default class UserStore { this.setCommitter(committer); const githubRemotes = (await this.repository.getRemotes()).filter(remote => remote.isGithubRepo()); - githubRemotes.length === 0 - ? await this.loadUsersFromLocalRepo() - : await this.loadUsersFromGraphQL(githubRemotes); + if (githubRemotes.length === 0) { + await this.loadUsersFromLocalRepo(); + } else { + await this.loadUsersFromGraphQL(githubRemotes); + } } async loadUsersFromLocalRepo() { const users = await this.repository.getAuthors({max: MAX_COMMITS}); this.addUsers(users); - this.didUpdate(); } loadUsersFromGraphQL(remotes) { - for (const remote of remotes) { - getMentionableUsers(remote, users => { - this.addUsers(users); - this.didUpdate(); + return Promise.all( + remotes.map(remote => this.loadMentionableUsers(remote)), + ); + } + + async loadMentionableUsers(remote) { + const fetchQuery = RelayNetworkLayerManager.getExistingFetchQuery('https://api.github.com/graphql'); + if (!fetchQuery) { + // No authentication token + return; + } + + let hasMore = true; + let cursor = null; + + while (hasMore) { + const response = await fetchQuery({ + name: 'GetMentionableUsers', + text: ` + query GetMentionableUsers { + repository(owner: $owner, name: $name) { + mentionableUsers(first: $first, after: $after) { + nodes { + login + email + name + } + pageInfo { + hasNextPage + endCursor + } + } + } + } + `, + }, { + owner: remote.getOwner(), + name: remote.getRepo(), + first: 100, + after: cursor, }); + + const connection = response.data.repository.mentionableUsers; + + this.addUsers(connection.nodes.map(node => { + if (node.email === '') { + node.email = `${node.login}@users.noreply.github.com`; + } + + return new Author(node.email, node.name, node.login); + })); + + cursor = connection.pageInfo.endCursor; + hasMore = connection.pageInfo.hasNextPage; } } addUsers(users) { - this.users = {...this.users, ...users}; + let changed = false; + for (const author of users) { + if (!this.allUsers.has(author.getEmail())) { + changed = true; + } + this.allUsers.set(author.getEmail(), author); + } + + if (changed) { + this.finalize(); + } + } + + finalize() { + // TODO: [ku 3/2018] consider sorting based on most recent authors or commit frequency + const users = []; + for (const author of this.allUsers.values()) { + if (author.matches(this.committer)) { continue; } + if (author.isNoReply()) { continue; } + + users.push(author); + } + users.sort(Author.compare); + this.users = users; + this.didUpdate(); } setCommitter(committer) { + const changed = !this.committer.matches(committer); this.committer = committer; + if (changed) { + this.finalize(); + } } didUpdate() { @@ -68,74 +147,6 @@ export default class UserStore { } getUsers() { - // TODO: [ku 3/2018] consider sorting based on most recent authors or commit frequency - // Also, this is obviously not the most performant. Optimize once we incorporate github username info, - // as this will likely impact the shape of the data we store - const users = this.users; - - // you wouldn't download a car. you wouldn't add yourself as a co author. - delete users[this.committer.email]; - delete users[NO_REPLY_GITHUB_EMAIL]; - - return Object.keys(users) - .map(email => ({email, name: this.users[email]})) - .sort((a, b) => { - if (a.name < b.name) { return -1; } - if (a.name > b.name) { return 1; } - return 0; - }); - } -} - -async function getMentionableUsers(remote, callback) { - const fetchQuery = RelayNetworkLayerManager.getExistingFetchQuery('https://api.github.com/graphql'); - if (!fetchQuery) { - // No authentication token - return; - } - - let hasMore = true; - let cursor = null; - - while (hasMore) { - const response = await fetchQuery({ - name: 'GetMentionableUsers', - text: ` - query GetMentionableUsers { - repository(owner: $owner, name: $name) { - mentionableUsers(first: $first, after: $after) { - nodes { - login - email - name - } - pageInfo { - hasNextPage - endCursor - } - } - } - } - `, - }, { - owner: remote.getOwner(), - name: remote.getRepo(), - first: 100, - after: cursor, - }); - - const connection = response.data.repository.mentionableUsers; - - callback(connection.nodes.reduce((acc, node) => { - if (node.email === '') { - node.email = `${node.login}@users.noreply.github.com`; - } - - acc[node.email] = node.name; - return acc; - }, {})); - - cursor = connection.pageInfo.endCursor; - hasMore = connection.pageInfo.hasNextPage; + return this.users; } } diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index b71d5a541d..1a519b9695 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -1,6 +1,7 @@ import dedent from 'dedent-js'; -import UserStore, {NO_REPLY_GITHUB_EMAIL} from '../../lib/models/user-store'; +import UserStore from '../../lib/models/user-store'; +import Author, {nullAuthor} from '../../lib/models/author'; import RelayNetworkLayerManager, {expectRelayQuery} from '../../lib/relay-network-layer-manager'; import {cloneRepository, buildRepository, FAKE_USER} from '../helpers'; @@ -11,16 +12,13 @@ describe('UserStore', function() { const store = new UserStore({repository}); assert.deepEqual(store.getUsers(), []); - assert.deepEqual(store.committer, {}); + assert.strictEqual(store.committer, nullAuthor); // Store is populated asynchronously await assert.async.deepEqual(store.getUsers(), [ - { - email: 'kuychaco@github.com', - name: 'Katrina Uychaco', - }, + new Author('kuychaco@github.com', 'Katrina Uychaco'), ]); - await assert.async.deepEqual(store.committer, FAKE_USER); + await assert.async.deepEqual(store.committer, new Author(FAKE_USER.email, FAKE_USER.name)); }); it('loads store with mentionable users from the GitHub API in a repo with a GitHub remote', async function() { @@ -59,9 +57,9 @@ describe('UserStore', function() { resolve(); await assert.async.deepEqual(store.getUsers(), [ - {email: 'smashwilson@github.com', name: 'Ash Wilson'}, - {email: 'mona@lisa.com', name: 'Mona Lisa'}, - {email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, + new Author('smashwilson@github.com', 'Ash Wilson', 'smashwilson'), + new Author('mona@lisa.com', 'Mona Lisa', 'octocat'), + new Author('annthurium@github.com', 'Tilde Ann Thurium', 'annthurium'), ]); }); @@ -116,18 +114,18 @@ describe('UserStore', function() { resolve0(); await assert.async.deepEqual(store.getUsers(), [ - {email: 'smashwilson@github.com', name: 'Ash Wilson'}, - {email: 'mona@lisa.com', name: 'Mona Lisa'}, - {email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, + new Author('smashwilson@github.com', 'Ash Wilson', 'smashwilson'), + new Author('mona@lisa.com', 'Mona Lisa', 'octocat'), + new Author('annthurium@github.com', 'Tilde Ann Thurium', 'annthurium'), ]); resolve1(); await assert.async.deepEqual(store.getUsers(), [ - {email: 'aaa@github.com', name: 'Aahhhhh'}, - {email: 'smashwilson@github.com', name: 'Ash Wilson'}, - {email: 'mona@lisa.com', name: 'Mona Lisa'}, - {email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, - {email: 'zzz@github.com', name: 'Zzzzz'}, + new Author('aaa@github.com', 'Aahhhhh', 'aaa'), + new Author('smashwilson@github.com', 'Ash Wilson', 'smashwilson'), + new Author('mona@lisa.com', 'Mona Lisa', 'octocat'), + new Author('annthurium@github.com', 'Tilde Ann Thurium', 'annthurium'), + new Author('zzz@github.com', 'Zzzzz', 'zzz'), ]); }); @@ -161,7 +159,7 @@ describe('UserStore', function() { resolve(); await assert.async.deepEqual(store.getUsers(), [ - {email: 'simurai@users.noreply.github.com', name: 'simurai'}, + new Author('simurai@users.noreply.github.com', 'simurai', 'simurai'), ]); }); @@ -182,12 +180,10 @@ describe('UserStore', function() { // verify that FAKE_USER is not in users returned from `getUsers` const users = store.getUsers(); - const committerFromStore = users.find(user => user.email === FAKE_USER.email); - assert.isUndefined(committerFromStore); + assert.isFalse(users.some(user => user.getEmail() === FAKE_USER.email)); // verify that no-reply email address is not in users array - const noReplyUser = users.find(user => user.email === NO_REPLY_GITHUB_EMAIL); - assert.isUndefined(noReplyUser); + assert.isFalse(users.some(user => user.isNoReply())); }); describe('addUsers', function() { @@ -198,24 +194,15 @@ describe('UserStore', function() { await assert.async.lengthOf(store.getUsers(), 1); - store.addUsers({ - 'mona@lisa.com': 'Mona Lisa', - 'hubot@github.com': 'Hubot Robot', - }); + store.addUsers([ + new Author('mona@lisa.com', 'Mona Lisa'), + new Author('hubot@github.com', 'Hubot Robot'), + ]); - await assert.async.deepEqual(store.getUsers(), [ - { - name: 'Hubot Robot', - email: 'hubot@github.com', - }, - { - name: 'Katrina Uychaco', - email: 'kuychaco@github.com', - }, - { - name: 'Mona Lisa', - email: 'mona@lisa.com', - }, + assert.deepEqual(store.getUsers(), [ + new Author('hubot@github.com', 'Hubot Robot'), + new Author('kuychaco@github.com', 'Katrina Uychaco'), + new Author('mona@lisa.com', 'Mona Lisa'), ]); }); }); @@ -225,18 +212,18 @@ describe('UserStore', function() { const repository = await buildRepository(workdirPath); const store = new UserStore({repository}); - await assert.async.deepEqual(store.committer, FAKE_USER); + await assert.async.deepEqual(store.committer, new Author(FAKE_USER.email, FAKE_USER.name)); const newEmail = 'foo@bar.com'; await repository.setConfig('user.email', newEmail); repository.refresh(); - await assert.async.deepEqual(store.committer, {name: FAKE_USER.name, email: newEmail}); + await assert.async.deepEqual(store.committer, new Author(FAKE_USER.email, FAKE_USER.name)); const newName = 'Foo Bar'; await repository.setConfig('user.name', newName); repository.refresh(); - await assert.async.deepEqual(store.committer, {name: newName, email: newEmail}); + await assert.async.deepEqual(store.committer, new Author(newEmail, newName)); }); it('refetches users when HEAD changes', async function() { @@ -249,10 +236,7 @@ describe('UserStore', function() { const store = new UserStore({repository}); await assert.async.deepEqual(store.getUsers(), [ - { - email: 'kuychaco@github.com', - name: 'Katrina Uychaco', - }, + new Author('kuychaco@github.com', 'Katrina Uychaco'), ]); sinon.spy(store, 'addUsers'); @@ -265,8 +249,8 @@ describe('UserStore', function() { `, {allowEmpty: true}); await assert.async.equal(store.addUsers.callCount, 1); - assert.isOk(store.getUsers().find(user => { - return user.name === 'New Author' && user.email === 'new-author@email.com'; + assert.isTrue(store.getUsers().some(user => { + return user.getFullName() === 'New Author' && user.getEmail() === 'new-author@email.com'; })); // Change head due to branch checkout From 9576c1fd0329033332184f287380671598b76882 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 10:40:41 -0400 Subject: [PATCH 14/64] Use a ModelObserver to track the Repository --- lib/models/user-store.js | 48 ++++++++++++++++------------------ test/models/user-store.test.js | 2 +- 2 files changed, 24 insertions(+), 26 deletions(-) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index 2c3211d476..bfb558d13a 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -1,53 +1,47 @@ +import yubikiri from 'yubikiri'; + import RelayNetworkLayerManager from '../relay-network-layer-manager'; import Author, {nullAuthor} from './author'; +import ModelObserver from './model-observer'; // This is a guess about what a reasonable value is. Can adjust if performance is poor. const MAX_COMMITS = 5000; export default class UserStore { constructor({repository, onDidUpdate}) { - this.repository = repository; - this.repository.onDidUpdate(() => { - this.loadUsers(); - }); this.onDidUpdate = onDidUpdate || (() => {}); // TODO: [ku 3/2018] Consider using Dexie (indexDB wrapper) like Desktop and persist users across sessions this.allUsers = new Map(); this.users = []; this.committer = nullAuthor; - this.populate(); + + this.repositoryObserver = new ModelObserver({ + fetchData: r => yubikiri({ + committer: r.getCommitter(), + authors: r.getAuthors({max: MAX_COMMITS}), + remotes: r.getRemotes(), + }), + didUpdate: () => this.loadUsers(this.repositoryObserver.getActiveModelData()), + }); + this.repositoryObserver.setActiveModel(repository); } - populate() { - if (this.repository.isPresent()) { - this.loadUsers(); - } else { - this.repository.onDidChangeState(({from, to}) => { - if (!from.isPresent() && to.isPresent()) { - this.loadUsers(); - } - }); + async loadUsers(data) { + if (!data) { + return; } - } - async loadUsers() { - const committer = await this.repository.getCommitter(); - this.setCommitter(committer); + this.setCommitter(data.committer); - const githubRemotes = (await this.repository.getRemotes()).filter(remote => remote.isGithubRepo()); + const githubRemotes = data.remotes.filter(remote => remote.isGithubRepo()); if (githubRemotes.length === 0) { - await this.loadUsersFromLocalRepo(); + this.addUsers(data.authors); } else { await this.loadUsersFromGraphQL(githubRemotes); } } - async loadUsersFromLocalRepo() { - const users = await this.repository.getAuthors({max: MAX_COMMITS}); - this.addUsers(users); - } - loadUsersFromGraphQL(remotes) { return Promise.all( remotes.map(remote => this.loadMentionableUsers(remote)), @@ -134,6 +128,10 @@ export default class UserStore { this.didUpdate(); } + setRepository(repository) { + this.repositoryObserver.setActiveModel(repository); + } + setCommitter(committer) { const changed = !this.committer.matches(committer); this.committer = committer; diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index 1a519b9695..66bd3ca47c 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -167,7 +167,7 @@ describe('UserStore', function() { const workdirPath = await cloneRepository('multiple-commits'); const repository = await buildRepository(workdirPath); const store = new UserStore({repository}); - await store.loadUsersFromLocalRepo(); + await assert.async.lengthOf(store.getUsers(), 1); sinon.spy(store, 'addUsers'); // make a commit with FAKE_USER as committer From 0a6a903c90b722c910dd594c47fc3a15b3db0b95 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 10:48:09 -0400 Subject: [PATCH 15/64] Turn UserStore into an event Emitter --- lib/models/user-store.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index bfb558d13a..cf4d7b3d92 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -1,4 +1,5 @@ import yubikiri from 'yubikiri'; +import {Emitter} from 'event-kit'; import RelayNetworkLayerManager from '../relay-network-layer-manager'; import Author, {nullAuthor} from './author'; @@ -8,10 +9,10 @@ import ModelObserver from './model-observer'; const MAX_COMMITS = 5000; export default class UserStore { - constructor({repository, onDidUpdate}) { - this.onDidUpdate = onDidUpdate || (() => {}); - // TODO: [ku 3/2018] Consider using Dexie (indexDB wrapper) like Desktop and persist users across sessions + constructor({repository}) { + this.emitter = new Emitter(); + // TODO: [ku 3/2018] Consider using Dexie (indexDB wrapper) like Desktop and persist users across sessions this.allUsers = new Map(); this.users = []; this.committer = nullAuthor; @@ -141,7 +142,11 @@ export default class UserStore { } didUpdate() { - this.onDidUpdate(this.getUsers()); + this.emitter.emit('did-update', this.getUsers()); + } + + onDidUpdate(callback) { + return this.emitter.on('did-update', callback); } getUsers() { From e4f92a8f29cb57473075d61255a52f910528472a Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 14:07:23 -0400 Subject: [PATCH 16/64] PropType shapes for new models --- lib/prop-types.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/prop-types.js b/lib/prop-types.js index b099508491..2d17a1994a 100644 --- a/lib/prop-types.js +++ b/lib/prop-types.js @@ -38,8 +38,8 @@ export const CommitPropType = PropTypes.shape({ }); export const AuthorPropType = PropTypes.shape({ - email: PropTypes.string.isRequired, - name: PropTypes.string.isRequired, + getEmail: PropTypes.func.isRequired, + getFullName: PropTypes.func.isRequired, }); export const RelayConnectionPropType = nodePropType => PropTypes.shape({ @@ -86,3 +86,8 @@ export const MergeConflictItemPropType = PropTypes.shape({ theirs: PropTypes.oneOf(statusNames).isRequired, }).isRequired, }); + +export const UserStorePropType = PropTypes.shape({ + getUsers: PropTypes.func.isRequired, + onDidUpdate: PropTypes.func.isRequired, +}); From edf3bb1a04092636b70209421d17a9be6409b333 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 14:07:37 -0400 Subject: [PATCH 17/64] getAuthors() stub should return [] --- lib/models/repository-states/state.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/models/repository-states/state.js b/lib/models/repository-states/state.js index aa1756107e..c24f9080a1 100644 --- a/lib/models/repository-states/state.js +++ b/lib/models/repository-states/state.js @@ -293,7 +293,7 @@ export default class State { // Author information getAuthors() { - return Promise.resolve({}); + return Promise.resolve([]); } // Branches From 7ee56cbcac7db2688d40d392f83ba1cad36bc5b8 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 14:09:14 -0400 Subject: [PATCH 18/64] The UserStore observes its Repository --- lib/controllers/git-tab-controller.js | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/lib/controllers/git-tab-controller.js b/lib/controllers/git-tab-controller.js index 7b98772179..7d5a49b5dc 100644 --- a/lib/controllers/git-tab-controller.js +++ b/lib/controllers/git-tab-controller.js @@ -66,12 +66,7 @@ export default class GitTabController extends React.Component { selectedCoAuthors: [], }; - this.userStore = new UserStore({ - repository: this.props.repository, - onDidUpdate: users => { - this.setState({mentionableUsers: users}); - }, - }); + this.userStore = new UserStore({repository: this.props.repository}); } render() { @@ -135,16 +130,8 @@ export default class GitTabController extends React.Component { this.refView.refRoot.addEventListener('focusin', this.rememberLastFocus); } - componentDidUpdate(prevProps) { - if (prevProps.repository !== this.props.repository) { - this.userStore = new UserStore({ - repository: this.props.repository, - onDidUpdate: users => { - this.setState({mentionableUsers: users}); - }, - }); - } - + componentDidUpdate() { + this.userStore.setRepository(this.props.repository); this.refreshResolutionProgress(false, false); } From c4292dea6dafd96cb1eaf76b3aa8bf4361de59aa Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 14:09:53 -0400 Subject: [PATCH 19/64] Update the UserStore in the updateSelectedCoAuthors callback --- lib/controllers/git-tab-controller.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/controllers/git-tab-controller.js b/lib/controllers/git-tab-controller.js index 7d5a49b5dc..43c9649864 100644 --- a/lib/controllers/git-tab-controller.js +++ b/lib/controllers/git-tab-controller.js @@ -5,6 +5,7 @@ import PropTypes from 'prop-types'; import GitTabView from '../views/git-tab-view'; import UserStore from '../models/user-store'; +import Author from '../models/author'; import {CommitPropType, BranchPropType, FilePatchItemPropType, MergeConflictItemPropType} from '../prop-types'; import {autobind} from '../helpers'; @@ -245,7 +246,7 @@ export default class GitTabController extends React.Component { updateSelectedCoAuthors(selectedCoAuthors, newAuthor) { if (newAuthor) { - this.userStore.addUsers({[newAuthor.email]: newAuthor.name}); + this.userStore.addUsers([newAuthor]); selectedCoAuthors = selectedCoAuthors.concat([newAuthor]); } this.setState({selectedCoAuthors}); From e7772b54225d3fb8a1fb91ee40970f1fba7cd2ec Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 14:10:22 -0400 Subject: [PATCH 20/64] Pass the UserStore through the component tree --- lib/controllers/commit-controller.js | 6 +++--- lib/controllers/git-tab-controller.js | 3 +-- lib/views/git-tab-view.js | 6 +++--- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/lib/controllers/commit-controller.js b/lib/controllers/commit-controller.js index 3aa490d25b..240d4a618d 100644 --- a/lib/controllers/commit-controller.js +++ b/lib/controllers/commit-controller.js @@ -7,7 +7,7 @@ import {CompositeDisposable} from 'event-kit'; import fs from 'fs-extra'; import CommitView from '../views/commit-view'; -import {AuthorPropType} from '../prop-types'; +import {AuthorPropType, UserStorePropType} from '../prop-types'; import {autobind} from '../helpers'; export const COMMIT_GRAMMAR_SCOPE = 'text.git-commit'; @@ -31,7 +31,7 @@ export default class CommitController extends React.Component { stagedChangesExist: PropTypes.bool.isRequired, lastCommit: PropTypes.object.isRequired, currentBranch: PropTypes.object.isRequired, - mentionableUsers: PropTypes.arrayOf(AuthorPropType), + userStore: UserStorePropType.isRequired, selectedCoAuthors: PropTypes.arrayOf(AuthorPropType), updateSelectedCoAuthors: PropTypes.func, prepareToCommit: PropTypes.func.isRequired, @@ -105,7 +105,7 @@ export default class CommitController extends React.Component { onChangeMessage={this.handleMessageChange} toggleExpandedCommitMessageEditor={this.toggleExpandedCommitMessageEditor} deactivateCommitBox={this.isCommitMessageEditorExpanded()} - mentionableUsers={this.props.mentionableUsers} + userStore={this.props.userStore} selectedCoAuthors={this.props.selectedCoAuthors} updateSelectedCoAuthors={this.props.updateSelectedCoAuthors} /> diff --git a/lib/controllers/git-tab-controller.js b/lib/controllers/git-tab-controller.js index 43c9649864..2c9d432304 100644 --- a/lib/controllers/git-tab-controller.js +++ b/lib/controllers/git-tab-controller.js @@ -63,7 +63,6 @@ export default class GitTabController extends React.Component { this.refView = null; this.state = { - mentionableUsers: [], selectedCoAuthors: [], }; @@ -89,7 +88,7 @@ export default class GitTabController extends React.Component { mergeConflicts={this.props.mergeConflicts} workingDirectoryPath={this.props.workingDirectoryPath} mergeMessage={this.props.mergeMessage} - mentionableUsers={this.state.mentionableUsers} + userStore={this.userStore} selectedCoAuthors={this.state.selectedCoAuthors} updateSelectedCoAuthors={this.updateSelectedCoAuthors} diff --git a/lib/views/git-tab-view.js b/lib/views/git-tab-view.js index ac75712030..44c6bd9d9e 100644 --- a/lib/views/git-tab-view.js +++ b/lib/views/git-tab-view.js @@ -8,7 +8,7 @@ import GitLogo from './git-logo'; import CommitController from '../controllers/commit-controller'; import RecentCommitsController from '../controllers/recent-commits-controller'; import {isValidWorkdir, autobind} from '../helpers'; -import {AuthorPropType} from '../prop-types'; +import {AuthorPropType, UserStorePropType} from '../prop-types'; export default class GitTabView extends React.Component { static focus = { @@ -32,7 +32,7 @@ export default class GitTabView extends React.Component { mergeConflicts: PropTypes.arrayOf(PropTypes.object), workingDirectoryPath: PropTypes.string, mergeMessage: PropTypes.string, - mentionableUsers: PropTypes.arrayOf(AuthorPropType), + userStore: UserStorePropType.isRequired, selectedCoAuthors: PropTypes.arrayOf(AuthorPropType), updateSelectedCoAuthors: PropTypes.func.isRequired, @@ -184,7 +184,7 @@ export default class GitTabView extends React.Component { isLoading={this.props.isLoading} lastCommit={this.props.lastCommit} repository={this.props.repository} - mentionableUsers={this.props.mentionableUsers} + userStore={this.props.userStore} selectedCoAuthors={this.props.selectedCoAuthors} updateSelectedCoAuthors={this.props.updateSelectedCoAuthors} /> From 65066a3909ec7095e68b50340c74d42d23332495 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 14:10:45 -0400 Subject: [PATCH 21/64] Observe the passed UserStore and update the Select component --- lib/views/commit-view.js | 46 ++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/lib/views/commit-view.js b/lib/views/commit-view.js index b721ab97b7..9c79af933f 100644 --- a/lib/views/commit-view.js +++ b/lib/views/commit-view.js @@ -8,8 +8,9 @@ import Tooltip from '../atom/tooltip'; import AtomTextEditor from '../atom/atom-text-editor'; import CoAuthorForm from './co-author-form'; import RefHolder from '../models/ref-holder'; +import ObserveModel from './observe-model'; import {LINE_ENDING_REGEX, autobind} from '../helpers'; -import {AuthorPropType} from '../prop-types'; +import {AuthorPropType, UserStorePropType} from '../prop-types'; const TOOLTIP_DELAY = 200; @@ -39,7 +40,7 @@ export default class CommitView extends React.Component { deactivateCommitBox: PropTypes.bool.isRequired, maximumCharacterLimit: PropTypes.number.isRequired, message: PropTypes.string.isRequired, - mentionableUsers: PropTypes.arrayOf(AuthorPropType), + userStore: UserStorePropType.isRequired, selectedCoAuthors: PropTypes.arrayOf(AuthorPropType), updateSelectedCoAuthors: PropTypes.func, commit: PropTypes.func.isRequired, @@ -246,30 +247,33 @@ export default class CommitView extends React.Component { } renderCoAuthorInput() { - if (!this.state.showCoAuthorInput) { return null; } return ( - + )} + ); } From ea65ac0ad1e3a8251350c34720f463a74210c62e Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 14:11:29 -0400 Subject: [PATCH 22/64] Pass an empty UserStore in tests --- test/controllers/commit-controller.test.js | 3 +++ test/views/commit-view.test.js | 3 +++ 2 files changed, 6 insertions(+) diff --git a/test/controllers/commit-controller.test.js b/test/controllers/commit-controller.test.js index 396b69c2dc..fa0b7d867a 100644 --- a/test/controllers/commit-controller.test.js +++ b/test/controllers/commit-controller.test.js @@ -5,6 +5,7 @@ import {shallow} from 'enzyme'; import Commit from '../../lib/models/commit'; import {nullBranch} from '../../lib/models/branch'; +import UserStore from '../../lib/models/user-store'; import CommitController, {COMMIT_GRAMMAR_SCOPE} from '../../lib/controllers/commit-controller'; import {cloneRepository, buildRepository, buildRepositoryWithPipeline} from '../helpers'; @@ -24,6 +25,7 @@ describe('CommitController', function() { lastCommit = new Commit({sha: 'a1e23fd45', message: 'last commit message'}); const noop = () => {}; + const store = new UserStore({}); app = ( {}; const returnTruthyPromise = () => Promise.resolve(true); + const store = new UserStore({}); app = ( Date: Tue, 22 May 2018 14:11:40 -0400 Subject: [PATCH 23/64] Import shuffle --- test/controllers/git-tab-controller.test.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/test/controllers/git-tab-controller.test.js b/test/controllers/git-tab-controller.test.js index b6eba8e027..1221d77492 100644 --- a/test/controllers/git-tab-controller.test.js +++ b/test/controllers/git-tab-controller.test.js @@ -1,20 +1,17 @@ import fs from 'fs'; import path from 'path'; - import React from 'react'; import {mount} from 'enzyme'; - import dedent from 'dedent-js'; import until from 'test-until'; import GitTabController from '../../lib/controllers/git-tab-controller'; import {gitTabControllerProps} from '../fixtures/props/git-tab-props'; - import {cloneRepository, buildRepository, buildRepositoryWithPipeline, initRepository} from '../helpers'; import Repository from '../../lib/models/repository'; -import {GitError} from '../../lib/git-shell-out-strategy'; - +import Author from '../../lib/models/author'; import ResolutionProgress from '../../lib/models/conflicts/resolution-progress'; +import {GitError} from '../../lib/git-shell-out-strategy'; describe('GitTabController', function() { let atomEnvironment, workspace, workspaceElement, commandRegistry, notificationManager; From b9b97053a3de934729046106b5ac4818e70827a8 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 14:11:53 -0400 Subject: [PATCH 24/64] I... have no idea how this was passing before? --- test/controllers/git-tab-controller.test.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/controllers/git-tab-controller.test.js b/test/controllers/git-tab-controller.test.js index 1221d77492..af5a4f17ca 100644 --- a/test/controllers/git-tab-controller.test.js +++ b/test/controllers/git-tab-controller.test.js @@ -65,6 +65,9 @@ describe('GitTabController', function() { assert.lengthOf(wrapper.find('StagingView'), 1); assert.lengthOf(wrapper.find('CommitController'), 1); + await repository.getLoadPromise(); + await updateWrapper(repository, wrapper); + await assert.async.isFalse(wrapper.update().find('.github-Panel').hasClass('is-loading')); assert.lengthOf(wrapper.find('StagingView'), 1); assert.lengthOf(wrapper.find('CommitController'), 1); From 3985f558d990fe854e727b08be67bb3665bb10d8 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 14:12:05 -0400 Subject: [PATCH 25/64] Use the Author model in the test fixture --- test/controllers/git-tab-controller.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/controllers/git-tab-controller.test.js b/test/controllers/git-tab-controller.test.js index af5a4f17ca..e864c58cba 100644 --- a/test/controllers/git-tab-controller.test.js +++ b/test/controllers/git-tab-controller.test.js @@ -169,8 +169,8 @@ describe('GitTabController', function() { const repository = await buildRepository(workdirPath); const wrapper = mount(await buildApp(repository)); - const coAuthors = [{name: 'Mona Lisa', email: 'mona@lisa.com'}]; - const newAuthor = {name: 'Mr. Hubot', email: 'hubot@github.com'}; + const coAuthors = [new Author('mona@lisa.com', 'Mona Lisa')]; + const newAuthor = new Author('hubot@github.com', 'Mr. Hubot'); wrapper.instance().updateSelectedCoAuthors(coAuthors, newAuthor); From 0c64139478ea921166f33793677d9194aa33026b Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 14:32:44 -0400 Subject: [PATCH 26/64] :fire: unused import --- lib/controllers/git-tab-controller.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/controllers/git-tab-controller.js b/lib/controllers/git-tab-controller.js index 2c9d432304..e57e06ff70 100644 --- a/lib/controllers/git-tab-controller.js +++ b/lib/controllers/git-tab-controller.js @@ -5,7 +5,6 @@ import PropTypes from 'prop-types'; import GitTabView from '../views/git-tab-view'; import UserStore from '../models/user-store'; -import Author from '../models/author'; import {CommitPropType, BranchPropType, FilePatchItemPropType, MergeConflictItemPropType} from '../prop-types'; import {autobind} from '../helpers'; From 1d84a611a4c2ffa16164df984594f08e36d36b23 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 14:33:02 -0400 Subject: [PATCH 27/64] Present talks model objects, GSOS talks raw objects --- lib/models/repository-states/present.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index cf3780eae1..bd8c2fb7c9 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -233,7 +233,16 @@ export default class Present extends State { ], // eslint-disable-next-line no-shadow () => this.executePipelineAction('COMMIT', (message, options) => { - return this.git().commit(message, options); + const opts = (!options || !options.coAuthors) + ? options + : { + ...options, + coAuthors: options.coAuthors.map(author => { + return {email: author.getEmail(), name: author.getFullName()}; + }), + }; + + return this.git().commit(message, opts); }, message, options), ); } From 68ea76c2016aedeb17030460c10c99bed9d77469 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 14:33:10 -0400 Subject: [PATCH 28/64] More Author model usage --- test/controllers/git-tab-controller.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/controllers/git-tab-controller.test.js b/test/controllers/git-tab-controller.test.js index e864c58cba..5a1e7a7f32 100644 --- a/test/controllers/git-tab-controller.test.js +++ b/test/controllers/git-tab-controller.test.js @@ -609,7 +609,7 @@ describe('GitTabController', function() { assert.deepEqual(commitBeforeAmend.coAuthors, []); // add co author - const author = {email: 'foo@bar.com', name: 'foo bar'}; + const author = new Author('foo@bar.com', 'foo bar'); const commitView = wrapper.find('CommitView').instance(); commitView.setState({showCoAuthorInput: true}); commitView.onSelectedCoAuthorsChanged([author]); @@ -621,7 +621,7 @@ describe('GitTabController', function() { await repository.commit.returnValues[0]; await updateWrapper(repository, wrapper); - assert.deepEqual(getLastCommit().coAuthors, [author]); + assert.deepEqual(getLastCommit().coAuthors, [{email: author.getEmail(), name: author.getFullName()}]); assert.strictEqual(getLastCommit().getMessageSubject(), commitBeforeAmend.getMessageSubject()); }); From f9e71462c5eb6cad923684dc4c26b938e37ca832 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 22 May 2018 14:43:30 -0400 Subject: [PATCH 29/64] Another Author model --- test/controllers/git-tab-controller.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/controllers/git-tab-controller.test.js b/test/controllers/git-tab-controller.test.js index 5a1e7a7f32..2a6abbb17a 100644 --- a/test/controllers/git-tab-controller.test.js +++ b/test/controllers/git-tab-controller.test.js @@ -631,7 +631,7 @@ describe('GitTabController', function() { assert.deepEqual(commitBeforeAmend.coAuthors, []); // add co author - const author = {email: 'foo@bar.com', name: 'foo bar'}; + const author = new Author('foo@bar.com', 'foo bar'); const commitView = wrapper.find('CommitView').instance(); commitView.setState({showCoAuthorInput: true}); commitView.onSelectedCoAuthorsChanged([author]); @@ -645,7 +645,7 @@ describe('GitTabController', function() { await updateWrapper(repository, wrapper); // verify that commit message has coauthor - assert.deepEqual(getLastCommit().coAuthors, [author]); + assert.deepEqual(getLastCommit().coAuthors, [{email: author.getEmail(), name: author.getFullName()}]); assert.strictEqual(getLastCommit().getMessageSubject(), newMessage); }); From c8c60510466cd12e561947cdcd8f23e1096a3e2e Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 23 May 2018 11:41:26 -0400 Subject: [PATCH 30/64] Acquire the token from the GithubLoginModel --- lib/models/user-store.js | 26 ++++-- lib/relay-network-layer-manager.js | 9 +- test/models/user-store.test.js | 140 ++++++++++++++++++++++++----- 3 files changed, 139 insertions(+), 36 deletions(-) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index cf4d7b3d92..f0898b8d4a 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -3,13 +3,14 @@ import {Emitter} from 'event-kit'; import RelayNetworkLayerManager from '../relay-network-layer-manager'; import Author, {nullAuthor} from './author'; +import {UNAUTHENTICATED} from '../shared/keytar-strategy'; import ModelObserver from './model-observer'; // This is a guess about what a reasonable value is. Can adjust if performance is poor. const MAX_COMMITS = 5000; export default class UserStore { - constructor({repository}) { + constructor({repository, login}) { this.emitter = new Emitter(); // TODO: [ku 3/2018] Consider using Dexie (indexDB wrapper) like Desktop and persist users across sessions @@ -23,12 +24,19 @@ export default class UserStore { authors: r.getAuthors({max: MAX_COMMITS}), remotes: r.getRemotes(), }), - didUpdate: () => this.loadUsers(this.repositoryObserver.getActiveModelData()), + didUpdate: () => this.loadUsers(), }); this.repositoryObserver.setActiveModel(repository); + + this.loginObserver = new ModelObserver({ + didUpdate: () => this.loadUsers(), + }); + this.loginObserver.setActiveModel(login); } - async loadUsers(data) { + async loadUsers() { + const data = this.repositoryObserver.getActiveModelData(); + if (!data) { return; } @@ -50,12 +58,18 @@ export default class UserStore { } async loadMentionableUsers(remote) { - const fetchQuery = RelayNetworkLayerManager.getExistingFetchQuery('https://api.github.com/graphql'); - if (!fetchQuery) { - // No authentication token + const loginModel = this.loginObserver.getActiveModel(); + if (!loginModel) { return; } + const token = await loginModel.getToken('https://api.github.com'); + if (token === UNAUTHENTICATED) { + return; + } + + const fetchQuery = RelayNetworkLayerManager.getFetchQuery('https://api.github.com/graphql', token); + let hasMore = true; let cursor = null; diff --git a/lib/relay-network-layer-manager.js b/lib/relay-network-layer-manager.js index f63f74b92c..cf1a821b79 100644 --- a/lib/relay-network-layer-manager.js +++ b/lib/relay-network-layer-manager.js @@ -100,7 +100,7 @@ export default class RelayNetworkLayerManager { if (!environment) { const source = new RecordSource(); const store = new Store(source); - network = Network.create(this.getExistingFetchQuery(url)); + network = Network.create(this.getFetchQuery(url)); environment = new Environment({network, store}); relayEnvironmentPerGithubHost.set(host, {environment, network}); @@ -108,11 +108,8 @@ export default class RelayNetworkLayerManager { return environment; } - static getExistingFetchQuery(url) { - if (!tokenPerURL.has(url)) { - return null; - } - + static getFetchQuery(url, token) { + tokenPerURL.set(url, token); let fetch = fetchPerURL.get(url); if (!fetch) { fetch = createFetchQuery(url); diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index 66bd3ca47c..7aac4b49ac 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -2,10 +2,27 @@ import dedent from 'dedent-js'; import UserStore from '../../lib/models/user-store'; import Author, {nullAuthor} from '../../lib/models/author'; -import RelayNetworkLayerManager, {expectRelayQuery} from '../../lib/relay-network-layer-manager'; +import GithubLoginModel from '../../lib/models/github-login-model'; +import {InMemoryStrategy} from '../../lib/shared/keytar-strategy'; +import {expectRelayQuery} from '../../lib/relay-network-layer-manager'; import {cloneRepository, buildRepository, FAKE_USER} from '../helpers'; describe('UserStore', function() { + let login; + + function nextUpdatePromise(store) { + return new Promise(resolve => { + const sub = store.onDidUpdate(() => { + sub.dispose(); + resolve(); + }); + }); + } + + beforeEach(function() { + login = new GithubLoginModel(InMemoryStrategy); + }); + it('loads store with local git users and committer in a repo with no GitHub remote', async function() { const workdirPath = await cloneRepository('multiple-commits'); const repository = await buildRepository(workdirPath); @@ -15,14 +32,15 @@ describe('UserStore', function() { assert.strictEqual(store.committer, nullAuthor); // Store is populated asynchronously - await assert.async.deepEqual(store.getUsers(), [ + await nextUpdatePromise(store); + assert.deepEqual(store.getUsers(), [ new Author('kuychaco@github.com', 'Katrina Uychaco'), ]); - await assert.async.deepEqual(store.committer, new Author(FAKE_USER.email, FAKE_USER.name)); + assert.deepEqual(store.committer, new Author(FAKE_USER.email, FAKE_USER.name)); }); it('loads store with mentionable users from the GitHub API in a repo with a GitHub remote', async function() { - RelayNetworkLayerManager.getEnvironmentForHost('https://api.github.com', '1234'); + await login.setToken('https://api.github.com', '1234'); const workdirPath = await cloneRepository('multiple-commits'); const repository = await buildRepository(workdirPath); @@ -51,12 +69,13 @@ describe('UserStore', function() { }, }); - const store = new UserStore({repository}); - assert.deepEqual(store.getUsers(), []); + const store = new UserStore({repository, login}); + await nextUpdatePromise(store); resolve(); + await nextUpdatePromise(store); - await assert.async.deepEqual(store.getUsers(), [ + assert.deepEqual(store.getUsers(), [ new Author('smashwilson@github.com', 'Ash Wilson', 'smashwilson'), new Author('mona@lisa.com', 'Mona Lisa', 'octocat'), new Author('annthurium@github.com', 'Tilde Ann Thurium', 'annthurium'), @@ -64,7 +83,7 @@ describe('UserStore', function() { }); it('loads users from multiple pages from the GitHub API', async function() { - RelayNetworkLayerManager.getEnvironmentForHost('https://api.github.com', '1234'); + await login.setToken('https://api.github.com', '1234'); const workdirPath = await cloneRepository('multiple-commits'); const repository = await buildRepository(workdirPath); @@ -109,18 +128,24 @@ describe('UserStore', function() { }, }); - const store = new UserStore({repository}); + const store = new UserStore({repository, login}); + + await nextUpdatePromise(store); assert.deepEqual(store.getUsers(), []); resolve0(); - await assert.async.deepEqual(store.getUsers(), [ + await nextUpdatePromise(store); + + assert.deepEqual(store.getUsers(), [ new Author('smashwilson@github.com', 'Ash Wilson', 'smashwilson'), new Author('mona@lisa.com', 'Mona Lisa', 'octocat'), new Author('annthurium@github.com', 'Tilde Ann Thurium', 'annthurium'), ]); resolve1(); - await assert.async.deepEqual(store.getUsers(), [ + await nextUpdatePromise(store); + + assert.deepEqual(store.getUsers(), [ new Author('aaa@github.com', 'Aahhhhh', 'aaa'), new Author('smashwilson@github.com', 'Ash Wilson', 'smashwilson'), new Author('mona@lisa.com', 'Mona Lisa', 'octocat'), @@ -130,7 +155,7 @@ describe('UserStore', function() { }); it('infers no-reply emails for users without a public email address', async function() { - RelayNetworkLayerManager.getEnvironmentForHost('https://api.github.com', '1234'); + await login.setToken('https://api.github.com', '1234'); const workdirPath = await cloneRepository('multiple-commits'); const repository = await buildRepository(workdirPath); @@ -155,10 +180,13 @@ describe('UserStore', function() { }, }); - const store = new UserStore({repository}); + const store = new UserStore({repository, login}); + await nextUpdatePromise(store); resolve(); - await assert.async.deepEqual(store.getUsers(), [ + await nextUpdatePromise(store); + + assert.deepEqual(store.getUsers(), [ new Author('simurai@users.noreply.github.com', 'simurai', 'simurai'), ]); }); @@ -212,18 +240,18 @@ describe('UserStore', function() { const repository = await buildRepository(workdirPath); const store = new UserStore({repository}); - await assert.async.deepEqual(store.committer, new Author(FAKE_USER.email, FAKE_USER.name)); + await nextUpdatePromise(store); + assert.deepEqual(store.committer, new Author(FAKE_USER.email, FAKE_USER.name)); const newEmail = 'foo@bar.com'; - await repository.setConfig('user.email', newEmail); - - repository.refresh(); - await assert.async.deepEqual(store.committer, new Author(FAKE_USER.email, FAKE_USER.name)); - const newName = 'Foo Bar'; + + await repository.setConfig('user.email', newEmail); await repository.setConfig('user.name', newName); repository.refresh(); - await assert.async.deepEqual(store.committer, new Author(newEmail, newName)); + await nextUpdatePromise(store); + + assert.deepEqual(store.committer, new Author(newEmail, newName)); }); it('refetches users when HEAD changes', async function() { @@ -235,7 +263,8 @@ describe('UserStore', function() { await repository.checkout('master'); const store = new UserStore({repository}); - await assert.async.deepEqual(store.getUsers(), [ + await nextUpdatePromise(store); + assert.deepEqual(store.getUsers(), [ new Author('kuychaco@github.com', 'Katrina Uychaco'), ]); @@ -248,13 +277,76 @@ describe('UserStore', function() { Co-authored-by: New Author `, {allowEmpty: true}); - await assert.async.equal(store.addUsers.callCount, 1); + repository.refresh(); + await nextUpdatePromise(store); + + await assert.strictEqual(store.addUsers.callCount, 1); assert.isTrue(store.getUsers().some(user => { return user.getFullName() === 'New Author' && user.getEmail() === 'new-author@email.com'; })); // Change head due to branch checkout await repository.checkout('new-branch'); - await assert.async.equal(store.addUsers.callCount, 2); + repository.refresh(); + + await assert.async.strictEqual(store.addUsers.callCount, 2); + }); + + it('refetches users when a token becomes available', async function() { + const workdirPath = await cloneRepository('multiple-commits'); + const repository = await buildRepository(workdirPath); + + const gitAuthors = [ + new Author('kuychaco@github.com', 'Katrina Uychaco'), + ]; + + const graphqlAuthors = [ + new Author('smashwilson@github.com', 'Ash Wilson', 'smashwilson'), + new Author('mona@lisa.com', 'Mona Lisa', 'octocat'), + new Author('annthurium@github.com', 'Tilde Ann Thurium', 'annthurium'), + ]; + + const {resolve} = expectRelayQuery({ + name: 'GetMentionableUsers', + variables: {owner: 'me', name: 'stuff', first: 100, after: null}, + }, { + repository: { + mentionableUsers: { + nodes: [ + {login: 'annthurium', email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, + {login: 'octocat', email: 'mona@lisa.com', name: 'Mona Lisa'}, + {login: 'smashwilson', email: 'smashwilson@github.com', name: 'Ash Wilson'}, + ], + pageInfo: { + hasNextPage: false, + endCursor: null, + }, + }, + }, + }); + resolve(); + + const store = new UserStore({repository, login}); + await nextUpdatePromise(store); + console.log('initial load complete'); + + assert.deepEqual(store.getUsers(), gitAuthors); + + await repository.setConfig('remote.origin.url', 'git@github.com:me/stuff.git'); + await repository.setConfig('remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*'); + + console.log('about to refresh repository'); + repository.refresh(); + console.log('updated after repository refresh'); + + // Token is not available, so authors are still queried from git + assert.deepEqual(store.getUsers(), gitAuthors); + + console.log('about to fire login update'); + await login.setToken('https://api.github.com', '1234'); + + await nextUpdatePromise(store); + console.log('updated after login update'); + assert.deepEqual(store.getUsers(), graphqlAuthors); }); }); From 138dadc562ad13b7aeacd57991ac130abf5751da Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 23 May 2018 12:35:58 -0400 Subject: [PATCH 31/64] Track the last source of users in a UserStore --- lib/models/user-store.js | 23 +++++++++++++++++++---- test/models/user-store.test.js | 7 ++++--- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index f0898b8d4a..df3abf61c2 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -9,6 +9,12 @@ import ModelObserver from './model-observer'; // This is a guess about what a reasonable value is. Can adjust if performance is poor. const MAX_COMMITS = 5000; +export const source = { + PENDING: Symbol('pending'), + GITLOG: Symbol('git log'), + GITHUBAPI: Symbol('github API'), +}; + export default class UserStore { constructor({repository, login}) { this.emitter = new Emitter(); @@ -18,6 +24,10 @@ export default class UserStore { this.users = []; this.committer = nullAuthor; + this.last = { + source: source.PENDING, + }; + this.repositoryObserver = new ModelObserver({ fetchData: r => yubikiri({ committer: r.getCommitter(), @@ -42,10 +52,10 @@ export default class UserStore { } this.setCommitter(data.committer); - const githubRemotes = data.remotes.filter(remote => remote.isGithubRepo()); + if (githubRemotes.length === 0) { - this.addUsers(data.authors); + this.addUsers(data.authors, source.GITLOG); } else { await this.loadUsersFromGraphQL(githubRemotes); } @@ -108,14 +118,18 @@ export default class UserStore { } return new Author(node.email, node.name, node.login); - })); + }), source.GITHUBAPI); cursor = connection.pageInfo.endCursor; hasMore = connection.pageInfo.hasNextPage; } } - addUsers(users) { + addUsers(users, nextSource) { + if (nextSource !== this.last.source) { + this.allUsers.clear(); + } + let changed = false; for (const author of users) { if (!this.allUsers.has(author.getEmail())) { @@ -127,6 +141,7 @@ export default class UserStore { if (changed) { this.finalize(); } + this.last.source = nextSource; } finalize() { diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index 7aac4b49ac..21663da61f 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -1,6 +1,6 @@ import dedent from 'dedent-js'; -import UserStore from '../../lib/models/user-store'; +import UserStore, {source} from '../../lib/models/user-store'; import Author, {nullAuthor} from '../../lib/models/author'; import GithubLoginModel from '../../lib/models/github-login-model'; import {InMemoryStrategy} from '../../lib/shared/keytar-strategy'; @@ -219,13 +219,14 @@ describe('UserStore', function() { const workdirPath = await cloneRepository('multiple-commits'); const repository = await buildRepository(workdirPath); const store = new UserStore({repository}); + await nextUpdatePromise(store); - await assert.async.lengthOf(store.getUsers(), 1); + assert.lengthOf(store.getUsers(), 1); store.addUsers([ new Author('mona@lisa.com', 'Mona Lisa'), new Author('hubot@github.com', 'Hubot Robot'), - ]); + ], source.GITLOG); assert.deepEqual(store.getUsers(), [ new Author('hubot@github.com', 'Hubot Robot'), From bb734d4a958c6018e4aa7e1b41e80146ac2bff7a Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 23 May 2018 13:53:38 -0400 Subject: [PATCH 32/64] :fire: console.logs --- test/models/user-store.test.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index 21663da61f..a516377bd2 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -329,25 +329,20 @@ describe('UserStore', function() { const store = new UserStore({repository, login}); await nextUpdatePromise(store); - console.log('initial load complete'); assert.deepEqual(store.getUsers(), gitAuthors); await repository.setConfig('remote.origin.url', 'git@github.com:me/stuff.git'); await repository.setConfig('remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*'); - console.log('about to refresh repository'); repository.refresh(); - console.log('updated after repository refresh'); // Token is not available, so authors are still queried from git assert.deepEqual(store.getUsers(), gitAuthors); - console.log('about to fire login update'); await login.setToken('https://api.github.com', '1234'); await nextUpdatePromise(store); - console.log('updated after login update'); assert.deepEqual(store.getUsers(), graphqlAuthors); }); }); From 6bf5413b9c6ad48c4340f260ee670ba7f9aff4de Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 23 May 2018 14:39:38 -0400 Subject: [PATCH 33/64] Override results when the Repository has changed --- lib/models/user-store.js | 7 ++++++- test/models/user-store.test.js | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index df3abf61c2..45b2b50030 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -26,6 +26,7 @@ export default class UserStore { this.last = { source: source.PENDING, + repository: null, }; this.repositoryObserver = new ModelObserver({ @@ -126,7 +127,10 @@ export default class UserStore { } addUsers(users, nextSource) { - if (nextSource !== this.last.source) { + if ( + nextSource !== this.last.source || + this.repositoryObserver.getActiveModel() !== this.last.repository + ) { this.allUsers.clear(); } @@ -142,6 +146,7 @@ export default class UserStore { this.finalize(); } this.last.source = nextSource; + this.last.repository = this.repositoryObserver.getActiveModel(); } finalize() { diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index a516377bd2..41ad2de45f 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -345,4 +345,38 @@ describe('UserStore', function() { await nextUpdatePromise(store); assert.deepEqual(store.getUsers(), graphqlAuthors); }); + + it('refetches users when the repository changes', async function() { + const workdirPath0 = await cloneRepository('multiple-commits'); + const repository0 = await buildRepository(workdirPath0); + await repository0.setConfig('user.email', 'committer0@github.com'); + await repository0.setConfig('user.name', 'committer0'); + await repository0.commit('on repo 0', {allowEmpty: true}); + await repository0.setConfig('user.email', 'committer@github.com'); + await repository0.setConfig('user.name', 'committer'); + + const workdirPath1 = await cloneRepository('multiple-commits'); + const repository1 = await buildRepository(workdirPath1); + await repository1.setConfig('user.email', 'committer1@github.com'); + await repository1.setConfig('user.name', 'committer1'); + await repository1.commit('on repo 1', {allowEmpty: true}); + await repository1.setConfig('user.email', 'committer@github.com'); + await repository1.setConfig('user.name', 'committer'); + + const store = new UserStore({repository: repository0}); + await nextUpdatePromise(store); + + assert.deepEqual(store.getUsers(), [ + new Author('kuychaco@github.com', 'Katrina Uychaco'), + new Author('committer0@github.com', 'committer0'), + ]); + + store.setRepository(repository1); + await nextUpdatePromise(store); + + assert.deepEqual(store.getUsers(), [ + new Author('kuychaco@github.com', 'Katrina Uychaco'), + new Author('committer1@github.com', 'committer1'), + ]); + }); }); From 92a2cfad00d48dc28ce71fb95c2ba483399b2413 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 23 May 2018 15:18:39 -0400 Subject: [PATCH 34/64] Pass the GithubLoginModel to the UserStore --- lib/controllers/git-tab-controller.js | 7 ++++++- lib/controllers/root-controller.js | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/controllers/git-tab-controller.js b/lib/controllers/git-tab-controller.js index e57e06ff70..d773a5cee7 100644 --- a/lib/controllers/git-tab-controller.js +++ b/lib/controllers/git-tab-controller.js @@ -15,6 +15,7 @@ export default class GitTabController extends React.Component { static propTypes = { repository: PropTypes.object.isRequired, + loginModel: PropTypes.object.isRequired, lastCommit: CommitPropType.isRequired, recentCommits: PropTypes.arrayOf(CommitPropType).isRequired, @@ -65,7 +66,10 @@ export default class GitTabController extends React.Component { selectedCoAuthors: [], }; - this.userStore = new UserStore({repository: this.props.repository}); + this.userStore = new UserStore({ + repository: this.props.repository, + login: this.props.loginModel, + }); } render() { @@ -131,6 +135,7 @@ export default class GitTabController extends React.Component { componentDidUpdate() { this.userStore.setRepository(this.props.repository); + this.userStore.setLoginModel(this.props.loginModel); this.refreshResolutionProgress(false, false); } diff --git a/lib/controllers/root-controller.js b/lib/controllers/root-controller.js index ca1e10db50..3e9cde0d52 100644 --- a/lib/controllers/root-controller.js +++ b/lib/controllers/root-controller.js @@ -193,6 +193,7 @@ export default class RootController extends React.Component { confirm={this.props.confirm} config={this.props.config} repository={this.props.repository} + loginModel={this.loginModel} initializeRepo={this.initializeRepo} resolutionProgress={this.props.resolutionProgress} ensureGitTab={this.gitTabTracker.ensureVisible} From 79b10a1dc4ad495d1c3ec047baab2c9b16b24162 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 23 May 2018 15:18:54 -0400 Subject: [PATCH 35/64] Declare query variables --- lib/models/user-store.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index 45b2b50030..cbd0e54c21 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -88,7 +88,7 @@ export default class UserStore { const response = await fetchQuery({ name: 'GetMentionableUsers', text: ` - query GetMentionableUsers { + query GetMentionableUsers($owner: String!, $name: String!, $first: Int!, $after: String) { repository(owner: $owner, name: $name) { mentionableUsers(first: $first, after: $after) { nodes { From 4f8906a14a9bb1b3c421a89f3ce5ddbc4e3efb18 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 23 May 2018 15:19:16 -0400 Subject: [PATCH 36/64] Change the loginModel --- lib/models/user-store.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index cbd0e54c21..0bc9a594c3 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -167,6 +167,11 @@ export default class UserStore { this.repositoryObserver.setActiveModel(repository); } + setLoginModel(login) { + this.loginObserver.setActiveModel(login); + console.log('setLoginModel login model:', login); + } + setCommitter(committer) { const changed = !this.committer.matches(committer); this.committer = committer; From 06c073a654e713ebc3fc8a93e631b6ee4a297d18 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 24 May 2018 14:08:59 -0400 Subject: [PATCH 37/64] Create and test for new Authors --- lib/models/author.js | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/lib/models/author.js b/lib/models/author.js index f2f6f32660..4a699ecd82 100644 --- a/lib/models/author.js +++ b/lib/models/author.js @@ -1,10 +1,17 @@ +const NEW = Symbol('new'); + export const NO_REPLY_GITHUB_EMAIL = 'noreply@github.com'; export default class Author { - constructor(email, fullName, login = null) { + constructor(email, fullName, login = null, isNew = null) { this.email = email; this.fullName = fullName; this.login = login; + this.new = isNew === NEW; + } + + static createNew(email, fullName) { + return new this(email, fullName, null, NEW); } getEmail() { @@ -27,6 +34,10 @@ export default class Author { return this.login !== null; } + isNew() { + return this.new; + } + isPresent() { return true; } @@ -71,6 +82,10 @@ export const nullAuthor = { return false; }, + isNew() { + return false; + }, + isPresent() { return false; }, From 423f25807671b97c9d7682cd2f4fa4288a036cc6 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 24 May 2018 14:09:14 -0400 Subject: [PATCH 38/64] Create Author instances in CoAuthorForm --- lib/views/co-author-form.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/views/co-author-form.js b/lib/views/co-author-form.js index a4aa6b0bbf..23d5f9f1ba 100644 --- a/lib/views/co-author-form.js +++ b/lib/views/co-author-form.js @@ -1,6 +1,7 @@ import React from 'react'; import PropTypes from 'prop-types'; +import Author from '../models/author'; import Commands, {Command} from '../atom/commands'; import {autobind} from '../helpers'; @@ -75,7 +76,7 @@ export default class CoAuthorForm extends React.Component { confirm() { if (this.isInputValid()) { - this.props.onSubmit({name: this.state.name, email: this.state.email}); + this.props.onSubmit(new Author(this.state.email, this.state.name)); } } From 137f61ab6a447358760a6b4ab5aec48c47ad4c64 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 24 May 2018 14:09:30 -0400 Subject: [PATCH 39/64] Use Author models in the CommitView co-author forms --- lib/views/commit-view.js | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/lib/views/commit-view.js b/lib/views/commit-view.js index 9c79af933f..40b9b9ca07 100644 --- a/lib/views/commit-view.js +++ b/lib/views/commit-view.js @@ -8,6 +8,7 @@ import Tooltip from '../atom/tooltip'; import AtomTextEditor from '../atom/atom-text-editor'; import CoAuthorForm from './co-author-form'; import RefHolder from '../models/ref-holder'; +import Author from '../models/author'; import ObserveModel from './observe-model'; import {LINE_ENDING_REGEX, autobind} from '../helpers'; import {AuthorPropType, UserStorePropType} from '../prop-types'; @@ -260,7 +261,7 @@ export default class CommitView extends React.Component { placeholder="Co-Authors" arrowRenderer={null} options={mentionableUsers} - labelKey="name" + labelKey="fullName" valueKey="email" filterOptions={this.matchAuthors} optionRenderer={this.renderCoAuthorListItem} @@ -468,11 +469,12 @@ export default class CommitView extends React.Component { matchAuthors(authors, filterText, selectedAuthors) { const matchedAuthors = authors.filter((author, index) => { - const isAlreadySelected = selectedAuthors && selectedAuthors.find(selected => selected.email === author.email); - const matchesFilter = `${author.name}${author.email}`.toLowerCase().indexOf(filterText.toLowerCase()) !== -1; + const isAlreadySelected = selectedAuthors && selectedAuthors.find(selected => selected.matches(author)); + const matchesFilter = `${author.getFullName()}${author.getEmail()}`.toLowerCase() + .indexOf(filterText.toLowerCase()) !== -1; return !isAlreadySelected && matchesFilter; }); - matchedAuthors.push({name: filterText, email: 'Add new author', isNew: true}); + matchedAuthors.push(Author.createNew('Add new author', filterText)); return matchedAuthors; } @@ -488,24 +490,24 @@ export default class CommitView extends React.Component { renderCoAuthorListItem(author) { return ( -
- {this.renderCoAuthorListItemField('name', author.name)} - {this.renderCoAuthorListItemField('email', author.email)} +
+ {this.renderCoAuthorListItemField('name', author.getFullName())} + {this.renderCoAuthorListItemField('email', author.getEmail())}
); } renderCoAuthorValue(author) { return ( - {author.name} + {author.getFullName()} ); } onSelectedCoAuthorsChanged(selectedCoAuthors) { - const newAuthor = selectedCoAuthors.find(author => author.isNew); + const newAuthor = selectedCoAuthors.find(author => author.isNew()); if (newAuthor) { - this.setState({coAuthorInput: newAuthor.name, showCoAuthorForm: true}); + this.setState({coAuthorInput: newAuthor.getFullName(), showCoAuthorForm: true}); } else { this.props.updateSelectedCoAuthors(selectedCoAuthors); } From 90394fcdd5d4453d3251e2746f93267cda373ae6 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 24 May 2018 14:25:18 -0400 Subject: [PATCH 40/64] Update CoAuthor form test --- test/views/co-author-form.test.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/views/co-author-form.test.js b/test/views/co-author-form.test.js index cc1167f82f..b2980682ce 100644 --- a/test/views/co-author-form.test.js +++ b/test/views/co-author-form.test.js @@ -2,6 +2,7 @@ import React from 'react'; import {mount} from 'enzyme'; import CoAuthorForm from '../../lib/views/co-author-form'; +import Author from '../../lib/models/author'; describe('CoAuthorForm', function() { let atomEnv; @@ -50,10 +51,7 @@ describe('CoAuthorForm', function() { wrapper.find('.btn-primary').simulate('click'); - assert.deepEqual(didSubmit.firstCall.args[0], { - name, - email, - }); + assert.deepEqual(didSubmit.firstCall.args[0], new Author(email, name)); }); it('submit button is initially disabled', function() { From 25c3edd64f55a93ad30076da8061cfae9ba1302f Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 24 May 2018 14:37:24 -0400 Subject: [PATCH 41/64] :fire: console.log again --- lib/models/user-store.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index 0bc9a594c3..291c9dc861 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -169,7 +169,6 @@ export default class UserStore { setLoginModel(login) { this.loginObserver.setActiveModel(login); - console.log('setLoginModel login model:', login); } setCommitter(committer) { From db7e78f01e7f044d8021dee41cc75444d6ad470e Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 24 May 2018 14:58:40 -0400 Subject: [PATCH 42/64] Create a helper for stubbing paginated data --- test/models/user-store.test.js | 144 +++++++++++++-------------------- 1 file changed, 56 insertions(+), 88 deletions(-) diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index 41ad2de45f..4a7d2d5f04 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -19,6 +19,38 @@ describe('UserStore', function() { }); } + function expectPagedRelayQueries(options, ...pages) { + const opts = { + owner: 'me', + name: 'stuff', + ...options, + }; + + let lastCursor = null; + return pages.map((page, index) => { + const isLast = index === pages.length - 1; + const nextCursor = isLast ? null : `page-${index + 1}`; + + const {resolve} = expectRelayQuery({ + name: 'GetMentionableUsers', + variables: {owner: opts.owner, name: opts.name, first: 100, after: lastCursor}, + }, { + repository: { + mentionableUsers: { + nodes: page, + pageInfo: { + hasNextPage: !isLast, + endCursor: nextCursor, + }, + }, + }, + }); + + lastCursor = nextCursor; + return resolve; + }); + } + beforeEach(function() { login = new GithubLoginModel(InMemoryStrategy); }); @@ -50,24 +82,11 @@ describe('UserStore', function() { await repository.setConfig('remote.old.url', 'git@sourceforge.com:me/stuff.git'); await repository.setConfig('remote.old.fetch', '+refs/heads/*:refs/remotes/old/*'); - const {resolve} = expectRelayQuery({ - name: 'GetMentionableUsers', - variables: {owner: 'me', name: 'stuff', first: 100, after: null}, - }, { - repository: { - mentionableUsers: { - nodes: [ - {login: 'annthurium', email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, - {login: 'octocat', email: 'mona@lisa.com', name: 'Mona Lisa'}, - {login: 'smashwilson', email: 'smashwilson@github.com', name: 'Ash Wilson'}, - ], - pageInfo: { - hasNextPage: false, - endCursor: null, - }, - }, - }, - }); + const [resolve] = expectPagedRelayQueries({}, [ + {login: 'annthurium', email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, + {login: 'octocat', email: 'mona@lisa.com', name: 'Mona Lisa'}, + {login: 'smashwilson', email: 'smashwilson@github.com', name: 'Ash Wilson'}, + ]); const store = new UserStore({repository, login}); await nextUpdatePromise(store); @@ -91,42 +110,17 @@ describe('UserStore', function() { await repository.setConfig('remote.origin.url', 'git@github.com:me/stuff.git'); await repository.setConfig('remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*'); - const {resolve: resolve0} = expectRelayQuery({ - name: 'GetMentionableUsers', - variables: {owner: 'me', name: 'stuff', first: 100, after: null}, - }, { - repository: { - mentionableUsers: { - nodes: [ - {login: 'annthurium', email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, - {login: 'octocat', email: 'mona@lisa.com', name: 'Mona Lisa'}, - {login: 'smashwilson', email: 'smashwilson@github.com', name: 'Ash Wilson'}, - ], - pageInfo: { - hasNextPage: true, - endCursor: 'foo', - }, - }, - }, - }); - - const {resolve: resolve1} = expectRelayQuery({ - name: 'GetMentionableUsers', - variables: {owner: 'me', name: 'stuff', first: 100, after: 'foo'}, - }, { - repository: { - mentionableUsers: { - nodes: [ - {login: 'zzz', email: 'zzz@github.com', name: 'Zzzzz'}, - {login: 'aaa', email: 'aaa@github.com', name: 'Aahhhhh'}, - ], - pageInfo: { - hasNextPage: false, - endCursor: 'bar', - }, - }, - }, - }); + const [resolve0, resolve1] = expectPagedRelayQueries({}, + [ + {login: 'annthurium', email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, + {login: 'octocat', email: 'mona@lisa.com', name: 'Mona Lisa'}, + {login: 'smashwilson', email: 'smashwilson@github.com', name: 'Ash Wilson'}, + ], + [ + {login: 'zzz', email: 'zzz@github.com', name: 'Zzzzz'}, + {login: 'aaa', email: 'aaa@github.com', name: 'Aahhhhh'}, + ], + ); const store = new UserStore({repository, login}); @@ -163,22 +157,9 @@ describe('UserStore', function() { await repository.setConfig('remote.origin.url', 'git@github.com:me/stuff.git'); await repository.setConfig('remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*'); - const {resolve} = expectRelayQuery({ - name: 'GetMentionableUsers', - variables: {owner: 'me', name: 'stuff', first: 100, after: null}, - }, { - repository: { - mentionableUsers: { - nodes: [ - {login: 'simurai', email: '', name: 'simurai'}, - ], - pageInfo: { - hasNextPage: false, - endCursor: null, - }, - }, - }, - }); + const [resolve] = expectPagedRelayQueries({}, [ + {login: 'simurai', email: '', name: 'simurai'}, + ]); const store = new UserStore({repository, login}); await nextUpdatePromise(store); @@ -307,24 +288,11 @@ describe('UserStore', function() { new Author('annthurium@github.com', 'Tilde Ann Thurium', 'annthurium'), ]; - const {resolve} = expectRelayQuery({ - name: 'GetMentionableUsers', - variables: {owner: 'me', name: 'stuff', first: 100, after: null}, - }, { - repository: { - mentionableUsers: { - nodes: [ - {login: 'annthurium', email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, - {login: 'octocat', email: 'mona@lisa.com', name: 'Mona Lisa'}, - {login: 'smashwilson', email: 'smashwilson@github.com', name: 'Ash Wilson'}, - ], - pageInfo: { - hasNextPage: false, - endCursor: null, - }, - }, - }, - }); + const [resolve] = expectPagedRelayQueries({}, [ + {login: 'annthurium', email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, + {login: 'octocat', email: 'mona@lisa.com', name: 'Mona Lisa'}, + {login: 'smashwilson', email: 'smashwilson@github.com', name: 'Ash Wilson'}, + ]); resolve(); const store = new UserStore({repository, login}); From 0d3b316f97ff63e2eddfad7532e8bfa1c7c29114 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 24 May 2018 16:27:31 -0400 Subject: [PATCH 43/64] getSlug() on Remote because "slug" is more fun than "nwo" --- lib/models/remote.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/models/remote.js b/lib/models/remote.js index e272d62767..d7002a3f13 100644 --- a/lib/models/remote.js +++ b/lib/models/remote.js @@ -33,6 +33,10 @@ export default class Remote { return this.getName(); } + getSlug() { + return `${this.owner}/${this.repo}`; + } + isPresent() { return true; } @@ -90,6 +94,10 @@ export const nullRemote = { return fallback; }, + getSlug() { + return ''; + }, + isPresent() { return false; }, From 25095cacbc25f44074c3660bf1807545c168a661 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 24 May 2018 16:27:55 -0400 Subject: [PATCH 44/64] Disable stubbed GraphQL queries --- lib/relay-network-layer-manager.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/relay-network-layer-manager.js b/lib/relay-network-layer-manager.js index cf1a821b79..390d01ba85 100644 --- a/lib/relay-network-layer-manager.js +++ b/lib/relay-network-layer-manager.js @@ -26,7 +26,9 @@ export function expectRelayQuery(operationPattern, response) { existing.push({promise, response, variables: operationPattern.variables || {}}); responsesByQuery.set(operationPattern.name, existing); - return {promise, resolve, reject}; + const disable = () => responsesByQuery.delete(operationPattern.name); + + return {promise, resolve, reject, disable}; } export function clearRelayExpectations() { From d687e7b7df50396601cd9fbf935ce492a29f6f93 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 24 May 2018 16:28:22 -0400 Subject: [PATCH 45/64] Cache GraphQL responses for an hour --- lib/models/user-store.js | 44 ++++++++++++++-- test/models/user-store.test.js | 95 +++++++++++++++++++++++++++++++--- 2 files changed, 130 insertions(+), 9 deletions(-) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index 291c9dc861..ca8cdb302e 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -15,6 +15,33 @@ export const source = { GITHUBAPI: Symbol('github API'), }; +class GraphQLCache { + // One hour + static MAX_AGE_MS = 3.6e6 + + constructor() { + this.bySlug = new Map(); + } + + get(remote) { + const slug = remote.getSlug(); + const {ts, data} = this.bySlug.get(slug) || { + ts: -Infinity, + data: {}, + }; + + if (Date.now() - ts > this.constructor.MAX_AGE_MS) { + this.bySlug.delete(slug); + return null; + } + return data; + } + + set(remote, data) { + this.bySlug.set(remote.getSlug(), {ts: Date.now(), data}); + } +} + export default class UserStore { constructor({repository, login}) { this.emitter = new Emitter(); @@ -28,6 +55,7 @@ export default class UserStore { source: source.PENDING, repository: null, }; + this.cache = new GraphQLCache(); this.repositoryObserver = new ModelObserver({ fetchData: r => yubikiri({ @@ -69,6 +97,12 @@ export default class UserStore { } async loadMentionableUsers(remote) { + const cached = this.cache.get(remote); + if (cached !== null) { + this.addUsers(cached, source.GITHUBAPI); + return; + } + const loginModel = this.loginObserver.getActiveModel(); if (!loginModel) { return; @@ -83,6 +117,7 @@ export default class UserStore { let hasMore = true; let cursor = null; + const remoteUsers = []; while (hasMore) { const response = await fetchQuery({ @@ -112,18 +147,21 @@ export default class UserStore { }); const connection = response.data.repository.mentionableUsers; - - this.addUsers(connection.nodes.map(node => { + const authors = connection.nodes.map(node => { if (node.email === '') { node.email = `${node.login}@users.noreply.github.com`; } return new Author(node.email, node.name, node.login); - }), source.GITHUBAPI); + }); + this.addUsers(authors, source.GITHUBAPI); + remoteUsers.push(...authors); cursor = connection.pageInfo.endCursor; hasMore = connection.pageInfo.hasNextPage; } + + this.cache.set(remote, remoteUsers); } addUsers(users, nextSource) { diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index 4a7d2d5f04..a8f9e8e476 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -31,7 +31,7 @@ describe('UserStore', function() { const isLast = index === pages.length - 1; const nextCursor = isLast ? null : `page-${index + 1}`; - const {resolve} = expectRelayQuery({ + const result = expectRelayQuery({ name: 'GetMentionableUsers', variables: {owner: opts.owner, name: opts.name, first: 100, after: lastCursor}, }, { @@ -47,7 +47,7 @@ describe('UserStore', function() { }); lastCursor = nextCursor; - return resolve; + return result; }); } @@ -82,7 +82,7 @@ describe('UserStore', function() { await repository.setConfig('remote.old.url', 'git@sourceforge.com:me/stuff.git'); await repository.setConfig('remote.old.fetch', '+refs/heads/*:refs/remotes/old/*'); - const [resolve] = expectPagedRelayQueries({}, [ + const [{resolve}] = expectPagedRelayQueries({}, [ {login: 'annthurium', email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, {login: 'octocat', email: 'mona@lisa.com', name: 'Mona Lisa'}, {login: 'smashwilson', email: 'smashwilson@github.com', name: 'Ash Wilson'}, @@ -110,7 +110,7 @@ describe('UserStore', function() { await repository.setConfig('remote.origin.url', 'git@github.com:me/stuff.git'); await repository.setConfig('remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*'); - const [resolve0, resolve1] = expectPagedRelayQueries({}, + const [{resolve: resolve0}, {resolve: resolve1}] = expectPagedRelayQueries({}, [ {login: 'annthurium', email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, {login: 'octocat', email: 'mona@lisa.com', name: 'Mona Lisa'}, @@ -157,7 +157,7 @@ describe('UserStore', function() { await repository.setConfig('remote.origin.url', 'git@github.com:me/stuff.git'); await repository.setConfig('remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*'); - const [resolve] = expectPagedRelayQueries({}, [ + const [{resolve}] = expectPagedRelayQueries({}, [ {login: 'simurai', email: '', name: 'simurai'}, ]); @@ -288,7 +288,7 @@ describe('UserStore', function() { new Author('annthurium@github.com', 'Tilde Ann Thurium', 'annthurium'), ]; - const [resolve] = expectPagedRelayQueries({}, [ + const [{resolve}] = expectPagedRelayQueries({}, [ {login: 'annthurium', email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, {login: 'octocat', email: 'mona@lisa.com', name: 'Mona Lisa'}, {login: 'smashwilson', email: 'smashwilson@github.com', name: 'Ash Wilson'}, @@ -347,4 +347,87 @@ describe('UserStore', function() { new Author('committer1@github.com', 'committer1'), ]); }); + + describe('GraphQL response caching', function() { + it('caches mentionable users acquired from GraphQL', async function() { + await login.setToken('https://api.github.com', '1234'); + + const workdirPath = await cloneRepository('multiple-commits'); + const repository = await buildRepository(workdirPath); + + await repository.setConfig('remote.origin.url', 'git@github.com:me/stuff.git'); + await repository.setConfig('remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*'); + + const [{resolve, disable}] = expectPagedRelayQueries({}, [ + {login: 'annthurium', email: 'annthurium@github.com', name: 'Tilde Ann Thurium'}, + {login: 'octocat', email: 'mona@lisa.com', name: 'Mona Lisa'}, + {login: 'smashwilson', email: 'smashwilson@github.com', name: 'Ash Wilson'}, + ]); + resolve(); + + const store = new UserStore({repository, login}); + sinon.spy(store, 'loadUsers'); + + // The first update is triggered by the commiter, the second from GraphQL results arriving. + await nextUpdatePromise(store); + await nextUpdatePromise(store); + + disable(); + + repository.refresh(); + + await assert.async.strictEqual(store.loadUsers.callCount, 2); + await store.loadUsers.returnValues[1]; + + assert.deepEqual(store.getUsers(), [ + new Author('smashwilson@github.com', 'Ash Wilson', 'smashwilson'), + new Author('mona@lisa.com', 'Mona Lisa', 'octocat'), + new Author('annthurium@github.com', 'Tilde Ann Thurium', 'annthurium'), + ]); + }); + + it('re-uses cached users per repository', async function() { + await login.setToken('https://api.github.com', '1234'); + + const workdirPath0 = await cloneRepository('multiple-commits'); + const repository0 = await buildRepository(workdirPath0); + await repository0.setConfig('remote.origin.url', 'git@github.com:me/zero.git'); + await repository0.setConfig('remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*'); + + const workdirPath1 = await cloneRepository('multiple-commits'); + const repository1 = await buildRepository(workdirPath1); + await repository1.setConfig('remote.origin.url', 'git@github.com:me/one.git'); + await repository1.setConfig('remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*'); + + const results = id => [ + {login: 'aaa', email: `aaa-${id}@a.com`, name: 'AAA'}, + {login: 'bbb', email: `bbb-${id}@b.com`, name: 'BBB'}, + {login: 'ccc', email: `ccc-${id}@c.com`, name: 'CCC'}, + ]; + const [{resolve: resolve0, disable: disable0}] = expectPagedRelayQueries({name: 'zero'}, results('0')); + const [{resolve: resolve1, disable: disable1}] = expectPagedRelayQueries({name: 'one'}, results('1')); + resolve0(); + resolve1(); + + const store = new UserStore({repository: repository0, login}); + await nextUpdatePromise(store); + await nextUpdatePromise(store); + + store.setRepository(repository1); + await nextUpdatePromise(store); + + sinon.spy(store, 'loadUsers'); + disable0(); + disable1(); + + store.setRepository(repository0); + await nextUpdatePromise(store); + + assert.deepEqual(store.getUsers(), [ + new Author('aaa-0@a.com', 'AAA', 'aaa'), + new Author('bbb-0@b.com', 'BBB', 'bbb'), + new Author('ccc-0@c.com', 'CCC', 'ccc'), + ]); + }); + }); }); From 3d8073f06e81ad9dfc0050e15730ecfe387c3c89 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 25 May 2018 09:51:12 -0400 Subject: [PATCH 46/64] Check a token's OAuth scopes against the required ones on each getToken --- lib/models/github-login-model.js | 52 ++++++++++++++++++++++++-- lib/shared/keytar-strategy.js | 3 ++ test/models/github-login-model.test.js | 39 ++++++++++++++++++- 3 files changed, 90 insertions(+), 4 deletions(-) diff --git a/lib/models/github-login-model.js b/lib/models/github-login-model.js index 7da9bc6e72..3934e3f8a6 100644 --- a/lib/models/github-login-model.js +++ b/lib/models/github-login-model.js @@ -1,10 +1,15 @@ +import crypto from 'crypto'; import {Emitter} from 'event-kit'; -import {UNAUTHENTICATED, createStrategy} from '../shared/keytar-strategy'; +import {UNAUTHENTICATED, INSUFFICIENT, createStrategy} from '../shared/keytar-strategy'; let instance = null; export default class GithubLoginModel { + // Be sure that we're requesting at least this many scopes on the token we grant through github.atom.io or we'll + // give everyone a really frustrating experience ;-) + static REQUIRED_SCOPES = ['repo', 'user:email'] + static get() { if (!instance) { instance = new GithubLoginModel(); @@ -16,6 +21,7 @@ export default class GithubLoginModel { this._Strategy = Strategy; this._strategy = null; this.emitter = new Emitter(); + this.checked = new Set(); } async getStrategy() { @@ -34,11 +40,34 @@ export default class GithubLoginModel { async getToken(account) { const strategy = await this.getStrategy(); - let password = await strategy.getPassword('atom-github', account); + const password = await strategy.getPassword('atom-github', account); if (!password) { // User is not logged in - password = UNAUTHENTICATED; + return UNAUTHENTICATED; } + + if (/^https?:/.test(account)) { + // Avoid storing tokens in memory longer than necessary. Let's cache token scope checks by storing a set of + // checksums instead. + const hash = crypto.createHash('md5'); + hash.update(password); + const fingerprint = hash.digest('base64'); + + if (!this.checked.has(fingerprint)) { + const scopes = new Set(await this.getScopes(account, password)); + + for (const scope of this.constructor.REQUIRED_SCOPES) { + if (!scopes.has(scope)) { + // Token doesn't have enough OAuth scopes, need to reauthenticate + return INSUFFICIENT; + } + } + + // We're good + this.checked.add(fingerprint); + } + } + return password; } @@ -54,6 +83,23 @@ export default class GithubLoginModel { this.didUpdate(); } + async getScopes(host, token) { + if (atom.inSpecMode()) { + throw new Error('Attempt to check token scopes in specs'); + } + + const response = await fetch(host, { + method: 'HEAD', + headers: {Authorization: `bearer ${token}`}, + }); + + if (response.status !== 200) { + throw new Error(`Unable to check token for OAuth scopes against ${host}: ${await response.text()}`); + } + + return response.headers.get('X-OAuth-Scopes').split(/\s*,\s*/); + } + didUpdate() { this.emitter.emit('did-update'); } diff --git a/lib/shared/keytar-strategy.js b/lib/shared/keytar-strategy.js index b68f71b0ce..86dcea24e2 100644 --- a/lib/shared/keytar-strategy.js +++ b/lib/shared/keytar-strategy.js @@ -18,6 +18,8 @@ if (typeof atom === 'undefined') { const UNAUTHENTICATED = Symbol('UNAUTHENTICATED'); +const INSUFFICIENT = Symbol('INSUFFICIENT'); + class KeytarStrategy { static get keytar() { return require('keytar'); @@ -245,6 +247,7 @@ async function createStrategy() { module.exports = { UNAUTHENTICATED, + INSUFFICIENT, KeytarStrategy, SecurityBinaryStrategy, InMemoryStrategy, diff --git a/test/models/github-login-model.test.js b/test/models/github-login-model.test.js index 0a3e03da85..2925e76e3e 100644 --- a/test/models/github-login-model.test.js +++ b/test/models/github-login-model.test.js @@ -1,5 +1,11 @@ import GithubLoginModel from '../../lib/models/github-login-model'; -import {KeytarStrategy, SecurityBinaryStrategy, InMemoryStrategy, UNAUTHENTICATED} from '../../lib/shared/keytar-strategy'; +import { + KeytarStrategy, + SecurityBinaryStrategy, + InMemoryStrategy, + UNAUTHENTICATED, + INSUFFICIENT, +} from '../../lib/shared/keytar-strategy'; describe('GithubLoginModel', function() { [null, KeytarStrategy, SecurityBinaryStrategy, InMemoryStrategy].forEach(function(Strategy) { @@ -23,4 +29,35 @@ describe('GithubLoginModel', function() { }); }); }); + + describe('required OAuth scopes', function() { + let loginModel; + + beforeEach(async function() { + loginModel = new GithubLoginModel(InMemoryStrategy); + await loginModel.setToken('https://api.github.com', '1234'); + }); + + it('returns INSUFFICIENT if scopes are present', async function() { + sinon.stub(loginModel, 'getScopes').returns(Promise.resolve(['repo'])); + + assert.strictEqual(await loginModel.getToken('https://api.github.com'), INSUFFICIENT); + }); + + it('returns the token if at least the required scopes are present', async function() { + sinon.stub(loginModel, 'getScopes').returns(Promise.resolve(['repo', 'user:email', 'extra'])); + + assert.strictEqual(await loginModel.getToken('https://api.github.com'), '1234'); + }); + + it('caches checked tokens', async function() { + sinon.stub(loginModel, 'getScopes').returns(Promise.resolve(['repo', 'user:email'])); + + assert.strictEqual(await loginModel.getToken('https://api.github.com'), '1234'); + assert.strictEqual(loginModel.getScopes.callCount, 1); + + assert.strictEqual(await loginModel.getToken('https://api.github.com'), '1234'); + assert.strictEqual(loginModel.getScopes.callCount, 1); + }); + }); }); From 875540ba40a8f7ba73c158303dbcc00475f4a1b6 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 25 May 2018 09:52:31 -0400 Subject: [PATCH 47/64] Handle an INSUFFICIENT result in UserStore --- lib/models/user-store.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index ca8cdb302e..057d5ebda4 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -3,7 +3,7 @@ import {Emitter} from 'event-kit'; import RelayNetworkLayerManager from '../relay-network-layer-manager'; import Author, {nullAuthor} from './author'; -import {UNAUTHENTICATED} from '../shared/keytar-strategy'; +import {UNAUTHENTICATED, INSUFFICIENT} from '../shared/keytar-strategy'; import ModelObserver from './model-observer'; // This is a guess about what a reasonable value is. Can adjust if performance is poor. @@ -109,7 +109,7 @@ export default class UserStore { } const token = await loginModel.getToken('https://api.github.com'); - if (token === UNAUTHENTICATED) { + if (token === UNAUTHENTICATED || token === INSUFFICIENT) { return; } From b5c49c635185fb5f2aa203faf94c63c4f5a58724 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 25 May 2018 09:54:10 -0400 Subject: [PATCH 48/64] Stub getScopes in UserStore tests --- test/models/user-store.test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index a8f9e8e476..241f7b0273 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -53,6 +53,7 @@ describe('UserStore', function() { beforeEach(function() { login = new GithubLoginModel(InMemoryStrategy); + sinon.stub(login, 'getScopes').returns(Promise.resolve(GithubLoginModel.REQUIRED_SCOPES)); }); it('loads store with local git users and committer in a repo with no GitHub remote', async function() { From 29e8590d2868f4fe96a79256f93f4c85ba2afb71 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 25 May 2018 13:02:40 -0400 Subject: [PATCH 49/64] Log GraphQL expected variables in spec mode --- lib/relay-network-layer-manager.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/relay-network-layer-manager.js b/lib/relay-network-layer-manager.js index 390d01ba85..69e3e5cb68 100644 --- a/lib/relay-network-layer-manager.js +++ b/lib/relay-network-layer-manager.js @@ -1,3 +1,4 @@ +import util from 'util'; import {Environment, Network, RecordSource, Store} from 'relay-runtime'; import moment from 'moment'; @@ -58,7 +59,10 @@ function createFetchQuery(url) { if (!match) { // eslint-disable-next-line no-console - console.log(`GraphQL query ${operation.name} was:\n ${operation.text.replace(/\n/g, '\n ')}`); + console.log( + `GraphQL query ${operation.name} was:\n ${operation.text.replace(/\n/g, '\n ')}\n` + + util.inspect(variables), + ); const e = new Error(`Unexpected GraphQL query: ${operation.name}`); e.rawStack = e.stack; From 6faf54e52c8a01671f8ba7e5099e2c2175aba721 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 25 May 2018 13:15:42 -0400 Subject: [PATCH 50/64] Handle errors during the getScopes() request --- lib/models/github-login-model.js | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/lib/models/github-login-model.js b/lib/models/github-login-model.js index 3934e3f8a6..1c149235ae 100644 --- a/lib/models/github-login-model.js +++ b/lib/models/github-login-model.js @@ -46,7 +46,7 @@ export default class GithubLoginModel { return UNAUTHENTICATED; } - if (/^https?:/.test(account)) { + if (/^https?:\/\//.test(account)) { // Avoid storing tokens in memory longer than necessary. Let's cache token scope checks by storing a set of // checksums instead. const hash = crypto.createHash('md5'); @@ -54,17 +54,24 @@ export default class GithubLoginModel { const fingerprint = hash.digest('base64'); if (!this.checked.has(fingerprint)) { - const scopes = new Set(await this.getScopes(account, password)); - - for (const scope of this.constructor.REQUIRED_SCOPES) { - if (!scopes.has(scope)) { - // Token doesn't have enough OAuth scopes, need to reauthenticate - return INSUFFICIENT; + try { + const scopes = new Set(await this.getScopes(account, password)); + + for (const scope of this.constructor.REQUIRED_SCOPES) { + if (!scopes.has(scope)) { + // Token doesn't have enough OAuth scopes, need to reauthenticate + return INSUFFICIENT; + } } - } - // We're good - this.checked.add(fingerprint); + // We're good + this.checked.add(fingerprint); + } catch (e) { + // Bad credential most likely + // eslint-disable-next-line no-console + console.error(`Unable to validate token scopes against ${account}`, e); + return UNAUTHENTICATED; + } } } From 744c2674e8c30617d5a9154c0e60d91504c1629e Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 25 May 2018 13:17:21 -0400 Subject: [PATCH 51/64] Handle the "insufficient scopes" case in RemotePrController --- lib/controllers/remote-pr-controller.js | 52 ++++++----- lib/views/loading-view.js | 11 +++ test/controllers/remote-pr-controller.test.js | 91 +++++++++++++++++++ 3 files changed, 131 insertions(+), 23 deletions(-) create mode 100644 lib/views/loading-view.js create mode 100644 test/controllers/remote-pr-controller.test.js diff --git a/lib/controllers/remote-pr-controller.js b/lib/controllers/remote-pr-controller.js index 5c10783454..3e9a7c2a4a 100644 --- a/lib/controllers/remote-pr-controller.js +++ b/lib/controllers/remote-pr-controller.js @@ -4,9 +4,10 @@ import yubikiri from 'yubikiri'; import {shell} from 'electron'; import {RemotePropType, BranchSetPropType} from '../prop-types'; +import LoadingView from '../views/loading-view'; import GithubLoginView from '../views/github-login-view'; import ObserveModel from '../views/observe-model'; -import {UNAUTHENTICATED} from '../shared/keytar-strategy'; +import {UNAUTHENTICATED, INSUFFICIENT} from '../shared/keytar-strategy'; import {nullRemote} from '../models/remote'; import PrInfoController from './pr-info-controller'; import {autobind} from '../helpers'; @@ -49,29 +50,34 @@ export default class RemotePrController extends React.Component { ); } - renderWithData(loginData) { - const { - host, remote, branches, loginModel, selectedPrUrl, - aheadCount, pushInProgress, onSelectPr, onUnpinPr, - } = this.props; - const token = loginData.token; + renderWithData({token}) { + let inner; + if (token === null) { + inner = ; + } else if (token === UNAUTHENTICATED) { + inner = ; + } else if (token === INSUFFICIENT) { + inner = ; + } else { + const { + host, remote, branches, loginModel, selectedPrUrl, + aheadCount, pushInProgress, onSelectPr, onUnpinPr, + } = this.props; - return ( -
- {token && token !== UNAUTHENTICATED && - - } - {(!token || token === UNAUTHENTICATED) && } -
- ); + inner = ( + + ); + } + + return
{inner}
; } handleLogin(token) { diff --git a/lib/views/loading-view.js b/lib/views/loading-view.js new file mode 100644 index 0000000000..847496adb3 --- /dev/null +++ b/lib/views/loading-view.js @@ -0,0 +1,11 @@ +import React from 'react'; + +export default class LoadingView extends React.Component { + render() { + return ( +
+ +
+ ); + } +} diff --git a/test/controllers/remote-pr-controller.test.js b/test/controllers/remote-pr-controller.test.js new file mode 100644 index 0000000000..1be18cade4 --- /dev/null +++ b/test/controllers/remote-pr-controller.test.js @@ -0,0 +1,91 @@ +import React from 'react'; +import {mount} from 'enzyme'; + +import GithubLoginModel from '../../lib/models/github-login-model'; +import BranchSet from '../../lib/models/branch-set'; +import Remote from '../../lib/models/remote'; +import {expectRelayQuery} from '../../lib/relay-network-layer-manager'; +import {InMemoryStrategy, UNAUTHENTICATED, INSUFFICIENT} from '../../lib/shared/keytar-strategy'; +import RemotePrController from '../../lib/controllers/remote-pr-controller'; + +describe('RemotePrController', function() { + let loginModel, remote, branchSet; + + beforeEach(function() { + loginModel = new GithubLoginModel(InMemoryStrategy); + sinon.stub(loginModel, 'getToken').returns(Promise.resolve('1234')); + + remote = new Remote('origin', 'git@github.com:atom/github'); + branchSet = new BranchSet(); + + expectRelayQuery({ + name: 'prInfoControllerByBranchQuery', + variables: {repoOwner: 'atom', repoName: 'github', branchName: ''}, + }, { + repository: { + defaultBranchRef: { + prefix: 'refs/heads', + name: 'master', + }, + pullRequests: { + totalCount: 0, + edges: [], + }, + id: '1', + }, + }); + }); + + function createApp(props = {}) { + const noop = () => {}; + + return ( + + ); + } + + it('renders a loading message while fetching the token', function() { + const wrapper = mount(createApp()); + assert.isTrue(wrapper.find('LoadingView').exists()); + }); + + it('shows the login view if unauthenticated', async function() { + loginModel.getToken.restore(); + sinon.stub(loginModel, 'getToken').returns(Promise.resolve(UNAUTHENTICATED)); + + const wrapper = mount(createApp()); + + await assert.async.isTrue(wrapper.update().find('GithubLoginView').exists()); + assert.isFalse(wrapper.find('GithubLoginView').prop('scopeExpansion')); + }); + + it('shows the login view if more scopes are required', async function() { + loginModel.getToken.restore(); + sinon.stub(loginModel, 'getToken').returns(Promise.resolve(INSUFFICIENT)); + + const wrapper = mount(createApp()); + + await assert.async.isTrue(wrapper.update().find('GithubLoginView').exists()); + assert.isTrue(wrapper.find('GithubLoginView').prop('scopeExpansion')); + }); + + it('renders pull request info if authenticated', async function() { + const wrapper = mount(createApp()); + + await assert.async.isTrue(wrapper.update().find('PrInfoController').exists()); + + const controller = wrapper.update().find('PrInfoController'); + assert.strictEqual(controller.prop('remote'), remote); + assert.strictEqual(controller.prop('branches'), branchSet); + assert.strictEqual(controller.prop('loginModel'), loginModel); + }); +}); From 7c24b6bbc05039de115151fb1a380edc0180e427 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 25 May 2018 13:28:09 -0400 Subject: [PATCH 52/64] Oh GithubLoginView already lets you customize a message --- lib/controllers/remote-pr-controller.js | 10 ++++++++-- test/controllers/remote-pr-controller.test.js | 10 ++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/lib/controllers/remote-pr-controller.js b/lib/controllers/remote-pr-controller.js index 3e9a7c2a4a..19100aef4d 100644 --- a/lib/controllers/remote-pr-controller.js +++ b/lib/controllers/remote-pr-controller.js @@ -55,9 +55,15 @@ export default class RemotePrController extends React.Component { if (token === null) { inner = ; } else if (token === UNAUTHENTICATED) { - inner = ; + inner = ; } else if (token === INSUFFICIENT) { - inner = ; + inner = ( + +

+ Your token no longer has sufficient authorizations. Please re-authenticate and generate a new one. +

+ + ); } else { const { host, remote, branches, loginModel, selectedPrUrl, diff --git a/test/controllers/remote-pr-controller.test.js b/test/controllers/remote-pr-controller.test.js index 1be18cade4..7a9e3a4462 100644 --- a/test/controllers/remote-pr-controller.test.js +++ b/test/controllers/remote-pr-controller.test.js @@ -65,7 +65,10 @@ describe('RemotePrController', function() { const wrapper = mount(createApp()); await assert.async.isTrue(wrapper.update().find('GithubLoginView').exists()); - assert.isFalse(wrapper.find('GithubLoginView').prop('scopeExpansion')); + assert.strictEqual( + wrapper.find('GithubLoginView').find('p').text(), + 'Log in to GitHub to access PR information and more!', + ); }); it('shows the login view if more scopes are required', async function() { @@ -75,7 +78,10 @@ describe('RemotePrController', function() { const wrapper = mount(createApp()); await assert.async.isTrue(wrapper.update().find('GithubLoginView').exists()); - assert.isTrue(wrapper.find('GithubLoginView').prop('scopeExpansion')); + assert.strictEqual( + wrapper.find('GithubLoginView').find('p').text(), + 'Your token no longer has sufficient authorizations. Please re-authenticate and generate a new one.', + ); }); it('renders pull request info if authenticated', async function() { From dbf5ac99f47f60300ebbcf6050ec1c86fbabd4c4 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 25 May 2018 13:34:38 -0400 Subject: [PATCH 53/64] Report GraphQL errors from non-200 responses --- lib/relay-network-layer-manager.js | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/lib/relay-network-layer-manager.js b/lib/relay-network-layer-manager.js index 69e3e5cb68..10409ac9c9 100644 --- a/lib/relay-network-layer-manager.js +++ b/lib/relay-network-layer-manager.js @@ -73,10 +73,10 @@ function createFetchQuery(url) { }; } - return function fetchQuery(operation, variables, cacheConfig, uploadables) { + return async function fetchQuery(operation, variables, cacheConfig, uploadables) { const currentToken = tokenPerURL.get(url); - return fetch(url, { + const response = await fetch(url, { method: 'POST', headers: { 'content-type': 'application/json', @@ -87,13 +87,20 @@ function createFetchQuery(url) { query: operation.text, variables, }), - }).then(response => { - try { - atom && atom.inDevMode() && logRatelimitApi(response.headers); - } catch (_e) { /* do nothing */ } - - return response.json(); }); + + try { + atom && atom.inDevMode() && logRatelimitApi(response.headers); + } catch (_e) { /* do nothing */ } + + if (response.status !== 200) { + const e = new Error(`GraphQL API endpoint at ${url} returned ${response.status}`); + e.response = response; + e.rawStack = e.stack; + throw e; + } + + return response.json(); }; } From 930e89ffe72a5058adec7f804c60cd9519411dfb Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 25 May 2018 13:47:20 -0400 Subject: [PATCH 54/64] Don't lose the token on every launch :eyes: --- lib/relay-network-layer-manager.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/relay-network-layer-manager.js b/lib/relay-network-layer-manager.js index 10409ac9c9..cd0c6c4982 100644 --- a/lib/relay-network-layer-manager.js +++ b/lib/relay-network-layer-manager.js @@ -113,7 +113,7 @@ export default class RelayNetworkLayerManager { if (!environment) { const source = new RecordSource(); const store = new Store(source); - network = Network.create(this.getFetchQuery(url)); + network = Network.create(this.getFetchQuery(url, token)); environment = new Environment({network, store}); relayEnvironmentPerGithubHost.set(host, {environment, network}); From e7523020cf67a38de2dd87ff243c503dd2b6f9f3 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 25 May 2018 13:47:30 -0400 Subject: [PATCH 55/64] Autocomplete on login --- lib/views/commit-view.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/views/commit-view.js b/lib/views/commit-view.js index 40b9b9ca07..053cd00d7c 100644 --- a/lib/views/commit-view.js +++ b/lib/views/commit-view.js @@ -470,8 +470,12 @@ export default class CommitView extends React.Component { matchAuthors(authors, filterText, selectedAuthors) { const matchedAuthors = authors.filter((author, index) => { const isAlreadySelected = selectedAuthors && selectedAuthors.find(selected => selected.matches(author)); - const matchesFilter = `${author.getFullName()}${author.getEmail()}`.toLowerCase() - .indexOf(filterText.toLowerCase()) !== -1; + const matchesFilter = [ + author.getLogin(), + author.getFullName(), + author.getEmail(), + ].some(field => field && field.toLowerCase().indexOf(filterText.toLowerCase()) !== -1); + return !isAlreadySelected && matchesFilter; }); matchedAuthors.push(Author.createNew('Add new author', filterText)); From cbf56abf15cef18a5e1cf186ff74e07692990847 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 25 May 2018 13:55:12 -0400 Subject: [PATCH 56/64] Render a handle if we have one --- lib/views/commit-view.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/lib/views/commit-view.js b/lib/views/commit-view.js index 053cd00d7c..141b063e27 100644 --- a/lib/views/commit-view.js +++ b/lib/views/commit-view.js @@ -496,15 +496,22 @@ export default class CommitView extends React.Component { return (
{this.renderCoAuthorListItemField('name', author.getFullName())} + {author.hasLogin() && this.renderCoAuthorListItemField('login', '@' + author.getLogin())} {this.renderCoAuthorListItemField('email', author.getEmail())}
); } renderCoAuthorValue(author) { - return ( - {author.getFullName()} - ); + const fullName = author.getFullName(); + if (fullName && fullName.length > 0) { + return {author.getFullName()}; + } + if (author.hasLogin()) { + return @{author.getLogin()}; + } + + return {author.getEmail()}; } onSelectedCoAuthorsChanged(selectedCoAuthors) { From 1948962788105f5ae0c1f222acbc71232a31e853 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 25 May 2018 14:00:18 -0400 Subject: [PATCH 57/64] Our tokens have read:org --- lib/models/github-login-model.js | 2 +- test/models/github-login-model.test.js | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/models/github-login-model.js b/lib/models/github-login-model.js index 1c149235ae..f37352a7f7 100644 --- a/lib/models/github-login-model.js +++ b/lib/models/github-login-model.js @@ -8,7 +8,7 @@ let instance = null; export default class GithubLoginModel { // Be sure that we're requesting at least this many scopes on the token we grant through github.atom.io or we'll // give everyone a really frustrating experience ;-) - static REQUIRED_SCOPES = ['repo', 'user:email'] + static REQUIRED_SCOPES = ['repo', 'read:org', 'user:email'] static get() { if (!instance) { diff --git a/test/models/github-login-model.test.js b/test/models/github-login-model.test.js index 2925e76e3e..9dd2ed41b8 100644 --- a/test/models/github-login-model.test.js +++ b/test/models/github-login-model.test.js @@ -39,19 +39,19 @@ describe('GithubLoginModel', function() { }); it('returns INSUFFICIENT if scopes are present', async function() { - sinon.stub(loginModel, 'getScopes').returns(Promise.resolve(['repo'])); + sinon.stub(loginModel, 'getScopes').returns(Promise.resolve(['repo', 'read:org'])); assert.strictEqual(await loginModel.getToken('https://api.github.com'), INSUFFICIENT); }); it('returns the token if at least the required scopes are present', async function() { - sinon.stub(loginModel, 'getScopes').returns(Promise.resolve(['repo', 'user:email', 'extra'])); + sinon.stub(loginModel, 'getScopes').returns(Promise.resolve(['repo', 'read:org', 'user:email', 'extra'])); assert.strictEqual(await loginModel.getToken('https://api.github.com'), '1234'); }); it('caches checked tokens', async function() { - sinon.stub(loginModel, 'getScopes').returns(Promise.resolve(['repo', 'user:email'])); + sinon.stub(loginModel, 'getScopes').returns(Promise.resolve(['repo', 'read:org', 'user:email'])); assert.strictEqual(await loginModel.getToken('https://api.github.com'), '1234'); assert.strictEqual(loginModel.getScopes.callCount, 1); From f9f6ac7ab37719d80e43634e4bc052f369f7f328 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 29 May 2018 15:56:44 -0400 Subject: [PATCH 58/64] Pass UNAUTHENTICATED results through GithubLoginModel --- lib/models/github-login-model.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/models/github-login-model.js b/lib/models/github-login-model.js index f37352a7f7..b9f6fe75c3 100644 --- a/lib/models/github-login-model.js +++ b/lib/models/github-login-model.js @@ -41,7 +41,7 @@ export default class GithubLoginModel { async getToken(account) { const strategy = await this.getStrategy(); const password = await strategy.getPassword('atom-github', account); - if (!password) { + if (!password || password === UNAUTHENTICATED) { // User is not logged in return UNAUTHENTICATED; } From 240c754fc09d56a7a79e86495b807d324bc147dd Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 29 May 2018 15:57:38 -0400 Subject: [PATCH 59/64] Respect an excludedUsers config setting --- lib/models/user-store.js | 30 +++++- test/models/user-store.test.js | 192 ++++++++++++++++++++++++--------- 2 files changed, 167 insertions(+), 55 deletions(-) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index 057d5ebda4..e09da74add 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -1,5 +1,5 @@ import yubikiri from 'yubikiri'; -import {Emitter} from 'event-kit'; +import {Emitter, CompositeDisposable} from 'event-kit'; import RelayNetworkLayerManager from '../relay-network-layer-manager'; import Author, {nullAuthor} from './author'; @@ -43,17 +43,20 @@ class GraphQLCache { } export default class UserStore { - constructor({repository, login}) { + constructor({repository, login, config}) { this.emitter = new Emitter(); + this.subs = new CompositeDisposable(); // TODO: [ku 3/2018] Consider using Dexie (indexDB wrapper) like Desktop and persist users across sessions this.allUsers = new Map(); + this.excludedUsers = new Set(); this.users = []; this.committer = nullAuthor; this.last = { source: source.PENDING, repository: null, + excludedUsers: this.excludedUsers, }; this.cache = new GraphQLCache(); @@ -71,6 +74,20 @@ export default class UserStore { didUpdate: () => this.loadUsers(), }); this.loginObserver.setActiveModel(login); + + this.subs.add( + config.observe('github.excludedUsers', value => { + this.excludedUsers = new Set( + (value || '').split(/\s*,\s*/).filter(each => each.length > 0), + ); + return this.loadUsers(); + }), + ); + } + + dispose() { + this.subs.dispose(); + this.emitter.dispose(); } async loadUsers() { @@ -165,14 +182,17 @@ export default class UserStore { } addUsers(users, nextSource) { + let changed = false; + if ( nextSource !== this.last.source || - this.repositoryObserver.getActiveModel() !== this.last.repository + this.repositoryObserver.getActiveModel() !== this.last.repository || + this.excludedUsers !== this.last.excludedUsers ) { + changed = true; this.allUsers.clear(); } - let changed = false; for (const author of users) { if (!this.allUsers.has(author.getEmail())) { changed = true; @@ -185,6 +205,7 @@ export default class UserStore { } this.last.source = nextSource; this.last.repository = this.repositoryObserver.getActiveModel(); + this.last.excludedUsers = this.excludedUsers; } finalize() { @@ -193,6 +214,7 @@ export default class UserStore { for (const author of this.allUsers.values()) { if (author.matches(this.committer)) { continue; } if (author.isNoReply()) { continue; } + if (this.excludedUsers.has(author.getEmail())) { continue; } users.push(author); } diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index 241f7b0273..10ef5c993c 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -8,9 +8,24 @@ import {expectRelayQuery} from '../../lib/relay-network-layer-manager'; import {cloneRepository, buildRepository, FAKE_USER} from '../helpers'; describe('UserStore', function() { - let login; + let login, atomEnv, config, store; - function nextUpdatePromise(store) { + beforeEach(function() { + atomEnv = global.buildAtomEnvironment(); + config = atomEnv.config; + + login = new GithubLoginModel(InMemoryStrategy); + sinon.stub(login, 'getScopes').returns(Promise.resolve(GithubLoginModel.REQUIRED_SCOPES)); + }); + + afterEach(function() { + if (store) { + store.dispose(); + } + atomEnv.destroy(); + }); + + function nextUpdatePromise() { return new Promise(resolve => { const sub = store.onDidUpdate(() => { sub.dispose(); @@ -51,21 +66,30 @@ describe('UserStore', function() { }); } - beforeEach(function() { - login = new GithubLoginModel(InMemoryStrategy); - sinon.stub(login, 'getScopes').returns(Promise.resolve(GithubLoginModel.REQUIRED_SCOPES)); - }); + async function commitAs(repository, ...accounts) { + const committerName = await repository.getConfig('user.name'); + const committerEmail = await repository.getConfig('user.email'); + + for (const {name, email} of accounts) { + await repository.setConfig('user.name', name); + await repository.setConfig('user.email', email); + await repository.commit('message', {allowEmpty: true}); + } + + await repository.setConfig('user.name', committerName); + await repository.setConfig('user.email', committerEmail); + } it('loads store with local git users and committer in a repo with no GitHub remote', async function() { const workdirPath = await cloneRepository('multiple-commits'); const repository = await buildRepository(workdirPath); - const store = new UserStore({repository}); + store = new UserStore({repository, config}); assert.deepEqual(store.getUsers(), []); assert.strictEqual(store.committer, nullAuthor); // Store is populated asynchronously - await nextUpdatePromise(store); + await nextUpdatePromise(); assert.deepEqual(store.getUsers(), [ new Author('kuychaco@github.com', 'Katrina Uychaco'), ]); @@ -89,11 +113,11 @@ describe('UserStore', function() { {login: 'smashwilson', email: 'smashwilson@github.com', name: 'Ash Wilson'}, ]); - const store = new UserStore({repository, login}); - await nextUpdatePromise(store); + store = new UserStore({repository, login, config}); + await nextUpdatePromise(); resolve(); - await nextUpdatePromise(store); + await nextUpdatePromise(); assert.deepEqual(store.getUsers(), [ new Author('smashwilson@github.com', 'Ash Wilson', 'smashwilson'), @@ -123,13 +147,13 @@ describe('UserStore', function() { ], ); - const store = new UserStore({repository, login}); + store = new UserStore({repository, login, config}); - await nextUpdatePromise(store); + await nextUpdatePromise(); assert.deepEqual(store.getUsers(), []); resolve0(); - await nextUpdatePromise(store); + await nextUpdatePromise(); assert.deepEqual(store.getUsers(), [ new Author('smashwilson@github.com', 'Ash Wilson', 'smashwilson'), @@ -138,7 +162,7 @@ describe('UserStore', function() { ]); resolve1(); - await nextUpdatePromise(store); + await nextUpdatePromise(); assert.deepEqual(store.getUsers(), [ new Author('aaa@github.com', 'Aahhhhh', 'aaa'), @@ -162,11 +186,11 @@ describe('UserStore', function() { {login: 'simurai', email: '', name: 'simurai'}, ]); - const store = new UserStore({repository, login}); - await nextUpdatePromise(store); + store = new UserStore({repository, login, config}); + await nextUpdatePromise(); resolve(); - await nextUpdatePromise(store); + await nextUpdatePromise(); assert.deepEqual(store.getUsers(), [ new Author('simurai@users.noreply.github.com', 'simurai', 'simurai'), @@ -176,7 +200,7 @@ describe('UserStore', function() { it('excludes committer and no reply user from `getUsers`', async function() { const workdirPath = await cloneRepository('multiple-commits'); const repository = await buildRepository(workdirPath); - const store = new UserStore({repository}); + store = new UserStore({repository, config}); await assert.async.lengthOf(store.getUsers(), 1); sinon.spy(store, 'addUsers'); @@ -200,8 +224,8 @@ describe('UserStore', function() { it('adds specified users and does not overwrite existing users', async function() { const workdirPath = await cloneRepository('multiple-commits'); const repository = await buildRepository(workdirPath); - const store = new UserStore({repository}); - await nextUpdatePromise(store); + store = new UserStore({repository, config}); + await nextUpdatePromise(); assert.lengthOf(store.getUsers(), 1); @@ -222,8 +246,8 @@ describe('UserStore', function() { const workdirPath = await cloneRepository('multiple-commits'); const repository = await buildRepository(workdirPath); - const store = new UserStore({repository}); - await nextUpdatePromise(store); + store = new UserStore({repository, config}); + await nextUpdatePromise(); assert.deepEqual(store.committer, new Author(FAKE_USER.email, FAKE_USER.name)); const newEmail = 'foo@bar.com'; @@ -232,7 +256,7 @@ describe('UserStore', function() { await repository.setConfig('user.email', newEmail); await repository.setConfig('user.name', newName); repository.refresh(); - await nextUpdatePromise(store); + await nextUpdatePromise(); assert.deepEqual(store.committer, new Author(newEmail, newName)); }); @@ -245,8 +269,8 @@ describe('UserStore', function() { await repository.commit('commit 2', {allowEmpty: true}); await repository.checkout('master'); - const store = new UserStore({repository}); - await nextUpdatePromise(store); + store = new UserStore({repository, config}); + await nextUpdatePromise(); assert.deepEqual(store.getUsers(), [ new Author('kuychaco@github.com', 'Katrina Uychaco'), ]); @@ -261,7 +285,7 @@ describe('UserStore', function() { `, {allowEmpty: true}); repository.refresh(); - await nextUpdatePromise(store); + await nextUpdatePromise(); await assert.strictEqual(store.addUsers.callCount, 1); assert.isTrue(store.getUsers().some(user => { @@ -296,8 +320,8 @@ describe('UserStore', function() { ]); resolve(); - const store = new UserStore({repository, login}); - await nextUpdatePromise(store); + store = new UserStore({repository, login, config}); + await nextUpdatePromise(); assert.deepEqual(store.getUsers(), gitAuthors); @@ -311,29 +335,21 @@ describe('UserStore', function() { await login.setToken('https://api.github.com', '1234'); - await nextUpdatePromise(store); + await nextUpdatePromise(); assert.deepEqual(store.getUsers(), graphqlAuthors); }); it('refetches users when the repository changes', async function() { const workdirPath0 = await cloneRepository('multiple-commits'); const repository0 = await buildRepository(workdirPath0); - await repository0.setConfig('user.email', 'committer0@github.com'); - await repository0.setConfig('user.name', 'committer0'); - await repository0.commit('on repo 0', {allowEmpty: true}); - await repository0.setConfig('user.email', 'committer@github.com'); - await repository0.setConfig('user.name', 'committer'); + await commitAs(repository0, {name: 'committer0', email: 'committer0@github.com'}); const workdirPath1 = await cloneRepository('multiple-commits'); const repository1 = await buildRepository(workdirPath1); - await repository1.setConfig('user.email', 'committer1@github.com'); - await repository1.setConfig('user.name', 'committer1'); - await repository1.commit('on repo 1', {allowEmpty: true}); - await repository1.setConfig('user.email', 'committer@github.com'); - await repository1.setConfig('user.name', 'committer'); + await commitAs(repository1, {name: 'committer1', email: 'committer1@github.com'}); - const store = new UserStore({repository: repository0}); - await nextUpdatePromise(store); + store = new UserStore({repository: repository0, config}); + await nextUpdatePromise(); assert.deepEqual(store.getUsers(), [ new Author('kuychaco@github.com', 'Katrina Uychaco'), @@ -341,7 +357,7 @@ describe('UserStore', function() { ]); store.setRepository(repository1); - await nextUpdatePromise(store); + await nextUpdatePromise(); assert.deepEqual(store.getUsers(), [ new Author('kuychaco@github.com', 'Katrina Uychaco'), @@ -366,12 +382,12 @@ describe('UserStore', function() { ]); resolve(); - const store = new UserStore({repository, login}); + store = new UserStore({repository, login, config}); sinon.spy(store, 'loadUsers'); // The first update is triggered by the commiter, the second from GraphQL results arriving. - await nextUpdatePromise(store); - await nextUpdatePromise(store); + await nextUpdatePromise(); + await nextUpdatePromise(); disable(); @@ -410,19 +426,19 @@ describe('UserStore', function() { resolve0(); resolve1(); - const store = new UserStore({repository: repository0, login}); - await nextUpdatePromise(store); - await nextUpdatePromise(store); + store = new UserStore({repository: repository0, login, config}); + await nextUpdatePromise(); + await nextUpdatePromise(); store.setRepository(repository1); - await nextUpdatePromise(store); + await nextUpdatePromise(); sinon.spy(store, 'loadUsers'); disable0(); disable1(); store.setRepository(repository0); - await nextUpdatePromise(store); + await nextUpdatePromise(); assert.deepEqual(store.getUsers(), [ new Author('aaa-0@a.com', 'AAA', 'aaa'), @@ -431,4 +447,78 @@ describe('UserStore', function() { ]); }); }); + + describe('excluded users', function() { + it('do not appear in the list from git', async function() { + config.set('github.excludedUsers', 'evil@evilcorp.org'); + + const workdirPath = await cloneRepository('multiple-commits'); + const repository = await buildRepository(workdirPath); + await commitAs(repository, + {name: 'evil0', email: 'evil@evilcorp.org'}, + {name: 'ok', email: 'ok@somewhere.net'}, + {name: 'evil1', email: 'evil@evilcorp.org'}, + ); + + store = new UserStore({repository, config}); + await nextUpdatePromise(); + + assert.deepEqual(store.getUsers(), [ + new Author('kuychaco@github.com', 'Katrina Uychaco'), + new Author('ok@somewhere.net', 'ok'), + ]); + }); + + it('do not appear in the list from GraphQL', async function() { + config.set('github.excludedUsers', 'evil@evilcorp.org, other@evilcorp.org'); + await login.setToken('https://api.github.com', '1234'); + + const workdirPath = await cloneRepository('multiple-commits'); + const repository = await buildRepository(workdirPath); + await repository.setConfig('remote.origin.url', 'git@github.com:me/stuff.git'); + await repository.setConfig('remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*'); + + const [{resolve}] = expectPagedRelayQueries({}, [ + {login: 'evil0', email: 'evil@evilcorp.org', name: 'evil0'}, + {login: 'octocat', email: 'mona@lisa.com', name: 'Mona Lisa'}, + ]); + resolve(); + + store = new UserStore({repository, login, config}); + await nextUpdatePromise(); + await nextUpdatePromise(); + + assert.deepEqual(store.getUsers(), [ + new Author('mona@lisa.com', 'Mona Lisa', 'octocat'), + ]); + }); + + it('are updated when the config option changes', async function() { + config.set('github.excludedUsers', 'evil0@evilcorp.org'); + + const workdirPath = await cloneRepository('multiple-commits'); + const repository = await buildRepository(workdirPath); + await commitAs(repository, + {name: 'evil0', email: 'evil0@evilcorp.org'}, + {name: 'ok', email: 'ok@somewhere.net'}, + {name: 'evil1', email: 'evil1@evilcorp.org'}, + ); + + store = new UserStore({repository, config}); + await nextUpdatePromise(); + + assert.deepEqual(store.getUsers(), [ + new Author('kuychaco@github.com', 'Katrina Uychaco'), + new Author('evil1@evilcorp.org', 'evil1'), + new Author('ok@somewhere.net', 'ok'), + ]); + + config.set('github.excludedUsers', 'evil0@evilcorp.org, evil1@evilcorp.org'); + + assert.deepEqual(store.getUsers(), [ + new Author('kuychaco@github.com', 'Katrina Uychaco'), + new Author('ok@somewhere.net', 'ok'), + ]); + }); + }); }); From f2610c4b7f41c68fc99dd22a2216e501c1ef4938 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 29 May 2018 15:58:50 -0400 Subject: [PATCH 60/64] Include schema for excludedUsers --- package.json | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/package.json b/package.json index c945151630..ed556aeb3f 100644 --- a/package.json +++ b/package.json @@ -150,6 +150,11 @@ "type": "boolean", "default": true, "description": "Resolve merge conflicts with in-editor controls" + }, + "excludedUsers": { + "type": "string", + "default": "", + "description": "Comma-separated list of email addresses to exclude from the co-author selection list" } }, "deserializers": { From 086ee0a20363ff802cb2c82bf3977621f2dce1bc Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 30 May 2018 07:54:39 -0400 Subject: [PATCH 61/64] Pass config to the UserStore initializer --- lib/controllers/git-tab-controller.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/controllers/git-tab-controller.js b/lib/controllers/git-tab-controller.js index d773a5cee7..ec38e5517d 100644 --- a/lib/controllers/git-tab-controller.js +++ b/lib/controllers/git-tab-controller.js @@ -69,6 +69,7 @@ export default class GitTabController extends React.Component { this.userStore = new UserStore({ repository: this.props.repository, login: this.props.loginModel, + config: this.props.config, }); } From 1ec5ed9d5b2387631db8bb004f3cf5028b5bdf93 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 30 May 2018 08:38:20 -0400 Subject: [PATCH 62/64] Gracefully handle GraphQL errors in the UserStore --- lib/models/user-store.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index e09da74add..2894c8c53d 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -163,6 +163,15 @@ export default class UserStore { after: cursor, }); + if (response.errors && response.errors.length > 1) { + // eslint-disable-next-line no-console + console.error(`Error fetching mentionable users:\n${response.errors.map(e => e.message).join('\n')}`); + } + + if (!response.data) { + break; + } + const connection = response.data.repository.mentionableUsers; const authors = connection.nodes.map(node => { if (node.email === '') { From a82ce61e0d790ed7a77f0199c225f28f81892240 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 30 May 2018 08:38:37 -0400 Subject: [PATCH 63/64] Exclude co-authors on shift-delete --- keymaps/git.cson | 1 + lib/views/commit-view.js | 17 ++++++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/keymaps/git.cson b/keymaps/git.cson index af3ff892bf..1f24a4282f 100644 --- a/keymaps/git.cson +++ b/keymaps/git.cson @@ -70,3 +70,4 @@ 'home': 'github:co-author:home' 'end': 'github:co-author:end' 'delete': 'github:co-author:delete' + 'shift-backspace': 'github:co-author-exclude' diff --git a/lib/views/commit-view.js b/lib/views/commit-view.js index 141b063e27..debdea7442 100644 --- a/lib/views/commit-view.js +++ b/lib/views/commit-view.js @@ -57,7 +57,7 @@ export default class CommitView extends React.Component { this, 'submitNewCoAuthor', 'cancelNewCoAuthor', 'didChangeCommitMessage', 'didMoveCursor', 'toggleHardWrap', 'toggleCoAuthorInput', 'abortMerge', 'commit', 'amendLastCommit', 'toggleExpandedCommitMessageEditor', - 'renderCoAuthorListItem', 'onSelectedCoAuthorsChanged', + 'renderCoAuthorListItem', 'onSelectedCoAuthorsChanged', 'excludeCoAuthor', ); this.state = { @@ -132,6 +132,7 @@ export default class CommitView extends React.Component { 'github:co-author:home': this.proxyKeyCode(36), 'github:co-author:delete': this.proxyKeyCode(46), 'github:co-author:escape': this.proxyKeyCode(27), + 'github:co-author-exclude': this.excludeCoAuthor, }), this.props.config.onDidChange('github.automaticCommitMessageWrapping', () => this.forceUpdate()), ); @@ -386,6 +387,20 @@ export default class CommitView extends React.Component { }); } + excludeCoAuthor() { + const author = this.refCoAuthorSelect.map(c => c.getFocusedOption()); + if (!author || author.isNew()) { + return; + } + + let excluded = this.props.config.get('github.excludedUsers'); + if (excluded && excluded !== '') { + excluded += ', '; + } + excluded += author.getEmail(); + this.props.config.set('github.excludedUsers', excluded); + } + abortMerge() { this.props.abortMerge(); } From dd9647641beb263253c5678b888b7964c5c675cb Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 30 May 2018 08:49:20 -0400 Subject: [PATCH 64/64] Initialize UserStore correctly in tests --- test/controllers/commit-controller.test.js | 2 +- test/views/commit-view.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/controllers/commit-controller.test.js b/test/controllers/commit-controller.test.js index fa0b7d867a..2988d871e4 100644 --- a/test/controllers/commit-controller.test.js +++ b/test/controllers/commit-controller.test.js @@ -25,7 +25,7 @@ describe('CommitController', function() { lastCommit = new Commit({sha: 'a1e23fd45', message: 'last commit message'}); const noop = () => {}; - const store = new UserStore({}); + const store = new UserStore({config}); app = ( {}; const returnTruthyPromise = () => Promise.resolve(true); - const store = new UserStore({}); + const store = new UserStore({config}); app = (