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/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 7b98772179..ec38e5517d 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, @@ -62,15 +63,13 @@ export default class GitTabController extends React.Component { this.refView = null; this.state = { - mentionableUsers: [], selectedCoAuthors: [], }; this.userStore = new UserStore({ repository: this.props.repository, - onDidUpdate: users => { - this.setState({mentionableUsers: users}); - }, + login: this.props.loginModel, + config: this.props.config, }); } @@ -93,7 +92,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} @@ -135,16 +134,9 @@ 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.userStore.setLoginModel(this.props.loginModel); this.refreshResolutionProgress(false, false); } @@ -258,7 +250,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}); diff --git a/lib/controllers/remote-pr-controller.js b/lib/controllers/remote-pr-controller.js index 5c10783454..19100aef4d 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,40 @@ 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 = ( + +

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

+ + ); + } 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/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} diff --git a/lib/models/author.js b/lib/models/author.js new file mode 100644 index 0000000000..4a699ecd82 --- /dev/null +++ b/lib/models/author.js @@ -0,0 +1,100 @@ +const NEW = Symbol('new'); + +export const NO_REPLY_GITHUB_EMAIL = 'noreply@github.com'; + +export default class Author { + 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() { + return this.email; + } + + getFullName() { + return this.fullName; + } + + getLogin() { + return this.login; + } + + isNoReply() { + return this.email === NO_REPLY_GITHUB_EMAIL; + } + + hasLogin() { + return this.login !== null; + } + + isNew() { + return this.new; + } + + isPresent() { + return true; + } + + matches(other) { + return this.getEmail() === other.getEmail(); + } + + toString() { + let s = `${this.fullName} <${this.email}>`; + if (this.hasLogin()) { + s += ` @${this.login}`; + } + 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 = { + getEmail() { + return ''; + }, + + getFullName() { + return ''; + }, + + getLogin() { + return null; + }, + + isNoReply() { + return false; + }, + + hasLogin() { + return false; + }, + + isNew() { + return false; + }, + + isPresent() { + return false; + }, + + matches(other) { + return other === this; + }, + + toString() { + return 'null author'; + }, +}; diff --git a/lib/models/github-login-model.js b/lib/models/github-login-model.js index 7da9bc6e72..b9f6fe75c3 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', 'read:org', '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,41 @@ export default class GithubLoginModel { async getToken(account) { const strategy = await this.getStrategy(); - let password = await strategy.getPassword('atom-github', account); - if (!password) { + const password = await strategy.getPassword('atom-github', account); + if (!password || password === UNAUTHENTICATED) { // 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)) { + 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); + } catch (e) { + // Bad credential most likely + // eslint-disable-next-line no-console + console.error(`Unable to validate token scopes against ${account}`, e); + return UNAUTHENTICATED; + } + } + } + return password; } @@ -54,6 +90,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/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; }, diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index 6075a609fa..bd8c2fb7c9 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'; @@ -232,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), ); } @@ -616,8 +626,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/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 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/lib/models/user-store.js b/lib/models/user-store.js index dd1b5d9374..2894c8c53d 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -1,79 +1,262 @@ +import yubikiri from 'yubikiri'; +import {Emitter, CompositeDisposable} from 'event-kit'; + +import RelayNetworkLayerManager from '../relay-network-layer-manager'; +import Author, {nullAuthor} from './author'; +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. const MAX_COMMITS = 5000; -export const NO_REPLY_GITHUB_EMAIL = 'noreply@github.com'; +export const source = { + PENDING: Symbol('pending'), + GITLOG: Symbol('git log'), + 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, onDidUpdate}) { - this.repository = repository; - this.repository.onDidUpdate(() => { - this.loadUsers(); - }); - this.onDidUpdate = onDidUpdate || (() => {}); + 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.users = {}; - this.committer = {}; - this.populate(); + 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(); + + this.repositoryObserver = new ModelObserver({ + fetchData: r => yubikiri({ + committer: r.getCommitter(), + authors: r.getAuthors({max: MAX_COMMITS}), + remotes: r.getRemotes(), + }), + didUpdate: () => this.loadUsers(), + }); + this.repositoryObserver.setActiveModel(repository); + + this.loginObserver = new ModelObserver({ + 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(); } - populate() { - if (this.repository.isPresent()) { - this.loadUsers(); + async loadUsers() { + const data = this.repositoryObserver.getActiveModelData(); + + if (!data) { + return; + } + + this.setCommitter(data.committer); + const githubRemotes = data.remotes.filter(remote => remote.isGithubRepo()); + + if (githubRemotes.length === 0) { + this.addUsers(data.authors, source.GITLOG); } else { - this.repository.onDidChangeState(({from, to}) => { - if (!from.isPresent() && to.isPresent()) { - this.loadUsers(); + await this.loadUsersFromGraphQL(githubRemotes); + } + } + + loadUsersFromGraphQL(remotes) { + return Promise.all( + remotes.map(remote => this.loadMentionableUsers(remote)), + ); + } + + 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; + } + + const token = await loginModel.getToken('https://api.github.com'); + if (token === UNAUTHENTICATED || token === INSUFFICIENT) { + return; + } + + const fetchQuery = RelayNetworkLayerManager.getFetchQuery('https://api.github.com/graphql', token); + + let hasMore = true; + let cursor = null; + const remoteUsers = []; + + while (hasMore) { + const response = await fetchQuery({ + name: 'GetMentionableUsers', + text: ` + query GetMentionableUsers($owner: String!, $name: String!, $first: Int!, $after: String) { + 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, + }); + + 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 === '') { + node.email = `${node.login}@users.noreply.github.com`; } + + return new Author(node.email, node.name, node.login); }); + this.addUsers(authors, source.GITHUBAPI); + remoteUsers.push(...authors); + + cursor = connection.pageInfo.endCursor; + hasMore = connection.pageInfo.hasNextPage; } + + this.cache.set(remote, remoteUsers); } - loadUsers() { - this.loadUsersFromLocalRepo(); + addUsers(users, nextSource) { + let changed = false; + + if ( + nextSource !== this.last.source || + this.repositoryObserver.getActiveModel() !== this.last.repository || + this.excludedUsers !== this.last.excludedUsers + ) { + changed = true; + this.allUsers.clear(); + } + + for (const author of users) { + if (!this.allUsers.has(author.getEmail())) { + changed = true; + } + this.allUsers.set(author.getEmail(), author); + } + + if (changed) { + this.finalize(); + } + this.last.source = nextSource; + this.last.repository = this.repositoryObserver.getActiveModel(); + this.last.excludedUsers = this.excludedUsers; } - async loadUsersFromLocalRepo() { - const users = await this.repository.getAuthors({max: MAX_COMMITS}); - const committer = await this.repository.getCommitter(); - this.setCommitter(committer); - this.addUsers(users); + 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; } + if (this.excludedUsers.has(author.getEmail())) { continue; } + + users.push(author); + } + users.sort(Author.compare); + this.users = 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); + setRepository(repository) { + this.repositoryObserver.setActiveModel(repository); } - addUsers(users) { - this.users = {...this.users, ...users}; + setLoginModel(login) { + this.loginObserver.setActiveModel(login); } setCommitter(committer) { + const changed = !this.committer.matches(committer); this.committer = committer; + if (changed) { + this.finalize(); + } } didUpdate() { - this.onDidUpdate(this.getUsers()); + this.emitter.emit('did-update', this.getUsers()); + } + + onDidUpdate(callback) { + return this.emitter.on('did-update', callback); } 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; - }); + return this.users; } } 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, +}); diff --git a/lib/relay-network-layer-manager.js b/lib/relay-network-layer-manager.js index 1ae291dcaa..cd0c6c4982 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'; @@ -13,12 +14,69 @@ 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; + }); + + const existing = responsesByQuery.get(operationPattern.name) || []; + existing.push({promise, response, variables: operationPattern.variables || {}}); + responsesByQuery.set(operationPattern.name, existing); + + const disable = () => responsesByQuery.delete(operationPattern.name); + + return {promise, resolve, reject, disable}; +} + +export function clearRelayExpectations() { + responsesByQuery.clear(); +} + +const tokenPerURL = new Map(); +const fetchPerURL = new Map(); function createFetchQuery(url) { - return function fetchQuery(operation, variables, cacheConfig, uploadables) { - const currentToken = tokenPerEnvironmentUrl.get(url); - return fetch(url, { + if (atom.inSpecMode()) { + return function specFetchQuery(operation, variables, cacheConfig, uploadables) { + 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 ')}\n` + + util.inspect(variables), + ); + + const e = new Error(`Unexpected GraphQL query: ${operation.name}`); + e.rawStack = e.stack; + throw e; + } + + return match.promise; + }; + } + + return async function fetchQuery(operation, variables, cacheConfig, uploadables) { + const currentToken = tokenPerURL.get(url); + + const response = await fetch(url, { method: 'POST', headers: { 'content-type': 'application/json', @@ -29,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(); }; } @@ -43,17 +108,26 @@ 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.getFetchQuery(url, token)); environment = new Environment({network, store}); relayEnvironmentPerGithubHost.set(host, {environment, network}); } return environment; } + + static getFetchQuery(url, token) { + tokenPerURL.set(url, token); + let fetch = fetchPerURL.get(url); + if (!fetch) { + fetch = createFetchQuery(url); + fetchPerURL.set(fetch); + } + return fetch; + } } 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/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)); } } diff --git a/lib/views/commit-view.js b/lib/views/commit-view.js index b721ab97b7..debdea7442 100644 --- a/lib/views/commit-view.js +++ b/lib/views/commit-view.js @@ -8,8 +8,10 @@ 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} from '../prop-types'; +import {AuthorPropType, UserStorePropType} from '../prop-types'; const TOOLTIP_DELAY = 200; @@ -39,7 +41,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, @@ -55,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 = { @@ -130,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()), ); @@ -246,30 +249,33 @@ export default class CommitView extends React.Component { } renderCoAuthorInput() { - if (!this.state.showCoAuthorInput) { return null; } return ( - + )} + ); } @@ -381,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(); } @@ -464,11 +484,16 @@ 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.getLogin(), + author.getFullName(), + author.getEmail(), + ].some(field => field && field.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; } @@ -484,24 +509,31 @@ 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())} + {author.hasLogin() && this.renderCoAuthorListItemField('login', '@' + author.getLogin())} + {this.renderCoAuthorListItemField('email', author.getEmail())}
); } renderCoAuthorValue(author) { - return ( - {author.name} - ); + const fullName = author.getFullName(); + if (fullName && fullName.length > 0) { + return {author.getFullName()}; + } + if (author.hasLogin()) { + return @{author.getLogin()}; + } + + return {author.getEmail()}; } 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); } 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} /> 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/package.json b/package.json index bbc41482e3..81f7360a1c 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": { diff --git a/test/controllers/commit-controller.test.js b/test/controllers/commit-controller.test.js index 396b69c2dc..2988d871e4 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({config}); app = ( {}; + + 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.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() { + 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.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() { + 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); + }); +}); 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 diff --git a/test/models/author.test.js b/test/models/author.test.js new file mode 100644 index 0000000000..6f5952ec2c --- /dev/null +++ b/test/models/author.test.js @@ -0,0 +1,31 @@ +import Author, {nullAuthor, 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()); + }); + + 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)); + }); +}); diff --git a/test/models/github-login-model.test.js b/test/models/github-login-model.test.js index 0a3e03da85..9dd2ed41b8 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', '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', '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', 'read:org', '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); + }); + }); }); diff --git a/test/models/repository.test.js b/test/models/repository.test.js index acab459700..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,43 @@ 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()); + }); + }); + + 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}>`, + ); + } }); }); diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index b6653999db..10ef5c993c 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -1,33 +1,207 @@ import dedent from 'dedent-js'; -import UserStore, {NO_REPLY_GITHUB_EMAIL} 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'; +import {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() { + let login, atomEnv, config, 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(); + resolve(); + }); + }); + } + + 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 result = 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 result; + }); + } + + 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.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', - }, + await nextUpdatePromise(); + assert.deepEqual(store.getUsers(), [ + new Author('kuychaco@github.com', 'Katrina Uychaco'), + ]); + 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() { + 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/*'); + 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({}, [ + {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'}, + ]); + + store = new UserStore({repository, login, config}); + await nextUpdatePromise(); + + resolve(); + await nextUpdatePromise(); + + 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('loads users from multiple pages from the GitHub API', 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: resolve0}, {resolve: 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'}, + ], + ); + + store = new UserStore({repository, login, config}); + + await nextUpdatePromise(); + assert.deepEqual(store.getUsers(), []); + + resolve0(); + await nextUpdatePromise(); + + 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 nextUpdatePromise(); + + 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'), + new Author('annthurium@github.com', 'Tilde Ann Thurium', 'annthurium'), + new Author('zzz@github.com', 'Zzzzz', 'zzz'), + ]); + }); + + it('infers no-reply emails for users without a public email address', 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}] = expectPagedRelayQueries({}, [ + {login: 'simurai', email: '', name: 'simurai'}, + ]); + + store = new UserStore({repository, login, config}); + await nextUpdatePromise(); + + resolve(); + await nextUpdatePromise(); + + assert.deepEqual(store.getUsers(), [ + new Author('simurai@users.noreply.github.com', 'simurai', 'simurai'), ]); - await assert.async.deepEqual(store.committer, FAKE_USER); }); 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}); - await store.loadUsersFromLocalRepo(); + store = new UserStore({repository, config}); + await assert.async.lengthOf(store.getUsers(), 1); sinon.spy(store, 'addUsers'); // make a commit with FAKE_USER as committer @@ -40,40 +214,30 @@ 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() { 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}); + store = new UserStore({repository, config}); + await nextUpdatePromise(); - await assert.async.lengthOf(store.getUsers(), 1); + assert.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'), + ], source.GITLOG); - 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'), ]); }); }); @@ -82,19 +246,19 @@ describe('UserStore', function() { const workdirPath = await cloneRepository('multiple-commits'); const repository = await buildRepository(workdirPath); - const store = new UserStore({repository}); - await assert.async.deepEqual(store.committer, FAKE_USER); + store = new UserStore({repository, config}); + await nextUpdatePromise(); + 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, {name: FAKE_USER.name, email: newEmail}); - const newName = 'Foo Bar'; + + await repository.setConfig('user.email', newEmail); await repository.setConfig('user.name', newName); repository.refresh(); - await assert.async.deepEqual(store.committer, {name: newName, email: newEmail}); + await nextUpdatePromise(); + + assert.deepEqual(store.committer, new Author(newEmail, newName)); }); it('refetches users when HEAD changes', async function() { @@ -105,12 +269,10 @@ describe('UserStore', function() { await repository.commit('commit 2', {allowEmpty: true}); await repository.checkout('master'); - const store = new UserStore({repository}); - await assert.async.deepEqual(store.getUsers(), [ - { - email: 'kuychaco@github.com', - name: 'Katrina Uychaco', - }, + store = new UserStore({repository, config}); + await nextUpdatePromise(); + assert.deepEqual(store.getUsers(), [ + new Author('kuychaco@github.com', 'Katrina Uychaco'), ]); sinon.spy(store, 'addUsers'); @@ -122,13 +284,241 @@ describe('UserStore', function() { Co-authored-by: New Author `, {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'; + repository.refresh(); + await nextUpdatePromise(); + + 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}] = 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(); + + store = new UserStore({repository, login, config}); + await nextUpdatePromise(); + + 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/*'); + + repository.refresh(); + + // Token is not available, so authors are still queried from git + assert.deepEqual(store.getUsers(), gitAuthors); + + await login.setToken('https://api.github.com', '1234'); + + 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 commitAs(repository0, {name: 'committer0', email: 'committer0@github.com'}); + + const workdirPath1 = await cloneRepository('multiple-commits'); + const repository1 = await buildRepository(workdirPath1); + await commitAs(repository1, {name: 'committer1', email: 'committer1@github.com'}); + + store = new UserStore({repository: repository0, config}); + await nextUpdatePromise(); + + assert.deepEqual(store.getUsers(), [ + new Author('kuychaco@github.com', 'Katrina Uychaco'), + new Author('committer0@github.com', 'committer0'), + ]); + + store.setRepository(repository1); + await nextUpdatePromise(); + + assert.deepEqual(store.getUsers(), [ + new Author('kuychaco@github.com', 'Katrina Uychaco'), + 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(); + + 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(); + await nextUpdatePromise(); + + 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(); + + store = new UserStore({repository: repository0, login, config}); + await nextUpdatePromise(); + await nextUpdatePromise(); + + store.setRepository(repository1); + await nextUpdatePromise(); + + sinon.spy(store, 'loadUsers'); + disable0(); + disable1(); + + store.setRepository(repository0); + await nextUpdatePromise(); + + 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'), + ]); + }); + }); + + 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'), + ]); + }); }); }); 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() { diff --git a/test/views/commit-view.test.js b/test/views/commit-view.test.js index 50fb298fa9..6bef7bd4ab 100644 --- a/test/views/commit-view.test.js +++ b/test/views/commit-view.test.js @@ -4,6 +4,7 @@ import {shallow, mount} from 'enzyme'; import {cloneRepository, buildRepository} from '../helpers'; import Commit, {nullCommit} from '../../lib/models/commit'; import Branch, {nullBranch} from '../../lib/models/branch'; +import UserStore from '../../lib/models/user-store'; import CommitView from '../../lib/views/commit-view'; describe('CommitView', function() { @@ -19,6 +20,7 @@ describe('CommitView', function() { lastCommit = new Commit({sha: '1234abcd', message: 'commit message'}); const noop = () => {}; const returnTruthyPromise = () => Promise.resolve(true); + const store = new UserStore({config}); app = (