Closed Bug 299848 Opened 19 years ago Closed 19 years ago

enter_bug's automatic OS/Platform code does not work with the new default OS list

Categories

(Bugzilla :: Creating/Changing Bugs, defect, P2)

2.19.2

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: timeless, Assigned: mkanat)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

mkanat broke enter_bug for new bugzilla installs.

pickos/pickplatform expect things to be in specific places.

needless to say i'm not happy, this was my code and if i gave any review i must
have been asleep at the wheel when it happened.

the reason i found this bug is that i went to see about adding mac os x 10.4 to
the bugzilla os list, because bmo really needs it (i need to be able to find
java-10.4 crashers without reading every bug in the db).
Severity: blocker → major
Flags: blocking2.20?
OS: MacOS X → All
Hardware: Macintosh → All
Summary: enter_bug should currently generate bogus os fields for fresh installs → enter_bug's automatic OS/Platform code does not work with the new default OS list
Target Milestone: --- → Bugzilla 2.20
Flags: blocking2.20? → blocking2.20+
Status: NEW → ASSIGNED
Priority: -- → P2
Okay. I've patched the pickos/pickplatform subs in enter_bug to operate
correctly with both lists. Basically, we just return the first *valid* choice
for platform/op_sys from a list of possible choices. If there are no valid
choices, we return "Other."

Because old Bugzillas use "other" (lowercase O) for op_sys, I had to make that
consistent in checksetup.
Attachment #189580 - Flags: review?(timeless)
Whiteboard: [patch awaiting review]
Blocks: 296054
Comment on attachment 189580 [details] [diff] [review]
Make pick* functions work with both old and new list

You need to change the code Bugzilla/Config.pm upgrading from usebrowserinfo,
too, so that defaultopsys gets its capital O.

>Index: checksetup.pl
>+# Old Bugzillas have "other" as an OS choice, new ones have "Other"
>+# (capital O).
>+# XXX - This should be moved inside of a later schema change, once
>+#       we have one to move it to the inside of.
>+print "Setting any 'other' op_sys to 'Other'...\n";
>+$dbh->do('UPDATE op_sys SET value = ? WHERE value = ?', 
>+         undef, "Other", "other");
>+$dbh->do('UPDATE bugs SET op_sys = ? WHERE op_sys = ?',
>+         undef, "Other", "other");

You need to check the defaultopsys parameter whether you need to update "other"
to "Other" here, too.

>Index: enter_bug.cgi
>+# Takes the name of a field and a list of possible values for that 
>+# field. Returns the first value in the list that is actually a 
>+# valid value for that field.
>+# The field should be named after its DB table.
>+# Returns undef if none of the platforms match.
>+sub pick_valid_field_value (@) {
>+    my ($field, @values) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    foreach my $value (@values) {
>+        return $value if $dbh->selectrow_array(
>+            "SELECT 1 FROM $field WHERE value = ?", undef, $value); 
>+    }
>+    return undef;
>+}

Suggestion: maybe this can return "Other" directly if it doesn't find a value
so that you don't need to do it at the callsites?

> sub pickplatform {
>     if (Param('defaultplatform')) {
>-        return Param('defaultplatform');
>+        $platform = Param('defaultplatform');
>     } else {

Hmmm... I don't know... On the one hand, defaultplatform is already validated
against the legal platform values, so you could return it immediately. On the
other hand, somebody might have changed the list of legal platforms after the
parameter has been set. Thoughts on this?

The same goes for defaultopsys.

Suggestion: even if not necessary from the way you wrote it, maybe you want to
consider using a list for the platform variable in pickplatform (@platform
instead of $platform) so that the code is similar to pickos.
Attachment #189580 - Flags: review?(timeless) → review-
Whiteboard: [patch awaiting review] → [bug awaiting patch]
Attached patch v2Splinter Review
I addressed most of your comments. I wanted to leave the 'Other' in the
callers, in enter_bug.

I couldn't really set the param, because checksetup isn't modular enough. See
my comment in checksetup. But I do print a warning for the user.

After this code is checked in, the checksetup parts will be moved inside of a
later database schema change, so that they don't run every time we run
checksetup.
Attachment #189580 - Attachment is obsolete: true
Attachment #192642 - Flags: review?(wurblzap)
Whiteboard: [bug awaiting patch] → [patch awaiting review]
Comment on attachment 192642 [details] [diff] [review]
v2

pickplatform() and pickos() should definitely be in a separate file.
(In reply to comment #4)
> (From update of attachment 192642 [details] [diff] [review] [edit])
> pickplatform() and pickos() should definitely be in a separate file.

  OK. We can do that in another bug that's not as critical as this one.
Attachment #192642 - Flags: review?(wurblzap) → review+
Flags: approval?
Flags: approval2.20?
This should get lots of testing in the next 2.20 release candidate.  Let's add
that to the release notes so anyone using the candidate knows to pay special
attention to it.
Flags: approval?
Flags: approval2.20?
Flags: approval2.20+
Flags: approval+
Actually, I'm not too certain there will be another release candidate -- I was
expecting the next release to be 2.20 final. However, I did actually try to
modify this code as little as possible so as not to affect folks who have the
two different OS lists.

tip:

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.427; previous revision: 1.426
done
Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.116; previous revision: 1.115
done
Checking in Bugzilla/Config.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config.pm,v  <--  Config.pm
new revision: 1.45; previous revision: 1.44
done

2.20:

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.412.2.6; previous revision: 1.412.2.5
done
Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.114.4.1; previous revision: 1.114
done
Checking in Bugzilla/Config.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config.pm,v  <--  Config.pm
new revision: 1.43.2.3; previous revision: 1.43.2.2
done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Keywords: relnote
Resolution: --- → FIXED
Whiteboard: [patch awaiting review]
Blocks: 304997
I don't think this requires any particular relnote for the final 2.20 release,
that I can think of.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: