Moodle
  1. Moodle
  2. MDL-32005

groups and groupings should have 'idnumber' field

    Details

    • Testing Instructions:
      Hide

      These test instructions will:

      • Create some initial test data
      • Attempt to manipulate (edit, delete) it both:
        • with permission to change the idnumber; and
        • without permission to change the idnumber.
      • test uniqueness constraints
      • test backup/restore of group(ing)s with idnumbers
      • test importing data from CSV
      • test automatic creation of groups and placement into groupings

      Note: The idnumber has been marked as an advanced setting in the forms. You will need to toggle the Show Advanced button to reveal it.

      Run Unit Tests

      A set of unit tests have been created for this feature. These can either be run as a full set of unit tests, or in isolation with:

      phpunit --verbose lib/tests/grouplib_test.php
      

      Create Test Data

      • Navigate to Settings -> Course administration -> Users -> Groups
      • Create new groups:
        name idnumber
        group-id-1 group-id-1
        group-id-2 group-id-2
        group-noid-1  
        group-noid-2  
      • Open the Groupings tab
      • Create new groupings:
        name idnumber
        grouping-id-1 grouping-id-1
        grouping-id-2 grouping-id-2
        grouping-noid-1  
        grouping-noid-2  

      Test initial deletion

      As a teacher:

      • Open the 'Groups' tab again
      • Select group-id-1
      • Choose 'Delete selected group'
        • Choose yes on the confirmation page
        • Confirm that the group was removed
      • Select group-noid-1
      • Choose 'Delete selected group'
        • Choose yes on the confirmation page
        • Confirm that the group was removed
      • Open the 'Groupings' tab
      • Choose the delete icon on grouping-id-1
        • Choose yes on the confirmation page
        • Confirm that the grouping was removed
      • Choose the delete icon on grouping-noid-1
        • Choose yes on the confirmation page
        • Confirm that the grouping was removed

      Remove permission

      • Remove the moodle/course:changeidnumber capability from the editingteacher role

      Re-Test deletion

      As a teacher:

      • Open the 'Groups' tab again
      • Select group-id-2
      • The delete button should be greyed out

      For extra brownie points, open an inspector and un-grey it

      • Choose 'Delete selected group'
        • Confirm that an error was shown informing the user that they cannot remove the group
      • Select group-noid-2
        • Confirm that the Delete selected group button is no longer greyed out
      • Select both group-id-2 and group-noid-2
        • Confirm that the button is once again greyed out
      • Select group-noid-2
      • Choose 'Delete selected group'
        • Choose yes on the confirmation page
        • Confirm that the group was removed
      • Open the 'Groupings' tab
      • Confirm that there is no delete icon for grouping-id-2
        • Make a note of the id in the address bar for the Grouping-2 edit page
      • Choose the delete icon on grouping-noid-2
        • Replace the id in the address bar with that of grouping-id-2
      • Choose the delete icon on grouping-noid-2
        • Choose yes on the confirmation page
        • Confirm that the grouping was removed

      Test data re-entry

      • Choose the 'Groups' tab again
      • Click the 'Create group' button
      • Create a new group:
        • Use a group name of group-noid-3
        • Confirm that it is not possible to set the idnumber any more
        • Click 'Save changes'
      • Open the 'Groupings' tab again
      • Click the 'Create grouping' button
      • Create a new grouping:
        • Use a grouping name of grouping-noid-3
        • Confirm that it is not possible to set the idnumber any more
        • Click 'Save changes'

      Test update existing entries

      Groups

      Choose the 'Groups' tab again

      • Select group-id-2
      • Choose 'Edit group settings'
        • Change the group name to group-id-2-updated
        • Confirm that you cannot change the idnumber
        • Choose save
      • Select the same group again and click the 'Edit group settings' button
        • Confirm the new name
        • Confirm that the idnumber is still present

      Groupings

      Choose the 'Groupings' tab again

      • Select the edit icon for grouping-id-2
        • Change the group name to grouping-id-2-updated
        • Confirm that you cannot change the idnumber
        • Choose save
      • Select the edit icon for the grouping again
        • Confirm the new name
        • Confirm that the idnumber is still present

      Testing with a user with the permission

      Login as admin

      Groups

      Choose the Groups tab

      • Select group-id-2-updated and choose 'Edit group settings'
        • Change the name again to group-id-2
        • Change the idnumber to group-id-two
        • Choose save settings
      • Select the group again and edit it's settings again
        • Confirm that the new name took
        • Confirm that the idnumber was changed
      • Go back to the edit page
      • Select the group and choose 'Delete selected group'
      • Click yes on the confirmation page
      • Confirm that the group was removed
      • Remove group-noid-3

      Groupings

      Choose the Groupings tab

      • Select grouping-id-2-updated's edit button
        • Change the name to grouping-id-2
        • Change the idnumber
        • Choose save settings
      • Select the grouping again and edit it's settings again
        • Confirm that the new name took
        • Confirm that the idnumber was changed
      • Go back to the edit page
      • Select grouping-id-2's delete button
      • Click yes on the confirmation page
      • Confirm that the group was removed
      • Remove grouping-noid-3

      Testing Uniqueness

      User: As a user with permission to change the idnumber
      These tests check that it is not possible to create group(ing)s with a duplicate idnumber. Please note that:

      • idnumbers are unique within a 'course' (not sitewide); and
      • it is possible to have blank idnumbers for multiple items but this was tested in the initial data creation test instructions.

      Groups

      Navigate to the Groups tab again:

      • Choose Create Group:
      • Choose Create Group:
        • name 'group-x'
        • idnumber 'MDL-32005'
        • Confirm that the group was not created and an error was shown on the form
        • Change the idnumber to 'group-x' and save
      • Re-edit 'group-x':
        • Update idnumber to 'MDL-32005'
        • Choose Save
        • Confirm that the group was not updated and an error was shown on the form
      • Delete both groups

      Groupings

      Navigate to the Groupings tab again:

      • Choose Create Grouping:
      • Choose Create Grouping:
        • name 'grouping-x'
        • idnumber 'MDL-32005'
        • Confirm that the group was not created and an error was shown on the form
        • Change the idnumber to 'grouping-x' and save
      • Re-edit 'grouping-x':
        • Update idnumber to 'MDL-32005'
        • Choose Save
        • Confirm that the group was not updated and an error was shown on the form
      • Delete both groupings

      Backup/Restore with permissions

      User: As an administrator (permission required to backup user data)

      Create several groups:

      Group Name idnumber
      group-id-1 group-id-1
      group-id-2 group-id-2
      group-noid-1  
      group-noid-2  

      And a pair of groupings:

      Name idnumber
      grouping-id grouping-id
      grouping-noid  

      Create a backup of the course including user data

      As user with permission to changeidnumber

      • Delete the following groups:
        • group-id-1
        • group-noid-1
      • Delete both groupings
      • Restore the backup
        • Confirm that:
        • All four groups are back with relevant idnumbers
        • both groupings are back with relevant idnumbers

      To prepare for the next set of tests:

      • Delete the following groups:
        • group-id-1
        • group-noid-1
      • Delete both groupings

      As user without permission to changeidnumber

      • Restore the backup
        • Confirm that:
        • All four groups are back but the re-created groups have no idnumber
        • both groupings are back but have no idnumber

      Note: The behaviour of restoring groups but not adding group members is consistent with existing behaviour

      Importing Groups with an idnumber

      The 'idnumber' field in group import from CSV is already reserved for the course idnumber.

      • Create a new CSV file as follows:
        groupname, description, groupidnumber
        group-id-1, group-id-1, group-id-1
        group-id-2, group-id-2, group-id-2
        group-id-1-duplicate, Duplicate of group-id-1, group-id-1
        group-noid-1, group-noid-1,
        group-noid-2, group-noid-2,
        

      As user with permission to changeidnumber

      • Remove all of the existing groups and groupings
      • Import the CSV file
      • Confirm:
        • All five groups were created
        • A warning was shown confirming that the idnumber for group-id-1-duplicate already existed
        • group-id-1 and group-id-2 have the correct idnumber
        • group-id-1-duplicate, group-noid-1 and group-noid-2 have no idnumber
      • Remove all of the existing groups and groupings

      As user without permission to changeidnumber

      • Import the CSV file
      • Confirm:
        • All five groups were created
        • None of the groups have an idnumber

      Automatic Creation of groups

      The idnumber changes shouldn't affect automatic creation of groups

      • Create a course with a number of students enrolled within it (e.g. 10)
      • Open the Groups tab
      • Choose 'Auto-create groups'
      • Specify:
        • Group/member count: 2
        • Create in grouping: New Grouping
        • Grouping Name: MDL-32005
      • Preview and submit - ensure that groups and the grouping are created correctly with no idnumber
      Show
      These test instructions will: Create some initial test data Attempt to manipulate (edit, delete) it both: with permission to change the idnumber; and without permission to change the idnumber. test uniqueness constraints test backup/restore of group(ing)s with idnumbers test importing data from CSV test automatic creation of groups and placement into groupings Note: The idnumber has been marked as an advanced setting in the forms. You will need to toggle the Show Advanced button to reveal it. Run Unit Tests A set of unit tests have been created for this feature. These can either be run as a full set of unit tests, or in isolation with: phpunit --verbose lib/tests/grouplib_test.php Create Test Data Navigate to Settings -> Course administration -> Users -> Groups Create new groups: name idnumber group-id-1 group-id-1 group-id-2 group-id-2 group-noid-1   group-noid-2   Open the Groupings tab Create new groupings: name idnumber grouping-id-1 grouping-id-1 grouping-id-2 grouping-id-2 grouping-noid-1   grouping-noid-2   Test initial deletion As a teacher: Open the 'Groups' tab again Select group-id-1 Choose 'Delete selected group' Choose yes on the confirmation page Confirm that the group was removed Select group-noid-1 Choose 'Delete selected group' Choose yes on the confirmation page Confirm that the group was removed Open the 'Groupings' tab Choose the delete icon on grouping-id-1 Choose yes on the confirmation page Confirm that the grouping was removed Choose the delete icon on grouping-noid-1 Choose yes on the confirmation page Confirm that the grouping was removed Remove permission Remove the moodle/course:changeidnumber capability from the editingteacher role Re-Test deletion As a teacher: Open the 'Groups' tab again Select group-id-2 The delete button should be greyed out For extra brownie points, open an inspector and un-grey it Choose 'Delete selected group' Confirm that an error was shown informing the user that they cannot remove the group Select group-noid-2 Confirm that the Delete selected group button is no longer greyed out Select both group-id-2 and group-noid-2 Confirm that the button is once again greyed out Select group-noid-2 Choose 'Delete selected group' Choose yes on the confirmation page Confirm that the group was removed Open the 'Groupings' tab Confirm that there is no delete icon for grouping-id-2 Make a note of the id in the address bar for the Grouping-2 edit page Choose the delete icon on grouping-noid-2 Replace the id in the address bar with that of grouping-id-2 Choose the delete icon on grouping-noid-2 Choose yes on the confirmation page Confirm that the grouping was removed Test data re-entry Choose the 'Groups' tab again Click the 'Create group' button Create a new group: Use a group name of group-noid-3 Confirm that it is not possible to set the idnumber any more Click 'Save changes' Open the 'Groupings' tab again Click the 'Create grouping' button Create a new grouping: Use a grouping name of grouping-noid-3 Confirm that it is not possible to set the idnumber any more Click 'Save changes' Test update existing entries Groups Choose the 'Groups' tab again Select group-id-2 Choose 'Edit group settings' Change the group name to group-id-2-updated Confirm that you cannot change the idnumber Choose save Select the same group again and click the 'Edit group settings' button Confirm the new name Confirm that the idnumber is still present Groupings Choose the 'Groupings' tab again Select the edit icon for grouping-id-2 Change the group name to grouping-id-2-updated Confirm that you cannot change the idnumber Choose save Select the edit icon for the grouping again Confirm the new name Confirm that the idnumber is still present Testing with a user with the permission Login as admin Groups Choose the Groups tab Select group-id-2-updated and choose 'Edit group settings' Change the name again to group-id-2 Change the idnumber to group-id-two Choose save settings Select the group again and edit it's settings again Confirm that the new name took Confirm that the idnumber was changed Go back to the edit page Select the group and choose 'Delete selected group' Click yes on the confirmation page Confirm that the group was removed Remove group-noid-3 Groupings Choose the Groupings tab Select grouping-id-2-updated's edit button Change the name to grouping-id-2 Change the idnumber Choose save settings Select the grouping again and edit it's settings again Confirm that the new name took Confirm that the idnumber was changed Go back to the edit page Select grouping-id-2's delete button Click yes on the confirmation page Confirm that the group was removed Remove grouping-noid-3 Testing Uniqueness User: As a user with permission to change the idnumber These tests check that it is not possible to create group(ing)s with a duplicate idnumber. Please note that: idnumbers are unique within a 'course' (not sitewide); and it is possible to have blank idnumbers for multiple items but this was tested in the initial data creation test instructions. Groups Navigate to the Groups tab again: Choose Create Group: name ' MDL-32005 ' idnumber ' MDL-32005 ' Confirm Group Creation Choose Create Group: name 'group-x' idnumber ' MDL-32005 ' Confirm that the group was not created and an error was shown on the form Change the idnumber to 'group-x' and save Re-edit 'group-x': Update idnumber to ' MDL-32005 ' Choose Save Confirm that the group was not updated and an error was shown on the form Delete both groups Groupings Navigate to the Groupings tab again: Choose Create Grouping: name ' MDL-32005 ' idnumber ' MDL-32005 ' Confirm Group Creation Choose Create Grouping: name 'grouping-x' idnumber ' MDL-32005 ' Confirm that the group was not created and an error was shown on the form Change the idnumber to 'grouping-x' and save Re-edit 'grouping-x': Update idnumber to ' MDL-32005 ' Choose Save Confirm that the group was not updated and an error was shown on the form Delete both groupings Backup/Restore with permissions User: As an administrator (permission required to backup user data) Create several groups: Group Name idnumber group-id-1 group-id-1 group-id-2 group-id-2 group-noid-1   group-noid-2   And a pair of groupings: Name idnumber grouping-id grouping-id grouping-noid   Create a backup of the course including user data As user with permission to changeidnumber Delete the following groups: group-id-1 group-noid-1 Delete both groupings Restore the backup Confirm that: All four groups are back with relevant idnumbers both groupings are back with relevant idnumbers To prepare for the next set of tests: Delete the following groups: group-id-1 group-noid-1 Delete both groupings As user without permission to changeidnumber Restore the backup Confirm that: All four groups are back but the re-created groups have no idnumber both groupings are back but have no idnumber Note: The behaviour of restoring groups but not adding group members is consistent with existing behaviour Importing Groups with an idnumber The 'idnumber' field in group import from CSV is already reserved for the course idnumber. Create a new CSV file as follows: groupname, description, groupidnumber group-id-1, group-id-1, group-id-1 group-id-2, group-id-2, group-id-2 group-id-1-duplicate, Duplicate of group-id-1, group-id-1 group-noid-1, group-noid-1, group-noid-2, group-noid-2, As user with permission to changeidnumber Remove all of the existing groups and groupings Import the CSV file Confirm: All five groups were created A warning was shown confirming that the idnumber for group-id-1-duplicate already existed group-id-1 and group-id-2 have the correct idnumber group-id-1-duplicate, group-noid-1 and group-noid-2 have no idnumber Remove all of the existing groups and groupings As user without permission to changeidnumber Import the CSV file Confirm: All five groups were created None of the groups have an idnumber Automatic Creation of groups The idnumber changes shouldn't affect automatic creation of groups Create a course with a number of students enrolled within it (e.g. 10) Open the Groups tab Choose 'Auto-create groups' Specify: Group/member count: 2 Create in grouping: New Grouping Grouping Name: MDL-32005 Preview and submit - ensure that groups and the grouping are created correctly with no idnumber
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32005-master-10
    • Rank:
      38676

      Description

      At present several tables related to users and courses have an idnumber field allowing them to be easily created and maintained by some form of plugin (most likely enrolment). This includes:

      • users
      • cohorts
      • courses
      • course categories

      It would be particularly helpful to many larger institutions if it were possible to also handle groups and groupings in this manner

      Rather than creating a new capability, it would be reasonable to re-use the existing course:changeidnumber capability.

      Additionally, to prevent deletion of automatically created groups, it should not be possible to delete a group if:

      • it has an idnumber set; and
      • the user does not have the moodle/course:changeidnumber capability.

      However if a user does not have the capability but no idnumber is set, it should be possible to remove the group.

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          At the OU we currently encode our group IDs into the group names. This is actually OK for us because we also need a way for tutors who have two groups to distinguish them in dropdowns etc. This might look like:

          Michelle Smith tutor group [123456]
          Michelle Smith tutor group [234567]

          So if Michelle has two she can tell them apart by the number.

          However it would be neater if we had a 'proper' place to store the group id as this proposal suggests. That way we could make the human-readable names something like:

          Michelle Smith tutor group #1
          Michelle Smith tutor group #2

          and for tutors who only had one group, they wouldn't need the # part at all.

          Not sure we will rush to actually do this, even if the feature is developed. But definitely the option would be a welcome one, so +1 from me.

          Show
          Sam Marshall added a comment - At the OU we currently encode our group IDs into the group names. This is actually OK for us because we also need a way for tutors who have two groups to distinguish them in dropdowns etc. This might look like: Michelle Smith tutor group [123456] Michelle Smith tutor group [234567] So if Michelle has two she can tell them apart by the number. However it would be neater if we had a 'proper' place to store the group id as this proposal suggests. That way we could make the human-readable names something like: Michelle Smith tutor group #1 Michelle Smith tutor group #2 and for tutors who only had one group, they wouldn't need the # part at all. Not sure we will rush to actually do this, even if the feature is developed. But definitely the option would be a welcome one, so +1 from me.
          Hide
          Andrew Nicols added a comment -

          Added some notes following discussion in the developer chatroom

          Show
          Andrew Nicols added a comment - Added some notes following discussion in the developer chatroom
          Hide
          Dan Poltawski added a comment -

          Andrew, you can save Michael some work by 'traiging' your own issues and adding the triaged flag: http://docs.moodle.org/dev/Bug_triage

          Show
          Dan Poltawski added a comment - Andrew, you can save Michael some work by 'traiging' your own issues and adding the triaged flag: http://docs.moodle.org/dev/Bug_triage
          Hide
          Dan Poltawski added a comment -

          Hi Andrew,

          I'm not peer reviewing this yet as this is a major change whilst the test instructions are long i'm not convinced that you have run through all the places where groups are involved.

          For example, have you considered?

          • Auto group creation
          • Group import
          • Bulk user upload

          In particular one issue seen is that it looks like group import currently uses idnumber to match on the idnumber of the course - this might be something repeated elsewhere.

          Please run through these areas and ensure that they have been covered throughly. This is a crucial area of functionality for Moodle and we need to ensure that this has been covered throughly.

          thanks as always for your work on this!

          dan

          Show
          Dan Poltawski added a comment - Hi Andrew, I'm not peer reviewing this yet as this is a major change whilst the test instructions are long i'm not convinced that you have run through all the places where groups are involved. For example, have you considered? Auto group creation Group import Bulk user upload In particular one issue seen is that it looks like group import currently uses idnumber to match on the idnumber of the course - this might be something repeated elsewhere. Please run through these areas and ensure that they have been covered throughly. This is a crucial area of functionality for Moodle and we need to ensure that this has been covered throughly. thanks as always for your work on this! dan
          Hide
          Andrew Nicols added a comment -

          Thanks Dan... a few more things to consider there.

          • Auto-group creation works as normal. idnumbers aren't touched in auto-group creation anyway
          • group import has a potential naming conflict
          • I've just written the code for backup/restore

          I have a few areas I'd appreciate comments on:

          Uniqueness

          With other moodle idnumber fields (courses, course categories, etc), the idnumber must be unique across the entire site (or be left blank). However, groups and groupings are limited in scope to a single course. This leaves two options:

          1. enforce a table-wide unique requirement for all groups/groupings; or
          2. enforce the unique requirement only within a single course.

          The latter option would allow for a potentially more user-friendly approach and in my opinion makes more sense given that groups/groupings only affect one course. Group and Grouping names are already unique within a course, but the same name can be used multiple times across a site. This is the one that I've implemented so far, but it would be relatively easy to change this to be a site-wide constraint.

          Group Import already uses idnumber

          At present, a CSV import of groups can define an idnumber. This relates to the idnumber for a course and takes preference over an alternative field 'coursename'.
          As suggested by Tim (http://moodle.org/local/chatlogs/index.php?conversationid=9731) I'd like to suggest:

          • rename the current CSV import 'idnumber' to 'courseidnumber'; AND
          • name the CSV import for the group idnumber to 'groupidnumber'.

          This should remove any potential ambiguity.

          Show
          Andrew Nicols added a comment - Thanks Dan... a few more things to consider there. Auto-group creation works as normal. idnumbers aren't touched in auto-group creation anyway group import has a potential naming conflict I've just written the code for backup/restore I have a few areas I'd appreciate comments on: Uniqueness With other moodle idnumber fields (courses, course categories, etc), the idnumber must be unique across the entire site (or be left blank). However, groups and groupings are limited in scope to a single course. This leaves two options: enforce a table-wide unique requirement for all groups/groupings; or enforce the unique requirement only within a single course. The latter option would allow for a potentially more user-friendly approach and in my opinion makes more sense given that groups/groupings only affect one course. Group and Grouping names are already unique within a course, but the same name can be used multiple times across a site. This is the one that I've implemented so far, but it would be relatively easy to change this to be a site-wide constraint. Group Import already uses idnumber At present, a CSV import of groups can define an idnumber. This relates to the idnumber for a course and takes preference over an alternative field 'coursename'. As suggested by Tim ( http://moodle.org/local/chatlogs/index.php?conversationid=9731 ) I'd like to suggest: rename the current CSV import 'idnumber' to 'courseidnumber'; AND name the CSV import for the group idnumber to 'groupidnumber'. This should remove any potential ambiguity.
          Hide
          Petr Škoda added a comment -

          1/ my +1 for curse unique group->idnumber
          2/ renaming of CSV columns is a big BC problem especially for course related columns, my +1 to just add groupidnumber

          In the long term we should add option to dynamically select meaning of each column (like Outlook CSV import), it should be donevia new mforms element that picks file and previews columns, defines column meaning, selects encoding, separators, quotings, etc. and submits file + CSV processing info... GSOC anyone?

          Show
          Petr Škoda added a comment - 1/ my +1 for curse unique group->idnumber 2/ renaming of CSV columns is a big BC problem especially for course related columns, my +1 to just add groupidnumber In the long term we should add option to dynamically select meaning of each column (like Outlook CSV import), it should be donevia new mforms element that picks file and previews columns, defines column meaning, selects encoding, separators, quotings, etc. and submits file + CSV processing info... GSOC anyone?
          Hide
          Andrew Nicols added a comment -

          Updated with the various suggested changes.
          Need to update the test instructions and go through to make sure that no areas have been missed

          Show
          Andrew Nicols added a comment - Updated with the various suggested changes. Need to update the test instructions and go through to make sure that no areas have been missed
          Hide
          Andrew Nicols added a comment -

          I've updated the test instructions and patch to fix some minor bugs.

          Show
          Andrew Nicols added a comment - I've updated the test instructions and patch to fix some minor bugs.
          Hide
          Andrew Nicols added a comment -

          Added some JS to stop users clicking the delete button

          Show
          Andrew Nicols added a comment - Added some JS to stop users clicking the delete button
          Hide
          Andrew Nicols added a comment -

          Rebased on latest weekly

          Show
          Andrew Nicols added a comment - Rebased on latest weekly
          Hide
          Dan Poltawski added a comment -

          Hi Andrew,

          Some comments after my first quick review:

          1. Should the idnumber fields be advanced elements in forms (I think yes)?
          2. I see a lot of trim()ing of the idnumber all over the show as well as lowercasing. Is this necessary? At the very least if we are always going to compare lowercased why not store lowercased rather than lowercasing the value from the db each time?
          3. There are a few newly introduced get_context_instance(CONTEXT_COURSE, $id) - these should be context_course::instance($id)
          4. $spacer can use $OUTPUT->spacer()
          5. $preventremoval = 'preventremoval="yes"' seems weird. Is this valid XHTML? I assume this check is also happening server side?
          6. groups_get_group_by_idnumber() and groups_get_grouping_by_idnumber function names imply they return a group object not a group id.
          7. Do we use the same rules on course deletion if idnumber is set as we are using here with groups?
          Show
          Dan Poltawski added a comment - Hi Andrew, Some comments after my first quick review: Should the idnumber fields be advanced elements in forms (I think yes)? I see a lot of trim()ing of the idnumber all over the show as well as lowercasing. Is this necessary? At the very least if we are always going to compare lowercased why not store lowercased rather than lowercasing the value from the db each time? There are a few newly introduced get_context_instance(CONTEXT_COURSE, $id) - these should be context_course::instance($id) $spacer can use $OUTPUT->spacer() $preventremoval = 'preventremoval="yes"' seems weird. Is this valid XHTML? I assume this check is also happening server side? groups_get_group_by_idnumber() and groups_get_grouping_by_idnumber function names imply they return a group object not a group id. Do we use the same rules on course deletion if idnumber is set as we are using here with groups?
          Hide
          Dan Poltawski added a comment -

          ps. can we have some unit tests for this?

          Show
          Dan Poltawski added a comment - ps. can we have some unit tests for this?
          Hide
          Sam Marshall added a comment -

          Regarding lower-casing - depending on the institution it may be standard practice to use capital-letter numbers (eh you know what I mean) in this case. I don't have a problem with the case-insensitive compare but it might be nicer to continue storing the idnumber as-typed in the database (rather than turn it lower-case in db as per dan's suggestion 2).

          Show
          Sam Marshall added a comment - Regarding lower-casing - depending on the institution it may be standard practice to use capital-letter numbers (eh you know what I mean) in this case. I don't have a problem with the case-insensitive compare but it might be nicer to continue storing the idnumber as-typed in the database (rather than turn it lower-case in db as per dan's suggestion 2).
          Hide
          Andrew Nicols added a comment -

          1) I'm not convinced these should be advanced fields. The form only has three fields in total so making one advanced seems a bit OTT, especially when none of the other idnumber fields are considered advanced (e.g. course idnumber)
          2) With the trimming, I'm not sure what's best here as we don't actually trim on the insert. So far I've followed the same logic that the name field uses but any advice would be good here. I've removed the strtolowers though - on reflection they shouldn't have been there in the first place.
          3) I've converted those to context_course
          4) Converted spacers
          5) The check happens server-side, but I also wanted to make sure that users can't click the button if they can't delete a group. I'll see about putting the undeletable groups into a JS array for checking instead.
          6) I can see what you mean on this, but the function names and return types match the existing functions for groups_get_group_by_name(). I'm happy to adjust accordingly though rather than continue with the confusing naming pattern.
          7) At present we don't prevent course deletion when an idnumber is present. This was discussed on the developer chat and was Petr's suggestion (http://moodle.org/local/chatlogs/index.php?conversationid=9720#c338551). I think that it makes sense on the whole - if an idnumber is set for a group, then a user without permission to change the idnumber shouldn't be able to change details of the group. Permission to change the mmbership is a different matter IMHO as the changes introduced in MDL-31973 will prevent any user from changing membership if the component is set. Since we don't already automatically create groups or groupings, this will (hopefully) be written in when the relevant changes are added to the database enrolment plugin (and potentially others).

          Re unit tests... phpunit or simpletest?

          Show
          Andrew Nicols added a comment - 1) I'm not convinced these should be advanced fields. The form only has three fields in total so making one advanced seems a bit OTT, especially when none of the other idnumber fields are considered advanced (e.g. course idnumber) 2) With the trimming, I'm not sure what's best here as we don't actually trim on the insert. So far I've followed the same logic that the name field uses but any advice would be good here. I've removed the strtolowers though - on reflection they shouldn't have been there in the first place. 3) I've converted those to context_course 4) Converted spacers 5) The check happens server-side, but I also wanted to make sure that users can't click the button if they can't delete a group. I'll see about putting the undeletable groups into a JS array for checking instead. 6) I can see what you mean on this, but the function names and return types match the existing functions for groups_get_group_by_name(). I'm happy to adjust accordingly though rather than continue with the confusing naming pattern. 7) At present we don't prevent course deletion when an idnumber is present. This was discussed on the developer chat and was Petr's suggestion ( http://moodle.org/local/chatlogs/index.php?conversationid=9720#c338551 ). I think that it makes sense on the whole - if an idnumber is set for a group, then a user without permission to change the idnumber shouldn't be able to change details of the group. Permission to change the mmbership is a different matter IMHO as the changes introduced in MDL-31973 will prevent any user from changing membership if the component is set. Since we don't already automatically create groups or groupings, this will (hopefully) be written in when the relevant changes are added to the database enrolment plugin (and potentially others). Re unit tests... phpunit or simpletest?
          Hide
          Andrew Nicols added a comment -

          Added relevant unit tests for group and grouping idnumber:
          2 test, 32 assertions

          phpunit --verbose lib/tests/grouplib_test.php

          I believe I've addressed all of the issues except for the question of trim() - no-one has suggested any preferred solution and the code uses the same logic as the name field for trimming.

          Show
          Andrew Nicols added a comment - Added relevant unit tests for group and grouping idnumber: 2 test, 32 assertions phpunit --verbose lib/tests/grouplib_test.php I believe I've addressed all of the issues except for the question of trim() - no-one has suggested any preferred solution and the code uses the same logic as the name field for trimming.
          Hide
          Andrew Nicols added a comment -

          Ahem - just noticed a debugging comment in my post-submit sanity check.

          Show
          Andrew Nicols added a comment - Ahem - just noticed a debugging comment in my post-submit sanity check.
          Hide
          Dan Poltawski added a comment -

          Sorry for slow response. This looks good for integration.

          > 1) I'm not convinced these should be advanced fields. The form only has three fields in total so making one advanced seems a bit OTT, especially when none of the other idnumber fields are considered advanced (e.g. course idnumber)

          I disagree. Just because we are cluttering other forms doesn't mean we should do it here. I actually struggle to think of a time when a normal user would be modifying these fields. If its not being modified by normal users why should they have to consider the fields every time they look at the form.

          Show
          Dan Poltawski added a comment - Sorry for slow response. This looks good for integration. > 1) I'm not convinced these should be advanced fields. The form only has three fields in total so making one advanced seems a bit OTT, especially when none of the other idnumber fields are considered advanced (e.g. course idnumber) I disagree. Just because we are cluttering other forms doesn't mean we should do it here. I actually struggle to think of a time when a normal user would be modifying these fields. If its not being modified by normal users why should they have to consider the fields every time they look at the form.
          Hide
          Petr Škoda added a comment -

          1/ echo $OUTPUT->notification("$groupname :".get_string('groupexistforcoursewithidnumber', 'error', $existing));
          is problematic because idnumber must go through s() or p() before display because it is not html, but plain text where < > must be converted to entities

          2/ the storage is problematic - missing idnumber can be either NULL or '' which makes searching for groups without idnumber via SQL selects problematic.

          Show
          Petr Škoda added a comment - 1/ echo $OUTPUT->notification("$groupname :".get_string('groupexistforcoursewithidnumber', 'error', $existing)); is problematic because idnumber must go through s() or p() before display because it is not html, but plain text where < > must be converted to entities 2/ the storage is problematic - missing idnumber can be either NULL or '' which makes searching for groups without idnumber via SQL selects problematic.
          Hide
          Andrew Nicols added a comment -

          Dan:
          It seems that the setAdvanced code uses the header (fieldset) as an anchor to display the 'Show advanced' button. Neither the group nor groupings tables currently have any elements in a fieldset so it's not possible to set any advanced fields without adding one first.
          I think that if we were to add a fieldset and make some elements 'advanced', then we may also want to make some others advanced too, or to change the grouping which would probably be best done in a separate bug. We could of course just add the fieldsets and set the relevant fields as advanced though. Any thoughts or preference?

          Petr:
          I've changed the data storage to be not nullable, giving it a default of an empty string - thanks for the suggestion.
          I've passed the name and idnumber through s() before displaying them in the error messages. I notice though that there are some existing strings which are echoed in the same way with non-HTML strings (e.g. name) which don't pass through s(). I guess it'd be beyond the scope of this feature to add those in the same commit. If you agree, I'll create a new issue and fix those too.

          Cheers,

          Andrew

          Show
          Andrew Nicols added a comment - Dan: It seems that the setAdvanced code uses the header (fieldset) as an anchor to display the 'Show advanced' button. Neither the group nor groupings tables currently have any elements in a fieldset so it's not possible to set any advanced fields without adding one first. I think that if we were to add a fieldset and make some elements 'advanced', then we may also want to make some others advanced too, or to change the grouping which would probably be best done in a separate bug. We could of course just add the fieldsets and set the relevant fields as advanced though. Any thoughts or preference? Petr: I've changed the data storage to be not nullable, giving it a default of an empty string - thanks for the suggestion. I've passed the name and idnumber through s() before displaying them in the error messages. I notice though that there are some existing strings which are echoed in the same way with non-HTML strings (e.g. name) which don't pass through s(). I guess it'd be beyond the scope of this feature to add those in the same commit. If you agree, I'll create a new issue and fix those too. Cheers, Andrew
          Hide
          Petr Škoda added a comment -

          Thanks! Hopefully I am going to review all places that use idnumber(plaintext) and general names(multilang) in course, modules, groups etc. during the freeze before 2.3

          Show
          Petr Škoda added a comment - Thanks! Hopefully I am going to review all places that use idnumber(plaintext) and general names(multilang) in course, modules, groups etc. during the freeze before 2.3
          Hide
          Dan Poltawski added a comment -

          It seems that the setAdvanced code uses the header (fieldset) as an anchor to display the 'Show advanced' button. Neither the group nor groupings tables currently have any elements in a fieldset so it's not possible to set any advanced fields without adding one first.
          I think that if we were to add a fieldset and make some elements 'advanced', then we may also want to make some others advanced too, or to change the grouping which would probably be best done in a separate bug. We could of course just add the fieldsets and set the relevant fields as advanced though. Any thoughts or preference?

          Fast and cheap would be the age old 'general' section.

          Show
          Dan Poltawski added a comment - It seems that the setAdvanced code uses the header (fieldset) as an anchor to display the 'Show advanced' button. Neither the group nor groupings tables currently have any elements in a fieldset so it's not possible to set any advanced fields without adding one first. I think that if we were to add a fieldset and make some elements 'advanced', then we may also want to make some others advanced too, or to change the grouping which would probably be best done in a separate bug. We could of course just add the fieldsets and set the relevant fields as advanced though. Any thoughts or preference? Fast and cheap would be the age old 'general' section.
          Hide
          Andrew Davis added a comment -

          Make sure to include running the new unit tests in the testing instructions.

          Show
          Andrew Davis added a comment - Make sure to include running the new unit tests in the testing instructions.
          Hide
          Dan Poltawski added a comment -

          This looks good for integration.

          Some minor comments..

          1. In group/lib.php, you have some weird spacing in a number of places like where you are setting $data->idnumber. Its come from the 'lining up =' in the lines about the if I think. But you are not lining up anything there. I agree with sam.
          2. I think that this logic is kinda weird and hard to understand because of the level of nesting and two variabels for the same thing. Is $newgroup->groupidnumber or $newgroup->idnumber used?
          if (isset($newgroup->groupidnumber)) {
          	$newgroup->groupidnumber = trim($newgroup->groupidnumber);
          	if (has_capability('moodle/course:changeidnumber', $newgrpcoursecontext)) {
          		$newgroup->idnumber = $newgroup->groupidnumber;
          		if ($existing = groups_get_group_by_idnumber($newgroup->courseid, $newgroup->idnumber)) {
          			// idnumbers must be unique to a course but we shouldn't ignore group creation for duplicates
          			$existing->name = s($existing->name);
          			$existing->idnumber = s($existing->idnumber);
          			$existing->problemgroup = $groupname;
          			echo $OUTPUT->notification(get_string('groupexistforcoursewithidnumber', 'error', $existing));
          			unset($newgroup->idnumber);
          		}
          	}
          	unset($newgroup->groupidnumber);
          }
          
          Show
          Dan Poltawski added a comment - This looks good for integration. Some minor comments.. In group/lib.php, you have some weird spacing in a number of places like where you are setting $data->idnumber. Its come from the 'lining up =' in the lines about the if I think. But you are not lining up anything there. I agree with sam . I think that this logic is kinda weird and hard to understand because of the level of nesting and two variabels for the same thing. Is $newgroup->groupidnumber or $newgroup->idnumber used? if (isset($newgroup->groupidnumber)) { $newgroup->groupidnumber = trim($newgroup->groupidnumber); if (has_capability('moodle/course:changeidnumber', $newgrpcoursecontext)) { $newgroup->idnumber = $newgroup->groupidnumber; if ($existing = groups_get_group_by_idnumber($newgroup->courseid, $newgroup->idnumber)) { // idnumbers must be unique to a course but we shouldn't ignore group creation for duplicates $existing->name = s($existing->name); $existing->idnumber = s($existing->idnumber); $existing->problemgroup = $groupname; echo $OUTPUT->notification(get_string('groupexistforcoursewithidnumber', 'error', $existing)); unset($newgroup->idnumber); } } unset($newgroup->groupidnumber); }
          Hide
          Andrew Nicols added a comment -

          Will fix the spacing.

          The $newgroup->groupidnumber and $newgroup->idnumber confusion is caused by the previous use of 'idnumber' as a field to define the course idnumber to which a group belongs. Petr preferred keeping idnumber and adding groupidnumber.

          Show
          Andrew Nicols added a comment - Will fix the spacing. The $newgroup->groupidnumber and $newgroup->idnumber confusion is caused by the previous use of 'idnumber' as a field to define the course idnumber to which a group belongs. Petr preferred keeping idnumber and adding groupidnumber.
          Hide
          Andrew Nicols added a comment -

          1: Fixed. I've corrected the spacing. I've swung around to Sam's way of thinking, but try to keep consistency where previous precedent has been set
          2: I've added comments to explain the naming. If you have a better suggestion though, I'm all ears!

          Show
          Andrew Nicols added a comment - 1: Fixed. I've corrected the spacing. I've swung around to Sam's way of thinking, but try to keep consistency where previous precedent has been set 2: I've added comments to explain the naming. If you have a better suggestion though, I'm all ears!
          Hide
          Andrew Nicols added a comment -

          requesting peer review in case you have any better ideas about groupidnumber

          Otherwise, this is ready for integration on master only.

          Show
          Andrew Nicols added a comment - requesting peer review in case you have any better ideas about groupidnumber Otherwise, this is ready for integration on master only.
          Hide
          Andrew Nicols added a comment -

          Submitting for integration after checking with Dan

          Show
          Andrew Nicols added a comment - Submitting for integration after checking with Dan
          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
          Aparup Banerjee added a comment -

          Hello, i've noticed some tables have an index created for the idnumber field, has the need for an index been considered here?

          Show
          Aparup Banerjee added a comment - Hello, i've noticed some tables have an index created for the idnumber field, has the need for an index been considered here?
          Hide
          Andrew Nicols added a comment -

          I considered it briefly – I had looked at several other places where idnumbers were used and found that most tables with an idnumber don't have an index on that field.

          That said, it should be fine to add an index here and should give a performance improvement for anyone using the group idnumber for a bulk sync/creation. For reference, the following core tables have idnumbers:

          table has index
          course yes
          user yes
          course_categories no
          course_modules no
          grade_items no
          grade_items_history no
          cohort no

          Would you like me to rebase and add an index? I should be able to do so when I get into work in 40 minutes or so.

          Show
          Andrew Nicols added a comment - I considered it briefly – I had looked at several other places where idnumbers were used and found that most tables with an idnumber don't have an index on that field. That said, it should be fine to add an index here and should give a performance improvement for anyone using the group idnumber for a bulk sync/creation. For reference, the following core tables have idnumbers: table has index course yes user yes course_categories no course_modules no grade_items no grade_items_history no cohort no Would you like me to rebase and add an index? I should be able to do so when I get into work in 40 minutes or so.
          Hide
          Aparup Banerjee added a comment -

          Hi Andrew,

          • adding index : i'm not sure whether we need an index here or not. Perhaps its better to address the need (if at all) for indexes in all the idnumber columns in a separate issue (or after a need arises). That said i don't know how the index could hurt here see as next up i was wondering..
          • is there any ideas about adding idnumber to auto create groups (and groupings)? could users of 'Group @' and 'Group #' for automated group creations likely want an idnumber generator/seed or even look up. In any case that would likely be another issue, i think this is currently complete enough.

          ah, its always better to create an index (if needed) on a new column than to create indexes on existing data. the index builds on existing data can take awhile.

          i'll wait for your reply before pushing this through.

          Show
          Aparup Banerjee added a comment - Hi Andrew, adding index : i'm not sure whether we need an index here or not. Perhaps its better to address the need (if at all) for indexes in all the idnumber columns in a separate issue (or after a need arises). That said i don't know how the index could hurt here see as next up i was wondering.. is there any ideas about adding idnumber to auto create groups (and groupings)? could users of 'Group @' and 'Group #' for automated group creations likely want an idnumber generator/seed or even look up. In any case that would likely be another issue, i think this is currently complete enough. ah, its always better to create an index (if needed) on a new column than to create indexes on existing data. the index builds on existing data can take awhile. i'll wait for your reply before pushing this through.
          Hide
          Andrew Nicols added a comment -

          Rebasing on current master and then I'll add the index and re-push. Be with you RSN.

          Show
          Andrew Nicols added a comment - Rebasing on current master and then I'll add the index and re-push. Be with you RSN.
          Hide
          Andrew Nicols added a comment -

          Added the index and done a quick test to make sure everything's okay. I've also rebased on origin/master to aid you in your quest.

          Andrew

          Show
          Andrew Nicols added a comment - Added the index and done a quick test to make sure everything's okay. I've also rebased on origin/master to aid you in your quest. Andrew
          Hide
          Aparup Banerjee added a comment -

          Thanks Andrew thats been integrated now into master.

          ps: Documentors, http://docs.moodle.org/22/en/Capabilities/moodle/course:changeidnumber
          (rather the 23 version) will have to be updated to say its used in more than just course settings.

          Show
          Aparup Banerjee added a comment - Thanks Andrew thats been integrated now into master. ps: Documentors, http://docs.moodle.org/22/en/Capabilities/moodle/course:changeidnumber (rather the 23 version) will have to be updated to say its used in more than just course settings.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note: Restoring courses with this patch applied is throwing all the time:

          1) Notice: Undefined property: stdClass::$idnumber in backup/moodle2/restore_stepslib.php on line 723
          2) Notice: Undefined property: stdClass::$idnumber in backup/moodle2/restore_stepslib.php on line 783
          

          And curiously, that's happening both with old backups (normal), but also with current backups (generated with current integration.git, seems I don't get any idnumber tag in the groups.xml file.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Note: Restoring courses with this patch applied is throwing all the time: 1) Notice: Undefined property: stdClass::$idnumber in backup/moodle2/restore_stepslib.php on line 723 2) Notice: Undefined property: stdClass::$idnumber in backup/moodle2/restore_stepslib.php on line 783 And curiously, that's happening both with old backups (normal), but also with current backups (generated with current integration.git, seems I don't get any idnumber tag in the groups.xml file. Ciao
          Hide
          Rajesh Taneja added a comment -

          Strange, I don't see any notice while backup/restore process.

          1. Tried restoring old course
          2. Backed-up course with groupid and id's are visible in groups.xml
          3. Restored above course and no notice.

          Not sure, if I am missing something... It seems to be working fine for me. Will wait for someone else to counter-check it before passing it.

          Thanks Andrew

          FYI:
          Just wondering why we use PARAM_RAW for idnumber.

          Show
          Rajesh Taneja added a comment - Strange, I don't see any notice while backup/restore process. Tried restoring old course Backed-up course with groupid and id's are visible in groups.xml Restored above course and no notice. Not sure, if I am missing something... It seems to be working fine for me. Will wait for someone else to counter-check it before passing it. Thanks Andrew FYI: Just wondering why we use PARAM_RAW for idnumber.
          Hide
          Petr Škoda added a comment -

          Very simple - idnumbers are supposed to allow connection of moodle with external systems, hence any altering of characters is strictly forbidden (apart from whitespace trim), we can not treat it as html, that is why we have to use p() and s() when printing it out to web pages.

          Show
          Petr Škoda added a comment - Very simple - idnumbers are supposed to allow connection of moodle with external systems, hence any altering of characters is strictly forbidden (apart from whitespace trim), we can not treat it as html, that is why we have to use p() and s() when printing it out to web pages.
          Hide
          Rajesh Taneja added a comment -

          Thanks Petr.

          Show
          Rajesh Taneja added a comment - Thanks Petr.
          Hide
          Rajesh Taneja added a comment - - edited

          If group idnumber is 0 (zero), user without moodle/course:changeidnumber capability can click delete button (It is not disabled). But while editing, it is hardfreeze. Should this be handled in another bug?

          Show
          Rajesh Taneja added a comment - - edited If group idnumber is 0 (zero), user without moodle/course:changeidnumber capability can click delete button (It is not disabled). But while editing, it is hardfreeze. Should this be handled in another bug?
          Hide
          Rajesh Taneja added a comment -

          Sorry Andrew,

          I could reproduce notice, while restoring the attached course.

          Notice:  Undefined property: stdClass::$idnumber in /var/www/im/backup/moodle2/restore_stepslib.php on line 723
          Stack trace:
          1. {main}() /var/www/im/backup/restore.php:0, 
          2. restore_ui->execute() /var/www/im/backup/restore.php:46
          3. restore_controller->execute_plan() /var/www/im/backup/util/ui/restore_ui.class.php:147
          4. restore_plan->execute() /var/www/im/backup/controller/restore_controller.class.php:315
          5. base_plan->execute() /var/www/im/backup/util/plan/restore_plan.class.php:157
          6. base_task->execute() /var/www/im/backup/util/plan/base_plan.class.php:148
          7. restore_structure_step->execute() /var/www/im/backup/util/plan/base_task.class.php:153
          8. progressive_parser->process() /var/www/im/backup/util/plan/restore_structure_step.class.php:105
          9. progressive_parser->parse() /var/www/im/backup/util/xml/parser/progressive_parser.class.php:137
          10. xml_parse() /var/www/im/backup/util/xml/parser/progressive_parser.class.php:158
          11. progressive_parser->end_tag() /var/www/im/backup/util/xml/parser/progressive_parser.class.php:0
          12. progressive_parser->publish() /var/www/im/backup/util/xml/parser/progressive_parser.class.php:253
          13. progressive_parser_processor->receive_chunk() /var/www/im/backup/util/xml/parser/progressive_parser.class.php:169
          14. simplified_parser_processor->process_chunk() /var/www/im/backup/util/xml/parser/processors/progressive_parser_processor.class.php:92
          15. restore_structure_parser_processor->postprocess_chunk() /var/www/im/backup/util/xml/parser/processors/simplified_parser_processor.class.php:148
          16. grouped_parser_processor->postprocess_chunk() /var/www/im/backup/util/helper/restore_structure_parser_processor.class.php:91
          17. restore_structure_parser_processor->dispatch_chunk() /var/www/im/backup/util/xml/parser/processors/grouped_parser_processor.class.php:125
          18. restore_structure_step->process() /var/www/im/backup/util/helper/restore_structure_parser_processor.class.php:103
          19. restore_groups_structure_step->process_group() /var/www/im/backup/util/plan/restore_structure_step.class.php:131
          

          Steps to reproduce:

          1. try restore attached backup file
          2. On last restore step (7. Complete), you can see above notice.
          Show
          Rajesh Taneja added a comment - Sorry Andrew, I could reproduce notice, while restoring the attached course. Notice: Undefined property: stdClass::$idnumber in /var/www/im/backup/moodle2/restore_stepslib.php on line 723 Stack trace: 1. {main}() /var/www/im/backup/restore.php:0, 2. restore_ui->execute() /var/www/im/backup/restore.php:46 3. restore_controller->execute_plan() /var/www/im/backup/util/ui/restore_ui.class.php:147 4. restore_plan->execute() /var/www/im/backup/controller/restore_controller.class.php:315 5. base_plan->execute() /var/www/im/backup/util/plan/restore_plan.class.php:157 6. base_task->execute() /var/www/im/backup/util/plan/base_plan.class.php:148 7. restore_structure_step->execute() /var/www/im/backup/util/plan/base_task.class.php:153 8. progressive_parser->process() /var/www/im/backup/util/plan/restore_structure_step.class.php:105 9. progressive_parser->parse() /var/www/im/backup/util/xml/parser/progressive_parser.class.php:137 10. xml_parse() /var/www/im/backup/util/xml/parser/progressive_parser.class.php:158 11. progressive_parser->end_tag() /var/www/im/backup/util/xml/parser/progressive_parser.class.php:0 12. progressive_parser->publish() /var/www/im/backup/util/xml/parser/progressive_parser.class.php:253 13. progressive_parser_processor->receive_chunk() /var/www/im/backup/util/xml/parser/progressive_parser.class.php:169 14. simplified_parser_processor->process_chunk() /var/www/im/backup/util/xml/parser/processors/progressive_parser_processor.class.php:92 15. restore_structure_parser_processor->postprocess_chunk() /var/www/im/backup/util/xml/parser/processors/simplified_parser_processor.class.php:148 16. grouped_parser_processor->postprocess_chunk() /var/www/im/backup/util/helper/restore_structure_parser_processor.class.php:91 17. restore_structure_parser_processor->dispatch_chunk() /var/www/im/backup/util/xml/parser/processors/grouped_parser_processor.class.php:125 18. restore_structure_step->process() /var/www/im/backup/util/helper/restore_structure_parser_processor.class.php:103 19. restore_groups_structure_step->process_group() /var/www/im/backup/util/plan/restore_structure_step.class.php:131 Steps to reproduce: try restore attached backup file On last restore step (7. Complete), you can see above notice.
          Hide
          Andrew Nicols added a comment -

          Thanks all – I'm looking into both of these issues now.

          Show
          Andrew Nicols added a comment - Thanks all – I'm looking into both of these issues now.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Aha, in my site I was having problems because my test database had NOT the new idnumber columns created (sure due to some interim integration problem).

          I've replayed upgrade again, and with the columns created, the notices continue to happen, but only when the course being restored lacks the corresponding ->idnumber attributes.

          So I've quick-created https://github.com/stronk7/moodle/commit/ce0c32925bb37e55fd432e3bcaf3046cf56d8db6

          to avoid the notices. Sound ok?

          About the '0' use case, I would move it to separate issue, because there are a lot of places where they are forbidden (evaluated with empty() )

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Aha, in my site I was having problems because my test database had NOT the new idnumber columns created (sure due to some interim integration problem). I've replayed upgrade again, and with the columns created, the notices continue to happen, but only when the course being restored lacks the corresponding ->idnumber attributes. So I've quick-created https://github.com/stronk7/moodle/commit/ce0c32925bb37e55fd432e3bcaf3046cf56d8db6 to avoid the notices. Sound ok? About the '0' use case, I would move it to separate issue, because there are a lot of places where they are forbidden (evaluated with empty() )
          Hide
          Andrew Nicols added a comment -

          Just been discussing this with Eloy - he's got a fix for the backup issue which he's about to push.

          Regarding the issue with being able to delete groups and grouping which have an idnumber of '0', I'll open a new issue for this and work on a fix there.

          Show
          Andrew Nicols added a comment - Just been discussing this with Eloy - he's got a fix for the backup issue which he's about to push. Regarding the issue with being able to delete groups and grouping which have an idnumber of '0', I'll open a new issue for this and work on a fix there.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated with the new commit introducing the isset() solution for old backups not having idnumber(s).

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated with the new commit introducing the isset() solution for old backups not having idnumber(s).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passing tested:

          1) The notices are out with the added commit
          2) The '0' case is going to be handled in another issue.

          Thanks all!

          Show
          Eloy Lafuente (stronk7) added a comment - Passing tested: 1) The notices are out with the added commit 2) The '0' case is going to be handled in another issue. Thanks all!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          U P S T R E A M I Z E D !

          Many thanks for the hard work, closing this as fixed.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Many thanks for the hard work, closing this as fixed. Ciao
          Show
          Helen Foster added a comment - Details of group and grouping ID numbers added to the following documentation pages: http://docs.moodle.org/23/en/Groups http://docs.moodle.org/23/en/Groupings http://docs.moodle.org/23/en/Import_groups http://docs.moodle.org/23/en/Capabilities/moodle/course:changeidnumber

            People

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

              Dates

              • Created:
                Updated:
                Resolved: