Skip to content
This repository was archived by the owner on Apr 17, 2018. It is now read-only.
Open
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
Changed move proceedure to no longer create new comments. All calls t…
…o comment.parent_id now safed.
  • Loading branch information
LucasSloan committed Jul 27, 2013
commit 582d9fd6b2900f99500c39abe757afe8445e4fa9
42 changes: 26 additions & 16 deletions r2/r2/controllers/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -203,9 +203,10 @@ def POST_verifyemail(self, res, code):
@validate(VModhash(),
VSrCanBan('id'),
thing = VByName('id'),
ip = ValidIP(),
destination = VMoveURL('destination'),
reason = VComment('comment'))
def POST_move(self, res, thing, destination, reason):
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):
Expand All @@ -216,25 +217,34 @@ def POST_move(self, res, thing, destination, reason):
res._focus("comment_replacement_" + thing._fullname)
return

from r2.lib.comment_tree import lock_key
print Link._byID(thing.link_id).title
currlink = Link._byID(thing.link_id)
if hasattr(thing, 'parent_id'):
parent = Comment._byID(thing.parent_id)
else:
parent = None

from r2.lib.comment_tree import lock_key, comments_key

with g.make_lock(lock_key(thing.link_id)):
comment, inbox_rel = Comment._new(Account._byID(thing.author_id),
destination, None, thing.body,
thing.ip)
children = list(Comment._query(Comment.c.parent_id == thing._id))
for child in children:
child.recursive_move(destination, comment)

thing.moved = True
thing.set_body('This comment was moved by ' + c.user.name +
' to [here]({0}).\n\n'.format(comment.make_anchored_permalink(destination)) +
(reason if reason else ''))
thing.recursive_move(currlink, destination, True)

if g.write_query_queue:
queries.new_comment(comment, None)
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])

res._send_things(thing)
print Link._byID(thing.link_id).title

@Json
@validate(VCaptcha(),
Expand Down
2 changes: 1 addition & 1 deletion r2/r2/controllers/front.py
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
10 changes: 8 additions & 2 deletions r2/r2/lib/jsontemplates.py
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: 6 additions & 3 deletions r2/r2/models/builder.py
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
57 changes: 39 additions & 18 deletions r2/r2/models/link.py
Original file line number Diff line number Diff line change
Expand Up @@ -923,6 +923,39 @@ def _new(cls, author, link, parent, body, ip, spam = False, date = None):

return (comment, inbox_rel)

@classmethod
def _move(cls, comment, currlink, newlink, top=False):
author = Account._byID(comment.author_id)

if top:
if hasattr(comment, 'parent_id'):
comment.parent_id = None
comment.link_id = newlink._id
comment.sr_id = newlink.sr_id

comment._commit()

if not top:
parent = Comment._byID(comment.parent_id)
comment.parent_permalink = parent.make_anchored_permalink(Link._byID(parent.link_id), Subreddit._byID(parent.sr_id))
comment.permalink = comment.make_permalink_slow()
comment._commit()
Copy link
Contributor

Choose a reason for hiding this comment

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

You commit the comment twice. Can one of these be avoided?

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, changed.


currlink._incr('num_comments', -1)
newlink._incr('num_comments', 1)

#clear that chache
clear_memo('builder.link_comments2', newlink._id)
clear_memo('builder.link_comments2', currlink._id)

# flag search indexer that something has changed
tc.changed(comment)

#update last modified
set_last_modified(author, 'overview')
set_last_modified(author, 'commented')
set_last_modified(newlink, 'comments')

def try_parent(self, func, default):
"""
If this comment has a parent, return `func(parent)`; otherwise
Expand Down Expand Up @@ -956,29 +989,17 @@ def has_children(self):
child = list(q)
return len(child)>0

def recursive_move(self, destination, parent):
def recursive_move(self, origin, destination, top=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to infer origin from self somehow and avoid passing in an extra parameter?

What are the types of origin (if it's required) and destination, and can you document that in a docstring?

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 think we could, but then we'd needlessly increase the number of calls to query. I think that this way is better.

q = Comment._query(Comment.c.parent_id == self._id)
children = list(q)
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 Query defines an __iter__ method, so you don't actually need to listify the result before iterating over it. I think you could do this:

children = Comment._query(...)
.
.
.
for child in children:
   ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

comment, inbox_rel = Comment._new(Account._byID(self.author_id),
destination, parent, self.body,
self.ip)

Comment._move(self, origin, destination, top)

if not children:
pass
else:
for child in children:
child.recursive_move(destination, comment)


self.moderator_banned = not c.user_is_admin
self.banner = c.user.name
self.moved = True
self._commit()
# NB: change table updated by reporting
from r2.models.report import unreport
unreport(self, correct=True, auto=False)

if g.write_query_queue:
queries.new_comment(comment, None)
child.recursive_move(origin, destination)

def can_delete(self):
if not self._loaded:
Expand Down Expand Up @@ -1105,7 +1126,7 @@ def add_props(cls, user, wrapped):
item.link = links.get(item.link_id)
if not hasattr(item, 'subreddit'):
item.subreddit = item.subreddit_slow
if hasattr(item, 'parent_id'):
if hasattr(item, 'parent_id') and item.parent_id:
parent = Comment._byID(item.parent_id, data=True)
parent_author = Account._byID(parent.author_id, data=True)
item.parent_author = parent_author
Expand Down
10 changes: 4 additions & 6 deletions r2/r2/public/static/comments.js
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,6 @@ Comment.prototype.cancel = function() {
Comment.prototype.cancelmove = function() {
var edit_box = this.cloneAndReIDNodeOnce("samplemove");
hide(edit_box);
BeforeUnload.unbind(Comment.checkModified, this._id);
this.show();
};

Expand All @@ -195,7 +194,6 @@ Comment.comment = function(r, context) {
var id = r.id;
var parent_id = r.parent;
new Comment(parent_id, context).cancel();
new Comment(parent_id, context).cancelmove();
new Listing(parent_id, context).push(unsafe(r.content));
new Comment(r.id, context).show();
vl[id] = r.vl;
Expand Down Expand Up @@ -231,8 +229,8 @@ Comment.editcomment = function(r, context) {
com.show();
};

Comment.move = function(r, context) {
var com = new Comment(r.id, context);
Comment.move = function(r, s, context) {
var com = new Comment(s.id, context);
com.get('body').innerHTML = unsafe(r.contentHTML);
com.get('edit_body').innerHTML = unsafe(r.contentTxt);
com.cancelmove();
Expand Down Expand Up @@ -393,8 +391,8 @@ function chkmove(form) {

var obj = res_obj && res_obj.response && res_obj.response.object;
if (obj && obj.length)
for (var o = 0, ol = obj.length; o < ol; ++o)
Comment[action](obj[o].data, context);
for (var o = 0, ol = obj.length; o < ol; o += 2)
Comment[action](obj[o].data, obj[o+1].data, context);
}

return post_form(form, action, null, null, true, null, {
Expand Down
2 changes: 1 addition & 1 deletion r2/r2/templates/comment.html
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@
</%def>

<%def name="Child()">
${parent.Child(not (thing.collapsed or ((not c.user_is_admin) and thing.moved)), not ((not c.user_is_admin) and thing.moved))}
${parent.Child(not thing.collapsed)}
</%def>

<%def name="commentBody()">
Expand Down
22 changes: 22 additions & 0 deletions r2/r2/templates/movebox.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
## 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
## http://code.reddit.com/LICENSE. The License is based on the Mozilla Public
## License Version 1.1, but Sections 14 and 15 have been added to cover use of
## software over a computer network and provide for limited attribution for the
## Original Developer. In addition, Exhibit A has been modified to be consistent
## with Exhibit B.
##
## Software distributed under the License is distributed on an "AS IS" basis,
## WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License for
## the specific language governing rights and limitations under the License.
##
## The Original Code is Reddit.
##
## The Original Developer is the Initial Developer. The Initial Developer of
## the Original Code is CondeNet, Inc.
##
## All portions of the code written by CondeNet are Copyright (c) 2006-2008
## CondeNet, Inc. All Rights Reserved.
################################################################################