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

          Activity

          Hide
          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
          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.
          Hide
          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
          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.
          Hide
          Tom Lanyon added a comment -

          Added test instructions.

          Show
          Tom Lanyon added a comment - Added test instructions.
          Hide
          David Monllaó added a comment -

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

          Show
          David Monllaó added a comment - Thanks for the patch Tom. Updating pull branches. Thanks DanP for the comments
          Hide
          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
          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.
          Hide
          David Monllaó added a comment -

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

          Show
          David Monllaó added a comment - Thanks for the comments Dan. Query tested with mysql, postgres, oracle and mssql
          Hide
          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
          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)
          Hide
          David Monllaó added a comment -

          Thanks Dan, adding more instructions and submitting for integration

          Show
          David Monllaó added a comment - Thanks Dan, adding more instructions and submitting for integration
          Hide
          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
          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
          David Monllaó added a comment -

          Pull branches rebased against upstream. Thanks

          Show
          David Monllaó added a comment - Pull branches rebased against upstream. Thanks
          Hide
          Sam Hemelryk added a comment -

          Thanks David, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks David, this has been integrated now.
          Hide
          Jason Fowler added a comment -

          works fine David

          Show
          Jason Fowler added a comment - works fine David
          Hide
          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
          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!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: