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

          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