Bug 1209599 - group general preferences by category

15 files changed, 177 insertions, 96 deletions

diff --git a/Bugzilla/Install.pm b/Bugzilla/Install.pm
index 9950b222a..715251154 100644
--- a/Bugzilla/Install.pm
+++ b/Bugzilla/Install.pm
@@ -281,13 +281,8 @@ sub update_settings {
     }
 
     my @settings = @{SETTINGS()};
-    foreach my $setting (@settings) {
-        add_setting($setting->{name},
-                    $setting->{options},
-                    $setting->{default},
-                    $setting->{subclass},
-                    undef,
-                    !$any_settings);
+    foreach my $params (@settings) {
+        add_setting($params);
     }
 }
 
diff --git a/Bugzilla/User/Setting.pm b/Bugzilla/User/Setting.pm
index e083a251e..f566b3459 100644
--- a/Bugzilla/User/Setting.pm
+++ b/Bugzilla/User/Setting.pm
@@ -66,10 +66,10 @@ sub new {
     # to retrieve the information for this setting ourselves.
     if (scalar @_ == 0) {
 
-        my ($default, $is_enabled, $value);
-        ($default, $is_enabled, $value, $subclass) = 
+        my ($default, $is_enabled, $value, $category);
+        ($default, $is_enabled, $value, $subclass, $category) =
           $dbh->selectrow_array(
-             q{SELECT default_value, is_enabled, setting_value, subclass
+             q{SELECT default_value, is_enabled, setting_value, subclass, category
                  FROM setting
             LEFT JOIN profile_setting
                    ON setting.name = profile_setting.setting_name
@@ -80,17 +80,18 @@ sub new {
 
         # if not defined, then grab the default value
         if (! defined $value) {
-            ($default, $is_enabled, $subclass) =
+            ($default, $is_enabled, $subclass, $category) =
               $dbh->selectrow_array(
-                 q{SELECT default_value, is_enabled, subclass
+                 q{SELECT default_value, is_enabled, subclass, category
                    FROM setting
                    WHERE name = ?},
               undef,
               $setting_name);
         }
 
-        $self->{'is_enabled'} = $is_enabled;
+        $self->{'is_enabled'}    = $is_enabled;
         $self->{'default_value'} = $default;
+        $self->{'category'}      = $category;
 
         # IF the setting is enabled, AND the user has chosen a setting
         # THEN return that value
@@ -109,6 +110,7 @@ sub new {
         $self->{'value'}         = shift;
         $self->{'is_default'}    = shift;
         $subclass                = shift;
+        $self->{'category'}      = shift;
     }
     if ($subclass) {
         eval('require ' . $class . '::' . $subclass);
@@ -129,14 +131,27 @@ sub new {
 ###############################
 
 sub add_setting {
-    my ($name, $values, $default_value, $subclass, $force_check,
-        $silently) = @_;
+    my ($params) = @_;
+    my ($name, $options, $default, $subclass, $force_check, $silently, $category)
+        = @$params{qw( name options default subclass force_check silently category )};
     my $dbh = Bugzilla->dbh;
 
+    # Categories were added later, so we need to check if the old
+    # setting has the correct category if provided
     my $exists = _setting_exists($name);
+    if ($exists && $category) {
+        my $old_category = $dbh->selectrow_arrayref(
+            "SELECT category FROM setting WHERE name = ?", undef, $name);
+        if ($old_category ne $category) {
+            $dbh->do('UPDATE setting SET category = ? WHERE name = ?',
+                     undef, $category, $name);
+            Bugzilla->memcached->clear_config();
+        }
+    }
+
     return if ($exists && !$force_check);
 
-    ($name && $default_value)
+    ($name && $default)
       ||  ThrowCodeError("setting_info_invalid");
 
     if ($exists) {
@@ -144,8 +159,8 @@ sub add_setting {
         $dbh->do('DELETE FROM setting_value WHERE name = ?', undef, $name);
         $dbh->do('DELETE FROM setting WHERE name = ?', undef, $name);
         # Remove obsolete user preferences for this setting.
-        if (defined $values && scalar(@$values)) {
-            my $list = join(', ', map {$dbh->quote($_)} @$values);
+        if (defined $options && scalar(@$options)) {
+            my $list = join(', ', map {$dbh->quote($_)} @$options);
             $dbh->do("DELETE FROM profile_setting
                       WHERE setting_name = ? AND setting_value NOT IN ($list)",
                       undef, $name);
@@ -154,15 +169,15 @@ sub add_setting {
     elsif (!$silently) {
         print get_text('install_setting_new', { name => $name }) . "\n";
     }
-    $dbh->do(q{INSERT INTO setting (name, default_value, is_enabled, subclass)
-                    VALUES (?, ?, 1, ?)},
-             undef, ($name, $default_value, $subclass));
+    $dbh->do(q{INSERT INTO setting (name, default_value, is_enabled, subclass, category)
+                    VALUES (?, ?, 1, ?, ?)},
+             undef, ($name, $default, $subclass, $category));
 
     my $sth = $dbh->prepare(q{INSERT INTO setting_value (name, value, sortindex)
                                     VALUES (?, ?, ?)});
 
     my $sortindex = 5;
-    foreach my $key (@$values){
+    foreach my $key (@$options){
         $sth->execute($name, $key, $sortindex);
         $sortindex += 5;
     }
@@ -177,7 +192,7 @@ sub get_all_settings {
     my $rows = Bugzilla->memcached->get_config({ key => $cache_key });
     if (!$rows) {
         $rows = $dbh->selectall_arrayref(
-            q{SELECT name, default_value, is_enabled, setting_value, subclass
+            q{SELECT name, default_value, is_enabled, setting_value, subclass, category
                 FROM setting
            LEFT JOIN profile_setting
                      ON setting.name = profile_setting.setting_name
@@ -186,7 +201,7 @@ sub get_all_settings {
     }
 
     foreach my $row (@$rows) {
-        my ($name, $default_value, $is_enabled, $value, $subclass) = @$row;
+        my ($name, $default_value, $is_enabled, $value, $subclass, $category) = @$row;
 
         my $is_default;
 
@@ -199,7 +214,8 @@ sub get_all_settings {
 
         $settings->{$name} = new Bugzilla::User::Setting(
            $name, $user_id, $is_enabled,
-           $default_value, $value, $is_default, $subclass);
+           $default_value, $value, $is_default,
+           $subclass, $category);
     }
 
     return $settings;
@@ -217,15 +233,15 @@ sub get_defaults {
 
     $user_id ||= 0;
 
-    my $rows = $dbh->selectall_arrayref(q{SELECT name, default_value, is_enabled, subclass
+    my $rows = $dbh->selectall_arrayref(q{SELECT name, default_value, is_enabled, subclass, category
                                             FROM setting});
 
     foreach my $row (@$rows) {
-        my ($name, $default_value, $is_enabled, $subclass) = @$row;
+        my ($name, $default_value, $is_enabled, $subclass, $category) = @$row;
 
         $default_settings->{$name} = new Bugzilla::User::Setting(
             $name, $user_id, $is_enabled, $default_value, $default_value, 1,
-            $subclass);
+            $subclass, $category);
     }
 
     return $default_settings;
@@ -347,7 +363,7 @@ $settings->{$setting_name} = new Bugzilla::User::Setting(
 
 =over 4
 
-=item C<add_setting($name, \@values, $default_value, $subclass, $force_check)>
+=item C<add_setting($name, \@values, $default_value, $subclass, $force_check, $category)>
 
 Description: Checks for the existence of a setting, and adds it 
              to the database if it does not yet exist.
@@ -361,6 +377,8 @@ Params:      C<$name> - string - the name of the new setting
                not stored in the DB.
              C<$force_check> - boolean - when true, the existing setting
                and all its values are deleted and replaced by new data.
+             C<$category> - string - Category the setting should be
+               grouped by.
 
 Returns:     a pointer to a hash of settings
 
diff --git a/extensions/BMO/Extension.pm b/extensions/BMO/Extension.pm
index 6e04c2637..67f7cc160 100644
--- a/extensions/BMO/Extension.pm
+++ b/extensions/BMO/Extension.pm
@@ -1089,13 +1089,21 @@ sub install_before_final_checks {
     my ($self, $args) = @_;
 
     # Add product chooser setting
-    add_setting('product_chooser',
-                ['pretty_product_chooser', 'full_product_chooser'],
-                'pretty_product_chooser');
+    add_setting({
+        name     => 'product_chooser',
+        options  => ['pretty_product_chooser', 'full_product_chooser'],
+        default  => 'pretty_product_chooser',
+        category => 'User Interface'
+    });
 
     # Add option to inject x-bugzilla headers into the message body to work
     # around gmail filtering limitations
-    add_setting('headers_in_body', ['on', 'off'], 'off');
+    add_setting({
+        name     => 'headers_in_body',
+        options  => ['on', 'off'],
+        default  => 'off',
+        category => 'Email Notifications'
+    });
 
     # Migrate from 'gmail_threading' setting to 'bugmail_new_prefix'
     my $dbh = Bugzilla->dbh;
diff --git a/extensions/BugModal/Extension.pm b/extensions/BugModal/Extension.pm
index 2c60ef35a..e929f3fcb 100644
--- a/extensions/BugModal/Extension.pm
+++ b/extensions/BugModal/Extension.pm
@@ -292,8 +292,18 @@ sub webservice {
 
 sub install_before_final_checks {
     my ($self, $args) = @_;
-    add_setting('ui_experiments', ['on', 'off'], 'off');
-    add_setting('ui_remember_collapsed', ['on', 'off'], 'off');
+    add_setting({
+        name     => 'ui_experiments',
+        options  => ['on', 'off'],
+        default  => 'off',
+        category => 'User Interface'
+    });
+    add_setting({
+        name     => 'ui_remember_collapsed',
+        options  => ['on', 'off'],
+        default  => 'off',
+        category => 'User Interface'
+    });
 
     # ensure the correct skin is being used
     my $dbh = Bugzilla->dbh;
diff --git a/extensions/Example/Extension.pm b/extensions/Example/Extension.pm
index 5b76935e3..4fd2d987f 100644
--- a/extensions/Example/Extension.pm
+++ b/extensions/Example/Extension.pm
@@ -543,10 +543,11 @@ sub install_before_final_checks {
     
     # Add a new user setting like this:
     #
-    # add_setting('product_chooser',           # setting name
-    #             ['pretty', 'full', 'small'], # options
-    #             'pretty');                   # default
-    #
+    # add_setting({
+    #     name     => 'product_chooser',           # setting name
+    #     options  => ['pretty', 'full', 'small'], # options
+    #     category => 'pretty'                     # default
+    # });
     # To add descriptions for the setting and choices, add extra values to 
     # the hash defined in global/setting-descs.none.tmpl. Do this in a hook: 
     # hook/global/setting-descs-settings.none.tmpl .
diff --git a/extensions/Gravatar/Extension.pm b/extensions/Gravatar/Extension.pm
index 3338790e7..06e98fb78 100644
--- a/extensions/Gravatar/Extension.pm
+++ b/extensions/Gravatar/Extension.pm
@@ -38,8 +38,18 @@ sub _user_gravatar {
 
 sub install_before_final_checks {
     my ($self, $args) = @_;
-    add_setting('show_gravatars', ['On', 'Off'], 'Off');
-    add_setting('show_my_gravatar', ['On', 'Off'], 'On');
+    add_setting({
+        name     => 'show_gravatars',
+        options  => ['On', 'Off'],
+        default  => 'Off',
+        category => 'Bug Editing'
+    });
+    add_setting({
+        name     => 'show_my_gravatar',
+        options  => ['On', 'Off'],
+        default  => 'On',
+        category => 'Bug Editing'
+    });
 }
 
 __PACKAGE__->NAME;
diff --git a/extensions/InlineHistory/Extension.pm b/extensions/InlineHistory/Extension.pm
index 86536719f..d563bede8 100644
--- a/extensions/InlineHistory/Extension.pm
+++ b/extensions/InlineHistory/Extension.pm
@@ -231,7 +231,12 @@ sub _add_duplicates {
 
 sub install_before_final_checks {
     my ($self, $args) = @_;
-    add_setting('inline_history', ['on', 'off'], 'off');
+    add_setting({
+        name     => 'inline_history',
+        options  => ['on', 'off'],
+        default  => 'off',
+        category => 'Bug Editing'
+    });
 }
 
 __PACKAGE__->NAME;
diff --git a/extensions/Needinfo/Extension.pm b/extensions/Needinfo/Extension.pm
index 7d94fb9a4..c1659a1eb 100644
--- a/extensions/Needinfo/Extension.pm
+++ b/extensions/Needinfo/Extension.pm
@@ -58,7 +58,12 @@ sub install_update_db {
 
 sub install_before_final_checks {
     my ($self, $args) = @_;
-    add_setting('block_needinfo', ['on', 'off'], 'off');
+    add_setting({
+        name     => 'block_needinfo',
+        options  => ['on', 'off'],
+        default  => 'off',
+        category => 'Reviews and Needinfo'
+    });
 }
 
 # Clear the needinfo? flag if comment is being given by
diff --git a/extensions/OrangeFactor/Extension.pm b/extensions/OrangeFactor/Extension.pm
index af629e323..14d500ba0 100644
--- a/extensions/OrangeFactor/Extension.pm
+++ b/extensions/OrangeFactor/Extension.pm
@@ -37,7 +37,12 @@ sub template_before_process {
 
 sub install_before_final_checks {
     my ($self, $args) = @_;
-    add_setting('orange_factor', ['on', 'off'], 'off');
+    add_setting({
+        name     => 'orange_factor',
+        options  => ['on', 'off'],
+        default  => 'off',
+        category => 'User Interface'
+    });
 }
 
 __PACKAGE__->NAME;
diff --git a/extensions/RequestNagger/Extension.pm b/extensions/RequestNagger/Extension.pm
index 169f76b1e..63d79040e 100644
--- a/extensions/RequestNagger/Extension.pm
+++ b/extensions/RequestNagger/Extension.pm
@@ -379,7 +379,12 @@ sub install_filesystem {
 
 sub install_before_final_checks {
     my ($self, $args) = @_;
-    add_setting('request_nagging', ['on', 'off'], 'on');
+    add_setting({
+        name     => 'request_nagging',
+        options  => ['on', 'off'],
+        default  => 'on',
+        category => 'Reviews and Needinfo'
+    });
 }
 
 __PACKAGE__->NAME;
diff --git a/extensions/Review/Extension.pm b/extensions/Review/Extension.pm
index 84ab3e77b..4fd965b4c 100644
--- a/extensions/Review/Extension.pm
+++ b/extensions/Review/Extension.pm
@@ -1041,7 +1041,12 @@ sub install_filesystem {
 
 sub install_before_final_checks {
     my ($self, $args) = @_;
-    add_setting('block_reviews', ['on', 'off'], 'off');
+    add_setting({
+        name     => 'block_reviews',
+        options  => ['on', 'off'],
+        default  => 'off',
+        category => 'Reviews and Needinfo'
+    });
 }
 
 sub config_modify_panels {
diff --git a/skins/standard/admin.css b/skins/standard/admin.css
index 6a91965e4..2b56c1831 100644
--- a/skins/standard/admin.css
+++ b/skins/standard/admin.css
@@ -231,6 +231,16 @@ input[disabled] {
     background: #fff;
 }
 
+.category_header {
+    text-align: left;
+    font-weight: bold;
+    font-size: larger;
+}
+
+.setting_label {
+    text-align: right;
+}
+
 /* mfa */
 
 #mfa {
diff --git a/template/en/default/account/prefs/settings.html.tmpl b/template/en/default/account/prefs/settings.html.tmpl
index 0147f95ef..b09d7a491 100644
--- a/template/en/default/account/prefs/settings.html.tmpl
+++ b/template/en/default/account/prefs/settings.html.tmpl
@@ -16,9 +16,9 @@
   #%]
 
 [%# INTERFACE:
-  # setting_names: an array of strings
-  # settings:      a hash of hashes, keyed by setting_name.
-  #                Each hash contains:
+  # settings:      a hash of hashes, keyed by category ame.
+  #                Each hash value is a list of hashes containing:
+  #                 name          - string (name of the setting)
   #                 is_enabled    - boolean
   #                 default_value - string (global default for this setting)
   #                 value         - string (user-defined preference)
@@ -29,7 +29,9 @@
 
 [% PROCESS "global/setting-descs.none.tmpl" %]
 
-[% IF settings.size %]
+[% SET category_names = settings.keys.sort %]
+
+[% IF category_names.size %]
   [% UNLESS has_settings_enabled %]
     <p class="criticalmessages">
       All user preferences have been disabled by the
@@ -39,49 +41,48 @@
   [% END %]
 
   <table border="0" cellpadding="8">
-    [% FOREACH name = setting_names %]
-      [% default_name = name _ '-isdefault' %]
-      [% default_val = settings.${name}.default_value %]
-      <tr id="[% name FILTER html %]_row">
-        <td align="right">
-          [% setting_descs.$name OR name FILTER html %]
+  [% FOREACH category = category_names %]
+    <tr>
+      <td class="category_header">
+        [% category FILTER html %]
+      </td>
+    </tr>
+    [% FOREACH setting = settings.$category %]
+      [% setting_name = setting._setting_name %]
+      [% default_name = setting_name _ '-isdefault' %]
+      [% default_val  = setting.default_value %]
+      <tr id="[% setting_name FILTER html %]_row">
+        <td class="setting_label">
+          [% setting_descs.$setting_name OR setting_name FILTER html %]
         </td>
-        <td>
-          [% IF settings.${name}.is_enabled %]
-            <select name="[% name FILTER html %]" id="[% name FILTER html %]">
-              <option value="[% default_name FILTER html %]"
-                [% ' selected="selected"' IF settings.${name}.is_default %]>
-                Site Default ([% setting_descs.${default_val} OR default_val FILTER html %])
+        <td class="setting_choice">
+          <select name="[% setting_name FILTER html %]" id="[% setting_name FILTER html %]">
+            <option value="[% default_name FILTER html %]"
+              [% ' selected="selected"' IF setting.is_default %]>
+              Site Default ([% setting_descs.${default_val} OR default_val FILTER html %])
+            </option>
+            [% FOREACH x = setting.legal_values %]
+              <option value="[% x FILTER html %]"
+                [% ' selected="selected"'
+                  IF x == setting.value
+                    AND NOT setting.is_default %]>
+                [% setting_descs.${x} OR x FILTER html %]
               </option>
-              [% FOREACH x = settings.${name}.legal_values %]
-                <option value="[% x FILTER html %]"
-                  [% ' selected="selected"' 
-                    IF x == settings.${name}.value
-                    AND NOT settings.${name}.is_default %]>
-                  [% setting_descs.${x} OR x FILTER html %]
-                </option>
-              [% END %]
-            </select>
-            [% IF name == "api_key_only" %]
-              [% INCLUDE "mfa/protected.html.tmpl" %]
             [% END %]
-          [% ELSE %]
-            <select name="[% name FILTER html %]" id="[% name FILTER html %]" disabled="disabled">
-              <option value="[% default_name FILTER html %]">
-                Site Default ([% setting_descs.${default_val} OR default_val FILTER html %])
-              </option>
-            </select>
+          </select>
+          [% IF setting_name == "api_key_only" %]
+            [% INCLUDE "mfa/protected.html.tmpl" %]
           [% END %]
         </td>
       </tr>
     [% END %]
+  [% END %]
   </table>
 [% END %]
-<br>
 
 <script>
-YAHOO.util.Event.onDOMReady(function() {
+$().ready(function() {
   var id = document.location.hash.substring(1) + '_row';
-  YAHOO.util.Dom.addClass(id, 'highlighted');
+  $('#' + id).addClass('highlighted');
 });
 </script>
diff --git a/template/en/default/admin/settings/edit.html.tmpl b/template/en/default/admin/settings/edit.html.tmpl
index 7f95f883e..eeb6c3203 100644
--- a/template/en/default/admin/settings/edit.html.tmpl
+++ b/template/en/default/admin/settings/edit.html.tmpl
@@ -51,6 +51,7 @@ page, and the Default Value will automatically apply to everyone.
       <table border="1" cellpadding="4">
       <tr>
         <th>Preference Text</th>
+        <th>Category</th>
         <th>Default Value</th>
         <th>Enabled</th>
       </tr>
@@ -61,6 +62,9 @@ page, and the Default Value will automatically apply to everyone.
             <td align="right">
               [% setting_descs.$name OR name FILTER html %]
             </td>
+            <td align="left">
+              [% settings.$name.category FILTER html %]
+            </td>
             <td>
               <select name="[% name FILTER html %]" id="[% name FILTER html %]">
                 [% FOREACH x = settings.${name}.legal_values %]
diff --git a/userprefs.cgi b/userprefs.cgi
index bd1bb8ab7..3f5d2038c 100755
--- a/userprefs.cgi
+++ b/userprefs.cgi
@@ -228,21 +228,20 @@ sub DisableAccount {
 sub DoSettings {
     my $user = Bugzilla->user;
 
-    my $settings = $user->settings;
-    $vars->{'settings'} = $settings;
-
-    my @setting_list = sort keys %$settings;
-    $vars->{'setting_names'} = \@setting_list;
-
-    $vars->{'has_settings_enabled'} = 0;
-    # Is there at least one user setting enabled?
-    foreach my $setting_name (@setting_list) {
-        if ($settings->{"$setting_name"}->{'is_enabled'}) {
-            $vars->{'has_settings_enabled'} = 1;
-            last;
-        }
+    my %settings;
+    my $has_settings_enabled = 0;
+    foreach my $name (sort keys %{ $user->settings }) {
+        my $setting = $user->settings->{$name};
+        next if !$setting->{is_enabled};
+        my $category = $setting->{category};
+        $settings{$category} ||= [];
+        push(@{ $settings{$category} }, $setting);
+        $has_settings_enabled = 1 if $setting->{is_enabled};
     }
-    $vars->{'dont_show_button'} = !$vars->{'has_settings_enabled'};
+
+    $vars->{settings}             = \%settings;
+    $vars->{has_settings_enabled} = $has_settings_enabled;
+    $vars->{dont_show_button}     = !$has_settings_enabled;
 }
 
 sub SaveSettings {