Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.4.7, 2.5.3, 2.6
    • Component/s: SCORM
    • Labels:
    • Testing Instructions:
      Hide
      1. Create a new scorm activity.
      2. Goto reports tab as admin and click on graph.
      3. Make sure the graph is displayed properly.
      4. Make sure there are no "division by zero" warnings in your apache error logs.
      5. Attempt the scorm from a few different student accounts
      6. Go back to graph report as admin/teacher and make sure it is displayed correctly.
      Show
      Create a new scorm activity. Goto reports tab as admin and click on graph. Make sure the graph is displayed properly. Make sure there are no "division by zero" warnings in your apache error logs. Attempt the scorm from a few different student accounts Go back to graph report as admin/teacher and make sure it is displayed correctly.
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull Master Branch:
      MDL-42604-master

      Description

      While working on MDL-41979, when I clicked on the Reports tab as an Administrator after attending a SCO having provided status, time and score, no data is returned and I've found the 3 warnings below, in the web server error log:

      [Tue Oct 29 23:07:52 2013] [error] [client 192.168.0.202] PHP Warning:  Division by zero in /path/to/moodle-master/lib/graphlib.php on line 1021, referer: http://hostname/moodle-master/mod/scorm/report.php?id=88&mode=graphs
      [Tue Oct 29 23:07:52 2013] [error] [client 192.168.0.202] PHP Warning:  Division by zero in /path/to/moodle-master/lib/graphlib.php on line 1023, referer: http://hostname/moodle-master/mod/scorm/report.php?id=88&mode=graphs
      [Tue Oct 29 23:07:52 2013] [error] [client 192.168.0.202] PHP Warning:  Division by zero in /path/to/moodle-master/lib/graphlib.php on line 707, referer: http://hostname/moodle-master/mod/scorm/report.php?id=88&mode=graphs
      

      The same applies when clicking on the Graph report subtab: the image is missing and the warning are equal except for the missing of referer info.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            danmarsden Dan Marsden added a comment -

            Thanks Matteo - adding Ankit here as he wrote that report - Ankit do you want to pick this one up? - if not I might get to it one day.... Thanks!

            Show
            danmarsden Dan Marsden added a comment - Thanks Matteo - adding Ankit here as he wrote that report - Ankit do you want to pick this one up? - if not I might get to it one day.... Thanks!
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks Dan and Matteo.

            This issue describes two separate problems:-

            1. Admin attempts are not reported:- This has always been the case, even before the new reports were written. You need to be in a role with explicit 'mod/scorm:savetrack' capability for your attempts to be reported as get_users_by_capability() never returns admin accounts. Do we really need admin attempts to be reported here?
            2. The warnings in error log:- I have added a patch for that. Also I added an extra commit for master only with some code style cleanup.

            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks Dan and Matteo. This issue describes two separate problems:- Admin attempts are not reported:- This has always been the case, even before the new reports were written. You need to be in a role with explicit 'mod/scorm:savetrack' capability for your attempts to be reported as get_users_by_capability() never returns admin accounts. Do we really need admin attempts to be reported here? The warnings in error log:- I have added a patch for that. Also I added an extra commit for master only with some code style cleanup. Thanks
            Hide
            matteo Matteo Scaramuccia added a comment -

            TNX Ankit,
            I'll deep into your PR one of the next days.
            In the mean time two quick notes: if you run the simple sample described in 41979, in master will produce the issue while in 2.5 no i.e. that's the reason why I create this issue (sound like a functional regression).
            About excluding the admins' tracking statement: tracking data is recorded in the DB for both branches and I'm fine with excluding admins from those reports - I'd say that I'd include them but sounds like a personal preference -, just drop somewhere a comment in the docs or better in a FAQ... 2.5 shows the admins' attempts.

            TIA,
            Matteo

            Show
            matteo Matteo Scaramuccia added a comment - TNX Ankit, I'll deep into your PR one of the next days. In the mean time two quick notes: if you run the simple sample described in 41979 , in master will produce the issue while in 2.5 no i.e. that's the reason why I create this issue (sound like a functional regression). About excluding the admins' tracking statement: tracking data is recorded in the DB for both branches and I'm fine with excluding admins from those reports - I'd say that I'd include them but sounds like a personal preference -, just drop somewhere a comment in the docs or better in a FAQ... 2.5 shows the admins' attempts. TIA, Matteo
            Hide
            ankit_frenz Ankit Agarwal added a comment - - edited

            Hi Matteo,

            Can you please verify that you didn't enroll your admin as student in the course? I just re-tested to confirm, I do not get admin attempts reported in 2.5 as well. (The tracks are stored irrespective)

            Show
            ankit_frenz Ankit Agarwal added a comment - - edited Hi Matteo, Can you please verify that you didn't enroll your admin as student in the course? I just re-tested to confirm, I do not get admin attempts reported in 2.5 as well. (The tracks are stored irrespective)
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Ankit,
            good spotting! Yes, I enrolled the admin as Student in the course running 2.5: I've missed this important difference, it was late evening .

            Show
            matteo Matteo Scaramuccia added a comment - Hi Ankit, good spotting! Yes, I enrolled the admin as Student in the course running 2.5 : I've missed this important difference, it was late evening .
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Ankit,
            find below my peer review:

            [Y] Syntax
            [N] Whitespace
            [-] Output
            [-] Language
            [-] Databases
            [Y] Testing (instructions and automated tests)
            [-] Security
            [-] Documentation
            [Y] Git
            [-] Third party code
            [Y] Sanity check
            

            The only trivial issue is a couple of extra blank space before the dot in master, line 139 and line 159.

            For my reference:

            • Apply PR to master branch:

              $ cd moodle-master
              $ git status
              $ git pull
              $ curl https://github.com/ankitagarwal/moodle/compare/68291f2...MDL-42604-master.patch | git apply -
              $ git diff
              

            • Apply PR to MOODLE_25_STABLE branch:

              $ cd moodle-25
              $ git status
              $ git pull
              $ curl https://github.com/ankitagarwal/moodle/compare/1115bb3...MDL-42604-25.patch | git apply -
              $ git diff
              

            Show
            matteo Matteo Scaramuccia added a comment - Hi Ankit, find below my peer review: [Y] Syntax [N] Whitespace [-] Output [-] Language [-] Databases [Y] Testing (instructions and automated tests) [-] Security [-] Documentation [Y] Git [-] Third party code [Y] Sanity check The only trivial issue is a couple of extra blank space before the dot in master , line 139 and line 159 . For my reference: Apply PR to master branch: $ cd moodle-master $ git status $ git pull $ curl https://github.com/ankitagarwal/moodle/compare/68291f2...MDL-42604-master.patch | git apply - $ git diff Apply PR to MOODLE_25_STABLE branch: $ cd moodle-25 $ git status $ git pull $ curl https://github.com/ankitagarwal/moodle/compare/1115bb3...MDL-42604-25.patch | git apply - $ git diff
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks Matteo for the review,

            Patch updated, pushing forward.

            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks Matteo for the review, Patch updated, pushing forward. Thanks
            Hide
            marina Marina Glancy added a comment -

            hi Ankit,
            this line looks suspicious:

            $gridlines = max(2, ($gridlines + 1)); // We need a minimum of two lines.

            should not it be

            min(array(2, $gridlines + 1))

            ?

            Show
            marina Marina Glancy added a comment - hi Ankit, this line looks suspicious: $gridlines = max(2, ($gridlines + 1)); // We need a minimum of two lines. should not it be min(array(2, $gridlines + 1)) ?
            Hide
            marina Marina Glancy added a comment -

            Please add to testing instructions checking graph with data

            Show
            marina Marina Glancy added a comment - Please add to testing instructions checking graph with data
            Hide
            ankit_frenz Ankit Agarwal added a comment - - edited

            Hi Marina,

            The Number of gridlines depends on the highest number, on the Y-axis. The minimum for this should be 2 (bottom and top line), the max is variable. Hence

            $gridlines = max(2, ($gridlines + 1));
            

            is correct, we can't use min there.

            I have updated the testing instructions.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - - edited Hi Marina, The Number of gridlines depends on the highest number, on the Y-axis. The minimum for this should be 2 (bottom and top line), the max is variable. Hence $gridlines = max(2, ($gridlines + 1)); is correct, we can't use min there. I have updated the testing instructions. Thanks
            Hide
            marina Marina Glancy added a comment -

            Ankit, you do realise, that max(2, $gridlines+1) will never be more than 2, even if $gridlines is 10?

            Show
            marina Marina Glancy added a comment - Ankit, you do realise, that max(2, $gridlines+1) will never be more than 2, even if $gridlines is 10?
            Hide
            marina Marina Glancy added a comment -

            or maybe I should just stop working at 7 pm..... Sorry Ankit

            Show
            marina Marina Glancy added a comment - or maybe I should just stop working at 7 pm..... Sorry Ankit
            Hide
            marina Marina Glancy added a comment -

            Thanks Ankit, integrated in 2.4, 2.5 and 2.6

            Show
            marina Marina Glancy added a comment - Thanks Ankit, integrated in 2.4, 2.5 and 2.6
            Hide
            skodak Petr Skoda added a comment -

            works for me, thanks

            Show
            skodak Petr Skoda added a comment - works for me, thanks
            Hide
            damyon Damyon Wiese added a comment -

            Here lies 52 bugs.
            All fixed or swept under a rug.
            If they come back one day,
            To our dismay,
            We all will feel quite un-smug.

            Thanks for the reporting/fixing/testing on this issue. It has been sent upstream.

            Show
            damyon Damyon Wiese added a comment - Here lies 52 bugs. All fixed or swept under a rug. If they come back one day, To our dismay, We all will feel quite un-smug. Thanks for the reporting/fixing/testing on this issue. It has been sent upstream.

              People

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

                Dates

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