328904 - User account log should display disabled account events

Status: ASSIGNED

(Bugzilla :: Administration, task)

Description

[Albert Ting]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6

The account log should have an entry whenever an account got disabled/reenabled.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Albert Ting]

Attachment: v1

Suggested patch

Comment 2

[Frédéric Buclin]

Comment on attachment 274577 [details] [diff] [review]
v1

>+        $dbh->do(qq{INSERT INTO profiles_activity (

profiles.disabledtext      => {TYPE => 'MEDIUMTEXT', ...}

profiles_activity.oldvalue => {TYPE => 'TINYTEXT'}
profiles_activity.oldvalue => {TYPE => 'TINYTEXT'}

It's not hard to see that you cannot insert the text in the user account log.

Comment 3

[Frédéric Buclin]

(In reply to comment #2)
I meant:

 profiles_activity.oldvalue => {TYPE => 'TINYTEXT'}
 profiles_activity.newvalue => {TYPE => 'TINYTEXT'}

Comment 4

[Max Kanat-Alexander]

Comment on attachment 274577 [details] [diff] [review]
v1

Also, you need to put this inside of Bugzilla::User->update, not in editusers.cgi.

Comment 5

[Albert Ting]

Attachment: v2

updated, per comment #3/#4.  Also per some external discussions, I have hardcoded the string to just say "disable".

Not sure if I need to lock the tables though, as the existing code in User::update() doesn't do a lock.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 274841 [details] [diff] [review]
v2

>--- Bugzilla/User.pm~	2007-08-01 14:59:25.742575000 -0700
>+++ Bugzilla/User.pm	2007-08-01 15:16:08.162542000 -0700
>@@ -178,6 +178,25 @@
>         }
>     }
> 
>+    # update disabled text changes
>+    if ($changes->{"disabledtext"}) {
>+        my ($oldvalue,$newvalue) = @{$changes->{"disabledtext"}};
>+        trick_taint($oldvalue);
>+        trick_taint($newvalue);

  Neither of these should be tainted. If they are, then something is wrong.

>+        $dbh->do(qq{INSERT INTO profiles_activity (
>+                           userid, who,
>+                           profiles_when, fieldid,
>+                           oldvalue, newvalue
>+                          ) VALUES (
>+                           ?, ?, now(), ?, ?, ?
>+                          )

  Nit: This formatting has way too many linebreaks. If it were me, I'd write it more like:

  $dbh->do(
      'INSERT INTO profiles_activity(userid, who, profiles_when,
                                     fieldid, oldvalue, newvalue)
            VALUES (?,?,NOW(),?,?,?)');

  The NOW() *must* be capitalized, though. That's not a nit.

>+                   },
>+                 undef,
>+                 ($self->id, Bugzilla::user()->id, get_field_id('disabledtext'),

  That must be Bugzilla->user, not Bugzilla::user. 

>--- Bugzilla/Field.pm~	2007-07-30 19:18:03.630898000 -0700
>+    {name => 'disabledtext',          desc => 'Disabled text'},

  I think we need a separate table for the ids of profile fields, because adding entries to fielddefs will mess up other parts of Bugzilla.

Comment 7

[Max Kanat-Alexander]

As far as locking goes, yes, technically this should happen in a lock. However, I think it already happens in an external lock in editusers.cgi, and we can't re-lock tables.

What really needs to happen is this code should all be using transactions, including editusers.cgi.

Comment 8

[Albert Ting]

(In reply to comment #6)

> >+    # update disabled text changes
> >+    if ($changes->{"disabledtext"}) {
> >+        my ($oldvalue,$newvalue) = @{$changes->{"disabledtext"}};
> >+        trick_taint($oldvalue);
> >+        trick_taint($newvalue);
> 
>   Neither of these should be tainted. If they are, then something is wrong.

Yes, there were tainted.  But given that I'm now hardcoding the string, I don't need these. 

> >+        $dbh->do(qq{INSERT INTO profiles_activity (
> >+                           userid, who,
> >+                           profiles_when, fieldid,
> >+                           oldvalue, newvalue
> >+                          ) VALUES (
> >+                           ?, ?, now(), ?, ?, ?
> >+                          )
> 
>   Nit: This formatting has way too many linebreaks. 

I was following the example in editusers.cgi, for groupsAddedTo.  (The coding standards sometimes feels like a moving target)

> >--- Bugzilla/Field.pm~	2007-07-30 19:18:03.630898000 -0700
> >+    {name => 'disabledtext',          desc => 'Disabled text'},
> 
>   I think we need a separate table for the ids of profile fields, because
> adding entries to fielddefs will mess up other parts of Bugzilla.

I didn't consider this, how does it mess up other parts of Bugzilla?  

Comment 9

[Max Kanat-Alexander]

(In reply to comment #8)
> Yes, there were tainted.  But given that I'm now hardcoding the string, I don't
> need these. 

  Hrm. I think that indicates some deeper problem, but I'm not sure.

> I was following the example in editusers.cgi, for groupsAddedTo.  (The coding
> standards sometimes feels like a moving target)

  Sorry. The correct standards are in the Developer's Guide. editusers.cgi is, unfortunately, a poor example of most of these standards.

  http://www.bugzilla.org/docs/developer.html#sql-style

> I didn't consider this, how does it mess up other parts of Bugzilla?  

  Well, for example, we rely on the fielddefs table for the list of Boolean Charts fields. Also, search for anywhere that we access the fielddefs table, or use get_fields without "custom => 1" and that's a potential problem. fielddefs is designed to be a description of bug fields.

Comment 10

[Albert Ting]

(In reply to comment #9)
> > I didn't consider this, how does it mess up other parts of Bugzilla?  
> 
>   Well, for example, we rely on the fielddefs table for the list of Boolean
> Charts fields.

Yup, the advanced search fields sees this.  Thanks for point that out.

Well, do we want a profilefielddefs table?  Would we then create a ProfileField.pm?  Seems a bit of an overhead for just this? 

Comment 11

[Max Kanat-Alexander]

profile_field should be the name of the table. It can just contain id, name, and description.

If you want to make an object, you could perhaps subclass Bugzilla::Field and make Bugzilla::Field::Profile, but an object isn't necessary at this stage if you don't want to make one.

Comment 12

[Max Kanat-Alexander]

Or really, come to think of it, with Bugzilla::Object, it's not even that hard to make a new object if you want--you wouldn't even have to subclass Bugzilla::Field.

Comment 13

[Albert Ting]

If we create Bugzilla::ProfileField and profilefielddefs, we'd also have to update profiles_activity.fieldid to only reference profilefielddefs.

The more I think about it, this probably makes sense?

Comment 14

[Max Kanat-Alexander]

(In reply to comment #13)
> The more I think about it, this probably makes sense?

  Yep, definitely makes sense. We were going to have to do it eventually one way or the other, anyway.

  Alternately, you could just change profile_activity to store the field name, which I think wouldn't be so bad.

Comment 15

[Albert Ting]

Attachment: v3

Updated version, based on current feedback from Max/Frederic.  Key changes:

  - don't use profiles_activity.fieldid, just use profiles_activity.field
  - no need to create a ProfileFields Object at this time

Things I'm not clear how it should be done:
  - Bugzilla doesn't have a subroutine to drop foreign keys.  For now, I did it explicitly.  Not sure what a bz_drop_fk should require.
  - I referenced fielddefs.description whenever I knew they were available.  Or should I just hardcode the field value?
  - Unclear if I updated contrib/recode.pl correctly.

Comment 16

[Max Kanat-Alexander]

Comment on attachment 275705 [details] [diff] [review]
v3

Explicitly dropping the FK shouldn't be necessary. bz_drop_column should be doing that for you, and if it isn't, that's a bug.

Do we actually need an index on 'field'?

Use bz_start_transaction, don't use bz_lock_tables in new code.

> +                  new Bugzilla::Field({'name' => "bug_group"})->description

That needs to be some constant, not something based on the fielddefs table.

The changes to recode.pl will break upgrades, I'm pretty sure. :-( We have to figure out what to do about that.

The way you're using the string "Disabled Text" isn't localizable.

Comment 17

[Frédéric Buclin]

Comment on attachment 275705 [details] [diff] [review]
v3

>+++ Bugzilla/User.pm	2007-08-07 17:11:36.439449000 -0700

>+                 ($self->id, Bugzilla->user()->id, "Disabled Text",
>+                  $oldvalue eq "" ? "" : "disabled",
>+                  $newvalue eq "" ? "" : "disabled"));

Why not using a simple 0/1 as we do everywhere else?

 | Disabled Text | 0 | 1 |

seems clear enough to me. And you avoid l10n problems.

This patch seems very complex compared to what we try to achieve in this bug. :( Maybe should we have another table like fielddefs, but for administrative stuff? Remember we will very likely record more and more admin changes, not only about user accounts.

Comment 18

[Albert Ting]

(In reply to comment #17)
> Why not using a simple 0/1 as we do everywhere else?
> 
>  | Disabled Text | 0 | 1 |

That's because you suggested I use "disable" in our email exchanges.  Plus I think we just want to update the "old" or "new' column, not both?

   |> Would it be sufficient to just say "account disabled" and "account 
   |> enabled" for the profiles_activity values?
   |
   | I think so, yes. After all, that's what we are interested in. The text
   | itself is not important.  

> This patch seems very complex compared to what we try to achieve in this bug.
> :( Maybe should we have another table like fielddefs, but for administrative
> stuff? Remember we will very likely record more and more admin changes, not
> only about user accounts.

Max approved that we just hold the field name and not add another fielddefs table. 

> The way you're using the string "Disabled Text" isn't localizable.

How does one make it localizable (and make it a constant)?  Do I just add it to Bugzilla::Constants?

> Explicitly dropping the FK shouldn't be necessary. bz_drop_column should be
> doing that for you, and if it isn't, that's a bug.

I think there's a bug.  I got a warning on this line:
     Bugzilla/DB.pm:667:
         eval { $self->do($sql); } or warn "Failed SQL: [$sql] Error: $@";

I'll have to recreate my test case to show the error message.  

Comment 19

[Max Kanat-Alexander]

(In reply to comment #18)
> How does one make it localizable (and make it a constant)?  Do I just add it to
> Bugzilla::Constants?

  Just insert the actual *field name* into the database, and then you can translate it on display. That is, insert "disabledtext" into the DB. The description should be shown *on display*, it shouldn't be stored in the DB.

> I think there's a bug.  I got a warning on this line:
>      Bugzilla/DB.pm:667:
>          eval { $self->do($sql); } or warn "Failed SQL: [$sql] Error: $@";
> 
> I'll have to recreate my test case to show the error message.  

  Okay. If you find something, file a separate bug in the Database category.

Comment 20

[Albert Ting]

> Just insert the actual *field name* into the database, and then you can
> translate it on display. That is, insert "disabledtext" into the DB. 

Got it.  But is it sufficient to do the conversion in the template files, inline?  

I'll also update the table structure to use "1", per Frederic.

Are we ok with the database structure?

Comment 21

[Max Kanat-Alexander]

(In reply to comment #20)
> > Just insert the actual *field name* into the database, and then you can
> > translate it on display. That is, insert "disabledtext" into the DB. 
> 
> Got it.  But is it sufficient to do the conversion in the template files,
> inline?  

  Yes. In fact, it's not only sufficient, but it's required for localization.

  I'd make a new hash in field-descs called profile_field.

> Are we ok with the database structure?

  I need some evidence that you actually need an index on the "field" column. You only need indexes on things that are using in JOINs or WHERE clauses, and I can't imagine doing that on that column.

Comment 22

[Albert Ting]

> I need some evidence that you actually need an index on the "field" column.

I don't need a field index, per se.  I just did it for consistency.

Comment 23

[Max Kanat-Alexander]

(In reply to comment #22)
> I don't need a field index, per se.  I just did it for consistency.

  Okay. Then it shouldn't be there. We should only have indexes that we actually need.

Comment 24

[Albert Ting]

(In reply to comment #21)
> I'd make a new hash in field-descs called profile_field. 

Hi Max.

   In field-desc, I have this:

[% profiles_activity_descs = { "bug_group"      => "Group",
                               "creation_ts"    => "Creation date",
                               "disabledtext"   => "Disabled" } %]

[% MACRO get_profiles_activity(res) GET profiles_activity_descs.$res || res %]

How do I use this in profile-activity.html.tmpl?  The admin/table.html.tmpl supports an "override" feature, but I don't see that can be used to do a lookup table.  Seems like I would have to update table.html.tmpl and add a "content_use_profile_field" flag?

Comment 25

[Albert Ting]

> Seems like I would have to update table.html.tmpl and add a 
> "content_use_profile_field" flag?

Hi Max, would this be sufficient?

Comment 26

[Max Kanat-Alexander]

(In reply to comment #25)
> > Seems like I would have to update table.html.tmpl and add a 
> > "content_use_profile_field" flag?
> 
> Hi Max, would this be sufficient?

  No, it's possible to use overrides to handle this situation. See admin/components/list.html.tmpl as an example.

Comment 27

[Albert Ting]

Attachment: v4

Patch updated per comments from Frederic and Max.

Comment 28

[Max Kanat-Alexander]

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

I am nearly certain that your recode.pl changes will break things for new users.

Also, it should be fairly easy to combine those two loops in User::update() into one loop, and just translate the disabledtext values if they exist.

Oh, and don't use "res" as the variable name for get_profiles_activity--that means "resolution".

Also, just call the hash profile_field--seems easier to type that, and then get_profile_field can be the macro.

Otherwise this looks OK, though I didn't test it.

Comment 29

[Albert Ting]

(In reply to comment #28)
> (From update of attachment 277480 [details] [diff] [review])
> I am nearly certain that your recode.pl changes will break things for new
> users.

Agreed.  Any suggestions what it should do?

Comment 30

[Max Kanat-Alexander]

(In reply to comment #29)
> Agreed.  Any suggestions what it should do?

  I think it will have to special-case that table. If the fieldid column exists, it can use fieldid as the last item in the PK, otherwise it can use field.

Comment 31

[Albert Ting]

(In reply to comment #30)
> > Agreed.  Any suggestions what it should do [with recode.pl]?
> 
>   I think it will have to special-case that table. If the fieldid column
> exists, it can use fieldid as the last item in the PK, otherwise it can use
> field.
> 

Max, based on the recode.pl code, I suggest we not make SPECIAL_KEYS a constant, and then update profiles_activity to not include 'fieldid' if such a column doesn't exist.  Sounds good? 

Comment 32

[Max Kanat-Alexander]

(In reply to comment #31)
> Max, based on the recode.pl code, I suggest we not make SPECIAL_KEYS a
> constant, and then update profiles_activity to not include 'fieldid' if such a
> column doesn't exist.  Sounds good? 

  You could make it a sub instead, if you want. Constants are just really subs anyway.

Comment 33

[Albert Ting]

Attachment: v5

Updated patch.  Cleaned up recode.pl, created a LogProfileActivityEntry, similar to LogActivityEntry, and cleaned up the field-descs.  Now logs all changes.

Comment 34

[Max Kanat-Alexander]

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

>+sub LogProfileActivityEntry {

  Let's just call this log_change.

>+    my ($i, $field, $oldvalue, $newvalue, $whoid, $timestamp) = @_;

  Use named parameters instead of positional parameters, here. I can never remember what all of these parameters are.

  Also, you could just make this a method and then you wouldn't have to have $i.

>+[% profile_field = { "bug_group"      => "Group",
>+                     "creation_ts"    => "Creation date",
>+                     "cryptpassword"  => "Password",
>+                     "realname"       => "Real Name",
>+                     "login_name"     => "Login Name",
>+                     "disable_mail"   => "Disable Mail",

  Seems like that one should be "Mail Disabled"

>--- contrib/recode.pl.orig	2007-09-20 12:13:01.640414000 -0700
>+sub init_special_keys {
>+    # For certain tables, we can't automatically determine their Primary Key.
>+    # So, we specify it here as a string.
>+    $special_keys = {

  Um, just have this return a hashref and then you can assign it to $special_keys.

Comment 35

[Albert Ting]

Attachment: v6

(In reply to comment #34)
> (From update of attachment 281707 [details] [diff] [review])
> >+sub LogProfileActivityEntry {
> 
>   Let's just call this log_change.

Done
 
> >+    my ($i, $field, $oldvalue, $newvalue, $whoid, $timestamp) = @_;
> 
>   Use named parameters instead of positional parameters, here. I can never
> remember what all of these parameters are.

Sorry, I had lazily copied what's already in LogActivityEntry().  But note that editusers.cgi also calls this function

> >+                     "disable_mail"   => "Disable Mail",
> 
>   Seems like that one should be "Mail Disabled"

Done

> >--- contrib/recode.pl.orig	2007-09-20 12:13:01.640414000 -0700
>   Um, just have this return a hashref and then you can assign it to
> $special_keys.

Done

Comment 36

[Max Kanat-Alexander]

Comment on attachment 281728 [details] [diff] [review]
v6

This looks to actually be v5.

Comment 37

[Albert Ting]

Attachment: v6.1

Hmm, must be some add caching going on.  Here's another attempt...

Comment 38

[Max Kanat-Alexander]

Comment on attachment 281742 [details] [diff] [review]
v6.1

It's v5. Look:

https://bugzilla.mozilla.org/attachment.cgi?oldid=281707&action=interdiff&newid=281742&headers=1

Comment 39

[Albert Ting]

Comment on attachment 281742 [details] [diff] [review]
v6.1

Hi Max,

   I don't know why interdiff is showing no differences, but v5 and v6.1 is indeed different (they have different file size).  Also search for "disable_mail" and you'll see the line is different.

Thanks,
Albert

Comment 40

[Frédéric Buclin]

(In reply to comment #39)
>    I don't know why interdiff is showing no differences

This sometimes happens when you updated your installation meanwhile, and at least one file has a newer revision number. interdiff is then lost and is unable to display the diff.

Comment 41

[Albert Ting]

Can this still be reviewed even with the interdiff issue?

Thanks,
Albert

Comment 42

[Max Kanat-Alexander]

Comment on attachment 281742 [details] [diff] [review]
v6.1

Sure, this looks fine. Have you tested it a fair amount?

I still would prefer if log_change had named parameters instead of positional parameters, but I can live with it.

Comment 43

[Max Kanat-Alexander]

Comment on attachment 281742 [details] [diff] [review]
v6.1

>+++ Bugzilla/Install/DB.pm	2007-08-07 16:28:27.873799000 -0700
>@@ -133,6 +133,7 @@
> 
>     _populate_longdescs();
>     _update_bugs_activity_field_to_fieldid();
>+    _update_profiles_activity_fieldid_to_field();

  Wait, why is this way up here and not at the end? New changes should be at the end, not in the middle.

>+        $dbh->bz_drop_index('profiles_activity',
>+                            'profiles_activity_fieldid_idx');
>+
>+        $dbh->bz_drop_column('profiles_activity', 'fieldid');

  
  These are both failing for me on landfill against a tip database.

Comment 44

[Max Kanat-Alexander]

Okay, they're failing because of some Foreign Key problem. I think it's possible that bz_drop_column and bz_drop_index don't understand FKs yet.

For reference, here's what I get:

Failed SQL: [DROP INDEX `profiles_activity_fieldid_idx` ON profiles_activity] Error: DBD::mysql::db do failed: Error on rename of './bugs_mkanat/#sql-af9_106e0' to './bugs_mkanat/profiles_activity' (errno: 150)

Failed SQL: [ALTER TABLE profiles_activity DROP COLUMN fieldid] Error: DBD::mysql::db do failed: Error on rename of './bugs_mkanat/#sql-af9_106e0' to './bugs_mkanat/profiles_activity' (errno: 150)

If this is really a Bugzilla::DB problem, we should fix it in a separate bug.

Comment 45

[Albert Ting]

(In reply to comment #42)
> (From update of attachment 281742 [details] [diff] [review])
> Sure, this looks fine. Have you tested it a fair amount?

Yup, I'm using this in production and successfully upgraded our 2.22 bugzilla database to 3.x.

> I still would prefer if log_change had named parameters instead of positional
> parameters, but I can live with it.

I believe it is named parameters.  Where do you see positional parameters?

> >     _update_bugs_activity_field_to_fieldid();
> >+    _update_profiles_activity_fieldid_to_field();
> 
>   Wait, why is this way up here and not at the end? New changes should be at
> the end, not in the middle.

Unfortunately I don't remember, I vaguely recall seeing some order of evaluation conflicts but it's been too long :(.  I'll move this down.

> Failed SQL: [DROP INDEX `profiles_activity_fieldid_idx` ON profiles_activity]
> Error: DBD::mysql::db do failed: Error on rename of
> './bugs_mkanat/#sql-af9_106e0' to './bugs_mkanat/profiles_activity' (errno:
> 150)
> 
> Failed SQL: [ALTER TABLE profiles_activity DROP COLUMN fieldid] Error:
> DBD::mysql::db do failed: Error on rename of './bugs_mkanat/#sql-af9_106e0' to
> './bugs_mkanat/profiles_activity' (errno: 150)

I don't recall seeing this bug when I did the upgrade.  

Comment 46

[Max Kanat-Alexander]

(In reply to comment #45)
> I believe it is named parameters.  Where do you see positional parameters?

  Named parameters means passing the parameters in a hashref, so the function call looks like this:

  foo({ bar => 'yes', baz => 'qux' });

> I don't recall seeing this bug when I did the upgrade.  

  It definitely happens on the tip. Were you using a tip database since the InnoDB conversion?

Comment 47

[Albert Ting]

(In reply to comment #46)
> (In reply to comment #45)
> > I believe it is named parameters.  Where do you see positional parameters?
> 
>   Named parameters means passing the parameters in a hashref, so the function
> call looks like this:
> 
>   foo({ bar => 'yes', baz => 'qux' });

Oh, ok.  Yea, I'm using positional parameters, but also use hashref at times. 

> > I don't recall seeing this bug when I did the upgrade.  
> 
>   It definitely happens on the tip. Were you using a tip database since the
> InnoDB conversion?

I upgraded from 2.22 to 3.x, so it was from MyISAM to InnoDB.

Comment 48

[Max Kanat-Alexander]

(In reply to comment #47)
> I upgraded from 2.22 to 3.x, so it was from MyISAM to InnoDB.

  Probably what happened was that you had all your foreign keys added *after* this, because that's how Install::DB works. Try upgrading from a tip database.

Comment 49

[Albert Ting]

Bug 410873 needs to address the foreign key issue before this bug can be resolved.  

Comment 50

[Frédéric Buclin]

(In reply to Albert Ting from comment #49)
> Bug 410873 needs to address the foreign key issue before this bug can be
> resolved.

Now that this problem has been fixed, is there a chance to see an updated patch? :)

Comment 51

[Albert Ting]

Attachment: v7

It's been a while, but attached is the updated version I'm using with 4.0.3.

Comment 52

[Frédéric Buclin]

Comment on attachment 618391 [details] [diff] [review]
v7

>--- Bugzilla/DB/Schema.pm~	2007-08-07 15:39:50.716987000 -0700
>+++ Bugzilla/DB/Schema.pm	2007-08-07 15:53:46.080430000 -0700

None of the edited file has the correct |=== modified file 'Bugzilla/DB/Schema.pm'| header, which makes it impossible for PatchReader to parse your patch correctly, which makes the review very hard. Could you update your patch so that it has the correct headers?



>--- Bugzilla/User.pm~	2007-08-07 15:39:50.930953000 -0700
>+++ Bugzilla/User.pm	2007-08-07 17:11:36.439449000 -0700

Changes to User.pm no longer apply correctly:

patching file Bugzilla/User.pm
Hunk #1 FAILED at 156.
Hunk #2 succeeded at 1873 with fuzz 2 (offset 172 lines).

Could you please update your patch so that it applies to trunk? This step is required in all cases as we won't take it for 4.0.x. All other changes still apply correctly, so it shouldn't be too hard to fix. :)

Comment 53

[Frédéric Buclin]

The DB change is needed to fix bugs marked as blocked by this bug. Albert, do you have time to update your patch to apply to 4.3.1 or do you want us to update the patch for you?

Comment 54

[Frédéric Buclin]

Too late for 4.4. We already released 4.4rc1, and 4.4rc2 is just around the corner. We don't have a plan for a 3rd release candidate, so this bug is postponed to 5.0.

Comment 55

[Albert Ting]

Attachment: v8

Updated version based on 5.0 branch

Comment 56

[Frédéric Buclin]

Comment on attachment 8546192 [details] [diff] [review]
v8

>--- Bugzilla/DB/Schema.pm~	2007-08-07 15:39:50.716987000 -0700

>-            profiles_activity_fieldid_idx => ['fieldid'],

Did you investigate if an index based on 'field' would make some queries faster? I would guess it does.



>+++ Bugzilla/Install/DB.pm	2007-08-07 16:28:27.873799000 -0700

>+    # 2012-04-24 [e-mail] - Bug 328904

Please update the date.


>+sub _update_profiles_activity_fieldid_to_field {

>+    # 2007-08-08: [e-mail]; bug 328904: don't make

No need to repeat the date and bug ID here (and the date doesn't match the date above).


>+    # store the field rather than the fieildid.

typo + I don't understand the meaning of this last sentence about simplicity.


>+        print "Populating new profiles_activity.field field...\n";

Write |say "foo"| instead of |print "foo\n"|. Also, the last word should be 'column', not 'field'.


>+        my $ids = $dbh->selectall_arrayref(
>+            'SELECT DISTINCT fielddefs.id, fielddefs.name
>+               FROM profiles_activity LEFT JOIN fielddefs
>+                    ON profiles_activity.fieldid = fielddefs.id', {Slice=>{}});

This SQL query looks weird to me. You should rather write |FROM fielddefs INNER JOIN profiles_activity|.


>+        foreach my $item (@$ids) {
>+            my $id    = $item->{id};
>+            my $name = $item->{name};
>+            $dbh->do("UPDATE profiles_activity

As we are inside a loop, prepare the SQL statement to make things faster.


>+        $dbh->do('ALTER TABLE DROP FOREIGN KEY fk_profiles_activity_fieldid_fielddefs_id');

Don't drop FK yourself. First of all, you forgot to name the table to alter, secondly, this SQL statement only works with MySQL. FK are automatically handled by bz_drop_index().



>+++ Bugzilla/User.pm	2007-08-07 17:11:36.439449000 -0700

>+sub log_change {

>+    if ($field eq "groups") {
>+        $oldvalue = $oldvalue ? join(', ', @$oldvalue) : "";
>+        $newvalue = $newvalue ? join(', ', @$newvalue) : "";
>+    }

Instead of hardcoding groups, it would probably make more sense to check the type of $field, and concatenate data if it's of type ARRAY.


>+    if ($field eq "disabledtext" || $field eq "cryptpassword") {
>+        $oldvalue = $oldvalue eq "" ? "" : "text";
>+        $newvalue = $newvalue eq "" ? "" : "text";
>+    }

'text' is hardcoded and not localizable. Maybe '****'?


>+    $dbh->do("INSERT INTO profiles_activity
>+                  (userid, who, profiles_when, field, oldvalue, newvalue)

As log_change() is called from inside a loop, the statement could also be prepared for performance reasons, using prepare_cached().


This was a quick review. I didn't check all the details. But this way you don't have to wait another couple of weeks to get some feedback. :)

Comment 57

[Albert Ting]

Attachment: v9

Updated version per feedback.  Comments below

> Did you investigate if an index ... would make some queries faster?

It's been too long, don't remember if an index was needed.  I don't see the harm.

Have not had a chance to test the DB.pm updates, relying on Bugzilla's builtin test cases

Comment 58

[Frédéric Buclin]

Comment on attachment 8576240 [details] [diff] [review]
v9

Globally looks good, but there are a few things to fix.


>+++ Bugzilla/Install/DB.pm	2007-08-07 16:28:27.873799000 -0700

>+sub _update_profiles_activity_fieldid_to_field {

Move this subroutine to the end of the file, so that they are listed chronologically.


>+        my $ids = $dbh->selectall_arrayref(
>+            'SELECT DISTINCT fielddefs.id, fielddefs.name
>+               FROM fielddefs INNER JOIN profiles_activity
>+                    ON profiles_activity.fieldid = fielddefs.id', {Slice=>{}});

Slice is useless if you call columns in the right order: name, id. Also, name the variable $fields rather than $ids.


>+        my $query = $dbh->prepare("UPDATE profiles_activity SET field = ? WHERE fieldid = ?");

This is not a query. $sth is better.


>+        foreach my $item (@$ids) {
>+            my $id    = $item->{id};
>+            my $name = $item->{name};
>+            $query->execute($name, $id);
>+        }

You can simply write: $sth->execute(@$_) foreach (@$fields);



>+++ Bugzilla/User.pm	2007-08-07 17:11:36.439449000 -0700
>@@ -120,6 +123,8 @@
>+    my $timestamp = $dbh->selectrow_array("SELECT NOW()");

This variable is unused.


>+    my $timestamp = $dbh->selectrow_array("SELECT NOW()");
>+    foreach my $field (keys %$changes) {

Please call this code before logging out the user.


>+        my $from = defined $change->[0] ? $change->[0] : '';
>+        my $to   = defined $change->[1] ? $change->[1] : '';

For consistency, $old and $new are better names than $from and $to (they make me think about emails).
Also, use Perl 5.10+ new // operator and simply write:

  my $old = $change->[0] // '';
  my $new = $change->[1] // '';


>+        my $who = Bugzilla->user->id || $self->id;

The user doing changes must always be logged in. So no reason why Bugzilla->user->id would be 0 or undefined.


>+sub log_change {

>+    trick_taint($oldvalue);
>+    trick_taint($newvalue);

As mkanat said in comment 6, don't call trick_taint() here. These values are already detainted.


>+    my $query = $dbh->prepare("INSERT INTO profiles_activity
>+                               (userid, who, profiles_when, field, oldvalue, newvalue)
>+                               VALUES (?, ?, ?, ?, ?, ?)");
>+    $query->execute($userid, $whoid, $timestamp, $field, $oldvalue, $newvalue);

Calling ->prepare() followed by ->execute() is unhelpful, performance wise. You must call ->prepare_cached(), not ->prepare().
Also, use $sth instead of $query.



>+++ template/en/default/account/profile-activity.html.tmpl	2007-08-20 19:01:24.054483000 -0700

>    {name               => 'what'
>     heading            => 'What'
>-    content_use_field  => 1
>    }

The internal name is displayed instead of the user visible one.



>+++ contrib/recode.pl	2007-09-20 12:28:42.481374000 -0700

>     # profiles_activity since 4.4 has a unique primary key added
>-    profiles_activity => 'userid,profiles_when,fieldid',
>+    profiles_activity => 'userid,profiles_when,field',

You must not change this line. As the comment says, profiles_activity has a PK since 4.4, so this line is for older installations.

Comment 59

[Albert Ting]

Attachment: v10

Requested items addressed, although same thing, have not tested DB.pm changes

Also had to update Field.pm in order to display the user visible version of the profile field names.

Comment 60

[Frédéric Buclin]

Comment on attachment 8605556 [details] [diff] [review]
v10

>+++ Bugzilla/Install/DB.pm	2007-08-07 16:28:27.873799000 -0700

>+        my $fields = $dbh->selectall_arrayref(
>+            'SELECT DISTINCT fielddefs.name as name, fielddefs.id as id

Aliases are useless. You don't use them.


>+               FROM fielddefs INNER JOIN profiles_activity
>+                    ON profiles_activity.fieldid = fielddefs.id', undef);

No need to specify 'undef' as the last argument.


Note that SQLite fails badly when it tries to delete the index. Not sure how the SQLite code works, but it must be broken. So ignore this problem for now.



>+++ Bugzilla/User.pm	2007-08-07 17:11:36.439449000 -0700

>+    my $timestamp = $dbh->selectrow_array("SELECT NOW()");

Use LOCALTIMESTAMP(0) instead of NOW().


>+    if ($field eq "disabledtext" || $field eq "cryptpassword") {
>+        $oldvalue = $oldvalue eq "" ? "" : "****";
>+        $newvalue = $newvalue eq "" ? "" : "****";
>+    }

Write: |$oldvalue = '****' if $oldvalue;| Same for $newvalue. We don't care about the case where $oldvalue eq '0'. This shouldn't happen in this context.
Also, I wonder if we shouldn't display the first few words of disabledtext. This may be useful. Maybe the first 50 or 100 characters.



>+++ Bugzilla/Field.pm	2015-05-13 18:02:30.408579000 -0500

>+    {name => 'disable_mail',          desc => 'Ticketmail Disabled'},
>+    {name => 'disabledtext',          desc => 'Disable Text'},
>+    {name => 'is_enabled',            desc => 'Enabled'},
>+    {name => 'groups',                desc => 'Groups'},
>+    {name => 'bless_groups',          desc => 'Bless Groups'},

As already said in previous comments, they must not be in this module, else they will be listed in query.cgi and the SQL server will fail because these fields do not exist in bugs. Also, "Ticketmail" must be "${terms.Bug}mail" and must be in a template so that they can be translated. And the realname and login_name are missing. Also, 'groups' is useless as editusers.cgi already use 'bug_group'. Or if we want a separate field name, then the old bug_group must be converted into the new groups, else you have a mix of both names in the DB.


Except this last point, everything else is easy to fix. If you can fix that, I think we will be close. :)