Closed Bug 355847 Opened 14 years ago Closed 12 years ago

Make the WebService able to add a comment to a bug

Categories

(Bugzilla :: WebService, enhancement)

2.23
enhancement
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 3.2

People

(Reporter: mkanat, Assigned: tsahi_75)

References

Details

Attachments

(3 files, 9 obsolete files)

2.62 KB, patch
mkanat
: review+
Details | Diff | Splinter Review
3.47 KB, patch
mkanat
: review+
Details | Diff | Splinter Review
5.97 KB, patch
Details | Diff | Splinter Review
The most common modification that an external application wants to do to a bug is to add a comment to it. (For example, CVS integration scripts often add a comment when a checkin is made.)
*** Bug 357492 has been marked as a duplicate of this bug. ***
(In reply to comment #1)
> *** Bug 357492 has been marked as a duplicate of this bug. ***
> 

Hmm. That can teach me that the simple search is apparently case sensitive... ;-)
This patch tries (hard) to add a sensible "append_comment" and "set_resolution" to WebService/Bug.pm. I think it fails because the access control needed is not clear to me. 

I would appreciate a review to steer me in the right direction. I am aware that no POD is present - this will follow as the patch improves.
Attachment #243829 - Flags: review?(mkanat)
Comment on attachment 243829 [details] [diff] [review]
Add append_comment and set_resolution to WebService/Bug.pm

  Remove set_resolution from this patch--that's a separate bug, and we won't be taking it for 3.0.

