248613 - Custom global default platform/OS in non-usebrowserinfo scenarios

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Marc Schumann [:Wurblzap]]

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.

Comment 1

[Marc Schumann [:Wurblzap]]

Attachment: 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.

Comment 2

[Marc Schumann [:Wurblzap]]

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...

Comment 3

[timeless]

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'

Comment 4

[Marc Schumann [:Wurblzap]]

Attachment: Patch with changes from comment 3

Going with your suggestion.
You have witnessed my pains of being non-native...

Comment 5

[Vlad Dascalu]

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.

Comment 6

[Marc Schumann [:Wurblzap]]

Attachment: 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?

Comment 7

[Marc Schumann [:Wurblzap]]

Attachment: Polished patch: defaults for platform/os, incorporating usebrowserinfo

Thoroughly tested, and works very well for me.

Comment 8

[Christian Reis]

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.

Comment 9

[Marc Schumann [:Wurblzap]]

Attachment: 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.

Comment 10

[Christian Reis]

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.

Comment 11

[Marc Schumann [:Wurblzap]]

Attachment: 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.

Comment 12

[Christian Reis]

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.

Comment 13

[Vlad Dascalu]

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

Comment 14

[Shane H. W. Travis]

*** Bug 199715 has been marked as a duplicate of this bug. ***