Bug 1146761 - clicking on a date/flag/etc should scroll to the corresponding change

6 files changed, 163 insertions, 50 deletions

diff --git a/extensions/BugModal/lib/ActivityStream.pm b/extensions/BugModal/lib/ActivityStream.pm
index b25bff861..f950bea1c 100644
--- a/extensions/BugModal/lib/ActivityStream.pm
+++ b/extensions/BugModal/lib/ActivityStream.pm
@@ -32,9 +32,10 @@ use Bugzilla::Constants;
 #               cc_only => boolean
 #               changes => [
 #                   {
-#                       fieldname   => field name :)
-#                       added       => string
-#                       removed     => string
+#                       fieldname     => field name :)
+#                       added         => string
+#                       removed       => string
+#                       flagtype_name => string (optional, name of flag if fieldname is 'flagtypes.name')
 #                   }
 #                   ...
 #               ]
@@ -80,15 +81,63 @@ sub activity_stream {
     return $self->{activity_stream};
 }
 
+sub find_activity_id_for_attachment {
+    my ($self, $attachment) = @_;
+    my $attach_id = $attachment->id;
+    my $stream = $self->activity_stream;
+    foreach my $change_set (@$stream) {
+        next unless exists $change_set->{attach_id};
+        return $change_set->{id} if $change_set->{attach_id} == $attach_id;
+    }
+    return undef;
+}
+
+sub find_activity_id_for_flag {
+    my ($self, $flag) = @_;
+    my $flagtype_name = $flag->type->name;
+    my $date = $flag->modification_date;
+    my $setter_id = $flag->setter->id;
+    my $stream = $self->activity_stream;
+
+    # unfortunately bugs_activity treats all flag changes as the same field, so
+    # we don't have an object_id to match on
+
+    if (!exists $self->{activity_cache}->{flag}->{$flag->id}) {
+        foreach my $change_set (@$stream) {
+            foreach my $activity (@{ $change_set->{activity} }) {
+                # match by user, timestamp, and flag-type name
+                next unless
+                    $activity->{who}->id == $setter_id
+                    && $activity->{when} eq $date;
+                foreach my $change (@{ $activity->{changes} }) {
+                    next unless
+                        $change->{fieldname} eq 'flagtypes.name'
+                        && $change->{flagtype_name} eq $flagtype_name;
+                    $self->{activity_cache}->{flag}->{$flag->id} = $change_set->{id};
+                    return $change_set->{id};
+                }
+            }
+        }
+        # if we couldn't find the flag in bugs_activity it means it was set
+        # during bug creation
+        $self->{activity_cache}->{flag}->{$flag->id} = 'c0';
+    }
+    return $self->{activity_cache}->{flag}->{$flag->id};
+}
+
 # comments are processed first, so there's no need to merge into existing entries
 sub _add_comment_to_stream {
     my ($stream, $time, $user_id, $comment) = @_;
-    push @$stream, {
+    my $rh = {
         time     => $time,
         user_id  => $user_id,
         comment  => $comment,
         activity => [],
     };
+    if ($comment->type == CMT_ATTACHMENT_CREATED || $comment->type == CMT_ATTACHMENT_UPDATED) {
+        $rh->{attach_id} = $comment->extra_data;
+    }
+    push @$stream, $rh;
 }
 
 sub _add_activity_to_stream {
@@ -235,25 +284,26 @@ sub _add_activities_to_stream {
             }
 
             # split multiple flag changes (must be processed last)
+            # set $change->{flagtype_name} to make searching the activity
+            # stream for flag changes easier and quicker
             if ($change->{fieldname} eq 'flagtypes.name') {
                 my @added = split(/, /, $change->{added});
                 my @removed = split(/, /, $change->{removed});
-                next if scalar(@added) <= 1 && scalar(@removed) <= 1;
+                if (scalar(@added) <= 1 && scalar(@removed) <= 1) {
+                    $change->{flagtype_name} = _extract_flagtype($added[0] || $removed[0]);
+                    next;
+                }
                 # remove current change
                 splice(@{$operation->{changes}}, $i, 1);
                 # restructure into added/removed for each flag
                 my %flags;
-                foreach my $added (@added) {
-                    my ($value, $name) = $added =~ /^((.+).)$/;
-                    next unless defined $name;
-                    $flags{$name}{added} = $value;
-                    $flags{$name}{removed} |= '';
+                foreach my $flag (@added) {
+                    $flags{$flag}{added} = $flag;
+                    $flags{$flag}{removed} = '';
                 }
-                foreach my $removed (@removed) {
-                    my ($value, $name) = $removed =~ /^((.+).)$/;
-                    next unless defined $name;
-                    $flags{$name}{added} |= '';
-                    $flags{$name}{removed} = $value;
+                foreach my $flag (@removed) {
+                    $flags{$flag}{added} = '';
+                    $flags{$flag}{removed} = $flag;
                 }
                 # clone current change, modify and insert
                 foreach my $flag (sort keys %flags) {
@@ -263,6 +313,7 @@ sub _add_activities_to_stream {
                     }
                     $flag_change->{removed} = $flags{$flag}{removed};
                     $flag_change->{added} = $flags{$flag}{added};
+                    $flag_change->{flagtype_name} = _extract_flagtype($flag);
                     splice(@{$operation->{changes}}, $i, 0, $flag_change);
                 }
                 $i--;
@@ -276,6 +327,11 @@ sub _add_activities_to_stream {
     $user->visible_bugs([keys %visible_bug_ids]);
 }
 
+sub _extract_flagtype {
+    my ($value) = @_;
+    return $value =~ /^(.+)[\?\-\+]/ ? $1 : undef;
+}
+
 # display 'duplicate of this bug' as an activity entry, not a comment
 sub _add_duplicates_to_stream {
     my ($bug, $stream) = @_;
diff --git a/extensions/BugModal/template/en/default/bug_modal/attachments.html.tmpl b/extensions/BugModal/template/en/default/bug_modal/attachments.html.tmpl
index 1658a0408..ca47047fc 100644
--- a/extensions/BugModal/template/en/default/bug_modal/attachments.html.tmpl
+++ b/extensions/BugModal/template/en/default/bug_modal/attachments.html.tmpl
@@ -40,7 +40,13 @@
           [% END %]
         </div>
         <div>
-          <span class="attach-time">[% INCLUDE bug_modal/rel_time.html.tmpl ts=attachment.attached %]</span>
+          [% activity_id = bug.find_activity_id_for_attachment(attachment) %]
+          [% IF activity_id %]
+            <a href="#[% activity_id FILTER none %]" class="attach-time activity-ref">
+              [% INCLUDE bug_modal/rel_time.html.tmpl ts=attachment.attached %]</a>
+          [% ELSE %]
+            <span class="attach-time">[% INCLUDE bug_modal/rel_time.html.tmpl ts=attachment.attached %]</span>
+          [% END %]
           <span class="attach-author">[% INCLUDE bug_modal/user.html.tmpl u=attachment.attacher %]</span>
         </div>
         <div class="attach-info">
@@ -56,9 +62,18 @@
         [% FOREACH flag IN attachment.flags %]
           <div class="attach-flag">
             [% INCLUDE bug_modal/user.html.tmpl u=flag.setter simple=1 %]:
-            <span class="flag-name-status">
+            [% activity_id = bug.find_activity_id_for_flag(flag) %]
+            [% IF activity_id %]
+              <a href="#[% activity_id FILTER none %]"
+            [% ELSE %]
+              <span
+            [% END %]
+              class="flag-name-status rel-time-title[% " activity-ref" IF activity_id %]"
+              title="[% flag.creation_date FILTER time_duration FILTER html %]"
+              data-time="[% flag.creation_date FILTER epoch FILTER none %]"
+            >
               [%+ flag.type.name FILTER html %][% flag.status FILTER none %]
-            </span>
+            [% activity_id ? "</a>" : "</span>" %]
             [% IF flag.requestee %]
               [%+ INCLUDE bug_modal/user.html.tmpl u=flag.requestee simple=1 %]
             [% END %]
diff --git a/extensions/BugModal/template/en/default/bug_modal/edit.html.tmpl b/extensions/BugModal/template/en/default/bug_modal/edit.html.tmpl
index 9d7588641..7e77e1582 100644
--- a/extensions/BugModal/template/en/default/bug_modal/edit.html.tmpl
+++ b/extensions/BugModal/template/en/default/bug_modal/edit.html.tmpl
@@ -216,19 +216,32 @@
       [% END %]
       [% IF needinfo.size %]
         <div id="status-needinfo">
-          (NeedInfo from
-          [%+
-            IF needinfo.size == 1;
-              IF needinfo.0.requestee;
-                INCLUDE bug_modal/user.html.tmpl u=needinfo.0.requestee nick_only=1;
+          [%~ IF needinfo.size == 1 %]
+            [%
+              ni = needinfo.0;
+              activity_id = bug.find_activity_id_for_flag(ni);
+            %]
+            [% IF activity_id %]
+              (<a href="#[% activity_id FILTER none %]"
+            [% ELSE %]
+              (<span
+            [% END %]
+              class="flag-name-status rel-time-title[% " activity-ref" IF activity_id %]"
+              title="[% ni.creation_date FILTER time_duration FILTER html %]"
+              data-time="[% ni.creation_date FILTER epoch FILTER none %]"
+            >NeedInfo
+            [% activity_id ? "</a>" : "</span>" %]
+            from
+            [%
+              IF ni.requestee;
+                INCLUDE bug_modal/user.html.tmpl u=ni.requestee nick_only=1;
               ELSE;
                 "anyone";
               END;
-            ELSE;
-              " " _ needinfo.size _ " people";
-            END;
-          ~%]
-          )
+            %])
+          [% ELSE %]
+            (Needinfo from [% needinfo.size FILTER none %] people)
+          [% END ~%]
         </div>
       [% END %]
     [% END %]
diff --git a/extensions/BugModal/template/en/default/bug_modal/flags.html.tmpl b/extensions/BugModal/template/en/default/bug_modal/flags.html.tmpl
index d86a8c8cf..0c0362902 100644
--- a/extensions/BugModal/template/en/default/bug_modal/flags.html.tmpl
+++ b/extensions/BugModal/template/en/default/bug_modal/flags.html.tmpl
@@ -74,10 +74,18 @@
     </td>
 
     <td class="flag-name">
-      <span class="rel-time-title" title="[% f.creation_date FILTER time_duration FILTER html %]"
-        data-time="[% f.creation_date FILTER epoch FILTER none %]">
+      [% activity_id = bug.find_activity_id_for_flag(f) %]
+      [% IF activity_id %]
+        <a href="#[% activity_id FILTER none %]"
+      [% ELSE %]
+        <span
+      [% END %]
+        class="rel-time-title[% " activity-ref" IF activity_id %]"
+        title="[% f.creation_date FILTER time_duration FILTER html %]"
+        data-time="[% f.creation_date FILTER epoch FILTER none %]"
+      >
         [% f.type.name FILTER html %]
-      </span>
+      [% activity_id ? "</a>" : "</span>" %]
     </td>
 
     <td class="flag-value">
diff --git a/extensions/BugModal/web/bug_modal.css b/extensions/BugModal/web/bug_modal.css
index ff804e4db..696c5e192 100644
--- a/extensions/BugModal/web/bug_modal.css
+++ b/extensions/BugModal/web/bug_modal.css
@@ -88,6 +88,10 @@ select[multiple], .text_input, .yui-ac-input, input {
     box-shadow: 0 0 2px 2px #f88;
 }
 
+a.activity-ref {
+    color: #000;
+}
+
 /* modules */
 
 .module {
diff --git a/extensions/BugModal/web/bug_modal.js b/extensions/BugModal/web/bug_modal.js
index 89b73c3b5..a72319c72 100644
--- a/extensions/BugModal/web/bug_modal.js
+++ b/extensions/BugModal/web/bug_modal.js
@@ -319,16 +319,22 @@ $(function() {
         .click(function(event) {
             event.preventDefault();
             var id = $('.comment:last')[0].parentNode.id;
-            $.scrollTo($('#' + id));
-            window.location.hash = id;
+            $.scrollTo(id);
         });
 
+    // use scrollTo for in-page activity links
+    $('.activity-ref')
+        .click(function(event) {
+            event.preventDefault();
+            $.scrollTo($(this).attr('href').substr(1));
+        });
+
+    if (BUGZILLA.user.id === 0) return;
+
     //
     // anything after this point is only executed for logged in users
     //
 
-    if (BUGZILLA.user.id === 0) return;
-
     // dirty field tracking
     $('#changeform select').each(function() {
         var that = $(this);
@@ -1256,22 +1262,33 @@ function lb_close(event) {
             return -1;
         },
 
-        // Animated scroll to bring an element into view
-        scrollTo: function(el, complete) {
-            var offset = el.offset();
-            $('html, body')
-                .animate({
-                        scrollTop: offset.top - 20,
-                        scrollLeft: offset.left = 20
-                    },
-                    200,
-                    complete
-                );
+        // Bring an element into view, leaving space for the outline.
+        // If passed a string, it will be treated as an id - the page will scroll
+        // unanimated and the url will be added to the browser's history.
+        // If passed an element, an smooth scroll will take place and no entry
+        // will be added to the history.
+        scrollTo: function(target, complete) {
+            if (typeof target === 'string') {
+                var el = $('#' + target);
+                window.location.hash = target;
+                var $html = $('html');
+                if (Math.abs($html.scrollTop() - el.offset().top) <= 1) {
+                    $html.scrollTop($html.scrollTop() - 10);
+                }
+                $html.scrollLeft(0);
+            }
+            else {
+                var offset = target.offset();
+                $('html')
+                    .animate({
+                            scrollTop: offset.top - 20,
+                            scrollLeft: 0
+                        },
+                        200,
+                        complete
+                    );
+            }
         }
 
     });
 })(jQuery);
-
-// no-ops
-function initHidingOptionsForIE() {}
-function showFieldWhen() {}