Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-32931

Forum overview 'new posts' functionality returns incorrect results and is horribly slow

    Details

    • Database:
      Any, PostgreSQL
    • Testing Instructions:
      Hide
      1. As an admin create / go to a course and add 'UserA' as a student and 'UserB' as a teacher
      2. Login as UserA, go to "My moodle" page, enable the course overview block and check the number of new forum posts listed (if there are new forum posts)
      3. Visit the course as UserB and create a new forum
      4. Visit the "My Moodle" page as UserA and check that the number of new forum posts is the same
      5. Visit the course as UserB and create a new forum discussion
      6. Visit the "My Moodle" page as UserA and check that the number of new forum posts has been increased by one
      7. Visit the course as UserB and create a new forum post to the discussion
      8. Visit the "My Moodle" page as UserA and check that the number of new forum posts has been increased by one
      9. Visit the course as UserA, read the forum posts and logout
      10. Login again as UserA and visit the "My Moodle" page to check that there are no pending posts
      Show
      As an admin create / go to a course and add 'UserA' as a student and 'UserB' as a teacher Login as UserA, go to "My moodle" page, enable the course overview block and check the number of new forum posts listed (if there are new forum posts) Visit the course as UserB and create a new forum Visit the "My Moodle" page as UserA and check that the number of new forum posts is the same Visit the course as UserB and create a new forum discussion Visit the "My Moodle" page as UserA and check that the number of new forum posts has been increased by one Visit the course as UserB and create a new forum post to the discussion Visit the "My Moodle" page as UserA and check that the number of new forum posts has been increased by one Visit the course as UserA, read the forum posts and logout Login again as UserA and visit the "My Moodle" page to check that there are no pending posts
    • Workaround:
      Hide

      Avoid log table

      Show
      Avoid log table
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32931_master

      Description

      The forum overview used by the course overview block counts the number of new posts in forum discussions for courses in which the user is enrolled.

      This currently does not return the correct results; its search method is to scan the log table for 'add post' actions - however, posts can be added through other means without an 'add post' action being logged (for example, during the start of a new discussion, a new post is added but only the action 'add discussion' is logged).

      A side consequence of using the database log table for this work is that it can by abysmally slow given a large dataset in the log table. This is especially the case when scanning for new posts in a course which the user has never accessed ($course->lastaccess == 0) as this can then do a scan on the entirety of the log table for all posts since the beginning of the Moodle and can cause cache evictions from the database buffers for other, more useful data.

        Gliffy Diagrams

          Attachments

            Activity

            tomlanyon Tom Lanyon created issue -
            Hide
            tomlanyon Tom Lanyon added a comment -

            Patch to change forum overview to use the authoritative source of forum post information (forum, forum_post and forum_discussion tables) rather than the log table. Fixes course overview block to return accurate results and can significantly improve performance.

            Show
            tomlanyon Tom Lanyon added a comment - Patch to change forum overview to use the authoritative source of forum post information (forum, forum_post and forum_discussion tables) rather than the log table. Fixes course overview block to return accurate results and can significantly improve performance.
            tomlanyon Tom Lanyon made changes -
            Field Original Value New Value
            Pull Master Diff URL https://github.com/tomlanyon/moodle/compare/master...MDL-32931/master
            Pull Master Branch MDL-32931/master
            Pull 2.0 Diff URL https://github.com/tomlanyon/moodle/compare/MOODLE_20_STABLE...MDL-32931/MOODLE_20_STABLE
            Pull 2.0 Branch MDL-32931/MOODLE_20_STABLE
            Database Any,PostgreSQL [ 10000,10002 ]
            Pull 2.2 Diff URL https://github.com/tomlanyon/moodle/compare/MOODLE_22_STABLE...MDL-32931/MOODLE_22_STABLE
            Pull 2.1 Branch MDL-32931/MOODLE_21_STABLE
            Pull 2.2 Branch MDL-32931/MOODLE_22_STABLE
            Pull 2.1 Diff URL https://github.com/tomlanyon/moodle/compare/MOODLE_21_STABLE...MDL-32931/MOODLE_21_STABLE
            Pull from Repository git://github.com/tomlanyon/moodle.git
            tomlanyon Tom Lanyon made changes -
            Labels netspot partner performance
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting that and providing a solution.

            It would help if you could provide a set of testing instructions for your solution.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting that and providing a solution. It would help if you could provide a set of testing instructions for your solution.
            salvetore Michael de Raadt made changes -
            Fix Version/s STABLE backlog [ 10463 ]
            Labels netspot partner performance netspot partner patch triaged
            Hide
            tomlanyon Tom Lanyon added a comment -

            Added test instructions.

            Show
            tomlanyon Tom Lanyon added a comment - Added test instructions.
            tomlanyon Tom Lanyon made changes -
            Testing Instructions 1. Enable the course overview block and ensure it still appears to function (e.g. doesn't throw errors)
            2. Take a course that contains forum discussions and two users ('UserA' and 'UserB').
            3. Visit the course as UserA and check the number of new forum posts listed.
            4. Visit the course as UserB and create a new forum post in an existing discussion, or start a new discussion.
            5. Reload the course page as UserA and check that the number of new forum posts has increased.
            dmonllao David Monllaó made changes -
            Assignee moodle.com [ moodle.com ] David Monllaó [ davmon ]
            Hide
            dmonllao David Monllaó added a comment -

            Thanks for the patch Tom. Updating pull branches. Thanks DanP for the comments

            Show
            dmonllao David Monllaó added a comment - Thanks for the patch Tom. Updating pull branches. Thanks DanP for the comments
            dmonllao David Monllaó made changes -
            Pull Master Diff URL https://github.com/tomlanyon/moodle/compare/master...MDL-32931/master https://github.com/dmonllao/moodle/compare/master...MDL-32931_master
            Pull Master Branch MDL-32931/master MDL-32931_master
            Pull 2.3 Diff URL https://github.com/dmonllao/moodle/compare/MOODLE_23_STABLE...MDL-32931_23
            Pull 2.0 Diff URL https://github.com/tomlanyon/moodle/compare/MOODLE_20_STABLE...MDL-32931/MOODLE_20_STABLE
            Pull 2.0 Branch MDL-32931/MOODLE_20_STABLE
            Peer reviewer poltawski
            Testing Instructions 1. Enable the course overview block and ensure it still appears to function (e.g. doesn't throw errors)
            2. Take a course that contains forum discussions and two users ('UserA' and 'UserB').
            3. Visit the course as UserA and check the number of new forum posts listed.
            4. Visit the course as UserB and create a new forum post in an existing discussion, or start a new discussion.
            5. Reload the course page as UserA and check that the number of new forum posts has increased.
            # Enable the course overview block and ensure it still appears to function (e.g. doesn't throw errors)
            # Take a course that contains forum discussions and two users ('UserA' and 'UserB').
            # Visit the course as UserA and check the number of new forum posts listed.
            # Visit the course as UserB and create a new forum post in an existing discussion, or start a new discussion.
            # Reload the course page as UserA and check that the number of new forum posts has increased.
            Workaround Avoid log table
            Pull 2.2 Diff URL https://github.com/tomlanyon/moodle/compare/MOODLE_22_STABLE...MDL-32931/MOODLE_22_STABLE https://github.com/dmonllao/moodle/compare/MOODLE_22_STABLE...MDL-32931_22
            Pull 2.1 Branch MDL-32931/MOODLE_21_STABLE
            Pull 2.2 Branch MDL-32931/MOODLE_22_STABLE MDL-32931_22
            Pull 2.1 Diff URL https://github.com/tomlanyon/moodle/compare/MOODLE_21_STABLE...MDL-32931/MOODLE_21_STABLE
            Pull from Repository git://github.com/tomlanyon/moodle.git git://github.com/dmonllao/moodle.git
            Affects Version/s 2.3 [ 10657 ]
            Affects Version/s 2.4 [ 12255 ]
            Pull 2.3 Branch MDL-32931_23
            dmonllao David Monllaó made changes -
            Status Open [ 1 ] Waiting for peer review [ 10012 ]
            moodle.com moodle.com made changes -
            Fix Version/s STABLE Sprint 22 [ 12156 ]
            Fix Version/s STABLE backlog [ 10463 ]
            poltawski Dan Poltawski made changes -
            Original Estimate 0 minutes [ 0 ]
            Remaining Estimate 0 minutes [ 0 ]
            Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
            Hide
            poltawski Dan Poltawski added a comment -

            In general I think the approach seems to make sense. But you need to check if this change is cross-db compatible.

            It looks to me like it might break need both the count and forumid in the group by statement to work on all dbs.

            Show
            poltawski Dan Poltawski added a comment - In general I think the approach seems to make sense. But you need to check if this change is cross-db compatible. It looks to me like it might break need both the count and forumid in the group by statement to work on all dbs.
            poltawski Dan Poltawski made changes -
            Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
            Hide
            dmonllao David Monllaó added a comment -

            Thanks for the comments Dan. Query tested with mysql, postgres, oracle and mssql

            Show
            dmonllao David Monllaó added a comment - Thanks for the comments Dan. Query tested with mysql, postgres, oracle and mssql
            dmonllao David Monllaó made changes -
            Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
            poltawski Dan Poltawski made changes -
            Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
            Hide
            poltawski Dan Poltawski added a comment -

            Hi David,

            It looks good for integration, the only comment is that it'd be much easier for us to integrate if could add more extensive testing instructions to cover a more detailed scenario to ensure we are not introducing regressions. (e.g. more users and discussions etc)

            Show
            poltawski Dan Poltawski added a comment - Hi David, It looks good for integration, the only comment is that it'd be much easier for us to integrate if could add more extensive testing instructions to cover a more detailed scenario to ensure we are not introducing regressions. (e.g. more users and discussions etc)
            poltawski Dan Poltawski made changes -
            Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
            Hide
            dmonllao David Monllaó added a comment -

            Thanks Dan, adding more instructions and submitting for integration

            Show
            dmonllao David Monllaó added a comment - Thanks Dan, adding more instructions and submitting for integration
            dmonllao David Monllaó made changes -
            Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
            Testing Instructions # Enable the course overview block and ensure it still appears to function (e.g. doesn't throw errors)
            # Take a course that contains forum discussions and two users ('UserA' and 'UserB').
            # Visit the course as UserA and check the number of new forum posts listed.
            # Visit the course as UserB and create a new forum post in an existing discussion, or start a new discussion.
            # Reload the course page as UserA and check that the number of new forum posts has increased.
            # As an admin create / go to a course and add 'UserA' as a student and 'UserB' as a teacher
            # Login as UserA, go to "My moodle" page, enable the course overview block and check the number of new forum posts listed (if there are new forum posts)
            # Visit the course as UserB and create a new forum
            # Visit the "My Moodle" page as UserA and check that the number of new forum posts is the same
            # Visit the course as UserB and create a new forum discussion
            # Visit the "My Moodle" page as UserA and check that the number of new forum posts has been increased by one
            # Visit the course as UserB and create a new forum post to the discussion
            # Visit the "My Moodle" page as UserA and check that the number of new forum posts has been increased by one
            # Visit the course as UserA, read the forum posts and logout
            # Login again as UserA and visit the "My Moodle" page to check that there are no pending posts
            Hide
            nebgor Aparup Banerjee added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            nebgor Aparup Banerjee added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            dmonllao David Monllaó added a comment -

            Pull branches rebased against upstream. Thanks

            Show
            dmonllao David Monllaó added a comment - Pull branches rebased against upstream. Thanks
            poltawski Dan Poltawski made changes -
            Currently in integration Yes [ 10041 ]
            samhemelryk Sam Hemelryk made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator samhemelryk
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks David, this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks David, this has been integrated now.
            samhemelryk Sam Hemelryk made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            Fix Version/s 2.2.5 [ 12352 ]
            Fix Version/s 2.3.2 [ 12353 ]
            salvetore Michael de Raadt made changes -
            Tester phalacee
            phalacee Jason Fowler made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Hide
            phalacee Jason Fowler added a comment -

            works fine David

            Show
            phalacee Jason Fowler added a comment - works fine David
            phalacee Jason Fowler made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Hide
            poltawski Dan Poltawski added a comment -

            asko, Дзякуй, ধন্যবাদ, Благодаря, Gràcies, 感谢, 謝謝, Hvala, Díky, Tak, Bedankt, Tänan, متشکریم, Salamat, Kiitokset, Merci, Grazas, Danke, Ευχαριστώ, આભાર, תודה, धन्यवाद, Köszönjük, Takk fyrir, Terima Kasih, Grazie, ありがとうございます, Рахмет, សូមអរគុណ, 감사합니다, gratiās, Pateicamies, Ačiū, Благодарам, Tēnā koa, Kia Ora Rawa Atu, आभारी आहोत, Талархал, Takk, Dziękuję, Obrigado, Mulţumesc, Engraziel, Спасибо, Fa'afetai, Хвала, Hvala, ස්තූතියි, Vďaka, Hvala, Mahadsanid, Thanks, Gracias, Tack, Salamat, நன்றி, నెనరులు, ขอบคุณค่ะ!

            Your work has made it into this weeks Moodle release! There are no gold medals available this week - but millions around the world will benefit. Thank you!

            Show
            poltawski Dan Poltawski added a comment - asko, Дзякуй, ধন্যবাদ, Благодаря, Gràcies, 感谢, 謝謝, Hvala, Díky, Tak, Bedankt, Tänan, متشکریم, Salamat, Kiitokset, Merci, Grazas, Danke, Ευχαριστώ, આભાર, תודה, धन्यवाद, Köszönjük, Takk fyrir, Terima Kasih, Grazie, ありがとうございます, Рахмет, សូមអរគុណ, 감사합니다, gratiās, Pateicamies, Ačiū, Благодарам, Tēnā koa, Kia Ora Rawa Atu, आभारी आहोत, Талархал, Takk, Dziękuję, Obrigado, Mulţumesc, Engraziel, Спасибо, Fa'afetai, Хвала, Hvala, ස්තූතියි, Vďaka, Hvala, Mahadsanid, Thanks, Gracias, Tack, Salamat, நன்றி, నెనరులు, ขอบคุณค่ะ! Your work has made it into this weeks Moodle release! There are no gold medals available this week - but millions around the world will benefit. Thank you!
            poltawski Dan Poltawski made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Integration date 02/Aug/12
            poltawski Dan Poltawski made changes -
            Currently in integration Yes [ 10041 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Fix Version/s STABLE Sprint 22 [ 12156 ]

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Sep/12