Details

    • Type: New Feature
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.2
    • Fix Version/s: 2.3
    • Component/s: SCORM
    • Testing Instructions:
      Hide
      1. Deploy the following sco packages (or any other single and multiple sco package you use) :-
        http://moodle.org/mod/data/view.php?d=50&rid=4083&filter=1
        http://moodle.org/mod/data/view.php?d=50&rid=1655
      2. Create two groups if you already dont have any from course admin>users>groups. Let's name these groups to be group 1 and group 2.
      3. Goto Course> Edit settings, Set group mode to "visible" and set force group mode to "Yes" and save.
      4. Enrol 4 students a,b,c and d if you already dont have any.
      5. Assign a,b to group 1 and c,d to group 2
      6. Login as a,b,c,d and submit a few attempts from each student on each scorm pack.
      7. Make sure you exit the scorm package properly after the attempt has been finished by clicking on "Exit activity" on top right
      8. Now browse to scorm>reports>graphs as admin and make sure you can see a graph with correct data.
      9. Make sure you can see a group selection drop down on top left.
      10. Change groups and validate you see only data specific to the group you selected.
      11. Check above three points for both scorm package
      12. Also make sure there is an empty chart displayed when there are no students to report.
      Show
      Deploy the following sco packages (or any other single and multiple sco package you use) :- http://moodle.org/mod/data/view.php?d=50&rid=4083&filter=1 http://moodle.org/mod/data/view.php?d=50&rid=1655 Create two groups if you already dont have any from course admin>users>groups. Let's name these groups to be group 1 and group 2. Goto Course> Edit settings, Set group mode to "visible" and set force group mode to "Yes" and save. Enrol 4 students a,b,c and d if you already dont have any. Assign a,b to group 1 and c,d to group 2 Login as a,b,c,d and submit a few attempts from each student on each scorm pack. Make sure you exit the scorm package properly after the attempt has been finished by clicking on "Exit activity" on top right Now browse to scorm>reports>graphs as admin and make sure you can see a graph with correct data. Make sure you can see a group selection drop down on top left. Change groups and validate you see only data specific to the group you selected. Check above three points for both scorm package Also make sure there is an empty chart displayed when there are no students to report.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      scorm_graph_report

      Description

      Develop graph based reports for Scorm module. The basic idea is to generate a graph with distribution of (%) secured among various students.

      This is not a bug but an new-feature.

        Gliffy Diagrams

        1. allgroups.png
          57 kB
        2. group1.png
          55 kB
        3. group2.png
          55 kB
        4. multiscoes.png
          177 kB

          Issue Links

            Activity

            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Remaining things Todo:-
            Check on Groups support.
            Add to core plugin list.

            Show
            ankit_frenz Ankit Agarwal added a comment - Remaining things Todo:- Check on Groups support. Add to core plugin list.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            adding screens

            Show
            ankit_frenz Ankit Agarwal added a comment - adding screens
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Group support added
            Plugin added to core list
            version files added
            Commits squashed

            Up for review
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Group support added Plugin added to core list version files added Commits squashed Up for review Thanks
            Hide
            danmarsden Dan Marsden added a comment -

            looks good to me - +1 to integrate

            Show
            danmarsden Dan Marsden added a comment - looks good to me - +1 to integrate
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks Dan

            @integrators
            feel free to integrate this when you start accepting master only commits.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks Dan @integrators feel free to integrate this when you start accepting master only commits. Thanks
            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
            samhemelryk Sam Hemelryk added a comment -

            Thanks guys, I've updated the fix version to 2.3 and indeed this will sit in the integration queue until such time that we start integrating master only changes.
            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks guys, I've updated the fix version to 2.3 and indeed this will sit in the integration queue until such time that we start integrating master only changes. Cheers Sam
            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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

            This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

            This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

            Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
            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
            ankit_frenz Ankit Agarwal added a comment -

            rebased
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - rebased Thanks
            Hide
            dougiamas Martin Dougiamas added a comment -

            I hadn't posted before but it gets my "Lead developer" stamp of approval to add to core (assuming it passes integration reviews of course). +MD

            Show
            dougiamas Martin Dougiamas added a comment - I hadn't posted before but it gets my "Lead developer" stamp of approval to add to core (assuming it passes integration reviews of course). +MD
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Ankit,

            I'm sending this back at the moment so that the following can be fixed up:

            1. @package and friends should be changed to the new system we have settled upon using. (including @author should be @copyright)
            2. graph.php when calling require_login pass the course object rather than just its id. This will save a database call.
            3. When fetching context instances we should use the new API when possible. e.g. $context = context_module::instance($cm->id);
            4. You are missing a file: mod/scorm/report/graphs/reportsettings_form.php
            5. report.php
              • Unused globals
              • check the indenting on line 40
              • spacing line 45
              • phpdocs for the class.
            6. There are two params for graph.php scoid, and cmid. If I read things right an a cm instance has sco instances right? If so then it looks like there is a security hole there, you could pass a cmid that you have access within and any scoid. To fix this either you only pass scoid, and then get the cmid from that OR you check that the scoid belongs to the cmid you have. Certainly to me passing just one argument seems the better way, although perhaps I have missed something here so please correct me if I've misunderstood scorm structure.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Ankit, I'm sending this back at the moment so that the following can be fixed up: @package and friends should be changed to the new system we have settled upon using. (including @author should be @copyright) graph.php when calling require_login pass the course object rather than just its id. This will save a database call. When fetching context instances we should use the new API when possible. e.g. $context = context_module::instance($cm->id); You are missing a file: mod/scorm/report/graphs/reportsettings_form.php report.php Unused globals check the indenting on line 40 spacing line 45 phpdocs for the class. There are two params for graph.php scoid, and cmid. If I read things right an a cm instance has sco instances right? If so then it looks like there is a security hole there, you could pass a cmid that you have access within and any scoid. To fix this either you only pass scoid, and then get the cmid from that OR you check that the scoid belongs to the cmid you have. Certainly to me passing just one argument seems the better way, although perhaps I have missed something here so please correct me if I've misunderstood scorm structure. Cheers Sam
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks Sam for the pointing out the issues.
            I have fixed all issues..sending this for another round of review.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks Sam for the pointing out the issues. I have fixed all issues..sending this for another round of review. Thanks
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Some hours ago...

            the main moodle.git repository has 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 - Some hours ago... the main moodle.git repository has 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
            ankit_frenz Ankit Agarwal added a comment -

            rebased!

            Show
            ankit_frenz Ankit Agarwal added a comment - rebased!
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Ankit,

            Sending this back again so that the following can be touched up:

            1. graph.php should be defining the following so that any errors that may occur during production of the graph don't result in messy images (reference theme/image.php)

              define('NO_DEBUG_DISPLAY', true);

            2. Strings should not be using camel case.. Graph Report ===> Graph report
            3. graph.php is using $OUTPUT->notification which will 100% cause an error in image production.
            4. indenting line 93

            It's looking pretty spot on now and those are all minor things.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Ankit, Sending this back again so that the following can be touched up: graph.php should be defining the following so that any errors that may occur during production of the graph don't result in messy images (reference theme/image.php) define('NO_DEBUG_DISPLAY', true); Strings should not be using camel case.. Graph Report ===> Graph report graph.php is using $OUTPUT->notification which will 100% cause an error in image production. indenting line 93 It's looking pretty spot on now and those are all minor things. Cheers Sam
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Sam,
            Made all the changes and also done some improvments on the report for the case when there are no students to report.
            Hope it gets through this time.

            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Sam, Made all the changes and also done some improvments on the report for the case when there are no students to report. Hope it gets through this time. Thanks
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Ankit this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Ankit this has been integrated now.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            This is working great.

            Thanks

            Test passed.

            Show
            rwijaya Rossiani Wijaya added a comment - This is working great. Thanks Test passed.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks guys!
            Nice to see this integrated!
            yay!!

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks guys! Nice to see this integrated! yay!!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            FCT (fixed, closing, thanks). Ciao

            "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!"
            ~ Benjamin Disraeli

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - FCT (fixed, closing, thanks). Ciao "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!" ~ Benjamin Disraeli
            Hide
            tsala Helen Foster added a comment -

            Ankit and Dan, thanks for this new feature in Moodle 2.3. I've added a brief note of it in the user docs: http://docs.moodle.org/23/en/Using_SCORM

            Help coming up with a proper description and a nice screenshot to illustrate it would be much appreciated!

            Show
            tsala Helen Foster added a comment - Ankit and Dan, thanks for this new feature in Moodle 2.3. I've added a brief note of it in the user docs: http://docs.moodle.org/23/en/Using_SCORM Help coming up with a proper description and a nice screenshot to illustrate it would be much appreciated!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12