Bug 2043229 - Display a "Show External" button beneath attachments list to display all attachments that would redirect such as Github and Phabricator

Bugzilla/Config/Attachment.pm

@@ -54,6 +54,11 @@ sub get_param_list {
     },
     {name => 'github_pr_linking_enabled',  type => 'b', default => 0},
     {name => 'github_pr_signature_secret', type => 't', default => ''},
+    {
+      name    => 'attachment_hide_content_types',
+      type    => 't',
+      default => 'text/x-phabricator-request text/x-github-pull-request',
+    },
   );
   return @param_list;
 }

extensions/BugModal/template/en/default/bug_modal/attachments.html.tmpl

@@ -8,8 +8,11 @@
 
 [%#
   # bug: (bug object) the main bug object
-  # active_attachments: array of active attachment objects
-  # obsolete_attachments: array of obsolete attachment objects
+  # active_attachments: count of active attachment objects
+  # obsolete_attachments: count of obsolete attachment objects
+  # external_attachments: count of non-obsolete external attachments
+  # external_attachments_total: count of all external attachments (incl. obsolete)
+  # attachment_hide_types: hash of content types hidden behind "Show External"
   #%]
 
 [% IF active_attachments || obsolete_attachments %]
@@ -23,12 +26,14 @@
       attachment_rendered = 0;
       Hook.process("row");
       NEXT IF attachment_rendered;
+      is_external = attachment_hide_types.${attachment.contenttype} ? 1 : 0;
     %]
     <tr data-attachment-id="[% attachment.id FILTER none %]" class="
         [%~ " bz_private" IF attachment.isprivate %]
         [%~ " attach-obsolete" IF attachment.isobsolete %]
+        [%~ " attach-external" IF is_external %]
         [%~ " attach-patch" IF attachment.can_review %]
-    " [% IF attachment.isobsolete %]style="display:none"[% END %]>
+    " [% IF attachment.isobsolete || is_external %]style="display:none"[% END %]>
       <td class="attach-desc-td">
         <div class="attach-desc">
           <a href="[% basepath FILTER none %]attachment.cgi?id=[% attachment.id FILTER none %]"
@@ -103,6 +108,9 @@
         <button type="button" role="none" id="attachments-add-btn" class="secondary">Attach New File</button>
       </a>
     [% END %]
+    [% IF external_attachments_total %]
+      <button type="button" id="attachments-external-btn" class="secondary">Show External</button>
+    [% END %]
     [% IF obsolete_attachments %]
       <button type="button" id="attachments-obsolete-btn" class="secondary">Show Obsolete</button>
     [% END %]

extensions/BugModal/template/en/default/bug_modal/edit.html.tmpl

@@ -51,12 +51,32 @@
   # count attachments
   active_attachments = 0;
   obsolete_attachments = 0;
+  external_attachments = 0;
+  # external_attachments_total also counts obsolete externals so the "Show
+  # External" button is still rendered (and the rows remain reachable) when
+  # every external attachment happens to be obsolete.
+  external_attachments_total = 0;
+
+  attachment_hide_types = {};
+  IF Param('attachment_hide_content_types');
+    FOREACH _type IN Param('attachment_hide_content_types').split('[,\s]+');
+      attachment_hide_types.$_type = 1 IF _type;
+    END;
+  END;
+
   FOREACH attachment IN bug.attachments;
     NEXT IF attachment.isprivate && !(user.is_insider || attachment.attacher.id == user.id || (attachment.is_bounty_attachment && bug.reporter.id == user.id)) ;
+    is_external = attachment_hide_types.${attachment.contenttype} ? 1 : 0;
+    IF is_external;
+      external_attachments_total = external_attachments_total + 1;
+    END;
     IF attachment.isobsolete;
       obsolete_attachments = obsolete_attachments + 1;
     ELSE;
       active_attachments = active_attachments + 1;
+      IF is_external;
+        external_attachments = external_attachments + 1;
+      END;
     END;
   END;
 
