Add Phabricator support

[sidpremkumar]

Adds Phabricator support to review-rot. Look at examples/sampleinput.yaml to see how to specify users. If no repos are specified will search for all open reviews on the host (status = status-open).

• @ralphbean
• @pbortlov

[ralphbean]

Check out this diff:

diff --git a/examples/sampleinput.yaml b/examples/sampleinput.yaml
index c624ea1..53c8fd8 100644
--- a/examples/sampleinput.yaml
+++ b/examples/sampleinput.yaml
@@ -28,6 +28,6 @@
   token: my_phabricator_token
   host: my_phabricator_host
   repos:
-    - phabricator/user_1
-    - phabricator/user_2
-    - phabricator/user_3
+    - user_1
+    - user_2
+    - user_3
diff --git a/reviewrot/phabricatorstack.py b/reviewrot/phabricatorstack.py
index bef613e..22788c8 100644
--- a/reviewrot/phabricatorstack.py
+++ b/reviewrot/phabricatorstack.py
@@ -10,11 +10,11 @@ class PhabricatorService(BaseService):
     """
         This class represents Phabricator Service for Review Rot.
         The reference can be found here: https://www.phacility.com/phabricator/
-        This class takes in a list of repo_name (i.e. usernames) and then
+        This class takes in a list of user_name and then
         attempts to find all open reviews where those users are responsible.
         Currently this does not query based on  different repositories.
     """
-    def request_reviews(self, host, token, repo_name=None, state_=None,
+    def request_reviews(self, host, token, user_name=None, state_=None,
                         value=None, duration=None, show_last_comment=None,
                         **kwargs):
         """
@@ -26,8 +26,8 @@ class PhabricatorService(BaseService):
             host (str): The host Phabricator server url
             token (str): Phabricator token for authentication
                             (Looks like 'api-1234567890x')
-            repo_name (str): Phabricator repository name for specified
-                             username or organization
+            user_name (str): Phabricator repository name for specified
+                             username
             state_ (str): The filter state for pull requests, e.g, older
                           or newer
             value (int): The value in terms of duration for requests
@@ -49,10 +49,10 @@ class PhabricatorService(BaseService):
         phab.update_interfaces()
         # Create response list
         response = []
-        if repo_name:
-            # Find open reviews for user (aka repo_name)
+        if user_name:
+            # Find open reviews for user (aka user_name)
             # Look up users phid
-            user_phid = self.user_search(repo_name, phab)['data'][0]['phid']
+            user_phid = self.user_search(user_name, phab)['data'][0]['phid']
             reviews = self.differential_query(status='status-open',
                                               responsibleUsers=[user_phid],
                                               phab=phab)

Above, I think you can make that repo_name argument go away and instead use the much more natural user_name.

[ralphbean]

Two functional points that need to be addressed:

• If I list 12 users in my config, it queries phab for the reviews for each of them, and then returns the whole list. But if all twelve users are all cc'd on the same reviews in phabricator, then I get twelve duplicates printed out by review-rot for each of those reviews. These should be de-duplicated in the output.
• The runtime performance of this gets pretty slow if I list 12 users in my config. It has to query phabricator twelve different times, once for each of them.

Now - you can solve both of the above problems by (somehow) querying phabricator only once, asking for all reviews on which any of the given twelve persons are "responsible persons". Phabricator supports this, but you may need to do some re-wiring in bin/review-rot around the call to git_service.request_reviews. Change the interface? Or - maybe phabricator is odd enough that it doesn't fit the mold and needs to have special handling there.

[ralphbean]

I just tested based on 5366aa0 and this works great. It meets my needs, functionally.

I'll leave it to @sidpremkumar, @ian-panzica, and anyone else from AQE to haggle over code style, cleanliness, and tests, etc.

Thanks for working on this, @sidpremkumar! Give me a ping please once it is merged so I can roll it out for the team that needs it.