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)
Bugzilla
Creating/Changing Bugs
Tracking
()
RESOLVED
FIXED
Bugzilla 2.20
People
(Reporter: Wurblzap, Assigned: Wurblzap)
References
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
kiko
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•20 years ago
|
||
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.
Assignee | ||
Comment 2•20 years ago
|
||
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'
Assignee | ||
Comment 4•20 years ago
|
||
Going with your suggestion.
You have witnessed my pains of being non-native...
Attachment #151715 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #151715 -
Flags: review?(gerv)
Assignee | ||
Updated•20 years ago
|
Attachment #151857 -
Flags: review?(gerv)
Comment 5•20 years ago
|
||
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-
Assignee | ||
Comment 6•20 years ago
|
||
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
Assignee | ||
Comment 7•20 years ago
|
||
Thoroughly tested, and works very well for me.
Attachment #153504 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #153670 -
Flags: review?
Assignee | ||
Updated•20 years ago
|
Attachment #151857 -
Flags: review?(gerv)
Comment 8•20 years ago
|
||
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-
Assignee | ||
Comment 9•20 years ago
|
||
(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.
Assignee | ||
Updated•20 years ago
|
Attachment #153670 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #156874 -
Flags: review?
Comment 10•20 years ago
|
||
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-
Updated•20 years ago
|
Assignee: myk → marcschum
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 11•20 years ago
|
||
(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.
Assignee | ||
Updated•20 years ago
|
Attachment #156874 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #156971 -
Flags: review?
Comment 12•20 years ago
|
||
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+
Updated•20 years ago
|
Status: NEW → ASSIGNED
Flags: approval?
Updated•20 years ago
|
Flags: approval? → approval+
Comment 13•20 years ago
|
||
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
Updated•20 years ago
|
Target Milestone: --- → Bugzilla 2.20
Comment 14•20 years ago
|
||
*** Bug 199715 has been marked as a duplicate of this bug. ***
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•