Bug 1429344 - Review requests in requests dropdown should link to MozReview or GitHub instead of Bugzilla details page

4 files changed, 47 insertions, 18 deletions

diff --git a/extensions/Review/web/js/badge.js b/extensions/Review/web/js/badge.js
index d104ce5ae..4ae135407 100644
--- a/extensions/Review/web/js/badge.js
+++ b/extensions/Review/web/js/badge.js
@@ -70,8 +70,8 @@ Bugzilla.Review.Badge = class Badge {
 
         // Sort requests from new to old, then group reviews/feedbacks asked by the same person in the same bug
         _requests.reverse().forEach(_req => {
-            const dup_index = requests.findIndex(req => req.requester === _req.requester
-                && req.bug_id === _req.bug_id && req.type === _req.type && req.attach_id && _req.attach_id);
+            const dup_index = requests.findIndex(req => req.requester === _req.requester && req.type === _req.type
+                && req.bug_id === _req.bug_id && req.attach_mimetype === _req.attach_mimetype);
 
             if (dup_index > -1) {
                 requests[dup_index].dup_count++;
@@ -86,17 +86,14 @@ Bugzilla.Review.Badge = class Badge {
             const $li = document.createElement('li');
             const [, name, email] = req.requester.match(/^(?:(.*)\s<)?(.+?)>?$/);
             const pretty_name = name ? name.replace(/([\[\(<‹].*?[›>\)\]]|\:[\w\-]+|\s+\-\s+.*)/g, '').trim() : email;
-            const link = req.attach_id && req.dup_count === 1
-                ? `attachment.cgi?id=${req.attach_id}&amp;action=edit` : `show_bug.cgi?id=${req.bug_id}`;
+            const [link, attach_label] = this.get_link(req);
 
             $li.setAttribute('role', 'none');
-            $li.innerHTML = `<a href="${link}" role="menuitem" tabindex="-1" `
+            $li.innerHTML = `<a href="${link.htmlEncode()}" role="menuitem" tabindex="-1" `
                 + `class="${(req.restricted ? 'secure' : '')}" data-type="${req.type}">`
                 + `<img src="https://secure.gravatar.com/avatar/${md5(email.toLowerCase())}?d=mm&amp;size=64" alt="">`
                 + `<label><strong>${pretty_name.htmlEncode()}</strong> asked for your `
-                + (req.type === 'needinfo' ? 'info' : req.type) + (req.attach_id ? ' on ' : '')
-                + (req.attach_id && req.ispatch ? (req.dup_count > 1 ? `${req.dup_count} patches` : 'a patch') : '')
-                + (req.attach_id && !req.ispatch ? (req.dup_count > 1 ? `${req.dup_count} files` : 'a file') : '')
+                + (req.type === 'needinfo' ? 'info' : req.type) + (attach_label ? ` on ${attach_label}` : '')
                 + ' in ' + (req.restricted ? '<span class="icon" aria-label="secure"></span>&nbsp;' : '')
                 + `<strong>Bug ${req.bug_id} &ndash; ${req.bug_summary.htmlEncode()}</strong>.</label>`
                 + `<time datetime="${req.created}">${timeAgo(new Date(req.created))}</time></a>`;
@@ -107,6 +104,36 @@ Bugzilla.Review.Badge = class Badge {
         $ul.appendChild($fragment);
         $ul.hidden = false;
     }
+
+    /**
+     * Get the link to a request as well as the label of any attachment. It could be the direct link to the attachment
+     * unless multiple requests are grouped.
+     * @param {Object} req - A request object.
+     * @returns {Array<String>} The result including the link and attachment label.
+     */
+    get_link(req) {
+        const dup = req.dup_count > 1;
+        const splinter_base = BUGZILLA.param.splinter_base;
+        const x_types = ['github-pull-request', 'review-board-request', 'phabricator-request', 'google-doc'];
+        const is_patch = req.attach_ispatch;
+        const [is_ghpr, is_rbr, is_phr, is_gdoc] = x_types.map(type => req.attach_mimetype === `text/x-${type}`);
+        const is_redirect = is_ghpr || is_rbr || is_phr || is_gdoc;
+        const is_file = req.attach_id && !is_patch && !is_redirect;
+
+        const link = (is_patch && !dup && splinter_base)
+            ? `${splinter_base}&bug=${req.bug_id}&attachment=${req.attach_id}`
+            : (is_redirect && !dup) ? `attachment.cgi?id=${req.attach_id}` // external redirect
+                : ((is_patch || is_file) && !dup) ? `attachment.cgi?id=${req.attach_id}&action=edit`
+                    : `show_bug.cgi?id=${req.bug_id}`;
+
+        const attach_label = (is_patch || is_rbr || is_phr) ? (dup ? `${req.dup_count} patches` : 'a patch')
+            : is_ghpr ? (dup ? `${req.dup_count} pull requests` : 'a pull request')
+                : is_gdoc ? (dup ? `${req.dup_count} Google Docs` : 'a Google Doc')
+                    : is_file ? (dup ? `${req.dup_count} files` : 'a file')
+                        : undefined;
+
+        return [link, attach_label];
+    }
 }
 
 window.addEventListener('DOMContentLoaded', () => new Bugzilla.Review.Badge(), { once: true });
diff --git a/request.cgi b/request.cgi
index 0d1dd138b..9f5d249cc 100755
--- a/request.cgi
+++ b/request.cgi
@@ -102,6 +102,7 @@ sub queue {
                 requesters.realname, requesters.login_name,
                 requestees.realname, requestees.login_name, COUNT(privs.group_id),
     " . $dbh->sql_date_format('flags.modification_date', '%Y.%m.%d %H:%i') . ",
+                attachments.mimetype,
                 attachments.ispatch,
                 bugs.bug_status,
                 bugs.priority,
@@ -248,7 +249,7 @@ sub queue {
                 requesters.login_name, requestees.realname,
                 requestees.login_name, flags.modification_date, attachments.ispatch
                 cclist_accessible, bugs.reporter, bugs.reporter_accessible,
-                bugs.assigned_to, attachments.ispatch');
+                bugs.assigned_to, attachments.mimetype');
 
     # Group the records, in other words order them by the group column
     # so the loop in the display template can break them up into separate
@@ -291,10 +292,11 @@ sub queue {
           'requestee'       => ($data->[11] ? "$data->[11] <$data->[12]>" : $data->[12]) ,
           'restricted'      => $data->[13] ? 1 : 0,
           'created'         => $data->[14],
-          'ispatch'         => $data->[15],
-          'bug_status'      => $data->[16],
-          'priority'        => $data->[17],
-          'bug_severity'    => $data->[18],
+          'attach_mimetype' => $data->[15],
+          'attach_ispatch'  => $data->[16],
+          'bug_status'      => $data->[17],
+          'priority'        => $data->[18],
+          'bug_severity'    => $data->[19],
         };
         push(@requests, $request);
     }
diff --git a/template/en/default/global/header.html.tmpl b/template/en/default/global/header.html.tmpl
index 428354233..ded28d186 100644
--- a/template/en/default/global/header.html.tmpl
+++ b/template/en/default/global/header.html.tmpl
@@ -107,6 +107,7 @@
     [%- js_BUGZILLA = {
             param => {
                 maxusermatches => Param('maxusermatches'),
+                splinter_base => Param('splinter_base'),
             },
             constant => {
                 COMMENT_COLS => constants.COMMENT_COLS,
diff --git a/template/en/default/request/queue.json.tmpl b/template/en/default/request/queue.json.tmpl
index 7f86072b4..50638de9e 100644
--- a/template/en/default/request/queue.json.tmpl
+++ b/template/en/default/request/queue.json.tmpl
@@ -6,10 +6,9 @@
   # defined by the Mozilla Public License, v. 2.0. #%]
 
 [% RAWPERL %]
-my @display_columns = (
-    "requester", "requestee",      "type",    "status",  "bug_id", "bug_summary",
-    "attach_id", "attach_summary", "ispatch", "created", "category", "restricted"
-);
+my @display_columns = ('requester', 'requestee', 'type', 'created', 'category',
+                       'restricted', 'bug_id', 'bug_summary', 'attach_id',
+                       'attach_summary', 'attach_mimetype', 'attach_ispatch');
 my $requests    = $stash->get('requests');
 my $time_filter = $context->filter('time', [ '%Y-%m-%dT%H:%M:%SZ', 'UTC' ]);
 my $mail_filter = $context->filter('email');
@@ -28,7 +27,7 @@ foreach my $request (@$requests) {
         elsif ( $column =~ /_id$/ ) {
             $val = $request->{$column} ? 0 + $request->{$column} : undef;
         }
-        elsif ( $column =~ /^is/ or $column eq 'restricted' ) {
+        elsif ( $column =~ /^(restricted|attach_ispatch)$/ ) {
             $val = $request->{$column} ? \1 : \0;
         }
         else {