98123 - Create a user preferences infrastructure (became 'General Settings')

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement, P1)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

Originally, it was thought that the emailprefs table would also hold general
prefs.  The thinking has changed and we need a general prefs table.  See bug
73665 for the emailprefs table.

user prefs such as the one in bug 7710 would go here.  The 'mybugslink' from
profiles table might also want to get moved here.  And any new user prefs should
go here as well.  Mark dependencies as needed.

Comment 1

[GavinS]

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

Comment 2

[GavinS]

Some other prefs which could be useful... (copied from bug#95618):

1) turn on/off quips
2) enable/disable css
3) specify user css sheet
4) specify user inline style
4) specify preferred layout of bug activity (seperate/inline)
5) show simple/complex query page
6) preferred buglist column layout

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

I think each of those prefs should be filed as a separate bug if they aren't
already and marked as dependent on this.

Comment 4

[GavinS]

Most of that list are already bugs - I'll search out and update the dependencies
soon.

Comment 5

[Matthew Tuck [:CodeMachine]]

What's wrong with the profiles table?  None of those things seem to need
multiple values.

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

First, the profiles table should be kept as small as possible with as few fields
as possible since it used for authentication. (IMO, the profiles table is poorly
named since it's primary function is user authentication.)

Second, we should store things on a relational basis not based on which tables
store multiple instances of something and which doesn't.  Preferences and
authentication information are different.

Thirdly and probably the selling point, if we have a separate table we only have
to store preferences for users that modify the default preferences, thus saving
DB space.  Were we to include it in the profiles table, everyone would have to
have preferences stored be they set or not.

Comment 7

[Dave Miller [:justdave]]

This is now on the "we really want this for 2.16, but won't hold the release for
it if it's not done by then" list.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Modifying summary to make this bug easier to find.

Comment 9

[Dave Miller [:justdave]]

This obviously isn't going to make it in 2.16...  -> 2.18

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Darn.  It's so necessary.

Comment 11

[Dave Miller [:justdave]]

OK, this table needs three columns...

userid, preferencename, and value.

The big question is what data type do we use for value?  some preferences will
be simple booleans.  Others could potentially be pretty long (the mybugslink for
example).  On the other hand, mybugslink could and probably should become a
named query instead.

Comment 12

[Jeff Hedlund]

I was thinking two tables:

prefdefs        and       profileprefs
--------                  ------------
prefid                    userid
name                      prefid
default                   value

This allows for a SQL query like this:  
SELECT IFNULL(profileprefs.value, prefdefs.default) etc.

But I ran into the same problem, what type of field to use for prefdefs.default
and profileprefs.value.   Most seem to be boolean, but there are some that will
require text, like bug 69654 which allows for some user defined CSS.  I wonder
if a tinytext would work?  And maybe have a prefdefs.type that the cgi's could
then interpret the correct value of default/value with?

Comment 13

[GavinS]

When I was thinking about user prefs, I also wanted a prefdefs.type
(boolean/string/url/list/...) which made things a lot easier when it came to
displaying prefs and for specifying the GUI for editting prefs.

Also, a prefdefs.desc may be needed (to explain the pref on the GUI)?

I  was wondering if it would be possible to combine this user prefs table with
the email prefs in bug#73665?

(And also what about the system params (bug#46296) -- could they use a prefdefs
table to store system parameters maybe?)

Comment 14

[Jeff Hedlund]

> Also, a prefdefs.desc may be needed (to explain the pref on the GUI)?

I think this will go away in feilddefs.desc in favor of a template that
describes the fields (fielddefs has field-descs.html.tmpl that is not in full
use yet).

So I think we would just have a prefs-descs.html.tmpl.

Comment 15

[Jeff Hedlund]

Reassigning to me...

WRT comment 13

> I  was wondering if it would be possible to combine this user prefs table with
> the email prefs in bug#73665?

I vote on a seperate table.  If we use the same table as the email prefs, then
we will have to have a field for every pref.  If we use a profile_prefs table,
as I suggest, then only those values which are not the default are stored.

> (And also what about the system params (bug#46296) -- could they use a prefdefs
> table to store system parameters maybe?)

I think this is possible, if the prefdefs table gets a system/user flag.

Comment 16

[Dave Miller [:justdave]]

Enhancements which don't currently have patches on them which are targetted at
2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. 
Consideration will be taken for moving items back to 2.18 on a case-by-case
basis (but is unlikely for enhancements)

Comment 17

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 18

[Max Kanat-Alexander]

OK, I guess since I'm thinking about this, I might as well take it.

Here are my thoughts, after some discussion with travis:

  We should just use the profiles table for the preferences. Then we only need a
WHERE and no JOIN for most preferences lookups. After all, I don't plan any
many-to-one or one-to-many relationships for preferences-to-profiles. Why have
them in separate tables? Just like "a bug HAS a state", "a user HAS a
preference." (I've implemented systems where a User and UserPrefs were separate
objects, and it was sort of silly. You just have a User carrying around a
UserPrefs object that he has to keep digging into. That would be similar to
having a separate table.)
  As far as comment 6 goes, We're looking up on an index; table size doesn't
matter for speed. The defaults won't take up significant space, even for
thousands of users.
  Does the concern have something to do with a LOCK TABLE statement that can't
be avoided somewhere (perhaps in the yet-unwritten code for this bug)?

  All we're doing in this bug is creating the table and the mechanism to modify
it with. We should also implement ONE simple pref, so that we know the
implementation works and is easy to code with. It will also encourage me to
create a decent UI.
  How about implementing bug 41972 inside this bug? I think that would be pretty
simple.

  For now, changing default preferences will require changing the schema.

  Future
  ------
  + A "prefdefs" table that allows the admin to enable/disable the use of
certain preferences.
  + Perhaps the ability for the admin to change the default preferences?

Comment 19

[Shane H. W. Travis]

(In reply to comment #18)
> Here are my thoughts, after some discussion with travis:

Here are my thoughts, after some more thought and discussion.

>   We should just use the profiles table for the preferences.

Agreed wholeheartedly.

>  We should also implement ONE simple pref, so that we know the
> implementation works and is easy to code with. ... How about
> implementing bug 41972 inside this bug?

Agree in principle, dislike conflating two different bugs.

I think we should *test* this with a fix for bug 41972 (I agree 
that's a nice, simple one), but not try and check them in together.


>  For now, changing default preferences will require changing the schema.

This is *not* going to fly, I know that already. As such, we need to
implement at least one additional table -- the prefdef table. It can
look like Jeff's in comment #12, because I think that's all we need for
now.

>  Future
>  ------
>   + A "prefdefs" table that allows the admin to enable/disable the use of
> certain preferences.
>   + Perhaps the ability for the admin to change the default preferences?

Already gotta have the prefdefs. Since that's the case, we definitely need
an admin-userpref page as well as a plain old userpref page. The former would
let the admin change default value for a given userpref. I'm torn on trying
to do the disable in round one of this as well; might be conceptually easier
to do it all at once, but I'd rather get it *in* and come back to it if it's
going to cause us all sorts of troubles. This has sat for long enough already,
and it already has eleven other bugs waiting for it to appear. :)

Comment 20

[GavinS]

I had always imagined this happening with a new table. If we used 'profiles',
would we not have to edit the schema each time we added a new user-pref?

One things it would be nice to decide on now, and is independent of the schema
change and implementation, is the interface to the routine used to check a
user_pref, so that the blocked bugs could be worked on. Any proposals for that?

What about the completely just made up, but probably not too surprising:

my $user_pref_value = user_pref('pref_name',$user);

where $user is optional and defaults to the logged in user if not specified. Is
it userid, username or Bugzilla::User object?

If the user is not logged in, then the system default is returned. If the user
has not set a pref, then the system default is returned.

Comment 21

[Max Kanat-Alexander]

(In reply to comment #19)
Yes, I've been thinking about it, and we'll use a separate prefs table, and a
prefdefs table, to handle the defaults.

(In reply to comment #20)
Here's the interface I imagine:

Bugzilla::User::get_pref(prefname)
prefname; string: the short name of the preference to get (DB column name)

The user is already defined, because we're calling this from an initialized user
object.

Bugzilla::User::set_pref(prefname, value)
prefname; string: the short name of the preference to get (DB column name)
value; scalar: The new value to set the preference to.

In templates, I believe that we should just be able to do:

user.get_pref.prefname
or
user.prefs.prefname

Or something like that. justdave was telling me how to do it in IRC, but I forget.

Comment 22

[Frédéric Buclin]

mkanat, travis, could you please already sumbit a "work in progress" patch? I'm
interested to see the structure of your pref table and how to access it. I need
it for bug 26257. ;) Thanks!

Comment 23

[Dave Miller [:justdave]]

For the record, I would prefer to see something set up where the names of the
preferences are not db columns, but a row in the table.  Customization is my
primary reasoning.  If the preference names are db columns, then the schema has
to be modified to add a preference.  If a third-party plugin to Bugzilla comes
along and needs to install a new user preference, they should be able to do so
simply by adding a default for it in the defaults table and adding some UI to
twiddle it via a template hook.

Comment 24

[Shane H. W. Travis]

Alright, new schema idea. I think this one works, but I *really* want some 
feedback from people.

This is going to work like the 'groups' subs in User.pm; when called, it will 
load a user's preferences into a hash table if they don't already exist, or do 
a simple hash lookup if they do. (An update to the hash table is forced if the 
admin changes a preference default, or the user changes prefs.)

Three tables:

1) 'prefdefs' table. This stores the values germane to the preferences
themselves.

. pref_id            : integer, unique (KEY)
. pref_name          : varchar   (This is what is shown to admin and users)
. default_pref_val   : int       (Default value if no individual pref is set)
. enabled            : int       (0=Off, 1=On)

If a given pref_id is disabled, then the default_pref_val will *always* be 
used. The UI for changing user prefs will show the pref_name and the default 
value, and will indicate that it is locked. (Alternately: it won't show 
anything at all, so the user won't even know that the option exists. Thoughts?) 


2) 'userprefs' table. Stores the preferences of individual users

. user_id  : from profiles. (COMBINED KEY)
. pref_id  : from prefdefs  (COMBINED KEY)
. pref_val : int

* If the user_id/pref_id key does not produce a row, the prefdefs.default 
   for that pref_id is loaded into the hash.
* If a pref is disabled after a user has entered a value, the row is not
   deleted in case the preference is enabled again in the future... it
   simply is never looked up.


3) 'prefvals' table. Stores the meaning of any given preference value

. pref_id   : from prefdefs (COMBINED KEY)
. pref_val  : int           (COMBINED KEY)
. display   : varchar(100)  (displayed text for this value)

When defining a new preference, the Admin will make entries into this table to 
define what the legal values are. These can be as simple as 0=off, 1=on (which 
will be offered to the Admin as a default when creating a new userpref), or 
complex (0="magenta and mauve", 1="passionfruit, pink, and peach", etc.) Aside: 
anyone got a better column name than 'display'? I'm sure one exists, but 
couldn't come up with one...


In the to-be-created userprefs.cgi?tab=individual-preferences UI, users will be 
presented with a series of lines with the following information, one per 
existing (or enabled) userpref:

prefdefs.pref_name      [drop-down with prefval combos]
                        or [X] Use system default (set to: default_pref_val)

Things will be disabled/not shown as appropriate if prefdef.enabled = 0.

I'm a little jammed on creating the Admin UI; should we allow them to edit the 
meaning of prefvals.display after it is entered? Do we let the admin to remove 
a given pref_id or pref_val if people have already chosen them? If so, do we 
just delete the corresponding entries in userprefs? These can be determined 
later... and I think Max is doing the UI anyway. (Max, confirm?)

Anyway, I *think* that this is an optimal schema solution... but I wouldn't be 
surprised if there's a flaw somewhere or some facet that I hadn't considered. 
Feedback, please!

Comment 25

[Shane H. W. Travis]

(In reply to comment #24)
> I'm a little jammed on creating the Admin UI; should we allow them to edit 
> the meaning of prefvals.display after it is entered? Do we let the admin to
> remove a given pref_id or pref_val if people have already chosen them? 

Okay, ignore these ravings. For some reason I was thinking that non-code-
hacking admins would/should be able to create their own custom userprefs from 
their admin page... when in fact that is not the case any more than it is with 
parameters. 

Admins will be presented with a list of all existing userprefs, allowed to 
choose a default for their site (which will be saved in the database) and 
allowed to enable/disable a userpref completely if he does not want users to 
have access to anything *but* the default value. 

No entry/edit capabilities are needed... just drop-down boxes, an 
enable/disable checkbox, and a Commit button. D'oh!

Comment 26

[Frédéric Buclin]

(In reply to comment #24)
> Three tables:

Fine! I also have three. ;)


> 1) 'prefdefs' table. This stores the values germane to the preferences
> themselves.
> 
> . pref_id            : integer, unique (KEY)
> . pref_name          : varchar   (This is what is shown to admin and users)
> . default_pref_val   : int       (Default value if no individual pref is set)
> . enabled            : int       (0=Off, 1=On)

I would suggest the following column names, for clarity:
. name    : VARCHAR(20)  PRIMARY KEY [short name of the pref, without spaces]
. desc    : VARCHAR(200) [Pref full name / description displayed to users]
. default : SMALLINT     [Default value if no individual pref is set]
. enabled : TINYINT      [If 0 (= off), the 'default' value is forced to users]

'default' must be a valid 'val' values from defvals!

Instead of an integer as a primary key, I suggest here a string. Or at least, if
you *absolutely* want an integer as a primary key, you should add a 'descr'
field which will be displayed to users. So we could have:

=================================================================
                             prefdefs
=================================================================
id   name       desc                           default   enabled
=================================================================
 1   security   "Security Level"               1         0
 2   bug_order  "Bugs in buglists ordered by"  1         1
=================================================================


=================================================================
                              prefvals
=================================================================
pref_id   val   desc
=================================================================
 1        0     "No security: accept all changes"
 1        1     "Do not accept changes submitted from outside Bugzilla"
 2        0     "Bug Name"
 2        1     "Bug ID"
 2        2     "Creation Date"
 2        3     "Status"
=================================================================

So you don't need to remember which pref has which ID, you can call it using
it's (short) name and still be able to display a more friendly full name /
description of the pref to users.


> 2) 'userprefs' table. Stores the preferences of individual users
> 
> . user_id  : from profiles. (COMBINED KEY)
> . pref_id  : from prefdefs  (COMBINED KEY)
> . pref_val : int

OK here, with pref_val referencing a valid 'val' in defvals.


> * If the user_id/pref_id key does not produce a row, the prefdefs.default 
>    for that pref_id is loaded into the hash.

OK!


> * If a pref is disabled after a user has entered a value, the row is not
>    deleted in case the preference is enabled again in the future... it
>    simply is never looked up.

Clever! ;) I like it.



