609968 - Allow local See Also links to show status/summary when mouseover

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement)

Description

[David Lawrence [:dkl]]

Attachment: Patch to allow mouseover of local See Also links (v1)

Submitting a patch that allows See Also links that are local the Bugzilla installation to show a title popup when mousing over the link. The patch adds a new filter 'see_also_link' to Bugzilla/Template.pl and then in field.html.tmpl the See Also url uses the filter to either create a normal link for a non-local reference, or a link with mouseover title for local.

Please review
Dave

Comment 1

[Frédéric Buclin]

This would be a first step towards bug 504164.

Comment 2

[Max Kanat-Alexander]

Actually, timello is now the assigned reviewer for all inter-Bugzilla work.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 488533 [details] [diff] [review]
Patch to allow mouseover of local See Also links (v1)

I'm going to redirect this to timello since he handles this area now.

Comment 4

[Tiago Mello [:timello]]

Comment on attachment 488533 [details] [diff] [review]
Patch to allow mouseover of local See Also links (v1)

>=== modified file 'Bugzilla/Template.pm'
>+            see_also_link => sub
>+            {
>+                my $link = shift;
>+                my $urlbase_re = '(' . join('|',
>+                    map { qr/$_/ } grep($_, Bugzilla->params->{'urlbase'},
>+                    Bugzilla->params->{'sslbase'})) . ')';
>+                if ($link =~ /\b(${urlbase_re}\Qshow_bug.cgi?id=\E([0-9]+))/) {
>+                    return get_bug_link($3, $1) . " (local)";
>+                }

You should be using Bugzilla::BugUrl->class_for instead and check if the returned class is Bugzilla::BugUrl::Bugzilla::Local.
Also, the class_for method returns an URI object which you get the referenced bug id using ->query_param('id');


>+                else {
>+                    $link = html_quote($link);
>+                    return "<a href=\"$link\">$link</a>";
>+                }
>+            },

Maybe you can pass only local bug link to the FILTER and then you wouldn't need to treat other links here.

That is:

[% IF bug_url.class == "Bugzilla::BugUrl::Bugzilla::Local" %]
  [% bug_url.name FILTER local_see_also_link ... %]


>=== modified file 'template/en/default/bug/field.html.tmpl'
>--- template/en/default/bug/field.html.tmpl	2010-11-02 22:58:27 +0000
>+++ template/en/default/bug/field.html.tmpl	2010-11-05 18:58:18 +0000
>@@ -165,7 +165,7 @@
>        [% '<ul class="bug_urls">' IF value.size %]
>        [% FOREACH url = value %]
>          <li>
>-           <a href="[% url FILTER html %]">[% url FILTER html %]</a>
>+           [% url FILTER see_also_link FILTER none %]

It's bug_url now, and it's a BugUrl object instead.

Comment 5

[David Lawrence [:dkl]]

Attachment: Patch to allow mouseover of local See Also links (v2)

Thanks Timello. I have updated the patch for review.

Comment 6

[Tiago Mello [:timello]]

Comment on attachment 512864 [details] [diff] [review]
Patch to allow mouseover of local See Also links (v2)

>=== modified file 'Bugzilla/Template.pm'
>+            local_see_also_link => sub
>+            {
>+                my $link = shift;
>+                my ($class, $uri) = Bugzilla::BugUrl->class_for($link);

Just wondering if we shouldn't be adding the 'use Bugzilla::BugUrl'... 

>=== modified file 'template/en/default/bug/field.html.tmpl'
>+           [% IF bug_url.class == 'Bugzilla::BugUrl::Bugzilla::Local' %]
>+             [% bug_url.name FILTER local_see_also_link(bug_url.name) FILTER none %] 

I think you don't need to pass again the bug_url.name to the filter, you can just use:

  bug_url.name FILTER local_see_also_link FILTER none

isn't it?

Nit: There's a trailing space in the line above.


Otherwise, it looks good and it passes in the tests. I think it's fine to fix it in the checkin.

Comment 7

[Frédéric Buclin]

Comment on attachment 512864 [details] [diff] [review]
Patch to allow mouseover of local See Also links (v2)

>=== modified file 'Bugzilla/Template.pm'

>+                my ($class, $uri) = Bugzilla::BugUrl->class_for($link);

timello is right, you must load Bugzilla::BugUrl first.


>+                return get_bug_link($bug_id, $bug_id) . " (local)";

Don't add hardcoded strings outside templates. They cannot be translated.

Comment 8

[Max Kanat-Alexander]

(In reply to comment #6)
> >+           [% IF bug_url.class == 'Bugzilla::BugUrl::Bugzilla::Local' %]

  You need to be checking .isa(), not the class name.

> >+             [% bug_url.name FILTER local_see_also_link(bug_url.name) FILTER none %] 

  Why not just filter the whole bug_url object?

Comment 9

[Tiago Mello [:timello]]

(In reply to comment #8)
> (In reply to comment #6)
> > >+             [% bug_url.name FILTER local_see_also_link(bug_url.name) FILTER none %] 
> 
>   Why not just filter the whole bug_url object?

If I'm not wrong you can't pass the object, because it will pass a string and not the object.

Comment 10

[Max Kanat-Alexander]

(In reply to comment #9)
> If I'm not wrong you can't pass the object, because it will pass a string and
> not the object.

  Oh maybe. Try it and see.

Comment 11

[David Lawrence [:dkl]]

Attachment: Patch to allow mouseover of local See Also links (v3)

Patch addressing timello and LpSolit's comments. Passing in the bug_url object to FILTER did in fact not work so I still pass bug_url.name.

Dave

Comment 12

[Max Kanat-Alexander]

Comment on attachment 513284 [details] [diff] [review]
Patch to allow mouseover of local See Also links (v3)

You know, I actually just realized that this whole filter is not necessary. You can just use the normal bug_link filter, because the bug_url object has the remote bug id on it, as an accessor. (And if it doesn't, it sure should have!)

Comment 13

[David Lawrence [:dkl]]

(In reply to comment #12)
> Comment on attachment 513284 [details] [diff] [review]
> Patch to allow mouseover of local See Also links (v3)
> 
> You know, I actually just realized that this whole filter is not necessary. You
> can just use the normal bug_link filter, because the bug_url object has the
> remote bug id on it, as an accessor. (And if it doesn't, it sure should have!)

It doesn't from what I can tell. It has an 'id' column as a standard serial id and it has bug_id which is the local bug id that the see_also entry is for. But the remote bug id is embedded in the full url in the 'value' field so is not in a column by itself.

Dave

Comment 14

[Max Kanat-Alexander]

(In reply to comment #13)
> But
> the remote bug id is embedded in the full url in the 'value' field so is not in
> a column by itself.

  Ah, okay. Perhaps add an accessor that returns the ref_bug, then.

Comment 15

[David Lawrence [:dkl]]

Attachment: Patch to allow mouseover of local See Also links (v4)

New patch with Max's suggested changes. Added a ref_bug method to each of the BugUrl sub modules that returns that BugUrl's bug id if possible. The field.html.tmpl template now uses bug_link FILTER for local bug references.

Dave

Comment 16

[Tiago Mello [:timello]]

Comment on attachment 513579 [details] [diff] [review]
Patch to allow mouseover of local See Also links (v4)

I didn't understand why you created all the ref_bug methods for every subclass if you just use it for Bugzilla::BugUrl::Bugzilla::Local, and also there is already a method for that, please see: Bugzilla::BugUrl::Bugzilla::Local->ref_bug_url

Comment 17

[David Lawrence [:dkl]]

Attachment: Patch to allow mouseover of local See Also links (v5)

Thanks Timello. The reason I added it to them all was to satisfy Max's suggestion to add a consistent way for each of the different subclasses to return their bug id's without manually parsing each URL. I did it for all of them just in case it was needed in the future. I also was not aware of the ref_bug_url->id method for getting the local bug's id which is now the better way.

I have attached a much simplified patch that just uses bug_url->ref_bug_url->id for local bug links only. The rest work the same as before.

Dave

Comment 18

[Tiago Mello [:timello]]

Comment on attachment 513601 [details] [diff] [review]
Patch to allow mouseover of local See Also links (v5)

Much better now! :-)

But:

>=== modified file 'template/en/default/bug/field.html.tmpl'
>-           <a href="[% bug_url.name FILTER html %]">
>-             [% bug_url.name FILTER html %]</a>
>+           [% IF bug_url.isa('Bugzilla::BugUrl::Bugzilla::Local') %]
>+             [% bug_url.ref_bug_url.id FILTER bug_link(bug_url.ref_bug_url.id, use_alias => 1) FILTER none %]

It should be bug_url.ref_bug_url.bug_id instead of 'id'

Nit: Couldn't we break this line somehow?

Also, it has a different behavior when seeing the bug when you are not logged in. I think we should change something else to also work when not logged in.

Comment 19

[Max Kanat-Alexander]

The problem with using ref_bug_url is that you will now have to create an object from the database for every single Local link.

I liked dkl's patch that added ref_bug (although it should probably be called target_bug_id), but I agree that we should only add it where it's necessary. (So only in Bugzilla::BugUrl::Bugzilla for now.)

Comment 20

[David Lawrence [:dkl]]

Attachment: Patch to allow mouseover of local See Also links (v6)

New patch with Max's suggested solution.

Comment 21

[Max Kanat-Alexander]

Comment on attachment 514191 [details] [diff] [review]
Patch to allow mouseover of local See Also links (v6)

FWIW, I think it will be obvious that it's local, I'm not sure (local) is necessary.

Comment 22

[David Lawrence [:dkl]]

Attachment: Patch to allow mouseover of local See Also links (v7)

Removed the (local) from appearing next to a local bug link as it is now obvious.

Comment 23

[Tiago Mello [:timello]]

Comment on attachment 514216 [details] [diff] [review]
Patch to allow mouseover of local See Also links (v7)

It does not work when the bug is not *editable*. You should also change the block below:

[% ELSIF field.type == constants.FIELD_TYPE_BUG_URLS %]

......

Comment 24

[David Lawrence [:dkl]]

Attachment: Patch to allow mouseover of local See Also links (v8)

Ok, I apologize for missing the not logged in case before. The new patch should fix that as well. 

Dave

Comment 25

[Tiago Mello [:timello]]

Comment on attachment 514239 [details] [diff] [review]
Patch to allow mouseover of local See Also links (v8)

Ideally, it would be nice to write a BLOCK for all bug_urls section, then we would avoid repeating code... sorry for not mentioning that before...

Comment 26

[David Lawrence [:dkl]]

Attachment: Patch to allow mouseover of local See Also links (v9)

Good idea. I added a bug_url_link BLOCK in the template and use that instead. I feel we might be getting close ;)

Dave

Comment 27

[Tiago Mello [:timello]]

Comment on attachment 514282 [details] [diff] [review]
Patch to allow mouseover of local See Also links (v9)

Cool! It works fine and also all tests pass.

Comment 28

[Max Kanat-Alexander]

Beautiful! Thank you, you guys!! :-) :-) :-)

Comment 29

[David Lawrence [:dkl]]

trunk:
Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/ modified template/en/default/bug/field.html.tmpl
modified Bugzilla/BugUrl/Bugzilla.pm
Committed revision 7723.