Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      16940

      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.

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

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

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

          adding screens

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

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

          Up for review
          Thanks

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

          looks good to me - +1 to integrate

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

          Thanks Dan

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

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

          rebased
          Thanks

          Show
          Ankit Agarwal added a comment - rebased Thanks
          Hide
          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
          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
          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
          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 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 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
          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
          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 Agarwal added a comment -

          rebased!

          Show
          Ankit Agarwal added a comment - rebased!
          Hide
          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
          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 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 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
          Sam Hemelryk added a comment -

          Thanks Ankit this has been integrated now.

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

          This is working great.

          Thanks

          Test passed.

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

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

          Show
          Ankit Agarwal added a comment - Thanks guys! Nice to see this integrated! yay!!
          Hide
          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
          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
          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
          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: