Closed Bug 313123 Opened 19 years ago Closed 17 years ago

implement validations and database persistence functions for Component.pm and clean-up editcomponents.cgi.

Categories

(Bugzilla :: Administration, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.2

People

(Reporter: batosti, Assigned: LpSolit)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 12 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050725 Firefox/1.0.6 (Ubuntu package 1.0.6)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050725 Firefox/1.0.6 (Ubuntu package 1.0.6)

We need methods and/or subroutines for validations and database persistence.
All related routines about inserts, updates, deletes and validations for those
routines have to be implemented at this bug.

Reproducible: Always
Blocks: 297791
Assignee: administration → batosti
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch batosti_v1 (obsolete) (deleted) — Splinter Review
Attachment #200800 - Flags: review?(LpSolit)
Comment on attachment 200800 [details] [diff] [review]
batosti_v1

A few things I have seen so far:


>Index: Bugzilla/Component.pm

>+use Bugzilla::Config qw($datadir);

$datadir is only required when deleting versioncache. But I don't think this deletion should be done from this .pm module.


>+sub product {
>+    my $self = shift;
>+
>+    if (!defined $self->{'product'}) {
>+        $self->{'product'} = new Bugzilla::Product($self->product_id);
>+    }
>+    return $self->{'product'};
>+}

We don't want module dependency loops, explaining why we didn't define this method earlier. As series should be created in Series.pm, you don't need it anymore.


>+sub create_series {

>+        my $series = new Bugzilla::Series(undef, $self->product->name,
>+                                          $self->name, $sdata->[0],
>+                                          $::userid, 1, $sdata->[1], 1);
>+        $series->writeToDatabase();

See, this is really Bugzilla::Series related.


>+sub remove_from_db {

>+    unlink "$datadir/versioncache";

Nit: I'm not sure this line should be in this file. I would leave it in the .cgi file for now.
Attachment #200800 - Flags: review?(LpSolit) → review-
Attached patch batosti_v1_fix (obsolete) (deleted) — Splinter Review
Attachment #200800 - Attachment is obsolete: true
Attachment #200864 - Flags: review?(LpSolit)
Severity: normal → enhancement
Target Milestone: --- → Bugzilla 2.24
Depends on: 311258
Comment on attachment 200864 [details] [diff] [review]
batosti_v1_fix

This patch no longer applies due to the checkin of bug 311258.
Attachment #200864 - Flags: review?(LpSolit) → review-
Attached patch batosti_v2 (obsolete) (deleted) — Splinter Review
Attachment #200864 - Attachment is obsolete: true
Attachment #214227 - Flags: review?(LpSolit)
Comment on attachment 214227 [details] [diff] [review]
batosti_v2

As said in several other places already, do not lock tables in Component.pm. I will review an updated patch.
Attachment #214227 - Flags: review?(LpSolit) → review-
Attached patch batosti_v3 (obsolete) (deleted) — Splinter Review
Attachment #214227 - Attachment is obsolete: true
Attachment #218994 - Flags: review?(LpSolit)
Comment on attachment 218994 [details] [diff] [review]
batosti_v3

>Index: editcomponents.cgi

>+    my $component = Bugzilla::Component::create_component(\%new_component);
> 
>     # Insert default charting queries for this product.
>     # If they aren't using charting, this won't do any harm.

Series must be created by create_component(), else the caller would have to do it itself, which will never happen. ;)


>-    if ($component->bug_count) {
>-        if (Param("allowbugdeletion")) {
>-            foreach my $bug_id (@{$component->bug_ids}) {
>-                my $bug = new Bugzilla::Bug($bug_id, $whoid);
>-                $bug->remove_from_db();
>-            }
>-        } else {
>-            ThrowUserError("component_has_bugs",
>-                           {nb => $component->bug_count });
>-        }
>-    }
>-    
>     $dbh->bz_lock_tables('components WRITE', 'flaginclusions WRITE',
>                          'flagexclusions WRITE');

You know, there are good reasons to remove bugs before locking tables here:
1. Bugzilla::Bug::remove_from_db() locks tables internally.
2. The number of tables to lock is much higher than the 3 tables locked here.

In other words, everything will crash here.

IMO, what $component->remove_from_db should do is to check for bugs in this component. If there are no bugs in it, let's delete the component. Else complain that bugs being in this component must be removed first. That's the safest approach I can think about.


>-    $dbh->bz_lock_tables('components WRITE', 'profiles READ');

>+    $component->update(\%updates);
> 
>-    $dbh->bz_unlock_tables();

Err... Please do not remove these locks.



>Index: Bugzilla/Component.pm

>+use Bugzilla::Config;

Do not use Config.pm here. You really don't want it, see below. ;)


>+sub remove_from_db {

>+    if ($self->bug_count) {

That's all you need to know, really. If the component still contains bugs, complain! But do not remove them from here.


>+sub update {

>+    foreach my $field (keys(%$updates)) {

This foreach loop is useless. What you should do is to scan all 4 fields, and if a field exists, treat it. Something like:

if (exists $updates->{'name'}) {
    ...
}
if (exists $updates->{'description'}) {
    ...
}
etc...


>+            trick_taint($value);
>+            $value || ThrowUserError('component_blank_name');

It's too early to detaint $value. If $value is undefined, trick_taint() will die().


>+            trick_taint($value);
>+            $value || ThrowUserError('component_blank_description',
>+                                     {name => $self->name});

Same comment here. First make sure $value is defined, else trick_taint() will die.


>+        $dbh->do($query, undef, @values, $self->id);

Nit: I would prefer to see ->do($query, undef, (@values, $self->id)); to clearly indicate what arguments are.


>+sub write_changes {

This method is used nowhere. Remove it!


>+sub create_component {

>+    trick_taint($new_component->{'name'});
>+    trick_taint($new_component->{'description'});
>+    $new_component->{'name'} || ThrowUserError('component_blank_name');

Again, make sure these values are defined first!


>+    my $initialownerid =
>+        Bugzilla::User::login_to_id($new_component->{'initialowner'});
>+    my $initialqacontactid = Bugzilla::Config::Param('useqacontact') ?
>+        (Bugzilla::User::login_to_id($new_component->{'initialqacontact'}) ||
>+        undef) : undef;

Huh? And what do you do if they are invalid login names?? Previously, it was safe to not check these logins in editparams.cgi because Bugzilla::User::match() was doing this check for us. But this is no longer true. You have no idea if the caller has checked these login names or not. So you have to do it here. An easy way to do it is to call login_to_id() with the THROW_ERROR argument: login_to_id($login_name, THROW_ERROR) will throw an error if the login name doesn't exist. Moreover, login_to_id() is already exported by User.pm, so there is no need to append Bugzilla::User:: to it.


>+=item C<change_field($field, $value)>

What is change_field()??? This method doesn't exist.


>+=item C<write_changes()>

Bye bye!


>+=item C<create_component($comp_name, $default_assignee, $default_qa_contact, $description, $product)>

Wrong! create_component() passes a reference to a hash.



>Index: template/en/default/admin/components/updated.html.tmpl

>+[% IF comp.changed_fields.size > 0 %]

Heh, good idea!
Attachment #218994 - Flags: review?(LpSolit) → review-
Attached patch batosti_v4 (obsolete) (deleted) — Splinter Review
Attachment #218994 - Attachment is obsolete: true
Attachment #228136 - Flags: review?(LpSolit)
Attached patch batosti_v4_fix (obsolete) (deleted) — Splinter Review
Attachment #228136 - Attachment is obsolete: true
Attachment #228425 - Flags: review?(LpSolit)
Attachment #228136 - Flags: review?(LpSolit)
Comment on attachment 228425 [details] [diff] [review]
batosti_v4_fix

>Index: editcomponents.cgi

editcomponents.cgi doesn't need to use Bugzilla::Series anymore.


>if ($action eq 'new') {

>+    $new_component{'who_id'} = $whoid;

Nit: do we really want it? Or should we look at Bugzilla->user-id in Component.pm directly? If we keep it, creator_id would probably be a better name.


>if ($action eq 'update') {
>     my $default_qa_contact    = trim($cgi->param('initialqacontact') || '');
>     my $description           = trim($cgi->param('description')      || '');
> 
>+    if ($comp_name ne $component->name) {
>+        $updates{'name'} = $comp_name;
>     }

Instead of doing all these checks here (if old == new), pass everything to %updated and let ->update() do these checks for you. I don't expect the caller to do these checks anyway.



>Index: Bugzilla/Component.pm

Component.pm has to use Bugzilla::Constants due to FORCE_REFRESH and Bugzilla::Series due to create_component().


> sub bug_count {
>     my $self = shift;
>+    my $refresh = shift;

Nit: $force_refresh would probably be a better name than $refresh.


>+sub remove_from_db {

>+    ThrowCodeError("component_has_bugs")
>+        if $self->bug_count(FORCE_REFRESH);

component_has_bugs already exists in user-error.html.tmpl. Use it here too, even if in this case this would rather be a code error than a user error. I don't like duplicating messages in both error files.


>+sub update {
>+    my $self = shift;
>+    my ($updates) = (@_);

Nit: my $updates = shift; or my ($updates) = @_;


>+    if (exists $updates->{'name'}) {

>+        $self->{'name'} = $name;
>+        push (@values, $name);
>+        $query .= qq{name = ?, };
>+        push @{$self->{'changed_fields'}}, 'name';

Only include the name if and only if $name differs from the old value. I don't want to update the DB with data which is unchanged (I don't trust the caller).


>+    if (exists $updates->{'description'}) {

>+        $self->{'description'} = $description;
>+        push (@values, $description);
>+        $query .= qq{description = ?, };
>+        push @{$self->{'changed_fields'}}, 'description';

Same remark here as well as for the default assignee and QA contact.


>+    if (exists $updates->{'initialqacontact'}) {

Ignore this change if 'useqacontact' is off.


>+        my $qa_contact =
>+            Bugzilla::User->new_from_login($updates->{'initialqacontact'});
>+        $qa_contact || ThrowUserError('invalid_username',
>+                                      {name => $updates->{'initialqacontact'}});

You are allowed to delete the default QA contact, i.e. having no QA contact. From what I see, your code doesn't allowe me to do that.


>+sub create_component {

We should call it 'create' instead. After all, we don't have update_component nor remove_component_from_db. We should be consistent, and create() makes more sense.


>+    $new_component->{'name'} || ThrowUserError('component_blank_name');

Nit: here, you write $new_component->{'name'} all the time while in update, you wrote $name to have shorter variable names. Please be consistent.


>+    my $component = new Bugzilla::Component(
>+        {name => $new_component->{'name'},
>+         product_id => $new_component->{'product'}->id});

As you define $product later in this routine, define it earlier and write $product->id here.


>+    if ($component) {
>+        ThrowUserError('component_already_exists',
>+                       {'name' => $component->name});
>+    }

Nit: the code is duplicated between create() and update() when validating data. Maybe should we have check_name(), check_description() and so on which would be called from both places. This can be done in a separate bug though.


>+    my $initialownerid =
>+        Bugzilla::User::login_to_id($new_component->{'initialowner'});

As I said at least twice, login_to_id() is already exported by User.pm, so appending Bugzilla::User:: is useless here. Moreover, we have to make sure that the assignee is valid, so login_to_id() must be called with THROW_ERROR.


>+    if (Bugzilla->params->{'useqacontact'} &&
>+        $new_component->{'initialqacontact'}) {

Nit: please respect our policy: put && on a newline and add the bracked on a newline too when the test doesn't fit on one line.


>+=item C<remove_from_db()>

The SYNOPSIS part of the POD should mention these new methods and routines too.


>+=item C<create_component($new_component)>
>+
>+ Description: Create a new Bugzilla::Product object.
>+
>+ Params:      $new_component - A reference to a hash with the component
>+                               data.

You should detail what the keys of the hash must be.

POD is missing for the update() method.


Moreover, your patch doesn't pass tests:

Failed Test     Stat Wstat Total Fail  Failed  List of Failed
-------------------------------------------------------------------------------
t/001compile.t     8  2048   124    8   6.45%  9 15-16 28 33 39 61 78
t/008filter.t      1   256   265    1   0.38%  34
t/009bugwords.t    1   256   265    1   0.38%  178
Attachment #228425 - Flags: review?(LpSolit) → review-
(In reply to comment #11)
> (From update of attachment 228425 [details] [diff] [review] [edit])
> >+sub remove_from_db {
> 
> >+    ThrowCodeError("component_has_bugs")
> >+        if $self->bug_count(FORCE_REFRESH);
> 
> component_has_bugs already exists in user-error.html.tmpl. Use it here too,
> even if in this case this would rather be a code error than a user error. I
> don't like duplicating messages in both error files.
> 

What is your sugestion here? use the user error or rewrite the name and text of the code error?
Attached patch batosti_v5 (obsolete) (deleted) — Splinter Review
Attachment #228425 - Attachment is obsolete: true
Attachment #229651 - Flags: review?(LpSolit)
Comment on attachment 229651 [details] [diff] [review]
batosti_v5

>Index: Bugzilla/Component.pm

>+sub remove_from_db {

>+    ThrowCodeError("component_still_has_bugs")
>+        if $self->bug_count(FORCE_REFRESH);

As I said, we already have a similar error message in user-error.html.tmpl. Use it!


>+sub update {

>+    if ($updates->{'name'} ne $self->name) {

If a field is undefined, then you should ignore it. This way, you could send partial data to update(), if desired. To make things clear: undefined and an empty string are two *different* things. So you should *always* make sure the value is defined (even if it's empty) before considering it. This comment applies to *all* fields in this method.


>+    if (Bugzilla->params->{'useqacontact'} &&
>+        $initial_qa_contact ne $self->default_qa_contact->login) {

>+        } else {
>+            push (@values, 0);
>+        }

No! a user ID of 0 is *invalid*! No QA contact means user ID = NULL (undef).


All my comments about POD have been ignored. I will ignore the rest of your patch as well for now.
Attachment #229651 - Flags: review?(LpSolit) → review-
Attached patch batosti_v5_fix (obsolete) (deleted) — Splinter Review
Attachment #229651 - Attachment is obsolete: true
Attachment #229815 - Flags: review?(LpSolit)
Comment on attachment 229815 [details] [diff] [review]
batosti_v5_fix

>Index: Bugzilla/Component.pm

>+sub remove_from_db {

>+    ThrowUserError("component_has_bugs")
>+        if $self->bug_count(FORCE_REFRESH);

'component_has_bugs' takes an argument {nb => $self->bug_count }.


>+sub update {

>+        $self->{'name'} = $name;

>+    if (defined $updates->{'description'} &&

>+        $description || ThrowUserError('component_blank_description',
>+                                 {name => $self->name});

Nit: as the component name is the first field to be checked and updated, all subsequent error messages will mention the new component name instead of the old one. Maybe it would be better to still mention the old one as changes have not been inserted into the DB yet.


>+        $self->{'default_qa_contact'} = undef;

>+        $self->{initialqacontact} = $qa_contact_id;

You are pretty inconsistent in your way to update fields. For the default assignee, you create and update the user object, but for the QA contact, you clear the user object and update 'initialqacontact' only. I would prefer to update *both* the ID field + the user object (note that $self->{'initialowner'} has never been updated btw). To do this, call Bugzilla::User->new_from_login() as you did for the default assignee.


>+        my $dbh = Bugzilla->dbh;
>+    if (scalar(@{$self->{'changed_fields'}})) {

Nit: indentation! Move $dbh inside the IF block.


>+sub create {

>+    trick_taint($name);
>+    trick_taint($description);

Nit: you should only detaint these values right before the SQL insert and explaining why it's safe to detaint them.


>+=item C<remove_from_db()>
>+
>+ Description: Remove the component from database.

Nit: only if there are no more bugs in this component!


>+ Returns:     none.

Nit: it returns "nothing", not "none".


>+ Params:      $updates - An hash ref with the fields to update (name,
>+                         description, initialowner, initialqacontact).

Nit: we could say that 'initialqacontact' is optional.


>+ Params:      $new_component - A reference to a hash with the fields of
>+                               the new component (name, description,
>+                               initialowner, initialqacontact) and the
>+                               product that the component belongs.

Nit: more precisely, the product *object*.



>Index: template/en/default/filterexceptions.pl

>-'admin/components/confirm-delete.html.tmpl' => [
>-  'comp.bug_count'
>-],

There is no reason to remove this entry.



>Index: template/en/default/admin/components/confirm-delete.html.tmpl

>-          [%- product.name FILTER url_quote %]">[% comp.bug_count %]</a>
>+          [%- product.name FILTER url_quote %]">[% comp.bug_count FILTER url_quote %]</a>

"FILTER url_quote" outside an URL doesn't make sense. First of all, it should be "FILTER html", or even "FILTER none" as it's an integer here. But per my previous comment, leave it unfiltered; it's safe.

This comment applies everywhere within this file, which means there is no reason to alter this file at all.



>Index: template/en/default/admin/components/updated.html.tmpl

>+[% IF comp.changed_fields.size > 0 %]
>+  [% FOREACH field = comp.changed_fields %]

We could easily avoid this FOREACH loop using changed_fields as a hash instead of an array. Better is to use a hash in Component::update() of the form $foo{field_name} => 1.


There are some bitrots in your patch which are easy to fix. I haven't tested your patch yet but it looks good.
Attachment #229815 - Flags: review?(LpSolit) → review-
Attached patch batosti_v6 (obsolete) (deleted) — Splinter Review
Attachment #229815 - Attachment is obsolete: true
Attachment #230813 - Flags: review?(LpSolit)
Comment on attachment 230813 [details] [diff] [review]
batosti_v6

Thanks to methods defined in Object.pm, you don't need FORCE_REFRESH anymore.
Attachment #230813 - Flags: review?(LpSolit) → review-
Attached patch batosti_v7 (obsolete) (deleted) — Splinter Review
now with Bugzilla::Object
Attachment #230813 - Attachment is obsolete: true
Attachment #238637 - Flags: review?(LpSolit)
Comment on attachment 238637 [details] [diff] [review]
batosti_v7

bitrotten (due to security bugs, probably)
Attachment #238637 - Flags: review?(LpSolit) → review-
We are in "soft freeze" mode to prepare 3.0 RC1.
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2
Assignee: batosti → LpSolit
Attached patch patch, v8 (obsolete) (deleted) — Splinter Review
Attachment #238637 - Attachment is obsolete: true
Attachment #284355 - Flags: review?(mkanat)
Comment on attachment 284355 [details] [diff] [review]
patch, v8

>Index: editcomponents.cgi
[snip]
>+      Bugzilla::Component->create({ name             => $comp_name,
[snip]
>+                                    initialowner     => $default_assignee,
>+                                    initialqacontact => $default_qa_contact,
>+                                    initial_cc_list  => \@initial_cc,

  Either you call everything by its accessor name, or you call everything by its DB name. If you're going to call things by their accessor name, you'll probably need a CREATE_FIELD_MAP constant in Bugzilla::Object that's an empty hashref by default.

>+                                    open_name        => $cgi->param('open_name'),
>+                                    nonopen_name     => $cgi->param('nonopen_name') });

  Series creation is actually a separate action from Component creation, so these shouldn't go into the Component constructor. Is there some reason we would *always* want to make a new series when we make a new Component? (Imagine, say, that we were making a new component from the WebService.)

  Even if we do keep Series creation inside of Component::create, these two parameters should go, and should be somehow replaced with get_text inside of Component::_create_series. (Think about the WebService situation, or the customizer writing a patch that creates Components--he won't have this information easily.)

Bugzilla/Component.pm
---------------------

>+sub _update_cc_list {
[snip]
+    # Reset the internal variable. No need to reset initial_cc,
+    # set_cc_list() did it already.
+    delete $self->{cc_ids};

  That should not be necessary.

>+sub _check_owner {

  Component now uses three names for this thing: initialowner, default_assignee, and owner. We need to pick one and stick with it. I suggest "owner". That is, as far as people *outside* the module are concerned, the only thing they should know about is "owner". We can still use the variable "initialowner" inside the module, but default_assignee should be renamed to "owner".

>+sub _check_qa_contact {
>+        $qa_contact_id = Bugzilla::User->check({name => $qa_contact})->id if $qa_contact;

  Nit: Just so you know, you can just do Bugzilla::User->check($qa_contact) instead of passing the hashref here.

>+        # If set_cc_list() has been called but data are not yet written
>+        # into the DB, use new values defined by it.
>+        if (defined $self->{cc_ids}) {
>+            $cc_ids = $self->{cc_ids};
>+        }

  Nit: Just do $self->{cc_ids} ||= $dbh->selectcol_arrayref

>-Returns an arrayref of L<Bugzilla::User> objects representing the
>-Initial CC List.
>+  Description: Returns a list of user objects representing users being
>+               in the initial CC list.
>+
>+  Params:      none.
>+
>+  Returns:     An arrayref of L<Bugzilla::User> objects.

  That makes a whole block of "pre formatted text" like you were writing code. This should be in the style of Bugzilla::DB's POD, instead. At the very least, it shouldn't be pre-formatted text.

>+=item C<create(\%params)>
> [snip]
>+              product         - a Bugzilla::Product object.

  add "to which the Component is being added."

>+              description     - description of the new milestone (string).

  s/milestone/component/

>+              initialowner    - login name of the default owner (string).

  Either say "default assignee" or "component owner".

>Index: Bugzilla/WebService/Constants.pm
> [snip]
>@@ -65,7 +65,6 @@ use constant WS_ERROR_CODE => {
>     # Component errors
>     require_component       => 105,
>     component_name_too_long => 105,
>-    component_not_valid     => 105,

  You'll have to somehow add in here the errors thrown by check(), and then add them to the docs of the various webservice modules that might trigger them.

>Index: template/en/default/global/user-error.html.tmpl

>   [% ELSIF error == "component_blank_description" %]
>     [% title = "Blank Component Description Not Allowed" %]
>-    You must enter a non-blank description for component '[% name FILTER html %]'.
>-     
>+    The description of components cannot be blank.

  "You must enter a non-blank description for this component."

  You also have to update object_name at the bottom of this file.
Attachment #284355 - Flags: review?(mkanat) → review-
(In reply to comment #23)
>   Series creation is actually a separate action from Component creation, so
> these shouldn't go into the Component constructor. Is there some reason we
> would *always* want to make a new series when we make a new Component?

It has always been so so far. I see no reason to change this behavior when created from the webservice.



> Bugzilla/Component.pm

> >+sub _update_cc_list {
> [snip]
> +    # Reset the internal variable. No need to reset initial_cc,
> +    # set_cc_list() did it already.
> +    delete $self->{cc_ids};
> 
>   That should not be necessary.

Why? I clear it to not try to commit the list again.


> >+        if (defined $self->{cc_ids}) {
> >+            $cc_ids = $self->{cc_ids};
> >+        }
> 
>   Nit: Just do $self->{cc_ids} ||= $dbh->selectcol_arrayref

No, this won't work. You would then be unable to clear the CC list. That's why I wrote "defined".



> >Index: Bugzilla/WebService/Constants.pm

> >-    component_not_valid     => 105,
> 
>   You'll have to somehow add in here the errors thrown by check(), and then add
> them to the docs of the various webservice modules that might trigger them.

Huh? Not sure I understood everything. :)
Status: NEW → ASSIGNED
(In reply to comment #24)
> (In reply to comment #23)
> >   Series creation is actually a separate action from Component creation, so
> > these shouldn't go into the Component constructor. Is there some reason we
> > would *always* want to make a new series when we make a new Component?
> 
> It has always been so so far. I see no reason to change this behavior when
> created from the webservice.

  Okay. In that case, we just shouldn't have to pass open_name and closed_name to Component::create.

> > +    # Reset the internal variable. No need to reset initial_cc,
> > +    # set_cc_list() did it already.
> > +    delete $self->{cc_ids};
> > 
> >   That should not be necessary.
> 
> Why? I clear it to not try to commit the list again.

  Committing the list again wouldn't be too harmful. It won't show up in %changes. Also, if you can just do $self->{cc_ids} ||= in the accessor, that makes the code simpler.

> >   Nit: Just do $self->{cc_ids} ||= $dbh->selectcol_arrayref
> 
> No, this won't work. You would then be unable to clear the CC list. That's why
> I wrote "defined".

  Eh? No, you could still clear the CC list, An empty arrayref is true.

> >   You'll have to somehow add in here the errors thrown by check(), and then add
> > them to the docs of the various webservice modules that might trigger them.
> 
> Huh? Not sure I understood everything. :)

  Ah. Each webservice function documents the errors it could possibly throw. If one of them could now throw object_not_valid, we should probably document that somewhere, and object_not_valid should get its own ID here in Bugzilla::WebService::Constants.
Attached patch patch, v9 (obsolete) (deleted) — Splinter Review
I fixed all your comments.
Attachment #284355 - Attachment is obsolete: true
Attachment #284391 - Flags: review?(mkanat)
Comment on attachment 284391 [details] [diff] [review]
patch, v9

>Index: editcomponents.cgi
>+      Bugzilla::Component->create({ name             => $comp_name,
>+                                    product          => $product,
>+                                    description      => $description,
>+                                    initialowner     => $default_assignee,
>+                                    initialqacontact => $default_qa_contact,
>+                                    initial_cc_list  => \@initial_cc });

  Shouldn't we just call it initial_cc, if that's what we call the accessor?

>Index: Bugzilla/Component.pm
>+use constant REQUIRED_CREATE_FIELDS => qw(
>+    name
>+    product
>+    initialowner
>+    initialqacontact

  initialqacontact can be NULL, so it's not required.

>@@ -79,6 +106,225 @@ sub new {
>+sub set_initialowner {

  Oh, I'm sorry, I was unclear about this.

  The set_ functions *must* be named after the accessors, but the create() arguments can be named after the DB columns. The idea is that someday we'll fix things so DB columns can be easily named after the accessors, but it's not easy right now. It's easy to name the set_ functions, so we do that.

  So this should be set_default_assignee, as long as the accessor is called default_assignee.

>+sub set_initialqacontact {

  And this should be called set_default_qa_contact.

>Index: Bugzilla/WebService/Constants.pm
> use constant WS_ERROR_CODE => {
>+    # Generic Bugzilla::Object errors are 50-99.
>+    object_name_not_specified   => 50,
>+    object_does_not_exist       => 51,

  Great. But now you have to add documentation to any WebService method that can throw this error. You'll notice that most of them have an "Errors" section.
Attachment #284391 - Flags: review?(mkanat) → review-
Attached patch patch, v9.1 (deleted) — Splinter Review
Attachment #284391 - Attachment is obsolete: true
Attachment #284526 - Flags: review?(mkanat)
Comment on attachment 284526 [details] [diff] [review]
patch, v9.1

>Index: Bugzilla/WebService/Bug.pm
>+=item 51 (Invalid Object)
>+
>+The component you specified is not a known one in this product.

  "is not valid for this Product"

  You can fix that on checkin.
Attachment #284526 - Flags: review?(mkanat) → review+
Flags: approval+
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cgi
new revision: 1.83; previous revision: 1.82
done
Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.51; previous revision: 1.50
done
Checking in request.cgi;
/cvsroot/mozilla/webtools/bugzilla/request.cgi,v  <--  request.cgi
new revision: 1.43; previous revision: 1.42
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.209; previous revision: 1.208
done
Checking in Bugzilla/Component.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Component.pm,v  <--  Component.pm
new revision: 1.16; previous revision: 1.15
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.82; previous revision: 1.81
done
Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/WebService/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Constants.pm,v  <--  Constants.pm
new revision: 1.11; previous revision: 1.10
done
Checking in template/en/default/admin/components/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/components/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.14; previous revision: 1.13
done
Checking in template/en/default/admin/components/updated.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/components/updated.html.tmpl,v  <--  updated.html.tmpl
new revision: 1.9; previous revision: 1.8
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.66; previous revision: 1.65
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.234; previous revision: 1.233
done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Blocks: 252003
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: