Custom global default platform/OS in non-usebrowserinfo scenarios

RESOLVED FIXED in Bugzilla 2.20

Status

()

Bugzilla
Creating/Changing Bugs
--
enhancement
RESOLVED FIXED
14 years ago
6 years ago

People

(Reporter: Wurblzap, Assigned: Wurblzap)

Tracking

unspecified
Bugzilla 2.20
Bug Flags:
approval +

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

14 years ago
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

14 years ago
Created attachment 151715 [details] [diff] [review]
Patch

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

14 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 3

14 years ago
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

14 years ago
Created attachment 151857 [details] [diff] [review]
Patch with changes from comment 3

Going with your suggestion.
You have witnessed my pains of being non-native...
Attachment #151715 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #151715 - Flags: review?(gerv)
(Assignee)

Updated

14 years ago
Attachment #151857 - Flags: review?(gerv)

Comment 5

14 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

14 years ago
Created attachment 153504 [details] [diff] [review]
Parameters for default platform and OS; incorporating usebrowserinfo

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

14 years ago
Created attachment 153670 [details] [diff] [review]
Polished patch: defaults for platform/os, incorporating usebrowserinfo

Thoroughly tested, and works very well for me.
Attachment #153504 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #153670 - Flags: review?
(Assignee)

Updated

14 years ago
Attachment #151857 - Flags: review?(gerv)

Comment 8

14 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

14 years ago
Created attachment 156874 [details] [diff] [review]
Patch v2.3

(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

14 years ago
Attachment #153670 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #156874 - Flags: review?

Comment 10

14 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

14 years ago
Assignee: myk → marcschum
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 11

14 years ago
Created attachment 156971 [details] [diff] [review]
Patch v2.4

(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

14 years ago
Attachment #156874 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #156971 - Flags: review?

Comment 12

14 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

14 years ago
Status: NEW → ASSIGNED
Flags: approval?
Flags: approval? → approval+

Comment 13

14 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
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

14 years ago
Target Milestone: --- → Bugzilla 2.20

Comment 14

13 years ago
*** 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.