Moodle
  1. Moodle
  2. MDL-35775

Support for groupings in group import

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.5
    • Component/s: Groups
    • Labels:
    • Testing Instructions:
      Hide

      Test 1 (No regressions)

      1. Create a group import file as detailed at http://docs.moodle.org/23/en/Import_groups
      2. Go to the group import page in a course and upload the file
      3. Check that the groups have been created without errors, and no PHP notices have been shown

      Test 2 (New functionality)

      1. Create a group import file as detailed at http://docs.moodle.org/23/en/Import_groups. Make sure the file has at least a dozen groups.
      2. Add a column called "groupingname" and assign groups to groupings of 3 groups
      3. Go to the group import page in a course and upload the file
      4. Check that the groupings have been created (once each) and that the correct groups have been assigned to them
      Show
      Test 1 (No regressions) Create a group import file as detailed at http://docs.moodle.org/23/en/Import_groups Go to the group import page in a course and upload the file Check that the groups have been created without errors, and no PHP notices have been shown Test 2 (New functionality) Create a group import file as detailed at http://docs.moodle.org/23/en/Import_groups . Make sure the file has at least a dozen groups. Add a column called "groupingname" and assign groups to groupings of 3 groups Go to the group import page in a course and upload the file Check that the groupings have been created (once each) and that the correct groups have been assigned to them
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-35775-master
    • Rank:
      44527

      Description

      It would be extremely useful if the group import allowed the new groups to be assigned to groupings.

        Issue Links

          Activity

          Hide
          Michael Aherne added a comment -

          Attached a patch that implements this.

          Show
          Michael Aherne added a comment - Attached a patch that implements this.
          Hide
          Michael de Raadt added a comment -

          Sounds like a good idea.

          Show
          Michael de Raadt added a comment - Sounds like a good idea.
          Hide
          Michael de Raadt added a comment -

          Feel free to request a peer review on this when you are ready.

          Show
          Michael de Raadt added a comment - Feel free to request a peer review on this when you are ready.
          Hide
          Dan Poltawski added a comment -

          Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..

          Show
          Dan Poltawski added a comment - Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Dan Poltawski added a comment -

          This went straight through to integration but i'm sending it back for peer review first because its missing testing instructions and could do with another set of eyes.

          cheers!

          Show
          Dan Poltawski added a comment - This went straight through to integration but i'm sending it back for peer review first because its missing testing instructions and could do with another set of eyes. cheers!
          Hide
          Michael Aherne added a comment -

          Thanks for the feedback, Dan! I've added some testing instructions. Cheers, Michael.

          Show
          Michael Aherne added a comment - Thanks for the feedback, Dan! I've added some testing instructions. Cheers, Michael.
          Hide
          Rex Lorenzo added a comment -

          Some comments:

          • Line 199: if ($newgroup->groupingname)

          This will give a PHP notice if groupingname wasn't set in the import file. Try an isset check. Also, make sure in the testing instructions that an import file without the additional groupingname field also works, so there are no regressions.

          • Line 215: When you are checking the result of groups_assign_grouping, it never returns false. Rather the return value is either true or an exception is thrown. If you are handling the case in which a new grouping fails, add a try/catch block instead of an if/else
          Show
          Rex Lorenzo added a comment - Some comments: Line 199: if ($newgroup->groupingname) This will give a PHP notice if groupingname wasn't set in the import file. Try an isset check. Also, make sure in the testing instructions that an import file without the additional groupingname field also works, so there are no regressions. Line 215: When you are checking the result of groups_assign_grouping, it never returns false. Rather the return value is either true or an exception is thrown. If you are handling the case in which a new grouping fails, add a try/catch block instead of an if/else
          Hide
          Michael Aherne added a comment - - edited

          Thanks for the feedback. I've made the changes you suggested. For some reason I wasn't seeing the PHP notice when the groupingname was missing, even though I'm displaying all debugging messages, but you're right - it should be there! I've also rebased to the latest master.

          Show
          Michael Aherne added a comment - - edited Thanks for the feedback. I've made the changes you suggested. For some reason I wasn't seeing the PHP notice when the groupingname was missing, even though I'm displaying all debugging messages, but you're right - it should be there! I've also rebased to the latest master.
          Hide
          Rex Lorenzo added a comment -

          Looks good to me. Moodle.org is busy with getting Moodle 2.4 out the door the first week of December and are in a code freeze that is preventing them from adding in new features.

          But after Moodle 2.4 is out, you can try pinging for progress on testing of this patch.

          Show
          Rex Lorenzo added a comment - Looks good to me. Moodle.org is busy with getting Moodle 2.4 out the door the first week of December and are in a code freeze that is preventing them from adding in new features. But after Moodle 2.4 is out, you can try pinging for progress on testing of this patch.
          Hide
          Rex Lorenzo added a comment -

          Assigning to component lead to await testing/integration review

          Show
          Rex Lorenzo added a comment - Assigning to component lead to await testing/integration review
          Hide
          Dan Poltawski added a comment -

          Assigning to Michael, since its his issue and should take credit, and pushing for review

          Show
          Dan Poltawski added a comment - Assigning to Michael, since its his issue and should take credit, and pushing for review
          Hide
          Dan Poltawski added a comment -

          BTW, I added the docs_required flag as I suppose this will need adding to moodle docs after integration.

          Show
          Dan Poltawski added a comment - BTW, I added the docs_required flag as I suppose this will need adding to moodle docs after integration.
          Hide
          Dan Poltawski added a comment -

          I've added the tag 'integration_held', as we are keeping master and 24_STABLE in sync without new features for a few weeks. We'll integrate this after that period is over.

          Show
          Dan Poltawski added a comment - I've added the tag 'integration_held', as we are keeping master and 24_STABLE in sync without new features for a few weeks. We'll integrate this after that period is over.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (master only), thanks!

          If anybody is interested about backporting this to 2.4, plz. propose it into a separate issue. This is a new feature, hence, lands only to master by default.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks! If anybody is interested about backporting this to 2.4, plz. propose it into a separate issue. This is a new feature, hence, lands only to master by default. Ciao
          Hide
          Frédéric Massart added a comment -

          Test passed. Thanks!

          Show
          Frédéric Massart added a comment - Test passed. Thanks!
          Hide
          Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!
          Hide
          Mary Cooch added a comment -

          Removing qa_test_required as there is a test for this now MDLQA-5266 ready for the next cycle.

          Show
          Mary Cooch added a comment - Removing qa_test_required as there is a test for this now MDLQA-5266 ready for the next cycle.
          Hide
          Mary Cooch added a comment -

          Removing docs_required label as this was documented in http://docs.moodle.org/25/en/Import_groups

          Show
          Mary Cooch added a comment - Removing docs_required label as this was documented in http://docs.moodle.org/25/en/Import_groups

            People

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

              Dates

              • Created:
                Updated:
                Resolved: