Need a Bugzilla::Comment object

RESOLVED FIXED in Bugzilla 3.6

Status

()

enhancement
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: mkanat, Assigned: mkanat)

Tracking

(Blocks 1 bug)

Bugzilla 3.6
Dependency tree / graph
Bug Flags:
approval +

Details

(Whiteboard: [es-ita])

Attachments

(1 attachment, 15 obsolete attachments)

36.65 KB, patch
mkanat
: review+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
The more I work with comments, the more apparent it becomes that they should be objects. They can be Bugzilla::Object-based even, since they have an id.

Comment 1

10 years ago
(In reply to comment #0)
I'd like to try and take a crack at this, if I may. Any guidance you could give? 

I've been poking around the source, and it seems like a fairly straightforward task. We'd basically be making a Bugzilla::Object descendant out of the longdescs table. Is there a current Bugzilla::Object descendant that would be good to copy and modify as a sort of template?

Also, it seems like Bugzilla::Bug::GetComments() would return the Bugzilla::Bug::Comments (Bugzilla::Comments?) object, so that templates would continue to be able to do

[% PROCESS bug/comments.html.tmpl                                          
   comments = bug.longdescs
   mode = user.id ? "edit" : "show"                                        
%]

My observations are fairly perfunctory at this point. Anything else I might need to be aware of? Thanks, James
(Assignee)

Comment 2

10 years ago
(In reply to comment #1)
> Is there a current Bugzilla::Object descendant that would be good to copy and modify as a sort of template?

  Bugzilla::Keyword is usually the simplest object to look at.

> Also, it seems like Bugzilla::Bug::GetComments() would return the
> Bugzilla::Bug::Comments (Bugzilla::Comments?) object, so that templates would
> continue to be able to do

  Yeah, that would be ideal, but when you first create the object, you should use it in the least invasive way possible and it should do as little as possible. Then we can build on that in later bugs.
Assignee: general → arbingersys
Target Milestone: --- → Bugzilla 4.0

Comment 3

10 years ago
(In reply to comment #1)
> 
> Also, it seems like Bugzilla::Bug::GetComments() would return the
> Bugzilla::Bug::Comments (Bugzilla::Comments?) object

An array of Bugzilla::Comment objects sounds like the most appropriate choice here.
(Assignee)

Comment 4

10 years ago
(In reply to comment #3)
> An array of Bugzilla::Comment objects sounds like the most appropriate choice
> here.

  Of course.

Comment 5

10 years ago
Posted file Bugzilla::Comment class (obsolete) —
Here's the package. The only things not essentially boiler plate are a couple of accessors so the templates would be happy. I also added a validator for the 'thetext' field, but I'm not sure whether it was necessary or not.
Attachment #366893 - Flags: review?(mkanat)

Comment 6

10 years ago
GetComments now queries for a list of comment_ids for a bug, and does Comment->new_from_list().
Attachment #366899 - Flags: review?(mkanat)
(Assignee)

Comment 7

10 years ago
Comment on attachment 366893 [details]
Bugzilla::Comment class

># The Original Code is the Bugzilla Bug Tracking System.

  You are missing the Initial Developer block.

>use constant DB_COLUMNS => qw(
>    longdescs.comment_id

  You don't need the table name in front of the column names. (I know that we do this elsewhere, but it's actually unnecessary.)

>use constant DB_TABLE => 'longdescs';
>
>use constant ID_FIELD => 'comment_id';
>
>use constant LIST_ORDER => 'bug_when';

  Nit: Don't need an extra newline between these.

>use constant VALIDATORS => { 
>    thetext => \&_check_thetext
>};

  I'm fairly sure that everything needs a validator, but don't implement create() yet. Wait for another bug. We never implement anything unless it's actually being used.

>use constant UPDATE_COLUMNS => qw(

  Same comment here, don't implement update() until you're using it somewhere.

>sub bug_id   { return $_[0]->{'bug_id'}; }
>sub who      { return $_[0]->{'who'}; }
>sub userid   { return who(); }

  userid is a bad name for a field, and all names should be canonical. If the templates are using a bad name, then the templates should just be changed.

>sub bug_when { return $_[0]->{'bug_when'}; }

  bug_when is probably not a good standard name for a field. Perhaps creation_ts would be a better name for the accessor to be consistent with the rest of Bugzilla?

>sub time     { return bug_when(); }

  Same comment here. (Also, even if you were to do this, you would need to call it in $_[0].)

>###############################
>####       Mutators       #####
>###############################

  Let's not worry about these for now.

>=head1 SYNOPSIS
>
> use Bugzilla::Comment;
>
> my $comment = Bugzilla::Comment->new($comment_id);
>
> my $comments = Bugzilla::Comment->new_from_list($comment_ids);

  Nit: Remove the extra newline between the $comment and $comments line.
Attachment #366893 - Flags: review?(mkanat) → review-
(Assignee)

Comment 8

10 years ago
Comment on attachment 366899 [details] [diff] [review]
patch to Bugzilla::Bug::GetComments

>Index: Bugzilla/Bug.pm

>+    for ( @{Bugzilla::Comment->new_from_list($comment_ids)} ) {
> 
>-        push (@comments, \%comment);
>+        $_->{'author'} = new Bugzilla::User($_->{'userid'});

  That should be an accessor of the Comment object, not created here.

>+        
>+        $_->{'body'} = $_->{'thetext'};
>+        delete($_->{'thetext'});
>+        $_->{'body'} = format_comment($_) unless $raw;

  And perhaps there should be a body and raw_body accessor instead of doing this.
Attachment #366899 - Flags: review?(mkanat) → review-
(Assignee)

Comment 9

10 years ago
  By the way, you can add a new file to a cvs patch by using the "cvsdo" command, like "cvsdo add Bugzilla/Comment.pm". Then a "cvs diff -Nu" will pick it up.
Status: NEW → ASSIGNED

Comment 10

10 years ago
(In reply to comment #8)

> >+        $_->{'body'} = $_->{'thetext'};
> >+        delete($_->{'thetext'});
> >+        $_->{'body'} = format_comment($_) unless $raw;
> 
>   And perhaps there should be a body and raw_body accessor instead of doing
> this.

I've gotten most of what you suggest above done (I think). However, I realized that the 'body' returned by Bug->longdescs() is retrieved as a hash element rather than a sub elsewhere in the code, i.e.

post_bug.cgi
209:        if ($bug->longdescs->[0]->{'body'} !~ /^\s+$/) {
210:            $new_comment .= "\n" . $bug->longdescs->[0]->{'body'};

Bugzilla/Bug.pm
2949:    $current_comment_obj[0]->{'body'} = $new_comment;
3048:# $comment{'body'} itself, see BugMail::prepare_comments().

So should I just go through the entire source and change any reference to ...->{'body'} to ...->{'thetext'}, or to the accessor, i.e. ...->body, since 'thetext' is the actual name of the field? Or is it better to assign thetext to 'body' inside the Comment object on instantiation?

Thanks, James
(Assignee)

Comment 11

10 years ago
All objects should be accessed only through accessors. Their hash elements should never be accessed outside of their own code.

Comment 12

10 years ago
Posted patch patch3 (obsolete) — Splinter Review
Well, I finally got some time to work on this. I think this patch should cover everything discussed. In my testing comments still seem to behave how they did previously. James
Attachment #366893 - Attachment is obsolete: true
Attachment #366899 - Attachment is obsolete: true
Attachment #372688 - Flags: review?(mkanat)
(Assignee)

Comment 13

10 years ago
Comment on attachment 372688 [details] [diff] [review]
patch3

>Index: Bugzilla/Bug.pm
>+    my $query = 'SELECT longdescs.comment_id, longdescs.bug_when 

  You don't need to select bug_when--you're only using comment_id.

>+    my $comment_ids = $dbh->selectcol_arrayref($query, undef, @args);
>+    for ( @{Bugzilla::Comment->new_from_list($comment_ids)} ) {
>+        $_->set_body(format_comment($_)) unless $raw;

  No, instead there should be both a body() and a body_raw() accessor in the Bugzilla::Comment object. body() should return a formatted body, and body_raw() should return an unformatted body.

> sub format_comment {

  And this should be moved to be an appropriate private subroutine within Bugzilla::Comment, probably the code for body().

>Index: Bugzilla/Comment.pm
>+use constant REQUIRED_CREATE_FIELDS => qw(
>+    bug_id
>+    who
>+    bug_when
>+    thetext
>+);
>+
>+use constant VALIDATORS => { 
>+    thetext => \&_check_thetext
>+};

  Don't set those up unless you're going to implement or use create(), which we haven't yet.

>+###############################
>+#####      Mutators       #####
>+###############################
>+
>+sub set_bug_id {
>+    my ($self, $bug_id) = @_;
>+    $self->set('bug_id', $bug_id);
>+}

  Oh, that definitely should not exist. A comment's bug_id is not mutable.

  At this point, actually, there should be no mutators, because we aren't using or implementing update().

>Index: Bugzilla/WebService/Bug.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v
>retrieving revision 1.34
>diff -p -u -r1.34 Bug.pm
>--- Bugzilla/WebService/Bug.pm	31 Mar 2009 06:37:57 -0000	1.34
>+++ Bugzilla/WebService/Bug.pm	14 Apr 2009 20:45:40 -0000
>@@ -94,8 +94,8 @@ sub comments {
>             $bug->id, 'oldest_to_newest', $params->{new_since});
>         my @result;
>         foreach my $comment (@$comments) {
>-            next if $comment->{isprivate} && !$user->is_insider;
>-            $comment->{bug_id} = $bug->id;
>+            next if $comment->isprivate && !$user->is_insider;
>+            $comment->set_bug_id($bug->id);

  Since you have a comment object, you don't even need to do this. There's a working bug_id accessor. Also, you should change _translate_comment to use accessors instead of direct hash access.
Attachment #372688 - Flags: review?(mkanat) → review-
(Assignee)

Comment 14

10 years ago
Comment on attachment 372688 [details] [diff] [review]
patch3

Oh, also, don't expose "who" as an accessor. Just expose "author".
(Assignee)

Comment 15

10 years ago
Comment on attachment 372688 [details] [diff] [review]
patch3

Oh, and there's probably no need to expose extra_data (or maybe even type) as external accessors. Also, don't call thetext two different things, just expose a body() accessor and clean up any place referring to it as thetext.

Comment 16

10 years ago
Posted patch patch4 (obsolete) — Splinter Review
> You don't need to select bug_when--you're only using comment_id.

Actually, I left that in so Pg wouldn't break, since bug_when is used in the ORDER BY clause --

https://bugzilla.mozilla.org/show_bug.cgi?id=440259#c14

It probably shouldn't matter, since selectcol_arrayref as it's used only returns the first column anyway.

The $raw param has been removed from GetComments(), meaning you'll need to use the accessor $comment->body_raw in templates or wherever. I didn't see anywhere that it was currently being passed in, so I assume this won't break anything.

I'm not entirely sure if I got the WebService::Bug::comments() changes right, since I couldn't figure out how to test. In ack'ing the source, I couldn't find any calls to the routine, either.
Attachment #372688 - Attachment is obsolete: true
Attachment #372964 - Flags: review?(mkanat)
(Assignee)

Comment 17

10 years ago
(In reply to comment #16)
> Actually, I left that in so Pg wouldn't break, since bug_when is used in the
> ORDER BY clause --
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=440259#c14

  No, you don't need that. The restriction is that when you SELECT DISTINCT, you must have the ORDER BY column in your SELECT. It's not for a normal SELECT.

> I'm not entirely sure if I got the WebService::Bug::comments() changes right,
> since I couldn't figure out how to test. In ack'ing the source, I couldn't find
> any calls to the routine, either.

  It has API docs, you should make sure that it still conforms to them. One way is to use the XMLRPCsh.pl script that comes with SOAP::Lite.

Comment 18

10 years ago
Your patch is going to conflict with bug 457657, which will probably land before yours. You should probably wait for wurblzap's checkin.

Comment 19

10 years ago
(In reply to comment #17)
> (In reply to comment #16)

>   No, you don't need that. The restriction is that when you SELECT DISTINCT,
> you must have the ORDER BY column in your SELECT. It's not for a normal SELECT.

Okay, got it.
(Assignee)

Comment 20

10 years ago
Comment on attachment 372964 [details] [diff] [review]
patch4

>--- Bugzilla/Bug.pm	6 Apr 2009 20:02:51 -0000	1.277
>+++ Bugzilla/Bug.pm	15 Apr 2009 20:48:06 -0000
>
>+    for ( @{Bugzilla::Comment->new_from_list($comment_ids)} ) {
>+        push(@comments, $_);
>     }

  That is unnecessary. You could just do:

  my $comments = Bugzilla::Comment->new_from_list($comment_ids);

  And then dereference the array (@$comments) as necessary.

>Index: Bugzilla/Comment.pm
> [snip]
>+use constant REQUIRED_CREATE_FIELDS => qw(

  We don't need this yet.

>+sub isprivate   { return $_[0]->{'isprivate'}; }

  For consistency, we should probably call that is_private.

>+sub body {
>+    my $self = shift;
>+    my $body;
>+
>+    if ($self->{'type'} == CMT_DUPE_OF) {
>+        $body = $self->{'thetext'} . "\n\n" .

  Use $self->body_raw instead of {thetext}.

>+###############################
>+#####      Mutators       #####
>+###############################
>+
>+# none yet
>+

  Just leave the sections out entirely if they don't have functions yet.

>+=head1 SUBROUTINES
>+
>+None.

  Same here.

  You can document the accessors, by the way. I'd recommend it.

>Index: Bugzilla/WebService/Bug.pm
>@@ -105,11 +105,10 @@ sub comments {
>+            'SELECT comment_id FROM longdescs WHERE ' . 
>+            $dbh->sql_in('comment_id', \@sql_ids), {Slice=>{}});

  Instead of this, you can just call Bugzilla::Comment->new_from_list.

>+        for ( @{Bugzilla::Comment->new_from_list($comment_data)} ) {
>+            if ($_->isprivate && !$user->is_insider) {
>+                ThrowUserError('comment_is_private', { id => $_->id });
>             }

  #1, you should never be doing multi-line foreach loops without a "my" var (and you should be calling them "foreach", not "for").

  #2, you should be creating an arrayref like "my $comments = " and then using it.
Attachment #372964 - Flags: review?(mkanat) → review-
(Assignee)

Comment 21

10 years ago
(In reply to comment #18)
> Your patch is going to conflict with bug 457657, which will probably land
> before yours. You should probably wait for wurblzap's checkin.

  Actually, given the level of activity on both bugs, I suspect this one will land first.

Comment 22

10 years ago
Posted patch patch5 (obsolete) — Splinter Review
Been a bit since I visited this, but I think I got all the changes.

As for testing, runtests.pl was successful, comments work for me on my dev install, and I tested for comments against WebService/Bug.pm with bz_webservice_demo.pl

James
Attachment #372964 - Attachment is obsolete: true
Attachment #377484 - Flags: review?(mkanat)
(Assignee)

Comment 23

10 years ago
Comment on attachment 377484 [details] [diff] [review]
patch5

>Index: Bugzilla/Bug.pm
>+    for ( @{Bugzilla::Comment->new_from_list($comment_ids)} ) {
>+        push(@comments, $_);
>     }

  You don't need to do that. All you need is:

  my $comments = Bugzilla::Comment->new_from_list($comment_ids);

>Index: Bugzilla/BugMail.pm
>     if (Bugzilla->params->{'insidergroup'}) {
>-        $anyprivate = 1 if scalar(grep {$_->{'isprivate'} > 0} @$comments);
>+        $anyprivate = 1 if scalar(grep {$_->is_private > 0} @$comments);

  FWIW, you don't need the "> 0" there.

>Index: Bugzilla/Comment.pm
>+# The Initial Developer of the Original Code is Netscape Communications
>+# Corporation. Portions created by Netscape are
>+# Copyright (C) 1998 Netscape Communications Corporation. All
>+# Rights Reserved.

  I'd say enough of this is yours that you can just say it's yours.

>+sub bug_id      { return $_[0]->{'bug_id'}; }

  Is there ever a time that you need bug_id and not just "bug"? I'd rather have a "bug" accessor that returns a Bugzilla::Bug object (and when Bugzilla::Bug gets comments, it can just pre-populate this field with itself).

[snip]
>+sub body_raw    { return $_[0]->{'thetext'}; }
>+sub work_time   { return $_[0]->{'work_time'}; }
>+sub is_private   { return $_[0]->{'isprivate'}; }
>+sub already_wrapped { return $_[0]->{'already_wrapped'}; }

  By the way, I'd prefer it if these simple accessors were in alphabetical order.

>+=item C<body_raw>
>+
>+raw bytes of the comment

  "The body without any special additional text."

>+=item C<already_wrapped>
>+
>+whether comment formatting has occurred

  "If this comment is stored in the database word-wrapped, this will be C<1>. C<0> otherwise."

>+=item C<author>
>+
>+user who created the comment

  "L<Bugzilla::User> who created the comment"

>+=item C<body>
>+
>+formatted body of the comment

  Body of the comment, including any special text (such as "this bug was marked as a duplicate of...")

>Index: Bugzilla/WebService/Bug.pm
> [snip]
>     if (scalar @$comment_ids) {
>         my @ids = map { trim($_) } @$comment_ids;
>         my @sql_ids = map { $dbh->quote($_) } @ids;

  You don't need to quote the ids anymore if you're just passing them to new_from_list.


  Otherwise it looks pretty good.
Attachment #377484 - Flags: review?(mkanat) → review-
(Assignee)

Updated

10 years ago
Target Milestone: Bugzilla 4.0 → Bugzilla 3.6

Comment 24

10 years ago
Posted patch patch6 (obsolete) — Splinter Review
I added the bug() method, but perhaps I shouldn't have, since nothing is requiring it at the moment.

As to removing the bug_id accessor, it appears to be necessary in WebServices/Bug.pm for the _translate_comment() function.
Attachment #377484 - Attachment is obsolete: true
Attachment #377543 - Flags: review?(mkanat)
(Assignee)

Comment 25

10 years ago
(In reply to comment #24)
> I added the bug() method, but perhaps I shouldn't have, since nothing is
> requiring it at the moment.

  Ah, yeah, if nothing uses it, there's no need for it.

> As to removing the bug_id accessor, it appears to be necessary in
> WebServices/Bug.pm for the _translate_comment() function.

  Yeah, that's fine.

Comment 26

10 years ago
Posted patch patch6a (obsolete) — Splinter Review
Attachment #377544 - Flags: review?

Comment 27

10 years ago
Posted patch patch6b (obsolete) — Splinter Review
Gah. A little fast with the Enter button. Sorry.

I was returning the wrong hash element in bug() in patch6. This one should be right.
Attachment #377543 - Attachment is obsolete: true
Attachment #377544 - Attachment is obsolete: true
Attachment #377546 - Flags: review?(mkanat)
Attachment #377544 - Flags: review?
Attachment #377543 - Flags: review?(mkanat)

Comment 28

10 years ago
Posted patch patch7 (obsolete) — Splinter Review
Okay, last one (tonight anyway). I removed bug(). James
Attachment #377546 - Attachment is obsolete: true
Attachment #377547 - Flags: review?(mkanat)
Attachment #377546 - Flags: review?(mkanat)
(Assignee)

Comment 29

10 years ago
Comment on attachment 377547 [details] [diff] [review]
patch7

>Index: Bugzilla/Bug.pm
>+    my $comment_ids = $dbh->selectcol_arrayref($query, undef, @args);
>+    for ( @$comments ) {
>+        push(@comments, $_);
>     }

  Look, I understand Perl is confusing, but you DON'T have to have that for loop AT ALL. You dereference an arrayref like @$comments. You can do it anywhere to treat an arrayref just like an array.
Attachment #377547 - Flags: review?(mkanat) → review-
(Assignee)

Comment 30

10 years ago
Comment on attachment 377547 [details] [diff] [review]
patch7

>+# The Original Code is the Bugzilla Bug Tracking System.
>+#
>+# Contributor(s): James Robson <arbingersys@gmail.com> 

  You still need an Original Author section. Have you read the Bugzilla:Developers contribution process? It specifically mentions and links to the licensing guidelines.

>+=item C<bug_id>
>+
>+The bug object to which the comment belongs.

  A bug_id is not an object.

>+
>+=item C<creation_ts>
>+
>+The comment creation timestamp.

  It'd be nice if you could put some types on these. Like C<string> for this one.

>+=item C<already_wrapped>
>+
>+whether comment formatting has occurred

  No, use the text I said.


>         my @ids = map { trim($_) } @$comment_ids;
>-        my @sql_ids = map { $dbh->quote($_) } @ids;

  Actually, come to think of it, we probably don't need that trim anymore either...? I'm not sure, though.

Comment 31

10 years ago
Posted patch patch8 (obsolete) — Splinter Review
I think trim($_) might still be needed, since the comment IDs could come from user input, e.g.

$soapresult = $proxy->call('Bug.comments', {comment_ids => [$comment_id]});
Attachment #377547 - Attachment is obsolete: true
Attachment #377738 - Flags: review?(mkanat)
(Assignee)

Comment 32

10 years ago
Comment on attachment 377738 [details] [diff] [review]
patch8

This actually looks good, but we changed things upstream so that format_comment uses a template now, so this patch isn't quite up-to-date anymore. I'm sorry for the delayed review--somehow I didn't notice the review request until just now.
Attachment #377738 - Flags: review?(mkanat) → review-

Comment 33

10 years ago
(In reply to comment #32)
> (From update of attachment 377738 [details] [diff] [review])
> This actually looks good, but we changed things upstream so that format_comment
> uses a template now, so this patch isn't quite up-to-date anymore.

So what's next? Re-work to include the format_comment changes? It was this patch, wasn't it --

https://bugzilla.mozilla.org/show_bug.cgi?id=457657
(Assignee)

Comment 34

10 years ago
Yeah, re-work to include any changes that were made since you posted the patch.

Comment 35

10 years ago
Posted patch patch9 (obsolete) — Splinter Review
Moved format_comment with template to Bugzilla::Comment::body().
Attachment #377738 - Attachment is obsolete: true
Attachment #388219 - Flags: review?(mkanat)
(Assignee)

Comment 36

10 years ago
Comment on attachment 388219 [details] [diff] [review]
patch9

Actually, this looks great. If it doesn't currently apply or has some problems, I will fix them on checkin.
Attachment #388219 - Flags: review?(mkanat) → review+
(Assignee)

Updated

10 years ago
Flags: approval+
(Assignee)

Comment 37

10 years ago
Posted patch v10 (WIP) (obsolete) — Splinter Review
Okay, so actually, this required more than checkin fixes, but I did all the fixes myself, and it looks pretty nice now, except for one thing--calling the format_comment template strips leading whitespace from comments. :-( I have no idea how to work around it (or at least, no idea how to work around it cleanly).
Attachment #388219 - Attachment is obsolete: true
(Assignee)

Comment 38

10 years ago
Oh also, the way I have it now won't work right for wants_bug_mail, because of how we're doing "Created an attachment" now.
(Assignee)

Comment 39

10 years ago
Posted patch v11 (obsolete) — Splinter Review
Okay, this solves all the earlier problems with the patch. (Jason: Since the patch is now my responsibility, I'm taking the Assigned To field, but I will still credit you during checkin.)
Assignee: arbingersys → mkanat
Attachment #408780 - Attachment is obsolete: true
Attachment #408784 - Flags: review?(LpSolit)
(Assignee)

Updated

10 years ago
Flags: approval+
(Assignee)

Comment 40

10 years ago
By the way, s/Jason/James/ in that last comment. :-)

Comment 41

10 years ago
(In reply to comment #40)
> By the way, s/Jason/James/ in that last comment. :-)

No problem. With my name, 'Jim' is always the intentional replacement people make, and 'Jason' the accidental one. :)
(Assignee)

Comment 42

10 years ago
Posted patch v12 (obsolete) — Splinter Review
Fixed some bitrot.
Attachment #408784 - Attachment is obsolete: true
Attachment #410108 - Flags: review?(LpSolit)
Attachment #408784 - Flags: review?(LpSolit)
(Assignee)

Updated

10 years ago
Blocks: 452919
(Assignee)

Updated

10 years ago
Whiteboard: [es-ita]

Comment 43

10 years ago
Comment on attachment 410108 [details] [diff] [review]
v12

>Index: Bugzilla/Bug.pm

> sub GetComments {

>+    my $query = 'SELECT longdescs.comment_id, longdescs.bug_when 
>+                 FROM longdescs WHERE bug_id = ?';

No need to write longdescs.foo as we only query the longdescs table. Also, bug_when is used nowhere as you only care about comment IDs. It should go away from the query.


>         $query .= ' AND longdescs.bug_when > ?';

s/longdescs\.//; and in the remaining SQL query too.


>     $query .= " ORDER BY longdescs.bug_when $sort_order";

This is useless as new_from_list() already orders the list itself. This ORDER BY will have no effect.



>Index: Bugzilla/User.pm

>+    my $comments_concatenated = join("\n", map { $_->body_full(1) } @$comments);

We don't need the full body here. "Created an attachment" is hardcoded in the comment, and $comments_concatenated is only used for this purpose. This would make email sending a bit faster.



>Index: template/en/default/bug/comments.html.tmpl

>     <div class="bz_comment[% " bz_private" IF comment.isprivate %]

Should be comment.is_private. Appears again later in this template.


>+[% full_comment = BLOCK %]
>+  [% FILTER remove('^X') %][% PROCESS bug/format_comment.txt.tmpl %][% END %]
>+[% END %]

Why not using comment.body_full? This would make the template easier to read.



>Index: template/en/default/bug/format_comment.txt.tmpl

> [% IF comment.type == constants.CMT_DUPE_OF %]
>-X[% comment.body %]
>+X[% comment_body %]

This is not the single place to use comment.body. Other places should be fixed too.



>Index: template/en/default/bug/show.xml.tmpl

>+                [%- FILTER remove('^X') %][% PROCESS "bug/format_comment.txt.tmpl" comment = c %][% END ~%]

Why not using comment.body_full?



>Index: template/en/default/email/newchangedmail.txt.tmpl

>+[%+ FILTER remove('^X') %][% PROCESS bug/format_comment.txt.tmpl is_bugmail = 1 %][% END %]

Same here.
Attachment #410108 - Flags: review?(LpSolit) → review-
(Assignee)

Updated

10 years ago
Depends on: 355283
(Assignee)

Comment 44

10 years ago
Posted patch v13 (obsolete) — Splinter Review
Okay, I did even better than you asked for. I removed GetComments entirely and gave Bugzilla::Bug objects the functionality that we needed from GetComments. I also renamed the accessor from "longdescs" to "comments", both because it's a better name and because it now returns something so different that I don't want customizers or extension authors to think that they can just keep using it safely, normally (and this also helped me find everywhere that it was being used, and fix it).

And we use body_full everywhere now instead of calling format_comment. In fact, body_full is now the ONLY place where we call format_comment, making our "X" hack much less bad.
Attachment #410108 - Attachment is obsolete: true
Attachment #411305 - Flags: review?(LpSolit)

Updated

10 years ago
Attachment #411305 - Flags: review?(LpSolit) → review+

Comment 45

10 years ago
Comment on attachment 411305 [details] [diff] [review]
v13

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

>+    warn scalar(@$comments);

debug code



>=== added file 'Bugzilla/Comment.pm'

>+sub author { 
>+    my $self = shift;
>+    return $self->{'author'} if exists $self->{'author'};
>+    $self->{'author'} = new Bugzilla::User($self->{'who'});

Nit: would be cleaner as: $self->{'author'} ||= new Bugzilla::User(...), so that we have a single return in this method.


Seems to work fine (I extracted datetime_from() from bug 355283, so that this one can land first). r=LpSolit

Comment 46

10 years ago
Would be nice to land datetime_from() as part of this bug, so that I don't need to worry about it anymore when reviewing bug 355283.
Flags: approval+
(Assignee)

Comment 47

10 years ago
(In reply to comment #46)
> Would be nice to land datetime_from() as part of this bug, so that I don't need
> to worry about it anymore when reviewing bug 355283.

  Sure, I'd be happy to do that.
(Assignee)

Comment 48

10 years ago
Checking in email_in.pl;
/cvsroot/mozilla/webtools/bugzilla/email_in.pl,v  <--  email_in.pl
new revision: 1.28; previous revision: 1.27
done
Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.172; previous revision: 1.171
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.204; previous revision: 1.203
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.424; previous revision: 1.423
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.295; previous revision: 1.294
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.131; previous revision: 1.130
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Comment.pm,v
done
Checking in Bugzilla/Comment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Comment.pm,v  <--  Comment.pm
initial revision: 1.1
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.113; previous revision: 1.112
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.196; previous revision: 1.195
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.95; previous revision: 1.94
done
Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.45; previous revision: 1.44
done
Checking in template/en/default/bug/comments.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/comments.html.tmpl,v  <--  comments.html.tmpl
new revision: 1.44; previous revision: 1.43
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.168; previous revision: 1.167
done
Checking in template/en/default/bug/format_comment.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/format_comment.txt.tmpl,v  <--  format_comment.txt.tmpl
new revision: 1.5; previous revision: 1.4
done
Checking in template/en/default/bug/show-multiple.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show-multiple.html.tmpl,v  <--  show-multiple.html.tmpl
new revision: 1.46; previous revision: 1.45
done
Checking in template/en/default/bug/show.xml.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show.xml.tmpl,v  <--  show.xml.tmpl
new revision: 1.34; previous revision: 1.33
done
Checking in template/en/default/email/newchangedmail.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/email/newchangedmail.txt.tmpl,v  <--  newchangedmail.txt.tmpl
new revision: 1.17; previous revision: 1.16
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 49

10 years ago
Attachment #411305 - Attachment is obsolete: true
Attachment #411335 - Flags: review+
(Assignee)

Comment 50

10 years ago
Oh dang, I forgot to also credit Jason on the checkin.
(Assignee)

Comment 51

10 years ago
I mean JAMES. Hahaha. Man, that's the second time IN THIS BUG that I did that! :-)
(For reference the 'leading X' thing is needed because we use |TRIM => 1|)
(Assignee)

Updated

10 years ago
No longer depends on: 355283
(Assignee)

Updated

9 years ago
Keywords: relnote
(Assignee)

Comment 53

9 years ago
Added to the release notes in bug 547466.
Keywords: relnote

Updated

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