1. 29 Mar, 2020 1 commit
    • Daniel Stone's avatar
      WIP: Transactions: Hide transactions involving restricted objects · 3e949520
      Daniel Stone authored
      If a transaction has an object that a particular user cannot see, then
      mark the transaction as hidden for the default transaction view. This
      particularly elides 'foo moved this task from Restricted Workboard
      Column to Restricted Workboard Column on the Restricted Project board'
      messages in the timeline, which are not hugely useful.
      
      This would need a fair bit more work to go upstream, especially eliding
      notifications for restricted-only transactions as well.
      3e949520
  2. 28 May, 2019 1 commit
    • epriestley's avatar
      Test for "CAN_INTERACT" on comment edits in a way that survives objects which... · 53b9acfb
      epriestley authored
      Test for "CAN_INTERACT" on comment edits in a way that survives objects which only implement "CAN_VIEW"
      
      Summary:
      Ref T13289. See D20551. In D20551, I implemented some "CAN_INTERACT" checks against certain edits, but these checks end up testing "CAN_INTERACT" against objects like Conpherence threads which do not support a distinct "CAN_INTERACT" permission. I misrembered how the "CAN_INTERACT" fallback to "CAN_VIEW" actually works: it's not fully automatic, and needs some explicit "interact, or view if interact is not available" checks.
      
      Use the "interact" wrappers to test these policies so they fall back to "CAN_VIEW" if an object does not support "CAN_INTERACT". Generally, objects which have a "locked" state have a separate "CAN_INTERACT" permission; objects which don't have a "locked" state do not.
      
      Test Plan: Created and edited comments in Conpherence (or most applications other than Maniphest).
      
      Reviewers: amckinley
      
      Maniphest Tasks: T13289
      
      Differential Revision: https://secure.phabricator.com/D20558
      53b9acfb
  3. 24 May, 2019 1 commit
    • epriestley's avatar
      Prevent editing and deleting comments in locked conversations · aacc6246
      epriestley authored
      Summary:
      Ref T13289. This tightens up a couple of corner cases around locked threads.
      
      Locking is primarily motivated by two use cases: stopping nonproductive conversations on open source installs (similar to GitHub's feature); and freezing object state for audit/record-keeping purposes.
      
      Currently, you can edit or remove comments on a locked thread, but neither use case is well-served by allowing this. Require "CAN_INTERACT" to edit or remove a comment.
      
      Administrators can still remove comments from a locked thread to serve "lock a flamewar, then clean it up", since "Remove Comment" on a comment you don't own is fairly unambiguously an administrative action.
      
      Test Plan:
        - On a locked task, tried to edit and remove my comments as a non-administrator. Saw appropriate disabled UI state and error dialogs (actions were disallowed).
        - On a locked task, tried to remove another user's comments as an administrator. This works.
        - On a normal task, edited comments normally.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13289
      
      Differential Revision: https://secure.phabricator.com/D20551
      aacc6246
  4. 09 Feb, 2019 1 commit
    • epriestley's avatar
      When an edit overrides an object lock, note it in the transaction record · a20f1080
      epriestley authored
      Summary:
      Ref T13244. See PHI1059. When you lock a task, users who can edit the task can currently override the lock by using "Edit Task" if they confirm that they want to do this.
      
      Mark these edits with an emblem, similar to the "MFA" and "Silent" emblems, so it's clear that they may have bent the rules.
      
      Also, make the "MFA" and "Silent" emblems more easily visible.
      
      Test Plan:
      Edited a locked task, overrode the lock, got marked for it.
      
      {F6195005}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: aeiser
      
      Maniphest Tasks: T13244
      
      Differential Revision: https://secure.phabricator.com/D20131
      a20f1080
  5. 20 Dec, 2018 1 commit
    • epriestley's avatar
      Remove "willRenderTimeline()" from ApplicationTransactionInterface · 6c43d1d5
      epriestley authored
      Summary:
      Depends on D19914. Ref T11351. Some of the Phoilo rabbit holes go very deep.
      
      `PhabricatorApplicationTransactionInterface` currently requires you to implement `willRenderTimeline()`. Almost every object just implements this as `return $timeline`; only Pholio, Diffusion, and Differential specialize it. In all cases, they are specializing it mostly to render inline comments.
      
      The actual implementations are a bit of a weird mess and the way the data is threaded through the call stack is weird and not very modern.
      
      Try to clean this up:
      
        - Stop requiring `willRenderTimeline()` to be implemented.
        - Stop requiring `getApplicationTransactionViewObject()` to be implemented (only the three above, plus Legalpad, implement this, and Legalpad's implementation is a no-op). These two methods are inherently pretty coupled for almost any reasonable thing you might want to do with the timeline.
        - Simplify the handling of "renderdata" and call it "View Data". This is additional information about the current view of the transaction timeline that is required to render it correctly. This is only used in Differential, to decide if we can link an inline comment to an anchor on the same page or should link it to another page. We could perhaps do this on the client instead, but having this data doesn't seem inherently bad to me.
        - If objects want to customize timeline rendering, they now implement `PhabricatorTimelineInterface` and provide a `TimelineEngine` which gets a nice formal stack.
      
      This leaves a lot of empty `willRenderTimeline()` implementations hanging around. I'll remove these in the next change, it's just going to be deleting a couple dozen copies of an identical empty method implementation.
      
      Test Plan:
        - Viewed audits, revisions, and mocks with inline comments.
        - Used "Show Older" to page a revision back in history (this is relevant for "View Data").
        - Grepped for symbols: willRenderTimeline, getApplicationTransactionViewObject, Legalpad classes.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T11351
      
      Differential Revision: https://secure.phabricator.com/D19918
      6c43d1d5
  6. 12 Sep, 2018 1 commit
  7. 19 Jan, 2018 1 commit
    • epriestley's avatar
      Prepare TransactionEditor for silent transactions via bulk edit · 8b12fa6d
      epriestley authored
      Summary:
      Ref T13042. This adds a "silent" edit mechanism which suppresses feed stories, email, and notifications.
      
      The other behaviors here are:
      
        - The transactions are marked as "silent" so we can render a hint in the UI in the future to make it clear to users that they aren't missing email.
        - If the editor uses Herald, mail rules are suppressed so they don't fire incorrectly (this mostly affects "the first time this rule matches, send me an email" rules: without this, they'd match "the first time" on the bulk edit, not send email, then never match again since they already matched).
        - If the edit queues additional edits, those are applied silently too.
      
      This doesn't (or, at least, shouldn't) actually change any behavior since you can't apply silent edits yet.
      
      Test Plan:
      Somewhat theoretical, since this isn't reachable yet. Should get meaningful testing in an upcoming change.
      
      Did a bit of var_dump() / debug poking to attempt to verify that nothing too crazy is happening.
      
      Viewed and edited objects, no changes in behavior.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13042
      
      Differential Revision: https://secure.phabricator.com/D18882
      8b12fa6d
  8. 31 Dec, 2016 1 commit
  9. 18 Nov, 2016 1 commit
  10. 07 Jun, 2016 1 commit
    • epriestley's avatar
      Add links and diffs for text block edits to mail · fb2da8bd
      epriestley authored
      Summary:
      Ref T7643.
      
        - When a transaction edits a text block, add a link to the changes (for HTML mail).
        - Also, inline the changes in the mail (for HTML mail).
        - Do nothing for text mail since I don't think we really have room? And I don't know how we can make the diff look any good.
      
      Test Plan:
      Edited a task description, generated mail, examined mail.
      
        - It contained a link leading to a prose diff.
        - It had a more-or-less reasonable inline text diff.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T7643
      
      Differential Revision: https://secure.phabricator.com/D16063
      fb2da8bd
  11. 05 Dec, 2015 2 commits
    • epriestley's avatar
      Fix a loopy comment · 75f126c3
      epriestley authored
      Summary: I wrote this earlier in D14680 but have now realized that it's the same sentence twice when read carefully.
      
      Test Plan: read more carefully
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Differential Revision: https://secure.phabricator.com/D14687
      75f126c3
    • epriestley's avatar
      Fix confusing ordering of similar actions in transaction groups · 77d33ec7
      epriestley authored
      Summary:
      Fixes T7250. Currently, if a display group of transactions (multiple transactions by the same author in a short period of time with no intervening comments) has several transactions of similar strength (e.g., several status change transactions) we can end up displaying them in reverse chronological order, which is confusing.
      
      Instead, make sure transactions of the same type/strength are always in logical order.
      
      Test Plan:
        - Merged a task into another task, then reopened the merged task.
        - Before patch: merge/reopen showed in wrong order.
      
      {F1014954}
      
        - After patch: merge/reopen show in correct order.
      
      {F1014955}
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T7250
      
      Differential Revision: https://secure.phabricator.com/D14680
      77d33ec7
  12. 24 Jul, 2015 1 commit
  13. 22 May, 2015 1 commit
  14. 02 Mar, 2015 1 commit
  15. 12 Jan, 2015 1 commit
  16. 09 Jan, 2015 1 commit
  17. 01 Jan, 2015 1 commit
    • Fabian Stelzer's avatar
      Settings History · 86eb7c0e
      Fabian Stelzer authored
      Summary:
      Shows a timeline of all modified settings Fixes T6545
      Will show all settings (no pagination, should be not so difficult to add if needed but most installs won't have hundreds of settings changes)
      I'm not happy by how the PhabricatorConfigTransaction object is instructed to render the config keys but i don't see any other reasonable way.
      We could always show the keys though.
      
      Test Plan: Changed settings and called the history page
      
      Reviewers: #blessed_reviewers, epriestley
      
      Reviewed By: #blessed_reviewers, epriestley
      
      Subscribers: Korvin, epriestley
      
      Maniphest Tasks: T6545
      
      Differential Revision: https://secure.phabricator.com/D11088
      86eb7c0e
  18. 11 Dec, 2014 1 commit
    • Bob Trahan's avatar
      Transactions - make quotes work for older transactions · b718b429
      Bob Trahan authored
      Summary: Fixes T6731. I don't really understand the intent behind the two view classes here, but to get this to work I need to pass yet more data to the lower-level class.
      
      Test Plan: Viewed a task with many comments. Clicked "show older". Quoted everything I could. Verified for each quote that it quoted correctly, inlcuding linking to the prior transaction.
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: Korvin, epriestley
      
      Maniphest Tasks: T6731
      
      Differential Revision: https://secure.phabricator.com/D10973
      b718b429
  19. 05 Dec, 2014 1 commit
    • Bob Trahan's avatar
      Transactions - fix pagination bug · a1a8083b
      Bob Trahan authored
      Summary: Fixes T6694. Ref T4712. Turns out the logic here was slightly incorrect; we don't want to use the id of the last thing we hid but rather the first thing we show. I had garbage test data ("asdsadsadsa", etc) I guess so I didn't notice this.
      
      Test Plan: made a new task where user a and user b alternated 3 comments each, cooperatively numbering them from 1 - 20. as both users, showed older transactions. pre-patch the issue described in T6694 occurred and post patch I saw the entire counting sequence.
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: Korvin, epriestley
      
      Maniphest Tasks: T4712, T6694
      
      Differential Revision: https://secure.phabricator.com/D10933
      a1a8083b
  20. 04 Dec, 2014 1 commit
    • Bob Trahan's avatar
      Transactions - adding willRenderTimeline to handle tricky cases · 6ab3f06b
      Bob Trahan authored
      Summary: Fixes T6693.
      
      Test Plan:
      Made a bunch of comments on a diff with differential, being sure to leave inlines here and there. This reproduced the issue in T6693. With this patch this issue no longer reproduces!
      
      Successfully "showed older changes" in Maniphest too.
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: Korvin, epriestley
      
      Maniphest Tasks: T6693
      
      Differential Revision: https://secure.phabricator.com/D10931
      6ab3f06b
  21. 02 Dec, 2014 2 commits
    • Bob Trahan's avatar
      Transactions - deploy buildTransactionTimeline against a few more applications · 69cc5df6
      Bob Trahan authored
      Summary:
      Ref T4712. Thus far, it seems that most "non-standard" things can be done pretty easily in the controller. Aside from deploying, this diff had to fix a few bugs / missing implementations of stuff.
      
      (Notably, PhabricatorAuthProviderConfig, HeraldRule, PhabricatorSlowvotePoll, and AlmanacNetwork needed to implement PhabricatorApplicationTransactionInterface, PhabricatorAuthAuthProviderPHIDType had to be added, and a rendering bug in transactions of type PhabricatorOAuth2AuthProvider had to be fixed.)
      
      Test Plan: Almanac - looked at binding, device, network, and service view controllers and verified timeline displayed properly. Herald - looked at a rule and verified timeline. Slowvote - looked at a vote and verified timeline. Auth - looked at an auth provider (Facebook) and verified proper display of transactions within timeline.
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: Korvin, epriestley
      
      Maniphest Tasks: T4712
      
      Differential Revision: https://secure.phabricator.com/D10921
      69cc5df6
    • Bob Trahan's avatar
      Transactions - add pagination to application transactions · d6341cff
      Bob Trahan authored
      Summary: Ref T4712. This adds pagination. Future diffs will need to deploy `buildTransactionTimeline` everywhere and massage this stuff as necessary if we hit any special cases.
      
      Test Plan: Set page size to "5" to make it need to paginate often. Verified proper transactions loaded in and the javascript actions worked.
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: Korvin, epriestley
      
      Maniphest Tasks: T4712
      
      Differential Revision: https://secure.phabricator.com/D10887
      d6341cff
  22. 16 Sep, 2014 1 commit
    • Bob Trahan's avatar
      Transactions - hide "mentioned in X" story if you can't see X · 444ced16
      Bob Trahan authored
      Summary: ...also re-jiggers all the anchor stuff to use $xaction ID. This seemed like the simplest way once I got in the code, as well as having nice properties for if / when we want to re-add some ajax stuff since the ID is a pretty solid piece of data to key off. Fixes T6083.
      
      Test Plan: mentioned DX in private DX+1. Could see on DX the mention as me and not as the other user. For transactions, I left a comment on Paste and it worked, and I edited an existing transaction and it worked.
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: epriestley, Korvin
      
      Maniphest Tasks: T6083
      
      Differential Revision: https://secure.phabricator.com/D10488
      444ced16
  23. 28 Jul, 2014 1 commit
    • epriestley's avatar
      Convert Audit comment rendering to standard infrastructure · 2082eda6
      epriestley authored
      Summary: Ref T4896. Depends on D10055. This uses core rendering stuff for audit comments, and fixes all the wonkiness with inlines so we can actually land the migration.
      
      Test Plan: Viewed, previewed and edited various types of comments in Diffusion.
      
      Reviewers: chad, btrahan, joshuaspence
      
      Reviewed By: joshuaspence
      
      Subscribers: epriestley
      
      Maniphest Tasks: T4896
      
      Differential Revision: https://secure.phabricator.com/D10056
      2082eda6
  24. 16 Jun, 2014 1 commit
  25. 09 Jun, 2014 1 commit
    • Joshua Spence's avatar
      Change double quotes to single quotes. · 0a62f134
      Joshua Spence authored
      Summary: Ran `arc lint --apply-patches --everything` over rP, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
      
      Test Plan: Eyeballed it.
      
      Reviewers: #blessed_reviewers, epriestley
      
      Reviewed By: #blessed_reviewers, epriestley
      
      Subscribers: epriestley, Korvin, hach-que
      
      Differential Revision: https://secure.phabricator.com/D9431
      0a62f134
  26. 19 May, 2014 1 commit
    • epriestley's avatar
      Disable the edit/quote menu for non-standard comments · 703e0b39
      epriestley authored
      Summary: Fixes T4930. We currently show the edit/quote menu if a transaction group has //inline// comments, but this doesn't make sense and doesn't work properly. Only show this menu if the group has a normal comment.
      
      Test Plan:
      Viewed these groups:
      
        - Normal comment (edits fine).
        - Just inlines (no more edit menu).
        - Inline + comment (edits fine, affects the normal comment properly).
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      Subscribers: bitglue, epriestley
      
      Maniphest Tasks: T4930
      
      Differential Revision: https://secure.phabricator.com/D9180
      703e0b39
  27. 05 May, 2014 3 commits
    • epriestley's avatar
      Move all comment management junk into a dropdown menu · bfc1ccfd
      epriestley authored
      Summary:
      man I sure hate Javascript
      
      I removed the ajax-edit and ajax-remove interactions, becuase they were prohibitively complex to get working given that the entire menu has to change too. Instead, the page just reloads. This works perfectly fine in practice.
      
      If we want to restore these in the future, we should have the server re-render the entire transaction group or something. I think very little is lost here, though.
      
      Test Plan:
        - Took all the actions.
        - Used existing dropdown menus.
      
      {F150196}
      
      Reviewers: chad, btrahan
      
      Reviewed By: btrahan
      
      Subscribers: epriestley
      
      Differential Revision: https://secure.phabricator.com/D8966
      bfc1ccfd
    • epriestley's avatar
      Add a quote action to Differential and Maniphest · 707c5aec
      epriestley authored
      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
      707c5aec
    • epriestley's avatar
      Allow users to remove their own comments, and administrators to remove any comment · 58f66fea
      epriestley authored
      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
      58f66fea
  28. 21 Apr, 2014 1 commit
  29. 17 Apr, 2014 1 commit
  30. 04 Apr, 2014 1 commit
    • Bob Trahan's avatar
      Transactions - make edit transactions that are grouped work nicely · 496a7d89
      Bob Trahan authored
      Summary: ...the key is to move a layer lower and beam down the updated comment. There is a wee bit of Javascript gymnastics going on here. Fixes T4608.
      
      Test Plan: made a comment + resolve. clicked edit and made changes. noted transaction updated correctly and "history" link worked. edited again to a deletion and noted the "this is deleted" looked right and history link still worked
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: epriestley, Korvin, chad
      
      Maniphest Tasks: T4608
      
      Differential Revision: https://secure.phabricator.com/D8702
      496a7d89
  31. 14 Mar, 2014 2 commits
    • epriestley's avatar
      Fix rendering of comments deleted by editing · 48910537
      epriestley authored
      Summary:
      Fixes T4609. Steps are:
      
        - Make a comment.
        - Edit it.
        - Delete all the text.
      
      We expect to see "This comment has been deleted." -- instead, things currently render goofy.
      
      Root cause is that `hasComment()` means both "comment object exists" //and// "comment object is nonempty".
      
      Test Plan: {F128862}
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      Subscribers: aran, epriestley
      
      Maniphest Tasks: T4609
      
      Differential Revision: https://secure.phabricator.com/D8533
      48910537
    • Chad Little's avatar
      End Cap for Timeline · 32573725
      Chad Little authored
      Summary: End-cap for timeline. Fixes T4438
      
      Test Plan: Tested on a timeline with and without endcap.
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: aran, epriestley, Korvin, chad, btrahan
      
      Maniphest Tasks: T4438
      
      Differential Revision: https://secure.phabricator.com/D8530
      32573725
  32. 26 Feb, 2014 2 commits
    • epriestley's avatar
      Stop markup cache from hitting cache O(N^2) times · 7ff21106
      epriestley authored
      Summary:
      The intent of getOrBuildEngine() is to save some boilerplate in cases where we're just using a standard engine, but it didn't get cached so we'd rebuilt it over and over again.
      
      This was especially bad in Differential with a large number of inlines.
      
      Test Plan: "Query" tab of services is no longer quite so ridiculous in Differential.
      
      Reviewers: btrahan, dctrwatson
      
      Reviewed By: btrahan
      
      CC: aran
      
      Differential Revision: https://secure.phabricator.com/D8352
      7ff21106
    • Joshua Spence's avatar
      Various linter fixes. · 62701147
      Joshua Spence authored
      Summary:
      - Removed trailing newlines.
      - Added newline at EOF.
      - Removed leading newlines.
      - Trimmed trailing whitespace.
      - Spelling fix.
      - Added newline at EOF
      
      Test Plan: N/A
      
      Reviewers: epriestley, #blessed_reviewers
      
      Reviewed By: epriestley
      
      CC: hach-que, chad, Korvin, epriestley, aran
      
      Differential Revision: https://secure.phabricator.com/D8344
      62701147
  33. 21 Feb, 2014 1 commit
    • epriestley's avatar
      Allow CustomField to provide ApplicationTransaction change details · 88227d26
      epriestley authored
      Summary:
      Ref T3886. Ref T418. For fields like "Summary" and "Test Plan" where changes can't be summarized in one line, allow CustomField to provide a "(Show Details)" link and render a diff.
      
      Also consolidate some of the existing copy/paste, and simplify this featuer slightly now that we've move to dialogs.
      
      Test Plan:
      {F115918}
      
        - Viewed "description"-style field changes in phlux, pholio, legalpad, maniphest, differential, ponder (questions), ponder (answers), and repositories.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      CC: aran
      
      Maniphest Tasks: T3886, T418
      
      Differential Revision: https://secure.phabricator.com/D8284
      88227d26
  34. 14 Feb, 2014 1 commit
    • epriestley's avatar
      Hide seen transactions by default in all modern applications · 7a96de10
      epriestley authored
      Summary:
      Ref T2222. This restores the "N older comments are hidden." shield to all ApplicationTransactions applications. Roughly the rule this uses is that transactions older than your most recent comment are hidden, under the assumption that you've already read and dealt with them, since you replied afterward. Then we show your last comment to remind/contextualize you, and anything afterward. We also don't hide transactions if we'd only be hiding a handfull, and we never hide the few most recent transactions.
      
      This might need some #design help.
      
      Test Plan:
      The tricky part here is the anchor rule, which deals with the case where you follow a link to `T123#4`, but that would normally be hidden. We simulate a click on "show all" if you hit an anchor which is hidden. Here's what it looks like in Maniphest:
      
      {F112891}
      
      Reviewers: btrahan, chad
      
      Reviewed By: btrahan
      
      CC: chad, aran
      
      Maniphest Tasks: T2222
      
      Differential Revision: https://secure.phabricator.com/D8229
      7a96de10