590334 - Change Bug.pm to use the comment object (Bugzilla::Comment) when creating or updating bug comments

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[LegNeato]

Bug.pm talks with the database directly rather than going through the comment object.

Attached is a patch that changes this. This makes the code more consistent and benefits my extension by allowing existing hooks to "just work" (tm) for the comment case.

Comment 1

[LegNeato]

Attachment: Use the comment object

Comment 2

[LegNeato]

Attachment: Use the comment object, v2

Comment 3

[LegNeato]

Found a situation where the comment privacy value is not sticking. Canceling review until I fix it.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 468844 [details] [diff] [review]
Use the comment object, v2

Also, you can't call create() within update, because create() can throw errors. Instead, see what I do in run_create_validators with CF_CLASS here:

  http://bzr.mozilla.org/bugzilla/extensions/vcs/trunk/annotate/head:/lib/Commit.pm#L128

Aaaalso, I'm pretty sure that Bugzilla::Comment doesn't have all the required validators yet, so you'll have to write some.

Comment 5

[LegNeato]

Hmmm, ok, there goes my quick patch :-). Are validators necessary for this? It doesn't look like current code validates anything...

Comment 6

[LegNeato]

Nvm, it does. I just need to port the validators out of bug.pm into comment.pm

Comment 7

[LegNeato]

Attachment: Use the comment object, now with verifiers

Not 100% happy with this patch yet...

Comment 8

[Max Kanat-Alexander]

Also, instead of create() inside of Bug::update() and Bug::create, you want to do insert_create_data.

Comment 9

[LegNeato]

Attachment: Use the comment object, v3

Comment 10

[LegNeato]

^^ Update with changes from comment 8

Comment 11

[Max Kanat-Alexander]

Comment on attachment 469189 [details] [diff] [review]
Use the comment object, v3

Okay. Are you ready for review?

You shouldn't be doing $comment->{'field'} = $value. You should be doing $comment->set_field($value). (That also means you have to make the validators work when updating a comment in addition to creating a comment.)

Comment 12

[LegNeato]

Not ready, need to fix that and my TODO first. Thanks for giving me feedback :-)

Comment 13

[Max Kanat-Alexander]

Okay. :-)

Comment 14

[LegNeato]

Attachment: Use the comment object, v4

Ok, review time. Feel free to tear apart more...this is deep-diving in Bugzilla code I am unfamiliar with :-)

Comment 15

[Max Kanat-Alexander]

Comment on attachment 469212 [details] [diff] [review]
Use the comment object, v4

>=== modified file 'Bugzilla/Bug.pm'
>-    my $qmarks = "?," x @columns;
>-    chop($qmarks);
>-    $dbh->do('INSERT INTO longdescs (' . join(',', @columns)  . ")
>-                   VALUES ($qmarks)", undef, @values);
>+
>+    # Validate the coment info to be safe
>+    Bugzilla::Comment->check_required_create_fields($creation_comment);
>+    $creation_comment = Bugzilla::Comment->run_create_validators($creation_comment);

  All Comment data validation needs to be done in Bug::run_create_validators, not in Bug::create.

>@@ -893,22 +897,23 @@
>+        # Other params are already filled out at this point, but we need to
>+        # set bug_when so the timestamp is the same as other changes in the
>+        # update session
>+        $comment->{'bug_when'} = $delta_ts;

  Hmm. I'm not sure that's the best way of doing this, although I'm not certain there's a better way?

>     # Comment Privacy 
>     foreach my $comment_id (keys %{$self->{comment_isprivate} || {}}) {
>-        $dbh->do("UPDATE longdescs SET isprivate = ? WHERE comment_id = ?",
>-                 undef, $self->{comment_isprivate}->{$comment_id}, $comment_id);
>+        my $comment = Bugzilla::Comment->new($comment_id);
>+        $comment->set_is_private($self->{comment_isprivate}->{$comment_id});
>+        $comment->update();

  You can't call set_*anything* inside of update. Instead, what you should do is change how comment_isprivate works--it should store updated comment objects that you call update() on. Perhaps you could update the objects in $self->comments themselves, and store the updated objects somewhere special, and then call update() on them.

> sub add_comment {
>+    # This makes it so we won't create new comments when they are changed

  I think you mean "When there's nothing to add"?

>     if ($comment eq '' && !($params->{type} || abs($params->{work_time}))) {
>         return;
>     }

>+    # Fill out info that doesn't change and callers may not pass in
>+    # bug_when is filled out later so the comment and any other changes 
>+    # all have the same timestamp
>+    $params->{'who'}     = Bugzilla->user->id;

  Actually, _check_who should just always return Bugzilla->user->id, like _check_reporter does for Bug.

  Also, you ideally shouldn't be modifying $params, because that changes stuff in the caller. Perhaps make a copy of it before starting to change things.

>+    $params->{'bug_id'}  = $self->{'bug_id'};

  Hmm. How about we just pass in the whole bug objects, as $params->{'bug'}, like we do for Attachment and other things? Then the validator can check if it got an object or what.

>=== modified file 'Bugzilla/Comment.pm' (properties changed: -x to +x)
>+sub set_body        { $_[0]->set('thetext',    $_[1]); }
>+sub set_bug_id      { $_[0]->set('bug_id',     $_[1]); }
>+sub set_creation_ts { $_[0]->set('bug_when',   $_[1]); }

  These can't change, so don't need mutators.

>+sub set_work_time   { $_[0]->set('work_time',  $_[1]); }
>+sub set_type        { $_[0]->set('type',       $_[1]); }
>+sub set_extra_data  { $_[0]->set('extra_data', $_[1]); }

  Same for all of these, I believe, although perhaps type and extra data do need mutators? If you're not using them in this patch, though, don't add them.

>@@ -206,6 +211,44 @@
>+    # Make sure we have a valid user
>+    unless ($who && Bugzilla::User->new($who)) {
>+        ThrowCodeError('invalid_user', {user_id => $who});
>+    }

  Instead, you should do:

  Bugzilla->login(LOGIN_REQUIRED);
  return Bugzilla->user->id;

  And then the permission checks can be done in _check_bug_id (which you're missing a validator for, by the way).

>+    # Make sure the user can add comments to the bug
>+    my $privs;
>+    Bugzilla::Bug->check_can_change_field('longdesc', 0, 1, \$privs)
>+        || ThrowUserError('illegal_change', 
>+                          { field => 'longdesc', privs => $privs });

  You can't call check_can_change_field on Bugzilla::Bug--it's an object method.

>+sub _check_thetext {
>+    my ($invocant, $thetext) = @_;
>+
>+    $thetext = '' unless defined $thetext;

  Actually, I think trying to create a comment with an undefined thetext should be an error. Probably param_required.

Comment 16

[LegNeato]

(In reply to comment #15)
> Comment on attachment 469212 [details] [diff] [review]
> Use the comment object, v4
> 
> >=== modified file 'Bugzilla/Bug.pm'
> >-    my $qmarks = "?," x @columns;
> >-    chop($qmarks);
> >-    $dbh->do('INSERT INTO longdescs (' . join(',', @columns)  . ")
> >-                   VALUES ($qmarks)", undef, @values);
> >+
> >+    # Validate the coment info to be safe
> >+    Bugzilla::Comment->check_required_create_fields($creation_comment);
> >+    $creation_comment = Bugzilla::Comment->run_create_validators($creation_comment);
> 
>   All Comment data validation needs to be done in Bug::run_create_validators,
> not in Bug::create.

I don't see how I can do that, as I need to pass a bug_id so that comment creation passes validation and that that time the bug isn't created. Thoughts?


> 
> >@@ -893,22 +897,23 @@
> >+        # Other params are already filled out at this point, but we need to
> >+        # set bug_when so the timestamp is the same as other changes in the
> >+        # update session
> >+        $comment->{'bug_when'} = $delta_ts;
> 
>   Hmm. I'm not sure that's the best way of doing this, although I'm not certain
> there's a better way?

I don't think there is a better way. We are using the timestamp in multiple places, and it is best to keep them the same. Maybe another timestamp select? Will that give me the same ts because it is in a transaction?


> 
> >     # Comment Privacy 
> >     foreach my $comment_id (keys %{$self->{comment_isprivate} || {}}) {
> >-        $dbh->do("UPDATE longdescs SET isprivate = ? WHERE comment_id = ?",
> >-                 undef, $self->{comment_isprivate}->{$comment_id}, $comment_id);
> >+        my $comment = Bugzilla::Comment->new($comment_id);
> >+        $comment->set_is_private($self->{comment_isprivate}->{$comment_id});
> >+        $comment->update();
> 
>   You can't call set_*anything* inside of update. Instead, what you should do
> is change how comment_isprivate works--it should store updated comment objects
> that you call update() on. Perhaps you could update the objects in
> $self->comments themselves, and store the updated objects somewhere special,
> and then call update() on them.

Ok, I fixed this by storind updated comment objects in comment_isprivate. All I do is call update() here.


> 
> > sub add_comment {
> >+    # This makes it so we won't create new comments when they are changed
> 
>   I think you mean "When there's nothing to add"?
> 
> >     if ($comment eq '' && !($params->{type} || abs($params->{work_time}))) {
> >         return;
> >     }

Fixed.


> 
> >+    # Fill out info that doesn't change and callers may not pass in
> >+    # bug_when is filled out later so the comment and any other changes 
> >+    # all have the same timestamp
> >+    $params->{'who'}     = Bugzilla->user->id;
> 
>   Actually, _check_who should just always return Bugzilla->user->id, like
> _check_reporter does for Bug.

We don't want to allow programatically setting a comment as written by someone other than the logged in user?


> 
>   Also, you ideally shouldn't be modifying $params, because that changes stuff
> in the caller. Perhaps make a copy of it before starting to change things.
> 

Ok.

> >+    $params->{'bug_id'}  = $self->{'bug_id'};
> 
>   Hmm. How about we just pass in the whole bug objects, as $params->{'bug'},
> like we do for Attachment and other things? Then the validator can check if it
> got an object or what.

I was thinking about that. I may kepe it as-is, not sure.


> 
> >=== modified file 'Bugzilla/Comment.pm' (properties changed: -x to +x)
> >+sub set_body        { $_[0]->set('thetext',    $_[1]); }
> >+sub set_bug_id      { $_[0]->set('bug_id',     $_[1]); }
> >+sub set_creation_ts { $_[0]->set('bug_when',   $_[1]); }
> 
>   These can't change, so don't need mutators.

Yeah, I just figured while I was there to add them.

> 
> >+sub set_work_time   { $_[0]->set('work_time',  $_[1]); }
> >+sub set_type        { $_[0]->set('type',       $_[1]); }
> >+sub set_extra_data  { $_[0]->set('extra_data', $_[1]); }
> 
>   Same for all of these, I believe, although perhaps type and extra data do
> need mutators? If you're not using them in this patch, though, don't add them.

2 were there I think. I just made the style consistent with the others. I'll undo it.

> 
> >@@ -206,6 +211,44 @@
> >+    # Make sure we have a valid user
> >+    unless ($who && Bugzilla::User->new($who)) {
> >+        ThrowCodeError('invalid_user', {user_id => $who});
> >+    }
> 
>   Instead, you should do:
> 
>   Bugzilla->login(LOGIN_REQUIRED);
>   return Bugzilla->user->id;
> 
>   And then the permission checks can be done in _check_bug_id (which you're
> missing a validator for, by the way).
> 
> >+    # Make sure the user can add comments to the bug
> >+    my $privs;
> >+    Bugzilla::Bug->check_can_change_field('longdesc', 0, 1, \$privs)
> >+        || ThrowUserError('illegal_change', 
> >+                          { field => 'longdesc', privs => $privs });
> 
>   You can't call check_can_change_field on Bugzilla::Bug--it's an object
> method.

Whoops!

> 
> >+sub _check_thetext {
> >+    my ($invocant, $thetext) = @_;
> >+
> >+    $thetext = '' unless defined $thetext;
> 
>   Actually, I think trying to create a comment with an undefined thetext should
> be an error. Probably param_required.

I just copied the validator as it was in bug.pm before...will this break assumptions elsewhere?

Comment 17

[LegNeato]

Attachment: Use the comment object, v5

A lot happier with this one. I still allow empty comment text in the comment validation as it was allowed previously (and the creation comment/description is allowed to be empty). Of course, because of the check above comments won't be created unless they have text or other metadata set. Don't know how you could get to that case, but there is already enough change in this patch ;-)

Comment 18

[LegNeato]

Attachment: Use the comment object, v5

Whoops, a x+ prop change snuck in there somehow. Fixed.

Comment 19

[Max Kanat-Alexander]

Comment on attachment 469967 [details] [diff] [review]
Use the comment object, v5

>=== modified file 'Bugzilla/Bug.pm'
>+    my $creation_comment = delete $params->{creation_comment};

  I'd rather just keep the parameter named "comment", if that's all right. You can just replace the text "comment" field with a hashref, in run_create_validators.
  
>@@ -731,6 +722,41 @@
>+    # Create the new comment params so we can check them
> [snip]

  Instead of having all of this code in run_create_validators, what you should do is put it into _check_comment, and have _check_comment return a hashref. You'll also have to put "comment" into VALIDATOR_DEPENDENCIES, so that the appropriate fields that you need will be available and validated within _check_comment.

>+    $params->{creation_comment} = {
>+        who      => Bugzilla->user->id,

  I think you should call _check_who for this.

  In fact, here's a general idea:

  Add an argument to run_create_validators in Bugzilla::Object, { skip => ['field1', 'field2'] }. Then you can call run_create_validators for Comment and skip the validators that you don't want. So the run_create_validators arguments would look something like:

  Bugzilla::Comment->run_create_validators($params, { skip => ['bug_id'} });

>+        type     => CMT_NORMAL,

  CMT_NORMAL is the default, so you shouldn't have to specify it.

>+    # Validate comment. We have to do this special as a comment normally

  specially

>+        if ($varname ne 'bug_id' && $varname ne 'extra_data') {

  Why skip extra_data?

  Also, this is better written as:

  next if ($varname eq 'bug_id' or $varname eq 'extra_data');

>+            $params->{creation_comment}->{$varname} = $comment_validators->{$varname}($class,$unverified, $params->{creation_comment});

  Nit: That line is way too long.

>@@ -893,26 +919,25 @@
>+        # Other params are already filled out at this point, but we need to
>+        # set bug_when so the timestamp is the same as other changes in the
>+        # update session
>+        $comment->{'bug_when'} = $delta_ts;

  Now you've set bug_when during run_create_validators, so you shouldn't need this.

>@@ -2616,23 +2612,25 @@
> sub add_comment {
>+    # Fill out info that doesn't change and callers may not pass in
>+    # bug_when is filled out later so the comment and any other changes 
>+    # all have the same timestamp
>+    $params->{'who'}     = Bugzilla->user->id;

  That you don't need, because the validator will do it for you.

>+    $params->{'bug_id'}  = $self->{'bug_id'};

  You should pass $self instead.

>=== modified file 'Bugzilla/Comment.pm'
>+sub set_work_time   { $_[0]->set('work_time',  $_[1]); }

  It looks like set_work_time is new. work_time is immutable, though. Is there a reason to have this?

>@@ -206,6 +209,61 @@
>+sub _check_bug_id {
>+    my ($invocant, $bug_id) = @_;
>+
>+    # Load the bug from the database
>+    my $bug = Bugzilla::Bug->new($bug_id);

  my $bug;
  if (blessed $bug_id) {
      $bug = $bug_id;
      $bug_id->check_is_visible;
  }
  else {
      $bug = Bugzilla::Bug->check({ id => $bug_id });
  }

>+    # Make sure it was loaded from the database 
> [snip]

  And then that can go away.

>+    # Make sure the user is logged in
>+    Bugzilla->login(LOGIN_REQUIRED);

  That's something you should do in _check_who, and then you make bug_id depend on who in VALIDATOR_DEPENDENCIES.

>+sub _check_thetext {
>+    $thetext = '' unless defined $thetext;

  I do want an error thrown here, because nobody should call Comment->create with an undefined thetext. add_comment already handles the situation for bugs, so this is a situation we should never reach.

  I think you also need to check the "bug_when" field somehow, no?

  And we should probably have a validator for work_time here.

Comment 20

[LegNeato]

(In reply to comment #19)
> Comment on attachment 469967 [details] [diff] [review]
> Use the comment object, v5
> 
> >=== modified file 'Bugzilla/Bug.pm'
> >+    my $creation_comment = delete $params->{creation_comment};
> 
>   I'd rather just keep the parameter named "comment", if that's all right. You
> can just replace the text "comment" field with a hashref, in
> run_create_validators.

I thought it would be clearer that we take values in comment & commentprivacy and construct a creation_comment. I can do it your way though.


> 
> >@@ -731,6 +722,41 @@
> >+    # Create the new comment params so we can check them
> > [snip]
> 
>   Instead of having all of this code in run_create_validators, what you should
> do is put it into _check_comment, and have _check_comment return a hashref.
> You'll also have to put "comment" into VALIDATOR_DEPENDENCIES, so that the
> appropriate fields that you need will be available and validated within
> _check_comment.
> 

I originally had this and then undid it. There seemed to be a lot of manipulation happening in run_create_validators so I thought that was the best place to do it. I can revert back to what I had previously.


> >+    $params->{creation_comment} = {
> >+        who      => Bugzilla->user->id,
> 
>   I think you should call _check_who for this.

I don't want to assume how _check_who behaves here. I think it's better to set what we want which will later be checked/overridden. That way, if for some reason _check_who changes we don't have to remember to come back here and change it.

> 
>   In fact, here's a general idea:
> 
>   Add an argument to run_create_validators in Bugzilla::Object, { skip =>
> ['field1', 'field2'] }. Then you can call run_create_validators for Comment and
> skip the validators that you don't want. So the run_create_validators arguments
> would look something like:
> 
>   Bugzilla::Comment->run_create_validators($params, { skip => ['bug_id'} });

I'll think about this a bit. Again, I was trying to do this patch with minimal impact to other code.

> 
> >+        type     => CMT_NORMAL,
> 
>   CMT_NORMAL is the default, so you shouldn't have to specify it.

Similar to who above, I feel this makes it clearer and doesn't rely on behavior that may change. Of course, down the road we want it to change everywhere this would be an additional place to change.


> 
> >+    # Validate comment. We have to do this special as a comment normally
> 
>   specially
> 
> >+        if ($varname ne 'bug_id' && $varname ne 'extra_data') {
> 
>   Why skip extra_data?

I think I did it because I wasn't passing a type and it depended on it / threw a warning. I'll remove.


> 
>   Also, this is better written as:
> 
>   next if ($varname eq 'bug_id' or $varname eq 'extra_data');
> 
> >+            $params->{creation_comment}->{$varname} = $comment_validators->{$varname}($class,$unverified, $params->{creation_comment});
> 
>   Nit: That line is way too long.

I'll look into breaking it up, though my perl-foo isn't super strong.

> 
> >@@ -893,26 +919,25 @@
> >+        # Other params are already filled out at this point, but we need to
> >+        # set bug_when so the timestamp is the same as other changes in the
> >+        # update session
> >+        $comment->{'bug_when'} = $delta_ts;
> 
>   Now you've set bug_when during run_create_validators, so you shouldn't need
> this.

There are 2 cases, the creation and the update case, correct? I believe this is still needed for the update case but I'll double check.

> 
> >@@ -2616,23 +2612,25 @@
> > sub add_comment {
> >+    # Fill out info that doesn't change and callers may not pass in
> >+    # bug_when is filled out later so the comment and any other changes 
> >+    # all have the same timestamp
> >+    $params->{'who'}     = Bugzilla->user->id;
> 
>   That you don't need, because the validator will do it for you.

See comments about "who" above.

> 
> >+    $params->{'bug_id'}  = $self->{'bug_id'};
> 
>   You should pass $self instead.

Again, I just wanted to match current comment object semantics. The only benefit I see is less database access to look up the bug object in validators.

> 
> >=== modified file 'Bugzilla/Comment.pm'
> >+sub set_work_time   { $_[0]->set('work_time',  $_[1]); }
> 
>   It looks like set_work_time is new. work_time is immutable, though. Is there
> a reason to have this?

Removed.

> 
> >@@ -206,6 +209,61 @@
> >+sub _check_bug_id {
> >+    my ($invocant, $bug_id) = @_;
> >+
> >+    # Load the bug from the database
> >+    my $bug = Bugzilla::Bug->new($bug_id);
> 
>   my $bug;
>   if (blessed $bug_id) {
>       $bug = $bug_id;
>       $bug_id->check_is_visible;
>   }
>   else {
>       $bug = Bugzilla::Bug->check({ id => $bug_id });
>   }
> 
> >+    # Make sure it was loaded from the database 
> > [snip]
> 
>   And then that can go away.
> 
> >+    # Make sure the user is logged in
> >+    Bugzilla->login(LOGIN_REQUIRED);
> 
>   That's something you should do in _check_who, and then you make bug_id depend
> on who in VALIDATOR_DEPENDENCIES.
> 
> >+sub _check_thetext {
> >+    $thetext = '' unless defined $thetext;
> 
>   I do want an error thrown here, because nobody should call Comment->create
> with an undefined thetext. add_comment already handles the situation for bugs,
> so this is a situation we should never reach.

Ok.

> 
>   I think you also need to check the "bug_when" field somehow, no?

Out of scope for this change as there wasn't one before. Will file a follow-up.

> 
>   And we should probably have a validator for work_time here.

Out of scope for this change as there wasn't one before. Will file a follow-up.

Comment 21

[Max Kanat-Alexander]

(In reply to comment #20)
> >   I think you should call _check_who for this.
> 
> I don't want to assume how _check_who behaves here. I think it's better to set
> what we want which will later be checked/overridden. That way, if for some
> reason _check_who changes we don't have to remember to come back here and
> change it.

  That's fine. We never add features that we aren't currently using, and we *will* remember to go back and change this if we ever need it to be different. It's preferable to keep things simpler for the caller and not have to specify Bugzilla->user->id everywhere.

> I'll think about this a bit. Again, I was trying to do this patch with minimal
> impact to other code.

  I understand that, but on trunk, the goal isn't to write minimal patches--it's to write patches that are factored completely correctly, so that we don't have to go back and clean them up again later.

> >   CMT_NORMAL is the default, so you shouldn't have to specify it.
> 
> Similar to who above, I feel this makes it clearer and doesn't rely on behavior
> that may change. Of course, down the road we want it to change everywhere this
> would be an additional place to change.

  That's fine, we should just implement this system for what we have now, not what we might have. It's impossible to know what we might have, so we should always stick with implementing against what we *do* have.

  A few more of my thoughts on this are here:

  http://www.codesimplicity.com/post/designing-too-far-into-the-future/

> > >+    $params->{'bug_id'}  = $self->{'bug_id'};
> > 
> >   You should pass $self instead.
> 
> Again, I just wanted to match current comment object semantics. The only
> benefit I see is less database access to look up the bug object in validators.

  That's a significant benefit.

  Passing the object here is the standard way that we do things.

> >   I think you also need to check the "bug_when" field somehow, no?
> 
> Out of scope for this change as there wasn't one before. Will file a follow-up.

  As long as you're passing a bug_when value to create() or run_create_validators at any point in this patch, it needs to be validated independently by Bugzilla::Comment.

> >   And we should probably have a validator for work_time here.
> 
> Out of scope for this change as there wasn't one before. Will file a follow-up.

  It was being validated by add_comment before, and shouldn't need to be validated by add_comment anymore, although I can see where that would be tricky.

Comment 22

[LegNeato]

Attachment: Use the comment object, v6

Comment 23

[Max Kanat-Alexander]

Comment on attachment 470701 [details] [diff] [review]
Use the comment object, v6

>=== modified file 'Bugzilla/Bug.pm'
>+        creation_ts    => \&_check_creation_ts,

  This is getting too complex for one patch--if you're going to add validators for creation_ts and delta_ts and change the way that Bugzilla::Bug generates them, that should be a separate patch.

>+    # Get the creation timestamp from the database
>+    $params->{creation_ts} = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');

  And this should either be coming from a validator or not.

>@@ -676,20 +668,14 @@
>+    # We now have a bug id so we can fill this out
>+    $creation_comment->{'bug_id'} = $bug->bug_id;

  Nit: Should be $bug->id

>@@ -716,9 +702,7 @@
>+    $params->{delta_ts} = $class->_check_creation_ts();

  At the worst, you should be doing $params->{delta_ts} = $params->{creation_ts}, not calling the validator again.

>@@ -731,6 +715,9 @@
>+    # Don't need this anymore as it is now in the comment hash
>+    delete $params->{commentprivacy};

  You should be doing that inside of check_comment.

>@@ -1398,33 +1380,42 @@
> sub _check_comment {
>+    my ($invocant, $comment_txt, undef, $params) = @_;
>+
>+    # Comment can be empty. We should force it to be empty if the text is undef
>+    $comment_txt ||= '';

  You're also modifying comments of "0" there too, which we want to allow.

>+    my $isprivate = blessed($invocant) ? $invocant->{commentprivacy}

  $invocant is a Bugzilla::Bug, which never has {commentprivacy} as an attribute.

>+    my $timestamp = blessed($invocant) ? $invocant->{creation_ts}
>+                                     : $params->{creation_ts};

  Similar comment there...when are we ever calling _check_comment on an instantiated Bugzilla::Bug, though? (I mean, why even have the blessed check?)

>+        who      => Bugzilla::Comment->_check_who(),
>+        type     => Bugzilla::Comment->_check_type(),

  Bugzilla::Comment->run_create_validators will call those itself, no need to call them directly in the hash.

>+sub _check_creation_ts {
>+    my ($invocant, $timestamp) = @_;
>+
>+    if (!defined $timestamp) {
>+        $timestamp = Bugzilla->dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
>+    }

  You're allowing people to specify anything they want for creation_ts here, even WebService Bug.create callers.

>@@ -2245,8 +2245,9 @@
>+        $self->{comment_isprivate} ||= [];
>+        $comment->set_is_private($isprivate);
>+        push @{$self->{comment_isprivate}}, $comment;

  Perhaps the isprivate updating part should be done in a separate bug, since this patch is getting so big now.

>=== modified file 'Bugzilla/Comment.pm'
>+sub _check_bug_id {
>+    my ($invocant, $bug_id) = @_;
>+
>+    ThrowCodeError('bug_undef') unless $bug_id;

  I think you want param_required there instead.

>+    # Make sure the user can comment
>+    my $privs;
>+    $bug->check_can_change_field('longdesc', 0, 1, \$privs)
>+        || ThrowUserError('illegal_change', 
>+                          { field => 'longdesc', privs => $privs });

  And also check $user->can_edit_product, right?

>+    # Make sure the timestamp is defined and valid
>+    if (!defined $when || !datetime_from($when)) {

  If !datetime_from($when), then we should throw an error.

>+sub _check_work_time {
>+    my ($invocant, $work_time) = @_;
>+    $work_time = 0 unless defined $work_time && $work_time > 0;
>+    return $work_time;

  I think you also need to check Bugzilla->user->is_timetracker here.

>=== modified file 'Bugzilla/Object.pm'
>-    my @sorted_names = $self->_sort_by_dep(keys %field_values);
>+    my @keys = keys %field_values;
>+    my @sorted_names = $self->_sort_by_dep(\@keys);

  That change doesn't look necessary to me. You could just pass [keys %field_values] if you have to pass an arrayref instead of an array for some reason. (Also not too clear on why you'd need to pass an arrayref.)

>@@ -531,10 +535,28 @@
>+    # Make a hash skiplist for easier searching later
>+    my $skiplist = {};
>+    for (@{$options->{skip} || []}) { $skiplist->{$_} = 1 }

  Why did you implement this inside of sort_by_dep? I think it should be inside of run_create_validators instead.

  To clarify, sort_by_dep does not do *mandatory* dependency chaining. (See the comment above it.) So I think that this doesn't need to be here.

>+    # Find the difference between the specified fields to skip and the available
>+    # fields
>+    my $difference = [];
>+    my %count = ();
>+    foreach my $e (@$fields, @{$options->{skip} || []}) { $count{$e}++ }
>+    foreach my $e (keys %count) {
>+        if ($count{$e} < 2) {
>+            push @$difference, $e;
>+        }
>+    }
>+    # The fields to process are the difference between all fields and the fields
>+    # to skip
>+    $fields = $difference;

  This is really complicated. Why not just skip validating the field in run_create_validators if it's in the skip hash?

>=== modified file 'template/en/default/global/code-error.html.tmpl'
>+  [% ELSIF error == "comment_text_undef" %]
>+    Comment text cannot be undefined.

  Probably, instead of this, you should just use param_required in _check_thetext.

Comment 24

[Max Kanat-Alexander]

For simplicity's sake, how about we limit this bug to fixing only Bugzilla::Bug::update's adding of comments, and do Bugzilla::Bug::create and set_isprivate in separate patches?

Comment 25

[LegNeato]

(In reply to comment #23)
> Comment on attachment 470701 [details] [diff] [review]
> Use the comment object, v6
> 
> >=== modified file 'Bugzilla/Bug.pm'
> >+        creation_ts    => \&_check_creation_ts,
> 
>   This is getting too complex for one patch--if you're going to add validators
> for creation_ts and delta_ts and change the way that Bugzilla::Bug generates
> them, that should be a separate patch.

The only reason I created the validator was so that the comment validator could rely on it.

> 
> >+    # Get the creation timestamp from the database
> >+    $params->{creation_ts} = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
> 
>   And this should either be coming from a validator or not.

Removed it entirely.

> 
> >@@ -676,20 +668,14 @@
> >+    # We now have a bug id so we can fill this out
> >+    $creation_comment->{'bug_id'} = $bug->bug_id;
> 
>   Nit: Should be $bug->id

Fixed.

> 
> >@@ -716,9 +702,7 @@
> >+    $params->{delta_ts} = $class->_check_creation_ts();
> 
>   At the worst, you should be doing $params->{delta_ts} =
> $params->{creation_ts}, not calling the validator again.

Set it equal to $params->{creation_ts}

> 
> >@@ -731,6 +715,9 @@
> >+    # Don't need this anymore as it is now in the comment hash
> >+    delete $params->{commentprivacy};
> 
>   You should be doing that inside of check_comment.

Done.

> 
> >@@ -1398,33 +1380,42 @@
> > sub _check_comment {
> >+    my ($invocant, $comment_txt, undef, $params) = @_;
> >+
> >+    # Comment can be empty. We should force it to be empty if the text is undef
> >+    $comment_txt ||= '';
> 
>   You're also modifying comments of "0" there too, which we want to allow.

Switched it to a !defined check.

> 
> >+    my $isprivate = blessed($invocant) ? $invocant->{commentprivacy}
> 
>   $invocant is a Bugzilla::Bug, which never has {commentprivacy} as an
> attribute.
> 
> >+    my $timestamp = blessed($invocant) ? $invocant->{creation_ts}
> >+                                     : $params->{creation_ts};
> 
>   Similar comment there...when are we ever calling _check_comment on an
> instantiated Bugzilla::Bug, though? (I mean, why even have the blessed check?)
> 

Removed. I had just blindly copied _check_component which is obviously wrong.

> >+        who      => Bugzilla::Comment->_check_who(),
> >+        type     => Bugzilla::Comment->_check_type(),
> 
>   Bugzilla::Comment->run_create_validators will call those itself, no need to
> call them directly in the hash.

Ok, removed. I was trying to be explicit/have less "magic" happen.

> 
> >+sub _check_creation_ts {
> >+    my ($invocant, $timestamp) = @_;
> >+
> >+    if (!defined $timestamp) {
> >+        $timestamp = Bugzilla->dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
> >+    }
> 
>   You're allowing people to specify anything they want for creation_ts here,
> even WebService Bug.create callers.

That's not good ha. I thought we had to handle the update case and didn't want to be creating a new cration_ts every time, but because that field is not in the update fields it won't happen.

> 
> >@@ -2245,8 +2245,9 @@
> >+        $self->{comment_isprivate} ||= [];
> >+        $comment->set_is_private($isprivate);
> >+        push @{$self->{comment_isprivate}}, $comment;
> 
>   Perhaps the isprivate updating part should be done in a separate bug, since
> this patch is getting so big now.

Is there anything wrong with this? It seems to work in all my testing.

> 
> >=== modified file 'Bugzilla/Comment.pm'
> >+sub _check_bug_id {
> >+    my ($invocant, $bug_id) = @_;
> >+
> >+    ThrowCodeError('bug_undef') unless $bug_id;
> 
>   I think you want param_required there instead.

Ok, I had some reason for adding this but I can't remember now...fixed.

> 
> >+    # Make sure the user can comment
> >+    my $privs;
> >+    $bug->check_can_change_field('longdesc', 0, 1, \$privs)
> >+        || ThrowUserError('illegal_change', 
> >+                          { field => 'longdesc', privs => $privs });
> 
>   And also check $user->can_edit_product, right?

The check wasn't there before, but I have now added it.

> 
> >+    # Make sure the timestamp is defined and valid
> >+    if (!defined $when || !datetime_from($when)) {
> 
>   If !datetime_from($when), then we should throw an error.

Ok, I now throw a code error. Is it ok to let people pass in their own timestamp here? Do we ever need to allow code (or users I guess) to pass an arbitrary value?

> 
> >+sub _check_work_time {
> >+    my ($invocant, $work_time) = @_;
> >+    $work_time = 0 unless defined $work_time && $work_time > 0;
> >+    return $work_time;
> 
>   I think you also need to check Bugzilla->user->is_timetracker here.

Ok, added. Again, this wasn't really there originally, I just copied and pasted (and added some more checks to make >= 0).

> 
> >=== modified file 'Bugzilla/Object.pm'
> >-    my @sorted_names = $self->_sort_by_dep(keys %field_values);
> >+    my @keys = keys %field_values;
> >+    my @sorted_names = $self->_sort_by_dep(\@keys);
> 
>   That change doesn't look necessary to me. You could just pass [keys
> %field_values] if you have to pass an arrayref instead of an array for some
> reason. (Also not too clear on why you'd need to pass an arrayref.)

This was me being a perl n00b. Fixed by going a different way.

> 
> >@@ -531,10 +535,28 @@
> >+    # Make a hash skiplist for easier searching later
> >+    my $skiplist = {};
> >+    for (@{$options->{skip} || []}) { $skiplist->{$_} = 1 }
> 
>   Why did you implement this inside of sort_by_dep? I think it should be inside
> of run_create_validators instead.

Errr, hmm. Ok, moved to run_create_validators.

> 
>   To clarify, sort_by_dep does not do *mandatory* dependency chaining. (See the
> comment above it.) So I think that this doesn't need to be here.
> 
> >+    # Find the difference between the specified fields to skip and the available
> >+    # fields
> >+    my $difference = [];
> >+    my %count = ();
> >+    foreach my $e (@$fields, @{$options->{skip} || []}) { $count{$e}++ }
> >+    foreach my $e (keys %count) {
> >+        if ($count{$e} < 2) {
> >+            push @$difference, $e;
> >+        }
> >+    }
> >+    # The fields to process are the difference between all fields and the fields
> >+    # to skip
> >+    $fields = $difference;
> 
>   This is really complicated. Why not just skip validating the field in
> run_create_validators if it's in the skip hash?

Ended up doing this.

> 
> >=== modified file 'template/en/default/global/code-error.html.tmpl'
> >+  [% ELSIF error == "comment_text_undef" %]
> >+    Comment text cannot be undefined.
> 
>   Probably, instead of this, you should just use param_required in
> _check_thetext.

Fixed.

Comment 26

[LegNeato]

Attachment: Use the comment object, v7

Cleaned up patch with latest feedback. Let me know if you are tired of this review ping-pong and I can try to not bug you as much. This works in my testing, though I understand we want the code up to a certain level of quality.

Comment 27

[LegNeato]

Comment on attachment 477342 [details] [diff] [review]
Use the comment object, v7

Whoops, missed diffing Comment.pm. New patch coming up

Comment 28

[LegNeato]

Attachment: Use the comment object, v7+

Comment 29

[LegNeato]

FYI, I have been testing this patch on landfill (https://landfill.bugzilla.org/push/)

Comment 30

[Max Kanat-Alexander]

Comment on attachment 477349 [details] [diff] [review]
Use the comment object, v7+

>=== modified file 'Bugzilla/Bug.pm'
>+    # Get the creation timestamp from the database
>+    $params->{creation_ts} = $class->_check_creation_ts();

  There's no need to do that manually. The validator will always be called, because the database column is NOT NULL.

>+sub _check_creation_ts {
>+    my ($invocant) = @_;
>+    return Bugzilla->dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');

  Nit: Might as well get rid of the "my ($invocant)" line, since we don't use it.

>=== modified file 'Bugzilla/Comment.pm'
>+sub _check_bug_id {
>+    my ($invocant, $bug_id) = @_;
>+
>+    ThrowCodeError('param_requred',
>+                   {
>+                       function => '_check_bug_id',
>+                       param    => 'bug_id'
>+                   }) unless $bug_id;

  Nit: The spacing there isn't normally how we would space that. Also, "function" should contain ${class} or some indication of which _check_bug_id is being called.

>+    return $bug->bug_id;

  Nit: I generally prefer "$bug->id".

>+sub _check_work_time {

  To some degree, this duplicates Bugzilla::Bug::_check_work_time. The two functions should be consolidated.

>+    my ($invocant, $work_time) = @_;
>+
>+    # Make sure if the user is setting it they are a timetracker
>+    Bugzilla->login(LOGIN_REQUIRED);

  Just make work_time dependent upon who, instead.

>+    if ( !Bugzilla->user->is_timetracker() ) {
>+        ThrowUserError('timetracker_required');
>+    }

  work_time is NOT NULL, so this validator will *always* be called. That means that instead of throwing an error, we need to return 0 for non-timetrackers.

>+sub _check_thetext {
>+    my ($invocant, $thetext) = @_;
>+
>+    ThrowCodeError('param_required',
>+                   {
>+                       function => '_check_thetext',
>+                       param    => 'thetext',
>+                   }) unless defined $thetext;

  Nit: Spacing, needs ${class} in function or something like that.

  Oh, also, I just realized--function should be "Bugzilla::Comment->create", not "_check_thetext". (Same for the other param_required that I commented on, too.)

>=== modified file 'Bugzilla/Object.pm'
> sub run_create_validators {
>+    $options ||= {};

  Thanks to autovivification, I think you don't need that line.

>+    my $skiplist = {};
>+    for (@{$options->{skip} || []}) { $skiplist->{$_} = 1 }

  Actually, what you want there is:

  my %skip_list = map { $_ => 1 } @{ $options->{skip} || [] };

>     my @sorted_names = $class->_sort_by_dep(keys %field_values);
>-    foreach my $field (@sorted_names) {
>+    foreach my $field (grep { !$skiplist->{$_} } @sorted_names) {

  Instead of doing the grep inside the foreach, do it above the loop structure, for clarity's sake.


  Overall, this patch is looking really good! I think that once all of the above is fixed, this will pass review. :-)

Comment 31

[LegNeato]

Attachment: Use the comment object, v8

Ok, I addressed all the issues in the previous release. A couple of things:
1. creation_ts wasn't NOTNULL, which is why I needed that check call. I have patched the underlying issue in this patch. Let me know if you want it in a different bug, but the fix was trivial
2. I moved the time stuff down into Object.pm and now both Bug.pm and Comment.pm use it

Comment 32

[Max Kanat-Alexander]

Comment on attachment 480304 [details] [diff] [review]
Use the comment object, v8

Oh, right. creation_ts is nullable on purpose. See the code in create() for an explanation.

Instead, you need to add it to EXTRA_REQUIRED_FIELDS.

Comment 33

[LegNeato]

Attachment: Use the comment object, v9

Yeah, I did that first and then decided to go "fix" it in the db. I should have looked around rather than assuming it was a bug.

Comment 34

[Max Kanat-Alexander]

Comment on attachment 480340 [details] [diff] [review]
Use the comment object, v9

>=== modified file 'Bugzilla/Object.pm'
>+sub check_time {
>+    my ($invocant, $value_in, $field, $params, $allow_negative) = @_;
>+
>+    # If we don't have a current value default to zero
>+    my $current = blessed($invocant) ? $invocant->{$field}
>+                                     : 0;
>+
>+    # Don't let the user set the value if they aren't a timetracker
>+    return $current unless Bugzilla->user->is_timetracker;

  That will return undef for work_time when the user is not a timetracker.

>+    # Make sure the new value is well formed
>+    ValidateTime($value_in, $field, $allow_negative);

  If this is only used inside of this function, you might as well just put its code here.

  Otherwise, it should become a private, standardly-named function, like _validate_time.

>+    return $value_in;

  Just call it $value if you're also going to return it with the same name.

Comment 35

[Max Kanat-Alexander]

Looks like ValidateTime is also used in importxml.pl though, by the way.

Comment 36

[LegNeato]

Attachment: Use the comment object, v10

Fix up w/ changes from review. Add importxml.pl changes. I grepped around and didn't see any other uses of ValidateTime to change.

Comment 37

[LegNeato]

Attachment: Use the comment object, v10+

Fix up errors found while testing...

Comment 38

[Max Kanat-Alexander]

Comment on attachment 480757 [details] [diff] [review]
Use the comment object, v10+

This looks good. Thank you quite a bit for this, Christian! :-)

There's some duplicate between Bugzilla::Bug::_check_datetime_field and Bugzilla::Comment::_check_bug_when that should be cleaned up in another bug.

Comment 39

[LegNeato]

Woo-hoo! Thanks for the patience and reviews :-)

Comment 40

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified importxml.pl
modified Bugzilla/Bug.pm
modified Bugzilla/Comment.pm
modified Bugzilla/Object.pm
modified template/en/default/global/code-error.html.tmpl
Committed revision 7535.