profiles table needs a "last seen" column

RESOLVED FIXED in Bugzilla 4.4

Status

()

Bugzilla
User Accounts
--
enhancement
RESOLVED FIXED
14 years ago
5 years ago

People

(Reporter: gerv, Assigned: glob)

Tracking

2.17.7
Bugzilla 4.4
Bug Flags:
approval +

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

14 years ago
It would be good to know when a particular account was last used. This would,
for example:

- Allow any site which wished to to auto-disable accounts not used for a period
- Allow admins to see when particular users last logged in (we found a use for
this recently on b.m.o)

Gerv
We already store this information in the logincookies table, but it goes away
after 30 days...
(Reporter)

Comment 2

14 years ago
Yeah; for this application we need something persistent. Do you think it should
replace or be in addition to the lastused value in logincookies?

Gerv
in addition to would probably work better.  we need the age of each cookie
whether or not it's the last time the person logged in (a person can have
multiple active cookies if they log in from more than one computer)
(Reporter)

Comment 4

14 years ago
We don't actually want to know when the user last logged in (people with static
IPs can stay logged in for months), we want to know when that account last made
a request for a page. So here's what we do:

- Add a datetime field to profiles, called "last_seen".
- Add a new method to the Bugzilla::Auth interface, called 
  update_last_seen($username).
- This method gets called when a login is successful (even if it's a Cookie.pm
  login, we still call it on Bugzilla::Auth)
- This method is implemented by DB.pm to update the profile field. It's a no-op  
  in LDAP.pm (at the moment; someone could implement it later, perhaps.)

CCing bbaetz for his views. Have I managed to avoid breaking the Auth
architecture? :-)

Gerv
Sounds fine

Updated

13 years ago
Severity: normal → enhancement

Updated

12 years ago
QA Contact: mattyt-bugzilla → default-qa
(Reporter)

Comment 6

8 years ago
I'm not sure what I was smoking in comment #4; I said we want to measure the last time a request was made, not the last time we logged in (which is right) but this means it's nothing to do with auth at all.

So yes, we need a new field last_seen on the profiles table, which is updated when the user makes a request.

However, we also want to avoid turning every read into a write (the "atime" problem). So we should probably store the value with a granularity of 1 day, and only update it if it's different to the current value.

Gerv
(Reporter)

Comment 7

6 years ago
I'd still like to see this done, but having it assigned to me is misleading, as I'm not working on it.

Gerv
Assignee: gerv → user-accounts
(Assignee)

Comment 8

6 years ago
Created attachment 570956 [details] [diff] [review]
patch v1

this patch adds and updates the profiles.last_login_date column, but doesn't do anything with it yet.
Assignee: user-accounts → glob
Status: NEW → ASSIGNED
Attachment #570956 - Flags: review?

Updated

6 years ago
Attachment #570956 - Flags: review? → review?(dkl)
Comment on attachment 570956 [details] [diff] [review]
patch v1

Review of attachment 570956 [details] [diff] [review]:
-----------------------------------------------------------------

Comments below.

::: Bugzilla/Install/DB.pm
@@ +656,5 @@
>      # 2011-06-15 dkl@mozilla.com - Bug 658929
>      _migrate_disabledtext_boolean();
>  
> +    # 2011-11-01 glob@mozilla.com - Bug 240437
> +    $dbh->bz_add_column('profiles', 'last_login_date', {TYPE => 'DATETIME'});

Should we also add a migrate function to update the profiles.last_login_date from the user's last cookie in logincookies? If yes, then there is the question as to what to do with the user's who do not have a cookie currently. Suppose we could leave those as NULL and anyone mining the data would understand that those users had not logged in since the change was implemented.

Otherwise if noone really cares for this information prior to the change being committed, we can leave last_login_date completely NULL and go from now.

::: Bugzilla/User.pm
@@ +286,4 @@
>      $_[0]->set('is_enabled', $_[1] ? 0 : 1);
>  }
>  
> +sub touch_last_login_date {

My preference would be that this should be called set_last_login_date() to be consistent with the other mutators but that is not completely accurate as we do not pass a value to the method and it also happens outside of the normal update(). "touch" is not always undertstandable to someone not coming from the Unix world. I would either 1) change it to "set" if you don't feel this causes confusion with the other setters since it doesn't wait for update() or 2) at a minimum, change it to either update or refresh to better describe what it does.

