Edgewall Software
Modify

Opened 7 years ago

Last modified 4 years ago

#10736 new enhancement

env.get_known_users() should also return session.last_visit

Reported by: lkraav <leho@…> Owned by:
Priority: normal Milestone: next-major-releases
Component: general Version: 0.12.3
Severity: normal Keywords: users, patch
Cc: osimons, Ryan J Ollos Branch:
Release Notes:
API Changes:

Description (last modified by lkraav <leho@…>)

I'm trying to build up something really lightweight to get people visible in my projects without #2456 and various hardcore TH plugins. I'd like to make use of the last_visited attribute to for example only show recently logged in people. But it turns out I gotta jump through hoops to get to it.

For example th:AccountManagerPlugin goes straight SQL here:

th:source:accountmanagerplugin/trunk/acct_mgr/api.py@11554#L277

accountmanagerplugin/trunk/acct_mgr/api.py:277:    def last_seen(self, user =
None):

In 1.0dev source:/trunk/trac/env.py@11073#618 we have:

618     def get_known_users(self, cnx=None):
619         """Generator that yields information about all known users,
620         i.e. users that have logged in to this Trac environment and
621         possibly set their name and email.
622
623         This function generates one tuple for every user, of the form
624         (username, name, email) ordered alpha-numerically by username.

Why couldn't this function also include the apparently orphanized last_visit attribute right there? Can't be a significant performance penalty, can it?

Oh yeah, going through trac.web.session.SessionAdmin source:/trunk/trac/web/session.py@11012#L398 doesn't seem to make much sense either. Or…?

Attachments (0)

Change History (22)

comment:1 by lkraav <leho@…>, 7 years ago

Description: modified (diff)

comment:2 by lkraav <leho@…>, 7 years ago

Description: modified (diff)

comment:3 by Remy Blank, 7 years ago

What's wrong with _do_list()?

comment:4 by lkraav <leho@…>, 7 years ago

Well, it prints its results right now. Are you suggesting just subclassing SessionAdmin and overriding _do_list() with my own is the way to go here?

comment:5 by Remy Blank, 7 years ago

Oh, sorry I misread your sentence. I read "Oh yeah, trac.web.session.SessionAdmin … doesn't seem to make much sense either." (without the "going through"). And I was wondering why that method didn't make much sense ;)

I suppose we could add an argument to Environment.get_known_users() to tell it to return the "last seen" info as well, but then again, the method is nothing else than a simple SQL query, so you could just as well put the query in your own code.

comment:6 by lkraav <leho@…>, 7 years ago

Well yeah, but as #2456 is still stalled for unknown amount of time and there are other plugins out there looking for this info, why not just include it in core. I don't think much else is planned for the session model anyway, in favor of, let's say anything related to #2456, for any "Pandoras box" concern or am I wrong?

Anyway, I'll go ahead and present a simple patch for get_known_users() soon. It'd definitely be nice not having to write SQL in plugins.

comment:7 by Ryan J Ollos <ryano@…>, 7 years ago

Cc: ryano@… added

comment:8 by lkraav <leho@…>, 7 years ago

So rblank, perhaps you can point out some obvious errors for what I have so far. For one, I am not sure how to approach the for + yield part. I'd like to to just add a variable to the for loop list based on last_visit, but I'm not sure that's syntactically possible? Or can you give a list as an argument to

for list in cursor:
  • trac/env.py

    diff --git a/trac/env.py b/trac/env.py
    index 96f4425..75e1b72 100644
    a b class Environment(Component, ComponentManager):  
    460460        self.log.info('-' * 32 + ' environment startup [Trac %s] ' + '-' * 32,
    461461                      get_pkginfo(core).get('version', VERSION))
    462462
    463     def get_known_users(self, cnx=None):
     463    def get_known_users(self, cnx=None, last_visit=False):
    464464        """Generator that yields information about all known users, i.e. users
    465465        that have logged in to this Trac environment and possibly set their name
    466466        and email.
    class Environment(Component, ComponentManager):  
    470470
    471471        @param cnx: the database connection; if ommitted, a new connection is
    472472                    retrieved
     473        @param last_visit: pass True here if you need to access session.last_visit
    473474        """
    474475        if not cnx:
    475476            cnx = self.get_db_cnx()
    476477        cursor = cnx.cursor()
    477         cursor.execute("SELECT DISTINCT s.sid, n.value, e.value "
     478
     479        last_visit = " s.last_visit," if last_visit else ""
     480
     481        cursor.execute("SELECT DISTINCT s.sid,{} n.value, e.value"
    478482                       "FROM session AS s "
    479483                       " LEFT JOIN session_attribute AS n ON (n.sid=s.sid "
    480484                       "  and n.authenticated=1 AND n.name = 'name') "
    481485                       " LEFT JOIN session_attribute AS e ON (e.sid=s.sid "
    482486                       "  AND e.authenticated=1 AND e.name = 'email') "
    483                        "WHERE s.authenticated=1 ORDER BY s.sid")
    484         for username, name, email in cursor:
    485             yield username, name, email
     487                       "WHERE s.authenticated=1 ORDER BY s.sid"
     488                       .format(last_visit))
     489
     490        for username, name, email, last_visit in cursor:
     491            yield username, name, email, last_visit
    486492
    487493    def backup(self, dest=None):
    488494        """Create a backup of the database.

