Summary: Ref T4986. Move push logs to a View, then have all the stuff that needs to use it use that View.
Test Plan: Viewed push logs and transaction detail in Diffusion. Created a panel.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9104
Summary: Ref T2683. Throw a more tailored exception to allow callers to distinguish between bad refs (which are expected, if users try to visit garbage branches) and other types of errors.
Test Plan: Tried to view branch "alksndfklansdf". Viewed branch "master".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9094
Summary: Ref T4986. Allows the Drydock search engines to render as panels.
Test Plan: Viewed affected interfaces in Drydock. Created panels from each engine.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9103
Summary: Ref T2683. This query is currently unbatched and happens inside a view. Leave it inside the view for now, but separate it and make it batchable.
Test Plan: {F34848}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, vrana, aran
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D5252
Summary:
Ref T2683. This is a refinement and simplification of D5257. In particular:
- D5257 only cached the commit chain, not path changes. This meant that we had to go issue an awkward query (which was slow on Facebook's install) periodically while reading the cache. This was reasonable locally but killed performance at FB scale. Instead, we can include path information in the cache. It is very rare that this is large except in Subversion, and we do not need to use this cache in Subversion. In other VCSes, the scale of this data is quite small (a handful of bytes per commit on average).
- D5257 required a large, slow offline computation step. This relies on D9044 to populate parent data so we can build the cache online at will, and let it expire with normal LRU/LFU/whatever semantics. We need this parent data for other reasons anyway.
- D5257 separated graph chunks per-repository. This change assumes we'll be able to pull stuff from APC most of the time and that the cost of switching chunks is not very large, so we can just build one chunk cache across all repositories. This allows the cache to be simpler.
- D5257 needed an offline cache, and used a unique cache structure. Since this one can be built online it can mostly use normal cache code.
- This also supports online appends to the cache.
- Finally, this has a timeout to guarantee a ceiling on the worst case: the worst case is something like a query for a file that has never existed, in a repository which receives exactly 1 commit every time other repositories receive 4095 commits, on a cold cache. If we hit cases like this we can bail after warming the cache up a bit and fall back to asking the VCS for an answer.
This cache isn't perfect, but I believe it will give us substantial gains in the average case. It can often satisfy "average-looking" queries in 4-8ms, and pathological-ish queries in 20ms on my machine; `hg` usually can't even start up in less than 100ms. The major thing that's attractive about this approach is that it does not require anything external or complicated, and will "just work", even producing reasonble improvements for users without APC.
In followups, I'll modify queries to use this cache and see if it holds up in more realistic workloads.
Test Plan:
- Used `bin/repository cache` to examine the behavior of this cache.
- Did some profiling/testing from the web UI using `debug.php`.
- This //appears// to provide a reasonable fast way to issue this query very quickly in the average case, without the various issues that plagued D5257.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, jhurwitz
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9045
Summary:
Ref T4455. This adds a `repository_parents` table which stores `<childCommitID, parentCommitID>` relationships.
For new commits, it is populated when commits are discovered.
For older commits, there's a `bin/repository parents` script to rebuild the data.
Right now, there's no UI suggestion that you should run the script. I haven't come up with a super clean way to do this, and this table will only improve performance for now, so it's not important that we get everyone to run the script right away. I'm just leaving it for the moment, and we can figure out how to tell admins to run it later.
The ultimate goal is to solve T2683, but solving T4455 gets us some stuff anyway (for example, we can serve `diffusion.commitparentsquery` faster out of this cache).
Test Plan:
- Used `bin/repository discover` to discover new commits in Git, SVN and Mercurial repositories.
- Used `bin/repository parents` to rebuild Git and Mercurial repositories (SVN repos just exit with a message).
- Verified that the table appears to be sensible.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: jhurwitz, epriestley
Maniphest Tasks: T4455
Differential Revision: https://secure.phabricator.com/D9044
Summary: The removes the sprite sheet 'icons' and replaces it with FontAwesome fonts.
Test Plan:
- Grep for SPRITE_ICONS and replace
- Grep for sprite-icons and replace
- Grep for PhabricatorActionList and choose all new icons
- Grep for Crumbs and fix icons
- Test/Replace PHUIList Icon support
- Test/Replace ObjectList Icon support (foot, epoch, etc)
- Browse as many pages as I could get to
- Remove sprite-icons and move remarkup to own sheet
- Review this diff in Differential
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9052
Summary:
Also fix a few other minor issues:
- Use lint config.
- Fix a method signature from `arc unit --everything` (unrelated).
- Add a javelin doc.
Test Plan: Ran `arc lint`, `arc unit`, `arc linters`.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9072
Summary: See <http://fab.wmflabs.org/T88>. While this issue is on MW's side, these links are kind of ugly and have more readable alternate forms now. Update them to use proper modern forms.
Test Plan: See inlines.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9042
Summary:
Provides a working SMS implementation with support for Twilio.
This version doesn't really retry if we get any gruff at all. Future versions should retry.
Test Plan: used bin/sms to send messages and look at them.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: aurelijus, epriestley, Korvin
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D8930
Summary: Ref T4986. I think this is the last of the easy ones, there are about 10 not-quite-so-trivial ones left.
Test Plan:
- Viewed app results.
- Created panels.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9025
Summary: Ref T4986. Getting closer. Nothing out of the ordinary in this group.
Test Plan:
For each application:
- Viewed the normal search results.
- Created a panel version and viewed it.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9024
Summary: Ref T4986. These are mostly mechanical now, I skipped a couple of slightly tricky ones. Still a bunch to go.
Test Plan:
For each engine:
- Viewed the application;
- created a panel to issue the query.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9017
Summary:
Ref T4986. This one needs `getApplicationURI()` so make it a little beefier to deal with that.
(It would be vaguely nice to somehow share the handle and application stuff between Controllers and Engine classes like this, but I don't immediately see a clean way to do it without traits. Not a big deal, in any case.)
Test Plan:
- Viewed Calendar.
- Made a Calendar panel.
- Viewed feed.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9012
Summary:
Ref T4986. This adds a bit of structure for handles, since we used to have Controller utilities but no longer do.
Hopefully these will start going faster soon...
Test Plan:
- Checked feed for collateral damage.
- Checked slowvote for collateral damage.
- Made a slowvote panel.
{F151550}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9010
Summary:
Ref T4986. We need to introduce alternate views to make this more pleasant, but let rendering move to engines so it can be shared between panels and controllers.
I also moved some of the pagination logic in to avoid duplicating that.
So far, only Feed works. I'm going to do these gradually since we have ~40-50 of them.
Test Plan:
- Used global search to check for collateral damage.
- Used not-global search too.
- Used normal feed.
{F151541}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9008
Summary: Ref T4986. This isn't pretty/usable yet (I need to move rendering out of ListController classes and into SearchEngine classes, I think) but does pull the correct results.
Test Plan: {F151537}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9007
Summary:
This plugin provides an OAuth authentication provider to authenticate users using WordPress.com Connect.
This diff corresponds to github pull request https://github.com/facebook/phabricator/pull/593/ and had its libphutil counterpart reviewed in D9004.
Test Plan: Configured WordPress.com as an authentication provider, saw it show up on the login screen, registered a new account, got expected defaults for my username/name/email/profile picture.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9019
Summary:
Ref T4119. This is ugly for now, but technically works.
The comment area and transaction log don't realy know about each other, so for the moment the linking is a bit manual. Differential/Maniphest are special cases anyway.
Test Plan: {F149992}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4119
Differential Revision: https://secure.phabricator.com/D8957
Summary:
Fixes T4909. Adds a "remove" link next to the edit link, which permanently hides a comment. Addresses two use cases:
- Allowing administrators to clean up spam.
- Allowing users to try to put the genie back in the bottle if they post passwords or sensitive links, etc.
The user who removed the comment is named in the removal text to enforce some level of administrative accountability.
No data is deleted, but there's currently no method to restore these comments. We'll see if we need one.
This is cheating a little bit by storing "removed" as "2" in the isDeleted field. This doesn't seem tooooo bad for now.
Test Plan:
- Removed some of my comments.
- As an administrator, removed other users' comments.
- Failed to view history of a removed comment.
- Failed to edit a removed comment.
- Failed to remove a removed comment.
- Verified feed doesn't show the old comment after comment removal.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: qgil, chad, epriestley
Maniphest Tasks: T4909
Differential Revision: https://secure.phabricator.com/D8945
Summary: Ref T4843. Chips away at a few more things.
Test Plan: Used VoiceOver and got a generally more sensible-seeming result.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4843
Differential Revision: https://secure.phabricator.com/D8978
Summary: Fixes T4931. Each new credential should come with the ability to lock the credential permanently, so that no one can ever edit again. Each existing credential must allow user to lock existing credential.
Test Plan: Create new credential, verify that you can lock it before saving it. Open existing unlocked credential, verify that option to lock it exists. Once credential is locked, the option to reveal it should be disabled, and editing the credential won't allow username/password updates.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4931
Differential Revision: https://secure.phabricator.com/D8947
Summary:
Ref T4749. Ref T3265. Ref T4909. Several goals here:
- Move user destruction to the CLI to limit the power of rogue admins.
- Start consolidating all "destroy named object" scripts into a single UI, to make it easier to know how to destroy things.
- Structure object destruction so we can do a better and more automatic job of cleaning up transactions, edges, search indexes, etc.
- Log when we destroy objects so there's a record if data goes missing.
Test Plan: Used `bin/remove destroy` to destroy several users.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3265, T4749, T4909
Differential Revision: https://secure.phabricator.com/D8940
Summary: Ref T4938. `arc close` needs to know about custom statuses and this conduit method is step 1 of letting it know
Test Plan: See next diff, which works!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4938
Differential Revision: https://secure.phabricator.com/D8937
Summary:
Ref T4398. This prompts users for multi-factor auth on login.
Roughly, this introduces the idea of "partial" sessions, which we haven't finished constructing yet. In practice, this means the session has made it through primary auth but not through multi-factor auth. Add a workflow for bringing a partial session up to a full one.
Test Plan:
- Used Conduit.
- Logged in as multi-factor user.
- Logged in as no-factor user.
- Tried to do non-login-things with a partial session.
- Reviewed account activity logs.
{F149295}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8922
Summary: Ref T4398. Prevent users from brute forcing multi-factor auth by rate limiting attempts. This slightly refines the rate limiting to allow callers to check for a rate limit without adding points, and gives users credit for successfully completing an auth workflow.
Test Plan: Tried to enter hisec with bad credentials 11 times in a row, got rate limited.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8911
Summary:
Ref T4398. The major goals here is to let administrators strip auth factors in two cases:
- A user lost their phone and needs access restored to their account; or
- an install previously used an API-based factor like SMS, but want to stop supporting it (this isn't possible today).
Test Plan:
- Used `bin/auth list-factors` to show installed factors.
- Used `bin/auth strip` with various mixtures of flags to selectively choose and strip factors from accounts.
- Also ran `bin/auth refresh` to verify refreshing OAuth tokens works (small `OAuth` vs `OAuth2` tweak).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8909
Summary: Ref T3583. Use the same approach Harbormaster does to give panels cheap forms.
Test Plan:
{F149218}
{F149219}
{F149220}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3583
Differential Revision: https://secure.phabricator.com/D8919
Summary:
Ref T3583. Adds edges, query relationships, etc. Lots of debugging/temporary UI.
My general intent here is to use edges to track where panels appear, and then put additional data on the dashboard itself to control layout, positioning, etc.
Dashboards don't actually render yet so this is still pretty boring.
Test Plan:
{F149175}
{F149176}
{F149177}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3583
Differential Revision: https://secure.phabricator.com/D8916
Summary: Ref T3583. These will be the primary class carrying panel implementations.
Test Plan:
{F149125}
{F149126}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3583
Differential Revision: https://secure.phabricator.com/D8912
Summary: 'cuz those can be complicated. Fixes T4738. I needed to do a fair amount of heavy lifting to get the policy stuff rendering correctly. For now, I made this end point very one purpose and tried to make that clear.
Test Plan: looked at some custom policies. see screenshots.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4738
Differential Revision: https://secure.phabricator.com/D8890
Summary:
Ref T4398. This is still pretty rough and isn't exposed in the UI yet, but basically works. Some missing features / areas for improvement:
- Rate limiting attempts (see TODO).
- Marking tokens used after they're used once (see TODO), maybe. I can't think of ways an attacker could capture a token without also capturing a session, offhand.
- Actually turning this on (see TODO).
- This workflow is pretty wordy. It would be nice to calm it down a bit.
- But also add more help/context to help users figure out what's going on here, I think it's not very obvious if you don't already know what "TOTP" is.
- Add admin tool to strip auth factors off an account ("Help, I lost my phone and can't log in!").
- Add admin tool to show users who don't have multi-factor auth? (so you can pester them)
- Generate QR codes to make the transfer process easier (they're fairly complicated).
- Make the "entering hi-sec" workflow actually check for auth factors and use them correctly.
- Turn this on so users can use it.
- Adding SMS as an option would be nice eventually.
- Adding "password" as an option, maybe? TOTP feels fairly good to me.
I'll post a couple of screens...
Test Plan:
- Added TOTP token with Google Authenticator.
- Added TOTP token with Authy.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8875
Summary:
Ref T4398. This adds a settings panel for account activity so users can review activity on their own account. Some goals are:
- Make it easier for us to develop and support auth and credential information, see T4398. This is the primary driver.
- Make it easier for users to understand and review auth and credential information (see T4842 for an example -- this isn't there yet, but builds toward it).
- Improve user confidence in security by making logging more apparent and accessible.
Minor corresponding changes:
- Entering and exiting hisec mode is now logged.
- This, sessions, and OAuth authorizations have moved to a new "Sessions and Logs" area, since "Authentication" was getting huge.
Test Plan:
- Viewed new panel.
- Viewed old UI.
- Entered/exited hisec and got prompted.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8871
Summary:
Ref T4398. Ref T4842. I want to let users review their own account activity, partly as a general security measure and partly to make some of the multi-factor stuff easier to build and debug.
To support this, implement modern policies and application search.
I also removed the "old" and "new" columns from this output, since they had limited utility and revealed email addresses to administrators for some actions. We don't let administrators access email addresses from other UIs, and the value of doing so here seems very small.
Test Plan: Used interface to issue a bunch of queries against user logs, got reasonable/expected results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: keir, epriestley
Maniphest Tasks: T4842, T4398
Differential Revision: https://secure.phabricator.com/D8856
Summary:
Ref T4398. This is roughly a "sudo" mode, like GitHub has for accessing SSH keys, or Facebook has for managing credit cards. GitHub actually calls theirs "sudo" mode, but I think that's too technical for big parts of our audience. I've gone with "high security mode".
This doesn't actually get exposed in the UI yet (and we don't have any meaningful auth factors to prompt the user for) but the workflow works overall. I'll go through it in a comment, since I need to arrange some screenshots.
Test Plan: See guided walkthrough.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8851
Summary: ...also kills off "PhabricatorAuditCommitQuery" and "PhabricatorAuditQuery", by moving the work to "DiffusionCommitQuery". Generally cleans up some code around the joint on this too. Also provides policies for audit requests, which is basically the policy for the underlying commit. Fixes T4715. (For the TODO I added about files, I just grabbed T4713.)
Test Plan:
Audit: verified the three default views all showed the correct things, including highligthing. did some custom queries and got the correct results.
Diffusion: verified "blame view" still worked. verified paths were highlighted for packages i owned.
Home: verified audit boxes showed up with proper commits w/ audits
bin/audit: played around with it via --dry-run and got the right audits back
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, epriestley, Korvin
Maniphest Tasks: T4715
Differential Revision: https://secure.phabricator.com/D8805
Summary: Fixes T3566 List of poll actions should include ability to close an open poll or reopen a closed poll.
Test Plan: Poll author should be able to close/reopen poll. Non-author should get policy screen when attempting to close/reopen poll.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T3566
Differential Revision: https://secure.phabricator.com/D8846
Summary:
Ref T4830. A few methods, like `conduit.ping`, are callable without authentication, so this even has some use cases. Also:
- Make some Differential stuff a little more consistent.
- Use slightly more modern rendering.
- Deprecate the status-oriented `user` calls; these will be replaced by Calendar methods.
Test Plan: Browsed console as logged out / logged in users.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4830
Differential Revision: https://secure.phabricator.com/D8826
Summary:
Ref T3662. Releeph blocks users from requsting unparsed commits, but there's no real technical reason for this.
The `releephwork.getorigcommitmessage` method assumes data exists, but should be replaced with `diffusion.querycommits` anyway.
Test Plan: Ran `diffusion.querycommits`. Requested a commit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3662
Differential Revision: https://secure.phabricator.com/D8823
Summary:
Ref T3662. Ref T3549. These methods are pretty conservative for now, but get the structure in place.
Also do a bunch more project -> product stuff.
Test Plan: Made calls to both methods, browsed around the UI a fair amount.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3549, T3662
Differential Revision: https://secure.phabricator.com/D8816
Summary:
Ref T3718. Ref T3644. Ref T3092. Switches from the Releeph UI elements to standard ones. I'll attach some screenshots.
Also fixes CSRF against the request action endpoint.
Test Plan:
- Viewed request details.
- Took actions on a request from detail page.
- Viewed request list.
- Took actions on a request from list page.
- Used keyboard shortcuts to navigate list.
- Used keyboard shortcuts to take actions.
- Simulated errors.
- Viewed on devices.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: grp, FacebookPOC, mattlqx, tala, beng, LegNeato, epriestley
Maniphest Tasks: T3718, T3092, T3644
Differential Revision: https://secure.phabricator.com/D8771
Summary:
Ref T4810. Ultimate goal is to let Harbormaster post a "build passed/failed" transaction. To prepare for that, implement `PhabricatorApplicationTransactionInterface` in Differential.
To allow Harbormaster to take action on //diffs// but have the transactions apply to //revisions//, I added a new method so that objects can redirect transactions to some other object.
Test Plan:
- Subscribed/unsubscribed/attached/detached from Differential, saw transactions appear properly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4810
Differential Revision: https://secure.phabricator.com/D8802
Summary:
Ref T4809. Currently, buildables have a status field but nothing populates it. Populate it:
- When builds change state, update the Buildable state.
- Use the new Buildable state on the web UI.
- Return the new Buildable state from Conduit.
To make it easier to debug/test this:
- Provide `bin/harbormaster update Bxxx ...` to force foreground update of a Buildable.
Test Plan:
- Used `bin/harbormaster update Bxxx --force --trace` to update buildables.
- Looked at buidlable list, saw statuses reported properly.
- Used Conduit to read statuses.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4809
Differential Revision: https://secure.phabricator.com/D8799
Summary:
Ref T4809. This one is more straightforward. A couple of tweaks:
- Remove the WAITING status, since nothing ever sets it and I suspect nothing ever will with the modern way artifacts work (maybe). At a minimum, it's confusing with the new Target status that's also called "WAITING" but means something different.
- Consolidate 17 copies of these status names into one method.
Test Plan: Ran some queries via Conduit, got reasonable looking results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4809
Differential Revision: https://secure.phabricator.com/D8795
