Closed
Bug 299848
Opened 20 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)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.20
People
(Reporter: timeless, Assigned: mkanat)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Wurblzap
:
review+
|
Details | Diff | Splinter Review |
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).
Assignee | ||
Updated•20 years ago
|
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
Updated•20 years ago
|
Flags: blocking2.20? → blocking2.20+
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
Assignee | ||
Comment 1•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
Whiteboard: [patch awaiting review]
Comment 2•19 years ago
|
||
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-
Updated•19 years ago
|
Whiteboard: [patch awaiting review] → [bug awaiting patch]
Assignee | ||
Comment 3•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
Whiteboard: [bug awaiting patch] → [patch awaiting review]
Comment 4•19 years ago
|
||
Comment on attachment 192642 [details] [diff] [review] v2 pickplatform() and pickos() should definitely be in a separate file.
Assignee | ||
Comment 5•19 years ago
|
||
(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.
Updated•19 years ago
|
Attachment #192642 -
Flags: review?(wurblzap) → review+
Updated•19 years ago
|
Flags: approval?
Flags: approval2.20?
Comment 6•19 years ago
|
||
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+
Assignee | ||
Comment 7•19 years ago
|
||
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]
Assignee | ||
Comment 8•19 years ago
|
||
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.
Description
•