72118 - Remember user's columns in "account" rather than per browser

Status: ASSIGNED

(Bugzilla :: User Accounts, enhancement, P1)

Description

[Peter Lubczynski]

When you pick columns in a bugzilla query, they reset if you use another
browser. It would be nice to remember these.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

->New product Bugzilla

Comment 2

[Frédéric Buclin]

Should have been done a long time ago. Does not look that hard to implement.

Comment 3

[Dave Miller [:justdave]]

this could in theory be stored with saved queries, too.

Comment 4

[Shane H. W. Travis]

No reason why both couldn't be accomplished; a default set of query-headers 
that is stored somewhere on a per-user basis, but also create the capability to 
store headers on a per-query basis to override the default.

To use this with 'settings' (bug 98123), you would have to significantly modify 
the current UI for that. At this point, that UI only works for picklists of 
discrete values. I can imagine expanding that to allow a single freeform field 
(so people could enter something like a URL) but creating a colchange.cgi-like 
interaction is way in the future. 

Perhaps, once the freeform stuff is implemented, it might be possible just to 
implement a solution like this:
 * have an additional checkbox in colchange.cgi that says something like "store
   this column list as my default selection". That would create a string akin
   to what's currently in the cookies, and store it the same as a freeform
   entry is stored
 * buglist.cgi would look for the column-setup in the DB instead of in a
   cookie. If it didn't find one, it would use the site default.
 * User's column choices would be displayed on the 
   http://rs-svr.sedsystems.ca/bugzilla-test/userprefs.cgi?tab=settings page,
   but wouldn't be modifiable from there. Instead there'd be instructions on
   how to modify them through the colchange.cgi page.

That's just one thought on how it could work. As I said, it would require some 
extension of the current Settings.pm capability.

Comment 5

[Greg Hendricks]

Attachment: Saved search with columns

This patch adds a checkbox next to the saved search entry box to also save the column list. Buglist already looks at the URL for a column list before looking at the cookie. This just saves the columnlist with the query. 
You can update the column list and save the query again if needed as well.

Comment 6

[Marc Schumann [:Wurblzap]]

Hmmm... For whining, we don't want the column set of the user who set up the saved search, but the column set of the user who receives the whine mail. I'm not too sure the search is the right place to store the column set with.

Same goes for the last HTTP_ACCEPT_LANGUAGE string a user used. Although that's another bug, maybe we can use the same infrastructure for both.

Comment 7

[Marc Schumann [:Wurblzap]]

Related: bug 294708, bug 245375.

Comment 8

[Greg Hendricks]

Attachment: Unbitrotted

Comment 9

[Greg Hendricks]

(In reply to comment #6)
> Hmmm... For whining, we don't want the column set of the user who set up the
> saved search, but the column set of the user who receives the whine mail. I'm
> not too sure the search is the right place to store the column set with.
> 
> Same goes for the last HTTP_ACCEPT_LANGUAGE string a user used. Although that's
> another bug, maybe we can use the same infrastructure for both.
> 

We could just strip the columnlist from the query when we run it for whines.

Comment 10

[Marc Schumann [:Wurblzap]]

What I mean is that, for whining, we need what the bug's current summary says, which is to remember a user's columns in the account rather than per browser. You're not solving this if you save the columns with the query. Your stone would hit only one of the two flies it *could* hit :)

Comment 11

[Rob Siklos [:robzilla]]

The patch as-is can cause confusing behaviour.  For example:
  1) run a saved search
  2) click on "Change Columns" link and add column X to the display
  3) click on the Change Columns button
  4) re-save the search under the same name, with "remember columns" checked
  5) run the query again
  6) click on the "Change Columns" link

Two problems:
1) column X is not checked off
2) changing your column selection has no effect

Comment 12

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

Comment on attachment 200940 [details] [diff] [review]
Unbitrotted

This change doesn't address the issue this bug is about, but it's still a valuable improvement to the current behavior and should make it into Bugzilla.  I see just two issues with it:

1. as Rob notes, it doesn't update saved searches when users resave them;
2. it unnecessarily prompts users to save column lists.

About the latter: when a user saves a search, she has either modified the set of columns to display, or she hasn't.  If she hasn't modified the set, chances are she wants the search to display the default set of columns, either the site default (if she has never specified her own default) or her own default (if she has).

If, on the other hand, the user has modified the set of columns to display, chances are she wants the search to display the set of columns she specified.  In 99% percent of all cases, then we'd do the right thing by automatically storing the column list when the user explicitly modified it and not storing it otherwise.

Since the 1% case is not hard to deal with (it just requires a user to modify the column list for a search), we should optimize for the 99% case and not prompt the user whether or not to store the list.

Comment 13

[Max Kanat-Alexander]

The Saved Search part of this is handled in bug 345826, and I think it might make sense to fix that first.

Comment 14

[Frédéric Buclin]

greg, any progress?

Comment 15

[Greg Hendricks]

I have it working (mostly) at bnc. There are a few quirks to work out still though. I will see what I can do.

Comment 18

[Michael Thomas (Mockodin)]

Attachment: Patch v3

quick patch that should satisfy the original issue with minimal impact otherwise

Create New Table:
   buglist_settings
     COLUMNS
         user_id
         setting_type
         value

setting types for now are only two: COLUMNLIST and SPLITHEADER

Anonymous users will use cookies with no change to existing process

Logged in Users use the values stored in bug_settings EXCEPT if columnlist is part of the $cgi->param().

Updating of column lists is done with no change to process.

If no record is found in the database, then DEFAULT_COLUMN_LIST IS used for logged in users.

Comment 19

[Frédéric Buclin]

Comment on attachment 390038 [details] [diff] [review]
Patch v3

Max, do we want a whole new table for this?

Comment 20

[Max Kanat-Alexander]

Comment on attachment 390038 [details] [diff] [review]
Patch v3

Yeah, no, we don't need a whole new table for this--just add a column to the profiles table.

And in general this seems like a lot of added complexity. It looks like some refactoring needs to be done on Bugzilla before this happens (as to what refactoring, I'll leave that up to the implementer to figure otu).

Comment 21

[Frédéric Buclin]

(In reply to comment #20)
> Yeah, no, we don't need a whole new table for this--just add a column to the
> profiles table.

Now we should think about this a bit further:

Are there other per-user settings which wouldn't be treated as user prefs and which could still be added to the DB in the future? For instance, moving buglists from cookies to the DB, etc... If there are only one or two such settings, then we could consider adding them to the profiles table. But if there are ten or so such settings, what would be the best? Still add them to the profiles table or have a separate one? Personally, I don't really know (what would be best from a performance point of view, knowing that the profiles table is used to build user objects and so is called pretty often, while these per-user settings are not needed that much?).

With this in mind, I think we can take the best decision instead of having to alter the DB schema again and again later.

Comment 22

[Michael Thomas (Mockodin)]

(In reply to comment #21)

Performance wise so long as SELECT * is not used, the fields being in the table should be fine. Part of the reason I used the separate table upfront was the idea that while not now, maybe in the future more settings would be included. Things like a buglist are a bit tricky I think, in that a user's buglist would update frequently. The feel of the data is different in relation to profiles data or even the columns information we are addressing here as those generally are set and infrequently change and then only by direct request of the user or admin. If we were to move the buglist, or similar, to a db I would lean toward making these a new table and leave profiles to be as static data wise as possible. Granted buglist was just an example and there are potentially other settings that could move. Such as the DEFAULTFORMAT setting which while not wholly buglist related is related through the search module.

(In reply to comment #20)
I think also something to consider, existing patch included, my goal in the change was to disrupt the existing code as little as possible and insert the enhancement for the specific use case of a logged in user, everyone else would proceed with no changes to there interaction. The patch submitted is actually fairly simple, the appearance of complexity is the expansion of the existing IF ELSE blocks as well as a few new ones. In each case this was to isolate the user to the appropriate process, cookies or db but not both and allow for passed in column lists for on-the-fly one time views or bookmarked views using &columnlist=. This could of course be refactored to condense the code into two main blocks, logged in actions and not logged in actions. Intent had to been to code inline, add to the code, without completely reformatting/rewriting it.

The question of new table or existing table remains.

Comment 23

[Max Kanat-Alexander]

We're going to stick with the profiles table. One shouldn't anticipate problems before one proves they exist.

When doing upstream development on HEAD, changing as little code as possible is not a goal. The goal is to change as much code as is needed in order for the final result to be simple. So, for example, here you may want to find more places that Bugzilla::Search::Saved could be used or somehow simplified (in another bug), and also add something to Bugzilla::User to handle this update to the profiles table column (in this bug). (In modern times, we should never be adding raw SQL to .cgi files.)

Comment 24

[Michael Thomas (Mockodin)]

(In reply to comment #23)

Profiles no problem.
Update through Bugzilla::User, yep makes sense given the change in tables.
No direct SQL in CGI, agreed, I was being lazy ;-) and at the time didn't spend time looking for an alternative location (I guess still qualifies under reason #1)

should have the needed changes later today/tonight

Comment 25

[Michael Thomas (Mockodin)]

Attachment: v4

updated to use profiles table to update using Bugzilla::Object

Added update code to Bugzilla::Install::DB

Comment 26

[Frédéric Buclin]

Comment on attachment 409032 [details] [diff] [review]
v4

>Index: bugzilla/Bugzilla/Install/DB.pm

>+    $dbh->bz_add_column('profiles', 'columnlist', {TYPE => 'LONGTEXT'});
>+    $dbh->do("UPDATE profiles SET columnlist = '' WHERE columnlist is null");
>+    $dbh->bz_alter_column('profiles', 'columnlist', {TYPE => 'LONGTEXT', NOTNULL => 1});

Why cannot you create the column as NOTNULL in one shoot? Also, why not allowing NULL values?

Comment 27

[Michael Thomas (Mockodin)]

We could, I threw the update code togeather rather quickly last night to get the patch up, either way should work.

+sub _remember_buglist_columnlist_and_splitheader {
+    my $dbh = Bugzilla->dbh;
+    $dbh->bz_add_column('profiles', 'columnlist',\
+                        {TYPE => 'LONGTEXT', NOTNULL => 1}, '');
+    $dbh->bz_add_column('profiles', 'splitheader', { TYPE => 'BOOLEAN',
+		         NOTNULL => 1, DEFAULT => 'TRUE'});
+}
+

NULL vs NOTNULL it seemed that we were shooting for NOTNULL 90% of the time so for consistency sake...

Comment 28

[Michael Thomas (Mockodin)]

Attachment: v5

Comment 29

[David E. Ross]

This appears fixed in the Bugzilla version now in use at bugzilla.mozilla.org.

Comment 30

[Frédéric Buclin]

(In reply to comment #29)
> This appears fixed in the Bugzilla version now in use at bugzilla.mozilla.org.

No, this isn't implemented yet.

Comment 31

[David E. Ross]

Aha!  It appears implemented for specific saved searches but not in general as a default for the user's account.  

Bugs #533050 and #533067 resulted when I tried to save a selected set of columns for a tag-based search.  Lack of this capability also resulted in bug #503398.  This, this is a very much needed capability.  

It seems to me that merely adding a data element (e.g., a file) to the user's account that contains the data currently in the COLUMNLIST cookie would implement this capability.  Of course, changes to the selected set of columns would then require a confirmation whether to save those changes or merely use them in the current bug list.

Comment 32

[David E. Ross]

Oops!

"This, this is a very much needed capability." should be "Thus, this is a very much needed capability."

The cited bug reports are: 
Bug #533050
Bug #533067
Bug #503398

Comment 33

[Michael Thomas (Mockodin)]

re comment 29 comment 31 and comment 32

Given Max has not yet to reviewed my patch I'm fairly sure this is not yet on BMO.
It certainly is not in the CVS tip yet.

Also it should have no effect on tagged searches. As I recall once this patch is applied the result is that logged in users will no longer look at the cookie for the column list and rather at the profiles table. Beyond that if columns are included in a bookmark or saved search they should properly display.

Comment 34

[Max Kanat-Alexander]

Comment on attachment 409139 [details] [diff] [review]
v5

>Index: bugzilla/Bugzilla/Install/DB.pm
>+sub _remember_buglist_columnlist_and_splitheader {
>+    my $dbh = Bugzilla->dbh;
>+    $dbh->bz_add_column('profiles', 'columnlist',\
>+                        {TYPE => 'LONGTEXT', NOTNULL => 1}, '');

  You don't need the \ at the end of the line.

  This doesn't need to be a LONGTEXT, it can be a MEDIUMTEXT. I doubt anybody's going to have a columnlist longer than 4000 characters.

  For new columns, they should have their names separated with underscores. We should probably call this one buglist_columns.

>+    $dbh->bz_add_column('profiles', 'splitheader', { TYPE => 'BOOLEAN',
>+                 NOTNULL => 1, DEFAULT => 'TRUE'});

  Nit: The formatting of this isn't consistent with other bz_add_column calls in this file.

  I'm not sure that "splitheader" is the best name. Perhaps buglist_stagger_headers?

>Index: bugzilla/buglist.cgi
>+} elsif (Bugzilla->user->id) {

  Hmm, is there really not a $user variable at this point? There should be. Better than calling Bugzilla->user over and over.

>+    my $columnlist = Bugzilla->user->columnlist;
>+    @displaycolumns = $columnlist ? split(/ /, $columnlist) : DEFAULT_COLUMN_LIST;

  I think you want ' ' instead of / /.

  Also, how about you modify $user->buglist_columns to return the cookie information if there's no database info or if we're logged out, and DEFAULT_COLUMN_LIST if neither of those exist?

>-$vars->{'splitheader'} = $cgi->cookie('SPLITHEADER') ? 1 : 0;
>+if (Bugzilla->user->id){
>+    $vars->{'splitheader'} = Bugzilla->user->splitheader; 
>+} else {
>+    $vars->{'splitheader'} = $cgi->cookie('SPLITHEADER') ? 1 : 0;
>+}

  Same here--$user->buglist_stagger_headers should return the cookie if there's no DB value or if we're logged out.

>Index: bugzilla/colchange.cgi
>+            if (Bugzilla->user->id){
>+                Bugzilla->user->set_columnlist($list);
>+                Bugzilla->user->update();
>+	     } else {
>+                $cgi->send_cookie(-name => 'COLUMNLIST',
>+                                  -value => $list,
>+                                  -expires => 'Fri, 01-Jan-2038 00:00:00 GMT');
>+	     }

  And here, set_buglist_columns should send the cookie if we're logged out, instead of updating the database.

  (Same note for any other places that we update, delete, or create the columnlist or stagger_headers.)

>Index: bugzilla/Bugzilla/User.pm
>+    'columnlist'     => '',
>+    'splitheader'    => 0,   

  And then this won't be necessary, because we'll have the logic inside the function instead.

>@@ -217,6 +223,8 @@
>+sub set_columnlist   { $_[0]->set('columnlist', $_[1]); }
>+sub set_splitheader  { $_[0]->set('splitheader', $_[1]); }

  You're missing validators for these.

Comment 35

[Michael Thomas (Mockodin)]

(In reply to comment #34)
>  I'm not sure that "splitheader" is the best name. Perhaps buglist_stagger_headers?
It is the name of the cookie as is I have no problem changing the cookie, but then you'll kill everyone's existing cookies..

>  I think you want ' ' instead of / /.
Seems to work either way, but yeah.

>Same here--$user->buglist_stagger_headers should return the cookie if there's

>Hmm, is there really not a $user variable at this point? There should be.
>Better than calling Bugzilla->user over and over.
Correct, there is not all potential previous calls were contained in function calls. Otherwise all the calls are Bugzilla->user->blah..

For the comments on logged in vs out, aren't I already setting a cookie if we are logged out? If we are logged out Bugzilla->user->id should be undef or at least evaluate false? So we then set a Cookie.

>You're missing validators for these.
What would I be validating? That I can think of there is no point in which unmanaged values could be set here. I'm just not sure what I would need to test.

Comment 36

[Max Kanat-Alexander]

(In reply to comment #35)
> (In reply to comment #34)
> >  I'm not sure that "splitheader" is the best name. Perhaps buglist_stagger_headers?
> It is the name of the cookie as is I have no problem changing the cookie, but
> then you'll kill everyone's existing cookies..

  Don't change the name of the cookie. Just everything else.

> For the comments on logged in vs out, aren't I already setting a cookie if we
> are logged out? If we are logged out Bugzilla->user->id should be undef or at
> least evaluate false? So we then set a Cookie.

  Right.

> What would I be validating? That I can think of there is no point in which
> unmanaged values could be set here. I'm just not sure what I would need to
> test.

  You need to test that they're strings, at least. stagger_headers needs to be tested to be a boolean.

Comment 38

[jessn]

(In reply to Michael Thomas (Mockodin) from comment #18)

I have been using this fix and it works out of the box.
I'll recommend this to be pushed into production.

Michael Thomas, thanks for the fix.

Comment 39

[jessn]

(In reply to jessn from comment #38)

Btw; I have been using the fix for version 4.4.6

Comment 41

[Mukhtar Ali]

HI 
can u assign this to me . I can spare sm time to work on this. Let me know if u have some direction/info on this.

Comment 42

[Simon Green]

(In reply to Mukhtar Ali from comment #41)
> can u assign this to me . I can spare sm time to work on this. Let me know
> if u have some direction/info on this.

Done

Comment 43

[Mukhtar Ali]

Attachment: patch1.diff

Made changes as per earlier comments

Comment 44

[Mukhtar Ali]

I tested this out on Windows with Chrome and Mozilla. OK.

Comment 45

[Frédéric Buclin]

Comment on attachment 8696739 [details] [diff] [review]
patch1.diff

>+++ b/Bugzilla/DB/Schema.pm

>+            buglist_columns         => {TYPE => 'LONGTEXT', NOTNULL => 1},

LONGTEXT is way too big (16 MB on MySQL). It should be MEDIUMTEXT at most.

Comment 46

[Mukhtar Ali]

Attachment: patch2.diff

v2 updated as per review.

Comment 47

[Simon Green]

Comment on attachment 8696826 [details] [diff] [review]
patch2.diff

Please assign review requests to a specific reviewer.

Comment 48

[Frédéric Buclin]

Comment on attachment 8696826 [details] [diff] [review]
patch2.diff

>diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm

>+            buglist_stagger_headers => {TYPE => 'BOOLEAN', NOTNULL => 1,
>+                                        DEFAULT => 'FALSE'},            

I think this column should be named "buglist_headers". Also, please remove trailing whitespaces.



>+++ b/Bugzilla/Install/DB.pm

>+sub _remember_buglist_buglistcolumns_and_splitheader {

Rename this super-long name to _add_profiles_buglist_columns. Also, you forgot to call this method.


I didn't read the remaining code carefully, but:

>+++ b/colchange.cgi

>+    trick_taint($list);
>+    if ($list) 
>+    {

You must shift both lines, because trick_taint() will croak if $list is undefined. Also, for short if tests, simply write:

  if ($list) {


>+	    if ($user->id){

Nit: missing whitespace before {.


>+    @collist = split(/[ ,]+/, $db_collist) if $db_collist && $db_collist ne '';
>+    @collist = DEFAULT_COLUMN_LIST unless  $db_collist && $db_collist ne '';

Better to write:

  @collist = $db_collist ? split(/[ ,]+/, $db_collist) : DEFAULT_COLUMN_LIST;


>+elsif (defined $cgi->cookie('COLUMNLIST')) {

Remove 'defined'. If the list is empty, we also want the default list.


>+    my $db_splitheader = $user->buglist_stagger_headers();
>+    $vars->{'splitheader'} = $db_splitheader;

You can merge both lines: $vars->{'splitheader'} = $user->buglist_stagger_headers();

Comment 49

[Mukhtar Ali]

Attachment: patch3.diff

fixed all problems patch should work. can you check thanks.

Comment 50

[Frédéric Buclin]

Comment on attachment 8697296 [details] [diff] [review]
patch3.diff

Please address all review comments correctly.


>+++ b/Bugzilla/DB/Schema.pm

>+            buglist_headers => {TYPE => 'BOOLEAN', NOTNULL => 1,
>+                                DEFAULT => 'FALSE'},            


>+++ b/Bugzilla/Install/DB.pm

>+_add_profiles_buglist_columns {

You still have no call to this method. So this code will never be executed.


>+    $dbh->bz_add_column('profiles', 'buglist_stagger_headers', 
>+                        {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'TRUE'});

This column name no longer matches the one set above.



>+++ b/buglist.cgi

>+if ($user->id){

Nit: missing whitespace.



>+++ b/colchange.cgi

>+    trick_taint    ($list);
>+    if     ($list) 
>+    {

What are all these whitespaces?? See how the code is written.

The remaining code is full of tabs, which are not allowed in the Bugzilla code. Do not forget to run ./runtests.pl to make sure your code pass tests.

Comment 51

[Mukhtar Ali]

Attachment: patch 3.diff

Comment 52

[Firefox Bug Husbandry Bot]

Removing good-first-bug keyword because team does not have bandwidth to mentor at the moment.