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

Postmovement #80

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

Postmovement #80

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
03bae0d
Moderators can now move comment threads between posts.
LucasSloan Jul 23, 2013
a95fa1a
Children of original movers now hidden for non-admins.
LucasSloan Jul 23, 2013
c7fc246
Made it impossible to vote on moved items.
LucasSloan Jul 23, 2013
2739bef
Status message removed in the event of failed call.
LucasSloan Jul 23, 2013
eefd50e
Entering an properly formatted URL that doesn't lead to a post sends …
LucasSloan Jul 23, 2013
f3721e5
Changed movebox doc string, renamed Link._byURL to Link._move_url.
LucasSloan Jul 24, 2013
afe23b1
Added a lock to movement.
LucasSloan Jul 24, 2013
582d9fd
Changed move proceedure to no longer create new comments. All calls …
LucasSloan Jul 27, 2013
ac9ed72
Removed debugging statements, moved boolean.
LucasSloan Jul 27, 2013
fa13a33
Safed movement against editor trying to move children of already move…
LucasSloan Jul 27, 2013
b7e7355
Merge branch 'interesting_comments' of https://github.com/PotatoDumpl…
LucasSloan Sep 21, 2013
91d0c4e
Descendant karma attribute no longer disappears off python objects.
LucasSloan Oct 1, 2013
bedd944
Merge branch 'interesting_comments' of https://github.com/PotatoDumpl…
LucasSloan Oct 1, 2013
cf10a31
Thread movement now properly increments descendant karma.
LucasSloan Oct 1, 2013
2edef55
Descendant karma attribute no longer disappears off python objects.
LucasSloan Oct 1, 2013
33a3850
Merge branch 'interesting_comments' of https://github.com/PotatoDumpl…
LucasSloan Oct 1, 2013
2124bd7
Changed testing syntax.
LucasSloan Dec 9, 2013
9bdf974
Merge branch 'postmovement' of https://github.com/PotatoDumplings/les…
LucasSloan Dec 9, 2013
ce3d6ff
Merge branch 'master' of https://github.com/tricycle/lesswrong into p…
LucasSloan Dec 9, 2013
5e5f092
Variety of small changes for readability.
LucasSloan Dec 9, 2013
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
54 changes: 53 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 @@ -23,7 +23,7 @@
from reddit_base import RedditController

from pylons.i18n import _
from pylons import c, request, response
from pylons import c, request, response, g
from pylons.controllers.util import etag_cache

import hashlib
Expand Down Expand Up @@ -199,6 +199,58 @@ def POST_verifyemail(self, res, code):
c.user._commit()
res._success()

@Json
@validate(VModhash(),
VSrCanBan('id'),
thing = VByName('id'),
ip = ValidIP(),
destination = VMoveURL('destination'),
reason = VComment('comment'))
def POST_move(self, res, thing, destination, reason, ip):
res._update('status_' + thing._fullname, innerHTML = '')
if res._chk_errors((errors.NO_URL, errors.BAD_URL),
thing._fullname):
res._focus("destination_url_" + thing._fullname)
return
if res._chk_error(errors.COMMENT_TOO_LONG,
thing._fullname):
res._focus("comment_replacement_" + thing._fullname)
return
if destination._id == thing.link_id:
c.errors.add(errors.ALREADY_MOVED)
res._chk_error(errors.ALREADY_MOVED, thing._fullname)
res._focus("destination_url_" + thing._fullname)
return

currlink = Link._byID(thing.link_id)
currlink._incr('_descendant_karma', -(thing._descendant_karma + thing._ups - thing._downs))
destination._incr('_descendant_karma', thing._descendant_karma + thing._ups - thing._downs)
if hasattr(thing, 'parent_id'):
parent = Comment._byID(thing.parent_id)
parent.incr_descendant_karma([], -(thing._descendant_karma + thing._ups - thing._downs))
Copy link
Contributor

Choose a reason for hiding this comment

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

Pylons gives me an error here when I try to move a comment thread to a different article:

Module r2.lib.jsonresponse:181 in _Json        
>>  val = func(self, res, *a, **kw)
Module r2.controllers.validator.validator:78 in newfn        
>>  return fn(self, *a, **kw)
Module r2.controllers.api:229 in POST_move        
>>  currlink._incr('_descendant_karma', -(thing._descendant_karma + thing._ups - thing._downs))
Module r2.lib.db.thing:497 in __getattr__        
>>  return DataThing.__getattr__(self, attr)
Module r2.lib.db.thing:124 in __getattr__        
>>  raise AttributeError, '%s not found' % attr
<type 'exceptions.AttributeError'>: _descendant_karma not found

I had created a couple of articles while on the master branch, then switched to the postmovement branch and tried to move some comments around and got this error. Perhaps it is relying on something that is not being set by current (in production) code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have to run a script in the sql folder to add descendant karma to the SQL database.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, you need #94.

else:
parent = None

from r2.lib.comment_tree import lock_key, comments_key

with g.make_lock(lock_key(thing.link_id)):
thing.recursive_move(currlink, destination, True)

g.permacache.delete(comments_key(currlink._id))
g.permacache.delete(comments_key(destination._id))

body = "A comment was moved from here to [here]({0}).\n\n".format(thing.make_anchored_permalink(destination)) + (reason if reason else '')
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 "A comment was moved to here." will be a bit clearer. In its current form, you have "here" referring to both the current location and the new location, which is a little confusing.

Also, is it possible to hide the children of the moved comment as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are hidden, you just have to refresh.


comment, inbox_rel = Comment._new(c.user,
currlink, parent, body,
ip)


if g.write_query_queue:
queries.new_comment(comment, None)

res._send_things([comment, thing])

@Json
@validate(VCaptcha(),
VUser(),
Expand Down
2 changes: 1 addition & 1 deletion r2/r2/controllers/front.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ def GET_comments(self, article, comment, context, sort, num_comments):
context = context + 1 if context else 1,
anchor = 'comments'
),
has_more_comments = hasattr(comment, 'parent_id')
has_more_comments = hasattr(comment, 'parent_id') and comment.parent_id
)
displayPane.append(permamessage)

Expand Down
14 changes: 14 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 @@ -377,6 +377,20 @@ def run(self, name):
except NotFound:
return name

class VMoveURL(VRequired):
def __init__(self, item, *a, **kw):
VRequired.__init__(self, item, errors.NO_URL, *a, **kw)

def run(self, url):
if not url:
return self.error()
else:
link = Link._move_url(url)
if not link:
return self.error(errors.BAD_URL)
else:
return link

class VSubredditTitle(Validator):
def run(self, title):
if not title:
Expand Down
86 changes: 57 additions & 29 deletions r2/r2/lib/db/tdb_sql.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,30 +129,50 @@ def get_rel_type_table(metadata):