@@ -1391,11 +1411,15 @@
 [% IF active_attachments || obsolete_attachments || user.id %]
   [%
     sub = [];
+    visible_attachments = active_attachments - external_attachments;
     IF active_attachments + obsolete_attachments == 0;
       sub.push("No files");
     END;
-    IF active_attachments;
-      sub.push(active_attachments _ " file" _ (active_attachments == 1 ? "" : "s"));
+    IF visible_attachments;
+      sub.push(visible_attachments _ " file" _ (visible_attachments == 1 ? "" : "s"));
+    END;
+    IF external_attachments;
+      sub.push(external_attachments _ " external file" _ (external_attachments == 1 ? "" : "s"));
     END;
     IF obsolete_attachments;
       sub.push(obsolete_attachments _ " obsolete file" _ (obsolete_attachments == 1 ? "" : "s"));
@@ -1408,6 +1432,9 @@
     [% INCLUDE bug_modal/attachments.html.tmpl
         active_attachments = active_attachments
         obsolete_attachments = obsolete_attachments
+        external_attachments = external_attachments
+        external_attachments_total = external_attachments_total
+        attachment_hide_types = attachment_hide_types
     %]
   [% END %]
 [% END %]

extensions/BugModal/web/bug_modal.js

@@ -272,13 +272,36 @@ $(function() {
             }
         });
 
