Last Comment Bug 384009 - Global fields (priority, severity, OS, and platform) are required when using WebService instead of using the defaults
: Global fields (priority, severity, OS, and platform) are required when using ...
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: WebService (show other bugs)
: 3.0
: All All
: -- normal (vote)
: Bugzilla 3.0
Assigned To: Frédéric Buclin
: default-qa
Mentors:
http://www.bugzilla.org/docs/3.0/html...
Depends on:
Blocks: 415471
  Show dependency treegraph
 
Reported: 2007-06-11 04:52 PDT by Reed Loden [:reed] (use needinfo?)
Modified: 2008-06-30 23:32 PDT (History)
3 users (show)
mkanat: approval+
LpSolit: blocking3.2+
mkanat: approval3.0+
LpSolit: blocking3.0.4+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (6.12 KB, patch)
2008-02-03 08:08 PST, Frédéric Buclin
no flags Details | Diff | Review
patch, v1.1 (6.29 KB, patch)
2008-02-03 12:08 PST, Frédéric Buclin
mkanat: review-
Details | Diff | Review
patch, v2 (10.66 KB, patch)
2008-02-03 16:37 PST, Frédéric Buclin
mkanat: review-
Details | Diff | Review
patch, v3 (11.04 KB, patch)
2008-02-11 11:44 PST, Frédéric Buclin
mkanat: review+
Details | Diff | Review
patch for 3.0, v1 (4.31 KB, patch)
2008-02-11 14:49 PST, Frédéric Buclin
mkanat: review+
Details | Diff | Review

Description Reed Loden [:reed] (use needinfo?) 2007-06-11 04:52:58 PDT
The Bug.create documentation at http://www.bugzilla.org/docs/3.0/html/api/Bugzilla/WebService/Bug.html says the "severity" setting will default to the defaulted setting if nothing is explicitly set. I have 'normal' set as the default under editvalues.cgi?field=bug_severity, but the webservice is still throwing error 104 ("A legal Severity was not set."). So, either the code is wrong or the documentation is wrong. Either way, it should be fixed. :)
Comment 1 Max Kanat-Alexander 2007-06-11 14:55:31 PDT
You mean you have it set as the default in editparams under Bug Fields? That's where the defaults get set.

If that's true, and it still throws an error, then yes, this would be a bug in Bugzilla (not the docs).
Comment 2 Reed Loden [:reed] (use needinfo?) 2007-06-11 14:57:37 PDT
(In reply to comment #1)
> You mean you have it set as the default in editparams under Bug Fields? That's
> where the defaults get set.

Considering looking at
https://landfill.bugzilla.org/bugzilla-3.0-branch/editparams.cgi?section=bugfields#defaultseverity
saying that "normal" is the default, I would say this is a Bugzilla bug then.
Comment 3 Reed Loden [:reed] (use needinfo?) 2007-06-11 14:58:33 PDT
s/saying/says/
Comment 4 Frédéric Buclin 2008-02-03 08:08:40 PST
Created attachment 301127 [details] [diff] [review]
patch, v1

The bug status is required, else _check_bug_status() throws an error. Global fields fall back to default values if undefined.
Comment 5 Frédéric Buclin 2008-02-03 12:08:38 PST
Created attachment 301151 [details] [diff] [review]
patch, v1.1

Some more cleanup.
Comment 6 Max Kanat-Alexander 2008-02-03 13:27:19 PST
Comment on attachment 301151 [details] [diff] [review]
patch, v1.1

bug_status should not be required. It should default if not selected--_check_bug_status is supposed to make sure of that.

There's no reason to also edit email_in in this patch; that should be a separate bug.

I also generally prefer "if !defined" to "unless defined".

Finally, I was pretty sure the validators themselves were supposed to set the default values if passed undef, but I guess they don't?
Comment 7 Frédéric Buclin 2008-02-03 13:41:08 PST
(In reply to comment #6)
> bug_status should not be required. It should default if not
> selected--_check_bug_status is supposed to make sure of that.

_check_bug_status() first call Bugzilla::Status->check() which throws an error if no bug status is given. Moreover, there is no default value in 3.2 as bug statuses are customizable so Bug::create() would pick one randomly. What could be done is to:
- select UNCONFIRMED for users without canconfirm privs and if votes to confirm > 0, or:
- select the first non-UNCONFIRMED status with the lowest sortkey if the user has not canconfirm privs and votes to confirm = 0, or if the user has canconfirm privs but didn't provide any bug status. If a powerful user provided a valid one, use it instead, else throw an error (i.e. if the given bug status is illegal on bug creation).


> There's no reason to also edit email_in in this patch; that should be a
> separate bug.

The reason I do it here is because I cannot remove the code in it without first implementing fallbacks in Bug::create(). So that's definitely related (and I did enough testing on it at the same time I was testing XML-RPC).


> I also generally prefer "if !defined" to "unless defined".

Yeah, but I don't. ;) That's my french touch. :-D


> Finally, I was pretty sure the validators themselves were supposed to set the
> default values if passed undef, but I guess they don't?

This would be pretty difficult as missing fields are not validated. So no, validators do not do it themselves, which seems reasonable to me.
Comment 8 Max Kanat-Alexander 2008-02-03 14:29:28 PST
(In reply to comment #7)
> _check_bug_status() first call Bugzilla::Status->check() which throws an error
> if no bug status is given.

  Ah, that's a problem.

> - select UNCONFIRMED for users without canconfirm privs and if votes to confirm > 0, or:
> [snip]

  Yes, your whole plan is what 3.0 did, per its code, and it's what the 3.2 code was actually *designed* to do--look at the $default_status stuff in _check_bug_status.

> The reason I do it here is because I cannot remove the code in it without first
> implementing fallbacks in Bug::create(). So that's definitely related (and I
> did enough testing on it at the same time I was testing XML-RPC).

  Okay.

> This would be pretty difficult as missing fields are not validated. So no,
> validators do not do it themselves, which seems reasonable to me.

  Yes, that's fine, because it makes the API of create() easier to use. (That is, it was kind of silly of us to "require" people to specify "undef" for certain fields.)
Comment 9 Frédéric Buclin 2008-02-03 16:37:50 PST
Created attachment 301187 [details] [diff] [review]
patch, v2

No longer require the bug status field.
Comment 10 Max Kanat-Alexander 2008-02-07 16:20:41 PST
Comment on attachment 301187 [details] [diff] [review]
patch, v2

This is fine, but your changes make "comment" required for creating bugs through the WebService API, when that field was not previously required.

We will have to add a "History" section to the POD for Bug.create that has a single item:

=item Before B<3.0.4>, parameters marked as B<Defaulted> were actually B<Required>, due to a bug in Bugzilla.
Comment 11 Frédéric Buclin 2008-02-11 11:44:18 PST
Created attachment 302636 [details] [diff] [review]
patch, v3

The comment is no longer required. Note bug 416481 about "require comment" (this is not a regression introduced by this patch).
Comment 12 Max Kanat-Alexander 2008-02-11 13:39:46 PST
Comment on attachment 302636 [details] [diff] [review]
patch, v3

Yep, works nicely and looks good. :-)
Comment 13 Max Kanat-Alexander 2008-02-11 13:42:14 PST
Comment on attachment 302636 [details] [diff] [review]
patch, v3

This patch doesn't seem to apply on 3.0, though, so I can't test it.
Comment 14 Frédéric Buclin 2008-02-11 14:13:29 PST
(In reply to comment #13)
> (From update of attachment 302636 [details] [diff] [review])
> This patch doesn't seem to apply on 3.0, though, so I can't test it.

Not surprising as Bugzilla/Status.pm doesn't exist on 3.0. :) I wanted to get your review first before backporting it to the branch.
Comment 15 Frédéric Buclin 2008-02-11 14:49:36 PST
Created attachment 302678 [details] [diff] [review]
patch for 3.0, v1

Tested with both XML-RPC and email_in.pl. Bugs reported about 3.2 do not affect 3.0 (fortunately!).
Comment 16 Max Kanat-Alexander 2008-02-11 17:25:59 PST
Comment on attachment 302678 [details] [diff] [review]
patch for 3.0, v1

Yeah, this seems to work just fine.
Comment 17 Frédéric Buclin 2008-02-11 17:35:32 PST
tip:

Checking in email_in.pl;
/cvsroot/mozilla/webtools/bugzilla/email_in.pl,v  <--  email_in.pl
new revision: 1.14; previous revision: 1.13
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.232; previous revision: 1.231
done
Checking in Bugzilla/Status.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Status.pm,v  <--  Status.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.11; previous revision: 1.10
done
Checking in Bugzilla/WebService/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Constants.pm,v  <--  Constants.pm
new revision: 1.15; previous revision: 1.14
done


3.0.3:

Checking in email_in.pl;
/cvsroot/mozilla/webtools/bugzilla/email_in.pl,v  <--  email_in.pl
new revision: 1.5.2.7; previous revision: 1.5.2.6
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.171.2.4; previous revision: 1.171.2.3
done
Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.4.2.4; previous revision: 1.4.2.3
done
Comment 18 Marc Schumann [:Wurblzap] 2008-02-19 02:15:21 PST
For the record, this also fixes a 3.1.3 problem with manually entering bugs. Unprivileged users were forced to enter bugs in the default state:

    my $default_status = Bugzilla::Status->can_change_to->[0];
    if ($user->in_group('editbugs', $product->id)
        || $user->in_group('canconfirm', $product->id)) {
       # If the user with privs hasn't selected another status,
       # select the first one of the list.
       $new_status ||= $default_status;
    }
    else {
        # A user with no privs cannot choose the initial status.
        $new_status = $default_status;
    }

This makes bug creation fail with 3.1.3 if there is voting switched on in general, but not for the particular product I'm entering a bug in.

The new part

    if (!$product->votes_to_confirm) {
        # UNCONFIRMED becomes an invalid status if votes_to_confirm is 0,
        # even if you are in editbugs.
        @valid_statuses = grep {$_->name ne 'UNCONFIRMED'} @valid_statuses;
    }

fixes this.
Comment 19 Max Kanat-Alexander 2008-06-30 23:32:15 PDT
This was fixed in 3.0.4, so no need to relnote it for 3.2.

Note You need to log in before you can comment on or make changes to this bug.