Reversed order on graft
heliogabalus - October 13, 2006 - 00:18
| Project: | Comment mover |
| Version: | 4.7.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
When node comments are in threaded view from older to newer, when pruned/grafted to turn a comment into a new node the whole tree below it gets reversed (newer to older instead of the original older to newer). New comments are added in the right order afterwards.
#1
I just noticed this as well.
#2
Here's a patch that fixes it.
It was using the same awful thread ID generation code I replaced in the phpbb2drupal module in this issue. I've fixed that.
It was also grabbing the comments in reverse thread ID order. This meant it was processing them newest to oldest, but adding them into the target node newest to oldest. Oopsie.
I tested it with a forum post with one level of comments and one with multiple levels of comments to be sure the graft still worked as desired. It did.
#3
Thanks a lot.
The patch has a few problems, however. I've tried it on a new node with threaded comments, oldest first. I've created for comments and gave all comments three children. Next, I made sure that all children of two, three and four and three and four themselves became children of two in one long branch. When finally moving the entire branch two to become a child of comment one, threading was completely off.
Here's an overview of the comment table.
mysql> SELECT cid, pid, thread, subject from comments;
+-----+-----+--------+---------+
| cid | pid | thread | subject |
+-----+-----+--------+---------+
| 35 | 0 | 01/ | 1 |
| 36 | 0 | 02/ | 2 |
| 37 | 0 | 03/ | 3 |
| 38 | 0 | 04/ | 4 |
| 39 | 35 | 01.00/ | 11 |
| 40 | 35 | 01.01/ | 12 |
| 41 | 35 | 01.02/ | 13 |
| 42 | 36 | 02.00/ | 21 |
| 43 | 36 | 02.01/ | 22 |
| 44 | 36 | 02.02/ | 23 |
| 45 | 37 | 03.00/ | 31 |
| 46 | 37 | 03.01/ | 32 |
| 47 | 37 | 03.02/ | 33 |
| 48 | 38 | 04.00/ | 41 |
| 49 | 38 | 04.01/ | 42 |
| 50 | 38 | 04.02/ | 43 |
+-----+-----+--------+---------+
16 rows in set (0.00 sec)
Moving all 2+ comments as a straight branch (child->child->child--//-->child under 2) and all children of 1 as a straight branch under 1).
mysql> SELECT cid, pid, thread, subject from comments;+-----+-----+--------------------------------------+---------+
| cid | pid | thread | subject |
+-----+-----+--------------------------------------+---------+
| 35 | 0 | 01/ | 1 |
| 36 | 0 | 02/ | 2 |
| 37 | 44 | 02.00.02.01.01/ | 3 |
| 38 | 47 | 02.00.02.01.01.01.01.01.01/ | 4 |
| 39 | 35 | 01.00/ | 11 |
| 40 | 39 | 01.00.01/ | 12 |
| 41 | 40 | 01.00.01.01/ | 13 |
| 42 | 36 | 02.00/ | 21 |
| 43 | 42 | 02.00.02/ | 22 |
| 44 | 43 | 02.00.02.01/ | 23 |
| 45 | 37 | 02.00.02.01.01.01/ | 31 |
| 46 | 45 | 02.00.02.01.01.01.01/ | 32 |
| 47 | 46 | 02.00.02.01.01.01.01.01/ | 33 |
| 48 | 38 | 02.00.02.01.01.01.01.01.01.01/ | 41 |
| 49 | 48 | 02.00.02.01.01.01.01.01.01.01.01/ | 42 |
| 50 | 49 | 02.00.02.01.01.01.01.01.01.01.01.01/ | 43 |
+-----+-----+--------------------------------------+---------+
Pruning 2 (36) and grafting on 13 (41)
mysql> SELECT cid, pid, thread, subject from comments;+-----+-----+--------------------------------------+---------+
| cid | pid | thread | subject |
+-----+-----+--------------------------------------+---------+
| 35 | 0 | 01/ | 1 |
| 36 | 41 | 01.00.01.01.01/ | 2 |
| 37 | 44 | 02.00.02.01.02/ | 3 |
| 38 | 47 | 02.00.02.01.01.01.01.01.02/ | 4 |
| 39 | 35 | 01.00/ | 11 |
| 40 | 39 | 01.00.01/ | 12 |
| 41 | 40 | 01.00.01.01/ | 13 |
| 42 | 36 | 01.00.01.01.01.01/ | 21 |
| 43 | 42 | 02.00.03/ | 22 |
| 44 | 43 | 02.00.02.02/ | 23 |
| 45 | 37 | 02.00.02.01.01.02/ | 31 |
| 46 | 45 | 02.00.02.01.01.01.02/ | 32 |
| 47 | 46 | 02.00.02.01.01.01.01.02/ | 33 |
| 48 | 38 | 02.00.02.01.01.01.01.01.01.02/ | 41 |
| 49 | 48 | 02.00.02.01.01.01.01.01.01.01.02/ | 42 |
| 50 | 49 | 02.00.02.01.01.01.01.01.01.01.01.02/ | 43 |
+-----+-----+--------------------------------------+---------+
#4
I'm having a hard time understanding what went on in your example, but I'll try to replicate it on a testbed myself within the next couple of days and see what happens. I might make more snapshots in between that will help me see the problem and why not, others too. Thanks for bringing this to my attention.
#5
I'm not seeing a problem. Here's what I did:
Created four threads, and then two comments per thread.
thread subject01/ This should be 01.
01.00/ This should be 01.00.
01.01/ This should be 01.01.
02/ This should be 02.
02.00/ This should be 02.00.
02.01/ This should be 02.01.
03/ 03.
03.00/ 03.00.
03.01/ 03.01.
04/ 04.
04.00/ 04.00.
04.01/ 04.01.
First, I pruned 03 and grafted it to 02.
thread subject01/ This should be 01.
01.00/ This should be 01.00.
01.01/ This should be 01.01.
02/ This should be 02.
02.00/ This should be 02.00.
02.01/ This should be 02.01.
02.02/ 03.
02.02.00/ 03.00.
02.02.01/ 03.01.
04/ 04.
04.00/ 04.00.
04.01/ 04.01.
Then I pruned 04 and grafted it onto 02.
thread subject01/ This should be 01.
01.00/ This should be 01.00.
01.01/ This should be 01.01.
02/ This should be 02.
02.00/ This should be 02.00.
02.01/ This should be 02.01.
02.02/ 03.
02.02.00/ 03.00.
02.02.01/ 03.01.
02.03/ 04.
02.03.00/ 04.00.
02.03.01/ 04.01.
And lastly I grafted 02 onto 01:
thread subject01/ This should be 01.
01.00/ This should be 01.00.
01.01/ This should be 01.01.
01.02/ This should be 02.
01.02.00/ This should be 02.00.
01.02.01/ This should be 02.01.
01.02.02/ 03.
01.02.02.00/ 03.00.
01.02.02.01/ 03.01.
01.02.03/ 04.
01.02.03.00/ 04.00.
01.02.03.01/ 04.01.
The sorts continue to work.
I have updated the patch with a minor fix. The comment vancodes were starting at 01 when they should start at 00. That's it.
Can you explain in finer detail how to reproduce what you've seen.
#6
An updated patch makes sure the '/' is disregarded when ASC sorting.
#7
I've snipped the first few lines of the patch which made applying it break. I've seen this before. Some method some developers are using to post patches is tacking on a few lines of information at the top of the file, at least that's how it looks when I download it.
Typically I see patches written with a .patch on the end so I tend to use that suffix on the resulting patch file.
Anyway I tested this patch and the sort is working properly with the SQL change on PostgreSQL.
#8
Thank you.
Patch in #6 is created using CVS, so we know exactly what revision it is against. Your patch program should simply ignore this (weird it doesn't).
I'll test a few more scenarios, then commit.
#9
Committed to 4.7.x-1.x-dev and HEAD. Will backport to 4.6.
Thanks.
#10
if 4.6 is not relevant anymore, maybe this issue is fixed?
#11
Automatically closed -- issue fixed for two weeks with no activity.