>+# Append a comment to a bug.
>+sub append_comment {
>+    # Get the params.
>+    my ( $self, $params ) = @_;

  Nit: Our usual style is to not have the extra spaces there in the parens.

>+    # non-readonly functions must check parameters. I reckon.
>+    my $bug_id = $params->{bug_id}; 
>+    defined $bug_id 
>+        || ThrowCodeError('param_required', { param => 'bug_id' });
>+    trick_taint($bug_id);

  Use ValidateBugID for this. That checks all the permissions and so forth.

  You also want to call $bug->check_can_change_field for the 'comment' field. It should always return "true," but somebody might have customized their installation.

>+    my $private = $params->{private}; 
>+    defined $private 
>+        || ThrowCodeError('param_required', { param => 'private' });
>+    trick_taint($private);

  That shouldn't be required.

>+    my $timestamp = $params->{timestamp}; 
>+    trick_taint($timestamp);

  You don't need to call trick_taint in this function at all. AppendComment will do that for you.

>+    my $worktime = $params->{worktime};
>+    trick_taint($worktime);

  You don't need to do that. AppendComment will validate that for you.

>+    # We need login for this to work, I believe?
>+    Bugzilla->login;

  Nope.

>+    ValidateComment($comment);

  You should do this above.

>+    # TODO: How to make sure, that the user is actually allowed to modify
>+    # this product?!
>+    # A try:
>+    my $bug = new Bugzilla::Bug($bug_id);
>+    Bugzilla->user->can_edit_product($bug->product_id)
>+        || ThrowUserError("product_access_denied",
>+                          { product => $bug->product });

  That looks right to me. ValidateBugID should handle everything else.

>+    product_access_denied       => 103,
>+    

  In the POD, document that as "Bug Edit Denied." 102 should be "access denied" and 103 should be "edit denied."
Attachment #243829 - Flags: review?(mkanat) → review-
This a new patch to add "append_comment" to the WebService interface.

I have tried to incoperate the comments from the review of the previous patch, however, I did not fully understand the comments about the error codes - so, this is a stab at it...

Includes POD. Passes my tests. No patch for bz_webservice_demo.pl, I am afraid.
Attachment #243829 - Attachment is obsolete: true
Attachment #245406 - Flags: review?(mkanat)
Comment on attachment 245406 [details] [diff] [review]
Add append_comment to WebService/Bug.pm

>+# Append a comment to a bug.
>+sub append_comment {
>+    # Get the params.
>+    my ($self, $params) = @_;
>+    
>+    # Check parameters
>+    ValidateBugID($params->{id});

  You should throw param_required if the id is undefined.

>+    # Append comment
>+    AppendComment($params->{id}, Bugzilla->user->id, $comment,
>+                  $params->{private}, 
>+                  $params->{timestamp}, $params->{worktime});

  I don't think we should allow timestamp as a parameter to the function. People shouldn't be able to append out-of-sequence comments using the webservice interface.

  Also, isprivate needs to be validated -- just a "? 1 : 0" after it will do it.
Attachment #245406 - Flags: review?(mkanat) → review-
Changed as per reviewers instructions.

Two additional things added:
- A Bugzilla->login call, to be removed when bug 358354 is fixed
- A call to  Bugzilla::BugMail::Send($bug->bug_id, { changer => Bugzilla->user->login });. This is _untested_ as I do not have a mail sending bugzilla installation to test on :-(
Attachment #245406 - Attachment is obsolete: true
Attachment #245546 - Flags: review?(mkanat)
Component: Bugzilla-General → WebService
Assignee: general → webservice
Since I have written a patch for this, I reckoned I might as well assign it to myself.

Status: NEW → ASSIGNED
Isn't it too late for 3.0?
Assignee: webservice → mbd
Status: ASSIGNED → NEW
(In reply to comment #9)
> Isn't it too late for 3.0?

  Yes.
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2
Comment on attachment 245546 [details] [diff] [review]
Add append_comment to WebService/Bug.pm

This should use Bug->add_comment and Bug->update instead. (I forget if it's available yet.)
Attachment #245546 - Flags: review?(mkanat) → review-
Duplicate of this bug: 397577
what's the status of this bug? does it need help?
(In reply to comment #13)
> what's the status of this bug? does it need help?

  Yeah, Mads isn't around to work on this anymore, so somebody needs to take it over.
Assignee: mbd → webservice
(In reply to comment #14)
> (In reply to comment #13)
> > what's the status of this bug? does it need help?
> 
>   Yeah, Mads isn't around to work on this anymore, so somebody needs to take it
> over.
> 

Yeah, sorry about that. The focus of my company changed somewhat, and I simply can not seem to find the time to fix the things Max have pointed out. I keep trying and failing. 

The good news is, that it actually should be fairly easy to do the work - I am just to hard pressed for time currently and has been for a while, will be for at while.
Attached patch add append_comment to Bug.pm (obsolete) — Splinter Review
this is my first ever real perl code, so please bear with me.
this patch uses $bug->add_comment, and also calls $bug->update(), i hope it's ok.

it worked for me, on win32, with XML-RPC.NET library (and Bugzproxy on top of it).
Attachment #287448 - Flags: review?(mkanat)
Comment on attachment 287448 [details] [diff] [review]
add append_comment to Bug.pm

>Index: Bugzilla/WebService/Bug.pm
>+    ValidateComment($comment);

  You don't need to do that, add_comment will do it for you.

>+    # Check user allowed.
>+    my $PrivilegesRequired;
>+    my $bug = new Bugzilla::Bug($params->{id});

  Use Bugzilla::Bug::check() instead, which will throw an error if they can't access it or if it doesn't exist.

>+    $bug->check_can_change_field("longdesc", '', $comment, 
>+                                 \$PrivilegesRequired, undef) 
>+        || ThrowUserError("bug_access_denied", 
>+                          {bug_id => $params->{id}});

  Once again, add_comment is doing that for you.

>+    Bugzilla->user->can_edit_product($bug->product_id)
>+        || ThrowUserError("product_access_denied",
>+                          {product => $bug->product});

  Once you use check(), it will do this for you.

>+    # Append comment
>+    $bug->add_comment($comment, {isprivate=>($params->{private} ? 1 : 0),

  You don't have to do the ? 1 : 0, add_comment handles that for you.

>+                      work_time=>$params->{worktime}, type => CMT_NORMAL});

  The param they pass should be work_time, not worktime.

  You don't have to specify type, because CMT_NORMAL is the default.

>@@ -467,5 +504,49 @@
>+=item C<private> (string) - If set to true, the comment is private, otherwise it is assumed to be public.

  That's an int, not a string.

>+=item C<worktime> (int) - If set, this is used for the comments worktime, otherwise 0 is used.

  You should note what will happen if the user isn't in the time-tracking group.

>+You did not have the neccessary rights to edit the bug.

  Nit: necessary

  Otherwise it's a great job, for your first Perl! :-)
Attachment #287448 - Flags: review?(mkanat) → review-
(In reply to comment #17)
> (From update of attachment 287448 [details] [diff] [review])
> >+    # Check user allowed.
> >+    my $PrivilegesRequired;
> >+    my $bug = new Bugzilla::Bug($params->{id});
> 
>   Use Bugzilla::Bug::check() instead, which will throw an error if they can't
> access it or if it doesn't exist.

I didn't see a check() function in Bugzilla::Bug. even so, I still need to create a Bug object, don't I?


> 
> >+=item C<worktime> (int) - If set, this is used for the comments worktime, otherwise 0 is used.
> 
>   You should note what will happen if the user isn't in the time-tracking
> group.

I have hard time figuring that out. All I could make out is that 0 will be returned, maybe. I've set timetrackinggroup to nothing (empty), but i was still able to post a comment with work_time value (although the time was not added when i re-enabled time tracking for admin group).

(In reply to comment #18)
> I didn't see a check() function in Bugzilla::Bug. even so, I still need to
> create a Bug object, don't I?

  check() is inherited from Bugzilla::Object.

> I have hard time figuring that out. All I could make out is that 0 will be
> returned, maybe. I've set timetrackinggroup to nothing (empty), but i was still
> able to post a comment with work_time value (although the time was not added
> when i re-enabled time tracking for admin group).

  Okay. You should say that the value will be ignored if the user isn't in the time-tracking group, then.
Attached patch yet another attempt (obsolete) — Splinter Review
per IRC chat with mkanat, i use new and not check. this requires calling can_edit_product.
also fixed the doc so the work time parameter is work_time instead of worktime.
Attachment #287448 - Attachment is obsolete: true
You have to request review from someone if you want a reviewer to look at your patch.
Comment on attachment 287924 [details] [diff] [review]
yet another attempt

on IRC, Max said it looks like check() doesn't work well, so i used the new() ctor instead. but i understood that he wants to fix it.
Attachment #287924 - Flags: review?(mkanat)
Comment on attachment 287924 [details] [diff] [review]
yet another attempt

This looks good to me. Does it look OK to you, LpSolit?
Attachment #287924 - Flags: review?(mkanat)
Attachment #287924 - Flags: review?(LpSolit)
Attachment #287924 - Flags: review+
Comment on attachment 287924 [details] [diff] [review]
yet another attempt

>+# Append a comment to a bug.
>+sub append_comment {

Nit: why do we call this method append_comment() when we already have an add_comment() in Bugzilla::Bug? This is so confusing. We should rename it to add_comment() too, IMO.


>+    Bugzilla->user->can_edit_product($bug->product_id)
>+        || ThrowUserError("product_access_denied", {product => $bug->product});

That's the wrong error tag. This one is used with can_see_product(). You must use product_edit_denied here.


>+    $bug->add_comment($comment, {isprivate=>$params->{private},
>+                      work_time=>$params->{work_time}});
>+    $bug->update();

I have been able to add comments without passing --login and --password, and the longdescs table is now full of comments with who = 0. Maybe unrelated to this bug, but $bug->update should forbid any change if $user->id = 0!

Nit: also, please add a whitespace before and after =>.


>+    return undef;

Nit: why undef instead of e.g. 1?


>+=item C<append_comment> B<EXPERIMENTAL>

Nit: why experimental?


>+=item C<id> (int) B<Required> - The id of the bug to append a comment to.

Nit: could we write (integer) instead of (int)? It took me some time to understand it.


>+=item 102 (Bug Edit Denied)

Error code 102 is for bug_access_denied, i.e. you cannot *access/view* the bug, but I think we should also add one for product_edit_denied, i.e. you cannot *edit* the bug, no?


Else your patch is working fine. It would be cool to add it to contrib/bz_webservice_demo.pl too.
Attachment #287924 - Flags: review?(LpSolit) → review-
(In reply to comment #24)
> Nit: why do we call this method append_comment() when we already have an
> add_comment() in Bugzilla::Bug? This is so confusing. We should rename it to
> add_comment() too, IMO.

  Agreed! I didn't notice that.

> >+    return undef;
> 
> Nit: why undef instead of e.g. 1?

  All modifiers should return undef. That's the equivalent of "returns nothing".

> Nit: why experimental?

  You think it should be UNSTABLE instead?

> Nit: could we write (integer) instead of (int)? It took me some time to
> understand it.

  I think most programmers know what int means...also, I think that's what the XML-RPC spec says, but maybe not.
> Error code 102 is for bug_access_denied, i.e. you cannot *access/view* the bug,
> but I think we should also add one for product_edit_denied, i.e. you cannot
> *edit* the bug, no?

  Agreed, we should have a special mapping for that in Bugzilla::WebService::Constants.
Comment on attachment 287924 [details] [diff] [review]
yet another attempt

The function needs to call Bugzilla->login(LOGIN_REQUIRED) to make sure somebody is logged in before changing anything.
Attachment #287924 - Flags: review+ → review-
(In reply to comment #25)
>   All modifiers should return undef. That's the equivalent of "returns
> nothing".

OK, then.


>   You think it should be UNSTABLE instead?

Ah no, I prefer EXPERIMENTAL in this case. But I thought it shouldn't change too much in the future, so maybe STABLE? :)
Assignee: webservice → tsahi_75
(In reply to comment #27)
> Ah no, I prefer EXPERIMENTAL in this case. But I thought it shouldn't change
> too much in the future, so maybe STABLE? :)

  Yeah, I had that thought too, but I think we should wait for the Release Candidate to mark anything STABLE.
(In reply to comment #25)
> (In reply to comment #24)
> 
> > Nit: could we write (integer) instead of (int)? It took me some time to
> > understand it.
> 
>   I think most programmers know what int means...also, I think that's what the
> XML-RPC spec says, but maybe not.

indeed, the xml-rpc spec specifies <int> or <i4> to mark a 4 byte integer.http://www.xmlrpc.com/spec
(In reply to comment #24)
> 
> >+    $bug->add_comment($comment, {isprivate=>$params->{private},
> >+                      work_time=>$params->{work_time}});
> >+    $bug->update();
> 
> I have been able to add comments without passing --login and --password, and
> the longdescs table is now full of comments with who = 0. Maybe unrelated to
> this bug, but $bug->update should forbid any change if $user->id = 0!
> 

i've been trying to add a comment without logging in using Bugzproxy, and i keep getting disconnected by the server. it works only when i do a login first. how do you test it?

> 
> >+=item 102 (Bug Edit Denied)
> 
> Error code 102 is for bug_access_denied, i.e. you cannot *access/view* the bug,
> but I think we should also add one for product_edit_denied, i.e. you cannot
> *edit* the bug, no?
> 
> 
should i wait with the patch until you add this?
Status: NEW → ASSIGNED
(In reply to comment #30)
> i've been trying to add a comment without logging in using Bugzproxy, and i
> keep getting disconnected by the server. it works only when i do a login first.
> how do you test it?

Using contrib/bz_webservice_demo.pl and hacked to call Bug.add_comment.


> should i wait with the patch until you add this?

No. You should add it yourself as part of your bug.
should i add this error code (106, i assume) to the possible error codes in the doc for the function?
(In reply to comment #32)
> should i add this error code (106, i assume) to the possible error codes in the
> doc for the function?

  Yes.
Attached patch add_comment v6 (obsolete) — Splinter Review
addressing recent comments
Attachment #292118 - Flags: review?(mkanat)
Comment on attachment 292118 [details] [diff] [review]
add_comment v6

Ah, my error. product_edit_denied should be 108, not 106. It needs to be its own new error.
Attachment #292118 - Flags: review?(mkanat) → review-
i hope the comment i added about the required --login and --password parameters is correct. i don't know how would you authenticate with cookies here.

in an unrelated change, i also changed the quotes in the --rememberlogin parameter help from “Bugzilla_remember” to "Bugzilla_remember", because in the generated HTML (http://www.bugzilla.org/docs/tip/html/api/contrib/bz_webservice_demo.html) it turns into “Bugzilla_rememberâ€�, perhaps because of the ANSI to Unicode conversion.
Attachment #292124 - Flags: review?(mkanat)
Attached patch error code 108 (obsolete) — Splinter Review
Attachment #287924 - Attachment is obsolete: true
Attachment #292118 - Attachment is obsolete: true
Attachment #292125 - Flags: review?(mkanat)
(In reply to comment #36)
> in an unrelated change, i also changed the quotes in the --rememberlogin
> parameter help from “Bugzilla_remember” to "Bugzilla_remember", because in
> the generated HTML
> (http://www.bugzilla.org/docs/tip/html/api/contrib/bz_webservice_demo.html) it
> turns into “Bugzilla_remember�, perhaps because of the ANSI to
> Unicode conversion.

What you're putting your finger on here is a bug in HTML generation. The original contrib/bz_webservice_demo.pl file is encoded in UTF-8 all right, so we should expect the HTML to be all right. What I'm seeing in the generated HTML code, though, is &#226;&#128;&#156;Bugzilla_remember&#226;&#128;&#157;. This suggests that the HTML generator assumes its source to be ANSI, while it is UTF-8. So, rather than work around this by restricting ourselves to ANSI or ASCII or something, we should rather fix HTML generation and keep multi-byte characters in place in this file. (With it, we even have a non-critical means to see whether we're doing it all right.)

Just my ,02 €.
Perl sources are always interpreted as ASCII without a "use encoding" or "use utf8" statement in them, which we shouldn't have. So, all our source should be ASCII.
Comment on attachment 292125 [details] [diff] [review]
error code 108

>Index: Bugzilla/WebService/Bug.pm
>+sub add_comment {
> [snip]
>+    Bugzilla->login(LOGIN_REQUIRED);

  That needs to happen at the top of the method, not down here.

>+=item C<work_time> (int) - If set, this is used for the comments work-time, otherwise 0
>+is used. If you are not in the time tracking group, this value will be ignored.

  This is not an int, it's a <double>. It can be a decimal number.

>Index: Bugzilla/WebService/Constants.pm
>@@ -72,6 +72,7 @@
>     no_products         => 106,
>     entry_access_denied => 106,
>     product_access_denied => 106,
>+    product_edit_denied => 108,
>     product_disabled    => 106,
>     # Invalid Summary
>     require_summary => 107,

  That needs to come at the end of the list, not in the middle. 108 comes after 107.
Attachment #292125 - Flags: review?(mkanat) → review-
Comment on attachment 292125 [details] [diff] [review]
error code 108

>Index: Bugzilla/WebService/Bug.pm
>+=item C<private> (int) - If set to true (non-zero), the comment is private, otherwise
>+it is assumed to be public.

  And that is a <boolean>, not an <int>.
(In reply to comment #40)
> (From update of attachment 292125 [details] [diff] [review])
> >Index: Bugzilla/WebService/Constants.pm
> >@@ -72,6 +72,7 @@
> >     no_products         => 106,
> >     entry_access_denied => 106,
> >     product_access_denied => 106,
> >+    product_edit_denied => 108,
> >     product_disabled    => 106,
> >     # Invalid Summary
> >     require_summary => 107,
> 
>   That needs to come at the end of the list, not in the middle. 108 comes after
> 107.
> 

wouldn't that be confusing with the # Invalid Summary comment?
after reading the docs for Getopt::Long, i fixed some parameters values, as well as a typo and the value type for --worktime.
Attachment #292124 - Attachment is obsolete: true
Attachment #294374 - Flags: review?(mkanat)
Attachment #292124 - Flags: review?(mkanat)
(In reply to comment #42)
> wouldn't that be confusing with the # Invalid Summary comment?

  Obviously you need to add a new comment.
Attached patch add_comment patch v8 (obsolete) — Splinter Review
with required changes.
Attachment #292125 - Attachment is obsolete: true
Attachment #294409 - Flags: review?(mkanat)
Comment on attachment 294409 [details] [diff] [review]
add_comment patch v8

This has some spacing issues that need to be fixed on checkin, but it looks good.
Attachment #294409 - Flags: review?(mkanat) → review+
Here are the whitespace and POD fixes that needed to be made before checkin.

I haven't tested this yet, but I'm pretty sure it will work, so I'm going to grant approval now, and test it and check it in later.
Attachment #245546 - Attachment is obsolete: true
Attachment #294409 - Attachment is obsolete: true
Attachment #294452 - Flags: review+
Pushing this patch into our radar.
Flags: approval?
Comment on attachment 294374 [details] [diff] [review]
bz_webservice_demo.pl v2

This looks fine, too.
Attachment #294374 - Flags: review?(mkanat) → review+
Okay, I tested it and it works fine.
Flags: approval? → approval+
Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.8; previous revision: 1.7
done
Checking in Bugzilla/WebService/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Constants.pm,v  <--  Constants.pm
new revision: 1.12; previous revision: 1.11
done
Checking in contrib/bz_webservice_demo.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/bz_webservice_demo.pl,v  <--  bz_webservice_demo.pl
new revision: 1.12; previous revision: 1.11
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Keywords: relnote
FWIW, I needed this feature on Bugzilla 3.0.x.  Backporting the patch was not entirely trivial because $bug->add_comment() does not exist on 3.0; I had to modify it to use AppendComment() instead.  To save others the bother of having to figure this out, here is my adaptation of this patch to Bugzilla 3.0.x.  (It's relative to 3.0.2, if that matters.)
Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.
Keywords: relnote
Flags: testcase?
webservice_bug_add_comment.t -> testcase+
Flags: testcase? → testcase+
You need to log in before you can comment on or make changes to this bug.