286357 - user preference settings only works if all users sets non-default values for all settings

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Albert Ting]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041223 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041223 Firefox/1.0

Best I can tell, profile_settings is setup to only contain non-default setting
values, such that the database size doesn't get too big.  

However, this doesn't work for me, or at least with mysql-4.1.3.  If a user A
sets a non-default value for a setting, all other users who opted for the
default value will automatically get the value selected by user A.

For a test case, I did below based on User/Setting.pm.

mysql> select * from setting;
+--------------------+------------------+------------+
| name               | default_value    | is_enabled |
+--------------------+------------------+------------+
| display_quips      | on               |          1 |
| comment_sort_order | oldest_to_newest |          1 |
+--------------------+------------------+------------+
2 rows in set (0.00 sec)

mysql> select * from profile_setting;
+---------+--------------------+------------------+
| user_id | setting_name       | setting_value    |
+---------+--------------------+------------------+
|       1 | display_quips      | off              |
|       2 | comment_sort_order | newest_to_oldest |
+---------+--------------------+------------------+
2 rows in set (0.00 sec)

The command below should return 2 rows, but only returns the row specifically
tied to userid=1.

mysql> select user_id, name, default_value, is_enabled, setting_value from
setting left join profile_setting on setting.name = profile_setting.setting_name
where profile_setting.user_id=1 or profile_setting.user_id=NULL;
+---------+---------------+---------------+------------+---------------+
| user_id | name          | default_value | is_enabled | setting_value |
+---------+---------------+---------------+------------+---------------+
|       1 | display_quips | on            |          1 | off           |
+---------+---------------+---------------+------------+---------------+
1 row in set (0.01 sec)


Reproducible: Always

Steps to Reproduce:

Comment 1

[Albert Ting]

Attachment: suggested patch

Here's an initial patch that solves my specific problem.  But I believe the
new() function also needs to be fixed. 

In any case, I think this is a blocker for the 2.20 release.

Comment 2

[Shane H. W. Travis]

Frederic helped me with the code on this, so I'm blaming him (and cc:ing him).

:)

Comment 3

[Max Kanat-Alexander]

Right, nothing ever = NULL.

Comment 4

[Albert Ting]

Attachment: version 2

Here's a cleaner implementation.  The new() function still needs to be updated.
 I suggest removing the large "if" code that auto-fetches the undefined
function parameters.  I don't think that code is being used anywhere.

Comment 5

[Shane H. W. Travis]

(In reply to comment #4)
>  I suggest removing the large "if" code that auto-fetches the undefined
> function parameters.  I don't think that code is being used anywhere.  

It is not, yet I'm not removing it. See discussion of this same point on bug 
98123 for why.

Review within the next day or two.

Comment 6

[Frédéric Buclin]

(In reply to comment #2)
> Frederic helped me with the code on this, so I'm blaming him (and cc:ing him).
> 

(00:35:53) LpSolit: travis: I did what??? :)
(00:36:04) travis: I've got the logs to prove it, if you make me.
(00:36:07) travis: IRC logs, that is.
(00:36:14) LpSolit: yeah... show me :)
(00:38:10) travis: okay, I take it back
(00:38:24) travis: I found the conversation I was remembering, and it was about
a different bug. Sorry Fred!
(00:39:42) LpSolit: I want a comment in the bug to mention that I'm innocent :-p
(00:39:47) travis: lol

Comment 7

[Shane H. W. Travis]

+    $user_id=0 if (! defined $user_id);
+

Please change this to:
  $user_id = $user_id || 0;

Jouni taught me that one; it's much more elegant and succinct, and is what 
people are moving too all over Bugzilla.

This version does look much better/cleaner than the first. I will try it out in 
the morning and confirm that it works; if so, and if you can change that above, 
I'll give it an r+.

Comment 8

[Shane H. W. Travis]

Albert, I understand from our e-mail correspondence that you're fixing this for 
new() also... are you doing that on this bug? If so, I won't review until your 
next patch is up; just let me know.

Comment 9

[Albert Ting]

Attachment: version 3

Here you go!  I've updated the new() function per our discussions we've had
offline.

Comment 10

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

>Please change this to:
>  $user_id = $user_id || 0;

Even better: $user_id ||= 0;

Comment 11

[Albert Ting]

(In reply to comment #10)
> >Please change this to:
> >  $user_id = $user_id || 0;
> 
> Even better: $user_id ||= 0;

Is this the only thing preventing the patch from getting approved?  Any other
issues? 

Comment 12

[Albert Ting]

Attachment: v4

Minor update per comment #10

Comment 13

[Gary Penn]

I tried this latest patch and received this output:

[root@bugzilla bugzilla]# patch -b -p0 <bug-286357-user-preference
patching file Bugzilla/User/Setting.pm
Hunk #2 FAILED at 137.
Hunk #4 FAILED at 170.
2 out of 5 hunks FAILED -- saving rejects to file Bugzilla/User/Setting.pm.rej
[root@bugzilla bugzilla]#

Comment 14

[Max Kanat-Alexander]

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

OK, this looks good, but you also have to update the POD docs for get_defaults.

Please do post another patch with the updates, don't just fix it on checkin.
But you can carry forward r+.

Comment 15

[Max Kanat-Alexander]

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

Oh, also, this patch is bitrotted.

Comment 16

[Albert Ting]

Attachment: v5

Cleaned up, per comment #14 and comment #15.  And I hate it when a patch gets
bitrotted in just 1 day!  :)

Comment 17

[Frédéric Buclin]

Checking in Bugzilla/User/Setting.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User/Setting.pm,v  <--  Setting.pm
new revision: 1.3; previous revision: 1.2
done