Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Bug 4772 - stats page uses oldstyle urls #337

Closed
wants to merge 7 commits into from

2 participants

@rickscott
Collaborator

http://bugs.dwscoalition.org/show_bug.cgi?id=4772

This gets rid of the old-style hand-rolled user URLs in stats.bml, and displays a standard userhead & name instead.

Newbie move: I totally forgot to set my dreamwidth email address (shadowspar@dreamwidth.org) in git's config. Happy to squash and/or rebase if needed, for this or other reasons. =)

rickscott added some commits
@rickscott rickscott Use ljuser_display() instead of handrolled URLs.
First pass at eliminating old-style user URLs from the stats page.
9f0a5ee
@rickscott rickscott refactor recentupdates section to use load_userids
Database efficiency: refactor to use 3 calls to LJ::load_userids()
instead of 30 calls to LJ::load_user().
b6912e8
@rickscott rickscott refactor newjournals section to use load_userids
Analogous change to the previous commit, applied to the newjournals
section instead of the recentupdates section.
7d24f97
@rickscott rickscott Display user name with userhead & date, per orig.
Took out the user's name when I changed the page to use
ljuser_display() for users; re-adding it now.
Also cleaned this section up by using sprintf() instead of
several string concatenations.
773d133
@rickscott rickscott Fix broken/inconsistent html: <p>/<ul> nesting
Namely, <ul>s don't nest inside <p>s.  Code had this right
in places, wrong in others.
ea0538a
@rickscott rickscott ws cleanup: tabs to spaces, remove trailing spaces a2133f5
@rickscott rickscott Merge branch 'develop' into bug4772-stats-old-urls 3993ef8
@afuna
Owner

I have comments further on -- so yeah, I'd recommend rebasing to include the dreamwidth email address, at the same time.

@afuna afuna commented on the diff
htdocs/stats.bml
((15 lines not shown))
$sth->execute;
- my $ct;
- while (my ($iuser, $iname, $itime) = $sth->fetchrow_array) {
- $ret .= "<li><a href='/users/$iuser/'><?_eh $iname _eh?></a>, $itime</li>\n";
- $ct++;
+ my $accounts = $sth->fetchall_arrayref({});
+
+ if (scalar @$accounts == 0) {
@afuna Owner
afuna added a note

for reference: $ret .= "..." unless scalar @$accounts; is more idiomatic, and follows our coding guidelines more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff
htdocs/stats.bml
((24 lines not shown))
}
- $ret .= "<li><i>$ML{'.notavailable'}</i></li>" unless $ct;
- $ret .= "</ul> </p>";
- # Community accounts
- $ret .= "<p>$ML{'.recent.desc.community'}<ul>";
- $sth = $dbr->prepare( "SELECT u.user, u.name, uu.timeupdate FROM user u, userusage uu WHERE u.userid=uu.userid AND uu.timeupdate > DATE_SUB(NOW(), INTERVAL 30 DAY) AND u.journaltype = 'C' ORDER BY uu.timeupdate DESC LIMIT 10" );
+ else {
+ my $userobj_for = LJ::load_userids(map { $_->{userid} } @$accounts);
+
+ foreach my $account (@$accounts) {
+ $ret .= sprintf("<li>%s - %s, %s</li>\n",
+ $userobj_for->{$account->{userid}}->ljuser_display(),
@afuna Owner
afuna added a note

(ignore what was here before, changed my mind)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff
htdocs/stats.bml
((24 lines not shown))
}
- $ret .= "<li><i>$ML{'.notavailable'}</i></li>" unless $ct;
- $ret .= "</ul> </p>";
- # Community accounts
- $ret .= "<p>$ML{'.recent.desc.community'}<ul>";
- $sth = $dbr->prepare( "SELECT u.user, u.name, uu.timeupdate FROM user u, userusage uu WHERE u.userid=uu.userid AND uu.timeupdate > DATE_SUB(NOW(), INTERVAL 30 DAY) AND u.journaltype = 'C' ORDER BY uu.timeupdate DESC LIMIT 10" );
+ else {
+ my $userobj_for = LJ::load_userids(map { $_->{userid} } @$accounts);
+
@afuna Owner
afuna added a note

It is possible for load_userids to return an invalid object, so always check that $u is valid. So what I'd suggest (and following conventions used in other places in the codebase):

my $us = LJ::load_userids...
foreach my $u ( values %$us ) { 
    next unless $u && $u->is_visible;

    $ret .= ...
}

-- also convention, we use $u to refer to user objects.

@rickscott Collaborator

One thing -- we can't just dump the @$accounts array and display users out of the %$us hash, because that throws away the ordering of the users -- they're sorted from most recent update to least recent. <_<;

Here's how I've reworked things in the next PR -- hopefully it's a bit better! ^_^;
rickscott@99b1d1b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff
htdocs/stats.bml
((24 lines not shown))
}
- $ret .= "<li><i>$ML{'.notavailable'}</i></li>" unless $ct;
- $ret .= "</ul> </p>";
- # Community accounts
- $ret .= "<p>$ML{'.recent.desc.community'}<ul>";
- $sth = $dbr->prepare( "SELECT u.user, u.name, uu.timeupdate FROM user u, userusage uu WHERE u.userid=uu.userid AND uu.timeupdate > DATE_SUB(NOW(), INTERVAL 30 DAY) AND u.journaltype = 'C' ORDER BY uu.timeupdate DESC LIMIT 10" );
+ else {
+ my $userobj_for = LJ::load_userids(map { $_->{userid} } @$accounts);
+
+ foreach my $account (@$accounts) {
+ $ret .= sprintf("<li>%s - %s, %s</li>\n",
+ $userobj_for->{$account->{userid}}->ljuser_display(),
+ $userobj_for->{$account->{userid}}->name_html(),
@afuna Owner
afuna added a note

S'cool, just a style nitpick: you don't need those trailing parentheses. $u->name_html; looks cleaner than $u->name_html();

(note: using $u as according to my example above)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff
htdocs/stats.bml
((55 lines not shown))
}
- $ret .= "<li><i>$ML{'.notavailable'}</i></li>" unless $ct;
- $ret .= "</ul> </p>";
- # Feeds
- $ret .= "<p>$ML{'.recent.desc.feeds'}</p><ul>";
- $sth = $dbr->prepare( "SELECT u.user, u.name, uu.timeupdate FROM user u, userusage uu WHERE u.userid=uu.userid AND uu.timeupdate > DATE_SUB(NOW(), INTERVAL 30 DAY) AND u.journaltype = 'Y' ORDER BY uu.timeupdate DESC LIMIT 10" );
+ else {
+ my $userobj_for = LJ::load_userids(map { $_->{userid} } @$accounts);
@afuna Owner
afuna added a note

similar comment here and where applicable!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna
Owner

One final: I'd recommend removing the merge branch develop into yoru current branch. Merge has its uses, but in something like this it can clutter up the history, and it's unneeded unless you're on a particularly large long-running branch that is running in paralellel with the main branch.

@afuna
Owner

Oh hey, just a reminder, when you update this, please comment so I don't miss it! (I get an email notification of new comments, but not of new commits pushed to a pull request)

@afuna
Owner

hear tell there'll be a new PR for this, so closing the current one!

@afuna afuna closed this
@tweath tweath referenced this pull request from a commit in tweath/dw-free
@rickscott rickscott (bug 4772) show users using standard userhead
Also, use load_userids to pull users from the database as a group,
instead of loading them one by one with calls to LJ::load_user.

Previous branch: bug4772-stats-old-urls; previous pull req:
dreamwidth#337
52805aa
@tweath tweath referenced this pull request from a commit in tweath/dw-free
@rickscott rickscott (bug 4772) use more idiomatic "if ( @$accounts )"
Address Fu's comment here:
dreamwidth#337 (comment)

Alter this from "if (scalar @$accounts == 0)" (if there are
no accounts in the results, do this, else process them)
to the (hopefully) more readable "if ( @$accounts )"
(if there are accounts, process them, else display an error message).
4373ccd
@tweath tweath referenced this pull request from a commit in tweath/dw-free
@rickscott rickscott (bug 4772) rework code that creates <li>s
Each element of @$accounts is a hashref like so:
 { userid => 1234 , timeupdate => '2013-03-30 01:58:32' }

Then LJ::load_userids gives us a hash that lets us map userid numbers
to user objects.  Unfortunately, we need to keep the @$accounts array
around to preserve the ordering -- they're sorted by last update time.

Hopefully subbing in the conventional $u will make what is going
on here clearer.  If not, we'll have to rework this in a more
readable but less efficient way. ^_^;

Addresses afuna's comment:
dreamwidth#337 (comment)
c393ab6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 11, 2013
  1. @rickscott

    Use ljuser_display() instead of handrolled URLs.

    rickscott authored
    First pass at eliminating old-style user URLs from the stats page.
Commits on Mar 30, 2013
  1. @rickscott

    refactor recentupdates section to use load_userids

    rickscott authored
    Database efficiency: refactor to use 3 calls to LJ::load_userids()
    instead of 30 calls to LJ::load_user().
Commits on Mar 31, 2013
  1. @rickscott

    refactor newjournals section to use load_userids

    rickscott authored
    Analogous change to the previous commit, applied to the newjournals
    section instead of the recentupdates section.
  2. @rickscott

    Display user name with userhead & date, per orig.

    rickscott authored
    Took out the user's name when I changed the page to use
    ljuser_display() for users; re-adding it now.
    Also cleaned this section up by using sprintf() instead of
    several string concatenations.
  3. @rickscott

    Fix broken/inconsistent html: <p>/<ul> nesting

    rickscott authored
    Namely, <ul>s don't nest inside <p>s.  Code had this right
    in places, wrong in others.
  4. @rickscott
  5. @rickscott
This page is out of date. Refresh to see the latest.
Showing with 122 additions and 56 deletions.
  1. +122 −56 htdocs/stats.bml
View
178 htdocs/stats.bml
@@ -99,80 +99,146 @@ body<=
ret => \$ret,
});
- if ( LJ::is_enabled('stats-recentupdates') )
- {
+ if ( LJ::is_enabled('stats-recentupdates') )
+ {
$ret .= "<h1>$ML{'.recent.header'}</h1>";
- # Personal accounts
- $ret .= "<p>$ML{'.recent.desc.personal'}<ul>";
- $sth = $dbr->prepare( "SELECT u.user, u.name, uu.timeupdate FROM user u, userusage uu WHERE u.userid=uu.userid AND uu.timeupdate > DATE_SUB(NOW(), INTERVAL 30 DAY) AND u.journaltype = 'P' ORDER BY uu.timeupdate DESC LIMIT 10" );
+ # Personal accounts
+ $ret .= "<p>$ML{'.recent.desc.personal'}</p><ul>";
+ $sth = $dbr->prepare( "SELECT u.userid, uu.timeupdate FROM user u, userusage uu WHERE u.userid=uu.userid AND uu.timeupdate > DATE_SUB(NOW(), INTERVAL 30 DAY) AND u.journaltype = 'P' ORDER BY uu.timeupdate DESC LIMIT 10" );
$sth->execute;
- my $ct;
- while (my ($iuser, $iname, $itime) = $sth->fetchrow_array) {
- $ret .= "<li><a href='/users/$iuser/'><?_eh $iname _eh?></a>, $itime</li>\n";
- $ct++;
+ my $accounts = $sth->fetchall_arrayref({});
+
+ if (scalar @$accounts == 0) {
@afuna Owner
afuna added a note

for reference: $ret .= "..." unless scalar @$accounts; is more idiomatic, and follows our coding guidelines more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ $ret .= "<li><i>$ML{'.notavailable'}</i></li>";
}
- $ret .= "<li><i>$ML{'.notavailable'}</i></li>" unless $ct;
- $ret .= "</ul> </p>";
- # Community accounts
- $ret .= "<p>$ML{'.recent.desc.community'}<ul>";
- $sth = $dbr->prepare( "SELECT u.user, u.name, uu.timeupdate FROM user u, userusage uu WHERE u.userid=uu.userid AND uu.timeupdate > DATE_SUB(NOW(), INTERVAL 30 DAY) AND u.journaltype = 'C' ORDER BY uu.timeupdate DESC LIMIT 10" );
+ else {
+ my $userobj_for = LJ::load_userids(map { $_->{userid} } @$accounts);
+
@afuna Owner
afuna added a note

It is possible for load_userids to return an invalid object, so always check that $u is valid. So what I'd suggest (and following conventions used in other places in the codebase):

my $us = LJ::load_userids...
foreach my $u ( values %$us ) { 
    next unless $u && $u->is_visible;

    $ret .= ...
}

-- also convention, we use $u to refer to user objects.

@rickscott Collaborator

One thing -- we can't just dump the @$accounts array and display users out of the %$us hash, because that throws away the ordering of the users -- they're sorted from most recent update to least recent. <_<;

Here's how I've reworked things in the next PR -- hopefully it's a bit better! ^_^;
rickscott@99b1d1b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ foreach my $account (@$accounts) {
+ $ret .= sprintf("<li>%s - %s, %s</li>\n",
+ $userobj_for->{$account->{userid}}->ljuser_display(),
@afuna Owner
afuna added a note

(ignore what was here before, changed my mind)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ $userobj_for->{$account->{userid}}->name_html(),
@afuna Owner
afuna added a note

S'cool, just a style nitpick: you don't need those trailing parentheses. $u->name_html; looks cleaner than $u->name_html();

(note: using $u as according to my example above)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ $account->{timeupdate}
+ );
+ }
+ }
+
+ $ret .= "</ul>\n";
+ # Community accounts
+ $ret .= "<p>$ML{'.recent.desc.community'}</p><ul>";
+ $sth = $dbr->prepare( "SELECT u.userid, uu.timeupdate FROM user u, userusage uu WHERE u.userid=uu.userid AND uu.timeupdate > DATE_SUB(NOW(), INTERVAL 30 DAY) AND u.journaltype = 'C' ORDER BY uu.timeupdate DESC LIMIT 10" );
$sth->execute;
- my $ct = 0;
- while (my ($iuser, $iname, $itime) = $sth->fetchrow_array) {
- $ret .= "<li><a href='/users/$iuser/'><?_eh $iname _eh?></a>, $itime</li>\n";
- $ct++;
+ my $accounts = $sth->fetchall_arrayref({});
+
+ if (scalar @$accounts == 0) {
+ $ret .= "<li><i>$ML{'.notavailable'}</i></li>";
}
- $ret .= "<li><i>$ML{'.notavailable'}</i></li>" unless $ct;
- $ret .= "</ul> </p>";
- # Feeds
- $ret .= "<p>$ML{'.recent.desc.feeds'}</p><ul>";
- $sth = $dbr->prepare( "SELECT u.user, u.name, uu.timeupdate FROM user u, userusage uu WHERE u.userid=uu.userid AND uu.timeupdate > DATE_SUB(NOW(), INTERVAL 30 DAY) AND u.journaltype = 'Y' ORDER BY uu.timeupdate DESC LIMIT 10" );
+ else {
+ my $userobj_for = LJ::load_userids(map { $_->{userid} } @$accounts);
@afuna Owner
afuna added a note

similar comment here and where applicable!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ foreach my $account (@$accounts) {
+ $ret .= sprintf("<li>%s - %s, %s</li>\n",
+ $userobj_for->{$account->{userid}}->ljuser_display(),
+ $userobj_for->{$account->{userid}}->name_html(),
+ $account->{timeupdate}
+ );
+ }
+ }
+
+ $ret .= "</ul>\n";
+ # Feeds
+ $ret .= "<p>$ML{'.recent.desc.feeds'}</p><ul>";
+ $sth = $dbr->prepare( "SELECT u.userid, uu.timeupdate FROM user u, userusage uu WHERE u.userid=uu.userid AND uu.timeupdate > DATE_SUB(NOW(), INTERVAL 30 DAY) AND u.journaltype = 'Y' ORDER BY uu.timeupdate DESC LIMIT 10" );
$sth->execute;
- my $ct = 0;
- while (my ($iuser, $iname, $itime) = $sth->fetchrow_array) {
- $ret .= "<li><a href='/users/$iuser/'><?_eh $iname _eh?></a>, $itime</li>\n";
- $ct++;
+ my $accounts = $sth->fetchall_arrayref({});
+
+ if (scalar @$accounts == 0) {
+ $ret .= "<li><i>$ML{'.notavailable'}</i></li>";
}
- $ret .= "<li><i>$ML{'.notavailable'}</i></li>" unless $ct;
- $ret .= "</ul> \n";
+ else {
+ my $userobj_for = LJ::load_userids(map { $_->{userid} } @$accounts);
+
+ foreach my $account (@$accounts) {
+ $ret .= sprintf("<li>%s - %s, %s</li>\n",
+ $userobj_for->{$account->{userid}}->ljuser_display(),
+ $userobj_for->{$account->{userid}}->name_html(),
+ $account->{timeupdate}
+ );
+ }
+ }
+
+ $ret .= "</ul>\n";
}
- if ( LJ::is_enabled('stats-newjournals') )
+ if ( LJ::is_enabled('stats-newjournals') )
{
$ret .= "<h1>$ML{'.new.header'}</h1>";
- # Personal accounts
- $ret .= "<p>$ML{'.new.desc.personal'}<ul>";
- $sth = $dbr->prepare("SELECT u.user, u.name, uu.timeupdate FROM user u, userusage uu WHERE u.userid=uu.userid AND uu.timeupdate IS NOT NULL AND u.journaltype = 'P' ORDER BY uu.timecreate DESC LIMIT 10");
+ # Personal accounts
+ $ret .= "<p>$ML{'.new.desc.personal'}</p><ul>";
+ $sth = $dbr->prepare("SELECT u.userid, uu.timeupdate FROM user u, userusage uu WHERE u.userid=uu.userid AND uu.timeupdate IS NOT NULL AND u.journaltype = 'P' ORDER BY uu.timecreate DESC LIMIT 10");
$sth->execute;
- my $ct;
- while (my ($iuser, $iname, $itime) = $sth->fetchrow_array) {
- $ret .= "<li><a href='/users/$iuser/'><?_eh $iname _eh?></a>, $itime</li>\n";
- $ct++;
+ my $accounts = $sth->fetchall_arrayref({});
+
+ if (scalar @$accounts == 0) {
+ $ret .= "<li><i>$ML{'.notavailable'}</i></li>";
}
- $ret .= "<li><i>$ML{'.notavailable'}</i></li>" unless $ct;
- $ret .= "</ul> </p>";
- # Community accounts
- $ret .= "<p>$ML{'.new.desc.community'}<ul>";
- $sth = $dbr->prepare("SELECT u.user, u.name, uu.timeupdate FROM user u, userusage uu WHERE u.userid=uu.userid AND uu.timeupdate IS NOT NULL AND u.journaltype = 'C' ORDER BY uu.timecreate DESC LIMIT 10");
+ else {
+ my $userobj_for = LJ::load_userids(map { $_->{userid} } @$accounts);
+
+ foreach my $account (@$accounts) {
+ $ret .= sprintf("<li>%s - %s, %s</li>\n",
+ $userobj_for->{$account->{userid}}->ljuser_display(),
+ $userobj_for->{$account->{userid}}->name_html(),
+ $account->{timeupdate}
+ );
+ }
+ }
+
+ $ret .= "</ul>\n";
+ # Community accounts
+ $ret .= "<p>$ML{'.new.desc.community'}</p><ul>";
+ $sth = $dbr->prepare("SELECT u.userid, uu.timeupdate FROM user u, userusage uu WHERE u.userid=uu.userid AND uu.timeupdate IS NOT NULL AND u.journaltype = 'C' ORDER BY uu.timecreate DESC LIMIT 10");
$sth->execute;
- my $ct;
- while (my ($iuser, $iname, $itime) = $sth->fetchrow_array) {
- $ret .= "<li><a href='/users/$iuser/'><?_eh $iname _eh?></a>, $itime</li>\n";
- $ct++;
+ my $accounts = $sth->fetchall_arrayref({});
+
+ if (scalar @$accounts == 0) {
+ $ret .= "<li><i>$ML{'.notavailable'}</i></li>";
}
- $ret .= "<li><i>$ML{'.notavailable'}</i></li>" unless $ct;
- $ret .= "</ul> </p>";
- # Feeds
+ else {
+ my $userobj_for = LJ::load_userids(map { $_->{userid} } @$accounts);
+
+ foreach my $account (@$accounts) {
+ $ret .= sprintf("<li>%s - %s, %s</li>\n",
+ $userobj_for->{$account->{userid}}->ljuser_display(),
+ $userobj_for->{$account->{userid}}->name_html(),
+ $account->{timeupdate}
+ );
+ }
+ }
+
+ $ret .= "</ul>\n";
+ # Feeds
$ret .= "<p>$ML{'.new.desc.feeds'}</p><ul>";
- $sth = $dbr->prepare("SELECT u.user, u.name, uu.timeupdate FROM user u, userusage uu WHERE u.userid=uu.userid AND uu.timeupdate IS NOT NULL AND u.journaltype = 'Y' ORDER BY uu.timecreate DESC LIMIT 10");
+ $sth = $dbr->prepare("SELECT u.userid, uu.timeupdate FROM user u, userusage uu WHERE u.userid=uu.userid AND uu.timeupdate IS NOT NULL AND u.journaltype = 'Y' ORDER BY uu.timecreate DESC LIMIT 10");
$sth->execute;
- my $ct;
- while (my ($iuser, $iname, $itime) = $sth->fetchrow_array) {
- $ret .= "<li><a href='/users/$iuser/'><?_eh $iname _eh?></a>, $itime</li>\n";
- $ct++;
+ my $accounts = $sth->fetchall_arrayref({});
+
+ if (scalar @$accounts == 0) {
+ $ret .= "<li><i>$ML{'.notavailable'}</i></li>";
}
- $ret .= "<li><i>$ML{'.notavailable'}</i></li>" unless $ct;
- $ret .= "</ul> \n";
+ else {
+ my $userobj_for = LJ::load_userids(map { $_->{userid} } @$accounts);
+
+ foreach my $account (@$accounts) {
+ $ret .= sprintf("<li>%s - %s, %s</li>\n",
+ $userobj_for->{$account->{userid}}->ljuser_display(),
+ $userobj_for->{$account->{userid}}->name_html(),
+ $account->{timeupdate}
+ );
+ }
+ }
+
+ $ret .= "</ul>\n";
}
$ret .= "<h1>$ML{'.demographics.header'}</h1>";
Something went wrong with that request. Please try again.