> 3) 'prefvals' table. Stores the meaning of any given preference value
> 
> . pref_id   : from prefdefs (COMBINED KEY)
> . pref_val  : int           (COMBINED KEY)
> . display   : varchar(100)  (displayed text for this value)

To be consistent with what I have already said:

. pref_name : VARCHAR(20)  [Name of the pref we are pointing to]
. val       : SMALLINT     [ID of this option]
. desc      : VARCHAR(200) [Full name / description of the option displayed]

Here pref_name references 'name' in prefdefs. Again, if you prefer an integer as
a primary key, I fully agree with your solution.


> anyone got a better column name than 'display'?

'desc' (description)?

Comment 27

[Colin Ogilvie [:cso]]

(In reply to comment #26)
> I would suggest the following column names, for clarity:
> . name    : VARCHAR(20)  PRIMARY KEY [short name of the pref, without spaces]
> . desc    : VARCHAR(200) [Pref full name / description displayed to users]
> . default : SMALLINT     [Default value if no individual pref is set]
> . enabled : TINYINT      [If 0 (= off), the 'default' value is forced to users]
> 
> 'default' must be a valid 'val' values from defvals!
> 
> Instead of an integer as a primary key, I suggest here a string. Or at least, if
> you *absolutely* want an integer as a primary key, you should add a 'descr'
> field which will be displayed to users.

I think the Primary Key should be an integer - this gives you the scope to
rename the prefrence for clarity without breaking the references to it.

I was informed on #webtools tonight that this was the reason that products are
now referred to as id's and not names as far as I could gather.

Comment 28

[Frédéric Buclin]

(In reply to comment #27)
> I think the Primary Key should be an integer - this gives you the scope to
> rename the prefrence for clarity without breaking the references to it.

But I defined *two* names here: a short one which should never change and a long
one (the one displayed to users) which can be change. But I know some of you
prefer integers as primary keys in any case. ;)

Comment 29

[Max Kanat-Alexander]

OK, as far as customization goes, I agree with justdave. I suppose we should use
table rows.

For the record, here's what I don't like about table rows:

* We lose the built-in typing of MySQL.
* It's more difficult to enforce "Um, don't delete that row," than "don't delete
that column," and more difficult for checksetup to figure out the validity of
the prefdefs and profile_prefs table.


For the purposes of customization, the Primary Key for the prefdefs table should
be a varchar. This keeps the PK for a custom pref the same between multiple
instances of Bugzilla, whereas otherwise it would be (most likely) randomly
assigned as an auto_increment.

We use "isactive" as a colum name in the BZ schema, instead of "enabled."

If we have a prefvals table, the "prefvals" column in profile_prefs must be a
string (varchar) -- some preferences allow freeform text to be entered. This
brings me to the problem of how large that varchar should be... Also, once we
start using a varchar, we lose the ability to easily rename the pref value...

Ah! I know how we'll do it. :-) There will be a column called preftext that's
read in as the prefvalue if the prefval is NULL. That will be a bit difficult in
Bugzilla::User::set_pref, but that's OK; I can deal with that. :-)

Comment 30

[Shane H. W. Travis]

(In reply to comment #29)

> For the record, here's what I don't like about table rows:
> * We lose the built-in typing of MySQL.

Do we *need* this any more? That's part of what I came to realize; we can't use 
ENUMS (because they're unique to MySQL and thus break other DBs) so we were 
going to have to store any text strings as references anyway. This does prevent 
us from using numbers *quite* so easily... but how many preferences are going 
to be numbers? Certainly none of the bugs that are blocked by this one... 

> * It's more difficult to enforce "Um, don't delete that row," than "don't
> delete that column," 

True, but the only deletions that should be happening are in the code itself. 
That someone can directly access the DB and completely FUBAR themselves has 
always been a danger, and not one that code is usually written to minimize.

> and more difficult for checksetup to figure out the validity of
> the prefdefs and profile_prefs table.

True, I'll give you that one... at least to a small degree.

> For the purposes of customization, the Primary Key for the prefdefs table
> should be a varchar. 

Sounds congruent to Frederic's comment #26. I guess I can see that 
  ... WHERE prefdefs.pref_id = 'quips_pref' 
is a lot more descriptive and human-readable than 
  ... WHERE prefdefs.pref_id = 17

> We use "isactive" as a colum name in the BZ schema, instead of "enabled."

Aaah, I *knew* there was a better word. Consider it accepted.


> If we have a prefvals table, the "prefvals" column in profile_prefs must be a
> string (varchar) -- some preferences allow freeform text to be entered.

Several comments:
1) wth is profiles_prefs? You talking about the userprefs table?
2) No preferences allow freeform text to be entered. They're all choices from
   a drop-down list... at least for this iteration. Only one of the blocked
   bugs (bug 109692) needs anything more than that for now, and that can still
   be settled by a series of discrete choices -- checkboxes, perhaps.

Remember; we're building a foundation, not the 14th floor. If someone else 
needs freeform text boxes, they can add them then.

That renders the rest of your comment moot, except for this one:

> ... but that's OK; I can deal with that. :-)

You and I need to talk about division of labour. I wanted to get a start on 
this over my Christmas break, and I thought you didn't have time. (I had even 
thought of reassigning to me, based on IRC conversations.) Has time freed up 
for you?

Comment 31

[Dave Miller [:justdave]]

The human readable descriptions should come from a template so they can be
localized.  Putting them in the database makes it unlocalizable.

Comment 32

[Frédéric Buclin]

so we have to drop the 'desc' column in both tables.

Comment 33

[Shane H. W. Travis]

Final schema, I think, incorporating Frederic's suggestion of a string key, 
Max's rename of 'enabled', and Dave's contention that the user-display part 
needs to be in a template rather than a database for easier localization.

