Details

    • Type: New Feature
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.2
    • Fix Version/s: 2.3
    • Component/s: SCORM
    • Labels:
    • 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

          Attachments

          1. allgroups.png
            allgroups.png
            57 kB
          2. group1.png
            group1.png
            55 kB
          3. group2.png
            group2.png
            55 kB
          4. multiscoes.png
            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