617477 - Update the POD for Bug.update, fix error codes, and make Bug.update more consistent

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Max Kanat-Alexander]

Right now I'm working on a test for Bug.update. As I work on this test, I'm finding that there are many errors that need new codes, and some small issues that need to be fixed. Since it's a lot of codes and a lot of small issues, I figured I'd just post one patch for all of them.

Comment 1

[Max Kanat-Alexander]

Attachment: Work In Progress

Comment 2

[Max Kanat-Alexander]

Attachment: v1 (4.0 branch)

Okay, this has a lot of changes in one patch. If you want me to split them into separate bugs, let me know and I will. I just did everything at once to get the webservice_bug_update.t test working properly.

Here are the changes:

* When updating bugs, now if you try to set a time-tracking field but you aren't allowed to, you will get an error. This is so that Bug.update doesn't fail silently in that case--we want the user to specifically know that what they asked to do didn't happen. (This is particularly important for work_time, which doesn't get tracked in "changes", so otherwise the user would have no way of knowing whether or not their change succeeded.) I didn't change what happens when you file bugs, because we're frozen and I don't want to change too much.

* I made _check_keyword use Bugzilla::Keyword->check instead of throwing unknown_keyword, which keeps our error messages and error codes consistent in the webservice.

* Attempting to change the privacy of existing comments on bugs will now throw an error if you are not an insider, instead of just failing silently. (This is important for the WebService, so that users know their action failed.)

* Passing an empty string as a resolution was being silently ignored instead of being treated as an error.

* The "deadline" field is now treated as a literal string instead of as a dateTime field. The problem was two things: first, we do timezone changes on input and output dateTime fields, and we don't want to do that for deadline. Secondly, the code that translates dateTime fields on input was adding "00:00:00" to the deadlines on input, which Bugzilla::Bug then considered invalid. So instead we just accept it as a string and return it as a string.

* Certain fields were being totally ignored when passed to Bug.update: platform, severity, url. (The fix for this is the change in the parameters we pass to map_fields.)

* It was possible for the %changes hash returned by Bugzilla::Bug to contain "undef" for the old or new value, but we want empty strings instead, in the WebService.

* "Bug Edit Denied" was listed as error 108 in the POD, but it's really error 109. This particular fix should also be backported.

* I updated the POD for Bug.update to list all possible errors it can throw.

* I added a lot of error codes to Bugzilla::WebService::Constants.

* If the WebService gets an invalid date when processing inbound dates, it will now throw an error instead of dying with a 500 Internal Server Error (due to trying to call methods on an undef value). This fix should probably also be backported to 3.6.

* The short_desc field is now checked for being too long.

Comment 3

[Max Kanat-Alexander]

Attachment: v1 (trunk)

Okay, here's the same patch for trunk. The major difference is that work_time is in Bugzilla::Comment, and I had to minorly refactor stuff surrounding the validation of timetracking fields in order to get behavior identical to the 4.0 patch.

Also, the Bugzilla::Keyword->check stuff was already done on trunk, so didn't have to be brought forward.

Comment 4

[Frédéric Buclin]

Comment on attachment 496607 [details] [diff] [review]
v1 (trunk)

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

>+sub run_create_validators {

>+    # Sometimes this run_create_validators is called with parameters that
>+    # skip bug_id validation, so it might not exist in the resulting hash.
>+    if (defined $params->{bug_id}) {
>+        $params->{bug_id} = $params->{bug_id}->id;
>+    }

Wouldn't it make more sense to store the bug object in ->{bug} instead of ->{bug_id}? It's very confusing to see ->{bug_id} as being a bug object. This way, you wouldn't need run_create_validators() at all.



>=== modified file 'Bugzilla/WebService/Bug.pm'

>-        $item{'deadline'}       = $self->type('dateTime', $bug->deadline);
>+        my $deadline = datetime_from($bug->deadline);
>+        $item{'deadline'} = $self->type('string', $deadline->ymd);

This change seems to be the exact opposite of what we want. Calling datetime_from() is going to take the timezone into account, which is what you wanted to avoid.


>+Currently, some fields are not tracked in changes: C<comment>,
>+C<comment_is_private>, and C<work_time>.

comment_is_private is listed in the bug history (in show_activity.cgi). So how can it not be in %changes?


Otherwise looks good, but I didn't test it yet.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> Wouldn't it make more sense to store the bug object in ->{bug} instead of
> ->{bug_id}? It's very confusing to see ->{bug_id} as being a bug object. This
> way, you wouldn't need run_create_validators() at all.

  Ah, no, it's simpler to just have the validator return an object. (Anyhow, I'd still need a run_create_validators to delete $params->{bug}.)

> This change seems to be the exact opposite of what we want. Calling
> datetime_from() is going to take the timezone into account, which is what you
> wanted to avoid.

  Ah, true! I should pass Bugzilla->local_timezone as the second argument.

> >+Currently, some fields are not tracked in changes: C<comment>,
> >+C<comment_is_private>, and C<work_time>.
> 
> comment_is_private is listed in the bug history (in show_activity.cgi). So how
> can it not be in %changes?

  %changes is just a hash generated by update(). show_activity.cgi is reading from the database, and the stuff relating to comment privacy is put into the database in a separate part of update() than the part that reads from %changes. Also, the format of the data surrounding comment_is_private is different than normal %changes data, so I just don't want to make those modifications this close to a freeze.

> Otherwise looks good, but I didn't test it yet.

  Thanks! :-)

Comment 6

[Frédéric Buclin]

(In reply to comment #5)
>   Ah, true! I should pass Bugzilla->local_timezone as the second argument.

No, I meant to not call datetime_from() at all. The deadline is already correctly formatted, when returned by $bug->deadline.

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> No, I meant to not call datetime_from() at all. The deadline is already
> correctly formatted, when returned by $bug->deadline.

  Ah, so it is! :-) Okay.

Comment 8

[Max Kanat-Alexander]

Attachment: v2 (4.0)

Okay, here's an updated patch without the datetime_from.

Comment 9

[Max Kanat-Alexander]

Attachment: v2 (trunk)

And here's a trunk patch without datetime_from.

Comment 10

[Frédéric Buclin]

Comment on attachment 497171 [details] [diff] [review]
v2 (trunk)

Looks good. I'm not sure the comment about datetime_from() is useful, though. r=LpSolit

Comment 11

[Frédéric Buclin]

Comment on attachment 497170 [details] [diff] [review]
v2 (4.0)

Looks good. r=LpSolit

Comment 12

[Max Kanat-Alexander]

Sweet!! Thanks for the reviews!! :-)

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified Bugzilla/Bug.pm
modified Bugzilla/Comment.pm
modified Bugzilla/Object.pm
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Constants.pm
modified Bugzilla/WebService/Server.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 7620.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/                         
modified Bugzilla/Bug.pm
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Constants.pm
modified Bugzilla/WebService/Server.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 7496.