def get_thing_table(metadata, name):
table = sa.Table(settings.DB_APP_NAME + '_thing_' + name, metadata,
sa.Column('thing_id', BigInteger, primary_key = True),
sa.Column('ups', sa.Integer, default = 0, nullable = False),
sa.Column('downs',
sa.Integer,
default = 0,
nullable = False),
sa.Column('deleted',
sa.Boolean,
default = False,
nullable = False),
sa.Column('spam',
sa.Boolean,
default = False,
nullable = False),
sa.Column('date',
sa.DateTime(timezone = True),
default = sa.func.now(),
nullable = False))
if name in ('comment', 'link'):
table.append_column(sa.Column('descendant_karma',
sa.Integer,
default = 0,
nullable = False))
if name not in ('comment', 'link'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you've caused a fair bit of duplication here. Why is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just adding the column doesn't work.

table = sa.Table(settings.DB_APP_NAME + '_thing_' + name, metadata,
sa.Column('thing_id', BigInteger, primary_key = True),
sa.Column('ups', sa.Integer, default = 0, nullable = False),
sa.Column('downs',
sa.Integer,
default = 0,
nullable = False),
sa.Column('deleted',
sa.Boolean,
default = False,
nullable = False),
sa.Column('spam',
sa.Boolean,
default = False,
nullable = False),
sa.Column('date',
sa.DateTime(timezone = True),
default = sa.func.now(),
nullable = False))
else:
table = sa.Table(settings.DB_APP_NAME + '_thing_' + name, metadata,
sa.Column('thing_id', BigInteger, primary_key = True),
sa.Column('ups', sa.Integer, default = 0, nullable = False),
sa.Column('downs',
sa.Integer,
default = 0,
nullable = False),
sa.Column('deleted',
sa.Boolean,
default = False,
nullable = False),
sa.Column('spam',
sa.Boolean,
default = False,
nullable = False),
sa.Column('date',
sa.DateTime(timezone = True),
default = sa.func.now(),
nullable = False),
sa.Column('descendant_karma',
sa.Integer,
default = 0,
nullable = False))

return table

Expand Down Expand Up @@ -542,11 +562,19 @@ def get_thing(type_id, thing_id):
#if single, only return one storage, otherwise make a dict
res = {} if not single else None
for row in r:
stor = storage(ups = row.ups,
downs = row.downs,
date = row.date,
deleted = row.deleted,
spam = row.spam)
if type_id in (1, 7):
Copy link
Contributor

Choose a reason for hiding this comment

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

What are types 1 and 7, and is it possible to write this without resorting to magic numbers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment and Link. Maybe... could ask the comment and link classes for their type ids.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it turns out yes. Changed.

stor = storage(ups = row.ups,
downs = row.downs,
date = row.date,
deleted = row.deleted,
spam = row.spam,
descendant_karma = row.descendant_karma)
else:
stor = storage(ups = row.ups,
downs = row.downs,
date = row.date,
deleted = row.deleted,
spam = row.spam)
if single:
res = stor
else:
Expand Down
1 change: 1 addition & 0 deletions r2/r2/lib/errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

error_list = dict((
('NO_URL', _('Url required')),
('ALREADY_MOVED', _('This comment is already at that destination')),
('BAD_URL', _('You should check that url')),
('NO_TITLE', _('Title required')),
('TITLE_TOO_LONG', _('Title too long')),
Expand Down
10 changes: 8 additions & 2 deletions r2/r2/lib/jsontemplates.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,10 @@ def thing_attr(self, thing, attr):
return make_fullname(Link, thing.link_id)
elif attr == "parent_id":
try:
return make_fullname(Comment, thing.parent_id)
if thing.parent_id:
return make_fullname(Comment, thing.parent_id)
else:
return make_fullname(Link, thing.link_id)
except AttributeError:
return make_fullname(Link, thing.link_id)
return ThingJsonTemplate.thing_attr(self, thing, attr)
Expand All @@ -184,7 +187,10 @@ def rendered_data(self, wrapped):
except AttributeError:
parent_id = make_fullname(Link, wrapped.link_id)
else:
parent_id = make_fullname(Comment, parent_id)
if parent_id:
parent_id = make_fullname(Comment, parent_id)
else:
parent_id = make_fullname(Link, wrapped.link_id)
d = ThingJsonTemplate.rendered_data(self, wrapped)
d.update(mass_part_render(wrapped, contentHTML = 'commentBody',
contentTxt = 'commentText'))
Expand Down
9 changes: 8 additions & 1 deletion r2/r2/lib/pages/pages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,7 @@ def __init__(self, link = None, comment = None,

# link is a wrapped Link object
self.link = self.link_listing.things[0]
self.movebox = MoveBox()

link_title = ((self.link.title) if hasattr(self.link, 'title') else '')
if comment:
Expand All @@ -665,7 +666,7 @@ def __init__(self, link = None, comment = None,
Reddit.__init__(self, title = title, body_class = 'post', robots = self.robots, *a, **kw)

def content(self):
return self.content_stack(self.infobar, self.link_listing, self._content)
return self.content_stack(self.infobar, self.link_listing, self.movebox, self._content)

def build_toolbars(self):
return []
Expand Down Expand Up @@ -989,6 +990,12 @@ def __init__(self, link_name='', captcha=None, action = 'comment'):
Wrapped.__init__(self, link_name = link_name, captcha = captcha,
action = action)

class MoveBox(Wrapped):
"""Used on LinkInfoPage to render the move thread form."""
def __init__(self, link_name='', captcha=None, action = 'comment'):
Wrapped.__init__(self, link_name = link_name, captcha = captcha,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the default action for a move thread form "comment"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason. Holdover from the initialization for what the movebox was created from. I'll remove it.

action = action)

class CommentListing(Wrapped):
"""Comment heading and sort, limit options"""
def __init__(self, content, num_comments, nav_menus = []):
Expand Down
9 changes: 6 additions & 3 deletions r2/r2/models/builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,10 @@ def keep_item(self, item):
except AttributeError:
return True
else:
return False
if item.parent_id:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a really awkward way to write the function. You don't need the else block. Just save the result of the first access of item.parent_id and do the test after that:

try:
    parent_id = item.parent_id
except AttributeError:
    return True

return parent_id is not None

return False
else:
return True

class ContextualCommentBuilder(CommentBuilderMixin, UnbannedCommentBuilder):
def __init__(self, query, sr_ids, **kw):
Expand Down Expand Up @@ -574,7 +577,7 @@ def get_items(self, num, nested = True, starting_depth = 0):
top = self.comment
dont_collapse.append(top._id)
#add parents for context
while self.context > 0 and hasattr(top, 'parent_id'):
while self.context > 0 and hasattr(top, 'parent_id') and top.parent_id:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to see tests for ... is not None, in case that top.parent_id evaluates to a falsy value.

self.context -= 1
new_top = comment_dict[top.parent_id]
comment_tree[new_top._id] = [top]
Expand Down Expand Up @@ -657,7 +660,7 @@ def sort_candidates():
to_add = candidates.pop(0)
direct_child = True
#ignore top-level comments for now
if not hasattr(to_add, 'parent_id'):
if not hasattr(to_add, 'parent_id') or not to_add.parent_id:
Copy link
Contributor

Choose a reason for hiding this comment

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

As above (favour is not None over not foo is None).

p_id = None
else:
#find the parent actually being displayed
Expand Down
Loading