Bug 1440828 - Phabricator review requests should show up on the BMO dashboard

7 files changed, 312 insertions, 53 deletions

diff --git a/extensions/MyDashboard/template/en/default/pages/mydashboard.html.tmpl b/extensions/MyDashboard/template/en/default/pages/mydashboard.html.tmpl
index 36a8db6f2..a81a5903b 100644
--- a/extensions/MyDashboard/template/en/default/pages/mydashboard.html.tmpl
+++ b/extensions/MyDashboard/template/en/default/pages/mydashboard.html.tmpl
@@ -105,6 +105,7 @@
       <div id="query_container">
         <div class="query_heading"></div>
         <div class="query_description"></div>
+        <span id="query_loading" class="items_found">Loading...</span>
         <span id="query_count_refresh" class="bz_default_hidden">
           <span class="items_found" id="query_bugs_found">0 [% terms.bugs %] found</span>
           | <a class="refresh"     href="javascript:void(0);" id="query_refresh">Refresh</a>
@@ -126,29 +127,28 @@
         %]
       </div>
 
-      <div id="requestee_container"> 
-        <div class="query_heading">
-          Flags Requested of You
+      [% BLOCK requests_table %]
+        <div id="[% name FILTER html %]_container" class="requests">
+            <div class="query_heading">[% title FILTER html %]</div>
+            <span id="[% name FILTER html %]_loading" class="items_found">Loading...</span>
+            <span id="[% name FILTER html %]_count_refresh" class="bz_default_hidden">
+            <span class="items_found" id="[% name FILTER html %]_flags_found">0 reviews found</span>
+            | <a class="refresh" href="javascript:void(0);" id="[% name FILTER html %]_refresh">Refresh</a>
+            | <a class="buglist" href="javascript:void(0);" id="[% name FILTER html %]_buglist">Buglist</a>
+            </span>
+            <div id="[% name FILTER html %]_table"></div>
         </div>
-        <span id="requestee_count_refresh" class="bz_default_hidden">
-          <span class="items_found" id="requestee_flags_found">0 flags found</span>
-          | <a class="refresh" href="javascript:void(0);" id="requestee_refresh">Refresh</a>
-          | <a class="buglist" href="javascript:void(0);" id="requestee_buglist">Buglist</a>
-        </span>
-        <div id="requestee_table"></div>
-      </div>
+      [% END %]
 
-      <div id="requester_container">
-        <div class="query_heading">
-          Flags You Have Requested
-        </div>
-        <span id="requester_count_refresh" class="bz_default_hidden">
-          <span class="items_found" id="requester_flags_found">0 flags found</span>
-          | <a class="refresh" href="javascript:void(0);" id="requester_refresh">Refresh</a>
-          | <a class="buglist" href="javascript:void(0);" id="requester_buglist">Buglist</a>
-        </span>
-        <div id="requester_table"></div>
-      </div>
+      [% ## no-008filter
+        # requires PhabBugz extension
+        IF Param('phabricator_enabled');
+            PROCESS requests_table name='reviews' title='Reviews Requested of You';
+        END;
+
+        PROCESS requests_table name='requestee' title='Flags Requested of You';
+        PROCESS requests_table name='requester' title='Flags You Have Requested';
+      %]
     </div>
     <div style="clear:both;"></div>
     [% IF user.showmybugslink OR user.queries.size OR user.queries_subscribed.size %]
diff --git a/extensions/MyDashboard/web/js/flags.js b/extensions/MyDashboard/web/js/flags.js
index 95b256708..b56559ae3 100644
--- a/extensions/MyDashboard/web/js/flags.js
+++ b/extensions/MyDashboard/web/js/flags.js
@@ -16,15 +16,18 @@ $(function () {
         // Common
         var counter = 0;
         var dataSource = {
+            reviews: null,
             requestee: null,
             requester: null
         };
         var dataTable = {
+            reviews: null,
             requestee: null,
             requester: null
         };
+        var hasReviews = !!document.getElementById('reviews_container');
 
-        var updateFlagTable = function(type) {
+        var updateRequestsTable = function(type) {
             if (!type) return;
 
             counter = counter + 1;
@@ -32,32 +35,40 @@ $(function () {
             var callback = {
                 success: function(e) {
                     if (e.response) {
+                        Y.one('#' + type + '_loading').addClass('bz_default_hidden');
                         Y.one('#' + type + '_count_refresh').removeClass('bz_default_hidden');
                         Y.one("#" + type + "_flags_found").setHTML(
-                            e.response.results.length + ' flags found');
+                            e.response.results.length +
+                            ' request' + (e.response.results.length == 1 ? '' : 's') +
+                            ' found');
                         dataTable[type].set('data', e.response.results);
                     }
                 },
                 failure: function(o) {
-                    if (o.error) {
-                        alert("Failed to load flag list from Bugzilla:\n\n" + o.error.message);
+                    Y.one('#' + type + '_loading').addClass('bz_default_hidden');
+                    Y.one('#' + type + '_count_refresh').removeClass('bz_default_hidden');
+                    if (o.error && o.error.message) {
+                        alert("Failed to load requests:\n\n" + o.error.message);
                     } else {
-                        alert("Failed to load flag list from Bugzilla.");
+                        alert("Failed to load requests.");
                     }
                 }
             };
 
+            var method = type === 'reviews' ? 'PhabBugz.needs_review' : 'MyDashboard.run_flag_query';
             var json_object = {
                 version: "1.1",
-                method:  "MyDashboard.run_flag_query",
+                method:  method,
                 id:      counter,
-                params:  { type : type,
-                        Bugzilla_api_token : (BUGZILLA.api_token ? BUGZILLA.api_token : '')
+                params:  {
+                    type : type,
+                    Bugzilla_api_token : (BUGZILLA.api_token ? BUGZILLA.api_token : '')
                 }
             };
 
             var stringified = Y.JSON.stringify(json_object);
 
+            Y.one('#' + type + '_loading').removeClass('bz_default_hidden');
             Y.one('#' + type + '_count_refresh').addClass('bz_default_hidden');
 
             dataTable[type].set('data', []);
@@ -86,14 +97,17 @@ $(function () {
         };
 
         var bugLinkFormatter = function(o) {
+            if (!o.data.bug_id) {
+                return '-';
+            }
             var bug_closed = "";
             if (o.data.bug_status == 'RESOLVED' || o.data.bug_status == 'VERIFIED') {
                 bug_closed = "bz_closed";
             }
-            return '<a href="show_bug.cgi?id=' + encodeURIComponent(o.value) +
+            return '<a href="show_bug.cgi?id=' + encodeURIComponent(o.data.bug_id) +
                 '" target="_blank" ' + 'title="' + Y.Escape.html(o.data.bug_status) + ' - ' +
-                Y.Escape.html(o.data.bug_summary) + '" class="' + Y.Escape.html(bug_closed) +
-                '">' + o.value + '</a>';
+                Y.Escape.html(o.data.bug_summary) + '" class="' + bug_closed +
+                '">' + o.data.bug_id + '</a>';
         };
 
         var updatedFormatter = function(o) {
@@ -124,6 +138,85 @@ $(function () {
             }
         };
 
+        var phabAuthorFormatter = function(o) {
+            return '<span title="' + Y.Escape.html(o.data.author_email) + '">' +
+                Y.Escape.html(o.data.author_name) + '</span>';
+        };
+
+        var phabRowFormatter = function(o) {
+            var row = o.cell.ancestor();
+
+            // space in the 'flags' tables is tight
+            // render requests as two rows - diff title on first row, columns
+            // on second
+
+            row.insert(
+                '<tr class="' + row.getAttribute('class') + '">' +
+                '<td class="yui3-datatable-cell" colspan="4">' +
+                '<a href="' + o.data.url + '" target="_blank">' +
+                Y.Escape.html('D' + o.data.id + ' - ' + o.data.title) +
+                '</a></td></tr>',
+                'before');
+
+            o.cell.set('text', o.data.status == 'added' ? 'pending' : o.data.status);
+
+            return false;
+        };
+
+        // Reviews
+        if (hasReviews) {
+            dataSource.reviews = new Y.DataSource.IO({ source: 'jsonrpc.cgi' });
+            dataSource.reviews.on('error', function(e) {
+                console.log(e);
+                try {
+                    var response = Y.JSON.parse(e.data.responseText);
+                    if (response.error)
+                        e.error.message = response.error.message;
+                } catch(ex) {
+                    // ignore
+                }
+            });
+            dataTable.reviews = new Y.DataTable({
+                columns: [
+                    { key: 'author_email', label: 'Requester', sortable: true,
+                        formattter: phabAuthorFormatter, allowHTML: true },
+                    { key: 'id', label: 'Status', sortable: true,
+                        nodeFormatter: phabRowFormatter, allowHTML: true },
+                    { key: 'bug_id', label: 'Bug', sortable: true,
+                        formatter: bugLinkFormatter, allowHTML: true },
+                    { key: 'updated', label: 'Updated', sortable: true,
+                        formatter: updatedFormatter, allowHTML: true }
+                ],
+                strings: {
+                    emptyMessage: 'No review requests.',
+                }
+            });
+
+            dataTable.reviews.plug(Y.Plugin.DataTableSort);
+
+            dataTable.reviews.plug(Y.Plugin.DataTableDataSource, {
+                datasource: dataSource.reviews
+            });
+
+            dataSource.reviews.plug(Y.Plugin.DataSourceJSONSchema, {
+                schema: {
+                    resultListLocator: 'result.result',
+                    resultFields: [ 'author_email', 'author_name', 'bug_id',
+                        'bug_status', 'bug_summary', 'id', 'status', 'title',
+                        'updated', 'updated_fancy', 'url' ]
+                }
+            });
+
+            dataTable.reviews.render("#reviews_table");
+
+            Y.one('#reviews_refresh').on('click', function(e) {
+                updateRequestsTable('reviews');
+            });
+            Y.one('#reviews_buglist').on('click', function(e) {
+                loadBugList('reviews');
+            });
+        }
+
         // Requestee
         dataSource.requestee = new Y.DataSource.IO({ source: 'jsonrpc.cgi' });
         dataSource.requestee.on('error', function(e) {
@@ -146,7 +239,7 @@ $(function () {
                 formatter: updatedFormatter, allowHTML: true }
             ],
             strings: {
-                emptyMessage: 'No flag data found.',
+                emptyMessage: 'No flags requested of you.',
             }
         });
 
@@ -167,7 +260,7 @@ $(function () {
         dataTable.requestee.render("#requestee_table");
 
         Y.one('#requestee_refresh').on('click', function(e) {
-            updateFlagTable('requestee');
+            updateRequestsTable('requestee');
         });
         Y.one('#requestee_buglist').on('click', function(e) {
             loadBugList('requestee');
@@ -196,7 +289,7 @@ $(function () {
                 formatter: updatedFormatter, allowHTML: true }
             ],
             strings: {
-                emptyMessage: 'No flag data found.',
+                emptyMessage: 'No requested flags found.',
             }
         });
 
@@ -214,19 +307,24 @@ $(function () {
             }
         });
 
-        // Initial load
-        Y.on("contentready", function (e) {
-            updateFlagTable("requestee");
-        }, "#requestee_table");
-        Y.on("contentready", function (e) {
-            updateFlagTable("requester");
-        }, "#requester_table");
-
         Y.one('#requester_refresh').on('click', function(e) {
-            updateFlagTable('requester');
+            updateRequestsTable('requester');
         });
         Y.one('#requester_buglist').on('click', function(e) {
             loadBugList('requester');
         });
+
+        // Initial load
+        if (hasReviews) {
+            Y.on("contentready", function (e) {
+                updateRequestsTable('reviews');
+            }, "#reviews_table");
+        }
+        Y.on("contentready", function (e) {
+            updateRequestsTable("requestee");
+        }, "#requestee_table");
+        Y.on("contentready", function (e) {
+            updateRequestsTable("requester");
+        }, "#requester_table");
     });
 });
diff --git a/extensions/MyDashboard/web/js/query.js b/extensions/MyDashboard/web/js/query.js
index a95c0be61..e5e0979a1 100644
--- a/extensions/MyDashboard/web/js/query.js
+++ b/extensions/MyDashboard/web/js/query.js
@@ -77,6 +77,7 @@ $(function() {
         var bugQueryCallback = {
             success: function(e) {
                 if (e.response) {
+                    Y.one('#query_loading').addClass('bz_default_hidden');
                     Y.one('#query_count_refresh').removeClass('bz_default_hidden');
                     Y.one("#query_container .query_description").setHTML(e.response.meta.description);
                     Y.one("#query_container .query_heading").setHTML(e.response.meta.heading);
@@ -100,6 +101,8 @@ $(function() {
                 }
             },
             failure: function(o) {
+                Y.one('#query_loading').addClass('bz_default_hidden');
+                Y.one('#query_count_refresh').removeClass('bz_default_hidden');
                 if (o.error) {
                     alert("Failed to load bug list from Bugzilla:\n\n" + o.error.message);
                 } else {
@@ -114,6 +117,7 @@ $(function() {
             counter = counter + 1;
             lastChangesCache = {};
 
+            Y.one('#query_loading').removeClass('bz_default_hidden');
             Y.one('#query_count_refresh').addClass('bz_default_hidden');
             bugQueryTable.set('data', []);
             bugQueryTable.render("#query_table");
@@ -173,6 +177,9 @@ $(function() {
                 { key: "bug_status", label: "Status", sortable: true },
                 { key: "short_desc", label: "Summary", sortable: true },
             ],
+            strings: {
+                emptyMessage: 'Zarro Boogs found'
+            }
         });
 
         var last_changes_source   = Y.one('#last-changes-template').getHTML(),
diff --git a/extensions/MyDashboard/web/styles/mydashboard.css b/extensions/MyDashboard/web/styles/mydashboard.css
index 1011a9143..ef34bb100 100644
--- a/extensions/MyDashboard/web/styles/mydashboard.css
+++ b/extensions/MyDashboard/web/styles/mydashboard.css
@@ -18,10 +18,13 @@
     white-space: nowrap;
 }
 
+#mydashboard .requests {
+    margin-bottom: 2em;
+}
+
 .query_heading {
     font-size: 18px;
     font-weight: 600;
-    margin: 5px 0;
     color: rgb(72, 72, 72);
 }
 
diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm
index a640f52a1..0f9351285 100644
--- a/extensions/PhabBugz/lib/Util.pm
+++ b/extensions/PhabBugz/lib/Util.pm
@@ -21,20 +21,22 @@ use Bugzilla::Extension::PhabBugz::Constants;
 use JSON::XS qw(encode_json decode_json);
 use List::Util qw(first);
 use LWP::UserAgent;
+use Taint::Util qw(untaint);
 
 use base qw(Exporter);
 
-our @EXPORT = qw(
+our @EXPORT_OK = qw(
     add_comment_to_revision
     add_security_sync_comments
-    create_revision_attachment
     create_private_revision_policy
     create_project
+    create_revision_attachment
     edit_revision_policy
     get_attachment_revisions
     get_bug_role_phids
     get_members_by_bmo_id
     get_members_by_phid
+    get_needs_review
     get_phab_bmo_ids
     get_project_phid
     get_revisions_by_ids
@@ -490,7 +492,12 @@ sub request {
       if $response->is_error;
 
     my $result;
-    my $result_ok = eval { $result = decode_json( $response->content); 1 };
+    my $result_ok = eval {
+        my $content = $response->content;
+        untaint($content);
+        $result = decode_json( $content );
+        1;
+    };
     if (!$result_ok || $result->{error_code}) {
         ThrowCodeError('phabricator_api_error',
             { reason => 'JSON decode failure' }) if !$result_ok;
@@ -548,4 +555,41 @@ sub add_security_sync_comments {
     Bugzilla->set_user($old_user);
 }
 
+sub get_needs_review {
+    my ($user) = @_;
+    $user //= Bugzilla->user;
+    return unless $user->id;
+
+    my $ids = get_members_by_bmo_id([$user]);
+    return [] unless @$ids;
+    my $phid_user = $ids->[0];
+
+    my $diffs = request(
+        'differential.revision.search',
+        {
+            attachments => {
+                reviewers => 1,
+            },
+            constraints => {
+                reviewerPHIDs => [$phid_user],
+                statuses      => [qw( needs-review )],
+            },
+            order       => 'newest',
+        }
+    );
+    ThrowCodeError('phabricator_api_error', { reason => 'Malformed Response' })
+        unless exists $diffs->{result}{data};
+
+    # extract this reviewer's status from 'attachments'
+    my @result;
+    foreach my $diff (@{ $diffs->{result}{data} }) {
+        my $attachments = delete $diff->{attachments};
+        my $reviewers   = $attachments->{reviewers}{reviewers};
+        my $review      = first { $_->{reviewerPHID} eq $phid_user } @$reviewers;
+        $diff->{fields}{review_status} = $review->{status};
+        push @result, $diff;
+    }
+    return \@result;
+}
+
 1;
diff --git a/extensions/PhabBugz/lib/WebService.pm b/extensions/PhabBugz/lib/WebService.pm
index 5b6310de6..adf127a1f 100644
--- a/extensions/PhabBugz/lib/WebService.pm
+++ b/extensions/PhabBugz/lib/WebService.pm
@@ -20,34 +20,42 @@ use Bugzilla::Constants;
 use Bugzilla::Error;
 use Bugzilla::Extension::Push::Util qw(is_public);
 use Bugzilla::User;
-use Bugzilla::Util qw(detaint_natural);
+use Bugzilla::Util qw(detaint_natural datetime_from time_ago);
 use Bugzilla::WebService::Constants;
 
 use Bugzilla::Extension::PhabBugz::Constants;
 use Bugzilla::Extension::PhabBugz::Util qw(
     add_security_sync_comments
-    create_revision_attachment
     create_private_revision_policy
+    create_revision_attachment
     edit_revision_policy
     get_bug_role_phids
+    get_phab_bmo_ids
+    get_needs_review
     get_project_phid
     get_revisions_by_ids
+    get_security_sync_groups
     intersect
     is_attachment_phab_revision
     make_revision_public
     request
-    get_security_sync_groups
 );
 
-use List::Util qw(first);
+use DateTime ();
+use List::Util qw(first uniq);
 use List::MoreUtils qw(any);
 use MIME::Base64 qw(decode_base64);
 
+use constant READ_ONLY => qw(
+    needs_review
+);
+
 use constant PUBLIC_METHODS => qw(
     check_user_permission_for_bug
     obsolete_attachments
     revision
     update_reviewer_statuses
+    needs_review
 );
 
 sub revision {
@@ -291,6 +299,96 @@ sub obsolete_attachments {
     return { result => \@updated_attach_ids };
 }
 
+sub needs_review {
+    my ($self, $params) = @_;
+    ThrowUserError('phabricator_not_enabled')
+        unless Bugzilla->params->{phabricator_enabled};
+    my $user = Bugzilla->login(LOGIN_REQUIRED);
+    my $dbh  = Bugzilla->dbh;
+
+    my $reviews = get_needs_review();
+
+    # map author phids to bugzilla users
+    my $author_id_map = get_phab_bmo_ids({
+        phids => [
+            uniq
+            grep { defined }
+            map { $_->{fields}{authorPHID} }
+            @$reviews
+        ]
+    });
+    my %author_phab_to_id = map { $_->{phid} => $_->{id} } @$author_id_map;
+    my $author_users      = Bugzilla::User->new_from_list([ map { $_->{id} } @$author_id_map ]);
+    my %author_id_to_user = map { $_->id => $_ } @$author_users;
+
+    # bug data
+    my $visible_bugs = $user->visible_bugs([
+        uniq
+        grep { $_ }
+        map { $_->{fields}{'bugzilla.bug-id'} }
+        @$reviews
+    ]);
+
+    # get all bug statuses and summaries in a single query to avoid creation of
+    # many bug objects
+    my %bugs;
+    if (@$visible_bugs) {
+        #<<<
+        my $bug_rows =$dbh->selectall_arrayref(
+            'SELECT bug_id, bug_status, short_desc ' .
+            '  FROM bugs ' .
+            ' WHERE bug_id IN (' . join(',', ('?') x @$visible_bugs) . ')',
+            { Slice => {} },
+            @$visible_bugs
+        );
+        #>>>
+        %bugs = map { $_->{bug_id} => $_ } @$bug_rows;
+    }
+
+    # build result
+    my $datetime_now = DateTime->now(time_zone => $user->timezone);
+    my @result;
+    foreach my $review (@$reviews) {
+        my $review_flat = {
+            id     => $review->{id},
+            status => $review->{fields}{review_status},
+            title  => $review->{fields}{title},
+            url    => Bugzilla->params->{phabricator_base_uri} . 'D' . $review->{id},
+        };
+
+        # show date in user's timezone
+        my $datetime = DateTime->from_epoch(
+            epoch     => $review->{fields}{dateModified},
+            time_zone => 'UTC'
+        );
+        $datetime->set_time_zone($user->timezone);
+        $review_flat->{updated}       = $datetime->strftime('%Y-%m-%d %T %Z');
+        $review_flat->{updated_fancy} = time_ago($datetime, $datetime_now);
+
+        # review requester
+        if (my $author = $author_id_to_user{$author_phab_to_id{ $review->{fields}{authorPHID} }}) {
+            $review_flat->{author_name}  = $author->name;
+            $review_flat->{author_email} = $author->email;
+        }
+        else {
+            $review_flat->{author_name}  = 'anonymous';
+            $review_flat->{author_email} = 'anonymous';
+        }
+
+        # referenced bug
+        if (my $bug_id = $review->{fields}{'bugzilla.bug-id'}) {
+            my $bug = $bugs{$bug_id};
+            $review_flat->{bug_id}      = $bug_id;
+            $review_flat->{bug_status}  = $bug->{bug_status};
+            $review_flat->{bug_summary} = $bug->{short_desc};
+        }
+
+        push @result, $review_flat;
+    }
+
+    return { result => \@result };
+}
+
 sub _phabricator_precheck {
     my ($user) = @_;
 
@@ -334,7 +432,13 @@ sub rest_resources {
             PUT => {
                 method => 'obsolete_attachments',
             }
-        }
+        },
+        # Review requests
+        qw{^/phabbugz/needs_review$}, {
+            GET => {
+                method => 'needs_review',
+            },
+        },
     ];
 }
 
diff --git a/t/008filter.t b/t/008filter.t
index cae1e6880..443fb2b4f 100644
--- a/t/008filter.t
+++ b/t/008filter.t
@@ -147,6 +147,9 @@ sub directive_ok {
     $directive =~ s/^\s*//;
     $directive =~ s/\s*$//;
 
+    # Ignore blocks explicitly marked as ok
+    return 1 if $directive =~ /\b## no-008filter\b/;
+
     # Empty directives are ok; they are usually line break helpers
     return 1 if $directive eq '';