comment:9 by lkraav <leho@…>, 7 years ago

Btw is {{{#!diff }}} rendering supposed to work in the instant preview here? I'm definitely not getting a nice colored rendering in the preview, just regular black and white text, although it has proper layout. Fortunately the final rendering seems to be fine.

in reply to:  9 comment:10 by Christian Boos, 7 years ago

Replying to lkraav <leho@…>:

Btw is {{{#!diff }}} rendering supposed to work in the instant preview here?

#10470

in reply to:  6 comment:11 by Ryan J Ollos <ryano@…>, 7 years ago

Replying to lkraav <leho@…>:

Anyway, I'll go ahead and present a simple patch for get_known_users() soon. It'd definitely be nice not having to write SQL in plugins.

I agree, though I'm not so much against writing SQL in plugin as much as I think anyone looking at the API would question why last_visit is not being returned, and why they have to go out of their way to get that info. Things like that always make me wonder if I'm doing coding correctly against the API or rather just completely missing something key. Its just one of those little things that makes it slightly harder to write a plugin, so it would be nice to improve upon.

comment:12 by osimons, 7 years ago

Cc: osimons added

comment:13 by Christian Boos, 7 years ago

Milestone: 0.12.4next-dev-1.1.x

Not for 0.12.x.

comment:15 by Ryan J Ollos, 5 years ago

Cc: Ryan J Ollos added; ryano@… removed

comment:16 by lkraav <leho@…>, 5 years ago

Keywords: patch added

comment:17 by Ryan J Ollos, 5 years ago

After #11597 default_handler is also a session attribute, and the number of session attributes is likely to grow in forthcoming releases. Therefore a parameter that specified a list of additional attributes to return might be more generally useful than a last_visit parameter.

comment:18 by Ryan J Ollos, 5 years ago

With the proposed changes in log:rjollos.git:t10736 you could access the last_visit attribute using:

for session in Session.select(self.env):
   last_visit = session.get('last_visit')

This way we don't need to change Environment.get_known_users, which has a narrowly-defined purpose anyway, and we've exposed an easy way to work with all of the existing sessions and session attributes. We've also moved another SQL query back into the "model" (though Session/DetactedSession doesn't look quite like the other model classes yet, see #11701 and specifically comment:3:ticket:11701 which suggests method parameters for select that would allow for ordering and limiting the number of results returned).

Furthermore, if the changes in #8172 are committed (comment:14:ticket:8172), all of the SQL queries will likely have been moved out of env.py and into the classes that they are more-closely associated with, which together with this ticket would result in a nice overall refactoring of the classes in env.py.

in reply to:  18 comment:19 by Peter Suter, 5 years ago

Replying to rjollos:

With the proposed changes in log:rjollos.git:t10736

I suspect performance is a problem. On a synthetic Trac environment with 2500 sessions counting the number of Environment.get_known_users() makes one DB query and takes about 0.02 seconds. After your changes it would make about 5000 DB queries and take about 2 seconds.

for session in Session.select(self.env):

It could be a bit surprising that Session.select() returns DetachedSessions.

This way we don't need to change Environment.get_known_users, which has a narrowly-defined purpose anyway, and we've exposed an easy way to work with all of the existing sessions and session attributes.

All authenticated sessions. But I guess that would be fixed in #11701 with the where parameter?

comment:20 by Ryan J Ollos, 5 years ago

Thanks for profiling. I'll look into it again soon and see if we can do better with the restructured code.

comment:21 by Ryan J Ollos, 5 years ago

Milestone: next-dev-1.1.xnext-major-releases

Retargetting tickets to narrow focus for milestone:1.2. Please move the ticket back to milestone:next-dev-1.1.x if you intend to resolve it by milestone:1.2.

comment:22 by lkraav <leho@…>, 4 years ago

Is this likely to land some time in the near future? Since lots of changes in the infrastructure, perhaps this has become easier?

comment:23 by Ryan J Ollos, 4 years ago

It won't be included in milestone:1.2, but it's possible for 1.3.x if someone reworks the patch.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned. Next status will be 'new'.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.