Closed Bug 248613 Opened 20 years ago Closed 20 years ago

Custom global default platform/OS in non-usebrowserinfo scenarios

Categories

(Bugzilla :: Creating/Changing Bugs, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: Wurblzap, Assigned: Wurblzap)

References

Details

Attachments

(1 file, 5 obsolete files)

User-Agent: Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0) Build Identifier: I think it would be a good idea to be able to set global platform/OS defaults other than Other/other. Reproducible: Always Steps to Reproduce: 1. Switch usebrowserinfo off. 2. Go to enter_bug.cgi. 3. Choose a product. Actual Results: Platform defaults to Other, OS defaults to other. There is no way to have other defaults short of hacking enter_bug.cgi. Expected Results: Bugzilla should allow an administrator to specify the defaults for platform and OS. I'm going to file an update of attachment 149523 [details] [diff] [review] here (from bug 9410) as a patch which does this. This bug will be obsoleted by bug 9410, but it does cover the time until then.
Attached patch Patch (obsolete) (deleted) — Splinter Review
With this patch, the default platform or OS is the last entry in the list as specified in localconfig. The standard localconfig has Other/other as the last entry, so the patch doesn't change existing installations. You can change the default globally to All/All by reordering the list and putting these at the end. checksetup.pl issues a warning message if the default changed.
Comment on attachment 151715 [details] [diff] [review] Patch Gerv, you seemed pretty much inclined towards this patch over at bug 9410, so I hope you don't mind me asking you for a review...
Attachment #151715 - Flags: review?(gerv)
Comment on attachment 151715 [details] [diff] [review] Patch > # What operatings systems may your products run on? >+# Users may choose from this list when entering a new bug. this line, in context is confusing. It might be sufficient to add an empty #'d line between this line and the next line: >+# Unless \'usebrowserinfo\' in the system parameters is set to \'on\', >+# the last entry specified here serves as the default. 'specified' is fairly awkward, would 'listed' be ok? and instead of 'serves' perhaps 'is used'
Attached patch Patch with changes from comment 3 (obsolete) (deleted) — Splinter Review
Going with your suggestion. You have witnessed my pains of being non-native...
Attachment #151715 - Attachment is obsolete: true
Attachment #151715 - Flags: review?(gerv)
Attachment #151857 - Flags: review?(gerv)
Comment on attachment 151857 [details] [diff] [review] Patch with changes from comment 3 This is such an ugly hack, from conceptual point of view. A lot of administrators will probably want to keep the specific order already present. I mean, it makes sense to have Windows 98 between Windows 95 and Windows ME. If I would like to make Windows 98 the default, I don't think I would need to move it at the bottom of that list. I suggest the obvious and more clean solution, which is keeping the ID or the value of the default option somewhere else(localconfig/DB) and use that.
Attachment #151857 - Flags: review-
This is a more comprehensive way to go at it. The usebrowserinfo parameter is gone and replaced by defaultplatform and defaultopsys, which act as usebrowserinfo if left empty. What do you think about this?
Attachment #151857 - Attachment is obsolete: true
Thoroughly tested, and works very well for me.
Attachment #153504 - Attachment is obsolete: true
Attachment #153670 - Flags: review?
Attachment #151857 - Flags: review?(gerv)
Comment on attachment 153670 [details] [diff] [review] Polished patch: defaults for platform/os, incorporating usebrowserinfo Oh, this is cool! >Index: defparams.pl >+ if (lsearch(['', @::legal_platform], $value) < Isn't this better written as lsearch(\@::legal_platform, $value) -- ? At least from reading the code it appears that's what you want.. >Index: enter_bug.cgi >- if ( Param('usebrowserinfo') ) { >+ if ( Param('defaultplatform') eq '' ) { [nit] You don't actually need to check if it's empty, because an empty string evaluates to false. And, to be honest, maybe we should invert these checks: if (Param('defaultplatform')) { } else { } reads easier. > sub pickos { > if (formvalue('op_sys') ne "") { > return formvalue('op_sys'); > } >- if ( Param('usebrowserinfo') ) { >+ if ( Param('defaultopsys') eq '' ) { same here.
Attachment #153670 - Flags: review? → review-
Attached patch Patch v2.3 (obsolete) (deleted) — Splinter Review
(In reply to comment #8) > >Index: defparams.pl > >+ if (lsearch(['', @::legal_platform], $value) < > > Isn't this better written as > > lsearch(\@::legal_platform, $value) > > -- ? At least from reading the code it appears that's what you want.. I need to allow the empty string, too. > >Index: enter_bug.cgi > >- if ( Param('usebrowserinfo') ) { > >+ if ( Param('defaultplatform') eq '' ) { > > [nit] You don't actually need to check if it's empty, because an empty string > evaluates to false. > > And, to be honest, maybe we should invert these checks: > > if (Param('defaultplatform')) { > } else { > } > > reads easier. I agree. Done. > > sub pickos { > > if (formvalue('op_sys') ne "") { > > return formvalue('op_sys'); > > } > >- if ( Param('usebrowserinfo') ) { > >+ if ( Param('defaultopsys') eq '' ) { > > same here. Done. In addition to this, I enhanced the transition coding in Config.pm a bit to have a transparent upgrade from usebrowserinfo.
Attachment #153670 - Attachment is obsolete: true
Attachment #156874 - Flags: review?
Comment on attachment 156874 [details] [diff] [review] Patch v2.3 >diff -x CVS -x data -r -u3 orig/Bugzilla/Config.pm patched/Bugzilla/Config.pm >+ if (!exists $param{'defaultplatform'}) { >+ $param{'defaultplatform'} = 'Other'; >+ } >+ if (!exists $param{'defaultopsys'}) { >+ $param{'defaultopsys'} = 'other'; Would "Unspecified/unspecified" be better choices here? They are definitely "more correct" than other. This is something I'd like to get someone else's opinion on, but for me it would be right on. >--- orig/defparams.pl 2004-08-21 >+ name => 'defaultplatform', >+ desc => 'This is the platform that is preselected on the bug '. >+ 'entry form.<br>'. >+ 'You can leave this empty. '. >+ 'Bugzilla will then use the platform that the browser '. >+ 'reports to be running on as the default.', I'd suggest using a colon to join the phrases: You can leave this empty: Bugzilla will then ... >+ name => 'defaultopsys', >+ desc => 'This is the operating system that is preselected on the bug '. >+ 'entry form.<br>'. >+ 'You can leave this empty. '. >+ 'Bugzilla will then use the operating system that the browser '. >+ 'reports to be running on as the default.', Same here. >diff -x CVS -x data -r -u3 orig/enter_bug.cgi >+ [% IF Param('defaultplatform') %] >+ operating system. Please check it >+ [% ELSE %] >+ [% IF Param('defaultopsys') %] >+ platform. Please check it You can use ELSIF here to simplify.
Attachment #156874 - Flags: review? → review-
Assignee: myk → marcschum
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Patch v2.4 (deleted) — Splinter Review
(In reply to comment #10) > >+ $param{'defaultopsys'} = 'other'; > > Would "Unspecified/unspecified" be better choices here? They are definitely > "more correct" than other. I would propose "default", but neither value is included in the default lists for platform or OS in localconfig. Moreover, enter_bug.cgi hardcodedly uses "other", and this behaviour should imo be retained when upgrading. > I'd suggest using a colon to join the phrases: > > You can leave this empty: Bugzilla will then ... Ok. > >+ [% ELSE %] > >+ [% IF Param('defaultopsys') %] > > You can use ELSIF here to simplify. True.
Attachment #156874 - Attachment is obsolete: true
Attachment #156971 - Flags: review?
Comment on attachment 156971 [details] [diff] [review] Patch v2.4 Yes, I guess you're right about undefined. I should file a new bug for that, though.
Attachment #156971 - Flags: review? → review+
Status: NEW → ASSIGNED
Flags: approval?
Flags: approval? → approval+
Checking in defparams.pl; /cvsroot/mozilla/webtools/bugzilla/defparams.pl,v <-- defparams.pl new revision: 1.136; previous revision: 1.135 done Checking in enter_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v <-- enter_bug.cgi new revision: 1.96; previous revision: 1.95 done Checking in Bugzilla/Config.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config.pm,v <-- Config.pm new revision: 1.26; previous revision: 1.25 done Checking in template/en/default/bug/create/create.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create.html.tmpl,v <-- create.html.tmpl new revision: 1.34; previous revision: 1.33 done
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Target Milestone: --- → Bugzilla 2.20
*** Bug 199715 has been marked as a duplicate of this bug. ***
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: