Skip to content
This repository has been archived by the owner on Apr 17, 2018. It is now read-only.

Realnames #96

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Realnames #96

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ed22f70
Merge branch 'master' of https://github.com/tricycle/lesswrong
LucasSloan Jul 9, 2013
3a61c26
Merge branch 'master' of https://github.com/tricycle/lesswrong
LucasSloan Jan 6, 2014
300ada3
Merge branch 'master' of https://github.com/tricycle/lesswrong
LucasSloan Apr 15, 2014
d3078f7
New feature - real names. Add a real name in preferences and user fa…
LucasSloan Apr 22, 2014
fec33b9
New feature - real names. Add a real name in preferences and user fa…
LucasSloan Apr 22, 2014
1a2f01c
Merge branch 'realnames' of https://github.com/PotatoDumplings/lesswr…
LucasSloan Apr 22, 2014
bdba277
Allow real names to use full Unicode character set.
LucasSloan May 6, 2014
2dff67e
Removed unnecessary Account commit.
Jul 26, 2014
e3dea7f
Move realname error checking into a multiple boolean, not multiple if…
LucasSloan Dec 22, 2014
30b9399
Clear the realname field to remove a realname.
LucasSloan Jan 10, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion r2/r2/controllers/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,15 +577,32 @@ def POST_resendconfirmation(self, res):
VModhash(),
curpass = nop('curpass'),
email = ValidEmail("email"),
realname = VRname("real_name"),
newpass = nop("newpass"),
verpass = nop("verpass"),
password = VPassword(['newpass', 'verpass']))
def POST_update(self, res, email, curpass, password, newpass, verpass):
def POST_update(self, res, email, curpass, realname, password, newpass, verpass):
res._update('status', innerHTML='')
if res._chk_error(errors.WRONG_PASSWORD):
res._focus('curpass')
res._update('curpass', value='')
return

if res._chk_error(errors.BAD_REALNAME_CHARS):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are all going to have the same effect, so you could merge the conditions like:

if (res._chk_error(errors.one) or
    res._chk_error(errors.two) or
    res._chk_error(errors.three)):
  res._focus('real_name')

res._focus('real_name')
elif res._chk_error(errors.BAD_REALNAME_SHORT):
res._focus('real_name')
elif res._chk_error(errors.BAD_REALNAME_LONG):
res._focus('real_name')
if realname and realname == c.user.real_name:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong to me. I think you should remove minimum length limits on real names and make it so that an empty string is interpreted as a request to delete.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried that but it doesn't really work in this case - the update password/email/realname form handles 3 different actions - entering nothing should result in nothing happening. I suspect that people will want to remove their real name rarely enough to make the bizarre syntax acceptable, unless you have a better suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the best way to deal with this is to pre-fill the form with the user's current realname, and if they blank it out then delete it from their profile. If it's unchanged then we can infer that the form is being used for something else (password change etc.).

c.user.real_name = None
c.user._commit()
res._update('status', innerHTML=_('Your real name has been removed'))
elif realname:
c.user.real_name = realname
c.user._commit()
res._update('status', innerHTML=_('Your real name has been updated'))

updated = False
if res._chk_error(errors.BAD_EMAIL):
res._focus('email')
Expand Down
33 changes: 32 additions & 1 deletion r2/r2/controllers/validator/validator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# coding: utf8
# The contents of this file are subject to the Common Public Attribution
# License Version 1.0. (the "License"); you may not use this file except in
# compliance with the License. You may obtain a copy of the License at
Expand All @@ -23,7 +24,7 @@
from pylons.i18n import _
from pylons.controllers.util import abort
from r2.lib import utils, captcha
from r2.lib.filters import unkeep_space, websafe, _force_utf8, _force_ascii
from r2.lib.filters import unkeep_space, websafe, _force_utf8, _force_ascii, _force_unicode
from r2.lib.wikipagecached import WikiPageCached
from r2.lib.db.operators import asc, desc
from r2.config import cache
Expand Down Expand Up @@ -600,6 +601,36 @@ def run(self, user_name):
except NotFound:
return user_name

realname_rx = re.compile(ur"^[\w\s\-]{1,40}$", re.UNICODE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this needs to be compiled and run every time, but if it must I'd call it realname_re for consistency with the re module.


def chkrealname(x):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't match the pattern for function names in this codebase. Can you change it to (say) check_real_name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, these should be methods on the validator object.

try:
return x if realname_rx.match(x) else None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather use re.match(...) than an explicit pattern, unless there are big performance gains to be had here.

except TypeError:
return None
except UnicodeEncodeError:
return None

def whyrealnamebad(x):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would fold this stuff into (what will be called) check_real_name() and have it return the error message.

if not x:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under what circumstances does not x evaluate to True, and will this be triggered by accident if the user doen't put anything in the name field?

return errors.BAD_REALNAME_CHARS
if len(x)<1:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the above comments re:removing lower length limit, this could go away.

return errors.BAD_REALNAME_SHORT
if len(x)>40:
return errors.BAD_REALNAME_LONG
return errors.BAD_REALNAME_CHARS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any invalid characters any more? I think I remember asking for the regex to be much more permissive, so can this still happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we changed the regex to allow any unicode "word" characters, but given the role of this field as a real name, I think it still makes sense to exclude names like "&&&*()%#".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks for the reminder.


class VRname(VRequired):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VRealName, please.

def __init__(self, item, *a, **kw):
VRequired.__init__(self, item, errors.BAD_REALNAME, *a, **kw)
def run(self, real_name):
original_real_name = real_name
real_name = chkrealname(real_name)
if not real_name:
return self.error(whyrealnamebad(original_real_name))
else:
return real_name.encode('utf8')

class VLogin(VRequired):
def __init__(self, item, *a, **kw):
VRequired.__init__(self, item, errors.WRONG_PASSWORD, *a, **kw)
Expand Down
4 changes: 4 additions & 0 deletions r2/r2/lib/errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@
('BAD_USERNAME_SHORT', _('Username is too short')),
('BAD_USERNAME_LONG', _('Username is too long')),
('BAD_USERNAME_CHARS', _('Username may not contain special characters')),
('BAD_REALNAME', _('Invalid name')),
('BAD_REALNAME_SHORT', _('Name is too short')),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove any of these that don't apply.

('BAD_REALNAME_LONG', _('Name is too long')),
('BAD_REALNAME_CHARS', _('Name may not contain special characters')),
('USERNAME_TAKEN', _('That username is already taken')),
('NO_THING_ID', _('Id not specified')),
('NOT_AUTHOR', _("Only the author can do that")),
Expand Down
10 changes: 9 additions & 1 deletion r2/r2/models/account.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,18 @@ class Account(Thing):
messagebanned = False,
dashboard_visit = datetime(2006,10,1, tzinfo = g.tz),
wiki_association_attempted_at = None, # None or datetime
wiki_account = None # None, str(account name) or the special string '__taken__', if a new
wiki_account = None, # None, str(account name) or the special string '__taken__', if a new
# user didn't get an account because someone else already had the name.
real_name = None
)

@property
def printable_name(self):
if self.real_name:
return self.real_name + " [" + self.name + "]"
else:
return self.name

def karma_ups_downs(self, kind, sr = None):
# NOTE: There is a legacy inconsistency in this method. If no subreddit
# is specified, karma from all subreddits will be totaled, with each
Expand Down
2 changes: 1 addition & 1 deletion r2/r2/templates/articlenavigation.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
${_('Unknown')}
%else:
<% author_path = unsafe(add_sr("/user/%s/" % websafe(author.name), sr_path = False)) %>
<a href="${author_path}">${author.name}</a>
<a href="${author_path}">${author.printable_name}</a>
%endif
</%def>

