Moodle
  1. Moodle
  2. MDL-40524

Wiki comments displayed out of order

    Details

    • Testing Instructions:
      Hide

      */ Find or create a wiki activity
      */ if no comments exist on a wiki page, add some
      */ Verify they're displayed in time ascending order

      More verbose test:
      */ After adding some comments, directly alter their timecreated timestamp in the mdl_comments table so that comments added later have timestamps saying they were made earlier than ones before it.
      */ Verify the page still displays them in time ascending order.

      Show
      */ Find or create a wiki activity */ if no comments exist on a wiki page, add some */ Verify they're displayed in time ascending order More verbose test: */ After adding some comments, directly alter their timecreated timestamp in the mdl_comments table so that comments added later have timestamps saying they were made earlier than ones before it. */ Verify the page still displays them in time ascending order.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      51341

      Description

      When viewing a mod_wiki page, the comments displayed aren't ordered in any fashion. They're displayed in whatever order the database decided to return the comment records back in.

      Most of the time, a database will return them in insert order. But that's not guarenteed.

        Activity

        Hide
        Adam Olley added a comment -

        Probably up for debate if the default ordering should be ASC or DESC...

        Show
        Adam Olley added a comment - Probably up for debate if the default ordering should be ASC or DESC...
        Hide
        Jason Fowler added a comment -

        Code looks fine Adam, I am also unsure if it should be ASC or DESC, or alternatively, should we allow the user to decide? Would like to get some more input on this, so I am going to mention it in the Developer Chatroom, and see what happens.

        Show
        Jason Fowler added a comment - Code looks fine Adam, I am also unsure if it should be ASC or DESC, or alternatively, should we allow the user to decide? Would like to get some more input on this, so I am going to mention it in the Developer Chatroom, and see what happens.
        Hide
        Jason Fowler added a comment -

        Ah, and this probably needs to be cherry picked back on to older branches.

        Show
        Jason Fowler added a comment - Ah, and this probably needs to be cherry picked back on to older branches.
        Hide
        Russell Smith added a comment -

        The current expected order of users is ASC due to the nature of the default sort order for databases in most cases.

        Second to that, most purist types dislike top posting. There is no specific option now, introducing one would be a feature that's not really needed in back-branches. A fix to ensure it's consistent and address the issue as reported.

        If you want to change the behaviour, that's a whole different request in my view.

        Show
        Russell Smith added a comment - The current expected order of users is ASC due to the nature of the default sort order for databases in most cases. Second to that, most purist types dislike top posting. There is no specific option now, introducing one would be a feature that's not really needed in back-branches. A fix to ensure it's consistent and address the issue as reported. If you want to change the behaviour, that's a whole different request in my view.
        Hide
        Adam Olley added a comment -

        Having thought about it some more, ASC makes the most sense. It's what most of the web (that comes to mind right now) does anyway. It's trivial to flip in code if anyone particularly wants it DESC after all.

        I've thus changed it to ASC.

        Show
        Adam Olley added a comment - Having thought about it some more, ASC makes the most sense. It's what most of the web (that comes to mind right now) does anyway. It's trivial to flip in code if anyone particularly wants it DESC after all. I've thus changed it to ASC.
        Hide
        Jason Fowler added a comment -

        Great! Precedent followed, consensus reached. Pushing for integration.

        Show
        Jason Fowler added a comment - Great! Precedent followed, consensus reached. Pushing for integration.
        Hide
        Jason Fowler added a comment -

        Integrators: This should be able to be cleanly cherry pick back to the stable branches.

        Show
        Jason Fowler added a comment - Integrators: This should be able to be cleanly cherry pick back to the stable branches.
        Hide
        Damyon Wiese added a comment -

        Thanks for the patch Matt,

        This has been integrated to 24, 25 and master.

        Show
        Damyon Wiese added a comment - Thanks for the patch Matt, This has been integrated to 24, 25 and master.
        Hide
        Mark Nelson added a comment -

        Works as expected, passing. I did find a debugging message when testing this which I noted in MDL-40604.

        Show
        Mark Nelson added a comment - Works as expected, passing. I did find a debugging message when testing this which I noted in MDL-40604 .
        Hide
        Damyon Wiese added a comment -

        Moodle has many old functions,
        And although they cause no malfunction,
        There comes a day,
        When they get deprecated away,
        And get and put on the list for expulsion.

        Thanks for all the reports/testing/fixes this week. This issue has been sent upstream.

        Show
        Damyon Wiese added a comment - Moodle has many old functions, And although they cause no malfunction, There comes a day, When they get deprecated away, And get and put on the list for expulsion. Thanks for all the reports/testing/fixes this week. This issue has been sent upstream.

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: