Moodle
  1. Moodle
  2. MDL-26568

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      16150

      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()
      1. patch_to_expose_issue.patch
        0.5 kB
        Dan Poltawski
      1. Online-Users.jpg
        5 kB

        Activity

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

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

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

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

        Show
        Michael Blake added a comment - This issue is causing problems for MP client. Please give it priority.
        Hide
        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
        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
        Tim Lock added a comment - - edited

        Online User showing number of users incorrectly (patched).

        Show
        Tim Lock added a comment - - edited Online User showing number of users incorrectly (patched).
        Hide
        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
        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
        Dan Poltawski added a comment -

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

        Show
        Dan Poltawski added a comment - Ahha, so its incorrectly reporting the number of users in the summary if there are over 50.
        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        Aparup Banerjee added a comment -

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

        Show
        Aparup Banerjee added a comment - stopping review - setting integrator to Eloy who's seen this previously.
        Hide
        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
        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
        Eloy Lafuente (stronk7) added a comment - - edited

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

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

        Passed thanks all

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

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

        Closing as fixed, ciao

        Show
        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: