Threaded comments display in the wrong order

[Aaron Wells]

To replicate:

1. Clean install of Mahara
2. In the "No institution" settings, turn on threaded comments. Leave the comment sort order on the default "ascending"
3. Create a page
4. Leave about 10 comments on the page. Don't make any of them be replies.

Expected result: Because the comments aren't replies, they should be displayed on the page in the order they were created
Actual result: After the first few comments are posted, comments will start showing up at the top of the list of comments

The reason for this is because the sortorder for "threaded ascending" is (artefact.path, artefact.ctime, id). Since artefact.path is a string, and gets sorted in alphabetical order, when the artefact IDs go from 1 digit to 2 digits, the 1 digit ones will start sorting below the 2 digit ones. I.e. "/11" comes before "/6", in string sorting order.

A similar thing will happen if threaded comments are sorted in descending order.

[Aaron Wells (u-aaronw)]

As long as comment ID numbers keep incrementing (so that "SORT BY id" gives the same order as "SORT BY ctime") this problem only shows up when comment IDs "Roll over" to the next larger size, i.e. 1 digit -> 2 digits; 2 digits -> 3 digits, etc. That's why it's easiest to test it if this is the first thing you do on a clean install.

In the forum posts, which also sort threaded data in ascending manner, we solved the problem by padding the IDs in the paths out to 10 digits with leading zeroes. This ensures that e.g. 0000000006 comes before 0000000011 in string sort order.

That is a possibility here too... but in this case the path column is artefact.path, and it's used for many purposes, so I think it's probably not safe to assume that we can just drop leading zeroes in there and it'll keep working for everything.

Instead, perhaps what we can do is add an additional column to artefact_comment_comment that contains the path with padded IDs for each comment artefact.

[Aaron Wells (u-aaronw)]

Since threaded feedback is a new feature in Mahara 15.10, we should really try to get this fixed for the .0 release.

[Mahara Bot (dev-mahara)]

Patch for "master" branch: https://reviews.mahara.org/5440

[Son Nguyen (ngson2000)]

The drawbacka of using paddedpath for sorting are the fix number of padding 0 and the limit depth of the path.
This will cause the wrong order when the artefact id is too big or for some reasons, the db admin set a big number for starting the artefact id.

I have a better idea for this but require more work
We will add a column 'threadedorder' to the table artefact_comment_comment which store the position of the comment in the list of threaded comments for a page/artefact.
When a comment is added, we will update the position of the comment and other following comments in the list.

[Mahara Bot (dev-mahara)]

Patch for "master" branch: https://reviews.mahara.org/5562

[Mahara Bot (dev-mahara)]

Reviewed: https://reviews.mahara.org/5562
Committed: https://git.nzoss.org.nz/mahara/mahara/commit/289b680fba0b52efdaa2c8fd3ef325eace508fac
Submitter: Aaron Wells (<email address hidden>)
Branch: master

commit 289b680fba0b52efdaa2c8fd3ef325eace508fac
Author: Son Nguyen <email address hidden>
Date: Wed Oct 21 12:23:15 2015 +1300

Fix sorting threaded comments. Bug 1499122

- Add new column 'threadedposition' to table artefact_comment_comment
- Update the column for existing comments
- Calculate the position for new comments
- Display threaded commented using this for correct order
- Add a behat test

Change-Id: I607f26fccccee8f761754a41a21c6f58dd74cfb6

[Mahara Bot (dev-mahara)]

Patch for "15.10_STABLE" branch: https://reviews.mahara.org/5568

[Mahara Bot (dev-mahara)]

Reviewed: https://reviews.mahara.org/5568
Committed: https://git.nzoss.org.nz/mahara/mahara/commit/acfa725937c99b586a0fc31fc3e5523f6549b647
Submitter: Robert Lyon (<email address hidden>)
Branch: 15.10_STABLE

commit acfa725937c99b586a0fc31fc3e5523f6549b647
Author: Son Nguyen <email address hidden>
Date: Wed Oct 21 12:23:15 2015 +1300

Fix sorting threaded comments. Bug 1499122

- Add new column 'threadedposition' to table artefact_comment_comment
- Update the column for existing comments
- Calculate the position for new comments
- Display threaded commented using this for correct order
- Add a behat test
- Fix problems in upgrade script (Bug 1508301)

Change-Id: I607f26fccccee8f761754a41a21c6f58dd74cfb6