Closed Bug 384009 Opened 17 years ago Closed 16 years ago

Global fields (priority, severity, OS, and platform) are required when using WebService instead of using the defaults

Categories

(Bugzilla :: WebService, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.0

People

(Reporter: reed, Assigned: LpSolit)

References

()

Details

Attachments

(2 files, 3 obsolete files)

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. :)
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).
(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.
s/saying/says/
Attached patch patch, v1 (obsolete) — Splinter Review
The bug status is required, else _check_bug_status() throws an error. Global fields fall back to default values if undefined.
Assignee: webservice → LpSolit
Status: NEW → ASSIGNED
Attachment #301127 - Flags: review?(mkanat)
Flags: blocking3.2+
Flags: blocking3.0.4+
Attached patch patch, v1.1 (obsolete) — Splinter Review
Some more cleanup.
Attachment #301127 - Attachment is obsolete: true
Attachment #301151 - Flags: review?(mkanat)
Attachment #301127 - Flags: review?(mkanat)
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?
Attachment #301151 - Flags: review?(mkanat) → review-
(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.
(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.)
Attached patch patch, v2 (obsolete) — Splinter Review
No longer require the bug status field.
Attachment #301151 - Attachment is obsolete: true
Attachment #301187 - Flags: review?(mkanat)
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.
Attachment #301187 - Flags: review?(mkanat) → review-
Attached patch patch, v3Splinter Review
The comment is no longer required. Note bug 416481 about "require comment" (this is not a regression introduced by this patch).
Attachment #301187 - Attachment is obsolete: true
Attachment #302636 - Flags: review?(mkanat)
Blocks: 415471
Comment on attachment 302636 [details] [diff] [review]
patch, v3

Yep, works nicely and looks good. :-)
Attachment #302636 - Flags: review?(mkanat) → review+
Flags: approval+
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.
(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.
Tested with both XML-RPC and email_in.pl. Bugs reported about 3.2 do not affect 3.0 (fortunately!).
Attachment #302678 - Flags: review?(mkanat)
Comment on attachment 302678 [details] [diff] [review]
patch for 3.0, v1

Yeah, this seems to work just fine.
Attachment #302678 - Flags: review?(mkanat) → review+
Flags: approval3.0+
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: relnote
Resolution: --- → FIXED
Summary: severity is being required instead of just using the default → Global fields (priority, severity, OS, and platform) are required when using WebService instead of using the defaults
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.
This was fixed in 3.0.4, so no need to relnote it for 3.2.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.