Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.4.7, 2.5.3, 2.6
    • Component/s: SCORM
    • Labels:
    • Rank:
      54432

      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.

        Activity

        Hide
        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
        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 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 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 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 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 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 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 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 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 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 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 Agarwal added a comment -

        Thanks Matteo for the review,

        Patch updated, pushing forward.

        Thanks

        Show
        Ankit Agarwal added a comment - Thanks Matteo for the review, Patch updated, pushing forward. Thanks
        Hide
        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 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 Glancy added a comment -

        Please add to testing instructions checking graph with data

        Show
        Marina Glancy added a comment - Please add to testing instructions checking graph with data
        Hide
        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 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 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 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 Glancy added a comment -

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

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

        Thanks Ankit, integrated in 2.4, 2.5 and 2.6

        Show
        Marina Glancy added a comment - Thanks Ankit, integrated in 2.4, 2.5 and 2.6
        Hide
        Petr Škoda added a comment -

        works for me, thanks

        Show
        Petr Škoda added a comment - works for me, thanks
        Hide
        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 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: