343809 - Merge FlagType::validate() with Flag::validate()

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement)

Description

[Frédéric Buclin]

FlagType::validate() has 90% of its code which is the same as in Flag::validate(). The reason is that FlagType is validating... flags, not flagtypes! (more exactly, potentially new flags, but they remain flags). So FlagType::validate() has to go away.

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

Requesting review from myk as this patch involves security checks. That's my last patch before the final one about flags. \o/ Applying my patch from bug 345929 before testing this one is definitely a good idea.

Comment 2

[Frédéric Buclin]

I already found an error in my patch. In _validate(), it should be:

 $attach_id ||= $flag->attach_id if $flag; # Maybe it's a bug flag.

i.e. adding "if $flag". I will do additional tests tomorrow and fix my patch accordingly. But you can already have a look if you want. ;)

Comment 3

[Frédéric Buclin]

Attachment: patch, v1.1

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 230750 [details] [diff] [review]
patch, v1.1

>+sub bug_id { return $_[0]->{'bug_id'}; }
>+sub attach_id { return $_[0]->{'attach_id'}; }

These accessors should be documented in the pod section above these statements.


>+    # All new flags must belong to active flag types.
>+    if (scalar(@flagtype_ids)) {
>+        my $inactive_flagtypes =
>+            $dbh->selectrow_array('SELECT 1 FROM flagtypes
>+                                   WHERE id IN (' . join(',', @flagtype_ids) . ')
>+                                   AND is_active = 0 ' .
>+                                   $dbh->sql_limit(1));
>+
>+        ThrowCodeError('flag_type_inactive') if $inactive_flagtypes;

It'd be nice if the error message displayed the flag types that were invalid.


>+    # We don't check that these new flags are valid for this bug/attachment.
>+    # This check will be done later when creating new flags, see FormToNewFlags().

Shouldn't FormToNewFlags then call _validate?


Otherwise this looks fine, although it doesn't apply anymore.  I looked through the logic, much of which seems to have been refactored, and I didn't notice any errors.  But then it's fairly complicated, and I don't understand it well anymore, so it's unclear how likely I am to find errors anyway.

Comment 5

[Frédéric Buclin]

(In reply to comment #4)

> >+        ThrowCodeError('flag_type_inactive') if $inactive_flagtypes;
> 
> It'd be nice if the error message displayed the flag types that were invalid.

No need to fix that. I will remove this error as part of bug 345958.


> >+    # We don't check that these new flags are valid for this bug/attachment.
> >+    # This check will be done later when creating new flags, see FormToNewFlags().
> 
> Shouldn't FormToNewFlags then call _validate?

No. The reason is that flags are first validated, then bug changes are processed, then flags are created/updated/deleted. If the bug has been moved to another product meanwhile where the inclusion and/or exclusion lists are different, we have to ignore these invalid flags. I added a comment in the file explaining that.

Comment 6

[Frédéric Buclin]

Attachment: patch, v1.2

Unbitrot + add POD for bug_id and attach_id.

Comment 7

[Frédéric Buclin]

myk, if you want to review this patch, taking into account my comment 5, feel free. Else I'm requesting approval directly.

Comment 8

[Frédéric Buclin]

Attachment: patch, v1.3

Fixing two comments which were still mentioning FlagType::validate.

Comment 9

[Frédéric Buclin]

Comment on attachment 235136 [details] [diff] [review]
patch, v1.3

wurblzap suggested that module owners r+ their own patches themselves so that they appear in the reviewer mailing list. So here is for you, Marc. :)

Comment 10

[Frédéric Buclin]

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.121; previous revision: 1.120
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.169; previous revision: 1.168
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.338; previous revision: 1.337
done
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.39; previous revision: 1.38
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.74; previous revision: 1.73
done
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
new revision: 1.37; previous revision: 1.36
done