283926 - Make process_bug add comments by using Bugzilla::Bug

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

There should be an add_comment function. It should itself call the code in
ValidateComment, so we don't have use that externally anymore.

This should be a very easy start to bug 122922.

Comment 1

[Paul]

Attachment: added the function add_comment

Comment 2

[Max Kanat-Alexander]

Comment on attachment 207187 [details] [diff] [review]
added the function add_comment

1) bug_id($self) won't work, that's not how Object-Oriented stuff in perl works.

2) I'd like to actually get rid of AppendComment entirely and *refactor* its code into add_comment.

Comment 3

[Paul]

Attachment: removed AppendComment() and changed all calling code

Comment 4

[Max Kanat-Alexander]

Comment on attachment 207193 [details] [diff] [review]
removed AppendComment() and changed all calling code

As I said in IRC:

I'm a little uncertain about the safety of this patch when/if the bug object comes up undefined.

Also, we should create the bug object much higher in the script, so that other patches can use it in the future.

And add_comment should probably clear {longdescs} if it exists.

And finally, I'd like to see it used named parameters (with sensible defaults) for everything except the comment text.

Comment 5

[Max Kanat-Alexander]

And where possible, I'd like to call it $bug instead of $bugobj, but if that variable name is already taken, I understand.

Comment 6

[Paul]

(In reply to comment #5)
> And where possible, I'd like to call it $bug instead of $bugobj, but if that
> variable name is already taken, I understand.
> 

Not normally a nice thing to do.  It helps slightly in typing the identifier, but is harmful for debugging.

For example I have to look through this to find what I'm looking for.
http://landfill.bugzilla.org/mxr/bugzilla/search?string=%5C%24bug

Comment 7

[Paul]

Attachment: Implemented suggested changes

Comment 8

[Max Kanat-Alexander]

Comment on attachment 207345 [details] [diff] [review]
Implemented suggested changes

This *looks* generally great. :-) Except that I'd rather you just added "who" to the (new) _validate_attribute sub instead of calling it "inspector."

Comment 9

[Frédéric Buclin]

Comment on attachment 207345 [details] [diff] [review]
Implemented suggested changes

Bug.pm still tries to export AppendComment() despite this function no longer exists. And Bug::CheckIfVotedConfirmed() still uses AppendComment(), doesn't find it and crashes (I didn't test, but this seems pretty obvious).

Comment 10

[Frédéric Buclin]

(In reply to comment #8)
> (From update of attachment 207345 [details] [diff] [review] [edit])
> This *looks* generally great. :-) Except that I'd rather you just added "who"
> to the (new) _validate_attribute sub instead of calling it "inspector."
> 

Oh, and I second mkanat's comment. Use 'who' instead of 'inspector'.

Comment 11

[Paul]

