Group user preferences by category

NEW
Unassigned

Status

()

Bugzilla
User Accounts
--
enhancement
7 years ago
3 years ago

People

(Reporter: Frédéric Buclin, Unassigned)

Tracking

Details

Attachments

(1 attachment)

36.65 KB, patch
Max Kanat-Alexander
: review-
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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)
(Reporter)

Comment 1

7 years ago
I have a patch mostly ready. Taking.
Assignee: user-accounts → LpSolit
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 4.2
(Reporter)

Comment 2

7 years ago
Created attachment 484090 [details] [diff] [review]
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.
Attachment #484090 - Flags: review?(mkanat)

Comment 3

7 years ago
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.
Attachment #484090 - Flags: review?(mkanat) → review-
(Reporter)

Comment 4

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

7 years ago
Yeah, I'd be happier without defined. :-)
(Reporter)

Updated

7 years ago
Assignee: LpSolit → user-accounts

Updated

7 years ago
Target Milestone: Bugzilla 4.2 → ---
(Reporter)

Updated

7 years ago
Status: ASSIGNED → NEW
(Reporter)

Comment 6

4 years ago
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.
Target Milestone: --- → Bugzilla 5.0

Updated

3 years ago
Target Milestone: Bugzilla 5.0 → ---
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1081078

Comment 8

3 years ago
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>
(Reporter)

Comment 9

3 years ago
(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

3 years ago
(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

3 years ago
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.
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1162016
You need to log in before you can comment on or make changes to this bug.