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.

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

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

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

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

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

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.

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.

Modifying summary to make this bug easier to find.

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

Darn. It's so necessary.

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.

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?

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?)

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

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.

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)

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.

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?

(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. :)

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.

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

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!

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.

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!

(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!

(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)?

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

(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. ;)

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

(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?

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

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

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.

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 :)

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

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.

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.

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

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

(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!

(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. :-)

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

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

(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. :-)

(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. :)

(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!

(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." :-)

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

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!

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

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

(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?)

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?

(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 on attachment 175228 [details] [diff] [review] Code patch for tip, take 3 OK, r=mkanat, then. :-)

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.

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

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

> 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 on attachment 176893 [details] [diff] [review] Code patch for tip, take 7 Great. Interdiff looks right. :-)

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.

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

Oh, fixed it in the index, too.

/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

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?

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

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

Added to the release notes in bug 286274.