589138 - Group user preferences by category

Status: NEW

(Bugzilla :: User Accounts, enhancement)

Description

[Frédéric Buclin]

Before adding any new user preference, we should re-order the existing ones. Currently, they are displayed out of order, coming from a hash, IIRC. Maybe should we add a "category" (in the backend) for each user preference, allowing us to group them more logically. Obvious categories which come to mind: "Bug Editing", "Email notifications" and "General":

General:
 - Bugzilla's general appearance (skin)
 - Timezone used to display dates and times
 - Zoom textareas large when in use (requires JavaScript)
 - Field separator character for CSV files
 - Show a quip at the top of each bug list

Bug Editing:
 - Quote the associated comment when you click on its reply link
 - Position of the additional comments box
 - After changing a bug
 - Enable tags for bugs
 - Automatically add me to the CC list of bugs I change
 - When viewing a bug, show comments in this order

Email Notifications:
 - Language used in email
 - Bugmail format (not yet implemented)

Comment 1

[Frédéric Buclin]

I have a patch mostly ready. Taking.

Comment 2

[Frédéric Buclin]

Attachment: patch, v1

While working on this, I also did some code cleanup to make it easier to use and/or to better match modern code style. I also removed obsolete code. My goal was not to make Bugzilla::User::Setting be based on Bugzilla::Object, though.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 484090 [details] [diff] [review]
patch, v1

>Index: editsettings.cgi
>+my $settings = Bugzilla::User::Setting->get_all();

  Logically, in a normal Bugzilla::Object, that would return the entire contents of the profile_setting table. That's a little confusing, but I suppose it's OK for now, since we're not a real Bugzilla::Object yet.

>+        my $enabled = defined $cgi->param("${name}-enabled") ? 1 : 0;

  Somebody could pass ${name}-enabled=0 and you would treat it as 1, there.

>Index: Bugzilla/Install.pm
>+                            category => '10-general' },

  Ah, don't combine the category name with its sortkey. If we have to, we can have a separate table for the categories, with a sortkey. Or we can just let the template sort them appropriately.

  Also, perhaps the empty category should be "general". So then if people forget to add a category, they just get general.

>+    # Existing settings will be updated from here. New ones will be set
>+    # by Bugzilla::Install::update_settings() later.
>+    foreach my $setting (keys %settings) {

  Tiny nit: my $name might be better.

>-@Bugzilla::User::Setting::EXPORT = qw(get_all_settings get_defaults
>-     add_setting);
>+@Bugzilla::User::Setting::EXPORT = qw(get_all add_setting);

  I think you don't need to export get_all, since it's a class method now.


  Otherwise, I gave the rest of this a quick look over, and it seems like a very nice cleanup and a generally sensible improvement. However, we really do need to split this out into several separate Bugzilla::Object subclasses at some point. Maybe Setting, Setting::Type, and Setting::Value, or something like that. Or Setting, SettingType, and SettingValue.

Comment 4

[Frédéric Buclin]

(In reply to comment #3)
>   Somebody could pass ${name}-enabled=0 and you would treat it as 1, there.

I know, but this is an admin page, and someone would have to hack the URL to get this result. I don't really care about this testcase. I can remove "defined" to make you happy, though. :)

Comment 5

[Max Kanat-Alexander]

Yeah, I'd be happier without defined. :-)

Comment 6

[Frédéric Buclin]

With Perl 5.18, the order in which prefs are displayed in userprefs.cgi varies on each page reload. This is totally confusing (I had to scan all prefs to find the one I wanted). This bug should really be fixed for 5.0.

Comment 8

[Koosha KM]

Isn't it possible to fix this with just a 'sort' method in the templates?

--- a/template/en/default/account/prefs/settings.html.tmpl
+++ b/template/en/default/account/prefs/settings.html.tmpl
@@ -30,7 +30,7 @@
   [% END %]
 
   <table id="user_prefs">
-    [% FOREACH name = setting_names %]
+    [% FOREACH name = setting_names.sort %]
       [% default_name = name _ '-isdefault' %]
       [% default_val = settings.${name}.default_value %]
       <tr>


--- a/template/en/default/admin/settings/edit.html.tmpl
+++ b/template/en/default/admin/settings/edit.html.tmpl
@@ -44,7 +44,7 @@
           <th>Enabled</th>
         </tr>
 
-        [% FOREACH name = settings.keys %]
+        [% FOREACH name = settings.keys.sort %]
           [% checkbox_name = name _ '-enabled' %]
           <tr>
             <td>

Comment 9

[Frédéric Buclin]

(In reply to Koosha KM from comment #8)
> Isn't it possible to fix this with just a 'sort' method in the templates?

This wouldn't make sense. Setting names are internal, and this wouldn't fix what I suggest in comment 0, i.e. group them by category.

Comment 10

[Koosha KM]

(In reply to Frédéric Buclin from comment #9)
> This wouldn't make sense. Setting names are internal, and this wouldn't fix
> what I suggest in comment 0, i.e. group them by category.

I know. The point is that, at least, the order of the items would be fixed no matter how Perl stores them in the hash. But, it does not categorize the items anyway!

Comment 11

[bjoern]

definetely a huge improvement for the randomnes that is currently given back. Would be a perfect quick fix for the stable bugzilla trees. I use it already in our bugzilla.