@@ +296,5 @@
> +        # pending changes
> +        my $dbh = Bugzilla->dbh;
> +        $dbh->do("UPDATE profiles SET last_login_date=? WHERE userid=?",
> +                 undef, $date, $_[0]->id);
> +    }

$_[0] is used enough in this method that I would just use $self everywhere instead. It is easier to read and it only adds one line. For methods with only one or two lines, $_[0] is acceptable.
(Reporter)

Comment 10

6 years ago
glob: what about the issue I raise in para 3 of comment 6? Is it a problem or not?

Gerv
(Assignee)

Comment 11

6 years ago
(In reply to Gervase Markham [:gerv] from comment #10)
> glob: what about the issue I raise in para 3 of comment 6? Is it a problem
> or not?

i agree that it could be a problem, so the patch does exactly what you suggested; granularity of 1 day, and only update if required.

Comment 12

6 years ago
Comment on attachment 570956 [details] [diff] [review]
patch v1

>=== modified file 'Bugzilla/User.pm'

>+sub touch_last_login_date {

I don't really like this name. If its goal is to update the date, it should probably be set_* or update_*. We don't use touch_* anywhere else in our code for similar methods.


>+        $dbh->do("UPDATE profiles SET last_login_date=? WHERE userid=?",
>+                 undef, $date, $_[0]->id);

Why not last_login_date = CURRENT_TIMESTAMP instead of what you do? This would be much more efficient.


>+sub last_login_date { $_[0]->{last_login_date}; }

I guess you will have to format it before returning it.

Comment 13

6 years ago
Comment on attachment 570956 [details] [diff] [review]
patch v1

>=== modified file 'Bugzilla.pm'

>+    $class->user->touch_last_login_date() if $class->user->id;

If the user is already logged in, we won't trigger this code, and so the "1 day granularity" discussion can go away. A user doesn't relogin every time he accesses another page. So it's fine to use the real date and time instead of the date only. So my previous suggestion still stands.
Attachment #570956 - Flags: review?(dkl) → review-
(Assignee)

Comment 14

6 years ago
(In reply to Frédéric Buclin from comment #13)
> Comment on attachment 570956 [details] [diff] [review] [diff] [details] [review]
> patch v1
> 
> >=== modified file 'Bugzilla.pm'
> 
> >+    $class->user->touch_last_login_date() if $class->user->id;
> 
> If the user is already logged in, we won't trigger this code

this isn't correct; this code is triggered on each page load for me, even if i'm already logged in.
(Reporter)

Comment 15

6 years ago
LpSolit's question raises an important point - is this a "last login date", in that it's the last time a user supplied their password and authenticated, or is it a "last seen date", as in the last time the user, logged in, loaded a page or took an action? I think it is, and should be, the latter - that's the useful information. So perhaps the field requires a change of name? (Yes, I know I filed the original bug, but I had a minor change of direction in comment 4.)
(Assignee)

Comment 16

6 years ago
(In reply to Gervase Markham [:gerv] from comment #15)
> LpSolit's question raises an important point - is this a "last login date",
> in that it's the last time a user supplied their password and authenticated,
> or is it a "last seen date", as in the last time the user, logged in, loaded
> a page or took an action? I think it is, and should be, the latter - that's
> the useful information. So perhaps the field requires a change of name?

ah, indeed, the name of the field is wrong.  last_seen is better (and what you suggested in comment 4 - sorry i missed that somehow).

Comment 17

6 years ago
(In reply to Byron Jones ‹:glob› from comment #14)
> > If the user is already logged in, we won't trigger this code
> 
> this isn't correct; this code is triggered on each page load for me, even if
> i'm already logged in.

sub login {
    my ($class, $type) = @_;

    return $class->user if $class->user->id;


If the user is already logged in, you won't go further and so you won't trigger your code.
(In reply to Frédéric Buclin from comment #17)
> sub login {
>     my ($class, $type) = @_;
> 
>     return $class->user if $class->user->id;
> 
> If the user is already logged in, you won't go further and so you won't
> trigger your code.

That is only if Bugzilla->login is called more than once in a single page load. If it is a new page load, then the first call to Bugzilla->login will always call $class->user->touch_last_login_date(). $class->user is an empty User object until $class->set_user() is called.

dkl
(Assignee)

Comment 19

6 years ago
Created attachment 571252 [details] [diff] [review]
patch v2

> Should we also add a migrate function to update the profiles.last_login_date
> from the user's last cookie in logincookies?

i don't think migration's required.

> last-login or last-seen?

last-seen :)

> rename sub touch_last_login_date {

now "update_last_seen_date".  to me set_* implies changing the field to a specified value, which isn't the case here.

> >+sub last_login_date { $_[0]->{last_login_date}; }
> I guess you will have to format it before returning it.

it's already in a format which matches other datetime fields.
Attachment #570956 - Attachment is obsolete: true
Attachment #571252 - Flags: review?(dkl)
(Reporter)

Comment 20

6 years ago
How long do login cookies stay in logincookies? If it's a significant amount of time, then migration would be useful, as an approximate value for last_seen would help us to do analysis on the data earlier and easier. If it's not too complicated. 

Gerv

Comment 21

6 years ago
(In reply to Gervase Markham [:gerv] from comment #20)
> How long do login cookies stay in logincookies?

30 days, but it really doesn't worth the migration code at all.

Comment 22

6 years ago
Comment on attachment 571252 [details] [diff] [review]
patch v2

>=== modified file 'Bugzilla.pm'

>+    $class->user->update_last_seen_date() if $class->user->id;

To make your code more robust, the |if $class->user->id| check should be in update_last_seen_date() itself, in case the method is reused elsewhere, e.g. in an extension. It's much better/robust to let the method do the required checks itself that supposing that the caller will do it.



>=== modified file 'Bugzilla/User.pm'

>+sub update_last_seen_date {
>+    my ($year, $month, $day) = (localtime)[5, 4, 3];

Login cookies are stored using the local time of the DB server, not the one of the web server. In the case they are not located in the same timezone, we should be consistent and also use the DB server as reference here.


>+    my $date = sprintf("%4d-%02d-%02d 00:00:00", $year + 1900, $month + 1, $day);

Nit: ++$month


>+    if ($date ne $self->last_seen_date) {

This won't work based on the way you defined last_seen_date(). You cannot assume the format of the date stored in the DB to be of the form "%4d-%02d-%02d 00:00:00". You must use $dbh->sql_date_format() to correctly format the datetime coming from the DB, see e.g. Bugzilla::Bug::DB_COLUMNS to see how it's done with creation_ts and delta_ts.


>+sub last_seen_date { $_[0]->{last_seen_date} || ''; }

As an empty string is not a valid date, I think we should make this method return undef here if not yet defined.
Attachment #571252 - Flags: review?(dkl) → review-
(Reporter)

Comment 23

6 years ago
glob: ping? Looks like this one is pretty close...

Gerv
Summary: profiles table needs a "last logged in" column → profiles table needs a "last seen" column
(Assignee)

Comment 24

6 years ago
(In reply to Gervase Markham [:gerv] from comment #23)
> glob: ping? Looks like this one is pretty close...

thanks, i haven't forgotten about this, i've been sidetracked by other projects.
(Assignee)

Comment 25

6 years ago
Created attachment 582793 [details] [diff] [review]
patch v3
Attachment #571252 - Attachment is obsolete: true
Attachment #582793 - Flags: review?(LpSolit)

Comment 26

6 years ago
Comment on attachment 582793 [details] [diff] [review]
patch v3

>=== modified file 'Bugzilla.pm'

>+    $class->user->update_last_seen_date();

While testing your patch, I realized that the last_seen attribute was updated also when impersonating another user. I don't think this should be the case, else this would give the wrong impression that the account is still active when it was only used by an admin for some reason. So IMO, we should only update this last_seen column when the user himself logs in.



>=== modified file 'Bugzilla/Install/DB.pm'

>+    # 2011-11-01 glob@mozilla.com - Bug 240437
>+    $dbh->bz_add_column('profiles', 'last_seen_date', {TYPE => 'DATETIME'});

You forgot to include this new column in the DB schema (Bugzilla/DB/Schema.pm).
Also, why not last_seen instead of last_seen_date as column name?



>=== modified file 'Bugzilla/User.pm'

>+sub update_last_seen_date {

>+    my $date = $dbh->selectrow_array(
>+        'SELECT ' . $dbh->sql_date_format('NOW()', '%Y-%m-%d'));
>+    if (!$self->last_seen_date or $date ne $self->last_seen_date) {

Nit: please insert an empty line between my $date and if (). It's IMO easier to see where the IF block begins.


>+        $dbh->do("UPDATE profiles SET last_seen_date=? WHERE userid=?",

Add a whitespace around each =, as we do everywhere else.



>=== modified file 'template/en/default/admin/users/edit.html.tmpl'

>+    <th>Last Login:</th>

Why not also add it to the user list, to have this information easily? This information doesn't require any new SQL query, so we would get it for free.


Otherwise looks good.
Attachment #582793 - Flags: review?(LpSolit) → review-

Updated

6 years ago
Target Milestone: --- → Bugzilla 4.4
(Assignee)

Comment 27

6 years ago
Created attachment 590655 [details] [diff] [review]
patch v4

> While testing your patch, I realized that the last_seen attribute was
> updated also when impersonating another user.

nice catch.  this revision updates the last_seen_date for the sudoer, not the user they are impersonating.

> Also, why not last_seen instead of last_seen_date as column name?

because the column is a DATETIME type; i want to ensure it's clear that the time portion of the value is not valid.

> Why not also add it to the user list, to have this information easily? This
> information doesn't require any new SQL query, so we would get it for free.

while not totally free, i've added it to this revision.
Attachment #582793 - Attachment is obsolete: true
Attachment #590655 - Flags: review?(LpSolit)

Comment 28

6 years ago
Comment on attachment 590655 [details] [diff] [review]
patch v4

>=== modified file 'Bugzilla.pm'

>+    if (Bugzilla->sudoer) {
>+        Bugzilla->sudoer->update_last_seen_date();
>+    } else {
>+        $class->user->update_last_seen_date();
>+    }

For consistency with $class->user, write $class->sudoer.


Otherwise looks good and works fine. r=LpSolit
Attachment #590655 - Flags: review?(LpSolit) → review+

Updated

6 years ago
Flags: approval+
Keywords: relnote
Can we also get a "last seen ip" column, or should that be a separate bug? logincookies is not sufficient for that purpose, as it does not store an IP unless you have it restricted to a netmask.

Comment 30

6 years ago
(In reply to Reed Loden [:reed] (very busy) from comment #29)
> Can we also get a "last seen ip" column, or should that be a separate bug?

This is unrelated to this bug.


> logincookies is not sufficient for that purpose, as it does not store an IP
> unless you have it restricted to a netmask.

Bugzilla stopped using netmasks for a long time, since 3.6, see bug 399073. So I don't see why you would need that.
(Assignee)

Comment 31

6 years ago
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla.pm
modified editusers.cgi
modified Bugzilla/User.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/Install/DB.pm
modified template/en/default/admin/users/edit.html.tmpl
modified template/en/default/admin/users/list.html.tmpl
Committed revision 8088.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 32

6 years ago
Comment on attachment 590655 [details] [diff] [review]
patch v4

Review of attachment 590655 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry that I didn't comment on this until now, but:

::: Bugzilla/User.pm
@@ +75,5 @@
> +        'profiles.disabledtext',
> +        'profiles.disable_mail',
> +        'profiles.extern_id',
> +        'profiles.is_enabled',
> +        $dbh->sql_date_format('last_seen_date', '%Y-%m-%d') . ' AS last_seen_date',

WHY ARE YOU FORMATTING THINGS INSIDE OF OBJECTS? Please stop doing this. It makes it impossible to use this date in any other fashion! Formatting is for UIs, not for backends.

Comment 33

6 years ago
(In reply to Frédéric Buclin from comment #22)
> This won't work based on the way you defined last_seen_date(). You cannot
> assume the format of the date stored in the DB to be of the form
> "%4d-%02d-%02d 00:00:00". You must use $dbh->sql_date_format() to correctly
> format the datetime coming from the DB, see e.g. Bugzilla::Bug::DB_COLUMNS
> to see how it's done with creation_ts and delta_ts.

  Ahh, I see why you're doing this. If you're going to do this, you should have a date-formatting constant, and that constant should always format down to seconds.

  If we really did do this consistently across Bugzilla, we could support databases that have non-standard date formats, which could be nice.
(Assignee)

Comment 34

6 years ago
(In reply to Max Kanat-Alexander from comment #33)
>   Ahh, I see why you're doing this. If you're going to do this, you should
> have a date-formatting constant, and that constant should always format down
> to seconds.

filed as bug 720639

Comment 35

5 years ago
Added to relnotes for 4.4.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.