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

RESOLVED FIXED in Bugzilla 3.0

Status

()

--
enhancement
RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: mkanat, Assigned: LpSolit)

Tracking

2.19.2
Bugzilla 3.0
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

14 years ago
Blocks: 121112
(Assignee)

Comment 1

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

Comment 2

13 years ago
(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.
(Assignee)

Updated

13 years ago
No longer blocks: 121112
(Assignee)

Comment 3

13 years ago
/me hesitates to take this one. mkanat, are you volunteer? Note that all three
routines mentioned in comment 1 have to move together.
Target Milestone: --- → Bugzilla 2.22

Comment 4

13 years ago
> 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.

(Reporter)

Comment 5

13 years ago
Sure, I'll take a look at it. I've done some hacking of GetBugLink before.
Assignee: general → mkanat
Summary: Move GetBugLink out of globals.pl → Move GetBugLink, GetAttachmentLink, and quoteUrls out of globals.pl
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?
(Reporter)

Comment 7

13 years ago
(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.
(Assignee)

Comment 8

13 years ago
The trunk is now frozen to prepare Bugzilla 2.22. Enhancement bugs are retargetted to 2.24.
Target Milestone: Bugzilla 2.22 → Bugzilla 2.24
(Reporter)

Comment 9

13 years ago
wicked-chu, I choose YOU! :-D
Assignee: mkanat → wicked
(Assignee)

Comment 10

13 years ago
Created attachment 213103 [details] [diff] [review]
patch, v1

wicked, yeah... I choose you too. ;)
Assignee: wicked → LpSolit
Status: NEW → ASSIGNED
Attachment #213103 - Flags: review?(wicked)
(Assignee)

Comment 11

13 years ago
Created attachment 213107 [details] [diff] [review]
patch, v1.1

Fix the indentation in user-error.html.tmpl.
Attachment #213103 - Attachment is obsolete: true
Attachment #213107 - Flags: review?(wicked)
Attachment #213103 - Flags: review?(wicked)
(Reporter)

Comment 12

13 years ago
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.
Attachment #213107 - Flags: review?(wicked) → review-
(Assignee)

Comment 13

13 years ago
Created attachment 213197 [details] [diff] [review]
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.
Attachment #213107 - Attachment is obsolete: true
Attachment #213197 - Flags: review?(mkanat)
(Reporter)

Comment 14

13 years ago
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.
Attachment #213197 - Flags: review?(mkanat) → review+
(Reporter)

Updated

13 years ago
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 15

13 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Blocks: 291500
You need to log in before you can comment on or make changes to this bug.