284875 - Move GetBugLink, GetAttachmentLink, and quoteUrls out of globals.pl

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

Really, the thing belongs as a method of Bug.pm. If that's too difficult,
though, it should just be moved.

Comment 1

[Frédéric Buclin]

I don't think so. GetBugLink(), GetAttachmentLink() and quoteUrls() should go
into Template.pm as they are all related to HTML code.

Comment 2

[Max Kanat-Alexander]

(In reply to comment #1)
> I don't think so. GetBugLink(), GetAttachmentLink() and quoteUrls() should go
> into Template.pm as they are all related to HTML code.

  Agreed, particularly since eventually their entire job should be done inside
of template plugins.

Comment 3

[Frédéric Buclin]

/me hesitates to take this one. mkanat, are you volunteer? Note that all three
routines mentioned in comment 1 have to move together.

Comment 4

[Emmanuel Seyman]

> Agreed, particularly since eventually their entire job should be done
> inside of template plugins.

Seconded. GetBugLink is the only thing that keeps me from renaming 'bug' to
'bogue' in the french templates.

Comment 5

[Max Kanat-Alexander]

Sure, I'll take a look at it. I've done some hacking of GetBugLink before.

Comment 6

[Marc Schumann [:Wurblzap]]

Does this bug cover linking on the variables.none.tmpl variable string for the bug term instead of on the fixed string 'bug' as comment 4 indicates? If so, does it cover to convert linking on 'comment' to something more localizable as well?

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> Does this bug cover linking on the variables.none.tmpl variable string for the
> bug term instead of on the fixed string 'bug' as comment 4 indicates? If so,
> does it cover to convert linking on 'comment' to something more localizable as
> well?

  No, that should be done in another bug. We may already have one filed for it.

Comment 8

[Frédéric Buclin]

The trunk is now frozen to prepare Bugzilla 2.22. Enhancement bugs are retargetted to 2.24.

Comment 9

[Max Kanat-Alexander]

wicked-chu, I choose YOU! :-D

Comment 10

[Frédéric Buclin]

Attachment: patch, v1

wicked, yeah... I choose you too. ;)

Comment 11

[Frédéric Buclin]

Attachment: patch, v1.1

Fix the indentation in user-error.html.tmpl.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 213107 [details] [diff] [review]
patch, v1.1