==================
1) 'prefdef' table. 

. pref_name          : varchar, unique (KEY)
. default_value      : varchar
. isactive           : int       (0=no, 1=yes)

This stores the master list of all possible user preferences. It also stores
one default value, which is an index into the template-replacement scheme (as
Dave suggested, so that it's easier to localize). If a user does not set a
preference, this value will be used. Finally, 'isactive' indicates whether 
or not the user even has an option to set their own preference. If this is 
set to 'no', the user will not even see the preference option; it will be 
as though the option does not exist as far as the user is concerned. if 
isactive=0, everyone uses the default.

2) 'prefval' table. 

. pref_name          : varchar (COMPOUND KEY)
. value              : varchar (COMPOUND KEY)

This stores the values germane to the preferences themselves. 
- prefval.pref_name must exist in prefdef.name 
- Each name/value combination must be unique
This scheme allows us to re-use values, so we don't have to have 
quips_on/quips_off, borders_on/borders_off, etc. We define a preference 
named 'quips' in prefdefs, and indicate what its acceptable values are in 
this table. Again, value is an index into the template-replacement scheme 
mentioned above.


3) 'userprefs' table. Stores the preferences of individual users

. user_id      : from profiles.   (COMPOUND KEY)
. pref_name    : as from prefval  (COMPOUND KEY)
. pref_val     : as from prefval  

This table stores a user's individual preferences, if they have ever been set.
The combination of user_id/pref_name must be unique; the combination of
pref_name/pref_val must appear in the prefval table.
* If a pref is enabled, the pref-load algorithm looks in this table to 
   try and find a value for the given user_id/pref_name combination. If
   no row is produced, the prefdef.default for that pref_name is loaded
   into the hash.
* If a pref is disabled by the admin after a user has entered a value, 
   the row is not deleted in case the preference is enabled again in the
   future... it simply is never looked up by the pref-load algorithm.
* UI will give the user an option to delete their stored value and use 
   the admin-set default instead.

Comment 34

[GavinS]

My 2 comments:

1) What about if we need to store a value such as an URL, or the buglist sort
order, or buglist column order, or some other less constrained value for a user
pref?

2) The DB design would look better to me with a numeric pref_id in the 'prefdef'
table which was used to do the SQL joins. The pref_name would still be needed in
the 'prefdef' table, and used in the code for pref lookup, but the SQL joins
would use the numeric pref_id (in my world, anyway :)

Comment 35

[Shane H. W. Travis]

(In reply to comment #34)
> My 2 comments:
> 1) What about if we need to store a value such as an URL, or the buglist sort
> order, or buglist column order, or some other less constrained value for a 
user
> pref?

I answered that objection already, in comment #30; see the response 
to 'freeform text'.

(Do people not read through the historical comments on a bug before responding? 
This isn't aimed at you specifically, Gavin... you're not the only, or even the 
first, person who has asked me questions I've already answered.)

Right now, there is *no* functionality for user prefs. I'm building *some* 
functionality for user prefs - enough to satisfy most of our current needs. It 
is not reasonable to expect the ultimate, absolute, be-all end-all solution to 
a problem *as a first step*. What you're asking for is a reasonable extension, 
but it is not a critical initial component.

Remember, folks... admins can't just create new user prefs for their users; 
like parameters, they are defined by programmers. Right now, nothing *needs* a 
freeform box, so I'm not going to go to extra work to incorporate one. Max and 
I have discussed it, and this schema is extensible to accommodate the need 
should it arise (perhaps with the need for one more table), so that's good 
enough for me.


> 2) The DB design would look better to me ...

... and it wouldn't to me. Since I'm coding it, I win. :)

(Again, read back please. Much of this discussion has already taken place.)

Integers for the sake of integers ... why? The point is to have a unique way of 
indexing into the table, which is provided by the name (never changes, not 
visible to the user) and the value (never changes, not visible to the user). 
Both of these things are translated into visible strings by pulling values from 
a template, for easier localization... so the only people who will need to see 
the actual table values are programmers.

Look at the following two examples:

if (UserPref(17) = 2)
  vs.
if (UserPref(comment_sort_order) = "oldest_to_newest")

Which one can be read by a human? Which one is going to be easier to program, 
and will require fewer references back to the table to determine whether 
comment_sort_order was 17 or 16? Which is (therefore) going to be easier to 
debug and maintain? (Very important considerations on a project like this.)

I know where I stand. (And for the record, it's not where I stood when I 
started this debate.)

Comment 36

[Shane H. W. Travis]

Minor modification to schema, and rationale:

2) 'prefval' table. 

. pref_name          : varchar (COMPOUND KEY)
. value              : varchar (COMPOUND KEY)
. sortorder          : integer

unique(pref_name, value)
unique(pref_name, sortorder)

The one good thing about integers is that you can select by them and order them 
easily... words, on the other hand, don't sort so well. This would be a pain 
when it comes to displaying the information to the user, as a programmer would 
have to be very creative when creating values to ensure that they got displayed 
in the order he/she wanted... either that, or go with something ugly like 1-
on/2-off/3-random. Either one destroys the whole string-reusability aspect.

Adding a .sortorder column so the order in which the various choices are 
presented to the user can be easily and explicitly defined by the programmer at 
the time of the creation of a new User Pref.

Comment 37

[Shane H. W. Travis]

Attachment: Code patch for tip

First of all... thanks tons to Max for the help with this. Without his
insights, or time as a sounding board, this patch wouldn't be nearly as good as
it is.

Secondly, a little explanation. I have changed the name from 'User Preferences'
to 'Settings'. This was necessitated by the fact that there is already a User
Preferences page with multiple tabs on it, and this needed to go there, so it
needed a different name.

There exist three ways that Bugzilla can be controlled/customized:
1) Parameters: Admin has complete control over them for the site as a whole.
2) User Prefs: things like email, stored queries, etc. Completely controlled 
     by the user on an individual level.

This patch creates a third method -- Settings -- which fall midway between the
two. Each setting has a 'global default' which will apply to people who are not
logged in, and to people who have not chosen to override it. This global
default can be changed by the Admin -- like a Parameter -- and the new setting
will immediately affect every user on the site... *except* those who have
chosen to override the global setting with their own personal preference.
Furthermore, if an Admin decides to, a Setting can be completely disabled; this
removes it from the Users' display, and the Global Setting will then apply to
all users whether or not they have ever chosen a personal value for that
setting. (In essence, disabling a Setting turns it into a Parameter.)

Any other questions, I'll be happy to answer. I asked Max for review because he
understands how it works already, but I'll gladly accept code/usability reviews
from anyone.

Comment 38

[Shane H. W. Travis]

Attachment: Testing data/values for the above patch

Here is some test data for people if they wanted to use it. Two files; one SQL
file that adds three settings to the database, and a diff file that adds the
necessary information to the (new) file settings-desc.none.tmpl

Comment 39

[Shane H. W. Travis]

Forgot to add this dependency. This v1 patch should apply just fine, but it 
needs the latest patch for bug 281574 to be applied before it will compile or 
execute.

Comment 40

[Max Kanat-Alexander]

Comment on attachment 173917 [details] [diff] [review]
Code patch for tip

>diff -Naur --exclude=CVS --exclude=data --exclude=.htaccess --exclude=localconfig --exclude=skins tip/Bugzilla/User/Setting.pm tip.userprefs/Bugzilla/User/Setting.pm
>--- tip/Bugzilla/User/Setting.pm	1969-12-31 18:00:00.000000000 -0600
>+++ tip.userprefs/Bugzilla/User/Setting.pm	2005-02-09 15:26:31.251790429 -0600

  I realized this later in the review (so I can't put the comment right below
the EXPORT statement), but I think that we shouldn't EXPORT the functions that
are only used by Bugzilla::User. We should just EXPORT the functions that are
used by CGIs. The functions used by Bugzilla::User can be called by their full
name.

>+# The Initial Developer of the Original Code is Netscape Communications
>+# Corporation. Portions created by Netscape are

  Actually, you are the original developer. :-) I usually take this section
*out* of new files for Bugzilla, but I'm not sure if that's actually the
correct thing to do.

>+use Safe;

  You must have patterned this off of Bugzilla::Config. :-) "Safe" isn't
necessary, for this code.

>+    # create a blank $self
>+    my $self = {
>+        'is_enabled'      => 0,
>+        'legal_values'   => [],
>+        'default_value'  => "",
>+        'value'          => "",
>+        'is_default'     => 0,
>+    };

  Hrm... should we actually set these values here? I think that we should just
leave them blank, and then if they're not filled-in somewhere, we'll get errors
(like we should). I don't think creation of a blank Setting should be allowed,
yet.

>+    # If there's nothing left after taking out the setting and ID
>+    # then we need to retrieve the information ourselves.
>+    if (scalar @_ == 0) {

  I know that I suggested this, but either this method of creating the Setting
or the other one might not actually be used in the current code, right? I
generally prefer to remove dead code, now that I've thought about it. If one of
the methods isn't used, it shouldn't yet be allowed.

>+        my $sth = $dbh->prepare(q{SELECT default_value, is_enabled, setting_value
>+                                    FROM setting

  The SQL shouldn't have newlines in it. See bug 139559.

>+                               LEFT JOIN profile_setting
>+                                      ON setting.name = profile_setting.setting_name
>+                                   WHERE name = ?
>+                                     AND (profile_setting.user_id = ?
>+                                          OR profile_setting.user_id IS NULL)} );
>+        $sth->execute($setting_name, $user_id);
>+        my ($default, $is_enabled, $value) = $sth->fetchrow_array();

  Nit: If that all returns only one row, it can be a selectrow_arrayref. See
the DBI docs at: http://search.cpan.org/~timb/DBI-1.46/DBI.pm

>+        if ( ($is_enabled) && (defined $value) ) {

  Nit: You don't need the parens around those inner statements.

>+    }
>+    else {
>+      # If the values were passed in, simply assign them and return.

  This is the "other method" I was talking about. Are they actually both used?

>+#####################################################################
>+##########################   Subroutines   ##########################
>+#####################################################################

  The standard Bugzilla format for a header like that is:

>#####################################################################
># Subroutines
>#####################################################################

>+
>+sub GetDefaultSettings {

  If this is public and exported, it needs to be in lowercase_with_underscores.
If it's private (not to be used by code outside of this package) it needs to
_start_with_an_underscore. Same for all of the other functions. It's standard
perl module style, which we are striving for.

>+    my $sth = $dbh->prepare(q{SELECT name, default_value, is_enabled
>+                                FROM setting
>+                            ORDER BY name});

  Newlines again. Sorry I didn't comment on this before -- I wasn't really
thinking about it.

  I won't comment on the other ones -- they just need a quick fixing (to be a
series of joined strings).

>+    $sth->execute();
>+    while (my ($name, $default_value, $is_enabled) = $sth->fetchrow_array()) {
>+        $default_settings->{$name} = new Bugzilla::User::Setting
>+          ($name, 0, $is_enabled, GetLegalValues($name),

  The normal Bugzilla style is to have the paren next to the function call
itself. So it would be:

  $default_settings->{$name} = new Bugzilla::User::Setting(

>+        if ( ($is_enabled) && (defined $value) ) {

  Nit: Parens again.

  perlstyle (the man page that we follow) actually says to avoid too many
parens. So that's what we do. :-)

>+        $settings->{$name} = new Bugzilla::User::Setting
>+          ($name, $user_id, $is_enabled, GetLegalValues($name),

  And the paren-location, again.

>--- tip/Bugzilla/User.pm	2005-02-09 00:36:43.000000000 -0600
>+++ tip.userprefs/Bugzilla/User.pm	2005-02-09 17:30:01.525070459 -0600
> [snip]
>+sub settings {
> [snip]
>+    if ($self->id) {
>+        $self->{'settings'} = GetUserSettings($self->id);
>+    } else {
>+        $self->{'settings'} = GetDefaultSettings();
>+    }

  Does that actually work for a user who isn't logged-in? Normally in the other
functions, I think that we're returning an empty object, instead of 

>+sub delete_user_setting{

  This is going to be called as Bugzilla->user->delete_user_setting(), so I
think the "user" in the name of the function is redundant.

>+    my ($self, $user_id, $setting_name) = @_;
>+
>+    $self->settings;

  You don't actually need to do that $self->settings, there.

>+    $self->{'settings'}->{$setting_name}->{'is_default'} = 1;
>+    $self->{'settings'}->{$setting_name}->{'value'}
>+      = $self->{'settings'}->{$setting_name}->{'default_value'};

  These two lines should be:

  $self->settings->{$setting_name}->{'is_default'} = 1;
  $self->settings->{$setting_name}->{'value'}
      = $self->settings->{$setting_name}->{'default_value'};

  You'll have to note that they're there to deal with the fact that the
settings object could have already been loaded, and we need to update it if
that's the case.

>+sub set_user_setting {

  My comment about the "user" in this sub name applies here, too.

>+    $self->{'settings'} = GetUserSettings($self->id);

  You don't need that call at all, there.

>+    if ($self->{'settings'}->{$setting_name}->{'is_default'}) {

  Should be $self->settings->{$setting_name}->{'is_default'}. ALL the other
calls to $self->settings should be this way. (I won't add comments to the rest
of them.)

>@@ -1100,6 +1163,32 @@
>+=item C<settings>

  Cool. This is well-documented. :-)

>+# user pref system
>+
>+$table{setting} = 
>+    'name           varchar(32)     not null primary key,
>+     default_value  varchar(32)     not null,
>+     is_enabled     tinyint         not null default 1';

  I usually do tinyint(1), there, I think, for booleans.

>+
>+$table{setting_value} = 
>+    'name           varchar(32)     not null,
>+     value          varchar(32)     not null,
>+     sortindex      tinyint,        not null,

  sortindex should actually be a smallint, like it is for target_milestones.

>--- tip/editsettings.cgi	1969-12-31 18:00:00.000000000 -0600
>+++ tip.userprefs/editsettings.cgi	2005-02-09 16:22:09.523894160 -0600
>+
>+require "CGI.pl";
>+
>+# Use global template variables.
>+use vars qw($template $vars);

  I have come to realize only recently that you don't need that use vars
statement. Instead, you can have:

  my $template = Bugzilla->template;
  my $vars;

  Which is better. You might need to make that "our $vars" since you modify
$vars in a subroutine, though.

  But I think that it's possible that you wouldn't need CGI.pl then, either.

>+sub SaveSettings{
> [snip]
>+
>+        if ($value =~ /^(\w+)$/ ) {
>+            $value = $1;
>+        }

  What's that check for? Is it just for taint purposes?

>+print Bugzilla->cgi->header;

  Nit: You could do the my $cgi above this, and then use that.

>+my $cgi = Bugzilla->cgi;
>+my $action  = trim($cgi->param('action')  || 'load');

  Usually we just leave the action empty, in this case, and just have a choice
for what happens when the action is empty. I suppose I don't mind this way of
doing it here, so much, though.

>--- tip/template/en/default/admin/settings/edit.html.tmpl	1969-12-31 18:00:00.000000000 -0600
>+++ tip.userprefs/template/en/default/admin/settings/edit.html.tmpl	2005-02-09 17:15:13.974798651 -0600
[snip]
>+        <table border cellpadding="4">

  In HTML 4.0, every attribute must have a value. So that would be border="1"
if that's what you want. If it's not what you want, it would be border="0".

[userprefs.cgi]

>+        elsif (defined $cgi->param("${name}") ) {
>+            my $value = $cgi->param("${name}");

  Nit: You actually don't need the quotes and brackets, there.

  Overall, this patch looks really great and clean. Just a few tiny little bits
here and there to fix up.

  The overall design is excellent, and the code is very elegant. :-)

  I didn't look over the UI very hard -- I'm going to let Jouni do that.

Comment 41

[Shane H. W. Travis]

(In reply to comment #40)

>   Actually, you are the original developer.
Yup, realized this myself later; have already taken it out of the current 
version.


> >+    # create a blank $self
>   Hrm... should we actually set these values here? I think that we should
> just leave them blank, and then if they're not filled-in somewhere, we'll get
> errors (like we should). 

Been wondering about that myself... but I noticed it being done in either the 
User or Bug code (I forget which) so I followed that style.


> If one of
> the methods isn't used, it shouldn't yet be allowed.

I understand the logic of 'removing unused code', but in this case I disagree. 
The constructor for this object shouldn't *only* depend on other subroutines to 
dig up the values for it; it should have some way of populating itself if those 
values aren't passed in. Someone wanting to use a single Setting in the future 
(as opposed to hash full of them) shouldn't also have to write the constructor 
for this object just to get that functionality. For one thing, they won't know 
the code, and I do... so why not put it in now for completeness' sake?

>   The SQL shouldn't have newlines in it. See bug 139559.

I see an old bug with no discussion or consensus, and I don't recall seeing any 
other reviews mentioning it or adhering to it. Why here? Why now? I think it's 
worth getting some input from Dave on whether or not that's bug is/is going to 
become canon; if it is, it's definitely worthy of an update to the Developer's 
Guide since a lot of old code (and a lot of new code, even) is not following it.


> >+        my ($default, $is_enabled, $value) = $sth->fetchrow_array();
>   Nit: If that all returns only one row, it can be a selectrow_arrayref. See
> the DBI docs at: http://search.cpan.org/~timb/DBI-1.46/DBI.pm

I looked at that when you first suggested it. I need three distinct pieces of 
data; why would I want to use a call that returns a hash reference?


>   The standard Bugzilla format for a header like that is:
> >#####################################################################
> ># Subroutines
> >#####################################################################

I see it done four different places in four different ways. How does that make 
this way the standard?

(Don't get me wrong, if there *is* a defined standard with a reason for being, 
I'll likely follow it... I just haven't seen any evidence of one.)


>   If this is public and exported, it needs to be in 
lowercase_with_underscores.
> If it's private (not to be used by code outside of this package) it needs to
> _start_with_an_underscore. Same for all of the other functions. It's standard
> perl module style, which we are striving for.

Reference please - bug # or perl docs or perl books - so I can look it up and 
know for next time?


>   Does that actually work for a user who isn't logged-in? 

Did when I tried it... I think. Pretty sure I tried it, anyway; 85% or so.

> Normally in the other
> functions, I think that we're returning an empty object, instead of 

... instead of what? Don't leave me hanging here, man! :)

If we return nothing for a non-logged-in user, then the very next thing the 
calling code would have to do (on realizing that User->setting->{$name}->
{'value'} has nothing in it) is perform some sort of call to return the default 
value, because code that uses Settings will need to know what the global value 
for this setting is. My thought process was; why make the code make two 
different calls? Why not just return the value regardless, since it has to have 
*something* before it can continue anyway?


>   I usually do tinyint(1), there, I think, for booleans.

There are exactly two instances of tinyint(1) being used for booleans in 
checksetup.pl, vice dozens - literally - that aren't. Also, according to 
several comments in the MySQL documentation, tinyint(1) causes problems with 
MySQL connector, and furthermore doesn't even modify the storage size in some 
(older) versions of MySQL.

I guess what I'm asking is... do we really care? This a nit, or a show-stopper?


>   sortindex should actually be a smallint, like it is for target_milestones.

I would argue the other way around - that the sort index in target_milestones 
should actually have been a tinyint. Given that moth Milestones and Settings 
are converted to pull-down lists, nobody is going to get anywhere *near* 255 
values (or even 127 if it's signed - I can't remember) and have it remain 
usable.


>   What's that check for? Is it just for taint purposes?

Yes. Got a better way to do it? Or just suggesting that I need a comment to 
that effect?


> >+        <table border cellpadding="4">
>   In HTML 4.0, every attribute must have a value. So that would be border="1"
> if that's what you want. If it's not what you want, it would be border="0".

Stole this directly from show_activity.cgi, as I wanted what it had... so 
prolly needs fixing there too.

Thx for the review!

Comment 42

[Max Kanat-Alexander]

(In reply to comment #41)
> > >+    # create a blank $self
> >   Hrm... should we actually set these values here? I think that we should
> > just leave them blank, and then if they're not filled-in somewhere, we'll get
> > errors (like we should). 
> 
> Been wondering about that myself... but I noticed it being done in either the 
> User or Bug code (I forget which) so I followed that style.

  OK. Let's try not doing it here, and see how it goes, I think.

> I understand the logic of 'removing unused code', but in this case I disagree.
> [snip]

  OK, I agree with your logic. :-)

> >   The SQL shouldn't have newlines in it. See bug 139559.
> 
> I see an old bug with no discussion or consensus, and I don't recall seeing any 
> other reviews mentioning it or adhering to it. Why here? Why now? I think it's 
> worth getting some input from Dave on whether or not that's bug is/is going to 
> become canon; if it is, it's definitely worthy of an update to the Developer's 
> Guide since a lot of old code (and a lot of new code, even) is not following it.

  It's not that hard to do. I've done it, in all my recent patches.

  It's true, there's definitely some code that doesn't do it, but if we can make
life easier for people using administrative tools for relatively little effort,
I don't see the downside.

> > >+        my ($default, $is_enabled, $value) = $sth->fetchrow_array();
> >   Nit: If that all returns only one row, it can be a selectrow_arrayref. See
> > the DBI docs at: http://search.cpan.org/~timb/DBI-1.46/DBI.pm
> 
> I looked at that when you first suggested it. I need three distinct pieces of 
> data; why would I want to use a call that returns a hash reference?

  It doesn't return a hash reference. selectrow_arrayref.

> >   The standard Bugzilla format for a header like that is:
> > >#####################################################################
> > ># Subroutines
> > >#####################################################################
> 
> I see it done four different places in four different ways. How does that make 
> this way the standard?

  It's most commonly been done this way in modern patches. I'd like to keep it
that way. Most of the modules have it in that way, although it's true that some
older modules have it differently.

> Reference please - bug # or perl docs or perl books - so I can look it up and 
> know for next time?

  from "man perlstyle":

  "While short identifiers like $gotit are probably ok, use under-
  scores to separate words.  It is generally easier to read
  $var_names_like_this than $VarNamesLikeThis, especially for non-
  native speakers of English. It’s also a simple rule that works
  consistently with VAR_NAMES_LIKE_THIS."

> >   Does that actually work for a user who isn't logged-in? 
> 
> Did when I tried it... I think. Pretty sure I tried it, anyway; 85% or so.

  OK. We're probably OK, then. :-)

> > Normally in the other
> > functions, I think that we're returning an empty object, instead of 
> 
> ... instead of what? Don't leave me hanging here, man! :)
> [snip]

  Hahahaha. Sorry, I got caught up in other parts of the review and I must have
just forgotten.

  Yeah, I agree with the way that you're returning it, actually. That's probably
why I left off on the comment and didn't finish it. :-)

> There are exactly two instances of tinyint(1) being used for booleans in 
> checksetup.pl, vice dozens - literally - that aren't. Also, according to 
> several comments in the MySQL documentation, tinyint(1) causes problems with 
> MySQL connector, and furthermore doesn't even modify the storage size in some 
> (older) versions of MySQL.

  OK. If it causes problems with some software, it's probably not a problem.
It's just a nice thing to explicitly state what the thing should be.

  Eventually, when we have the cross-DB schema patch, all of these things will
become normalized. I'm just sort of trying to normalize them, now. It doesn't
particularly matter one way or another, I suppose. :-)

> I would argue the other way around - that the sort index in target_milestones 
> should actually have been a tinyint. Given that moth Milestones and Settings 
> are converted to pull-down lists, nobody is going to get anywhere *near* 255 
> values (or even 127 if it's signed - I can't remember) and have it remain 
> usable.

  Actually, it's fairly useful to have a sortindex that's a smallint. It helps
for creating values like 100, 200, 300, 400, in case you plan to possibly put a
lot of values between them in the future and you don't know how they're going to
sort.

  All the sortindexes in the enums patch are smallints, too.

> >   What's that check for? Is it just for taint purposes?
> 
> Yes. Got a better way to do it? Or just suggesting that I need a comment to 
> that effect?

  No, just a comment would be fine! :-) Will \w+ approve of numbers, too? I forget.

> > >+        <table border cellpadding="4">
> >   In HTML 4.0, every attribute must have a value. So that would be border="1"
> > if that's what you want. If it's not what you want, it would be border="0".
> 
> Stole this directly from show_activity.cgi, as I wanted what it had... so 
> prolly needs fixing there too.

  OK. :-)

> Thx for the review!

  Hey, no problem! Thanks for writing the code! It's great. :-)

Comment 43

[Shane H. W. Travis]

(In reply to comment #40)

> I think that we shouldn't EXPORT the functions that
> are only used by Bugzilla::User. We should just EXPORT the functions that are
> used by CGIs. The functions used by Bugzilla::User can be called by their full
> name.

Forgot to comment on this in my initial response.

What you're saying is diametrically opposed to what Dave said to me in IRC, and 
I agree with him. The only thing that should be using Setting is User; 
everything else that wants setting information should be making calls to User 
to get this info. For the most part, that's just what happens; the lone 
exception to this is editsetting.cgi, and only there because it needs to access 
Setting directly for admin purposes. Since that *is* an exception, and we don't 
want people mucking with that stuff as a general rule, I don't export those 
admin subroutines, and they must be called by their full name. 

OTOH, the subs that User needs *are* exported... because User is the only place 
that should have the "use Bugzilla::User::Setting;" directive, and the Setting 
subs should really be part of the overall User namespace.

That't the logic behind having it this way, and it makes a lot of sense to me. 
If I'm missing something, please point out where.

Comment 44

[Jouni Heikniemi]

Comment on attachment 173917 [details] [diff] [review]
Code patch for tip

A few general notes:

You seem to be exceeding 80 chars for line length in quite a few places. I'm
not going demand you to wrap all the lines, but please do pay attention and
attempt to keep at max ~78 chars for lines that absolutely don't need the extra
width.

Re discussion on IRC, I support having syntactic sugar for accessing prefs:
Somethign like User->setting('foo') would be great.

The patch crashes when going to userprefs.cgi with no setting entries in the
DB. Either we ship with some, or we fix this so it won't crash. The error is
"Can't use an undefined value as a HASH reference at
/var/www/bugzilla/tip/userprefs.cgi line 257." 

The patch doesn't pass the testing suite; fix this before next iteration.



Onto the code, UI notes embedded:


>--- tip/Bugzilla/User/Setting.pm	1969-12-31 18:00:00.000000000 -0600
>+++ tip.userprefs/Bugzilla/User/Setting.pm	2005-02-09 15:26:31.251790429 -0600

>+#    bless ($self, $class);

Nit: Why is this here, commented out?

>+sub GetDefaultSettings {
>+sub SetDefaultSetting 

For some reason, I find "Default Settings" particularly clumsy in this context.
I could quite well go with "GetDefaults" and "SetDefault" (or SetDefaultValue
perhaps), but this is naturally no showstopper.

>+        $settings->{$name} = new Bugzilla::User::Setting
>+          ($name, $user_id, $is_enabled, GetLegalValues($name),
>+           $default_value, $value, $is_default);

I'm slightly afraid of the perf impact the continous GetLegalValues calls are
going to cause. Continously retrieving practically never-changing data feels
like an awkward design decision. Although premature optimization blows, I still
strongly think we should avoid adding payload to User object construction.
Versioncache is evil, but it does come to mind. The other alternative might be
to just get the alternatives when we need them (prefs page?).



User.pm:
========

FWIW, and obviously a nit, I think the following code is clumsy:

>+    return $self->{'settings'} if defined $self->{'settings'};
>+
>+    # IF the user is logged in
>+    # THEN get the user's settings
>+    # ELSE get default settings
>+    if ($self->id) {
>+        $self->{'settings'} = GetUserSettings($self->id);
>+    } else {
>+        $self->{'settings'} = GetDefaultSettings();
>+    }
>+
>+    return $self->{'settings'};

I'd find it much more elegant with structure like the following:

--
if (!defined $self->{'settings'}) {
  # If logged in, get user's settings; else use the defaults
  $self->{'settings'} = ($self->id) ? GetUserSettings($self->id)
				    : GetDefaultSettings();
}
return $self->{'settings'};
--

Note the absence of two returns and the clarified structure of the if block.
Just my $0.02.


>+sub delete_user_setting{

Nit: Space before {

>+Returns a hash of hashes which holds the user's Settings. The first key is

Unless there's particular reason to capitalize Setting, don't do it. You're
using very mixed case anyway; I think all lower will suffice here.

>+is_enabled     - whether or not the user is allowed to create a preference
>+                 for themselves or must accept the global site default value

Slightly unclear for me. Perhaps: "true if the user is allowed to set the
preference themselves; false to force the site defaults".

>+legal_values   - an array of all allowable values for this setting

I'd say "allowed", but this is nitpicking.

>+value          - the value of this setting for this user. Guaranteed to be
>+                  the same as default_value if the user is not logged in,
>+                  or if is_default is true; otherwise, could be any value
>+                  found in legal_values.
>+is_default     - a boolean to indicate whether the user has chosen to make
>+                  a preference for themself or use the site default.

Nit: The additional lines of value and is_default documentation are indented by
one space too many.


Checksetup.pl
=============

>+$table{setting_value} = 
>+    'name           varchar(32)     not null,
>+     value          varchar(32)     not null,
>+     sortindex      tinyint,        not null,

That comma over there crashed my checksetup (invalid SQL Syntax). Running MySQL
4.0.23.




The UI, some generic comments:

I don't find the way the prefs page works quite optimal. First, it looks fairly
complex (it isn't that difficult a thing, really!). Second, the way the "rely
on default" option works isn't very intuitive. My recommendations are:

1) Dramatically reduce the amount of explanatory text (I'll give better
pointers later)

2) Waste the "Use default" and "Default value" columns, and rather integrate
them into a new combo box choice (f.e. ("Site default (On)")). Unless you do
this, you'll have to write JavaScript to block useless controls; I refuse to
accept an UI where you can check "Use default" and then make totally irrelevant
choices in the "Current value", which will then be nuked on submission. ;-)

So what I'd like to see is options like


Use quips at the top of bug list?  [			|v]
				   | Site default (No)	|
				   | Yes		|
				   | No 		|
				   | Randomly		|
				   +--------------------+



>+++ tip.userprefs/template/en/default/account/prefs/settings.html.tmpl	2005-02-09 17:14:37.022406199 -0600

>+  #                 is_enabled    - integer.

Nit: Boolean actually. It may technically be an integer, but conceptually it is
bool.

>+  #                 is_default    - integer (true if user has no preference)

Likewise (and integers cannot even have the value "true" ;-)).

>+<table>
>+  <tr>
>+    <th align="right">Setting Text:</th>
>+    <td>A description of what the Setting controls</td>
>+  </tr>              
>+  <tr>
>+    <th align="right">Current Value:</th>
>+    <td>A list of choices for this Setting; your current value is displayed. Only relevant if 'Use Default' is not checked.</td>
>+  </tr>              
>+  <tr>
>+    <th align="right">Default Value:</th>
>+    <td>The default value of this Setting for this site</td>
>+  </tr>              
>+  <tr>
>+    <th align="right">Use Default:</th>
>+    <td>If checked, the default value will always be used; if the Admin changes the default, your Setting will change too.</td>
>+  </tr>              
>+  <tr>
>+    <td colspan="2"><hr></td>
>+  </tr>
>+</table>

I think all of this can be removed. Really, "A list of choices for this
Setting; your current value is displayed" isn't particularly necessary for a
person generally able to use Bugzilla anyway. :-)

>+[% IF settings.size %]
>+  <table border cellpadding="4">
>+  <tr>
>+    <th>Setting Text</th>
>+    <th>Current Value</th>
>+    <th>Default Value</th>
>+    <th>Use Default</th>
>+  <tr>

Nit: You're missing a "</tr>" here.

You can also remove these column headings. Just having the setting descriptions
and the combo boxes will do quite well if we have no other columns (option 2
above).

>+                <label for="[% name %]">
>+                  <select name="[% name %]" id="[% name %]">
>+                    [% FOREACH x = settings.${name}.legal_values %]
>+                        <option value="[% x FILTER html %]"
>+                          [% " selected=\"selected\"" IF x == settings.${name}.value %]>
>+                          [% setting_descs.${x} OR x FILTER html %]
>+                        </option>
>+                    [% END %]
>+                  </select>
>+                </label>

The label element is unnecessary here; you can just remove it. 

>+              <td align=center>

Quotes around center. Repeat a few lines further down.

>+                [% setting_descs.${settings.${name}.default_value} FILTER none %]

Why filter none? In general, why do we treat setting_descs as already HTML
encoded? Also, should we have a fallback here in case setting_descs doesn't
have the proper translation?

>+                  [% " checked=\"checked\"" IF settings.${name}.is_default %]>

No need to change this if you don't want to, but a tip for the future: you can
write " checked='checked'" or ' checked="checked"' to get rid of the ugly
backslashes.


>+++ tip.userprefs/template/en/default/admin/settings/edit.html.tmpl	2005-02-09 17:15:13.974798651 -0600

>+  # settings:      array of hashes, keyed by setting_name.

How is this an "array of hashes"? I'd expect this to be a "hash of hashes",
huh?

>+  #                Each hash contains:
>+  #                 is_enabled    - integer.
>+  #                 legal_values  - array of strings
>+  #                 default_value - string (global default for this setting)
>+  #                 value         - string (user-defined preference)
>+  #                 is_default    - integer (true if user has no preference)

As above with the integer/boolean stuff. What is that value in this context
anyway?

>+  <br>
>+  The Default Value displayed for each Setting will apply to all users who
>+  do not choose their own value, and to anyone who is not logged in.<br>
>+  <br>
>+  The 'Enabled' checkbox controls whether or not this Setting is available to users.<br>
>+  If it is checked, users will see this Setting on their User Preference page,
>+  and will be allowed to choose their own value if they desire.<br>
>+  If it is not checked, this Setting will not apppear on the User Preference page,
>+  and the Default Value will automatically apply to everyone.<br>
>+  <br>

Use <p>...</p> elements to delimit paragraphs.

Nit: "User preferences page" (note the plural: preferences).

>+  <tr><td colspan=2><hr></td></tr>

Quote "2"

>+                <label for="[% name %]">
>+                  <select name="[% name %]" id="[% name %]">
>+                    [% FOREACH x = settings.${name}.legal_values %]
>+                        <option value="[% x FILTER html %]"
>+                          [% " selected=\"selected\"" IF x == settings.${name}.default_value %]>
>+                          [% setting_descs.${x} OR x FILTER html %]
>+                        </option>
>+                    [% END %]
>+                  </select>
>+                </label>

As above for the label.

>+                <input type="checkbox"
>+                  name="[% checkbox_name %]"
>+                  id="[% checkbox_name %]"

Technically, the name might not be a valid ID (not necessarily a valid name
either, but more likely so). I'm not sure if I really care that much, but it
would be rather easy to prefix it with a letter and make sure no illegal
characters occur. This comment applies for numerous other lines as well.

>+          <td>
>+            <input type="submit" value="Submit Changes">
>+           </td>

Nit: Closing td indented one space too much.

>+  [% ELSE %]
>+    There are no Settings to edit.
>+  [% END %]

On this template, Settings tend to be capitalized. I'd say "settings", despite
all the architectural finesse involved. ;-)


Footer links:
=============

>+        [% ' | <a href="editparams.cgi">Parameters</a> ' _
>+           ' | <a href="editsettings.cgi">Settings</a>' 

We can't have "parameters" and "settings" side by side, they mean quite the
same to the normal person (including me). "User settings" is fine with me, even
if it is somewhat suboptimal.

--

As I already told Travis on IRC, it's a good patch and the structure feels
solid. All that despite my previous comments ;-) Some parts still need a bit of
hammering and polish, but we're definitely getting there this time.

Comment 45

[Shane H. W. Travis]

(In reply to comment #42)
> ... if we can make life easier
> for people using administrative tools for relatively little effort,
> I don't see the downside.

Downside is that:
"SELECT foo FROM bar WHERE baz AND (condition2 OR condition3) OR condition3"

is not as easy for a human to parse or maintain as:
q{SELECT foo
   FROM bar
  WHERE baz
    AND (condition 1
         OR condition2)
     OR condition3}

Human readability and ease of maintenance is important too when we're dealing 
with this many programmers. 

Yes, it's possible to do something akin to the above with quotes and 
concatenation around every line instead of a single q{}, but now you're adding 
a LOT more characters, obfuscating readability again, and significantly 
compounding the chance of syntactical error (forgetting a quote or a period).


>   It doesn't return a hash reference.

Okay, fine, my mistake... a *list* reference. :) I still don't see how that's 
useful when I want three distinct and different pieces of data, but maybe you 
can show me why this is more effective. I'm willing to learn, I'm just not 
convinced atm.

(I did change other places based on your earlier pre-formal review suggestions, 
but this didn't seem like a useful place to try and shoehorn it in.)

 
>   Actually, it's fairly useful to have a sortindex that's a smallint. It helps
> for creating values like 100, 200, 300, 400, in case you plan to possibly put 
a
> lot of values between them in the future and you don't know how they're going 
to
> sort.

Fair enough. This is a good reason for it, and doing it this way doesn't hurt 
anything. I'll change it.


>   All the sortindexes in the enums patch are smallints, too.

OTOH, this (IMHO) is *not* a good reason. "I did it this way, so you should 
too" is a nit, not an r-. I too believe in that consistency is good, but I balk 
at doing something ONLY for the sake of consistency when the 'inconsistent' way 
has merit too.

(This stance also explains my reasons for my resistance to the one-line-SQL 
comments above. Right now, there are pros and cons to either side. If we adopt 
one or the other as canon, I'd go along with it because That's The Way It's 
Done. I don't see that as having happened, though, so it's still up to each 
programmer to decide which factors are more important to them, and code 
accordingly. I've asked Dave to take a look at the bug, though, and decide if 
there is going to be a Developers' Guide change for this recommendation.)

Comment 46

[Max Kanat-Alexander]

(In reply to comment #45)
> Downside is that:
> "SELECT foo FROM bar WHERE baz AND (condition2 OR condition3) OR condition3"
> 
> is not as easy for a human to parse or maintain as:
.
  Yeah. I was thinking about that, too, the other day. I think that we should
definitely go one way or the other, and make that the standard as much as possible.

  I do tend to do:

  "SELECT foo, bar, baz"
 . " FROM qux"
 . "WHERE shazam = ?"

> >   It doesn't return a hash reference.
> 
> Okay, fine, my mistake... a *list* reference. :) I still don't see how that's 
> useful when I want three distinct and different pieces of data, but maybe you 
> can show me why this is more effective. I'm willing to learn, I'm just not 
> convinced atm.

  OK! :-)

  my ($foo, $bar, $baz) = $dbh->selectrow_array($query, undef, $value);

> >   All the sortindexes in the enums patch are smallints, too.
> 
> OTOH, this (IMHO) is *not* a good reason.

  I know. :-) I just threw it in there. :-)

Comment 47

[Shane H. W. Travis]

(In reply to comment #40)
> Nit: If that all returns only one row, it can be a selectrow_arrayref.

(In reply to comment #42)
>   It doesn't return a hash reference. selectrow_arrayref.

(In reply to comment #46)
>   my ($foo, $bar, $baz) = $dbh->selectrow_array($query, undef, $value);

Oh, well, if you're going to change the name of the function, then yeah... I 
can see how this could be useful. Everywhere else (including the pre-formal 
review) you kept harping on how I should use selectrow_arrayref, though, so you 
can understand my confusion. :)

Comment 48

[Shane H. W. Travis]

(In reply to comment #44)
> The patch crashes when going to userprefs.cgi with no setting entries in the
> DB. Either we ship with some, or we fix this so it won't crash.

Latter would probably be good, but the former is definitely going to be the 
case. I have a patch on bug 41972 ready to go as soon as this lands, which will 
add the first Setting to the table.

> >+#    bless ($self, $class);
> Nit: Why is this here, commented out?

Because I saw it in a few other .pm files, but didn't know what it did; I had 
it here as a mental note to ask and learn, but then forgot to do either.

> So what I'd like to see is options like
> Use quips at the top of bug list?  [	                  |v]
>                                    | Site default (No)  |
>                                    | Yes                |
>                                    | No                 |
>                                    | Randomly           |
>                                    +--------------------+

I like this idea a lot. I'll work to implement it.


> >+                [% setting_descs.${settings.${name}.default_value} FILTER 
none %]
> Why filter none? In general, why do we treat setting_descs as already HTML
> encoded? 
We don't. Three instances on that page, two are 'FILTER html'. This one was a 
mistake/oversight.


> Also, should we have a fallback here in case setting_descs doesn't
> have the proper translation?

Yes. I added the fallback very late in the process, and simply missed this 
instance. Thanks.


> >+++ tip.userprefs/template/en/default/admin/settings/edit.html.tmpl	2005-02-
09 17:15:13.974798651 -0600
> >+  # settings:      array of hashes, keyed by setting_name.
> How is this an "array of hashes"? I'd expect this to be a "hash of hashes",

At the time that I wrote the interface docs, it *was* an array of hashes. It 
got changed later in the design process (to a hash of hashes) and I forgot to 
update the docs. Good catch.


> We can't have "parameters" and "settings" side by side, they mean quite the
> same to the normal person (including me). "User settings" is fine with me, 
even
> if it is somewhat suboptimal.

I'd like to leave it as Settings, although I'll change it if it's a sticking 
point. Ultimately, what I want is for 'Settings' to be another panel on a multi-
panel Parameters page, but that can't happen until Dave (codes and) lands bug 
46296.

As usual, an excellent review. Thanks, Jouni!

Comment 49

[Max Kanat-Alexander]

(In reply to comment #47)
> Oh, well, if you're going to change the name of the function, then yeah... I 
> can see how this could be useful. Everywhere else (including the pre-formal 
> review) you kept harping on how I should use selectrow_arrayref, though, so you 
> can understand my confusion. :)

  Yeah. :-) I only recently realized that selectrow has a pure "array" version,
and doesn't need an arrayref. And that thing about hashref was just me being
confused for a second. :-) I realized right after I posted that I meant "array,"
not "hash." :-)

Comment 50

[Shane H. W. Travis]

(In reply to comment #45)
> >   Actually, it's fairly useful to have a sortindex that's a smallint.
> > It helps for creating values like 100, 200, 300, 400, in case you plan
> > to possibly put a lot of values between them in the future and you don't
> > know how they're going to sort.

I *knew* I had a problem with this logic, but it's taken me three days to 
remember what it was.

I completely agree that this is a valid comment for *user-created* 
smallindices, like those for flags, which may be modified in the future by a 
User or Admin. This index, however, is only accessible by the programmer, and 
is not modifiable in any way by any user. 

The programmer who creates a setting (i.e. the person who fixes one of the bugs 
that this bug blocks) is the one who defines the sort order and choices, and 
the likelihood is that nobody else should ever be touching it beyond that 
point. For that reason, I think that it works perfectly well as a tinyint.

It's no real skin off my nose regardless; I've already made the changes in the 
code, and I don't care enough to change it back, but it's been bugging me that 
I examined and rejcted this logic once already during the design process, then 
couldn't remember the reasoning when you brought it up.

Comment 51

[Shane H. W. Travis]

Attachment: Code patch for tip, take 2

This patch incorporates most of the suggestions above. 
Max:
- I have not taken newlines out of SQL, as Dave has a good comment on that bug
   about attacking the problem in a single location instead of stamping out
   dozens of little fires.
- I also did not take out 'use vars' as your suggested replacement looked like
   more code than the original.
- renamed all units as suggested

Jouni:
- Tried to keep it below 80 chars everywhere. If anyone finds one and it
   doesn't look absolutely necessary, please mention it.
- I *just* realized that I forgot the 'syntactic sugar' you mentioned. If
   there's a third version, I'll do it then; if not, I'll make a 3rd version
   with one in it.
- now passes testing suite
- renamed all units as suggested
- GetLegalValues now called only when info is needed for editsettings.cgi or
   userprefs.cgi
- Changed userprefs.cgi UI to work as you suggested; now consists only of
   string and picklist
- Removed extraneous explanatory text from userprefs.cgi page
... and about everything else you asked for. :)


I did not fix all nits, as I didn't agree with all of them. (For the most part,
though I did.) Since they're just nits, however, I did not bring them up to
argue about them.

Thanks to both of you for reviews!

Comment 52

[Joel Peshkin]

This is a very good idea.  I would not keep this from landing to slide this in,
but isn't a varchar(32) a bit small especially because we have bugs depending on
this to store column-list orders, etc...??

Comment 53

[Jouni Heikniemi]

Comment on attachment 174615 [details] [diff] [review]
Code patch for tip, take 2

>--- tip/Bugzilla/User/Setting.pm	1969-12-31 18:00:00.000000000 -0600
>+++ tip.userprefs/Bugzilla/User/Setting.pm	2005-02-17 14:58:50.682724772 -0600

>+    # Confirm that the $setting_name is properly formed;
>+    # if not, throw a code error.

Please add a comment saying "Note that the due to the way this is currently
used, setting names must conform to the limitations set for HTML NAMEs and
IDs." (or something equivalent)

>+    # If there's nothing left after taking out the setting and ID

Nit: "If there were only two parameters" rather than this explanation; the
exact method of popping the param stack isn't quite as descriptive as the
parameter count -based explanation.

>+          $dbh->selectrow_array(
>+              q{SELECT default_value, is_enabled, setting_value
>+                  FROM setting
>+             LEFT JOIN profile_setting
>+                    ON setting.name = profile_setting.setting_name
>+                 WHERE name = ?
>+                   AND (profile_setting.user_id = ?
>+                        OR profile_setting.user_id IS NULL)},
>+                                undef, $setting_name, $user_id);

Nit: Why is the last line indented like that? I'd expect the u of the undef to
be aligned with the q that starts the first parameter.

>+        # Don't flog the db with requests for legal values 
>+        # unless they're needed.
>+        my $legal_values = 0;
>+        $get_legal_values && do {$legal_values = _get_legal_values($name);};

Re this, I think this is an acceptable approach, but it would be a better idea
to refactor this code a bit to avoid repeating the logic and also to keep the
code API better documented (after all, you shouldn't have know _at object
construction time_ whether the legal values should are going to be needed).
Make "get_legal_values" a method of the Setting object, and retrieve the data
in it (caching internally as necessary for multiple invocation of the method).
This way the object construction would never load that data, and it would
always be loaded automatically when needed. How does that sound?

(well, we discussed this a bit on IRC - but I'll still post the suggestion
here)


>--- tip/template/en/default/global/setting-descs.none.tmpl	1969-12-31 18:00:00.000000000 -0600
>+++ tip.userprefs/template/en/default/global/setting-descs.none.tmpl	2005-02-10 14:32:09.000000000 -0600

>--- tip/template/en/default/global/setting-descs.none.tmpl.for_testing	1969-12-31 18:00:00.000000000 -0600
>+++ tip.userprefs/template/en/default/global/setting-descs.none.tmpl.for_testing	2005-02-10 14:32:21.000000000 -0600

FWIW, these two are equal. This won't matter for checkin of course, but the
other reviewers might want to pull the descs from the 2nd attachment of this
bug.



Re this:

>> We can't have "parameters" and "settings" side by side, they mean quite the
>> same to the normal person (including me). "User settings" is fine with me, 
>> even if it is somewhat suboptimal.
>I'd like to leave it as Settings, although I'll change it if it's a sticking 
>point. Ultimately, what I want is for 'Settings' to be another panel on a multi-
>panel Parameters page, but that can't happen until Dave (codes and) lands bug 
>46296.

Ultimately, we all want that sandwich-making machine ;-) Since we're going to
have to update the administration documents and UI twice anyway (once for
adding this feature, the second time integrating it into the multi-panel
params), it's simply a question of how to name it. Settings and Parameters mean
essentially the same (to the normal person), thus they cannot exist
side-by-side meaning two fairly different aspects of Bugzilla configuration. I
still want it changed, and vote for "User settings" in absence of a better
term.


My thanks on a job well done - with patches of this size, it's fairly common to
have to comment on the same things twice. You did exceptionally well in getting
all the comments integrated and/or responded to. That really makes reviewing
much smoother. 

Although I'd like to see the API issue with loading the legal values being
addressed, the current solution is acceptable as well. I'm confident in your
ability to pick a solution and implement it properly. :-) Knowing the schedule
I will have in the near future (lots of vacations and other interruptions), I
don't want to be the one holding this up. Therefore, r=jouni with the fixes
mentioned above.

Comment 54

[Shane H. W. Travis]

Attachment: Code patch for tip, take 3

- Fixed the legal_values issue as per IRC discussions with Jouni.
- Fixed all nits/comments made by Jouni in comment #53
- Added syntactic sugar subroutine setting_value()

Approval for r+ in advance by jouni considering those, but asking for one final
once-over from Max too, just to keep it all legal. :)

Comment 55

[Max Kanat-Alexander]

Comment on attachment 175228 [details] [diff] [review]
Code patch for tip, take 3

>+    if ( !($setting_name =~ /^[a-zA-Z][-.:\w]*$/) ) {
>+      ThrowCodeError("setting_name_invalid", { name => $setting_name });
>+    }

   Maybe we should have a validate_setting_name function some day...

>+             undef, 
>+             $setting_name, $user_id);

  Nit: Those arguments can all be on the same line.

  Also, I think the General Settings tab should be next to Account Settings.

  Only one actual review- point: Now that get_legal_values is public, it needs
POD docs.

Comment 56

[Shane H. W. Travis]

(In reply to comment #55)
>    Maybe we should have a validate_setting_name function some day...

That's sort of what this is. Remember that only developers will be adding 
settings, not users or administrators. Given that, even a single run of testing 
will cause this code to spit an error.

>   Only one actual review- point: Now that get_legal_values is public, it needs
> POD docs.

This confuses me. Are you saying that only get_legal_values needs POD 
documentation? Or, having done those, would you then r- me for not having POD 
docs for everything else in the unit? (Or did you miss the fact that there are 
no POD documents for Setting.pm?)

Comment 57

[Max Kanat-Alexander]

Oh, I must have missed that there are no POD docs for Setting.pm.

It does need POD docs. Would you write them for another bug if we check this in now?

Comment 58

[Shane H. W. Travis]

(In reply to comment #57)
> It does need POD docs. Would you write them for another bug
> if we check this in now?

Absolutely... and much preferred. I don't want to have to do three more 
iterations of this while I get those right, if at all possible. OTOH, if this 
gets checked in, people can start working on the dependents while I finish up 
the internal documentation.

Created bug 283357 to track that.

Comment 59

[Max Kanat-Alexander]

Comment on attachment 175228 [details] [diff] [review]
Code patch for tip, take 3

OK, r=mkanat, then. :-)

Comment 60

[Shane H. W. Travis]

Attachment: updated to avoid bitrot

Patch has rotted a bit in the last couple of weeks (not much, though), and I
found two small errors while implementing bug 41972. Fixed both of those. I
also removed the 'syntactic sugar' I added in v3, as it turned out not to be
worth it.

Carrying r+ forward.

Comment 61

[Shane H. W. Travis]

Attachment: Code patch for tip, take 5

I know... once you've got r+ you're supposed to stop changing things. :)

In fixing bug 41972, however, I added some code there that really belongs in
this bug -- namely, the changes to checksetup.pl and code-error.html.tmpl. I
justified it by saying to myself that they were going in at the same time, but
since I'm making a new patch *anyway*, I may as well include them in the proper
place.

The new patch is needed because I've moved the Bugzilla::User::delete_setting
to Bugzilla::User::Setting::reset_to_default, and Bugzilla::User::set_setting
Bugzilla::User::Setting::set. The make more sense as methods within Setting.pm
than within User. I also made legal_setting into a proper method, so very
nicely fixes up the problem Jouni raised earlier. Finally, I added POD
documentation to Setting.pm, as was asked for in bug 283357; I figured that I
was respinning the patch anyway, so I may as well do it properly from the
ground up.

Max, some of your comments (about placeholders) from bug 41972 now transfer
over here, since the code moved; I fixed them as part of this patch.

Comment 62

[Max Kanat-Alexander]

Comment on attachment 176779 [details] [diff] [review]
Code patch for tip, take 5

>+sub get_legal_values {
>+    my ($self) = @_;
>+    my $dbh = Bugzilla->dbh;

  This probably ought to have a $self->{legal_values} hash to cache itself in.

>+sub reset_to_default {
>+    my ($self) = @_;
>+
>+    my $dbh = Bugzilla->dbh;
>+    my $sth = $dbh->do(q{ DELETE
>+                            FROM profile_setting
>+                           WHERE setting_name = ?
>+                             AND user_id = ?},
>+                       undef, $self->{'_setting_name'}, $self->{'_user_id'});

  You also need to actually change the in-memory Setting, too. That is, the
$self->{is_default} value and so forth.

>+sub set {
[snip]
>+    $dbh->do($query, undef, $value, $self->{'_setting_name'}, $self->{'_user_id'});

  Same for this one, too. You need to change the $self->{value} value.

>+=head1 NAME
>+Bugzilla::User::Setting - Object for a user preference setting
>+
>+=head1 SYNOPSIS
>+Setting.pm creates a setting object, which is a hash containing the user
>+preference information for a single preference for a single user.

  You'll want to note that these are usually accessed through the "settings"
object of a user, and not directly.
>+=item C<get_all_settings($user_id)>
>+
>+Description: Provides the user's choices for each setting in the 
>+system; if the user has made no choice, uses the site default instead.

  We usually indent the stuff all the way out to where the text for Description
starts, for all of the parts. So, for example:

Description: Provides the user's choices for each setting in the 
	     system; if the user has made no choice, uses the site
	     default instead.

Params:      C<$user_id> - integer - the user id.
Returns:     A pointer to a hash of settings.

>+
>+# tables for the 'settings' 
>+

  Just to let you know, these are going to have to go into the
Bugzilla::DB::Schema structure. You'll probably want to start looking at the
final patch on bug 146679 for an idea of how that works. I'll send out an email
to developers@ after I check that code in, explaining it.

>+###########################################################################
>+# Populate setting tables
>+###########################################################################

  I'd really like it if these subs could go into Bugzilla::User::Setting, and
then we did a

  require Bugzilla::User::Setting;
  import Bugzilla::User::Setting qw(setting_exists add_setting);

  (You can't call 'use' in checksetup.)

>+    # We need to obtain the list of legal values for each
>+    # setting, so it can be displayed on the UI picklist
>+    foreach my $name (@setting_list) {
>+        $vars->{'settings'}->{$name}->{'legal_values'} =
>+          $vars->{'settings'}->{$name}->get_legal_values;
>+    }

  You know, now that it's a method of the Setting objects, you don't even
really need to do that. You can just pass the Settings array into the template
and get the legal_values object out of each one.

  Same deal with userprefs.cgi.

Comment 63

[Shane H. W. Travis]

Attachment: Code patch for tip, take 6

Alright... I have updated this to use the new DB::SCHEMA, moved the subroutines
from checksetup into Setting.pm, and addressed pretty much everything else you
mentioned. Should be good to go.

Comment 64

[Max Kanat-Alexander]

Comment on attachment 176887 [details] [diff] [review]
Code patch for tip, take 6

>+    # SETTINGS
>+    # --------
[snip]

  Yay, this all looks perfect! And thank you for the descriptive comments, too!
:-D

>+sub add_setting {
>+    my ($name, $values, $default_value) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    return if setting_exists($name);

  Forgot the underscore.

>+sub _setting_exists {
>+    my ($setting_name) = @_;
>+    my $dbh = Bugzilla->dbh;
>+    my $sth = $dbh->prepare("SELECT name FROM setting WHERE name = ?");
>+    $sth->execute($setting_name);
>+    return ($sth->rows) ? 1 : 0;
>+}

  Nit: A perhaps nicer way to do this would be:

  return $dbh->selectrow_array("SELECT COUNT(*) FROM setting WHERE name = ?",
undef, $setting_name);

>+sub get_legal_values {

  Nit: At this point, it's probably better just to call it legal_values. But
it's not a huge deal.

>+=item C<add_setting($user_id)>

  Missing the other two vars in the prototype, here.

>--- tip/Bugzilla/User.pm	2005-03-07 12:10:57.000000000 -0600
>+++ tip.quips/Bugzilla/User.pm	2005-03-09 01:00:30.000000000 -0600
[snip]
> 
> use base qw(Exporter);

  Did we remove the setting_value export from this patch?

  Everything else looks great. :-)

Comment 65

[Shane H. W. Travis]

Attachment: Code patch for tip, take 7

>  Forgot the underscore.
Fixed

>  Nit: A perhaps nicer way to do this would be:
At this point, I don't really care. :) Untouched.

>  Nit: At this point, it's probably better just to call it legal_values.
This I agree with, care about, and fixed.

> Missing the other two vars in the prototype, here.
Fixed

> Did we remove the setting_value export from this patch?
Yes, the Export was removed from this patch. The actual subroutine was removed
a couple of patches back, but I forgot about the Export, and just caught it
this time around.

Comment 66

[Max Kanat-Alexander]

Comment on attachment 176893 [details] [diff] [review]
Code patch for tip, take 7

Great. Interdiff looks right. :-)

Comment 67

[Shane H. W. Travis]

Comment on attachment 176893 [details] [diff] [review]
Code patch for tip, take 7

NOTE: 'default_value' should just be 'value' on both of the following lines:

+	     default_value => {TYPE => 'varchar(32)', NOTNULL => 1},

+	     setting_value_nv_unique_idx  => {FIELDS => [qw(name
default_value)],

I will be fixing this on checkin.

Comment 68

[Max Kanat-Alexander]

Attachment: Code patch for tip, take 8

This fixes a typo in the Schema that travis mentioned in IRC.

Comment 69

[Max Kanat-Alexander]

Attachment: Code patch for tip, take 8.1

Oh, fixed it in the index, too.

Comment 70

[Shane H. W. Travis]

/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.365; previous revision: 1.364
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/editsettings.cgi,v
done
Checking in editsettings.cgi;
/cvsroot/mozilla/webtools/bugzilla/editsettings.cgi,v  <--  editsettings.cgi
initial revision: 1.1
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.70; previous revision: 1.69
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.45; previous revision: 1.44
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.5; previous revision: 1.4
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User/Setting.pm,v
done
Checking in Bugzilla/User/Setting.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User/Setting.pm,v  <--  Setting.pm
initial revision: 1.1
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <-
-  filterexceptions.pl
new revision: 1.37; previous revision: 1.36
done
Checking in template/en/default/account/prefs/prefs.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/prefs.html.
tmpl,v  <--  prefs.html.tmpl
new revision: 1.17; previous revision: 1.16
done
RCS 
file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/setti
ngs.html.tmpl,v
done
Checking in template/en/default/account/prefs/settings.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/settings.ht
ml.tmpl,v  <--  settings.html.tmpl
initial revision: 1.1
done
RCS 
file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/settings/edit
.html.tmpl,v
done
Checking in template/en/default/admin/settings/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/settings/edit.html.
tmpl,v  <--  edit.html.tmpl
initial revision: 1.1
done
RCS 
file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/settings/upda
ted.html.tmpl,v
done
Checking in template/en/default/admin/settings/updated.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/settings/updated.ht
ml.tmpl,v  <--  updated.html.tmpl
initial revision: 1.1
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-
error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.47; previous revision: 1.46
done
Checking in template/en/default/global/field-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/field-
descs.none.tmpl,v  <--  field-descs.none.tmpl
new revision: 1.7; previous revision: 1.6
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/setting-
descs.none.tmpl,v
done
Checking in template/en/default/global/setting-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/setting-
descs.none.tmpl,v  <--  setting-descs.none.tmpl
initial revision: 1.1
done
Checking in template/en/default/global/useful-links.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/useful-
links.html.tmpl,v  <--  useful-links.html.tmpl
new revision: 1.35; previous revision: 1.34
done

Comment 71

[Stephen Lee]

Went to see what this looks like, and
http://landfill.bugzilla.org/bugzilla-tip/userprefs.cgi?tab=settings
says:
"Can't use an undefined value as a HASH reference at /var/www/html/bugzilla-
tip/userprefs.cgi line 146."

Problem with this patch or with landfill?

Comment 72

[Shane H. W. Travis]

Neither; there's just no Settings defined yet. That's why I had to wait for bug 
41972 to be ready before this could land... but I got called away after 
committing one before I could commit the other. Give it another 20 minutes, 
then check out the tip again. :)

Comment 73

[Max Kanat-Alexander]

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

Comment 74

[Max Kanat-Alexander]

Added to the release notes in bug 286274.