Expand Down
4 changes: 2 additions & 2 deletions r2/r2/templates/award.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@
<div>
<h3>
<a href="http://${get_domain(cname=True, subreddit=False)}/user/${thing.author.name}}" class="reddit-link-title">
${thing.author.name}
${thing.author.printable_name}
</a>
</h3>
<span>gave&#32;<a href="http://${get_domain(cname=True, subreddit=False)}/user/${thing.recipient().name}">
${thing.recipient().name}</a>&#32;${thing.amount}&#32;
${thing.recipient().printable_name}</a>&#32;${thing.amount}&#32;
karma&#32;for&#32;${thing.reason}&#32;&#40;${prettytime(thing._date)}&#41;
</span>
</div>
Expand Down
2 changes: 1 addition & 1 deletion r2/r2/templates/award.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
<%
domain = get_domain(cname = c.cname, subreddit = False)
%>
Awarded by &lt;a href="http://${domain}/user/${thing.author.name}"&gt;${thing.author.name}&lt;/a&gt; to &lt;a href="http://${domain}/user/${thing.recipient().name}"&gt;${thing.recipient().name}&lt;/a&gt; for ${thing.reason}
Awarded by &lt;a href="http://${domain}/user/${thing.author.name}"&gt;${thing.author.printable_name}&lt;/a&gt; to &lt;a href="http://${domain}/user/${thing.recipient().name}"&gt;${thing.recipient().printable_name}&lt;/a&gt; for ${thing.reason}
</description>

</item>
Expand Down
2 changes: 1 addition & 1 deletion r2/r2/templates/comment.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
%>

<item>
<title>${thing.author.name} ${_("on")} ${thing.link.title}</title>
<title>${thing.author.printable_name} ${_("on")} ${thing.link.title}</title>
<link>${url}</link>
<guid isPermaLink="true">${url}</guid>
<dc:date>${thing._date.isoformat()}</dc:date>
Expand Down
4 changes: 2 additions & 2 deletions r2/r2/templates/edit.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
<pubDate>${rfc822format(thing._date)}</pubDate>
<description>
<% domain = get_domain(cname = c.cname, subreddit = False) %>
Edited by &lt;a href="http://${domain}/user/${thing.link_author.name}"&gt;${thing.author.name}&lt;/a&gt;
Original by &lt;a href="http://${domain}/user/${thing.link_author.name}"&gt;${thing.link_author.name}&lt;/a&gt;
Edited by &lt;a href="http://${domain}/user/${thing.link_author.name}"&gt;${thing.author.printable_name}&lt;/a&gt;
Original by &lt;a href="http://${domain}/user/${thing.link_author.name}"&gt;${thing.link_author.printable_name}&lt;/a&gt;
<% entities = {"\n": "&#x0a;"} %>
&lt;pre&gt;
${saxutils.escape("\n".join(thing.diff[2:]), entities)}
Expand Down
2 changes: 1 addition & 1 deletion r2/r2/templates/inlinearticle.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ <h3>
</a>
</h3>
<span>by&#32;<a href="http://${get_domain(cname=True, subreddit=False)}/user/${thing.author.name}">
${thing.author.name}</a>&#32;|&#32;
${thing.author.printable_name}</a>&#32;|&#32;
${thing.score_fmt(thing.score)['label']}v&#32;(${thing.num_comments}c)
</span>
</div>
Expand Down
2 changes: 1 addition & 1 deletion r2/r2/templates/inlinearticle.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
<pubDate>${rfc822format(thing._date)}</pubDate>
<description>
<% domain = get_domain(cname = c.cname, subreddit = False) %>
Submitted by &lt;a href="http://${domain}/user/${thing.author.name}"&gt;${thing.author.name}&lt;/a&gt;
Submitted by &lt;a href="http://${domain}/user/${thing.author.name}"&gt;${thing.author.printable_name}&lt;/a&gt;
&lt;a href="${url}#comments"&gt;${thing.num_comments} ${com_label}&lt;/a&gt;
</description>
%if use_thumbs:
Expand Down
2 changes: 1 addition & 1 deletion r2/r2/templates/inlinecomment.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
<h3><a href="${thing.make_anchored_permalink()}">${killhtml(thing.body)}</a></h3>
<span class='tagline'>
by&#32;<a href="http://${get_domain(cname=True, subreddit=False)}/user/${thing.author.name}">
<strong>${thing.author.name}</strong></a>&#32;
<strong>${thing.author.printable_name}</strong></a>&#32;
on&#32;
${thing.link.title}&#32;|&#32;
<span id="score_${thing._fullname}">${thing.score_fmt(thing.score)['label']}</span>
Expand Down
2 changes: 1 addition & 1 deletion r2/r2/templates/link.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
votes = thing.score_fmt(thing.score)['label']
votes_lbl = ungettext("vote", "votes", votes)
%>
Submitted by &lt;a href="http://${domain}/user/${thing.author.name}"&gt;${thing.author.name}&lt;/a&gt;
Submitted by &lt;a href="http://${domain}/user/${thing.author.name}"&gt;${thing.author.printable_name}&lt;/a&gt;
&amp;bull;
${votes} ${votes_lbl}
&amp;bull;
Expand Down
9 changes: 9 additions & 0 deletions r2/r2/templates/prefupdate.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,15 @@
${error_field("NO_EMAIL")}
</td>
</tr>
<tr>
<td align="right">${_("Real Name")}:</td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make it clear that this field is optional and will be shown publicly. Also, should it be added to the registration form?

<td><input name="real_name" type="text" value="${hasattr(c.user,'real_name') and c.user.real_name or ''}" title="Enter your current real name to remove it"/></td>
<td>
${error_field("BAD_REALNAME_CHARS")}
${error_field("BAD_REALNAME_SHORT")}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you find that you can remove any of these error messages elsewhere they'll have to be removed from here too.

${error_field("BAD_REALNAME_LONG")}
</td>
</tr>
<tr>
<td align="right">${_("New password")}:</td>
<td><input name="newpass" type="password" maxlength="20"/></td>
Expand Down
2 changes: 1 addition & 1 deletion r2/r2/templates/printable.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@
author_cls += " friend"
elif gray:
author_cls += " gray"
name = websafe(author.name)
name = websafe(author.printable_name)
href = unsafe('href="%s"' % add_sr("/user/%s/" % name, sr_path = False))
if c.user_is_admin: name += " (%d)" % (author.link_karma)
%>
Expand Down
2 changes: 1 addition & 1 deletion r2/r2/templates/profilebar.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
%if thing.user:
<div id="side-user" class="sidebox">
<div id="side-status" class="clear">
<h2><a href="/user/${thing.user.name}">${thing.user.name}</a></h2>
<h2><a href="/user/${thing.user.name}">${thing.user.printable_name}</a></h2>
%if thing.buttons:
${thing.buttons.render()}
%endif
Expand Down
2 changes: 1 addition & 1 deletion r2/r2/templates/showmeetup.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ <h1>
%else:
<%
author = meetup.author()
name = websafe(author.name)
name = websafe(author.printable_name)
href = unsafe('href="%s"' % "/user/%s/" % name)
if c.user_is_admin: name += " (%d)" % (author.link_karma)
%>
Expand Down
2 changes: 1 addition & 1 deletion r2/r2/templates/topcontributors.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ <h2>${_('Top Contributors, All Time')}:</h2>
<div class="contributors">
%for user in thing.things:
<div class="user">
<a href="/user/${user.name}">${user.name}</a>&#32;
<a href="/user/${user.name}">${user.printable_name}</a>&#32;
(<b>${user.safe_karma}</b>)
</div>
%endfor
Expand Down
2 changes: 1 addition & 1 deletion r2/r2/templates/topmonthlycontributors.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ <h2>${_('Top Contributors, 30 Days')}</h2>
<div class="contributors">
%for user in thing.things:
<div class="user">
<a href="/user/${user.name}">${user.name}</a>&#32;
<a href="/user/${user.name}">${user.printable_name}</a>&#32;
(<b>${user.monthly_karma}</b>)
</div>
%endfor
Expand Down
2 changes: 1 addition & 1 deletion r2/r2/templates/usertableitem.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
<%def name="cell_type(name)">
%if name == "user":
<span class="user">
${plain_link(thing.user.name, "/user/%s/" % thing.user.name, _sr_path=False)}
${plain_link(thing.user.printable_name, "/user/%s/" % thing.user.name, _sr_path=False)}
&nbsp;(<b>${thing.user.safe_karma}</b>)
</span>&nbsp;
%elif c.user_is_loggedin and name == "sendmessage" and c.user != thing.user:
Expand Down
2 changes: 1 addition & 1 deletion tasks/manual_test_script
Open in desktop
Submodule manual_test_script updated from c2d154 to a49ad9