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 6 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
20 changes: 19 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 All @@ -594,6 +611,7 @@ def POST_update(self, res, email, curpass, password, newpass, verpass):
elif email and (not hasattr(c.user,'email')
or c.user.email != email):
c.user.email = email
c.user._commit()
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't required because the self.send_confirmation() call will commit anyway.

self.send_confirmation()
res._update('status',
innerHTML=_('Your email has been updated. You will have to confirm before commenting or posting.'))
Expand Down
30 changes: 30 additions & 0 deletions r2/r2/controllers/validator/validator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,36 @@ def run(self, user_name):
except NotFound:
return user_name

realname_rx = re.compile(r"^[a-zA-Z\s\-]{3,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.

Hi Lucas,

I think this regex excludes many valid names. Personally I know someone with é in their name and another with ä in their surname. Can you please tweak to allow non-ASCII characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

On Thu, May 1, 2014 at 5:25 PM, Wesley Moore [email protected]:

In r2/r2/controllers/validator/validator.py:

@@ -600,6 +600,36 @@ def run(self, user_name):
except NotFound:
return user_name

+realname_rx = re.compile(r"^[a-zA-Z\s-]{3,40}$", re.UNICODE)

Hi Lucas,

I this regex excludes many valid names. Personally I know someone with é
in their name and another with ä in their surname. Can you please tweak to
allow non-ASCII characters.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/96/files#r12214902
.

"And someday when the descendants of humanity have spread from star to
star, they won't tell the children about the history of Ancient Earth until
they're old enough to bear it; and when they learn they'll weep to hear
that such a thing as Death had ever once existed!"

-Eliezer Yudkowsky

"Trust me" means "I love you" http://project-apollo.net/mos/mos114.html

"This isn't even my final form!"

-Tim Hausler


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 str(x) if realname_rx.match(x) else None
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)<3:
Copy link
Contributor

Choose a reason for hiding this comment

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

And here too.

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

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