(In reply to comment #10)
> (In reply to comment #8)
> 
> Oh, and I second mkanat's comment. Use 'who' instead of 'inspector'.
> 

Because you don't like to have self documenting code?  who? who? who?
who's looking at the bug?
who's commented on the bug?
who's assigned to the bug?
who's reported the bug?

I agree let's call them all "$who"  !!!

Comment 12

[Max Kanat-Alexander]

(In reply to comment #11)
> Because you don't like to have self documenting code?  who? who? who?
> who's looking at the bug?
> who's commented on the bug?
> who's assigned to the bug?
> who's reported the bug?

  Yes, "who" could be unclear, but it's what we already use. If you want it to be more consistent, you can also change the fact that the internal Bug field is called "who". That would be fine. I'm not sure I like "inspector," but it's definitely possible we could come up with something clearer than "who." Naming variables can definitely be quite a mental task. :-)

  Also, I understand the need for discussion, but that can be done in IRC if you feel frustrated by something.  Please don't put inflamatory comments into Bugzilla. They stay here forever.

Comment 13

[victory <[e-mail]>]

Attachment: updated

changes from 207345:

s/inspector/who/g
remove AppendComment from @Bugzilla::Bug::EXPORT

previous patch caused 'Global symbol "$bug_obj" requires explicit package name at process_bug.cgi line 1693.'
-> +        my $bug_obj = new Bugzilla::Bug($id, $whoid);

Comment 14

[Frédéric Buclin]

Comment on attachment 225209 [details] [diff] [review]
updated

>Index: attachment.cgi

You forgot AppendComment() around line 1390:

   # Paste the reason provided by the admin into a comment.
   AppendComment($bug_id, $user->id, $msg);



>Index: process_bug.cgi

>@@ -1690,8 +1690,11 @@

>+        my $bug_obj = new Bugzilla::Bug($id, $whoid);

Use the existing $old_bug_obj bug object.



>Index: Bugzilla/Bug.pm

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

"who" is too vague. Probably "applicant" would be better. You could also add a comment which specifies that "applicant" is the user account used to build the bug object.


>+sub add_comment {

>+    $params->{'isprivate'} ||= 0;

Instead of defining $privacyval later, you should validate 'isprivate' here:
  $params->{'isprivate'} = $params->{'isprivate'} ? 1 : 0;


>+    #must be logged in to add a comment
>+    if (! $params->{'whoid'}) {
>+        return;
>+    }

Nit: You could write this in a single line: return unless $params->{'whoid'}. Moreover, as $params->{'whoid'} is used only once, I'm not sure it worths defining this variable. You could use $self->who->id directly.


>+    my $privacyval = $params->{'isprivate'} ? 1 : 0;

As said above, this variable is useless.


>+        my $votedconfirmedbug = new Bugzilla::Bug($id, $who);
>+        $votedconfirmedbug->add_comment("*** This bug has been confirmed" .
>+                                        " by popular vote. ***",
>+                                        {'timestamp' => $timestamp});
>+
>         AppendComment($id, $who,
>                       "*** This bug has been confirmed by popular vote. ***",
>                       0, $timestamp);

Hrm... Remove AppendComment() now that you use add_comment()! :)


Note that there is a small bitrot in Bug.pm. Else the patch looks good.

Comment 15

[victory <[e-mail]>]

Attachment: v5, take #14

Comment 16

[victory <[e-mail]>]

Attachment: update 226011 due to bug 5179

Comment 17

[Frédéric Buclin]

Bugzilla::Bug::CheckIfVotedConfirmed() needs to be fixed in order to use add_comment(), but CheckIfVotedConfirmed() is called in votes.cgi while tables are locked. This means that:
1) you have to lock all missing tables in votes.cgi. I hate to lock extra tables though, this slows down the system. I wrote a patch to remove some useless locked tables and I don't want to bring them back. We keep having regressions when building bug objects while tables are locked.

2) create these bug objects outside the lock. I much prefer this solution.

3) make bug object creation much lighter. This solution is even better!! :)

I strongly prefer solution 3). I would be tempted to r- any patch implementing 1).

Comment 18

[Frédéric Buclin]

Comment on attachment 226038 [details] [diff] [review]
update 226011 due to bug 5179

per my previous comment.

Comment 19

[Frédéric Buclin]

(In reply to comment #17)
> 3) make bug object creation much lighter. This solution is even better!! :)
> 
> I strongly prefer solution 3).

This solution has finally been implemented (or fixed). This means that it's now easy to update this patch.

Comment 20

[Frédéric Buclin]

doesn't block bug 305136.

Comment 21

[Frédéric Buclin]

This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Comment 22

[Max Kanat-Alexander]

We're going to re-purpose this bug slightly, and narrow its focus. We're just going to replace the AppendComment calls in process_bug with a call to $bug->add_comment( ) and then $bug->update(). This will be a simple way to get started on $bug->update and also get rid of some calls to AppendComment.

Comment 23

[Max Kanat-Alexander]

Attachment: v1

Okay, here we are. I tested this on landfill with bug moving, dup'ing, and just adding a normal comment.

It theoretically depends on my patch from bug 371070, but it may not actually require that code in order to work.

Comment 24

[Frédéric Buclin]

Comment on attachment 255850 [details] [diff] [review]
v1

You should kill AppendComment() entirely, IMO, i.e. removing it from attachment.cgi.

r=LpSolit

Comment 25

[Frédéric Buclin]

Comment on attachment 255850 [details] [diff] [review]
v1

>diff -u Bugzilla/Bug.pm Bugzilla/Bug.pm

>+#####################################################################
>+# Mutators 
>+#####################################################################
>+
>+ 	|<-- TAB

Please remove the tab on the last line above:

t/005no_tabs.........NOK 62
#   Failed test 'Bugzilla/Bug.pm contains tabs --WARNING'

Comment 26

[Max Kanat-Alexander]

Oops, I checked this in but somehow didn't commit the checkin message here. Anyhow, here are the revisions:

process_bug.cgi: revision 1.353
Bugzilla/Bug.pm: revision 1.174