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

Online Users block is miss reporting 100's of users online as 1

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.2, 2.1.2, 2.2
    • Fix Version/s: 2.0.6, 2.1.3
    • Component/s: Blocks
    • Labels:
    • Testing Instructions:
      Hide

      1/ Add the online users block to either a course or frontpage
      2/ Get 51 users to visit the course/site [NOTE BELOW]
      3/ Notice the "(last 5 minutes: x)" display

      Expected Result:


      The 'x' should show 51

      Actual result


      The 'x' is showing 1
      Notice the developer warnings about incorrect get_records call as specified in this bug report.

      [NOTE] An alternative way to expose this issue is to apply the patch_to_expose_issue.patch which i've attached. Which ensures the summary mentioned is always displayed

      Show
      1/ Add the online users block to either a course or frontpage 2/ Get 51 users to visit the course/site [NOTE BELOW] 3/ Notice the "(last 5 minutes: x)" display Expected Result: The 'x' should show 51 Actual result The 'x' is showing 1 Notice the developer warnings about incorrect get_records call as specified in this bug report. [NOTE] An alternative way to expose this issue is to apply the patch_to_expose_issue.patch which i've attached. Which ensures the summary mentioned is always displayed
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:

      Description

      Online Users block is miss reporting 100's of users online as 1 because count_records_sql return multiple records with 1 and generates this error :-

      Did you remember to make the first column something unique in your call to get_records? Duplicate value '1' found in column 'count'.

      • line 699 of /lib/dml/pgsql_native_moodle_database.php: call to debugging()
      • line 1256 of /lib/dml/moodle_database.php: call to pgsql_native_moodle_database->get_records_sql()
      • line 1331 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql()
      • line 1502 of /lib/dml/moodle_database.php: call to moodle_database->get_field_sql()
      • line 123 of /blocks/online_users/block_online_users.php: call to moodle_database->count_records_sql()
      • line 280 of /blocks/moodleblock.class.php: call to block_online_users->get_content()
      • line 232 of /blocks/moodleblock.class.php: call to block_base->formatted_contents()
      • line 895 of /lib/blocklib.php: call to block_base->get_content_for_output()
      • line 947 of /lib/blocklib.php: call to block_manager->create_block_contents()
      • line 342 of /lib/blocklib.php: call to block_manager->ensure_content_created()
      • line 4 of /theme/base/layout/frontpage.php: call to block_manager->region_has_content()
      • line 647 of /lib/outputrenderers.php: call to include()
      • line 605 of /lib/outputrenderers.php: call to core_renderer->render_page_layout()
      • line ? of unknownfile: call to core_renderer->header()
      • line 1229 of /lib/setuplib.php: call to call_user_func_array()
      • line 89 of /index.php: call to bootstrap_renderer->__call()
      • line 89 of /index.php: call to bootstrap_renderer->header()

        Gliffy Diagrams

        1. patch_to_expose_issue.patch
          0.5 kB
          Dan Poltawski
        1. Online-Users.jpg
          5 kB

          Activity

          Show
          tlock Tim Lock added a comment - This is fixed here :- https://github.com/tlock/moodle/commit/ed95109813b2f57fc1527c96a4936a8fe7b3ba90
          Hide
          poltawski Dan Poltawski added a comment -

          How do you reproduce this Tim? I can't reproduce this on my postgres system

          Show
          poltawski Dan Poltawski added a comment - How do you reproduce this Tim? I can't reproduce this on my postgres system
          Hide
          mblake Michael Blake added a comment -

          This issue is causing problems for MP client. Please give it priority.

          Show
          mblake Michael Blake added a comment - This issue is causing problems for MP client. Please give it priority.
          Hide
          poltawski Dan Poltawski added a comment -

          Hi Tim,

          Can you give any more info on how to reproduce this? I've tested it on a few different postgres installations and not been able to reproduce the issue..

          Show
          poltawski Dan Poltawski added a comment - Hi Tim, Can you give any more info on how to reproduce this? I've tested it on a few different postgres installations and not been able to reproduce the issue..
          Hide
          tlock Tim Lock added a comment - - edited

          Online User showing number of users incorrectly (patched).

          Show
          tlock Tim Lock added a comment - - edited Online User showing number of users incorrectly (patched).
          Hide
          tlock Tim Lock added a comment -

          Hi Dan,

          In my attachment I have one line changed in latest 2.2 :-

          • if (count($users) < 50) {
            + if (count($users) < 0) {

          This shows my problem, that a busy site with only ever show 1 for the number of the users.

          This is caused by count_records_sql() only taking the first row of the resulting SQL. Hence SQL changed to count() the users into the first row.

          Another fix would be using this SQL :-

          SELECT COUNT FROM mdl_user u WHERE u.lastaccess > 1312331000;

          Show
          tlock Tim Lock added a comment - Hi Dan, In my attachment I have one line changed in latest 2.2 :- if (count($users) < 50) { + if (count($users) < 0) { This shows my problem, that a busy site with only ever show 1 for the number of the users. This is caused by count_records_sql() only taking the first row of the resulting SQL. Hence SQL changed to count() the users into the first row. Another fix would be using this SQL :- SELECT COUNT FROM mdl_user u WHERE u.lastaccess > 1312331000;
          Hide
          poltawski Dan Poltawski added a comment -

          Ahha, so its incorrectly reporting the number of users in the summary if there are over 50.

          Show
          poltawski Dan Poltawski added a comment - Ahha, so its incorrectly reporting the number of users in the summary if there are over 50.
          Hide
          poltawski Dan Poltawski added a comment -

          Hi Tim,

          Thanks, I understand now. I don't believe your fix is correct though as the sql is doing uncessary group bys - we only need one value and thats the count. I also don't think we need the distinct as the user id should already be distinct.

          Show
          poltawski Dan Poltawski added a comment - Hi Tim, Thanks, I understand now. I don't believe your fix is correct though as the sql is doing uncessary group bys - we only need one value and thats the count. I also don't think we need the distinct as the user id should already be distinct.
          Hide
          poltawski Dan Poltawski added a comment -

          Petr - please could you peer review this change in SQL - do you agree the distinct on user ID is not required?

          Show
          poltawski Dan Poltawski added a comment - Petr - please could you peer review this change in SQL - do you agree the distinct on user ID is not required?
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week due to time constraints. Thanks for your support and patience!

          Sorry and ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week due to time constraints. Thanks for your support and patience! Sorry and ciao
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          I'm reopening this because:

          the site count() does not seem to need any distinct because the join with group_members cannot lead to multiple records per user returned...

          but the course count(), if I'm not wrong can return the same user multiple times if there are users with multiple roles in the same course. So perhaps the distinct could be necessary there to avoid counting the same user more than once.

          To reproduce, create course and enrol the same user twice, as teacher and student or so. What does it show in the block?

          So one more iteration is ok for this issue IMO, as said, reopening... rejecting. Thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - I'm reopening this because: the site count() does not seem to need any distinct because the join with group_members cannot lead to multiple records per user returned... but the course count(), if I'm not wrong can return the same user multiple times if there are users with multiple roles in the same course. So perhaps the distinct could be necessary there to avoid counting the same user more than once. To reproduce, create course and enrol the same user twice, as teacher and student or so. What does it show in the block? So one more iteration is ok for this issue IMO, as said, reopening... rejecting. Thanks!
          Hide
          poltawski Dan Poltawski added a comment -

          Hi Eloy,

          I have tested that scenario and still it does not seem to require the distinct. (We could add it if you really think its necessary, but I do think that it negatively impacts the query performance?)

          I have rebased on the current 21_STABLE and submitting for integraiton.

          Show
          poltawski Dan Poltawski added a comment - Hi Eloy, I have tested that scenario and still it does not seem to require the distinct. (We could add it if you really think its necessary, but I do think that it negatively impacts the query performance?) I have rebased on the current 21_STABLE and submitting for integraiton.
          Hide
          stronk7 Eloy Lafuente (stronk7) 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
          stronk7 Eloy Lafuente (stronk7) 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
          nebgor Aparup Banerjee added a comment -

          stopping review - setting integrator to Eloy who's seen this previously.

          Show
          nebgor Aparup Banerjee added a comment - stopping review - setting integrator to Eloy who's seen this previously.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Some comments:

          1) The patch properly fixes the call to count_records_sql(), indeed, so I'm going to integrate this.

          2) BUT, that query is dynamically built using three parts: the main query (A), the enrol query (B) and the groups query (C). And my concerns, previously expressed are that, under situations where (B) or (C) are added, the count can return the same u.id multiple times, aka, counting on excess.

          3) Said that, it seems that (B) is safe because it has its own DISTINCT clause, so we won't get dupes here. And also (C) seems to be safe as far as it only adds members from ONE group, so no way to get dupes here.

          4) So, given 3) I think this can be implemented safely. Just trying to express my original concerns, now verified to be unnecessary.

          So.... integrating (I guess 20, 21 and master are the targets). Thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Some comments: 1) The patch properly fixes the call to count_records_sql(), indeed, so I'm going to integrate this. 2) BUT, that query is dynamically built using three parts: the main query (A), the enrol query (B) and the groups query (C). And my concerns, previously expressed are that, under situations where (B) or (C) are added, the count can return the same u.id multiple times, aka, counting on excess. 3) Said that, it seems that (B) is safe because it has its own DISTINCT clause, so we won't get dupes here. And also (C) seems to be safe as far as it only adds members from ONE group, so no way to get dupes here. 4) So, given 3) I think this can be implemented safely. Just trying to express my original concerns, now verified to be unnecessary. So.... integrating (I guess 20, 21 and master are the targets). Thanks!
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment - - edited

          Integrated (20, 21 and master), thanks Tim & Dan!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - - edited Integrated (20, 21 and master), thanks Tim & Dan!
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Passed thanks all

          Show
          samhemelryk Sam Hemelryk added a comment - Passed thanks all
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Done, your delicious hacks have been sent upstream, many thanks!

          Closing as fixed, ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Done, your delicious hacks have been sent upstream, many thanks! Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                28/Nov/11