>+sub GetAttachmentLink {

  If we're going to keep this function then it should be renamed to something like attachment_link or get_attachment_link. I'd rather see it as a "link" method of an Attachment object, but I understand that might be ridiculous for performance.

  Also, I sort of think this should be Bugzilla::Attachment::get_link, and GetBugLink should be Bugzilla::Bug::get_link.

>+    # If we've run GetAttachmentLink() for this attachment before,
>+    # %attachlink will contain an anonymous array ref of relevant
>+    # values.  If not, we need to get the information from the database.
>+    my %attachlink;
>+    if (exists Bugzilla->user->{'attachlink'}) {
>+        %attachlink = %{Bugzilla->user->{'attachlink'}};
>+    }

  I'd rather the Template object didn't try to create/modify fields in Bugzilla->user. Particularly because you shouldn't be modifying the "nobody" user, when somebody is logged-out. I think we can just get rid of the %attachlink hash for now.

>+    unless (defined $attachlink{$attachid}) {
>+        my ($bugid, $isobsolete, $desc) =
>+            $dbh->selectrow_array('SELECT bug_id, isobsolete, description
>+                                   FROM attachments WHERE attach_id = ?',
>+                                   undef, $attachid);

  Nit: You know, come to think of it, though, if this is what we're doing anyway, it wouldn't be all that much more work to create an Attachment object whenever you need one. It would be about as fast as this.

>+        if ($bugid) {
>+            my $title = "";
>+            my $className = "";
>+            if (Bugzilla->user->can_see_bug($bugid)) {
>+                $title = $desc;
>+            }
>+            if ($isobsolete) {
>+                $className = "bz_obsolete";
>+            }
>+            $attachlink{$attachid} = [value_quote($title), $className];
>+        }

  Nit: Why do we value_quote that? And why do we do it here? Ah well, I suppose that's not a big issue, since that was in the original code.

>+sub GetBugLink {

  If we're keeping this, it should be get_bug_link.

>+    my ($bug_num, $link_text, $comment_num) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    if (! defined $bug_num || $bug_num eq "") {
>+        return "<missing bug number>";
>+    }
>+    my $quote_bug_num = html_quote($bug_num);

  Nit: Once again, we probably shouldn't be quoting stuff here. Seems like we should just leave that to the templates. But that's how it was in the original, so we don't need to change it now.

>+    # If we've run GetBugLink() for this bug number before, %buglink
>+    # will contain an anonymous array ref of relevent values, if not
>+    # we need to get the information from the database.
>+    my %buglink;
>+    if (exists Bugzilla->user->{'buglink'}) {
>+       %buglink = %{Bugzilla->user->{'buglink'}};
>+    }

  Once again, I think this is unnecessary, unless anybody ever reported a real perf issue which caused us to add this. I checked, and this was added originally as premature optimization. I don't want to use Bugzilla->user as a cache for non-user-related items.

>+        # If the bug exists, save its data off for use later in the sub
>+        if ($bug_state) {
>+            # Initialize these variables to be "" so that we don't get warnings
>+            # if we don't change them below (which is highly likely).
>+            my ($pre, $title, $post) = ("", "", "");
>+
>+            $title = $bug_state;
>+            if ($bug_state eq 'UNCONFIRMED') {
>+                $pre = "<i>";
>+                $post = "</i>";
>+            }

  Nit: Instead of this we should be using a class, just like GetAttachmentLink does. But that's how this already was, so we can leave it for now.

>+            elsif (! &::IsOpenedState($bug_state)) {
>+                $pre = '<span class="bz_closed">';
>+                $title .= " $bug_res";
>+                $post = '</span>';
>+            }

  We might as well move this (GetBugLink) to Bugzilla::Bug, since that's where is_open_state will be, anyhow.

>+            if (Bugzilla->user->can_see_bug($bug_num)) {
>+                $title .= " - $bug_desc";
>+            }

  Nit: And making this a method of the Bug object would eliminate this step, which would simplify the code.


  Everything else looks good, on inspection.

Comment 13

[Frédéric Buclin]

Attachment: patch, v2

I'm definitely against having get_foo_link in either Attachment.pm or Bug.pm, as discussed on IRC. That's a Template thing.

This patch renames the two functions to get_foo_link and also removes the cache.

Comment 14

[Max Kanat-Alexander]

Comment on attachment 213197 [details] [diff] [review]
patch, v2

>+# Creates a link to an attachment, including its title.
>+sub get_attachment_link {
[snip]
>+    my @attachlink;

  I think that's not needed anymore. You can fix that on checkin.

  Everything else looks great. :-) Tested, too.

Comment 15

[Frédéric Buclin]

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.104; previous revision: 1.103
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.351; previous revision: 1.350
done
Checking in summarize_time.cgi;
/cvsroot/mozilla/webtools/bugzilla/summarize_time.cgi,v  <--  summarize_time.cgi
new revision: 1.15; previous revision: 1.14
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.110; previous revision: 1.109
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.42; previous revision: 1.41
done
Checking in t/008filter.t;
/cvsroot/mozilla/webtools/bugzilla/t/008filter.t,v  <--  008filter.t
new revision: 1.22; previous revision: 1.21
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.64; previous revision: 1.63
done
Checking in template/en/default/attachment/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.24; previous revision: 1.23
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.33; previous revision: 1.32
done
Checking in template/en/default/attachment/show-multiple.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/show-multiple.html.tmpl,v  <--  show-multiple.html.tmpl
new revision: 1.18; previous revision: 1.17
done
Checking in template/en/default/bug/summarize-time.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/summarize-time.html.tmpl,v  <--  summarize-time.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.149; previous revision: 1.148
done
Checking in template/en/default/global/variables.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/variables.none.tmpl,v  <--  variables.none.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/extension/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/extension/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.2; previous revision: 1.1
done