685363 - Add a sightings table to the show bug page

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Robert Webb]

We should add a sightings table to the show bug page that will list all of the sightings attached to the master bug (if there are any) in addition to a "Create Sighting" button for the creation of new sightings.

Comment 1

[Max Kanat-Alexander]

For now, don't worry about "Create Sighting" -- do that in a separate bug.

Comment 2

[Robert Webb]

Attachment: Adding sightings tables to the show bug screen. patch v1

Here's the template changes to just show the sightings tables for bugs if they have any or if they are a sighting themselves. The master bug is currently just colored differently.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 560238 [details] [diff] [review]
Adding sightings tables to the show bug screen. patch v1

Review of attachment 560238 [details] [diff] [review]:
-----------------------------------------------------------------

::: skins/standard/global.css
@@ +410,5 @@
>      border-collapse: collapse;
>      border: 1px solid #333333;
>  }
>  
> +#attachment_table th, .bz_attach_footer, .bz_sighting_table th, .bz_sighting_footer, .bz_time_tracking_table th {

This line looks too long; length limit is 80 chars.

@@ +415,5 @@
>      background-color: #E0E0E0;
>      color: black;
>  }
>  
> +#attachment_table td, .bz_sighting_table th, .bz_sighting_table td, .bz_time_tracking_table th, .bz_time_tracking_table td {

Also looks too long.

::: template/en/default/sighting/list.html.tmpl
@@ +7,5 @@
> +  # IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
> +  # implied. See the License for the specific language governing
> +  # rights and limitations under the License.
> +  #
> +  # The Original Code is the Bugzilla Bug Tracking System.

This header is missing the Initial Developer section, which would be Riverbed.

@@ +21,5 @@
> +[% IF bug.master_bug_id %]
> +  [% master_bug = bug.master_bug %]
> +[% END %]
> +[% sightings = master_bug.sightings %]
> +[% IF sightings.size > 0 %]

If you want less indentation, what you could do here instead is:

[% IF sightings.size == 0 %]
  [% RETURN %]
[% END %]

@@ +22,5 @@
> +  [% master_bug = bug.master_bug %]
> +[% END %]
> +[% sightings = master_bug.sightings %]
> +[% IF sightings.size > 0 %]
> +  <br />

Don't use br to create space, use CSS.

@@ +23,5 @@
> +[% END %]
> +[% sightings = master_bug.sightings %]
> +[% IF sightings.size > 0 %]
> +  <br />
> +  <table id="sighting_table" class="bz_sighting_table" cellspacing="0" cellpadding="4">

Don't use cellpadding for styling--it should only be set to 0.

@@ +25,5 @@
> +[% IF sightings.size > 0 %]
> +  <br />
> +  <table id="sighting_table" class="bz_sighting_table" cellspacing="0" cellpadding="4">
> +    <tr>
> +      <th align="center">

In general, don't use any of HTML's styling attributes. So, don't use align, use CSS to control this instead if you have to. (Although <th> defaults to center alignment anyway.) This applies to every other align= as well.

@@ +26,5 @@
> +  <br />
> +  <table id="sighting_table" class="bz_sighting_table" cellspacing="0" cellpadding="4">
> +    <tr>
> +      <th align="center">
> +        Sighting

This should probably go into field_descs.

@@ +29,5 @@
> +      <th align="center">
> +        Sighting
> +      </th>
> +      <th align="center">
> +        Status

[% field_descs.bug_status FILTER html %]

@@ +35,5 @@
> +    </tr>
> +  
> +    <tr id="s0">
> +      <th align="center">
> +        <a href="show_bug.cgi?id=[% master_bug.id %]">[% master_bug.id %]</a>

If you insert things into URLs or display them anywhere in the UI, they must be filtered. The filter to use here in the URL is "FILTER uri", and "FITLER html" for the stuff that's being displayed to the user.

@@ +46,5 @@
> +    [% count = 0 %]
> +  
> +    [% FOREACH sighting = sightings %]
> +      [% count = count + 1 %]
> +      <tr id="s[% count %]">

count FITLER html.

(In general, run "prove -lv t/" before submitting patches, particularly if they involve template changes.)

@@ +48,5 @@
> +    [% FOREACH sighting = sightings %]
> +      [% count = count + 1 %]
> +      <tr id="s[% count %]">
> +        <td align="center">
> +          <a href="show_bug.cgi?id=[% sighting.bug_id %]">[% sighting.bug_id %]</a>

Also needs FILTERs.

@@ +56,5 @@
> +        </td>
> +      </tr>
> +    [% END %]
> +  </table>
> +  <br />

Also eliminate this <br>.

@@ +59,5 @@
> +  </table>
> +  <br />
> +[% END %]
> +
> +[% BLOCK bug_status %]

Template-Toolkit has no namespaces, so there's a chance you will overwrite a variable named bug_status by defining this block. Instead, you might want to call this something like display_status or something similar.

@@ +71,5 @@
> +    [% IF target.dup_id %]
> +      of [% "${terms.bug} ${target.dup_id}" FILTER bug_link(target.dup_id) FILTER none %]
> +    [% END %]
> +  [% END %]
> +[% END %]

This block in general looks like a duplicate of code from edit.html.tmpl. If you're going to use it in both files, let's just break it out into its own tiny file.

Comment 4

[Robert Webb]

Attachment: Adding sightings tables to the show bug screen. patch v2

Made the suggested changes.

Comment 5

[Robert Webb]

Attachment: Adding sightings tables to the show bug screen. patch v3

Added the master_bug subroutine to Bug.pm; the template was referring to this subroutine for when you look at sightings but the subroutine wasn't there.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 564316 [details] [diff] [review]
Adding sightings tables to the show bug screen. patch v3

Review of attachment 564316 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry it took me so long to get to this! This is really looking awesome! There's a few things to fix, though:

::: Bugzilla/Bug.pm
@@ +3360,5 @@
> +sub master_bug {
> +    my $self = shift;
> +    return $self->{'master_bug_obj'} if exists $self->{'master_bug_obj'};
> +    return undef if $self->{'error'};
> +    $self->{'master_bug_obj'} = Bugzilla::Bug->check($self->master_bug_id);

Ah, calling check in an accessor seems risky to me. You're going to be potentially throwing errors any time it's accessed. Instead you can just rely on bug_link in the templates to do the right thing, security-wise.

::: skins/standard/global.css
@@ +410,4 @@
>      border-collapse: collapse;
>      border: 1px solid #333333;
> +    margin-top: 2ex;
> +    margin-bottom: 2ex;

This is often more simply written as:

margin: 2ex 0;

@@ +418,3 @@
>      background-color: #E0E0E0;
>      color: black;
> +    padding: 4px;

Usually we try to avoid doing padding or things like that in pixels, because it doesn't scale with the font.

::: template/en/default/sighting/list.html.tmpl
@@ +42,5 @@
> +
> +  <tr id="s0">
> +    <th>
> +      <a href="show_bug.cgi?id=[% master_bug.id FILTER uri %]">
> +        [% master_bug.id FILTER html %]</a>

Use FILTER bug_link here instead.

@@ +45,5 @@
> +      <a href="show_bug.cgi?id=[% master_bug.id FILTER uri %]">
> +        [% master_bug.id FILTER html %]</a>
> +    </th>
> +    <th>
> +      [% PROCESS display_status target = master_bug %]

You're depending on being called from a template which has display_status defined. I think that may cause bugs in the future. Perhaps factor display_status out into bug/status.html.tmpl?

@@ +53,5 @@
> +  [% count = 0 %]
> +
> +  [% FOREACH sighting = sightings %]
> +    [% count = count + 1 %]
> +    <tr id="s[% count FILTER html %]">

I'm concerned that this will change. Use the bug_id as the s anchor, instead.

@@ +56,5 @@
> +    [% count = count + 1 %]
> +    <tr id="s[% count FILTER html %]">
> +      <td>
> +        <a href="show_bug.cgi?id=[% sighting.bug_id FILTER uri %]">
> +          [% sighting.bug_id FILTER html %]</a>

FILTER bug_link.

@@ +61,5 @@
> +      </td>
> +      <td>
> +        [% PROCESS display_status target = sighting %]
> +      </td>
> +    </tr>

This can be factored out into a BLOCK and re-used for the master bug above as well.

Comment 7

[Robert Webb]

Attachment: Adding sightings tables to the show bug screen. patch v4

Made the requested changes.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 575575 [details] [diff] [review]
Adding sightings tables to the show bug screen. patch v4

Review of attachment 575575 [details] [diff] [review]:
-----------------------------------------------------------------

::: skins/standard/global.css
@@ +410,3 @@
>      border-collapse: collapse;
>      border: 1px solid #333333;
> +    margin: 2ex 2ex;

That makes a 2ex margin all around, which would be more simply written as:

  margin: 2ex.

If you intended to only make a top and bottom margin, you should instead write:

  margin: 2ex 0.

@@ +417,3 @@
>      background-color: #E0E0E0;
>      color: black;
> +    padding: 0.5ex;

Usually we use "em" as the unit if we're not talking about height.

(em is the width of the letter M in the current font, ex is the height of the letter X--at least theoretically.)

::: template/en/default/attachment/list.html.tmpl
@@ -54,4 @@
>  //-->
>  </script>
>  
> -<br>

Thank you for removing these. :-)

::: template/en/default/sighting/list.html.tmpl
@@ +56,5 @@
> +      </td>
> +      <td>
> +        [% PROCESS bug/status.html.tmpl target = sighting %]
> +      </td>
> +    </tr>

Ah, what happened to creating a BLOCK for this?

Comment 9

[Robert Webb]

Attachment: Adding sightings tables to the show bug screen. patch v5

Made the suggested changes.

Comment 10

[Max Kanat-Alexander]

Comment on attachment 575594 [details] [diff] [review]
Adding sightings tables to the show bug screen. patch v5

Review of attachment 575594 [details] [diff] [review]:
-----------------------------------------------------------------

Beautiful! :-)

Comment 11

[Max Kanat-Alexander]

Approved for sightings branch.

Comment 12

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/sightings/
modified Bugzilla/Bug.pm
modified skins/standard/global.css
added template/en/default/sighting
modified template/en/default/attachment/list.html.tmpl
modified template/en/default/bug/edit.html.tmpl
added template/en/default/bug/status.html.tmpl
modified template/en/default/global/field-descs.none.tmpl
added template/en/default/sighting/list.html.tmpl
Committed revision 7959.

Comment 13

[Frédéric Buclin]

The sightings branch has never been merged with upstream. Not sure what the resolution/milestone for this bug should be, but certainly not 5.0.