-    // toggle obsolete attachments
-    $('#attachments-obsolete-btn')
-        .click(function(event) {
-            event.preventDefault();
-            $(event.target).text(($('#attachments tr:hidden').length ? 'Hide Obsolete' : 'Show Obsolete'));
-            $('#attachments tr.attach-obsolete').toggle();
+    // toggle obsolete/external attachments
+    let attachObsoleteShowing = false;
+    let attachExternalShowing = false;
+
+    function updateAttachmentRows() {
+        // A row may be obsolete, external, or both. It is only visible when
+        // every category it belongs to has been toggled on, so external rows
+        // stay hidden while "Show External" is off even if they are obsolete.
+        $('#attachments tr.attach-obsolete, #attachments tr.attach-external').each(function() {
+            const $row = $(this);
+            const visible =
+                (!$row.hasClass('attach-obsolete') || attachObsoleteShowing) &&
+                (!$row.hasClass('attach-external') || attachExternalShowing);
+            $row.toggle(visible);
         });
+        $('#attachments-obsolete-btn').text(attachObsoleteShowing ? 'Hide Obsolete' : 'Show Obsolete');
+        $('#attachments-external-btn').text(attachExternalShowing ? 'Hide External' : 'Show External');
+    }
+
+    $('#attachments-obsolete-btn').click(function(event) {
+        event.preventDefault();
+        attachObsoleteShowing = !attachObsoleteShowing;
+        updateAttachmentRows();
+    });
+
+    $('#attachments-external-btn').click(function(event) {
+        event.preventDefault();
+        attachExternalShowing = !attachExternalShowing;
+        updateAttachmentRows();
+    });
 
     // URL --> unsafe warning
     $('.bug-url')

qa/t/3_test_external_attachments.t

@@ -0,0 +1,187 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+#
+# This Source Code Form is "Incompatible With Secondary Licenses", as
+# defined by the Mozilla Public License, v. 2.0.
+
+use strict;
+use warnings;
+use lib qw(lib ../../lib ../../local/lib/perl5);
+
+use Test::More 'no_plan';
+
+use QA::Util;
+
+# This test exercises the "Show External" button in the bug modal attachments
+# list. External attachments are those whose content type is listed in the
+# 'attachment_hide_content_types' parameter; they are hidden by default and
+# revealed by the "Show External" button. We use GitHub Pull Request URL
+# attachments (content type text/x-github-pull-request) as the external type
+# because normal users are allowed to create them (unlike Phabricator links).
+
+my ($sel, $config) = get_selenium();
+
+# Return the inline "display" style of an attachment row in the bug modal. The
+# updateAttachmentRows() handler in bug_modal.js toggles this directly, so it is
+# "none" when the row is hidden and "" (empty) when the row is visible.
+sub row_display {
+  my ($id) = @_;
+  return $sel->driver->execute_script(
+    qq{return document.querySelector('#attachments tr[data-attachment-id="$id"]').style.display;}
+  );
+}
+
+# Look up the id of an attachment from its description link in the modal
+# attachments table (works even while the row is hidden).
+sub attachment_id_for {
+  my ($description) = @_;
+  my $href = $sel->get_attribute(
+    qq{//table[\@id="attachments"]//a[normalize-space(text())="$description"]\@href});
+  my ($id) = $href =~ /id=(\d+)/;
+  return $id;
+}
+
+# Add a text/URL attachment to the given bug via the create-attachment page.
+# When $obsoletes is set, the existing attachment with that id is marked
+# obsolete by the new attachment.
+sub add_text_attachment {
+  my ($bug_id, $content, $description, $obsoletes) = @_;
+  $sel->click_ok('attachments-add-link');
+  $sel->wait_for_page_to_load_ok(WAIT_TIME);
+  $sel->title_is("Create New Attachment for Bug #$bug_id");
+  $sel->type_ok('att-textarea', $content, 'Enter attachment content');
+  $sel->type_ok('att-description', $description, 'Enter attachment description');
+  if ($obsoletes) {
+    $sel->click_ok(qq{//input[\@name="obsolete" and \@value="$obsoletes"]},
+      undef, "Mark attachment $obsoletes obsolete");
+  }
+  $sel->click_ok('create');
+  $sel->wait_for_page_to_load_ok(WAIT_TIME);
+  $sel->is_text_present_ok('regexp:Attachment #\d+ to bug \d+ created');
+}
+
+my $gh_pr_url_1 = 'https://github.com/mozilla-bteam/bmo/pull/1';
+my $gh_pr_url_2 = 'https://github.com/mozilla-bteam/bmo/pull/2';
+
+log_in($sel, $config, 'admin');
+
+# Make sure GitHub Pull Request attachments are treated as external. This is
+# the default, but set it explicitly so the test is self-contained.
+set_parameters(
+  $sel,
+  {
+    'Attachments' => {
+      'attachment_hide_content_types' =>
+        {type => 'text', value => 'text/x-github-pull-request'},
+    }
+  }
+);
+
+################################################################################
+# Scenario 1: a bug with a normal attachment and a (non-obsolete) external
+# attachment. The "Show External" button must appear, the external row must be
+# hidden by default, and the button must toggle its visibility.
+################################################################################
+
+file_bug_in_product($sel, 'TestProduct');
+$sel->type_ok('short_desc', 'External attachments toggle test');
+$sel->click_ok('commit');
+$sel->wait_for_page_to_load_ok(WAIT_TIME);
+$sel->is_text_present_ok('has been added to the database', 'Bug created');
+my $bug1_id = $sel->get_value('//input[@name="id" and @type="hidden"]');
+
+go_to_bug($sel, $bug1_id);
+add_text_attachment($bug1_id, 'This is a plain text attachment.',
+  'plain text attachment s1');
+go_to_bug($sel, $bug1_id);
+add_text_attachment($bug1_id, $gh_pr_url_1, 'github pr external s1');
+
+go_to_bug($sel, $bug1_id);
+my $normal1_id = attachment_id_for('plain text attachment s1');
+my $external1_id = attachment_id_for('github pr external s1');
+ok($normal1_id,   "Found normal attachment id: $normal1_id");
+ok($external1_id, "Found external attachment id: $external1_id");
+
+# The "Show External" button should be rendered, "Show Obsolete" should not
+# (there are no obsolete attachments on this bug).
+$sel->is_element_present_ok('//button[@id="attachments-external-btn"]',
+  undef, 'Show External button is present');
+ok(
+  !$sel->is_element_present('//button[@id="attachments-obsolete-btn"]'),
+  'Show Obsolete button is not present'
+);
+
+# By default the normal row is visible and the external row is hidden.
+is(row_display($normal1_id), '', 'Normal attachment row is visible by default');
+is(row_display($external1_id), 'none',
+  'External attachment row is hidden by default');
+
+# Clicking "Show External" reveals the external row and updates the label.
+$sel->click_ok('attachments-external-btn');
+sleep(1);
+is(row_display($external1_id), '',
+  'External attachment row is visible after Show External');
+is($sel->get_text('//button[@id="attachments-external-btn"]'),
+  'Hide External', 'Button label switched to Hide External');
+
+# Clicking again hides it once more.
+$sel->click_ok('attachments-external-btn');
+sleep(1);
+is(row_display($external1_id), 'none',
+  'External attachment row is hidden again after Hide External');
+is($sel->get_text('//button[@id="attachments-external-btn"]'),
+  'Show External', 'Button label switched back to Show External');
+
+################################################################################
+# Scenario 2 (regression, bug 2043229): a bug whose only external attachment is
+# also obsolete. The "Show External" button must still be rendered so the row is
+# reachable, and revealing it requires BOTH "Show External" and "Show Obsolete"
+# to be toggled on.
+################################################################################
+
+file_bug_in_product($sel, 'TestProduct');
+$sel->type_ok('short_desc', 'Obsolete external attachment test');
+$sel->click_ok('commit');
+$sel->wait_for_page_to_load_ok(WAIT_TIME);
+$sel->is_text_present_ok('has been added to the database', 'Bug created');
+my $bug2_id = $sel->get_value('//input[@name="id" and @type="hidden"]');
+
+# Add the external attachment first.
+go_to_bug($sel, $bug2_id);
+add_text_attachment($bug2_id, $gh_pr_url_2, 'github pr obsolete s2');
+go_to_bug($sel, $bug2_id);
+my $external2_id = attachment_id_for('github pr obsolete s2');
+ok($external2_id, "Found external attachment id: $external2_id");
+
+# Add a normal attachment that obsoletes the external one. Now the only external
+# attachment on the bug is obsolete.
+add_text_attachment($bug2_id, 'This is a plain text attachment.',
+  'plain text obsoleter s2', $external2_id);
+
+go_to_bug($sel, $bug2_id);
+
+# Regression check: the "Show External" button must be present even though the
+# only external attachment is obsolete (external_attachments_total drives this).
+$sel->is_element_present_ok('//button[@id="attachments-external-btn"]',
+  undef, 'Show External button is present for obsolete-only external');
+$sel->is_element_present_ok('//button[@id="attachments-obsolete-btn"]',
+  undef, 'Show Obsolete button is present');
+
+# The obsolete+external row is hidden by default.
+is(row_display($external2_id), 'none',
+  'Obsolete external row is hidden by default');
+
+# Showing external alone is not enough: the row is also obsolete.
+$sel->click_ok('attachments-external-btn');
+sleep(1);
+is(row_display($external2_id), 'none',
+  'Obsolete external row stays hidden with only Show External toggled on');
+
+# Toggling Show Obsolete as well finally reveals the row.
+$sel->click_ok('attachments-obsolete-btn');
+sleep(1);
+is(row_display($external2_id), '',
+  'Obsolete external row is visible once both toggles are on');
+
+logout($sel);

template/en/default/admin/params/attachment.html.tmpl

@@ -77,5 +77,13 @@
 
   google_storage_service_account =>
     "Google Cloud Storage Service Account",
+
+  attachment_hide_content_types =>
+    "A comma- or whitespace-separated list of MIME types that should be hidden"
+    _ " by default in the bug modal attachments list. Attachments matching these"
+    _ " types will not appear in the attachments table unless the user clicks the"
+    _ " <em>Show External</em> button. This is useful for content types such as"
+    _ " <tt>text/x-phabricator-request</tt> and <tt>text/x-github-pull-request</tt>"
+    _ " that are already displayed in a dedicated section above the attachments list.",
   }
 %]