Postmovement #80

LucasSloan · 2013-07-23T21:58:19Z

http://code.google.com/p/lesswrong/issues/detail?id=384&sort=-id&colspec=ID%20Estimate%20Type%20Status%20Priority%20Milestone%20Owner%20Summary%20Contributions

Allows moderators to move comment threads from one post to another.

…an error message instead of crashing.

wezm · 2013-07-24T05:05:38Z

r2/r2/models/link.py

+
+        self.moderator_banned = not c.user_is_admin
+        self.banner = c.user.name
+        #self.moved = True


Should this be commented out?

…o comment.parent_id now safed.

…d comment.

LucasSloan · 2013-07-29T22:04:07Z

The changes you suggested are implemented.

keyist · 2013-09-06T01:25:53Z

Now that 'interestingness' is live, moving posts also needs to change descendant_karma accordingly

LucasSloan · 2013-09-06T06:41:49Z

Moving posts?

On Thu, Sep 5, 2013 at 6:25 PM, keyist notifications@github.com wrote:

Now that 'interestingness' is live, moving posts also needs to change
descendant_karma accordingly

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/80#issuecomment-23913411
.

"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

keyist · 2013-09-16T06:19:08Z

Sorry, meant moving comments between posts, i.e. when a comment is moved, descendant_karma of {source,destination} parent need to be updated

LucasSloan · 2013-09-16T19:41:54Z

Hmm. My working theory was that if the original context caused a
discussion of merit or demerit, then it's descendant_karma should reflect
that.

On Sun, Sep 15, 2013 at 11:19 PM, keyist notifications@github.com wrote:

Sorry, meant moving comments between posts, i.e. when a comment is moved,
descendant_karma of {source,destination} parent need to be updated

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/80#issuecomment-24490715
.

"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

keyist · 2013-09-19T07:02:15Z

It boils down to whether we want descendant_karma to have a precise meaning of "sum of present descendants' karma" or a vague meaning -- present descendants' karma + potentially vacated descendants' karma - potentially newly added descendants' karma.

I feel the latter should be avoided: deliberately storing imprecise values makes any functionality that exercises that data suspect. Sorting by descendant_karma will no longer be reliable for threads that have moved.

If you think the impact on the original context has value, then we should track both values separately (descendant_karma, accumulated_descendant_karma).

LucasSloan · 2013-09-19T16:36:56Z

Ok, that definitely makes sense. I'll have the change up by (your) monday.

On Thu, Sep 19, 2013 at 12:02 AM, keyist notifications@github.com wrote:

It boils down to whether we want descendant_karma to have a precise
meaning of "sum of present descendants' karma" or a vague meaning --
present descendants' karma + potentially vacated descendants' karma -
potentially newly added descendants' karma.

I feel the latter should be avoided: deliberately storing imprecise values
makes any functionality that exercises that data suspect. Sorting by
descendant_karma will no longer be reliable for threads that have moved.

If you think the impact on the original context has value, then we should
track both values separately (descendant_karma,
accumulated_descendant_karma).

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/80#issuecomment-24720553
.

"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

…ings/lesswrong into postmovement Conflicts: r2/r2/templates/comment.html

LucasSloan · 2013-09-22T23:51:50Z

Sorry, this is going to take more time. I've come across an issue with how
descendant karma is stored.

On Thu, Sep 19, 2013 at 9:36 AM, Lucas Sloan <
predictably.defenestrated@gmail.com> wrote:

Ok, that definitely makes sense. I'll have the change up by (your) monday.

On Thu, Sep 19, 2013 at 12:02 AM, keyist notifications@github.com wrote:

It boils down to whether we want descendant_karma to have a precise
meaning of "sum of present descendants' karma" or a vague meaning --
present descendants' karma + potentially vacated descendants' karma -
potentially newly added descendants' karma.

I feel the latter should be avoided: deliberately storing imprecise
values makes any functionality that exercises that data suspect. Sorting by
descendant_karma will no longer be reliable for threads that have moved.

If you think the impact on the original context has value, then we should
track both values separately (descendant_karma,
accumulated_descendant_karma).

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/80#issuecomment-24720553
.

"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

"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

…ings/lesswrong into postmovement

…ings/lesswrong into postmovement Conflicts: r2/r2/models/link.py

jack-trikeapps · 2013-12-02T04:53:28Z

r2/r2/lib/db/tdb_sql.py

@@ -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,


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

Just adding the column doesn't work.

jack-trikeapps · 2013-12-02T04:55:13Z

r2/r2/lib/pages/pages.py

+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,


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

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

jack-trikeapps · 2013-12-02T22:20:32Z

I think I'm still getting my head into the LW codebase and it means I'm stuck code-reviewing at the nitpick-level. I'll pull this into a private branch and have a play with it.

jack-trikeapps · 2013-12-02T23:52:05Z

r2/r2/controllers/api.py

+        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))


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?

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

Also, you need #94.

jack-trikeapps · 2013-12-03T00:44:24Z

I can't seem to move a comment tree under another comment. It will always put the comment tree at the top level:

Suppose on article A1 I have the following comment tree:

A1
 |
 `-B
   |
   `-C

And I have this comment tree on article A2:

A2
 |
 `-D

I moved comment B, giving it the URL of comment D. I expected this sort of tree:

A2
 |
 `-D
   |
   `-B
     |
     `-C

But it made this:

A2
 |
 |-D
 |
 `-B
   |
   `-C

Is this a bug or am I misunderstanding the feature request?

jack-trikeapps · 2013-12-03T00:45:09Z

Also, I am seeing this error occasionally but I can't reliably reproduce it yet. Any ideas:

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:232 in POST_move        
>>  parent = Comment._byID(thing.parent_id)
Module r2.lib.db.thing:282 in _byID        
>>  raise NotFound, '%s %s' % (cls.__name__, missing)
<class 'r2.lib.db.thing.NotFound'>: Comment [None]

EDIT: Somehow I have comments with parent_id == None. Maybe it's worth setting a default parent_id of None in the Comment class? (link.py:918)

…swrong into postmovement

…ostmovement

LucasSloan added 5 commits July 22, 2013 20:56

Moderators can now move comment threads between posts.

03bae0d

Children of original movers now hidden for non-admins.

a95fa1a

Made it impossible to vote on moved items.

c7fc246

Status message removed in the event of failed call.

2739bef

Entering an properly formatted URL that doesn't lead to a post sends …

eefd50e

…an error message instead of crashing.

wezm reviewed Jul 24, 2013
View reviewed changes

LucasSloan added 5 commits July 23, 2013 22:45

Changed movebox doc string, renamed Link._byURL to Link._move_url.

f3721e5

Added a lock to movement.

afe23b1

Changed move proceedure to no longer create new comments. All calls t…

582d9fd

…o comment.parent_id now safed.

Removed debugging statements, moved boolean.

ac9ed72

Safed movement against editor trying to move children of already move…

fa13a33

…d comment.

Merge branch 'interesting_comments' of https://github.com/PotatoDumpl…

b7e7355

…ings/lesswrong into postmovement Conflicts: r2/r2/templates/comment.html

LucasSloan added 5 commits October 1, 2013 16:19

Descendant karma attribute no longer disappears off python objects.

91d0c4e

Merge branch 'interesting_comments' of https://github.com/PotatoDumpl…

bedd944

…ings/lesswrong into postmovement

Thread movement now properly increments descendant karma.

cf10a31

Descendant karma attribute no longer disappears off python objects.

2edef55

Merge branch 'interesting_comments' of https://github.com/PotatoDumpl…

33a3850

…ings/lesswrong into postmovement Conflicts: r2/r2/models/link.py

jack-trikeapps reviewed Dec 2, 2013
View reviewed changes

LucasSloan added 4 commits December 8, 2013 17:22

Changed testing syntax.

2124bd7

Merge branch 'postmovement' of https://github.com/PotatoDumplings/les…

9bdf974

…swrong into postmovement

Merge branch 'master' of https://github.com/tricycle/lesswrong into p…

ce3d6ff

…ostmovement

Variety of small changes for readability.

5e5f092

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Postmovement #80

Postmovement #80

LucasSloan commented Jul 23, 2013

wezm Jul 24, 2013

LucasSloan commented Jul 29, 2013

keyist commented Sep 6, 2013

LucasSloan commented Sep 6, 2013

keyist commented Sep 16, 2013

LucasSloan commented Sep 16, 2013

keyist commented Sep 19, 2013

LucasSloan commented Sep 19, 2013

LucasSloan commented Sep 22, 2013

jack-trikeapps Dec 2, 2013

LucasSloan Dec 9, 2013

jack-trikeapps Dec 2, 2013

LucasSloan Dec 9, 2013

jack-trikeapps commented Dec 2, 2013

jack-trikeapps Dec 2, 2013

LucasSloan Dec 9, 2013

LucasSloan Dec 9, 2013

jack-trikeapps commented Dec 3, 2013

jack-trikeapps commented Dec 3, 2013

		@@ -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,

Postmovement #80

Are you sure you want to change the base?

Postmovement #80

Conversation

LucasSloan commented Jul 23, 2013

Choose a reason for hiding this comment

LucasSloan commented Jul 29, 2013

keyist commented Sep 6, 2013

LucasSloan commented Sep 6, 2013

keyist commented Sep 16, 2013

LucasSloan commented Sep 16, 2013

keyist commented Sep 19, 2013

LucasSloan commented Sep 19, 2013

LucasSloan commented Sep 22, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-trikeapps commented Dec 2, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-trikeapps commented Dec 3, 2013

jack-trikeapps commented Dec 3, 2013