Moodle
  1. Moodle
  2. MDL-27438

Course groups don't backup correctly

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.1, 2.2
    • Fix Version/s: 2.0.4, 2.1.1
    • Component/s: Backup
    • Labels:
      None
    • Database:
      MySQL
    • Testing Instructions:
      Hide
      1. Create one course (important, the course must have NO activities using groups at all).
      2. Enrol some users in the course
      3. Create some groups and add users to them
      4. Create some groupings and add groups to them
      5. Backup the course with users info
      6. Restore the course into new course
      7. TEST: Verify that groups, groupings and group members are 100% the same than the original ones.
      8. Backup the course without user info
      9. Restore the course into new course
      10. TEST: Verify that groups and groupings are 100% the same than the original ones (no group members this time).
      Show
      Create one course (important, the course must have NO activities using groups at all). Enrol some users in the course Create some groups and add users to them Create some groupings and add groups to them Backup the course with users info Restore the course into new course TEST: Verify that groups, groupings and group members are 100% the same than the original ones. Backup the course without user info Restore the course into new course TEST: Verify that groups and groupings are 100% the same than the original ones (no group members this time).
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      backup_groups_master
    • Rank:
      17113

      Description

      To reproduce:

      1. Backup a course that has one or more groups
      2. Observe that the groups.xml does not contain any groups in the backup file

      I was able to reproduce this on demo.moodle.org. The backup from that environment is attached.

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          Aha,

          I think I got it. groups are "annotated" as they are "used", where used means some user/activity/grouping have them.

          In one course without users and groupings and with activities not referencing to them (yet, because of no user data), they aren't considered "used".

          Said that... should I consider "used" (aka, included in backup, all the course groups, always?). Or via setting? Or only when no user data is being backup? Perhaps the safest way is to add all them always and done.

          Thinking... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Aha, I think I got it. groups are "annotated" as they are "used", where used means some user/activity/grouping have them. In one course without users and groupings and with activities not referencing to them (yet, because of no user data), they aren't considered "used". Said that... should I consider "used" (aka, included in backup, all the course groups, always?). Or via setting? Or only when no user data is being backup? Perhaps the safest way is to add all them always and done. Thinking... ciao
          Hide
          Chris Scribner added a comment -

          Ah, I see. It makes sense why I ran into the problem them and it hasn't been reported as a general issue.

          For what we need, groups should back up even if they have no members / references. We're considering groups part of the course workflow.

          As an example, a teacher sets up a group for students which have access to certain content, and creates some rules in the Personalized Learning Designer (a component we are developing) which actually unlocks the content for those users. It's important that we maintain the group across course copies so the workflow can be configured once and reused later.

          Do you see any harm in including groups that aren't "referenced" otherwise?

          Thanks,

          Chris

          Show
          Chris Scribner added a comment - Ah, I see. It makes sense why I ran into the problem them and it hasn't been reported as a general issue. For what we need, groups should back up even if they have no members / references. We're considering groups part of the course workflow. As an example, a teacher sets up a group for students which have access to certain content, and creates some rules in the Personalized Learning Designer (a component we are developing) which actually unlocks the content for those users. It's important that we maintain the group across course copies so the workflow can be configured once and reused later. Do you see any harm in including groups that aren't "referenced" otherwise? Thanks, Chris
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Yeah, so surely this approach is correct:

          1) In course backups, consider group definitions as integral part of it, so all them will be included in backup, no matter they are referenced or no.

          2) In section and activity backups, continue with current approach, so only referenced groups will be included.

          3) Of course, group members (users) only will be included if the backup has been configured to include them.

          4) Exactly the same than 1) and 2) apply to groupings.

          Sounds ok?

          PS: Note the there is one alternative that implies creating two new settings on backup & restore in order to be able to decide if we want to handle course groups/groupings or no, but I prefer to keep the number of settings small, so I won't be adding them initially.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Yeah, so surely this approach is correct: 1) In course backups, consider group definitions as integral part of it, so all them will be included in backup, no matter they are referenced or no. 2) In section and activity backups, continue with current approach, so only referenced groups will be included. 3) Of course, group members (users) only will be included if the backup has been configured to include them. 4) Exactly the same than 1) and 2) apply to groupings. Sounds ok? PS: Note the there is one alternative that implies creating two new settings on backup & restore in order to be able to decide if we want to handle course groups/groupings or no, but I prefer to keep the number of settings small, so I won't be adding them initially.
          Hide
          Chris Scribner added a comment -

          Your proposal sounds perfect to me.

          I don't really think settings are necessary, but if you do add them, please default them to on (at least in the case of course backup)

          Thanks,

          Chris

          Show
          Chris Scribner added a comment - Your proposal sounds perfect to me. I don't really think settings are necessary, but if you do add them, please default them to on (at least in the case of course backup) Thanks, Chris
          Hide
          Sam Marshall added a comment -

          Just to note as I was looking into this issue today...

          Eloy - I agree your approach is correct. Course backups should include all groups and groupings even if currently empty and not referenced.

          Show
          Sam Marshall added a comment - Just to note as I was looking into this issue today... Eloy - I agree your approach is correct. Course backups should include all groups and groupings even if currently empty and not referenced.
          Hide
          Sam Marshall added a comment -

          Also btw - it currently doesn't backup groups even if they contain members, if they are not used in an activity.

          Show
          Sam Marshall added a comment - Also btw - it currently doesn't backup groups even if they contain members, if they are not used in an activity.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sent to integration!

          Show
          Eloy Lafuente (stronk7) added a comment - Sent to integration!
          Hide
          Sam Hemelryk added a comment -

          Thanks Eloy - this has been integrated now

          In regards to the redundant backup_annotate_groups_from_groupings is there an MDL already to create a setting or review the need for the setting? or does one need to be created still?

          Show
          Sam Hemelryk added a comment - Thanks Eloy - this has been integrated now In regards to the redundant backup_annotate_groups_from_groupings is there an MDL already to create a setting or review the need for the setting? or does one need to be created still?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          No, I just kept it there because when doing because it's one step that can easily be forgotten if some day we decide to make the backup_annotate_course_groups_and_groupings() optional.

          This steps causes backup_annotate_groups_from_groupings to perform some (max = groupings*groups) extra queries but that, in the flood of queries performed by backup, shouldn't be really important IMO.

          So I think we can leave it as is or... alternatively... switch both steps, uhm... that way backup_annotate_course_groups_and_groupings() will only have to process 1 grouping (max = 1 + groups).

          Bah, really... I think it does not matter much, I'd consider it relatively ok. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - No, I just kept it there because when doing because it's one step that can easily be forgotten if some day we decide to make the backup_annotate_course_groups_and_groupings() optional. This steps causes backup_annotate_groups_from_groupings to perform some (max = groupings*groups) extra queries but that, in the flood of queries performed by backup, shouldn't be really important IMO. So I think we can leave it as is or... alternatively... switch both steps, uhm... that way backup_annotate_course_groups_and_groupings() will only have to process 1 grouping (max = 1 + groups). Bah, really... I think it does not matter much, I'd consider it relatively ok. Ciao
          Hide
          Andrew Davis added a comment -

          It all works as described

          Show
          Andrew Davis added a comment - It all works as described
          Hide
          Sam Hemelryk added a comment -

          Congratulations - this fix has just been released in the weeklies.

          Show
          Sam Hemelryk added a comment - Congratulations - this fix has just been released in the weeklies.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: